Nikolai Vyssotski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42859 )
Change subject: amdfwtool: Take a config file instead of command line parameters ......................................................................
Patch Set 60:
(8 comments)
Nice work! Sorry to add more. You are almost there.
https://review.coreboot.org/c/coreboot/+/42859/60/src/soc/amd/picasso/Makefi... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/60/src/soc/amd/picasso/Makefi... PS60, 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.
https://review.coreboot.org/c/coreboot/+/42859/60/src/soc/amd/picasso/Makefi... PS60, Line 227: 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.
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/amdfwtool.c... PS60, 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.
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/data_parse.... File util/amdfwtool/data_parse.c:
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/data_parse.... PS60, Line 48: void 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.
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/data_parse.... PS60, 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.
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/data_parse.... PS60, Line 68: fw_type = AMD_FW_INVALID; same as above
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/data_parse.... PS60, 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.
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/data_parse.... PS60, Line 306: 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.