[RFC PATCH 0/2] Simplified runtime PM for CPU devices?

[RFC PATCH 0/2] Simplified runtime PM for CPU devices?

[Lina Iyer]

Hello,

This is a re-examination of the genpd patches [1] and using runtime PM and
genpd for CPU devices. Discussions following my last submission [1] and many
one on one conversations at LPC and Linaro Connect conferences indicated that
the latency and the unpredictability associated with calling into runtime PM
from the cpuidle path needs to be understood better. I want to summarize some
of those discussions here and questions that stood out during those
discussions. I would like to hear your thoughts on them.

Why runtime PM and genpd, why not a new framework?
	Runtime PM and genpd are established frameworks for power savings in
devices and their domains. As we look beyond cpuidle to save power around the
CPUs, there is the option to invent a new framework for CPU domains or reuse
and extend existing frameworks to suit the specific usecase of CPU devices.
Inventing a new framework has been explored as well; other than adding a bunch
of code to parse and depict the hierarchy, the core code ends up the same as
genpd/runtime PM framework providing reference counting, locks to synchronize
state of the domains, traversing the hierarchy and powering off domains as we
go. The new framework would duplicate a lot of genpd code. CAF downstream tree
takes this approach [2]. It seems to me that the complexity and the feature
set, would bring in the same amount of additional latency to cpuidle
irrespective of the approach. But, at what cost?

Using genpd/runtime PM, risk for cpuidle?
	Using the same frameworks for both device idle and cpuidle would mean
that changes made for supporting a specific relationship of devices and domains
may affect generic cpuidle. This could be a problem, as these changes lie in
the critical path of a CPU operation. A new framework would isolate such risks.
What would be the middle ground of code re-use and stability? Could we stick to
the device struct but isolate some of the code in the CPU critical path from
runtime PM?

What about performance, latency and predictability for RT kernel?
	We did a bit of profiling on the existing patchset [1], and saw some
really a big range of latency 50-80 usec on a 800 Mhz quad core ARM SoC. Upon
further investigation, it became clear that there is quite a bit of
unpredictability as a result of CPUs waiting for locks.

Using runtime PM and genpd framework for CPU PM domain brings out a few
interesting requirements - interrupts are disabled, runtime PM and genpd
locking, spinlocks in -RT.

Locking?
	CPUs can only tell with absolute certainity the PM state of 'their'
devices. Reporting runtime PM state of another CPU would be inaccurate, as CPUs
enter idle independently. So if CPUs can only runtime PM on 'their' devices, do
we need to lock the CPU device?
	Genpd on the other hand, needs locks. It is a shared resource among
CPUs within a cluster, or as dictated in the DT for the SoC. Most likely there
are multiple CPUs sharing the same domain and there is bound be a collision to
acquire locks. This increases latency and unpredictability.
	-RT kernels bring out a different set of requirement, regular spinlocks
may sleep in -RT kernel. Should we use raw_spinlocks for genpd when called from
CPU idle in -RT or for all cases, when the domain is defined as IRQ-safe?

Investigation results and questions:
	While investigating these, I saw an opportunity to prototype a bare
minimum code to solve the problems using runtime PM and genpd. I came with
additional functions specifically for the CPU devices to do runtime PM, that
would be lockless and call into genpd. With that I can see a predictable value
of about additional 9 usec for any CPU to call into runtime PM as part of
cpuidle and about 50 usec for the last CPU in the cluster to lock genpd and do
a NULL callback to platform to suspend the domain. Reference counting of the
last CPU in a domain is done using atomic variables instead of locking and
iterating through all the devices in the domain to determine if they are
runtime suspended. Locks are only used when the CPU is the last non-suspended
device in the domain or the first CPU in the domain to be active.
The proof of concept patches are included herewith. 

It still needs to be seen how do we solve the -RT kernel problem. I am hoping
to hear more about other cases and problems that could be foreseen at this
time.

Thanks,
Lina

[1] https://lwn.net/Articles/656793/
[2] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.18/tree/drivers/cpuidle/lpm_levels.c?h=aosp/android-3.10&id=AU_LINUX_ANDROID_LNX.LA.3.7.2.04.04.04.151.083	

Lina Iyer (2): PM / runtime: Add CPU runtime PM suspend/resume api
  PM / Domains: Atomic counters for domain usage count

 drivers/base/power/domain.c  | 16 ++++++++++--
 drivers/base/power/runtime.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h    |  1 +
 include/linux/pm_runtime.h   |  3 ++-
 4 files changed, 78 insertions(+), 3 deletions(-)

-- 
2.1.4

[RFC PATCH 1/2] PM / runtime: Add CPU runtime PM suspend/resume api

[Lina Iyer]

CPU devices that use runtime PM, have the followign characteristics -
	- Runs in a IRQs disabled context
	- Every CPU does its own runtime PM
	- CPUs do not access other CPU's runtime PM
	- The runtime PM state of the CPU is determined by the CPU

These allow for some interesting optimizations -
	- The CPUs have a limited runtime PM states
	- The runtime state of CPU need not be protected by spinlocks
	- Options like auto-suspend/async are not relevant to CPU
	  devices

A simplified runtime PM would therefore provide all that is needed for
the CPU devices. After making a quick check for the usage count of the
CPU devices (to allow for the CPU to not power down the domain), the
runtime PM could just call the PM callbacks for the CPU devices. Locking
is also avoided.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/base/power/runtime.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_runtime.h   |  3 ++-
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index e1a10a0..5f7512c 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -13,6 +13,7 @@
 #include <linux/pm_wakeirq.h>
 #include <trace/events/rpm.h>
 #include "power.h"
+#include <linux/cpu.h>
 
 typedef int (*pm_callback_t)(struct device *);
 
@@ -577,6 +578,66 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 	goto out;
 }
 
