Am 28.06.2012 20:28 schrieb Denis 'GNUtoo' Carikli:

On Wed, 2012-06-27 at 09:05 +0200, Carl-Daniel Hailfinger wrote:

...

...

Hi Denis,

Am 26.06.2012 01:53 schrieb Denis 'GNUtoo' Carikli:

...

...

# ./flashrom -p gfxnvidia -VVV flashrom v0.9.5.2-r1546 on Linux 3.0.0-19-generic-pae (i686) flashrom was built with libpci 3.1.7, GCC 4.6.1, little endian Command line (3 args): ./flashrom -p gfxnvidia -VVV Calibrating delay loop... OK. Initializing gfxnvidia programmer Found "NVIDIA RIVA TNT2 Ultra" (168c:0029, BDF 03:06.0). [...]

This should not have happened. Apparently the PCI code (either in flashrom, or libpci) ignores the vendor, and only matches the device in this case.

Ah yes. pcidev.c:pcidev_validate() only checks device_id match, not vendor_id match. Ouch!

Thanks for reporting this bug!

I'd be very interested in the flashrom verbose output for the following patch: Check vendor_id for PCI based external programmers. Clean up PCI device detection code. Note: Slight changes in behaviour are possible, especially on dual/quad chip NICs which appear as more than one PCI device. Found devices are no longer printed at _pinfo level, but rather at _pdbg level. This patch is essentially a small code move, better device counting and lots of indentation changes (look at diff -uw to see real code changes). Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Index: flashrom-pcidev_check_vendorid_cleanup/pcidev.c =================================================================== --- flashrom-pcidev_check_vendorid_cleanup/pcidev.c (Revision 1547) +++ flashrom-pcidev_check_vendorid_cleanup/pcidev.c (Arbeitskopie) @@ -35,157 +35,130 @@ TYPE_UNKNOWN }; -uintptr_t pcidev_validate(struct pci_dev *dev, int bar, - const struct pcidev_status *devs) +uintptr_t pcidev_validate(struct pci_dev *dev, int bar) { - int i; uint64_t addr; uint32_t upperaddr; uint8_t headertype; uint16_t supported_cycles; enum pci_bartype bartype = TYPE_UNKNOWN; - for (i = 0; devs[i].device_name != NULL; i++) { - if (dev->device_id != devs[i].device_id) - continue; - msg_pinfo("Found \"%s %s\" (%04x:%04x, BDF %02x:%02x.%x).\n", - devs[i].vendor_name, devs[i].device_name, - dev->vendor_id, dev->device_id, dev->bus, dev->dev, - dev->func); + headertype = pci_read_byte(dev, PCI_HEADER_TYPE) & 0x7f; + msg_pspew("PCI header type 0x%02x\n", headertype); - headertype = pci_read_byte(dev, PCI_HEADER_TYPE) & 0x7f; - msg_pspew("PCI header type 0x%02x\n", headertype); + /* Don't use dev->base_addr[x] (as value for 'bar'), won't work on older libpci. */ + addr = pci_read_long(dev, bar); - /* - * Don't use dev->base_addr[x] (as value for 'bar'), won't - * work on older libpci. - */ - addr = pci_read_long(dev, bar); - - /* Sanity checks. */ - switch (headertype) { - case PCI_HEADER_TYPE_NORMAL: - switch (bar) { - case PCI_BASE_ADDRESS_0: - case PCI_BASE_ADDRESS_1: - case PCI_BASE_ADDRESS_2: - case PCI_BASE_ADDRESS_3: - case PCI_BASE_ADDRESS_4: - case PCI_BASE_ADDRESS_5: - if ((addr & PCI_BASE_ADDRESS_SPACE) == - PCI_BASE_ADDRESS_SPACE_IO) - bartype = TYPE_IOBAR; - else - bartype = TYPE_MEMBAR; - break; - case PCI_ROM_ADDRESS: - bartype = TYPE_ROMBAR; - break; - } + /* Sanity checks. */ + switch (headertype) { + case PCI_HEADER_TYPE_NORMAL: + switch (bar) { + case PCI_BASE_ADDRESS_0: + case PCI_BASE_ADDRESS_1: + case PCI_BASE_ADDRESS_2: + case PCI_BASE_ADDRESS_3: + case PCI_BASE_ADDRESS_4: + case PCI_BASE_ADDRESS_5: + if ((addr & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) + bartype = TYPE_IOBAR; + else + bartype = TYPE_MEMBAR; break; - case PCI_HEADER_TYPE_BRIDGE: - switch (bar) { - case PCI_BASE_ADDRESS_0: - case PCI_BASE_ADDRESS_1: - if ((addr & PCI_BASE_ADDRESS_SPACE) == - PCI_BASE_ADDRESS_SPACE_IO) - bartype = TYPE_IOBAR; - else - bartype = TYPE_MEMBAR; - break; - case PCI_ROM_ADDRESS1: - bartype = TYPE_ROMBAR; - break; - } + case PCI_ROM_ADDRESS: + bartype = TYPE_ROMBAR; break; - case PCI_HEADER_TYPE_CARDBUS: + } + break; + case PCI_HEADER_TYPE_BRIDGE: + switch (bar) { + case PCI_BASE_ADDRESS_0: + case PCI_BASE_ADDRESS_1: + if ((addr & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) + bartype = TYPE_IOBAR; + else + bartype = TYPE_MEMBAR; break; - default: - msg_perr("Unknown PCI header type 0x%02x, BAR type " - "cannot be determined reliably.\n", headertype); + case PCI_ROM_ADDRESS1: + bartype = TYPE_ROMBAR; break; } + break; + case PCI_HEADER_TYPE_CARDBUS: + break; + default: + msg_perr("Unknown PCI header type 0x%02x, BAR type cannot be determined reliably.\n", + headertype); + break; + } - supported_cycles = pci_read_word(dev, PCI_COMMAND); + supported_cycles = pci_read_word(dev, PCI_COMMAND); - msg_pdbg("Requested BAR is "); - switch (bartype) { - case TYPE_MEMBAR: - msg_pdbg("MEM"); - if (!(supported_cycles & PCI_COMMAND_MEMORY)) { - msg_perr("MEM BAR access requested, but device " - "has MEM space accesses disabled.\n"); - /* TODO: Abort here? */ - } - msg_pdbg(", %sbit, %sprefetchable\n", - ((addr & 0x6) == 0x0) ? "32" : - (((addr & 0x6) == 0x4) ? "64" : "reserved"), - (addr & 0x8) ? "" : "not "); - if ((addr & 0x6) == 0x4) { - /* The spec says that a 64-bit register consumes - * two subsequent dword locations. - */ - upperaddr = pci_read_long(dev, bar + 4); - if (upperaddr != 0x00000000) { - /* Fun! A real 64-bit resource. */ - if (sizeof(uintptr_t) != sizeof(uint64_t)) { - msg_perr("BAR unreachable!"); - /* TODO: Really abort here? If - * multiple PCI devices match, - * we might never tell the user - * about the other devices. - */ - return 0; - } - addr |= (uint64_t)upperaddr << 32; + msg_pdbg("Requested BAR is "); + switch (bartype) { + case TYPE_MEMBAR: + msg_pdbg("MEM"); + if (!(supported_cycles & PCI_COMMAND_MEMORY)) { + msg_perr("MEM BAR access requested, but device " + "has MEM space accesses disabled.\n"); + /* TODO: Abort here? */ + } + msg_pdbg(", %sbit, %sprefetchable\n", + ((addr & 0x6) == 0x0) ? "32" : + (((addr & 0x6) == 0x4) ? "64" : "reserved"), + (addr & 0x8) ? "" : "not "); + if ((addr & 0x6) == 0x4) { + /* The spec says that a 64-bit register consumes + * two subsequent dword locations. + */ + upperaddr = pci_read_long(dev, bar + 4); + if (upperaddr != 0x00000000) { + /* Fun! A real 64-bit resource. */ + if (sizeof(uintptr_t) != sizeof(uint64_t)) { + msg_perr("BAR unreachable!"); + /* TODO: Really abort here? If + * multiple PCI devices match, + * we might never tell the user + * about the other devices. + */ + return 0; } + addr |= (uint64_t)upperaddr << 32; } - addr &= PCI_BASE_ADDRESS_MEM_MASK; - break; - case TYPE_IOBAR: - msg_pdbg("I/O\n"); + } + addr &= PCI_BASE_ADDRESS_MEM_MASK; + break; + case TYPE_IOBAR: + msg_pdbg("I/O\n"); #if __FLASHROM_HAVE_OUTB__ - if (!(supported_cycles & PCI_COMMAND_IO)) { - msg_perr("I/O BAR access requested, but device " - "has I/O space accesses disabled.\n"); - /* TODO: Abort here? */ - } + if (!(supported_cycles & PCI_COMMAND_IO)) { + msg_perr("I/O BAR access requested, but device " + "has I/O space accesses disabled.\n"); + /* TODO: Abort here? */ + } #else - msg_perr("I/O BAR access requested, but flashrom does " - "not support I/O BAR access on this platform " - "(yet).\n"); + msg_perr("I/O BAR access requested, but flashrom does " + "not support I/O BAR access on this platform " + "(yet).\n"); #endif - addr &= PCI_BASE_ADDRESS_IO_MASK; - break; - case TYPE_ROMBAR: - msg_pdbg("ROM\n"); - /* Not sure if this check is needed. */ - if (!(supported_cycles & PCI_COMMAND_MEMORY)) { - msg_perr("MEM BAR access requested, but device " - "has MEM space accesses disabled.\n"); - /* TODO: Abort here? */ - } - addr &= PCI_ROM_ADDRESS_MASK; - break; - case TYPE_UNKNOWN: - msg_perr("BAR type unknown, please report a bug at " - "flashrom@flashrom.org\n"); + addr &= PCI_BASE_ADDRESS_IO_MASK; + break; + case TYPE_ROMBAR: + msg_pdbg("ROM\n"); + /* Not sure if this check is needed. */ + if (!(supported_cycles & PCI_COMMAND_MEMORY)) { + msg_perr("MEM BAR access requested, but device " + "has MEM space accesses disabled.\n"); + /* TODO: Abort here? */ } - - if (devs[i].status == NT) { - msg_pinfo("===\nThis PCI device is UNTESTED. Please " - "report the 'flashrom -p xxxx' output \n" - "to flashrom@flashrom.org if it works " - "for you. Please add the name of your\n" - "PCI device to the subject. Thank you for " - "your help!\n===\n"); - } - - return (uintptr_t)addr; + addr &= PCI_ROM_ADDRESS_MASK; + break; + case TYPE_UNKNOWN: + msg_perr("BAR type unknown, please report a bug at " + "flashrom@flashrom.org\n"); } - return 0; + return (uintptr_t)addr; } uintptr_t pcidev_init(int bar, const struct pcidev_status *devs) @@ -195,6 +168,7 @@ char *pcidev_bdf; char *msg = NULL; int found = 0; + int i; uintptr_t addr = 0, curaddr = 0; pacc = pci_alloc(); /* Get the pci_access structure */ @@ -214,10 +188,29 @@ for (dev = pacc->devices; dev; dev = dev->next) { if (pci_filter_match(&filter, dev)) { + /* Check against list of supported devices. */ + for (i = 0; devs[i].device_name != NULL; i++) + if ((dev->vendor_id == devs[i].vendor_id) && + (dev->device_id == devs[i].device_id)) + break; + /* Not supported, try the next one. */ + if (devs[i].device_name == NULL) + continue; + + msg_pdbg("Found \"%s %s\" (%04x:%04x, BDF %02x:%02x.%x).\n", devs[i].vendor_name, + devs[i].device_name, dev->vendor_id, dev->device_id, dev->bus, dev->dev, + dev->func); + if (devs[i].status == NT) + msg_pinfo("===\nThis PCI device is UNTESTED. Please report the 'flashrom -p " + "xxxx' output \n" + "to flashrom@flashrom.org if it works for you. Please add the name " + "of your\n" + "PCI device to the subject. Thank you for your help!\n===\n"); + /* FIXME: We should count all matching devices, not * just those with a valid BAR. */ - if ((addr = pcidev_validate(dev, bar, devs)) != 0) { + if ((addr = pcidev_validate(dev, bar)) != 0) { curaddr = addr; pcidev_dev = dev; found++; Index: flashrom-pcidev_check_vendorid_cleanup/nicintel.c =================================================================== --- flashrom-pcidev_check_vendorid_cleanup/nicintel.c (Revision 1547) +++ flashrom-pcidev_check_vendorid_cleanup/nicintel.c (Arbeitskopie) @@ -87,7 +87,7 @@ goto error_out_unmap; /* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */ - addr = pcidev_validate(pcidev_dev, PCI_BASE_ADDRESS_0, nics_intel); + addr = pcidev_validate(pcidev_dev, PCI_BASE_ADDRESS_0); /* FIXME: This is not an aligned mapping. Use 4k? */ nicintel_control_bar = physmap("Intel NIC control/status reg", addr, NICINTEL_CONTROL_MEMMAP_SIZE); Index: flashrom-pcidev_check_vendorid_cleanup/programmer.h =================================================================== --- flashrom-pcidev_check_vendorid_cleanup/programmer.h (Revision 1547) +++ flashrom-pcidev_check_vendorid_cleanup/programmer.h (Arbeitskopie) @@ -227,7 +227,7 @@ const char *vendor_name; const char *device_name; }; -uintptr_t pcidev_validate(struct pci_dev *dev, int bar, const struct pcidev_status *devs); +uintptr_t pcidev_validate(struct pci_dev *dev, int bar); uintptr_t pcidev_init(int bar, const struct pcidev_status *devs); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. -- http://www.hailfinger.org/

