Re: [PATCH v3 1/3] drm/bridge: split hpd_mutex into two mutexes

[Sebastian Reichel <sebastian.reichel@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 3946 bytes --]

Hi,

On Thu, May 28, 2026 at 10:10:48AM +0300, Dmitry Baryshkov wrote:
> Currently almost all bridge drivers which implement hpd_enable /
> hpd_disable callbacks simply toggle the hardware registers generating
> the interrupt. However, as pointed out by Jonas Karlman and Sashiko bot,
> using those callbacks for enable_irq() / disable_irq() calls or
> scheduling and cancelling the work can cause a AB-BA deadlock (between
> hpd_mutex lock and the corresponding lock).
> 
> Split the hpd_mutex into two locks: one simply making sure that hpd_cb /
> hpd_data are consistent and another one, hpd_state_mutex, making sure
> that concurrent drm_bridge_hpd_enable() / drm_bridge_hpd_disable() calls
> can't end up with inconsistency between hpd_cb/_data and bridge's
> internal state.
> 
> Link: https://lore.kernel.org/dri-devel/9aa4bd35-bff6-4009-a959-ce31010c7b35@kwiboo.se
> Link: https://sashiko.dev/#/patchset/20260513-dp-connector-hpd-v2-0-42f757bfcbf9%40oss.qualcomm.com
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/gpu/drm/drm_bridge.c | 16 +++++++++++++---
>  include/drm/drm_bridge.h     |  4 ++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 687b36eea0c7..9a185032a3bd 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -417,6 +417,7 @@ void drm_bridge_add(struct drm_bridge *bridge)
>  	if (!list_empty(&bridge->list))
>  		list_del_init(&bridge->list);
>  
> +	mutex_init(&bridge->hpd_state_mutex);
>  	mutex_init(&bridge->hpd_mutex);
>  
>  	if (bridge->ops & DRM_BRIDGE_OP_HDMI)
> @@ -469,6 +470,7 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>  	mutex_unlock(&bridge_lock);
>  
>  	mutex_destroy(&bridge->hpd_mutex);
> +	mutex_destroy(&bridge->hpd_state_mutex);
>  
>  	drm_bridge_put(bridge);
>  }
> @@ -1451,19 +1453,25 @@ void drm_bridge_hpd_enable(struct drm_bridge *bridge,
>  	if (!(bridge->ops & DRM_BRIDGE_OP_HPD))
>  		return;
>  
> +	mutex_lock(&bridge->hpd_state_mutex);
> +
>  	mutex_lock(&bridge->hpd_mutex);
>  
> -	if (WARN(bridge->hpd_cb, "Hot plug detection already enabled\n"))
> +	if (WARN(bridge->hpd_cb, "Hot plug detection already enabled\n")) {
> +		mutex_unlock(&bridge->hpd_mutex);
>  		goto unlock;
> +	}
>  
>  	bridge->hpd_cb = cb;
>  	bridge->hpd_data = data;
>  
> +	mutex_unlock(&bridge->hpd_mutex);
> +
>  	if (bridge->funcs->hpd_enable)
>  		bridge->funcs->hpd_enable(bridge);
>  
>  unlock:
> -	mutex_unlock(&bridge->hpd_mutex);
> +	mutex_unlock(&bridge->hpd_state_mutex);
>  }
>  EXPORT_SYMBOL_GPL(drm_bridge_hpd_enable);
>  
> @@ -1484,13 +1492,15 @@ void drm_bridge_hpd_disable(struct drm_bridge *bridge)
>  	if (!(bridge->ops & DRM_BRIDGE_OP_HPD))
>  		return;
>  
> -	mutex_lock(&bridge->hpd_mutex);
> +	mutex_lock(&bridge->hpd_state_mutex);
>  	if (bridge->funcs->hpd_disable)
>  		bridge->funcs->hpd_disable(bridge);
>  
> +	mutex_lock(&bridge->hpd_mutex);
>  	bridge->hpd_cb = NULL;
>  	bridge->hpd_data = NULL;
>  	mutex_unlock(&bridge->hpd_mutex);
> +	mutex_unlock(&bridge->hpd_state_mutex);
>  }
>  EXPORT_SYMBOL_GPL(drm_bridge_hpd_disable);
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 4ba3a5deef9a..00a95f927e34 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -1256,6 +1256,10 @@ struct drm_bridge {
>  	 * @hpd_mutex: Protects the @hpd_cb and @hpd_data fields.
>  	 */
>  	struct mutex hpd_mutex;
> +	/**
> +	 * @hpd_state_mutex: Protects the HPD en/disablement state for the bridge.
> +	 */
> +	struct mutex hpd_state_mutex;
>  	/**
>  	 * @hpd_cb: Hot plug detection callback, registered with
>  	 * drm_bridge_hpd_enable().
> 
> -- 
> 2.47.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]