Re: [PATCH v2 11/15] 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] Possible use of uninitialized stack memory in `inno_read()` if `regmap_read()` fails.
- [High] Kernel panic from division by zero in clock rate recalculation if PHY registers are uninitialized.
--

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

phy: Add common Innosilicon HDMI PHY helpers

The Innosilicon HDMI PHY IP is used by several SoCs. They differ in
where the PHY register block sits in the register space and in which
pixel clocks they support, but the pre-PLL programming sequence and the
layout of its registers are the same.

Add a small library holding that shared part: the pre-PLL configuration
table format, a lookup, clk_ops determine_rate and recalc_rate helpers,
and the pre-PLL register programming. Callers pass a regmap, a register
offset for the PHY block, and their own pixel clock table.

> diff --git a/drivers/phy/phy-inno-hdmi.c b/drivers/phy/phy-inno-hdmi.c
> new file mode 100644
> index 0000000000000..deb2b73a86f5d
> --- /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;
> +
> +	regmap_read(pll->regmap, inno_reg(pll, reg), &val);
> +
> +	return val;
> +}

[Severity: High]
This isn't a bug introduced by this patch, as it was inherited from
drivers/phy/rockchip/phy-rockchip-inno-hdmi.c, but is it possible for
regmap_read() to fail here?

If regmap_read() returns an error code, it leaves the stack variable val
uninitialized, which is then returned to the caller.

Could we check the return value of regmap_read() to avoid leaking
uninitialized memory?

[ ... ]
> +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;
> +	u64 vco;
> +	u16 nf;
> +
> +	nd = inno_read(pll, INNO_PRE_PLL_DIV_1) & INNO_PRE_PLL_PRE_DIV_MASK;
[ ... ]
> +	if (inno_read(pll, INNO_PRE_PLL_CONTROL) & INNO_PCLK_VCO_DIV_5_MASK) {
> +		do_div(vco, nd * 5);

[Severity: High]
This is also a pre-existing issue from the original driver, but if the PHY
registers are uninitialized (containing default 0s) or if inno_read() fails
and returns an uninitialized 0, the nd variable will be 0.

Doesn't passing a 0 divisor to do_div() cause a divide-by-zero exception
leading to a kernel panic?

> +	} 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]
Similarly, if nd, no_a, or no_d evaluate to 0 due to register read failures
or uninitialized hardware, can this do_div() call also trigger a division by
zero panic during boot when the common clock framework calls recalc_rate()?

> +	}
> +
> +	return DIV_ROUND_CLOSEST((unsigned long)vco, 1000) * 1000;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-jh7110-clean-send-v2-0-331680c8b9d1@samsung.com?part=11

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