Attention is currently required from: Patrick Georgi, Martin Roth, Subrata Banik, Caveh Jalali, Stefan Reinauer, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Alex Levin, Nick Vaccaro, YH Lin, Boris Mittelberg. Nico Huber 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 6:
(1 comment)
Patchset:
PS6:
So, if this code change is considered a missed step in HW sequencing recommendation, then this could still be merged..correct?
A version of this change that takes the applicable context into account (e.g. other waiting loops in this driver, implications when multiple instances would reach this point in the code simultaneously) would be accepted immediately.
As for the failure that was noticed, it is when flashrom was being used in 2 modes
- flashrom standalone
- via futility which uses libflashrom implementation
The standalone flashrom already has a fix for the issue, i.e, lockfile mechanism. libflashrom did not utilize/inherit the lockfile feature and did not acquire the lock. Which allowed other instances to run. This is already being fixed https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+...
AFAIK, there is no such locking (yet) in this project. However, I fully agree that file-based locking is the correct way. And it's interesting to see that the problem is already solved for the fork. Makes me wonder why Subrata asks how to solve such things here.
Isn't the HW based locking is much better than file based locking
Absolutely not. HW based locking would require hardware designed for such locking. Even if we would have that (which we don't), it would still need such a mechanism in every single driver (not everybody uses flashrom only for internal flashing on Intel hardware). That would mean more work to implement, more effort to maintain, to achieve little. But doesn't need any argument about it anyway, because we don't have the hardware ;) You're just wasting our time here.
and how one can achieve file based locking even in low-level fw like coreboot spi driver or depthcharge ?
I don't know about depthcharge. But in coreboot there shouldn't be multiple processes accessing the SPI controller. Even if there are, one could handle it with a simple mutex because it's all one program.
The reason, we opted for file based locking is to have a short term approach and share a fix rather waiting here in loop in code review :-). we are looking for a long term fix inside flashrom to have a `_wait_before` executing run execution.
Can you tell me more about the reasoning? The code is more than a decade old, I don't see how that's a short term approach.
Again, these are discussions for the mailing list. By constraining the discussion to Gerrit, you create a bottleneck in the review process for a project that has close to zero review resources. It hurts the project.