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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/1/26 14:31, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Calculations using `14835 * MEGA`, `5 * GIGA`, and `12 * GIGA` silently overflow 32-bit `unsigned long` types on 32-bit architectures, completely breaking clock tiering and boundary checks.
> --
> 
> commit 8179a32cf1dac8f1d080e57bbce79246661b50eb
> Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
> phy: mediatek: phy-mtk-hdmi-mt8195: Fix PLL calc divisor overflow
> 
> This commit fixes an issue where calculating a PLL rate for high display
> resolutions results in a pixel clock exceeding 32 bits, but the digital
> clock divider calculation uses div_u64(), which expects a 32-bit divisor.
> It resolves this by using div64_u64() instead to prevent overflow.
> 
>> 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,
> 
> [Severity: High]
> This is a pre-existing issue, but do the constants in this function silently
> overflow on 32-bit architectures?
> 

There is no 32-bit architecture compatible with this driver. This will run only
on AArch64, and it's very unlikely to see a 32-bits kernel running on MT8195.

This can be eventually fixed with a later series, but shall not block the proposed
fixes from being picked, both because it's unlikely to happen, and because those
are pre-existing issues.

Besides, the series *is* actually fixing some other overflow issues and was already
tested good.



> Earlier in mtk_hdmi_pll_calc(), there are tier threshold calculations like:
> 
>      else if ((tmds_clk * 100) >= 14835 * MEGA && (tmds_clk * 10) < 2967 * MEGA)
> 
> and later boundary checks:
> 
>      if (ns_hdmipll_ck >= 5 * GIGA &&
>          ns_hdmipll_ck <= 12 * GIGA)
> 
> If MEGA and GIGA evaluate as 32-bit types on 32-bit systems, a multiplication
> like 14835 * MEGA evaluates to 14,835,000,000, which wraps around modulo 2^32
> to 1,950,098,112 before being promoted to u64 for the comparison against
> tmds_clk * 100.
> 
> Similarly, 5 * GIGA and 12 * GIGA would overflow, potentially rendering the
> valid PLL clock range mathematically unreachable and forcing the function to
> always return -EINVAL on 32-bit builds.
> 
> Should these constants be explicitly typed or cast to u64 to ensure the
> comparisons work correctly on 32-bit kernels?
> 
>>   	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);
>>   	if (!(digital_div <= 32 && digital_div >= 1))
>>   		return -EINVAL;
>>
> 


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