Attention is currently required from: Jason Glenesk, Raul Rangel, Jakub Czapiga, Matt DeVillier, Paul Menzel, Grzegorz Bernacki, Fred Reitberger, Karthik Ramasubramanian, Felix Held.
Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74716 )
Change subject: soc/amd/common/block: Add missing cbfs_unmap() ......................................................................
Patch Set 2:
(7 comments)
File src/soc/amd/common/block/cpu/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/74716/comment/36c881b6_74964255 PS2, Line 98: ucode = find_microcode(ucode, equivalent_processor_rev_id); This is a bug, since we can no long `cbfs_unmap(ucode)`, since we've lost the address.
Please restore `ucode_cache`, so we can call `cbfs_unmap(ucode)` before returning if there is an error.
https://review.coreboot.org/c/coreboot/+/74716/comment/e61532db_5d21c1d8 PS2, Line 100: cbfs_unmap((void *)ucode); As noted in the previous comment, this is essentially calling `cbfs_unmap(NULL)` (as enforced by the conditional to get here).
https://review.coreboot.org/c/coreboot/+/74716/comment/a5b71908_b3c3e905 PS2, Line 111: (void *) Are these casts necessary?
File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/74716/comment/ff1eb536_467c4fc0 PS1, Line 168: if (ram == NULL) { : printk(BIOS_ERR, "%s: Unable to load ROM for %s\n", : __func__, dev_path(dev)); : }
Actually, I did not add 'return' here on purpose. […]
Ack
https://review.coreboot.org/c/coreboot/+/74716/comment/a605a277_dc4a2686 PS1, Line 173: cbfs_unmap(rom);
I talked with Jakub and Konrad and we decided to add `pci_rom_free` function which for now directly […]
`cbfs_unmap()` ignores addresses it doesn't recognize, so it's (currently) safe to always call it, even if it's not entirely correct.
File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/74716/comment/0ac5898f_f4590eff PS2, Line 157: timestamp_add_now(TS_OPROM_COPY_END); : return; ref1
https://review.coreboot.org/c/coreboot/+/74716/comment/b24f8365_fabea334 PS2, Line 164: return; Based on the comment below, this appears to be missing a call to `timestamp_add_now()`.
The error case above (ref1) also includes it, so it appears to have been missed.
-
Tim Van Patten (Code Review)