Re: [PATCH v8 9/9] clk: eyeq: Add EyeQ7H compatibles

["Benoît Monin" <benoit.monin@bootlin.com>]

On Thursday, 4 June 2026 at 15:43:02 CEST, sashiko-bot@kernel.org wrote:
...
> [Severity: High]
> This isn't a bug introduced by this patch, but do these calculations silently
> overflow and truncate on 32-bit architectures?
> 
> The variables *mult and *div are unsigned long pointers, which are 32 bits
> wide on 32-bit systems. When *div is multiplied by (1ULL << 24) and then
> by 2048 (if DOWN_SPREAD is enabled), it evaluates to ref_div * (1 << 35).
> If ref_div is 32, this becomes 1 << 40, which truncates to exactly 0.
> 
> This truncated 0 is later passed to clk_hw_register_fixed_factor_pdata(),
> which can cause an immediate division-by-zero panic in the Common Clock
> Framework when clk_factor_recalc_rate() divides by fix->div.
> 
This driver is specific to EyeQ SoCs, and Mobileye EyeQ SoCs are 64 bits.
So no risk of overflow here.

> > +
> > +	if (!*mult || !*div)
> > +		return -EINVAL;
> > +
> > +	if (r2 & (JFRACR_PCSR2_RESET | JFRACR_PCSR2_DIS_SSCG)) {
> > +		*acc = 0;
> > +	} else {
> > +		/* spread spectrum is identical to FRACG PLL */
> > +		spread = FIELD_GET(JFRACR_PCSR2_SPREAD, r2);
> > +		*acc = DIV_ROUND_CLOSEST(spread * 1000000000, 1024 * 2);
> 
> [Severity: High]
> This is a pre-existing issue in the FRACG code duplicated here, but does
> this spread spectrum accuracy calculation overflow on 32-bit architectures?
> 
> The operation spread * 1000000000 is evaluated in 32-bit math. If spread
> is greater than 4, it wraps around UINT_MAX, resulting in an incorrect
> clock accuracy.
> 
Same as above. No risk of overflow on EyeQ SoCs.

> [ ... ]
> 
> > @@ -339,6 +481,32 @@ static int eqc_probe_pll_fracg(struct device *dev, struct device_node *np,
> >  	return 0;
> >  }
> >  
> > +static int eqc_probe_pll_jfracr(struct device *dev, struct device_node *np,
> > +				const struct eqc_clock *clk, void __iomem *base,
> > +				struct clk_hw_onecell_data *cells)
> > +{
> 
> [ ... ]
> 
> > +	hw = clk_hw_register_fixed_factor_pdata(dev, np, clk->name, &parent_data, 0, mult,
> > +						div, acc, CLK_FIXED_FACTOR_FIXED_ACCURACY);
> 
> [Severity: Medium]
> This isn't a new bug as it exists in the FRACG probe function as well, but
> does passing CLK_FIXED_FACTOR_FIXED_ACCURACY unconditionally override the
> parent clock accuracy when Spread Spectrum is disabled?
> 
> When the DIS_SSCG bit is set, the driver calculates acc = 0. By always
> providing this flag, the Common Clock Framework treats the PLL's absolute
> accuracy as exactly 0 ppb error. This wipes out and fails to inherit the
> parent clock's natural accuracy, which could break downstream consumers
> that rely on accurate tolerance reporting.
> 
The clock structure of Mobileye EyeQ SoC is such that PLLs with a spread
spectrum modulator (FRACG, JFRACR) are fed by the main oscillator, or a
fixed factor from the oscillator. So the parent clock of those PLLs has an
accuracy of 0 ppb.

> [ ... ]
> 
> > @@ -411,6 +579,22 @@ static int eqc_probe(struct platform_device *pdev)
> 
> [Severity: Medium]
> This is a pre-existing issue in eqc_probe(), but does the error path leak
> resources?
> 
> At the end of eqc_probe(), if of_clk_add_hw_provider() fails, the function
> returns directly without unmapping the I/O memory from ioremap(), freeing
> the cells array from kzalloc_flex(), or unregistering the previously
> initialized clocks.
> 
> Because the clocks are registered with clk_hw_register_fixed_factor_pdata()
> (which bypasses the devm_ auto-cleanup wrapper), failing to unregister them
> manually leaves orphaned clock objects in the system.
> 
If the clock driver fails to probe, the SoC wont be functional anyway. I do
not think it is worth adding code for a code path that only leads to a dead
system when reached.

Best regards,
-- 
Benoît Monin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com