[PATCH][RFC] ASoC: rsnd: don't call clk_disable_unprepare() if can't use

[Geert Uytterhoeven]

Hi Morimoto-san,

On Tue, Dec 15, 2020 at 1:06 AM Kuninori Morimoto
<kuninori.morimoto.gx at renesas.com> wrote:
> We need to care clock accessibility,
> because we might can't use clock for some reasons.
>
> It sets clk_rate for each clocks when enabled.
> This means it doesn't have clk_rate if we can't use.
> We can avoid to call clk_disable_unprepare() in such case.
>
> Reported-by: Geert Uytterhoeven <geert at linux-m68k.org>

Feel free to use geert+renesas at glider.be instead ;-)

> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx at renesas.com>
> ---
>
> Hi Geert.
>
> Thank you for your reporting.
> I have never seen this kind of error, but it possible to happen.
> Unfortunately, I can't reproduce this but I hope this patch can solve it.
> Could you please check this ?
> I added [RFC] on this patch Subject.

The patch looks good to me, but I also cannot trigger the issue at will.
I went through my old boot logs, and found 2 other occurrences, also
on Ebisu.  In all cases, it happened while a lot of output was printed to
the serial console (either a WARN() splat, or DEBUG_PINCTRL output).
My guess is that console output or disabling interrupts too long is
triggering a race condition or other issue in the i2c driver (clk 1 is the
cs2000 clock generator, controlled through i2c).

> --- a/sound/soc/sh/rcar/adg.c
> +++ b/sound/soc/sh/rcar/adg.c
> @@ -366,25 +366,25 @@ void rsnd_adg_clk_control(struct rsnd_priv *priv, int enable)
>         struct rsnd_adg *adg = rsnd_priv_to_adg(priv);
>         struct device *dev = rsnd_priv_to_dev(priv);
>         struct clk *clk;
> -       int i, ret;
> +       int i;
>
>         for_each_rsnd_clk(clk, adg, i) {
> -               ret = 0;
>                 if (enable) {
> -                       ret = clk_prepare_enable(clk);
> +                       int ret = clk_prepare_enable(clk);
>
>                         /*
>                          * We shouldn't use clk_get_rate() under
>                          * atomic context. Let's keep it when
>                          * rsnd_adg_clk_enable() was called
>                          */
> -                       adg->clk_rate[i] = clk_get_rate(adg->clk[i]);
> +                       if (ret < 0)
> +                               dev_warn(dev, "can't use clk %d\n", i);
> +                       else
> +                               adg->clk_rate[i] = clk_get_rate(adg->clk[i]);
>                 } else {
> -                       clk_disable_unprepare(clk);
> +                       if (adg->clk_rate[i])
> +                               clk_disable_unprepare(clk);

As pointed out by Mark, you may want to clear adg->clk_rate[i] here?

>                 }
> -
> -               if (ret < 0)
> -                       dev_warn(dev, "can't use clk %d\n", i);
>         }
>  }

With the above sorted out:
Reviewed-by: Geert Uytterhoeven <geert+renesas at glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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