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-amazing-camouflaged-barracuda-bb79cb@houat>
Date: Wed, 10 Sep 2025 09:52:21 +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: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.

Maxime

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