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

Maxime Ripard
2022-04-23T07:32:56Z

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