Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40561 )
Change subject: soc/intel/common: Add downgrade support for CSE Firmware ......................................................................
Patch Set 22:
(11 comments)
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@10 PS22, Line 10: When CSE FW is downgraded, CSE may get into data compatibility issues. Please add a blank line above (between paragraphs).
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@12 PS22, Line 12: on preventive basis Isn’t *To avoid such issues* already conveying this?
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@13 PS22, Line 13: the a
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@13 PS22, Line 13: indicate indicates
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 473: printk(BIOS_ERR, "cse_lite: CSE doesn't meet DATA CLEAR cmd prerequisites\n"); Error messages have to be user understandable. Please elaborate, what the consequence is.
… CSE data is not cleared, and this causes …
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 548: printk(BIOS_ERR, "cse_lite: RW data partition is not valid\n"); The error messages should be moved to the caller, so the consequences can be added to the message.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 555: cse_fix_data_failure_err What error is fixed? Isn’t `heci_clear_data()` or something similar a better name?
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 557: if (cse_is_rw_dp_valid(cse_bp_info)) Please move the check to the caller, as from the function name, it does not belong here.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 561: return false; The error message should be printed here, and not in the function.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 592: cse_is_fw_downgrade Can you please improve the name?
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 661: printk(BIOS_ERR, "cse_lite: CSE FW downgrade is aborted\n");
CSE data couldn’t be cleared, so abort CSE FW downgrade from … to …. Please check … and try again.