Attention is currently required from: Patrick Georgi, Rizwan Qureshi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Nico Huber, Martin Roth, Caveh Jalali, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 )
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq ......................................................................
Patch Set 9:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/61854/comment/b837a580_44b983d1 PS7, Line 17: Without this synchronisation being implemented, flashrom is running : into below error: : : Erasing and writing flash chip... Timeout error between offset : 0x0061c000 and 0x0061c03f (= 0x0061c000 + 63)! FAILED! : Uh oh. Erase/write failed. Checking if anything has changed.
What I'm still missing is how it relates to the change.
isn't the observation and solution part being added in the commit msg is ample for you to understand how this changes are relevant in terms of perform sync the operation coming from different agents without having a file lock mechanism in place?
AIUI, it's only the background story what led to its finding, and doesn't say anything about the change.
I think it's generally ok to add such information (even though I don't see any value). But it must be clear how it relates to the change. Confusing information can be harmful, especially in commit messages that can't be changed later on.
https://review.coreboot.org/c/flashrom/+/61854/comment/51d97639_71895236 PS7, Line 24: BUG=b:215255210
I don't see any answer. Please never again mark my comments as resolved.
Please make sure all information on that issue tracker is sane, doesn't lead to further wrong assumptions about this change and can't be changed anymore, e.g. close the ticket. Or, just don't reference it.
Do you want me to update every post from bug b/215255210 to here? I have marked this comment resolved because I have updated the problem, observation and solution all together in this commit msg. Please be specific what you want to know from that bug.
https://review.coreboot.org/c/flashrom/+/61854/comment/88cad64c_e101b92b PS7, Line 25: TEST=Concurrent flashrom access is not throwing timeout.
Please also test older platforms, not just the newest (also, platforms that are not supported yet don't matter). If you need help with testing you can always ask on the mailing list and IRC.
Sorry, I don't have those older platform POC information. I will try to set a context with those owners informally. Looking at older platform EDS specification, I don't expect anything would behave differently there as this is same HW seq on Intel platform started with SKL. (you can always ask question about how since then so many years AU worked without this bit being implement on older platform, I don't have that answer either with me)