+void cpu_pm_runtime_suspend(void)
+{
+	int ret;
+	int (*callback)(struct device *);
+	struct device *dev = get_cpu_device(smp_processor_id());
+
+	trace_rpm_suspend(dev, 0);
+
+	/**
+	 * Use device usage_count to disallow bubbling up suspend.
+	 * This CPU has already decided to suspend, we cannot
+	 * prevent it here.
+	 */
+	if (!atomic_dec_and_test(&dev->power.usage_count))
+		return 0;
+
+	ret = rpm_check_suspend_allowed(dev);
+	if (ret)
+		return ret;
+
+	__update_runtime_status(dev, RPM_SUSPENDING);
+
+	pm_runtime_cancel_pending(dev);
+	callback = RPM_GET_CALLBACK(dev, runtime_suspend);
+
+	ret = callback(dev);
+	if (!ret)
+		__update_runtime_status(dev, RPM_SUSPENDED);
+	else
+		__update_runtime_status(dev, RPM_ACTIVE);
+
+	trace_rpm_return_int(dev, _THIS_IP_, ret);
+}
+
+void cpu_pm_runtime_resume(void)
+{
+	int ret;
+	int (*callback)(struct device *);
+	struct device *dev = get_cpu_device(smp_processor_id());
+
+	trace_rpm_resume(dev, 0);
+
+	if (dev->power.runtime_status == RPM_ACTIVE)
+		return 1;
+
+	atomic_inc(&dev->power.usage_count);
+
+	__update_runtime_status(dev, RPM_RESUMING);
+
+	callback = RPM_GET_CALLBACK(dev, runtime_resume);
+
+	ret = callback(dev);
+	if (!ret)
+		__update_runtime_status(dev, RPM_ACTIVE);
+	else
+		__update_runtime_status(dev, RPM_SUSPENDED);
+
+	trace_rpm_return_int(dev, _THIS_IP_, ret);
+}
+
 /**
  * rpm_resume - Carry out runtime resume of given device.
  * @dev: Device to resume.
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 3bdbb41..3655ead 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -31,6 +31,8 @@ static inline bool queue_pm_work(struct work_struct *work)
 	return queue_work(pm_wq, work);
 }
 
+extern void cpu_pm_runtime_suspend(void);
+extern void cpu_pm_runtime_resume(void);
 extern int pm_generic_runtime_suspend(struct device *dev);
 extern int pm_generic_runtime_resume(struct device *dev);
 extern int pm_runtime_force_suspend(struct device *dev);
@@ -273,5 +275,4 @@ static inline void pm_runtime_dont_use_autosuspend(struct device *dev)
 {
 	__pm_runtime_use_autosuspend(dev, false);
 }
-
 #endif
-- 
2.1.4

Re: [RFC PATCH 1/2] PM / runtime: Add CPU runtime PM suspend/resume api

[Ulf Hansson]

On 6 October 2015 at 23:57, Lina Iyer <lina.iyer@linaro.org> wrote:
> CPU devices that use runtime PM, have the followign characteristics -
>         - Runs in a IRQs disabled context
>         - Every CPU does its own runtime PM
>         - CPUs do not access other CPU's runtime PM
>         - The runtime PM state of the CPU is determined by the CPU
>
> These allow for some interesting optimizations -
>         - The CPUs have a limited runtime PM states
>         - The runtime state of CPU need not be protected by spinlocks
>         - Options like auto-suspend/async are not relevant to CPU
>           devices
>
> A simplified runtime PM would therefore provide all that is needed for
> the CPU devices. After making a quick check for the usage count of the
> CPU devices (to allow for the CPU to not power down the domain), the
> runtime PM could just call the PM callbacks for the CPU devices. Locking
> is also avoided.

It's an interesting idea. :-)

While I need to give it some more thinking for how/if this could fit
into the runtime PM API, let me start by providing some initial
feedback on the patch as such.

>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  drivers/base/power/runtime.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_runtime.h   |  3 ++-
>  2 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index e1a10a0..5f7512c 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -13,6 +13,7 @@
>  #include <linux/pm_wakeirq.h>
>  #include <trace/events/rpm.h>
>  #include "power.h"
> +#include <linux/cpu.h>
>
>  typedef int (*pm_callback_t)(struct device *);
>
> @@ -577,6 +578,66 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>         goto out;
>  }
>
> +void cpu_pm_runtime_suspend(void)

I think you want to return int instead of void.

> +{
> +       int ret;
> +       int (*callback)(struct device *);
> +       struct device *dev = get_cpu_device(smp_processor_id());

Perhaps we should follow the other runtime PM APIs and have the struct
*device provided as an in-parameter!?

> +
> +       trace_rpm_suspend(dev, 0);
> +
> +       /**
> +        * Use device usage_count to disallow bubbling up suspend.
> +        * This CPU has already decided to suspend, we cannot
> +        * prevent it here.
> +        */
> +       if (!atomic_dec_and_test(&dev->power.usage_count))
> +               return 0;
> +
> +       ret = rpm_check_suspend_allowed(dev);

I don't think you can use this function. For example it calls
__dev_pm_qos_read_value() which expects the dev->power.lock to be
held.

> +       if (ret)
> +               return ret;
> +
> +       __update_runtime_status(dev, RPM_SUSPENDING);
> +
> +       pm_runtime_cancel_pending(dev);

Hmm. For the same struct device (CPU) could really calls to
cpu_pm_runtime_suspend|resume() happen in parallel? Do we need to
protect against that?
I don't have such in-depth knowledge about CPU idle, so apologize if
this may be a stupid question.

If the answer to the above is *no*, I believe you don't need to care
about the intermediate RPM_SUSPENDING state and you don't need an
atomic counter either, right!?
Instead you could then just update the runtime PM status to
RPM_SUSPENDED if the RPM callback doesn't return an error.

> +       callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> +
> +       ret = callback(dev);
> +       if (!ret)
> +               __update_runtime_status(dev, RPM_SUSPENDED);
> +       else
> +               __update_runtime_status(dev, RPM_ACTIVE);
> +
> +       trace_rpm_return_int(dev, _THIS_IP_, ret);
> +}
> +
> +void cpu_pm_runtime_resume(void)

Similar comments as for the suspend function.

