Re: [PATCH v8 14/15] drm/msm/dp: separate dp_display_prepare() into its own API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Splitting `msm_dp_display_atomic_enable()` into `prepare` and `enable` drops the early return protection on failure paths, causing the display stream to be unconditionally enabled even if link preparation fails, leading to unclocked/unpowered hardware access and crashes.
--

--- Patch [14]: [PATCH v8 14/15] drm/msm/dp: separate dp_display_prepare() into its own API ---
commit b0785b0b736b2b15642aeae00b1fdb850ab52d5c
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>
drm/msm/dp: separate dp_display_prepare() into its own API

This commit splits the display enable sequence into separate link preparation
and stream enable functions. This allows MST link setup to run once for
multiple sinks, while the stream setup runs per-sink.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index deb21cee11830..74d885e38b8cf 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -1428,10 +1428,16 @@ void msm_dp_display_atomic_enable(struct msm_dp *msm_dp_display,
>  	}
>  
>  	rc = msm_dp_display_prepare_link(dp);
> -	if (rc) {
> +	if (rc)
>  		DRM_ERROR("DP display prepare failed, rc=%d\n", rc);
> -		return;
> -	}
> +}

[Severity: High]
By splitting the function and removing this early return, the failure state is
discarded. If crtc is NULL or hardware power/clocks fail to initialize in
msm_dp_display_prepare_link() (for example, if pm_runtime_resume_and_get()
fails), the caller will still proceed to the enable step.

Could this cause a crash (e.g., Synchronous External Abort) when accessing
unpowered hardware registers in the enable path?

> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> index aa98f101b093e..75ea52afd9f3e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -55,7 +55,8 @@ static void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>  	struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
>  	struct msm_dp *dp = dp_bridge->msm_dp_display;
>  
> -	msm_dp_display_atomic_enable(dp, state);
> +	msm_dp_display_atomic_prepare(dp, state);
> +	msm_dp_display_atomic_enable(dp);
>  }

[Severity: High]
Since msm_dp_display_atomic_prepare() returns void, this function (and
msm_edp_bridge_atomic_enable() below) unconditionally calls
msm_dp_display_atomic_enable() regardless of preparation success.

Should msm_dp_display_atomic_prepare() return an error code so callers can skip
enabling the stream if preparation fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-dp_mstclean-v8-0-bea261bf4e7d@oss.qualcomm.com?part=14