Attention is currently required from: Raul Rangel, Karthik Ramasubramanian. Julius Werner 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/73584417_36fd03b4 PS1, Line 328: DEBUG("%s: cbfs preload thread done\n", __func__);
Honestly, I share all the same concerns. […]
Woah, wait a second, let's not go overboard. I don't think futures are going to solve these problems any better than threads. With a futures model you still need to "kick" your asynchronous tasks regularly because your DMA hardware can only transfer so many bytes at once before it needs software intervention, and the only real good place to integrate those "kicks" is in udelay(). So you still have exactly the same problem that any udelay() can potentially trigger concurrent accesses to data structures you're currently handling. Maybe not for this one particular function, because this wouldn't run in an extra thread in that case... but for the generic case that is still totally true (e.g. if the driver that does the DMA continue operation also needs to maintain some list that's shared between different contexts for some reason). Just because you're not calling it "multitasking" doesn't mean it's immune to concurrency issues. Using a threading model at least makes these issues more familiar to most programmers.
I think we'll just need to come up with a good model to handle these issues. The classic "secure every data structure with its own mutex" is maybe a bit overkill for our case, although it would work. I think what I suggested at the end of my last comment is probably the best option: Just use disable_coop() as a big hammer around all write operations to the list. It's a very cheap call, and this way you don't need to worry about securing the parts that only read the list.
-
Julius Werner (Code Review)