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

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

Hi, Ulf,

On 23.05.2025 12:47, Ulf Hansson wrote:
> On Fri, 23 May 2025 at 01:06, Dmitry Torokhov <dmitry.torokhov@gmail.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@gmail.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@tuxon.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.

Maybe we can add something like:

diff --git a/MAINTAINERS b/MAINTAINERS
index 96e64f3d7b47..568a8307863b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10100,6 +10100,7 @@ F:
Documentation/devicetree/bindings/power/power?domain*
 T:     git git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/linux-pm.git
 F:     drivers/pmdomain/
 F:     include/linux/pm_domain.h
+K:      \bpm_runtime_\w+\b

in MAINTAINERS file so that any new patch using the RPM will also be sent
to PM maintainers and checked accordingly?

Thank you,
Claudiu