linux-kernel - Re: [PATCH 9/9] PM / Domains: Add dev_pm_domain_attach_by

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]
Date:   Fri, 25 May 2018 12:45:51 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Jon Hunter <jonathanh@...dia.com>
Cc:     "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Linux PM <linux-pm@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Todor Tomov <todor.tomov@...aro.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Kevin Hilman <khilman@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-tegra@...r.kernel.org
Subject: Re: [PATCH 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to
 manage multi PM domains

On 25 May 2018 at 10:31, Jon Hunter <jonathanh@...dia.com> wrote:
>
> On 24/05/18 22:11, Ulf Hansson wrote:
>>
>> On 24 May 2018 at 17:48, Jon Hunter <jonathanh@...dia.com> wrote:
>>>
>>>
>>> On 18/05/18 11:31, Ulf Hansson wrote:
>>>>
>>>>
>>>> The existing dev_pm_domain_attach() function, allows a single PM domain
>>>> to
>>>> be attached per device. To be able to support devices that are
>>>> partitioned
>>>> across multiple PM domains, let's introduce a new interface,
>>>> dev_pm_domain_attach_by_id().
>>>>
>>>> The dev_pm_domain_attach_by_id() returns a new allocated struct device
>>>> with
>>>> the corresponding attached PM domain. This enables for example a driver
>>>> to
>>>> operate on the new device from a power management point of view. The
>>>> driver
>>>> may then also benefit from using the received device, to set up so
>>>> called
>>>> device-links towards its original device. Depending on the situation,
>>>> these
>>>> links may then be dynamically changed.
>>>>
>>>> The new interface is typically called by drivers during their probe
>>>> phase,
>>>> in case they manages devices which uses multiple PM domains. If that is
>>>> the
>>>> case, the driver also becomes responsible of managing the detaching of
>>>> the
>>>> PM domains, which typically should be done at the remove phase.
>>>> Detaching
>>>> is done by calling the existing dev_pm_domain_detach() function and for
>>>> each of the received devices from dev_pm_domain_attach_by_id().
>>>>
>>>> Note, currently its only genpd that supports multiple PM domains per
>>>> device, but dev_pm_domain_attach_by_id() can easily by extended to cover
>>>> other PM domain types, if/when needed.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>
>>>> ---
>>>>    drivers/base/power/common.c | 33 ++++++++++++++++++++++++++++++++-
>>>>    include/linux/pm_domain.h   |  7 +++++++
>>>>    2 files changed, 39 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>>>> index 7ae62b6..d3db974 100644
>>>> --- a/drivers/base/power/common.c
>>>> +++ b/drivers/base/power/common.c
>>>> @@ -117,13 +117,44 @@ int dev_pm_domain_attach(struct device *dev, bool
>>>> power_on)
>>>>    EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>>>>      /**
>>>> + * dev_pm_domain_attach_by_id - Attach a device to one of its PM
>>>> domains.
>>>
>>>
>>>
>>> Isn't this more of a 'get'?
>>
>>
>> I don't think so, at least according to the common understanding of
>> how we use get and put APIs.
>>
>> For example, clk_get() returns a cookie to a clk, which you then can
>> do a hole bunch of different clk specific operations on.
>>
>> This is different, there are no specific PM domain operations the
>> caller can or should do. Instead the idea is to keep drivers more or
>> less transparent, still using runtime PM as before. The only care the
>> caller need to take is to use device links, which BTW isn't a PM
>> domain specific thing.
>
>
> Yes, but a user can still call pm_runtime_get/put with the device returned
> if they wish to, right?

Correct.

[...]

>>>> + */
>>>> +struct device *dev_pm_domain_attach_by_id(struct device *dev,
>>>> +                                         unsigned int index)
>>>> +{
>>>> +       if (dev->pm_domain)
>>>
>>>
>>>
>>> I wonder if this is worthy of a ...
>>>
>>>          if (WARN_ON(dev->pm_domain))
>>>
>>>> +               return NULL;
>>>
>>>
>>>
>>> Don't we consider this an error case? I wonder why not return PTR_ERR
>>> here
>>> as well? This would be consistent with dev_pm_domain_attach().
>>
>>
>> Please see above comment.
>
>
> Right, but this case still seems like an error. My understanding is that
> only drivers will use this API directly and it will not be used by the
> device driver core (unlike dev_pm_domain_attach), so if anyone calls this
> attempting to attach another PM domain when one is already attached, they
> are doing something wrong.

[...]

You may be right!

What I was thinking of is whether multiple PM domains may be optional
in some cases, but instead a PM domain have already been attached by
dev_pm_domain_attach(), prior the driver starts to probe.

Then, assuming we return an error for this case, that means the caller
then need to check the dev->pm_domain pointer, prior calling
dev_pm_domain_attach_by_id(). Wouldn't it? Perhaps that is more clear
though?

Kind regards
Uffe