[PATCH v5 05/10] phy: rockchip: samsung-hdptx: Handle uncommitted PHY config changes

[sashiko-bot at kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Ignored PLL configuration errors lead to silent hardware failures and a permanent power leak.
--

--- Patch [5]: [PATCH v5 05/10] phy: rockchip: samsung-hdptx: Handle uncommitted PHY config changes ---
commit ecdc0a2489cf7f8fad47abbac851ea7a49482fed
Author: Cristian Ciocaltea <cristian.ciocaltea at collabora.com>

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

This commit introduces a flag to track uncommitted PHY configuration changes.
It uses this flag in the clock determine_rate and phy power_on callbacks to
ensure the configuration is properly programmed into the hardware, even when
the clock remains prepared or in FRL mode.

> 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);
>  	}

[Severity: High]
Is it safe to unconditionally ignore the PLL configuration error here?

If the display is disabled and re-enabled without a mode change (like a DPMS
off/on cycle), phy_configure() is not called, leaving pll_config_dirty = false.

If rk_hdptx_pll_cmn_config() fails (for example, due to a PLL lock timeout),
the error is swallowed and clk_prepare() will incorrectly report success.

>  	return 0;
> -
> -dec_usage:
> -	atomic_dec(&hdptx->usage_count);
> -	return ret;
>  }

[ ... ]

> @@ -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]
Since pll_config_dirty can be false during a re-enable, the retry here would
be bypassed.

Will this cause the phy_power_on() callback to return success even though the
PLL is broken, leading to a silent failure and a black screen?

Additionally, if the PLL fails to lock during the earlier get() call, does
this prevent the cleanup sequence in rk_hdptx_phy_consumer_put() from
executing:

rk_hdptx_phy_consumer_put() {
        ...
        if (status & HDPTX_O_PLL_LOCK_DONE) {
                if (mode == PHY_MODE_DP)
                        rk_hdptx_dp_reset(hdptx);
                else
                        rk_hdptx_phy_disable(hdptx);
        }
        ...
}

Could this permanently leak the power resources allocated during the failed
rk_hdptx_phy_consumer_get() call?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-hdptx-clk-fixes-v5-0-8e786067865f@collabora.com?part=5