[PATCH v2 1/3] time: Don't bother to run rtc_resume() for the nonstop clocksource

[PATCH v2 1/3] time: Don't bother to run rtc_resume() for the nonstop clocksource

[Xunlei Pang]

If a system does not provide a persistent_clock(), the time
will be updated on resume by rtc_resume(). With the addition
of the non-stop clocksources for suspend timing, those systems
set the time on resume in timekeeping_resume(), but may not
provide a valid persistent_clock().

This results in the rtc_resume() logic thinking no one has set
the time and it then will over-write the suspend time again,
which is not necessary and only increases clock error.

So, fix this for rtc_resume().

Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
---
v1->v2:
Modify according to Thomas' feedback.

 drivers/rtc/class.c         |  2 +-
 include/linux/timekeeping.h |  7 +++++++
 kernel/time/timekeeping.c   | 16 +++++-----------
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 472a5ad..6100af5 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -102,7 +102,7 @@ static int rtc_resume(struct device *dev)
 	struct timespec64	sleep_time;
 	int err;
 
-	if (has_persistent_clock())
+	if (timekeeping_sleeptime_injected())
 		return 0;
 
 	rtc_hctosys_ret = -ENODEV;
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 9b63d13..2b87c64 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -238,6 +238,13 @@ extern void getnstime_raw_and_real(struct timespec *ts_raw,
  */
 extern bool persistent_clock_exist;
 extern int persistent_clock_is_local;
+extern bool timekeeping_sleeptime_inject;
+
+/* Used by rtc_resume() */
+static inline bool timekeeping_sleeptime_injected(void)
+{
+	return timekeeping_sleeptime_inject;
+}
 
 static inline bool has_persistent_clock(void)
 {
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 6a93185..732ccd0 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -65,6 +65,7 @@ int __read_mostly timekeeping_suspended;
 
 /* Flag for if there is a persistent clock on this platform */
 bool __read_mostly persistent_clock_exist = false;
+bool timekeeping_sleeptime_inject;
 
 static inline void tk_normalize_xtime(struct timekeeper *tk)
 {
@@ -1140,13 +1141,6 @@ void timekeeping_inject_sleeptime64(struct timespec64 *delta)
 	struct timekeeper *tk = &tk_core.timekeeper;
 	unsigned long flags;
 
-	/*
-	 * Make sure we don't set the clock twice, as timekeeping_resume()
-	 * already did it
-	 */
-	if (has_persistent_clock())
-		return;
-
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
 	write_seqcount_begin(&tk_core.seq);
 
@@ -1178,8 +1172,8 @@ static void timekeeping_resume(void)
 	struct timespec64 ts_new, ts_delta;
 	struct timespec tmp;
 	cycle_t cycle_now, cycle_delta;
-	bool suspendtime_found = false;
 
+	timekeeping_sleeptime_inject = false;
 	read_persistent_clock(&tmp);
 	ts_new = timespec_to_timespec64(tmp);
 
@@ -1226,13 +1220,13 @@ static void timekeeping_resume(void)
 		nsec += ((u64) cycle_delta * mult) >> shift;
 
 		ts_delta = ns_to_timespec64(nsec);
-		suspendtime_found = true;
+		timekeeping_sleeptime_inject = true;
 	} else if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) {
 		ts_delta = timespec64_sub(ts_new, timekeeping_suspend_time);
-		suspendtime_found = true;
+		timekeeping_sleeptime_inject = true;
 	}
 
-	if (suspendtime_found)
+	if (timekeeping_sleeptime_inject)
 		__timekeeping_inject_sleeptime(tk, &ts_delta);
 
 	/* Re-base the last cycle value */
-- 
1.9.1

[PATCH v2 2/3] rtc: Remove redundant rtc_valid_tm() from rtc_resume()

[Xunlei Pang]

rtc_read_time() has already judged valid tm by rtc_valid_tm(),
so just remove it.

Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
---
 drivers/rtc/class.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 6100af5..7db4c90 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -117,10 +117,6 @@ static int rtc_resume(struct device *dev)
 		return 0;
 	}
 
