Re: [PATCH] drm/bridge: analogix: Propagate PHY errors from resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The return value of the platform-specific dp->plat_data->power_on() callback is ignored in analogix_dp_resume(), potentially leading to hardware faults and unbalanced clock disables in the newly added error path.
--

commit 777e12c640d41a75f043282a8ce210a68d6eca97
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

drm/bridge: analogix: Propagate PHY errors from resume

This patch updates analogix_dp_resume() to check the return values of
phy_set_mode() and phy_power_on(). If either fails, it rolls back the power
configuration using a newly added power_off error path instead of ignoring
the error.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 30c0c0b41d5aa..00844f68d748e 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1461,12 +1461,23 @@ int analogix_dp_resume(struct analogix_dp_device *dp)
>  	if (dp->plat_data->power_on)
>  		dp->plat_data->power_on(dp->plat_data);
>  

[Severity: High]
This isn't a bug introduced by this patch, but should the return value of
dp->plat_data->power_on() be checked here?

If dp->plat_data->power_on() fails (for example, if clk_prepare_enable()
fails in rockchip_dp_poweron()), it returns a negative error code and
leaves the platform clock disabled. By ignoring this error, execution
proceeds to phy_set_mode() and analogix_dp_init_dp(), which can result in
accessing unpowered hardware registers. Accessing unpowered hardware
typically causes a synchronous external abort on Arm platforms.

Furthermore, this patch actively exacerbates this issue. If the platform
power initialization fails and subsequently phy_set_mode() or
phy_power_on() fails, the newly added goto power_off error path will
execute:

    power_off:
        if (dp->plat_data->power_off)
            dp->plat_data->power_off(dp->plat_data);
        clk_disable_unprepare(dp->clock);

Can this cause an unbalanced clock disable? The power_off path will blindly
call clk_disable_unprepare() on the platform clock that was never
successfully enabled.

> -	phy_set_mode(dp->phy, PHY_MODE_DP);
> -	phy_power_on(dp->phy);
> +	ret = phy_set_mode(dp->phy, PHY_MODE_DP);
> +	if (ret)
> +		goto power_off;
> +
> +	ret = phy_power_on(dp->phy);
> +	if (ret)
> +		goto power_off;
>  
>  	analogix_dp_init_dp(dp);
>  
>  	return 0;
> +
> +power_off:
> +	if (dp->plat_data->power_off)
> +		dp->plat_data->power_off(dp->plat_data);
> +	clk_disable_unprepare(dp->clock);
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828091334.5793-1-pengpeng@iscas.ac.cn?part=1