Re: [PATCH v4 4/8] phy: rockchip: samsung-hdptx: Handle uncommitted PHY config changes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] rk_hdptx_phy_clk_determine_rate() incorrectly forces a 0 Hz clock rate when pll_config_dirty is true, breaking CCF semantics.
- [High] rk_hdptx_phy_consumer_get() silently suppresses PLL hardware lock failures, breaking the clk_prepare() error propagation contract.
- [High] Data race and missing synchronization between CCF and PHY APIs leading to concurrent hardware initialization.
--

commit 2dc0dee2b507137b074178dc88bd9b69dbe45d38
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

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

This commit defers programming HDMI PHY configuration changes to the hardware
until the PHY usage count increments. It introduces a pll_config_dirty flag
to track uncommitted state and invalidates the clock rate in the CCF callback
to force hardware updates.

> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> --- 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;

[Severity: High]
Does silently discarding this error break the clk_prepare error propagation
contract?

When this function is invoked via the clock provider's prepare phase, a
hardware failure such as an I/O timeout during the HDPTX_O_PHY_CLK_RDY wait
gets completely swallowed. The clk_prepare call would return success, allowing
CCF consumers to proceed while the PHY hardware remains uninitialized.

Although the comment assumes operations will be retried, if clk_set_rate is
not subsequently called, no further attempts will be made.

[ ... ]

> @@ -1700,13 +1704,18 @@ 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);

[Severity: High]
Can this missing synchronization lead to concurrent hardware initialization?

If thread A calls clk_prepare and thread B concurrently calls phy_power_on:

Thread A increments usage_count to 1 in rk_hdptx_phy_consumer_get and
enters rk_hdptx_pll_cmn_config.

Thread B increments usage_count to 2, returns immediately from
rk_hdptx_phy_consumer_get, and proceeds here to check pll_config_dirty.
Because thread A hasn't cleared the flag yet, thread B will also invoke
rk_hdptx_pll_cmn_config.

Could this data race allow both threads to concurrently execute non-atomic
regmap multi-write sequences and corrupt the PHY state? Are driver-level
locks needed to protect the shared pll_config_dirty and hdmi_cfg structures?

[ ... ]

> @@ -2323,8 +2335,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;
> +	}

[Severity: High]
Will returning 0 here break CCF semantics and cause a divide-by-zero crash
in downstream consumers?

When req->rate is explicitly forced to 0 Hz, the Common Clock Framework
uses this returned value to fulfill clk_round_rate and update its cached
clock rate. Consumers like the VOP2 display controller or DRM core that
rely on clk_get_rate will receive 0 Hz, which is known to lead to
crashes when calculating timestamping constants or display timings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-hdptx-clk-fixes-v4-0-ce5e1d456cda@collabora.com?part=4

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