Re: [PATCH v7 08/15] drm/msm/dp: break up dp_display_enable into two parts

[Yongxing Mou]

On 6/9/2026 5:59 PM, sashiko-...@kernel.org wrote:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) 
to consider:
- [Critical] Missing error return when sink_count == 0 leads to unclocked 
hardware accesses and kernel panic.
- [High] Missing early return when link training fails causes stream enablement 
on a broken link.
--

commit b8a02a483af0ac409e7c1790ebfbe296067ea49d
Author: Abhinav Kumar <quic_abhin...@quicinc.com>

drm/msm/dp: break up dp_display_enable into two parts

This commit splits dp_display_enable() into two parts: prepare and
enable APIs. This allows inserting MST payload functions between enabling
stream clocks and programming the registers.

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 155beefdfdef2..2cf4c984c7ca7 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c

[ ... ]

@@ -617,7 +617,40 @@ static int msm_dp_display_set_mode(struct msm_dp 
*msm_dp_display,
        return 0;
  }

-static int msm_dp_display_enable(struct msm_dp_display_private *dp, bool force_link_train)

+static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
+{
+       struct msm_dp *msm_dp_display = &dp->msm_dp_display;
+       int rc = 0;
+       bool force_link_train = false;
+
+       drm_dbg_dp(dp->drm_dev, "sink_count=%d\n", dp->link->sink_count);
+
+       if (msm_dp_display->is_edp)
+               msm_dp_hpd_plug_handle(dp);
+
+       rc = pm_runtime_resume_and_get(&msm_dp_display->pdev->dev);
+       if (rc) {
+               DRM_ERROR("failed to pm_runtime_resume\n");
+               return rc;
+       }
+
+       if (dp->link->sink_count == 0)
+               return rc;

[Severity: Critical]
Does this unintentionally allow the enable sequence to continue when the
sink_count is 0?

Since pm_runtime_resume_and_get() succeeded, rc is 0 here. Returning 0
causes the caller, msm_dp_bridge_atomic_enable(), to continue directly into
msm_dp_display_enable() because the prepare step appears successful:

msm_dp_bridge_atomic_enable() {
     ...
     rc = msm_dp_display_prepare_link(dp);
     if (rc) {
         DRM_ERROR("DP display prepare failed, rc=%d\n", rc);
         return;
     }

     rc = msm_dp_display_enable(dp);
     ...
}

Because msm_dp_ctrl_on_link() was skipped, the DP link clocks are not
enabled. Will the subsequent call to msm_dp_ctrl_on_stream() within
msm_dp_display_enable() then perform an unclocked hardware access when
writing to REG_DP_STATE_CTRL, potentially triggering a bus stall and kernel
panic?

+
+       if (!msm_dp_display->power_on) {
+               msm_dp_display_host_phy_init(dp);
+               force_link_train = true;
+       }
+
+       rc = msm_dp_ctrl_on_link(dp->ctrl);
+       if (rc)
+               DRM_ERROR("Failed link training (rc=%d)\n", rc);
+       // TODO: schedule drm_connector_set_link_status_property()
+
+       return msm_dp_ctrl_prepare_stream_on(dp->ctrl, force_link_train);
+}

[Severity: High]
If msm_dp_ctrl_on_link() fails, does this mask the failure and force the
pipeline to stream video over a broken link?

In the original code, an error from msm_dp_ctrl_on_link() would explicitly
return and abort the atomic enable sequence. Here, the error is logged, but
execution falls through to msm_dp_ctrl_prepare_stream_on(), which often
returns 0.

If it returns 0, the caller receives a success status and proceeds to
configure and start the video stream on an uninitialized link.

Will fix it next patch.