> +{
> +       int ret;
> +       int (*callback)(struct device *);
> +       struct device *dev = get_cpu_device(smp_processor_id());
> +
> +       trace_rpm_resume(dev, 0);
> +
> +       if (dev->power.runtime_status == RPM_ACTIVE)
> +               return 1;
> +
> +       atomic_inc(&dev->power.usage_count);
> +
> +       __update_runtime_status(dev, RPM_RESUMING);
> +
> +       callback = RPM_GET_CALLBACK(dev, runtime_resume);
> +
> +       ret = callback(dev);
> +       if (!ret)
> +               __update_runtime_status(dev, RPM_ACTIVE);
> +       else
> +               __update_runtime_status(dev, RPM_SUSPENDED);
> +
> +       trace_rpm_return_int(dev, _THIS_IP_, ret);
> +}
> +
>  /**
>   * rpm_resume - Carry out runtime resume of given device.
>   * @dev: Device to resume.
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 3bdbb41..3655ead 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -31,6 +31,8 @@ static inline bool queue_pm_work(struct work_struct *work)
>         return queue_work(pm_wq, work);
>  }
>
> +extern void cpu_pm_runtime_suspend(void);
> +extern void cpu_pm_runtime_resume(void);

extern int ...

>  extern int pm_generic_runtime_suspend(struct device *dev);
>  extern int pm_generic_runtime_resume(struct device *dev);
>  extern int pm_runtime_force_suspend(struct device *dev);
> @@ -273,5 +275,4 @@ static inline void pm_runtime_dont_use_autosuspend(struct device *dev)
>  {
>         __pm_runtime_use_autosuspend(dev, false);
>  }
> -
>  #endif
> --
> 2.1.4
>

Kind regards
Uffe

Re: [RFC PATCH 1/2] PM / runtime: Add CPU runtime PM suspend/resume api

[Lina Iyer]

On Mon, Oct 19 2015 at 03:44 -0600, Ulf Hansson wrote:
>On 6 October 2015 at 23:57, Lina Iyer <lina.iyer@linaro.org> wrote:
>> CPU devices that use runtime PM, have the followign characteristics -
>>         - Runs in a IRQs disabled context
>>         - Every CPU does its own runtime PM
>>         - CPUs do not access other CPU's runtime PM
>>         - The runtime PM state of the CPU is determined by the CPU
>>
>> These allow for some interesting optimizations -
>>         - The CPUs have a limited runtime PM states
>>         - The runtime state of CPU need not be protected by spinlocks
>>         - Options like auto-suspend/async are not relevant to CPU
>>           devices
>>
>> A simplified runtime PM would therefore provide all that is needed for
>> the CPU devices. After making a quick check for the usage count of the
>> CPU devices (to allow for the CPU to not power down the domain), the
>> runtime PM could just call the PM callbacks for the CPU devices. Locking
>> is also avoided.
>
>It's an interesting idea. :-)
>
>While I need to give it some more thinking for how/if this could fit
>into the runtime PM API, let me start by providing some initial
>feedback on the patch as such.
>
>>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>>  drivers/base/power/runtime.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_runtime.h   |  3 ++-
>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index e1a10a0..5f7512c 100644
>> --- a/drivers/base/power/runtime.c
>> +++ b/drivers/base/power/runtime.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/pm_wakeirq.h>
>>  #include <trace/events/rpm.h>
>>  #include "power.h"
>> +#include <linux/cpu.h>
>>
>>  typedef int (*pm_callback_t)(struct device *);
>>
>> @@ -577,6 +578,66 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>>         goto out;
>>  }
>>
>> +void cpu_pm_runtime_suspend(void)
>
>I think you want to return int instead of void.
>
The outcome of this function would not change the runtime state of the
CPU. The void return seems appropriate.

>> +{
>> +       int ret;
>> +       int (*callback)(struct device *);
>> +       struct device *dev = get_cpu_device(smp_processor_id());
>
>Perhaps we should follow the other runtime PM APIs and have the struct
>*device provided as an in-parameter!?
>
But that information is can be deduced by this function - the function
is called for that CPU from *that* CPU. Also, the absence of an
argument, ensures that the caller won't make a mistake of calling any
other CPUs runtime PM from a CPU or worse, pass a device that is not a
CPU.

>> + +       trace_rpm_suspend(dev, 0);
>> +
>> +       /**
>> +        * Use device usage_count to disallow bubbling up suspend.
>> +        * This CPU has already decided to suspend, we cannot
>> +        * prevent it here.
>> +        */
>> +       if (!atomic_dec_and_test(&dev->power.usage_count))
>> +               return 0;
>> +
>> +       ret = rpm_check_suspend_allowed(dev);
>
>I don't think you can use this function. For example it calls
>__dev_pm_qos_read_value() which expects the dev->power.lock to be
>held.
>
Right. I realized that. Will fix.

>> +       if (ret)
>> +               return ret;
>> +
>> +       __update_runtime_status(dev, RPM_SUSPENDING);
>> +
>> +       pm_runtime_cancel_pending(dev);
>
>Hmm. For the same struct device (CPU) could really calls to
>cpu_pm_runtime_suspend|resume() happen in parallel? Do we need to
>protect against that?
>
That wouldnt happen, the functions are only called that CPU on that CPU.
See the explanation above.

>I don't have such in-depth knowledge about CPU idle, so apologize if
>this may be a stupid question.
>
>If the answer to the above is *no*, I believe you don't need to care
>about the intermediate RPM_SUSPENDING state and you don't need an
>atomic counter either, right!?
>
This calls into genpd framework, which expects devices to be
RPM_SUSPENDING in pm_genpd_power_off; I wanted to keep the behavior
between the frameworks consistent.

