Re: [PATCH] update clocksource raw_time in timekeeping_suspend

[john stultz]

On Wed, 2009-09-09 at 15:35 +0800, ye janboe wrote:
> after resume from suspend, raw_time is not updated in
> timekeeping_suspend. CLOCK_MONOTONIC_RAW could not get the real hw
> time.
> This patch fix this issue.

Hmm.. I'll admit suspend probably was less considered with
CLOCK_MONOTONIC_RAW, so the semantics aren't well established.

However, I do think we want CLOCK_MONOTONIC_RAW to at-least closely map
to CLOCK_MONOTONIC (but *not* be NTP adjusted). I think that is what
folks would most likely expect.

However, that isn't what this patch seems to do.

Over suspend, I believe all hardware counters reset, so this patch would
seem to try to subtract the value back. 

This sort of makes sense for something like the TSC, which never wraps,
so the raw_time would be set back to a tranlation of the actual TSC
counter,  but for other clocksources like the ACPI PM, it would only
subtract at most 5 seconds. So this leaks hardware specific detail in an
ugly way.

Instead I suspect the most intuitive change would be to add in the
sleep_length to the raw time. This keeps CLOCK_MONOTONIC_RAW behaving
similarly to CLOCK_MONOTONIC, which I believe makes it more useful for
folks using CLOCK_MONOTONIC_RAW for things like tuning time
synchronization.

But let me know more why you chose this implementation and maybe that
will show some better insight in to how you expect it to behave.

thanks
-john



> Signed-off-by: janboe <janboe.ye@xxxxxxxxx>
> ---
>  kernel/time/timekeeping.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index e8c77d9..8420b85 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -331,6 +331,8 @@ static unsigned long timekeeping_suspend_time;
>  static int timekeeping_resume(struct sys_device *dev)
>  {
>         unsigned long flags;
> +       s64 nsec;
> +       cycle_t last_cycle, cycle_delta;
>         unsigned long now = read_persistent_clock();
> 
>         clocksource_resume();
> @@ -346,8 +348,12 @@ static int timekeeping_resume(struct sys_device *dev)
>         }
>         update_xtime_cache(0);
>         /* re-base the last cycle value */
> +       last_cycle = clock->cycle_last;
>         clock->cycle_last = 0;
>         clock->cycle_last = clocksource_read(clock);
> +       cycle_delta = clock->cycle_last - last_cycle;
> +       nsec = cyc2ns(clock, cycle_delta);
> +       timespec_add_ns(&clock->raw_time, nsec);
>         clock->error = 0;
>         timekeeping_suspended = 0;
>         write_sequnlock_irqrestore(&xtime_lock, flags);

--
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/