diff mbox series

ASoC: soc-core: call snd_soc_unbind_card() under mutex_lock;

Message ID 878stywfjt.wl-kuninori.morimoto.gx@renesas.com (mailing list archive)
State Accepted
Commit b545542a0b866f7975254e41c595836e9bc0ff2f
Headers show
Series ASoC: soc-core: call snd_soc_unbind_card() under mutex_lock; | expand

Commit Message

Kuninori Morimoto June 19, 2019, 1:07 a.m. UTC 
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

commit 34ac3c3eb8f0c07 ("ASoC: core: lock client_mutex while removing
link components") added mutex_lock() at soc_remove_link_components().

Is is called from snd_soc_unbind_card()

	snd_soc_unbind_card()
=>		soc_remove_link_components()
		soc_cleanup_card_resources()
			soc_remove_dai_links()
=>				soc_remove_link_components()

And, there are 2 way to call it.

(1)
	snd_soc_unregister_component()
**		mutex_lock()
			snd_soc_component_del_unlocked()
=>				snd_soc_unbind_card()
**		mutex_unlock()

(2)
	snd_soc_unregister_card()
=>		snd_soc_unbind_card()

(1) case is already using mutex_lock() when it calles
snd_soc_unbind_card(), thus, we will get lockdep warning.

commit 495f926c68ddb90 ("ASoC: core: Fix deadlock in
snd_soc_instantiate_card()") tried to fixup it, but still not
enough. We still have lockdep warning when we try unbind/bind.

We need mutex_lock() under snd_soc_unregister_card()
instead of snd_remove_link_components()/snd_soc_unbind_card().

Fixes: 34ac3c3eb8f0c07 ("ASoC: core: lock client_mutex while removing link components")
Fixes: 495f926c68ddb90 ("ASoC: core: Fix deadlock in snd_soc_instantiate_card()")
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/soc-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Kuninori Morimoto June 21, 2019, 12:58 a.m. UTC | #1 
Hi Ranjani

Can you please confirm this patch ?
It is no problem if I unbind "sound card" 1st,
but, have issue if I unbind "CPU driver" 1st.

-----------------------
# echo ec500000.sound > /sys/bus/platform/drivers/rcar_sound/unbind
[   29.972081] 
[   29.973581] ============================================
[   29.978891] WARNING: possible recursive locking detected
[   29.984203] 5.2.0-rc5+ #312 Not tainted
[   29.988036] --------------------------------------------
[   29.993346] sh/190 is trying to acquire lock:
[   29.997700] (____ptrval____) (client_mutex){+.+.}, at: snd_soc_unbind_card.part.9+0x78/0xf8
[   30.006070] 
[   30.006070] but task is already holding lock:
[   30.011901] (____ptrval____) (client_mutex){+.+.}, at: snd_soc_unregister_component+0x54/0xe8
[   30.020432] 
[   30.020432] other info that might help us debug this:
[   30.026959]  Possible unsafe locking scenario:
[   30.026959] 
[   30.032876]        CPU0
[   30.035318]        ----
[   30.037759]   lock(client_mutex);
[   30.041071]   lock(client_mutex);
[   30.044383] 
[   30.044383]  *** DEADLOCK ***
[   30.044383] 
[   30.050301]  May be due to missing lock nesting notation
[   30.050301] 
[   30.057088] 4 locks held by sh/190:
[   30.060572]  #0: (____ptrval____) (sb_writers#6){.+.+}, at: vfs_write+0x198/0x1b8
[   30.068063]  #1: (____ptrval____) (&of->mutex){+.+.}, at: kernfs_fop_write+0xc0/0x1e8
[   30.075900]  #2: (____ptrval____) (&dev->mutex){....}, at: __device_driver_lock+0x38/0x68
[   30.084085]  #3: (____ptrval____) (client_mutex){+.+.}, at: snd_soc_unregister_component+0x54/0xe8
[   30.093049] 
[   30.093049] stack backtrace:
[   30.097406] CPU: 2 PID: 190 Comm: sh Not tainted 5.2.0-rc5+ #312
[   30.103411] Hardware name: Renesas H3ULCB Kingfisher board based on r8a7795 ES2.0+ (DT)
[   30.111414] Call trace:
[   30.113860]  dump_backtrace+0x0/0x140
[   30.117520]  show_stack+0x24/0x30
[   30.120837]  dump_stack+0xc8/0x114
[   30.124238]  __lock_acquire+0x1de4/0x1e48
[   30.128244]  lock_acquire+0xdc/0x258
[   30.131819]  __mutex_lock+0x80/0x7c8
[   30.135392]  mutex_lock_nested+0x3c/0x50
[   30.139312]  snd_soc_unbind_card.part.9+0x78/0xf8
[   30.144015]  snd_soc_unregister_component+0xe4/0xe8
[   30.148894]  devm_component_release+0x20/0x30
[   30.153250]  release_nodes+0x1c8/0x240
[   30.156996]  devres_release_all+0x3c/0x58
[   30.161004]  device_release_driver_internal+0x100/0x1c0
[   30.166226]  device_driver_detach+0x28/0x38
[   30.170407]  unbind_store+0x94/0x100
[   30.173979]  drv_attr_store+0x40/0x58
[   30.177640]  sysfs_kf_write+0x50/0x78
[   30.181299]  kernfs_fop_write+0xf0/0x1e8
[   30.185219]  __vfs_write+0x48/0x90
[   30.188617]  vfs_write+0xac/0x1b8
[   30.191928]  ksys_write+0x74/0xf8
[   30.195240]  __arm64_sys_write+0x24/0x30
[   30.199163]  el0_svc_common.constprop.0+0x98/0x170
[   30.203952]  el0_svc_compat_handler+0x2c/0x38
[   30.208307]  el0_svc_compat+0x8/0x10
-----------------------

> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> commit 34ac3c3eb8f0c07 ("ASoC: core: lock client_mutex while removing
> link components") added mutex_lock() at soc_remove_link_components().
> 
> Is is called from snd_soc_unbind_card()
> 
> 	snd_soc_unbind_card()
> =>		soc_remove_link_components()
> 		soc_cleanup_card_resources()
> 			soc_remove_dai_links()
> =>				soc_remove_link_components()
> 
> And, there are 2 way to call it.
> 
> (1)
> 	snd_soc_unregister_component()
> **		mutex_lock()
> 			snd_soc_component_del_unlocked()
> =>				snd_soc_unbind_card()
> **		mutex_unlock()
> 
> (2)
> 	snd_soc_unregister_card()
> =>		snd_soc_unbind_card()
> 
> (1) case is already using mutex_lock() when it calles
> snd_soc_unbind_card(), thus, we will get lockdep warning.
> 
> commit 495f926c68ddb90 ("ASoC: core: Fix deadlock in
> snd_soc_instantiate_card()") tried to fixup it, but still not
> enough. We still have lockdep warning when we try unbind/bind.
> 
> We need mutex_lock() under snd_soc_unregister_card()
> instead of snd_remove_link_components()/snd_soc_unbind_card().
> 
> Fixes: 34ac3c3eb8f0c07 ("ASoC: core: lock client_mutex while removing link components")
> Fixes: 495f926c68ddb90 ("ASoC: core: Fix deadlock in snd_soc_instantiate_card()")
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  sound/soc/soc-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 2353886..2a408cc 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -2747,14 +2747,12 @@ static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister)
>  		snd_soc_dapm_shutdown(card);
>  		snd_soc_flush_all_delayed_work(card);
>  
> -		mutex_lock(&client_mutex);
>  		/* remove all components used by DAI links on this card */
>  		for_each_comp_order(order) {
>  			for_each_card_rtds(card, rtd) {
>  				soc_remove_link_components(card, rtd, order);
>  			}
>  		}
> -		mutex_unlock(&client_mutex);
>  
>  		soc_cleanup_card_resources(card);
>  		if (!unregister)
> @@ -2773,7 +2771,9 @@ static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister)
>   */
>  int snd_soc_unregister_card(struct snd_soc_card *card)
>  {
> +	mutex_lock(&client_mutex);
>  	snd_soc_unbind_card(card, true);
> +	mutex_unlock(&client_mutex);
>  	dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
>  
>  	return 0;
> -- 
> 2.7.4
>
Ranjani Sridharan June 21, 2019, 9:19 p.m. UTC | #2 
> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > 
> > commit 34ac3c3eb8f0c07 ("ASoC: core: lock client_mutex while
> > removing
> > link components") added mutex_lock() at
> > soc_remove_link_components().
> > 
> > Is is called from snd_soc_unbind_card()
> > 
> > 	snd_soc_unbind_card()
> > =>		soc_remove_link_components()
> > 		soc_cleanup_card_resources()
> > 			soc_remove_dai_links()
> > =>				soc_remove_link_components()
> > 
> > And, there are 2 way to call it.
> > 
> > (1)
> > 	snd_soc_unregister_component()
> > **		mutex_lock()
> > 			snd_soc_component_del_unlocked()
> > =>				snd_soc_unbind_card()
> > **		mutex_unlock()
> > 
> > (2)
> > 	snd_soc_unregister_card()
> > =>		snd_soc_unbind_card()
> > 
> > (1) case is already using mutex_lock() when it calles
> > snd_soc_unbind_card(), thus, we will get lockdep warning.

Thanks, morimoto-san. You are correct. Case 1 will result in a lockdep
warning. This patch looks good. 

Thanks,
Ranjani
> > 
> > commit 495f926c68ddb90 ("ASoC: core: Fix deadlock in
> > snd_soc_instantiate_card()") tried to fixup it, but still not
> > enough. We still have lockdep warning when we try unbind/bind.
> > 
> > We need mutex_lock() under snd_soc_unregister_card()
> > instead of snd_remove_link_components()/snd_soc_unbind_card().
> > 
> > Fixes: 34ac3c3eb8f0c07 ("ASoC: core: lock client_mutex while
> > removing link components")
> > Fixes: 495f926c68ddb90 ("ASoC: core: Fix deadlock in
> > snd_soc_instantiate_card()")
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > ---
> >  sound/soc/soc-core.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> > index 2353886..2a408cc 100644
> > --- a/sound/soc/soc-core.c
> > +++ b/sound/soc/soc-core.c
> > @@ -2747,14 +2747,12 @@ static void snd_soc_unbind_card(struct
> > snd_soc_card *card, bool unregister)
> >  		snd_soc_dapm_shutdown(card);
> >  		snd_soc_flush_all_delayed_work(card);
> >  
> > -		mutex_lock(&client_mutex);
> >  		/* remove all components used by DAI links on this card
> > */
> >  		for_each_comp_order(order) {
> >  			for_each_card_rtds(card, rtd) {
> >  				soc_remove_link_components(card, rtd,
> > order);
> >  			}
> >  		}
> > -		mutex_unlock(&client_mutex);
> >  
> >  		soc_cleanup_card_resources(card);
> >  		if (!unregister)
> > @@ -2773,7 +2771,9 @@ static void snd_soc_unbind_card(struct
> > snd_soc_card *card, bool unregister)
> >   */
> >  int snd_soc_unregister_card(struct snd_soc_card *card)
> >  {
> > +	mutex_lock(&client_mutex);
> >  	snd_soc_unbind_card(card, true);
> > +	mutex_unlock(&client_mutex);
> >  	dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card-
> > >name);
> >  
> >  	return 0;
> > -- 
> > 2.7.4
> >
Kuninori Morimoto June 24, 2019, 1:09 a.m. UTC | #3 
Hi Ranjani

Thank you for your confirming.
Can you give it your Acked-by ?
It is easy for Mark, I think.

> > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > > 
> > > commit 34ac3c3eb8f0c07 ("ASoC: core: lock client_mutex while
> > > removing
> > > link components") added mutex_lock() at
> > > soc_remove_link_components().
> > > 
> > > Is is called from snd_soc_unbind_card()
> > > 
> > > 	snd_soc_unbind_card()
> > > =>		soc_remove_link_components()
> > > 		soc_cleanup_card_resources()
> > > 			soc_remove_dai_links()
> > > =>				soc_remove_link_components()
> > > 
> > > And, there are 2 way to call it.
> > > 
> > > (1)
> > > 	snd_soc_unregister_component()
> > > **		mutex_lock()
> > > 			snd_soc_component_del_unlocked()
> > > =>				snd_soc_unbind_card()
> > > **		mutex_unlock()
> > > 
> > > (2)
> > > 	snd_soc_unregister_card()
> > > =>		snd_soc_unbind_card()
> > > 
> > > (1) case is already using mutex_lock() when it calles
> > > snd_soc_unbind_card(), thus, we will get lockdep warning.
> 
> Thanks, morimoto-san. You are correct. Case 1 will result in a lockdep
> warning. This patch looks good. 
> 
> Thanks,
> Ranjani
> > > 
> > > commit 495f926c68ddb90 ("ASoC: core: Fix deadlock in
> > > snd_soc_instantiate_card()") tried to fixup it, but still not
> > > enough. We still have lockdep warning when we try unbind/bind.
> > > 
> > > We need mutex_lock() under snd_soc_unregister_card()
> > > instead of snd_remove_link_components()/snd_soc_unbind_card().
> > > 
> > > Fixes: 34ac3c3eb8f0c07 ("ASoC: core: lock client_mutex while
> > > removing link components")
> > > Fixes: 495f926c68ddb90 ("ASoC: core: Fix deadlock in
> > > snd_soc_instantiate_card()")
> > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > > ---
> > >  sound/soc/soc-core.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> > > index 2353886..2a408cc 100644
> > > --- a/sound/soc/soc-core.c
> > > +++ b/sound/soc/soc-core.c
> > > @@ -2747,14 +2747,12 @@ static void snd_soc_unbind_card(struct
> > > snd_soc_card *card, bool unregister)
> > >  		snd_soc_dapm_shutdown(card);
> > >  		snd_soc_flush_all_delayed_work(card);
> > >  
> > > -		mutex_lock(&client_mutex);
> > >  		/* remove all components used by DAI links on this card
> > > */
> > >  		for_each_comp_order(order) {
> > >  			for_each_card_rtds(card, rtd) {
> > >  				soc_remove_link_components(card, rtd,
> > > order);
> > >  			}
> > >  		}
> > > -		mutex_unlock(&client_mutex);
> > >  
> > >  		soc_cleanup_card_resources(card);
> > >  		if (!unregister)
> > > @@ -2773,7 +2771,9 @@ static void snd_soc_unbind_card(struct
> > > snd_soc_card *card, bool unregister)
> > >   */
> > >  int snd_soc_unregister_card(struct snd_soc_card *card)
> > >  {
> > > +	mutex_lock(&client_mutex);
> > >  	snd_soc_unbind_card(card, true);
> > > +	mutex_unlock(&client_mutex);
> > >  	dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card-
> > > >name);
> > >  
> > >  	return 0;
> > > -- 
> > > 2.7.4
> > > 
>
Ranjani Sridharan June 25, 2019, 1:13 a.m. UTC | #4 
On Wed, 2019-06-19 at 10:07 +0900, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> commit 34ac3c3eb8f0c07 ("ASoC: core: lock client_mutex while removing
> link components") added mutex_lock() at soc_remove_link_components().
> 
> Is is called from snd_soc_unbind_card()
> 
> 	snd_soc_unbind_card()
> =>		soc_remove_link_components()
> 		soc_cleanup_card_resources()
> 			soc_remove_dai_links()
> =>				soc_remove_link_components()
> 
> And, there are 2 way to call it.
> 
> (1)
> 	snd_soc_unregister_component()
> **		mutex_lock()
> 			snd_soc_component_del_unlocked()
> =>				snd_soc_unbind_card()
> **		mutex_unlock()
> 
> (2)
> 	snd_soc_unregister_card()
> =>		snd_soc_unbind_card()
> 
> (1) case is already using mutex_lock() when it calles
> snd_soc_unbind_card(), thus, we will get lockdep warning.
> 
> commit 495f926c68ddb90 ("ASoC: core: Fix deadlock in
> snd_soc_instantiate_card()") tried to fixup it, but still not
> enough. We still have lockdep warning when we try unbind/bind.
> 
> We need mutex_lock() under snd_soc_unregister_card()
> instead of snd_remove_link_components()/snd_soc_unbind_card().
> 
> Fixes: 34ac3c3eb8f0c07 ("ASoC: core: lock client_mutex while removing
> link components")
> Fixes: 495f926c68ddb90 ("ASoC: core: Fix deadlock in
> snd_soc_instantiate_card()")
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Acked-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

> ---
>  sound/soc/soc-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 2353886..2a408cc 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -2747,14 +2747,12 @@ static void snd_soc_unbind_card(struct
> snd_soc_card *card, bool unregister)
>  		snd_soc_dapm_shutdown(card);
>  		snd_soc_flush_all_delayed_work(card);
>  
> -		mutex_lock(&client_mutex);
>  		/* remove all components used by DAI links on this card
> */
>  		for_each_comp_order(order) {
>  			for_each_card_rtds(card, rtd) {
>  				soc_remove_link_components(card, rtd,
> order);
>  			}
>  		}
> -		mutex_unlock(&client_mutex);
>  
>  		soc_cleanup_card_resources(card);
>  		if (!unregister)
> @@ -2773,7 +2771,9 @@ static void snd_soc_unbind_card(struct
> snd_soc_card *card, bool unregister)
>   */
>  int snd_soc_unregister_card(struct snd_soc_card *card)
>  {
> +	mutex_lock(&client_mutex);
>  	snd_soc_unbind_card(card, true);
> +	mutex_unlock(&client_mutex);
>  	dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card-
> >name);
>  
>  	return 0;
diff mbox series

Patch

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 2353886..2a408cc 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2747,14 +2747,12 @@  static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister)
 		snd_soc_dapm_shutdown(card);
 		snd_soc_flush_all_delayed_work(card);
 
-		mutex_lock(&client_mutex);
 		/* remove all components used by DAI links on this card */
 		for_each_comp_order(order) {
 			for_each_card_rtds(card, rtd) {
 				soc_remove_link_components(card, rtd, order);
 			}
 		}
-		mutex_unlock(&client_mutex);
 
 		soc_cleanup_card_resources(card);
 		if (!unregister)
@@ -2773,7 +2771,9 @@  static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister)
  */
 int snd_soc_unregister_card(struct snd_soc_card *card)
 {
+	mutex_lock(&client_mutex);
 	snd_soc_unbind_card(card, true);
+	mutex_unlock(&client_mutex);
 	dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name);
 
 	return 0;