[flashrom] [PATCH] Add mediatek_pata programmer

Michael Karcher flashrom at mkarcher.dialup.fu-berlin.de
Mon Mar 7 02:42:47 CET 2011


Hello Carl-Daniel,

> > +ifeq ($(CONFIG_MEDIATEK_PATA), yes)
> > +FEATURE_CFLAGS += -D'CONFIG_MEDIATEK_PATA=1'
> > +PROGRAMMER_OBJS += mediatek.o
> > +# actually, I/O access, not PCI
> > +NEED_PCI := yes
> > +ifeq ($(OS_ARCH), Linux)
> > +LIBS += $(shell pkg-config --libs libsysfs)
> >   
> 
> "pkg-config --libs libsysfs" returns an error on my opensuse 11.3
> machine, but libsysfs is installed. This is caused by a missing
> libsysfs.pc file. Could you fall back to -lsysfs in that case?

libsysfs-devel or the like is also installed? If yes, we need the
fallback.

> > +FEATURE_CFLAGS += $(shell pkg-config --cflags libsysfs)
> Same problem here. Fallback: -I/usr/include/sysfs
> 
> Besides that, do we want to test for libsysfs like we test for libpci?
Probably we need to. The sysfs code (for kicking the linux driver and
reenabling it) might even be made optional.

> Related side note: Should we finally kill the overly complicated libftdi
> detection code, enable it always, and do a libpci-style detection
> instead (with a slightly modified error message which suggests to set
> CONFIG_FT2232_SPI=no to avoid the compile error)?
Didn't look at it yet, but auto-setting to "no" is more user friendly
than requiring some magic option.

> > +#include <libsysfs.h>
> sysfs/libsysfs.h instead? Not sure where it lives on Debian.
No, not <sysfs/libsysfs.h>. The file resides in /usr/include/sysfs, but
that is passed to the "-I" option, so just <libsysfs.h> needs to be
specified here.

> > +struct ataintf {
> A comment what "intf" means would be appreciated.
"interface". OK, I might make a comment.


> > +		outb(command[i], ata->portbase+i);
> OUTB please, otherwise non-Linux platforms will fail.
Good point.

> > +	outb(command[0], ata->portbase+7);
> > +	programmer_delay(10000);
> A comment explaining the delay would be nice.
[..]
> INB
> Magic numbers. Please use #defines instead where possible.
Yep, will change that.

> > +{
> > +	ata->portbase = portbase;
> > +	if (is_slave)
> > +		ata->slavebit = 0x10;
> #define for 0x10 please.
OK, that looks so obvious to IDE-knowing people that you don't think
about naming it, but you are right.


> > +#define MTK_A19toA17 1
> > +#define MTK_CONTROL 2
> > +#define MTK_DATA 3
> > +#define MTK_A7toA0 4
> > +#define MTK_A15toA8 5
> > +
> > +#define MTK_CTL_RAISE_nWE 0x01
> > +#define MTK_CTL_LOWER_nWE 0x02
> > +#define MTK_CTL_RAISE_nOE 0x04
> > +#define MTK_CTL_LOWER_nOE 0x08
> > +#define MTK_CTL_RAISE_nCS 0x10
> > +#define MTK_CTL_LOWER_nCS 0x20	/* latches low 8 address bits */
> > +#define MTK_CTL_DRIVE_DATA 0x40
> > +#define MTK_CTL_ADDRESS_A16 0x80
> Some #defines have lowercase chars, but I don't really think that
> readability would benefit from making them uppercase. Hm.

I even suspect that readability is improved by using this small amount
of lowercase letters, just like a small amount of uppercase letters can
improve readability of variable names. If there is a strict "all defines
pure uppercase" policy, I will reconsider the names, otherwise, I prefer
to keep them as is.


> > +static int mtk_open_flash(struct ataintf * ata)
> > +{
> > +	unsigned char task[7];
> > +	task[0] = 0x80;	/* Vendor specific */
> > +	task[3] = 0x2A;	/* mediatek: start flash access */
> > +	ide_submit(ata, task);

> I think Linux once supported ATA command filters for security reasons.
> Do we have to catch something like that, and does that security feature
> even impact us?

If I were using the Linux ATA command submission function, you might
raise a valid point, but "ide_submit" is the direct I/O function some
lines above. In fact, this is the only command which could be sent
through the linux interface, as it generates an IRQ. But most likely
only if the drive currently has a valid firmware. If the drive has no
valid firmware, the linux command layer can't be used anyway.

> > +int mediatek_pata_init(void)
> > +{
> > +	unsigned iobase;
> > +	int isslave;
> > +	char *portpos = NULL, *devpos = NULL;
> > +
> > +	/* IDE port specified */
> > +	if ((portpos = extract_programmer_param("iobase"))) {
> > +		char *endptr = NULL;
> > +		char *slaveptr;
> > +		unsigned long tmp;
> > +		tmp = strtoul(portpos, &endptr, 0);
> > +		free(portpos);
> We free portpos here.

> > +		/* Port 0, port >0x10000, unaligned ports and garbage strings
> > +		 * are rejected.
> > +		 */
> > +		if (!tmp || (tmp >= 0x10000) || (tmp & 0x7) ||
> Is the alignment really 0x8, or something different?
For IDE, alignment is 8.

> > +		    (*endptr != '\0')) {
> And dereference portpos here. Boom. (If this is copy-pasted, this means
> we access freed memory in another driver as well.)
The freeing error is my fault. I felt clever moving the free to the top
to not have it in the error case as well as in the success case.
Obviously I outsmarted myself. Excellent catch.

> > +		slaveptr = extract_programmer_param("slave");
> Mh, so anything starting with slave= will match?
The intention was that the pure mentioning of "slave" as option will
match (a boolean option toggled by presence), but it turned out that
"slave=" will be needed, so I will change that to
"drive=master"/"drive=slave".

> > +	if (mtk_open(iobase, isslave, &mtk_intf) < 0)
> > +	{
> > +		msg_perr("can't access that device, aborting\n");
> > +//		return -1;
> Why is this commented out?
Because that's a leftover from a test. mtk_open currently never fails,
and I am considering merging it with mtk_open_flash. Previously, it
checked for a valid ATAPI device on that port. But that check fails if
there is no valid firmware, so I disabled it.

> I did not review any Linux-specific stuff, but the rest looks OK apart
> from the coding style issues you mentioned.
> Someone with more sysfs/libsysfs knowledge should take a look at the
> sysfs related stuff.

Yeah, that's a good idea. Maybe that code profits from some comments
telling the expected position in the device tree.

Thank you for the review. I will send an improved patch soon.

Regards,
  Michael Karcher





More information about the flashrom mailing list