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

[Cristian Ciocaltea]

On 6/12/26 2:59 AM, sashiko-bot at kernel.org wrote:
> 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.
> --

Nothing new here, already discussed before - we can move on..

> 
> commit 2dc0dee2b507137b074178dc88bd9b69dbe45d38
> Author: Cristian Ciocaltea <cristian.ciocaltea at 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.
>