Hello, On Mon, 29 Sep 2025 16:31:27 +0200 Luca Ceresoli <[email protected]> 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
