Re: [flashrom] [PATCH] Add mediatek_pata programmer

[Carl-Daniel Hailfinger]

Auf 07.03.2011 01:05, Michael Karcher schrieb:
> Add support for flashing using CD/DVD drives with mediatek chipsets.
>
> As the driver is now, it only supports parallel ATA drives with parallel
> flash chips. The kind of drive access needed is not possible through IDE
> drivers in the OS, flashrom has to access the IDE port directly. This will
> interfere with OS drivers for the drive, so you better make sure that
> the OS does not access the drive (or another drive on the same PATA channel)
> during the flash process.
>
> On recent linux kernels (with libata), the invocation
> "-p mediatek_pata:devname=sr0" should work. It is supposed to forcibly
> unregister all drives on the IDE channel sr0 is on (you better don't have
> a file system mounted on a hard drive on the same channel), perform the
> flash job, and, if the drive successfully reboots, re-add all detectable
> IDE devices on that channel.
>
> This patch is known to lack a man page section and violate the flashrom
> coding style in some places - I send it out to catch some early reviews
> on it.
>
> Signed-off-by: Michael Karcher

Nice! Thanks for your patch. Short review follows.
> diff --git a/Makefile b/Makefile
> index 6028485..a38740f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -158,6 +158,9 @@ CONFIG_DEDIPROG ?= no
>  # Always enable Marvell SATA controllers for now.
>  CONFIG_SATAMV ?= yes
>
> +# Disable Mediatek PATA programmer until cleaned up
> +CONFIG_MEDIATEK_PATA ?= no
>
> # Disable wiki printing by default. It is only useful if you have wiki access.
>  CONFIG_PRINT_WIKI ?= no
>
> @@ -292,6 +295,17 @@ PROGRAMMER_OBJS += satamv.o
>  NEED_PCI := yes
>  endif
>
> +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?
> +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?

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)?
> +endif
> +endif
>
> ifeq ($(NEED_SERIAL), yes)
>  LIB_OBJS += serial.o
>  endif
> diff --git a/flashrom.c b/flashrom.c
> index 4c6c1fa..05c0a0b 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -52,7 +52,7 @@ enum programmer programmer = PROGRAMMER_DUMMY;
> if more than one of them is selected. If only one is selected, it is clear
> that the user wants that one to become the default.
>
> */
> -#if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG+CONFIG_RAYER_SPI+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV > 1
> +#if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG+CONFIG_RAYER_SPI+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV+CONFIG_MEDIATEK_PATA > 1
>  #error Please enable either CONFIG_DUMMY or CONFIG_INTERNAL or disable support for all programmers except one.
>  #endif
>  enum programmer programmer =
> @@ -102,6 +102,9 @@ enum programmer programmer =
>  #if CONFIG_SATAMV == 1
>    PROGRAMMER_SATAMV
>  #endif
> +#if CONFIG_MEDIATEK_PATA == 1
>    PROGRAMMER_MEDIATEK_PATA
>
> +#endif
>  ;
>  #endif
>
> @@ -483,6 +486,25 @@ const struct programmer_entry programmer_table[] = {
>    },
>  #endif
>
> +#if CONFIG_MEDIATEK_PATA == 1
> {
> .name			= "mediatek_pata",
> .init			= mediatek_pata_init,
> .shutdown		= mediatek_pata_shutdown,
> .map_flash_region	= fallback_map,
> .unmap_flash_region	= fallback_unmap,
> .chip_readb		= mediatek_pata_chip_readb,
> .chip_readw		= fallback_chip_readw,
> .chip_readl		= fallback_chip_readl,
> .chip_readn		= fallback_chip_readn,
> .chip_writeb		= mediatek_pata_chip_writeb,
> .chip_writew		= fallback_chip_writew,
> .chip_writel		= fallback_chip_writel,
> .chip_writen		= fallback_chip_writen,
> .delay			= internal_delay,
> },
>
> +#endif
> {}, /* This entry corresponds to PROGRAMMER_INVALID. */
>
> };
>
> diff --git a/mediatek.c b/mediatek.c
> new file mode 100644
> index 0000000..e44a261
> --- /dev/null
> +++ b/mediatek.c
> @@ -0,0 +1,350 @@
> +/*
> This file is part of the flashrom project.
> Copyright (C) 2011 Michael Karcher
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; version 2 of the License.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +/* driver for PATA connected CD/DVD drives with mediatek chipset */
>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include "flash.h"
> +#include "programmer.h"
>
> +#ifdef __linux__
> +#include <libsysfs.h>

