Attention is currently required from: Raul Rangel, Nico Huber, Xiang W, Philipp Hug, Jonathan Neuschäfer, Tim Wawrzynczak, Angel Pons, 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 15:
(1 comment)
Patchset:
PS15:
Well, I'm not sure what more to say here. […]
All this review is happening in a no-op change, while CB:59321 is where spinlock/mutex change really takes place. It has not been rebased for couple months.
I have provided pointers in my previous commentary (CB:60253 and CB:60280) that show it's easy to use compiler atomic builtins to provide a spinlock, and easy to build mutex with such a spinlock. It's unreviewed work that I still consider viable alternative solution, and a cleaner one. It does not answer riscv spinlocks either, it's incomplete work.
I don't agree with Julius that removing spinlock and only providing mutex would simplify codebse. We continue to have sequences where, IMHO spinlock makes more sense than newly introduced mutex.
This is known to not be SMP safe, as it turns out it completely lacks a spinlock currently, but why is mutex() ever better than spinlock() around a index/data IO access like these:
src/arch/x86/include/arch/pci_io_cfg.h
The case is somewhat the same with RTC / CMOS NVRAM access using split index/data IO access. Seems like those cmos_read/write() variants are currently SMP -safe either. CB:34929 hints that there is more work to be done on this area and talks about .data, while .bss seems to be the easier approach.