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

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

On 7/23/26 10:55 PM, sashiko-bot@kernel.org wrote:
> 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?

Already answered:

https://lore.kernel.org/all/0afce8ee-0277-4fad-adbe-ceb064f66437@collabora.com/

> [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.

This was part of the workaround removed via [PATCH v5 08/10] phy: rockchip:
samsung-hdptx: Drop restrict_rate_change handling:

-	/*
-	 * FIXME: Temporarily allow altering TMDS char rate via CCF.
-	 * To be dropped as soon as the RK DW HDMI QP bridge driver
-	 * switches to make use of phy_configure().
-	 */
-	if (!hdptx->restrict_rate_change && req->rate != hdptx->hdmi_cfg.rate) {
-		struct phy_configure_opts_hdmi hdmi = {
-			.tmds_char_rate = req->rate,
-		};
-
-		int ret = rk_hdptx_phy_verify_hdmi_config(hdptx, &hdmi, &hdptx->hdmi_cfg);
-
-		if (ret)
-			return ret;
-	}


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