-	if (rtc_valid_tm(&tm) != 0) {
-		pr_debug("%s:  bogus resume time\n", dev_name(&rtc->dev));
-		return 0;
-	}
 	new_rtc.tv_sec = rtc_tm_to_time64(&tm);
 	new_rtc.tv_nsec = 0;
 
-- 
1.9.1

[PATCH v2 3/3] time: clocksource: Add a comment to CLOCK_SOURCE_SUSPEND_NONSTOP

[Xunlei Pang]

When doing timekeeping_resume(), if the nonstop clocksource
wraps back, "cycle_delta" will miss the wrap time.

It's hard to determine the right CLOCKSOURCE_MASK(xxx) or
something to add code for inspecting such behavior, and we
don't have many existent nonstop clocksources, so just add
a comment to indicate that if have this flag set, people
are aware that this nonstop clocksource won't wrap back
during system suspend/resume.

Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
---
 include/linux/clocksource.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index abcafaa..8680189 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -207,6 +207,11 @@ struct clocksource {
 #define CLOCK_SOURCE_WATCHDOG			0x10
 #define CLOCK_SOURCE_VALID_FOR_HRES		0x20
 #define CLOCK_SOURCE_UNSTABLE			0x40
+
+/*
+ * When setting this flag, you're also supposed to mean that it doesn't
+ * wrap back during system suspend/resume. See timekeeping_resume().
+ */
 #define CLOCK_SOURCE_SUSPEND_NONSTOP		0x80
 #define CLOCK_SOURCE_RESELECT			0x100
 
-- 
1.9.1

Re: [PATCH v2 3/3] time: clocksource: Add a comment to CLOCK_SOURCE_SUSPEND_NONSTOP

[Thomas Gleixner]

On Thu, 22 Jan 2015, Xunlei Pang wrote:
> When doing timekeeping_resume(), if the nonstop clocksource
> wraps back, "cycle_delta" will miss the wrap time.
> 
> It's hard to determine the right CLOCKSOURCE_MASK(xxx) or
> something to add code for inspecting such behavior, and we
> don't have many existent nonstop clocksources, so just add
> a comment to indicate that if have this flag set, people
> are aware that this nonstop clocksource won't wrap back
> during system suspend/resume.

-ENOPARSE

What has the CLOCKSOURCE_MASK() to do with this and why is the fact
relevant, that we only have a few suspend_nonstop clock sources?

> +
> +/*
> + * When setting this flag, you're also supposed to mean that it doesn't
> + * wrap back during system suspend/resume. See timekeeping_resume().

-ENOPARSE

I guess what you want to say here is:

/*
 * clocksource continues to run during suspend and is guaranteed not to
 * wrap around during long suspend periods.
 */

> + */
>  #define CLOCK_SOURCE_SUSPEND_NONSTOP		0x80

Thanks,

	tglx

Re: [PATCH v2 3/3] time: clocksource: Add a comment to CLOCK_SOURCE_SUSPEND_NONSTOP

[Xunlei Pang]

Hi Thomas,

On 23 January 2015 at 05:07, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 22 Jan 2015, Xunlei Pang wrote:
>> When doing timekeeping_resume(), if the nonstop clocksource
>> wraps back, "cycle_delta" will miss the wrap time.
>>
>> It's hard to determine the right CLOCKSOURCE_MASK(xxx) or
>> something to add code for inspecting such behavior, and we
>> don't have many existent nonstop clocksources, so just add
>> a comment to indicate that if have this flag set, people
>> are aware that this nonstop clocksource won't wrap back
>> during system suspend/resume.
>
> -ENOPARSE
>
> What has the CLOCKSOURCE_MASK() to do with this and why is the fact
> relevant, that we only have a few suspend_nonstop clock sources?

Before this, I tried to add some code to catch such problem at the
time of registering
the clocksource, like using the CLOCKSOURCE_MASK(), for example 64bit counter
will never wrap for us. But there may be other values like CLOCKSOURCE_MASK(56),
I just can't figure out exactly how to do this judge.

So, I think we can only add a comment to let the developer be aware of
this when registering
nonstop clocksource, that's what I want to express.

>
>> +
>> +/*
>> + * When setting this flag, you're also supposed to mean that it doesn't
>> + * wrap back during system suspend/resume. See timekeeping_resume().
>
> -ENOPARSE
>
> I guess what you want to say here is:
>
> /*
>  * clocksource continues to run during suspend and is guaranteed not to
>  * wrap around during long suspend periods.
>  */
>

Yes, this description is way better :-)

