Felix Held 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 28:
(17 comments)
haven't fully reviewed the c files yet, but wanted to post at least what i've already reviewed/found
https://review.coreboot.org/c/coreboot/+/42859/28/src/soc/amd/picasso/Makefi... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/28/src/soc/amd/picasso/Makefi... PS28, Line 135: PSP_USE_PSPSECUREOS="--use-pspsecureos" i like that this is passed as a flag and the corresponding file from the cfg file is added depending on this flag
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 merged and before the next one is merged. is this necessary?
https://review.coreboot.org/c/coreboot/+/42859/28/src/southbridge/amd/pi/hud... File src/southbridge/amd/pi/hudson/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/28/src/southbridge/amd/pi/hud... PS28, Line 208: #cbfs-files-y += apu/amdfw same as my comment on the corresponding change in the stoneyridge makefile
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.0-only */
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
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/amdfwtool.c... PS28, Line 601: #if 0 see my comment below on this
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. process_config_file maybe?
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/amdfwtool.c... PS28, Line 1117: bytes+1 shouldn't that be [bytes] instead of [bytes + 1]. [bytes] addresses the byte+1th element. also why not just use the file descriptor and read and process the file contents line by line instead of copying the contents to a byte array and processing that array?
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.
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/amdfwtool.c... PS28, Line 1548: #if 0 maybe add some parameter to call to those functions conditionally? this is very useful functionality, so i'd like to have it in the tool, but code that won't see the compiler due to being commented out or ifdef'ed out tends to bit-rot over time
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 change might fix the issue i was seeing there
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.
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
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/data_parse.... PS28, Line 405: #if 1 remove the #if 1 and corresponding #endif
https://review.coreboot.org/c/coreboot/+/42859/28/util/amdfwtool/data_parse.... PS28, Line 441: tableptr = (void *)amd_bios_table; i'd avoid all these potentially dangerous casts and keep everything as the native type
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(). please use the existing function instead
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