Re: [PATCH 15/22] clk: Add missing clk_core_init_rate_req calls

From: Maxime Ripard <maxime@cerno.tech>
To: Stephen Boyd <sboyd@kernel.org>
Cc: Mike Turquette <mturquette@baylibre.com>,
	linux-clk@vger.kernel.org,
	Naresh Kamboju <naresh.kamboju@linaro.org>,
	Alexander Stein <alexander.stein@ew.tq-group.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Tony Lindgren <tony@atomide.com>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Yassine Oudjana <y.oudjana@protonmail.com>,
	Neil Armstrong <narmstrong@baylibre.com>
Subject: Re: [PATCH 15/22] clk: Add missing clk_core_init_rate_req calls
Date: Sat, 23 Apr 2022 09:32:47 +0200	[thread overview]
Message-ID: <20220423073247.75ni6bwtvk4pf5fo@houat> (raw)
In-Reply-To: <20220423035126.13A8DC385A0@smtp.kernel.org>

On Fri, Apr 22, 2022 at 08:51:24PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-04-08 02:10:30)
> > Some callers of clk_core_round_rate_nolock() will initialize the
> > clk_rate_request structure by hand, missing a few parameters that leads
> 
> Which parameters?

min_rate and max_rate

> > to inconsistencies in what drivers can expect from that structure.
> > 
> > Let's use clk_core_init_rate_req() everywhere to make sure it's built in
> > a consistent manner.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/clk/clk.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 83dd5f1df0b9..3a59152b06b8 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1480,7 +1480,7 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
> >         int ret;
> >         struct clk_rate_request req;
> >  
> > -       req.rate = rate;
> > +       clk_core_init_rate_req(hw->core, &req, rate);
> >  
> >         ret = clk_core_round_rate_nolock(hw->core, &req);
> 
> clk_core_round_rate_nolock() has a clk_core_init_rate_req() inside it.
> This is duplicating that.

Yeah, and it's an issue whenever we take the recursion path in
clk_core_round_rate_nolock() here:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L1412

We just call clk_core_round_rate_nolock() onto our parent clock, with
the child rate request.

clk_core_round_rate_nolock() will then call clk_core_init_rate_req(),
which will set the boundaries (after patch 12) and the best_parent_*
fields. The parent will then possibly modify the rate, its parent rate
(through best_parent_rate), and if it's a mux that allows reparenting
can modify best_parent_hw too.

We then return, and the caller of clk_core_round_rate_nolock() will have
its parent parent rate and clk_hw pointer, and not its parent.

The way I dealt with it is that I'll later remove the
clk_core_init_rate_req() call in clk_core_round_rate_nolock() (patch 17)
to stop updating the *child* request, and in patch 19 will create a new
request for the parent, with the parent details, separating properly the
child context and its parent's.

This isn't just theoretical, patch 19 has a unit test
(clk_leaf_mux_set_rate_parent_determine_rate) that will show how it
affects drivers too.

Let me know if you'd like to address this some other way.

>
> >         if (ret)
> > @@ -1512,7 +1512,7 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
> >         if (clk->exclusive_count)
> >                 clk_core_rate_unprotect(clk->core);
> >  
> > -       req.rate = rate;
> > +       clk_core_init_rate_req(clk->core, &req, rate);
> >  
> >         ret = clk_core_round_rate_nolock(clk->core, &req);
> 
> Duplicating again?
> 
> >  
> > @@ -2216,8 +2216,7 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
> >         if (cnt < 0)
> >                 return cnt;
> >  
> > -       clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
> > -       req.rate = req_rate;
> > +       clk_core_init_rate_req(core, &req, req_rate);
> 
> So we put the boundaries inside clk_core_init_rate_req()? That sounds
> like it's required now after we clamp all the time.

It was already done in a prior patch (patch 12)

Maxime