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