Re: [PATCH v3 15/19] phy: Add common Innosilicon HDMI PHY helpers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Kernel panic from division by zero in clock rate recalculation if regmap_read fails or registers are uninitialized.
- [High] Severe 32-bit integer overflows in VCO clock calculations on 32-bit architectures, leading to heavily truncated clock rates.
--

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

phy: Add common Innosilicon HDMI PHY helpers

This commit extracts shared Innosilicon HDMI PHY logic into a common
library. It provides pre-PLL configuration, lookup, and clock rate
recalculation helpers for SoC drivers that instantiate this IP.

> diff --git a/drivers/phy/phy-inno-hdmi.c b/drivers/phy/phy-inno-hdmi.c
> new file mode 100644
> index 0000000000000..77e745a87d22b
> --- /dev/null
> +++ b/drivers/phy/phy-inno-hdmi.c

[ ... ]

> +static u8 inno_read(const struct inno_hdmi_phy_pre_pll *pll, unsigned int reg)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(pll->regmap, inno_reg(pll, reg), &val);
> +	if (ret)
> +		return 0;

[Severity: High]
This isn't a bug introduced by this patch, but does explicitly returning 0
on a regmap read failure set up a deterministic kernel panic downstream?

If the registers are uninitialized or the read fails, the returned 0 will
propagate to the division operations in inno_hdmi_phy_pre_pll_recalc_rate().

[ ... ]

> +unsigned long
> +inno_hdmi_phy_pre_pll_recalc_rate(const struct inno_hdmi_phy_pre_pll *pll,
> +				  unsigned long parent_rate)
> +{
> +	u8 nd, no_a, no_b, no_d;
> +	unsigned long frac, rate;
> +	u64 vco, vco_rate;
> +	u16 nf;
> +
> +	nd = inno_read(pll, INNO_PRE_PLL_DIV_1) & INNO_PRE_PLL_PRE_DIV_MASK;
> +	nf = (inno_read(pll, INNO_PRE_PLL_DIV_2) &
> +	      INNO_PRE_PLL_FB_DIV_11_8_MASK) << 8;
> +	nf |= inno_read(pll, INNO_PRE_PLL_DIV_3);
> +	vco = parent_rate * nf;

[Severity: High]
This is a pre-existing issue, but on 32-bit architectures where
parent_rate is a 32-bit unsigned long, does this multiplication silently
overflow before being assigned to the 64-bit vco variable?

The nf variable can be up to 4095, which would severely truncate the
result if it exceeds the 32-bit limit. Should parent_rate be cast to
u64 here?

> +
> +	if (!(inno_read(pll, INNO_PRE_PLL_DIV_2) &
> +	      INNO_PRE_PLL_FRAC_DIV_DISABLE)) {
> +		frac = inno_read(pll, INNO_PRE_PLL_FRAC_DIV_7_0) |
> +		       (inno_read(pll, INNO_PRE_PLL_FRAC_DIV_15_8) << 8) |
> +		       (inno_read(pll, INNO_PRE_PLL_FRAC_DIV_23_16) << 16);
> +		vco += DIV_ROUND_CLOSEST(parent_rate * frac,
> +					 BIT(INNO_FRAC_DIV_WIDTH));

[Severity: High]
This isn't a bug introduced by this patch, but does the parent_rate * frac
multiplication silently overflow 32-bit limits as well?

If this calculation is upgraded to 64-bit to avoid the overflow, would it
also require switching DIV_ROUND_CLOSEST to DIV_ROUND_CLOSEST_ULL to prevent
__udivdi3 link errors on 32-bit systems?

> +	}
> +
> +	vco_rate = vco;
> +
> +	if (inno_read(pll, INNO_PRE_PLL_CONTROL) & INNO_PCLK_VCO_DIV_5_MASK) {
> +		do_div(vco, nd * 5);

[Severity: High]
This is a pre-existing issue, but if nd evaluates to 0 from a failed read,
will this do_div() trigger a divide by zero exception during probe?

> +	} else {
> +		no_a = inno_read(pll, INNO_PCLK_DIV_AB) & INNO_PCLK_DIV_A_MASK;
> +		no_b = FIELD_GET(INNO_PCLK_DIV_B_MASK,
> +				 inno_read(pll, INNO_PCLK_DIV_AB)) + 2;
> +		no_d = inno_read(pll, INNO_PCLK_DIV_CD) & INNO_PCLK_DIV_D_MASK;
> +
> +		do_div(vco, nd * (no_a == 1 ? no_b : no_a) * no_d * 2);

[Severity: High]
This is also a pre-existing issue, but similar to the above, if nd, no_a,
or no_d evaluate to 0 due to read failures, does this also cause a kernel
panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-jh7110-clean-send-v3-0-484f9ae72715@samsung.com?part=15