Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Peter Marheine.

Patch set 2:Code-Review +1

View Change

5 comments:

• Commit Message:

  • Patch Set #2, 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:

  • Patch Set #2:

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

• File internal.c:

  • Patch Set #2, 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;

  • Patch Set #2, 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:

  • Patch Set #2, 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;
     }

To view, visit change 59275. To unsubscribe, or for help writing mail filters, visit settings.