Re: [coreboot] [PATCH] flashrom: Force -p nic3com=bb:dd.f if multiple cards found

[Carl-Daniel Hailfinger]

On 14.05.2009 23:49, Uwe Hermann wrote:
> Make the nic3com code check how many supported NICs are found. If we find
> multiple ones, abort with a message to the user, suggesting to use the
>
> flashrom -p nic3com=bb:dd.f
>
> syntax. If exactly one supported NIC is found, use it. If none is found,
> abort with an error.
>
> Print the bb:dd.f numbers for all supported NICs we find, so the user
> doesn't have to poke around in lspci output to find the desired bb:dd.f.
>
> Also, drop one pci_read_long() in favor of using the already existing
> base_addr[0] struct field.
>
> While I'm at it, update the manpage some more to mention and fully document
> the external programmer support we have (or will have soon).
>
> The patch is tested on hardware:
>
> $ flashrom -p nic3com
> flashrom v0.9.0-r511
> Found NIC "3COM 3C905C: EtherLink 10/100 PCI (TX)" (10b7:9200, 05:04.0, 0xa800)
> Found NIC "3COM 3C905C: EtherLink 10/100 PCI (TX)" (10b7:9200, 05:03.0, 0xa400)
> Error: Multiple supported NICs found. Please use 'flashrom -p nic3com=bb:dd.f'.
>
> $ flashrom -p nic3com=05:04.0
> flashrom v0.9.0-r511
> Found NIC "3COM 3C905C: EtherLink 10/100 PCI (TX)" (10b7:9200, 05:04.0, 0xa800)
> Calibrating delay loop... OK.
> Found chip "Atmel AT49BV512" (64 KB) at physical address 0xffff0000.
> No operations were specified.
>
> Signed-off-by: Uwe Hermann

Acked-by: Carl-Daniel Hailfinger 
with the changes below.
> Index: flashrom.8
>
> --- flashrom.8	(revision 512)
> +++ flashrom.8	(working copy)
> @@ -1,13 +1,17 @@
> -.TH FLASHROM 8 "April 11, 2009"
> +.TH FLASHROM 8 "May 14, 2009"
>  .SH NAME
> -flashrom - read, write, verify and erase BIOS/ROM/flash chips
> +flashrom - detect, read, write, verify and erase flash chips
>  .SH SYNOPSIS
> -.B flashrom \fR[\fB-rwvEVfLhR\fR] [\fB-c\fR chipname] [\fB-s\fR exclude_start] [\fB-e\fR exclude_end]
>     [\fB-m\fR [vendor:]part] [\fB-l\fR file.layout] [\fB-i\fR image_name] [file]
>
> +.B flashrom \fR[\fB-EVfLhR\fR] [\fB-r\fR file] [\fB-w\fR file] [\fB-v\fR file]
>     [\fB\-c\fR chipname] [\fB\-s\fR addr] [\fB\-e\fR addr] [\fB\-m\fR [vendor:]part]
>     [\fB\-l\fR file] [\fB\-i\fR image] [\fB\-p\fR programmer] [file]
>
> .SH DESCRIPTION
>  .B flashrom
> -is a utility for reading, writing, verifying and erasing flash ROM chips.
> -It's often used to flash BIOS/coreboot/firmware images.
> +is a utility for detecting, reading, writing, verifying and erasing flash ROM
> +chips. It's often used to flash BIOS/EFI/coreboot/firmware images in-system
> +using a supported mainboard, but it also supports (or will support) flashing
> +of network cards (NICs), SATA controller cards, graphics cards, CD-ROM/DVD

Please remove graphics cards and CD-ROM/DVD drives unless you have
matching docs. I know that ATI graphics cards don't have sufficient
documentation for flashing and I just checked with the contact I had who
extended flashrom a bit. He was only able to read from ATI graphics
cards, and even the reads were unreliable. ID and other stuff was a no-go.

