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

[sashiko-bot@kernel.org]

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