[flashrom] Improving pcidev.c

[Michael Karcher]

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