[PATCH] drm/msm/dp: skip PUSH_IDLE when the link was never enabled

[PATCH] drm/msm/dp: skip PUSH_IDLE when the link was never enabled

[Jesse Casco]

msm_dp_display_atomic_enable() returns early when link training fails,
leaving ->power_on false and the main link down.
msm_dp_display_atomic_disable() nevertheless writes DP_STATE_CTRL_PUSH_IDLE
and waits for an idle-pattern completion that cannot arrive, so every failed
enable is followed by "PUSH_IDLE pattern timedout".

Every other step of the teardown is already gated on that flag:
msm_dp_display_disable(), called from .atomic_post_disable(), returns early
on !power_on. The PUSH_IDLE write is the only one that is not, so the
controller's runtime-PM reference is then dropped without the link having
been taken down.

On glymur (Snapdragon X2 Elite) the consequence is not a warning. The SoC
does not survive it: TrustZone force-stops the SOCCP and ADSP remote
processors and the machine resets silently about 50 ms later, with no oops
and no panic. On an ASUS Zenbook A16 (UX3607OA), whose eDP panel does not
currently train, this reproduces without any compositor or GPU involvement:

  # eDP enable has already failed with "Failed link training (rc=-104)"
  echo 1 > /sys/class/graphics/fb0/blank

  [535.645455] === marker ===
  [535.694833] qcom_q6v5_pas d00000.remoteproc: fatal error received: \
                 sys_m_smsm.c:512:TZ force stop
  [535.694875] remoteproc remoteproc0: crash detected in soccp: type fatal error
  [535.728857] qcom_q6v5_pas 6800000.remoteproc: fatal error received: \
                 sys_m_smsm.c:783:err fatal notification received from TZ
  <SoC reset>

Gate the PUSH_IDLE write on ->power_on so the disable path is consistent
with the rest of the teardown. With this applied the same sequence is
harmless and the machine stays up; without it, it resets every time.

The unconditional write dates back to the original DP driver
(c943b4948b58 ("drm/msm/dp: add displayPort driver support")), but the
surrounding code has been restructured several times since, so no Fixes:
tag is offered.

Note that the eDP link-training failure that exposes this on the A16 is a
separate problem in the glymur eDP PHY and is reported separately; this
change is about not damaging the machine when training fails, for whatever
reason.

Tested on ASUS Zenbook A16 (UX3607OA), Snapdragon X2 Elite Extreme, on
linux-next next-20260803 and next-20260807. The machine has since been
running next-20260807 with this patch as its daily driver.

Assisted-by: Anthropic:Claude-Opus-5
Signed-off-by: Jesse Casco <jesse.casco@gmail.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bc646d172..5d2ddf180 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1458,6 +1458,20 @@ void msm_dp_display_atomic_disable(struct msm_dp *dp)

 	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);

+	/*
+	 * If .atomic_enable() bailed out - link training failure is the common
+	 * case - the mainlink was never brought up and ->power_on stayed false.
+	 * Driving the PUSH_IDLE pattern into a controller that was never
+	 * enabled times out, and .atomic_post_disable() then drops the
+	 * controller's runtime-PM reference without tearing the PHY back down,
+	 * because msm_dp_display_disable() returns early on !power_on.  On
+	 * glymur (Snapdragon X2 Elite) that combination is answered by a
+	 * TrustZone-level SOCCP/ADSP force-stop and a silent SoC reset.
+	 * There is nothing to push idle, so leave it alone.
+	 */
+	if (!dp->power_on)
+		return;
+
 	msm_dp_ctrl_push_idle(msm_dp_display->ctrl);
 }

--
2.55.0

Re: [PATCH] drm/msm/dp: skip PUSH_IDLE when the link was never enabled

[sashiko-bot]

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

Pre-existing issues:
- [High] Clock references and mainlink hardware state are leaked if stream initialization fails.
- [High] Unbalanced pm_runtime_put_sync() when atomic_enable fails, causing PM usage count underflow.
- [High] Unbalanced pm_runtime_put_sync() in HPD disable path causing PM count underflow.
- [High] Use-after-free of dp->aux in the IRQ handler due to premature manual cleanup.
- [High] The bridge atomic disable callback calls a sleeping function, violating DRM atomic context constraints.
- [High] msm_dp_bridge_detect leaks a runtime PM reference on every successful probe of a connected display.
--

--- Patch [1]: [PATCH] drm/msm/dp: skip PUSH_IDLE when the link was never enabled ---
commit 439afaff7381d5903b6066c5f4c3f708d9f34ffd
Author: Jesse Casco <jesse.casco@gmail.com>

drm/msm/dp: skip PUSH_IDLE when the link was never enabled

msm_dp_display_atomic_enable() returns early when link training fails,
leaving ->power_on false and the main link down.
msm_dp_display_atomic_disable() nevertheless writes DP_STATE_CTRL_PUSH_IDLE
and waits for an idle-pattern completion that cannot arrive, so every failed
enable is followed by "PUSH_IDLE pattern timedout".