We can always raise expectations later if we actually see this stuff on
the horizon.
> +drives, and other external devices which can program flash chips.
>  .PP
>  It supports a wide range of DIP32, PLCC32, DIP8, SO8/SOIC8, TSOP32, and
>  TSOP40 chips, which use various protocols such as LPC, FWH, parallel flash,
> @@ -126,11 +130,24 @@
>  .B "-p, --programmer <name>"
>  Specify the programmer device. Currently supported are:
>  .sp
> -.BR " internal" " (default, for in-system flashing in the mainboard)"
> -.br
> -.BR " nic3com" " (for flash ROMs on 3COM network cards)"
> -.br
> -.BR " dummy" " (just prints all operations and accesses)"
> +.BR "* internal" " (default, for in-system flashing in the mainboard)"
> +.sp
> +.BR "* nic3com" " (for flash ROMs on 3COM network cards)"
> +.sp
> +If you have multiple supported NICs in your system, you must use
> +.B "flashrom -p nic3com=bb:dd.f"
> +to explicitly select one of them, where
> +.B bb
> +is the PCI bus number,
> +.B dd
> +is the PCI device number, and
> +.B f
> +is the PCI function number of the desired NIC.
> +.sp
> +Example:
> +.B "flashrom -p nic3com=05:04.0"
> +.sp
> +.BR "* dummy" " (just prints all operations and accesses)"
>  .TP
>  .B "-h, --help"
>  Show a help text and exit.
> Index: flash.h
> ===================================================================
> --- flash.h	(revision 512)
> +++ flash.h	(working copy)
> @@ -594,6 +594,7 @@
>  uint8_t internal_chip_readb(const volatile void *addr);
>  uint16_t internal_chip_readw(const volatile void *addr);
>  uint32_t internal_chip_readl(const volatile void *addr);
> +void get_io_perms(void);
>  #if defined(__FreeBSD__) || defined(__DragonFly__)
>  extern int io_fd;
>  #endif

Drop this hunk. It's already part of r512.
> Index: nic3com.c
>
> --- nic3com.c	(revision 512)
> +++ nic3com.c	(working copy)
> @@ -67,18 +67,18 @@
>
> uint32_t nic3com_validate(struct pci_dev *dev)
>  {
> int i = 0;
> uint32_t addr = -1;
>
> int i;
>
> uint32_t addr;
>
> for (i = 0; nics[i].device_name != NULL; i++) {
> if (dev->device_id != nics[i].device_id)
> 	continue;
> addr = pci_read_long(dev, PCI_IO_BASE_ADDRESS) & ~0x03;
> addr = (uint32_t)(dev->base_addr[0] & ~0x03);
>
> printf("Found NIC \"3COM %s\" (%04x:%04x), addr = 0x%x\n",
>        nics[i].device_name, PCI_VENDOR_ID_3COM,
>        nics[i].device_id, addr);
> printf("Found NIC \"3COM %s\" (%04x:%04x, %02x:%02x.%x, "
>        "0x%x)\n", nics[i].device_name, dev->vendor_id,
>        dev->device_id, dev->bus, dev->dev, dev->func, addr);

Can you please extend this message to tell the user what is what? I had
to read the code to understand what the third value (BAR) was.
> if (nics[i].status == NT) {
> 	printf("===\nThis NIC is UNTESTED. Please email a "
>
> @@ -90,41 +90,47 @@
>    	return addr;
>    }
> return addr;
> return -1;
>
> }
>
> int nic3com_init(void)
>  {
>    struct pci_dev *dev;
>    char *msg = NULL;
>
> int found = 0;
>
> get_io_perms();
>
> pacc = pci_alloc();     /* Get the pci_access structure */
> pci_init(pacc);         /* Initialize the PCI library */
> pci_scan_bus(pacc);     /* We want to get the list of devices */
>
> pci_filter_init(pacc, &filter);
>
> /* Filter by vendor, or bb:dd.f if supplied by the user. */
> if (nic_pcidev != NULL) {
> pci_filter_init(pacc, &filter);
> if ((msg = pci_filter_parse_slot(&filter, nic_pcidev))) {
> 	fprintf(stderr, "Error: %s\n", msg);
> 	exit(1);
> }
> } else {
> filter.vendor = PCI_VENDOR_ID_3COM;
> }

I'm unhappy with the new logic here. We either filter by bus:dev.fn or
by vendor ID. The vendor ID filter should always be active. Otherwise,
an incorrectly specified bdf can match a non-3com card by accident as
long as the device ID matches.
Please move the content of the else branch out of the if clause and run
it unconditionally.
> if (!filter.vendor && !filter.device) {
> pci_filter_init(pacc, &filter);
> filter.vendor = PCI_VENDOR_ID_3COM;
> for (dev = pacc->devices; dev; dev = dev->next) {
> if (pci_filter_match(&filter, dev)) {
> 	if ((io_base_addr = nic3com_validate(dev)) != -1)
> 		found++;
> }
> }
> dev = pci_dev_find_filter(filter);
> if (dev && (dev->vendor_id == PCI_VENDOR_ID_3COM))
> io_base_addr = nic3com_validate(dev);
> else {
>
> /* Only continue if exactly one supported NIC has been found. */
>
> if (found == 0) {
> fprintf(stderr, "Error: No supported 3COM NIC found.\n");
> exit(1);
>
> } else if (found > 1) {
> fprintf(stderr, "Error: Multiple supported NICs found. "
> 	"Please use 'flashrom -p nic3com=bb:dd.f'.\n");
> exit(1);
>
> }
>
> /*

Regards,
Carl-Daniel