Attention is currently required from: Anil Kumar K, Bora Guvendik, Jamie Ryu, Jeremy Compostella, Jérémy Compostella, Subrata Banik.
Wonkyu Kim has posted comments on this change by Wonkyu Kim. ( https://review.coreboot.org/c/coreboot/+/84229?usp=email )
Change subject: libpayload: add more condition to check valid PCI device id ......................................................................
Patch Set 3:
(5 comments)
File payloads/libpayload/drivers/usb/usbinit.c:
https://review.coreboot.org/c/coreboot/+/84229/comment/547604e1_326c412c?usp... : PS3, Line 126: /* Check if there's a device here at all. */ : if (pci_read_config32(pci_device, REG_VENDOR_ID) == 0xffffffff)
please specify the device where you have ran into this issue. […]
This is checking bad hardware to avoid hang issue. And current condition is not even checking 0x0 VID.
I think it's better to check valid both VID and DID and not enumerate the PCI ID.
https://review.coreboot.org/c/coreboot/+/84229/comment/6406b33b_3c9cc0ac?usp... : PS3, Line 119: int bus, int dev, int fun
Why can't it take a `pcidev_t` ?
We can use it as parameter.
https://review.coreboot.org/c/coreboot/+/84229/comment/0f4b995d_03f29fad?usp... : PS3, Line 130: (did == 0x0000) || (did == 0xffff)) {
It should fit in one line.
Acknowledged
https://review.coreboot.org/c/coreboot/+/84229/comment/b25f4a4b_f0982022?usp... : PS3, Line 141:
why ?
Acknowledged
https://review.coreboot.org/c/coreboot/+/84229/comment/df58fcb2_6f1ce982?usp... : PS3, Line 147: if ( is_valid_pci_dev(bus, dev, 0) == false)
The implementation is other parts of the lib is as follow: […]
This is also limited checking for VID 0x0 and 0xFFFFFF.