[PATCH 0/7] drm/bridge: protect encoder bridge chain with a mutex

[PATCH 0/7] drm/bridge: protect encoder bridge chain with a mutex

[Luca Ceresoli]

This series ensures that the bridge chain of the encoder will not be
modified while some other code flows are iterating over it.

This is part of the work towards removal of bridges from a still existing
DRM pipeline without use-after-free. The grand plan was discussed in [1].
Here's the work breakdown (➜ marks the current series):

 1. ➜ add refcounting to DRM bridges (struct drm_bridge)
    (based on devm_drm_bridge_alloc() [0])
    A. ✔ add new alloc API and refcounting (v6.16)
    B. ✔ convert all bridge drivers to new API (v6.17)
    C. ✔ kunit tests (v6.17)
    D. ✔ add get/put to drm_bridge_add/remove() + attach/detach()
         and warn on old allocation pattern (v6.17)
    E. … add get/put on drm_bridge accessors
       1. ✔ drm_bridge_chain_get_first_bridge() + add a cleanup action
            (drm-misc-next)
       2. ✔ drm_bridge_get_prev_bridge() (drm-misc-next)
       3. ✔ drm_bridge_get_next_bridge() (drm-misc-next)
       4. ✔ drm_for_each_bridge_in_chain() (drm-misc-next)
       5. … drm_bridge_connector_init
       6. ➜  protect encoder bridge chain with a mutex
       7. of_drm_find_bridge
       8. drm_of_find_panel_or_bridge, *_of_get_bridge
    F. ➜ debugfs improvements
       1. ✔ add top-level 'bridges' file (v6.16)
       2. ✔ show refcount and list removed bridges (drm-misc-next)
 2. … handle gracefully atomic updates during bridge removal
 3. … DSI host-device driver interaction
 4. removing the need for the "always-disconnected" connector
 5. finish the hotplug bridge work, moving code to the core and potentially
    removing the hotplug-bridge itself (this needs to be clarified as
    points 1-3 are developed)

The per-encoder bridge chain is currently assumed to be static once it is
fully initialized. Work is in progress to add hot-pluggable bridges,
breaking that assumption.

With hotplug and especially hot-unplug, bridges will be added and removed
without notice, and thus be added/removed to/from the encoder chain in
drm_bridge_attach/detach(), concurrently to the code iterating on the
chain. This can result in disruption of the code iterating over the
chain. Avoid it by introducing a mutex to make list insertion, removal and
iterations mutually exclusive.

[1] https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/#t

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (7):
      drm/encoder: add mutex to protect the bridge chain
      drm/encoder: drm_encoder_cleanup: take chain mutex while tearing down
      drm/bridge: lock the encoder bridge chain mutex during insertion
      drm/bridge: lock the encoder chain in scoped for_each loops
      drm/bridge: prevent encoder chain changes while iterating with list_for_each_entry_from()
      drm/bridge: prevent encoder chain changes while iterating with list_for_each_entry_reverse()
      drm/bridge: prevent encoder chain changes while iterating in drm_atomic_bridge_chain_post_disable/pre_enable()

 drivers/gpu/drm/drm_bridge.c  | 56 +++++++++++++++++++++-----------------
 drivers/gpu/drm/drm_encoder.c |  4 +++
 include/drm/drm_bridge.h      | 62 +++++++++++++++++++++++--------------------
 include/drm/drm_encoder.h     | 18 +++++++++++++
 4 files changed, 87 insertions(+), 53 deletions(-)
---
base-commit: a7493ff9ad96868f4c1c16813b205ba812a7573c
change-id: 20250925-drm-bridge-alloc-encoder-chain-mutex-b78d62085ee5

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH 1/7] drm/encoder: add mutex to protect the bridge chain

[Luca Ceresoli]

The per-encoder bridge chain is currently assumed to be static once it is
fully initialized. Work is in progress to add hot-pluggable bridges,
breaking that assumption.

With bridge removal, the encoder chain can change without notice, removing
tail bridges. This can be problematic while iterating over the chain.

Add a mutex to be taken whenever looping or changing the encoder chain.

Also add two APIs to lock/unlock the mutex without the need to manipulate
internal struct drm_encoder fields.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_encoder.c |  2 ++
 include/drm/drm_encoder.h     | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 8f2bc6a28482229fd0b030a1958f87753ad7885f..3261f142baea30c516499d23dbf8d0acf5952cd6 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -129,6 +129,7 @@ static int __drm_encoder_init(struct drm_device *dev,
 	}
 
 	INIT_LIST_HEAD(&encoder->bridge_chain);
+	mutex_init(&encoder->bridge_chain_mutex);
 	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
 	encoder->index = dev->mode_config.num_encoder++;
 
@@ -202,6 +203,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 	kfree(encoder->name);
 	list_del(&encoder->head);
 	dev->mode_config.num_encoder--;
+	mutex_destroy(&encoder->bridge_chain_mutex);
 
 	memset(encoder, 0, sizeof(*encoder));
 }
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 977a9381c8ba943b4d3e021635ea14856df8a17d..6c962de640a345bfbb18308c83076628547c9ab9 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -25,6 +25,7 @@
 
 #include <linux/list.h>
 #include <linux/ctype.h>
+#include <linux/mutex.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_mode.h>
 #include <drm/drm_mode_object.h>
@@ -189,6 +190,9 @@ struct drm_encoder {
 	 */
 	struct list_head bridge_chain;
 
+	/** @bridge_chain_mutex: protect bridge_chain from changes while iterating */
+	struct mutex bridge_chain_mutex;
+
 	const struct drm_encoder_funcs *funcs;
 	const struct drm_encoder_helper_funcs *helper_private;
 
@@ -319,6 +323,20 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
 	return mo ? obj_to_encoder(mo) : NULL;
 }
 
