Re: [PATCH v13 4/4] clk: imx: pll14xx: support spread spectrum clock generation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Potential 32-bit integer overflow in the numerator of the mrr calculation escapes bounds checking.
- [Medium] Invalid SSC configurations generate repeated warnings on every clock rate change.
--

commit 769a49cba9b51c72357919447fa12a82a6548ced
Author: Dario Binacchi <dario.binacchi@amarulasolutions.com>

    clk: imx: pll14xx: support spread spectrum clock generation
    
    Add support for spread spectrum clock (SSC) generation to the pll14xx
    driver.

> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index b6f1cc9f57005..5b187101c558c 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c

[ ... ]

> @@ -366,6 +377,58 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
>  	return 0;
>  }
>  
> +static void __clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
> +					       unsigned long parent_rate,
> +					       unsigned int pdiv,
> +					       unsigned int mdiv)
> +{
> +	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> +	struct clk_spread_spectrum *conf = &pll->ss_conf;
> +	u32 sscg_ctrl, mfr, mrr, sel_pf;

[ ... ]

> +	if (!mfr || mfr > FIELD_MAX(MFREQ_CTL_MASK)) {
> +		pr_warn("%s: SSC disabled, modulation frequency (%u Hz) out of range\n",
> +			clk_hw_get_name(hw), conf->modfreq_hz);
> +		goto out;
> +	}
> +
> +	mrr = (conf->spread_bp * mdiv * BIT(6)) / (10000 * mfr);

[Severity: Medium]
Could this calculation overflow 32-bit arithmetic?

If conf->spread_bp from the device tree is a large invalid value, the
left-to-right evaluated multiplication conf->spread_bp * mdiv occurs
in 32-bit arithmetic before being promoted.

Furthermore, on 32-bit architectures where BIT(6) is a 32-bit type,
the entire numerator conf->spread_bp * mdiv * BIT(6) is evaluated in 32 bits,
which can overflow for spread_bp values as low as 65600. This wrapped value
could then inadvertently pass the mrr > FIELD_MAX(...) bounds check that
follows.

While typical values are well within bounds, the commit message mentions
a goal of using 64-bit arithmetic to prevent 32-bit wrapping from
out-of-range DT values. It appears this code misses that protection
for spread_bp.

> +	if (!mrr || mrr > FIELD_MAX(MRAT_CTL_MASK)) {
> +		pr_warn("%s: SSC disabled, spread (%u permyriad) out of range\n",
> +			clk_hw_get_name(hw), conf->spread_bp);
> +		goto out;
> +	}
> +
> +	sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK, mfr) |
> +		FIELD_PREP(MRAT_CTL_MASK, mrr) |
> +		FIELD_PREP(SEL_PF_MASK, sel_pf);
> +
> +out:
> +	writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL);
> +}

[ ... ]

> +static int clk_pll1443x_set_spread_spectrum(struct clk_hw *hw,
> +					    const struct clk_spread_spectrum *ss_conf)
> +{
> +	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> +	struct clk_hw *parent = clk_hw_get_parent(hw);
> +	unsigned long parent_rate = parent ? clk_hw_get_rate(parent) : 0;
> +	unsigned long flags;
> +	u32 div_ctl0;
> +
> +	spin_lock_irqsave(&pll->lock, flags);
> +
> +	pll->ss_conf = *ss_conf;

[Severity: Medium]
Will caching an invalid configuration here cause excessive log spam?

If an out-of-bounds spread spectrum configuration is provided, it is
unconditionally cached to pll->ss_conf and 0 is returned instead of an error.

Subsequent runtime calls to clk_pll1443x_set_rate() will re-evaluate this
cached invalid configuration via __clk_pll1443x_set_spread_spectrum(), which
will repeatedly trigger the un-ratelimited pr_warn() calls on every clock
rate change.

> +
> +	/*
> +	 * Apply the configuration to the hardware right away, using the
> +	 * current PLL dividers: the clock framework does not call set_rate()
> +	 * if the requested rate is unchanged, so relying on it would leave
> +	 * the SSC settings unapplied when the PLL is already at the target
> +	 * rate (e.g. configured by the bootloader).
> +	 */
> +	div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
> +	__clk_pll1443x_set_spread_spectrum(hw, parent_rate,
> +					   FIELD_GET(PDIV_MASK, div_ctl0),
> +					   FIELD_GET(MDIV_MASK, div_ctl0));
> +
> +	spin_unlock_irqrestore(&pll->lock, flags);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903153836.373267-1-dario.binacchi@amarulasolutions.com?part=4