7 comments:
File src/soc/intel/common/block/cse/cse_lite.c:
Patch Set #22, 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.
Patch Set #22, 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;
What does the returned value indicate? It's not obvious at first glance.
Patch Set #22, 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.
Patch Set #22, 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.
Patch Set #22, Line 592: cse_is_fw_downgrade
Can you please improve the name?
Suggestions?
Patch Set #22, 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
To view, visit change 40561. To unsubscribe, or for help writing mail filters, visit settings.