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

Hi Morimoto-san,

On Tue, Dec 15, 2020 at 1:06 AM Kuninori Morimoto kuninori.morimoto.gx@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@linux-m68k.org

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

Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@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@glider.be

Gr{oetje,eeting}s,

Geert