RE: [PATCH v11 1/2] clk: validate spread spectrum configuration

[Peng Fan]

> 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@xxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxx>
> > >---
> > >
> > >(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.