+static inline struct drm_encoder *drm_encoder_chain_lock(struct drm_encoder *encoder)
+{
+	if (!WARN_ON_ONCE(!encoder))
+		mutex_lock(&encoder->bridge_chain_mutex);
+
+	return encoder;
+}
+
+static inline void drm_encoder_chain_unlock(struct drm_encoder *encoder)
+{
+	if (!WARN_ON_ONCE(!encoder))
+		mutex_unlock(&encoder->bridge_chain_mutex);
+}
+
 void drm_encoder_cleanup(struct drm_encoder *encoder);
 
 /**

-- 
2.51.0

Re: [PATCH 1/7] drm/encoder: add mutex to protect the bridge chain

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3101 bytes --]

On Fri, Sep 26, 2025 at 05:59:42PM +0200, Luca Ceresoli wrote:
> The per-encoder bridge chain is currently assumed to be static once it is
> fully initialized. Work is in progress to add hot-pluggable bridges,
> breaking that assumption.
> 
> With bridge removal, the encoder chain can change without notice, removing
> tail bridges. This can be problematic while iterating over the chain.
> 
> Add a mutex to be taken whenever looping or changing the encoder chain.
> 
> Also add two APIs to lock/unlock the mutex without the need to manipulate
> internal struct drm_encoder fields.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/drm_encoder.c |  2 ++
>  include/drm/drm_encoder.h     | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index 8f2bc6a28482229fd0b030a1958f87753ad7885f..3261f142baea30c516499d23dbf8d0acf5952cd6 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -129,6 +129,7 @@ static int __drm_encoder_init(struct drm_device *dev,
>  	}
>  
>  	INIT_LIST_HEAD(&encoder->bridge_chain);
> +	mutex_init(&encoder->bridge_chain_mutex);
>  	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
>  	encoder->index = dev->mode_config.num_encoder++;
>  
> @@ -202,6 +203,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
>  	kfree(encoder->name);
>  	list_del(&encoder->head);
>  	dev->mode_config.num_encoder--;
> +	mutex_destroy(&encoder->bridge_chain_mutex);
>  
>  	memset(encoder, 0, sizeof(*encoder));
>  }
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 977a9381c8ba943b4d3e021635ea14856df8a17d..6c962de640a345bfbb18308c83076628547c9ab9 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -25,6 +25,7 @@
>  
>  #include <linux/list.h>
>  #include <linux/ctype.h>
> +#include <linux/mutex.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_mode_object.h>
> @@ -189,6 +190,9 @@ struct drm_encoder {
>  	 */
>  	struct list_head bridge_chain;
>  
> +	/** @bridge_chain_mutex: protect bridge_chain from changes while iterating */
> +	struct mutex bridge_chain_mutex;
> +
>  	const struct drm_encoder_funcs *funcs;
>  	const struct drm_encoder_helper_funcs *helper_private;
>  
> @@ -319,6 +323,20 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
>  	return mo ? obj_to_encoder(mo) : NULL;
>  }
>  
> +static inline struct drm_encoder *drm_encoder_chain_lock(struct drm_encoder *encoder)
> +{
> +	if (!WARN_ON_ONCE(!encoder))
> +		mutex_lock(&encoder->bridge_chain_mutex);
> +
> +	return encoder;
> +}
> +
> +static inline void drm_encoder_chain_unlock(struct drm_encoder *encoder)
> +{
> +	if (!WARN_ON_ONCE(!encoder))
> +		mutex_unlock(&encoder->bridge_chain_mutex);
> +}
> +

Please provide documentation for these two functions. In particular, why
do we need to return the encoder pointer?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 1/7] drm/encoder: add mutex to protect the bridge chain

[Luca Ceresoli]

Hi Maxime,

On Mon, 29 Sep 2025 14:43:46 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Fri, Sep 26, 2025 at 05:59:42PM +0200, Luca Ceresoli wrote:
> > The per-encoder bridge chain is currently assumed to be static once it is
> > fully initialized. Work is in progress to add hot-pluggable bridges,
> > breaking that assumption.
> > 
> > With bridge removal, the encoder chain can change without notice, removing
> > tail bridges. This can be problematic while iterating over the chain.
> > 
> > Add a mutex to be taken whenever looping or changing the encoder chain.
> > 
> > Also add two APIs to lock/unlock the mutex without the need to manipulate
> > internal struct drm_encoder fields.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_encoder.c |  2 ++
> >  include/drm/drm_encoder.h     | 18 ++++++++++++++++++
> >  2 files changed, 20 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > index 8f2bc6a28482229fd0b030a1958f87753ad7885f..3261f142baea30c516499d23dbf8d0acf5952cd6 100644
> > --- a/drivers/gpu/drm/drm_encoder.c
> > +++ b/drivers/gpu/drm/drm_encoder.c
> > @@ -129,6 +129,7 @@ static int __drm_encoder_init(struct drm_device *dev,
> >  	}
> >  
> >  	INIT_LIST_HEAD(&encoder->bridge_chain);
> > +	mutex_init(&encoder->bridge_chain_mutex);
> >  	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
> >  	encoder->index = dev->mode_config.num_encoder++;
> >  
> > @@ -202,6 +203,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> >  	kfree(encoder->name);
> >  	list_del(&encoder->head);
> >  	dev->mode_config.num_encoder--;
> > +	mutex_destroy(&encoder->bridge_chain_mutex);
> >  
> >  	memset(encoder, 0, sizeof(*encoder));
> >  }
> > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> > index 977a9381c8ba943b4d3e021635ea14856df8a17d..6c962de640a345bfbb18308c83076628547c9ab9 100644
> > --- a/include/drm/drm_encoder.h
> > +++ b/include/drm/drm_encoder.h
> > @@ -25,6 +25,7 @@
> >  
> >  #include <linux/list.h>
> >  #include <linux/ctype.h>
> > +#include <linux/mutex.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_mode.h>
> >  #include <drm/drm_mode_object.h>
> > @@ -189,6 +190,9 @@ struct drm_encoder {
> >  	 */
> >  	struct list_head bridge_chain;
> >  
> > +	/** @bridge_chain_mutex: protect bridge_chain from changes while iterating */
> > +	struct mutex bridge_chain_mutex;
> > +
> >  	const struct drm_encoder_funcs *funcs;
> >  	const struct drm_encoder_helper_funcs *helper_private;
> >  
> > @@ -319,6 +323,20 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
> >  	return mo ? obj_to_encoder(mo) : NULL;
> >  }
> >  
> > +static inline struct drm_encoder *drm_encoder_chain_lock(struct drm_encoder *encoder)
> > +{
> > +	if (!WARN_ON_ONCE(!encoder))
> > +		mutex_lock(&encoder->bridge_chain_mutex);
> > +
> > +	return encoder;
> > +}
> > +
> > +static inline void drm_encoder_chain_unlock(struct drm_encoder *encoder)
> > +{
> > +	if (!WARN_ON_ONCE(!encoder))
> > +		mutex_unlock(&encoder->bridge_chain_mutex);
> > +}
> > +  
> 
> Please provide documentation for these two functions.

Ah, sure, here is a draft:

/**
 * drm_encoder_chain_lock - lock the encoder bridge chain
 * @encoder: encoder whose bridge chain must be locked
 *
 * Locks the mutex protecting the bridge chain from concurrent access.
 * To be called by code modifying ot iterating over the bridge chain to
 * prevent the list from changing while iterating over it.
 * Call drm_encoder_chain_unlock() to unlock the mutex.
 *
 * Returns:
 * Pointer to @encoder. Useful to lock the chain and then operate on the
 * in the same statement, e.g.
 * list_first_entry(&drm_encoder_chain_lock(encoder)->bridge_chain)
 */
static inline struct drm_encoder *drm_encoder_chain_lock(struct drm_encoder *encoder)
{
	if (!WARN_ON_ONCE(!encoder))
		mutex_lock(&encoder->bridge_chain_mutex);

	return encoder;
}

/**
 * drm_encoder_chain_unlock - unlock the encoder bridge chain
 * @encoder: encoder whose bridge chain must be unlocked
 *
 * Unlocks the mutex protecting the bridge chain from concurrent access,
 * matching drm_encoder_chain_lock().
 */
