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

2015-01-21 Thread 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-01-21 Thread 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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-01-20 Thread 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 
---
 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();
ts_new = timespec_to_timespec64(tmp);
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-01-20 Thread 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.xun...@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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/