Re: [PATCH v7 15/15] drm/msm/dp: pass panel to display enable/disable helpers

[sashiko-bot@kernel.org]

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 <yongxing.mou@oss.qualcomm.com>

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/20260609-dp_mstclean-v7-0-ea04113e8233@oss.qualcomm.com?part=15