Re: [PATCH v6 5/7] clk: introduce new flag CLK_V2_RATE_NEGOTIATION for sensitive clocks

[Maxime Ripard]

On Thu, Mar 19, 2026 at 06:35:56AM -0400, Brian Masney wrote:
> On Thu, Mar 19, 2026 at 5:35 AM Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> > On Fri, Mar 13, 2026 at 12:43:12PM -0400, Brian Masney wrote:
> > > As demonstrated by the kunit tests, clk_calc_subtree() in the clk core
> > > can overwrite a sibling clk with the parent rate. Clocks that are used
> > > for some subsystems like DRM and sound are particularly sensitive to
> > > this issue.
> > >
> > > I consider this to be a logic bug in the clk subsystem, however this
> > > functionality has existed since the clk core was introduced with
> > > commit b2476490ef11 ("clk: introduce the common clock framework"),
> > > and some boards are unknowingly dependent on this behavior.
> > >
> > > Let's add support for a v2 rate negotiation logic that addresses the
> > > logic error. Clks can opt into this new behavior by adding the flag
> > > CLK_V2_RATE_NEGOTIATION, or globally on all clks with the kernel
> > > parameter clk_v2_rate_negotiation.
> > >
> > > Link: https://lore.kernel.org/linux-clk/aUSWU7UymULCXOeF@xxxxxxxxxx/
> > > Link: https://lpc.events/event/19/contributions/2152/
> > > Signed-off-by: Brian Masney <bmasney@xxxxxxxxxx>
> > > ---
> > >  drivers/clk/clk.c            | 70 ++++++++++++++++++++++++++++++++++++--------
> > >  include/linux/clk-provider.h |  3 ++
> > >  2 files changed, 60 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 051fe755e3bf1b0a06db254b92f8a02889456db9..64c6de5ff5df2117b8d1aca663d40b41d974bf92 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -886,6 +886,43 @@ unsigned long clk_hw_get_children_lcm(struct clk_hw *hw, struct clk_hw *requesti
> > >  }
> > >  EXPORT_SYMBOL_GPL(clk_hw_get_children_lcm);
> > >
> > > +static bool clk_v2_rate_negotiation_enabled;
> > > +static int __init clk_v2_rate_negotiation_setup(char *__unused)
> > > +{
> > > +     clk_v2_rate_negotiation_enabled = true;
> > > +     return 1;
> > > +}
> > > +__setup("clk_v2_rate_negotiation", clk_v2_rate_negotiation_setup);
> > > +
> > > +/**
> > > + * clk_has_v2_rate_negotiation - Check if a clk should use v2 rate negotiation
> > > + * @core: The clock core to check
> > > + *
> > > + * This function recursively checks if the clk or any of its descendants have
> > > + * the CLK_V2_RATE_NEGOTIATION flag set. The v2 behavior can also be enabled
> > > + * globally by adding clk_v2_rate_negotiation to the kernel command line.
> > > + *
> > > + * Returns: true if the v2 logic should be used; false otherwise
> > > + */
> > > +bool clk_has_v2_rate_negotiation(const struct clk_core *core)
> > > +{
> > > +     struct clk_core *child;
> > > +
> > > +     if (clk_v2_rate_negotiation_enabled)
> > > +             return true;
> > > +
> > > +     if (core->flags & CLK_V2_RATE_NEGOTIATION)
> > > +             return true;
> > > +
> > > +     hlist_for_each_entry(child, &core->children, child_node) {
> > > +             if (clk_has_v2_rate_negotiation(child))
> > > +                     return true;
> > > +     }
> > > +
> > > +     return false;
> > > +}
> > > +EXPORT_SYMBOL_GPL(clk_has_v2_rate_negotiation);
> > > +
> >
> > Do we really need to export it? I'd expect it to be abstracted away for
> > consumers and providers?
> 
> This is abstracted away for the consumers, however the provider needs
> to be aware if it wants to support the v2 logic. Patch 6 of this
> series that adds support to clk-divider.c uses this export. Well
> technically clk-divider.c is built with CONFIG_COMMON_CLK, however
> other clk providers that want to use v2 logic will need this export.
> 
> The way I see it is that there are provider-specific things that need
> to change if the v2 logic is used. For instance, the current behavior
> of clk-divider.c is that when CLK_V2_RATE_NEGOTIATION is set, the
> parent rate is just set to the new desired child rate, without taking
> into account any of the sibling rates. We need to keep this behavior
> for the v1 logic for existing boards. Moving this to the clk core
> won't work since it doesn't know what kind of clock this is. Maybe
> some need to compute the LCM, others may need the GCD, some may need
> something else possibly. Some of the existing providers will need to
> change to support this.
> 
> Now if we decided to not support the v1 logic, and just go all on on
> v2 logic everywhere, then we won't need this export, and can just
> update the providers.

Yeah... If we take a step back, in your previous version, we were
reworking the entire rate propagation logic, which was potentially
pretty hard to test and not easy to do a partial test and opt-in for.

With your current work, you have a clock flag that does the opt-in on a
clock by clock basis which makes it much easier to enable, and also
wouldn't create any unforeseen side-effects.

So I'm not sure we need the module parameter now.

Maxime

Attachment: signature.asc
Description: PGP signature