| 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
| ||
|
Message-ID: <CAPDyKFpRUhTK=UfcEdRdT0f5EVoGN5okLosd9_tYjdGKr0qvkA@mail.gmail.com>
Date: Fri, 23 May 2025 11:47:42 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
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, 23 May 2025 at 01:06, Dmitry Torokhov <dmitry.torokhov@...il.com> wrote:
>
> 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.
Well, the problem is that the interface is just too difficult to
understand how to use correctly.
A quick look for deployments in drivers confirms my worries.
Kind regards
Uffe
Powered by blists - more mailing lists