[PATCH 0/2] drm/bridge: handle gracefully atomic updates during bridge removal

[PATCH 0/2] drm/bridge: handle gracefully atomic updates during bridge removal

[Luca Ceresoli]

This is a first attempt at gracefully handling the case of atomic updates
happening concurrently to physical removal of DRM bridges.

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 (in v6.16-rc1)
    B. ✔ convert all bridge drivers to new API (now in drm-misc-next)
    C. ✔ kunit tests (now in drm-misc-next)
    D. ✔ add get/put to drm_bridge_add/remove() + attach/detach()
         and warn on old allocation pattern (now in drm-misc-next)
    E. ➜ add get/put on drm_bridge accessors
       1. ✔ drm_bridge_chain_get_first_bridge() + add a cleanup action
       2. ✔ drm_bridge_get_prev_bridge()
       3. … drm_bridge_get_next_bridge()
       4. … drm_for_each_bridge_in_chain()
       5. drm_bridge_connector_init
       6. of_drm_find_bridge
       7. drm_of_find_panel_or_bridge, *_of_get_bridge
    F. debugfs improvements
 2. ➜ handle gracefully atomic updates during bridge removal
 3. … DSI host-device driver interaction
 4. finish the hotplug bridge work, removing the "always-disconnected"
    connector, moving code to the core and potentially removing the
    hotplug-bridge itself (this needs to be clarified as points 1-3 are
    developed)

The idea was proposed by Maxime [0] and is based on the existing
drm_dev_enter/exit() already existing for the DRM device.

This small series implements the core mechanism in drm_bridge.c and uses it
in the ti-sn65dsi83 driver. This prevents usage of device resources by
various code paths that can happen concurrently to unplug of the SN65DSI8x
bridge.

[0] https://lore.kernel.org/all/20250106-vigorous-talented-viper-fa49d9@houat/

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (2):
      drm/bridge: add drm_bridge_unplug() and drm_bridge_enter/exit()
      drm/bridge: ti-sn65dsi83: protect device resources on unplug

 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 53 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_bridge.c          | 58 +++++++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h              | 12 ++++++++
 3 files changed, 123 insertions(+)
---
base-commit: d2b48f2b30f25997a1ae1ad0cefac68c25f8c330
change-id: 20250808-drm-bridge-atomic-vs-remove-1d7bb202c8ef

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

[PATCH 1/2] drm/bridge: add drm_bridge_unplug() and drm_bridge_enter/exit()

[Luca Ceresoli]

To allow DRM bridges to be removable, add synchronization functions
allowing to tell when a bridge hardware has been physically unplugged and
to mark a critical section that should not be entered after that.

This is inspired by the drm_dev_unplugged/enter/exit() functions for struct
drm_device.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Link: https://lore.kernel.org/all/20250106-vigorous-talented-viper-fa49d9@houat/
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/drm_bridge.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h     | 12 +++++++++
 2 files changed, 70 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c3bfcd735a3c426a147bf0a7427b3d2cd0df3524..486bc48881c6a05ebe3c911754a40530f0d08e3e 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -27,6 +27,7 @@
 #include <linux/media-bus-format.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/srcu.h>
 
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_bridge.h>
@@ -200,6 +201,63 @@
 static DEFINE_MUTEX(bridge_lock);
 static LIST_HEAD(bridge_list);
 
