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

[Brian Masney]

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.

Brian