Re: [PATCH v10 1/1] clk: imx: pll14xx: support spread spectrum clock generation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing validation of spread spectrum parameters leads to division by zero panics.
- [High] Spread spectrum cannot be disabled and fails to clear stale hardware state.
- [Medium] Integer overflow in spread spectrum rate calculation (`mrr`).
- [Low] Missing clock infrastructure causes fatal compilation errors.
--

commit ab4effb250732b499e28c0bd53e1f0ee8a868e4c
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.
    
    Tested on the video PLL of i.MX8MN and i.MX8MP based boards.
    
    Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index b6f1cc9f57005..27a637847f570 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
> @@ -5,6 +5,7 @@
>  
>  #define pr_fmt(fmt) "pll14xx: " fmt
>  
> +#include <dt-bindings/clock/clock.h>

[Severity: Low]
Does this file build cleanly? It appears <dt-bindings/clock/clock.h> does
not exist. Additionally, struct clk_spread_spectrum added to struct
clk_pll14xx, and .set_spread_spectrum assigned in clk_pll1443x_ops are
undefined, which will cause fatal compilation errors.

[ ... ]

> @@ -366,6 +377,37 @@ 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 (conf->method == CLK_SSC_CENTER_SPREAD)
> +		sel_pf = SEL_PF_CENTER_SPREAD;
> +	else if (conf->method == CLK_SSC_UP_SPREAD)
> +		sel_pf = SEL_PF_UP_SPREAD;
> +	else if (conf->method == CLK_SSC_DOWN_SPREAD)
> +		sel_pf = SEL_PF_DOWN_SPREAD;
> +	else
> +		return;

[Severity: High]
Does this early return prevent spread spectrum from being disabled? If the
modulation method is unconfigured or set to disable, this path bypasses
clearing the SSCG_ENABLE bit.

If a bootloader had previously enabled spread spectrum, or if it is
dynamically disabled at runtime, the hardware may continue modulating using
stale configuration parameters alongside newly set PLL dividers.

> +
> +	sscg_ctrl = readl_relaxed(pll->base + SSCG_CTRL);
> +	sscg_ctrl &= ~(SSCG_ENABLE | MFREQ_CTL_MASK | MRAT_CTL_MASK | SEL_PF_MASK);
> +
> +	mfr = parent_rate / (conf->modfreq_hz * pdiv * BIT(5));

[Severity: High]
Can this calculation result in a division by zero panic? If
conf->modfreq_hz is uninitialized (0), this will cause a direct division
by zero.

Furthermore, if parent_rate < conf->modfreq_hz * pdiv * 32 (which is
possible with large pdiv values and a standard 32kHz modfreq), integer
division truncates mfr to 0. This will subsequently cause a division by
zero panic during the mrr calculation while clock subsystem locks are held,
permanently deadlocking the system.

> +	mrr = (conf->spread_bp * mdiv * BIT(6)) / (10000 * mfr);

[Severity: Medium]
Does the calculation 10000 * mfr risk a 32-bit integer overflow? Since
mfr is a 32-bit unsigned integer and 10000 is a 32-bit signed literal, the
multiplication is performed using 32-bit arithmetic.

If an anomalous device tree input causes mfr to exceed 429,496, the
multiplication will silently overflow the 32-bit boundary, producing a
truncated denominator and an invalid mrr value being pushed to the
hardware.

> +
> +	sscg_ctrl |= SSCG_ENABLE | FIELD_PREP(MFREQ_CTL_MASK, mfr) |
> +		FIELD_PREP(MRAT_CTL_MASK, mrr) |
> +		FIELD_PREP(SEL_PF_MASK, sel_pf);
> +
> +	writel_relaxed(sscg_ctrl, pll->base + SSCG_CTRL);
> +}

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