Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40621 )
Change subject: drivers/ipmi: Add function read_data_string() to make code cleaner ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/40621/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40621/2//COMMIT_MSG@7 PS2, Line 7: Added
Add
Done
https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c File src/drivers/ipmi/ipmi_fru.c:
https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... PS2, Line 96: length of the string or 0 when it's failed. */
Concise multi-line comments are meant for comments in code blocks. Please use: […]
Done
https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... PS2, Line 97: uint8_t *data_ptr
Can it be const?
Done
https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... PS2, Line 103: printk(BIOS_ERR, "%s:%d - failed due to length is zero\n", __func__, __LINE__);
In my opinion error log messages should be more elaborate. […]
Changed to DEBUG.
https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... PS2, Line 159: &info->manufacturer);
Shouldn’t you check if length is 0?
The check is done in read_data_string(), it would return 0 length if any error or there's no such data, and the code after that can work correctly.