[PATCH v5 1/4] time: Add needed macros for timekeeping_inject_sleeptime64()

[PATCH v5 1/4] time: Add needed macros for timekeeping_inject_sleeptime64()

[Xunlei Pang]

From: Xunlei Pang <pang.xunlei@linaro.org>

timekeeping_inject_sleeptime64() is only used by RTC suspend/resume,
so embrace it in RTC related macros.

Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
---
v5 changes:
Remove CONFIG_RTC_CLASS.

 kernel/time/timekeeping.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index b124af2..49b1643 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1125,6 +1125,7 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
 	tk_debug_account_sleep_time(delta);
 }
 
+#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE)
 /**
  * timekeeping_inject_sleeptime64 - Adds suspend interval to timeekeeping values
  * @delta: pointer to a timespec64 delta value
@@ -1162,6 +1163,7 @@ void timekeeping_inject_sleeptime64(struct timespec64 *delta)
 	/* signal hrtimers about time change */
 	clock_was_set();
 }
+#endif
 
 /**
  * timekeeping_resume - Resumes the generic timekeeping subsystem.
-- 
1.9.1

[PATCH v5 2/4] time: Fix a bug in timekeeping_suspend() with no persistent clock

[Xunlei Pang]

From: Xunlei Pang <pang.xunlei@linaro.org>

When there's no persistent clock, normally timekeeping_suspend_time
should always be zero, but this can break in timekeeping_suspend().

At T1, there was a system suspend, so old_delta was assigned T1.
After some time, one time adjustment happened, and xtime got the
value of T1-dt(0s<dt<2s). Then, there comes another system suspend
soon after this adjustment, obviously we will get a small negative
delta_delta, resulting in a negative timekeeping_suspend_time.

This is problematic, when doing timekeeping_resume() if there is
no nonstop clocksource for example, it will hit the else leg and
inject the improper sleeptime which is the wrong logic.

So, we can solve this problem by only doing delta related code when
the persistent clock is existent. Actually the code only makes sense
for persistent clock cases.

This patch also improves the name of timekeeping_suspend_time.

Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
---
 kernel/time/timekeeping.c | 50 ++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 49b1643..2aae419 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1100,8 +1100,8 @@ void __init timekeeping_init(void)
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
 }
 
-/* time in seconds when suspend began */
-static struct timespec64 timekeeping_suspend_time;
+/* time in seconds when suspend began for persistent clock */
+static struct timespec64 persistent_clock_suspendtime;
 
 /**
  * __timekeeping_inject_sleeptime - Internal function to add sleep interval
@@ -1229,8 +1229,9 @@ static void timekeeping_resume(void)
 
 		ts_delta = ns_to_timespec64(nsec);
 		suspendtime_found = true;
-	} else if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) {
-		ts_delta = timespec64_sub(ts_new, timekeeping_suspend_time);
+	} else if (timespec64_compare(&ts_new,
+				    &persistent_clock_suspendtime) > 0) {
+		ts_delta = timespec64_sub(ts_new, persistent_clock_suspendtime);
 		suspendtime_found = true;
 	}
 
@@ -1262,14 +1263,15 @@ static int timekeeping_suspend(void)
 	struct timespec tmp;
 
 	read_persistent_clock(&tmp);
-	timekeeping_suspend_time = timespec_to_timespec64(tmp);
+	persistent_clock_suspendtime = timespec_to_timespec64(tmp);
 
 	/*
 	 * On some systems the persistent_clock can not be detected at
 	 * timekeeping_init by its return value, so if we see a valid
 	 * value returned, update the persistent_clock_exists flag.
 	 */
-	if (timekeeping_suspend_time.tv_sec || timekeeping_suspend_time.tv_nsec)
+	if (persistent_clock_suspendtime.tv_sec ||
+	    persistent_clock_suspendtime.tv_nsec)
 		persistent_clock_exist = true;
 
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
@@ -1277,24 +1279,28 @@ static int timekeeping_suspend(void)
 	timekeeping_forward_now(tk);
 	timekeeping_suspended = 1;
 
