Patchwork Improving pcidev.c

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2013-01-06 21:13:09
Message ID <50E9E8E5.6060103@gmx.net>
Download mbox | patch
Permalink /patch/3847/
State New
Headers show

Comments

Carl-Daniel Hailfinger - 2013-01-06 21:13:09
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>
Michael Karcher - 2013-01-06 23:14:34
Hello Carl-Daniel,

all-in-all, your patch looks good. Still I have some comments to that
mail

Am Sonntag, den 06.01.2013, 22:13 +0100 schrieb Carl-Daniel Hailfinger:

> >>>  - Use the PCI command word for autoskip
> >> How would that work?
> I still haven't figured that out.
If a PCI device is completely disabled (and thus not eligible as
flasher), I expected the PCI command word to not enable I/O or memory
decoding while it is needed for flashing.

> >>>  - 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.
That could be solved by moving the bitmask into the pcidev_status
structure. Which means the mask can be different for each ID.

> >>>  - 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).
If things like that really happen, this does not work. I expected a
device to either get ressources assigned by the BIOS or PCI core of the
OS or none at all.

> >>>  - Hand in a "is_usable" callback (a standard callback for testing a
> >>> single BAR could be provided)
A standard pcidev_validator_bar0 in pcidev.c still might make sense to
avoid reimplement it in every second programmer driver.

> >> 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.
validator also sounds nice. go with it.

> +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);
> +}
> +
this would be pcidev_validator_bar0 (or pcidev_validate_bar0, depending
on whether you want a nominal clause or an action as function name).

> -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))
Function declarations including function pointer types are quite
difficult to read in my oppinion. What about
"typedef int (*pcidev_validator)(struct pci_dev *dev)"?

> -			if ((addr = pcidev_readbar(dev, bar)) != 0) {
> +			if (!(*validator)(dev)) {
The star and the parenthesis are not needed in C. "if (!validator(dev))"
would do as well. Furthermore, we don't use this explicit indirection
syntax for all those function pointers in structures.

> +static int gfxnvidia_validator(struct pci_dev *dev)
> +{
> +	/* Return 0 if we have a valid BAR. */
> +	return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
> +}
> +
A second instance of pcidev_validator_bar0

> +static int nicrealtek_validator(struct pci_dev *dev)
> +{
> +	/* Return 0 if we have a valid BAR. */
> +	return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
> +}
> +
A third one

> +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);
> +}
> +
A fourth one

> +static int nicnatsemi_validator(struct pci_dev *dev)
> +{
> +	/* Return 0 if we have a valid BAR. */
> +	return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
> +}
> +
Another one, no sense in counting them anymore ;)

> +static int nic3com_validator(struct pci_dev *dev)
> +{
> +	/* Return 0 if we have a valid BAR. */
> +	return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
> +}
> +
Well, yes.

> -	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)) {
Well, this works for sure, but couldn't we somehow associate this info
into the pci device ID list, instead of repeating some IDs here? Of
course this would also mean that pcidev_init needs to return a pointer
to the matched pcidev_status entry. Possibly not worth the hassle.


> +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. */
Maybe you want to move the FIXME comment into the validator declaration.


> -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));
typedef, as above.


Regards,
  Michael Karcher

Patch

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