Change in flashrom[master]: Revert "pcidev.c: Factor out pcidev_validate() into pure fn"
Hello Edward O'Callaghan, Arthur Heymans, build bot (Jenkins), Patrick Georgi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/flashrom/+/38319
to review the following change.
Change subject: Revert "pcidev.c: Factor out pcidev_validate() into pure fn" ......................................................................
Revert "pcidev.c: Factor out pcidev_validate() into pure fn"
This reverts commit e28d75ed7204d7fac2c0fac13978098530b0574e.
This is broken in multiple ways, e.g. pcidev_init() can only return NULL.
Change-Id: I06242147ba9d3a062d442f645eb0800ef51af19f Signed-off-by: Nico Huber nico.h@gmx.de --- M pcidev.c 1 file changed, 24 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/19/38319/1
diff --git a/pcidev.c b/pcidev.c index 4012840..54c1fd3 100644 --- a/pcidev.c +++ b/pcidev.c @@ -148,33 +148,6 @@ return (uintptr_t)addr; }
-static uintptr_t pcidev_validate(struct pci_dev *dev, int bar, const struct dev_entry *devs) -{ - unsigned i; - - /* Check against list of supported devices. */ - for (i = 0; devs[i].device_name != NULL; i++) { - if (dev->device_id != devs[i].device_id) - continue; - - msg_pinfo("Found "%s %s" (%04x:%04x, BDF %02x:%02x.%x).\n", - devs[i].vendor_name, devs[i].device_name, - dev->vendor_id, dev->device_id, dev->bus, dev->dev, - dev->func); - - if (devs[i].status == NT) - msg_pinfo("===\nThis PCI device is UNTESTED. Please report the 'flashrom -p " - "xxxx' output\n" - "to flashrom@flashrom.org if it works for you. Please add the name " - "of your\n" - "PCI device to the subject. Thank you for your help!\n===\n"); - - - return pcidev_readbar(dev, bar); - } - return 0; -} - static int pcidev_shutdown(void *data) { if (pacc == NULL) { @@ -210,10 +183,12 @@ struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar) { 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;
if (pci_init_common() != 0) @@ -232,10 +207,30 @@
for (dev = pacc->devices; dev; dev = dev->next) { if (pci_filter_match(&filter, dev)) { + /* Check against list of supported devices. */ + for (i = 0; devs[i].device_name != NULL; i++) + if ((dev->vendor_id == devs[i].vendor_id) && + (dev->device_id == devs[i].device_id)) + break; + /* Not supported, try the next one. */ + if (devs[i].device_name == NULL) + continue; + + msg_pdbg("Found "%s %s" (%04x:%04x, BDF %02x:%02x.%x).\n", devs[i].vendor_name, + devs[i].device_name, dev->vendor_id, dev->device_id, dev->bus, dev->dev, + dev->func); + if (devs[i].status == NT) + msg_pinfo("===\nThis PCI device is UNTESTED. Please report the 'flashrom -p " + "xxxx' output\n" + "to flashrom@flashrom.org if it works for you. Please add the name " + "of your\n" + "PCI device to the subject. Thank you for your help!\n===\n"); + /* FIXME: We should count all matching devices, not * just those with a valid BAR. */ - if ((addr = pcidev_validate(dev, bar, devs)) != 0) { + if ((addr = pcidev_readbar(dev, bar)) != 0) { + found_dev = dev; found++; } } @@ -251,7 +246,7 @@ return NULL; }
- return dev; + return found_dev; }
enum pci_write_type {
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38319 )
Change subject: Revert "pcidev.c: Factor out pcidev_validate() into pure fn" ......................................................................
Patch Set 1: Code-Review+2
Carl-Daniel Hailfinger has uploaded a new patch set (#2) to the change originally created by Nico Huber. ( https://review.coreboot.org/c/flashrom/+/38319 )
Change subject: Revert "pcidev.c: Factor out pcidev_validate() into pure fn" ......................................................................
Revert "pcidev.c: Factor out pcidev_validate() into pure fn"
This reverts commit e28d75ed7204d7fac2c0fac13978098530b0574e.
This is broken in multiple ways, e.g. pcidev_init() can only return NULL.
Change-Id: I06242147ba9d3a062d442f645eb0800ef51af19f Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net --- M pcidev.c 1 file changed, 24 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/19/38319/2
Carl-Daniel Hailfinger has uploaded a new patch set (#3) to the change originally created by Nico Huber. ( https://review.coreboot.org/c/flashrom/+/38319 )
Change subject: Revert "pcidev.c: Factor out pcidev_validate() into pure fn" ......................................................................
Revert "pcidev.c: Factor out pcidev_validate() into pure fn"
This reverts commit e28d75ed7204d7fac2c0fac13978098530b0574e.
This is broken in multiple ways, e.g. pcidev_init() can only return NULL.
Change-Id: I06242147ba9d3a062d442f645eb0800ef51af19f Signed-off-by: Nico Huber nico.h@gmx.de Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Reported-by: Michael Bishop cleverca22@gmail.com --- M pcidev.c 1 file changed, 24 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/19/38319/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38319 )
Change subject: Revert "pcidev.c: Factor out pcidev_validate() into pure fn" ......................................................................
Patch Set 3: Verified+1
Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/38319 )
Change subject: Revert "pcidev.c: Factor out pcidev_validate() into pure fn" ......................................................................
Revert "pcidev.c: Factor out pcidev_validate() into pure fn"
This reverts commit e28d75ed7204d7fac2c0fac13978098530b0574e.
This is broken in multiple ways, e.g. pcidev_init() can only return NULL.
Change-Id: I06242147ba9d3a062d442f645eb0800ef51af19f Signed-off-by: Nico Huber nico.h@gmx.de Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Reported-by: Michael Bishop cleverca22@gmail.com Reviewed-on: https://review.coreboot.org/c/flashrom/+/38319 Reviewed-by: Angel Pons th3fanbus@gmail.com --- M pcidev.c 1 file changed, 24 insertions(+), 29 deletions(-)
Approvals: Nico Huber: Verified Angel Pons: Looks good to me, approved
diff --git a/pcidev.c b/pcidev.c index 4012840..54c1fd3 100644 --- a/pcidev.c +++ b/pcidev.c @@ -148,33 +148,6 @@ return (uintptr_t)addr; }
-static uintptr_t pcidev_validate(struct pci_dev *dev, int bar, const struct dev_entry *devs) -{ - unsigned i; - - /* Check against list of supported devices. */ - for (i = 0; devs[i].device_name != NULL; i++) { - if (dev->device_id != devs[i].device_id) - continue; - - msg_pinfo("Found "%s %s" (%04x:%04x, BDF %02x:%02x.%x).\n", - devs[i].vendor_name, devs[i].device_name, - dev->vendor_id, dev->device_id, dev->bus, dev->dev, - dev->func); - - if (devs[i].status == NT) - msg_pinfo("===\nThis PCI device is UNTESTED. Please report the 'flashrom -p " - "xxxx' output\n" - "to flashrom@flashrom.org if it works for you. Please add the name " - "of your\n" - "PCI device to the subject. Thank you for your help!\n===\n"); - - - return pcidev_readbar(dev, bar); - } - return 0; -} - static int pcidev_shutdown(void *data) { if (pacc == NULL) { @@ -210,10 +183,12 @@ struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar) { 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;
if (pci_init_common() != 0) @@ -232,10 +207,30 @@
for (dev = pacc->devices; dev; dev = dev->next) { if (pci_filter_match(&filter, dev)) { + /* Check against list of supported devices. */ + for (i = 0; devs[i].device_name != NULL; i++) + if ((dev->vendor_id == devs[i].vendor_id) && + (dev->device_id == devs[i].device_id)) + break; + /* Not supported, try the next one. */ + if (devs[i].device_name == NULL) + continue; + + msg_pdbg("Found "%s %s" (%04x:%04x, BDF %02x:%02x.%x).\n", devs[i].vendor_name, + devs[i].device_name, dev->vendor_id, dev->device_id, dev->bus, dev->dev, + dev->func); + if (devs[i].status == NT) + msg_pinfo("===\nThis PCI device is UNTESTED. Please report the 'flashrom -p " + "xxxx' output\n" + "to flashrom@flashrom.org if it works for you. Please add the name " + "of your\n" + "PCI device to the subject. Thank you for your help!\n===\n"); + /* FIXME: We should count all matching devices, not * just those with a valid BAR. */ - if ((addr = pcidev_validate(dev, bar, devs)) != 0) { + if ((addr = pcidev_readbar(dev, bar)) != 0) { + found_dev = dev; found++; } } @@ -251,7 +246,7 @@ return NULL; }
- return dev; + return found_dev; }
enum pci_write_type {
-
Angel Pons (Code Review) -
Carl-Daniel Hailfinger (Code Review)
-
Nico Huber (Code Review)