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

[PATCH 1/3] time: Don't bother to run rtc_resume() for 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>
---
 drivers/rtc/class.c         |  2 +-
 include/linux/timekeeping.h | 11 +++++++++++
 kernel/time/timekeeping.c   |  3 ++-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 472a5ad..c8f35a5 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 (rtc_resume_skip())
 		return 0;
 
 	rtc_hctosys_ret = -ENODEV;
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 9b63d13..6f3283b 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -238,6 +238,17 @@ extern void getnstime_raw_and_real(struct timespec *ts_raw,
  */
 extern bool persistent_clock_exist;
 extern int persistent_clock_is_local;
+extern bool suspendtime_found;
+
+#ifdef CONFIG_RTC_LIB
+/* Used by rtc_resume() */
+static inline bool rtc_resume_skip(void)
+{
+	return suspendtime_found;
+}
+#else
+static inline void rtc_resume_skip(void) { }
+#endif
 
 static inline bool has_persistent_clock(void)
 {
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 6a93185..586bb5c 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 suspendtime_found;
 
 static inline void tk_normalize_xtime(struct timekeeper *tk)
 {
@@ -1178,8 +1179,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;
 
+	suspendtime_found = false;
 	read_persistent_clock(&tmp);
 	ts_new = timespec_to_timespec64(tmp);
 
-- 
1.9.1

[PATCH 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 c8f35a5..5953225 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 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.

So add a comment to indicate that if have this flag set, you are
aware that this nonstop clocksource won't wrap during suspension.

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..20bca76 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
+
+/*
+ * Setting this flag, also means 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 1/3] time: Don't bother to run rtc_resume() for nonstop clocksource

[Thomas Gleixner]

On Tue, 20 Jan 2015, Xunlei Pang wrote:
> ---
>  drivers/rtc/class.c         |  2 +-
>  include/linux/timekeeping.h | 11 +++++++++++
>  kernel/time/timekeeping.c   |  3 ++-
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index 472a5ad..c8f35a5 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 (rtc_resume_skip())
>  		return 0;
>  
>  	rtc_hctosys_ret = -ENODEV;
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index 9b63d13..6f3283b 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -238,6 +238,17 @@ extern void getnstime_raw_and_real(struct timespec *ts_raw,
>   */
>  extern bool persistent_clock_exist;
>  extern int persistent_clock_is_local;
> +extern bool suspendtime_found;
> +
> +#ifdef CONFIG_RTC_LIB
> +/* Used by rtc_resume() */
> +static inline bool rtc_resume_skip(void)
> +{
> +	return suspendtime_found;
> +}
> +#else
> +static inline void rtc_resume_skip(void) { }
> +#endif

What's the point of this ifdeffery and the inline which is different
than the real one?

>  static inline bool has_persistent_clock(void)
>  {
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 6a93185..586bb5c 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 suspendtime_found;

This is horrible. The variable name was ugly already, so now you just
make it global without any thought. Global variables are supposed to
have a proper name which allows us to figure out what they are used
for and to which susbsystem they belong.

Aside of that we have now two different mechanisms to guard
timekeeping_inject_sleeptime64(). One in the rtc code and one in the
timekeeping code itself. This is just wrong.

Thanks,

	tglx