On 28/08/2026 16:19, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 10 potential
> issue(s) to consider:
> - [High] Silent truncation of 64-bit integers passed to `gcd()` on 32-bit
> architectures leads to incorrect clock calculations.
> - [High] 32-bit integer overflow during PHY bit rate calculation causes
> incorrect hardware configuration for high-bandwidth displays.
> - [High] Deterministic division by zero during CCF clock registration due to
> uninitialized configuration.
> - [High] Severe CPU soft lockup risk due to excessive 3-second busy-wait loop
> using `readl_poll_timeout_atomic`.
> - [High] Premature integer division causes precision loss yielding incorrect
> hardware PLL parameters.
> - [Medium] Data race on PLL configuration state by bypassing Common Clock
> Framework (CCF) synchronization.
> - [Low] 64-bit division using '/' and standard division macros causes build
> failure on 32-bit platforms.
> - [Low] Unsupported floating-point literals used in integer calculations.
> - [Low] Use of `mdelay(8)` wastes CPU resources in a sleepable context.
> - [Low] Fragile design conflates the presence of PHY regulators with the name
> format of device tree memory resources.
> --
>
> commit 9c53defce3c459b1740fbff4f2434baad5c70aee
> Author: Krzysztof Kozlowski <[email protected]>
> Subject: drm/msm/hdmi_phy_eliza: Add support for Synopsys-based HDMI phy on
> Eliza
>
> Add driver for HDMI phy block from Synopsys used on Eliza SoC. Adjust core
> parts for handling HDMI phy initialization sequence.
>
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_phy.c
>> b/drivers/gpu/drm/msm/hdmi/hdmi_phy.c
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi_phy.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_phy.c
>> @@ -168,13 +171,17 @@ static int msm_hdmi_phy_probe(struct platform_device
>> *pdev)
>> if (!phy->cfg)
>> return -ENODEV;
>>
>> - phy->mmio = msm_ioremap(pdev, "hdmi_phy");
>> + if (phy->cfg->num_regs)
>> + phy->mmio = msm_ioremap(pdev, "hdmi_phy");
>> + else
>> + phy->mmio = msm_ioremap(pdev, NULL);
>
> [Severity: Low]
> Does this use a fragile heuristic? This logic overloads the presence of
> power regulators (num_regs) to guess the format of device tree memory
> resources. These two hardware properties are orthogonal, which could break
> if future device tree bindings diverge.
Ack
>
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_phy_eliza.c
>> b/drivers/gpu/drm/msm/hdmi/hdmi_phy_eliza.c
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_phy_eliza.c
> [ ... ]
>> +static int hdmi_eliza_pll_calculator(struct hdmi_pll_eliza *pll,
>> + unsigned long rate,
>> + unsigned long parent_rate,
>> + struct hdmi_eliza_phy_pll_reg_cfg *cfg)
>> +{
> [ ... ]
>> + s64 precision3 = 1e3;
>> + s64 precision6 = 1e6;
>
> [Severity: Low]
> Are these floating-point literals supported? Scientific notation defaults
> to double literals, which relies on compiler implicit conversion to integers.
> The kernel strictly prohibits floating-point types.
Yeah, that's not really a kernel style but downstream pattern which I
did not clean up.
>
> [ ... ]
>> + /* expon = floor(log2(4.999999e9/base_rate)) */
>> + utemp = 4.999999e9;
>
> [Severity: Low]
> Are these floating-point literals supported? This is another instance where
> scientific notation will evaluate as a double literal.
>
>> +
>> + expon = fls64(utemp / base_rate) - 1;
>
> [Severity: Low]
> Can this cause a build failure on 32-bit platforms? The direct 64-bit
> division operator triggers a dependency on __udivdi3, which is unresolved
> on 32-bit architectures. do_div or div64_u64 should be used instead.
>
> [ ... ]
>> + /* multiplier = 2*(floor(bit_rate) * 2**expon / ref_clk_int) -16) */
>> + multiplier = DIV_ROUND_UP(vco_freq, ref_clk_int);
>
> [Severity: Low]
> Could this also trigger a 32-bit build failure? DIV_ROUND_UP performs a
> native division, and since vco_freq is a 64-bit value, this requires a 64-bit
> division routine that is unavailable on 32-bit builds.
I got build warnings on this and remaining 32-bit, so I'll be fixing these.
>
> [ ... ]
>> + if (aux2 == 0) {
>> + cfg->remainder = 0;
>> + cfg->denominator = 1;
>> + } else {
>> + cfg->remainder = DIV_ROUND_UP(aux2, gcd(aux1, aux2));
>> + cfg->denominator = DIV_ROUND_UP(aux1, gcd(aux1, aux2));
>
> [Severity: High]
> Does this truncate 64-bit values on 32-bit architectures? The gcd function
> takes unsigned long parameters, which are 32-bit on 32-bit kernels. Since
> aux1 and aux2 can hold values requiring up to 36 bits, they will be silently
> truncated when passed into gcd, feeding incorrect fractions into the PLL
> configuration.
>
> [ ... ]
>> + /* stemp = (REF_CLK_MHZ/2**ref_ana_mpll_div) */
>> + stemp = (REF_CLK_HZ / 1000000) * precision3;
>
> [Severity: High]
> Does this premature division cause a loss of precision? REF_CLK_HZ is
> divided by 1,000,000 first, which drops the remainder before scaling. With
> REF_CLK_HZ at 38,400,000, the result truncates to 38 instead of preserving
> the 0.4 MHz. The error is then squared later, propagating significant drift
> into the PLL analog parameters.
>
> [ ... ]
>> + cfg->cp_int_holder = DIV_ROUND_CLOSEST(stemp3, precision6);
>
> [Severity: Low]
> Can this result in a build failure on 32-bit architectures?
> DIV_ROUND_CLOSEST triggers native division on s64 variables, which will fail
> to link without __divdi3.
>
> [ ... ]
>> +static int hdmi_eliza_pll_prepare(struct clk_hw *hw)
>> +{
> [ ... ]
>> + /*
>> + * This Synopsys PLL does not have a PLL lock-like register, thus there
>> + * is no way to tell when the PLL actually locked.
>> + * Returning now, would lead to stuck pclk or iface RCG clocks at off
>> + * (disp_cc_mdss_hdmi_pclk_clk status stuck at 'off'),
>> + * thus obviously the professional solution is to delay.
>> + * 8 ms was figured out during tests, not based on any datasheet.
>> + */
>> + mdelay(8);
>> +
>> + return 0;
>> +}
>
> [Severity: Low]
> Is mdelay strictly necessary here? Busy-waiting the CPU for 8 milliseconds
> wastes resources in a context where sleeping is fully permitted. The prepare
> callback can safely yield the CPU using msleep or usleep_range.
msleep should be fine
>
> [ ... ]
>> +static u64 hdmi_eliza_pll_vco_freq(u64 ref_clk_hz, u32 multiplier, u32
>> frac_quot,
>> + u32 frac_rem, u32 frac_den)
>> +{
>> + u64 num;
>> + u64 den;
>> +
>> + num = 32ULL * 65536ULL * frac_den +
>> + (u64)multiplier * 65536ULL * frac_den +
>> + 2ULL * ((u64)frac_quot * frac_den + frac_rem);
>> +
>> + den = 2ULL * 65536ULL * frac_den;
>> +
>> + return mul_u64_u64_div_u64(ref_clk_hz, num, den);
>> +}
>> +
>> +static unsigned long hdmi_eliza_pll_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> + struct hdmi_pll_eliza *pll = hw_clk_to_pll(hw);
>> + u64 rate;
>> +
>> + rate = hdmi_eliza_pll_vco_freq(REF_CLK_HZ, pll->cfg.multiplier,
>> + pll->cfg.quotient, pll->cfg.remainder,
>> + pll->cfg.denominator);
> [ ... ]
>> +int msm_hdmi_pll_eliza_init(struct platform_device *pdev)
>> +{
> [ ... ]
>> + pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
> [ ... ]
>> + ret = devm_clk_hw_register(dev, &pll->clk_hw);
>
> [Severity: High]
> Can this lead to a division by zero during probe?
Ack
>
> When devm_clk_hw_register is called, the Common Clock Framework immediately
> invokes recalc_rate. Because the pll structure is zero-allocated,
> pll->cfg.denominator is zero. This zero is passed into
> hdmi_eliza_pll_vco_freq, which results in den evaluating to 0 and
> triggering a kernel panic in mul_u64_u64_div_u64.
>
> [ ... ]
>> +static void hdmi_eliza_phy_tx_lane_config(struct hdmi_phy *phy,
>> + unsigned long pixclock)
>> +{
>> + const unsigned int bpp = 24;
> [ ... ]
>> + if (pixclock * bpp >= HDMI_PHY_MAX_BIT_RATE)
>> + hdmi_phy_update_bits(phy,
>> REG_HDMI_ELIZA_PHY_TX_COMMON_CONTROL_0, BIT(2), 0);
>> + else
>
> [Severity: High]
> Will this calculation overflow 32-bit bounds before the comparison?
>
> The pixclock parameter is typed as unsigned long, which is 32-bit on 32-bit
> kernels. For high bandwidth displays, multiplying a high pixclock by 24 will
> exceed the 32-bit integer maximum of 4.29 billion. This overflow will result
> in a smaller wrapped value, breaking the conditional check and incorrectly
> programming the PHY for high-resolution displays.
>
> [ ... ]
>> +static int hdmi_eliza_phy_ready_poll(struct hdmi_phy *phy, u32 reg, u32
>> mask, u32 val)
>> +{
>> + u32 state;
>> + const u32 sleep_us = 2000;
>> + const u32 timeout_us = 3000000;
>> +
>> + if (readl_poll_timeout_atomic((phy->mmio + reg), state,
>> + ((state & mask) == val),
>> + sleep_us, timeout_us)) {
>
> [Severity: High]
> Does this present a severe CPU soft lockup risk?
>
Ack, sleeping should be allowed, so readl_poll_timeout
> The readl_poll_timeout_atomic macro disables preemption and spins using a
> udelay. Looping a 2000 microsecond delay for up to 3,000,000 microseconds
> (3 seconds) violates scheduling rules and is very likely to trigger watchdog
> lockups if the hardware fails to respond.
>
> [ ... ]
>> +static void hdmi_eliza_phy_init(struct hdmi_phy *phy,
>> + unsigned long pixclock)
>> +{
> [ ... ]
>> + hdmi_eliza_pll_configure_pll(phy->pll, 396000000, 19200000);
>
> [Severity: Medium]
> Does this bypass the Common Clock Framework synchronization?
>
> Directly invoking hdmi_eliza_pll_configure_pll here forcefully mutates the
> shared pll->cfg state locklessly. CCF invariants generally mandate that
> clock state modifications are protected by prepare_lock. Concurrently
> reading the clock state from sysfs or debugfs could hit a data race.
Ack, but then programming guide will be violated.
Best regards,
Krzysztof
