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:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/74716/comment/e7bb2579_b2145a2b PS1, Line 6:
Subject line should not end with a period. […]
Done
https://review.coreboot.org/c/coreboot/+/74716/comment/840f6581_af28cb49 PS1, Line 13: debugs
What does *debugs* mean?
Debugs meant additional debug prints added in cbfs_map/cbfs_unmap, I updated commit log
https://review.coreboot.org/c/coreboot/+/74716/comment/fbaef2e4_5b2c5b07 PS1, Line 13: Built
Build
Done
https://review.coreboot.org/c/coreboot/+/74716/comment/fa27e25d_f791c710 PS1, Line 16: Change-Id: Ibbebe7401e9f5a5312da1216408bf42fa2449db1
Please fix: `No Signed-off-by line in commit message`
Added
File src/soc/amd/common/block/cpu/update_microcode.c:
https://review.coreboot.org/c/coreboot/+/74716/comment/e70e77d1_1c505631 PS1, Line 83: struct microcode *ucode = NULL;
This variable is only valid inside the `if` block below.
Yes, and it should be static, I fixed that, thanks
https://review.coreboot.org/c/coreboot/+/74716/comment/e833bf3d_113c6d64 PS1, Line 89: cache_valid
Yes, we can remove `cache_valid` and even remove `ucode_cache`. It has the same value as `ucode`. […]
I removed both `cache_valid` and `ucode_cache`
https://review.coreboot.org/c/coreboot/+/74716/comment/3c1f0d8c_4f472de6 PS1, Line 103: return;
Since `ucode` is static, it should be null'ed out here before returning, since the memory it's point […]
This `if` changed so no need to setup `ucode` to NULL here. But I added it below
https://review.coreboot.org/c/coreboot/+/74716/comment/10d0e703_58f11545 PS1, Line 113: if (cpus_updated == get_cpu_count())
paranoid-nit: `>=`
Changed
https://review.coreboot.org/c/coreboot/+/74716/comment/9e65e04a_52ce1736 PS1, Line 114: ucode
cache_valid should be false now.
I removed that variable in new version
File src/soc/amd/common/block/graphics/graphics.c:
https://review.coreboot.org/c/coreboot/+/74716/comment/5783f31e_1d481f5e PS1, Line 173: cbfs_unmap(rom);
+1 to both Jakub and Konrad's comments. […]
I talked with Jakub and Konrad and we decided to add `pci_rom_free` function which for now directly calls `cbfs_unmap()`. I need to figure out how to free `rom` only when it was mapped without akward `if`s or creating new variables to keep that information. I will split the patches in next version