linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] time, Fix setting of hardware clock in NTP code
@ 2013-02-08 12:55 Prarit Bhargava
  2013-02-08 21:46 ` John Stultz
  0 siblings, 1 reply; 6+ messages in thread
From: Prarit Bhargava @ 2013-02-08 12:55 UTC (permalink / raw)
  To: linux-kernel; +Cc: Prarit Bhargava, John Stultz, Thomas Gleixner

At init time, if the system time is "warped" forward in warp_clock()
it will differ from the hardware clock by sys_tz.tz_minuteswest.  This time
difference is not taken into account when ntp updates the hardware clock,
and this causes the system time to jump forward by this offset every reboot.

The kernel must take this offset into account when writing the system time
to the hardware clock in the ntp code.  This patch adds
persistent_clock_is_local which indicates that an offset has been applied
in warp_clock() and accounts for the "warp" before writing the hardware
clock.

x86 does not have this problem as rtc writes are software limited to a
+/-15 minute window relative to the current rtc time.  Other arches, such
as powerpc, however do a full synchronization of the system time to the
rtc and will see this problem.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/time.h |    1 +
 kernel/time.c        |    8 ++++++++
 kernel/time/ntp.c    |    8 ++++++--
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/linux/time.h b/include/linux/time.h
index 4d358e9..f3646b6 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -117,6 +117,7 @@ static inline bool timespec_valid_strict(const struct timespec *ts)
 
 extern void read_persistent_clock(struct timespec *ts);
 extern void read_boot_clock(struct timespec *ts);
+extern int persistent_clock_is_local;
 extern int update_persistent_clock(struct timespec now);
 void timekeeping_init(void);
 extern int timekeeping_suspended;
diff --git a/kernel/time.c b/kernel/time.c
index d226c6a..c2a27dd 100644
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -115,6 +115,12 @@ SYSCALL_DEFINE2(gettimeofday, struct timeval __user *, tv,
 }
 
 /*
+ * Indicates if there is an offset between the system clock and the hardware
+ * clock/persistent clock/rtc.
+ */
+int persistent_clock_is_local;
+
+/*
  * Adjust the time obtained from the CMOS to be UTC time instead of
  * local time.
  *
@@ -135,6 +141,8 @@ static inline void warp_clock(void)
 	struct timespec adjust;
 
 	adjust = current_kernel_time();
+	if (sys_tz.tz_minuteswest != 0)
+		persistent_clock_is_local = 1;
 	adjust.tv_sec += sys_tz.tz_minuteswest * 60;
 	do_settimeofday(&adjust);
 }
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 24174b4..e98f6b7 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -510,8 +510,12 @@ static void sync_cmos_clock(struct work_struct *work)
 	}
 
 	getnstimeofday(&now);
-	if (abs(now.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2)
-		fail = update_persistent_clock(now);
+	if (abs(now.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2) {
+		struct timespec adjust = now;
+		if (persistent_clock_is_local)
+			adjust.tv_sec -= (sys_tz.tz_minuteswest * 60);
+		fail = update_persistent_clock(adjust);
+	}
 
 	next.tv_nsec = (NSEC_PER_SEC / 2) - now.tv_nsec - (TICK_NSEC / 2);
 	if (next.tv_nsec <= 0)
-- 
1.7.9.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] time, Fix setting of hardware clock in NTP code
  2013-02-08 12:55 [PATCH] time, Fix setting of hardware clock in NTP code Prarit Bhargava
@ 2013-02-08 21:46 ` John Stultz
  2013-02-08 22:59   ` Prarit Bhargava
  0 siblings, 1 reply; 6+ messages in thread
From: John Stultz @ 2013-02-08 21:46 UTC (permalink / raw)
  To: Prarit Bhargava; +Cc: linux-kernel, Thomas Gleixner

On Fri, Feb 8, 2013 at 4:55 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index 24174b4..e98f6b7 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -510,8 +510,12 @@ static void sync_cmos_clock(struct work_struct *work)
>         }
>
>         getnstimeofday(&now);
> -       if (abs(now.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2)
> -               fail = update_persistent_clock(now);
> +       if (abs(now.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2) {
> +               struct timespec adjust = now;
> +               if (persistent_clock_is_local)
> +                       adjust.tv_sec -= (sys_tz.tz_minuteswest * 60);
> +               fail = update_persistent_clock(adjust);
> +       }
>
Do you mind reworking this patch on top of tip/timers/core?  There's
some recent changes that interact here.

thanks
-john

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] time, Fix setting of hardware clock in NTP code
  2013-02-08 21:46 ` John Stultz
@ 2013-02-08 22:59   ` Prarit Bhargava
  2013-02-08 23:12     ` John Stultz
  0 siblings, 1 reply; 6+ messages in thread
