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

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Maxime,

On Fri, 3 Oct 2025 16:04:50 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Fri, Oct 03, 2025 at 12:39:26PM +0200, 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. To make iterations safe, change the logic of 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).
> > 
> > Also remove the get/put on the current bridge because it is not needed
> > anymore. In fact all bridges in the encoder chain are refcounted already
> > thanks to the drm_bridge_get() in drm_bridge_attach() and the
> > drm_bridge_put() in drm_bridge_detach(). So while iterating with the mutex
> > held the list cannot change _and_ the refcount of all bridges in the list
> > cannot drop to zero.  
> 
> This second paragraph *really* needs to be its own patch. And I'm not
> really sure playing games when it comes to refcounting is a good idea.
> 
> A strict, simple, rule is way easier to follow than trying to figure out
> two years from now why this loop skips the refcounting.
> 
> Unless you have a performance issue that is, in which case you should
> add a comment (and we will need a meaningful benchmark to back that
> claim).

Just to give some background, I have realized we need to lock the
encoder chain after drm_for_each_bridge_in_chain_scoped() was added.
Should I had realized it before, I would have sent it in the form you
can see in this patch, without the get/put because it is not necessary.
Not sure whether that would have changed the reception.

But I'm not aware of any performance issue, and the impact of
refcounting should not be small, soI'll try re-adding an explicit
get/put on top of the current version. It will likely make the macro
more complicated but should be reasonably doable. So, expect a v3 with
that change to we can all see how it looks.

Best regards,
Luca

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