Skip to content

ipc: userspace: fix IPC serialization with multiple cores - #11186

Open
kv2019i wants to merge 1 commit into
thesofproject:mainfrom
kv2019i:202609-ipc-ordering-fix-multicore
Open

ipc: userspace: fix IPC serialization with multiple cores#11186
kv2019i wants to merge 1 commit into
thesofproject:mainfrom
kv2019i:202609-ipc-ordering-fix-multicore

Conversation

@kv2019i

@kv2019i kv2019i commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Fix a race in IPC serialization with multi-core. The sequence observed:

  • MOD_SET_DX IPC to power up core 1
  • IPC reply to host
  • CREATE_PIPELINE IPC (routed via core 0 to core 1)
  • core 1 IPC thread starts, signals ipc_user->sem semaphore
  • core 0 does NOT wait for thread as ipc_user->init_needed is set late
    • ipc_user->sem signal for thread start is handled as indication that IPC is handled (this is wrong)
  • IPC reply to host (before CREATE_PIPELINE is handled)
  • INIT_INSTANCE IPC (routed via core 0 to core 1)
    • core 0 sees init_needed, but it is already signaled so execution continues -> DSP panic as IPC mailbox is modified while still in use

The race is caused by a bug in setting "init_needed" and starting the IPC user thread on a secondary core (core 1 above). Due to the race, the CREATE_PIPELINE does not wait for core 1 to signal readiness and instead replies to host early, breaking IPC serialization.

Fix the problem by moving set of "init_needed" before the IPC thread is started on a new core. This will ensure CREATE_PIPELINE in above sequence will be not forwarded to core 1 until the IPC thread is ready on the new core.

Copilot AI lite review requested due to automatic review settings September 10, 2026 10:42
@kv2019i
kv2019i requested a review from lyakh September 10, 2026 10:42
@kv2019i

kv2019i commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

FYI, this is easily hit on sof-ptl-nocodec.tplg with PR11164 (see comment #11164 (review) ).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The updated logic still relies on a cross-core init_needed[] flag that is accessed non-atomically, which can allow stale reads and reintroduce the serialization race on weakly ordered systems.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes an IPC serialization race in multi-core scenarios by ensuring the per-core “userspace IPC thread not yet ready” flag is set before the secondary-core IPC userspace thread is started, so the primary core correctly blocks on the initial startup semaphore signal rather than misinterpreting it as command completion.

Changes:

  • Move ipc_user->init_needed[core] = true; ahead of k_thread_start() in ipc_user_init_secondary() to close the startup ordering race on secondary cores.
File summaries
File Description
src/ipc/ipc-common.c Adjusts secondary-core IPC userspace thread startup ordering to prevent premature host replies and broken IPC serialization.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/ipc/ipc-common.c Outdated
@intel-sofci

intel-sofci commented Sep 10, 2026

Copy link
Copy Markdown

PR 11186: test results

Run date: 2026-09-11 15:22 UTC

Tested commit: 31277bdac0006b90a1f81244939f4377c14d26cf

mtl pass rate lnl pass rate ptl pass rate wcl pass rate nvl pass rate

@kv2019i
kv2019i marked this pull request as draft September 10, 2026 12:13
@kv2019i

kv2019i commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Back to draft, still fails, V2 coming

@kv2019i
kv2019i force-pushed the 202609-ipc-ordering-fix-multicore branch from fe491d8 to 66120c3 Compare September 10, 2026 19:07
@kv2019i
kv2019i marked this pull request as ready for review September 10, 2026 19:07
@kv2019i

kv2019i commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

V2:

  • more problems found, rework the synchronization to not rely on ipc_user->sem to signal thread readiness and leave the semaphore solely to track IPC completions
  • the atomics are used purely to ensure compiler doesn't optimize or reorder

@kv2019i
kv2019i force-pushed the 202609-ipc-ordering-fix-multicore branch from 66120c3 to 5db4397 Compare September 10, 2026 19:10
@kv2019i

kv2019i commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

V3:

  • fix cmocka and alsa plugin

@lyakh lyakh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks strange... Would a very simple fix: set init_needed on core 0 before booting core 1 fix the problem too?

Fix a race in IPC serialization with multi-core. One sequence
observed:

- MOD_SET_DX IPC to power up core 1
- IPC reply to host
- CREATE_PIPELINE IPC (routed via core 0 to core 1)
- core 1 IPC thread starts, signals ipc_user->sem semaphore
- core 0 does NOT wait for thread as ipc_user->init_needed is set late
	- ipc_user->sem signal for thread start is handled as
	  indication that IPC is handled (this is wrong)
- IPC reply to host (before CREATE_PIPELINE is handled)
- INIT_INSTANCE IPC (routed via core 0 to core 1)
	- core 0 sees init_needed, but it is already signaled so execution
	  continues
	-> DSP panic as IPC mailbox is modified while still in use

Additional complication is that there is no hard requirement for host
to send an IPC destinated to a particular core x, just after MOD_SET_DX
was sent to power up this specific core. This is the normal sequence,
but FW needs to at least gracefully handle alternative sequences.

Fix the serialization issue by moving IPC thread startup synchronization
to MOD_SET_DX handling. When a secondary core is booted up the first
time after last primary core boot, additional setup steps are done.

Reset "init_needed[]" after each primary core boot, and make MOD_SET_DX
synchronous when a new secondary core is booted up, not sending a response
back to host until the secondary core is booted up and the one-time
initialization is done. Note that after this, secondary cores may be powered
down and up many times, but the initialization (and related synchronization)
is no longer needed.

With these changes, there is no longer need to synchronize with secondary
core when forwarding IPC messages (in ipc_user_forward_cmd()). It is now
guaranteed the target core is running and set up correctly. If any failures
happen, these are reported already at MOD_SET_DX.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
@kv2019i
kv2019i force-pushed the 202609-ipc-ordering-fix-multicore branch from 5db4397 to 31277bd Compare September 11, 2026 11:07
@kv2019i

kv2019i commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator Author

V4:

  • discussed offline with @lyakh and implemented a different approach
  • this passes same test as V3, but moves the synchronization of newly booted secondary cores, to MOD_SET_DX

@kv2019i
kv2019i requested a review from lyakh September 11, 2026 11:10

@lyakh lyakh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this: "Note that after this, secondary cores may be powered
down and up many times, but the initialization (and related synchronization)
is no longer needed."

Comment thread src/ipc/ipc-common.c

for (core = 0; core < CONFIG_CORE_COUNT; core++) {
if (core != PLATFORM_PRIMARY_CORE_ID)
ipc_user->init_needed[core] = true;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, and if you power off a core and then power it back on?

@lyakh lyakh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, clarified. The per-core userspace IPC thread isn't terminated when the respective core is powered down. All the threads stay there, they just aren't schedulable, because they are pinned to the powered off cores.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants