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

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

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

Hello Maxime,

On Wed, 20 Aug 2025 13:13:02 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> > > +	/*
> > > +	 * 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.

I'm catching up with this series after being busy a few weeks...

I looked at this, but contrary my initial impression I think it would
not be an improvement.

The reason is at least one of these cleanup actions (namely the
regulator_disable()) must be done only if there is a matching enable,
which is in atomic_pre_enable. This is why I introduced a flag in the
first place.

I'm not sure which usage of devres you had in mind, but I see two
options.

Option 1: in probe, add a devres action to call a function like:

sn65dsi83_cleanups()
{
	if (ctx->disable_resources_needed) {
		/* the same cleanups */
	}    
}

But that is just a more indirect way of doing the same thing, and
relies on the same flag.

Option 2: have a function to unconditionally do the cleanups:

sn65dsi83_cleanups()
{
	/* the same cleanups (no if) */
}

And then:
 * in atomic_pre_enable, instead of setting the flag
   add a devres action to call sn65dsi83_cleanups()
 * in atomic_disable, instead of clearing the flag
   remove the devres action

Even this option looks like more complicated and less readable code
to do the same thing.

Do you have in mind a better option that I haven't figured out?

If you don't, I think this part of the patch should stay as is.

Luca

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

On Mon, Sep 08, 2025 at 03:49:01PM +0200, Luca Ceresoli wrote:
> Hello Maxime,
> 
> On Wed, 20 Aug 2025 13:13:02 +0200
> Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> 
> > > > +	/*
> > > > +	 * 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.
> 
> I'm catching up with this series after being busy a few weeks...
> 
> I looked at this, but contrary my initial impression I think it would
> not be an improvement.
> 
> The reason is at least one of these cleanup actions (namely the
> regulator_disable()) must be done only if there is a matching enable,
> which is in atomic_pre_enable. This is why I introduced a flag in the
> first place.
> 
> I'm not sure which usage of devres you had in mind, but I see two
> options.
> 
> Option 1: in probe, add a devres action to call a function like:
> 
> sn65dsi83_cleanups()
> {
> 	if (ctx->disable_resources_needed) {
> 		/* the same cleanups */
> 	}    
> }
> 
> But that is just a more indirect way of doing the same thing, and
> relies on the same flag.
> 
> Option 2: have a function to unconditionally do the cleanups:
> 
> sn65dsi83_cleanups()
> {
> 	/* the same cleanups (no if) */
> }
> 
> And then:
>  * in atomic_pre_enable, instead of setting the flag
>    add a devres action to call sn65dsi83_cleanups()
>  * in atomic_disable, instead of clearing the flag
>    remove the devres action
> 
> Even this option looks like more complicated and less readable code
> to do the same thing.
> 
> Do you have in mind a better option that I haven't figured out?

Would using devm_add_action in atomic_pre_enable, and
devm_release_action in atomic_post_disable work?

That way, if you have a typical enable / disable cycle, the action will
get registered and executed properly, and if you only have an enable but
no matching disable, it will be collected after remove.

Maxime

Hi Maxime,

On Wed, 10 Sep 2025 09:52:21 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Mon, Sep 08, 2025 at 03:49:01PM +0200, Luca Ceresoli wrote:
> > Hello Maxime,
> > 
> > On Wed, 20 Aug 2025 13:13:02 +0200
> > Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> >   
> > > > > +	/*
> > > > > +	 * 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.  
> > 
> > I'm catching up with this series after being busy a few weeks...
> > 
> > I looked at this, but contrary my initial impression I think it would
> > not be an improvement.
> > 
> > The reason is at least one of these cleanup actions (namely the
> > regulator_disable()) must be done only if there is a matching enable,
> > which is in atomic_pre_enable. This is why I introduced a flag in the
> > first place.
> > 
> > I'm not sure which usage of devres you had in mind, but I see two
> > options.
> > 
> > Option 1: in probe, add a devres action to call a function like:
> > 
> > sn65dsi83_cleanups()
> > {
> > 	if (ctx->disable_resources_needed) {
> > 		/* the same cleanups */
> > 	}    
> > }
> > 
> > But that is just a more indirect way of doing the same thing, and
> > relies on the same flag.
> > 
> > Option 2: have a function to unconditionally do the cleanups:
> > 
> > sn65dsi83_cleanups()
> > {
> > 	/* the same cleanups (no if) */
> > }
> > 
> > And then:
> >  * in atomic_pre_enable, instead of setting the flag
> >    add a devres action to call sn65dsi83_cleanups()
> >  * in atomic_disable, instead of clearing the flag
> >    remove the devres action
> > 
> > Even this option looks like more complicated and less readable code
> > to do the same thing.
> > 
> > Do you have in mind a better option that I haven't figured out?  
> 
> Would using devm_add_action in atomic_pre_enable, and
> devm_release_action in atomic_post_disable work?
> 
> That way, if you have a typical enable / disable cycle, the action will
> get registered and executed properly, and if you only have an enable but
> no matching disable, it will be collected after remove.

So you're OK with option 2. I just implemented it, works well and the
resulting code is a bit cleaner. Queued for v2.

Luca

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

On Wed, Sep 10, 2025 at 06:34:40PM +0200, Luca Ceresoli wrote:
> Hi Maxime,
> 
> On Wed, 10 Sep 2025 09:52:21 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > On Mon, Sep 08, 2025 at 03:49:01PM +0200, Luca Ceresoli wrote:
> > > Hello Maxime,
> > > 
> > > On Wed, 20 Aug 2025 13:13:02 +0200
> > > Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > >   
> > > > > > +	/*
> > > > > > +	 * 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.  
> > > 
> > > I'm catching up with this series after being busy a few weeks...
> > > 
> > > I looked at this, but contrary my initial impression I think it would
> > > not be an improvement.
> > > 
> > > The reason is at least one of these cleanup actions (namely the
> > > regulator_disable()) must be done only if there is a matching enable,
> > > which is in atomic_pre_enable. This is why I introduced a flag in the
> > > first place.
> > > 
> > > I'm not sure which usage of devres you had in mind, but I see two
> > > options.
> > > 
> > > Option 1: in probe, add a devres action to call a function like:
> > > 
> > > sn65dsi83_cleanups()
> > > {
> > > 	if (ctx->disable_resources_needed) {
> > > 		/* the same cleanups */
> > > 	}    
> > > }
> > > 
> > > But that is just a more indirect way of doing the same thing, and
> > > relies on the same flag.
> > > 
> > > Option 2: have a function to unconditionally do the cleanups:
> > > 
> > > sn65dsi83_cleanups()
> > > {
> > > 	/* the same cleanups (no if) */
> > > }
> > > 
> > > And then:
> > >  * in atomic_pre_enable, instead of setting the flag
> > >    add a devres action to call sn65dsi83_cleanups()
> > >  * in atomic_disable, instead of clearing the flag
> > >    remove the devres action
> > > 
> > > Even this option looks like more complicated and less readable code
> > > to do the same thing.
> > > 
> > > Do you have in mind a better option that I haven't figured out?  
> > 
> > Would using devm_add_action in atomic_pre_enable, and
> > devm_release_action in atomic_post_disable work?
> > 
> > That way, if you have a typical enable / disable cycle, the action will
> > get registered and executed properly, and if you only have an enable but
> > no matching disable, it will be collected after remove.
> 
> So you're OK with option 2. I just implemented it, works well and the
> resulting code is a bit cleaner. Queued for v2.

Kind of, but you shouldn't remove but release it, and it doesn't have to
be a single action / function.

Maxime

Hi Maxime,

On Thu, 11 Sep 2025 08:44:34 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> > > > Option 2: have a function to unconditionally do the cleanups:
> > > > 
> > > > sn65dsi83_cleanups()
> > > > {
> > > > 	/* the same cleanups (no if) */
> > > > }
> > > > 
> > > > And then:
> > > >  * in atomic_pre_enable, instead of setting the flag
> > > >    add a devres action to call sn65dsi83_cleanups()
> > > >  * in atomic_disable, instead of clearing the flag
> > > >    remove the devres action
> > > > 
> > > > Even this option looks like more complicated and less readable code
> > > > to do the same thing.
> > > > 
> > > > Do you have in mind a better option that I haven't figured out?    
> > > 
> > > Would using devm_add_action in atomic_pre_enable, and
> > > devm_release_action in atomic_post_disable work?
> > > 
> > > That way, if you have a typical enable / disable cycle, the action will
> > > get registered and executed properly, and if you only have an enable but
> > > no matching disable, it will be collected after remove.  
> > 
> > So you're OK with option 2. I just implemented it, works well and the
> > resulting code is a bit cleaner. Queued for v2.  
> 
> Kind of, but you shouldn't remove but release it, and it doesn't have to
> be a single action / function.

Released instead of removed: yes.

Doesn't have to be a single function: I currently implemented a single
function with the 3 actions that are currently done in atomic_disable.
I think you propose to add 3 different devres actions, one for each,
but it would be more code and a little more resources used and I don't
see the advantage.

I think it makes sense that I send my current version and we can
continue discussion based on the code. I'm waiting a bit before
sending it, in case you have feedback about the other branch of this
discussion (placement of drm_bridge_unplug()).

Luca

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

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

Hi Maxime,

On Wed, 27 Aug 2025 09:46:03 +0200
Maxime Ripard <mripard@kernel.org> wrote:

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

I understand your words as "the drm_bridge_unplug() is OK where it is,
your patch is OK in this respect". Correct?

So if this is correct, and my reply on the devres cleanups is also
correct (other reply in this thread), that means the whole patch is OK.

Let me know if I'm wrong. :-)

Best regards,
Luca

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

On Mon, Sep 08, 2025 at 03:49:06PM +0200, Luca Ceresoli wrote:
> Hi Maxime,
> 
> On Wed, 27 Aug 2025 09:46:03 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > 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.
> 
> I understand your words as "the drm_bridge_unplug() is OK where it is,
> your patch is OK in this respect". Correct?
> 
> So if this is correct, and my reply on the devres cleanups is also
> correct (other reply in this thread), that means the whole patch is OK.

I'm still confused why it's so important than in your example
xyz_disable must be called after drm_bridge_unplug.

Maxime

On Wed, 10 Sep 2025 12:59:12 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Mon, Sep 08, 2025 at 03:49:06PM +0200, Luca Ceresoli wrote:
> > Hi Maxime,
> > 
> > On Wed, 27 Aug 2025 09:46:03 +0200
> > Maxime Ripard <mripard@kernel.org> wrote:
> >   
> > > 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.  
> > 
> > I understand your words as "the drm_bridge_unplug() is OK where it is,
> > your patch is OK in this respect". Correct?
> > 
> > So if this is correct, and my reply on the devres cleanups is also
> > correct (other reply in this thread), that means the whole patch is OK.  
> 
> I'm still confused why it's so important than in your example
> xyz_disable must be called after drm_bridge_unplug.

Let me clarify with an example.

As I wrote in another reply, I have moved from a flag
(disable_resources_needed) to a devres action as you had suggested, but
the example here is based on the old flag because it is more explicit,
code would be executed in the same order anyway, and, well, because I
had written the example before the devres action conversion.

Take these two functions (stripped versions of the actual ones):

/* Same as proposed, but with _unplug moved at the end */
static void sn65dsi83_remove()
{
	struct sn65dsi83 *ctx = i2c_get_clientdata(client);

	drm_bridge_remove(&ctx->bridge);
	
	/* 
	 * I moved the following code to a devm action, but keeping it
	 * explicit here for the discussion
	 */
	if (ctx->disable_resources_needed) {
		sn65dsi83_monitor_stop(ctx);
		regulator_disable(ctx->vcc);
	}
	
	drm_bridge_unplug(&ctx->bridge);     // At the end!
}

static void sn65dsi83_atomic_disable()
{
	if (!drm_bridge_enter(bridge, &idx))
		return;

	/* These 3 lines will be replaced by devm_release_action() */
	ctx->disable_resources_needed = false;
	sn65dsi83_monitor_stop(ctx);
	regulator_disable(ctx->vcc);

	drm_bridge_exit(idx);
}

Here the xyz_disable() in my pseudocode is the sn65dsi83_monitor_stop()
+ regulator_disable().

If sn65dsi83_remove() and sn65dsi83_atomic_disable() were to happen
concurrently, this sequence of events could happen:

1. atomic_disable:  drm_bridge_enter() -> OK, can go
2. remove:          drm_bridge_remove()
3. remove:          sn65dsi83_monitor_stop()
4. remove:          regulator_disable()
5. remove:          drm_bridge_unplug() -- too late to stop atomic_disable
6. atomic_disable:  ctx->disable_resources_needed = false -- too late to stop .remove
7. atomic_disable:  sn65dsi83_monitor_stop() -- twice, maybe no problem
8. atomic_disable:  regulator_disable() -- Twice, en/disable imbalance!

So there is an excess regulator disable, which is an error. I don't see
how this can be avoided if the drm_bridge_unplug() is called after the
regulator_disable().

Let me know whether this clarifies the need to _unplug at the beginning
of the .remove function.

Best regards,
Luca

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

On Wed, Sep 10, 2025 at 06:47:52PM +0200, Luca Ceresoli wrote:
> On Wed, 10 Sep 2025 12:59:12 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > On Mon, Sep 08, 2025 at 03:49:06PM +0200, Luca Ceresoli wrote:
> > > Hi Maxime,
> > > 
> > > On Wed, 27 Aug 2025 09:46:03 +0200
> > > Maxime Ripard <mripard@kernel.org> wrote:
> > >   
> > > > 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.  
> > > 
> > > I understand your words as "the drm_bridge_unplug() is OK where it is,
> > > your patch is OK in this respect". Correct?
> > > 
> > > So if this is correct, and my reply on the devres cleanups is also
> > > correct (other reply in this thread), that means the whole patch is OK.  
> > 
> > I'm still confused why it's so important than in your example
> > xyz_disable must be called after drm_bridge_unplug.
> 
> Let me clarify with an example.
> 
> As I wrote in another reply, I have moved from a flag
> (disable_resources_needed) to a devres action as you had suggested, but
> the example here is based on the old flag because it is more explicit,
> code would be executed in the same order anyway, and, well, because I
> had written the example before the devres action conversion.
> 
> Take these two functions (stripped versions of the actual ones):
> 
> /* Same as proposed, but with _unplug moved at the end */
> static void sn65dsi83_remove()
> {
> 	struct sn65dsi83 *ctx = i2c_get_clientdata(client);
> 
> 	drm_bridge_remove(&ctx->bridge);
> 	
> 	/* 
> 	 * I moved the following code to a devm action, but keeping it
> 	 * explicit here for the discussion
> 	 */
> 	if (ctx->disable_resources_needed) {
> 		sn65dsi83_monitor_stop(ctx);
> 		regulator_disable(ctx->vcc);
> 	}
> 	
> 	drm_bridge_unplug(&ctx->bridge);     // At the end!
> }

First off, why do we need to have drm_bridge_unplug and
drm_bridge_remove separate?

If we were to mirror drm_dev_enter and drm_dev_unplug, drm_dev_unplug
calls drm_dev_unregister itself, and I can't find a reason where we
might want to split the two.

> static void sn65dsi83_atomic_disable()
> {
> 	if (!drm_bridge_enter(bridge, &idx))
> 		return;
> 
> 	/* These 3 lines will be replaced by devm_release_action() */
> 	ctx->disable_resources_needed = false;
> 	sn65dsi83_monitor_stop(ctx);
> 	regulator_disable(ctx->vcc);
> 
> 	drm_bridge_exit(idx);
> }
> 
> Here the xyz_disable() in my pseudocode is the sn65dsi83_monitor_stop()
> + regulator_disable().
> 
> If sn65dsi83_remove() and sn65dsi83_atomic_disable() were to happen
> concurrently, this sequence of events could happen:
> 
> 1. atomic_disable:  drm_bridge_enter() -> OK, can go
> 2. remove:          drm_bridge_remove()
> 3. remove:          sn65dsi83_monitor_stop()
> 4. remove:          regulator_disable()
> 5. remove:          drm_bridge_unplug() -- too late to stop atomic_disable

drm_dev_unplug would also get delayed until drm_dev_exit is called,
mitigating your issue here.

> 6. atomic_disable:  ctx->disable_resources_needed = false -- too late to stop .remove
> 7. atomic_disable:  sn65dsi83_monitor_stop() -- twice, maybe no problem
> 8. atomic_disable:  regulator_disable() -- Twice, en/disable imbalance!
> 
> So there is an excess regulator disable, which is an error. I don't see
> how this can be avoided if the drm_bridge_unplug() is called after the
> regulator_disable().
> 
> Let me know whether this clarifies the need to _unplug at the beginning
> of the .remove function.

Another thing that just crossed my mind is why we don't call
atomic_disable when we're tearing down the bridge too. We're doing it
for the main DRM devices, it would make sense to me to disable the
encoder -> bridge -> connector (and possibly CRTC) chain if we remove a
bridge automatically.

Maxime

Hi Maxime,

thanks for the feedback, this discussion is getting very interesting!

On Mon, 15 Sep 2025 14:03:17 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> > > I'm still confused why it's so important than in your example
> > > xyz_disable must be called after drm_bridge_unplug.  
> > 
> > Let me clarify with an example.
> > 
> > As I wrote in another reply, I have moved from a flag
> > (disable_resources_needed) to a devres action as you had suggested, but
> > the example here is based on the old flag because it is more explicit,
> > code would be executed in the same order anyway, and, well, because I
> > had written the example before the devres action conversion.
> > 
> > Take these two functions (stripped versions of the actual ones):
> > 
> > /* Same as proposed, but with _unplug moved at the end */
> > static void sn65dsi83_remove()
> > {
> > 	struct sn65dsi83 *ctx = i2c_get_clientdata(client);
> > 
> > 	drm_bridge_remove(&ctx->bridge);
> > 	
> > 	/* 
> > 	 * I moved the following code to a devm action, but keeping it
> > 	 * explicit here for the discussion
> > 	 */
> > 	if (ctx->disable_resources_needed) {
> > 		sn65dsi83_monitor_stop(ctx);
> > 		regulator_disable(ctx->vcc);
> > 	}
> > 	
> > 	drm_bridge_unplug(&ctx->bridge);     // At the end!
> > }  
> 
> First off, why do we need to have drm_bridge_unplug and
> drm_bridge_remove separate?
> 
> If we were to mirror drm_dev_enter and drm_dev_unplug, drm_dev_unplug
> calls drm_dev_unregister itself, and I can't find a reason where we
> might want to split the two.

I think it could make sense and I'm definitely open to it.

After a quick analysis I have mostly one concern. Calls
to drm_bridge_add() and drm_bridge_remove() are balanced in current
code and that's very intuitive. If drm_bridge_unplug() were to call
drm_bridge_remove(), that symmetry would disappear. Some drivers would
still need to call drm_bridge_remove() directly (e.g. the DSI host
drivers which _add/remove() in the DSI attach/detach callbacks), while
other wouldn't because drm_bridge_unplug() would do that.

What do you think about this?

Another concern I initially had is about drivers whose usage of
drm_bridge is more complex than the average. Most simple drivers just
call drm_bridge_remove() in their .remove callback and that's
straightforward. I was suspicious about drivers such as
imx8qxp-pixel-combiner which instantiate multiple bridges, and whether
they need do all the drm_bridge_unplug()s before all the
drm_bridge_remove()s. However I don't think that's a real need because,
except for probe and removal, operations on bridges happen on a
per-bridge basis, so each bridge is independent from others, at least
for the driver I mentioned.

> > static void sn65dsi83_atomic_disable()
> > {
> > 	if (!drm_bridge_enter(bridge, &idx))
> > 		return;
> > 
> > 	/* These 3 lines will be replaced by devm_release_action() */
> > 	ctx->disable_resources_needed = false;
> > 	sn65dsi83_monitor_stop(ctx);
> > 	regulator_disable(ctx->vcc);
> > 
> > 	drm_bridge_exit(idx);
> > }
> > 
> > Here the xyz_disable() in my pseudocode is the sn65dsi83_monitor_stop()
> > + regulator_disable().
> > 
> > If sn65dsi83_remove() and sn65dsi83_atomic_disable() were to happen
> > concurrently, this sequence of events could happen:
> > 
> > 1. atomic_disable:  drm_bridge_enter() -> OK, can go
> > 2. remove:          drm_bridge_remove()
> > 3. remove:          sn65dsi83_monitor_stop()
> > 4. remove:          regulator_disable()
> > 5. remove:          drm_bridge_unplug() -- too late to stop atomic_disable  
> 
> drm_dev_unplug would also get delayed until drm_dev_exit is called,
> mitigating your issue here.

I don't think I got what you mean. With the above code the regulator
would still be subject to an en/disable imbalance.

However I realized the problem does not exist when using devres,
because devres itself takes care of executing each release function only
once, by means of a spinlock.

I think using devres actually solves my concerns about removal during
atomic[_post]_disable, but also for the atomic[_pre]_enable and other
call paths. Also, I think it makes the question of which goes first
(drm_bridge_unplug() or _remove()) way less relevant.

The concern is probably still valid for drivers which don't use devres.
However the concern is irrelevant until there is a need for a bridge to
become hot-pluggable. At that point a driver needs to either move to
devres or take other actions to avoid incurring in the same issue.

I'm going to send soon a v2 with my devres changes so we can continue
this discussion on actual code.

> > 6. atomic_disable:  ctx->disable_resources_needed = false -- too late to stop .remove
> > 7. atomic_disable:  sn65dsi83_monitor_stop() -- twice, maybe no problem
> > 8. atomic_disable:  regulator_disable() -- Twice, en/disable imbalance!
> > 
> > So there is an excess regulator disable, which is an error. I don't see
> > how this can be avoided if the drm_bridge_unplug() is called after the
> > regulator_disable().
> > 
> > Let me know whether this clarifies the need to _unplug at the beginning
> > of the .remove function.  
> 
> Another thing that just crossed my mind is why we don't call
> atomic_disable when we're tearing down the bridge too. We're doing it
> for the main DRM devices, it would make sense to me to disable the
> encoder -> bridge -> connector (and possibly CRTC) chain if we remove a
> bridge automatically.

Uh, interesting idea.

Do you mean something like:

void drm_bridge_unplug(struct drm_bridge *bridge)
{
    bridge->unplugged = true;
    synchronize_srcu(&drm_bridge_unplug_srcu);

    drm_bridge_remove(bridge); // as per discussion above

    drm_atomic_helper_shutdown(bridge->dev);
}

?

I'm not sure which is the right call to tear down the pipeline though.

Luca

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

On Mon, Sep 15, 2025 at 04:51:56PM +0200, Luca Ceresoli wrote:
> Hi Maxime,
> 
> thanks for the feedback, this discussion is getting very interesting!
> 
> On Mon, 15 Sep 2025 14:03:17 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > > > I'm still confused why it's so important than in your example
> > > > xyz_disable must be called after drm_bridge_unplug.  
> > > 
> > > Let me clarify with an example.
> > > 
> > > As I wrote in another reply, I have moved from a flag
> > > (disable_resources_needed) to a devres action as you had suggested, but
> > > the example here is based on the old flag because it is more explicit,
> > > code would be executed in the same order anyway, and, well, because I
> > > had written the example before the devres action conversion.
> > > 
> > > Take these two functions (stripped versions of the actual ones):
> > > 
> > > /* Same as proposed, but with _unplug moved at the end */
> > > static void sn65dsi83_remove()
> > > {
> > > 	struct sn65dsi83 *ctx = i2c_get_clientdata(client);
> > > 
> > > 	drm_bridge_remove(&ctx->bridge);
> > > 	
> > > 	/* 
> > > 	 * I moved the following code to a devm action, but keeping it
> > > 	 * explicit here for the discussion
> > > 	 */
> > > 	if (ctx->disable_resources_needed) {
> > > 		sn65dsi83_monitor_stop(ctx);
> > > 		regulator_disable(ctx->vcc);
> > > 	}
> > > 	
> > > 	drm_bridge_unplug(&ctx->bridge);     // At the end!
> > > }  
> > 
> > First off, why do we need to have drm_bridge_unplug and
> > drm_bridge_remove separate?
> > 
> > If we were to mirror drm_dev_enter and drm_dev_unplug, drm_dev_unplug
> > calls drm_dev_unregister itself, and I can't find a reason where we
> > might want to split the two.
> 
> I think it could make sense and I'm definitely open to it.
> 
> After a quick analysis I have mostly one concern. Calls
> to drm_bridge_add() and drm_bridge_remove() are balanced in current
> code and that's very intuitive. If drm_bridge_unplug() were to call
> drm_bridge_remove(), that symmetry would disappear. Some drivers would
> still need to call drm_bridge_remove() directly (e.g. the DSI host
> drivers which _add/remove() in the DSI attach/detach callbacks), while
> other wouldn't because drm_bridge_unplug() would do that.
> 
> What do you think about this?

