[flashrom-gerrit] Change in flashrom[master]: pcidev: Avoid internal relying on pacc global

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Peter Marheine.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59275 )

Change subject: pcidev: Avoid internal relying on pacc global
......................................................................

Patch Set 2: Code-Review+1

(5 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/59275/comment/99eb0bbd_60202630 
PS2, Line 7: Avoid internal relying on pacc global
`internal relying` should either be `internal reliance` or `internally relying` to be grammatically correct.

I don't like the concept of `internal` (what's internal and what's external?), so I'd instead say:

pcidev: Confine some uses of global variable `pacc`

Patchset:

PS2: 
I like the idea, but please avoid changing the behavior of the code when refactoring.

File internal.c:

https://review.coreboot.org/c/flashrom/+/59275/comment/9adaf0eb_1a7fbddf 
PS2, Line 41: temp = pcidev_scandev(&filter);
            : 	if (temp) {
            : 		/* Read PCI class */
            : 		tmp2 = pci_read_word(temp, 0x0a);
            : 		if (tmp2 == devclass)
            : 			return temp;
            : 	}
The original code would continue walking the linked list of devices if `tmp2 == devclass` is false. The current code returns NULL. This changes behavior when the linked list contains multiple devices that match the provided filter, but `tmp2 == devclass` is false for at least the first one.

After modifying `pcidev_scandev()` accordingly, I'd rewrite this part as follows to preserve the original behavior:

for (temp = pcidev_scandev(&filter, NULL); temp; temp = pcidev_scandev(&filter, temp)) {
    if (pci_filter_match(&filter, temp)) {
    	/* Read PCI class */
    	tmp2 = pci_read_word(temp, 0x0a);
    	if (tmp2 == devclass)
    		return temp;
    }
 }

return NULL;

https://review.coreboot.org/c/flashrom/+/59275/comment/1351f0cc_ab8393ef 
PS2, Line 73: temp = pcidev_scandev(&filter);
            : 	if (temp) {
            : 		if ((card_vendor == pci_read_word(temp, PCI_SUBSYSTEM_VENDOR_ID))
            : 		    && (card_device == pci_read_word(temp, PCI_SUBSYSTEM_ID)))
            : 			return temp;
            : 	}
Same situation as in `pci_dev_find_vendorclass()`, see comment above.

File pcidev.c:

https://review.coreboot.org/c/flashrom/+/59275/comment/6abf8142_882db4e0 
PS2, Line 151: struct pci_dev *pcidev_scandev(struct pci_filter *filter)
             : {
             : 	struct pci_dev *temp;
             : 	for (temp = pacc->devices; temp; temp = temp->next)
             : 		if (pci_filter_match(filter, temp))
             : 			return temp;
             : 	return NULL;
             : }
To allow resuming searches (see `internal.c` comments), I'd modify this function as follows:

struct pci_dev *pcidev_scandev(struct pci_filter *filter, struct pci_dev *start)
 {
    struct pci_dev *temp;
    for (temp = start ? start : pacc->devices; temp; temp = temp->next)
    	if (pci_filter_match(filter, temp))
    		return temp;
    return NULL;
 }