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