Sridhar Siricilla 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 23:
(13 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).
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@13 PS22, Line 13: indicate
indicates
Ack
https://review.coreboot.org/c/coreboot/+/40561/22//COMMIT_MSG@13 PS22, Line 13: the
a
Done
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");
How can you assume, that it returns or continues? You can not, so it’s better to state it explicitly […]
If the command fails, coreboot logs FW status registers and triggers ChromeOS recovery. The FW status registers help decode CSE's current working state(CWS) and current operation mode(COM).Also, the log trail indicates CSE's current boot partition. So, I think current error log seems to be ok.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 473: prerequisites
Mentioning the prerequisites would be useful.
Please see my response above.
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");
As this is an "is" function, I'd prefer if it just returned a boolean and nothing else. […]
Ack
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.
Ack
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 555: cse_fix_data_failure_err
Yes, the invalid check should be moved out.
I wanted to keep top level function abstracted, so I moved the if condition inside the function. I have added documentation to give more clarity. Let me know your comments.
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))
Yes, the caller should take care of that.
Please see my response above
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.
I have added comment to provide clarity.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 592: cse_is_fw_downgrade
I couldn’t come up with a good one yet. […]
Ack
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");
And the user asks: “Why and what now?”.
The chain of log message indicate CSE FW update is downgrade (one version to other) and abort of CSE FW downgrade action due to data clear command if it fails. So, I think "CSE FW downgrade is aborted" is sufficient.