Attention is currently required from: Julius Werner, Karthik Ramasubramanian. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58954 )
Change subject: lib/cbfs: Use a work queue for cbfs_preload ......................................................................
Patch Set 1:
(1 comment)
File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/58954/comment/1e74887d_5afc4b0f PS1, Line 328: DEBUG("%s: cbfs preload thread done\n", __func__);
Hmmm... […]
Honestly, I share all the same concerns. I've stepped on land mine after land mine and done some painful refactors. After all the time and energy I've invested in productionizing cooperative threading, I honestly think it's the wrong model for coreboot. It gives me anxiety thinking about trying to use coop-threading for anything else. i.e.,i2c transaction. There are just so many sharp corners that require a lot of time and energy to understand and fix. This is due to the fact that coreboot was always single threaded. So now we are left in a state (as you said) where we need to add mutexes everywhere, or know when to call coop_disable/enable. If I could I would delete coop-threading in a heart beat and replace it with the futures API.
Since `cbfs_preload` now encapsulates all the async behavior, we wouldn't need to add all the `cbfs_XXX_async` methods that I had originally proposed. We could get away with only adding `rdev_read_async`. This would correctly model the operation that is happening. It still stresses me out that a `cbfs_preload` operation can actually block if `can_use_dma` in `spi_dma.c` returns false. There is no signal that this is happening either.
This CL is essentially recreating a very specialized version of the futures API. I think we could simplify `cbfs_preload` to just calling `rdev_readat_async`, and then call `wait_for_future` in `get_preload_rdev`.
The futures API does force drivers that have `_async` methods to write a state machine, but it's at least isolated. Unlike threads where there state gets implicitly encoded in the stack and introduces a lot of unknowns everywhere. It gives the driver more control over scheduling the pending requests too.