-	/*
-	 * To avoid drift caused by repeated suspend/resumes,
-	 * which each can add ~1 second drift error,
-	 * try to compensate so the difference in system time
-	 * and persistent_clock time stays close to constant.
-	 */
-	delta = timespec64_sub(tk_xtime(tk), timekeeping_suspend_time);
-	delta_delta = timespec64_sub(delta, old_delta);
-	if (abs(delta_delta.tv_sec)  >= 2) {
+	if (has_persistent_clock()) {
 		/*
-		 * if delta_delta is too large, assume time correction
-		 * has occured and set old_delta to the current delta.
+		 * To avoid drift caused by repeated suspend/resumes,
+		 * which each can add ~1 second drift error,
+		 * try to compensate so the difference in system time
+		 * and persistent_clock time stays close to constant.
 		 */
-		old_delta = delta;
-	} else {
-		/* Otherwise try to adjust old_system to compensate */
-		timekeeping_suspend_time =
-			timespec64_add(timekeeping_suspend_time, delta_delta);
+		delta = timespec64_sub(tk_xtime(tk),
+					persistent_clock_suspendtime);
+		delta_delta = timespec64_sub(delta, old_delta);
+		if (abs(delta_delta.tv_sec) >= 2) {
+			/*
+			 * if delta_delta is too large, assume time correction
+			 * has occurred and set old_delta to the current delta.
+			 */
+			old_delta = delta;
+		} else {
+			/* Otherwise try to adjust old_system to compensate */
+			persistent_clock_suspendtime =
+				timespec64_add(persistent_clock_suspendtime,
+					delta_delta);
+		}
 	}
 
 	timekeeping_update(tk, TK_MIRROR);
-- 
1.9.1

[PATCH v5 3/4] time: rtc: Don't bother into rtc_resume() for the nonstop clocksource

[Xunlei Pang]

From: Xunlei Pang <pang.xunlei@linaro.org>

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().

This patch also improves the name of persistent_clock_exist to
make it more grammatical.

Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
---
v5 changes:
Rename persistent_clock_exist to persistent_clock_exists.
Rename sleeptime_inject to sleeptime_injected.
Place these two variables together.

 drivers/rtc/class.c         |  4 +--
 include/linux/timekeeping.h |  9 ++----
 kernel/time/timekeeping.c   | 68 +++++++++++++++++++++++++++++++++------------
 3 files changed, 55 insertions(+), 26 deletions(-)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 472a5ad..74a943e 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -55,7 +55,7 @@ static int rtc_suspend(struct device *dev)
 	struct timespec64	delta, delta_delta;
 	int err;
 
-	if (has_persistent_clock())
+	if (timekeeping_rtc_skipsuspend())
 		return 0;
 
 	if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
@@ -102,7 +102,7 @@ static int rtc_resume(struct device *dev)
 	struct timespec64	sleep_time;
 	int err;
 
-	if (has_persistent_clock())
+	if (timekeeping_rtc_skipresume())
 		return 0;
 
 	rtc_hctosys_ret = -ENODEV;
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 3eaae47..7cbd518 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -242,6 +242,9 @@ static inline void timekeeping_clocktai(struct timespec *ts)
 /*
  * RTC specific
  */
+extern bool timekeeping_rtc_skipsuspend(void);
+extern bool timekeeping_rtc_skipresume(void);
+
 extern void timekeeping_inject_sleeptime64(struct timespec64 *delta);
 
 /*
@@ -253,14 +256,8 @@ extern void getnstime_raw_and_real(struct timespec *ts_raw,
 /*
  * Persistent clock related interfaces
  */
-extern bool persistent_clock_exist;
 extern int persistent_clock_is_local;
 
-static inline bool has_persistent_clock(void)
-{
-	return persistent_clock_exist;
-}
-
 extern void read_persistent_clock(struct timespec *ts);
 extern void read_boot_clock(struct timespec *ts);
 extern int update_persistent_clock(struct timespec now);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 2aae419..eab6b09 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -63,9 +63,6 @@ static struct tk_fast tk_fast_mono ____cacheline_aligned;
 /* flag for if timekeeping is suspended */
 int __read_mostly timekeeping_suspended;
 
-/* Flag for if there is a persistent clock on this platform */
-bool __read_mostly persistent_clock_exist = false;
-
 static inline void tk_normalize_xtime(struct timekeeper *tk)
 {
 	while (tk->tkr.xtime_nsec >= ((u64)NSEC_PER_SEC << tk->tkr.shift)) {
@@ -1045,6 +1042,12 @@ void __weak read_boot_clock(struct timespec *ts)
 	ts->tv_nsec = 0;
 }
 
+/* Flag for if timekeeping_resume() has injected sleeptime */
+static bool sleeptime_injected;
+
+/* Flag for if there is a persistent clock on this platform */
+static bool persistent_clock_exists;
+
 /*
  * timekeeping_init - Initializes the clocksource and common timekeeping values
  */
@@ -1064,7 +1067,7 @@ void __init timekeeping_init(void)
 		now.tv_sec = 0;
 		now.tv_nsec = 0;
 	} else if (now.tv_sec || now.tv_nsec)
-		persistent_clock_exist = true;
+		persistent_clock_exists = true;
 
 	read_boot_clock(&ts);
 	boot = timespec_to_timespec64(ts);
@@ -1127,11 +1130,47 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
 
 #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE)
 /**
+ * We have three kinds of time sources to use for sleep time
+ * injection, the preference order is:
+ * 1) non-stop clocksource
+ * 2) persistent clock (ie: RTC accessible when irqs are off)
+ * 3) RTC
+ *
+ * 1) and 2) are used by timekeeping, 3) by RTC subsystem.
+ * If system has neither 1) nor 2), 3) will be used finally.
+ *
+ *
+ * If timekeeping has injected sleeptime via either 1) or 2),
+ * 3) becomes needless, so in this case we don't need to call
+ * rtc_resume(), and this is what timekeeping_rtc_skipresume()
+ * means.
+ */
+bool timekeeping_rtc_skipresume(void)
+{
+	return sleeptime_injected;
+}
+
+/**
+ * 1) can be determined whether to use or not only when doing
+ * timekeeping_resume() which is invoked after rtc_suspend(),
+ * so we can't skip rtc_suspend() surely if system has 1).
+ *
+ * But if system has 2), 2) will definitely be used, so in this
+ * case we don't need to call rtc_suspend(), and this is what
+ * timekeeping_rtc_skipsuspend() means.
+ */
+bool timekeeping_rtc_skipsuspend(void)
+{
+	return persistent_clock_exists;
+}
+
+/**
  * 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.
@@ -1141,13 +1180,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);
 
@@ -1180,8 +1212,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_injected = false;
 	read_persistent_clock(&tmp);
 	ts_new = timespec_to_timespec64(tmp);
 
@@ -1228,14 +1260,14 @@ static void timekeeping_resume(void)
 		nsec += ((u64) cycle_delta * mult) >> shift;
 
 		ts_delta = ns_to_timespec64(nsec);
-		suspendtime_found = true;
+		sleeptime_injected = true;
 	} else if (timespec64_compare(&ts_new,
 				    &persistent_clock_suspendtime) > 0) {
 		ts_delta = timespec64_sub(ts_new, persistent_clock_suspendtime);
-		suspendtime_found = true;
+		sleeptime_injected = true;
 	}
 
-	if (suspendtime_found)
+	if (sleeptime_injected)
 		__timekeeping_inject_sleeptime(tk, &ts_delta);
 
 	/* Re-base the last cycle value */
@@ -1272,14 +1304,14 @@ static int timekeeping_suspend(void)
 	 */
 	if (persistent_clock_suspendtime.tv_sec ||
 	    persistent_clock_suspendtime.tv_nsec)
-		persistent_clock_exist = true;
+		persistent_clock_exists = true;
 
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
 	write_seqcount_begin(&tk_core.seq);
 	timekeeping_forward_now(tk);
 	timekeeping_suspended = 1;
 
-	if (has_persistent_clock()) {
+	if (persistent_clock_exists) {
 		/*
 		 * To avoid drift caused by repeated suspend/resumes,
 		 * which each can add ~1 second drift error,
-- 
1.9.1

[PATCH v5 4/4] rtc: Remove redundant rtc_valid_tm() from rtc_resume()

[Xunlei Pang]

From: Xunlei Pang <pang.xunlei@linaro.org>

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 74a943e..c29ba7e 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

Re: [PATCH v5 2/4] time: Fix a bug in timekeeping_suspend() with no persistent clock

[John Stultz]

On Sun, Mar 8, 2015 at 11:27 PM, Xunlei Pang <xlpang@126.com> wrote:
> From: Xunlei Pang <pang.xunlei@linaro.org>
>
> When there's no persistent clock, normally timekeeping_suspend_time
> should always be zero, but this can break in timekeeping_suspend().
>
> At T1, there was a system suspend, so old_delta was assigned T1.
> After some time, one time adjustment happened, and xtime got the
> value of T1-dt(0s<dt<2s). Then, there comes another system suspend
> soon after this adjustment, obviously we will get a small negative
> delta_delta, resulting in a negative timekeeping_suspend_time.
>
> This is problematic, when doing timekeeping_resume() if there is
> no nonstop clocksource for example, it will hit the else leg and
> inject the improper sleeptime which is the wrong logic.
>
> So, we can solve this problem by only doing delta related code when
> the persistent clock is existent. Actually the code only makes sense
> for persistent clock cases.
>
> This patch also improves the name of timekeeping_suspend_time.

So re-reviewing this here. I really think the renaming here isn't an
improvement. It does clarify that its related only to the persistent
clock, but the resulting code is much uglier due to the 80col
reformatting you've done.

Can we drop the rename, which should make the fix in this code more clear?

thanks
-john

Re: [PATCH v5 1/4] time: Add needed macros for timekeeping_inject_sleeptime64()

[John Stultz]

On Sun, Mar 8, 2015 at 11:27 PM, Xunlei Pang <xlpang@126.com> wrote:
> From: Xunlei Pang <pang.xunlei@linaro.org>
>
> timekeeping_inject_sleeptime64() is only used by RTC suspend/resume,
> so embrace it in RTC related macros.
>
> Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>

Queued this patch with an improved commit message.

thanks
-john

Re: [PATCH v5 4/4] rtc: Remove redundant rtc_valid_tm() from rtc_resume()

[John Stultz]

On Sun, Mar 8, 2015 at 11:27 PM, Xunlei Pang <xlpang@126.com> wrote:
> From: Xunlei Pang <pang.xunlei@linaro.org>
>
> 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>

I've queued this patch, but the two prior patches in this series I left out.

thanks
-john