Angel Pons 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:
(7 comments)
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. […]
I'd say this is clear enough as is. The DATA CLEAR command has some prerequisites, and if they are not met, it can't be executed. And what happens next? Nothing special, the command didn't run. The caller could try again (and probably get the same error), skip this, or give up.
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.
As this is an "is" function, I'd prefer if it just returned a boolean and nothing else. This lets us have a single return statement:
return rw_bp->status != BP_STATUS_DATA_FAILURE;
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 555: bool What does the returned value indicate? It's not obvious at first glance.
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?
Let's try... Ew, `cse_clear_data_if_invalid_then_boot_to_rw()` might be accurate, but it's a horrible 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.
Well, one can't fix things when they aren't broken.
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?
Suggestions?
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 …. […]
I'd use something like this:
cse_lite: Could not perform CSE FW downgrade