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@xxxxxxxxxx> 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