>Instead you could then just update the runtime PM status to
>RPM_SUSPENDED if the RPM callback doesn't return an error.
>
>> +       callback = RPM_GET_CALLBACK(dev, runtime_suspend);
>> +
>> +       ret = callback(dev);
>> +       if (!ret)
>> +               __update_runtime_status(dev, RPM_SUSPENDED);
>> +       else
>> +               __update_runtime_status(dev, RPM_ACTIVE);
>> +
>> +       trace_rpm_return_int(dev, _THIS_IP_, ret);
>> +}
>> +
>> +void cpu_pm_runtime_resume(void)
>
>Similar comments as for the suspend function.
>
>> +{
>> +       int ret;
>> +       int (*callback)(struct device *);
>> +       struct device *dev = get_cpu_device(smp_processor_id());
>> +
>> +       trace_rpm_resume(dev, 0);
>> +
>> +       if (dev->power.runtime_status == RPM_ACTIVE)
>> +               return 1;
>> +
>> +       atomic_inc(&dev->power.usage_count);
>> +
>> +       __update_runtime_status(dev, RPM_RESUMING);
>> +
>> +       callback = RPM_GET_CALLBACK(dev, runtime_resume);
>> +
>> +       ret = callback(dev);
>> +       if (!ret)
>> +               __update_runtime_status(dev, RPM_ACTIVE);
>> +       else
>> +               __update_runtime_status(dev, RPM_SUSPENDED);
>> +
>> +       trace_rpm_return_int(dev, _THIS_IP_, ret);
>> +}
>> +
>>  /**
>>   * rpm_resume - Carry out runtime resume of given device.
>>   * @dev: Device to resume.
>> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
>> index 3bdbb41..3655ead 100644
>> --- a/include/linux/pm_runtime.h
>> +++ b/include/linux/pm_runtime.h
>> @@ -31,6 +31,8 @@ static inline bool queue_pm_work(struct work_struct *work)
>>         return queue_work(pm_wq, work);
>>  }
>>
>> +extern void cpu_pm_runtime_suspend(void);
>> +extern void cpu_pm_runtime_resume(void);
>
>extern int ...
>
>>  extern int pm_generic_runtime_suspend(struct device *dev);
>>  extern int pm_generic_runtime_resume(struct device *dev);
>>  extern int pm_runtime_force_suspend(struct device *dev);
>> @@ -273,5 +275,4 @@ static inline void pm_runtime_dont_use_autosuspend(struct device *dev)
>>  {
>>         __pm_runtime_use_autosuspend(dev, false);
>>  }
>> -
>>  #endif
>> --
>> 2.1.4
>>
>
>Kind regards
>Uffe

Re: [RFC PATCH 1/2] PM / runtime: Add CPU runtime PM suspend/resume api

[Ulf Hansson]

On 21 October 2015 at 03:59, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Mon, Oct 19 2015 at 03:44 -0600, Ulf Hansson wrote:
>>
>> On 6 October 2015 at 23:57, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>
>>> CPU devices that use runtime PM, have the followign characteristics -
>>>         - Runs in a IRQs disabled context
>>>         - Every CPU does its own runtime PM
>>>         - CPUs do not access other CPU's runtime PM
>>>         - The runtime PM state of the CPU is determined by the CPU
>>>
>>> These allow for some interesting optimizations -
>>>         - The CPUs have a limited runtime PM states
>>>         - The runtime state of CPU need not be protected by spinlocks
>>>         - Options like auto-suspend/async are not relevant to CPU
>>>           devices
>>>
>>> A simplified runtime PM would therefore provide all that is needed for
>>> the CPU devices. After making a quick check for the usage count of the
>>> CPU devices (to allow for the CPU to not power down the domain), the
>>> runtime PM could just call the PM callbacks for the CPU devices. Locking
>>> is also avoided.
>>
>>
>> It's an interesting idea. :-)
>>
>> While I need to give it some more thinking for how/if this could fit
>> into the runtime PM API, let me start by providing some initial
>> feedback on the patch as such.
>>
>>>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> ---
>>>  drivers/base/power/runtime.c | 61
>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/pm_runtime.h   |  3 ++-
>>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>>> index e1a10a0..5f7512c 100644
>>> --- a/drivers/base/power/runtime.c
>>> +++ b/drivers/base/power/runtime.c
>>> @@ -13,6 +13,7 @@
>>>  #include <linux/pm_wakeirq.h>
>>>  #include <trace/events/rpm.h>
>>>  #include "power.h"
>>> +#include <linux/cpu.h>
>>>
>>>  typedef int (*pm_callback_t)(struct device *);
>>>
>>> @@ -577,6 +578,66 @@ static int rpm_suspend(struct device *dev, int
>>> rpmflags)
>>>         goto out;
>>>  }
>>>
>>> +void cpu_pm_runtime_suspend(void)
>>
>>
>> I think you want to return int instead of void.
>>
> The outcome of this function would not change the runtime state of the
> CPU. The void return seems appropriate.

If the runtime PM suspend callbacks returns and error code, will that
prevent the CPU from going idle?

In other words do you manage idling of the CPU via runtime PM
callbacks for the CPU idle driver?

If not, don't you need to check a return value from this API to know
whether it's okay to proceed idling the CPU?

>
>>> +{
>>> +       int ret;
>>> +       int (*callback)(struct device *);
>>> +       struct device *dev = get_cpu_device(smp_processor_id());
>>
>>
>> Perhaps we should follow the other runtime PM APIs and have the struct
>> *device provided as an in-parameter!?
>>
> But that information is can be deduced by this function - the function
> is called for that CPU from *that* CPU. Also, the absence of an
> argument, ensures that the caller won't make a mistake of calling any
> other CPUs runtime PM from a CPU or worse, pass a device that is not a
> CPU.

Okay! As long as we decide to use the API *only* for CPUs that makes sense.

Although, I was thinking that we perhaps shouldn't limit the use of
the API to CPUs, but I don't know of any similar devices as of now.

>
>>> + +       trace_rpm_suspend(dev, 0);
>>> +
>>> +       /**
>>> +        * Use device usage_count to disallow bubbling up suspend.
>>> +        * This CPU has already decided to suspend, we cannot
>>> +        * prevent it here.
>>> +        */
>>> +       if (!atomic_dec_and_test(&dev->power.usage_count))
>>> +               return 0;
>>> +
>>> +       ret = rpm_check_suspend_allowed(dev);
>>
>>
>> I don't think you can use this function. For example it calls
>> __dev_pm_qos_read_value() which expects the dev->power.lock to be
>> held.
>>
> Right. I realized that. Will fix.
>
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       __update_runtime_status(dev, RPM_SUSPENDING);
>>> +
>>> +       pm_runtime_cancel_pending(dev);
>>
>>
>> Hmm. For the same struct device (CPU) could really calls to
>> cpu_pm_runtime_suspend|resume() happen in parallel? Do we need to
>> protect against that?
>>
> That wouldnt happen, the functions are only called that CPU on that CPU.
> See the explanation above.
>
>> I don't have such in-depth knowledge about CPU idle, so apologize if
>> this may be a stupid question.
>>
>> If the answer to the above is *no*, I believe you don't need to care
>> about the intermediate RPM_SUSPENDING state and you don't need an
>> atomic counter either, right!?
>>
> This calls into genpd framework, which expects devices to be
> RPM_SUSPENDING in pm_genpd_power_off; I wanted to keep the behavior
> between the frameworks consistent.

Okay, it makes sense. Thanks for clarifying.

>
>
>> Instead you could then just update the runtime PM status to
>> RPM_SUSPENDED if the RPM callback doesn't return an error.
>>
>>> +       callback = RPM_GET_CALLBACK(dev, runtime_suspend);
>>> +
>>> +       ret = callback(dev);
>>> +       if (!ret)
>>> +               __update_runtime_status(dev, RPM_SUSPENDED);
>>> +       else
>>> +               __update_runtime_status(dev, RPM_ACTIVE);
>>> +
>>> +       trace_rpm_return_int(dev, _THIS_IP_, ret);
>>> +}
>>> +
>>> +void cpu_pm_runtime_resume(void)
>>

