Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32083 )
Change subject: payload/libpayload: This is a fix for potential NULL pointer dereferences ......................................................................
Patch Set 7:
(15 comments)
Patch Set 5:
(26 comments)
Gentlemen,
Would you please review the following patch?
Regards, Francois
Julius,
Your comments have been addressed.
Regards, Francois
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ehci... File payloads/libpayload/drivers/usb/ehci.c:
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ehci... PS5, Line 638: memory
nit: clarify to "DMA memory"? this may come from a different allocation pool than the main heap
Done
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ehci... PS5, Line 651: continue;
I don't think continue is appropriate here? I think this kills the EHCI stack. […]
Done
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ehci... PS5, Line 837: if (EHCI_INST(controller)->dummy_qh) {
Same here, I don't think it makes sense to continue if the allocation failed.
Done
https://review.coreboot.org/#/c/32083/6/payloads/libpayload/drivers/usb/ehci... File payloads/libpayload/drivers/usb/ehci.c:
https://review.coreboot.org/#/c/32083/6/payloads/libpayload/drivers/usb/ehci... PS6, Line 647: if (!next_td)
trailing whitespace
Done
https://review.coreboot.org/#/c/32083/6/payloads/libpayload/drivers/usb/ehci... PS6, Line 648: fatal("Not enough DMA memory to create #reqcount TD.\n");
line over 80 characters
Ack
https://review.coreboot.org/#/c/32083/6/payloads/libpayload/drivers/usb/ehci... PS6, Line 833: if (!EHCI_INST(controller)->dummy_qh)
trailing whitespace
Done
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ohci... File payloads/libpayload/drivers/usb/ohci.c:
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ohci... PS5, Line 216: hcca
nit: HCCA in all caps (short for Host Controller <something> Area)
Done
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ohci... PS5, Line 454: if (final_td) {
Shouldn't continue without this. […]
Done
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ohci... PS5, Line 540: "first TD of bulk transfer.\n");
Please don't break strings (it's permissible to go beyond 80 characters if necessary to not break a […]
Done
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ohci... PS5, Line 581: break;
Again can't just break here
Done
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ohci... PS5, Line 710: continue;
... […]
Done
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ohci... PS5, Line 733: }
... […]
Done
https://review.coreboot.org/#/c/32083/6/payloads/libpayload/drivers/usb/ohci... File payloads/libpayload/drivers/usb/ohci.c:
https://review.coreboot.org/#/c/32083/6/payloads/libpayload/drivers/usb/ohci... PS6, Line 577: fatal("Not enough DMA memory for OHCI addition TD in bulk transfer.\n");
line over 80 characters
Ack
https://review.coreboot.org/#/c/32083/6/payloads/libpayload/drivers/usb/ohci... PS6, Line 703: fatal("Not enough memory to add transfer descriptor.\n");
line over 80 characters
Ack
https://review.coreboot.org/#/c/32083/7/payloads/libpayload/drivers/usb/ohci... File payloads/libpayload/drivers/usb/ohci.c:
https://review.coreboot.org/#/c/32083/7/payloads/libpayload/drivers/usb/ohci... PS7, Line 703: fatal("Not enough memory to add transfer descriptor.\n");
line over 80 characters
Ack