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 <[email protected]> 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-0d508505f...@collabora.com?part=3