static inline void drm_encoder_chain_unlock(struct drm_encoder *encoder)
{
	if (!WARN_ON_ONCE(!encoder))
		mutex_unlock(&encoder->bridge_chain_mutex);
}

> In particular, why
> do we need to return the encoder pointer?

It allows to lock the encoder chain and then dereference the pointer,
similar to what drm_bridge_get() does. of_node_get(), which I used as a
model for these functions, does the same.

This one in particular is used in patch 4 to write the new scoped
macros. Locking the encoder chain and then dereferencing the encoder
would be very awkward in a macro if drm_encoder_chain_lock() didn't
return the encoder.

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

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

[Luca Ceresoli]

drm_encoder_cleanup() modifies the encoder chain by removing bridges via
drm_bridge_detach(). Protect this whole operation by taking the mutex, so
any users iterating over the chain will not access it during the change.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_encoder.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 3261f142baea30c516499d23dbf8d0acf5952cd6..3a04bedf9b759acd6826864b7f2cc9b861a8f170 100644
--- 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);
 
 	drm_mode_object_unregister(dev, &encoder->base);
 	kfree(encoder->name);

-- 
2.51.0

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

On Fri, Sep 26, 2025 at 05:59:43PM +0200, Luca Ceresoli wrote:
> drm_encoder_cleanup() modifies the encoder chain by removing bridges via
> drm_bridge_detach(). Protect this whole operation by taking the mutex, so
> any users iterating over the chain will not access it during the change.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/drm_encoder.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index 3261f142baea30c516499d23dbf8d0acf5952cd6..3a04bedf9b759acd6826864b7f2cc9b861a8f170 100644
> --- 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?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Luca Ceresoli]

Hi Maxime,

On Mon, 29 Sep 2025 14:45:10 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Fri, Sep 26, 2025 at 05:59:43PM +0200, Luca Ceresoli wrote:
> > drm_encoder_cleanup() modifies the encoder chain by removing bridges via
> > drm_bridge_detach(). Protect this whole operation by taking the mutex, so
> > any users iterating over the chain will not access it during the change.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_encoder.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > index 3261f142baea30c516499d23dbf8d0acf5952cd6..3a04bedf9b759acd6826864b7f2cc9b861a8f170 100644
> > --- 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);
 
But it's not fully correct that list_for_each_entry_safe() protects
against concurrent removal. As I see it, all the _safe variants of
list_for_each*() macros protect against one specific and frequent use
case:

	list_for_each_entry_safe(curs, n, some_list, membername) {
		...
		list_del(&curs->membername);
		...
	}

So the _safe variant protect from removing the element at the current
iteration (*curs). It does so by taking the following element pointer in
advance, in the auxiliary variable @n. But _concurrent_ removal (the
topic of this series) means the element being removed could just be the
following one.

Consider this sequence:

 1. start loop: list_for_each_entry_safe() sets;
    pos = list_first_entry()   = <bridge 1>
    n   = list_next_entry(pos) = <bridge 2>
 2. enter the loop 1st time, do something with *pos (bridge 1)
 3. in the meanwhile bridge 2 is hot-unplugged
    -> another thread removes item 2 -> drm_bridge_detach()
    -> list_del() sets bridge->next = LIST_POISON1
 4. loop iteration 1 finishes, list_for_each_entry_safe() sets:
    pos = n (previously set to bridge 2)
    n   = (bridge 2)->next = LIST_POISON1
 5. enter the loop 2nd time, do something with *pos (bridge 2)
 6. loop iteration 2 finishes, list_for_each_entry_safe() sets:
    pos = n (previously set to LIST_POISON1) -> bug

Do you think this explains the need for this series?

If it does, I should probably add this to the cover letter and maybe
patch 1.

Luca

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

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@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
https://bootlin.com

[PATCH 3/7] drm/bridge: lock the encoder bridge chain mutex during insertion

[Luca Ceresoli]

drm_bridge_attach() modifies the encoder bridge chain, so take a mutex
around such operations to allow users of the chain to protect themselves
from chain modifications while iterating.

This change does not apply to drm_bridge_detach() because:
 * only the drm_encoder.c calls it, not bridge drivers (unlike
   drm_bridge_attach())
 * the only drm_bridge_detach() caller is drm_encoder_cleanup() which
   already locks the mutex for the entire cleanup loop, thus additionally
   locking it here would deadlock
 * drm_bridge_detach() is recursively calling itself along the chain, so
   care would be needed to avoid deadlocks
Add a comment to clarify that is intended.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_bridge.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 630b5e6594e0affad9ba48791207c7b403da5db8..90e467cf91a134342c80d2f958b928472aaf0d8b 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -453,10 +453,12 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	bridge->dev = encoder->dev;
 	bridge->encoder = encoder;
 
+	drm_encoder_chain_lock(encoder);
 	if (previous)
 		list_add(&bridge->chain_node, &previous->chain_node);
 	else
 		list_add(&bridge->chain_node, &encoder->bridge_chain);
+	drm_encoder_chain_unlock(encoder);
 
 	if (bridge->funcs->attach) {
 		ret = bridge->funcs->attach(bridge, encoder, flags);
@@ -487,7 +489,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 err_reset_bridge:
 	bridge->dev = NULL;
 	bridge->encoder = NULL;
+	drm_encoder_chain_lock(encoder);
 	list_del(&bridge->chain_node);
+	drm_encoder_chain_unlock(encoder);
 
 	if (ret != -EPROBE_DEFER)
 		DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
@@ -503,6 +507,11 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_bridge_attach);
 
+/*
+ * Invoked by the encoder during encoder cleanup in drm_encoder_cleanup(),
+ * so should generally *not* be called by driver code. As opposed to
+ * drm_bridge_attach(), does not lock the encoder chain mutex.
+ */
 void drm_bridge_detach(struct drm_bridge *bridge)
 {
 	if (WARN_ON(!bridge))

-- 
2.51.0

Re: [PATCH 3/7] drm/bridge: lock the encoder bridge chain mutex during insertion

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]