[...]

Kind regards
Uffe

Re: [RFC PATCH 1/2] PM / runtime: Add CPU runtime PM suspend/resume api

[Lina Iyer]

On Wed, Oct 28 2015 at 04:43 -0600, Ulf Hansson wrote:
>On 21 October 2015 at 03:59, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Mon, Oct 19 2015 at 03:44 -0600, Ulf Hansson wrote:
>>>
>>> On 6 October 2015 at 23:57, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>>
>>>> CPU devices that use runtime PM, have the followign characteristics -
>>>>         - Runs in a IRQs disabled context
>>>>         - Every CPU does its own runtime PM
>>>>         - CPUs do not access other CPU's runtime PM
>>>>         - The runtime PM state of the CPU is determined by the CPU
>>>>
>>>> These allow for some interesting optimizations -
>>>>         - The CPUs have a limited runtime PM states
>>>>         - The runtime state of CPU need not be protected by spinlocks
>>>>         - Options like auto-suspend/async are not relevant to CPU
>>>>           devices
>>>>
>>>> A simplified runtime PM would therefore provide all that is needed for
>>>> the CPU devices. After making a quick check for the usage count of the
>>>> CPU devices (to allow for the CPU to not power down the domain), the
>>>> runtime PM could just call the PM callbacks for the CPU devices. Locking
>>>> is also avoided.
>>>
>>>
>>> It's an interesting idea. :-)
>>>
>>> While I need to give it some more thinking for how/if this could fit
>>> into the runtime PM API, let me start by providing some initial
>>> feedback on the patch as such.
>>>
>>>>
>>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>>> ---
>>>>  drivers/base/power/runtime.c | 61
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/pm_runtime.h   |  3 ++-
>>>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>>>> index e1a10a0..5f7512c 100644
>>>> --- a/drivers/base/power/runtime.c
>>>> +++ b/drivers/base/power/runtime.c
>>>> @@ -13,6 +13,7 @@
>>>>  #include <linux/pm_wakeirq.h>
>>>>  #include <trace/events/rpm.h>
>>>>  #include "power.h"
>>>> +#include <linux/cpu.h>
>>>>
>>>>  typedef int (*pm_callback_t)(struct device *);
>>>>
>>>> @@ -577,6 +578,66 @@ static int rpm_suspend(struct device *dev, int
>>>> rpmflags)
>>>>         goto out;
>>>>  }
>>>>
>>>> +void cpu_pm_runtime_suspend(void)
>>>
>>>
>>> I think you want to return int instead of void.
>>>
>> The outcome of this function would not change the runtime state of the
>> CPU. The void return seems appropriate.
>
>If the runtime PM suspend callbacks returns and error code, will that
>prevent the CPU from going idle?
>
It should not. I dont think runtime PM should fail, because the CPU
determines its own state.

>In other words do you manage idling of the CPU via runtime PM
>callbacks for the CPU idle driver?
>
No.

>If not, don't you need to check a return value from this API to know
>whether it's okay to proceed idling the CPU?
>
>>
>>>> +{
>>>> +       int ret;
>>>> +       int (*callback)(struct device *);
>>>> +       struct device *dev = get_cpu_device(smp_processor_id());
>>>
>>>
>>> Perhaps we should follow the other runtime PM APIs and have the struct
>>> *device provided as an in-parameter!?
>>>
>> But that information is can be deduced by this function - the function
>> is called for that CPU from *that* CPU. Also, the absence of an
>> argument, ensures that the caller won't make a mistake of calling any
>> other CPUs runtime PM from a CPU or worse, pass a device that is not a
>> CPU.
>
>Okay! As long as we decide to use the API *only* for CPUs that makes sense.
>
>Although, I was thinking that we perhaps shouldn't limit the use of
>the API to CPUs, but I don't know of any similar devices as of now.
>
>>
>>>> + +       trace_rpm_suspend(dev, 0);
>>>> +
>>>> +       /**
>>>> +        * Use device usage_count to disallow bubbling up suspend.
>>>> +        * This CPU has already decided to suspend, we cannot
>>>> +        * prevent it here.
>>>> +        */
>>>> +       if (!atomic_dec_and_test(&dev->power.usage_count))
>>>> +               return 0;
>>>> +
>>>> +       ret = rpm_check_suspend_allowed(dev);
>>>
>>>
>>> I don't think you can use this function. For example it calls
>>> __dev_pm_qos_read_value() which expects the dev->power.lock to be
>>> held.
>>>
>> Right. I realized that. Will fix.
>>
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>> +       __update_runtime_status(dev, RPM_SUSPENDING);
>>>> +
>>>> +       pm_runtime_cancel_pending(dev);
>>>
>>>
>>> Hmm. For the same struct device (CPU) could really calls to
>>> cpu_pm_runtime_suspend|resume() happen in parallel? Do we need to
>>> protect against that?
>>>
>> That wouldnt happen, the functions are only called that CPU on that CPU.
>> See the explanation above.
>>
>>> I don't have such in-depth knowledge about CPU idle, so apologize if
>>> this may be a stupid question.
>>>
>>> If the answer to the above is *no*, I believe you don't need to care
>>> about the intermediate RPM_SUSPENDING state and you don't need an
>>> atomic counter either, right!?
>>>
>> This calls into genpd framework, which expects devices to be
>> RPM_SUSPENDING in pm_genpd_power_off; I wanted to keep the behavior
>> between the frameworks consistent.
>
>Okay, it makes sense. Thanks for clarifying.
>
>>
>>
>>> Instead you could then just update the runtime PM status to
>>> RPM_SUSPENDED if the RPM callback doesn't return an error.
>>>
>>>> +       callback = RPM_GET_CALLBACK(dev, runtime_suspend);
>>>> +
>>>> +       ret = callback(dev);
>>>> +       if (!ret)
>>>> +               __update_runtime_status(dev, RPM_SUSPENDED);
>>>> +       else
>>>> +               __update_runtime_status(dev, RPM_ACTIVE);
>>>> +
>>>> +       trace_rpm_return_int(dev, _THIS_IP_, ret);
>>>> +}
>>>> +
>>>> +void cpu_pm_runtime_resume(void)
>>>
>
>[...]
>
>Kind regards
>Uffe

Re: [RFC PATCH 1/2] PM / runtime: Add CPU runtime PM suspend/resume api

[Kevin Hilman]

Lina Iyer <lina.iyer@linaro.org> writes:

