Re: [PATCH v7 14/35] drm/msm/dp: replace power_on with active_stream_cnt

[sashiko-bot@kernel.org]

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 <quic_abhinavk@quicinc.com>
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/20260929-msm-dp-mst-v7-0-4b81473185e7@oss.qualcomm.com?part=14