linux-kernel - Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

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 for Android: free password hash cracker in your pocket

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

Message-ID: <hd3hobuaunmn2uqzl72yv7nz2ms25fczc264wmt6o7twrxdhsy@mm22ujnawutc>
Date: Thu, 22 May 2025 16:06:47 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Claudiu Beznea <claudiu.beznea@...on.dev>, 
	Jonathan Cameron <jic23@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, dakr@...nel.org, linux-kernel@...r.kernel.org, 
	linux-pm@...r.kernel.org, linux-renesas-soc@...r.kernel.org, geert@...ux-m68k.org, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	linux-iio@...r.kernel.org, bhelgaas@...gle.com
Subject: Re: [PATCH] driver core: platform: Use devres group to free driver
 probe resources

On Fri, May 23, 2025 at 12:09:08AM +0200, Ulf Hansson wrote:
> On Thu, 22 May 2025 at 20:47, Dmitry Torokhov <dmitry.torokhov@...il.com> wrote:
> >
> > On Thu, May 22, 2025 at 06:28:44PM +0200, Ulf Hansson wrote:
> > > On Thu, 22 May 2025 at 16:08, Claudiu Beznea <claudiu.beznea@...on.dev> wrote:
> > > >
> > > > Hi, Ulf,
> > > >
> > > > On 22.05.2025 14:53, Ulf Hansson wrote:
> > > > >
> > > > > That said, I think adding a devm_pm_domain_attach() interface would
> > > > > make perfect sense. Then we can try to replace
> > > > > dev_pm_domain_attach|detach() in bus level code, with just a call to
> > > > > devm_pm_domain_attach(). In this way, we should preserve the
> > > > > expectation for drivers around devres for PM domains. Even if it would
> > > > > change the behaviour for some drivers, it still sounds like the
> > > > > correct thing to do in my opinion.
> > > >
> > > > This looks good to me, as well. I did prototype it on my side and tested on
> > > > all my failure cases and it works.
> > >
> > > That's great! I am happy to help review, if/when you decide to post it.
> >
> > So you are saying you'd be OK with essentially the following (with
> > devm_pm_domain_attach() actually being elsewhere in a real patch and not
> > necessarily mimicked by devm_add_action_or_reset()):
> 
> Correct!
> 
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index cfccf3ff36e7..1e017bfa5caf 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -1376,6 +1376,27 @@ static int platform_uevent(const struct device *dev, struct kobj_uevent_env *env
> >         return 0;
> >  }
> >
> > +
> > +static void platform_pm_domain_detach(void *d)
> > +{
> > +       dev_pm_domain_detach(d, true);
> > +}
> 
> Well, I would not limit this to the platform bus, even if that is the
> most widely used.
> 
> Let's add the new generic interface along with
> dev_pm_domain_attach|detach* and friends instead.
> 
> Then we can convert bus level code (and others), such as the platform
> bus to use it, in a step-by-step approach.

Right, this was only a draft:

"... with devm_pm_domain_attach() actually being elsewhere in a real
patch and not necessarily mimicked by devm_add_action_or_reset() ..."

> 
> > +
> > +static int devm_pm_domain_attach(struct device *dev)
> > +{
> > +       int error;
> > +
> > +       error = dev_pm_domain_attach(dev, true);
> > +       if (error)
> > +               return error;
> > +
> > +       error = devm_add_action_or_reset(dev, platform_pm_domain_detach, dev);
> > +       if (error)
> > +               return error;
> > +
> > +       return 0;
> > +}
> > +
> >  static int platform_probe(struct device *_dev)
> >  {
> >         struct platform_driver *drv = to_platform_driver(_dev->driver);
> > @@ -1396,15 +1417,12 @@ static int platform_probe(struct device *_dev)
> >         if (ret < 0)
> >                 return ret;
> >
> > -       ret = dev_pm_domain_attach(_dev, true);
> > +       ret = devm_pm_domain_attach(_dev);
> >         if (ret)
> >                 goto out;
> >
> > -       if (drv->probe) {
> > +       if (drv->probe)
> >                 ret = drv->probe(dev);
> > -               if (ret)
> > -                       dev_pm_domain_detach(_dev, true);
> > -       }
> >
> >  out:
> >         if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> > @@ -1422,7 +1440,6 @@ static void platform_remove(struct device *_dev)
> >
> >         if (drv->remove)
> >                 drv->remove(dev);
> > -       dev_pm_domain_detach(_dev, true);
> >  }
> >
> >  static void platform_shutdown(struct device *_dev)
> >
> >
> > If so, then OK, it will work for me as well. This achieves the
> > same behavior as with using devres group. The only difference is that if
> > we ever need to extend the platform bus to acquire/release more
> > resources they will also have to use devm API and not the regular one.
> 
> Sounds reasonable to me! Thanks for a nice discussion!
> 
> When it comes to the devm_pm_runtime_enable() API, I think we
> seriously should consider removing it. Let me have a closer look at
> that.

I think once we sort out the power domain detach being out of order with
regard to other devm-managed resources in bus code you need to analyze
this again and you will find out that much as with IRQs, devm API for
runtime PM is useful for majority of cases. Of course there will be
exceptions, but by and large it will cut down on boilerplate code.

Thanks.

-- 
Dmitry