Which DSI host do you have in mind there? Because it's really not what
we document.

> Another concern I initially had is about drivers whose usage of
> drm_bridge is more complex than the average. Most simple drivers just
> call drm_bridge_remove() in their .remove callback and that's
> straightforward. I was suspicious about drivers such as
> imx8qxp-pixel-combiner which instantiate multiple bridges, and whether
> they need do all the drm_bridge_unplug()s before all the
> drm_bridge_remove()s. However I don't think that's a real need because,
> except for probe and removal, operations on bridges happen on a
> per-bridge basis, so each bridge is independent from others, at least
> for the driver I mentioned.

In this particular case, they would be unplugged all at the same time,
right? In which case, we would disable all the bridges starting from the
one in the chain that just got removed, and then we just have to remove
all of them.

All in all, I think it's ok to somewhat break things here: all this was
broken before. If we want to bring some consistency, we will have to
reduce what bridges are allowed to do. Let's figure out something that
works for all reasonable cases (straightforward, component framework,
DSI device, DSI host, and DSI device on another bus), and the hacky
drivers will move eventually.

That's pretty easy to solve with a documentation update :)

We can just further restrict the order in which

> > > static void sn65dsi83_atomic_disable()
> > > {
> > > 	if (!drm_bridge_enter(bridge, &idx))
> > > 		return;
> > > 
> > > 	/* These 3 lines will be replaced by devm_release_action() */
> > > 	ctx->disable_resources_needed = false;
> > > 	sn65dsi83_monitor_stop(ctx);
> > > 	regulator_disable(ctx->vcc);
> > > 
> > > 	drm_bridge_exit(idx);
> > > }
> > > 
> > > Here the xyz_disable() in my pseudocode is the sn65dsi83_monitor_stop()
> > > + regulator_disable().
> > > 
> > > If sn65dsi83_remove() and sn65dsi83_atomic_disable() were to happen
> > > concurrently, this sequence of events could happen:
> > > 
> > > 1. atomic_disable:  drm_bridge_enter() -> OK, can go
> > > 2. remove:          drm_bridge_remove()
> > > 3. remove:          sn65dsi83_monitor_stop()
> > > 4. remove:          regulator_disable()
> > > 5. remove:          drm_bridge_unplug() -- too late to stop atomic_disable  
> > 
> > drm_dev_unplug would also get delayed until drm_dev_exit is called,
> > mitigating your issue here.
> 
> I don't think I got what you mean. With the above code the regulator
> would still be subject to an en/disable imbalance.

My point was that drm_bridge_remove wouldn't be allowed to execute until
after atomic_disable has called drm_bridge_exit. So we wouldn't have the
sequence of events you described. atomic_disable would disable the
bridge, and then drm_bridge_remove wouln't have anything to disable
anymore by the time it runs.

> However I realized the problem does not exist when using devres,
> because devres itself takes care of executing each release function only
> once, by means of a spinlock.
> 
> I think using devres actually solves my concerns about removal during
> atomic[_post]_disable, but also for the atomic[_pre]_enable and other
> call paths. Also, I think it makes the question of which goes first
> (drm_bridge_unplug() or _remove()) way less relevant.
> 
> The concern is probably still valid for drivers which don't use devres.
> However the concern is irrelevant until there is a need for a bridge to
> become hot-pluggable. At that point a driver needs to either move to
> devres or take other actions to avoid incurring in the same issue.

I disagree with that statement. We never considered !devres as outdated,
and thus we need to support both. Especially if it's about races we know
about in a code path we might never run.

