[v11,1/2] clk: validate spread spectrum configuration
diff mbox series

Message ID 20260901090912.585681-2-dario.binacchi@amarulasolutions.com
State New
Headers show
Series
  • Support spread spectrum clocking for i.MX8M PLLs
Related show

Commit Message

Dario Binacchi Sept. 1, 2026, 9:05 a.m. UTC 
The spread spectrum configuration is passed to the provider's
set_spread_spectrum() callback without any validation, as clk-conf.c
only skips all-zero triplets from "assigned-clock-sscs". An invalid
device tree can hand providers a zero modulation frequency or a spread
ratio above 100%, and each provider would have to add the same checks
to protect e.g. divisions in its rate computations.

Fixes: c86814e70390 ("clk: Introduce clk_hw_set_spread_spectrum")
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

(no changes since v1)

 drivers/clk/clk.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Peng Fan Sept. 2, 2026, 8:38 a.m. UTC | #1 
On Tue, Sep 01, 2026 at 11:05:17AM +0200, Dario Binacchi wrote:
>The spread spectrum configuration is passed to the provider's
>set_spread_spectrum() callback without any validation, as clk-conf.c
>only skips all-zero triplets from "assigned-clock-sscs". An invalid
>device tree can hand providers a zero modulation frequency or a spread
>ratio above 100%, and each provider would have to add the same checks
>to protect e.g. divisions in its rate computations.
>
>Fixes: c86814e70390 ("clk: Introduce clk_hw_set_spread_spectrum")
>Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>---
>
>(no changes since v1)
>
> drivers/clk/clk.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
>diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>index fef87167a60b..208caf60eeb5 100644
>--- a/drivers/clk/clk.c
>+++ b/drivers/clk/clk.c
>@@ -2851,6 +2851,20 @@ int clk_hw_set_spread_spectrum(struct clk_hw *hw, const struct clk_spread_spectr
> 	if (!hw)
> 		return 0;
> 
>+	switch (ss_conf->method) {
>+	case CLK_SPREAD_NO:
>+		break;
>+	case CLK_SPREAD_CENTER:
>+	case CLK_SPREAD_UP:
>+	case CLK_SPREAD_DOWN:
>+		if (!ss_conf->modfreq_hz || !ss_conf->spread_bp ||
>+		    ss_conf->spread_bp > 10000)
>+			return -EINVAL;

I think sashiko comment is wrong.
In [1], we reached an agreement that spread "in permyriad, i.e. 0.01%".
So the input value must be the real value * 10000, saying 0.01% * 10000

[1] https://github.com/devicetree-org/dt-schema/pull/154

Regards
Peng

>+		break;
>+	default:
>+		return -EINVAL;
>+	}
>+
> 	core = hw->core;
> 
> 	clk_prepare_lock();
>-- 
>2.43.0
>
>

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Dario Binacchi Sept. 2, 2026, 10:14 a.m. UTC | #2 
Hi Peng,

On Wed, Sep 2, 2026 at 10:33 AM Peng Fan <peng.fan@oss.nxp.com> wrote:
>
> On Tue, Sep 01, 2026 at 11:05:17AM +0200, Dario Binacchi wrote:
> >The spread spectrum configuration is passed to the provider's
> >set_spread_spectrum() callback without any validation, as clk-conf.c
> >only skips all-zero triplets from "assigned-clock-sscs". An invalid
> >device tree can hand providers a zero modulation frequency or a spread
> >ratio above 100%, and each provider would have to add the same checks
> >to protect e.g. divisions in its rate computations.
> >
> >Fixes: c86814e70390 ("clk: Introduce clk_hw_set_spread_spectrum")
> >Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >---
> >
> >(no changes since v1)
> >
> > drivers/clk/clk.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> >diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >index fef87167a60b..208caf60eeb5 100644
> >--- a/drivers/clk/clk.c
> >+++ b/drivers/clk/clk.c
> >@@ -2851,6 +2851,20 @@ int clk_hw_set_spread_spectrum(struct clk_hw *hw, const struct clk_spread_spectr
> >       if (!hw)
> >               return 0;
> >
> >+      switch (ss_conf->method) {
> >+      case CLK_SPREAD_NO:
> >+              break;
> >+      case CLK_SPREAD_CENTER:
> >+      case CLK_SPREAD_UP:
> >+      case CLK_SPREAD_DOWN:
> >+              if (!ss_conf->modfreq_hz || !ss_conf->spread_bp ||
> >+                  ss_conf->spread_bp > 10000)
> >+                      return -EINVAL;
>
> I think sashiko comment is wrong.
> In [1], we reached an agreement that spread "in permyriad, i.e. 0.01%".
> So the input value must be the real value * 10000, saying 0.01% * 10000

The DT cell is a u32 holding the integer permyriad value, i.e. the
ratio * 10000 as agreed in [1]: 1% is written as 100, 100% as 10000.
extConfigValue[7:0] takes an integer percentage, so the conversion
is a division by 100, not by 10000:

  depth   DT value   / 10000   / 100
  0.1%        10         0        0
  1%         100         0        1
  3%         300         0        3
  100%     10000         1      100

With integer arithmetic the current code programs 0% for any depth
below 100%. I am sending the fix as the first patch of v12.

[1] https://github.com/devicetree-org/dt-schema/pull/154

Thanks and regards,
Dario
>
> [1] https://github.com/devicetree-org/dt-schema/pull/154
>
> Regards
> Peng
>
> >+              break;
> >+      default:
> >+              return -EINVAL;
> >+      }
> >+
> >       core = hw->core;
> >
> >       clk_prepare_lock();
> >--
> >2.43.0
> >
> >
Peng Fan Sept. 3, 2026, 1:51 a.m. UTC | #3 
> Subject: Re: [PATCH v11 1/2] clk: validate spread spectrum
> configuration
>
> Hi Peng,
>

NXP Confidential
> On Wed, Sep 2, 2026 at 10:33 AM Peng Fan <peng.fan@oss.nxp.com>
> wrote:
> >
> > On Tue, Sep 01, 2026 at 11:05:17AM +0200, Dario Binacchi wrote:
> > >The spread spectrum configuration is passed to the provider's
> > >set_spread_spectrum() callback without any validation, as clk-conf.c
> > >only skips all-zero triplets from "assigned-clock-sscs". An invalid
> > >device tree can hand providers a zero modulation frequency or a
> > >spread ratio above 100%, and each provider would have to add the
> same
> > >checks to protect e.g. divisions in its rate computations.
> > >
> > >Fixes: c86814e70390 ("clk: Introduce clk_hw_set_spread_spectrum")
> > >Signed-off-by: Dario Binacchi
> <dario.binacchi@amarulasolutions.com>
> > >---
> > >
> > >(no changes since v1)
> > >
> > > drivers/clk/clk.c | 14 ++++++++++++++
> > > 1 file changed, 14 insertions(+)
> > >
> > >diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
> > >fef87167a60b..208caf60eeb5 100644
> > >--- a/drivers/clk/clk.c
> > >+++ b/drivers/clk/clk.c
> > >@@ -2851,6 +2851,20 @@ int clk_hw_set_spread_spectrum(struct
> clk_hw *hw, const struct clk_spread_spectr
> > >       if (!hw)
> > >               return 0;
> > >
> > >+      switch (ss_conf->method) {
> > >+      case CLK_SPREAD_NO:
> > >+              break;
> > >+      case CLK_SPREAD_CENTER:
> > >+      case CLK_SPREAD_UP:
> > >+      case CLK_SPREAD_DOWN:
> > >+              if (!ss_conf->modfreq_hz || !ss_conf->spread_bp ||
> > >+                  ss_conf->spread_bp > 10000)
> > >+                      return -EINVAL;
> >
> > I think sashiko comment is wrong.
> > In [1], we reached an agreement that spread "in permyriad, i.e.
> 0.01%".
> > So the input value must be the real value * 10000, saying 0.01% *
> > 10000
>
> The DT cell is a u32 holding the integer permyriad value, i.e. the ratio *
> 10000 as agreed in [1]: 1% is written as 100, 100% as 10000.
> extConfigValue[7:0] takes an integer percentage, so the conversion is a
> division by 100, not by 10000:
>
>   depth   DT value   / 10000   / 100
>   0.1%        10         0        0
>   1%         100         0        1
>   3%         300         0        3
>   100%     10000         1      100
>
> With integer arithmetic the current code programs 0% for any depth
> below 100%. I am sending the fix as the first patch of v12.
>

ok, please include more pieces in your patch.

The spread_bp field is documented as "Modulation percent in permyriad"
(include/linux/clk-provider.h), where 1 unit = 0.01%, i.e. 10000 = 100%.
So the > 10000 check here correctly rejects spreads above 100%.
Actually 1000 should be enough.

However, the existing KUnit test data in kunit_clk_assigned_rates.h uses:

  #define ASSIGNED_SSCS_0_SPREAD    30000   /* 300% in permyriad */
  #define ASSIGNED_SSCS_1_SPREAD    40000   /* 400% in permyriad */

These values are above 10000 and would be rejected by the new
validation. When clk_hw_set_spread_spectrum() returns -EINVAL, clk-conf.c
prints an error but swallows it (rc = 0), and the dummy provider's
set_spread_spectrum callback is never called. So the test assertions like:

  KUNIT_EXPECT_EQ(test, ctx->clk0.sscs.spread_bp, ASSIGNED_SSCS_0_SPREAD);

will fail because ctx->clk0.sscs.spread_bp remains at its initial value.

I believe the test values were intended to represent 3% and 4% spread
(reasonable SSC values), which in permyriad should be 300 and 400,
not 30000 and 40000.

Similarly, the divisor in clk-scmi-oem.c:

  val = FIELD_PREP(..., ss_conf->spread_bp / 10000);

should be / 10.  ==> Here not / 100, because i.MX
use 10 as %1 in SCMI firmware.

The skip test data also has the same issue:

  .sscs = {50000, 60000, 3},

where 60000 should be 600 (6%) in permyriad.

Could you fold a fix for the test data and clk-scmi-oem.c divisor
into the series? Specifically:

  drivers/clk/kunit_clk_assigned_rates.h:
    ASSIGNED_SSCS_0_SPREAD: 30000 -> 300  (3%)
    ASSIGNED_SSCS_1_SPREAD: 40000 -> 400  (4%)

  drivers/clk/clk_test.c:
    .sscs = {50000, 60000, 3} -> {50000, 600, 3}  (all six instances)

  drivers/clk/clk-scmi-oem.c:
    ss_conf->spread_bp / 10000 -> ss_conf->spread_bp / 10

Thanks,
Peng.

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.

Patch
diff mbox series 

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fef87167a60b..208caf60eeb5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2851,6 +2851,20 @@  int clk_hw_set_spread_spectrum(struct clk_hw *hw, const struct clk_spread_spectr
 	if (!hw)
 		return 0;
 
+	switch (ss_conf->method) {
+	case CLK_SPREAD_NO:
+		break;
+	case CLK_SPREAD_CENTER:
+	case CLK_SPREAD_UP:
+	case CLK_SPREAD_DOWN:
+		if (!ss_conf->modfreq_hz || !ss_conf->spread_bp ||
+		    ss_conf->spread_bp > 10000)
+			return -EINVAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	core = hw->core;
 
 	clk_prepare_lock();