diff mbox

[08/10,v3] ASoC: rsnd: don't call clk_prepare_enable/unprepare() from inside spin lock

Message ID 87bnjpei8l.wl%kuninori.morimoto.gx@renesas.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kuninori Morimoto March 19, 2015, 4:15 a.m. UTC 
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

clk_prepare_enable/unprepare() uses mutex inside, and it uses __schedule().
Then, raw_spin_lock/unlock_irq() is called, and it breaks Renesas
sound driver's spin lock irq.
This patch moves clk_prepare_enable/unprepare to out of spin lock area.
Special thanks to Das Biju.

Reported-by: Das Biju <biju.das@bp.renesas.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v2 -> v3

 - no change

 sound/soc/sh/rcar/core.c | 29 ++++++++++++++++++++++++++++-
 sound/soc/sh/rcar/dvc.c  | 12 +-----------
 sound/soc/sh/rcar/rsnd.h |  9 +++++++--
 sound/soc/sh/rcar/src.c  |  6 ++----
 sound/soc/sh/rcar/ssi.c  | 22 ++++++++++++++++++----
 5 files changed, 56 insertions(+), 22 deletions(-)

Comments

Mark Brown March 22, 2015, 6:20 p.m. UTC | #1 
On Thu, Mar 19, 2015 at 04:15:01AM +0000, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> clk_prepare_enable/unprepare() uses mutex inside, and it uses __schedule().
> Then, raw_spin_lock/unlock_irq() is called, and it breaks Renesas
> sound driver's spin lock irq.
> This patch moves clk_prepare_enable/unprepare to out of spin lock area.
> Special thanks to Das Biju.

This is definitely a bug which should be fixed but this means we should
send a fix to Linus rather than having something that depends on -next.
Also...

