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@redhat.com/
Link: https://lpc.events/event/19/contributions/2152/
Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 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);
+
 /*
  * __clk_mux_determine_rate - clk_ops::determine_rate implementation for a mux type clk
  * @hw: mux type clk to determine rate on
@@ -2293,7 +2330,8 @@ static int __clk_speculate_rates(struct clk_core *core,
 }
 
 static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
-			     struct clk_core *new_parent, u8 p_index)
+			     struct clk_core *new_parent, u8 p_index,
+			     struct clk_core *initiating_clk)
 {
 	struct clk_core *child;
 
@@ -2306,8 +2344,12 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
 		new_parent->new_child = core;
 
 	hlist_for_each_entry(child, &core->children, child_node) {
-		child->new_rate = clk_recalc(child, new_rate);
-		clk_calc_subtree(child, child->new_rate, NULL, 0);
+		if (child != initiating_clk && clk_has_v2_rate_negotiation(child))
+			child->new_rate = child->rate;
+		else
+			child->new_rate = clk_recalc(child, new_rate);
+
+		clk_calc_subtree(child, child->new_rate, NULL, 0, initiating_clk);
 	}
 }
 
@@ -2316,7 +2358,8 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
  * changed.
  */
 static struct clk_core *clk_calc_new_rates(struct clk_core *core,
-					   unsigned long rate)
+					   unsigned long rate,
+					   struct clk_core *initiating_clk)
 {
 	struct clk_core *top = core;
 	struct clk_core *old_parent, *parent;
@@ -2364,7 +2407,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 		return NULL;
 	} else {
 		/* pass-through clock with adjustable parent */
-		top = clk_calc_new_rates(parent, rate);
+		top = clk_calc_new_rates(parent, rate, initiating_clk);
 		new_rate = parent->new_rate;
 		goto out;
 	}
@@ -2389,10 +2432,10 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 
 	if ((core->flags & CLK_SET_RATE_PARENT) && parent &&
 	    best_parent_rate != parent->rate)
-		top = clk_calc_new_rates(parent, best_parent_rate);
+		top = clk_calc_new_rates(parent, best_parent_rate, initiating_clk);
 
 out:
-	clk_calc_subtree(core, new_rate, parent, p_index);
+	clk_calc_subtree(core, new_rate, parent, p_index, initiating_clk);
 
 	return top;
 }
@@ -2440,7 +2483,7 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
  * walk down a subtree and set the new rates notifying the rate
  * change on the way
  */
-static void clk_change_rate(struct clk_core *core)
+static void clk_change_rate(struct clk_core *core, struct clk_core *initiating_clk)
 {
 	struct clk_core *child;
 	struct hlist_node *tmp;
@@ -2509,7 +2552,7 @@ static void clk_change_rate(struct clk_core *core)
 		__clk_notify(core, POST_RATE_CHANGE, old_rate, core->rate);
 
 	if (core->flags & CLK_RECALC_NEW_RATES)
-		(void)clk_calc_new_rates(core, core->new_rate);
+		(void)clk_calc_new_rates(core, core->new_rate, initiating_clk);
 
 	/*
 	 * Use safe iteration, as change_rate can actually swap parents
@@ -2519,12 +2562,12 @@ static void clk_change_rate(struct clk_core *core)
 		/* Skip children who will be reparented to another clock */
 		if (child->new_parent && child->new_parent != core)
 			continue;
-		clk_change_rate(child);
+		clk_change_rate(child, initiating_clk);
 	}
 
 	/* handle the new child who might not be in core->children yet */
 	if (core->new_child)
-		clk_change_rate(core->new_child);
+		clk_change_rate(core->new_child, initiating_clk);
 
 	clk_pm_runtime_put(core);
 }
@@ -2580,7 +2623,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 		return -EBUSY;
 
 	/* calculate new rates and get the topmost changed clock */
-	top = clk_calc_new_rates(core, req_rate);
+	top = clk_calc_new_rates(core, req_rate, core);
 	if (!top)
 		return -EINVAL;
 
@@ -2599,7 +2642,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	}
 
 	/* change the rates */
-	clk_change_rate(top);
+	clk_change_rate(top, core);
 
 	core->req_rate = req_rate;
 err:
@@ -3586,6 +3629,7 @@ static const struct {
 	ENTRY(CLK_IS_CRITICAL),
 	ENTRY(CLK_OPS_PARENT_ENABLE),
 	ENTRY(CLK_DUTY_CYCLE_PARENT),
+	ENTRY(CLK_V2_RATE_NEGOTIATION),
 #undef ENTRY
 };
 
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 2699b9759e13d0c1f0b54f4fa4f7f7bdd42e8dde..e0fc0bd347e5920e999ac96dbed9fc247f9443fa 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -32,6 +32,8 @@
 #define CLK_OPS_PARENT_ENABLE	BIT(12)
 /* duty cycle call may be forwarded to the parent clock */
 #define CLK_DUTY_CYCLE_PARENT	BIT(13)
+/* clock participates in v2 rate negotiation */
+#define CLK_V2_RATE_NEGOTIATION	BIT(14)
 
 struct clk;
 struct clk_hw;
@@ -1432,6 +1434,7 @@ void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 			   unsigned long max_rate);
 unsigned long clk_hw_get_children_lcm(struct clk_hw *hw, struct clk_hw *requesting_hw,
 				      unsigned long requesting_rate);
+bool clk_has_v2_rate_negotiation(const struct clk_core *core);
 
 static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 {

-- 
2.53.0

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@redhat.com/
> Link: https://lpc.events/event/19/contributions/2152/
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  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?

Maxime

On Thu, Mar 19, 2026 at 5:35 AM Maxime Ripard <mripard@kernel.org> 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@redhat.com/
> > Link: https://lpc.events/event/19/contributions/2152/
> > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > ---
> >  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

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@kernel.org> 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@redhat.com/
> > > Link: https://lpc.events/event/19/contributions/2152/
> > > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > > ---
> > >  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

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@kernel.org> 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@redhat.com/
> > > > Link: https://lpc.events/event/19/contributions/2152/
> > > > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > > > ---
> > > >  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

On Fri, Mar 20, 2026 at 03:33:36PM +0100, Maxime Ripard wrote:
> 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@kernel.org> 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@redhat.com/
> > > > > Link: https://lpc.events/event/19/contributions/2152/
> > > > > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > > > > ---
> > > > >  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.

That's a good point. The only reason I included the module parameter was
for testing purposes. If a board happens to use clk-divider, and has the
clk scaling issue, then we could ask them to boot the kernel with this
parameter to see if it addresses the problem.

I'll omit the kernel parameter from the next version. If we want someone
to test this, we can just have them add the clk flag to the relevant
part(s) of the clk tree.

Brian