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

[Mark Brown]

On Thu, Mar 19, 2015 at 04:15:01AM +0000, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150322/bd9ce763/attachment.sig>