Re: [PATCH v5 02/10] phy: rockchip: samsung-hdptx: Prevent divide-by-zero when computing clk rate

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Clock rate determination fails in FRL mode due to returning raw rate instead of 0.
- [High] Unintended state mutation of `hdmi_cfg` during `determine_rate` read queries.
--

--- Patch [2]: [PATCH v5 02/10] phy: rockchip: samsung-hdptx: Prevent divide-by-zero when computing clk rate ---
commit 3aac52ab862cd2c54c76fff590e3524e49936631
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

phy: rockchip: samsung-hdptx: Prevent divide-by-zero when computing clk rate

Calculating 'sdm' fraction in rk_hdptx_phy_clk_calc_rate_from_pll_cfg()
could trigger a divide-by-zero, as it uses div_u64() with a denominator
read directly from hardware. Provide the necessary sanitization to avoid
trusting the hardware state.

Link: https://sashiko.dev/#/patchset/20260611-hdptx-clk-fixes-v3-0-67b1b0c00e16@collabora.com?part=1

> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> index 710603afff866..44e99343f2499 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c

[ ... ]

> @@ -2270,10 +2270,15 @@ static u64 rk_hdptx_phy_clk_calc_rate_from_pll_cfg(struct rk_hdptx_phy *hdptx)
>  
>  	fout = PLL_REF_CLK * ropll_hw.pms_mdiv;
>  	if (ropll_hw.sdm_en) {
> +		val = 16U * ropll_hw.sdm_deno *
> +		      (ropll_hw.sdc_deno * ropll_hw.sdc_n - ropll_hw.sdc_num);
> +		if (!val) {
> +			dev_dbg(hdptx->dev, "Invalid ROPLL hw state: deno == 0\n");
> +			return 0;
> +		}
> +
>  		sdm = div_u64(PLL_REF_CLK * ropll_hw.sdc_deno *
> -			      ropll_hw.pms_mdiv * ropll_hw.sdm_num,
> -			      16 * ropll_hw.sdm_deno *
> -			      (ropll_hw.sdc_deno * ropll_hw.sdc_n - ropll_hw.sdc_num));
> +			      ropll_hw.pms_mdiv * ropll_hw.sdm_num, val);
>  
>  		if (ropll_hw.sdm_num_sign)
>  			fout = fout - sdm;

While reviewing this patch, I noticed a couple of pre-existing issues in the
adjacent rk_hdptx_phy_clk_determine_rate() function:

drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c:rk_hdptx_phy_clk_determine_rate() {
	...
	if (hdptx->hdmi_cfg.mode == PHY_HDMI_MODE_FRL)
		return hdptx->hdmi_cfg.rate;

[Severity: High]
This isn't a bug introduced by this patch, but does this violate the
determine_rate API contract?

When the PHY is in FRL mode, the callback directly returns
hdptx->hdmi_cfg.rate (e.g., up to 3.2GHz). However, the Common Clock
Framework expects determine_rate to return 0 on success and update
req->rate.

Returning these massive positive integers casts them to a signed 32-bit int,
causing rates like 3.2GHz to overflow into a negative error code
(e.g., -1094277376).

Will clk_core_determine_round_nolock() propagate this negative error,
causing clk_set_rate() to abort and fail completely for FRL modes?

	...
		int ret = rk_hdptx_phy_verify_hdmi_config(hdptx, &hdmi, &hdptx->hdmi_cfg);

[Severity: High]
This is a pre-existing issue, but does this mutate the driver's internal
PHY configuration state during what should be a stateless query?

The determine_rate clock operation must be a stateless read query
(clk_round_rate). However, passing &hdptx->hdmi_cfg as the output parameter
to rk_hdptx_phy_verify_hdmi_config() modifies hdptx->hdmi_cfg.rate and bpc.

Since clk_round_rate can be called multiple times with different
hypothetical rates before any actual rate changes are committed, could
this corrupt the actual hardware tracking state?

If the rate change is subsequently aborted or clk_set_rate is called
with a different best rate, would hdptx->hdmi_cfg be left in an
inconsistent state, causing the actual hardware configuration to be
misconfigured?
}

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

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