sysfs/libsysfs.h instead? Not sure where it lives on Debian.
> +static int grab_device(const char *devname, unsigned * portnum, int * isslave, char * rescanpath)
> +{
> struct sysfs_class *blockclass;
> struct sysfs_class_device *cdclassdevice;
> struct sysfs_device *cddevice, *targetdevice, *hostdevice, *atacontroller;
> struct dlist *targets, *hostadapters, *hosts;
> const char *subtarget, *haname;
> const char *resourcelist;
> int devhostnum, hostnum;
> int hostbus = 0, i;
> blockclass = sysfs_open_class("block");
> cdclassdevice = sysfs_get_class_device(blockclass,devname);
> if (!cdclassdevice)
> {
> fputs("Device not found\n", stderr);
> return -1;
> }
> cddevice = sysfs_get_classdev_device(cdclassdevice);
> /* should be only 0 or 1 */
> if (sscanf(cddevice->name,"%*d:%*d:%d",isslave) != 1)
> {
> fputs("can't parse device ID\n", stderr);
> return -1;
> }
> targetdevice = sysfs_get_device_parent(cddevice);
> hostdevice = sysfs_get_device_parent(targetdevice);
> sprintf(rescanpath,"%s/scsi_host",hostdevice->path);
> hostadapters = sysfs_open_directory_list(rescanpath);
> dlist_for_each_data(hostadapters, haname, const char) break;
> sprintf(rescanpath,"%s/scsi_host/%s/scan",hostdevice->path,haname);
> sysfs_close_list(hostadapters);
> targets = sysfs_open_directory_list(hostdevice->path);
> dlist_for_each_data (targets, subtarget, const char) {
> /* handle all targets on that bus */
> if (strncmp(subtarget,"target",6) == 0)
> {
> 	struct dlist * luns;
> 	const char * lundevname;
> 	char lunspath[SYSFS_PATH_MAX], deldevpath[SYSFS_PATH_MAX];
> 	sprintf(lunspath, "%s/%s", hostdevice->path, subtarget);
> 	/* handle all (i.e. one) LUN on that target */
> 	luns = sysfs_open_directory_list(lunspath);
> 	dlist_for_each_data (luns, lundevname, const char) {
> 		/* handle children, not the "power" subdir */
> 		if (strchr(lundevname,':'))
> 		{
> 			FILE * f;
> 			sprintf(deldevpath, "%s/%s/delete", lunspath, lundevname);
> 			f = fopen(deldevpath,"w");
> 			if (!f)
> 			{
> 				fprintf(stderr, "Can't open %s for grabbing device\n", deldevpath);
> 				return -1;
> 			}
> 			fputs("1\n", f);
> 			fclose(f);
> 		}
> 	}
> 	sysfs_close_list(luns);
> }
> }
> sysfs_close_list(targets);
> if (sscanf(hostdevice->name, "host%d", &devhostnum) != 1)
> {
> fputs("Can't parse host number\n",stderr);
> return -1;
> }
> atacontroller = sysfs_get_device_parent(hostdevice);
> hosts = sysfs_open_directory_list(atacontroller->path);
> dlist_for_each_data (hosts, haname, const char) {
> if (sscanf(haname, "host%d", &hostnum) == 1 &&
>     hostnum < devhostnum)
> 	hostbus++;
> }
> sysfs_close_list(hosts);
> resourcelist = sysfs_get_device_attr(atacontroller, "resource")->value;
> for (i = 0; i < 2*hostbus; i++)
> resourcelist = strchr(resourcelist, '\n') + 1;
> if (sscanf(resourcelist,"%i",portnum) != 1)
> {
> fputs("Can't parse port number\n", stderr);
> return 1;
> }
> sysfs_close_class(blockclass);
> return 0;
>
> +}
>
> +static void release_device(void *rescanpath)
> +{
> FILE * f;
> f = fopen((const char*)rescanpath, "w");
> if (!f)
> {
> fputs("Can't open rescanning metafile\n", stderr);
> return;
> }
> fputs("- - -\n",f);
> fclose(f);
>
> +}
>
> +#endif
>
> +struct ataintf {

A comment what "intf" means would be appreciated.
> unsigned portbase;
> int slavebit;
>
> +} mtk_intf;
>
> +static int ide_submit(struct ataintf *ata, unsigned char *command)
> +{
> int i, status, ret = 0;
> command[6] &= ~0x10;
> command[6] |= ata->slavebit;
> for (i = 1; i < 7; i++)
> outb(command[i], ata->portbase+i);

OUTB please, otherwise non-Linux platforms will fail.
> outb(command[0], ata->portbase+7);
> programmer_delay(10000);

A comment explaining the delay would be nice.
> i = 0;
> while ((status = inb(ata->portbase+7)) & 0x80 && ++i < 200)

INB
Magic numbers. Please use #defines instead where possible.
> programmer_delay(10000);
> if (i == 200)
> ret = -1;
> command[0] = status;
> for (i = 1; i < 7; i++)
> command[i] = inb(ata->portbase + i);
> return ret;
>
> +}
>
> +static int mtk_open(unsigned portbase, int is_slave, struct ataintf * ata)

