[PATCH 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.
[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 | 2 ++ sound/soc/intel/avs/pcm.c | 10 ++++++++-- 2 files changed, 10 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 --- sound/soc/intel/avs/ipc.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c index 152f8d0bdf8e..07655298b6c7 100644 --- a/sound/soc/intel/avs/ipc.c +++ b/sound/soc/intel/avs/ipc.c @@ -123,7 +123,9 @@ static void avs_dsp_recovery(struct avs_dev *adev) if (!substream || !substream->runtime) continue;
+ 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 --- 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..f95c530ffeb1 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) { + data->substream->runtime->status->state = + 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) { + data->substream->runtime->status->state = + SNDRV_PCM_STATE_DISCONNECTED; return ret; + } } } }
On 11/10/22 08:13, Cezary Rojewski wrote:
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
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..f95c530ffeb1 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) {data->substream->runtime->status->state =SNDRV_PCM_STATE_DISCONNECTED;
should runtime->state be used instead of runtime->status->state?
A quick grep shows the former seems more popular in drivers, status->seems to be only used in pcm_native.c?
Another plumbing question is whether it's actually ok to change the state of the runtime in a driver, are you not going to have locking issues? Very few drivers change the internal state and I wonder how legit/safe this is.
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) {data->substream->runtime->status->state =SNDRV_PCM_STATE_DISCONNECTED; return ret;} }
On 2022-11-10 4:39 PM, Pierre-Louis Bossart wrote:
On 11/10/22 08:13, Cezary Rojewski wrote:
--- 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) {data->substream->runtime->status->state =SNDRV_PCM_STATE_DISCONNECTED;should runtime->state be used instead of runtime->status->state?
A quick grep shows the former seems more popular in drivers, status->seems to be only used in pcm_native.c?
Another plumbing question is whether it's actually ok to change the state of the runtime in a driver, are you not going to have locking issues? Very few drivers change the internal state and I wonder how legit/safe this is.
Unless something new has been added/updated, there is no runtime->state field available. All the PCM code works with runtime->status->state.
component->resume() gets fired before any PCMs have a chance to be resumed. Userspace cannot access us _yet_. Similarly for suspend, component->suspend() is called once all the streams receive TRIGGER_SUSPEND and from then on, we're on device-pm level already.
Well, in regard to grep, very few drivers enter the recovery jungle. All of this is to help improve user experience when things go south. Unfortunately for us, restoring regmap is insufficient to get AudioDSP happy. Right now if things do go south, userspace still performs all of the PCM commands on the stream as nothing has happened - snd_soc_suspend/resume() return 0 in all cases.
Regards, Czarek
On 11/10/22 10:06, Cezary Rojewski wrote:
On 2022-11-10 4:39 PM, Pierre-Louis Bossart wrote:
On 11/10/22 08:13, Cezary Rojewski wrote:
--- 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) { + data->substream->runtime->status->state = + SNDRV_PCM_STATE_DISCONNECTED;
should runtime->state be used instead of runtime->status->state?
A quick grep shows the former seems more popular in drivers, status->seems to be only used in pcm_native.c?
Another plumbing question is whether it's actually ok to change the state of the runtime in a driver, are you not going to have locking issues? Very few drivers change the internal state and I wonder how legit/safe this is.
Unless something new has been added/updated, there is no runtime->state field available. All the PCM code works with runtime->status->state.
cd sound/
git grep -c 'runtime->state' core/compress_offload.c:27 core/oss/pcm_oss.c:21 core/pcm.c:2 core/pcm_compat.c:2 core/pcm_lib.c:8 core/pcm_native.c:50 drivers/aloop.c:2 firewire/bebob/bebob_pcm.c:2 firewire/dice/dice-pcm.c:2 firewire/digi00x/digi00x-pcm.c:2 firewire/fireface/ff-pcm.c:2 firewire/fireworks/fireworks_pcm.c:2 firewire/motu/motu-pcm.c:2 firewire/oxfw/oxfw-pcm.c:4 firewire/tascam/tascam-pcm.c:2 hda/hdmi_chmap.c:1 isa/gus/gus_pcm.c:1 soc/intel/skylake/skl-pcm.c:2 soc/sh/rz-ssi.c:1 usb/pcm.c:2 usb/usx2y/usbusx2yaudio.c:1 usb/usx2y/usx2yhwdeppcm.c:1
git grep -c 'status->state' core/pcm_compat.c:2 core/pcm_native.c:4
On 2022-11-10 5:18 PM, Pierre-Louis Bossart wrote:
On 11/10/22 10:06, Cezary Rojewski wrote:
Unless something new has been added/updated, there is no runtime->state field available. All the PCM code works with runtime->status->state.
cd sound/
git grep -c 'runtime->state' core/compress_offload.c:27
...
git grep -c 'status->state' core/pcm_compat.c:2 core/pcm_native.c:4
I see now, thanks. Commit from late September "ALSA: pcm: Avoid reference to status->state" add a new field. Will update the code to use __snd_pcm_set_state() instead.
My base did not have it yet.
On 11/10/22 10:29, Cezary Rojewski wrote:
On 2022-11-10 5:18 PM, Pierre-Louis Bossart wrote:
On 11/10/22 10:06, Cezary Rojewski wrote:
Unless something new has been added/updated, there is no runtime->state field available. All the PCM code works with runtime->status->state.
cd sound/
git grep -c 'runtime->state' core/compress_offload.c:27
...
git grep -c 'status->state' core/pcm_compat.c:2 core/pcm_native.c:4
I see now, thanks. Commit from late September "ALSA: pcm: Avoid reference to status->state" add a new field. Will update the code to use __snd_pcm_set_state() instead.
My base did not have it yet.
Right, it's relatively recent, and my point is that the helper does use stream locking when testing the same state you modify. Maybe that's a red herring but I thought I'd ask.
static void snd_pcm_set_state(struct snd_pcm_substream *substream, snd_pcm_state_t state) { snd_pcm_stream_lock_irq(substream); if (substream->runtime->state != SNDRV_PCM_STATE_DISCONNECTED) __snd_pcm_set_state(substream->runtime, state); snd_pcm_stream_unlock_irq(substream); }
On 2022-11-10 5:36 PM, Pierre-Louis Bossart wrote:
On 11/10/22 10:29, Cezary Rojewski wrote:
On 2022-11-10 5:18 PM, Pierre-Louis Bossart wrote:
On 11/10/22 10:06, Cezary Rojewski wrote:
Unless something new has been added/updated, there is no runtime->state field available. All the PCM code works with runtime->status->state.
cd sound/
git grep -c 'runtime->state' core/compress_offload.c:27
...
git grep -c 'status->state' core/pcm_compat.c:2 core/pcm_native.c:4
I see now, thanks. Commit from late September "ALSA: pcm: Avoid reference to status->state" add a new field. Will update the code to use __snd_pcm_set_state() instead.
My base did not have it yet.
Right, it's relatively recent, and my point is that the helper does use stream locking when testing the same state you modify. Maybe that's a red herring but I thought I'd ask.
static void snd_pcm_set_state(struct snd_pcm_substream *substream, snd_pcm_state_t state) { snd_pcm_stream_lock_irq(substream); if (substream->runtime->state != SNDRV_PCM_STATE_DISCONNECTED) __snd_pcm_set_state(substream->runtime, state); snd_pcm_stream_unlock_irq(substream); }
Your question is a right one and this is the right time to talk about locking. Our current test results and my analysis show that locking is not needed (what isn't the case for the first patch in the series) but races such as this one are hard to reproduce. If I'm proven wrong, no problem updating the code on my side.
participants (2)
-
Cezary Rojewski -
Pierre-Louis Bossart