[12/13] ASoC: rsnd: fixup not to call clk_get/set under non-atomic

Commit Message

Kuninori Morimoto Sept. 3, 2018, 7:08 a.m. UTC

From: Jiada Wang <[e-mail]>

Clocking operations clk_get/set_rate, are non-atomic,
they shouldn't be called in soc_pcm_trigger() which is atomic.

Following issue was found due to execution of clk_get_rate() causes
sleep in soc_pcm_trigger(), which shouldn't be blocked.

We can reproduce this issue by following
	> enable CONFIG_DEBUG_ATOMIC_SLEEP=y
	> compile, and boot
	> mount -t debugfs none /sys/kernel/debug
	> while true; do cat /sys/kernel/debug/clk/clk_summary > /dev/null; done &
	> while true; do aplay xxx; done

This patch adds support to .prepare callback, and moves non-atomic
clocking operations to it. As .prepare is non-atomic, it is always
called before trigger_start/trigger_stop.

	BUG: sleeping function called from invalid context at kernel/locking/mutex.c:620
	in_atomic(): 1, irqs_disabled(): 128, pid: 2242, name: aplay
	INFO: lockdep is turned off.
	irq event stamp: 5964
	hardirqs last enabled at (5963): [<ffff200008e59e40>] mutex_lock_nested+0x6e8/0x6f0
	hardirqs last disabled at (5964): [<ffff200008e623f0>] _raw_spin_lock_irqsave+0x24/0x68
	softirqs last enabled at (5502): [<ffff200008081838>] __do_softirq+0x560/0x10c0
	softirqs last disabled at (5495): [<ffff2000080c2e78>] irq_exit+0x160/0x25c
	Preemption disabled at:[ 62.904063] [<ffff200008be4d48>] snd_pcm_stream_lock+0xb4/0xc0
	CPU: 2 PID: 2242 Comm: aplay Tainted: G B C 4.9.54+ #186
	Hardware name: Renesas Salvator-X board based on r8a7795 (DT)
	Call trace:
	[<ffff20000808fe48>] dump_backtrace+0x0/0x37c
	[<ffff2000080901d8>] show_stack+0x14/0x1c
	[<ffff2000086f4458>] dump_stack+0xfc/0x154
	[<ffff2000081134a0>] ___might_sleep+0x57c/0x58c
	[<ffff2000081136b8>] __might_sleep+0x208/0x21c
	[<ffff200008e5980c>] mutex_lock_nested+0xb4/0x6f0
	[<ffff2000087cac74>] clk_prepare_lock+0xb0/0x184
	[<ffff2000087cb094>] clk_core_get_rate+0x14/0x54
	[<ffff2000087cb0f4>] clk_get_rate+0x20/0x34
	[<ffff20000113aa00>] rsnd_adg_ssi_clk_try_start+0x158/0x4f8 [snd_soc_rcar]
	[<ffff20000113da00>] rsnd_ssi_init+0x668/0x7a0 [snd_soc_rcar]
	[<ffff200001133ff4>] rsnd_soc_dai_trigger+0x4bc/0xcf8 [snd_soc_rcar]
	[<ffff200008c1af24>] soc_pcm_trigger+0x2a4/0x2d4

Fixes: e7d850dd10f4 ("ASoC: rsnd: use mod base common method on SSI-parent")
Signed-off-by: Jiada Wang <[e-mail]>
Signed-off-by: Timo Wischer <[e-mail]>
[Kuninori: tidyup for upstream]
Signed-off-by: Kuninori Morimoto <[e-mail]>
Tested-by: Hiroyuki Yokoyama <[e-mail]>
---
 sound/soc/sh/rcar/core.c | 11 +++++++++++
 sound/soc/sh/rcar/rsnd.h |  7 +++++++
 sound/soc/sh/rcar/ssi.c  | 16 ++++++++++------
 3 files changed, 28 insertions(+), 6 deletions(-)

Comments

Mark Brown Sept. 3, 2018, 1:36 p.m. UTC | #1

On Mon, Sep 03, 2018 at 07:08:58AM +0000, Kuninori Morimoto wrote:
> From: Jiada Wang <[e-mail]>
> 
> Clocking operations clk_get/set_rate, are non-atomic,
> they shouldn't be called in soc_pcm_trigger() which is atomic.

Generally bugfix patches like this should go at the start of the series
to make sure they don't have any dependencies on other patches and can
be applied separately and sent to Linus before the merge window.

Kuninori Morimoto Sept. 4, 2018, 12:15 a.m. UTC | #2

Hi Mark

> > Clocking operations clk_get/set_rate, are non-atomic,
> > they shouldn't be called in soc_pcm_trigger() which is atomic.
> 
> Generally bugfix patches like this should go at the start of the series
> to make sure they don't have any dependencies on other patches and can
> be applied separately and sent to Linus before the merge window.

Ahh, yes, indeed.
I'm sorry about that.

Best regards
---
Kuninori Morimoto

Patch

diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index c66b3da..299fb45 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -955,12 +955,23 @@  static void rsnd_soc_dai_shutdown(struct snd_pcm_substream *substream,
 	rsnd_dai_stream_quit(io);
 }
 