> I'm going to send soon a v2 with my devres changes so we can continue
> this discussion on actual code.
> 
> > > 6. atomic_disable:  ctx->disable_resources_needed = false -- too late to stop .remove
> > > 7. atomic_disable:  sn65dsi83_monitor_stop() -- twice, maybe no problem
> > > 8. atomic_disable:  regulator_disable() -- Twice, en/disable imbalance!
> > > 
> > > So there is an excess regulator disable, which is an error. I don't see
> > > how this can be avoided if the drm_bridge_unplug() is called after the
> > > regulator_disable().
> > > 
> > > Let me know whether this clarifies the need to _unplug at the beginning
> > > of the .remove function.  
> > 
> > Another thing that just crossed my mind is why we don't call
> > atomic_disable when we're tearing down the bridge too. We're doing it
> > for the main DRM devices, it would make sense to me to disable the
> > encoder -> bridge -> connector (and possibly CRTC) chain if we remove a
> > bridge automatically.
> 
> Uh, interesting idea.
> 
> Do you mean something like:
> 
> void drm_bridge_unplug(struct drm_bridge *bridge)
> {
>     bridge->unplugged = true;
>     synchronize_srcu(&drm_bridge_unplug_srcu);
> 
>     drm_bridge_remove(bridge); // as per discussion above
> 
>     drm_atomic_helper_shutdown(bridge->dev);
> }
> 
> ?
> 
> I'm not sure which is the right call to tear down the pipeline though.

No, the shutdown needs to happen before marking the bridge unplugged,
otherwise you'll never run the disable callbacks.

And we probably shouldn't disable the whole device, just everything from
the CRTC that feeds the bridge.

Maxime

Hi Maxime,

On Tue Oct 7, 2025 at 5:09 PM CEST, Maxime Ripard wrote:
>> > First off, why do we need to have drm_bridge_unplug and
>> > drm_bridge_remove separate?
>> >
>> > If we were to mirror drm_dev_enter and drm_dev_unplug, drm_dev_unplug
>> > calls drm_dev_unregister itself, and I can't find a reason where we
>> > might want to split the two.
>>
>> I think it could make sense and I'm definitely open to it.
>>
>> After a quick analysis I have mostly one concern. Calls
>> to drm_bridge_add() and drm_bridge_remove() are balanced in current
>> code and that's very intuitive. If drm_bridge_unplug() were to call
>> drm_bridge_remove(), that symmetry would disappear. Some drivers would
>> still need to call drm_bridge_remove() directly (e.g. the DSI host
>> drivers which _add/remove() in the DSI attach/detach callbacks), while
>> other wouldn't because drm_bridge_unplug() would do that.
>>
>> What do you think about this?
>
> Which DSI host do you have in mind there? Because it's really not what
> we document.

Most DSI host drivers (but not all). I did a classification a few months
ago (~v6.15) and found that the following drivers do the same
(drm_bridge_add in the DSI host attach callback, drm_bridge_remove in the
DSI host detach callback):

     drivers/gpu/drm/bridge/samsung-dsim.c           (see below)
     drivers/gpu/drm/vc4/vc4_dsi.c
     drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
     drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
     drivers/gpu/drm/mediatek/mtk_dsi.c
     drivers/gpu/drm/bridge/tc358768.c
     drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi2.c
     drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
     drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
     drivers/gpu/drm/adp/adp-mipi.c

while these do differently:

   - drm_bridge_add/remove() in driver probe/remove:
       drivers/gpu/drm/omapdrm/dss/dsi.c
       drivers/gpu/drm/bridge/nwl-dsi.c
   - drm_bridge_add/remove() in component_ops.bind/unbind:
       drivers/gpu/drm/mcde/mcde_dsi.c

I had sent a patch to change this in the samsung-dsim driver a while ago:
https://lore.kernel.org/lkml/20250725-drm-bridge-samsung-dsim-add-in-probe-v1-1-b23d29c23fbd@bootlin.com/

However your review to that patch suggested to me that you consider OK what
samsung-dsim and most other DSI host drivers currently do (but it's being
discussed in the above-linked thread, I'd say let's keep it there).

Now I am confused. Do you mean that those drivers' DSI host detach callback
should call drm_bridge_unplug(), which in turn would call
drm_bridge_remove()?

I was assuming drm_bridge_unplug() would always be called in the driver
.remove callback.

>> Another concern I initially had is about drivers whose usage of
>> drm_bridge is more complex than the average. Most simple drivers just
>> call drm_bridge_remove() in their .remove callback and that's
>> straightforward. I was suspicious about drivers such as
>> imx8qxp-pixel-combiner which instantiate multiple bridges, and whether
>> they need do all the drm_bridge_unplug()s before all the
>> drm_bridge_remove()s. However I don't think that's a real need because,
>> except for probe and removal, operations on bridges happen on a
>> per-bridge basis, so each bridge is independent from others, at least
>> for the driver I mentioned.
>
> In this particular case, they would be unplugged all at the same time,
> right? In which case, we would disable all the bridges starting from the
> one in the chain that just got removed, and then we just have to remove
> all of them.
>
> All in all, I think it's ok to somewhat break things here: all this was
> broken before.

...

> If we want to bring some consistency, we will have to
> reduce what bridges are allowed to do. Let's figure out something that
> works for all reasonable cases (straightforward, component framework,
> DSI device, DSI host, and DSI device on another bus), and the hacky
> drivers will move eventually.

I agree with the principle in the quoted lines. But I'm not sure exactly
which "something that works for all reasonable cases" you are suggesting:
moving drm_bridge_remove() inside drm_bridge_unplug()?

