Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Edward O'Callaghan, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56721 )
Change subject: internal: Return early from map_flash for >16MiB on x86 ......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS1:
Sorry that was Patchset 1, where I got asked to switch it to the physmap area... is there some way to get this implemented? I get told physmap isn't the right place and also flashrom.c is not the right place and also it is... I'm confused 😕
Well, you shouldn't rush things. IMHO, PS1 was closer to a correct solution (closer but not close, let's give PS1 20%, and PS2 15%). You should start by reading all the code involved. It's not an easy task. .map_flash_region seems to be abused as a general hook into programmer code during flash probing. I guess this would need to be fixed first.
If this was an easy task, we'd have it done years ago. Alas, things have moved backwards during the last two years. Hacks like the one in sb600spi were added without proper review. Now they block any progress. I can see at least three things that are wrong with this call (calling something that is not supposed to be an API, mixing that with another API, no return value checked, overlapping calls of map_flash()/unmap_flash(), no support for >16MiB; oh that's 5 already?). I think this needs to be addressed before we should even start thinking about changing anything map_flash() related. Btw. I have a feeling that the code would work without the call ;) Still there is the problem that the code uses the memory mapping for no documented reason. So I'd start by requesting public documentation from AMD.
I know you just wanted to fix a seemingly small issue. So I can fully understand if you don't want to dig deeper into it. But that's just how things are now for flashrom: somebody needs to clean up after your colleagues. The project is pretty much congested by now. To have another release from the master branch somebody needs to look into all the changes since the last release again, if there are more hacks like this lurking.
-
Nico Huber (Code Review)