Re: rsnd_adg_clk_control() sometimes disables already-disabled clock

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Morimoto-san,

On Tue, Dec 17, 2024 at 3:38 AM Kuninori Morimoto
<kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
> >     ------------[ cut here ]------------
> >     clk_multiplier already disabled
> (snip)
> >     ------------[ cut here ]------------
> >     clk_multiplier already unprepared
> (snip)
> > Unfortunately I cannot reproduce it at will.
> > The above is from today's renesas-devel release, but my logs indicate
> > it happens every few months since at least v6.1.
> > So far I have seen it on all Salvator-X(S) variants, but not on any other
> > SoCs or boards.
>
> Hmm... I'm not sure why, but according to the log, it seems it calls
> clk_disable_unprepare() without calling clk_prepare_enable().
> I think "clk_multiplier" means "cs2000" on Salvator-X(S).
>
> Basically, I don't think it can be happen. But current rsnd driver doesn't
> check return value of clk_prepare_enable(). So if cs2000 failed clk_enable()
> for some reasons, indeed clk_disable_unprepare() might be called without
> enabled (It is another issue, though...)

That sounds plausible...

> I'm not tesed, but can this patch improve situation ?
>
> If above assumption was correct, the clk WARNING issue itself can be solved,
> but sound driver itself will fail to probe...

I'm adding your patch to my local tree.
Let's see what happens during the next few months...

> ------------------
> From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> Date: Tue, 17 Dec 2024 11:30:46 +0900
> Subject: [PATCH] ASoC: rsnd: check rsnd_adg_clk_enable() return value
>
> rsnd_adg_clk_enable() might be failed for some reasons. In such case,
> we will get WARNING from clk.c when suspend was used that it try to
> disable clk without enabled. Check rsnd_adg_clk_enable() return value.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
>  sound/soc/renesas/rcar/adg.c  | 30 ++++++++++++++++++++++++------
>  sound/soc/renesas/rcar/core.c |  4 +---
>  sound/soc/renesas/rcar/rsnd.h |  2 +-
>  3 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/sound/soc/renesas/rcar/adg.c b/sound/soc/renesas/rcar/adg.c
> index 0f190abf00e75..723dcc88af306 100644
> --- a/sound/soc/renesas/rcar/adg.c
> +++ b/sound/soc/renesas/rcar/adg.c

> @@ -389,18 +389,33 @@ void rsnd_adg_clk_control(struct rsnd_priv *priv, int enable)
>
>         for_each_rsnd_clkin(clk, adg, i) {
>                 if (enable) {
> -                       clk_prepare_enable(clk);
> +                       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->clkin_rate[i] = clk_get_rate(clk);
> +                       if (ret < 0)
> +                               break;
> +                       else

No need for the else.

> +                               adg->clkin_rate[i] = clk_get_rate(clk);
>                 } else {
> -                       clk_disable_unprepare(clk);
> +                       if (adg->clkin_rate[i])
> +                               clk_disable_unprepare(clk);
> +
> +                       adg->clkin_rate[i] = 0;
>                 }
>         }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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