Question about DPCM locking

Question about DPCM locking

[Kuninori Morimoto]

Hi Takashi

May I ask about this patch ?

	ASoC: soc-pcm: Fix and cleanup DPCM locking
	b7898396f4bbe160f546d0c5e9fa17cca9a7d153

Q1.

It exchanges many function parameters

	- func(substream, xxx)
	+ func(rtd, substream, xxx)
	       ^^^^

I guess the purpose is it want to call snd_soc_dpcm_mutex_assert_held()
inside the func ? But it looks very verbose.
we can do like this, but am I misunderstanding ?

	func(substream, xxx) {
		struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);

		snd_soc_dpcm_mutex_assert_held(rtd);

		...
	}

Q2.

It added new __soc_pcm_close().
But soc_pcm_close() is using soc_pcm_clean() instead of __soc_pcm_close().
Is this just a mistake ?

	static int soc_pcm_close(struct snd_pcm_substream *substream)
	{
		struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);

		snd_soc_dpcm_mutex_lock(rtd);
-		soc_pcm_clean(substream, 0);
+		__soc_pcm_close(substream);
		snd_soc_dpcm_mutex_unlock(rtd);
		return 0;
	}


Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: Question about DPCM locking

[Takashi Iwai]

On Mon, 27 Jun 2022 05:04:38 +0200,
Kuninori Morimoto wrote:
> 
> 
> Hi Takashi
> 
> May I ask about this patch ?
> 
> 	ASoC: soc-pcm: Fix and cleanup DPCM locking
> 	b7898396f4bbe160f546d0c5e9fa17cca9a7d153

Sorry for the very late response.  It's been completely overlooked.

> Q1.
> 
> It exchanges many function parameters
> 
> 	- func(substream, xxx)
> 	+ func(rtd, substream, xxx)
> 	       ^^^^
> 
> I guess the purpose is it want to call snd_soc_dpcm_mutex_assert_held()
> inside the func ? But it looks very verbose.
> we can do like this, but am I misunderstanding ?
> 
> 	func(substream, xxx) {
> 		struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> 
> 		snd_soc_dpcm_mutex_assert_held(rtd);
> 
> 		...
> 	}

This could be changed in that way, too.
I just took the current code as the compiler produced a bit better
code.

> Q2.
> 
> It added new __soc_pcm_close().
> But soc_pcm_close() is using soc_pcm_clean() instead of __soc_pcm_close().
> Is this just a mistake ?
> 
> 	static int soc_pcm_close(struct snd_pcm_substream *substream)
> 	{
> 		struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> 
> 		snd_soc_dpcm_mutex_lock(rtd);
> -		soc_pcm_clean(substream, 0);
> +		__soc_pcm_close(substream);
> 		snd_soc_dpcm_mutex_unlock(rtd);
> 		return 0;
> 	}

Not really a mistake, as you can see, that's the very same code :)
That said, it's fine to call __soc_pcm_close() there instead of the
open code, too, as long as the resultant binary becomes same (or
better).


thanks,

Takashi

Re: Question about DPCM locking

[Kuninori Morimoto]

Hi Takashi-san


> Not really a mistake, as you can see, that's the very same code :)
> That said, it's fine to call __soc_pcm_close() there instead of the
> open code, too, as long as the resultant binary becomes same (or
> better).

Thanks.
I will post the patch at next -rc1 timing

Thank you for your help !!

Best regards
---
Kuninori Morimoto