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

drivers/gpu/drm/msm/dp/dp_display.c void msm_dp_bridge_atomic_disable(struct drm_bridge *drm_bridge, · line 1433

[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?

drivers/gpu/drm/msm/dp/dp_display.c void msm_dp_bridge_atomic_disable(struct drm_bridge *drm_bridge, · line 1434

[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/[email protected]?part=1