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 <[email protected]> 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/[email protected]?part=15
