Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/msm/dp: active_stream_cnt integer underflow on link preparation failure - [High] drm/msm/dp: PM runtime and PHY resource leak on failed link preparation - [Low] drm/msm/dp: misleading debug message in unprepare path --
commit 76ccdbadef8f1f0f27aff459380cb7a38f3abbd5 Author: Yongxing Mou <[email protected]> drm/msm/dp: add link_ready to manage link-level operations In MST mode, multiple streams share the same DP link. Track a link_ready state so msm_dp_display_prepare_link() runs only once per link and repeated calls are skipped. > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index 49a718f4e4649..487c76e0f990d 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c [ ... ] > @@ -1501,14 +1509,16 @@ void msm_dp_display_enable_helper(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, msm_dp_panel); > - if (rc) > - DRM_ERROR("DP display enable failed, rc=%d\n", rc); > + if (msm_dp_display->link_ready) { > + rc = msm_dp_display_enable(dp, msm_dp_panel); > + if (rc) > + DRM_ERROR("DP display enable failed, rc=%d\n", rc); [Severity: High] If msm_dp_display_prepare_link() failed (e.g., sink_count == 0), link_ready is false and we skip calling msm_dp_display_enable(). This means active_stream_cnt is never incremented. However, during atomic teardown, msm_dp_display_atomic_post_disable_helper() unconditionally calls msm_dp_display_disable(), which will decrement the counter regardless: msm_dp_display_disable() { ... msm_dp_display->active_stream_cnt--; ... } Can this cause active_stream_cnt to underflow to 0xFFFFFFFF? If it underflows, a subsequent call to msm_dp_display_prepare_link() would fail to initialize the PHY because it checks: if (!msm_dp_display->active_stream_cnt) { msm_dp_display_host_phy_init(dp); ... > > - rc = msm_dp_display_post_enable(msm_dp_display); > - if (rc) { > - DRM_ERROR("DP display post enable failed, rc=%d\n", rc); > - msm_dp_display_disable(dp, msm_dp_panel); > + rc = msm_dp_display_post_enable(msm_dp_display); > + if (rc) { > + DRM_ERROR("DP display post enable failed, rc=%d\n", rc); > + msm_dp_display_disable(dp, msm_dp_panel); > + } > } > > drm_dbg_dp(msm_dp_display->drm_dev, "type=%d Done\n", > msm_dp_display->connector_type); > @@ -1565,6 +1575,11 @@ void msm_dp_display_unprepare(struct msm_dp > *msm_dp_display) > > dp = container_of(msm_dp_display, struct msm_dp_display_private, > msm_dp_display); > > + if (!msm_dp_display->link_ready) { > + drm_dbg_dp(dp->drm_dev, "Link already setup, return\n"); [Severity: Low] Does this debug message contradict the check? It says "Link already setup", but it is printed when link_ready is false. [Severity: High] If msm_dp_display_prepare_link() failed after successfully calling pm_runtime_resume_and_get(), link_ready remains false. For example: msm_dp_display_prepare_link() { ... rc = pm_runtime_resume_and_get(&msm_dp_display->pdev->dev); ... if (dp->link->sink_count == 0) return -ENOTCONN; ... } When msm_dp_display_unprepare() is later called, this new early return will cause us to skip the cleanup code: msm_dp_display_host_phy_exit(dp); pm_runtime_put_sync(&msm_dp_display->pdev->dev); Will this result in leaking the PM runtime reference and leaving the PHY initialized? > + return; > + } > + > if (msm_dp_display->active_stream_cnt) { > drm_dbg_dp(dp->drm_dev, "stream still active, return\n"); > return; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
