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 63:
(5 comments)
Nice work! Jest a few more. Almost there.
https://review.coreboot.org/c/coreboot/+/42859/63/src/soc/amd/picasso/Makefi... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/63/src/soc/amd/picasso/Makefi... PS63, Line 265: $(AMDFWTOOL) \ Sorry, missed that one on previous round. We nee to add configuration file as a dependency now with $(call strip_quotes,$(CONFIG_AMDFW_CONFIG_FILE)) \
https://review.coreboot.org/c/coreboot/+/42859/63/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42859/63/util/amdfwtool/amdfwtool.c... PS63, Line 1468: "Config file is parsed error: %s\n", This is not clear/proper English. Something like this would be better: fprintf(stderr, "Configuration file %s parsing error\n", config).
https://review.coreboot.org/c/coreboot/+/42859/63/util/amdfwtool/amdfwtool.c... PS63, Line 1469: strerror(errno)) Calling strerror() here would be bad as proces_config() returns 0 for failure and 1 for success. Thus when it fails it would actually print "Success". We can drop it since proces_config() returns just binary values or we need to fix up return values to be able to use strerror().
https://review.coreboot.org/c/coreboot/+/42859/63/util/amdfwtool/data_parse.... File util/amdfwtool/data_parse.c:
https://review.coreboot.org/c/coreboot/+/42859/63/util/amdfwtool/data_parse.... PS63, Line 208: fw_type = AMD_FW_PSP_WHITELIST; I would not add whitelist file unless it is whitelist configuration. Same as you do with PSPBTLDR_WL_FILE.
https://review.coreboot.org/c/coreboot/+/42859/63/util/amdfwtool/data_parse.... PS63, Line 399: return 0; /* Stop parsing. */ It would be nice to print which line it choked on - something like: printf("AMDFWTOOL config file line #%d can't be parsed "%s"\n", line_num, oneline).