Attention is currently required from: Raul Rangel. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56351 )
Change subject: lib/thread: Make thread_run not block the current state ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
Patchset:
PS1: Yeah, I've been wondering about this too. It's probably a good idea. My only concern is that there's nothing really preventing your thread from running over the end of the stage in that case. It might be a good idea to add a check somewhere in payload_run() to make sure there are no more running threads in the background.
File src/lib/thread.c:
https://review.coreboot.org/c/coreboot/+/56351/comment/58a6f291_baa1d756 PS1, Line 325: printk(BIOS_ERR, "thread_run() No more threads!\n"); This is a bit off-topic for this patch, but I'm wondering if the better answer here would be to wait and yield until a new thread slot becomes available. With the way your thread_join() and the automatic termination and the storing the result in the thread_handle works now, it would be easy to accidentally spawn more threads over the course of the ramstage than CONFIG_NUM_THREADS, and it might be fine during testing because one of the threads terminates (even though its thread_join() hasn't been reached yet) before the other one starts. But then later patches or other external factors might introduce a tiny timing change, and suddenly that thread does not terminate before the next one tries to run. We would want to make sure that the system doesn't just fail to boot in that case (which is what returning -1 here would probably do most of the time), so I think printing a BIOS_WARNING and waiting until a thread slot frees up would be the best option.
-
Julius Werner (Code Review)