Attention is currently required from: Raul Rangel, Eric Peers, Felix Held. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56230 )
Change subject: lib/thread: Add mutex ......................................................................
Patch Set 5:
(5 comments)
File src/include/thread.h:
https://review.coreboot.org/c/coreboot/+/56230/comment/490e3269_f87728e4 PS5, Line 88: mutex_lock nit: I think it's better to keep everything here prefixed thread_ (e.g. make it thread_mutex_lock() or thread_lock()).
File src/lib/thread.c:
https://review.coreboot.org/c/coreboot/+/56230/comment/771c1007_ec2474fc PS2, Line 406: thread_yield_microseconds(10);
I had that thought, but wanted to avoid adding a return value to this method.
I don't think this function should return in that case, at least. Deadlock is a programming error and it's not safe to just continue execution while ignoring the lock. If anything, you could make it die() after a certain timeout, but it would have to be really long (like 20-30 seconds maybe) to make sure you can't hit it accidentally in cases where people have a lot of slow debug stuff enabled.
I'd say a lot of thing in firmware can hang when you write them wrong and firmware engineers are used to debugging that. ;) I don't think we need to add extra monitoring here until it turns out to be a real problem.
File src/lib/thread.c:
https://review.coreboot.org/c/coreboot/+/56230/comment/2271c9d2_2c97fa8f PS5, Line 391: thread_yield_microseconds(0); Should assert that this didn't fail (e.g. because can_yield is 0, because then you'll wait forever).
https://review.coreboot.org/c/coreboot/+/56230/comment/fe66de0c_e4fe10a6 PS5, Line 392: 1 Please only use `true` and `false` with bools, and only numbers with integers.
https://review.coreboot.org/c/coreboot/+/56230/comment/4a468bc4_ec3455eb PS5, Line 394: printk(BIOS_DEBUG, " took %lu us to acquire mutex\n", stopwatch_duration_usecs(&sw)); I doubt this is gonna be very useful, since there's 0 context on what thread is waiting on what mutex here. If you want to keep it, maybe make it BIOS_SPEW?
(Also, I'm unclear why this starts with a space. Is it supposed to be a direct continuation of another message? Which one?)