Re: [PATCH v5 03/10] phy: rockchip: samsung-hdptx: Guard against clk rate integer underflow

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] rk_hdptx_phy_clk_determine_rate() incorrectly returns a clock rate directly instead of 0 on success, violating the CCF API.
--

--- Patch [3]: [PATCH v5 03/10] phy: rockchip: samsung-hdptx: Guard against clk rate integer underflow ---
commit cf68603e6625cb39a98418e9f792a4e2e373dfcd
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

phy: rockchip: samsung-hdptx: Guard against clk rate integer underflow

This commit adds a check to avoid an integer underflow in the ROPLL
hardware state calculation. By validating that the computed sigma-delta
modulator offset does not exceed the base frequency, it prevents
returning excessively high clock rates to the Common Clock Framework.

Closes: https://lore.kernel.org/all/20260611235702.0E9691F000E9@smtp.kernel.org/

> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> index 44e99343f2499..fcf04e9814cc6 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> @@ -2280,10 +2280,16 @@ static u64 rk_hdptx_phy_clk_calc_rate_from_pll_cfg(struct rk_hdptx_phy *hdptx)
>  		sdm = div_u64(PLL_REF_CLK * ropll_hw.sdc_deno *
>  			      ropll_hw.pms_mdiv * ropll_hw.sdm_num, val);
>  
> -		if (ropll_hw.sdm_num_sign)
> +		if (ropll_hw.sdm_num_sign) {
> +			if (sdm > fout) {
> +				dev_dbg(hdptx->dev, "Invalid ROPLL hw state: sdm > fout\n");
> +				return 0;
> +			}
> +
>  			fout = fout - sdm;
> -		else
> +		} else {
>  			fout = fout + sdm;
> +		}
>  	}
>  
>  	return DIV_ROUND_CLOSEST_ULL(fout * 2 * 8, ropll_hw.pms_sdiv * 10 * bpc);
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in
rk_hdptx_phy_clk_determine_rate() located nearby in the same file:

drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c:rk_hdptx_phy_clk_determine_rate() {
	struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);

	if (hdptx->hdmi_cfg.mode == PHY_HDMI_MODE_FRL)
		return hdptx->hdmi_cfg.rate;
	...
}

Does this violate the Common Clock Framework API contract for
.determine_rate callbacks?

The .determine_rate callback is expected to update req->rate and return 0
on success. By returning the rate directly, it appears this fails to update
the request and instead returns a massive integer.

For FRL_8G4L_RATE (3,200,000,000), casting it to a 32-bit signed int would
wrap around to a negative value (-1,094,967,296).

When the clock rate is negotiated or set for 8Gbps FRL mode, CCF could
receive this wrapped negative value, interpret it as an error code, and
cause clk_set_rate() to fail entirely.

Even for smaller rates that do not wrap, leaving req->rate unmodified might
bypass rounding and cause downstream failures in HDMI configuration.

Should this instead update req->rate and return 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-hdptx-clk-fixes-v5-0-8e786067865f@collabora.com?part=3

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