+DEFINE_STATIC_SRCU(drm_bridge_unplug_srcu);
+
+/**
+ * drm_bridge_enter - Enter DRM bridge critical section
+ * @dev: DRM bridge
+ * @idx: Pointer to index that will be passed to the matching drm_bridge_exit()
+ *
+ * This function marks and protects the beginning of a section that should not
+ * be entered after the bridge has been unplugged. The section end is marked
+ * with drm_bridge_exit(). Calls to this function can be nested.
+ *
+ * Returns:
+ * True if it is OK to enter the section, false otherwise.
+ */
+bool drm_bridge_enter(struct drm_bridge *bridge, int *idx)
+{
+	*idx = srcu_read_lock(&drm_bridge_unplug_srcu);
+
+	if (bridge->unplugged) {
+		srcu_read_unlock(&drm_bridge_unplug_srcu, *idx);
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(drm_bridge_enter);
+
+/**
+ * drm_bridge_exit - Exit DRM bridge critical section
+ * @idx: index returned by drm_bridge_enter()
+ *
+ * This function marks the end of a section that should not be entered after
+ * the bridge has been unplugged.
+ */
+void drm_bridge_exit(int idx)
+{
+	srcu_read_unlock(&drm_bridge_unplug_srcu, idx);
+}
+EXPORT_SYMBOL(drm_bridge_exit);
+
+/**
+ * drm_bridge_unplug - unplug a DRM bridge
+ * @dev: DRM bridge
+ *
+ * This tells the bridge has been physically unplugged and no operations on
+ * device resources must be done anymore. Entry-points can use
+ * drm_bridge_enter() and drm_bridge_exit() to protect device resources in
+ * a race free manner.
+ */
+void drm_bridge_unplug(struct drm_bridge *bridge)
+{
+	bridge->unplugged = true;
+
+	synchronize_srcu(&drm_bridge_unplug_srcu);
+}
+EXPORT_SYMBOL(drm_bridge_unplug);
+
 static void __drm_bridge_free(struct kref *kref)
 {
 	struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 620e119cc24c3491c2be5f08efaf51dfa8f708b3..0c6869ebb2be9b89499ee5c42692fd94ca5b3161 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1095,6 +1095,14 @@ struct drm_bridge {
 	 */
 	struct kref refcount;
 
+	/**
+	 * @unplugged:
+	 *
+	 * Flag to tell if the bridge has been unplugged.
+	 * See drm_bridge_enter() and drm_bridge_unplug().
+	 */
+	bool unplugged;
+
 	/** @driver_private: pointer to the bridge driver's internal context */
 	void *driver_private;
 	/** @ops: bitmask of operations supported by the bridge */
@@ -1226,6 +1234,10 @@ drm_priv_to_bridge(struct drm_private_obj *priv)
 	return container_of(priv, struct drm_bridge, base);
 }
 
+bool drm_bridge_enter(struct drm_bridge *bridge, int *idx);
+void drm_bridge_exit(int idx);
+void drm_bridge_unplug(struct drm_bridge *bridge);
+
 struct drm_bridge *drm_bridge_get(struct drm_bridge *bridge);
 void drm_bridge_put(struct drm_bridge *bridge);
 

-- 
2.50.1

[PATCH 2/2] drm/bridge: ti-sn65dsi83: protect device resources on unplug

[Luca Ceresoli]

To support hot-unplug of this bridge we need to protect access to device
resources in case sn65dsi83_remove() happens concurrently to other code.

Some care is needed for the case when the unplug happens before
sn65dsi83_atomic_disable() has a chance to enter the critical section
(i.e. a successful drm_bridge_enter() call), which occurs whenever the
hardware is removed while the display is active. When that happens,
sn65dsi83_atomic_disable() in unable to release some resources, thus this
needs to be done in sn65dsi83_remove() after drm_bridge_unplug().

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

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 033c44326552ab167d4e8d9b74957c585e4c6fb7..9e4cecf4f7cb056f0c34e87007fcebf50780e49c 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -157,6 +157,7 @@ struct sn65dsi83 {
 	struct drm_bridge		*panel_bridge;
 	struct gpio_desc		*enable_gpio;
 	struct regulator		*vcc;
+	bool				disable_resources_needed;
 	bool				lvds_dual_link;
 	bool				lvds_dual_link_even_odd_swap;
 	int				lvds_vod_swing_conf[2];
@@ -406,6 +407,10 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
 {
 	struct sn65dsi83 *ctx = container_of(ws, struct sn65dsi83, reset_work);
 	int ret;
+	int idx;
+
+	if (!drm_bridge_enter(&ctx->bridge, &idx))
+		return;
 
 	/* Reset the pipe */
 	ret = sn65dsi83_reset_pipe(ctx);
@@ -415,12 +420,18 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
 	}
 	if (ctx->irq)
 		enable_irq(ctx->irq);
+
+	drm_bridge_exit(idx);
 }
 
 static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
 {
 	unsigned int irq_stat;
 	int ret;
+	int idx;
+
+	if (!drm_bridge_enter(&ctx->bridge, &idx))
+		return;
 
 	/*
 	 * Schedule a reset in case of:
@@ -441,6 +452,8 @@ static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
 
 		schedule_work(&ctx->reset_work);
 	}
+
+	drm_bridge_exit(idx);
 }
 
 static void sn65dsi83_monitor_work(struct work_struct *work)
@@ -478,10 +491,15 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 	__le16 le16val;
 	u16 val;
 	int ret;
+	int idx;
+
+	if (!drm_bridge_enter(bridge, &idx))
+		return;
 
 	ret = regulator_enable(ctx->vcc);
 	if (ret) {
 		dev_err(ctx->dev, "Failed to enable vcc: %d\n", ret);
+		drm_bridge_exit(idx);
 		return;
 	}
 
@@ -625,6 +643,8 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 		dev_err(ctx->dev, "failed to lock PLL, ret=%i\n", ret);
 		/* On failure, disable PLL again and exit. */
 		regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
+		ctx->disable_resources_needed = true;
+		drm_bridge_exit(idx);
 		return;
 	}
 
@@ -633,6 +653,9 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 
 	/* Wait for 10ms after soft reset as specified in datasheet */
 	usleep_range(10000, 12000);
+
+	ctx->disable_resources_needed = true;
+	drm_bridge_exit(idx);
 }
 
 static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
@@ -640,6 +663,10 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
 {
 	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
 	unsigned int pval;
+	int idx;
+
+	if (!drm_bridge_enter(bridge, &idx))
+		return;
 
 	/* Clear all errors that got asserted during initialization. */
 	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
@@ -659,6 +686,8 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
 		/* Use the polling task */
 		sn65dsi83_monitor_start(ctx);
 	}
+
+	drm_bridge_exit(idx);
 }
 
 static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
@@ -666,6 +695,10 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
 {
 	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
 	int ret;
+	int idx;
+
+	if (!drm_bridge_enter(bridge, &idx))
+		return;
 
 	if (ctx->irq) {
 		/* Disable irq */
@@ -685,6 +718,9 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
 		dev_err(ctx->dev, "Failed to disable vcc: %d\n", ret);
 
 	regcache_mark_dirty(ctx->regmap);
+
+	ctx->disable_resources_needed = false;
+	drm_bridge_exit(idx);
 }
 
 static enum drm_mode_status
@@ -1005,7 +1041,24 @@ static void sn65dsi83_remove(struct i2c_client *client)
 {
 	struct sn65dsi83 *ctx = i2c_get_clientdata(client);
 
+	drm_bridge_unplug(&ctx->bridge);
 	drm_bridge_remove(&ctx->bridge);
+
+	/*
+	 * sn65dsi83_atomic_disable() should release some resources, but it
+	 * cannot if we call drm_bridge_unplug() before it can
+	 * drm_bridge_enter(). If that happens, let's release those
+	 * resources now.
+	 */
+	if (ctx->disable_resources_needed) {
+		if (!ctx->irq)
+			sn65dsi83_monitor_stop(ctx);
+
+		gpiod_set_value_cansleep(ctx->enable_gpio, 0);
+		usleep_range(10000, 11000);
+
+		regulator_disable(ctx->vcc);
+	}
 }
 
 static const struct i2c_device_id sn65dsi83_id[] = {

-- 
2.50.1

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: protect device resources on unplug

[Maxime Ripard]

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

On Fri, Aug 08, 2025 at 03:24:29PM +0200, Luca Ceresoli wrote:
> To support hot-unplug of this bridge we need to protect access to device
> resources in case sn65dsi83_remove() happens concurrently to other code.
> 
> Some care is needed for the case when the unplug happens before
> sn65dsi83_atomic_disable() has a chance to enter the critical section
> (i.e. a successful drm_bridge_enter() call), which occurs whenever the
> hardware is removed while the display is active. When that happens,
> sn65dsi83_atomic_disable() in unable to release some resources, thus this
> needs to be done in sn65dsi83_remove() after drm_bridge_unplug().
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 53 +++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 033c44326552ab167d4e8d9b74957c585e4c6fb7..9e4cecf4f7cb056f0c34e87007fcebf50780e49c 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -157,6 +157,7 @@ struct sn65dsi83 {
>  	struct drm_bridge		*panel_bridge;
>  	struct gpio_desc		*enable_gpio;
>  	struct regulator		*vcc;
> +	bool				disable_resources_needed;
>  	bool				lvds_dual_link;
>  	bool				lvds_dual_link_even_odd_swap;
>  	int				lvds_vod_swing_conf[2];
> @@ -406,6 +407,10 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
>  {
>  	struct sn65dsi83 *ctx = container_of(ws, struct sn65dsi83, reset_work);
>  	int ret;
> +	int idx;
> +
> +	if (!drm_bridge_enter(&ctx->bridge, &idx))
> +		return;
>  
>  	/* Reset the pipe */
>  	ret = sn65dsi83_reset_pipe(ctx);
> @@ -415,12 +420,18 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
>  	}
>  	if (ctx->irq)
>  		enable_irq(ctx->irq);
> +
> +	drm_bridge_exit(idx);
>  }
>  
>  static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
>  {
>  	unsigned int irq_stat;
>  	int ret;
> +	int idx;
> +
> +	if (!drm_bridge_enter(&ctx->bridge, &idx))
> +		return;
>  
>  	/*
>  	 * Schedule a reset in case of:
> @@ -441,6 +452,8 @@ static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
>  
>  		schedule_work(&ctx->reset_work);
>  	}
> +
> +	drm_bridge_exit(idx);
>  }
>  
>  static void sn65dsi83_monitor_work(struct work_struct *work)
> @@ -478,10 +491,15 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
>  	__le16 le16val;
>  	u16 val;
>  	int ret;
> +	int idx;
> +
> +	if (!drm_bridge_enter(bridge, &idx))
> +		return;
>  
>  	ret = regulator_enable(ctx->vcc);
>  	if (ret) {
>  		dev_err(ctx->dev, "Failed to enable vcc: %d\n", ret);
> +		drm_bridge_exit(idx);
>  		return;
>  	}
>  
> @@ -625,6 +643,8 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
>  		dev_err(ctx->dev, "failed to lock PLL, ret=%i\n", ret);
>  		/* On failure, disable PLL again and exit. */
>  		regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> +		ctx->disable_resources_needed = true;
> +		drm_bridge_exit(idx);
>  		return;
>  	}
>  
> @@ -633,6 +653,9 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
>  
>  	/* Wait for 10ms after soft reset as specified in datasheet */
>  	usleep_range(10000, 12000);
> +
> +	ctx->disable_resources_needed = true;
> +	drm_bridge_exit(idx);
>  }
>  
>  static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
> @@ -640,6 +663,10 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
>  {
>  	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>  	unsigned int pval;
> +	int idx;
> +
> +	if (!drm_bridge_enter(bridge, &idx))
> +		return;
>  
>  	/* Clear all errors that got asserted during initialization. */
>  	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
> @@ -659,6 +686,8 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
>  		/* Use the polling task */
>  		sn65dsi83_monitor_start(ctx);
>  	}
> +
> +	drm_bridge_exit(idx);
>  }
>  
>  static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
> @@ -666,6 +695,10 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
>  {
>  	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>  	int ret;
> +	int idx;
> +
> +	if (!drm_bridge_enter(bridge, &idx))
> +		return;
>  
>  	if (ctx->irq) {
>  		/* Disable irq */
> @@ -685,6 +718,9 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
>  		dev_err(ctx->dev, "Failed to disable vcc: %d\n", ret);
>  
>  	regcache_mark_dirty(ctx->regmap);
> +
> +	ctx->disable_resources_needed = false;
> +	drm_bridge_exit(idx);
>  }
>  
>  static enum drm_mode_status
> @@ -1005,7 +1041,24 @@ static void sn65dsi83_remove(struct i2c_client *client)
>  {
>  	struct sn65dsi83 *ctx = i2c_get_clientdata(client);
>  
> +	drm_bridge_unplug(&ctx->bridge);
>  	drm_bridge_remove(&ctx->bridge);

Shouldn't we merge drm_bridge_unplug with the release part of
devm_drm_bridge_alloc?

> +
> +	/*
> +	 * sn65dsi83_atomic_disable() should release some resources, but it
> +	 * cannot if we call drm_bridge_unplug() before it can
> +	 * drm_bridge_enter(). If that happens, let's release those
> +	 * resources now.
> +	 */
> +	if (ctx->disable_resources_needed) {
> +		if (!ctx->irq)
> +			sn65dsi83_monitor_stop(ctx);
> +
> +		gpiod_set_value_cansleep(ctx->enable_gpio, 0);
> +		usleep_range(10000, 11000);
> +
> +		regulator_disable(ctx->vcc);
> +	}

I'm not sure you need this. Wouldn't registering a devm action do the
same thing?

Maxime

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

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: protect device resources on unplug

[Luca Ceresoli]

Hello Maxime,

On Tue, 19 Aug 2025 14:29:32 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> > @@ -1005,7 +1041,24 @@ static void sn65dsi83_remove(struct i2c_client *client)
> >  {
> >  	struct sn65dsi83 *ctx = i2c_get_clientdata(client);
> >  
> > +	drm_bridge_unplug(&ctx->bridge);
> >  	drm_bridge_remove(&ctx->bridge);  
> 
> Shouldn't we merge drm_bridge_unplug with the release part of
> devm_drm_bridge_alloc?

I'm not sure I got what you are suggesting here, sorry.

Do you mean that __devm_drm_bridge_alloc() should add a devres action
to call drm_bridge_unplug(), so the unplug is called implicitly and
does not need to be called explicitly by all drivers?

If that's what you mean, I don't think that would work. Unless I'm
missing something, devres actions are always invoked just after the
driver .remove callback. But we need to call drm_bridge_unplug() at the
beginning (or just before) .remove, at least for drivers that need to do
something in .remove that cannot be done by devm.

In pseudocode:

mybridge_remove()
{
  drm_bridge_unplug(); <-- explicit call as in my patch
  xyz_disable();
  drm_bridge_unplug(); <-- implicitly done by devres
}

We want xyz_disable() to be done after drm_bridge_unplug(), so other
code paths using drm_bridge_enter/exit() won't mess with xyz.

devres actions cannot be added to be executed _before_ .remove, AFAIK.

> > +	/*
> > +	 * sn65dsi83_atomic_disable() should release some resources, but it
> > +	 * cannot if we call drm_bridge_unplug() before it can
> > +	 * drm_bridge_enter(). If that happens, let's release those
> > +	 * resources now.
> > +	 */
> > +	if (ctx->disable_resources_needed) {
> > +		if (!ctx->irq)
> > +			sn65dsi83_monitor_stop(ctx);
> > +
> > +		gpiod_set_value_cansleep(ctx->enable_gpio, 0);
> > +		usleep_range(10000, 11000);
> > +
> > +		regulator_disable(ctx->vcc);
> > +	}  
> 
> I'm not sure you need this. Wouldn't registering a devm action do the
> same thing?

Good idea, thanks. I'll give it a try.

Luca

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

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: protect device resources on unplug

[Maxime Ripard]

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

On Wed, Aug 20, 2025 at 01:13:02PM +0200, Luca Ceresoli wrote:
> Hello Maxime,
> 
> On Tue, 19 Aug 2025 14:29:32 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > > @@ -1005,7 +1041,24 @@ static void sn65dsi83_remove(struct i2c_client *client)
> > >  {
> > >  	struct sn65dsi83 *ctx = i2c_get_clientdata(client);
> > >  
> > > +	drm_bridge_unplug(&ctx->bridge);
> > >  	drm_bridge_remove(&ctx->bridge);  
> > 
> > Shouldn't we merge drm_bridge_unplug with the release part of
> > devm_drm_bridge_alloc?
> 
> I'm not sure I got what you are suggesting here, sorry.
> 
> Do you mean that __devm_drm_bridge_alloc() should add a devres action
> to call drm_bridge_unplug(), so the unplug is called implicitly and
> does not need to be called explicitly by all drivers?

Yes

> If that's what you mean, I don't think that would work. Unless I'm
> missing something, devres actions are always invoked just after the
> driver .remove callback.

Yes, they are called in reverse order of registration, after remove.

> But we need to call drm_bridge_unplug() at the beginning (or just
> before) .remove, at least for drivers that need to do something in
> .remove that cannot be done by devm.
> 
> In pseudocode:
> 
> mybridge_remove()
> {
>   drm_bridge_unplug(); <-- explicit call as in my patch
>   xyz_disable();
>   drm_bridge_unplug(); <-- implicitly done by devres
> }
> 
> We want xyz_disable() to be done after drm_bridge_unplug(), so other
> code paths using drm_bridge_enter/exit() won't mess with xyz.

It's not clear to me why doing it before xyz_disable() is important
here? If anything, it would prevent from disabling the hardware for
example, even though you still have your memory mapping, clocks, power
domains, regulators, etc. to properly disable it.

You're still correct that it's a bad idea though because we want to do
it before we start freeing all those, so it needs to execute as the
before the devm actions ...

> devres actions cannot be added to be executed _before_ .remove, AFAIK.

... and we can't do that either.

Maxime

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