[v2,2/5] cpu_pm: call notifiers during suspend

Commit Message

Santosh Shilimkar Sept. 3, 2011, 2:39 p.m. UTC

From: Colin Cross <[e-mail]>

Implements syscore_ops in cpu_pm to call the cpu and
cpu cluster notifiers during suspend and resume,
allowing drivers receiving the notifications to
avoid implementing syscore_ops.

Signed-off-by: Colin Cross <[e-mail]>
[[e-mail]: Rebased against 3.1-rc4]
Signed-off-by: Santosh Shilimkar <[e-mail]>
---
 kernel/cpu_pm.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

Comments

Kevin Hilman Sept. 7, 2011, 8:02 p.m. UTC | #1

Santosh Shilimkar <[e-mail]> writes:

> From: Colin Cross <[e-mail]>
>
> Implements syscore_ops in cpu_pm to call the cpu and
> cpu cluster notifiers during suspend and resume,
> allowing drivers receiving the notifications to
> avoid implementing syscore_ops.
>
> Signed-off-by: Colin Cross <[e-mail]>
> [[e-mail]: Rebased against 3.1-rc4]
> Signed-off-by: Santosh Shilimkar <[e-mail]>

I don't think using syscore_ops is right here.  The platform code should
decide where in its own suspend path the notifiers should be triggered.

The reason is because while the syscore_ops run late in the suspend
path, they still run before some platform-specific decisions about the
low-power states are made.  That means that any notifiers that need to
use information about the target low-power state (e.g. whether context
will be lost or not) cannot do so since that information has not yet
been decided until the platform_suspend_ops->enter() runs.

Basically, I think the cpu_*_pm_enter() calls should be called by
platform-specific code, not by common code.

Kevin

> ---
>  kernel/cpu_pm.c |   33 +++++++++++++++++++++++++++++++++
>  1 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
> index 54aa892..3d115d0 100644
> --- a/kernel/cpu_pm.c
> +++ b/kernel/cpu_pm.c
> @@ -20,6 +20,7 @@
>  #include <linux/module.h>
>  #include <linux/notifier.h>
>  #include <linux/spinlock.h>
> +#include <linux/syscore_ops.h>
>  
>  static DEFINE_RWLOCK(cpu_pm_notifier_lock);
>  static RAW_NOTIFIER_HEAD(cpu_pm_notifier_chain);
> @@ -113,3 +114,35 @@ int cpu_cluster_pm_exit(void)
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(cpu_cluster_pm_exit);
> +
> +#ifdef CONFIG_PM
> +static int cpu_pm_suspend(void)
> +{
> +	int ret;
> +
> +	ret = cpu_pm_enter();
> +	if (ret)
> +		return ret;
> +
> +	ret = cpu_cluster_pm_enter();
> +	return ret;
> +}
> +
> +static void cpu_pm_resume(void)
> +{
> +	cpu_cluster_pm_exit();
> +	cpu_pm_exit();
> +}
> +
> +static struct syscore_ops cpu_pm_syscore_ops = {
> +	.suspend = cpu_pm_suspend,
> +	.resume = cpu_pm_resume,
> +};
> +
> +static int cpu_pm_init(void)
> +{
> +	register_syscore_ops(&cpu_pm_syscore_ops);
> +	return 0;
> +}
> +core_initcall(cpu_pm_init);
> +#endif

Santosh Shilimkar Sept. 8, 2011, 5:16 a.m. UTC | #2

On Thursday 08 September 2011 01:32 AM, Kevin Hilman wrote:
> Santosh Shilimkar<[e-mail]>  writes:
>
>> From: Colin Cross<[e-mail]>
>>
>> Implements syscore_ops in cpu_pm to call the cpu and
>> cpu cluster notifiers during suspend and resume,
>> allowing drivers receiving the notifications to
>> avoid implementing syscore_ops.
>>
>> Signed-off-by: Colin Cross<[e-mail]>
>> [[e-mail]: Rebased against 3.1-rc4]
>> Signed-off-by: Santosh Shilimkar<[e-mail]>
>
> I don't think using syscore_ops is right here.  The platform code should
> decide where in its own suspend path the notifiers should be triggered.
>
> The reason is because while the syscore_ops run late in the suspend
> path, they still run before some platform-specific decisions about the
> low-power states are made.  That means that any notifiers that need to
> use information about the target low-power state (e.g. whether context
> will be lost or not) cannot do so since that information has not yet
> been decided until the platform_suspend_ops->enter() runs.
>
Initially I thought the same but in general S2R, platform doesn't
support multiple states like CPUIDLE. On OMAP, we do have a debug
option to choose the state but on real product, it's always the
deepest supported state is used. So the driver saving the
full context for S2R, should be fine.

