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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Fri May 15 00:16:09 CEST 2009


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 <uwe at hermann-uwe.de>
>   

Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
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

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list