Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] msm: Unconditional stream decrement causes unclocked access and system crash - [Medium] msm: Unconditional stream count increment hides stream enablement failure --
commit fe53f5f1bd8c24a505850b13542381e72e0597f7 Author: Abhinav Kumar <[email protected]> Subject: drm/msm/dp: replace power_on with active_stream_cnt This commit replaces the power_on boolean with an active_stream_cnt to track the lifetime of the shared link for DP MST. It performs link initialization when the first stream is enabled and defers teardown until the last stream is disabled. > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -667,14 +667,10 @@ static int msm_dp_display_enable(struct > msm_dp_display_private *dp, [ ... ] > rc = msm_dp_ctrl_on_stream(dp->ctrl, msm_dp_panel, > msm_dp_display->mst_active); > - if (!rc) > - msm_dp_display->power_on = true; > + > + msm_dp_display->active_stream_cnt++; > > return rc; > } [Severity: Medium] Does this code introduce a regression by unconditionally incrementing active_stream_cnt even when the stream enablement fails? If msm_dp_ctrl_on_stream() encounters an error like a DP timeout or link training failure, it returns an error code in rc. The old code only updated the state if rc was zero. The new code increments active_stream_cnt regardless of the return code. Could this leave the software state out of sync with the hardware? If the caller logs the error but proceeds to post_enable successfully, the disable rollback path won't be invoked. This leaves active_stream_cnt artificially inflated. > @@ -723,13 +719,12 @@ static int msm_dp_display_disable(struct > msm_dp_display_private *dp, > { > struct msm_dp *msm_dp_display = &dp->msm_dp_display; > > - if (!msm_dp_display->power_on) > - return 0; > - > msm_dp_panel_disable_vsc_sdp(msm_dp_panel); [Severity: High] Does removing this guard cause a regression where unclocked hardware is accessed if a stream failed to enable? If a DRM atomic commit fails to prepare the link (for instance, if pm_runtime_resume_and_get() or link training fails), the DRM state can still be updated to enabled. When a userspace process like a Wayland compositor subsequently disables the display, msm_dp_display_disable() is invoked unconditionally. Without the early return guard, this calls msm_dp_panel_disable_vsc_sdp() without active clocks, which triggers an unclocked MMIO read of MMSS_DP_SDP_CFG that crashes Qualcomm SoCs. > > msm_dp_ctrl_off_pixel_clk(dp->ctrl, msm_dp_panel->stream_id); > > + msm_dp_display->active_stream_cnt--; [Severity: High] Can this unconditional decrement also cause a u32 underflow regression? If the stream failed to enable and active_stream_cnt is 0, decrementing it here unconditionally will cause it to wrap around to 0xFFFFFFFF, which might break subsequent PM runtime put and PHY teardown logic. > + > drm_dbg_dp(dp->drm_dev, "sink count: %d\n", dp->link->sink_count); > return 0; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
