Re: [PATCH v2 1/2] phy: mediatek: phy-mtk-hdmi-mt8195: Fix PLL calc divisor overflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Multiplications involving `MEGA` and `GIGA` macros overflow on 32-bit architectures, breaking clock calculations.
--

commit 83c70bcce00287d7e880c60c53f322581568117d
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: mediatek: phy-mtk-hdmi-mt8195: Fix PLL calc divisor overflow

When trying to calculate a PLL rate for target display resolutions
above 2560x1440, 24bpp, 30Hz, the pixel clock value will be more
than 32-bits long but the division to finally calculate the digital
clock divider is being done with div_u64(), which expects a 32bit
unsigned divisor.

> diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> index 1426a2db984d5..e6ee8e0800222 100644
> --- a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> @@ -290,7 +290,7 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy *hdmi_phy, struct clk_hw *hw,
>  	posdiv2 = 1;
>  
>  	/* Digital clk divider, max /32 */
> -	digital_div = div_u64(ns_hdmipll_ck, posdiv1 * posdiv2 * pixel_clk);
> +	digital_div = div64_u64(ns_hdmipll_ck, posdiv1 * posdiv2 * pixel_clk);

[Severity: High]
This isn't a bug introduced by this patch, but since this patch fixes
32-bit overflows, could the multiplications involving MEGA and GIGA in
mtk_hdmi_pll_calc() and mtk_hdmi_pll_drv_setting() also overflow on 32-bit
architectures?

In drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c:mtk_hdmi_pll_calc(), the
bounds checks use MEGA and GIGA:

    else if (tmds_clk >= 54 * MEGA && (tmds_clk * 100) < 14835 * MEGA)

and:

    if (ns_hdmipll_ck >= 5 * GIGA &&
        ns_hdmipll_ck <= 12 * GIGA)

Since MEGA and GIGA are defined with the UL suffix in <linux/units.h>, do
values like 14835 * MEGA and 5 * GIGA silently wrap around when evaluated as
32-bit unsigned long values (e.g., 14835 * MEGA becoming ~1.95 billion)?

Similarly, in mtk_hdmi_pll_drv_setting():

    } else if (((u64)pixel_clk * 1000) >= 74175 * MEGA && pixel_clk <= 300 * MEGA) {

Would 74175 * MEGA wrap to ~1.13 billion on 32-bit builds instead of 74.1
billion, potentially intercepting conditions that should fall to a different
impedance configuration block?

>  	if (!(digital_div <= 32 && digital_div >= 1))
>  		return -EINVAL;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911074015.9994-1-angelogioacchino.delregno@collabora.com?part=1