[RFC v2] ARM: sched_clock: update epoch_cyc on resume

[RFC v2] ARM: sched_clock: update epoch_cyc on resume

[Colin Cross]

Many clocks that are used to provide sched_clock will reset during
suspend.  If read_sched_clock returns 0 after suspend, sched_clock will
appear to jump forward.  This patch resets cd.epoch_cyc to the current
value of read_sched_clock during resume, which causes sched_clock() just
after suspend to return the same value as sched_clock() just before
suspend.

In addition, during the window where epoch_ns has been updated before
suspend, but epoch_cyc has not been updated after suspend, it is unknown
whether the clock has reset or not, and sched_clock() could return a
bogus value.  Add a suspended flag, and return the pre-suspend epoch_ns
value during this period.

The new behavior is triggered by calling setup_sched_clock_needs_suspend
instead of setup_sched_clock.

Signed-off-by: Colin Cross <ccross@android.com>
---
 arch/arm/include/asm/sched_clock.h |    2 ++
 arch/arm/kernel/sched_clock.c      |   24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/sched_clock.h b/arch/arm/include/asm/sched_clock.h
index e3f7572..05b8e82 100644
--- a/arch/arm/include/asm/sched_clock.h
+++ b/arch/arm/include/asm/sched_clock.h
@@ -10,5 +10,7 @@
 
 extern void sched_clock_postinit(void);
 extern void setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate);
+extern void setup_sched_clock_needs_suspend(u32 (*read)(void), int bits,
+		unsigned long rate);
 
 #endif
diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
index 27d186a..f451539 100644
--- a/arch/arm/kernel/sched_clock.c
+++ b/arch/arm/kernel/sched_clock.c
@@ -21,6 +21,8 @@ struct clock_data {
 	u32 epoch_cyc_copy;
 	u32 mult;
 	u32 shift;
+	bool suspended;
+	bool needs_suspend;
 };
 
 static void sched_clock_poll(unsigned long wrap_ticks);
@@ -49,6 +51,9 @@ static unsigned long long cyc_to_sched_clock(u32 cyc, u32 mask)
 	u64 epoch_ns;
 	u32 epoch_cyc;
 
