Change in flashrom[master]: pcidev.c: populate IDs with pci_fill_info()

[Daniel Verkamp (Code Review)]

Daniel Verkamp has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/46310 )

Change subject: pcidev.c: populate IDs with pci_fill_info()
......................................................................

pcidev.c: populate IDs with pci_fill_info()

With pciutils 3.7.0, flashrom is unable to match any PCI devices by
vendor/device ID because the vendor_id and device_id fields of struct
pci_dev are not filled in.

Call pci_fill_info() to request these identifiers before trying to match
them against the supported device list.

With this change, reading/writing a nicintel_spi boot ROM is successful.

Signed-off-by: Daniel Verkamp 
Change-Id: Ia011d4d801f8a54160e45a70b14b740e6dcc00ef
---
M pcidev.c
1 file changed, 1 insertion(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/10/46310/1

diff --git a/pcidev.c b/pcidev.c
index e13b78c..892f7b1 100644
--- a/pcidev.c
+++ b/pcidev.c
@@ -206,6 +206,7 @@

for (dev = pacc->devices; dev; dev = dev->next) {
    	if (pci_filter_match(&filter, dev)) {
+			pci_fill_info(dev, PCI_FILL_IDENT);
    		/* Check against list of supported devices. */
    		for (i = 0; devs[i].device_name != NULL; i++)
    			if ((dev->vendor_id == devs[i].vendor_id) &&

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46310 )

Change subject: pcidev.c: populate IDs with pci_fill_info()
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/flashrom/+/46310/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/46310/1//COMMIT_MSG@10 
PS1, Line 10: because the vendor_id and device_id fields of struct
            : pci_dev are not filled in
Huh? Why did this behavior change?

[Daniel Verkamp (Code Review)]

Daniel Verkamp has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46310 )

Change subject: pcidev.c: populate IDs with pci_fill_info()
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/flashrom/+/46310/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/46310/1//COMMIT_MSG@10 
PS1, Line 10: because the vendor_id and device_id fields of struct
            : pci_dev are not filled in
I did not dig into the history to find the exact pciutils change, but the ChangeLog entry for 3.7.0 (from 2020-05-1) says:
> Semantics of pci_fill_info() and pci_dev->known_fields was underspecified,
> which lead to inconsistencies between back-ends. Improved documentation
> to give a more precise definition and updated all back-ends to conform
> to it. Most importantly, pci_dev->known_fields shows all fields requested
> over the lifetime of the pci_dev, but never those which are not supported
> by the back-end.

The pciutils library API is almost undocumented, but I believe the pci_fill_info() call was always intended to be required; I verified that it has been around since at least a release from 1999, and it is used in example.c in pciutils.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46310 )

Change subject: pcidev.c: populate IDs with pci_fill_info()
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/flashrom/+/46310/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/46310/1//COMMIT_MSG@10 
PS1, Line 10: because the vendor_id and device_id fields of struct
            : pci_dev are not filled in
> I did not dig into the history to find the exact pciutils change, but the ChangeLog entry for 3.7. […]

Alright, I see. Could you please add this in the commit message? Thanks.

[Daniel Verkamp (Code Review)]

Hello build bot (Jenkins), Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/46310

to look at the new patch set (#2).

Change subject: pcidev.c: populate IDs with pci_fill_info()
......................................................................

pcidev.c: populate IDs with pci_fill_info()

With pciutils 3.7.0, flashrom is unable to match any PCI devices by
vendor/device ID because the vendor_id and device_id fields of struct
pci_dev are not filled in.

Call pci_fill_info() to request these identifiers before trying to match
them against the supported device list.

The pciutils ChangeLog for 3.7.0 mentions that the documentation and
back-end behavior for pci_fill_info() was updated; it seems that a call
to pci_fill_info() was always intended to be required, but some backends
(such as the sysfs one used on Linux) would fill the identifier fields
even when not requested by the user.  The pci_fill_info() function and
the PCI_FILL_IDENT flag have been available for all versions of pciutils
since at least 2.0 from 1999, so it should be safe to add without any
version checks.

With this change, reading/writing a nicintel_spi boot ROM is successful.

Signed-off-by: Daniel Verkamp 
Change-Id: Ia011d4d801f8a54160e45a70b14b740e6dcc00ef
---
M pcidev.c
1 file changed, 1 insertion(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/10/46310/2

[Daniel Verkamp (Code Review)]

Daniel Verkamp has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46310 )

Change subject: pcidev.c: populate IDs with pci_fill_info()
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/46310/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/46310/1//COMMIT_MSG@10 
PS1, Line 10: because the vendor_id and device_id fields of struct
            : pci_dev are not filled in
> Alright, I see. Could you please add this in the commit message? Thanks.

Sure, I added some more details to the commit message - does this look OK?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46310 )

Change subject: pcidev.c: populate IDs with pci_fill_info()
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/flashrom/+/46310/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/46310/1//COMMIT_MSG@10 
PS1, Line 10: because the vendor_id and device_id fields of struct
            : pci_dev are not filled in
> Sure, I added some more details to the commit message - does this look OK?

LGTM

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46310 )

Change subject: pcidev.c: populate IDs with pci_fill_info()
......................................................................

Patch Set 2: Code-Review+2

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46310 )

Change subject: pcidev.c: populate IDs with pci_fill_info()
......................................................................

Patch Set 2: Code-Review+2

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/46310 )

Change subject: pcidev.c: populate IDs with pci_fill_info()
......................................................................

pcidev.c: populate IDs with pci_fill_info()

With pciutils 3.7.0, flashrom is unable to match any PCI devices by
vendor/device ID because the vendor_id and device_id fields of struct
pci_dev are not filled in.

Call pci_fill_info() to request these identifiers before trying to match
them against the supported device list.

The pciutils ChangeLog for 3.7.0 mentions that the documentation and
back-end behavior for pci_fill_info() was updated; it seems that a call
to pci_fill_info() was always intended to be required, but some backends
(such as the sysfs one used on Linux) would fill the identifier fields
even when not requested by the user.  The pci_fill_info() function and
the PCI_FILL_IDENT flag have been available for all versions of pciutils
since at least 2.0 from 1999, so it should be safe to add without any
version checks.

With this change, reading/writing a nicintel_spi boot ROM is successful.

Signed-off-by: Daniel Verkamp 
Change-Id: Ia011d4d801f8a54160e45a70b14b740e6dcc00ef
Reviewed-on: https://review.coreboot.org/c/flashrom/+/46310
Reviewed-by: Angel Pons 
Reviewed-by: Nico Huber 
Reviewed-by: Edward O'Callaghan 
Tested-by: build bot (Jenkins) 
---
M pcidev.c
1 file changed, 1 insertion(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Angel Pons: Looks good to me, approved
  Edward O'Callaghan: Looks good to me, approved

diff --git a/pcidev.c b/pcidev.c
index e13b78c..892f7b1 100644
--- a/pcidev.c
+++ b/pcidev.c
@@ -206,6 +206,7 @@

for (dev = pacc->devices; dev; dev = dev->next) {
    	if (pci_filter_match(&filter, dev)) {
+			pci_fill_info(dev, PCI_FILL_IDENT);
    		/* Check against list of supported devices. */
    		for (i = 0; devs[i].device_name != NULL; i++)
    			if ((dev->vendor_id == devs[i].vendor_id) &&