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

From: Stephen Boyd <sboyd@kernel.org>
To: Maxime Ripard <maxime@cerno.tech>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>,
	Mike Turquette <mturquette@baylibre.com>,
	linux-clk@vger.kernel.org, Jerome Brunet <jbrunet@baylibre.com>,
	Naresh Kamboju <naresh.kamboju@linaro.org>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Alexander Stein <alexander.stein@ew.tq-group.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Yassine Oudjana <y.oudjana@protonmail.com>,
	Tony Lindgren <tony@atomide.com>
Subject: Re: [PATCH v9 13/25] clk: Set req_rate on reparenting
Date: Mon, 10 Oct 2022 18:15:23 -0700	[thread overview]
Message-ID: <20221011011525.01C9CC433C1@smtp.kernel.org> (raw)
In-Reply-To: <20221010145256.7zikxmkhjgor6esx@houat>

Quoting Maxime Ripard (2022-10-10 07:52:56)
> > > 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.

Yes reparenting does notifiers.

> 
> > > 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 :)

Awesome!

> 
> > > 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/
> 

Cool, thanks. Let me go stack that on top. I'd prefer to send that
branch to Linus in the next couple days.