ASoC: SOF: Fixes forIPC4 compressed stream support - #5892
Open
ujfalusi wants to merge 6 commits into
Open
Conversation
Revert the sink-path traversal changes, keeping only the NULL spipe /
pipe_widget checks in sof_widget_free_unlocked().
path->walking is DFS stack state: it means "this edge is currently being
walked", and it has to be unwound by whoever set it. Not clearing it
after the recursive call makes it a permanent 'visited' mark, so the FREE
walk traverses a different set of edges than the SETUP walk, which
unbalances swidget->use_count:
- sof_walk_widgets_in_order() starts a walk from every aif_in (playback)
or every dai_out/output (capture) widget in the list. With aggregated
DAIs the branches reconverge on the shared widgets towards the FE.
SETUP walks and increments them once per branch, but on FREE the first
branch leaves the shared edges marked and the second branch skips them
entirely, so use_count never reaches 0. The widget_free IPC is then
never sent, the cores are never put and spipe->complete is never
cleared: the DSP side leaks and the next stream reuses a widget that
was never re-configured.
- The same happens within a single walk whenever two branches of the
list reconverge, as the shared edge below the merge point must be
walked once per incoming branch.
Clearing the flags after the loop cannot fix either case, and doing it
for both directions of every widget in the list is unsafe on its own:
walking is owned by the walk in progress, and the free walk does not hold
the DAPM mutex, so a concurrent dapm_power_widgets() may have those same
edges on its stack.
The rest of the traversal churn is not needed either. FREE only sends
IPC, it never removes DAPM paths, so the _safe iterator has nothing to
protect against here - and against a concurrent topology teardown it
would not help, as next_p is read before the recursion. p->sink cannot be
NULL for a linked path, and the list can never be NULL as
sof_widget_list_free() already checks it.
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
=== everything below is dropped by 'git rebase --autosquash' ===
Once squashed, the only thing left of the original commit are the three
hunks in sof_widget_free_unlocked(); the traversal is byte identical to
the pre-2712ab5d30d6 state. The original subject no longer describes the
patch, so use this message for the squashed commit:
ASoC: SOF: sof-audio: do not dereference swidget->spipe unconditionally on free
sof_widget_free_unlocked() dereferences swidget->spipe without checking
it for two things: swidget->spipe->complete for a scheduler widget, and
swidget->spipe->pipe_widget for the recursive free of the pipeline's
scheduler widget. Both can be reached with spipe or pipe_widget not
set, which oopses in the free path - where there is nothing left to
bail out to.
Check both before use and cache swidget->spipe in the local spipe
variable that is already there. A widget with no pipeline has nothing
to put or complete, and no scheduler widget to free, so skipping is the
correct behaviour.
No functional change for a widget that was successfully set up:
sof_widget_setup_unlocked() already rejects a dynamic pipeline widget
with no spipe or no spipe->pipe_widget with -EINVAL.
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Two things to resolve before sending it out:
- The oops was most likely hit via the compress free path
(ipc4-compress.c sof_widget_list_free()), but the details were not
recovered. Replace "can be reached with spipe or pipe_widget not set"
with the actual path and add a Fixes: tag once known.
- If it cannot be reproduced, drop the patch instead: as the last
paragraph shows, the guards are unreachable for any widget that went
through sof_widget_setup_unlocked().
ujfalusi
requested review from
bardliao,
dbaluta,
kv2019i,
lgirdwood,
plbossart and
ranj063
as code owners
August 24, 2026 08:41
There was a problem hiding this comment.
Pull request overview
This PR adjusts the SOF widget “FREE” walk so that DAPM path traversal behaves like a proper DFS stack walk again (clearing p->walking after each recursive descent), and removes the prior “reset all walking flags” cleanup approach.
Changes:
- Reworks
sof_free_widgets_in_path()recursion to clearp->walkingimmediately after returning from the recursive call. - Drops the
_safepath iterator and the globalsof_reset_path_walking_flags()helper, along with its call sites insof_walk_widgets_in_order().
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+550
to
+551
| static int sof_free_widgets_in_path(struct snd_sof_dev *sdev, struct snd_soc_dapm_widget *widget, | ||
| int dir, struct snd_sof_pcm *spcm) |
Collaborator
Author
There was a problem hiding this comment.
the original code had used this long line, so we keep it, it is not part of a change to tidy-up things.
ujfalusi
force-pushed
the
peter/sof/pr/compress-fixups-02
branch
from
August 24, 2026 09:04
cb65cb1 to
f28036f
Compare
ujfalusi
force-pushed
the
peter/sof/pr/compress-fixups-02
branch
from
August 24, 2026 09:06
f28036f to
cb65cb1
Compare
…cess modules sof_ipc4_add_init_ext_module_data() writes the object header and copies init_ext_module_size bytes into the payload buffer before anything has checked that they fit. The only bounds check is in the caller, after all objects have been added, so an oversized module data would have already overflowed the kzalloc(max_payload_size) buffer by the time it runs. The other object in the array is of a fixed, known size and always fits, but the module data is provided by the module which is being set up, so validate it in the helper before any of it is written and let the caller propagate the error. Suggested commit message for the squashed patch: ASoC: SOF: ipc4-topology: Support init_ext_module_data for process modules Add support for handling init_ext_module_data for process modules, which is going to be used by decoder and encoder type of process modules. The support is generic and it can be extended to other type of process modules or other module types than process with a small update of sof_ipc4_add_init_ext_module_data() function. The module data is of variable size, unlike the other objects of the array, so its size is validated against the maximum payload size before it is copied into the message. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> Reviewed-by: Liam Girdwood <liam.r.girdwood@intel.com> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> (cherry picked from commit 572833d3b99ea7550ea198cf2ec7b564486578ce)
…streams" This reverts commit 89a7244. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
sound/soc/soc-compress.c:170
- soc_compr_free_fe_runtime() currently only frees hw_constraints.rules and the runtime itself. If soc_compr_alloc_fe_runtime() is updated to initialize the runtime similarly to snd_pcm_attach_substream(), the corresponding teardown should also mirror snd_pcm_detach_substream() (free status/control pages, destroy buffer_mutex, clear runtime pointer safely, etc.) to avoid leaks and keep runtime state consistent.
static void soc_compr_free_fe_runtime(struct snd_soc_pcm_runtime *fe, int stream)
{
struct snd_pcm_substream *fe_substream = snd_soc_dpcm_get_substream(fe, stream);
if (!fe_substream || !fe_substream->runtime)
return;
/* BE startup callbacks may have added hw constraint rules */
kfree(fe_substream->runtime->hw_constraints.rules);
kfree(fe_substream->runtime);
fe_substream->runtime = NULL;
Comment on lines
+146
to
+155
| static int soc_compr_alloc_fe_runtime(struct snd_soc_pcm_runtime *fe, int stream) | ||
| { | ||
| struct snd_pcm_substream *fe_substream = snd_soc_dpcm_get_substream(fe, stream); | ||
|
|
||
| if (!fe_substream || fe_substream->runtime) | ||
| return 0; | ||
|
|
||
| fe_substream->runtime = kzalloc_obj(*fe_substream->runtime); | ||
| if (!fe_substream->runtime) | ||
| return -ENOMEM; |
Collaborator
Author
There was a problem hiding this comment.
this is false positive, the BE side is no_pcm and we need the runtime for the random codec drivers that are out of our control.
When a compressed stream is used on the FE side of a DPCM link, the BE is still running as 'normal' PCM. DPCM lends the runtime of the FE to every BE it opens and dpcm_be_reparent() re-points it at another FE when the lending one disconnects. The internal PCM created for a compressed FE by snd_soc_new_compress() is never opened via the PCM API, so its substream has no runtime and the BEs of a compressed FE were left with a NULL one. BE DAI and codec drivers can look at substream->runtime, they add their constraints to it in startup(), for example cs42l43, cs42l42, hdac_hdmi. The other users of compressed DPCM do not hit this as their BEs are DSP internal ports, driven by code which takes everything from the hw_params it is passed and never looks at substream->runtime. The BEs here end at generic CODEC drivers which are shared with the non DPCM case and cannot be expected to know about the FE type. Allocate a runtime for the compressed FE substream while the stream is open and fill it in from the BE parameters once the mandatory machine level be_hw_params_fixup() has run, so that the compressed FE lends a valid and populated runtime just like a PCM FE does. This also allows a BE to be shared between a compressed and a PCM FE, which is needed to play a notification over PCM to an endpoint which a compressed stream is already using. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
sof_ipc4_compr_drain_done() checks swidget->spipe before walking the widget list, but the error path taken when no host copier is found on the pipeline goes on to dereference swidget->spipe->pipe_widget to print the scheduler widget name, without checking pipe_widget itself. The function runs from the IPC RX path, so an unset pipe_widget oopses in interrupt context. Check it together with spipe, the same way sof_widget_free_unlocked() does. Suggested commit message for the squashed patch: no change needed, the existing one still describes the patch correctly. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> (cherry picked from commit 6987c5022032eff92b93e63891271d8d0468f962)
sof_ipc4_compr_set_params() frees the list of DAPM widgets on its error paths but never unprepares them, so the widgets are left prepared with their prepare time resources still taken. sof_ipc4_compr_free() can not clean up after it either: it does call sof_widget_list_unprepare(), but by then spcm->stream[dir].list is NULL and there is no list left to walk. The widgets are prepared by sof_pcm_setup_connected_widgets(), so every error path taken after it is affected, a failing snd_sof_boot_dsp_firmware() for example. Use sof_widget_list_unprepare(), which walks the widgets before freeing the list and clearing spcm->stream[dir].list, the exact sequence which is open coded here otherwise. It is also safe on the path where sof_widget_list_setup() has failed and unprepared the widgets already, since sof_unprepare_widgets_in_path() skips widgets which are not prepared. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
ujfalusi
force-pushed
the
peter/sof/pr/compress-fixups-02
branch
from
August 25, 2026 05:44
1ff4e13 to
33c0334
Compare
Collaborator
Author
|
Changes since v3:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix issues found while preparing IPC4 compressed-stream support.
Initialize extended module data for process modules, handle compressed-stream
runtime allocation in soc-compress, and unprepare connected widgets before
freeing them on error paths. Also guard against missing pipeline widgets when
handling drain notifications.
The issues were found by local review using Claude 5.