Re: [PATCH v6 2/5] phy: Add common Innosilicon HDMI PHY helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] phy: inno-hdmi: 32-bit overflow in inno_hdmi_phy_pre_pll_recalc_rate()
--

commit ddb60d944975dab0de39c50bd23d32e884d239ae
Author: Michal Wilczynski <m.wilczynski@samsung.com>

phy: Add common Innosilicon HDMI PHY helpers

This commit introduces a shared helper library for the Innosilicon HDMI PHY
pre-PLL configuration, including table lookup, clock rate calculation helpers,
and register programming.

> diff --git a/drivers/phy/phy-inno-hdmi.c b/drivers/phy/phy-inno-hdmi.c
> new file mode 100644
> index 0000000000000..6900d4d83e464
> --- /dev/null
> +++ b/drivers/phy/phy-inno-hdmi.c
[ ... ]
> +unsigned long
> +inno_hdmi_phy_pre_pll_recalc_rate(const struct inno_hdmi_phy_pre_pll *pll,
> +				  unsigned long parent_rate)
> +{
> +	unsigned int control, div_1, div_2, div_3, pclk_div_ab, pclk_div_cd;
> +	unsigned int frac_div_7_0, frac_div_15_8, frac_div_23_16, div;
> +	u8 nd, no_a, no_b, no_d;
> +	unsigned long frac, rate;
> +	u64 vco, vco_rate;
> +	u16 nf;
> +	int ret;
[ ... ]
> +	nd = div_1 & INNO_PRE_PLL_PRE_DIV_MASK;
> +	nf = (div_2 & INNO_PRE_PLL_FB_DIV_11_8_MASK) << 8;
> +	nf |= div_3;
> +	vco = parent_rate * nf;
> +
> +	if (!(div_2 & INNO_PRE_PLL_FRAC_DIV_DISABLE)) {
> +		ret = inno_read(pll, INNO_PRE_PLL_FRAC_DIV_7_0, &frac_div_7_0);
> +		if (ret)
> +			goto err;
> +		ret = inno_read(pll, INNO_PRE_PLL_FRAC_DIV_15_8, &frac_div_15_8);
> +		if (ret)
> +			goto err;
> +		ret = inno_read(pll, INNO_PRE_PLL_FRAC_DIV_23_16,
> +				&frac_div_23_16);
> +		if (ret)
> +			goto err;
> +
> +		frac = frac_div_7_0 | (frac_div_15_8 << 8) |
> +		       (frac_div_23_16 << 16);
> +		vco += DIV_ROUND_CLOSEST(parent_rate * frac,
> +					 BIT(INNO_FRAC_DIV_WIDTH));
> +	}

[Severity: Medium]
Can the VCO calculations in inno_hdmi_phy_pre_pll_recalc_rate() in
drivers/phy/phy-inno-hdmi.c overflow on 32-bit architectures?

Both parent_rate and frac have type unsigned long, while nf is a u16 and vco
is a u64. On 32-bit targets where unsigned long is 32 bits wide,
parent_rate * nf is evaluated in 32-bit arithmetic before being widened to
u64. With a typical 24 MHz reference clock, any feedback divider nf >= 179
exceeds U32_MAX, whereas the 12-bit feedback divider reaches up to 757 in the
Innosilicon pre-PLL configuration tables.

Similarly, when the fractional divider is enabled, parent_rate * frac and
DIV_ROUND_CLOSEST() operate on 32-bit unsigned long values before adding to
vco. For a 24 MHz parent_rate and a 24-bit frac value, parent_rate * frac
requires up to 49 bits and overflows 32-bit unsigned long whenever frac >= 179.

Should parent_rate be cast to u64 in parent_rate * nf, and should the
fractional calculation use DIV_ROUND_CLOSEST_ULL((u64)parent_rate * frac,
BIT(INNO_FRAC_DIV_WIDTH)) to avoid 32-bit truncation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-jh7110-inno-hdmi-phy-v6-0-f3379684d97e@samsung.com?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy