[PATCH] NTP: Let update_persistent_clock() sleep

[PATCH] NTP: Let update_persistent_clock() sleep

[Maciej W. Rozycki]

 This is a change that makes the 11-minute RTC update be run in the
process context.  This is so that update_persistent_clock() can sleep,
which may be required for certain types of RTC hardware -- most notably
I2C devices.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
Hello,

 After the initial enthusiasm, I am not sure how my series of patches to
let read_persistent_clock() and update_persistent_clock() use the class
RTC subsystem is going to be handled.  As keeping the order of patches is 
required to avoid breakage in various places, I will try to coordinate the 
changes and submit them one by one as the dependencies get satisfied.  I 
hope this is OK and will take less than half a year. ;)

 Given this one applies to generic code and is required by all the other
changes, while not requiring anything and not meant to break anything, ;) 
I think this is ready to go.  It may be worth testing that moving the 
function into the process context does not cause any regressions for some 
obscure configuration.

 I am not sure who actually claims maintenance of kernel/time/ntp.c, but
it looks, Thomas, you seem to be our current time overseer -- could you
please speak out on this change?  I'd like this change to get applied
somewhere reasonable -- is it -mm?

  Maciej

patch-2.6.26-rc1-20080505-sync-cmos-work-0
diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/kernel/time/ntp.c linux-2.6.26-rc1-20080505/kernel/time/ntp.c
--- linux-2.6.26-rc1-20080505.macro/kernel/time/ntp.c	2008-05-05 02:56:03.000000000 +0000
+++ linux-2.6.26-rc1-20080505/kernel/time/ntp.c	2008-05-05 21:10:50.000000000 +0000
@@ -3,6 +3,8 @@
  *
  * NTP state machine interfaces and logic.
  *
+ * Copyright (c) 2008  Maciej W. Rozycki
+ *
  * This code was mainly moved from kernel/timer.c and kernel/time.c
  * Please see those files for relevant copyright info and historical
  * changelogs.
@@ -17,6 +19,7 @@
 #include <linux/capability.h>
 #include <linux/math64.h>
 #include <linux/clocksource.h>
+#include <linux/workqueue.h>
 #include <asm/timex.h>
 
 /*
@@ -218,11 +221,13 @@ void second_overflow(void)
 /* Disable the cmos update - used by virtualization and embedded */
 int no_sync_cmos_clock  __read_mostly;
 
-static void sync_cmos_clock(unsigned long dummy);
+static void sync_cmos_clock(unsigned long data);
+static void do_sync_cmos_clock(struct work_struct *work);
 
 static DEFINE_TIMER(sync_cmos_timer, sync_cmos_clock, 0, 0);
+static DECLARE_WORK(sync_cmos_work, do_sync_cmos_clock);
 
-static void sync_cmos_clock(unsigned long dummy)
+static void do_sync_cmos_clock(struct work_struct *work)
 {
 	struct timespec now, next;
 	int fail = 1;
@@ -261,6 +266,12 @@ static void sync_cmos_clock(unsigned lon
 	mod_timer(&sync_cmos_timer, jiffies + timespec_to_jiffies(&next));
 }
 
+static void sync_cmos_clock(unsigned long data)
+{
+	/* Some implementations of update_persistent_clock() may sleep.  */
+	schedule_work(&sync_cmos_work);
+}
+
 static void notify_cmos_timer(void)
 {
 	if (!no_sync_cmos_clock)

Re: [PATCH] NTP: Let update_persistent_clock() sleep

[Rik van Riel]

On Wed, 28 May 2008 19:15:41 +0100 (BST)
"Maciej W. Rozycki" <macro@linux-mips.org> wrote:

>  This is a change that makes the 11-minute RTC update be run in the
> process context.  This is so that update_persistent_clock() can sleep,
> which may be required for certain types of RTC hardware -- most notably
> I2C devices.
> 
> Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>

Acked-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed.

Re: [PATCH] NTP: Let update_persistent_clock() sleep

[Andrew Morton]

On Wed, 28 May 2008 19:15:41 +0100 (BST) "Maciej W. Rozycki" <macro@linux-mips.org> wrote:

>  This is a change that makes the 11-minute RTC update be run in the
> process context.  This is so that update_persistent_clock() can sleep,
> which may be required for certain types of RTC hardware -- most notably
> I2C devices.
> 
> Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
> ---
> Hello,
> 
>  After the initial enthusiasm, I am not sure how my series of patches to
> let read_persistent_clock() and update_persistent_clock() use the class
> RTC subsystem is going to be handled.  As keeping the order of patches is 
> required to avoid breakage in various places, I will try to coordinate the 
> changes and submit them one by one as the dependencies get satisfied.  I 
> hope this is OK and will take less than half a year. ;)
> 
>  Given this one applies to generic code and is required by all the other
> changes, while not requiring anything and not meant to break anything, ;) 
> I think this is ready to go.  It may be worth testing that moving the 
> function into the process context does not cause any regressions for some 
> obscure configuration.
> 
>  I am not sure who actually claims maintenance of kernel/time/ntp.c, but
> it looks, Thomas, you seem to be our current time overseer -- could you
> please speak out on this change?  I'd like this change to get applied
> somewhere reasonable -- is it -mm?

