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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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().
[ ... ]
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+
+ 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?
quoted hunk ↗ jump to hunk
+ }
+
+ 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?
quoted hunk ↗ jump to hunk
+ } 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
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy