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

[Cezary Rojewski <cezary.rojewski@intel.com>]

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.