Roman does most of the NTP work afaik.  I consider Thomas's git-hrt
tree to be the route via which NTP changes get into linux-next and
mainline.

> patch-2.6.26-rc1-20080505-sync-cmos-work-0
> diff -up --recursive --new-file linux-2.6.26-rc1-20080505.macro/kernel/time/ntp.c linux-2.6.26-rc1-20080505/kernel/time/ntp.c
> --- linux-2.6.26-rc1-20080505.macro/kernel/time/ntp.c	2008-05-05 02:56:03.000000000 +0000
> +++ linux-2.6.26-rc1-20080505/kernel/time/ntp.c	2008-05-05 21:10:50.000000000 +0000
> @@ -3,6 +3,8 @@
>   *
>   * NTP state machine interfaces and logic.
>   *
> + * Copyright (c) 2008  Maciej W. Rozycki
> + *
>   * This code was mainly moved from kernel/timer.c and kernel/time.c
>   * Please see those files for relevant copyright info and historical
>   * changelogs.
> @@ -17,6 +19,7 @@
>  #include <linux/capability.h>
>  #include <linux/math64.h>
>  #include <linux/clocksource.h>
> +#include <linux/workqueue.h>
>  #include <asm/timex.h>
>  
>  /*
> @@ -218,11 +221,13 @@ void second_overflow(void)
>  /* Disable the cmos update - used by virtualization and embedded */
>  int no_sync_cmos_clock  __read_mostly;
>  
> -static void sync_cmos_clock(unsigned long dummy);
> +static void sync_cmos_clock(unsigned long data);
> +static void do_sync_cmos_clock(struct work_struct *work);
>  
>  static DEFINE_TIMER(sync_cmos_timer, sync_cmos_clock, 0, 0);
> +static DECLARE_WORK(sync_cmos_work, do_sync_cmos_clock);
>  
> -static void sync_cmos_clock(unsigned long dummy)
> +static void do_sync_cmos_clock(struct work_struct *work)
>  {
>  	struct timespec now, next;
>  	int fail = 1;
> @@ -261,6 +266,12 @@ static void sync_cmos_clock(unsigned lon
>  	mod_timer(&sync_cmos_timer, jiffies + timespec_to_jiffies(&next));
>  }
>  
> +static void sync_cmos_clock(unsigned long data)
> +{
> +	/* Some implementations of update_persistent_clock() may sleep.  */
> +	schedule_work(&sync_cmos_work);
> +}
> +
>  static void notify_cmos_timer(void)
>  {
>  	if (!no_sync_cmos_clock)

OK, that timer code in there now officially makes my brain hurt.

Is it as simple as it could be?

Would schedule_delayed_work() make my brain feel better?

Re: [PATCH] NTP: Let update_persistent_clock() sleep

[Maciej W. Rozycki]

On Wed, 28 May 2008, Andrew Morton wrote:

> Roman does most of the NTP work afaik.  I consider Thomas's git-hrt
> tree to be the route via which NTP changes get into linux-next and
> mainline.

 Thanks for the clarification.

> OK, that timer code in there now officially makes my brain hurt.

 Ouch, it would be a misery to waste such a perfectly good brain!

> Is it as simple as it could be?
> 
> Would schedule_delayed_work() make my brain feel better?

 Hmm, I have missed this interface somehow.  I am sending an update 
straight away.

  Maciej

Re: [PATCH] NTP: Let update_persistent_clock() sleep

[Roman Zippel]

Hi,

On Wed, 28 May 2008, Andrew Morton wrote:

> >  I am not sure who actually claims maintenance of kernel/time/ntp.c, but
> > it looks, Thomas, you seem to be our current time overseer -- could you
> > please speak out on this change?  I'd like this change to get applied
> > somewhere reasonable -- is it -mm?
> 
> Roman does most of the NTP work afaik.  I consider Thomas's git-hrt
> tree to be the route via which NTP changes get into linux-next and
> mainline.

That function is a little misplaced, we already have a driver/rtc dir, 
where this should go in the long term and ntp.c only providing the 
trigger, that time is stable. There it would also be possible to better 
take into account any quirks needed to update the chip.

bye, Roman

Re: [PATCH] NTP: Let update_persistent_clock() sleep

[Maciej W. Rozycki]

Hi,

> > Roman does most of the NTP work afaik.  I consider Thomas's git-hrt
> > tree to be the route via which NTP changes get into linux-next and
> > mainline.
> 
> That function is a little misplaced, we already have a driver/rtc dir, 
> where this should go in the long term and ntp.c only providing the 
> trigger, that time is stable. There it would also be possible to better 
> take into account any quirks needed to update the chip.

 I posted a separate change to implement a backend using the RTC class
device.  I think this is the right solution till all the platforms are
moved away from legacy RTC drivers.

 I think you are right about the long-term implications and apart from any
possible quirks I think the interface could get improved as there are RTC
chips we support nowadays that provide sub-second resolution.

  Maciej

Re: [PATCH] NTP: Let update_persistent_clock() sleep

[Andrew Morton]

On Mon, 9 Jun 2008 19:18:52 +0100 (BST)
"Maciej W. Rozycki" <macro@linux-mips.org> wrote:

> Hi,
> 
> > > Roman does most of the NTP work afaik.  I consider Thomas's git-hrt
> > > tree to be the route via which NTP changes get into linux-next and
> > > mainline.
> > 
> > That function is a little misplaced, we already have a driver/rtc dir, 
> > where this should go in the long term and ntp.c only providing the 
> > trigger, that time is stable. There it would also be possible to better 
> > take into account any quirks needed to update the chip.
> 
>  I posted a separate change to implement a backend using the RTC class
> device.  I think this is the right solution till all the platforms are
> moved away from legacy RTC drivers.
> 
>  I think you are right about the long-term implications and apart from any
> possible quirks I think the interface could get improved as there are RTC
> chips we support nowadays that provide sub-second resolution.
> 


Are there any objections to the patch under discussion?

Thanks.


From: "Maciej W. Rozycki" <macro@linux-mips.org>

This is a change that makes the 11-minute RTC update be run in the process
context.  This is so that update_persistent_clock() can sleep, which may
be required for certain types of RTC hardware -- most notably I2C devices.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Roman Zippel <zippel@linux-m68k.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: David Brownell <david-b@pacbell.net>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/time/ntp.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff -puN kernel/time/ntp.c~ntp-let-update_persistent_clock-sleep kernel/time/ntp.c
--- a/kernel/time/ntp.c~ntp-let-update_persistent_clock-sleep
+++ a/kernel/time/ntp.c
@@ -10,13 +10,13 @@
 
 #include <linux/mm.h>
 #include <linux/time.h>
-#include <linux/timer.h>
 #include <linux/timex.h>
 #include <linux/jiffies.h>
 #include <linux/hrtimer.h>
 #include <linux/capability.h>
 #include <linux/math64.h>
 #include <linux/clocksource.h>
+#include <linux/workqueue.h>
 #include <asm/timex.h>
 
 /*
@@ -218,11 +218,11 @@ void second_overflow(void)
 /* Disable the cmos update - used by virtualization and embedded */
 int no_sync_cmos_clock  __read_mostly;
 
-static void sync_cmos_clock(unsigned long dummy);
+static void sync_cmos_clock(struct work_struct *work);
 
-static DEFINE_TIMER(sync_cmos_timer, sync_cmos_clock, 0, 0);
+static DECLARE_DELAYED_WORK(sync_cmos_work, sync_cmos_clock);
 
-static void sync_cmos_clock(unsigned long dummy)
+static void sync_cmos_clock(struct work_struct *work)
 {
 	struct timespec now, next;
 	int fail = 1;
@@ -258,13 +258,13 @@ static void sync_cmos_clock(unsigned lon
 		next.tv_sec++;
 		next.tv_nsec -= NSEC_PER_SEC;
 	}
-	mod_timer(&sync_cmos_timer, jiffies + timespec_to_jiffies(&next));
+	schedule_delayed_work(&sync_cmos_work, timespec_to_jiffies(&next));
 }
 
 static void notify_cmos_timer(void)
 {
 	if (!no_sync_cmos_clock)
-		mod_timer(&sync_cmos_timer, jiffies + 1);
+		schedule_delayed_work(&sync_cmos_work, 0);
 }
 
 #else
_

Re: [PATCH] NTP: Let update_persistent_clock() sleep

[Alessandro Zummo]

On Mon, 9 Jun 2008 14:59:23 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> Are there any objections to the patch under discussion?
> 
> Thanks.
> 
> 
> From: "Maciej W. Rozycki" <macro@linux-mips.org>
> 
> This is a change that makes the 11-minute RTC update be run in the process
> context.  This is so that update_persistent_clock() can sleep, which may
> be required for certain types of RTC hardware -- most notably I2C devices.
> 
> Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Roman Zippel <zippel@linux-m68k.org>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: David Brownell <david-b@pacbell.net>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---

 Acked-by: Alessandro Zummo <a.zummo@towertech.it>


-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it