RE: [PATCH 1/1] ASoC: soc-compress: lock pcm_mutex to resolve lockdep error

["Gyeongtaek Lee" <gt82.lee@samsung.com>]

On Tuesday, March 31, 2020 10:55 AM, Gyeongtaek Lee wrote:
>Hi,
>
>On 30-03-20, 17:17, Vinod Koul wrote:
>>Hello,
>>
>>On 30-03-20, 20:01,  ̰    wrote:
>>> snd_soc_runtime_activate() and snd_soc_runtime_deactivate() require 
>>> locked pcm_mutex.
>>> 
>>> Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
>>> ---
>>>  sound/soc/soc-compress.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>> 
>>> diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c index 
>>> 392a1c5b15d3..42d416ac7e9b 100644
>>> --- a/sound/soc/soc-compress.c
>>> +++ b/sound/soc/soc-compress.c
>>> @@ -207,7 +207,9 @@ static int soc_compr_open_fe(struct 
>>> snd_compr_stream
>>> *cstream)
>>>  	fe->dpcm[stream].state = SND_SOC_DPCM_STATE_OPEN;
>>>  	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
>>>  
>>> +	mutex_lock_nested(&fe->pcm_mutex, fe->pcm_subclass);
>>>  	snd_soc_runtime_activate(fe, stream);
>>> +	mutex_unlock(&fe->pcm_mutex);
>>
>>Can you please explain why you need the lock here, as
>>>  
>>>  	mutex_unlock(&fe->card->mutex);
>>
>>we already have a lock here..
>>
>>> @@ -285,7 +287,9 @@ static int soc_compr_free_fe(struct 
>>> snd_compr_stream
>>> *cstream)
>>>  	else
>>>  		stream = SNDRV_PCM_STREAM_CAPTURE;
>>>  
>>> +	mutex_lock_nested(&fe->pcm_mutex, fe->pcm_subclass);
>>>  	snd_soc_runtime_deactivate(fe, stream);
>>> +	mutex_unlock(&fe->pcm_mutex);
>>
>>And this instance is also using fe->card->mutex.. so I think double lock may not serve any purpose here..
>>
>>Can you explain why we need the extra lock?
>You are right.
>The mutex_lock with fe->pcm_mutex has no purpose.
>It just removes lockdep warning like the below
><4>[ 1437.857354]  [5:          cplay:11547] ------------[ cut here ]------------
><4>[ 1437.857463]  [5:          cplay:11547] WARNING: CPU: 5 PID: 11547 at sound/soc/soc-pcm.c:99 snd_soc_runtime_deactivate+0x88/0x140
><4>[ 1437.857498]  [5:          cplay:11547] Modules linked in:
><4>[ 1437.857557]  [5:          cplay:11547] CPU: 5 PID: 11547 Comm: cplay Tainted: G S      W         4.19.65-00198-ge6c3a8b64f3d-dirty #146
><4>[ 1437.857590]  [5:          cplay:11547] Hardware name: Samsung xxx board based on xxx (DT)
><4>[ 1437.857620]  [5:          cplay:11547] Call trace:
><4>[ 1437.857662]  [5:          cplay:11547] [<ffffff800808d598>] dump_backtrace+0x0/0x404
><4>[ 1437.857704]  [5:          cplay:11547] [<ffffff800808d9b0>] show_stack+0x14/0x1c
><4>[ 1437.857745]  [5:          cplay:11547] [<ffffff8008c5dc24>] dump_stack+0xa0/0xd8
><4>[ 1437.857784]  [5:          cplay:11547] [<ffffff80080a4b28>] __warn+0xcc/0x12c
><4>[ 1437.857821]  [5:          cplay:11547] [<ffffff8008c5cd78>] report_bug+0x78/0xcc
><4>[ 1437.857857]  [5:          cplay:11547] [<ffffff800808e5c0>] bug_handler+0x2c/0x88
><4>[ 1437.857895]  [5:          cplay:11547] [<ffffff8008085510>] brk_handler+0x88/0xc8
><4>[ 1437.857930]  [5:          cplay:11547] [<ffffff8008080f0c>] do_debug_exception+0x108/0x194
><4>[ 1437.857968]  [5:          cplay:11547] Exception stack(0xffffff8028b0b960 to 0xffffff8028b0baa0)
><4>[ 1437.858002]  [5:          cplay:11547] b960: 0000000000000024 ffffff8008e28a97 ffffffc975bb40a0 ffffff8028b0b748
><4>[ 1437.858035]  [5:          cplay:11547] b980: 0000000000000080 0000000000000000 ffffff8008129638 0000000000000000
><4>[ 1437.858066]  [5:          cplay:11547] b9a0: e0b1dc92eba18f00 e0b1dc92eba18f00 0000000000000003 0000000000000000
><4>[ 1437.858098]  [5:          cplay:11547] b9c0: 0000000000240022 0000000000000004 ffffff8009b2f420 00000000fffffff5
><4>[ 1437.858130]  [5:          cplay:11547] b9e0: ffffff8008c6baac 000000000000002c 00000000000000b0 ffffffc9673c1e80
><4>[ 1437.858161]  [5:          cplay:11547] ba00: 0000000000000000 ffffffc8190e6100 0000000000000000 ffffffc95c262e88
><4>[ 1437.858193]  [5:          cplay:11547] ba20: 0000000000000008 ffffffc8ec3050d0 ffffffc8fb81a4d0 0000000000000004
><4>[ 1437.858224]  [5:          cplay:11547] ba40: 0000000000000009 ffffff8028b0bac0 ffffff8008a895c8 ffffff8028b0baa0
><4>[ 1437.858256]  [5:          cplay:11547] ba60: ffffff8008a895c8 0000000060400005 ffffff8028b0ba48 ffffff800811d7b4
><4>[ 1437.858287]  [5:          cplay:11547] ba80: 0000007fffffffff e0b1dc92eba18f00 ffffff8028b0bac0 ffffff8008a895c8
><4>[ 1437.858318]  [5:          cplay:11547] [<ffffff8008082b18>] el1_dbg+0x18/0x78
><4>[ 1437.858355]  [5:          cplay:11547] [<ffffff8008a895c8>] snd_soc_runtime_deactivate+0x88/0x140
>
>from here
>void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
>{
>	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>	int i;
>
>	lockdep_assert_held(&rtd->pcm_mutex);
>
>and here.
>void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
>{
>	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>	int i;
>
>	lockdep_assert_held(&rtd->pcm_mutex);
>
>Approach method of this patch is too simple but,
>I think that simple approach can be used until the nicer patch arrives.
>
>Thank you for your fast review.
>>
>>Thanks
>>--
>>~Vinod
>>

Hi,

Could you review my answer and patch and give me a comment?
If you have any suggestion, just let me know it.

Thanks

Gyeongtaek