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

[Maxime Ripard <mripard@kernel.org>]

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

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

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