Re: [PATCH v2 07/20] drm/msm/dp: Serialize stream operations with HPD processing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm: sleepable mutex lock in atomic context (plugged_lock)
--

commit cb0347df94f2c11e58ecb3d98faa755fd4b526ef
Author: Xilin Wu <sophon@radxa.com>

drm/msm/dp: Serialize stream operations with HPD processing

HPD PHY test requests stop and restart the mainlink under plugged_lock,
but atomic stream enable and disable do not take that lock. Both paths
can check a resource flag before either updates it and acquire duplicate
clock or PHY references. They can also reset a link while the other path
is training it or enabling video.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 32a2ac89d8ed0..787efbcb6c124 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c

[ ... ]

> @@ -1449,7 +1468,13 @@ 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_prepare_link(dp);
> +	rc = msm_dp_display_prepare(dp);
> +	if (rc)
> +		return;
> +
> +	guard(mutex)(&dp->plugged_lock);

[Severity: High]
Does this introduce a sleeping lock into an atomic context?

The DRM subsystem guidelines state that encoder atomic enable/disable
callbacks run in atomic context during non-blocking commits. Calling sleeping
functions, like acquiring a mutex, from an atomic context can cause kernel
warnings, system instability, and potential deadlocks.

> +
> +	rc = msm_dp_display_prepare_link_locked(dp);
>  	if (rc) {
>  		DRM_ERROR("DP display prepare failed, rc=%d\n", rc);
>  		return;

[ ... ]

> @@ -1474,6 +1499,8 @@ void msm_dp_display_atomic_disable(struct msm_dp *dp)
>  
>  	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
>  
> +	guard(mutex)(&msm_dp_display->plugged_lock);

[Severity: High]
Does this also introduce a sleeping lock into an atomic disable callback?

> +
>  	/*
>  	 * If .atomic_enable() bailed out - link training failure is the common
>  	 * case - the mainlink was never brought up and ->power_on stayed false.

[ ... ]

> @@ -1511,7 +1538,8 @@ void msm_dp_display_atomic_post_disable(struct msm_dp *dp)
>  
>  	msm_dp_display_audio_notify_disable(msm_dp_display);
>  
> -	msm_dp_display_disable(msm_dp_display, msm_dp_display->panel);
> +	scoped_guard(mutex, &msm_dp_display->plugged_lock)
> +		msm_dp_display_disable(msm_dp_display, msm_dp_display->panel);

[Severity: High]
Could this scoped_guard() cause a sleep in atomic context during the
post-disable phase of an atomic commit?

>  
>  	msm_dp_display_unprepare(msm_dp_display);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-msm-dp-hdr10-v2-0-1835d4966da3@radxa.com?part=7