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

Maxime Ripard
2025-09-11T06:44:37Z

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

On Wed, Sep 10, 2025 at 06:34:40PM +0200, Luca Ceresoli wrote:
> Hi Maxime,
>
> On Wed, 10 Sep 2025 09:52:21 +0200
> Maxime Ripard <mripard@kernel.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@bootlin.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.

Kind of, but you shouldn't remove but release it, and it doesn't have to
be a single action / function.

Maxime

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