Re: [PATCH 2/7] drm/encoder: drm_encoder_cleanup: take chain mutex while tearing down

[Luca Ceresoli]

Hello,

On Mon, 29 Sep 2025 16:31:27 +0200
Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> wrote:

> > > --- a/drivers/gpu/drm/drm_encoder.c
> > > +++ b/drivers/gpu/drm/drm_encoder.c
> > > @@ -195,9 +195,11 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> > >  	 * the indices on the drm_encoder after us in the encoder_list.
> > >  	 */
> > >  
> > > +	mutex_lock(&encoder->bridge_chain_mutex);
> > >  	list_for_each_entry_safe(bridge, next, &encoder->bridge_chain,
> > >  				 chain_node)
> > >  		drm_bridge_detach(bridge);
> > > +	mutex_unlock(&encoder->bridge_chain_mutex);    
> > 
> > You were claiming that the mutex was to prevent issues with concurrent
> > iteration and removal of the list members. list_for_each_entry_safe() is
> > explicitly made to protect against that. Why do we need both?  
> 
> You're right saying we don't need both. With a mutex preventing the list
> from any change, we can actually simpify code a bit to use the non-safe
> list macro:
> 
> -	struct drm_bridge *bridge, *next;
> +	struct drm_bridge *bridge;
> ...
> +	mutex_lock(&encoder->bridge_chain_mutex);
> - 	list_for_each_entry_safe(bridge, next, &encoder->bridge_chain,
> + 	list_for_each_entry(bridge, &encoder->bridge_chain,
>  				 chain_node)
>  		drm_bridge_detach(bridge);
> +	mutex_unlock(&encoder->bridge_chain_mutex);

After looking at it better I realized the _safe variant here is still
needed as the current loop entry is removed inside the loop. The
non-safe version, at the end of the first iteration, would look for the
next element in the now-removed list_head, thus being derailed.

v2 on its way with this taken into account along with the other
discussed items.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com