> That's pretty easy to solve with a documentation update :)
>
> We can just further restrict the order in which

Oops, unfinished sentence?

>> > > static void sn65dsi83_atomic_disable()
>> > > {
>> > > 	if (!drm_bridge_enter(bridge, &idx))
>> > > 		return;
>> > >
>> > > 	/* These 3 lines will be replaced by devm_release_action() */
>> > > 	ctx->disable_resources_needed = false;
>> > > 	sn65dsi83_monitor_stop(ctx);
>> > > 	regulator_disable(ctx->vcc);
>> > >
>> > > 	drm_bridge_exit(idx);
>> > > }
>> > >
>> > > Here the xyz_disable() in my pseudocode is the sn65dsi83_monitor_stop()
>> > > + regulator_disable().
>> > >
>> > > If sn65dsi83_remove() and sn65dsi83_atomic_disable() were to happen
>> > > concurrently, this sequence of events could happen:
>> > >
>> > > 1. atomic_disable:  drm_bridge_enter() -> OK, can go
>> > > 2. remove:          drm_bridge_remove()
>> > > 3. remove:          sn65dsi83_monitor_stop()
>> > > 4. remove:          regulator_disable()
>> > > 5. remove:          drm_bridge_unplug() -- too late to stop atomic_disable
>> >
>> > drm_dev_unplug would also get delayed until drm_dev_exit is called,
>> > mitigating your issue here.
>>
>> I don't think I got what you mean. With the above code the regulator
>> would still be subject to an en/disable imbalance.
>
> My point was that drm_bridge_remove wouldn't be allowed to execute until
> after atomic_disable has called drm_bridge_exit. So we wouldn't have the
> sequence of events you described. atomic_disable would disable the
> bridge, and then drm_bridge_remove wouln't have anything to disable
> anymore by the time it runs.

Ah, I think you was reasoning about the case with drm_bridge_remove()
called by drm_bridge_unplug(). If that's the case, yes, that's probably
right, e.g. with this drm_bridge_unplug() implementation:

void drm_bridge_unplug(struct drm_bridge *bridge)
{
	bridge->unplugged = true;

	synchronize_srcu(&drm_bridge_unplug_srcu);

	drm_bridge_remove();
}

Oh, wow, this looks suspiciourly similar to drm_dev_unplug(). ;)

>> However I realized the problem does not exist when using devres,
>> because devres itself takes care of executing each release function only
>> once, by means of a spinlock.
>>
>> I think using devres actually solves my concerns about removal during
>> atomic[_post]_disable, but also for the atomic[_pre]_enable and other
>> call paths. Also, I think it makes the question of which goes first
>> (drm_bridge_unplug() or _remove()) way less relevant.
>>
>> The concern is probably still valid for drivers which don't use devres.
>> However the concern is irrelevant until there is a need for a bridge to
>> become hot-pluggable. At that point a driver needs to either move to
>> devres or take other actions to avoid incurring in the same issue.
>
> I disagree with that statement. We never considered !devres as outdated,
> and thus we need to support both. Especially if it's about races we know
> about in a code path we might never run.

I didn't mean that, sorry if I was not clear.

I was meaning: for drivers which don't care about supporting hotplug, this
whole discussion is quite irrelevant. Those which will need to support
hotplug will have to ensure to release device resources exactly once, and
can do that either using devres or by other means.

>> > Another thing that just crossed my mind is why we don't call
>> > atomic_disable when we're tearing down the bridge too. We're doing it
>> > for the main DRM devices, it would make sense to me to disable the
>> > encoder -> bridge -> connector (and possibly CRTC) chain if we remove a
>> > bridge automatically.
>>
>> Uh, interesting idea.
>>
>> Do you mean something like:
>>
>> void drm_bridge_unplug(struct drm_bridge *bridge)
>> {
>>     bridge->unplugged = true;
>>     synchronize_srcu(&drm_bridge_unplug_srcu);
>>
>>     drm_bridge_remove(bridge); // as per discussion above
>>
>>     drm_atomic_helper_shutdown(bridge->dev);
>> }
>>
>> ?
>>
>> I'm not sure which is the right call to tear down the pipeline though.
>
> No, the shutdown needs to happen before marking the bridge unplugged,
> otherwise you'll never run the disable callbacks.

OK.

> And we probably shouldn't disable the whole device, just everything from
> the CRTC that feeds the bridge.

OK, I'll need to figure out how todo that exactly. If you have hints, that
would be great.

Anyway I think it's better to do this in steps: first introduce
drm_bridge_unplug() as discussed, and at a later step augment it to disable
the involved pipeline components on bridge removal. Sounds good?

Best regards,
Luca

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

Hi Maxime,

On Mon, 15 Sep 2025 16:51:56 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> I'm going to send soon a v2 with my devres changes so we can continue
> this discussion on actual code.

v2 sent, with the devres changes. Discussion con continue there.

Best regards,
Luca

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