Attention is currently required from: Bao Zheng, Jason Glenesk, Raul Rangel, Martin Roth, Marshall Dawson, Zheng Bao. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56773 )
Change subject: amdfwtool: Add support for AMD's BIOS A/B recovery feature ......................................................................
Patch Set 26:
(7 comments)
Patchset:
PS26: haven't completely reviewed everything in this patch yet, but it's probably better when i already post the comments on what i've found so far.
have you verified that a timeless build will result in identical images for a kahlee/grunt, a zork and a guybrush chromebook?
File util/amdfwtool/amdfwtool.h:
https://review.coreboot.org/c/coreboot/+/56773/comment/6358cd6e_1c45192d PS26, Line 240: uint8_t recovery_ab; i'd use a boolean here; probably also the case for some of the struct elements above
File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/56773/comment/230ddeac_7e0dedf3 PS26, Line 669: int this is a different type compared to cb_config->recovery_ab
https://review.coreboot.org/c/coreboot/+/56773/comment/ffe7186e_a992f77c PS26, Line 1351: psp_directory_table *pspdir, *pspdir2 = NULL, *pspdir2_b = NULL; maybe put the 3 definitions on 3 separate lines
https://review.coreboot.org/c/coreboot/+/56773/comment/c5c428d7_c299c722 PS26, Line 1700: pspdir2_b = NULL; /* More explicitly */ since the corresponding if block has curly braces, please also use curly braces around the one statement in the else branch
https://review.coreboot.org/c/coreboot/+/56773/comment/b72077ec_65b9dbb1 PS26, Line 1707: 0 NULL
https://review.coreboot.org/c/coreboot/+/56773/comment/f0dc4245_2ab09257 PS26, Line 1733: , *biosdir2_b = NULL i'd put this second definition on a new line to make it more obvious what's done here
-
Felix Held (Code Review)