[PATCH v3 0/2] ASoC: Intel: avs: DSP recovery and resume fixes
Two fixes that are result of the recent discussions [1][2].
First adds missing locking around snd_pcm_stop() while the second fix sets substream state to DISCONNECTED if any suspend/resume related operation fails so that userspace has means to be aware that something went wrong during said operation.
Changes in v3: - added a comment why non-_irq() locking is used around snd_pcm_stop()
Changes in v2: - __snd_pcm_set_state() replaced direct assignments of PCM state
[1]: https://lore.kernel.org/alsa-devel/73e6425f-8e51-e26f-ad83-ccc5df517a43@inte... [2]: https://lore.kernel.org/alsa-devel/20221104131244.3920179-1-cezary.rojewski@...
Cezary Rojewski (2): ASoC: Intel: avs: Lock substream before snd_pcm_stop() ASoC: Intel: avs: Disconnect substream if suspend or resume fails
sound/soc/intel/avs/ipc.c | 3 +++ sound/soc/intel/avs/pcm.c | 10 ++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-)
snd_pcm_stop() shall be called with stream lock held to prevent any races between nonatomic streaming operations.
Fixes: 2f1f570cd730 ("ASoC: Intel: avs: Coredump and recovery flow") Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com ---
Changes in v3: - added a comment why non-_irq() locking is used around snd_pcm_stop()
sound/soc/intel/avs/ipc.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c index 152f8d0bdf8e..af8a260093f4 100644 --- a/sound/soc/intel/avs/ipc.c +++ b/sound/soc/intel/avs/ipc.c @@ -123,7 +123,10 @@ static void avs_dsp_recovery(struct avs_dev *adev) if (!substream || !substream->runtime) continue;
+ /* No need for _irq() as we are in nonatomic context. */ + snd_pcm_stream_lock(substream); snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED); + snd_pcm_stream_unlock(substream); } } }
To improve performance and overall system stability, suspend/resume operations for ASoC cards always return success status and defer the actual work.
Because of that, if a substream fails to resume, userspace may still attempt to invoke commands on it as from their perspective the operation completed successfully. Set substream's state to DISCONNECTED to ensure no further commands are attempted.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com ---
Changes in v2: - __snd_pcm_set_state() replaced direct assignments of PCM state
sound/soc/intel/avs/pcm.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c index ca624fbb5c0d..70442b5212c2 100644 --- a/sound/soc/intel/avs/pcm.c +++ b/sound/soc/intel/avs/pcm.c @@ -934,8 +934,11 @@ static int avs_component_pm_op(struct snd_soc_component *component, bool be, rtd = snd_pcm_substream_chip(data->substream); if (rtd->dai_link->no_pcm == be && !rtd->dai_link->ignore_suspend) { ret = op(dai, data); - if (ret < 0) + if (ret < 0) { + __snd_pcm_set_state(data->substream->runtime, + SNDRV_PCM_STATE_DISCONNECTED); return ret; + } } }
@@ -944,8 +947,11 @@ static int avs_component_pm_op(struct snd_soc_component *component, bool be, rtd = snd_pcm_substream_chip(data->substream); if (rtd->dai_link->no_pcm == be && !rtd->dai_link->ignore_suspend) { ret = op(dai, data); - if (ret < 0) + if (ret < 0) { + __snd_pcm_set_state(data->substream->runtime, + SNDRV_PCM_STATE_DISCONNECTED); return ret; + } } } }
On Wed, 16 Nov 2022 12:55:48 +0100, Cezary Rojewski wrote:
Two fixes that are result of the recent discussions [1][2].
First adds missing locking around snd_pcm_stop() while the second fix sets substream state to DISCONNECTED if any suspend/resume related operation fails so that userspace has means to be aware that something went wrong during said operation.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/2] ASoC: Intel: avs: Lock substream before snd_pcm_stop() commit: c30c8f9d51ec24b36e2c65a6307a5c8cbc5a0ebc [2/2] ASoC: Intel: avs: Disconnect substream if suspend or resume fails commit: f3fbb553f98563f692e356aca87d656baba910a0
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Cezary Rojewski -
Mark Brown