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: <20250910183440.3fe50fac@booty>
Date: Wed, 10 Sep 2025 18:34:40 +0200
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Maxime Ripard <mripard@...nel.org>
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

Hi Maxime,

On Wed, 10 Sep 2025 09:52:21 +0200
Maxime Ripard <mripard@...nel.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@...tlin.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