[PATCH 0/2] ASoC: Intel: avs: DSP recovery and resume fixes

[PATCH 0/2] ASoC: Intel: avs: DSP recovery and resume fixes

[Cezary Rojewski]

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@intel.com/
[2]: https://lore.kernel.org/alsa-devel/20221104131244.3920179-1-cezary.rojewski@intel.com/

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(-)

-- 
2.25.1

[PATCH 1/2] ASoC: Intel: avs: Lock substream before snd_pcm_stop()

[Cezary Rojewski]

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);
 			}
 		}
 	}
-- 
2.25.1

[PATCH 2/2] ASoC: Intel: avs: Disconnect substream if suspend or resume fails

[Cezary Rojewski]

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;
+				}
 			}
 		}
 	}
-- 
2.25.1

Re: [PATCH 2/2] ASoC: Intel: avs: Disconnect substream if suspend or resume fails

[Pierre-Louis Bossart]

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;
> +				}
>  			}
>  		}
>  	}

Re: [PATCH 2/2] ASoC: Intel: avs: Disconnect substream if suspend or resume fails

[Cezary Rojewski]

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

Re: [PATCH 2/2] ASoC: Intel: avs: Disconnect substream if suspend or resume fails

[Pierre-Louis Bossart]

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

Re: [PATCH 2/2] ASoC: Intel: avs: Disconnect substream if suspend or resume fails

[Cezary Rojewski]

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.

Re: [PATCH 2/2] ASoC: Intel: avs: Disconnect substream if suspend or resume fails

[Pierre-Louis Bossart]

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);
}

Re: [PATCH 2/2] ASoC: Intel: avs: Disconnect substream if suspend or resume fails

[Cezary Rojewski]

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.