On 6/9/2026 6:13 PM, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
to consider:
New issues:
- [Medium] Incomplete refactoring in `msm_dp_display_disable` leaving
`dp->panel` usage.
Pre-existing issues:
- [High] Missing early return in `msm_dp_display_atomic_enable` on display
enable failure.
- [High] Sleeping functions called from atomic context in `atomic_post_disable`.
--
commit a04f2001a63e30ecc08234abf0151576d165e356
Author: Yongxing Mou <[email protected]>
drm/msm/dp: pass panel to display enable/disable helpers
This patch passes struct msm_dp_panel to the display enable/disable helpers.
This aims to make them easier to reuse for MST stream handling.
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 03320071fc077..888ecf34f1602 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
@@ -707,20 +708,21 @@ static void msm_dp_display_audio_notify_disable(struct
msm_dp_display_private *d
msm_dp_display->audio_enabled = false;
}
-static int msm_dp_display_disable(struct msm_dp_display_private *dp)
+static int msm_dp_display_disable(struct msm_dp_display_private *dp,
+ struct msm_dp_panel *msm_dp_panel)
{
struct msm_dp *msm_dp_display = &dp->msm_dp_display;
if (!msm_dp_display->power_on)
return 0;
- msm_dp_panel_disable_vsc_sdp(dp->panel);
+ msm_dp_panel_disable_vsc_sdp(msm_dp_panel);
msm_dp_ctrl_off_pixel_clk(dp->ctrl);
/* dongle is still connected but sinks are disconnected */
if (dp->link->sink_count == 0)
- msm_dp_link_psm_config(dp->link, &dp->panel->link_info, true);
+ msm_dp_link_psm_config(dp->link, &msm_dp_panel->link_info,
true);
msm_dp_ctrl_off_link(dp->ctrl, dp->panel);
[Severity: Medium]
Will this disable VSC SDP on the wrong panel when reused for MST streams?
It looks like msm_dp_ctrl_off_link() still operates on the primary dp->panel
instead of the newly passed msm_dp_panel parameter.
[ ... ]
@@ -1437,14 +1439,14 @@ void msm_dp_display_atomic_enable(struct msm_dp
*msm_dp_display)
dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
- rc = msm_dp_display_enable(dp);
+ rc = msm_dp_display_enable(dp, dp->panel);
if (rc)
DRM_ERROR("DP display enable failed, rc=%d\n", rc);
rc = msm_dp_display_post_enable(msm_dp_display);
[Severity: High]
This is a pre-existing issue, but does execution proceed to post-enable even if
msm_dp_display_enable() fails?
If msm_dp_display_enable() fails, the error is logged, but there is no early
return. This allows msm_dp_display_post_enable() to proceed and configure
hardware features like PSR when clocks and power might not be properly
initialized, which could lead to hardware timeouts or unclocked access faults.
if (rc) {
DRM_ERROR("DP display post enable failed, rc=%d\n", rc);
- msm_dp_display_disable(dp);
+ msm_dp_display_disable(dp, dp->panel);
}
drm_dbg_dp(msm_dp_display->drm_dev, "type=%d Done\n", msm_dp_display->connector_type);
@@ -1479,7 +1481,7 @@ void msm_dp_display_atomic_post_disable(struct msm_dp *dp)
msm_dp_display_audio_notify_disable(msm_dp_display);
[Severity: High]
This isn't a bug introduced by this patch, but does this call sequence
sleep while in an atomic context?
msm_dp_display_atomic_post_disable() is an atomic commit callback, yet it calls
msm_dp_display_audio_notify_disable(), which can sleep:
These callbacks are DRM atomic modeset callbacks rather than
non-sleepable atomic contexts.
msm_dp_display_audio_notify_disable() {
...
if (!wait_for_completion_timeout(&dp->audio_comp, HZ * 5))
...
}
- msm_dp_display_disable(msm_dp_display);
+ msm_dp_display_disable(msm_dp_display, msm_dp_display->panel);
[Severity: High]
This is also a pre-existing issue, but does msm_dp_display_disable()
similarly sleep in this atomic context?
It calls msm_dp_link_psm_config(), which acquires a mutex:
msm_dp_link_psm_config() {
...
mutex_lock(&link->psm_mutex);
...
}
Calling sleeping functions in atomic contexts is forbidden by DRM/KMS
guidelines and could lead to kernel warnings, deadlocks, or system instability.
Same as above.
msm_dp_display_unprepare(msm_dp_display);
}
