On 03-04-20, 16:55, Vinod Koul wrote: >On 31-03-20, 10:54, 이경택 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@xxxxxxxxxxx> >> >> --- >> >> 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. > >Okay > >> 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 > >So if the lockdep is complaining, then we should add lockdep assert in >the open_fe as well.. soc_compr_open_fe() is a callback of the soc_compr_syn_ops. static struct snd_compr_ops soc_compr_dyn_ops = { .open = soc_compr_open_fe, So, if lockdep_assert_held() is inserted in soc_compr_open_fe(), doubled warning message will be printed. If you have any idea, just let me know it. Thanks, Gyeongtaek > >-- >~Vinod >