Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Read from cbmem console ......................................................................
Patch Set 2:
(6 comments)
I'm not very convinced if this must be inside libpayload, since it'll need to copy whole buffer; maybe the payload can be more efficient on reading only the contents of one page, and scroll/update on demand.
But if you think this is really the right thing to do, I think it's no harm to have an extra API in libpayload.
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 79: size_t uint32_t ?
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 79: cbmem_console_read I'll probably call this cbmem_console_readback
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
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 99: enough also print the expected size.
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?
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 107: corrupt corrupted
should we just insert this into beginning of the output buffer? since the print here won't be noticed by users