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

[Lina Iyer <lina.iyer@linaro.org>]

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