> CPU devices that use runtime PM, have the followign characteristics -
> 	- Runs in a IRQs disabled context
> 	- Every CPU does its own runtime PM
> 	- CPUs do not access other CPU's runtime PM
> 	- The runtime PM state of the CPU is determined by the CPU
>
> These allow for some interesting optimizations -
> 	- The CPUs have a limited runtime PM states
> 	- The runtime state of CPU need not be protected by spinlocks
> 	- Options like auto-suspend/async are not relevant to CPU
> 	  devices
>
> A simplified runtime PM would therefore provide all that is needed for
> the CPU devices. 

I like the idea of optimizing things for CPUs.  I've assumed we would
eventually run into latency issues when using runtime PM and genpd on
CPUs, but I guess we're already there.

> After making a quick check for the usage count of the
> CPU devices (to allow for the CPU to not power down the domain), the
> runtime PM could just call the PM callbacks for the CPU devices. Locking
> is also avoided.

This part is confusing (or more accurately, I am confused) more on that below...

> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  drivers/base/power/runtime.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_runtime.h   |  3 ++-
>  2 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index e1a10a0..5f7512c 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -13,6 +13,7 @@
>  #include <linux/pm_wakeirq.h>
>  #include <trace/events/rpm.h>
>  #include "power.h"
> +#include <linux/cpu.h>
>  
>  typedef int (*pm_callback_t)(struct device *);
>  
> @@ -577,6 +578,66 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>  	goto out;
>  }
>  
> +void cpu_pm_runtime_suspend(void)
> +{
> +	int ret;
> +	int (*callback)(struct device *);
> +	struct device *dev = get_cpu_device(smp_processor_id());
> +
> +	trace_rpm_suspend(dev, 0);
> +
> +	/**

nit: the double '*' indicates kerneldoc, but this is just a multi-line
comment.

> +	 * Use device usage_count to disallow bubbling up suspend.

I don't understand this comment.

> +	 * This CPU has already decided to suspend, we cannot
> +	 * prevent it here.
> +	 */
> +	if (!atomic_dec_and_test(&dev->power.usage_count))

Isn't this basically a _put_noidle() ?

> +		return 0;
> +
> +	ret = rpm_check_suspend_allowed(dev);
> +	if (ret)
> +		return ret;
> +
> +	__update_runtime_status(dev, RPM_SUSPENDING);
> +
> +	pm_runtime_cancel_pending(dev);
> +	callback = RPM_GET_CALLBACK(dev, runtime_suspend);

If the CPU device is part of a domain (e.g. cluster), then 'callback'
here will be the domain callback, right?

If that's true, I'm not sure I'm following the changelog description
that talks about avoiding the calling into the domain.

It seems to me that you'll still call into the domain, but patch 2/2
optimizes that path by only doing the *real* work of the domain for the
last man standing.  Am I understanding that correctly?

Hmm, if that's the case though, where would the callbacks associated with the
CPU (e.g. current CPU PM notifier stuff) get called?

> +	ret = callback(dev);
> +	if (!ret)
> +		__update_runtime_status(dev, RPM_SUSPENDED);
> +	else
> +		__update_runtime_status(dev, RPM_ACTIVE);
> +
> +	trace_rpm_return_int(dev, _THIS_IP_, ret);
> +}

Kevin

Re: [RFC PATCH 1/2] PM / runtime: Add CPU runtime PM suspend/resume api

[Lina Iyer]

