Attention is currently required from: Raul Rangel, Julius Werner, Rob Barnes, Karthik Ramasubramanian. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59320 )
Change subject: lib: Add a mutex ......................................................................
Patch Set 13:
(3 comments)
Patchset:
PS13: I won't be able to +1 this. This really adds a spinlock implementation with .bss while keeping the spinlock implementation in .data around.
Also, I feel this uses 3x the lines of code it should, follow-ups extend unnecessarily (and mostly without argumentation) to existing uses of spinlock, and I am not convinced this brings much DMA performance to romstage if the DMA thread is almost never serviced.
File src/lib/mutex.c:
https://review.coreboot.org/c/coreboot/+/59320/comment/0c14dc26_1248d055 PS13, Line 16: } What makes the two above a mutex and not a spinlock? Your struct mutex is really nothing more but a new spinlock implementation with .bss. It only becomes a mutex when these get used with the possibility to yield.
https://review.coreboot.org/c/coreboot/+/59320/comment/2cfe7b0a_7d54d90d PS13, Line 43: assert(_mutex_is_locked_smp(mutex)); Do we allow uses of assert() with side-effects?
Like commented before, the printk() here (which one should never hit) kills romstage DMA performance as you need to disable coop while in printk.
IMHO it should be allowed to have empty ASSERT() definition.
-
Kyösti Mälkki (Code Review)