Re: [PATCH v5 4/7] drm/bridge: lock the encoder chain in scoped for_each loops

[Louis Chauvet <louis.chauvet@bootlin.com>]

On 3/24/26 9:58 AM, Luca Ceresoli wrote:
> drm_for_each_bridge_in_chain_scoped() and
> drm_for_each_bridge_in_chain_from() currently get/put the bridge at each
> iteration. But they don't protect the encoder chain, so it could change
> (bridges added/removed) while some code is iterating over the list
> itself. Such code can then derail on incorrect pointers.
> 
> To make iterations safe, augment these for_each macros to lock the encoder
> chain mutex at the beginning and unlock it at the end of the loop (be it at
> the end of the list, or earlier due to a 'break' or 'return' statement).
> 
> This change requires more operations when starting and ending the loop. To
> avoid making the macros even more complex, move these operations to helper
> functions. Also remname some of the existing helper functions for
> consistency.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

> 
> ---
> 
> Changed in v4:
> - Slightly improve kerneldoc to clarify a bridge reference is held in
>    addition to the mutex
> 
> Changed in v3:
> - Re-add drm_bridge_get/put()
> 
> Changed in v2:
> - Fixed infinite loop in drm_for_each_bridge_in_chain_scoped() when
>    encoder->bridge_chain is empty, reported here:
>    https://lore.kernel.org/lkml/202509301358.38036b85-lkp@intel.com/
> - Slightly improved commit message
> ---
>   include/drm/drm_bridge.h | 73 +++++++++++++++++++++++++++++++-----------------
>   1 file changed, 47 insertions(+), 26 deletions(-)
> 
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 66ab89cf48aa..9aa10219ae36 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -1456,26 +1456,37 @@ drm_bridge_chain_get_last_bridge(struct drm_encoder *encoder)
>   						      struct drm_bridge, chain_node));
>   }
>   
> -/**
> - * drm_bridge_get_next_bridge_and_put - Get the next bridge in the chain
> - *                                      and put the previous
> - * @bridge: bridge object
> - *
> - * Same as drm_bridge_get_next_bridge() but additionally puts the @bridge.
> - *
> - * RETURNS:
> - * the next bridge in the chain after @bridge, or NULL if @bridge is the last.
> - */
> -static inline struct drm_bridge *
> -drm_bridge_get_next_bridge_and_put(struct drm_bridge *bridge)
> +/* Internal to drm_for_each_bridge_in_chain*() */
> +static inline struct drm_bridge *__drm_for_each_bridge_in_chain_next(struct drm_bridge *bridge)
>   {
>   	struct drm_bridge *next = drm_bridge_get_next_bridge(bridge);
>   
> +	if (!next)
> +		mutex_unlock(&bridge->encoder->bridge_chain_mutex);
> +
>   	drm_bridge_put(bridge);
>   
>   	return next;
>   }
>   
> +/* Internal to drm_for_each_bridge_in_chain*() */
> +DEFINE_FREE(__drm_for_each_bridge_in_chain_cleanup, struct drm_bridge *,
> +	if (_T) { mutex_unlock(&_T->encoder->bridge_chain_mutex); drm_bridge_put(_T); })
> +
> +/* Internal to drm_for_each_bridge_in_chain_scoped() */
> +static inline struct drm_bridge *
> +__drm_for_each_bridge_in_chain_scoped_start(struct drm_encoder *encoder)
> +{
> +	mutex_lock(&encoder->bridge_chain_mutex);
> +
> +	struct drm_bridge *bridge = drm_bridge_chain_get_first_bridge(encoder);
> +
> +	if (!bridge)
> +		mutex_unlock(&encoder->bridge_chain_mutex);
> +
> +	return bridge;
> +}
> +
>   /**
>    * drm_for_each_bridge_in_chain_scoped - iterate over all bridges attached
>    *                                       to an encoder
> @@ -1485,14 +1496,24 @@ drm_bridge_get_next_bridge_and_put(struct drm_bridge *bridge)
>    *
>    * Iterate over all bridges present in the bridge chain attached to @encoder.
>    *
> - * Automatically gets/puts the bridge reference while iterating, and puts
> - * the reference even if returning or breaking in the middle of the loop.
> + * Automatically gets/puts the bridge reference while iterating and locks
> + * the encoder chain mutex to prevent chain modifications while iterating.
>    */
> -#define drm_for_each_bridge_in_chain_scoped(encoder, bridge)		\
> -	for (struct drm_bridge *bridge __free(drm_bridge_put) =		\
> -	     drm_bridge_chain_get_first_bridge(encoder);		\
> -	     bridge;							\
> -	     bridge = drm_bridge_get_next_bridge_and_put(bridge))
> +#define drm_for_each_bridge_in_chain_scoped(encoder, bridge)				\
> +	for (struct drm_bridge *bridge __free(__drm_for_each_bridge_in_chain_cleanup) =	\
> +		__drm_for_each_bridge_in_chain_scoped_start((encoder));			\
> +	     bridge;									\
> +	     bridge = __drm_for_each_bridge_in_chain_next(bridge))			\
> +
> +/* Internal to drm_for_each_bridge_in_chain_from() */
> +static inline struct drm_bridge *
> +__drm_for_each_bridge_in_chain_from_start(struct drm_bridge *bridge)
> +{
> +	drm_bridge_get(bridge);
> +	mutex_lock(&bridge->encoder->bridge_chain_mutex);
> +
> +	return bridge;
> +}
>   
>   /**
>    * drm_for_each_bridge_in_chain_from - iterate over all bridges starting
> @@ -1504,14 +1525,14 @@ drm_bridge_get_next_bridge_and_put(struct drm_bridge *bridge)
>    * Iterate over all bridges in the encoder chain starting from
>    * @first_bridge, included.
>    *
> - * Automatically gets/puts the bridge reference while iterating, and puts
> - * the reference even if returning or breaking in the middle of the loop.
> + * Automatically gets/puts the bridge reference while iterating and locks
> + * the encoder chain mutex to prevent chain modifications while iterating.
>    */
> -#define drm_for_each_bridge_in_chain_from(first_bridge, bridge)		\
> -	for (struct drm_bridge *bridge __free(drm_bridge_put) =		\
> -		     drm_bridge_get(first_bridge);			\
> -	     bridge;							\
> -	     bridge = drm_bridge_get_next_bridge_and_put(bridge))
> +#define drm_for_each_bridge_in_chain_from(first_bridge, bridge)				\
> +	for (struct drm_bridge *bridge __free(__drm_for_each_bridge_in_chain_cleanup) =	\
> +		__drm_for_each_bridge_in_chain_from_start(first_bridge);		\
> +	     bridge;									\
> +	     bridge = __drm_for_each_bridge_in_chain_next(bridge))			\
>   
>   enum drm_mode_status
>   drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
>