On Fri, Oct 23 2015 at 15:19 -0600, Kevin Hilman wrote:
>Lina Iyer <lina.iyer@linaro.org> writes:
>
>> CPU devices that use runtime PM, have the followign characteristics -
>> 	- Runs in a IRQs disabled context
>> 	- Every CPU does its own runtime PM
>> 	- CPUs do not access other CPU's runtime PM
>> 	- The runtime PM state of the CPU is determined by the CPU
>>
>> These allow for some interesting optimizations -
>> 	- The CPUs have a limited runtime PM states
>> 	- The runtime state of CPU need not be protected by spinlocks
>> 	- Options like auto-suspend/async are not relevant to CPU
>> 	  devices
>>
>> A simplified runtime PM would therefore provide all that is needed for
>> the CPU devices.
>
>I like the idea of optimizing things for CPUs.  I've assumed we would
>eventually run into latency issues when using runtime PM and genpd on
>CPUs, but I guess we're already there.
>
>> After making a quick check for the usage count of the
>> CPU devices (to allow for the CPU to not power down the domain), the
>> runtime PM could just call the PM callbacks for the CPU devices. Locking
>> is also avoided.
>
>This part is confusing (or more accurately, I am confused) more on that below...
>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>>  drivers/base/power/runtime.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_runtime.h   |  3 ++-
>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index e1a10a0..5f7512c 100644
>> --- a/drivers/base/power/runtime.c
>> +++ b/drivers/base/power/runtime.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/pm_wakeirq.h>
>>  #include <trace/events/rpm.h>
>>  #include "power.h"
>> +#include <linux/cpu.h>
>>
>>  typedef int (*pm_callback_t)(struct device *);
>>
>> @@ -577,6 +578,66 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>>  	goto out;
>>  }
>>
>> +void cpu_pm_runtime_suspend(void)
>> +{
>> +	int ret;
>> +	int (*callback)(struct device *);
>> +	struct device *dev = get_cpu_device(smp_processor_id());
>> +
>> +	trace_rpm_suspend(dev, 0);
>> +
>> +	/**
>
>nit: the double '*' indicates kerneldoc, but this is just a multi-line
>comment.
>
>> +	 * Use device usage_count to disallow bubbling up suspend.
>
>I don't understand this comment.
>
>> +	 * This CPU has already decided to suspend, we cannot
>> +	 * prevent it here.
>> +	 */
>> +	if (!atomic_dec_and_test(&dev->power.usage_count))
>
>Isn't this basically a _put_noidle() ?
>
>> +		return 0;
>> +
>> +	ret = rpm_check_suspend_allowed(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	__update_runtime_status(dev, RPM_SUSPENDING);
>> +
>> +	pm_runtime_cancel_pending(dev);
>> +	callback = RPM_GET_CALLBACK(dev, runtime_suspend);
>
>If the CPU device is part of a domain (e.g. cluster), then 'callback'
>here will be the domain callback, right?
>
Yes, thats correct.

>If that's true, I'm not sure I'm following the changelog description
>that talks about avoiding the calling into the domain.
>
Partly correct. Avoid calling into the domain if its not the last
device.

>It seems to me that you'll still call into the domain, but patch 2/2
>optimizes that path by only doing the *real* work of the domain for the
>last man standing.  Am I understanding that correctly?
>
Yes

>Hmm, if that's the case though, where would the callbacks associated with the
>CPU (e.g. current CPU PM notifier stuff) get called?
>
They are called from cpuidle driver as it is today.

Thanks,
Lina

>> +	ret = callback(dev);
>> +	if (!ret)
>> +		__update_runtime_status(dev, RPM_SUSPENDED);
>> +	else
>> +		__update_runtime_status(dev, RPM_ACTIVE);
>> +
>> +	trace_rpm_return_int(dev, _THIS_IP_, ret);
>> +}
>
>Kevin

Re: [RFC PATCH 1/2] PM / runtime: Add CPU runtime PM suspend/resume api

[Kevin Hilman]

Lina Iyer <lina.iyer@linaro.org> writes:

> On Fri, Oct 23 2015 at 15:19 -0600, Kevin Hilman wrote:
>>Lina Iyer <lina.iyer@linaro.org> writes:
>>
>>> CPU devices that use runtime PM, have the followign characteristics -
>>> 	- Runs in a IRQs disabled context
>>> 	- Every CPU does its own runtime PM
>>> 	- CPUs do not access other CPU's runtime PM
>>> 	- The runtime PM state of the CPU is determined by the CPU
>>>
>>> These allow for some interesting optimizations -
>>> 	- The CPUs have a limited runtime PM states
>>> 	- The runtime state of CPU need not be protected by spinlocks
>>> 	- Options like auto-suspend/async are not relevant to CPU
>>> 	  devices
>>>
>>> A simplified runtime PM would therefore provide all that is needed for
>>> the CPU devices.
>>
>>I like the idea of optimizing things for CPUs.  I've assumed we would
>>eventually run into latency issues when using runtime PM and genpd on
>>CPUs, but I guess we're already there.
>>
>>> After making a quick check for the usage count of the
>>> CPU devices (to allow for the CPU to not power down the domain), the
>>> runtime PM could just call the PM callbacks for the CPU devices. Locking
>>> is also avoided.
>>
>>This part is confusing (or more accurately, I am confused) more on that below...
>>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> ---
>>>  drivers/base/power/runtime.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/pm_runtime.h   |  3 ++-
>>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>>> index e1a10a0..5f7512c 100644
>>> --- a/drivers/base/power/runtime.c
>>> +++ b/drivers/base/power/runtime.c
>>> @@ -13,6 +13,7 @@
>>>  #include <linux/pm_wakeirq.h>
>>>  #include <trace/events/rpm.h>
>>>  #include "power.h"
>>> +#include <linux/cpu.h>
>>>
>>>  typedef int (*pm_callback_t)(struct device *);
>>>
>>> @@ -577,6 +578,66 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>>>  	goto out;
>>>  }
>>>
>>> +void cpu_pm_runtime_suspend(void)
>>> +{
>>> +	int ret;
>>> +	int (*callback)(struct device *);
>>> +	struct device *dev = get_cpu_device(smp_processor_id());
>>> +
>>> +	trace_rpm_suspend(dev, 0);
>>> +
>>> +	/**
>>
>>nit: the double '*' indicates kerneldoc, but this is just a multi-line
>>comment.
>>
>>> +	 * Use device usage_count to disallow bubbling up suspend.
>>
>>I don't understand this comment.
>>
>>> +	 * This CPU has already decided to suspend, we cannot
>>> +	 * prevent it here.
>>> +	 */
>>> +	if (!atomic_dec_and_test(&dev->power.usage_count))
>>
>>Isn't this basically a _put_noidle() ?
>>
>>> +		return 0;
>>> +
>>> +	ret = rpm_check_suspend_allowed(dev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	__update_runtime_status(dev, RPM_SUSPENDING);
>>> +
>>> +	pm_runtime_cancel_pending(dev);
>>> +	callback = RPM_GET_CALLBACK(dev, runtime_suspend);
>>
>>If the CPU device is part of a domain (e.g. cluster), then 'callback'
>>here will be the domain callback, right?
>>
> Yes, thats correct.
>
>>If that's true, I'm not sure I'm following the changelog description
>>that talks about avoiding the calling into the domain.
>>
> Partly correct. Avoid calling into the domain if its not the last
> device.
>
>>It seems to me that you'll still call into the domain, but patch 2/2
>>optimizes that path by only doing the *real* work of the domain for the
>>last man standing.  Am I understanding that correctly?
>>
> Yes
>
>>Hmm, if that's the case though, where would the callbacks associated with the
>>CPU (e.g. current CPU PM notifier stuff) get called?
>>
>
> They are called from cpuidle driver as it is today.
>

And if the CPU _PM notifiers are eventually converted into runtime PM
callbacks, they would then be called by the domain callbacks, but
wouldn't that mean they would only be called after the last man
standing?

Kevin

Re: [RFC PATCH 1/2] PM / runtime: Add CPU runtime PM suspend/resume api

[Lina Iyer]

On Fri, Oct 23 2015 at 17:46 -0600, Kevin Hilman wrote:
>Lina Iyer <lina.iyer@linaro.org> writes:
>
>> On Fri, Oct 23 2015 at 15:19 -0600, Kevin Hilman wrote:
>>>Lina Iyer <lina.iyer@linaro.org> writes:
>>>
>>>> CPU devices that use runtime PM, have the followign characteristics -
>>>> 	- Runs in a IRQs disabled context
>>>> 	- Every CPU does its own runtime PM
>>>> 	- CPUs do not access other CPU's runtime PM
>>>> 	- The runtime PM state of the CPU is determined by the CPU
>>>>
>>>> These allow for some interesting optimizations -
>>>> 	- The CPUs have a limited runtime PM states
>>>> 	- The runtime state of CPU need not be protected by spinlocks
>>>> 	- Options like auto-suspend/async are not relevant to CPU
>>>> 	  devices
>>>>
>>>> A simplified runtime PM would therefore provide all that is needed for
>>>> the CPU devices.
>>>
>>>I like the idea of optimizing things for CPUs.  I've assumed we would
>>>eventually run into latency issues when using runtime PM and genpd on
>>>CPUs, but I guess we're already there.
>>>
>>>> After making a quick check for the usage count of the
>>>> CPU devices (to allow for the CPU to not power down the domain), the
>>>> runtime PM could just call the PM callbacks for the CPU devices. Locking
>>>> is also avoided.
>>>
>>>This part is confusing (or more accurately, I am confused) more on that below...
>>>
>>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>>> ---
>>>>  drivers/base/power/runtime.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/pm_runtime.h   |  3 ++-
>>>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>>>> index e1a10a0..5f7512c 100644
>>>> --- a/drivers/base/power/runtime.c
>>>> +++ b/drivers/base/power/runtime.c
>>>> @@ -13,6 +13,7 @@
>>>>  #include <linux/pm_wakeirq.h>
>>>>  #include <trace/events/rpm.h>
>>>>  #include "power.h"
>>>> +#include <linux/cpu.h>
>>>>
>>>>  typedef int (*pm_callback_t)(struct device *);
>>>>
>>>> @@ -577,6 +578,66 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>>>>  	goto out;
>>>>  }
>>>>
>>>> +void cpu_pm_runtime_suspend(void)
>>>> +{
>>>> +	int ret;
>>>> +	int (*callback)(struct device *);
>>>> +	struct device *dev = get_cpu_device(smp_processor_id());
>>>> +
>>>> +	trace_rpm_suspend(dev, 0);
>>>> +
>>>> +	/**
>>>
>>>nit: the double '*' indicates kerneldoc, but this is just a multi-line
>>>comment.
>>>
>>>> +	 * Use device usage_count to disallow bubbling up suspend.
>>>
>>>I don't understand this comment.
>>>
>>>> +	 * This CPU has already decided to suspend, we cannot
>>>> +	 * prevent it here.
>>>> +	 */
>>>> +	if (!atomic_dec_and_test(&dev->power.usage_count))
>>>
>>>Isn't this basically a _put_noidle() ?
>>>
>>>> +		return 0;
>>>> +
>>>> +	ret = rpm_check_suspend_allowed(dev);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	__update_runtime_status(dev, RPM_SUSPENDING);
>>>> +
>>>> +	pm_runtime_cancel_pending(dev);
>>>> +	callback = RPM_GET_CALLBACK(dev, runtime_suspend);
>>>
>>>If the CPU device is part of a domain (e.g. cluster), then 'callback'
>>>here will be the domain callback, right?
>>>
>> Yes, thats correct.
>>
>>>If that's true, I'm not sure I'm following the changelog description
>>>that talks about avoiding the calling into the domain.
>>>
>> Partly correct. Avoid calling into the domain if its not the last
>> device.
>>
>>>It seems to me that you'll still call into the domain, but patch 2/2
>>>optimizes that path by only doing the *real* work of the domain for the
>>>last man standing.  Am I understanding that correctly?
>>>
>> Yes
>>
>>>Hmm, if that's the case though, where would the callbacks associated with the
>>>CPU (e.g. current CPU PM notifier stuff) get called?
>>>
>>
>> They are called from cpuidle driver as it is today.
>>
>
>And if the CPU _PM notifiers are eventually converted into runtime PM
>callbacks, they would then be called by the domain callbacks, but
>wouldn't that mean they would only be called after the last man
>standing?
>
These runtime PM functions are called from every CPU that is going idle,
and should therefore be able to execute callbacks for _PM notifications
for all CPUs from runtime PM.

The genpd callbacks are however only for the last man standing.

Thanks,
LIna

[RFC PATCH 2/2] PM / Domains: Atomic counters for domain usage count

[Lina Iyer]

Locking a domain to check if the domain can be powered on/off is an
expensive operations and could hold up multiple devices executing
runtime PM at the same time.

In the case where there is atleast one active device, the domain would
remain active. This can be easily checked by using an atomic counter to
record domain usage. This restricts locking only to the last suspending
or the first resuming device.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/base/power/domain.c | 16 ++++++++++++++--
 include/linux/pm_domain.h   |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 3af1a63..80f8ea9 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -204,6 +204,7 @@ static bool genpd_sd_counter_dec(struct generic_pm_domain *genpd)
 
 	if (!WARN_ON(atomic_read(&genpd->sd_count) == 0))
 		ret = !!atomic_dec_and_test(&genpd->sd_count);
+	atomic_dec(&genpd->usage_count);
 
 	return ret;
 }
@@ -211,6 +212,7 @@ static bool genpd_sd_counter_dec(struct generic_pm_domain *genpd)
 static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
 {
 	atomic_inc(&genpd->sd_count);
+	atomic_inc(&genpd->usage_count);
 	smp_mb__after_atomic();
 }
 
@@ -583,6 +585,9 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 		return ret;
 	}
 
