ipc4: replace fw_reg spinlock with mutex#10693
ipc4: replace fw_reg spinlock with mutex#10693kv2019i wants to merge 2 commits intothesofproject:mainfrom
Conversation
To enable use of Zephyr sys/mutex.h interface in SOF audio pipeline code, add a rtos/mutex.h wrapper for this. In Zephyr build, the native Zephyr interface is used and in posix builds (e.g. testbench), a no-op implementation is provided (similarly that already exists for k_mutex). Signed-off-by: Kai Vehmanen <[email protected]>
Replace the spinlock-based synchronization mechanism for firmware register access with a mutex-based approach. This change removes the fw_reg_lock spinlock field from the sof structure (both POSIX and Zephyr variants), eliminates the spinlock initialization in primary_core_init(), and updates the dai_dma_release(), dai_release_llp_slot(), and dai_get_unused_llp_slot() functions to use sys_mutex_lock/unlock instead of k_spin_lock/unlock. All SOF IPC and audio pipeline code is run in threads, so using spinlocks is no longer necessary. Using mutex is a best fit and allows the audio pipeline code to be used also in user-space threads. Signed-off-by: Kai Vehmanen <[email protected]>
kv2019i
requested review from
abonislawski,
dbaluta,
iuliana-prodan,
lbetlej,
lgirdwood,
lyakh,
mmaka1 and
plbossart
as code owners
|
I'm using this to move audio pipelines to user-space in #10558 , but wanted to submit this early to get review feedback. If ok, I'll proceed to use sys_mutex more widely in the audio pipeline code. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR replaces IPC4 firmware register (fw_reg) synchronization from a per-struct sof Zephyr spinlock to a sys_mutex, with the goal of removing spinlocks from ipc4/dai.c.
Changes:
- Remove
fw_reg_lock(k_spinlock) fromstruct soffor both Zephyr and POSIX builds. - Introduce a file-scope
SYS_MUTEX_DEFINE(fw_reg_lock)insrc/ipc/ipc4/dai.cand usesys_mutex_lock/unlock()around mailbox register accesses. - Add
sys_mutexheader support in Zephyr and a stubsys_muteximplementation for POSIX builds.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| zephyr/include/rtos/sof.h | Removes IPC4 fw_reg_lock from struct sof. |
| zephyr/include/rtos/mutex.h | Includes Zephyr sys/mutex.h to support sys_mutex. |
| src/ipc/ipc4/dai.c | Replaces k_spin_lock usage with a static sys_mutex for fw register access. |
| src/init/init.c | Removes initialization of the deleted sof->fw_reg_lock spinlock. |
| posix/include/rtos/sof.h | Mirrors Zephyr: removes fw_reg_lock from struct sof. |
| posix/include/rtos/mutex.h | Adds a POSIX-side sys_mutex API stub. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* provide a no-op implementation for zephyr/sys/mutex.h */ | ||
|
|
||
| struct sys_mutex { | ||
| }; | ||
|
|
||
| #define SYS_MUTEX_DEFINE(name) \ | ||
| struct sys_mutex name | ||
|
|
||
| static inline void sys_mutex_init(struct sys_mutex *mutex) | ||
| { | ||
| } | ||
|
|
||
| static inline int sys_mutex_lock(struct sys_mutex *mutex, k_timeout_t timeout) | ||
| { | ||
| return 0; | ||
| } | ||
|
|
||
| static inline int sys_mutex_unlock(struct sys_mutex *mutex) | ||
| { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
The POSIX sys_mutex_*() implementation is a no-op, so the new critical sections in ipc4/dai.c are not actually synchronized under POSIX builds. If POSIX can execute these paths concurrently (threads or async callbacks), this introduces data races on mailbox fw registers. Implement sys_mutex using a real lock (e.g., pthread_mutex_t) or map sys_mutex_*() onto the existing k_mutex_*() wrappers in this header so the locking semantics remain effective on POSIX.
| sys_mutex_lock(&fw_reg_lock, K_FOREVER); | ||
| mailbox_sw_regs_write(dd->slot_info.reg_offset, &slot, sizeof(slot)); | ||
| k_spin_unlock(&sof_get()->fw_reg_lock, key); | ||
| sys_mutex_unlock(&fw_reg_lock); |
There was a problem hiding this comment.
sys_mutex_lock() / sys_mutex_unlock() return status codes, but the results are currently ignored. If the Zephyr implementation can fail (e.g., timeout/cancellation/invalid context), execution would continue without the intended protection. Consider checking the return values (at least asserting success for K_FOREVER, or propagating an error) in all three call sites so failures don’t silently degrade synchronization.
Series to add sys/mutex.h support for SOF audio code and use it to remove spinlocks from ipc4/dai.c code.