Re: [PATCH v2 2/6] phy: rockchip: samsung-hdptx: Handle uncommitted PHY config changes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Abuse of CCF determine_rate corrupts the clock tree rate cache.
- [High] Ignored return value in clk_ops.prepare masks hardware initialization failures.
- [High] Missing mutual exclusion between CCF and PHY frameworks causes concurrent hardware corruption.
- [High] Unbalanced PHY usage count on lane mode configuration failure in rk_hdptx_phy_power_on.
--

commit 53b6461d3d35f9f83438d0a57cf5379f7c1fcba8
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

phy: rockchip: samsung-hdptx: Handle uncommitted PHY config changes

Any changes to the PHY link rate and/or color depth done via the HDMI
PHY configuration API are not immediately programmed into the hardware,
but are delayed until the PHY usage count gets incremented from 0 to 1,
that is when it is powered on or when the PLL clock exposed through
the CCF API is prepared, whichever comes first.

[ ... ]

> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> index 7fb1c22318bbf..12c259d9544cf 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> @@ -1347,25 +1354,22 @@ static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx)
>  		return 0;
>  
>  	ret = regmap_read(hdptx->grf, GRF_HDPTX_STATUS, &status);
> -	if (ret)
> -		goto dec_usage;
> -
> -	if (status & HDPTX_O_PLL_LOCK_DONE)
> -		dev_warn(hdptx->dev, "PLL locked by unknown consumer!\n");
> +	if (ret) {
> +		atomic_dec(&hdptx->usage_count);
> +		return ret;
> +	}
>  
>  	if (mode == PHY_MODE_DP) {
>  		rk_hdptx_dp_reset(hdptx);
>  	} else {
> -		ret = rk_hdptx_pll_cmn_config(hdptx);
> -		if (ret)
> -			goto dec_usage;
> +		/*
> +		 * Ignore PLL config errors at this point as pll_config_dirty
> +		 * was not reset and, therefore, operation will be retried.
> +		 */
> +		rk_hdptx_pll_cmn_config(hdptx);
>  	}
>  
>  	return 0;

Does dropping the return value of rk_hdptx_pll_cmn_config() mask hardware
initialization failures?

If pll_config_dirty is false (for example, the clock was previously prepared
and unprepared), a subsequent failure won't be retried later.

Also, if a caller invokes clk_prepare_enable() but not clk_set_rate() or
phy_power_on(), will the hardware remain unconfigured while CCF incorrectly
assumes the clock is running?

> @@ -1700,16 +1704,20 @@ static int rk_hdptx_phy_power_on(struct phy *phy)
>  		if (ret)
>  			rk_hdptx_phy_consumer_put(hdptx, true);
>  	} else {
> -		regmap_write(hdptx->grf, GRF_HDPTX_CON0,
> -			     HDPTX_MODE_SEL << 16 | FIELD_PREP(HDPTX_MODE_SEL, 0x0));
> +		if (hdptx->pll_config_dirty)
> +			ret = rk_hdptx_pll_cmn_config(hdptx);
>  
> -		if (hdptx->hdmi_cfg.mode == PHY_HDMI_MODE_FRL)
> -			ret = rk_hdptx_frl_lcpll_mode_config(hdptx);
> -		else
> -			ret = rk_hdptx_tmds_ropll_mode_config(hdptx);
> +		if (!ret) {
> +			regmap_write(hdptx->grf, GRF_HDPTX_CON0,
> +				     HDPTX_MODE_SEL << 16 | FIELD_PREP(HDPTX_MODE_SEL, 0x0));
>  
> -		if (ret)
> +			if (hdptx->hdmi_cfg.mode == PHY_HDMI_MODE_FRL)
> +				ret = rk_hdptx_frl_lcpll_mode_config(hdptx);
> +			else
> +				ret = rk_hdptx_tmds_ropll_mode_config(hdptx);
> +		} else {
>  			rk_hdptx_phy_consumer_put(hdptx, true);
> +		}
>  	}

If rk_hdptx_tmds_ropll_mode_config() or rk_hdptx_frl_lcpll_mode_config()
fails, ret is set but the else block is never executed. Does this leave
usage_count unbalanced and prevent the PHY from being powered down? I noted
this was addressed later in the series by commit "phy: rockchip:
samsung-hdptx: Drop TMDS rate setup workaround", but wanted to point it out.

Also, does this logic introduce a race condition between CCF (clk_prepare)
and the PHY framework (phy_power_on)? The usage_count is atomic, but there
appears to be no mutual exclusion for the hardware configuration itself.

If Thread A calls clk_prepare() and increments the count to 1, it starts the
lengthy rk_hdptx_pll_cmn_config() hardware sequence.

If Thread B calls phy_power_on() concurrently and increments the count to 2,
it returns immediately from rk_hdptx_phy_consumer_get(). Thread B will then
evaluate hdptx->pll_config_dirty, see it is still true, and concurrently
execute rk_hdptx_pll_cmn_config() while Thread A is already modifying the
same registers.

> @@ -2303,8 +2314,19 @@ static int rk_hdptx_phy_clk_determine_rate(struct clk_hw *hw,
>  {
>  	struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);
>  
> -	if (hdptx->hdmi_cfg.mode == PHY_HDMI_MODE_FRL)
> -		return hdptx->hdmi_cfg.rate;
> +	/*
> +	 * Invalidate current clock rate to ensure rk_hdptx_phy_clk_set_rate()
> +	 * will be invoked to commit PLL configuration.
> +	 */
> +	if (hdptx->pll_config_dirty) {
> +		req->rate = 0;
> +		return 0;
> +	}

Does returning req->rate = 0 here corrupt the clock tree rate cache?

By returning 0 to bypass CCF's optimization and force set_rate(), the Common
Clock Framework will cache 0 Hz as the actual hardware rate and propagate it
downstream. 

Could this cause downstream consumers (like the VOP2 display clock dclk)
or rate change notifiers to receive 0 Hz, potentially leading to
division-by-zero errors or broken pixel clock math?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-hdptx-clk-fixes-v2-0-664e41379cab@collabora.com?part=2

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