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

Luca Ceresoli
2025-10-03T10:37:46Z

Hello,

On Mon, 29 Sep 2025 16:31:27 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> 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