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.