Re: [PATCH v10 03/21] drm/bridge: synopsys: dw-dp: Fix error handling for DP link enablement

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The new error handling path in `dw_dp_link_enable()` fails to clear the PHY transmit enable bit (`XMIT_ENABLE`) before powering off the PHY.
- [Medium] The error path in `dw_dp_link_enable()` unconditionally calls `drm_dp_link_power_down()` without checking if the display is still connected.
--

--- Patch [3]: [PATCH v10 03/21] drm/bridge: synopsys: dw-dp: Fix error handling for DP link enablement ---
commit 8b0823cf002b6b94f89ce0d8276e50b1fc919ef4
Author: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/[email protected]>

drm/bridge: synopsys: dw-dp: Fix error handling for DP link enablement

This patch introduces a new enabled state in the link structure to ensure
the link disabling only happens if it has been properly enabled. It also adds
missing error handling in dw_dp_link_enable() to power off the PHY and link
if training fails.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index f8ca464d43717..dd951fd447227 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
[ ... ]
> @@ -1615,6 +1616,9 @@ static void dw_dp_link_disable(struct dw_dp *dp)
>  {
>  	struct dw_dp_link *link = &dp->link;
>  
> +	if (!link->enabled)
> +		return;
> +

[Severity: Medium]
Does this early return prevent cleaning up the hardware state if link
enablement fails partway through?

If dw_dp_link_train() fails, the PHY transmit enable bit (XMIT_ENABLE) might
be set, but link->enabled will remain false. Bypassing the teardown here means
we could leave the MAC configured to transmit while the PHY is eventually
powered down, potentially causing hardware lockups.

[ ... ]
>  static int dw_dp_link_enable(struct dw_dp *dp)
> @@ -1636,10 +1641,21 @@ static int dw_dp_link_enable(struct dw_dp *dp)
>  
>  	ret = drm_dp_link_power_up(&dp->aux, dp->link.revision);
>  	if (ret < 0)
> -		return ret;
> +		goto err_phy_power_off;
>  
>  	ret = dw_dp_link_train(dp);
> +	if (ret < 0)
> +		goto err_link_power_down;
> +
> +	dp->link.enabled = true;
>  
> +	return 0;
> +
> +err_link_power_down:
> +	drm_dp_link_power_down(&dp->aux, dp->link.revision);

[Severity: Medium]
Could this unconditional call to drm_dp_link_power_down() cause a
performance regression on hot-unplugs?

If link training fails because the DP cable is physically disconnected,
attempting an AUX transaction on an unplugged display causes timeout delays
which unnecessarily block the atomic commit thread. Should this call be
guarded with dw_dp_hpd_detect(dp) just as it is in dw_dp_link_disable()?

[Severity: Medium]
Additionally, should there be a call to dw_dp_phy_xmit_enable(dp, 0) in this
error path?

If dw_dp_link_train() fails, the hardware might still have the PHY transmit
enable bit set. Shutting down the PHY before clearing the XMIT_ENABLE state
can lead to undefined hardware states.

> +
> +err_phy_power_off:
> +	phy_power_off(dp->phy);
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-synopsys-dw-dp-improvements-v10-0-d84a26c303d6-ZGY8ohtN/[email protected]?part=3