| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250929164526.08aa8118@booty>
Date: Mon, 29 Sep 2025 16:45:26 +0200
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann
<tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Simona Vetter
<simona@...ll.ch>, Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong
<neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, Laurent
Pinchart <Laurent.pinchart@...asonboard.com>, Jonas Karlman
<jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>, Hui Pu
<Hui.Pu@...ealthcare.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] drm/encoder: add mutex to protect the bridge chain
Hi Maxime,
On Mon, 29 Sep 2025 14:43:46 +0200
Maxime Ripard <mripard@...nel.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@...tlin.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
Powered by blists - more mailing lists