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

[Maxime Ripard]

On Fri, Mar 20, 2026 at 03:31:41PM +0100, Maxime Ripard wrote:
> 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.

I'd even think it makes it *harder* to test since now you have to test
two combinations for each clock you convert instead of one (with one
being unlikely to happen). It's something that would be easily
overlooked, and	would receive less real-world testing as well.

Maxime

Attachment: signature.asc
Description: PGP signature