From: Prarit Bhargava @ 2013-02-08 22:59 UTC (permalink / raw)
  To: linux-kernel; +Cc: Prarit Bhargava, John Stultz, Thomas Gleixner

At init time, if the system time is "warped" forward in warp_clock()
it will differ from the hardware clock by sys_tz.tz_minuteswest.  This time
difference is not taken into account when ntp updates the hardware clock,
and this causes the system time to jump forward by this offset every reboot.

The kernel must take this offset into account when writing the system time
to the hardware clock in the ntp code.  This patch adds
persistent_clock_is_local which indicates that an offset has been applied
in warp_clock() and accounts for the "warp" before writing the hardware
clock.

x86 does not have this problem as rtc writes are software limited to a
+/-15 minute window relative to the current rtc time.  Other arches, such
as powerpc, however do a full synchronization of the system time to the
rtc and will see this problem.

[v2]: generated against tip/timers/core

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/time.h | 1 +
 kernel/time.c        | 8 ++++++++
 kernel/time/ntp.c    | 8 ++++++--
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/linux/time.h b/include/linux/time.h
index 476e1d7..a3ab6a8 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -128,6 +128,7 @@ static inline bool has_persistent_clock(void)
 
 extern void read_persistent_clock(struct timespec *ts);
 extern void read_boot_clock(struct timespec *ts);
+extern int persistent_clock_is_local;
 extern int update_persistent_clock(struct timespec now);
 void timekeeping_init(void);
 extern int timekeeping_suspended;
