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 <[email protected]> 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 <[email protected]> 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 " - "[email protected]\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 [email protected] 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 [email protected]\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 [email protected] 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/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
