Re: [PATCH v5 5/7] drm/bridge: prevent encoder chain changes while iterating with list_for_each_entry_from()

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

On 3/24/26 9:58 AM, Luca Ceresoli wrote:
> These loops in drm_bridge.c iterate over the encoder chain using
> list_for_each_entry_from(), which does not prevent changes to the bridge
> chain while iterating over it.
> 
> Convert most of those loops to instead use
> drm_for_each_bridge_in_chain_from(), which locks the chain.
> 
> This also simplifies code.
> 
> All the "simple" loops are converted here. The only ones not touched are
> those in drm_atomic_bridge_chain_pre_enable() and
> drm_atomic_bridge_chain_post_disable(), because they have nested loops
> which are not well handled by drm_for_each_bridge_in_chain_from(). Those
> two functions are handled by a separate commit.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

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

> ---
>   drivers/gpu/drm/drm_bridge.c | 35 ++++++++++++-----------------------
>   1 file changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 42ca4b243579..37feb8143de4 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -725,7 +725,7 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>   /**
>    * drm_bridge_chain_mode_valid - validate the mode against all bridges in the
>    *				 encoder chain.
> - * @bridge: bridge control structure
> + * @first_bridge: bridge control structure
>    * @info: display info against which the mode shall be validated
>    * @mode: desired mode to be validated
>    *
> @@ -739,17 +739,14 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>    * MODE_OK on success, drm_mode_status Enum error code on failure
>    */
>   enum drm_mode_status
> -drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
> +drm_bridge_chain_mode_valid(struct drm_bridge *first_bridge,
>   			    const struct drm_display_info *info,
>   			    const struct drm_display_mode *mode)
>   {
> -	struct drm_encoder *encoder;
> -
> -	if (!bridge)
> +	if (!first_bridge)
>   		return MODE_OK;
>   
> -	encoder = bridge->encoder;
> -	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> +	drm_for_each_bridge_in_chain_from(first_bridge, bridge) {
>   		enum drm_mode_status ret;
>   
>   		if (!bridge->funcs->mode_valid)
> @@ -767,7 +764,7 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_valid);
>   /**
>    * drm_bridge_chain_mode_set - set proposed mode for all bridges in the
>    *			       encoder chain
> - * @bridge: bridge control structure
> + * @first_bridge: bridge control structure
>    * @mode: desired mode to be set for the encoder chain
>    * @adjusted_mode: updated mode that works for this encoder chain
>    *
> @@ -776,20 +773,16 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_valid);
>    *
>    * Note: the bridge passed should be the one closest to the encoder
>    */
> -void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
> +void drm_bridge_chain_mode_set(struct drm_bridge *first_bridge,
>   			       const struct drm_display_mode *mode,
>   			       const struct drm_display_mode *adjusted_mode)
>   {
> -	struct drm_encoder *encoder;
> -
> -	if (!bridge)
> +	if (!first_bridge)
>   		return;
>   
> -	encoder = bridge->encoder;
> -	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> +	drm_for_each_bridge_in_chain_from(first_bridge, bridge)
>   		if (bridge->funcs->mode_set)
>   			bridge->funcs->mode_set(bridge, mode, adjusted_mode);
> -	}
>   }
>   EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>   
> @@ -1013,7 +1006,7 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
>   
>   /**
>    * drm_atomic_bridge_chain_enable - enables all bridges in the encoder chain
> - * @bridge: bridge control structure
> + * @first_bridge: bridge control structure
>    * @state: atomic state being committed
>    *
>    * Calls &drm_bridge_funcs.atomic_enable (falls back on
> @@ -1023,22 +1016,18 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
>    *
>    * Note: the bridge passed should be the one closest to the encoder
>    */
> -void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
> +void drm_atomic_bridge_chain_enable(struct drm_bridge *first_bridge,
>   				    struct drm_atomic_state *state)
>   {
> -	struct drm_encoder *encoder;
> -
> -	if (!bridge)
> +	if (!first_bridge)
>   		return;
>   
> -	encoder = bridge->encoder;
> -	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> +	drm_for_each_bridge_in_chain_from(first_bridge, bridge)
>   		if (bridge->funcs->atomic_enable) {
>   			bridge->funcs->atomic_enable(bridge, state);
>   		} else if (bridge->funcs->enable) {
>   			bridge->funcs->enable(bridge);
>   		}
> -	}
>   }
>   EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
>   
>