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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250910-glittering-serval-of-piety-b32844@houat>
Date: Wed, 10 Sep 2025 12:59:12 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, 
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>, Jonas Karlman <jonas@...boo.se>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, Hui Pu <Hui.Pu@...ealthcare.com>, 
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	Dmitry Baryshkov <lumag@...nel.org>
Subject: Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: protect device resources
 on unplug

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

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)