On 05.07.2010 22:32, Michael Karcher wrote:
Am Montag, den 05.07.2010, 18:34 +0200 schrieb Carl-Daniel Hailfinger:
int drkaiser_init(void) { uint32_t addr;
char *devstring;
get_io_perms();
devstring = extract_param(&programmer_param, "pci", ","); pcidev_init(PCI_VENDOR_ID_DRKAISER, PCI_BASE_ADDRESS_2,
drkaiser_pcidev, programmer_param);
drkaiser_pcidev, devstring);- free(devstring);
can't we do the devsting stuff inside the pcidev_init function, if it is the same for all programmers?
Good point. Should I do that in a followup patch or in an update of this patch?
I suggest to do this in an update of this patch. This patch will actually get smaller by it, as you don't introduce lots of duplicated code.
Done.
if (programmer_param && !strlen(programmer_param)) {free(programmer_param);programmer_param = NULL;
/* Non-default port requested? */portpos = extract_param(&programmer_param, "it87spiport", ",:");if (portpos && strlen(portpos)) {flashport = strtol(portpos, (char **)NULL, 0);I know it wasn't in before, but some minimal error checking would be really nice here. strtol returns 0 on failure, and we don't want to set port 0. Think of "it87spiport=0y4800".
True. Well, if my docs are correct, strtol("123foo456") will return 123.
Your docs are correct.
If we really want error checking for the parameter, we probably need to check endptr in strtol, and should check if the value is nonzero and less than 65536 as well. Maybe even a check for alignment...
Right. This is the sophisticated and correct version. The classic test is "*endptr == 0".
The IT8705F support patch touches that area, and I'll extend it to take care of this issue as well.
OK.
- ret = buspirate_serialport_setup(dev);
- if (ret)
return ret;- free(dev);
- speed = extract_param(&programmer_param, "spispeed", ",:"); if (speed) { for (i = 0; spispeeds[i].name; i++) if (!strncasecmp(spispeeds[i].name, speed,
@@ -129,13 +123,11 @@ if (!spispeeds[i].name) msg_perr("Invalid SPI speed, using default.\n"); }
- free(speed);
- /* This works because speeds numbering starts at 0 and is contiguous. */ msg_pdbg("SPI speed is %sHz\n", spispeeds[spispeed].name);
- ret = buspirate_serialport_setup(dev);
- if (ret)
return ret;This hunk moves the initialization of the device. Is that intended?
Yes. This allows us to abort init in case of broken parameters even before we start to touch the hardware. The biggest benefit is not having to shut down the serial port due to errors in parameters.
Uh, do I read the diff in a wrong way? It seems like you now move the port setup function before parsing the speed while it was after parsing the speed. So the new code does touch the hardware before noticing broken parameters while the old code didn't.
You read the diff correctly, my explanation was incorrect. The correct explanation would have been: The move reduces the lifetime of the dev variable. I've reverted the move.
int programmer_init(void) {
- return programmer_table[programmer].init();
- int ret;
- ret = programmer_table[programmer].init();
- if (programmer_param && strlen(programmer_param)) {
In the end, this needs to be in a subfunction like get_unhandled_parameters()
Mh. You have a point. Given that the whole parameter handling stuff strips out the extracted parameters from programmer_param, the string should be empty in the end. Not sure if a single line checking for a non-empty programmer_param should be in its own function, though.
Sorry. I didn't get that this is the file programmer_param is declared in. Just keep it this way.
OK, thanks.
Thanks for the review. How should I proceed?
Please change the pcidev stuff to use common code in pcidev_init and recheck the ordering in the buspirate file. With these changes:
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Done, thanks for the review.
New version, addresses all comments except the it87spi stuff which will be handled in an updated it8705 support patch.
Various places in the flashrom source feature custom parameter extraction from programmer_param. This led to wildly differing syntax for programmer parameters, and it also voids pretty much every assumption you could make about programmer_param. The latter is a problem for libflashrom.
Use extract_param everywhere, clean up related code and make it more foolproof. Add two instances of exit(1) where we have no option to return an error. Remove six instances of exit(1) where returning an error was possible.
WARNING: This changes programmer parameter syntax for a few programmers!
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-extract_param_everywhere/flashrom.8 =================================================================== --- flashrom-extract_param_everywhere/flashrom.8 (Revision 1069) +++ flashrom-extract_param_everywhere/flashrom.8 (Arbeitskopie) @@ -183,7 +183,7 @@ .BR "* ft2232_spi" " (for SPI flash ROMs attached to a FT2232H/FT4232H based\ USB SPI programmer)" .sp -.BR "* serprog" " (for flash ROMs attached to Urja's AVR programmer)" +.BR "* serprog" " (for flash ROMs attached to a programmer speaking serprog)" .sp .BR "* buspirate_spi" " (for SPI flash ROMs attached to a Bus Pirate)" .sp @@ -296,22 +296,23 @@ .BR "dummy " programmer An optional parameter specifies the bus types it should support. For that you have to use the -.B "flashrom -p dummy:type" +.B "flashrom -p dummy:bus=[type[+type[+type]]]" syntax where .B type -can be any comma-separated combination of -.BR parallel ", " lpc ", " fwh ", " spi ", or " all -(in any order). +can be any of +.BR parallel ", " lpc ", " fwh ", " spi +in any order. If you specify bus without type, all buses will be disabled. +If you do not specify bus, all buses will be enabled. .sp Example: -.B "flashrom -p dummy:lpc,fwh" +.B "flashrom -p dummy:bus=lpc+fwh" .TP .BR "nic3com" , " nicrealtek" , " nicsmc1211" , " gfxnvidia" , " satasii \ " and " atahpt " programmers These programmers have an option to specify the PCI address of the card your want to use, which must be specified if more than one card supported by the selected programmer is installed in your system. The syntax is -.BR "flashrom -p xxxx:bb:dd.f" , +.BR "flashrom -p xxxx:pci=bb:dd.f" , where .B xxxx is the name of the programmer @@ -323,7 +324,7 @@ is the PCI function number of the desired device. .sp Example: -.B "flashrom -p nic3com:05:04.0" +.B "flashrom -p nic3com:pci=05:04.0" .TP .BR "it87spi " programmer An optional @@ -342,7 +343,7 @@ An optional parameter specifies the controller type and interface/port it should support. For that you have to use the .sp -.B " flashrom -p ft2232_spi:model,port=interface" +.B " flashrom -p ft2232_spi:type=model,port=interface" .sp syntax where .B model @@ -363,11 +364,11 @@ programmer. In the device/baud combination, the device has to start with a slash. For serial, you have to use the .sp -.B " flashrom -p serprog:/dev/device:baud" +.B " flashrom -p serprog:dev=/dev/device:baud" .sp syntax and for IP, you have to use .sp -.B " flashrom -p serprog:ip:port" +.B " flashrom -p serprog:ip=ipaddr:port" .sp instead. More information about serprog is available in .B serprog-protocol.txt Index: flashrom-extract_param_everywhere/flash.h =================================================================== --- flashrom-extract_param_everywhere/flash.h (Revision 1069) +++ flashrom-extract_param_everywhere/flash.h (Arbeitskopie) @@ -336,7 +336,7 @@ const char *device_name; }; uint32_t pcidev_validate(struct pci_dev *dev, uint32_t bar, const struct pcidev_status *devs); -uint32_t pcidev_init(uint16_t vendor_id, uint32_t bar, const struct pcidev_status *devs, char *pcidev_bdf); +uint32_t pcidev_init(uint16_t vendor_id, uint32_t bar, const struct pcidev_status *devs); #endif
/* print.c */ Index: flashrom-extract_param_everywhere/drkaiser.c =================================================================== --- flashrom-extract_param_everywhere/drkaiser.c (Revision 1069) +++ flashrom-extract_param_everywhere/drkaiser.c (Arbeitskopie) @@ -38,8 +38,9 @@ uint32_t addr;
get_io_perms(); + pcidev_init(PCI_VENDOR_ID_DRKAISER, PCI_BASE_ADDRESS_2, - drkaiser_pcidev, programmer_param); + drkaiser_pcidev);
/* Write magic register to enable flash write. */ pci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR, @@ -53,6 +54,7 @@ addr, 128 * 1024);
buses_supported = CHIP_BUSTYPE_PARALLEL; + return 0; }
@@ -60,7 +62,6 @@ { /* Write protect the flash again. */ pci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR, 0); - free(programmer_param); pci_cleanup(pacc); release_io_perms(); return 0; Index: flashrom-extract_param_everywhere/it87spi.c =================================================================== --- flashrom-extract_param_everywhere/it87spi.c (Revision 1069) +++ flashrom-extract_param_everywhere/it87spi.c (Arbeitskopie) @@ -142,22 +142,24 @@ flashport = sio_read(port, 0x64) << 8; flashport |= sio_read(port, 0x65); msg_pdbg("Serial flash port 0x%04x\n", flashport); - if (programmer_param && !strlen(programmer_param)) { - free(programmer_param); - programmer_param = NULL; + /* Non-default port requested? */ + portpos = extract_param(&programmer_param, "it87spiport", ",:"); + if (portpos && strlen(portpos)) { + flashport = strtol(portpos, (char **)NULL, 0); + msg_pinfo("Forcing serial flash port 0x%04x\n", + flashport); + sio_write(port, 0x64, (flashport >> 8)); + sio_write(port, 0x65, (flashport & 0xff)); + } else if (portpos) { + msg_perr("Error: it87spiport specified, but no port " + "given.\n"); + free(portpos); + /* FIXME: Return failure here once it87spi_common_init() + * can handle the return value sanely. + */ + exit(1); } - if (programmer_param) { - portpos = extract_param(&programmer_param, - "it87spiport=", ",:"); - if (portpos) { - flashport = strtol(portpos, (char **)NULL, 0); - msg_pinfo("Forcing serial flash port 0x%04x\n", - flashport); - sio_write(port, 0x64, (flashport >> 8)); - sio_write(port, 0x65, (flashport & 0xff)); - free(portpos); - } - } + free(portpos); exit_conf_mode_ite(port); break; /* TODO: Handle more IT87xx if they support flash translation */ Index: flashrom-extract_param_everywhere/buspirate_spi.c =================================================================== --- flashrom-extract_param_everywhere/buspirate_spi.c (Revision 1069) +++ flashrom-extract_param_everywhere/buspirate_spi.c (Arbeitskopie) @@ -101,24 +101,14 @@ char *speed = NULL; int spispeed = 0x7;
- if (programmer_param && !strlen(programmer_param)) { - free(programmer_param); - programmer_param = NULL; - } - if (programmer_param) { - dev = extract_param(&programmer_param, "dev=", ",:"); - speed = extract_param(&programmer_param, "spispeed=", ",:"); - if (strlen(programmer_param)) - msg_perr("Unhandled programmer parameters: %s\n", - programmer_param); - free(programmer_param); - programmer_param = NULL; - } - if (!dev) { + dev = extract_param(&programmer_param, "dev", ",:"); + if (!dev || !strlen(dev)) { msg_perr("No serial device given. Use flashrom -p " "buspirate_spi:dev=/dev/ttyUSB0\n"); return 1; } + + speed = extract_param(&programmer_param, "spispeed", ",:"); if (speed) { for (i = 0; spispeeds[i].name; i++) if (!strncasecmp(spispeeds[i].name, speed, @@ -129,12 +119,15 @@ if (!spispeeds[i].name) msg_perr("Invalid SPI speed, using default.\n"); } + free(speed); + /* This works because speeds numbering starts at 0 and is contiguous. */ msg_pdbg("SPI speed is %sHz\n", spispeeds[spispeed].name);
ret = buspirate_serialport_setup(dev); if (ret) return ret; + free(dev);
/* This is the brute force version, but it should work. */ for (i = 0; i < 19; i++) { Index: flashrom-extract_param_everywhere/pcidev.c =================================================================== --- flashrom-extract_param_everywhere/pcidev.c (Revision 1069) +++ flashrom-extract_param_everywhere/pcidev.c (Arbeitskopie) @@ -78,10 +78,11 @@ }
uint32_t pcidev_init(uint16_t vendor_id, uint32_t bar, - const struct pcidev_status *devs, char *pcidev_bdf) + const struct pcidev_status *devs) { struct pci_dev *dev; struct pci_filter filter; + char *pcidev_bdf; char *msg = NULL; int found = 0; uint32_t addr = 0, curaddr = 0; @@ -93,12 +94,14 @@
/* Filter by vendor and also bb:dd.f (if supplied by the user). */ filter.vendor = vendor_id; + pcidev_bdf = extract_param(&programmer_param, "pci", ","); if (pcidev_bdf != NULL) { if ((msg = pci_filter_parse_slot(&filter, pcidev_bdf))) { msg_perr("Error: %s\n", msg); exit(1); } } + free(pcidev_bdf);
for (dev = pacc->devices; dev; dev = dev->next) { if (pci_filter_match(&filter, dev)) { Index: flashrom-extract_param_everywhere/gfxnvidia.c =================================================================== --- flashrom-extract_param_everywhere/gfxnvidia.c (Revision 1069) +++ flashrom-extract_param_everywhere/gfxnvidia.c (Arbeitskopie) @@ -62,7 +62,8 @@ get_io_perms();
io_base_addr = pcidev_init(PCI_VENDOR_ID_NVIDIA, PCI_BASE_ADDRESS_0, - gfx_nvidia, programmer_param); + gfx_nvidia); + io_base_addr += 0x300000; msg_pinfo("Detected NVIDIA I/O base address: 0x%x.\n", io_base_addr);
@@ -87,7 +88,6 @@ reg32 |= (1 << 0); pci_write_long(pcidev_dev, 0x50, reg32);
- free(programmer_param); pci_cleanup(pacc); release_io_perms(); return 0; Index: flashrom-extract_param_everywhere/nicrealtek.c =================================================================== --- flashrom-extract_param_everywhere/nicrealtek.c (Revision 1069) +++ flashrom-extract_param_everywhere/nicrealtek.c (Arbeitskopie) @@ -44,7 +44,7 @@ get_io_perms();
io_base_addr = pcidev_init(PCI_VENDOR_ID_REALTEK, PCI_BASE_ADDRESS_0, - nics_realtek, programmer_param); + nics_realtek);
buses_supported = CHIP_BUSTYPE_PARALLEL;
@@ -56,7 +56,7 @@ get_io_perms();
io_base_addr = pcidev_init(PCI_VENDOR_ID_SMC1211, PCI_BASE_ADDRESS_0, - nics_realteksmc1211, programmer_param); + nics_realteksmc1211);
buses_supported = CHIP_BUSTYPE_PARALLEL;
@@ -66,7 +66,6 @@ int nicrealtek_shutdown(void) { /* FIXME: We forgot to disable software access again. */ - free(programmer_param); pci_cleanup(pacc); release_io_perms(); return 0; Index: flashrom-extract_param_everywhere/serprog.c =================================================================== --- flashrom-extract_param_everywhere/serprog.c (Revision 1069) +++ flashrom-extract_param_everywhere/serprog.c (Arbeitskopie) @@ -298,43 +298,80 @@ int serprog_init(void) { uint16_t iface; - int len; unsigned char pgmname[17]; unsigned char rbuf[3]; unsigned char c; - char *num; - char *dev; - msg_pspew("%s\n", __func__); - /* the parameter is either of format "/dev/device:baud" or "ip:port" */ - if ((!programmer_param) || (!strlen(programmer_param))) { - nodevice: - msg_perr("Error: No device/host given for the serial programmer driver.\n" - "Use flashrom -p serprog=/dev/device:baud or flashrom -p serprog=ip:port\n"); - exit(1); + char *device; + char *baudport; + int have_device = 0; + + /* the parameter is either of format "dev=/dev/device:baud" or "ip=ip:port" */ + device = extract_param(&programmer_param, "dev", ","); + if (device && strlen(device)) { + baudport = strstr(device, ":"); + if (baudport) { + /* Split device from baudrate. */ + *baudport = '\0'; + baudport++; + } + if (!baudport || !strlen(baudport)) { + msg_perr("Error: No baudrate specified.\n" + "Use flashrom -p serprog:dev=/dev/device:baud\n"); + free(device); + return 1; + } + if (strlen(device)) { + sp_fd = sp_openserport(device, atoi(baudport)); + have_device++; + } } - num = strstr(programmer_param, ":"); - len = num - programmer_param; - if (!len) goto nodevice; - if (!num) { - msg_perr("Error: No port or baudrate specified to serial programmer driver.\n" - "Use flashrom -p serprog=/dev/device:baud or flashrom -p serprog=ip:port\n"); - exit(1); + if (device && !strlen(device)) { + msg_perr("Error: No device specified.\n" + "Use flashrom -p serprog:dev=/dev/device:baud\n"); + free(device); + return 1; } - len = num - programmer_param; - dev = malloc(len + 1); - if (!dev) sp_die("Error: memory allocation failure"); - memcpy(dev, programmer_param, len); - dev[len] = 0; - num = strdup(num + 1); - if (!num) sp_die("Error: memory allocation failure"); - free(programmer_param); - programmer_param = NULL; + free(device);
- if (dev[0] == '/') sp_fd = sp_openserport(dev, atoi(num)); - else sp_fd = sp_opensocket(dev, atoi(num)); + device = extract_param(&programmer_param, "ip", ","); + if (have_device && device) { + msg_perr("Error: Both host and device specified.\n" + "Please use either dev= or ip= but not both.\n"); + free(device); + return 1; + } + if (device && strlen(device)) { + baudport = strstr(device, ":"); + if (baudport) { + /* Split host from port. */ + *baudport = '\0'; + baudport++; + } + if (!baudport || !strlen(baudport)) { + msg_perr("Error: No port specified.\n" + "Use flashrom -p serprog:ip=ipaddr:port\n"); + free(device); + return 1; + } + if (strlen(device)) { + sp_fd = sp_opensocket(device, atoi(baudport)); + have_device++; + } + } + if (device && !strlen(device)) { + msg_perr("Error: No host specified.\n" + "Use flashrom -p serprog:ip=ipaddr:port\n"); + free(device); + return 1; + } + free(device);
- free(dev); dev = NULL; - free(num); num = NULL; + if (!have_device) { + msg_perr("Error: Neither host nor device specified.\n" + "Use flashrom -p serprog:dev=/dev/device:baud or " + "flashrom -p serprog:ip=ipaddr:port\n"); + return 1; + }
msg_pdbg(MSGHEADER "connected - attempting to synchronize\n");
Index: flashrom-extract_param_everywhere/atahpt.c =================================================================== --- flashrom-extract_param_everywhere/atahpt.c (Revision 1069) +++ flashrom-extract_param_everywhere/atahpt.c (Arbeitskopie) @@ -45,7 +45,7 @@ get_io_perms();
io_base_addr = pcidev_init(PCI_VENDOR_ID_HPT, PCI_BASE_ADDRESS_4, - ata_hpt, programmer_param); + ata_hpt);
/* Enable flash access. */ reg32 = pci_read_long(pcidev_dev, REG_FLASH_ACCESS); @@ -66,7 +66,6 @@ reg32 &= ~(1 << 24); pci_write_long(pcidev_dev, REG_FLASH_ACCESS, reg32);
- free(programmer_param); pci_cleanup(pacc); release_io_perms(); return 0; Index: flashrom-extract_param_everywhere/nic3com.c =================================================================== --- flashrom-extract_param_everywhere/nic3com.c (Revision 1069) +++ flashrom-extract_param_everywhere/nic3com.c (Arbeitskopie) @@ -59,7 +59,8 @@ get_io_perms();
io_base_addr = pcidev_init(PCI_VENDOR_ID_3COM, PCI_BASE_ADDRESS_0, - nics_3com, programmer_param); + nics_3com); + id = pcidev_dev->device_id;
/* 3COM 3C90xB cards need a special fixup. */ @@ -96,7 +97,6 @@ OUTL(internal_conf, io_base_addr + INTERNAL_CONFIG); }
- free(programmer_param); pci_cleanup(pacc); release_io_perms(); return 0; Index: flashrom-extract_param_everywhere/ft2232_spi.c =================================================================== --- flashrom-extract_param_everywhere/ft2232_spi.c (Revision 1069) +++ flashrom-extract_param_everywhere/ft2232_spi.c (Arbeitskopie) @@ -76,46 +76,49 @@ int f; struct ftdi_context *ftdic = &ftdic_context; unsigned char buf[512]; - char *portpos = NULL; int ft2232_type = FTDI_FT4232H; enum ftdi_interface ft2232_interface = INTERFACE_B; + char *arg;
- if (ftdi_init(ftdic) < 0) { - msg_perr("ftdi_init failed\n"); - return EXIT_FAILURE; // TODO - } - - if (programmer_param && !strlen(programmer_param)) { - free(programmer_param); - programmer_param = NULL; - } - if (programmer_param) { - if (strstr(programmer_param, "2232")) + arg = extract_param(&programmer_param, "type", ",:"); + if (arg) { + if (!strcasecmp(arg, "2232H")) ft2232_type = FTDI_FT2232H; - if (strstr(programmer_param, "4232")) + else if (!strcasecmp(arg, "4232H")) ft2232_type = FTDI_FT4232H; - portpos = strstr(programmer_param, "port="); - if (portpos) { - portpos += 5; - switch (toupper(*portpos)) { - case 'A': - ft2232_interface = INTERFACE_A; - break; - case 'B': - ft2232_interface = INTERFACE_B; - break; - default: - msg_perr("Invalid interface specified, " - "using default.\n"); - } + else { + msg_perr("Error: Invalid device type specified.\n"); + free(arg); + return 1; } - free(programmer_param); } + free(arg); + arg = extract_param(&programmer_param, "port", ",:"); + if (arg) { + switch (toupper(*arg)) { + case 'A': + ft2232_interface = INTERFACE_A; + break; + case 'B': + ft2232_interface = INTERFACE_B; + break; + default: + msg_perr("Error: Invalid port/interface specified.\n"); + free(arg); + return 1; + } + } + free(arg); msg_pdbg("Using device type %s ", (ft2232_type == FTDI_FT2232H) ? "2232H" : "4232H"); msg_pdbg("interface %s\n", (ft2232_interface == INTERFACE_A) ? "A" : "B");
+ if (ftdi_init(ftdic) < 0) { + msg_perr("ftdi_init failed\n"); + return EXIT_FAILURE; // TODO + } + f = ftdi_usb_open(ftdic, 0x0403, ft2232_type);
if (f < 0 && f != -5) { Index: flashrom-extract_param_everywhere/satasii.c =================================================================== --- flashrom-extract_param_everywhere/satasii.c (Revision 1069) +++ flashrom-extract_param_everywhere/satasii.c (Arbeitskopie) @@ -46,8 +46,8 @@
get_io_perms();
- pcidev_init(PCI_VENDOR_ID_SII, PCI_BASE_ADDRESS_0, satas_sii, - programmer_param); + pcidev_init(PCI_VENDOR_ID_SII, PCI_BASE_ADDRESS_0, satas_sii); + id = pcidev_dev->device_id;
if ((id == 0x3132) || (id == 0x3124)) { @@ -71,7 +71,6 @@
int satasii_shutdown(void) { - free(programmer_param); pci_cleanup(pacc); release_io_perms(); return 0; Index: flashrom-extract_param_everywhere/dummyflasher.c =================================================================== --- flashrom-extract_param_everywhere/dummyflasher.c (Revision 1069) +++ flashrom-extract_param_everywhere/dummyflasher.c (Arbeitskopie) @@ -25,44 +25,46 @@ #include "flash.h" #include "chipdrivers.h"
+static void tolower_string(char *str) +{ + for (; *str != '\0'; str++) + *str = (char)tolower((unsigned char)*str); +} + int dummy_init(void) { - int i; + char *bustext = NULL; + msg_pspew("%s\n", __func__);
- /* "all" is equivalent to specifying no type. */ - if (programmer_param && (!strcmp(programmer_param, "all"))) { - free(programmer_param); - programmer_param = NULL; - } - if (!programmer_param) - programmer_param = strdup("parallel,lpc,fwh,spi"); + bustext = extract_param(&programmer_param, "bus", ",:"); + msg_pdbg("Requested buses are: %s\n", bustext ? bustext : "default"); + if (!bustext) + bustext = strdup("parallel+lpc+fwh+spi"); /* Convert the parameters to lowercase. */ - for (i = 0; programmer_param[i] != '\0'; i++) - programmer_param[i] = - (char)tolower((unsigned char)programmer_param[i]); + tolower_string(bustext);
buses_supported = CHIP_BUSTYPE_NONE; - if (strstr(programmer_param, "parallel")) { + if (strstr(bustext, "parallel")) { buses_supported |= CHIP_BUSTYPE_PARALLEL; msg_pdbg("Enabling support for %s flash.\n", "parallel"); } - if (strstr(programmer_param, "lpc")) { + if (strstr(bustext, "lpc")) { buses_supported |= CHIP_BUSTYPE_LPC; msg_pdbg("Enabling support for %s flash.\n", "LPC"); } - if (strstr(programmer_param, "fwh")) { + if (strstr(bustext, "fwh")) { buses_supported |= CHIP_BUSTYPE_FWH; msg_pdbg("Enabling support for %s flash.\n", "FWH"); } - if (strstr(programmer_param, "spi")) { + if (strstr(bustext, "spi")) { buses_supported |= CHIP_BUSTYPE_SPI; spi_controller = SPI_CONTROLLER_DUMMY; msg_pdbg("Enabling support for %s flash.\n", "SPI"); } if (buses_supported == CHIP_BUSTYPE_NONE) msg_pdbg("Support for all flash bus types disabled.\n"); - free(programmer_param); + free(bustext); return 0; }
Index: flashrom-extract_param_everywhere/chipset_enable.c =================================================================== --- flashrom-extract_param_everywhere/chipset_enable.c (Revision 1069) +++ flashrom-extract_param_everywhere/chipset_enable.c (Arbeitskopie) @@ -300,11 +300,8 @@ int max_decode_fwh_decode = 0; int contiguous = 1;
- if (programmer_param) - idsel = strstr(programmer_param, "fwh_idsel="); - - if (idsel) { - idsel += strlen("fwh_idsel="); + idsel = extract_param(&programmer_param, "fwh_idsel", ",:"); + if (idsel && strlen(idsel)) { fwh_conf = (uint32_t)strtoul(idsel, NULL, 0);
/* FIXME: Need to undo this on shutdown. */ @@ -312,7 +309,15 @@ pci_write_long(dev, 0xd0, fwh_conf); pci_write_word(dev, 0xd4, fwh_conf); /* FIXME: Decode settings are not changed. */ + } else if (idsel) { + msg_perr("Error: idsel= specified, but no number given.\n"); + free(idsel); + /* FIXME: Return failure here once internal_init() starts + * to care about the return value of the chipset enable. + */ + exit(1); } + free(idsel);
/* Ignore all legacy ranges below 1 MB. * We currently only support flashing the chip which responds to Index: flashrom-extract_param_everywhere/flashrom.c =================================================================== --- flashrom-extract_param_everywhere/flashrom.c (Revision 1069) +++ flashrom-extract_param_everywhere/flashrom.c (Arbeitskopie) @@ -434,6 +434,7 @@
int programmer_init(char *param) { + int ret; /* Initialize all programmer specific data. */ /* Default to unlimited decode sizes. */ max_rom_decode = (const struct decode_sizes) { @@ -456,7 +457,13 @@ programmer_param = param; msg_pdbg("Initializing %s programmer\n", programmer_table[programmer].name); - return programmer_table[programmer].init(); + ret = programmer_table[programmer].init(); + if (programmer_param && strlen(programmer_param)) { + msg_perr("Unhandled programmer parameters: %s\n", + programmer_param); + /* Do not error out here, the init itself was successful. */ + } + return ret; }
int programmer_shutdown(void) @@ -564,22 +571,24 @@ char *strcat_realloc(char *dest, const char *src) { dest = realloc(dest, strlen(dest) + strlen(src) + 1); - if (!dest) + if (!dest) { + msg_gerr("Out of memory!\n"); return NULL; + } strcat(dest, src); return dest; }
/* This is a somewhat hacked function similar in some ways to strtok(). - * It will look for needle in haystack, return a copy of needle and remove - * everything from the first occurrence of needle to the next delimiter - * from haystack. + * It will look for needle with a subsequent '=' in haystack, return a copy of + * needle and remove everything from the first occurrence of needle to the next + * delimiter from haystack. */ char *extract_param(char **haystack, char *needle, char *delim) { - char *param_pos, *rest, *tmp; - char *dev = NULL; - int devlen; + char *param_pos, *opt_pos, *rest; + char *opt = NULL; + int optlen; int needlelen;
needlelen = strlen(needle); @@ -595,43 +604,41 @@ do { if (!param_pos) return NULL; - /* Beginning of the string? */ - if (param_pos == *haystack) - break; - /* After a delimiter? */ - if (strchr(delim, *(param_pos - 1))) - break; + /* Needle followed by '='? */ + if (param_pos[needlelen] == '=') { + + /* Beginning of the string? */ + if (param_pos == *haystack) + break; + /* After a delimiter? */ + if (strchr(delim, *(param_pos - 1))) + break; + } /* Continue searching. */ param_pos++; param_pos = strstr(param_pos, needle); } while (1); - + if (param_pos) { - param_pos += strlen(needle); - devlen = strcspn(param_pos, delim); - if (devlen) { - dev = malloc(devlen + 1); - if (!dev) { - msg_gerr("Out of memory!\n"); - exit(1); - } - strncpy(dev, param_pos, devlen); - dev[devlen] = '\0'; + /* Get the string after needle and '='. */ + opt_pos = param_pos + needlelen + 1; + optlen = strcspn(opt_pos, delim); + /* Return an empty string if the parameter was empty. */ + opt = malloc(optlen + 1); + if (!opt) { + msg_gerr("Out of memory!\n"); + exit(1); } - rest = param_pos + devlen; + strncpy(opt, opt_pos, optlen); + opt[optlen] = '\0'; + rest = opt_pos + optlen; + /* Skip all delimiters after the current parameter. */ rest += strspn(rest, delim); - param_pos -= strlen(needle); memmove(param_pos, rest, strlen(rest) + 1); - tmp = realloc(*haystack, strlen(*haystack) + 1); - if (!tmp) { - msg_gerr("Out of memory!\n"); - exit(1); - } - *haystack = tmp; + /* We could shrink haystack, but the effort is not worth it. */ } -
- return dev; + return opt; }
/* start is an offset to the base address of the flash chip */ Index: flashrom-extract_param_everywhere/internal.c =================================================================== --- flashrom-extract_param_everywhere/internal.c (Revision 1069) +++ flashrom-extract_param_everywhere/internal.c (Arbeitskopie) @@ -121,36 +121,45 @@ int force_laptop = 0; char *arg;
- arg = extract_param(&programmer_param, "boardenable=", ",:"); + arg = extract_param(&programmer_param, "boardenable", ",:"); if (arg && !strcmp(arg,"force")) { force_boardenable = 1; } else if (arg && !strlen(arg)) { msg_perr("Missing argument for boardenable.\n"); + free(arg); + return 1; } else if (arg) { msg_perr("Unknown argument for boardenable: %s\n", arg); - exit(1); + free(arg); + return 1; } free(arg);
- arg = extract_param(&programmer_param, "boardmismatch=", ",:"); + arg = extract_param(&programmer_param, "boardmismatch", ",:"); if (arg && !strcmp(arg,"force")) { force_boardmismatch = 1; } else if (arg && !strlen(arg)) { msg_perr("Missing argument for boardmismatch.\n"); + free(arg); + return 1; } else if (arg) { msg_perr("Unknown argument for boardmismatch: %s\n", arg); - exit(1); + free(arg); + return 1; } free(arg);
- arg = extract_param(&programmer_param, "laptop=", ",:"); + arg = extract_param(&programmer_param, "laptop", ",:"); if (arg && !strcmp(arg,"force_I_want_a_brick")) { force_laptop = 1; } else if (arg && !strlen(arg)) { msg_perr("Missing argument for laptop.\n"); + free(arg); + return 1; } else if (arg) { msg_perr("Unknown argument for laptop: %s\n", arg); - exit(1); + free(arg); + return 1; } free(arg);
Index: flashrom-extract_param_everywhere/nicnatsemi.c =================================================================== --- flashrom-extract_param_everywhere/nicnatsemi.c (Revision 1069) +++ flashrom-extract_param_everywhere/nicnatsemi.c (Arbeitskopie) @@ -39,7 +39,7 @@ get_io_perms();
io_base_addr = pcidev_init(PCI_VENDOR_ID_NATSEMI, PCI_BASE_ADDRESS_0, - nics_natsemi, programmer_param); + nics_natsemi);
buses_supported = CHIP_BUSTYPE_PARALLEL;
@@ -48,7 +48,6 @@
int nicnatsemi_shutdown(void) { - free(programmer_param); pci_cleanup(pacc); release_io_perms(); return 0;