Thanks,
Xunlei

Re: [PATCH v2 3/3] time: clocksource: Add a comment to CLOCK_SOURCE_SUSPEND_NONSTOP

[Thomas Gleixner]

On Sat, 24 Jan 2015, Xunlei Pang wrote:

> Before this, I tried to add some code to catch such problem at the
> time of registering the clocksource, like using the
> CLOCKSOURCE_MASK(), for example 64bit counter will never wrap for
> us. But there may be other values like CLOCKSOURCE_MASK(56), I just
> can't figure out exactly how to do this judge.

I don't think there is a good way to do so. Registration time is the
wrong place anyway because the problem depends on:

 - The width of the counter
 - The frequency of the counter

The frequency of the counter might even change after registration. Now
add the unknown duration of the suspend to the picture and you're
completely lost.

All we can do is provide information about the actual wraparound time,
if the CLOCK_SOURCE_SUSPEND_NONSTOP flag is set and the wraparound
time is less than some reasonable margin.

Thanks,

	tglx

Re: [PATCH v2 3/3] time: clocksource: Add a comment to CLOCK_SOURCE_SUSPEND_NONSTOP

[Xunlei Pang]

Hi Thomas,

On 25 January 2015 at 01:07, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sat, 24 Jan 2015, Xunlei Pang wrote:
>
>> Before this, I tried to add some code to catch such problem at the
>> time of registering the clocksource, like using the
>> CLOCKSOURCE_MASK(), for example 64bit counter will never wrap for
>> us. But there may be other values like CLOCKSOURCE_MASK(56), I just
>> can't figure out exactly how to do this judge.
>
> I don't think there is a good way to do so. Registration time is the
> wrong place anyway because the problem depends on:
>
>  - The width of the counter
>  - The frequency of the counter
>
> The frequency of the counter might even change after registration. Now
> add the unknown duration of the suspend to the picture and you're
> completely lost.
>
> All we can do is provide information about the actual wraparound time,
> if the CLOCK_SOURCE_SUSPEND_NONSTOP flag is set and the wraparound
> time is less than some reasonable margin.
>

Yes, we can only deal with it approximately. How about this?

1) Add a new member about reference wraparound time(max system suspend
period allowed) to struct  clocksource. In
__clocksource_updatefreq_scale(), we can use  "sec" which already
applys 12.5% margin as its value.

2) Add a new tuneable sysctl threshold with a default time period
value(for example, 365 days)
    We can also printk its value when booting or changing its value to
notice people about this.

3) then,  in timekeeping_resume(), we can compare the reference
wraparound of the nonstop clocksource with the sysctl threshold to
decide if it is dependable to use.

Thanks,
Xunlei

>
>
>
>

Re: [PATCH v2 1/3] time: Don't bother to run rtc_resume() for the nonstop clocksource

[John Stultz]