diff --git a/kernel/time.c b/kernel/time.c
index d226c6a..c2a27dd 100644
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -115,6 +115,12 @@ SYSCALL_DEFINE2(gettimeofday, struct timeval __user *, tv,
 }
 
 /*
+ * Indicates if there is an offset between the system clock and the hardware
+ * clock/persistent clock/rtc.
+ */
+int persistent_clock_is_local;
+
+/*
  * Adjust the time obtained from the CMOS to be UTC time instead of
  * local time.
  *
@@ -135,6 +141,8 @@ static inline void warp_clock(void)
 	struct timespec adjust;
 
 	adjust = current_kernel_time();
+	if (sys_tz.tz_minuteswest != 0)
+		persistent_clock_is_local = 1;
 	adjust.tv_sec += sys_tz.tz_minuteswest * 60;
 	do_settimeofday(&adjust);
 }
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 313b161..b10a42b 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -511,13 +511,17 @@ static void sync_cmos_clock(struct work_struct *work)
 
 	getnstimeofday(&now);
 	if (abs(now.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2) {
+		struct timespec adjust = now;
+
 		fail = -ENODEV;
+		if (persistent_clock_is_local)
+			adjust.tv_sec -= (sys_tz.tz_minuteswest * 60);
 #ifdef CONFIG_GENERIC_CMOS_UPDATE
-		fail = update_persistent_clock(now);
+		fail = update_persistent_clock(adjust);
 #endif
 #ifdef CONFIG_RTC_SYSTOHC
 		if (fail == -ENODEV)
-			fail = rtc_set_ntp_time(now);
+			fail = rtc_set_ntp_time(adjust);
 #endif
 	}
 
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] time, Fix setting of hardware clock in NTP code
  2013-02-08 22:59   ` Prarit Bhargava
@ 2013-02-08 23:12     ` John Stultz
  2013-02-08 23:44       ` Prarit Bhargava
  0 siblings, 1 reply; 6+ messages in thread
From: John Stultz @ 2013-02-08 23:12 UTC (permalink / raw)
  To: Prarit Bhargava; +Cc: linux-kernel, Thomas Gleixner

On 02/08/2013 02:59 PM, Prarit Bhargava wrote:
> At init time, if the system time is "warped" forward in warp_clock()
> it will differ from the hardware clock by sys_tz.tz_minuteswest.  This time
> difference is not taken into account when ntp updates the hardware clock,
> and this causes the system time to jump forward by this offset every reboot.
>
> The kernel must take this offset into account when writing the system time
> to the hardware clock in the ntp code.  This patch adds
> persistent_clock_is_local which indicates that an offset has been applied
> in warp_clock() and accounts for the "warp" before writing the hardware
> clock.
>
> x86 does not have this problem as rtc writes are software limited to a
> +/-15 minute window relative to the current rtc time.  Other arches, such
> as powerpc, however do a full synchronization of the system time to the
> rtc and will see this problem.

Ok, I've got this queued in my tree. What sort of testing did you do 
with it?

I want to make sure we don't run into any bad interactions with the 
existing 15min cap on x86.

thanks
-john


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] time, Fix setting of hardware clock in NTP code
  2013-02-08 23:12     ` John Stultz
@ 2013-02-08 23:44       ` Prarit Bhargava
  2013-02-08 23:50         ` John Stultz
  0 siblings, 1 reply; 6+ messages in thread
From: Prarit Bhargava @ 2013-02-08 23:44 UTC (permalink / raw)
  To: John Stultz; +Cc: linux-kernel, Thomas Gleixner



On 02/08/2013 06:12 PM, John Stultz wrote:
> On 02/08/2013 02:59 PM, Prarit Bhargava wrote:

> 
> Ok, I've got this queued in my tree. What sort of testing did you do with it?
> 
> I want to make sure we don't run into any bad interactions with the existing
> 15min cap on x86.

John,

I did the following:

I used powerpc pseries systems and tested this using both positive and negative
values of sys_tz.minuteswest, with both UTC and LOCAL in /etc/adjtime.  I dumped
values of 'hwclock -D' and date and confirmed that I no longer see time
increasing by sys_tz.minuteswest each reboot.

I also tested x86 32-bit and 64-bit as a sanity check and verified that the
current behaviour on those arches is the same; ie) I don't see *any* impact to
the x86 rtc.  I dumped values of 'hwclock -D' and date, and again confirmed that
I see no differences in values.  I did that with both UTC and LOCAL.

I also tested a powerpc box and set the hwclock (via BIOS) back to Dec 6 2012 to
see what would happen when I enabled ntp.  The system booted, set the system
time to Dec 6 2012, and then properly ended up with both system time AND hwclock
as Feb 8 2013 after systemd init .... (The *exact* time-of-day was correct as
well.  I just can't remember the time I did it ;) )

And I did the same thing (adjusting the BIOS date back) on x86. I only see the
hours and minutes change, as we expect.  The year, month, day are unaffected
with both UTC and LOCAL.

tl;dr  Yup.  Tested as much as I could think of doing before submitting.  Tested
on a both x86, powerpc.  Fixed the bug on powerpc.  No change in behavior seen
with x86.

If you want me to do some other test I certainly can give it a shot.

P.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] time, Fix setting of hardware clock in NTP code
  2013-02-08 23:44       ` Prarit Bhargava
@ 2013-02-08 23:50         ` John Stultz
  0 siblings, 0 replies; 6+ messages in thread
From: John Stultz @ 2013-02-08 23:50 UTC (permalink / raw)
  To: Prarit Bhargava; +Cc: linux-kernel, Thomas Gleixner

On 02/08/2013 03:44 PM, Prarit Bhargava wrote:
>
> On 02/08/2013 06:12 PM, John Stultz wrote:
>> On 02/08/2013 02:59 PM, Prarit Bhargava wrote:
>> Ok, I've got this queued in my tree. What sort of testing did you do with it?
>>
>> I want to make sure we don't run into any bad interactions with the existing
>> 15min cap on x86.
> John,
>
> I did the following:
>
> I used powerpc pseries systems and tested this using both positive and negative
> values of sys_tz.minuteswest, with both UTC and LOCAL in /etc/adjtime.  I dumped
> values of 'hwclock -D' and date and confirmed that I no longer see time
> increasing by sys_tz.minuteswest each reboot.
>
> I also tested x86 32-bit and 64-bit as a sanity check and verified that the
> current behaviour on those arches is the same; ie) I don't see *any* impact to
> the x86 rtc.  I dumped values of 'hwclock -D' and date, and again confirmed that
> I see no differences in values.  I did that with both UTC and LOCAL.
>
> I also tested a powerpc box and set the hwclock (via BIOS) back to Dec 6 2012 to
> see what would happen when I enabled ntp.  The system booted, set the system
> time to Dec 6 2012, and then properly ended up with both system time AND hwclock
> as Feb 8 2013 after systemd init .... (The *exact* time-of-day was correct as
> well.  I just can't remember the time I did it ;) )
>
> And I did the same thing (adjusting the BIOS date back) on x86. I only see the
> hours and minutes change, as we expect.  The year, month, day are unaffected
> with both UTC and LOCAL.
>
> tl;dr  Yup.  Tested as much as I could think of doing before submitting.  Tested
> on a both x86, powerpc.  Fixed the bug on powerpc.  No change in behavior seen
> with x86.
Great! This is perfect!

Thanks for being so thorough!
-john


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-02-08 23:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-08 12:55 [PATCH] time, Fix setting of hardware clock in NTP code Prarit Bhargava
2013-02-08 21:46 ` John Stultz
2013-02-08 22:59   ` Prarit Bhargava
2013-02-08 23:12     ` John Stultz
2013-02-08 23:44       ` Prarit Bhargava
2013-02-08 23:50         ` John Stultz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).