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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] phy: inno-hdmi: 32-bit overflow in
  inno_hdmi_phy_pre_pll_recalc_rate()
- [Low] phy: inno-hdmi: unscaled vco_rate logged in
  inno_hdmi_phy_pre_pll_recalc_rate()
--

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

phy: Add common Innosilicon HDMI PHY helpers

This commit adds a helper library for the Innosilicon HDMI PHY pre-PLL,
providing pixel clock lookup, clock rate calculation, and register
programming functions to be shared across SoC drivers.

> 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 parent_rate * nf and parent_rate * frac overflow on 32-bit
architectures?

Both parent_rate and frac are unsigned long, and nf is u16, so both
multiplications are evaluated in 32-bit arithmetic on 32-bit systems
before widening to u64 vco.  With a 24 MHz reference clock, parent_rate *
nf overflows 32 bits when nf >= 179, and parent_rate * frac overflows for
any 24-bit fractional divider entry, causing
inno_hdmi_phy_pre_pll_recalc_rate() to return a corrupted pixel clock
rate.  Casting parent_rate to u64 and using DIV_ROUND_CLOSEST_ULL() would
avoid the truncation.

> +
> +	vco_rate = vco;
> +
> +	if (control & INNO_PCLK_VCO_DIV_5_MASK) {
> +		div = nd * 5;
> +	} else {
> +		no_a = pclk_div_ab & INNO_PCLK_DIV_A_MASK;
> +		no_b = FIELD_GET(INNO_PCLK_DIV_B_MASK, pclk_div_ab) + 2;
> +		no_d = pclk_div_cd & INNO_PCLK_DIV_D_MASK;
> +
> +		div = nd * (no_a == 1 ? no_b : no_a) * no_d * 2;
> +	}
> +
> +	/*
> +	 * An unprogrammed pre-PLL reads its dividers back as zero, which would
> +	 * leave do_div() dividing by it.
> +	 */
> +	if (!div) {
> +		dev_dbg(pll->dev, "pre-PLL dividers read back as zero\n");
> +		return 0;
> +	}
> +
> +	do_div(vco, div);
> +
> +	rate = DIV_ROUND_CLOSEST((unsigned long)vco, 1000) * 1000;
> +
> +	/* The VCO is only in spec between 1.4 and 3.2 GHz, so log it. */
> +	dev_dbg(pll->dev, "%s rate %lu vco %llu\n", __func__, rate, vco_rate);

[Severity: Low]
Does vco_rate miss the division by the reference pre-divider nd?

Because nd is folded into div rather than applied to vco before vco_rate =
vco, dev_dbg() logs nd times the actual VCO frequency whenever prediv > 1.

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

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