Whitespace after *.
> +{
> ata->portbase = portbase;
> if (is_slave)
> ata->slavebit = 0x10;

#define for 0x10 please.
> else
> ata->slavebit = 0;
> return 0;
>
> +}
>
> +#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.
> +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?
> msg_pdbg("status: %x\n", task[0]);
> if ((task[0] & 0xFC) != 0x70) {
> msg_perr("Mediatek flash mode signature didn't appear. Most likely "
>          "unsupported device\n");
> return 1;
> }
> if (task[0] == 0x70)	/* parallel flash */
> {
> outb(MTK_CTL_RAISE_nCS | MTK_CTL_RAISE_nOE | MTK_CTL_RAISE_nWE, ata->portbase + MTK_CONTROL);
> return 0;
> }
> else
> {
> msg_perr("Signature for parallel flash not found, maybe SPI flash?\n");
> return 1;
> }
>
> +}
>
> +static int mtk_close_flash(struct ataintf * ata)
> +{
> unsigned char task[8];
> task[0] = 0x81;	/* Vendor specific */
> task[2] = 8;
> task[3] = 0;
> if (ide_submit(ata, task) == -1)
> {
> msg_perr("error rebooting CD drive\n");
> return -1;
> }
> return 0;
>
> +}
>
> +static unsigned char mtk_readb(struct ataintf * ata, unsigned address)
> +{
> unsigned char val;
> unsigned char A16bit = address & 0x10000 ? MTK_CTL_ADDRESS_A16 : 0;
> outb(A16bit, ata->portbase + MTK_CONTROL);
> outb(address >> 17, ata->portbase + MTK_A19toA17);
> outb(address >> 8, ata->portbase + MTK_A15toA8);
> outb(address >> 0, ata->portbase + MTK_A7toA0);
> outb(MTK_CTL_LOWER_nCS | MTK_CTL_LOWER_nOE | A16bit, ata->portbase + MTK_CONTROL);
> val = inb(ata->portbase + MTK_DATA);
> outb(MTK_CTL_RAISE_nCS | MTK_CTL_RAISE_nOE, ata->portbase + MTK_CONTROL);
> return val;
>
> +}
>
> +static void mtk_writeb(struct ataintf * ata, unsigned char val, unsigned address)
> +{
> unsigned char A16bit = address & 0x10000 ? MTK_CTL_ADDRESS_A16 : 0;
> outb(A16bit, ata->portbase + MTK_CONTROL);
> outb(address >> 17, ata->portbase + MTK_A19toA17);
> outb(address >> 8, ata->portbase + MTK_A15toA8);
> outb(address >> 0, ata->portbase + MTK_A7toA0);
> outb(val, ata->portbase + MTK_DATA);
> outb(MTK_CTL_LOWER_nCS | MTK_CTL_DRIVE_DATA | A16bit, ata->portbase + MTK_CONTROL);
> outb(MTK_CTL_LOWER_nWE | MTK_CTL_DRIVE_DATA | A16bit, ata->portbase + MTK_CONTROL);
> outb(MTK_CTL_RAISE_nCS | MTK_CTL_RAISE_nWE, ata->portbase + MTK_CONTROL);
>
> +}
>
> +#ifdef __linux__
> +static char rescanpath[SYSFS_PATH_MAX] = "";
> +#endif
>
> +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?
>     (*endptr != '\0')) {

And dereference portpos here. Boom. (If this is copy-pasted, this means
we access freed memory in another driver as well.)
> 	/* Using ports below 0x100 is a really bad idea, and
> 	 * should only be done if no port between 0x100 and
> 	 * 0xfffc works due to routing issues.
> 	 */
> 	msg_perr("Error: iobase= specified, but the I/O base "
> 		 "given was invalid.\nIt must be a multiple of "
> 		 "0x8 and lie between 0x100 and 0xfff8.\n");
> 	return 1;
> }
> iobase = tmp;
> slaveptr = extract_programmer_param("slave");

