Re: [patch 02/23] GTOD: persistent clock support, core

[Andrew Morton <akpm@osdl.org>]

On Fri, 29 Sep 2006 23:58:21 -0000
Thomas Gleixner <tglx@linutronix.de> wrote:

> From: John Stultz <johnstul@us.ibm.com>
> 
> persistent clock support: do proper timekeeping across suspend/resume.

How?

> +/* Weak dummy function for arches that do not yet support it.
> + * XXX - Do be sure to remove it once all arches implement it.
> + */
> +unsigned long __attribute__((weak)) read_persistent_clock(void)
> +{
> +	return 0;
> +}

Seconds?  microseconds?  jiffies?  walltime?  uptime?

Needs some comments.


>  void __init timekeeping_init(void)
>  {
> -	unsigned long flags;
> +	unsigned long flags, sec = read_persistent_clock();

So it apparently returns seconds-since-epoch?

If so, why?

>  	write_seqlock_irqsave(&xtime_lock, flags);
>  
> @@ -758,11 +769,18 @@ void __init timekeeping_init(void)
>  	clocksource_calculate_interval(clock, tick_nsec);
>  	clock->cycle_last = clocksource_read(clock);
>  
> +	xtime.tv_sec = sec;
> +	xtime.tv_nsec = (jiffies % HZ) * (NSEC_PER_SEC / HZ);

Why is it valid to take the second from the persistent clock and the
fraction-of-a-second from jiffies?  Some comments describing the
implementation would improve its understandability and maintainability.

This statement can set xtime.tv_nsec to a value >= NSEC_PER_SEC.  Should it
not be normalised?

> +	set_normalized_timespec(&wall_to_monotonic,
> +		-xtime.tv_sec, -xtime.tv_nsec);
> +
>  	write_sequnlock_irqrestore(&xtime_lock, flags);
>  }
>  
>  
>  static int timekeeping_suspended;
> +static unsigned long timekeeping_suspend_time;

In what units?

> +
>  /**
>   * timekeeping_resume - Resumes the generic timekeeping subsystem.
>   * @dev:	unused
> @@ -773,14 +791,23 @@ static int timekeeping_suspended;
>   */
>  static int timekeeping_resume(struct sys_device *dev)
>  {
> -	unsigned long flags;
> +	unsigned long flags, now = read_persistent_clock();

Would whoever keeps doing that please stop it?  This:

	unsigned long flags;
	unsigned long now = read_persistent_clock();

is more readable and makes for more readable patches in the future.

>  	write_seqlock_irqsave(&xtime_lock, flags);
> -	/* restart the last cycle value */
> +
> +	if (now && (now > timekeeping_suspend_time)) {
> +		unsigned long sleep_length = now - timekeeping_suspend_time;
> +		xtime.tv_sec += sleep_length;
> +		jiffies_64 += sleep_length * HZ;

sleep_length will overflow if we slept for more than 49 days, and HZ=1000.

> +	}
> +	/* re-base the last cycle value */
>  	clock->cycle_last = clocksource_read(clock);
>  	clock->error = 0;
>  	timekeeping_suspended = 0;
>  	write_sequnlock_irqrestore(&xtime_lock, flags);
> +
> +	hrtimer_notify_resume();
> +
>  	return 0;
>  }
>  
> @@ -790,6 +817,7 @@ static int timekeeping_suspend(struct sy
>  
>  	write_seqlock_irqsave(&xtime_lock, flags);
>  	timekeeping_suspended = 1;
> +	timekeeping_suspend_time = read_persistent_clock();
>  	write_sequnlock_irqrestore(&xtime_lock, flags);
>  	return 0;
>  }
> 
> --