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:
(4 comments)
File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/58954/comment/9d5d4b44_bcad46ab PS1, Line 276: /* Only valid when done == true */ nit: maybe easier to just add a CB_IN_PROGRESS error type to model this?
https://review.coreboot.org/c/coreboot/+/58954/comment/f23bbfa3_8d255086 PS1, Line 313: list_for_each(context, cbfs_preload_context_list, list_node) { Would be good to add a comment here (or somewhere) explaining why we don't need a mutex to prevent concurrent access to the list.
https://review.coreboot.org/c/coreboot/+/58954/comment/1073bbd3_04566a32 PS1, Line 328: DEBUG("%s: cbfs preload thread done\n", __func__); Hmmm... so the one thing that concerns me a bit is that there are a lot of implicit assumptions about what constitutes an automatic critical section in our concurrency model here. For example, you rely on the fact that there can be no yield() in the middle of a list_append() call to make sure the list is always in a consistent state for the reader thread. Or here, you rely on the fact that there's no yield() between the last check in the list_for_each() loop (where it decides it has reached the end) and the end of the thread, to ensure that no new context may have been enqueued after we were done processing the list but before the thread state changed to THREAD_DONE.
Right now it's pretty easy to guarantee all these are true (you are guaranteeing that there can be no yield() in a printk(), right? Or did I misunderstand that?), but it just seems a bit "wrong" to the normal concurrency-aware programmer's mind. Dunno what the right solution here is... to just keep it this way and accept that coreboot concurrency is "special" and you have to manually pay attention to things that can udelay(), or to splatter the code with a ton of extra mutexes (or at least disable_coop()) that we know we don't really need. Maybe at least add comments warning about the various implicit assumptions?
(Maybe in this case it would be good enough to just have this function disable_coop() right at the start, and then only enable it inside the loop right before calling rdev_readat(), and disable it again before the end of the loop. That way all the list accesses and other areas of concern would be in the guaranteed coop-free zone, and disable_coop() is a bit cheaper than a mutex variable.)
https://review.coreboot.org/c/coreboot/+/58954/comment/8ae23d5a_e8e08acf PS1, Line 337: switch (cbfs_preload_handle.state) { nit: bit complicated way of saying
if (cbfs_preload_handle.state == THREAD_DONE) return CB_SUCCESS;
-
Julius Werner (Code Review)