* [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 ` [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, 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
* 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
* 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-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-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-30 11:01 ` 이경택
@ 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
* [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
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] <CGME20200326084608epcas2p32239121f201613573e7dd86c3172a29f@epcas2p3.samsung.com>
2020-03-26 8:46 ` [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
[not found] <CGME20200330110126epcas2p4525e5c6f61f7452df008696f9153770d@epcas2p4.samsung.com>
2020-03-30 11:01 ` 이경택
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
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.