On Fri, Sep 26, 2025 at 05:59:44PM +0200, Luca Ceresoli wrote:
> drm_bridge_attach() modifies the encoder bridge chain, so take a mutex
> around such operations to allow users of the chain to protect themselves
> from chain modifications while iterating.
> 
> This change does not apply to drm_bridge_detach() because:
>  * only the drm_encoder.c calls it, not bridge drivers (unlike
>    drm_bridge_attach())
>  * the only drm_bridge_detach() caller is drm_encoder_cleanup() which
>    already locks the mutex for the entire cleanup loop, thus additionally
>    locking it here would deadlock
>  * drm_bridge_detach() is recursively calling itself along the chain, so
>    care would be needed to avoid deadlocks
> Add a comment to clarify that is intended.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/drm_bridge.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 630b5e6594e0affad9ba48791207c7b403da5db8..90e467cf91a134342c80d2f958b928472aaf0d8b 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -453,10 +453,12 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	bridge->dev = encoder->dev;
>  	bridge->encoder = encoder;
>  
> +	drm_encoder_chain_lock(encoder);
>  	if (previous)
>  		list_add(&bridge->chain_node, &previous->chain_node);
>  	else
>  		list_add(&bridge->chain_node, &encoder->bridge_chain);
> +	drm_encoder_chain_unlock(encoder);
>  
>  	if (bridge->funcs->attach) {
>  		ret = bridge->funcs->attach(bridge, encoder, flags);
> @@ -487,7 +489,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  err_reset_bridge:
>  	bridge->dev = NULL;
>  	bridge->encoder = NULL;
> +	drm_encoder_chain_lock(encoder);
>  	list_del(&bridge->chain_node);
> +	drm_encoder_chain_unlock(encoder);
>  
>  	if (ret != -EPROBE_DEFER)
>  		DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
> @@ -503,6 +507,11 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL(drm_bridge_attach);
>  
> +/*
> + * Invoked by the encoder during encoder cleanup in drm_encoder_cleanup(),
> + * so should generally *not* be called by driver code.

Why not? Also, it looks entirely unrelated to the rest of the patch.

> As opposed to + * drm_bridge_attach(), does not lock the encoder chain mutex. + */ void
> drm_bridge_detach(struct drm_bridge *bridge) { if (WARN_ON(!bridge))

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 3/7] drm/bridge: lock the encoder bridge chain mutex during insertion

[Luca Ceresoli]

On Mon, 29 Sep 2025 14:46:18 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Fri, Sep 26, 2025 at 05:59:44PM +0200, Luca Ceresoli wrote:
> > drm_bridge_attach() modifies the encoder bridge chain, so take a mutex
> > around such operations to allow users of the chain to protect themselves
> > from chain modifications while iterating.
> > 
> > This change does not apply to drm_bridge_detach() because:
> >  * only the drm_encoder.c calls it, not bridge drivers (unlike
> >    drm_bridge_attach())
> >  * the only drm_bridge_detach() caller is drm_encoder_cleanup() which
> >    already locks the mutex for the entire cleanup loop, thus additionally
> >    locking it here would deadlock
> >  * drm_bridge_detach() is recursively calling itself along the chain, so
> >    care would be needed to avoid deadlocks
> > Add a comment to clarify that is intended.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 630b5e6594e0affad9ba48791207c7b403da5db8..90e467cf91a134342c80d2f958b928472aaf0d8b 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -453,10 +453,12 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >  	bridge->dev = encoder->dev;
> >  	bridge->encoder = encoder;
> >  
> > +	drm_encoder_chain_lock(encoder);
> >  	if (previous)
> >  		list_add(&bridge->chain_node, &previous->chain_node);
> >  	else
> >  		list_add(&bridge->chain_node, &encoder->bridge_chain);
> > +	drm_encoder_chain_unlock(encoder);
> >  
> >  	if (bridge->funcs->attach) {
> >  		ret = bridge->funcs->attach(bridge, encoder, flags);
> > @@ -487,7 +489,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >  err_reset_bridge:
> >  	bridge->dev = NULL;
> >  	bridge->encoder = NULL;
> > +	drm_encoder_chain_lock(encoder);
> >  	list_del(&bridge->chain_node);
> > +	drm_encoder_chain_unlock(encoder);
> >  
> >  	if (ret != -EPROBE_DEFER)
> >  		DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
> > @@ -503,6 +507,11 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >  }
> >  EXPORT_SYMBOL(drm_bridge_attach);
> >  
> > +/*
> > + * Invoked by the encoder during encoder cleanup in drm_encoder_cleanup(),
> > + * so should generally *not* be called by driver code.  
> 
> Why not?

Because this is what drm_bridge_attach() says O:-)

> * drm_bridge_attach - attach the bridge to an encoder's chain
...
> * Note that bridges attached to encoders are auto-detached during encoder
> * cleanup in drm_encoder_cleanup(), so drm_bridge_attach() should generally
> * *not* be balanced with a drm_bridge_detach() in driver code.

Also, it's what the code does.

> Also, it looks entirely unrelated to the rest of the patch.

Sure, I can split it. It is also redundant given that's repeating what
drm_bridge_attach() says.

I wrote this comment for future people looking at this code. If
_attach() takes a lock and _detach() does not, it could look like a
potential mistake, and someone could spend precious hours in trying to
fix it.

Maybe replace with:

  /* Must be called with the encoder bridge chain locked */

?

Luca

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

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

[Luca Ceresoli]

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'
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.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 include/drm/drm_bridge.h | 62 ++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 0ff7ab4aa8689a022458f935a7ffb23a2b715802..9b1aa1e29c8915648aef86ba9f8d85e843b22ca0 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1440,26 +1440,29 @@ drm_bridge_chain_get_last_bridge(struct drm_encoder *encoder)
 						      struct drm_bridge, chain_node));
 }
 
-/**
- * drm_bridge_get_next_bridge_and_put - Get the next bridge in the chain
- *                                      and put the previous
- * @bridge: bridge object
- *
- * Same as drm_bridge_get_next_bridge() but additionally puts the @bridge.
- *
- * RETURNS:
- * the next bridge in the chain after @bridge, or NULL if @bridge is the last.
- */
-static inline struct drm_bridge *
-drm_bridge_get_next_bridge_and_put(struct drm_bridge *bridge)
+static inline struct drm_bridge *drm_bridge_encoder_chain_lock(struct drm_bridge *bridge)
 {
-	struct drm_bridge *next = drm_bridge_get_next_bridge(bridge);
+	drm_encoder_chain_lock(bridge->encoder);
+
+	return bridge;
+}
 
-	drm_bridge_put(bridge);
+/* Internal to drm_for_each_bridge_in_chain*() */
+static inline struct drm_bridge *__drm_encoder_bridge_chain_next(struct drm_bridge *bridge)
+{
+	if (list_is_last(&bridge->chain_node, &bridge->encoder->bridge_chain)) {
+		drm_encoder_chain_unlock(bridge->encoder);
 
-	return next;
+		return NULL;
+	}
+
+	return list_next_entry(bridge, chain_node);
 }
 
+/* Internal to drm_for_each_bridge_in_chain*() */
+DEFINE_FREE(drm_bridge_encoder_chain_unlock, struct drm_bridge *,
+	    if (_T) drm_encoder_chain_unlock(_T->encoder);)
+
 /**
  * drm_for_each_bridge_in_chain_scoped - iterate over all bridges attached
  *                                       to an encoder
@@ -1469,14 +1472,15 @@ drm_bridge_get_next_bridge_and_put(struct drm_bridge *bridge)
  *
  * Iterate over all bridges present in the bridge chain attached to @encoder.
  *
- * Automatically gets/puts the bridge reference while iterating, and puts
- * the reference even if returning or breaking in the middle of the loop.
+ * Automatically locks the encoder chain mutex to prevent chain
+ * modifications while iterating.
  */
