diff mbox

[2/5] ASoC: rsnd: make sure it uses lock when it calls rsnd_dai_call

Message ID 8738488j2k.wl%kuninori.morimoto.gx@renesas.com (mailing list archive)
State Accepted
Commit e9c390df671fadc829550935ffb6b23549f26ded
Headers show

Commit Message

Kuninori Morimoto April 10, 2015, 8:49 a.m. UTC 
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

rsnd_dai_call() should be called under rsnd_lock

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/sh/rcar/core.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

Comments

Geert Uytterhoeven May 4, 2015, 9:39 a.m. UTC | #1 
Hi Morimoto-san,

On Fri, 10 Apr 2015, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> rsnd_dai_call() should be called under rsnd_lock

Always? Why?

> @@ -949,8 +956,11 @@ static const struct snd_soc_component_driver rsnd_soc_component = {
>  static int rsnd_rdai_continuance_probe(struct rsnd_priv *priv,
>  				       struct rsnd_dai_stream *io)
>  {
> +	unsigned long flags;
>  	int ret;
>  
> +	rsnd_lock(priv, flags);
> +
>  	ret = rsnd_dai_call(probe, io, priv);
>  	if (ret == -EAGAIN) {
>  		/*

This causes the following warning on r8a7791/koelsch:

WARNING: CPU: 1 PID: 1 at kernel/locking/lockdep.c:2755 lockdep_trace_alloc+0xd4/0x118()
DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
Modules linked in:

CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.1.0-rc2-koelsch-00564-g1817f2746e13e08e #1081
Hardware name: Generic R8A7791 (Flattened Device Tree)
Backtrace: 
[<c00135e0>] (dump_backtrace) from [<c0013800>] (show_stack+0x18/0x1c)
 r6:c0546ece r5:00000009 r4:00000000 r3:00204140
[<c00137e8>] (show_stack) from [<c04483ec>] (dump_stack+0x78/0x94)
[<c0448374>] (dump_stack) from [<c002d368>] (warn_slowpath_common+0x90/0xbc)
 r4:eec59c90 r3:00000001
[<c002d2d8>] (warn_slowpath_common) from [<c002d3cc>] (warn_slowpath_fmt+0x38/0x40)
 r8:eed90010 r7:000001a1 r6:000080d0 r5:000080d0 r4:60000193
[<c002d398>] (warn_slowpath_fmt) from [<c0069e94>] (lockdep_trace_alloc+0xd4/0x118)
 r3:c054c535 r2:c0547435
[<c0069dc0>] (lockdep_trace_alloc) from [<c00e3acc>] (__kmalloc_track_caller+0x34/0x11c)
 r5:eec000c0 r4:00000000
[<c00e3a98>] (__kmalloc_track_caller) from [<c02678b4>] (devres_alloc+0x20/0x48)
 r7:000001a1 r6:ee7e2138 r5:c007a3d8 r4:00000000
[<c0267894>] (devres_alloc) from [<c007a378>] (devm_request_threaded_irq+0x34/0x94)
 r5:ee7ea8d0 r4:00000000
[<c007a344>] (devm_request_threaded_irq) from [<c036ad4c>] (rsnd_src_probe_gen2+0x68/0x7c)
 r9:eef61120 r8:a0000113 r7:ee7ea8d8 r6:eef6113c r5:ee7ea8d0 r4:ee7e2138
[<c036ace4>] (rsnd_src_probe_gen2) from [<c0367e1c>] (rsnd_rdai_continuance_probe+0x80/0x29c)
 r5:eef61130 r4:c036ace4
[<c0367d9c>] (rsnd_rdai_continuance_probe) from [<c0368a80>] (rsnd_probe+0x114/0x304)
 r10:eef61110 r9:00000000 r8:ee7ea8d8 r7:eed90000 r6:eed90010 r5:ee7ea8d0
 r4:c048e960
[<c036896c>] (rsnd_probe) from [<c02667f4>] (platform_drv_probe+0x50/0xa0)
 r10:00000000 r9:c062f000 r8:c0624dec r7:00000000 r6:c0624dec r5:eed90010
 r4:fffffffe
[<c02667a4>] (platform_drv_probe) from [<c0264fac>] (driver_probe_device+0xfc/0x264)
 r6:00000000 r5:c0e1e9fc r4:eed90010 r3:c02667a4
[<c0264eb0>] (driver_probe_device) from [<c02651d8>] (__driver_attach+0x78/0x9c)
 r8:c0606460 r7:c0619e98 r6:c0624dec r5:eed90044 r4:eed90010 r3:00000000
[<c0265160>] (__driver_attach) from [<c02635c8>] (bus_for_each_dev+0x74/0x98)
 r6:c0265160 r5:c0624dec r4:00000000 r3:00000001
[<c0263554>] (bus_for_each_dev) from [<c0264a98>] (driver_attach+0x20/0x28)
 r6:ee7ea9c0 r5:00000000 r4:c0624dec
[<c0264a78>] (driver_attach) from [<c0264728>] (bus_add_driver+0xe8/0x1d0)
[<c0264640>] (bus_add_driver) from [<c0265868>] (driver_register+0xa4/0xe8)
 r7:c0606460 r6:00000000 r5:c05e6abc r4:c0624dec
[<c02657c4>] (driver_register) from [<c0266728>] (__platform_driver_register+0x50/0x64)
 r5:c05e6abc r4:ee7eec40
[<c02666d8>] (__platform_driver_register) from [<c05e6ad4>] (rsnd_driver_init+0x18/0x20)
[<c05e6abc>] (rsnd_driver_init) from [<c000a7d8>] (do_one_initcall+0x108/0x1bc)
[<c000a6d0>] (do_one_initcall) from [<c05c7dc8>] (kernel_init_freeable+0x11c/0x1e4)
 r9:c062f000 r8:c062f000 r7:c05fb160 r6:c05f3a40 r5:00000084 r4:00000006
[<c05c7cac>] (kernel_init_freeable) from [<c0445120>] (kernel_init+0x10/0xec)
 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0445110 r4:00000000
[<c0445110>] (kernel_init) from [<c000fef0>] (ret_from_fork+0x14/0x24)
 r4:00000000 r3:00000000

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds
Kuninori Morimoto May 11, 2015, 2:17 a.m. UTC | #2 
Hi Geert

> >  static int rsnd_rdai_continuance_probe(struct rsnd_priv *priv,
> >  				       struct rsnd_dai_stream *io)
> >  {
> > +	unsigned long flags;
> >  	int ret;
> >  
> > +	rsnd_lock(priv, flags);
> > +
> >  	ret = rsnd_dai_call(probe, io, priv);
> >  	if (ret == -EAGAIN) {
> >  		/*
> 
> This causes the following warning on r8a7791/koelsch:

Thank you for pointing this.
I will check/fixup it.

> 
> WARNING: CPU: 1 PID: 1 at kernel/locking/lockdep.c:2755 lockdep_trace_alloc+0xd4/0x118()
> DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
> Modules linked in:
> 
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.1.0-rc2-koelsch-00564-g1817f2746e13e08e #1081
> Hardware name: Generic R8A7791 (Flattened Device Tree)
> Backtrace: 
> [<c00135e0>] (dump_backtrace) from [<c0013800>] (show_stack+0x18/0x1c)
>  r6:c0546ece r5:00000009 r4:00000000 r3:00204140
> [<c00137e8>] (show_stack) from [<c04483ec>] (dump_stack+0x78/0x94)
> [<c0448374>] (dump_stack) from [<c002d368>] (warn_slowpath_common+0x90/0xbc)
>  r4:eec59c90 r3:00000001
> [<c002d2d8>] (warn_slowpath_common) from [<c002d3cc>] (warn_slowpath_fmt+0x38/0x40)
>  r8:eed90010 r7:000001a1 r6:000080d0 r5:000080d0 r4:60000193
> [<c002d398>] (warn_slowpath_fmt) from [<c0069e94>] (lockdep_trace_alloc+0xd4/0x118)
>  r3:c054c535 r2:c0547435
> [<c0069dc0>] (lockdep_trace_alloc) from [<c00e3acc>] (__kmalloc_track_caller+0x34/0x11c)
>  r5:eec000c0 r4:00000000
> [<c00e3a98>] (__kmalloc_track_caller) from [<c02678b4>] (devres_alloc+0x20/0x48)
>  r7:000001a1 r6:ee7e2138 r5:c007a3d8 r4:00000000
> [<c0267894>] (devres_alloc) from [<c007a378>] (devm_request_threaded_irq+0x34/0x94)
>  r5:ee7ea8d0 r4:00000000
> [<c007a344>] (devm_request_threaded_irq) from [<c036ad4c>] (rsnd_src_probe_gen2+0x68/0x7c)
>  r9:eef61120 r8:a0000113 r7:ee7ea8d8 r6:eef6113c r5:ee7ea8d0 r4:ee7e2138
> [<c036ace4>] (rsnd_src_probe_gen2) from [<c0367e1c>] (rsnd_rdai_continuance_probe+0x80/0x29c)
>  r5:eef61130 r4:c036ace4
> [<c0367d9c>] (rsnd_rdai_continuance_probe) from [<c0368a80>] (rsnd_probe+0x114/0x304)
>  r10:eef61110 r9:00000000 r8:ee7ea8d8 r7:eed90000 r6:eed90010 r5:ee7ea8d0
>  r4:c048e960
> [<c036896c>] (rsnd_probe) from [<c02667f4>] (platform_drv_probe+0x50/0xa0)
>  r10:00000000 r9:c062f000 r8:c0624dec r7:00000000 r6:c0624dec r5:eed90010
>  r4:fffffffe
> [<c02667a4>] (platform_drv_probe) from [<c0264fac>] (driver_probe_device+0xfc/0x264)
>  r6:00000000 r5:c0e1e9fc r4:eed90010 r3:c02667a4
> [<c0264eb0>] (driver_probe_device) from [<c02651d8>] (__driver_attach+0x78/0x9c)
>  r8:c0606460 r7:c0619e98 r6:c0624dec r5:eed90044 r4:eed90010 r3:00000000
> [<c0265160>] (__driver_attach) from [<c02635c8>] (bus_for_each_dev+0x74/0x98)
>  r6:c0265160 r5:c0624dec r4:00000000 r3:00000001
> [<c0263554>] (bus_for_each_dev) from [<c0264a98>] (driver_attach+0x20/0x28)
>  r6:ee7ea9c0 r5:00000000 r4:c0624dec
> [<c0264a78>] (driver_attach) from [<c0264728>] (bus_add_driver+0xe8/0x1d0)
> [<c0264640>] (bus_add_driver) from [<c0265868>] (driver_register+0xa4/0xe8)
>  r7:c0606460 r6:00000000 r5:c05e6abc r4:c0624dec
> [<c02657c4>] (driver_register) from [<c0266728>] (__platform_driver_register+0x50/0x64)
>  r5:c05e6abc r4:ee7eec40
> [<c02666d8>] (__platform_driver_register) from [<c05e6ad4>] (rsnd_driver_init+0x18/0x20)
> [<c05e6abc>] (rsnd_driver_init) from [<c000a7d8>] (do_one_initcall+0x108/0x1bc)
> [<c000a6d0>] (do_one_initcall) from [<c05c7dc8>] (kernel_init_freeable+0x11c/0x1e4)
>  r9:c062f000 r8:c062f000 r7:c05fb160 r6:c05f3a40 r5:00000084 r4:00000006
> [<c05c7cac>] (kernel_init_freeable) from [<c0445120>] (kernel_init+0x10/0xec)
>  r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0445110 r4:00000000
> [<c0445110>] (kernel_init) from [<c000fef0>] (ret_from_fork+0x14/0x24)
>  r4:00000000 r3:00000000
> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> 							    -- Linus Torvalds
Kuninori Morimoto May 11, 2015, 8:44 a.m. UTC | #3 
Hi Mark, Geert

> > >  static int rsnd_rdai_continuance_probe(struct rsnd_priv *priv,
> > >  				       struct rsnd_dai_stream *io)
> > >  {
> > > +	unsigned long flags;
> > >  	int ret;
> > >  
> > > +	rsnd_lock(priv, flags);
> > > +
> > >  	ret = rsnd_dai_call(probe, io, priv);
> > >  	if (ret == -EAGAIN) {
> > >  		/*
> > 
> > This causes the following warning on r8a7791/koelsch:
> 
> Thank you for pointing this.
> I will check/fixup it.

I checked about this issue.
Indeed these locks are not needed.
# 2 lock have issue when boot,
# 2 lock has issue when unbind/rmmod/probe fail
# 1 lock is not needed

Mark, is it possible to revert this patch ?

e9c390df671fadc829550935ffb6b23549f26ded
(ASoC: rsnd: make sure it uses lock when it calls rsnd_dai_call)

Best regards
---
Kuninori Morimoto
Mark Brown May 11, 2015, 2:57 p.m. UTC | #4 
On Mon, May 11, 2015 at 05:44:14PM +0900, Kuninori Morimoto wrote:

> Mark, is it possible to revert this patch ?

> e9c390df671fadc829550935ffb6b23549f26ded
> (ASoC: rsnd: make sure it uses lock when it calls rsnd_dai_call)

Sure, please send a patch for that with a changelog explaining why the
revert is being done.
Kuninori Morimoto May 12, 2015, 12:09 a.m. UTC | #5 
Hi Mark

> > Mark, is it possible to revert this patch ?
> 
> > e9c390df671fadc829550935ffb6b23549f26ded
> > (ASoC: rsnd: make sure it uses lock when it calls rsnd_dai_call)
> 
> Sure, please send a patch for that with a changelog explaining why the
> revert is being done.

OK, I will send it.
diff mbox

Patch

diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index a2a0b57..164653c 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -731,10 +731,15 @@  static int rsnd_hw_params(struct snd_pcm_substream *substream,
 {
 	struct snd_soc_dai *dai = rsnd_substream_to_dai(substream);
 	struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai);
+	struct rsnd_priv *priv = rsnd_dai_to_priv(dai);
 	struct rsnd_dai_stream *io = rsnd_rdai_to_io(rdai, substream);
+	unsigned long flags;
 	int ret;
 
+	rsnd_lock(priv, flags);
 	ret = rsnd_dai_call(hw_params, io, substream, hw_params);
+	rsnd_unlock(priv, flags);
+
 	if (ret)
 		return ret;
 
@@ -919,14 +924,16 @@  int rsnd_kctrl_new_e(struct rsnd_mod *mod,
 static int rsnd_pcm_new(struct snd_soc_pcm_runtime *rtd)
 {
 	struct snd_soc_dai *dai = rtd->cpu_dai;
+	struct rsnd_priv *priv = rsnd_dai_to_priv(dai);
 	struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai);
-	int ret;
+	unsigned long flags;
+	int ret = 0;
 
-	ret = rsnd_dai_call(pcm_new, &rdai->playback, rtd);
-	if (ret)
-		return ret;
+	rsnd_lock(priv, flags);
+	ret |= rsnd_dai_call(pcm_new, &rdai->playback, rtd);
+	ret |= rsnd_dai_call(pcm_new, &rdai->capture, rtd);
+	rsnd_unlock(priv, flags);
 
-	ret = rsnd_dai_call(pcm_new, &rdai->capture, rtd);
 	if (ret)
 		return ret;
 
@@ -949,8 +956,11 @@  static const struct snd_soc_component_driver rsnd_soc_component = {
 static int rsnd_rdai_continuance_probe(struct rsnd_priv *priv,
 				       struct rsnd_dai_stream *io)
 {
+	unsigned long flags;
 	int ret;
 
+	rsnd_lock(priv, flags);
+
 	ret = rsnd_dai_call(probe, io, priv);
 	if (ret == -EAGAIN) {
 		/*
@@ -983,6 +993,7 @@  static int rsnd_rdai_continuance_probe(struct rsnd_priv *priv,
 		 */
 		ret = rsnd_dai_call(probe, io, priv);
 	}
+	rsnd_unlock(priv, flags);
 
 	return ret;
 }
@@ -998,6 +1009,7 @@  static int rsnd_probe(struct platform_device *pdev)
 	struct rsnd_dai *rdai;
 	const struct of_device_id *of_id = of_match_device(rsnd_of_match, dev);
 	const struct rsnd_of_data *of_data;
+	unsigned long flags;
 	int (*probe_func[])(struct platform_device *pdev,
 			    const struct rsnd_of_data *of_data,
 			    struct rsnd_priv *priv) = {
@@ -1084,10 +1096,12 @@  static int rsnd_probe(struct platform_device *pdev)
 exit_snd_soc:
 	snd_soc_unregister_platform(dev);
 exit_snd_probe:
+	rsnd_lock(priv, flags);
 	for_each_rsnd_dai(rdai, priv, i) {
 		rsnd_dai_call(remove, &rdai->playback, priv);
 		rsnd_dai_call(remove, &rdai->capture, priv);
 	}
+	rsnd_unlock(priv, flags);
 
 	return ret;
 }
@@ -1096,6 +1110,7 @@  static int rsnd_remove(struct platform_device *pdev)
 {
 	struct rsnd_priv *priv = dev_get_drvdata(&pdev->dev);
 	struct rsnd_dai *rdai;
+	unsigned long flags;
 	void (*remove_func[])(struct platform_device *pdev,
 			      struct rsnd_priv *priv) = {
 		rsnd_ssi_remove,
@@ -1106,10 +1121,12 @@  static int rsnd_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 
+	rsnd_lock(priv, flags);
 	for_each_rsnd_dai(rdai, priv, i) {
 		ret |= rsnd_dai_call(remove, &rdai->playback, priv);
 		ret |= rsnd_dai_call(remove, &rdai->capture, priv);
 	}
+	rsnd_unlock(priv, flags);
 
 	for (i = 0; i < ARRAY_SIZE(remove_func); i++)
 		remove_func[i](pdev, priv);