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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional call to `drm_dp_link_power_down()` on error path causes severe stalls when unplugged.

Pre-existing issues:
- [High] Missing `reinit_completion()` in `dw_dp_aux_transfer()` leads to spurious AUX timeouts and data corruption.
- [High] TOCTOU race condition between asynchronous HPD worker and non-blocking atomic disable.
--

--- Patch [3]: [PATCH v11 03/21] drm/bridge: synopsys: dw-dp: Fix error handling for DP link enablement ---
commit a255c1363edf851c03c63d0e1832fe5878093ca6
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
drm/bridge: synopsys: dw-dp: Fix error handling for DP link enablement

This commit introduces an enabled state variable in the link structure to
ensure that the link is only disabled if it was properly enabled, preventing
PHY power state mismatches. It also adds missing error handling to
dw_dp_link_enable() to ensure the link enablement is an atomic operation.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index d7945f7fe9f05..21d60e89126f0 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -1636,10 +1641,22 @@ 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: High]
Can this unconditionally stall the DRM commit worker during a hot-unplug event?

If dw_dp_link_enable() fails link training due to the display being unplugged,
this error path unconditionally executes drm_dp_link_power_down() without
verifying if the hardware is still plugged in.

Because the sink is disconnected, DPCD writes over the AUX channel will
repeatedly time out, stalling the DRM commit worker for hundreds of
milliseconds. The existing cleanup code safely guards this with
if (dw_dp_hpd_detect(dp)), but this new inline error path omits the check.

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

[Severity: High]
This isn't a bug introduced by this patch, but does dw_dp_aux_transfer() have
a missing completion re-initialization that could cause data corruption?

Looking at dw_dp_aux_transfer():

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_aux_transfer() {
    ...
    status = wait_for_completion_timeout(&dp->complete, timeout);
    ...
}

The wait relies on the completion state being clean, but because it is not
re-initialized, a late interrupt from a previous timeout will prematurely
satisfy this wait. This could cause the next transfer to immediately return
without waiting for actual completion, returning stale data or falsely
reporting timeouts.

Should reinit_completion() be called before starting a transfer?

[Severity: High]
This is a pre-existing issue, but is there a TOCTOU race condition between the
asynchronous HPD worker and non-blocking atomic disable?

Looking at dw_dp_link_retrain():

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_link_retrain() {
    ...
    if (!ret)
        ret = dw_dp_link_train(dp);
    ...
}

And dw_dp_bridge_atomic_disable():

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bridge_atomic_disable() {
    ...
    dw_dp_link_disable(dp);
    ...
}

dw_dp_hpd_work() can trigger a link retrain asynchronously. It checks
dw_dp_needs_link_retrain() without the connection_mutex, then acquires the lock
and unconditionally calls dw_dp_link_train().

Since non-blocking atomic commits drop connection_mutex before
dw_dp_bridge_atomic_disable() executes, the HPD worker can run concurrently
with bridge teardown. If atomic_disable powers off the PHY, the HPD worker
could blindly access the powered-down hardware, potentially causing a lockup or
crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-synopsys-dw-dp-improvements-v11-0-0d508505f383@collabora.com?part=3