Ofcourse for CPUIDLE, the notifier call chain decisions are left
with platform CPUIDLE drivers since there can be multiple low
power states and the context save/restore has to be done based
on low power states.

The advantage with this is, the platform code is clean from the
notfiers calls. CPUIDLE driver needs to call the different notifier
events based on C-states and that perfectly works.

I liked this simplification for the S2R. Down side is in S2R if you
don't plan to hit deepest state, drivers end up saving full context
which is fine I guess.

> Basically, I think the cpu_*_pm_enter() calls should be called by
> platform-specific code, not by common code.
>
Sure and that's the case for CPUIDLE already. The change was
done only for S2R based on above points.

Regards
Santosh

Kevin Hilman Sept. 8, 2011, 2:01 p.m. UTC | #3

Santosh <[e-mail]> writes:

> On Thursday 08 September 2011 01:32 AM, Kevin Hilman wrote:
>> Santosh Shilimkar<[e-mail]>  writes:
>>
>>> From: Colin Cross<[e-mail]>
>>>
>>> Implements syscore_ops in cpu_pm to call the cpu and
>>> cpu cluster notifiers during suspend and resume,
>>> allowing drivers receiving the notifications to
>>> avoid implementing syscore_ops.
>>>
>>> Signed-off-by: Colin Cross<[e-mail]>
>>> [[e-mail]: Rebased against 3.1-rc4]
>>> Signed-off-by: Santosh Shilimkar<[e-mail]>
>>
>> I don't think using syscore_ops is right here.  The platform code should
>> decide where in its own suspend path the notifiers should be triggered.
>>
>> The reason is because while the syscore_ops run late in the suspend
>> path, they still run before some platform-specific decisions about the
>> low-power states are made.  That means that any notifiers that need to
>> use information about the target low-power state (e.g. whether context
>> will be lost or not) cannot do so since that information has not yet
>> been decided until the platform_suspend_ops->enter() runs.
>>
> Initially I thought the same but in general S2R, platform doesn't
> support multiple states like CPUIDLE. On OMAP, we do have a debug
> option to choose the state but on real product, it's always the
> deepest supported state is used. So the driver saving the
> full context for S2R, should be fine.
>
> Ofcourse for CPUIDLE, the notifier call chain decisions are left
> with platform CPUIDLE drivers since there can be multiple low
> power states and the context save/restore has to be done based
> on low power states.
>
> The advantage with this is, the platform code is clean from the
> notfiers calls. CPUIDLE driver needs to call the different notifier
> events based on C-states and that perfectly works.
>
> I liked this simplification for the S2R. Down side is in S2R if you
> don't plan to hit deepest state, drivers end up saving full context
> which is fine I guess.

That's not the downside I'm worried about.

If you have a driver that has a notifier, presumably it has something it
wants to do to prepare for suspend *and* for idle, and you'd only want a
single notifier callback in the driver to be used for both.  That
callback would look something like:

   start_preparing_for_suspend();

   if (next_state == OFF)
      save_context();

   finish_preparing_for_suspend();


The problem with the current cpu_*_pm_enter() calls in syscore_ops is
that they happen before the next states are programmed, so during
suspend the 'if (next_state == off)' above would never be true, but
during idle it might be.

Kevin

Patch

diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
index 54aa892..3d115d0 100644
--- a/kernel/cpu_pm.c
+++ b/kernel/cpu_pm.c
@@ -20,6 +20,7 @@ 
 #include <linux/module.h>
 #include <linux/notifier.h>
 #include <linux/spinlock.h>
+#include <linux/syscore_ops.h>
 
 static DEFINE_RWLOCK(cpu_pm_notifier_lock);
 static RAW_NOTIFIER_HEAD(cpu_pm_notifier_chain);
@@ -113,3 +114,35 @@  int cpu_cluster_pm_exit(void)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cpu_cluster_pm_exit);
+
+#ifdef CONFIG_PM
+static int cpu_pm_suspend(void)
+{
+	int ret;
+
+	ret = cpu_pm_enter();
+	if (ret)
+		return ret;
+
+	ret = cpu_cluster_pm_enter();
+	return ret;
+}
+
+static void cpu_pm_resume(void)
+{
+	cpu_cluster_pm_exit();
+	cpu_pm_exit();
+}
+
+static struct syscore_ops cpu_pm_syscore_ops = {
+	.suspend = cpu_pm_suspend,
+	.resume = cpu_pm_resume,
+};
+
+static int cpu_pm_init(void)
+{
+	register_syscore_ops(&cpu_pm_syscore_ops);
+	return 0;
+}
+core_initcall(cpu_pm_init);
+#endif