On Thu, Jan 22, 2015 at 4:01 AM, Xunlei Pang <pang.xunlei@linaro.org> wrote:
> If a system does not provide a persistent_clock(), the time
> will be updated on resume by rtc_resume(). With the addition
> of the non-stop clocksources for suspend timing, those systems
> set the time on resume in timekeeping_resume(), but may not
> provide a valid persistent_clock().
>
> This results in the rtc_resume() logic thinking no one has set
> the time and it then will over-write the suspend time again,
> which is not necessary and only increases clock error.
>
> So, fix this for rtc_resume().
>
> Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
> ---
> v1->v2:
> Modify according to Thomas' feedback.
>
>  drivers/rtc/class.c         |  2 +-
>  include/linux/timekeeping.h |  7 +++++++
>  kernel/time/timekeeping.c   | 16 +++++-----------
>  3 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index 472a5ad..6100af5 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -102,7 +102,7 @@ static int rtc_resume(struct device *dev)
>         struct timespec64       sleep_time;
>         int err;
>
> -       if (has_persistent_clock())
> +       if (timekeeping_sleeptime_injected())
>                 return 0;
>
>         rtc_hctosys_ret = -ENODEV;
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index 9b63d13..2b87c64 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -238,6 +238,13 @@ extern void getnstime_raw_and_real(struct timespec *ts_raw,
>   */
>  extern bool persistent_clock_exist;
>  extern int persistent_clock_is_local;
> +extern bool timekeeping_sleeptime_inject;
> +
> +/* Used by rtc_resume() */
> +static inline bool timekeeping_sleeptime_injected(void)
> +{
> +       return timekeeping_sleeptime_inject;
> +}
>

Again, there's no reason to make this a static inline, nor the
sleeptime_inject variable global.
Just make it a function in timekeeping and provide definition so it
can be used by the rtc resume.

thanks
-john

Re: [PATCH v2 1/3] time: Don't bother to run rtc_resume() for the nonstop clocksource

[Xunlei Pang]

On 23 January 2015 at 02:30, John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Jan 22, 2015 at 4:01 AM, Xunlei Pang <pang.xunlei@linaro.org> wrote:
>> If a system does not provide a persistent_clock(), the time
>> will be updated on resume by rtc_resume(). With the addition
>> of the non-stop clocksources for suspend timing, those systems
>> set the time on resume in timekeeping_resume(), but may not
>> provide a valid persistent_clock().
>>
>> This results in the rtc_resume() logic thinking no one has set
>> the time and it then will over-write the suspend time again,
>> which is not necessary and only increases clock error.
>>
>> So, fix this for rtc_resume().
>>
>> Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
>> ---
>> v1->v2:
>> Modify according to Thomas' feedback.
>>
>>  drivers/rtc/class.c         |  2 +-
>>  include/linux/timekeeping.h |  7 +++++++
>>  kernel/time/timekeeping.c   | 16 +++++-----------
>>  3 files changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
>> index 472a5ad..6100af5 100644
>> --- a/drivers/rtc/class.c
>> +++ b/drivers/rtc/class.c
>> @@ -102,7 +102,7 @@ static int rtc_resume(struct device *dev)
>>         struct timespec64       sleep_time;
>>         int err;
>>
>> -       if (has_persistent_clock())
>> +       if (timekeeping_sleeptime_injected())
>>                 return 0;
>>
>>         rtc_hctosys_ret = -ENODEV;
>> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
>> index 9b63d13..2b87c64 100644
>> --- a/include/linux/timekeeping.h
>> +++ b/include/linux/timekeeping.h
>> @@ -238,6 +238,13 @@ extern void getnstime_raw_and_real(struct timespec *ts_raw,
>>   */
>>  extern bool persistent_clock_exist;
>>  extern int persistent_clock_is_local;
>> +extern bool timekeeping_sleeptime_inject;
>> +
>> +/* Used by rtc_resume() */
>> +static inline bool timekeeping_sleeptime_injected(void)
>> +{
>> +       return timekeeping_sleeptime_inject;
>> +}
>>
>
> Again, there's no reason to make this a static inline, nor the
> sleeptime_inject variable global.
> Just make it a function in timekeeping and provide definition so it
> can be used by the rtc resume.


Hi John,

Thanks for your comments, I've refined it, is this one ok?

---
 drivers/rtc/class.c         |  2 +-
 include/linux/timekeeping.h |  1 +
 kernel/time/timekeeping.c   | 32 ++++++++++++++++++++------------
 3 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 472a5ad..6100af5 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -102,7 +102,7 @@ static int rtc_resume(struct device *dev)
  struct timespec64 sleep_time;
  int err;

- if (has_persistent_clock())
+ if (timekeeping_sleeptime_injected())
  return 0;

  rtc_hctosys_ret = -ENODEV;
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 9b63d13..17a460d 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -225,6 +225,7 @@ static inline void timekeeping_clocktai(struct timespec *ts)
 /*
  * RTC specific
  */
+extern bool timekeeping_sleeptime_injected(void);
 extern void timekeeping_inject_sleeptime64(struct timespec64 *delta);

 /*
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 6a93185..b02133e 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1125,12 +1125,26 @@ static void
__timekeeping_inject_sleeptime(struct timekeeper *tk,
  tk_debug_account_sleep_time(delta);
 }

+static bool sleeptime_inject;
+
+#if defined(CONFIG_RTC_CLASS) && \
+ defined(CONFIG_PM_SLEEP) && \
+ defined(CONFIG_RTC_HCTOSYS_DEVICE)
+/**
+ * Used by rtc_resume().
+ */
+bool timekeeping_sleeptime_injected(void)
+{
+ return sleeptime_inject;
+}
+
 /**
  * timekeeping_inject_sleeptime64 - Adds suspend interval to
timeekeeping values
  * @delta: pointer to a timespec64 delta value
  *
  * This hook is for architectures that cannot support read_persistent_clock
- * because their RTC/persistent clock is only accessible when irqs are enabled.
+ * because their RTC/persistent clock is only accessible when irqs are enabled,
+ * and also don't have an effective nonstop clocksource.
  *
  * This function should only be called by rtc_resume(), and allows
  * a suspend offset to be injected into the timekeeping values.
@@ -1140,13 +1154,6 @@ void timekeeping_inject_sleeptime64(struct
timespec64 *delta)
  struct timekeeper *tk = &tk_core.timekeeper;
  unsigned long flags;

- /*
- * Make sure we don't set the clock twice, as timekeeping_resume()
- * already did it
- */
- if (has_persistent_clock())
- return;
-
  raw_spin_lock_irqsave(&timekeeper_lock, flags);
  write_seqcount_begin(&tk_core.seq);

@@ -1162,6 +1169,7 @@ void timekeeping_inject_sleeptime64(struct
timespec64 *delta)
  /* signal hrtimers about time change */
  clock_was_set();
 }
+#endif

 /**
  * timekeeping_resume - Resumes the generic timekeeping subsystem.
@@ -1178,8 +1186,8 @@ static void timekeeping_resume(void)
  struct timespec64 ts_new, ts_delta;
  struct timespec tmp;
  cycle_t cycle_now, cycle_delta;
- bool suspendtime_found = false;

+ sleeptime_inject = false;
  read_persistent_clock(&tmp);
  ts_new = timespec_to_timespec64(tmp);

@@ -1226,13 +1234,13 @@ static void timekeeping_resume(void)
  nsec += ((u64) cycle_delta * mult) >> shift;

  ts_delta = ns_to_timespec64(nsec);
- suspendtime_found = true;
+ sleeptime_inject = true;
  } else if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) {
  ts_delta = timespec64_sub(ts_new, timekeeping_suspend_time);
- suspendtime_found = true;
+ sleeptime_inject = true;
  }

- if (suspendtime_found)
+ if (sleeptime_inject)
  __timekeeping_inject_sleeptime(tk, &ts_delta);

  /* Re-base the last cycle value */
-- 
1.9.1