* [PATCH 1/1] ASoC: soc-compress: lock pcm_mutex to resolve lockdep error [not found] <CGME20200330110126epcas2p4525e5c6f61f7452df008696f9153770d@epcas2p4.samsung.com> @ 2020-03-30 11:01 ` 이경택 2020-03-30 11:47 ` Vinod Koul 0 siblings, 1 reply; 9+ messages in thread From: 이경택 @ 2020-03-30 11:01 UTC (permalink / raw) To: lgirdwood, broonie, tiwai, vkoul; +Cc: alsa-devel 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); mutex_unlock(&fe->card->mutex); @@ -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); fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE; -- 2.21.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] ASoC: soc-compress: lock pcm_mutex to resolve lockdep error 2020-03-30 11:01 ` [PATCH 1/1] ASoC: soc-compress: lock pcm_mutex to resolve lockdep error 이경택 @ 2020-03-30 11:47 ` Vinod Koul 2020-03-31 1:54 ` 이경택 0 siblings, 1 reply; 9+ messages in thread From: Vinod Koul @ 2020-03-30 11:47 UTC (permalink / raw) To: �̰��� Cc: alsa-devel, broonie, lgirdwood, tiwai 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? Thanks -- ~Vinod ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/1] ASoC: soc-compress: lock pcm_mutex to resolve lockdep error 2020-03-30 11:47 ` Vinod Koul @ 2020-03-31 1:54 ` 이경택 2020-04-02 2:44 ` Gyeongtaek Lee 2020-04-03 7:55 ` Vinod Koul 0 siblings, 2 replies; 9+ messages in thread From: 이경택 @ 2020-03-31 1:54 UTC (permalink / raw) To: 'Vinod Koul'; +Cc: pilsun.jang, alsa-devel, broonie, lgirdwood, tiwai 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/1] ASoC: soc-compress: lock pcm_mutex to resolve lockdep error 2020-03-31 1:54 ` 이경택 @ 2020-04-02 2:44 ` Gyeongtaek Lee 2020-04-03 7:55 ` Vinod Koul 1 sibling, 0 replies; 9+ messages in thread From: Gyeongtaek Lee @ 2020-04-02 2:44 UTC (permalink / raw) To: 'Vinod Koul' Cc: alsa-devel, kimty, tiwai, lgirdwood, broonie, hmseo, tkjung, pilsun.jang 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] ASoC: soc-compress: lock pcm_mutex to resolve lockdep error 2020-03-31 1:54 ` 이경택 2020-04-02 2:44 ` Gyeongtaek Lee @ 2020-04-03 7:55 ` Vinod Koul 2020-04-09 1:32 ` Gyeongtaek Lee 1 sibling, 1 reply; 9+ messages in thread From: Vinod Koul @ 2020-04-03 7:55 UTC (permalink / raw) To: 이경택 Cc: pilsun.jang, alsa-devel, broonie, lgirdwood, tiwai 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@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. 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.. -- ~Vinod ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/1] ASoC: soc-compress: lock pcm_mutex to resolve lockdep error 2020-04-03 7:55 ` Vinod Koul @ 2020-04-09 1:32 ` Gyeongtaek Lee 0 siblings, 0 replies; 9+ messages in thread From: Gyeongtaek Lee @ 2020-04-09 1:32 UTC (permalink / raw) To: 'Vinod Koul' Cc: alsa-devel, tiwai, lgirdwood, broonie, hmseo, tkjung, pilsun.jang 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@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. > >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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <CGME20200326084608epcas2p32239121f201613573e7dd86c3172a29f@epcas2p3.samsung.com>]
* [PATCH 1/1] ASoC: soc-compress: lock pcm_mutex to resolve lockdep error [not found] <CGME20200326084608epcas2p32239121f201613573e7dd86c3172a29f@epcas2p3.samsung.com> @ 2020-03-26 8:46 ` 이경택 2020-03-30 1:22 ` 이경택 0 siblings, 1 reply; 9+ messages in thread From: 이경택 @ 2020-03-26 8:46 UTC (permalink / raw) To: lgirdwood, broonie, tiwai; +Cc: alsa-devel snd_soc_runtime_activate() and snd_soc_runtime_deactivate() require pcm_mutex lock. 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); mutex_unlock(&fe->card->mutex); @@ -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); fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE; -- 2.21.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH 1/1] ASoC: soc-compress: lock pcm_mutex to resolve lockdep error 2020-03-26 8:46 ` 이경택 @ 2020-03-30 1:22 ` 이경택 2020-03-30 10:33 ` Mark Brown 0 siblings, 1 reply; 9+ messages in thread From: 이경택 @ 2020-03-30 1:22 UTC (permalink / raw) To: lgirdwood, broonie, tiwai; +Cc: alsa-devel On Thu, 26 Mar 2020 17:46:07 +0900, Gyeongtaek Lee wrote: >Hi, > >I'd like to send a patch to fix a problem in soc-compress with DPCM. >soc_compr_open_fe() and soc_compr_free_fe() call snd_soc_runtime_activate() >and snd_soc_runtime_deactivate() but don't lock card->pcm_mutex. >It can cause lockdep error, because snd_soc_runtime_activate/deactivate() >checks whether pcm_mutex is held using lockdep_assert_held(). >I'd like to send a patch which adds mutex_lock/unlock() >before and after of the snd_soc_runtime_activate/deactivate() call. >If there is anything should be changed in my patch or email, >please let me know it. >I'll really appreciate it. > >Gyeongtaek Lee (1): > ASoC: soc-compress: lock pcm_mutex to resolve lockdep error > > sound/soc/soc-compress.c | 4 ++++ > 1 file changed, 4 insertions(+) > > >base-commit: 76ccd234269bd05debdbc12c96eafe62dd9a6180 >--- > >snd_soc_runtime_activate() and snd_soc_runtime_deactivate() >require pcm_mutex lock. > >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); > > mutex_unlock(&fe->card->mutex); > >@@ -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); > > fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE; > >-- >2.21.0 > Hi, Resending patch with removing tab expansion. Lee ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] ASoC: soc-compress: lock pcm_mutex to resolve lockdep error 2020-03-30 1:22 ` 이경택 @ 2020-03-30 10:33 ` Mark Brown 0 siblings, 0 replies; 9+ messages in thread From: Mark Brown @ 2020-03-30 10:33 UTC (permalink / raw) To: �̰���; +Cc: alsa-devel, lgirdwood, tiwai [-- Attachment #1: Type: text/plain, Size: 517 bytes --] On Mon, Mar 30, 2020 at 10:22:21AM +0900, �̰��� wrote: > >snd_soc_runtime_activate() and snd_soc_runtime_deactivate() > >require pcm_mutex lock. > > > >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 I can't do anything with this patch as it's quoted, please resend. Please also remember to CC Vinod, the compressed audio maintainer. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-04-09 1:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20200330110126epcas2p4525e5c6f61f7452df008696f9153770d@epcas2p4.samsung.com>
2020-03-30 11:01 ` [PATCH 1/1] ASoC: soc-compress: lock pcm_mutex to resolve lockdep error 이경택
2020-03-30 11:47 ` Vinod Koul
2020-03-31 1:54 ` 이경택
2020-04-02 2:44 ` Gyeongtaek Lee
2020-04-03 7:55 ` Vinod Koul
2020-04-09 1:32 ` Gyeongtaek Lee
[not found] <CGME20200326084608epcas2p32239121f201613573e7dd86c3172a29f@epcas2p3.samsung.com>
2020-03-26 8:46 ` 이경택
2020-03-30 1:22 ` 이경택
2020-03-30 10:33 ` Mark Brown
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.