Bao Zheng 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 30:
(12 comments)
https://review.coreboot.org/c/coreboot/+/42859/28/src/soc/amd/stoneyridge/Ma... File src/soc/amd/stoneyridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/28/src/soc/amd/stoneyridge/Ma... PS28, Line 273: #cbfs-files-y += apu/amdfw
haven't tested, but i'd expect that this will cause issues for stoneyridge after this patch is merge […]
Yes. it has problem. If we want to avoid this, we have to squash all the next 3 patches into one. What do you think?
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/amdfwtool.h File util/amdfwtool/amdfwtool.h:
PS28:
please add the spdx line: /* SPDX-License-Identifier: GPL-2. […]
Done. Added.
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/amdfwtool.c... PS28, Line 87: uint8_t g_have_whitelist, g_unlock_secure, g_use_secureos, : g_load_mp2_fw, g_s0i3;
this is unused and can be removed
Done. Removed.
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/amdfwtool.c... PS28, Line 601: #if 0
see my comment below on this
Done
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/amdfwtool.c... PS28, Line 1102: static void register_fw_fuses(int fd, amd_cb_config *cb_config)
i find this function name very misleading. […]
Done. Change to process_config_file
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/amdfwtool.c... PS28, Line 1257: // int abl_image = 0;
please remove the commented out lines. same below.
Done
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/amdfwtool.c... PS28, Line 1702: if (targetfd >= 0) { : ssize_t bytes; : bytes = write(targetfd, amd_romsig, ctx.current - romsig_offset); : if (bytes != ctx.current - romsig_offset) : retval = 1; : close(targetfd); : } else {
haven't tried, but splitting these lines out into another patch and moving it before the makefile ch […]
Done. Moved.
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/data_parse.... File util/amdfwtool/data_parse.c:
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/data_parse.... PS28, Line 65: //extern uint8_t g_have_whitelist, g_unlock_secure, g_use_secureos, : // g_load_mp2_fw, g_s0i3;
please remove the commented out lines of code. same below.
Done
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/data_parse.... PS28, Line 84: subprog = 0;
writing to a function parameter, especially before it got read, seems to be a bug to me
Done. The subprog match check has been removed.
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/data_parse.... PS28, Line 405: #if 1
remove the #if 1 and corresponding #endif
Done
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/data_parse.... PS28, Line 474: unsigned int read_line(char **config, char **oneline)
this function seems to be sort-of a reimplementation of getline(). […]
Done. use fgets to get a line.
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/data_parse.... PS28, Line 498: void process_config(char *config, amd_cb_config *cb_config)
i'd pass the file handle instead of a char * with the file contents
Done. Changed.