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: 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. > > msm_dp_display_unprepare(msm_dp_display); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
