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

[Andrew Morton]

On Wed, 28 May 2008 19:15:41 +0100 (BST) "Maciej W. Rozycki" <macro@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
> 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?

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