Nice work! Sorry to add more. You are almost there.
8 comments:
File src/soc/amd/picasso/Makefile.inc:
Patch Set #60, Line 288: --whitelist
It looks like we lost ability to add whitelist option. We will need to check CONFIG_HAVE_PSP_WHITELIST_FILE and add this option if it is set.
OPT_TOKEN_UNLOCK=$(call add_opt_prefix, $(PSP_TOKEN_UNLOCK), "")
OPT_PSP_USE_PSPSECUREOS=$(call add_opt_prefix, $(PSP_USE_PSPSECUREOS), "")
OPT_PSP_LOAD_MP2_FW=$(call add_opt_prefix, $(PSP_LOAD_MP2_FW), "")
OPT_PSP_LOAD_S0I3_FW=$(call add_opt_prefix, $(PSP_LOAD_S0I3_FW), "")
Do we really need these anymore? We are not really trying to concatenate two strings as we used to do before. There is only one string now PSP_*. It seems like we could set corresponding OPT_PSP_* in the lines above when we are checking CONFIG options and be done with it.
File util/amdfwtool/amdfwtool.c:
Patch Set #60, Line 1464: process_config(config_handle, &cb_config);
We should be checking the return status here if the config file is parsed correctly and failing the build if not.
File util/amdfwtool/data_parse.c:
We really should be returning status and failing the build if we can't parse a non-blank/non-comment line in configuration file correctly.
Patch Set #60, Line 61: fw_type = AMD_FW_INVALID;
It may make sense to init fw_type = AMD_FW_INVALID at the top and then set fw_type to a new type here e.g. AMD_FW_SKIP to differentiate between failing/non-parsing and simply non-applicable options. If you split them into two functions as I am suggesting below you can simply return here with a non-failing status.
Patch Set #60, Line 68: fw_type = AMD_FW_INVALID;
same as above
Patch Set #60, Line 262: fw_type = AMD_BIOS_INVALID;
It may make sense to split find_register_fw_filename() into two functions e.g. find_register_fw_filename_psp_dir() and void find_register_fw_filename_bios_dir(). If both of them return "no match" we should fail the build with a message that current line can not be parsed.
if (!regexec(&blank_or_comment_expr, oneline, 0, NULL, 0)) {
/* skip comment and blank */
retval = 1;
}
This does not seem to do much. Even if it detects blank line it will keep trying to process it below. I can see it doing something useful if you added "return" or "else" here.
To view, visit change 42859. To unsubscribe, or for help writing mail filters, visit settings.