+	if (!atomic_dec_and_test(&genpd->usage_count))
+		return 0;
+
 	/*
 	 * If power.irq_safe is set, this routine may be run with
 	 * IRQ disabled, so suspend only if the power domain is
@@ -620,6 +625,9 @@ static int pm_genpd_runtime_resume(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
+	if (atomic_inc_return(&genpd->usage_count) > 1)
+		goto out;
+
 	/*
 	 * As we dont power off a non IRQ safe domain, which holds
 	 * an IRQ safe device, we dont need to restore power to it.
@@ -1400,9 +1408,11 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 
 	if (ret)
 		genpd_free_dev_data(dev, gpd_data);
-	else
+	else {
 		dev_pm_qos_add_notifier(dev, &gpd_data->nb);
-
+		atomic_inc(&genpd->usage_count);
+		printk("Add device %d\n", atomic_read(&genpd->usage_count));
+	}
 	return ret;
 }
 
@@ -1457,6 +1467,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	genpd_unlock(genpd);
 
+	atomic_dec(&genpd->usage_count);
 	genpd_free_dev_data(dev, gpd_data);
 
 	return 0;
@@ -1799,6 +1810,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	atomic_set(&genpd->sd_count, 0);
+	atomic_set(&genpd->usage_count, 0);
 	genpd->status = is_off ? GPD_STATE_POWER_OFF : GPD_STATE_ACTIVE;
 	genpd->device_count = 0;
 	genpd->max_off_time_ns = -1;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 613f7a5..7e52923 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -55,6 +55,7 @@ struct generic_pm_domain {
 	struct work_struct power_off_work;
 	const char *name;
 	atomic_t sd_count;	/* Number of subdomains with power "on" */
+	atomic_t usage_count;	/* Number of active users of domain "on" */
 	enum gpd_status status;	/* Current state of the domain */
 	unsigned int device_count;	/* Number of devices */
 	unsigned int suspended_count;	/* System suspend device counter */
-- 
2.1.4