Mh, so anything starting with slave= will match?
> isslave = slaveptr != NULL;
> free(slaveptr);
> } else if ((devpos = extract_programmer_param("devname")))  {
>
> +#ifdef __linux__
> if(grab_device(devpos, &iobase, &isslave, rescanpath) == 0)
> {
> 	msg_pinfo("device grabbed: %s at port %x\n",
> 			isslave?"slave":"master", iobase);
> }
> else
> {
> 	msg_perr("can't get access to device %s\n", devpos);
> 	return -1;
> }
>
> +#else
> msg_perr("Open by devname is not implemented on this OS yet\n");
> return -1;
>
> +#endif
> } else {
> msg_perr("Need to specify either iobase or devname for the "
>          "mediatek_pata driver\n");
> return -1;
> }
> get_io_perms();
> if (mtk_open(iobase, isslave, &mtk_intf) < 0)
> {
> msg_perr("can't access that device, aborting\n");
>
> +//		return -1;

Why is this commented out?
> }
> if (mtk_open_flash(&mtk_intf) < 0)
> {
> msg_perr("can't get access to flash chip, aborting\n");
> return -1;
> }
> return 0;
>
> +}
>
> +void mediatek_pata_chip_writeb(uint8_t val, chipaddr addr)
> +{
> mtk_writeb(&mtk_intf, val, addr);
>
> +}
>
> +uint8_t mediatek_pata_chip_readb(const chipaddr addr)
> +{
> return mtk_readb(&mtk_intf, addr);
>
> +}
>
> +int mediatek_pata_shutdown(void)
> +{
> int res;
> res = mtk_close_flash(&mtk_intf);
>
> +#ifdef __linux__
> if (rescanpath[0]) {
> if (res >= 0)	/* don't rescan if the drive is broken now */
> 	release_device(rescanpath);
> else {
> 	msg_perr("probably bad flash contents, won't"
> 	         " rescan ATA bus\n");
> 	msg_perr("use \"echo - - - > %s\" to rescan after"
> 	         " fixing\n",rescanpath);
> 	msg_perr("use \"-p mediatek_pata:iobase=0x%x%s\" until"
> 	         " the drive is working again\n",
> 	         mtk_intf.portbase, 
> 	         mtk_intf.slavebit ? ":slave" : "");
> }
> }
> rescanpath[0] = 0;
>
> +#endif
> return 0;
>
> +}
> diff --git a/programmer.h b/programmer.h
> index 7a497f8..b62b86f 100644
> --- a/programmer.h
> +++ b/programmer.h
> @@ -82,6 +82,9 @@ enum programmer {
>  #if CONFIG_SATAMV == 1
>    PROGRAMMER_SATAMV,
>  #endif
> +#if CONFIG_MEDIATEK_PATA == 1
> PROGRAMMER_MEDIATEK_PATA,
>
> +#endif
>    PROGRAMMER_INVALID /* This must always be the last entry. */
>  };
>
> @@ -516,6 +519,11 @@ char *extract_programmer_param(char *param_name);
>  /* layout.c */
>  int show_id(uint8_t *bios, int size, int force);
>
> +int mediatek_pata_init(void);
> +int mediatek_pata_shutdown(void);
> +void mediatek_pata_chip_writeb(uint8_t val, chipaddr addr);
> +uint8_t mediatek_pata_chip_readb(const chipaddr addr);
>
> /* spi.c */
>  enum spi_controller {
>    SPI_CONTROLLER_NONE,

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.

Regards,
Carl-Daniel

[PATCH] Add mediatek_pata programmer

[Michael Karcher]

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

[PATCH] Add mediatek_pata programmer

[Carl-Daniel Hailfinger]

Auf 07.03.2011 02:42, Michael Karcher schrieb:
> 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.

On opensuse 11.3, all needed files are part of the sysfsutils package
(headers + lib).
> > > +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.

Hm. You're right, autodetection is more user friendly.
> > > +#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.

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

Thanks.
> > > 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.

Keep them. There is no hard policy AFAIK, and even if there was one,
code readability wins.
> > > +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.

Ah OK.
> > > +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.

Thanks!
> > > 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".

Looks good.
> > > 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.

It might be interesting to print a diagnostic message in that case, e.g.
"It seems this drive does not have a valid flash image matching the
drive. Continuing anyway."
Your choice, though.
> > 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.

Thank you for reverse engineering and coding this patch!

Regards,
Carl-Daniel