haven't fully reviewed the c files yet, but wanted to post at least what i've already reviewed/found
17 comments:
File src/soc/amd/picasso/Makefile.inc:
Patch Set #28, 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
File src/soc/amd/stoneyridge/Makefile.inc:
Patch Set #28, 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?
File src/southbridge/amd/pi/hudson/Makefile.inc:
Patch Set #28, Line 208: #cbfs-files-y += apu/amdfw
same as my comment on the corresponding change in the stoneyridge makefile
File util/amdfwtool/amdfwtool.h:
please add the spdx line: /* SPDX-License-Identifier: GPL-2.0-only */
File util/amdfwtool/amdfwtool.c:
uint8_t g_have_whitelist, g_unlock_secure, g_use_secureos,
g_load_mp2_fw, g_s0i3;
this is unused and can be removed
Patch Set #28, Line 601: #if 0
see my comment below on this
Patch Set #28, 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?
Patch Set #28, 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?
Patch Set #28, Line 1257: // int abl_image = 0;
please remove the commented out lines. same below.
Patch Set #28, 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
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
File util/amdfwtool/data_parse.c:
//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.
Patch Set #28, Line 84: subprog = 0;
writing to a function parameter, especially before it got read, seems to be a bug to me
Patch Set #28, Line 405: #if 1
remove the #if 1 and corresponding #endif
Patch Set #28, Line 441: tableptr = (void *)amd_bios_table;
i'd avoid all these potentially dangerous casts and keep everything as the native type
Patch Set #28, 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
Patch Set #28, 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
To view, visit change 42859. To unsubscribe, or for help writing mail filters, visit settings.