+	if (cd.suspended)
+		return cd.epoch_ns;
+
 	/*
 	 * Load the epoch_cyc and epoch_ns atomically.  We do this by
 	 * ensuring that we always write epoch_cyc, epoch_ns and
@@ -98,6 +103,13 @@ static void sched_clock_poll(unsigned long wrap_ticks)
 	update_sched_clock();
 }
 
+void __init setup_sched_clock_needs_suspend(u32 (*read)(void), int bits,
+		unsigned long rate)
+{
+	setup_sched_clock(read, bits, rate);
+	cd.needs_suspend = true;
+}
+
 void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
 {
 	unsigned long r, w;
@@ -169,11 +181,23 @@ void __init sched_clock_postinit(void)
 static int sched_clock_suspend(void)
 {
 	sched_clock_poll(sched_clock_timer.data);
+	if (cd.needs_suspend)
+		cd.suspended = true;
 	return 0;
 }
 
+static void sched_clock_resume(void)
+{
+	if (cd.needs_suspend) {
+		cd.epoch_cyc = read_sched_clock();
+		cd.epoch_cyc_copy = cd.epoch_cyc;
+		cd.suspended = false;
+	}
+}
+
 static struct syscore_ops sched_clock_ops = {
 	.suspend = sched_clock_suspend,
+	.resume = sched_clock_resume,
 };
 
 static int __init sched_clock_syscore_init(void)
-- 
1.7.7.3

[RFC v2] ARM: sched_clock: update epoch_cyc on resume

[Linus Walleij]

On Wed, Jul 25, 2012 at 4:49 AM, Colin Cross <ccross@android.com> wrote:

> Many clocks that are used to provide sched_clock will reset during
> suspend.  If read_sched_clock returns 0 after suspend, sched_clock will
> appear to jump forward.  This patch resets cd.epoch_cyc to the current
> value of read_sched_clock during resume, which causes sched_clock() just
> after suspend to return the same value as sched_clock() just before
> suspend.
>
> In addition, during the window where epoch_ns has been updated before
> suspend, but epoch_cyc has not been updated after suspend, it is unknown
> whether the clock has reset or not, and sched_clock() could return a
> bogus value.  Add a suspended flag, and return the pre-suspend epoch_ns
> value during this period.
>
> The new behavior is triggered by calling setup_sched_clock_needs_suspend
> instead of setup_sched_clock.
>
> Signed-off-by: Colin Cross <ccross@android.com>

Sweet!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[RFC v2] ARM: sched_clock: update epoch_cyc on resume

[Russell King - ARM Linux]

On Sat, Jul 28, 2012 at 01:32:50AM +0200, Linus Walleij wrote:
> On Wed, Jul 25, 2012 at 4:49 AM, Colin Cross <ccross@android.com> wrote:
> 
> > Many clocks that are used to provide sched_clock will reset during
> > suspend.  If read_sched_clock returns 0 after suspend, sched_clock will
> > appear to jump forward.  This patch resets cd.epoch_cyc to the current
> > value of read_sched_clock during resume, which causes sched_clock() just
> > after suspend to return the same value as sched_clock() just before
> > suspend.
> >
> > In addition, during the window where epoch_ns has been updated before
> > suspend, but epoch_cyc has not been updated after suspend, it is unknown
> > whether the clock has reset or not, and sched_clock() could return a
> > bogus value.  Add a suspended flag, and return the pre-suspend epoch_ns
> > value during this period.
> >
> > The new behavior is triggered by calling setup_sched_clock_needs_suspend
> > instead of setup_sched_clock.
> >
> > Signed-off-by: Colin Cross <ccross@android.com>
> 
> Sweet!
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Have any of you looked at the patch I originally posted for doing this?
It needs updating but shows the overall principle - which is to ensure
that the epoch is up to date before suspending.

It doesn't deal with resume, because different timers behave differently,
and there's no real way to deal with that properly.  The important thing
that this patch does is to ensure sched_clock() doesn't ever go backwards.

 arch/arm/kernel/sched_clock.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
index 9a46370..4be4019 100644
--- a/arch/arm/kernel/sched_clock.c
+++ b/arch/arm/kernel/sched_clock.c
@@ -10,6 +10,7 @@
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
+#include <linux/syscore_ops.h>
 #include <linux/timer.h>
 
 #include <asm/sched_clock.h>
@@ -72,3 +73,20 @@ void __init sched_clock_postinit(void)
 {
 	sched_clock_poll(sched_clock_timer.data);
 }
+
+static int sched_clock_suspend(void)
+{
+	sched_clock_poll(sched_clock_timer.data);
+	return 0;
+}
+
+static struct syscore_ops sched_clock_ops = {
+	.suspend	= sched_clock_suspend,
+};
+
+static int __init sched_clock_syscore_init(void)
+{
+	register_syscore_ops(&sched_clock_ops);
+	return 0;
+}
+device_initcall(sched_clock_syscore_init);

[RFC v2] ARM: sched_clock: update epoch_cyc on resume

[Colin Cross]

On Fri, Jul 27, 2012 at 4:38 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Jul 28, 2012 at 01:32:50AM +0200, Linus Walleij wrote:
>> On Wed, Jul 25, 2012 at 4:49 AM, Colin Cross <ccross@android.com> wrote:
>>
>> > Many clocks that are used to provide sched_clock will reset during
>> > suspend.  If read_sched_clock returns 0 after suspend, sched_clock will
>> > appear to jump forward.  This patch resets cd.epoch_cyc to the current
>> > value of read_sched_clock during resume, which causes sched_clock() just
>> > after suspend to return the same value as sched_clock() just before
>> > suspend.
>> >
>> > In addition, during the window where epoch_ns has been updated before
>> > suspend, but epoch_cyc has not been updated after suspend, it is unknown
>> > whether the clock has reset or not, and sched_clock() could return a
>> > bogus value.  Add a suspended flag, and return the pre-suspend epoch_ns
>> > value during this period.
>> >
>> > The new behavior is triggered by calling setup_sched_clock_needs_suspend
>> > instead of setup_sched_clock.
>> >
>> > Signed-off-by: Colin Cross <ccross@android.com>
>>
>> Sweet!
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Have any of you looked at the patch I originally posted for doing this?
> It needs updating but shows the overall principle - which is to ensure
> that the epoch is up to date before suspending.
>
> It doesn't deal with resume, because different timers behave differently,
> and there's no real way to deal with that properly.  The important thing
> that this patch does is to ensure sched_clock() doesn't ever go backwards.
>
>  arch/arm/kernel/sched_clock.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
> index 9a46370..4be4019 100644
> --- a/arch/arm/kernel/sched_clock.c
> +++ b/arch/arm/kernel/sched_clock.c
> @@ -10,6 +10,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/sched.h>
> +#include <linux/syscore_ops.h>
>  #include <linux/timer.h>
>
>  #include <asm/sched_clock.h>
> @@ -72,3 +73,20 @@ void __init sched_clock_postinit(void)
>  {
>         sched_clock_poll(sched_clock_timer.data);
>  }
> +
> +static int sched_clock_suspend(void)
> +{
> +       sched_clock_poll(sched_clock_timer.data);
> +       return 0;
> +}
> +
> +static struct syscore_ops sched_clock_ops = {
> +       .suspend        = sched_clock_suspend,
> +};
> +
> +static int __init sched_clock_syscore_init(void)
> +{
> +       register_syscore_ops(&sched_clock_ops);
> +       return 0;
> +}
> +device_initcall(sched_clock_syscore_init);
>

That patch was merged in 3.4, and my patch is on top of it.  Your
patch updates epoch_cyc and epoch_ns in suspend, but if the first call
to cyc_to_sched_clock after resume gets cyc = 0, cyc - epoch_cyc can
be negative, although it will be cast back to a large positive number.

With my patch, epoch_cyc is updated in resume to whatever
read_sched_clock() returns, and epoch_ns is still set to the suspend
value, so it appears that sched_clock has not changed between
sched_clock_suspend and sched_clock_resume.  It will work with any
timer behavior (reset to 0, reset to random value, or continuing
counting).  The old setup_sched_clock function maintains the old
behavior to appease those who like their 32kHz sched clock to continue
ticking in suspend, although I think it is more correct for all sched
clocks to be faster than 32 kHz (when possible) and stop in suspend.

[RFC v2] ARM: sched_clock: update epoch_cyc on resume

[Colin Cross]

On Fri, Jul 27, 2012 at 6:15 PM, Colin Cross <ccross@android.com> wrote:
> On Fri, Jul 27, 2012 at 4:38 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Sat, Jul 28, 2012 at 01:32:50AM +0200, Linus Walleij wrote:
>>> On Wed, Jul 25, 2012 at 4:49 AM, Colin Cross <ccross@android.com> wrote:
>>>
>>> > Many clocks that are used to provide sched_clock will reset during
>>> > suspend.  If read_sched_clock returns 0 after suspend, sched_clock will
>>> > appear to jump forward.  This patch resets cd.epoch_cyc to the current
>>> > value of read_sched_clock during resume, which causes sched_clock() just
>>> > after suspend to return the same value as sched_clock() just before
>>> > suspend.
>>> >
>>> > In addition, during the window where epoch_ns has been updated before
>>> > suspend, but epoch_cyc has not been updated after suspend, it is unknown
>>> > whether the clock has reset or not, and sched_clock() could return a
>>> > bogus value.  Add a suspended flag, and return the pre-suspend epoch_ns
>>> > value during this period.
>>> >
>>> > The new behavior is triggered by calling setup_sched_clock_needs_suspend
>>> > instead of setup_sched_clock.
>>> >
>>> > Signed-off-by: Colin Cross <ccross@android.com>
>>>
>>> Sweet!
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> Have any of you looked at the patch I originally posted for doing this?
>> It needs updating but shows the overall principle - which is to ensure
>> that the epoch is up to date before suspending.
>>
>> It doesn't deal with resume, because different timers behave differently,
>> and there's no real way to deal with that properly.  The important thing
>> that this patch does is to ensure sched_clock() doesn't ever go backwards.
>>
>>  arch/arm/kernel/sched_clock.c |   18 ++++++++++++++++++
>>  1 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
>> index 9a46370..4be4019 100644
>> --- a/arch/arm/kernel/sched_clock.c
>> +++ b/arch/arm/kernel/sched_clock.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/jiffies.h>
>>  #include <linux/kernel.h>
>>  #include <linux/sched.h>
>> +#include <linux/syscore_ops.h>
>>  #include <linux/timer.h>
>>
>>  #include <asm/sched_clock.h>
>> @@ -72,3 +73,20 @@ void __init sched_clock_postinit(void)
>>  {
>>         sched_clock_poll(sched_clock_timer.data);
>>  }
>> +
>> +static int sched_clock_suspend(void)
>> +{
>> +       sched_clock_poll(sched_clock_timer.data);
>> +       return 0;
>> +}
>> +
>> +static struct syscore_ops sched_clock_ops = {
>> +       .suspend        = sched_clock_suspend,
>> +};
>> +
>> +static int __init sched_clock_syscore_init(void)
>> +{
>> +       register_syscore_ops(&sched_clock_ops);
>> +       return 0;
>> +}
>> +device_initcall(sched_clock_syscore_init);
>>
>
> That patch was merged in 3.4, and my patch is on top of it.  Your
> patch updates epoch_cyc and epoch_ns in suspend, but if the first call
> to cyc_to_sched_clock after resume gets cyc = 0, cyc - epoch_cyc can
> be negative, although it will be cast back to a large positive number.
>
> With my patch, epoch_cyc is updated in resume to whatever
> read_sched_clock() returns, and epoch_ns is still set to the suspend
> value, so it appears that sched_clock has not changed between
> sched_clock_suspend and sched_clock_resume.  It will work with any
> timer behavior (reset to 0, reset to random value, or continuing
> counting).  The old setup_sched_clock function maintains the old
> behavior to appease those who like their 32kHz sched clock to continue
> ticking in suspend, although I think it is more correct for all sched
> clocks to be faster than 32 kHz (when possible) and stop in suspend.

I think the existing code can cause sched_clock to go backwards if the
register read by the read function resets to 0 in suspend:

In sched_clock_suspend epoch_cyc is updated to 0x00002000.

The first sched_clock call after resume gets cyc = 0, returning
epoch_ns + cyc_to_ns(0xffffe000)

Later, sched_clock gets cyc = 0x5000, returning epoch_ns +
cyc_to_ns(0x3000), which will be much smaller than the previous
sched_clock value.

[RFC v2] ARM: sched_clock: update epoch_cyc on resume

[Colin Cross]

On Fri, Jul 27, 2012 at 8:30 PM, Colin Cross <ccross@android.com> wrote:
> On Fri, Jul 27, 2012 at 6:15 PM, Colin Cross <ccross@android.com> wrote:
>> That patch was merged in 3.4, and my patch is on top of it.  Your
>> patch updates epoch_cyc and epoch_ns in suspend, but if the first call
>> to cyc_to_sched_clock after resume gets cyc = 0, cyc - epoch_cyc can
>> be negative, although it will be cast back to a large positive number.
>>
>> With my patch, epoch_cyc is updated in resume to whatever
>> read_sched_clock() returns, and epoch_ns is still set to the suspend
>> value, so it appears that sched_clock has not changed between
>> sched_clock_suspend and sched_clock_resume.  It will work with any
>> timer behavior (reset to 0, reset to random value, or continuing
>> counting).  The old setup_sched_clock function maintains the old
>> behavior to appease those who like their 32kHz sched clock to continue
>> ticking in suspend, although I think it is more correct for all sched
>> clocks to be faster than 32 kHz (when possible) and stop in suspend.
>
> I think the existing code can cause sched_clock to go backwards if the
> register read by the read function resets to 0 in suspend:
>
> In sched_clock_suspend epoch_cyc is updated to 0x00002000.
>
> The first sched_clock call after resume gets cyc = 0, returning
> epoch_ns + cyc_to_ns(0xffffe000)
>
> Later, sched_clock gets cyc = 0x5000, returning epoch_ns +
> cyc_to_ns(0x3000), which will be much smaller than the previous
> sched_clock value.

Russell, any update on this?  Should sched_clock.c handle read
functions that go backwards in suspend, or should I modify the read
function to track an offset in suspend and always return a
monotonically increasing value across suspend?

[RFC v2] ARM: sched_clock: update epoch_cyc on resume

[Linus Walleij]

On Fri, Aug 3, 2012 at 3:28 AM, Colin Cross <ccross@android.com> wrote:
> On Fri, Jul 27, 2012 at 8:30 PM, Colin Cross <ccross@android.com> wrote:
>> On Fri, Jul 27, 2012 at 6:15 PM, Colin Cross <ccross@android.com> wrote:
>>> That patch was merged in 3.4, and my patch is on top of it.  Your
>>> patch updates epoch_cyc and epoch_ns in suspend, but if the first call
>>> to cyc_to_sched_clock after resume gets cyc = 0, cyc - epoch_cyc can
>>> be negative, although it will be cast back to a large positive number.
>>>
>>> With my patch, epoch_cyc is updated in resume to whatever
>>> read_sched_clock() returns, and epoch_ns is still set to the suspend
>>> value, so it appears that sched_clock has not changed between
>>> sched_clock_suspend and sched_clock_resume.  It will work with any
>>> timer behavior (reset to 0, reset to random value, or continuing
>>> counting).  The old setup_sched_clock function maintains the old
>>> behavior to appease those who like their 32kHz sched clock to continue
>>> ticking in suspend, although I think it is more correct for all sched
>>> clocks to be faster than 32 kHz (when possible) and stop in suspend.
>>
>> I think the existing code can cause sched_clock to go backwards if the
>> register read by the read function resets to 0 in suspend:
>>
>> In sched_clock_suspend epoch_cyc is updated to 0x00002000.
>>
>> The first sched_clock call after resume gets cyc = 0, returning
>> epoch_ns + cyc_to_ns(0xffffe000)
>>
>> Later, sched_clock gets cyc = 0x5000, returning epoch_ns +
>> cyc_to_ns(0x3000), which will be much smaller than the previous
>> sched_clock value.
>
> Russell, any update on this?  Should sched_clock.c handle read
> functions that go backwards in suspend, or should I modify the read
> function to track an offset in suspend and always return a
> monotonically increasing value across suspend?

Colin, I suggest you put this into Russell's patch tracker so
he can keep track of it from there.

Yours,
Linus Walleij