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

[Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>]

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@xxxxxxxxx> 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()):

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);
+}
+
+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.

Thanks.

-- 
Dmitry