Attention is currently required from: Raul Rangel, Furquan Shaikh. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56350 )
Change subject: lib/thread: Allow nesting thread_cooperate and thread_prevent_coop ......................................................................
Patch Set 1:
(4 comments)
File src/include/thread.h:
https://review.coreboot.org/c/coreboot/+/56350/comment/d584c901_8674f468 PS1, Line 54: void thread_prevent_coop(void); While we're here, are we happy with the names for this? I feel like something that makes it a bit more obvious that they are matching pairs would be nice, maybe thread_enter_critical_section()/thread_exit_critical_section(), or thread_coop_disable()/thread_coop_enable()?
File src/lib/thread.c:
https://review.coreboot.org/c/coreboot/+/56350/comment/cd87d047_2ee5028d PS1, Line 217: thread_cooperate(); I agree this doesn't belong here, but maybe it would be better to put it at the end of threads_initialize() rather than into the thread setup part. The die() call above, if it ever gets hit (which it shouldn't but whatever) might theoretically yield on some platforms during the console output and cause issues if the idle thread is not set up correctly.
https://review.coreboot.org/c/coreboot/+/56350/comment/45c56bb5_2b1ddab3 PS1, Line 34: return (t != NULL && t->can_yield > 0); This check is a bit odd since under normal use can_yield should either be 0 or negative. Which I guess works but also seems a bit weird. I feel like an unsigned integer where 0 means "can yield" and critical section depth is counted upwards would be more intuitive (could call it critical_section_depth or something like that?).
https://review.coreboot.org/c/coreboot/+/56350/comment/1ea52a9a_a414923c PS1, Line 363: Would be good to assert() that we were in a critical section when this is called.