Am 14.07.2012 22:04 schrieb Michael Karcher:

Am Samstag, den 30.06.2012, 01:40 +0200 schrieb Carl-Daniel Hailfinger:

...

This patch is essentially a small code move, better device counting and lots of indentation changes (look at diff -uw to see real code changes). So basically you are moving the device test level check from pcidev_validate to pcidev_init. The result is that pcidev_validate does no longer in fact validate whether a candidate device is the correct device, as the name implies. A name like "pcidev_readbar" would make more sense, IMHO.

Indeed. Fixed.

Now, check where pcidev_validate is called from. It is called from pcidev_init to do its original job, namely validating a device, and it is called from nicintel code to get a second BAR, because the first BAR returned by pcidev_init is not really enough. That invocation of pcidev_validate already carries a nice comment reading "/* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */". This indicates that something is wrong with this code. And in my opinion, indeed, it is. The problem is that the current code contains an abstraction violation. Neither pcidev_validate (in the pre-patch form) is really meant to be called from higher level code, but it is meant to be called by pcidev_init only (doing it's job, validating a PCI device, poorly), nor is using a global variable pcidev_dev a good idea for communication between pcidev.c and the clients of that code.

Furthermore, pcidev_init (conveniently, I admit) does two things at once: It finds a PCI device, and it reads one BAR. What about having pcidev_init return the struct pci_dev* instead, so it is up to the client driver to handle that the value instead of putting it in a global variable? Of course, this implies all clients have to use something like pcidev_readbar (the post-patch pcidev_validate) function, even if just a single address is needed. Considering our current codebase, which uses pci_read_long to get base addresses in satamv and satasii, I think giving pcidev_readbar some extra exposure would even be a good educational thing.

And as I go along ranting about the current state of that code, I think it is nice that pcidev_validate does so many sanity checks, but there is one thing I do *not* like about it, you could call it a "missing vital sanity check". pcidev_validate checks whether the given BAR is I/O mapped or memory mapped, and handles each of those types correctly. Great! Finally, it returns the obtained base address, which might either be in I/O or memory space to the caller, *not* indicating whether it just returned an I/O or a memory address. This is quite "convenient", as the callers (should) know what type they expect, and they are not prepared to handle mismatches[1], but it would be even more convenient if this quite important verification of the address space would be performed. So pcidev_readbar (the new pcidev_validate) needs an extra parameter, which should indicate I/O or memory space (or, alternatively, provide pcidev_readbar_io and pcidev_readbar_mem, as there are some strict coding styles claiming the flag parameters are evil, because if there are two different ways a function can be executed (selected by that flag parameter), it really means there are two functions which should have their own name) and have it fail if the BAR type does not match the callers expectation.

I completely agree with all your points.

Considering all the stuff I was writing, I think your current patch might be the first of a series leading towards a better pcidev interface, if you apply the name fix for pcidev_validate.

Done.

I guess the second step would be removing the global pcidev_dev variable, which implies all callers of pcidev_init need to be adjusted.

Yes.

Maybe for modules that really only need one BAR, we can still provide the old behaviour as "pcidev_init_getbar" or "pcidev_init_simple".

I'd rather not introduce a special case here because some people might use it in ways we didn't anticipate.

In a third step, add I/O vs. memory validation to pcidev_readbar (ex pcidev_validate), and remove all pci_read_long(...) &~7; invocations to read BARs.

So the final vote is:

If you rename pcidev_validate to pcidev_readbar, this is Acked-by: Michael Karcher <flashrom@mkarcher.dialup.fu-berlin.de>

Thanks! Updated patch below (only changes from the previous version are 112 column reformattings and your suggested name change. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Index: flashrom-pcidev_check_vendorid_cleanup/pcidev.c =================================================================== --- flashrom-pcidev_check_vendorid_cleanup/pcidev.c (Revision 1547) +++ flashrom-pcidev_check_vendorid_cleanup/pcidev.c (Arbeitskopie) @@ -35,157 +35,122 @@ TYPE_UNKNOWN }; -uintptr_t pcidev_validate(struct pci_dev *dev, int bar, - const struct pcidev_status *devs) +uintptr_t pcidev_readbar(struct pci_dev *dev, int bar) { - int i; uint64_t addr; uint32_t upperaddr; uint8_t headertype; uint16_t supported_cycles; enum pci_bartype bartype = TYPE_UNKNOWN; - for (i = 0; devs[i].device_name != NULL; i++) { - if (dev->device_id != devs[i].device_id) - continue; - msg_pinfo("Found \"%s %s\" (%04x:%04x, BDF %02x:%02x.%x).\n", - devs[i].vendor_name, devs[i].device_name, - dev->vendor_id, dev->device_id, dev->bus, dev->dev, - dev->func); + headertype = pci_read_byte(dev, PCI_HEADER_TYPE) & 0x7f; + msg_pspew("PCI header type 0x%02x\n", headertype); - headertype = pci_read_byte(dev, PCI_HEADER_TYPE) & 0x7f; - msg_pspew("PCI header type 0x%02x\n", headertype); + /* Don't use dev->base_addr[x] (as value for 'bar'), won't work on older libpci. */ + addr = pci_read_long(dev, bar); - /* - * Don't use dev->base_addr[x] (as value for 'bar'), won't - * work on older libpci. - */ - addr = pci_read_long(dev, bar); - - /* Sanity checks. */ - switch (headertype) { - case PCI_HEADER_TYPE_NORMAL: - switch (bar) { - case PCI_BASE_ADDRESS_0: - case PCI_BASE_ADDRESS_1: - case PCI_BASE_ADDRESS_2: - case PCI_BASE_ADDRESS_3: - case PCI_BASE_ADDRESS_4: - case PCI_BASE_ADDRESS_5: - if ((addr & PCI_BASE_ADDRESS_SPACE) == - PCI_BASE_ADDRESS_SPACE_IO) - bartype = TYPE_IOBAR; - else - bartype = TYPE_MEMBAR; - break; - case PCI_ROM_ADDRESS: - bartype = TYPE_ROMBAR; - break; - } + /* Sanity checks. */ + switch (headertype) { + case PCI_HEADER_TYPE_NORMAL: + switch (bar) { + case PCI_BASE_ADDRESS_0: + case PCI_BASE_ADDRESS_1: + case PCI_BASE_ADDRESS_2: + case PCI_BASE_ADDRESS_3: + case PCI_BASE_ADDRESS_4: + case PCI_BASE_ADDRESS_5: + if ((addr & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) + bartype = TYPE_IOBAR; + else + bartype = TYPE_MEMBAR; break; - case PCI_HEADER_TYPE_BRIDGE: - switch (bar) { - case PCI_BASE_ADDRESS_0: - case PCI_BASE_ADDRESS_1: - if ((addr & PCI_BASE_ADDRESS_SPACE) == - PCI_BASE_ADDRESS_SPACE_IO) - bartype = TYPE_IOBAR; - else - bartype = TYPE_MEMBAR; - break; - case PCI_ROM_ADDRESS1: - bartype = TYPE_ROMBAR; - break; - } + case PCI_ROM_ADDRESS: + bartype = TYPE_ROMBAR; break; - case PCI_HEADER_TYPE_CARDBUS: + } + break; + case PCI_HEADER_TYPE_BRIDGE: + switch (bar) { + case PCI_BASE_ADDRESS_0: + case PCI_BASE_ADDRESS_1: + if ((addr & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) + bartype = TYPE_IOBAR; + else + bartype = TYPE_MEMBAR; break; - default: - msg_perr("Unknown PCI header type 0x%02x, BAR type " - "cannot be determined reliably.\n", headertype); + case PCI_ROM_ADDRESS1: + bartype = TYPE_ROMBAR; break; } + break; + case PCI_HEADER_TYPE_CARDBUS: + break; + default: + msg_perr("Unknown PCI header type 0x%02x, BAR type cannot be determined reliably.\n", + headertype); + break; + } - supported_cycles = pci_read_word(dev, PCI_COMMAND); + supported_cycles = pci_read_word(dev, PCI_COMMAND); - msg_pdbg("Requested BAR is "); - switch (bartype) { - case TYPE_MEMBAR: - msg_pdbg("MEM"); - if (!(supported_cycles & PCI_COMMAND_MEMORY)) { - msg_perr("MEM BAR access requested, but device " - "has MEM space accesses disabled.\n"); - /* TODO: Abort here? */ - } - msg_pdbg(", %sbit, %sprefetchable\n", - ((addr & 0x6) == 0x0) ? "32" : - (((addr & 0x6) == 0x4) ? "64" : "reserved"), - (addr & 0x8) ? "" : "not "); - if ((addr & 0x6) == 0x4) { - /* The spec says that a 64-bit register consumes - * two subsequent dword locations. - */ - upperaddr = pci_read_long(dev, bar + 4); - if (upperaddr != 0x00000000) { - /* Fun! A real 64-bit resource. */ - if (sizeof(uintptr_t) != sizeof(uint64_t)) { - msg_perr("BAR unreachable!"); - /* TODO: Really abort here? If - * multiple PCI devices match, - * we might never tell the user - * about the other devices. - */ - return 0; - } - addr |= (uint64_t)upperaddr << 32; + msg_pdbg("Requested BAR is "); + switch (bartype) { + case TYPE_MEMBAR: + msg_pdbg("MEM"); + if (!(supported_cycles & PCI_COMMAND_MEMORY)) { + msg_perr("MEM BAR access requested, but device has MEM space accesses disabled.\n"); + /* TODO: Abort here? */ + } + msg_pdbg(", %sbit, %sprefetchable\n", + ((addr & 0x6) == 0x0) ? "32" : (((addr & 0x6) == 0x4) ? "64" : "reserved"), + (addr & 0x8) ? "" : "not "); + if ((addr & 0x6) == 0x4) { + /* The spec says that a 64-bit register consumes + * two subsequent dword locations. + */ + upperaddr = pci_read_long(dev, bar + 4); + if (upperaddr != 0x00000000) { + /* Fun! A real 64-bit resource. */ + if (sizeof(uintptr_t) != sizeof(uint64_t)) { + msg_perr("BAR unreachable!"); + /* TODO: Really abort here? If multiple PCI devices match, + * we might never tell the user about the other devices. + */ + return 0; } + addr |= (uint64_t)upperaddr << 32; } - addr &= PCI_BASE_ADDRESS_MEM_MASK; - break; - case TYPE_IOBAR: - msg_pdbg("I/O\n"); + } + addr &= PCI_BASE_ADDRESS_MEM_MASK; + break; + case TYPE_IOBAR: + msg_pdbg("I/O\n"); #if __FLASHROM_HAVE_OUTB__ - if (!(supported_cycles & PCI_COMMAND_IO)) { - msg_perr("I/O BAR access requested, but device " - "has I/O space accesses disabled.\n"); - /* TODO: Abort here? */ - } + if (!(supported_cycles & PCI_COMMAND_IO)) { + msg_perr("I/O BAR access requested, but device has I/O space accesses disabled.\n"); + /* TODO: Abort here? */ + } #else - msg_perr("I/O BAR access requested, but flashrom does " - "not support I/O BAR access on this platform " - "(yet).\n"); + msg_perr("I/O BAR access requested, but flashrom does not support I/O BAR access on this " + "platform (yet).\n"); #endif - addr &= PCI_BASE_ADDRESS_IO_MASK; - break; - case TYPE_ROMBAR: - msg_pdbg("ROM\n"); - /* Not sure if this check is needed. */ - if (!(supported_cycles & PCI_COMMAND_MEMORY)) { - msg_perr("MEM BAR access requested, but device " - "has MEM space accesses disabled.\n"); - /* TODO: Abort here? */ - } - addr &= PCI_ROM_ADDRESS_MASK; - break; - case TYPE_UNKNOWN: - msg_perr("BAR type unknown, please report a bug at " - "flashrom@flashrom.org\n"); + addr &= PCI_BASE_ADDRESS_IO_MASK; + break; + case TYPE_ROMBAR: + msg_pdbg("ROM\n"); + /* Not sure if this check is needed. */ + if (!(supported_cycles & PCI_COMMAND_MEMORY)) { + msg_perr("MEM BAR access requested, but device has MEM space accesses disabled.\n"); + /* TODO: Abort here? */ } - - if (devs[i].status == NT) { - msg_pinfo("===\nThis PCI device is UNTESTED. Please " - "report the 'flashrom -p xxxx' output \n" - "to flashrom@flashrom.org if it works " - "for you. Please add the name of your\n" - "PCI device to the subject. Thank you for " - "your help!\n===\n"); - } - - return (uintptr_t)addr; + addr &= PCI_ROM_ADDRESS_MASK; + break; + case TYPE_UNKNOWN: + msg_perr("BAR type unknown, please report a bug at flashrom@flashrom.org\n"); } - return 0; + return (uintptr_t)addr; } uintptr_t pcidev_init(int bar, const struct pcidev_status *devs) @@ -195,6 +160,7 @@ char *pcidev_bdf; char *msg = NULL; int found = 0; + int i; uintptr_t addr = 0, curaddr = 0; pacc = pci_alloc(); /* Get the pci_access structure */ @@ -214,10 +180,29 @@ for (dev = pacc->devices; dev; dev = dev->next) { if (pci_filter_match(&filter, dev)) { + /* Check against list of supported devices. */ + for (i = 0; devs[i].device_name != NULL; i++) + if ((dev->vendor_id == devs[i].vendor_id) && + (dev->device_id == devs[i].device_id)) + break; + /* Not supported, try the next one. */ + if (devs[i].device_name == NULL) + continue; + + msg_pdbg("Found \"%s %s\" (%04x:%04x, BDF %02x:%02x.%x).\n", devs[i].vendor_name, + devs[i].device_name, dev->vendor_id, dev->device_id, dev->bus, dev->dev, + dev->func); + if (devs[i].status == NT) + msg_pinfo("===\nThis PCI device is UNTESTED. Please report the 'flashrom -p " + "xxxx' output \n" + "to flashrom@flashrom.org if it works for you. Please add the name " + "of your\n" + "PCI device to the subject. Thank you for your help!\n===\n"); + /* FIXME: We should count all matching devices, not * just those with a valid BAR. */ - if ((addr = pcidev_validate(dev, bar, devs)) != 0) { + if ((addr = pcidev_readbar(dev, bar)) != 0) { curaddr = addr; pcidev_dev = dev; found++; @@ -230,10 +215,8 @@ msg_perr("Error: No supported PCI device found.\n"); exit(1); } else if (found > 1) { - msg_perr("Error: Multiple supported PCI devices found. " - "Use 'flashrom -p xxxx:pci=bb:dd.f' \n" - "to explicitly select the card with the given BDF " - "(PCI bus, device, function).\n"); + msg_perr("Error: Multiple supported PCI devices found. Use 'flashrom -p xxxx:pci=bb:dd.f' \n" + "to explicitly select the card with the given BDF (PCI bus, device, function).\n"); exit(1); } Index: flashrom-pcidev_check_vendorid_cleanup/nicintel.c =================================================================== --- flashrom-pcidev_check_vendorid_cleanup/nicintel.c (Revision 1547) +++ flashrom-pcidev_check_vendorid_cleanup/nicintel.c (Arbeitskopie) @@ -87,7 +87,7 @@ goto error_out_unmap; /* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */ - addr = pcidev_validate(pcidev_dev, PCI_BASE_ADDRESS_0, nics_intel); + addr = pcidev_readbar(pcidev_dev, PCI_BASE_ADDRESS_0); /* FIXME: This is not an aligned mapping. Use 4k? */ nicintel_control_bar = physmap("Intel NIC control/status reg", addr, NICINTEL_CONTROL_MEMMAP_SIZE); Index: flashrom-pcidev_check_vendorid_cleanup/programmer.h =================================================================== --- flashrom-pcidev_check_vendorid_cleanup/programmer.h (Revision 1547) +++ flashrom-pcidev_check_vendorid_cleanup/programmer.h (Arbeitskopie) @@ -227,7 +227,7 @@ const char *vendor_name; const char *device_name; }; -uintptr_t pcidev_validate(struct pci_dev *dev, int bar, const struct pcidev_status *devs); +uintptr_t pcidev_readbar(struct pci_dev *dev, int bar); uintptr_t pcidev_init(int bar, const struct pcidev_status *devs); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. -- http://www.hailfinger.org/

Am 14.07.2012 22:04 schrieb Michael Karcher:

Am Samstag, den 30.06.2012, 01:40 +0200 schrieb Carl-Daniel Hailfinger:

...

This patch is essentially a small code move, better device counting and lots of indentation changes (look at diff -uw to see real code changes). So basically you are moving the device test level check from pcidev_validate to pcidev_init. The result is that pcidev_validate does no longer in fact validate whether a candidate device is the correct device, as the name implies. A name like "pcidev_readbar" would make more sense, IMHO.

Now, check where pcidev_validate is called from. It is called from pcidev_init to do its original job, namely validating a device, and it is called from nicintel code to get a second BAR, because the first BAR returned by pcidev_init is not really enough. That invocation of pcidev_validate already carries a nice comment reading "/* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */". This indicates that something is wrong with this code. And in my oppinion, indeed, it is. The problem is that the current code contains an abstraction violation. Neither pcidev_validate (in the pre-patch form) is really meant to be called from higher level code, but it is meant to be called by pcidev_init only (doing it's job, validating a PCI device, poorly), nor is using a global variable pcidev_dev a good idea for communication between pcidev.c and the clients of that code.

Furthermore, pcidev_init (conveniently, I admit) does two things at once: It finds a PCI device, and it reads one BAR. What about having pcidev_init return the struct pci_dev* instead, so it is up to the client driver to handle that the value instead of putting it in a global variable? Of course, this implies all clients have to use something like pcidev_readbar (the post-patch pcidev_validate) function, even if just a single address is needed. Considering our current codebase, which uses pci_read_long to get base addresses in satamv and satasii, I think giving pcidev_readbar some extra exposure would even be a good educational thing.

And as I go along ranting about the current state of that code, I think it is nice that pcidev_validate does so many sanity checks, but there is one thing I do *not* like about it, you could call it a "missing vital sanity check". pcidev_validate checks whether the given BAR is I/O mapped or memory mapped, and handles each of those types correctly. Great! Finally, it returns the obtained base address, which might either be in I/O or memory space to the caller, *not* indicating whether it just returned an I/O or a memory address. This is quite "convenient", as the callers (should) know what type they expect, and they are not prepared to handle mismatches[1], but it would be even more convenient if this quite important verification of the address space would be performed. So pcidev_readbar (the new pcidev_validate) needs an extra parameter, which should indicate I/O or memory space (or, alternatively, provide pcidev_readbar_io and pcidev_readbar_mem, as there are some strict coding styles claiming the flag parameters are evil, because if there are two different ways a function can be executed (selected by that flag parameter), it really means there are two functions which should have their own name) and have it fail if the BAR type does not match the callers expectation.

Considering all the stuff I was writing, I think your current patch might be the first of a series leading towards a better pcidev interface, if you apply the name fix for pcidev_validate.

I guess the second step would be removing the global pcidev_dev variable, which implies all callers of pcidev_init need to be adjusted. Maybe for modules that really only need one BAR, we can still provide the old behaviour as "pcidev_init_getbar" or "pcidev_init_simple".

In a third step, add I/O vs. memory validation to pcidev_readbar (ex pcidev_validate), and remove all pci_read_long(...) &~7; invocations to read BARs.

Second patch in the series. Error checking has not changed at all, and I intend to fix that in a second spin of this patch. Right now I just want to check if we're on the same page. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Index: flashrom-pcidev_init_do_not_return_bar/ogp_spi.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/ogp_spi.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/ogp_spi.c (Arbeitskopie) @@ -105,6 +105,7 @@ int ogp_spi_init(void) { + struct pci_dev *dev = NULL; char *type; type = extract_programmer_param("rom"); @@ -130,7 +131,8 @@ get_io_perms(); - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, ogp_spi); + dev = pcidev_init(PCI_BASE_ADDRESS_0, ogp_spi); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); ogp_spibar = physmap("OGP registers", io_base_addr, 4096); Index: flashrom-pcidev_init_do_not_return_bar/drkaiser.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/drkaiser.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/drkaiser.c (Arbeitskopie) @@ -65,15 +65,16 @@ int drkaiser_init(void) { + struct pci_dev *dev = NULL; uint32_t addr; get_io_perms(); - addr = pcidev_init(PCI_BASE_ADDRESS_2, drkaiser_pcidev); + dev = pcidev_init(PCI_BASE_ADDRESS_2, drkaiser_pcidev); + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_2); /* Write magic register to enable flash write. */ - rpci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR, - PCI_MAGIC_DRKAISER_VALUE); + rpci_write_word(dev, PCI_MAGIC_DRKAISER_ADDR, PCI_MAGIC_DRKAISER_VALUE); /* Map 128kB flash memory window. */ drkaiser_bar = physmap("Dr. Kaiser PC-Waechter flash memory", Index: flashrom-pcidev_init_do_not_return_bar/pcidev.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/pcidev.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/pcidev.c (Arbeitskopie) @@ -26,7 +26,6 @@ uint32_t io_base_addr; struct pci_access *pacc; -struct pci_dev *pcidev_dev = NULL; enum pci_bartype { TYPE_MEMBAR, @@ -153,15 +152,16 @@ return (uintptr_t)addr; } -uintptr_t pcidev_init(int bar, const struct pcidev_status *devs) +struct pci_dev *pcidev_init(int bar, const struct pcidev_status *devs) { struct pci_dev *dev; + struct pci_dev *found_dev = NULL; struct pci_filter filter; char *pcidev_bdf; char *msg = NULL; int found = 0; int i; - uintptr_t addr = 0, curaddr = 0; + uintptr_t addr = 0; pacc = pci_alloc(); /* Get the pci_access structure */ pci_init(pacc); /* Initialize the PCI library */ @@ -203,8 +203,7 @@ * just those with a valid BAR. */ if ((addr = pcidev_readbar(dev, bar)) != 0) { - curaddr = addr; - pcidev_dev = dev; + found_dev = dev; found++; } } @@ -220,7 +219,7 @@ exit(1); } - return curaddr; + return found_dev; } void print_supported_pcidevs(const struct pcidev_status *devs) Index: flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c (Arbeitskopie) @@ -89,11 +89,13 @@ int gfxnvidia_init(void) { + struct pci_dev *dev = NULL; uint32_t reg32; get_io_perms(); - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, gfx_nvidia); + dev = pcidev_init(PCI_BASE_ADDRESS_0, gfx_nvidia); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); io_base_addr += 0x300000; msg_pinfo("Detected NVIDIA I/O base address: 0x%x.\n", io_base_addr); @@ -105,9 +107,9 @@ return 1; /* Allow access to flash interface (will disable screen). */ - reg32 = pci_read_long(pcidev_dev, 0x50); + reg32 = pci_read_long(dev, 0x50); reg32 &= ~(1 << 0); - rpci_write_long(pcidev_dev, 0x50, reg32); + rpci_write_long(dev, 0x50, reg32); /* Write/erase doesn't work. */ programmer_may_write = 0; Index: flashrom-pcidev_init_do_not_return_bar/nicrealtek.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicrealtek.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/nicrealtek.c (Arbeitskopie) @@ -61,9 +61,12 @@ int nicrealtek_init(void) { + struct pci_dev *dev = NULL; + get_io_perms(); - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_realtek); + dev = pcidev_init(PCI_BASE_ADDRESS_0, nics_realtek); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); if (register_shutdown(nicrealtek_shutdown, NULL)) return 1; Index: flashrom-pcidev_init_do_not_return_bar/satamv.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/satamv.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/satamv.c (Arbeitskopie) @@ -82,6 +82,7 @@ */ int satamv_init(void) { + struct pci_dev *dev = NULL; uintptr_t addr; uint32_t tmp; @@ -91,7 +92,8 @@ /* No need to check for errors, pcidev_init() will not return in case * of errors. */ - addr = pcidev_init(PCI_BASE_ADDRESS_0, satas_mv); + dev = pcidev_init(PCI_BASE_ADDRESS_0, satas_mv); + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); mv_bar = physmap("Marvell 88SX7042 registers", addr, 0x20000); if (mv_bar == ERROR_PTR) @@ -143,8 +145,7 @@ pci_rmmio_writel(tmp, mv_bar + GPIO_PORT_CONTROL); /* Get I/O BAR location. */ - tmp = pci_read_long(pcidev_dev, PCI_BASE_ADDRESS_2) & - PCI_BASE_ADDRESS_IO_MASK; + tmp = pcidev_readbar(dev, PCI_BASE_ADDRESS_2); /* Truncate to reachable range. * FIXME: Check if the I/O BAR is actually reachable. * This is an arch specific check. Index: flashrom-pcidev_init_do_not_return_bar/nicintel_spi.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicintel_spi.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/nicintel_spi.c (Arbeitskopie) @@ -165,11 +165,13 @@ int nicintel_spi_init(void) { + struct pci_dev *dev = NULL; uint32_t tmp; get_io_perms(); - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_intel_spi); + dev = pcidev_init(PCI_BASE_ADDRESS_0, nics_intel_spi); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); nicintel_spibar = physmap("Intel Gigabit NIC w/ SPI flash", io_base_addr, 4096); Index: flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c (Arbeitskopie) @@ -59,9 +59,12 @@ int nicnatsemi_init(void) { + struct pci_dev *dev = NULL; + get_io_perms(); - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_natsemi); + dev = pcidev_init(PCI_BASE_ADDRESS_0, nics_natsemi); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); if (register_shutdown(nicnatsemi_shutdown, NULL)) return 1; Index: flashrom-pcidev_init_do_not_return_bar/atahpt.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/atahpt.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/atahpt.c (Arbeitskopie) @@ -65,16 +65,18 @@ int atahpt_init(void) { + struct pci_dev *dev = NULL; uint32_t reg32; get_io_perms(); - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_4, ata_hpt); + dev = pcidev_init(PCI_BASE_ADDRESS_4, ata_hpt); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_4); /* Enable flash access. */ - reg32 = pci_read_long(pcidev_dev, REG_FLASH_ACCESS); + reg32 = pci_read_long(dev, REG_FLASH_ACCESS); reg32 |= (1 << 24); - rpci_write_long(pcidev_dev, REG_FLASH_ACCESS, reg32); + rpci_write_long(dev, REG_FLASH_ACCESS, reg32); if (register_shutdown(atahpt_shutdown, NULL)) return 1; Index: flashrom-pcidev_init_do_not_return_bar/nic3com.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nic3com.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/nic3com.c (Arbeitskopie) @@ -87,11 +87,14 @@ int nic3com_init(void) { + struct pci_dev *dev = NULL; + get_io_perms(); - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_3com); + dev = pcidev_init(PCI_BASE_ADDRESS_0, nics_3com); + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); - id = pcidev_dev->device_id; + id = dev->device_id; /* 3COM 3C90xB cards need a special fixup. */ if (id == 0x9055 || id == 0x9001 || id == 0x9004 || id == 0x9005 Index: flashrom-pcidev_init_do_not_return_bar/satasii.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/satasii.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/satasii.c (Arbeitskopie) @@ -67,20 +67,21 @@ int satasii_init(void) { + struct pci_dev *dev = NULL; uint32_t addr; uint16_t reg_offset; get_io_perms(); - pcidev_init(PCI_BASE_ADDRESS_0, satas_sii); + dev = pcidev_init(PCI_BASE_ADDRESS_0, satas_sii); - id = pcidev_dev->device_id; + id = dev->device_id; if ((id == 0x3132) || (id == 0x3124)) { - addr = pci_read_long(pcidev_dev, PCI_BASE_ADDRESS_0) & ~0x07; + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); reg_offset = 0x70; } else { - addr = pci_read_long(pcidev_dev, PCI_BASE_ADDRESS_5) & ~0x07; + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_5); reg_offset = 0x50; } Index: flashrom-pcidev_init_do_not_return_bar/nicintel.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicintel.c (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/nicintel.c (Arbeitskopie) @@ -69,6 +69,7 @@ int nicintel_init(void) { + struct pci_dev *dev = NULL; uintptr_t addr; /* Needed only for PCI accesses on some platforms. @@ -80,14 +81,14 @@ * of errors. * FIXME: BAR2 is not available if the device uses the CardBus function. */ - addr = pcidev_init(PCI_BASE_ADDRESS_2, nics_intel); + dev = pcidev_init(PCI_BASE_ADDRESS_2, nics_intel); + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_2); nicintel_bar = physmap("Intel NIC flash", addr, NICINTEL_MEMMAP_SIZE); if (nicintel_bar == ERROR_PTR) goto error_out_unmap; - /* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */ - addr = pcidev_readbar(pcidev_dev, PCI_BASE_ADDRESS_0); + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); /* FIXME: This is not an aligned mapping. Use 4k? */ nicintel_control_bar = physmap("Intel NIC control/status reg", addr, NICINTEL_CONTROL_MEMMAP_SIZE); Index: flashrom-pcidev_init_do_not_return_bar/programmer.h =================================================================== --- flashrom-pcidev_init_do_not_return_bar/programmer.h (Revision 1548) +++ flashrom-pcidev_init_do_not_return_bar/programmer.h (Arbeitskopie) @@ -219,7 +219,6 @@ // FIXME: These need to be local, not global extern uint32_t io_base_addr; extern struct pci_access *pacc; -extern struct pci_dev *pcidev_dev; struct pcidev_status { uint16_t vendor_id; uint16_t device_id; @@ -228,7 +227,7 @@ const char *device_name; }; uintptr_t pcidev_readbar(struct pci_dev *dev, int bar); -uintptr_t pcidev_init(int bar, const struct pcidev_status *devs); +struct pci_dev *pcidev_init(int bar, const struct pcidev_status *devs); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. */ -- http://www.hailfinger.org/

Am 01.01.2013 19:46 schrieb Carl-Daniel Hailfinger:

Am 04.09.2012 12:14 schrieb Stefan Tauner:

...

On Tue, 31 Jul 2012 09:27:47 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:

...

Ping?

IMHO this patch fixes a few structural problems, and although it probably isn't the final desired result, it is a step in the right direction. After my short adventure into trying to understand the PCI code while refining the atavia patch by Jonathan, i have to agree 100% with Michael's first mail regarding the old code base.

Something that hasn't been mentioned (or i have missed) is that there are some drivers that don't need a BAR at all because they work by accessing the configuration space only. atavia is one case (at least i think so). and satasii is even more special as it needs different BARs for different PCI IDs, but you are aware of that afaics.

Yes, there is still quite a bit of work ahead. That said, your PCI init cleanup patch had a tremendous effect on code readability. Thanks for creating that patch.

...

I still don't have a complete overview on how all pcidev_init callers work, but the current patch seems to improve things and hence should go in ASAP (you can read that as an ACK if you think it is safe).

This patch has changed quite a bit since then... it would be great if you could take another look.

...

Since you touch all pcidev_init calls in this patch, it would be great if you could switch the parameters though. The PCI dev table should be first since it is the most important argument and the bar should IMHO even be optional in the future or some kind of flag/mask as you discussed... that argument apparently confused not only me but also Jonathan and Idwer in the past.

Very good point. I have changed the argument order, and it really looks nicer and more consistent

...

A comment explaining the parameters would certainly improve the situation too (e.g. mention that the bar parameter is not the number of the BAR!).

Hm yes... to be honest, I want to get rid of the bar parameter in a followup patch, and supply a validator function instead. That one can handle everything we might ever need in that direction. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Index: flashrom-pcidev_init_do_not_return_bar/ogp_spi.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/ogp_spi.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/ogp_spi.c (Arbeitskopie) @@ -105,6 +105,7 @@ int ogp_spi_init(void) { + struct pci_dev *dev = NULL; char *type; type = extract_programmer_param("rom"); @@ -131,8 +132,11 @@ if (rget_io_perms()) return 1; - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, ogp_spi); + dev = pcidev_init(ogp_spi, PCI_BASE_ADDRESS_0); + if (!dev) + return 1; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); ogp_spibar = physmap("OGP registers", io_base_addr, 4096); if (register_shutdown(ogp_spi_shutdown, NULL)) Index: flashrom-pcidev_init_do_not_return_bar/drkaiser.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/drkaiser.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/drkaiser.c (Arbeitskopie) @@ -64,17 +64,20 @@ int drkaiser_init(void) { + struct pci_dev *dev = NULL; uint32_t addr; if (rget_io_perms()) return 1; - /* No need to check for errors, pcidev_init() will not return in case of errors. */ - addr = pcidev_init(PCI_BASE_ADDRESS_2, drkaiser_pcidev); + dev = pcidev_init(drkaiser_pcidev, PCI_BASE_ADDRESS_2); + if (!dev) + return 1; + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_2); + /* Write magic register to enable flash write. */ - rpci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR, - PCI_MAGIC_DRKAISER_VALUE); + rpci_write_word(dev, PCI_MAGIC_DRKAISER_ADDR, PCI_MAGIC_DRKAISER_VALUE); /* Map 128kB flash memory window. */ drkaiser_bar = physmap("Dr. Kaiser PC-Waechter flash memory", Index: flashrom-pcidev_init_do_not_return_bar/pcidev.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/pcidev.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/pcidev.c (Arbeitskopie) @@ -27,7 +27,6 @@ uint32_t io_base_addr; struct pci_access *pacc; -struct pci_dev *pcidev_dev = NULL; enum pci_bartype { TYPE_MEMBAR, @@ -156,7 +155,6 @@ static int pcidev_shutdown(void *data) { - pcidev_dev = NULL; if (pacc == NULL) { msg_perr("%s: Tried to cleanup an invalid PCI context!\n" "Please report a bug at flashrom@flashrom.org\n", __func__); @@ -181,18 +179,24 @@ return 0; } -uintptr_t pcidev_init(int bar, const struct dev_entry *devs) +/* pcidev_init gets an array of allowed PCI device IDs and returns a pointer to struct pci_dev iff exactly one + * match was found. If the "pci=bb:dd.f" programmer parameter was specified, a match is only considered if it + * also matches the specified bus:device.function. + * For convenience, this function also registers its own undo handlers. + */ +struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar) { struct pci_dev *dev; + struct pci_dev *found_dev = NULL; struct pci_filter filter; char *pcidev_bdf; char *msg = NULL; int found = 0; int i; - uintptr_t addr = 0, curaddr = 0; + uintptr_t addr = 0; - if(pci_init_common() != 0) - return 1; + if (pci_init_common() != 0) + return NULL; pci_filter_init(pacc, &filter); /* Filter by bb:dd.f (if supplied by the user). */ @@ -200,7 +204,7 @@ if (pcidev_bdf != NULL) { if ((msg = pci_filter_parse_slot(&filter, pcidev_bdf))) { msg_perr("Error: %s\n", msg); - exit(1); + return NULL; } } free(pcidev_bdf); @@ -230,8 +234,7 @@ * just those with a valid BAR. */ if ((addr = pcidev_readbar(dev, bar)) != 0) { - curaddr = addr; - pcidev_dev = dev; + found_dev = dev; found++; } } @@ -240,14 +243,14 @@ /* Only continue if exactly one supported PCI dev has been found. */ if (found == 0) { msg_perr("Error: No supported PCI device found.\n"); - exit(1); + return NULL; } else if (found > 1) { msg_perr("Error: Multiple supported PCI devices found. Use 'flashrom -p xxxx:pci=bb:dd.f' \n" "to explicitly select the card with the given BDF (PCI bus, device, function).\n"); - exit(1); + return NULL; } - return curaddr; + return found_dev; } enum pci_write_type { Index: flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/gfxnvidia.c (Arbeitskopie) @@ -85,14 +85,17 @@ int gfxnvidia_init(void) { + struct pci_dev *dev = NULL; uint32_t reg32; if (rget_io_perms()) return 1; - /* No need to check for errors, pcidev_init() will not return in case of errors. */ - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, gfx_nvidia); + dev = pcidev_init(gfx_nvidia, PCI_BASE_ADDRESS_0); + if (!dev) + return 1; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); io_base_addr += 0x300000; msg_pinfo("Detected NVIDIA I/O base address: 0x%x.\n", io_base_addr); @@ -102,9 +105,9 @@ return 1; /* Allow access to flash interface (will disable screen). */ - reg32 = pci_read_long(pcidev_dev, 0x50); + reg32 = pci_read_long(dev, 0x50); reg32 &= ~(1 << 0); - rpci_write_long(pcidev_dev, 0x50, reg32); + rpci_write_long(dev, 0x50, reg32); /* Write/erase doesn't work. */ programmer_may_write = 0; Index: flashrom-pcidev_init_do_not_return_bar/nicrealtek.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicrealtek.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/nicrealtek.c (Arbeitskopie) @@ -59,16 +59,19 @@ int nicrealtek_init(void) { + struct pci_dev *dev = NULL; + if (rget_io_perms()) return 1; - /* No need to check for errors, pcidev_init() will not return in case of errors. */ - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_realtek); - if (register_shutdown(nicrealtek_shutdown, NULL)) + dev = pcidev_init(nics_realtek, PCI_BASE_ADDRESS_0); + if (!dev) return 1; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); + /* Beware, this ignores the vendor ID! */ - switch (pcidev_dev->device_id) { + switch (dev->device_id) { case 0x8139: /* RTL8139 */ case 0x1211: /* SMC 1211TX */ default: @@ -81,6 +84,9 @@ break; } + if (register_shutdown(nicrealtek_shutdown, NULL)) + return 1; + register_par_programmer(&par_programmer_nicrealtek, BUS_PARALLEL); return 0; Index: flashrom-pcidev_init_do_not_return_bar/satamv.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/satamv.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/satamv.c (Arbeitskopie) @@ -81,6 +81,7 @@ */ int satamv_init(void) { + struct pci_dev *dev = NULL; uintptr_t addr; uint32_t tmp; @@ -88,11 +89,11 @@ return 1; /* BAR0 has all internal registers memory mapped. */ - /* No need to check for errors, pcidev_init() will not return in case - * of errors. - */ - addr = pcidev_init(PCI_BASE_ADDRESS_0, satas_mv); + dev = pcidev_init(satas_mv, PCI_BASE_ADDRESS_0); + if (!dev) + return 1; + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); mv_bar = physmap("Marvell 88SX7042 registers", addr, 0x20000); if (mv_bar == ERROR_PTR) return 1; @@ -143,8 +144,7 @@ pci_rmmio_writel(tmp, mv_bar + GPIO_PORT_CONTROL); /* Get I/O BAR location. */ - tmp = pci_read_long(pcidev_dev, PCI_BASE_ADDRESS_2) & - PCI_BASE_ADDRESS_IO_MASK; + tmp = pcidev_readbar(dev, PCI_BASE_ADDRESS_2); /* Truncate to reachable range. * FIXME: Check if the I/O BAR is actually reachable. * This is an arch specific check. Index: flashrom-pcidev_init_do_not_return_bar/nicintel_spi.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicintel_spi.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/nicintel_spi.c (Arbeitskopie) @@ -166,14 +166,17 @@ int nicintel_spi_init(void) { + struct pci_dev *dev = NULL; uint32_t tmp; if (rget_io_perms()) return 1; - /* No need to check for errors, pcidev_init() will not return in case of errors. */ - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_intel_spi); + dev = pcidev_init(nics_intel_spi, PCI_BASE_ADDRESS_0); + if (!dev) + return 1; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); nicintel_spibar = physmap("Intel Gigabit NIC w/ SPI flash", io_base_addr, MEMMAP_SIZE); /* Automatic restore of EECD on shutdown is not possible because EECD Index: flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/nicnatsemi.c (Arbeitskopie) @@ -60,11 +60,17 @@ int nicnatsemi_init(void) { + struct pci_dev *dev = NULL; + if (rget_io_perms()) return 1; - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_natsemi); + dev = pcidev_init(nics_natsemi, PCI_BASE_ADDRESS_0); + if (!dev) + return 1; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); + if (register_shutdown(nicnatsemi_shutdown, NULL)) return 1; Index: flashrom-pcidev_init_do_not_return_bar/atahpt.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/atahpt.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/atahpt.c (Arbeitskopie) @@ -58,17 +58,22 @@ int atahpt_init(void) { + struct pci_dev *dev = NULL; uint32_t reg32; if (rget_io_perms()) return 1; - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_4, ata_hpt); + dev = pcidev_init(ata_hpt, PCI_BASE_ADDRESS_4); + if (!dev) + return 1; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_4); + /* Enable flash access. */ - reg32 = pci_read_long(pcidev_dev, REG_FLASH_ACCESS); + reg32 = pci_read_long(dev, REG_FLASH_ACCESS); reg32 |= (1 << 24); - rpci_write_long(pcidev_dev, REG_FLASH_ACCESS, reg32); + rpci_write_long(dev, REG_FLASH_ACCESS, reg32); register_par_programmer(&par_programmer_atahpt, BUS_PARALLEL); Index: flashrom-pcidev_init_do_not_return_bar/nic3com.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nic3com.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/nic3com.c (Arbeitskopie) @@ -86,14 +86,19 @@ int nic3com_init(void) { + struct pci_dev *dev = NULL; + if (rget_io_perms()) return 1; - /* No need to check for errors, pcidev_init() will not return in case of errors. */ - io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, nics_3com); + dev = pcidev_init(nics_3com, PCI_BASE_ADDRESS_0); + if (!dev) + return 1; - id = pcidev_dev->device_id; + io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); + id = dev->device_id; + /* 3COM 3C90xB cards need a special fixup. */ if (id == 0x9055 || id == 0x9001 || id == 0x9004 || id == 0x9005 || id == 0x9006 || id == 0x900a || id == 0x905a || id == 0x9058) { Index: flashrom-pcidev_init_do_not_return_bar/satasii.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/satasii.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/satasii.c (Arbeitskopie) @@ -76,21 +76,24 @@ int satasii_init(void) { + struct pci_dev *dev = NULL; uint32_t addr; uint16_t reg_offset; if (rget_io_perms()) return 1; - pcidev_init(PCI_BASE_ADDRESS_0, satas_sii); + dev = pcidev_init(satas_sii, PCI_BASE_ADDRESS_0); + if (!dev) + return 1; - id = pcidev_dev->device_id; + id = dev->device_id; if ((id == 0x3132) || (id == 0x3124)) { - addr = pci_read_long(pcidev_dev, PCI_BASE_ADDRESS_0) & ~0x07; + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); reg_offset = 0x70; } else { - addr = pci_read_long(pcidev_dev, PCI_BASE_ADDRESS_5) & ~0x07; + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_5); reg_offset = 0x50; } Index: flashrom-pcidev_init_do_not_return_bar/nicintel.c =================================================================== --- flashrom-pcidev_init_do_not_return_bar/nicintel.c (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/nicintel.c (Arbeitskopie) @@ -68,6 +68,7 @@ int nicintel_init(void) { + struct pci_dev *dev = NULL; uintptr_t addr; /* Needed only for PCI accesses on some platforms. @@ -76,17 +77,17 @@ if (rget_io_perms()) return 1; - /* No need to check for errors, pcidev_init() will not return in case of errors. - * FIXME: BAR2 is not available if the device uses the CardBus function. - */ - addr = pcidev_init(PCI_BASE_ADDRESS_2, nics_intel); + /* FIXME: BAR2 is not available if the device uses the CardBus function. */ + dev = pcidev_init(nics_intel, PCI_BASE_ADDRESS_2); + if (!dev) + return 1; + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_2); nicintel_bar = physmap("Intel NIC flash", addr, NICINTEL_MEMMAP_SIZE); if (nicintel_bar == ERROR_PTR) goto error_out_unmap; - /* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */ - addr = pcidev_readbar(pcidev_dev, PCI_BASE_ADDRESS_0); + addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); /* FIXME: This is not an aligned mapping. Use 4k? */ nicintel_control_bar = physmap("Intel NIC control/status reg", addr, NICINTEL_CONTROL_MEMMAP_SIZE); Index: flashrom-pcidev_init_do_not_return_bar/programmer.h =================================================================== --- flashrom-pcidev_init_do_not_return_bar/programmer.h (Revision 1643) +++ flashrom-pcidev_init_do_not_return_bar/programmer.h (Arbeitskopie) @@ -237,10 +237,9 @@ // FIXME: These need to be local, not global extern uint32_t io_base_addr; extern struct pci_access *pacc; -extern struct pci_dev *pcidev_dev; int pci_init_common(void); uintptr_t pcidev_readbar(struct pci_dev *dev, int bar); -uintptr_t pcidev_init(int bar, const struct dev_entry *devs); +struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. */ -- http://www.hailfinger.org/

Michael? This patch implements your "callback solution" suggestion. Comments welcome!

Am 22.07.2012 04:39 schrieb Carl-Daniel Hailfinger:

...

Am 21.07.2012 16:02 schrieb Michael Karcher:

...

Am Dienstag, den 17.07.2012, 09:17 +0200 schrieb Carl-Daniel Hailfinger:

...

if ((addr = pcidev_readbar(dev, bar)) != 0) { - curaddr = addr; - pcidev_dev = dev; + found_dev = dev; found++; }

I wonder what to do about this: While the goal of my suggestion was to decouple BAR access from the PCI device scan, it is an integral part of the scan loop, probably to avoid disabled chips (e.g. for onboard components that are not used). Intel dual port NICs are represented by two PCI devices, but only one of them has an active BAR for our purposes. It would be unfair of us to have users guess the right PCI device.

...

The nice property of the code as-is is that you can be sure reading the "primary" BAR will not fail.

As we sometimes need two BARs, having one valid BAR does not mean the device is necessarily usable for us, so this check is only half of what we need. As already discussed on IRC, passing a set of BARs into this function is not really the direction we want to head to, so client code needs to be prepared to find unusable BARs anyway. Still, we like autoskip. Several ideas come to my mind - Use the PCI command word for autoskip How would that work?

I still haven't figured that out.

...

...

- Implement a set-of-BAR (bitmask, array) anyway Depending on the PCI header type, BARs can be at completely different locations. Still, set-of-BAR sounds like a good idea.

That doesn't work for satasii which uses different BARs for different PCI IDs.

...

...

- Probe that all BARs (except ROM perhaps) that are not unused contain sensible values How do we determine which values are sensible?

That doesn't work either for external programmers which need more than one BAR, and one of those BARs may be disabled by default (i.e. unusable).

...

...

- Hand in a "is_usable" callback (a standard callback for testing a single BAR could be provided) Do you really trust new driver authors to get that right?

I think this one is the only viable solution, and with my automatic driver writer script (and plenty of good code to copy from) I think that there is a good chance new driver authors will get it right. I have named the callback "validator" instead of "is_usable", but I have no preference for either name.

...

...

All of these approaches of course complicate pcidev_init, but it seems like the only choices we have is: - do the BAR check the right way (TM) - lose the autoskip of disabled PCI devices

Am 04.09.2012 12:14 schrieb Stefan Tauner:

Something that hasn't been mentioned (or i have missed) is that there are some drivers that don't need a BAR at all because they work by accessing the configuration space only. atavia is one case (at least i think so). and satasii is even more special as it needs different BARs for different PCI IDs, but you are aware of that afaics.

Yes, this is now taken care of.

A comment explaining the parameters would certainly improve the situation too (e.g. mention that the bar parameter is not the number of the BAR!).

Do you feel the comment level is adequate now or should I add some more stuff? Changelog: Change pcidev_init() to take a validator function as parameter instead of a BAR specifier. This allows for code like atavia (which doesn't even use a BAR) and satasii (which uses different BARs depending on PCI ID). NOTE: satamv is not finished, and won't compile. No other known problems. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Index: flashrom-pcidev_init_validator_function/ogp_spi.c =================================================================== --- flashrom-pcidev_init_validator_function/ogp_spi.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/ogp_spi.c (Arbeitskopie) @@ -103,6 +103,12 @@ return 0; } +static int ogp_spi_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); +} + int ogp_spi_init(void) { struct pci_dev *dev = NULL; @@ -132,7 +138,7 @@ if (rget_io_perms()) return 1; - dev = pcidev_init(ogp_spi, PCI_BASE_ADDRESS_0); + dev = pcidev_init(ogp_spi, ogp_spi_validator); if (!dev) return 1; Index: flashrom-pcidev_init_validator_function/drkaiser.c =================================================================== --- flashrom-pcidev_init_validator_function/drkaiser.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/drkaiser.c (Arbeitskopie) @@ -62,6 +62,12 @@ return 0; } +static int drkaiser_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_2); +} + int drkaiser_init(void) { struct pci_dev *dev = NULL; @@ -70,7 +76,7 @@ if (rget_io_perms()) return 1; - dev = pcidev_init(drkaiser_pcidev, PCI_BASE_ADDRESS_2); + dev = pcidev_init(drkaiser_pcidev, drkaiser_validator); if (!dev) return 1; Index: flashrom-pcidev_init_validator_function/pcidev.c =================================================================== --- flashrom-pcidev_init_validator_function/pcidev.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/pcidev.c (Arbeitskopie) @@ -184,7 +184,7 @@ * also matches the specified bus:device.function. * For convenience, this function also registers its own undo handlers. */ -struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar) +struct pci_dev *pcidev_init(const struct dev_entry *devs, int (*validator) (struct pci_dev *dev)) { struct pci_dev *dev; struct pci_dev *found_dev = NULL; @@ -193,7 +193,6 @@ char *msg = NULL; int found = 0; int i; - uintptr_t addr = 0; if (pci_init_common() != 0) return NULL; @@ -233,7 +232,7 @@ /* FIXME: We should count all matching devices, not * just those with a valid BAR. */ - if ((addr = pcidev_readbar(dev, bar)) != 0) { + if (!(*validator)(dev)) { found_dev = dev; found++; } Index: flashrom-pcidev_init_validator_function/gfxnvidia.c =================================================================== --- flashrom-pcidev_init_validator_function/gfxnvidia.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/gfxnvidia.c (Arbeitskopie) @@ -83,6 +83,12 @@ return 0; } +static int gfxnvidia_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); +} + int gfxnvidia_init(void) { struct pci_dev *dev = NULL; @@ -91,7 +97,7 @@ if (rget_io_perms()) return 1; - dev = pcidev_init(gfx_nvidia, PCI_BASE_ADDRESS_0); + dev = pcidev_init(gfx_nvidia, gfxnvidia_validator); if (!dev) return 1; Index: flashrom-pcidev_init_validator_function/nicrealtek.c =================================================================== --- flashrom-pcidev_init_validator_function/nicrealtek.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/nicrealtek.c (Arbeitskopie) @@ -57,6 +57,12 @@ return 0; } +static int nicrealtek_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); +} + int nicrealtek_init(void) { struct pci_dev *dev = NULL; @@ -64,7 +70,7 @@ if (rget_io_perms()) return 1; - dev = pcidev_init(nics_realtek, PCI_BASE_ADDRESS_0); + dev = pcidev_init(nics_realtek, nicrealtek_validator); if (!dev) return 1; Index: flashrom-pcidev_init_validator_function/satamv.c =================================================================== --- flashrom-pcidev_init_validator_function/satamv.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/satamv.c (Arbeitskopie) @@ -63,6 +63,13 @@ return 0; } +static int satamv_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ +#error Check multiple BARs! + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_); +} + /* * Random notes: * FCE# Flash Chip Enable @@ -89,7 +96,7 @@ return 1; /* BAR0 has all internal registers memory mapped. */ - dev = pcidev_init(satas_mv, PCI_BASE_ADDRESS_0); + dev = pcidev_init(satas_mv, satamv_validator); if (!dev) return 1; Index: flashrom-pcidev_init_validator_function/nicintel_spi.c =================================================================== --- flashrom-pcidev_init_validator_function/nicintel_spi.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/nicintel_spi.c (Arbeitskopie) @@ -164,6 +164,12 @@ return 0; } +static int nicintel_spi_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); +} + int nicintel_spi_init(void) { struct pci_dev *dev = NULL; @@ -172,7 +178,7 @@ if (rget_io_perms()) return 1; - dev = pcidev_init(nics_intel_spi, PCI_BASE_ADDRESS_0); + dev = pcidev_init(nics_intel_spi, nicintel_spi_validator); if (!dev) return 1; Index: flashrom-pcidev_init_validator_function/nicnatsemi.c =================================================================== --- flashrom-pcidev_init_validator_function/nicnatsemi.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/nicnatsemi.c (Arbeitskopie) @@ -52,6 +52,12 @@ .chip_writen = fallback_chip_writen, }; +static int nicnatsemi_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); +} + int nicnatsemi_init(void) { struct pci_dev *dev = NULL; @@ -59,7 +65,7 @@ if (rget_io_perms()) return 1; - dev = pcidev_init(nics_natsemi, PCI_BASE_ADDRESS_0); + dev = pcidev_init(nics_natsemi, nicnatsemi_validator); if (!dev) return 1; Index: flashrom-pcidev_init_validator_function/atahpt.c =================================================================== --- flashrom-pcidev_init_validator_function/atahpt.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/atahpt.c (Arbeitskopie) @@ -56,6 +56,12 @@ .chip_writen = fallback_chip_writen, }; +static int atahpt_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_4); +} + int atahpt_init(void) { struct pci_dev *dev = NULL; @@ -64,7 +70,7 @@ if (rget_io_perms()) return 1; - dev = pcidev_init(ata_hpt, PCI_BASE_ADDRESS_4); + dev = pcidev_init(ata_hpt, atahpt_validator); if (!dev) return 1; Index: flashrom-pcidev_init_validator_function/nic3com.c =================================================================== --- flashrom-pcidev_init_validator_function/nic3com.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/nic3com.c (Arbeitskopie) @@ -84,6 +84,12 @@ return 0; } +static int nic3com_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); +} + int nic3com_init(void) { struct pci_dev *dev = NULL; @@ -91,7 +97,7 @@ if (rget_io_perms()) return 1; - dev = pcidev_init(nics_3com, PCI_BASE_ADDRESS_0); + dev = pcidev_init(nics_3com, nic3com_validator); if (!dev) return 1; Index: flashrom-pcidev_init_validator_function/satasii.c =================================================================== --- flashrom-pcidev_init_validator_function/satasii.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/satasii.c (Arbeitskopie) @@ -74,6 +74,19 @@ return ctrl_reg; } +static int satasii_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BAR. */ + switch (dev->device_id) { + case 0x3132: + case 0x3124: + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0); + break; + default: + return !pcidev_readbar(dev, PCI_BASE_ADDRESS_5); + } +} + int satasii_init(void) { struct pci_dev *dev = NULL; @@ -83,16 +96,19 @@ if (rget_io_perms()) return 1; - dev = pcidev_init(satas_sii, PCI_BASE_ADDRESS_0); + dev = pcidev_init(satas_sii, satasii_validator); if (!dev) return 1; id = dev->device_id; - if ((id == 0x3132) || (id == 0x3124)) { + switch (id) { + case 0x3132: + case 0x3124: addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); reg_offset = 0x70; - } else { + break; + default: addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_5); reg_offset = 0x50; } Index: flashrom-pcidev_init_validator_function/nicintel.c =================================================================== --- flashrom-pcidev_init_validator_function/nicintel.c (Revision 1644) +++ flashrom-pcidev_init_validator_function/nicintel.c (Arbeitskopie) @@ -66,6 +66,12 @@ return 0; } +static int nicintel_validator(struct pci_dev *dev) +{ + /* Return 0 if we have a valid BARs. */ + return !(pcidev_readbar(dev, PCI_BASE_ADDRESS_0) && pcidev_readbar(dev, PCI_BASE_ADDRESS_2)); +} + int nicintel_init(void) { struct pci_dev *dev = NULL; @@ -78,7 +84,7 @@ return 1; /* FIXME: BAR2 is not available if the device uses the CardBus function. */ - dev = pcidev_init(nics_intel, PCI_BASE_ADDRESS_2); + dev = pcidev_init(nics_intel, nicintel_validator); if (!dev) return 1; Index: flashrom-pcidev_init_validator_function/programmer.h =================================================================== --- flashrom-pcidev_init_validator_function/programmer.h (Revision 1644) +++ flashrom-pcidev_init_validator_function/programmer.h (Arbeitskopie) @@ -169,7 +169,7 @@ extern struct pci_access *pacc; int pci_init_common(void); uintptr_t pcidev_readbar(struct pci_dev *dev, int bar); -struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar); +struct pci_dev *pcidev_init(const struct dev_entry *devs, int (*validator) (struct pci_dev *dev)); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. */ -- http://www.hailfinger.org/