Re: [PATCH v5 02/28] clk: Skip clamping when rounding if there's no boundaries

From: Maxime Ripard <maxime@cerno.tech>
To: Stephen Boyd <sboyd@kernel.org>
Cc: Mike Turquette <mturquette@baylibre.com>,
	linux-clk@vger.kernel.org,
	Yassine Oudjana <y.oudjana@protonmail.com>,
	Naresh Kamboju <naresh.kamboju@linaro.org>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Tony Lindgren <tony@atomide.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Alexander Stein <alexander.stein@ew.tq-group.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Jerome Brunet <jbrunet@baylibre.com>
Subject: Re: [PATCH v5 02/28] clk: Skip clamping when rounding if there's no boundaries
Date: Tue, 28 Jun 2022 10:58:06 +0200	[thread overview]
Message-ID: <20220628085806.vwjnc6gdra2wgtz3@houat> (raw)
In-Reply-To: <20220627232200.C3ABDC34115@smtp.kernel.org>

[-- Attachment #1: Type: text/plain, Size: 3901 bytes --]

Hi Stephen,

Thanks for reviewing these patches

On Mon, Jun 27, 2022 at 04:21:58PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-05-16 06:25:01)
> > Commit 948fb0969eae ("clk: Always clamp the rounded rate") recently
> > started to clamp the request rate in the clk_rate_request passed as an
> > argument of clk_core_determine_round_nolock() with the min_rate and
> > max_rate fields of that same request.
> > 
> > While the clk_rate_requests created by the framework itself always have
> > those fields set, some drivers will create it themselves and don't
> > always fill min_rate and max_rate.
> > 
> > In such a case, we end up clamping the rate with a minimum and maximum
> > of 0, thus always rounding the rate to 0.
> > 
> > Let's skip the clamping if both min_rate and max_rate are set to 0 and
> > complain so that it gets fixed.
> > 
> > Fixes: 948fb0969eae ("clk: Always clamp the rounded rate")
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/clk/clk.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 2a32fa9f7618..d46c00bbedea 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1341,7 +1341,19 @@ static int clk_core_determine_round_nolock(struct clk_core *core,
> >         if (!core)
> >                 return 0;
> >  
> > -       req->rate = clamp(req->rate, req->min_rate, req->max_rate);
> > +       /*
> > +        * Some clock providers hand-craft their clk_rate_requests and
> > +        * might not fill min_rate and max_rate.
> > +        *
> > +        * If it's the case, clamping the rate is equivalent to setting
> > +        * the rate to 0 which is bad. Skip the clamping but complain so
> > +        * that it gets fixed, hopefully.
> > +        */
> > +       if (!req->min_rate && !req->max_rate)
> > +               pr_warn("%s: %s: clk_rate_request has initialized min or max rate.\n",
> > +                       __func__, core->name);
> 
> Is this going to trigger anytime soon?

drivers/clk/qcom/clk-rcg2.c was in this situation before
https://lore.kernel.org/all/20220419235447.1586192-1-dmitry.baryshkov@linaro.org/

Other than this one, there's a few other drivers doing this too:

* drivers/clk/clk-divider.c
  https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-divider.c#L389
  https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk-divider.c#L411

  That one isn't really critical. divider_ro_round_rate_parent isn't
  used by anyone (as of 5.19-rc4), and divider_round_rate_parent is only
  used by drivers/clk/sunxi-ng/ccu_div.c.

  This is then used by virtually all the Allwinner SoCs supported, but
  only for either internal bus clocks that are very unlikely to change
  their rate, DSI, CSI or I2S clocks. Fortunately for us, these are all
  fairly unusual on Allwinner devices. So the situation is likely to
  occur on those systems, but should still be fairly rare.

* drivers/clk/clk-composite.c
  drivers/clk/at91/clk-generated.c
  drivers/clk/at91/clk-master.c
  drivers/clk/at91/clk-peripheral.c

  All those will copy the request being passed. Since it comes from the
  framework, the boundaries are likely to be set but possibly wrong
  since they are the boundaries of the child clock, not the parent one.

  That part is addressed later in this series:
  https://lore.kernel.org/linux-clk/20220516132527.328190-23-maxime@cerno.tech/

So all in all, the impact should be fairly minimal, but it could still
happen.

> I'd prefer we return an error from here if the min/max is 0. The
> warning can stay. Also probably needs to be a pr_warn_once() or
> ratelimited because sometimes rate setting code is called very often.

Yeah, that makes sense, I'll change it

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]