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:
(6 comments)
As the file name is `cse_lite.c`, I’d remove `cse_` from the static function names.
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");
I'd say this is clear enough as is. […]
How can you assume, that it returns or continues? You can not, so it’s better to state it explicitly.
How would currently the error level messages look like? Will several messages be printed for the same issue?
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 473: prerequisites Mentioning the prerequisites would be useful.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 555: cse_fix_data_failure_err
Let's try... […]
Yes, the invalid check should be moved out.
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))
Well, one can't fix things when they aren't broken.
Yes, the caller should take care of that.
https://review.coreboot.org/c/coreboot/+/40561/22/src/soc/intel/common/block... PS22, Line 592: cse_is_fw_downgrade
Suggestions?
I couldn’t come up with a good one yet. But two variables are passed, so from the name, the ordering of both is not clear to me for example.
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");
I'd use something like this: […]
And the user asks: “Why and what now?”.