Question about DPCM private_data of soc-compress.c

Question about DPCM private_data of soc-compress.c

[Kuninori Morimoto]

Hi ALSA ML

I'm not familiar with soc-compress.c, but I think dpcm_capture case don't need "else" ?
I think it is same as soc-pcm.c :: soc_new_pcm().
I'm happy to post patch if someone acknowledge this question.

	--- soc-compress.c --------
	int snd_soc_new_compress(...)
	{
		...
		if (rtd->dai_link->dynamic) {
			...
			if (rtd->dai_link->dpcm_playback)
				be_pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd;
-			else if (rtd->dai_link->dpcm_capture)
+			if (rtd->dai_link->dpcm_capture)
				be_pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd;
			...
		}
		...
	}

	--- soc-pcm.c ---------------
	int soc_new_pcm(...)
	{
		...
		if (rtd->dai_link->no_pcm || rtd->dai_link->params) {
			if (playback)
				pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd;
=>			if (capture)
				pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd;
			goto out;
		}
		...
	}

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: Question about DPCM private_data of soc-compress.c

[Charles Keepax]

On Mon, Jan 23, 2023 at 03:08:49AM +0000, Kuninori Morimoto wrote:
> 
> Hi ALSA ML
> 
> I'm not familiar with soc-compress.c, but I think dpcm_capture case don't need "else" ?
> I think it is same as soc-pcm.c :: soc_new_pcm().
> I'm happy to post patch if someone acknowledge this question.
> 
> 	--- soc-compress.c --------
> 	int snd_soc_new_compress(...)
> 	{
> 		...
> 		if (rtd->dai_link->dynamic) {
> 			...
> 			if (rtd->dai_link->dpcm_playback)
> 				be_pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd;
> -			else if (rtd->dai_link->dpcm_capture)
> +			if (rtd->dai_link->dpcm_capture)
> 				be_pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd;
> 			...
> 		}
> 		...
> 	}
> 
> 	--- soc-pcm.c ---------------
> 	int soc_new_pcm(...)
> 	{
> 		...
> 		if (rtd->dai_link->no_pcm || rtd->dai_link->params) {
> 			if (playback)
> 				pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd;
> =>			if (capture)
> 				pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd;
> 			goto out;
> 		}
> 		...
> 	}
> 

I would certainly agree, pretty sure the else is redundant.

Thanks,
Charles

Re: Question about DPCM private_data of soc-compress.c

[Pierre-Louis Bossart]

On 1/23/23 06:01, Charles Keepax wrote:
> On Mon, Jan 23, 2023 at 03:08:49AM +0000, Kuninori Morimoto wrote:
>>
>> Hi ALSA ML
>>
>> I'm not familiar with soc-compress.c, but I think dpcm_capture case don't need "else" ?
>> I think it is same as soc-pcm.c :: soc_new_pcm().
>> I'm happy to post patch if someone acknowledge this question.
>>
>> 	--- soc-compress.c --------
>> 	int snd_soc_new_compress(...)
>> 	{
>> 		...
>> 		if (rtd->dai_link->dynamic) {
>> 			...
>> 			if (rtd->dai_link->dpcm_playback)
>> 				be_pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd;
>> -			else if (rtd->dai_link->dpcm_capture)
>> +			if (rtd->dai_link->dpcm_capture)
>> 				be_pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd;
>> 			...
>> 		}
>> 		...
>> 	}
>>
>> 	--- soc-pcm.c ---------------
>> 	int soc_new_pcm(...)
>> 	{
>> 		...
>> 		if (rtd->dai_link->no_pcm || rtd->dai_link->params) {
>> 			if (playback)
>> 				pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd;
>> =>			if (capture)
>> 				pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd;
>> 			goto out;
>> 		}
>> 		...
>> 	}
>>
> 
> I would certainly agree, pretty sure the else is redundant.

Agree as well on paper, but the two cases are different.

For the soc-pcm.c case, the configuration is for a Back-End (BE). We do
have usages where both .dpcm_playback and .dpcm_capture are set.

In the soc-compress.c case, the configuration is for a Front-End (FE). I
am not sure if there are any FE dailinks with both capture and playback
support or if there are any hidden restrictions?

We'd need to check that this has no impact on platforms with compressed
support - Srinivas and Vinod can you chime in?

At any rate this change would need a Fixes tag

Fixes: d3268a40d4b1 ("ASoC: soc-compress.c: fix NULL dereference")