+static int rsnd_soc_dai_prepare(struct snd_pcm_substream *substream,
+				struct snd_soc_dai *dai)
+{
+	struct rsnd_priv *priv = rsnd_dai_to_priv(dai);
+	struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai);
+	struct rsnd_dai_stream *io = rsnd_rdai_to_io(rdai, substream);
+
+	return rsnd_dai_call(prepare, io, priv);
+}
+
 static const struct snd_soc_dai_ops rsnd_soc_dai_ops = {
 	.startup	= rsnd_soc_dai_startup,
 	.shutdown	= rsnd_soc_dai_shutdown,
 	.trigger	= rsnd_soc_dai_trigger,
 	.set_fmt	= rsnd_soc_dai_set_fmt,
 	.set_tdm_slot	= rsnd_soc_set_dai_tdm_slot,
+	.prepare	= rsnd_soc_dai_prepare,
 };
 
 void rsnd_parse_connect_common(struct rsnd_dai *rdai,
diff --git a/sound/soc/sh/rcar/rsnd.h b/sound/soc/sh/rcar/rsnd.h
index 7ff58b0..7a04b19 100644
--- a/sound/soc/sh/rcar/rsnd.h
+++ b/sound/soc/sh/rcar/rsnd.h
@@ -301,6 +301,9 @@  struct rsnd_mod_ops {
 	int (*nolock_stop)(struct rsnd_mod *mod,
 		    struct rsnd_dai_stream *io,
 		    struct rsnd_priv *priv);
+	int (*prepare)(struct rsnd_mod *mod,
+		       struct rsnd_dai_stream *io,
+		       struct rsnd_priv *priv);
 };
 
 struct rsnd_dai_stream;
@@ -330,6 +333,7 @@  struct rsnd_mod {
  * H	0: fallback
  * H	0: hw_params
  * H	0: pointer
+ * H	0: prepare
  */
 #define __rsnd_mod_shift_nolock_start	0
 #define __rsnd_mod_shift_nolock_stop	0
@@ -344,6 +348,7 @@  struct rsnd_mod {
 #define __rsnd_mod_shift_fallback	28 /* always called */
 #define __rsnd_mod_shift_hw_params	28 /* always called */
 #define __rsnd_mod_shift_pointer	28 /* always called */
+#define __rsnd_mod_shift_prepare	28 /* always called */
 
 #define __rsnd_mod_add_probe		0
 #define __rsnd_mod_add_remove		0
@@ -358,6 +363,7 @@  struct rsnd_mod {
 #define __rsnd_mod_add_fallback		0
 #define __rsnd_mod_add_hw_params	0
 #define __rsnd_mod_add_pointer		0
+#define __rsnd_mod_add_prepare		0
 
 #define __rsnd_mod_call_probe		0
 #define __rsnd_mod_call_remove		0
@@ -372,6 +378,7 @@  struct rsnd_mod {
 #define __rsnd_mod_call_pointer		0
 #define __rsnd_mod_call_nolock_start	0
 #define __rsnd_mod_call_nolock_stop	1
+#define __rsnd_mod_call_prepare		0
 
 #define rsnd_mod_to_priv(mod)	((mod)->priv)
 #define rsnd_mod_name(mod)	((mod)->ops->name)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
index 85da4fc..3adcc4f 100644
--- a/sound/soc/sh/rcar/ssi.c
+++ b/sound/soc/sh/rcar/ssi.c
@@ -306,7 +306,7 @@  static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod,
 	if (rsnd_ssi_is_multi_slave(mod, io))
 		return 0;
 
-	if (ssi->usrcnt > 1) {
+	if (ssi->rate) {
 		if (ssi->rate != rate) {
 			dev_err(dev, "SSI parent/child should use same rate\n");
 			return -EINVAL;
@@ -464,7 +464,6 @@  static int rsnd_ssi_init(struct rsnd_mod *mod,
 			 struct rsnd_priv *priv)
 {
 	struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
-	int ret;
 
 	if (!rsnd_ssi_is_run_mods(mod, io))
 		return 0;
@@ -473,10 +472,6 @@  static int rsnd_ssi_init(struct rsnd_mod *mod,
 
 	rsnd_mod_power_on(mod);
 
-	ret = rsnd_ssi_master_clk_start(mod, io);
-	if (ret < 0)
-		return ret;
-
 	rsnd_ssi_config_init(mod, io);
 
 	rsnd_ssi_register_setup(mod);
@@ -872,6 +867,13 @@  static int rsnd_ssi_pio_pointer(struct rsnd_mod *mod,
 	return 0;
 }
 
+static int rsnd_ssi_prepare(struct rsnd_mod *mod,
+			    struct rsnd_dai_stream *io,
+			    struct rsnd_priv *priv)
+{
+	return rsnd_ssi_master_clk_start(mod, io);
+}
+
 static struct rsnd_mod_ops rsnd_ssi_pio_ops = {
 	.name	= SSI_NAME,
 	.probe	= rsnd_ssi_common_probe,
@@ -884,6 +886,7 @@  static struct rsnd_mod_ops rsnd_ssi_pio_ops = {
 	.pointer = rsnd_ssi_pio_pointer,
 	.pcm_new = rsnd_ssi_pcm_new,
 	.hw_params = rsnd_ssi_hw_params,
+	.prepare = rsnd_ssi_prepare,
 };
 
 static int rsnd_ssi_dma_probe(struct rsnd_mod *mod,
@@ -959,6 +962,7 @@  static struct rsnd_mod_ops rsnd_ssi_dma_ops = {
 	.pcm_new = rsnd_ssi_pcm_new,
 	.fallback = rsnd_ssi_fallback,
 	.hw_params = rsnd_ssi_hw_params,
+	.prepare = rsnd_ssi_prepare,
 };
 
 int rsnd_ssi_is_dma_mode(struct rsnd_mod *mod)