Re: [PATCH v5 04/10] drm/msm/dp: Move link training to atomic_enable()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Silent link training failure in atomic_enable leaves userspace with a broken display state.
- [High] Moving DP link training to `atomic_enable()` leaves the unplug handler performing unbalanced clock disable and PHY power off, which corrupts the PHY power state.
- [Medium] Commit message and implementation mismatch regarding hpd_notify().

Pre-existing issues:
- [High] Missing PM runtime put on error paths in `msm_dp_bridge_atomic_enable` causes a usage counter leak.
- [High] Failure during `msm_dp_ctrl_on_stream()` permanently leaks link clocks, stream clocks, and PHY power.
--

--- Patch [4]: [PATCH v5 04/10] drm/msm/dp: Move link training to atomic_enable() ---
commit c64d2d707d517a12463d9c1b31e96a4e25a831e3
Author: Jessica Zhang <jesszhan0024@gmail.com>

drm/msm/dp: Move link training to atomic_enable()

This commit moves DP link training from the HPD handler into atomic_enable().
It also states that plug and unplug handlers are called directly in
hpd_notify() to prevent preemption by other events.

> Finally, call the plug/unplug handlers directly in hpd_notify() instead
> of queueing them in the event thread so that they aren't preempted by
> other events.

[Severity: Medium]
Is this description accurate regarding hpd_notify()?

Looking at msm_dp_bridge_hpd_notify(), the code continues to queue events via
msm_dp_add_event() rather than calling the handlers directly:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_hpd_notify() {
    if (!msm_dp_display->link_ready && status == connector_status_connected)
        msm_dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
    else if (msm_dp_display->link_ready && status == connector_status_disconnected)
        msm_dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
}

Could the commit message or implementation be updated to match?

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index a05144de3b93f..e7b1ed5491c49 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -436,11 +436,6 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>  	msm_dp_link_psm_config(dp->link, &dp->panel->link_info, false);
>  
>  	msm_dp_link_reset_phy_params_vx_px(dp->link);
> -	rc = msm_dp_ctrl_on_link(dp->ctrl);
> -	if (rc) {
> -		DRM_ERROR("failed to complete DP link training\n");
> -		goto end;
> -	}

[Severity: High]
Does removing the link training here leave the unplug handler performing an
unbalanced clock disable and PHY power off?

If a user unplugs the display while in ST_MAINLINK_READY (before userspace
issues an atomic commit), msm_dp_hpd_unplug_handle() will call
msm_dp_ctrl_off_link().

This blindly calls clk_bulk_disable_unprepare() and phy_power_off() on clocks
and PHYs that were never enabled, leading to an unbalanced clock disable
warning and decrementing the PHY power_count to a negative value. Once the
power_count drops to -1, the next phy_power_on() increments it to 0 and
bypasses the actual hardware callback, which seems like it would break the
display until reboot.

>  
>  	msm_dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
>  

[ ... ]

> @@ -1695,6 +1690,11 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>  		force_link_train = true;
>  	}
>  
> +	rc = msm_dp_ctrl_on_link(msm_dp_display->ctrl);
> +	if (rc)
> +		DRM_ERROR("Failed link training (rc=%d)\n", rc);
> +	// TODO: schedule drm_connector_set_link_status_property()
> +

[Severity: High]
Will this leave userspace with a silently broken display state if
msm_dp_ctrl_on_link() fails?

The error is logged, but execution proceeds to enable the display anyway.
Without the mentioned drm_connector_set_link_status_property() update to mark
the link as BAD, does userspace have any way to know it must trigger a
recovery modeset?

[Severity: High]
This is a pre-existing issue, but does this function leak a PM runtime usage
counter on its error paths?

Earlier in msm_dp_bridge_atomic_enable(), a reference is acquired via
pm_runtime_resume_and_get(), but the early returns do not release it:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_bridge_atomic_enable() {
    ...
    hpd_state = msm_dp_display->hpd_state;
    if (hpd_state != ST_DISPLAY_OFF && hpd_state != ST_MAINLINK_READY) {
        mutex_unlock(&msm_dp_display->event_mutex);
        return;
    }

    rc = msm_dp_display_set_mode(dp, &msm_dp_display->msm_dp_mode);
    if (rc) {
        DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);
        mutex_unlock(&msm_dp_display->event_mutex);
        return;
    }
    ...
}

If either condition fails, the function returns early without calling
pm_runtime_put_sync(). Will this permanently prevent the display controller
from entering suspend states?

[Severity: High]
This is also a pre-existing issue, but can a failure during stream setup
permanently leak link clocks, stream clocks, and PHY power?

In msm_dp_ctrl_on_stream():

drivers/gpu/drm/msm/dp/dp_ctrl.c:msm_dp_ctrl_on_stream() {
    ...
    msm_dp_ctrl_enable_mainlink_clks(ctrl);

    ret = msm_dp_ctrl_set_tu(ctrl);
    if (ret)
        return ret;
    ...
}

If msm_dp_ctrl_set_tu() (or a subsequent step) fails, the function returns
without disabling the clocks and PHY power enabled earlier.

The caller (msm_dp_display_enable) then skips setting
msm_dp_display->power_on = true.

Later, msm_dp_display_disable() bypasses teardown:

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

Does this bypass cement the resource leak and permanently block PM?

>  	msm_dp_display_enable(msm_dp_display, force_link_train);
>  
>  	rc = msm_dp_display_post_enable(dp);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260314-hpd-refactor-v5-0-0c8450737d64@oss.qualcomm.com?part=4