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. */