Change in flashrom[master]: pcidev.c: Factor out pcidev_validate() into pure fn

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/37264 )

Change subject: pcidev.c: Factor out pcidev_validate() into pure fn
......................................................................

pcidev.c: Factor out pcidev_validate() into pure fn

This makes writing unit-tests easier.

Change-Id: Ia2718f1f40851d3122741cd0e50b0c2b647b727a
Signed-off-by: Edward O'Callaghan 
---
M pcidev.c
1 file changed, 29 insertions(+), 24 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/37264/1

diff --git a/pcidev.c b/pcidev.c
index 54c1fd3..4012840 100644
--- a/pcidev.c
+++ b/pcidev.c
@@ -148,6 +148,33 @@
    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 [e-mail] 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) {
@@ -183,12 +210,10 @@
 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)
@@ -207,30 +232,10 @@

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 [e-mail] 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_readbar(dev, bar)) != 0) {
-				found_dev = dev;
+			if ((addr = pcidev_validate(dev, bar, devs)) != 0) {
    			found++;
    		}
    	}
@@ -246,7 +251,7 @@
    	return NULL;
    }

-	return found_dev;
+	return dev;
 }

enum pci_write_type {

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37264 )

Change subject: pcidev.c: Factor out pcidev_validate() into pure fn
......................................................................

Patch Set 1: Code-Review+2

[Edward O'Callaghan (Code Review)]

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

Change subject: pcidev.c: Factor out pcidev_validate() into pure fn
......................................................................

pcidev.c: Factor out pcidev_validate() into pure fn

This makes writing unit-tests easier.

Change-Id: Ia2718f1f40851d3122741cd0e50b0c2b647b727a
Signed-off-by: Edward O'Callaghan 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/37264
Tested-by: build bot (Jenkins) 
Reviewed-by: Arthur Heymans 
---
M pcidev.c
1 file changed, 29 insertions(+), 24 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Arthur Heymans: Looks good to me, approved

diff --git a/pcidev.c b/pcidev.c
index 54c1fd3..4012840 100644
--- a/pcidev.c
+++ b/pcidev.c
@@ -148,6 +148,33 @@
    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 [e-mail] 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) {
@@ -183,12 +210,10 @@
 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)
@@ -207,30 +232,10 @@

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 [e-mail] 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_readbar(dev, bar)) != 0) {
-				found_dev = dev;
+			if ((addr = pcidev_validate(dev, bar, devs)) != 0) {
    			found++;
    		}
    	}
@@ -246,7 +251,7 @@
    	return NULL;
    }

-	return found_dev;
+	return dev;
 }

enum pci_write_type {

[Nico Huber (Code Review)]

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

Change subject: pcidev.c: Factor out pcidev_validate() into pure fn
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/flashrom/+/37264/2/pcidev.c 
File pcidev.c:

https://review.coreboot.org/c/flashrom/+/37264/2/pcidev.c@158 
PS2, Line 158: 			continue;
What happened to the vendor_id check?

https://review.coreboot.org/c/flashrom/+/37264/2/pcidev.c@254 
PS2, Line 254: 	return dev;
So we always return NULL now (only state that can exit the loop)? Was this tested? ;-P

[Nico Huber (Code Review)]

Nico Huber has created a revert of this change. ( https://review.coreboot.org/c/flashrom/+/37264 )

Change subject: pcidev.c: Factor out pcidev_validate() into pure fn
......................................................................