On Fri, 23 May 2025 at 12:52, Claudiu Beznea <claudiu.beznea@xxxxxxxxx> wrote:
>
> Hi, Ulf,
>
> On 23.05.2025 12:47, Ulf Hansson wrote:
> > On Fri, 23 May 2025 at 01:06, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> 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@xxxxxxxxx> 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@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()):
> >>>
> >>> 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?
Well, I like the idea, but I am worried that it may be too much for me
to review. :-)
Although, perhaps I should help Rafael, more officially, to helpt
review code under "POWER MANAGEMENT CORE". Runtime PM is part of it.
Rafael, what do you think?
Kind regards
Uffe
