Re: [PATCH] ASoC: SOF: Intel: Fix stream cleanup on pcm close

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

On 2020-02-18 17:45, Pierre-Louis Bossart wrote:
> On 2/18/20 8:10 AM, Cezary Rojewski wrote:
>> Field "substream" gets assigned during stream setup in
>> hda_dsp_pcm_hw_params() but it is never cleared afterwards during
>> hda_dsp_pcm_close(). Now, any non-pcm operation e.g.: compress can
>> mistakenly make use of that pointer as it's bypassing all
>> "if (s->substream)" checks.
>>
>> Nulling the pointer during close operation ensures no wild pointers are
>> left behind.
>>
>> Fixes: cdae3b9a47aa ("ASoC: SOF: Intel: Add Intel specific HDA PCM 
>> operations")
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   sound/soc/sof/intel/hda-pcm.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/sound/soc/sof/intel/hda-pcm.c 
>> b/sound/soc/sof/intel/hda-pcm.c
>> index a46a6baa1c3f..4b3a89cf20e7 100644
>> --- a/sound/soc/sof/intel/hda-pcm.c
>> +++ b/sound/soc/sof/intel/hda-pcm.c
>> @@ -246,5 +246,6 @@ int hda_dsp_pcm_close(struct snd_sof_dev *sdev,
>>       /* unbinding pcm substream to hda stream */
>>       substream->runtime->private_data = NULL;
>> +    hstream->substream = NULL;
>>       return 0;
>>   }
> 
> 
> Humm, yes we should clean this, but wondering if the close() operation 
> is the right place. Doing this is hda_dsp_stream_hw_free() sounds more 
> logical to me?

Ain't hda-pcm.c the best place for it as "hstream->substream = 
substream" happens there too? If the cleanup is to be done in 
_hw_free(), then I'd expect the same to happen to the original 
assignments. Doubt we want to do the later so.. _close() for the win?

In general the existing hstream->substream initialization looks kinda 
disconnected from the actual stream assignment code - _stream_get() - as 
if the duties of the state machine were shared.

Czarek