[ ... ]

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index dc6f33809ca5f..17e0410126ce8 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1416,6 +1416,20 @@ void msm_dp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
>  
>  	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
>  
> +	/*
> +	 * If .atomic_enable() bailed out - link training failure is the common
> +	 * case - the mainlink was never brought up and ->power_on stayed false.
> +	 * Driving the PUSH_IDLE pattern into a controller that was never
> +	 * enabled times out, and .atomic_post_disable() then drops the
> +	 * controller's runtime-PM reference without tearing the PHY back down,
> +	 * because msm_dp_display_disable() returns early on !power_on.  On
> +	 * glymur (Snapdragon X2 Elite) that combination is answered by a
> +	 * TrustZone-level SOCCP/ADSP force-stop and a silent SoC reset.
> +	 * There is nothing to push idle, so leave it alone.
> +	 */
> +	if (!dp->power_on)
> +		return;
> +
>  	msm_dp_ctrl_push_idle(msm_dp_display->ctrl);

[Severity: High]
This isn't a bug introduced by this patch, but does this function violate
DRM atomic context constraints?

In msm_dp_bridge_atomic_disable(), there is a call to msm_dp_ctrl_push_idle()
which pushes an idle pattern and waits:

drivers/gpu/drm/msm/dp/dp_ctrl.c:msm_dp_ctrl_push_idle() {
    ...
    if (!wait_for_completion_timeout(&ctrl->idle_comp,
            IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES))
    ...
}

Since bridge atomic disable callbacks execute in atomic context, sleeping
functions like wait_for_completion_timeout() are forbidden. While the patch
adds an early return for the !power_on case, won't this still sleep and cause
kernel warnings or deadlocks during normal display teardown paths?

>  }

[Severity: High]
This is a pre-existing issue, but does the error handling in
msm_dp_bridge_atomic_enable() leak clock references and mainlink hardware
state?

When msm_dp_display_enable() fails (e.g., wait4video_ready times out), it
leaves the pixel clock enabled but returns an error. The atomic enable
callback ignores this error:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_atomic_enable() {
    ...
    msm_dp_display_enable(msm_dp_display, force_link_train);
    rc = msm_dp_display_post_enable(dp);
    ...
}

Because the error is ignored and power_on remains false, the subsequent
teardown in msm_dp_display_disable() skips teardown entirely:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_disable() {
    ...
    if (!msm_dp_display->power_on)
        return 0;
    ...
}

Does this leave the clocks and PHY permanently enabled?

[Severity: High]
This is a pre-existing issue, but is there an unbalanced PM runtime put when
msm_dp_bridge_atomic_enable() bails out early?

If msm_dp_bridge_atomic_enable() returns early due to missing clock
parameters or if pm_runtime_resume_and_get() fails, it does not acquire a
PM reference:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_atomic_enable() {
    ...
    if (!msm_dp_display->msm_dp_mode.drm_mode.clock) {
        DRM_ERROR("invalid params\n");
        return;
    }
    ...
}

Since the DRM core still considers the enable step complete, the disable path
will later call msm_dp_bridge_atomic_post_disable(), which unconditionally
drops a reference:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_atomic_post_disable() {
    ...
    pm_runtime_put_sync(&dp->pdev->dev);
    ...
}

Will this cause the device's PM usage counter to underflow?

[Severity: High]
This isn't a bug introduced by this patch, but does the HPD disable path
suffer from a similar PM reference underflow?

If pm_runtime_resume_and_get() fails in msm_dp_bridge_hpd_enable(), it
returns early without acquiring the reference:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_hpd_enable() {
    ...
    if (pm_runtime_resume_and_get(&msm_dp_display->pdev->dev)) {
        DRM_ERROR("failed to resume power\n");
        return;
    }
    ...
}

Then msm_dp_bridge_hpd_disable() unconditionally drops the reference later:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_hpd_disable() {
    ...
    pm_runtime_put_sync(&msm_dp_display->pdev->dev);
    ...
}

Can this destabilize the device power state by dropping an unacquired
reference?

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during
driver removal?

In msm_dp_display_remove(), the sub-modules (including dp->aux) are manually
freed:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_remove() {
    ...
    msm_dp_display_deinit_sub_modules(dp);
    ...
}

Because the IRQ was registered with devm_request_threaded_irq(), the
interrupt remains active until devres cleanup runs after remove() returns.
If an interrupt fires in this window, msm_dp_display_irq_handler() will
dereference the freed aux pointer:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_irq_handler() {
    ...
    hpd_isr_status = msm_dp_aux_get_hpd_intr_status(dp->aux);
    ...
}

Could this lead to memory corruption or a kernel panic if the hardware
interrupt fires during unbind?

[Severity: High]
This is a pre-existing issue, but does msm_dp_bridge_detect() leak a runtime
PM reference on every successful probe of a connected display?

The function unconditionally acquires a PM reference at the start, and if a
connected display is detected, it skips dropping the reference:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_detect() {
    ...
    if (!priv->plugged) {
        if (phy_deinit) {
            msm_dp_aux_enable_xfers(priv->aux, false);
            msm_dp_display_host_phy_exit(priv);
        }

        pm_runtime_put_sync(&dp->pdev->dev);
    }
    ...
}

Does this mean every time userspace polls the connector state while a display
is connected, a new PM reference is acquired and never dropped, permanently
preventing the device from suspending?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260808171325.133041-1-jesse.casco@gmail.com?part=1