Re: [PATCH v9 13/25] clk: Set req_rate on reparenting

[Maxime Ripard <maxime@cerno.tech>]

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

Replying to my own questions after some time working on this.

On Mon, Oct 10, 2022 at 11:56:08AM +0200, Maxime Ripard wrote:
> > > > +/*
> > > > + * Update the orphan rate and req_rate of @core and all its children.
> > > > + */
> > > > +static void clk_core_update_orphan_child_rates(struct clk_core *core)
> > > > +{
> > > > +     struct clk_core *child;
> > > > +     unsigned long parent_rate = 0;
> > > > +
> > > > +     if (core->parent)
> > > > +             parent_rate = core->parent->rate;
> > > > +
> > > > +     core->rate = core->req_rate = clk_recalc(core, parent_rate);
> > > > +
> > > > +     hlist_for_each_entry(child, &core->children, child_node)
> > > > +             clk_core_update_orphan_child_rates(child);
> > > > +}
> > > > +
> > > >   static void clk_reparent(struct clk_core *core, struct clk_core *new_parent)
> > > >   {
> > > >       bool was_orphan = core->orphan;
> > > > @@ -2506,6 +2527,7 @@ static void clk_core_reparent(struct clk_core *core,
> > > >                                 struct clk_core *new_parent)
> > > >   {
> > > >       clk_reparent(core, new_parent);
> > > > +     clk_core_update_orphan_child_rates(core);
> > > >       __clk_recalc_accuracies(core);
> > > >       __clk_recalc_rates(core, POST_RATE_CHANGE);
> > 
> > I see a problem. __clk_recalc_rates() uses 'core->rate' as "old rate"
> > but we'll have already destroyed that by calling
> > clk_core_update_orphan_child_rates() and assigning 'core->rate' to the
> > recalc_rate. Are clk notifiers being used? If so, it will probably be
> > confused because the notifier will see the same rate as what was set
> > instead of the old rate. cpufreq is probably the biggest user of clk
> > notifiers.
> 
> That's a very good point... Which raises another one. Would it be ok to
> notify users on a reparenting? It would make sense to me, since the rate
> could be affected, but it's not been done so far so I'm not sure what
> the implications might be

Turns out it's already done, and the rates were indeed off like you
pointed out.

> > We should add a test for that so when a clk is reparented the old rate
> > is still what we expected it to be when the notifier is called.
> 
> I can do it, but I'm not sure what you want to test exactly. Let's
> assume we have a mux with a given rate, we change the parent of that
> mux, the rate is likely to be changed as well and we should put in the
> notifier that the old_rate is the first parent's, and the new rate the
> one of the new parent?

I implemented this, and this catches the issue you pointed out, so it
looks like a decent test :)

> > Also, clk_core_update_orphan_child_rates() is poorly named. It doesn't
> > care at all that the clk is an orphan. It seems like another
> > __clk_recalc_rates() without the notifier. I have no idea why we need
> > another recalc rates.
> 
> You're right, the only difference between the two (aside from the
> notifiers) is that req_rate is also updated in
> clk_core_update_orphan_child_rates().
> 
> > Possibly setting the req_rate in __clk_recalc_rates() is sufficient.
> > Or maybe we should bail out if the clk doesn't have the orphan bit
> > set.
> 
> Either way makes sense to me, the latter is probably less intrusive, but
> the former allows to consolidate __clk_recalc_rates() and
> clk_core_update_orphan_child_rates(). Which one would you prefer?

I ended up removing clk_core_update_orphan_child_rates() entirely,
adding the test, and submitting it here:
https://lore.kernel.org/linux-clk/20221010-rpi-clk-fixes-again-v1-0-d87ba82ac404@cerno.tech/

Maxime

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