-#define drm_for_each_bridge_in_chain_scoped(encoder, bridge)		\
-	for (struct drm_bridge *bridge __free(drm_bridge_put) =		\
-	     drm_bridge_chain_get_first_bridge(encoder);		\
-	     bridge;							\
-	     bridge = drm_bridge_get_next_bridge_and_put(bridge))
+#define drm_for_each_bridge_in_chain_scoped(encoder, bridge)				\
+	for (struct drm_bridge *bridge __free(drm_bridge_encoder_chain_unlock) =	\
+		     list_first_entry(&drm_encoder_chain_lock(encoder)->bridge_chain,	\
+				      struct drm_bridge, chain_node);			\
+	     bridge;									\
+	     bridge = __drm_encoder_bridge_chain_next(bridge))				\
 
 /**
  * drm_for_each_bridge_in_chain_from - iterate over all bridges starting
@@ -1488,14 +1492,14 @@ drm_bridge_get_next_bridge_and_put(struct drm_bridge *bridge)
  * Iterate over all bridges in the encoder chain starting from
  * @first_bridge, included.
  *
- * Automatically gets/puts the bridge reference while iterating, and puts
- * the reference even if returning or breaking in the middle of the loop.
+ * Automatically locks the encoder chain mutex to prevent chain
+ * modifications while iterating.
  */
