[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