Julius Werner 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 5:
(9 comments)
LGTM in general but I don't think you can just continue in all those cases where an allocation failed. I think you should just use fatal() everywhere.
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
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. Another fatal() would probably be better.
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.
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)
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ohci... PS5, Line 454: if (final_td) { Shouldn't continue without this. (If you want to fail gracefully, the correct behavior would be to roll the whole function back and return an error code, but OOM errors shouldn't really happen in practice anyway so I think a fatal() is also good enough.)
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 string).
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ohci... PS5, Line 581: break; Again can't just break here
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ohci... PS5, Line 710: continue; ...and here
https://review.coreboot.org/#/c/32083/5/payloads/libpayload/drivers/usb/ohci... PS5, Line 733: } ...and here