Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Implement reading from CBMEM console ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/c/coreboot/+/37667/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37667/2//COMMIT_MSG@7 PS2, Line 7: libpayload: Read from cbmem console
Implement reading from CBMEM console
Done
https://review.coreboot.org/c/coreboot/+/37667/2//COMMIT_MSG@11 PS2, Line 11: that
Could be removed.
Done
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/cbmem_console.c:
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 85: if (!console_p) { : printf("ERROR: No cbmem console found in coreboot table.\n"); : return; : } :
should we still set *buffer_p to NULL or *count to 0? especially there's no return value
Done
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 99: enough
also print the expected size.
Done
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 100: return
should we let caller know we've failed getting logs?
Done
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 107: corrupt
corrupted […]
How about treating this the same as other errors?