-#define drm_for_each_bridge_in_chain_from(first_bridge, bridge)		\
-	for (struct drm_bridge *bridge __free(drm_bridge_put) =		\
-		     drm_bridge_get(first_bridge);			\
-	     bridge;							\
-	     bridge = drm_bridge_get_next_bridge_and_put(bridge))
+#define drm_for_each_bridge_in_chain_from(first_bridge, bridge)				\
+	for (struct drm_bridge *bridge __free(drm_bridge_encoder_chain_unlock) =	\
+		     drm_bridge_encoder_chain_lock(first_bridge);			\
+	     bridge;									\
+	     bridge = __drm_encoder_bridge_chain_next(bridge))				\
 
 enum drm_mode_status
 drm_bridge_chain_mode_valid(struct drm_bridge *bridge,

-- 
2.51.0

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

[kernel test robot]

Hello,

kernel test robot noticed "BUG:workqueue_lockup-pool" on:

commit: a3ae9f413a9785cbf472a3168131747f79722d31 ("[PATCH 4/7] drm/bridge: lock the encoder chain in scoped for_each loops")
url: https://github.com/intel-lab-lkp/linux/commits/Luca-Ceresoli/drm-encoder-add-mutex-to-protect-the-bridge-chain/20250927-000253
patch link: https://lore.kernel.org/all/20250926-drm-bridge-alloc-encoder-chain-mutex-v1-4-23b62c47356a@bootlin.com/
patch subject: [PATCH 4/7] drm/bridge: lock the encoder chain in scoped for_each loops

in testcase: boot

config: x86_64-randconfig-071-20250929
compiler: clang-20
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G

(please refer to attached dmesg/kmsg for entire log/backtrace)



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202509301358.38036b85-lkp@intel.com


[   63.465088][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 45s!
[   63.465385][    C0] Showing busy workqueues and worker pools:
[   63.465405][    C0] workqueue mm_percpu_wq: flags=0x8
[   63.465454][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[   63.465500][    C0]     pending: vmstat_update
[   63.465696][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[   94.185049][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 76s!
[   94.185123][    C0] Showing busy workqueues and worker pools:
[   94.185136][    C0] workqueue mm_percpu_wq: flags=0x8
[   94.185146][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[   94.185169][    C0]     pending: vmstat_update
[   94.185278][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  124.905021][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 107s!
[  124.905073][    C0] Showing busy workqueues and worker pools:
[  124.905080][    C0] workqueue mm_percpu_wq: flags=0x8
[  124.905087][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  124.905100][    C0]     pending: vmstat_update
[  124.905172][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  155.624991][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 137s!
[  155.625049][    C0] Showing busy workqueues and worker pools:
[  155.625057][    C0] workqueue mm_percpu_wq: flags=0x8
[  155.625065][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  155.625081][    C0]     pending: vmstat_update
[  155.625161][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  186.345006][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 168s!
[  186.345099][    C0] Showing busy workqueues and worker pools:
[  186.345111][    C0] workqueue mm_percpu_wq: flags=0x8
[  186.345123][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  186.345151][    C0]     pending: vmstat_update
[  186.345277][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  217.065070][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 199s!
[  217.065206][    C0] Showing busy workqueues and worker pools:
[  217.065227][    C0] workqueue mm_percpu_wq: flags=0x8
[  217.065247][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  217.065290][    C0]     pending: vmstat_update
[  217.065477][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  247.785028][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 230s!
[  247.785093][    C0] Showing busy workqueues and worker pools:
[  247.785104][    C0] workqueue mm_percpu_wq: flags=0x8
[  247.785112][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  247.785130][    C0]     pending: vmstat_update
[  247.785223][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  278.504990][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 260s!
[  278.505051][    C0] Showing busy workqueues and worker pools:
[  278.505062][    C0] workqueue mm_percpu_wq: flags=0x8
[  278.505069][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  278.505086][    C0]     pending: vmstat_update
[  278.505169][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  309.224985][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 291s!
[  309.225073][    C0] Showing busy workqueues and worker pools:
[  309.225085][    C0] workqueue mm_percpu_wq: flags=0x8
[  309.225097][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  309.225122][    C0]     pending: vmstat_update
[  309.225238][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  339.944986][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 322s!
[  339.945076][    C0] Showing busy workqueues and worker pools:
[  339.945089][    C0] workqueue mm_percpu_wq: flags=0x8
[  339.945102][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  339.945130][    C0]     pending: vmstat_update
[  339.945258][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  370.665059][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 352s!
[  370.665192][    C0] Showing busy workqueues and worker pools:
[  370.665212][    C0] workqueue mm_percpu_wq: flags=0x8
[  370.665233][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  370.665276][    C0]     pending: vmstat_update
[  370.665467][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  401.385016][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 383s!
[  401.385074][    C0] Showing busy workqueues and worker pools:
[  401.385085][    C0] workqueue mm_percpu_wq: flags=0x8
[  401.385092][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  401.385108][    C0]     pending: vmstat_update
[  401.385195][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  432.104963][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 414s!
[  432.105013][    C0] Showing busy workqueues and worker pools:
[  432.105021][    C0] workqueue mm_percpu_wq: flags=0x8
[  432.105027][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  432.105041][    C0]     pending: vmstat_update
[  432.105111][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  462.824979][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 445s!
[  462.825065][    C0] Showing busy workqueues and worker pools:
[  462.825076][    C0] workqueue mm_percpu_wq: flags=0x8
[  462.825087][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  462.825112][    C0]     pending: vmstat_update
[  462.825229][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  493.544976][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 475s!
[  493.545072][    C0] Showing busy workqueues and worker pools:
[  493.545084][    C0] workqueue mm_percpu_wq: flags=0x8
[  493.545096][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  493.545124][    C0]     pending: vmstat_update
[  493.545252][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  524.265024][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 506s!
[  524.265136][    C0] Showing busy workqueues and worker pools:
[  524.265153][    C0] workqueue mm_percpu_wq: flags=0x8
[  524.265170][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  524.265206][    C0]     pending: vmstat_update
[  524.265371][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  554.985000][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 537s!
[  554.985069][    C0] Showing busy workqueues and worker pools:
[  554.985080][    C0] workqueue mm_percpu_wq: flags=0x8
[  554.985089][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  554.985107][    C0]     pending: vmstat_update
[  554.985213][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  585.704946][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 567s!
[  585.704998][    C0] Showing busy workqueues and worker pools:
[  585.705006][    C0] workqueue mm_percpu_wq: flags=0x8
[  585.705013][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  585.705026][    C0]     pending: vmstat_update
[  585.705098][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  616.424942][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 598s!
[  616.424990][    C0] Showing busy workqueues and worker pools:
[  616.424997][    C0] workqueue mm_percpu_wq: flags=0x8
[  616.425003][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  616.425015][    C0]     pending: vmstat_update
[  616.425082][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  647.144950][    C0] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0 nice=0 stuck for 629s!
[  647.145035][    C0] Showing busy workqueues and worker pools:
[  647.145047][    C0] workqueue mm_percpu_wq: flags=0x8
[  647.145059][    C0]   pwq 6: cpus=1 node=0 flags=0x0 nice=0 active=1 refcnt=2
[  647.145084][    C0]     pending: vmstat_update
[  647.145202][    C0] Showing backtraces of running workers in stalled CPU-bound worker pools:
[  674.477504][    C1] watchdog: BUG: soft lockup - CPU#1 stuck for 626s! [swapper/0:1]
[  674.477539][    C1] CPU#1 Utilization every 96s during lockup:
[  674.477543][    C1] 	#1: 100% system,	  1% softirq,	  1% hardirq,	  0% idle
[  674.477549][    C1] 	#2: 100% system,	  1% softirq,	  1% hardirq,	  0% idle
[  674.477553][    C1] 	#3: 100% system,	  0% softirq,	  1% hardirq,	  0% idle
[  674.477557][    C1] 	#4: 100% system,	  1% softirq,	  1% hardirq,	  0% idle
[  674.477561][    C1] 	#5: 100% system,	  1% softirq,	  1% hardirq,	  0% idle
[  674.477566][    C1] Modules linked in:
[  674.477571][    C1] irq event stamp: 852298
[ 674.477574][ C1] hardirqs last enabled at (852297): asm_sysvec_apic_timer_interrupt (arch/x86/include/asm/idtentry.h:702) 
[ 674.477592][ C1] hardirqs last disabled at (852298): sysvec_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:1050) 
[ 674.477603][ C1] softirqs last enabled at (852280): handle_softirqs (arch/x86/include/asm/preempt.h:27 kernel/softirq.c:426 kernel/softirq.c:607) 
[ 674.477612][ C1] softirqs last disabled at (852275): __irq_exit_rcu (arch/x86/include/asm/jump_label.h:36 kernel/softirq.c:682) 
[  674.477622][    C1] CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc6-01063-ga3ae9f413a97 #1 VOLUNTARY
[  674.477629][    C1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[ 674.477634][ C1] RIP: 0010:__sanitizer_cov_trace_pc (arch/x86/include/asm/preempt.h:27 kernel/kcov.c:183 kernel/kcov.c:217) 
[ 674.477644][ C1] Code: 48 c7 c7 00 a1 59 8a 48 89 de e8 be 1e 42 01 eb 85 cc cc cc cc cc cc cc cc cc cc cc cc 48 8b 04 24 65 48 8b 0c 25 08 30 0a 8d <65> 8b 15 dc c0 97 0b 81 e2 00 01 ff 00 74 11 81 fa 00 01 00 00 75
All code
========
   0:	48 c7 c7 00 a1 59 8a 	mov    $0xffffffff8a59a100,%rdi
   7:	48 89 de             	mov    %rbx,%rsi
   a:	e8 be 1e 42 01       	call   0x1421ecd
   f:	eb 85                	jmp    0xffffffffffffff96
  11:	cc                   	int3
  12:	cc                   	int3
  13:	cc                   	int3
  14:	cc                   	int3
  15:	cc                   	int3
  16:	cc                   	int3
  17:	cc                   	int3
  18:	cc                   	int3
  19:	cc                   	int3
  1a:	cc                   	int3
  1b:	cc                   	int3
  1c:	cc                   	int3
  1d:	48 8b 04 24          	mov    (%rsp),%rax
  21:	65 48 8b 0c 25 08 30 	mov    %gs:0xffffffff8d0a3008,%rcx
  28:	0a 8d 
  2a:*	65 8b 15 dc c0 97 0b 	mov    %gs:0xb97c0dc(%rip),%edx        # 0xb97c10d		<-- trapping instruction
  31:	81 e2 00 01 ff 00    	and    $0xff0100,%edx
  37:	74 11                	je     0x4a
  39:	81 fa 00 01 00 00    	cmp    $0x100,%edx
  3f:	75                   	.byte 0x75

Code starting with the faulting instruction
===========================================
   0:	65 8b 15 dc c0 97 0b 	mov    %gs:0xb97c0dc(%rip),%edx        # 0xb97c0e3
   7:	81 e2 00 01 ff 00    	and    $0xff0100,%edx
   d:	74 11                	je     0x20
   f:	81 fa 00 01 00 00    	cmp    $0x100,%edx
  15:	75                   	.byte 0x75
[  674.477650][    C1] RSP: 0018:ffffc9000001f138 EFLAGS: 00200287
[  674.477656][    C1] RAX: ffffffff831e1d39 RBX: ffff888160a713c0 RCX: ffff888100871b00
[  674.477660][    C1] RDX: 0000000000000000 RSI: 0000000000000008 RDI: ffffffff8bc06cf0
[  674.477664][    C1] RBP: 0000000000000032 R08: ffffffff8bc06cf7 R09: 1ffffffff1780d9e
[  674.477669][    C1] R10: dffffc0000000000 R11: fffffbfff1780d9f R12: dffffc0000000000
[  674.477674][    C1] R13: 1ffff1102c14e27e R14: 0000000000000000 R15: ffff88817b2bd5a0
[  674.477678][    C1] FS:  0000000000000000(0000) GS:ffff88842127d000(0000) knlGS:0000000000000000
[  674.477683][    C1] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  674.477687][    C1] CR2: 00000000f7f5ecd4 CR3: 000000000a0c3000 CR4: 00000000000406f0
[  674.477695][    C1] Call Trace:
[  674.477710][    C1]  <TASK>
[ 674.477713][ C1] drm_atomic_add_encoder_bridges (drivers/gpu/drm/drm_atomic.c:1319 (discriminator 1025)) 
[ 674.477734][ C1] drm_atomic_helper_check_modeset (drivers/gpu/drm/drm_atomic_helper.c:818) 
[ 674.477767][ C1] drm_atomic_helper_check (drivers/gpu/drm/drm_atomic_helper.c:1112) 
[ 674.477775][ C1] ? vgem_fence_release (drivers/gpu/drm/vkms/vkms_drv.c:95) 
[ 674.477786][ C1] drm_atomic_check_only (drivers/gpu/drm/drm_atomic.c:1503) 
[ 674.477808][ C1] drm_atomic_commit (drivers/gpu/drm/drm_atomic.c:1571) 
[ 674.477816][ C1] ? dump_dte_entry (drivers/gpu/drm/drm_print.c:210) 
[ 674.477829][ C1] drm_client_modeset_commit_atomic (drivers/gpu/drm/drm_client_modeset.c:?) 
[ 674.477862][ C1] drm_client_modeset_commit_locked (drivers/gpu/drm/drm_client_modeset.c:1206) 
[ 674.477874][ C1] drm_client_modeset_commit (drivers/gpu/drm/drm_client_modeset.c:1232) 
[ 674.477882][ C1] __drm_fb_helper_restore_fbdev_mode_unlocked (drivers/gpu/drm/drm_fb_helper.c:?) 
[ 674.477895][ C1] drm_fb_helper_set_par (drivers/gpu/drm/drm_fb_helper.c:?) 
[ 674.477904][ C1] ? drm_fb_pixel_format_equal (drivers/gpu/drm/drm_fb_helper.c:1316) 
[ 674.477913][ C1] fbcon_init (drivers/video/fbdev/core/fbcon.c:1153) 
[ 674.477935][ C1] visual_init (drivers/tty/vt/vt.c:?) 
[ 674.477949][ C1] do_bind_con_driver (drivers/tty/vt/vt.c:3916) 
[ 674.477964][ C1] do_take_over_console (drivers/tty/vt/vt.c:?) 
[ 674.477983][ C1] do_fbcon_takeover (drivers/video/fbdev/core/fbcon.c:589) 
[ 674.477993][ C1] fbcon_fb_registered (drivers/video/fbdev/core/fbcon.c:3022) 
[ 674.478005][ C1] register_framebuffer (drivers/video/fbdev/core/fbmem.c:512) 
[ 674.478022][ C1] __drm_fb_helper_initial_config_and_unlock (drivers/gpu/drm/drm_fb_helper.c:1835) 
[ 674.478050][ C1] drm_fbdev_client_hotplug (drivers/gpu/drm/clients/drm_fbdev_client.c:53) 
[ 674.478065][ C1] drm_client_register (drivers/gpu/drm/drm_client.c:142) 
[ 674.478077][ C1] drm_fbdev_client_setup (drivers/gpu/drm/clients/drm_fbdev_client.c:?) 
[ 674.478090][ C1] drm_client_setup (drivers/gpu/drm/clients/drm_client_setup.c:47) 
[ 674.478102][ C1] vkms_init (drivers/gpu/drm/vkms/vkms_drv.c:227) 
[ 674.478116][ C1] ? vgem_init (drivers/gpu/drm/vkms/vkms_drv.c:213) 
[ 674.478121][ C1] do_one_initcall (init/main.c:1269) 
[ 674.478137][ C1] ? stack_depot_save_flags (lib/stackdepot.c:722) 
[ 674.478153][ C1] ? kasan_save_track (arch/x86/include/asm/current.h:25 (discriminator 3) mm/kasan/common.c:60 (discriminator 3) mm/kasan/common.c:69 (discriminator 3)) 
[ 674.478161][ C1] ? kasan_save_track (mm/kasan/common.c:48 mm/kasan/common.c:68) 
[ 674.478168][ C1] ? __kasan_kmalloc (mm/kasan/common.c:409) 
[ 674.478176][ C1] ? __kmalloc_noprof (include/linux/kasan.h:260 mm/slub.c:4376 mm/slub.c:4388) 
[ 674.478181][ C1] ? do_initcalls (init/main.c:1341) 
[ 674.478188][ C1] ? kernel_init_freeable (init/main.c:1581) 
[ 674.478193][ C1] ? kernel_init (init/main.c:1471) 
[ 674.478200][ C1] ? ret_from_fork (arch/x86/kernel/process.c:154) 
[ 674.478207][ C1] ? ret_from_fork_asm (arch/x86/entry/entry_64.S:258) 
[ 674.478225][ C1] ? asm_sysvec_apic_timer_interrupt (arch/x86/include/asm/idtentry.h:702) 
[ 674.478241][ C1] ? strlen (lib/string.c:420 (discriminator 769)) 
[ 674.478250][ C1] ? next_arg (lib/cmdline.c:273) 
[ 674.478261][ C1] ? parameq (kernel/params.c:91 (discriminator 2048) kernel/params.c:99 (discriminator 2048)) 
[ 674.478263][ C1] ? parse_args (kernel/params.c:?) 
[ 674.478263][ C1] do_initcall_level (init/main.c:1330 (discriminator 6)) 
[ 674.478263][ C1] do_initcalls (init/main.c:1344 (discriminator 2)) 
[ 674.478263][ C1] kernel_init_freeable (init/main.c:1581) 
[ 674.478263][ C1] ? rest_init (init/main.c:1461) 
[ 674.478263][ C1] kernel_init (init/main.c:1471) 
[ 674.478263][ C1] ? rest_init (init/main.c:1461) 
[ 674.478263][ C1] ret_from_fork (arch/x86/kernel/process.c:154) 
[ 674.478341][ C1] ? rest_init (init/main.c:1461) 
[ 674.478341][ C1] ret_from_fork_asm (arch/x86/entry/entry_64.S:258) 
[  674.478341][    C1]  </TASK>
[  674.478341][    C1] Kernel panic - not syncing: softlockup: hung tasks
[  674.478341][    C1] CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Tainted: G             L      6.17.0-rc6-01063-ga3ae9f413a97 #1 VOLUNTARY
[  674.478341][    C1] Tainted: [L]=SOFTLOCKUP
[  674.478341][    C1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[  674.478341][    C1] Call Trace:
[  674.478341][    C1]  <IRQ>
[ 674.478341][ C1] vpanic (kernel/panic.c:?) 
[ 674.478341][ C1] panic (??:?) 
[ 674.478341][ C1] watchdog_timer_fn (kernel/watchdog.c:832) 
[ 674.478341][ C1] ? softlockup_count_show (kernel/watchdog.c:733) 
[ 674.478341][ C1] __hrtimer_run_queues (kernel/time/hrtimer.c:1761) 
[ 674.478341][ C1] hrtimer_interrupt (kernel/time/hrtimer.c:1890) 
[ 674.478341][ C1] __sysvec_apic_timer_interrupt (arch/x86/include/asm/jump_label.h:36 arch/x86/include/asm/trace/irq_vectors.h:40 arch/x86/kernel/apic/apic.c:1057) 
[ 674.478341][ C1] sysvec_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:1050 (discriminator 24)) 
[  674.478341][    C1]  </IRQ>
[  674.478341][    C1]  <TASK>
[ 674.478341][ C1] asm_sysvec_apic_timer_interrupt (arch/x86/include/asm/idtentry.h:702) 
[ 674.478341][ C1] RIP: 0010:__sanitizer_cov_trace_pc (arch/x86/include/asm/preempt.h:27 kernel/kcov.c:183 kernel/kcov.c:217) 
[ 674.478341][ C1] Code: 48 c7 c7 00 a1 59 8a 48 89 de e8 be 1e 42 01 eb 85 cc cc cc cc cc cc cc cc cc cc cc cc 48 8b 04 24 65 48 8b 0c 25 08 30 0a 8d <65> 8b 15 dc c0 97 0b 81 e2 00 01 ff 00 74 11 81 fa 00 01 00 00 75
All code
========
   0:	48 c7 c7 00 a1 59 8a 	mov    $0xffffffff8a59a100,%rdi
   7:	48 89 de             	mov    %rbx,%rsi
   a:	e8 be 1e 42 01       	call   0x1421ecd
   f:	eb 85                	jmp    0xffffffffffffff96
  11:	cc                   	int3
  12:	cc                   	int3
  13:	cc                   	int3
  14:	cc                   	int3
  15:	cc                   	int3
  16:	cc                   	int3
  17:	cc                   	int3
  18:	cc                   	int3
  19:	cc                   	int3
  1a:	cc                   	int3
  1b:	cc                   	int3


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20250930/202509301358.38036b85-lkp@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Luca Ceresoli]

Hello,

On Tue, 30 Sep 2025 14:16:23 +0800
kernel test robot <oliver.sang@intel.com> wrote:

> [  674.477504][    C1] watchdog: BUG: soft lockup - CPU#1 stuck for 626s! [swapper/0:1]
> [  674.477539][    C1] CPU#1 Utilization every 96s during lockup:
> [  674.477543][    C1] 	#1: 100% system,	  1% softirq,	  1% hardirq,	  0% idle
> [  674.477549][    C1] 	#2: 100% system,	  1% softirq,	  1% hardirq,	  0% idle
> [  674.477553][    C1] 	#3: 100% system,	  0% softirq,	  1% hardirq,	  0% idle
> [  674.477557][    C1] 	#4: 100% system,	  1% softirq,	  1% hardirq,	  0% idle
> [  674.477561][    C1] 	#5: 100% system,	  1% softirq,	  1% hardirq,	  0% idle
...
> [ 674.477713][ C1] drm_atomic_add_encoder_bridges (drivers/gpu/drm/drm_atomic.c:1319 (discriminator 1025)) 

The one reported is an actual bug, causing an infinite loop when the
encoder bridge chain is empty. Took a while to reproduce and debug, but
the fix is very simple:

 #define drm_for_each_bridge_in_chain_scoped(encoder, bridge)                           \
        for (struct drm_bridge *bridge __free(drm_bridge_encoder_chain_unlock) =        \
-                    list_first_entry(&drm_encoder_chain_lock(encoder)->bridge_chain,   \
+                    list_first_entry_or_null(&drm_encoder_chain_lock(encoder)->bridge_chain,   \

Fix queued for v2.

Luca

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

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

[Luca Ceresoli]

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(). These
two functions are handled by a separate commit.

Signed-off-by: Luca Ceresoli <luca.ceresoli@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 90e467cf91a134342c80d2f958b928472aaf0d8b..554959147a00efa9807c245290cba8977ce88fc5 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -623,7 +623,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
  *
@@ -637,17 +637,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)
@@ -665,7 +662,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
  *
@@ -674,20 +671,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);
 
@@ -911,7 +904,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
@@ -921,22 +914,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);
 

-- 
2.51.0

[PATCH 6/7] drm/bridge: prevent encoder chain changes while iterating with list_for_each_entry_reverse()

[Luca Ceresoli]

These loops in drm_bridge.c iterate over the encoder chain using
list_for_each_entry_reverse), which does not prevent changes to the bridge
chain while iterating over it.

Take the encoder chain mutex while iterating to avoid chain changes while
iterating.

All the "simple" loops are converted. drm_atomic_bridge_chain_pre_enable()
and drm_atomic_bridge_chain_post_disable() are handled by a separate
commit.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_bridge.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 554959147a00efa9807c245290cba8977ce88fc5..66c0a80db8426ffb360248895cfe2a11d6007ed7 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -706,6 +706,7 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 		return;
 
 	encoder = bridge->encoder;
+	drm_encoder_chain_lock(encoder);
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
 		if (iter->funcs->atomic_disable) {
 			iter->funcs->atomic_disable(iter, state);
@@ -716,6 +717,7 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 		if (iter == bridge)
 			break;
 	}
+	drm_encoder_chain_unlock(encoder);
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
 
@@ -1220,6 +1222,7 @@ int drm_atomic_bridge_chain_check(struct drm_bridge *bridge,
 		return ret;
 
 	encoder = bridge->encoder;
+	drm_encoder_chain_lock(encoder);
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
 		int ret;
 
@@ -1234,12 +1237,15 @@ int drm_atomic_bridge_chain_check(struct drm_bridge *bridge,
 						      crtc_state->state);
 
 		ret = drm_atomic_bridge_check(iter, crtc_state, conn_state);
-		if (ret)
+		if (ret) {
+			drm_encoder_chain_unlock(encoder);
 			return ret;
+		}
 
 		if (iter == bridge)
 			break;
 	}
+	drm_encoder_chain_unlock(encoder);
 
 	return 0;
 }

-- 
2.51.0

[PATCH 7/7] drm/bridge: prevent encoder chain changes while iterating in drm_atomic_bridge_chain_post_disable/pre_enable()

[Luca Ceresoli]

Take the encoder chain mutex while iterating over the encoder chain in
drm_atomic_bridge_chain_pre_enable() and
drm_atomic_bridge_chain_post-disable().

These functions have nested list_for_each_*() loops, which makes them
complicated. list_for_each_entry_from() loops could be replaced by
drm_for_each_bridge_in_chain_from(), but it would not work in a nested
way. Besides, there is no "_reverse" variant of
drm_for_each_bridge_in_chain_from().

Keep code simple and readable by explicitly locking around the outer
loop. Thankfully there are no return points inside the loops, so the change
is trivial and readable.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_bridge.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 66c0a80db8426ffb360248895cfe2a11d6007ed7..00e0a88128d0eeabf25911591ad8c482d6c1caf8 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -765,6 +765,7 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
 
 	encoder = bridge->encoder;
 
+	drm_encoder_chain_lock(encoder);
 	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
 		limit = NULL;
 
@@ -813,6 +814,7 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
 			/* Jump all bridges that we have already post_disabled */
 			bridge = limit;
 	}
+	drm_encoder_chain_unlock(encoder);
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
 
@@ -859,6 +861,7 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 
 	encoder = bridge->encoder;
 
+	drm_encoder_chain_lock(encoder);
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
 		if (iter->pre_enable_prev_first) {
 			next = iter;
@@ -901,6 +904,7 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 		if (iter == bridge)
 			break;
 	}
+	drm_encoder_chain_unlock(encoder);
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
 

-- 
2.51.0