Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44045 )
Change subject: util/amdfwtool: If no APOB_NV is specified, don't error out ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c@... PS1, Line 932: apob_idx = find_bios_entry(AMD_BIOS_APOB); Why do we perform this lookup prior to checking src field?
https://review.coreboot.org/c/coreboot/+/44045/1/util/amdfwtool/amdfwtool.c@... PS1, Line 933: if (!fw_table[i].src) {
so here we only need to check to see if there's also no source specified.
I'm not sure what scenarios this existing code is trying to cover, but I feel like this code is not really very clear. Aren't the scenarios the following?
1. error if src and !size 2. !dest in existing apob? -> apob not used 3. And you want to add !src && !size -> apob not used
if (fw_table[i].type == AMD_BIOS_APOB_NV) { if (!fw_table[i].size && !fw_table[i].src) continue; // not used if (fw_table[i].src && !fw_table[i].size) error apob_idx = find_bios_entry(); if (apob_idx < 0 || !fw_table[apob_idx].dest) continue; // not used // fall through to the baseline error }