Change in coreboot[master]: arch/x86/smp/spinlock: Fix threading when !STAGE_HAS_SPINLOCKS
Attention is currently required from: Raul Rangel, Furquan Shaikh, Tim Wawrzynczak, Kyösti Mälkki, Rob Barnes, Karthik Ramasubramanian. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59021 )
Change subject: arch/x86/smp/spinlock: Fix threading when !STAGE_HAS_SPINLOCKS ......................................................................
Patch Set 2:
(1 comment)
File src/arch/x86/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/59021/comment/ef407f62_5dc9e45d PS2, Line 50: thread_coop_disable();
For this particular __always_inline, it might be unnecessary. I just don't know. […]
Well, if threading is disabled then thread_mutex_lock() is a no-op right now. You're saying we should change that so it acts like a spinlock in SMP stages? I guess we can do that too, and then someone needs to check all existing uses of spin_lock() and replace with thread_mutex where appropriate.
I'm not sure it's really worth it to keep both concepts around independently, though? The only place where a spinlock without mutex would be valid are those where you can guarantee there's no udelay() and no other mutex in the critical section. That's not that easy to keep track of, and it just takes someone adding a debug print somewhere at a later point to subtly break things. Why take that risk -- it's not like the mutexes are that much more expensive? Why not just combine the concepts completely so programmers don't have to think about which one they use?
-
Julius Werner (Code Review)