Attention is currently required from: Jason Glenesk, Raul Rangel, Jakub Czapiga, Matt DeVillier, Paul Menzel, Tim Van Patten, Fred Reitberger, Karthik Ramasubramanian, Felix Held.
Grzegorz Bernacki 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:
(5 comments)
File src/soc/amd/common/block/cpu/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/74716/comment/293b46f7_3bf2faea 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. […]
Thanks for noticing this, I just put `find_microcode()` under if statement, so there is no need to have `ucode_cache` variable
https://review.coreboot.org/c/coreboot/+/74716/comment/c0c6a13f_0fd9d9df PS2, Line 100: cbfs_unmap((void *)ucode);
As noted in the previous comment, this is essentially calling `cbfs_unmap(NULL)` (as enforced by the […]
I don't modify ucode anymore, it should be fixed
https://review.coreboot.org/c/coreboot/+/74716/comment/a3326bd9_bc736ef7 PS2, Line 111: (void *)
Are these casts necessary?
Yes, it generates warning otherwise
File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/74716/comment/b20108aa_7e58c5dd PS1, Line 173: cbfs_unmap(rom);
`cbfs_unmap()` ignores addresses it doesn't recognize, so it's (currently) safe to always call it, e […]
Yes, you're right, but it would be good to have more clean solution. For now I just move unmapping to pci_rom_free().
File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/74716/comment/9df48731_9a318320 PS2, Line 164: return;
Based on the comment below, this appears to be missing a call to `timestamp_add_now()`. […]
Yes, it is, thanks. I added it in new version
-
Grzegorz Bernacki (Code Review)