> +int rsnd_mod_clk(struct rsnd_mod *mod, int enable)
> +{
> +	struct rsnd_priv *priv = rsnd_mod_to_priv(mod);
> +	struct device *dev = rsnd_priv_to_dev(priv);
> +
> +	/*
> +	 * clk_prepare_enable/unprepare() should not be called
> +	 * from inside spin lock
> +	 */
> +	if (enable)
> +		clk_prepare_enable(mod->clk);
> +	else
> +		clk_disable_unprepare(mod->clk);

It's not entirely clear what this wrapper function is doing (and note
that we don't check the error code from clk_prepare_enable()).

> @@ -345,6 +366,9 @@ static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  	int ret;
>  	unsigned long flags;
>  
> +	if (cmd == SNDRV_PCM_TRIGGER_START)
> +		rsnd_dai_call(clk, io, 1);
> +
>  	rsnd_lock(priv, flags);

The trigger function is called from atomic context so this seems to
leave us with the same problem this was supposed to be fixing?

> +static int rsnd_ssi_clk(struct rsnd_mod *mod, int enable)
> +{
> +	struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
> +	struct rsnd_dai_stream *io = rsnd_mod_to_io(mod);
> +	struct rsnd_dai *rdai = rsnd_io_to_rdai(io);
> +
> +	rsnd_mod_clk(mod, enable);
> +
> +	if (rsnd_rdai_is_clk_master(rdai)) {
> +		if (rsnd_ssi_clk_from_parent(ssi))
> +			rsnd_mod_clk(&ssi->parent->mod, enable);
> +	}

This would be clearer with a single if statement with an &&, however it
seems like something that the clock API should be doing itself - it's
already got support for handling enabling of parents.  If we need to
manually handle dependencies (which is what this looks like) that seems
like a problem.
Kuninori Morimoto March 23, 2015, 8:07 a.m. UTC | #2 
Hi Mark

> > +static int rsnd_ssi_clk(struct rsnd_mod *mod, int enable)
> > +{
> > +	struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
> > +	struct rsnd_dai_stream *io = rsnd_mod_to_io(mod);
> > +	struct rsnd_dai *rdai = rsnd_io_to_rdai(io);
> > +
> > +	rsnd_mod_clk(mod, enable);
> > +
> > +	if (rsnd_rdai_is_clk_master(rdai)) {
> > +		if (rsnd_ssi_clk_from_parent(ssi))
> > +			rsnd_mod_clk(&ssi->parent->mod, enable);
> > +	}
> 
> This would be clearer with a single if statement with an &&, however it
> seems like something that the clock API should be doing itself - it's
> already got support for handling enabling of parents.  If we need to
> manually handle dependencies (which is what this looks like) that seems
> like a problem.

Hmm... indeed.
Thank you for your feedback, I will reconsider about this
Kuninori Morimoto March 23, 2015, 9:03 a.m. UTC | #3 
Hi Mark again

> > +static int rsnd_ssi_clk(struct rsnd_mod *mod, int enable)
> > +{
> > +	struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
> > +	struct rsnd_dai_stream *io = rsnd_mod_to_io(mod);
> > +	struct rsnd_dai *rdai = rsnd_io_to_rdai(io);
> > +
> > +	rsnd_mod_clk(mod, enable);
> > +
> > +	if (rsnd_rdai_is_clk_master(rdai)) {
> > +		if (rsnd_ssi_clk_from_parent(ssi))
> > +			rsnd_mod_clk(&ssi->parent->mod, enable);
> > +	}
> 
> This would be clearer with a single if statement with an &&, however it
> seems like something that the clock API should be doing itself - it's
> already got support for handling enabling of parents.  If we need to
> manually handle dependencies (which is what this looks like) that seems
> like a problem.

I rechecked this.
In our sound device, sound LR and clock pin is controled by PFC (= pin function controller)
and, these pins are shared with other device. it depends on platform/board.
If other device uses this pin, we can't use it in sound device.
Then, this sound device share LR/clock pin in sound device.
And then, device clock has no relation to this

The image is like this

If no other devices use pin, sound device can use all pins

SSI0 deivce clock
 SSI0 LR  ----------------|------
 SSI0 clk ----------------|------
                          |
SSI1 deivce clock         |
 SSI1 LR  ----------------|------
 SSI1 clk ----------------|------
 
If other device uses pin, sound device share same pins

SSI0 deivce clock
 SSI0 LR  ---+------------|------
 SSI0 clk ---|+-----------|------
             ||           |
SSI1 deivce clock         |
 SSI1 LR  ---+|   Dev A --|------
 SSI1 clk ----+   Dev B --|------

Here, SSI1 needs SSI0's LR/clk pin, but it doesn't need SSI0's device clock
because SSI1 has SSI1 device clock.
SSI0 is called as parent for SSI1 in my driver, but it is not "clock parent".
And here, if you want to use SSI1, you need to start SSI0 too.
So Unfortunately, manually handle dependencies is understandable IMO.
diff mbox

Patch

diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index 54042d9..e6c1c7a4 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -190,7 +190,7 @@  u32 rsnd_get_adinr(struct rsnd_mod *mod)
 ({								\
 	struct rsnd_priv *priv = rsnd_mod_to_priv(mod);		\
 	struct device *dev = rsnd_priv_to_dev(priv);		\
-	u32 mask = 1 << __rsnd_mod_shift_##func;			\
+	u32 mask = (1 << __rsnd_mod_shift_##func) & ~(1 << 31);		\
 	u32 call = __rsnd_mod_call_##func << __rsnd_mod_shift_##func;	\
 	int ret = 0;							\
 	if ((mod->status & mask) == call) {				\
@@ -222,6 +222,27 @@  u32 rsnd_get_adinr(struct rsnd_mod *mod)
 	ret;							\
 })
 
+int rsnd_mod_clk(struct rsnd_mod *mod, int enable)
+{
+	struct rsnd_priv *priv = rsnd_mod_to_priv(mod);
+	struct device *dev = rsnd_priv_to_dev(priv);
+
+	/*
+	 * clk_prepare_enable/unprepare() should not be called
+	 * from inside spin lock
+	 */
+	if (enable)
+		clk_prepare_enable(mod->clk);
+	else
+		clk_disable_unprepare(mod->clk);
+
+	dev_dbg(dev, "%s[%d] clk %s\n",
+		rsnd_mod_name(mod), rsnd_mod_id(mod),
+		enable ? "ON" : "OFF");
+
+	return 0;
+}
+
 static int rsnd_dai_connect(struct rsnd_mod *mod,
 			    struct rsnd_dai_stream *io)
 {
@@ -345,6 +366,9 @@  static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 	int ret;
 	unsigned long flags;
 
+	if (cmd == SNDRV_PCM_TRIGGER_START)
+		rsnd_dai_call(clk, io, 1);
+
 	rsnd_lock(priv, flags);
 
 	switch (cmd) {
@@ -385,6 +409,9 @@  static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 dai_trigger_end:
 	rsnd_unlock(priv, flags);
 
+	if (cmd == SNDRV_PCM_TRIGGER_STOP)
+		rsnd_dai_call(clk, io, 0);
+
 	return ret;
 }
 
diff --git a/sound/soc/sh/rcar/dvc.c b/sound/soc/sh/rcar/dvc.c
index aeeef13..9420349 100644
--- a/sound/soc/sh/rcar/dvc.c
+++ b/sound/soc/sh/rcar/dvc.c
@@ -166,8 +166,6 @@  static int rsnd_dvc_init(struct rsnd_mod *dvc_mod,
 		return -EINVAL;
 	}
 
-	rsnd_mod_hw_start(dvc_mod);
-
 	/*
 	 * fixme
 	 * it doesn't support CTU/MIX
@@ -191,14 +189,6 @@  static int rsnd_dvc_init(struct rsnd_mod *dvc_mod,
 	return 0;
 }
 
-static int rsnd_dvc_quit(struct rsnd_mod *mod,
-			 struct rsnd_priv *priv)
-{
-	rsnd_mod_hw_stop(mod);
-
-	return 0;
-}
-
 static int rsnd_dvc_start(struct rsnd_mod *mod,
 			  struct rsnd_priv *priv)
 {
@@ -286,9 +276,9 @@  static struct rsnd_mod_ops rsnd_dvc_ops = {
 	.probe		= rsnd_dvc_probe_gen2,
 	.remove		= rsnd_dvc_remove_gen2,
 	.init		= rsnd_dvc_init,
-	.quit		= rsnd_dvc_quit,
 	.start		= rsnd_dvc_start,
 	.stop		= rsnd_dvc_stop,
+	.clk		= rsnd_mod_clk,
 	.pcm_new	= rsnd_dvc_pcm_new,
 };
 
diff --git a/sound/soc/sh/rcar/rsnd.h b/sound/soc/sh/rcar/rsnd.h
index 52c401c..40a12f4 100644
--- a/sound/soc/sh/rcar/rsnd.h
+++ b/sound/soc/sh/rcar/rsnd.h
@@ -237,6 +237,7 @@  struct rsnd_mod_ops {
 		     struct rsnd_priv *priv);
 	int (*stop)(struct rsnd_mod *mod,
 		    struct rsnd_priv *priv);
+	int (*clk)(struct rsnd_mod *mod, int enable);
 	int (*pcm_new)(struct rsnd_mod *mod,
 		       struct snd_soc_pcm_runtime *rtd);
 	int (*fallback)(struct rsnd_mod *mod,
@@ -262,6 +263,9 @@  struct rsnd_mod {
  * 2	0: start	1: stop
  * 3	0: pcm_new
  * 4	0: fallback
+ *
+ * 31 bit is always called (see __rsnd_mod_call)
+ * 31	0: clk
  */
 #define __rsnd_mod_shift_probe		0
 #define __rsnd_mod_shift_remove		0
@@ -271,6 +275,7 @@  struct rsnd_mod {
 #define __rsnd_mod_shift_stop		2
 #define __rsnd_mod_shift_pcm_new	3
 #define __rsnd_mod_shift_fallback	4
+#define __rsnd_mod_shift_clk		31 /* always called */
 
 #define __rsnd_mod_call_probe		0
 #define __rsnd_mod_call_remove		1
@@ -280,13 +285,12 @@  struct rsnd_mod {
 #define __rsnd_mod_call_stop		1
 #define __rsnd_mod_call_pcm_new		0
 #define __rsnd_mod_call_fallback	0
+#define __rsnd_mod_call_clk		0
 
 #define rsnd_mod_to_priv(mod) (rsnd_io_to_priv(rsnd_mod_to_io(mod)))
 #define rsnd_mod_to_dma(mod) (&(mod)->dma)
 #define rsnd_mod_to_io(mod) ((mod)->io)
 #define rsnd_mod_id(mod) ((mod)->id)
-#define rsnd_mod_hw_start(mod)	clk_prepare_enable((mod)->clk)
-#define rsnd_mod_hw_stop(mod)	clk_disable_unprepare((mod)->clk)
 
 void rsnd_mod_init(struct rsnd_mod *mod,
 		   struct rsnd_mod_ops *ops,
@@ -294,6 +298,7 @@  void rsnd_mod_init(struct rsnd_mod *mod,
 		   enum rsnd_mod_type type,
 		   int id);
 char *rsnd_mod_name(struct rsnd_mod *mod);
+int rsnd_mod_clk(struct rsnd_mod *mod, int enable);
 struct dma_chan *rsnd_mod_dma_req(struct rsnd_mod *mod);
 
 /*
diff --git a/sound/soc/sh/rcar/src.c b/sound/soc/sh/rcar/src.c
index cc93f32..83032ee 100644
--- a/sound/soc/sh/rcar/src.c
+++ b/sound/soc/sh/rcar/src.c
@@ -292,8 +292,6 @@  static int rsnd_src_init(struct rsnd_mod *mod)
 {
 	struct rsnd_src *src = rsnd_mod_to_src(mod);
 
-	rsnd_mod_hw_start(mod);
-
 	src->err = 0;
 
 	/*
@@ -311,8 +309,6 @@  static int rsnd_src_quit(struct rsnd_mod *mod,
 	struct rsnd_src *src = rsnd_mod_to_src(mod);
 	struct device *dev = rsnd_priv_to_dev(priv);
 
-	rsnd_mod_hw_stop(mod);
-
 	if (src->err)
 		dev_warn(dev, "%s[%d] under/over flow err = %d\n",
 			 rsnd_mod_name(mod), rsnd_mod_id(mod), src->err);
@@ -523,6 +519,7 @@  static struct rsnd_mod_ops rsnd_src_gen1_ops = {
 	.quit	= rsnd_src_quit,
 	.start	= rsnd_src_start_gen1,
 	.stop	= rsnd_src_stop_gen1,
+	.clk	= rsnd_mod_clk,
 };
 
 /*
@@ -803,6 +800,7 @@  static struct rsnd_mod_ops rsnd_src_gen2_ops = {
 	.quit	= rsnd_src_quit,
 	.start	= rsnd_src_start_gen2,
 	.stop	= rsnd_src_stop_gen2,
+	.clk	= rsnd_mod_clk,
 };
 
 struct rsnd_mod *rsnd_src_mod_get(struct rsnd_priv *priv, int id)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
index 060d3d2..583ca97 100644
--- a/sound/soc/sh/rcar/ssi.c
+++ b/sound/soc/sh/rcar/ssi.c
@@ -176,6 +176,22 @@  static void rsnd_ssi_master_clk_stop(struct rsnd_ssi *ssi)
 	rsnd_adg_ssi_clk_stop(&ssi->mod);
 }
 
+static int rsnd_ssi_clk(struct rsnd_mod *mod, int enable)
+{
+	struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
+	struct rsnd_dai_stream *io = rsnd_mod_to_io(mod);
+	struct rsnd_dai *rdai = rsnd_io_to_rdai(io);
+
+	rsnd_mod_clk(mod, enable);
+
+	if (rsnd_rdai_is_clk_master(rdai)) {
+		if (rsnd_ssi_clk_from_parent(ssi))
+			rsnd_mod_clk(&ssi->parent->mod, enable);
+	}
+
+	return 0;
+}
+
 static void rsnd_ssi_hw_start(struct rsnd_ssi *ssi,
 			      struct rsnd_dai_stream *io)
 {
@@ -186,8 +202,6 @@  static void rsnd_ssi_hw_start(struct rsnd_ssi *ssi,
 	u32 cr;
 
 	if (0 == ssi->usrcnt) {
-		rsnd_mod_hw_start(&ssi->mod);
-
 		if (rsnd_rdai_is_clk_master(rdai)) {
 			if (rsnd_ssi_clk_from_parent(ssi))
 				rsnd_ssi_hw_start(ssi->parent, io);
@@ -258,8 +272,6 @@  static void rsnd_ssi_hw_stop(struct rsnd_ssi *ssi)
 			else
 				rsnd_ssi_master_clk_stop(ssi);
 		}
-
-		rsnd_mod_hw_stop(&ssi->mod);
 	}
 
 	dev_dbg(dev, "%s[%d] hw stopped\n",
@@ -462,6 +474,7 @@  static struct rsnd_mod_ops rsnd_ssi_pio_ops = {
 	.quit	= rsnd_ssi_quit,
 	.start	= rsnd_ssi_start,
 	.stop	= rsnd_ssi_stop,
+	.clk	= rsnd_ssi_clk,
 };
 
 static int rsnd_ssi_dma_probe(struct rsnd_mod *mod,
@@ -581,6 +594,7 @@  static struct rsnd_mod_ops rsnd_ssi_dma_ops = {
 	.quit	= rsnd_ssi_quit,
 	.start	= rsnd_ssi_dma_start,
 	.stop	= rsnd_ssi_dma_stop,
+	.clk	= rsnd_ssi_clk,
 	.fallback = rsnd_ssi_fallback,
 };