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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Calculations for PLL multipliers, dividers, and spread spectrum accuracy silently overflow and truncate on 32-bit architectures, potentially causing division-by-zero panics.
- [Medium] JFRACR and FRACG PLLs improperly override parent clock accuracy with 0 when Spread Spectrum is disabled.
- [Medium] Probe error path leaks I/O mapping, clock structures, and clock arrays.
--

commit 7c67e45031944ff1b7e739015f039ca13888eefa
Author: Benoît Monin <benoit.monin@bootlin.com>

clk: eyeq: Add EyeQ7H compatibles

This commit adds entries for 14 Other Logic Blocks found in the
EyeQ7H SoC. It introduces parsing for two types of PLLs, JFRACR and
AINTP, and handles more complex clock tree routing.

> diff --git a/drivers/clk/clk-eyeq.c b/drivers/clk/clk-eyeq.c
> index 5200d5f0ff060..bf4a8421cfccf 100644
> --- a/drivers/clk/clk-eyeq.c
> +++ b/drivers/clk/clk-eyeq.c

[ ... ]

> @@ -229,6 +290,60 @@ static int eqc_pll_parse_fracg(void __iomem *base, unsigned long *mult,
>  	return 0;
>  }
>  
> +static int eqc_pll_parse_jfracr(void __iomem *base, unsigned long *mult,
> +				unsigned long *div, unsigned long *acc)
> +{

[ ... ]

> +	/* fractional part on 24 bits */
> +	if (r1 & JFRACR_PCSR1_DSM_EN) {
> +		*div *= (1ULL << 24);
> +		*mult = *mult * (1ULL << 24) + FIELD_GET(JFRACR_PCSR1_FRAC_IN, r1);
> +	}

[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.

> +
> +	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.

[ ... ]

> @@ -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.

[ ... ]

> @@ -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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-clk-eyeq7-v8-0-53f618174cf6@bootlin.com?part=9