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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Ignoring PLL configuration errors in `rk_hdptx_phy_consumer_get()` leads to silent hardware failure and violates the CCF API contract.
--

commit 6b14268a38889c73eedd6c085d780316eb0e7c3c
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

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

This commit introduces a pll_config_dirty flag to keep track of uncommitted
PHY configuration changes. It ensures that changes to the PHY link rate or
color depth are delayed and properly programmed into the hardware when the
PHY usage count is incremented.

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

[Severity: High]
Is it safe to ignore the return value of rk_hdptx_pll_cmn_config() here?

If rk_hdptx_pll_cmn_config() fails, we return 0 to the caller. This causes
clk_prepare() to return success to the Common Clock Framework even though
the hardware failed to initialize. Does this violate the API contract?

Also, hdptx->usage_count is left incremented despite the hardware failure.

[ ... ]
> @@ -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]
Will this retry block actually execute if pll_config_dirty was already false?

If the initial rk_hdptx_pll_cmn_config() failure happened in 
rk_hdptx_phy_consumer_get() when pll_config_dirty was false, it seems
pll_config_dirty remains false. When rk_hdptx_phy_power_on() is later called,
it checks if pll_config_dirty is true and might skip the retry entirely,
leaving the hardware in a partially initialized state with an unlocked PLL.

> +		if (!ret) {
> +			regmap_write(hdptx->grf, GRF_HDPTX_CON0,
> +				     HDPTX_MODE_SEL << 16 | FIELD_PREP(HDPTX_MODE_SEL, 0x0));
> +
> +			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)
>  			rk_hdptx_phy_consumer_put(hdptx, true);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-hdptx-clk-fixes-v3-0-67b1b0c00e16@collabora.com?part=2

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