Re: [tip:timers/core] time: Add timekeeping_inject_sleeptime

Re: [tip:timers/core] time: Add timekeeping_inject_sleeptime

[Arve Hjønnevåg]

On Fri, Apr 29, 2011 at 10:31 AM, tip-bot for John Stultz
<john.stultz@linaro.org> wrote:
...
>
> time: Add timekeeping_inject_sleeptime
>
> Some platforms cannot implement read_persistent_clock, as
> their RTC devices are only accessible when interrupts are enabled.
> This keeps them from being used by the timekeeping code on resume
> to measure the time in suspend.
>
> The RTC layer tries to work around this, by calling do_settimeofday
> on resume after irqs are reenabled to set the time properly. However,
> this only corrects CLOCK_REALTIME, and does not properly adjust
> the sleep time value. This causes btime in /proc/stat to be incorrect
> as well as making the new CLOCK_BOTTTIME inaccurate.
>
> This patch resolves the issue by introducing a new timekeeping hook
> to allow the RTC layer to inject the sleep time on resume.
>
> The code also checks to make sure that read_persistent_clock is
> nonfunctional before setting the sleep time, so that should the RTC's
> HCTOSYS option be configured in on a system that does support
> read_persistent_clock we will not increase the total_sleep_time twice.
>
> CC: Arve Hjønnevåg <arve@android.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/rtc/class.c       |   23 +++++++-----------
>  include/linux/time.h      |    1 +
>  kernel/time/timekeeping.c |   56 ++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 63 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index 3901386..4194e59 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -41,26 +41,21 @@ static void rtc_device_release(struct device *dev)
>  * system's wall clock; restore it on resume().
>  */
>
> -static struct timespec delta;
>  static time_t          oldtime;
> +static struct timespec oldts;
>
>  static int rtc_suspend(struct device *dev, pm_message_t mesg)
>  {
>        struct rtc_device       *rtc = to_rtc_device(dev);
>        struct rtc_time         tm;
> -       struct timespec         ts = current_kernel_time();
>
>        if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
>                return 0;
>
>        rtc_read_time(rtc, &tm);
> +       ktime_get_ts(&oldts);
>        rtc_tm_to_time(&tm, &oldtime);
>
> -       /* RTC precision is 1 second; adjust delta for avg 1/2 sec err */
> -       set_normalized_timespec(&delta,
> -                               ts.tv_sec - oldtime,
> -                               ts.tv_nsec - (NSEC_PER_SEC >> 1));
> -
>        return 0;
>  }
>
> @@ -70,10 +65,12 @@ static int rtc_resume(struct device *dev)
>        struct rtc_time         tm;
>        time_t                  newtime;
>        struct timespec         time;
> +       struct timespec         newts;
>
>        if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
>                return 0;
>
> +       ktime_get_ts(&newts);
>        rtc_read_time(rtc, &tm);
>        if (rtc_valid_tm(&tm) != 0) {
>                pr_debug("%s:  bogus resume time\n", dev_name(&rtc->dev));
> @@ -85,15 +82,13 @@ static int rtc_resume(struct device *dev)
>                        pr_debug("%s:  time travel!\n", dev_name(&rtc->dev));
>                return 0;
>        }
> +       /* calculate the RTC time delta */
> +       set_normalized_timespec(&time, newtime - oldtime, 0);
>
> -       /* restore wall clock using delta against this RTC;
> -        * adjust again for avg 1/2 second RTC sampling error
> -        */
> -       set_normalized_timespec(&time,
> -                               newtime + delta.tv_sec,
> -                               (NSEC_PER_SEC >> 1) + delta.tv_nsec);
> -       do_settimeofday(&time);
> +       /* subtract kernel time between rtc_suspend to rtc_resume */
> +       time = timespec_sub(time, timespec_sub(newts, oldts));

The delta you got from the rtc can be almost a second to long or
short. Do you do anything to prevent these errors from accumulating?

>
> +       timekeeping_inject_sleeptime(&time);
>        return 0;
>  }
>
...

-- 
Arve Hjønnevåg

Re: [tip:timers/core] time: Add timekeeping_inject_sleeptime

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 725 bytes --]

On Fri, 29 Apr 2011, Arve Hjønnevåg wrote:
> On Fri, Apr 29, 2011 at 10:31 AM, tip-bot for John Stultz
> <john.stultz@linaro.org> wrote:
> > -       set_normalized_timespec(&time,
> > -                               newtime + delta.tv_sec,
> > -                               (NSEC_PER_SEC >> 1) + delta.tv_nsec);
> > -       do_settimeofday(&time);
> > +       /* subtract kernel time between rtc_suspend to rtc_resume */
> > +       time = timespec_sub(time, timespec_sub(newts, oldts));
> 
> The delta you got from the rtc can be almost a second to long or
> short. Do you do anything to prevent these errors from accumulating?

By using the the magic crystal ball to avoid it or what do you have in
mind ?

Thanks,

	tglx

Re: [tip:timers/core] time: Add timekeeping_inject_sleeptime

[John Stultz]

On Fri, 2011-04-29 at 16:28 -0700, Arve Hjønnevåg wrote:
> On Fri, Apr 29, 2011 at 10:31 AM, tip-bot for John Stultz
> > @@ -85,15 +82,13 @@ static int rtc_resume(struct device *dev)
> >                        pr_debug("%s:  time travel!\n", dev_name(&rtc->dev));
> >                return 0;
> >        }
> > +       /* calculate the RTC time delta */
> > +       set_normalized_timespec(&time, newtime - oldtime, 0);
> >
> > -       /* restore wall clock using delta against this RTC;
> > -        * adjust again for avg 1/2 second RTC sampling error
> > -        */
> > -       set_normalized_timespec(&time,
> > -                               newtime + delta.tv_sec,
> > -                               (NSEC_PER_SEC >> 1) + delta.tv_nsec);
> > -       do_settimeofday(&time);
> > +       /* subtract kernel time between rtc_suspend to rtc_resume */
> > +       time = timespec_sub(time, timespec_sub(newts, oldts));
> 
> The delta you got from the rtc can be almost a second to long or
> short. Do you do anything to prevent these errors from accumulating?

Indeed. Right now we don't do anything.

I'm hoping to extend the RTC interface to provide finer resolution where
possible, but that won't help on hardware that really only gives us
seconds.

We could maybe not only track the suspend time but the RTC time deltas
for when the system is running as well and utilize those values to avoid
accumulating the error long term. But then there can be other
complications between the NTP corrected system time and the uncorrected
RTC time.

Other ideas? I know you've got a patch in the Android tree to try to
address this, should I try to adapt it for use here?

thanks
-john

Re: [tip:timers/core] time: Add timekeeping_inject_sleeptime

[Arve Hjønnevåg]

2011/4/29 John Stultz <john.stultz@linaro.org>:
> On Fri, 2011-04-29 at 16:28 -0700, Arve Hjønnevåg wrote:
>> On Fri, Apr 29, 2011 at 10:31 AM, tip-bot for John Stultz
>> > @@ -85,15 +82,13 @@ static int rtc_resume(struct device *dev)
>> >                        pr_debug("%s:  time travel!\n", dev_name(&rtc->dev));
>> >                return 0;
>> >        }
>> > +       /* calculate the RTC time delta */
>> > +       set_normalized_timespec(&time, newtime - oldtime, 0);
>> >
>> > -       /* restore wall clock using delta against this RTC;
>> > -        * adjust again for avg 1/2 second RTC sampling error
>> > -        */
>> > -       set_normalized_timespec(&time,
>> > -                               newtime + delta.tv_sec,
>> > -                               (NSEC_PER_SEC >> 1) + delta.tv_nsec);
>> > -       do_settimeofday(&time);
>> > +       /* subtract kernel time between rtc_suspend to rtc_resume */
>> > +       time = timespec_sub(time, timespec_sub(newts, oldts));
>>
>> The delta you got from the rtc can be almost a second to long or
>> short. Do you do anything to prevent these errors from accumulating?
>
> Indeed. Right now we don't do anything.
>
> I'm hoping to extend the RTC interface to provide finer resolution where
> possible, but that won't help on hardware that really only gives us
> seconds.
>
> We could maybe not only track the suspend time but the RTC time deltas
> for when the system is running as well and utilize those values to avoid
> accumulating the error long term. But then there can be other
> complications between the NTP corrected system time and the uncorrected
> RTC time.
>
> Other ideas? I know you've got a patch in the Android tree to try to
> address this, should I try to adapt it for use here?
>

Unless you can make the generic code handle this (in case
read_persistent_clock has the same problem), then I would recommend
adapting that patch to your new rtc code. We have not noticed the
problem after applying that patch.

-- 
Arve Hjønnevåg

Re: [tip:timers/core] time: Add timekeeping_inject_sleeptime

[Arve Hjønnevåg]

2011/4/29 Thomas Gleixner <tglx@linutronix.de>:
> On Fri, 29 Apr 2011, Arve Hjønnevåg wrote:
>> On Fri, Apr 29, 2011 at 10:31 AM, tip-bot for John Stultz
>> <john.stultz@linaro.org> wrote:
>> > -       set_normalized_timespec(&time,
>> > -                               newtime + delta.tv_sec,
>> > -                               (NSEC_PER_SEC >> 1) + delta.tv_nsec);
>> > -       do_settimeofday(&time);
>> > +       /* subtract kernel time between rtc_suspend to rtc_resume */
>> > +       time = timespec_sub(time, timespec_sub(newts, oldts));
>>
>> The delta you got from the rtc can be almost a second to long or
>> short. Do you do anything to prevent these errors from accumulating?
>
> By using the the magic crystal ball to avoid it or what do you have in
> mind ?
>

This is how we worked around the problem with the old code:

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 09b4437..e55828a 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -42,25 +42,32 @@ static void rtc_device_release(struct device *dev)
  */

 static struct timespec delta;
+static struct timespec delta_delta;
 static time_t          oldtime;

 static int rtc_suspend(struct device *dev, pm_message_t mesg)
 {
        struct rtc_device       *rtc = to_rtc_device(dev);
        struct rtc_time         tm;
-       struct timespec         ts = current_kernel_time();
+       struct timespec         ts;
+       struct timespec         new_delta;

        if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
                return 0;

+       getnstimeofday(&ts);
        rtc_read_time(rtc, &tm);
        rtc_tm_to_time(&tm, &oldtime);

        /* RTC precision is 1 second; adjust delta for avg 1/2 sec err */
-       set_normalized_timespec(&delta,
+       set_normalized_timespec(&new_delta,
                                ts.tv_sec - oldtime,
                                ts.tv_nsec - (NSEC_PER_SEC >> 1));

+       /* prevent 1/2 sec errors from accumulating */
+       delta_delta = timespec_sub(new_delta, delta);
+       if (delta_delta.tv_sec < -2 || delta_delta.tv_sec >= 2)
+               delta = new_delta;
        return 0;
 }

@@ -80,6 +87,8 @@ static int rtc_resume(struct device *dev)
                return 0;
        }
        rtc_tm_to_time(&tm, &newtime);
+       if (delta_delta.tv_sec < -1)
+               newtime++;
        if (newtime <= oldtime) {
                if (newtime < oldtime)
                        pr_debug("%s:  time travel!\n", dev_name(&rtc->dev));



-- 
Arve Hjønnevåg

Re: [tip:timers/core] time: Add timekeeping_inject_sleeptime

[John Stultz]

On Fri, 2011-04-29 at 18:12 -0700, Arve Hjønnevåg wrote:
>         /* RTC precision is 1 second; adjust delta for avg 1/2 sec err */
> -       set_normalized_timespec(&delta,
> +       set_normalized_timespec(&new_delta,
>                                 ts.tv_sec - oldtime,
>                                 ts.tv_nsec - (NSEC_PER_SEC >> 1));
> 
> +       /* prevent 1/2 sec errors from accumulating */
> +       delta_delta = timespec_sub(new_delta, delta);
> +       if (delta_delta.tv_sec < -2 || delta_delta.tv_sec >= 2)
> +               delta = new_delta;
>         return 0;

So, the basic idea is to keep the RTC vs CLOCK_REALTIME delta constant
(by using the same value as the last time) for each resume cycle
assuming it stays within +/-2 seconds.

If it changes more then that since the last suspend (due to user running
hwclock or the periodic NTP sync or drift of the uncorrected RTC) then
throw away the old value and use the new delta.


> @@ -80,6 +87,8 @@ static int rtc_resume(struct device *dev)
>                 return 0;
>         }
>         rtc_tm_to_time(&tm, &newtime);
> +       if (delta_delta.tv_sec < -1)
> +               newtime++;


So this part isn't quite clicking at the moment (forgive my brain, its a
sunny friday!). Wouldn't this trigger even if we threw away the old
delta (since nothing clears delta_delta)?

I'll spend some more time looking at it, but if you can clarify why we
want to inject a second here (and why there's not a corresponding dec
for delta_delta being > 1 sec to make it symmetric) it would help? 

thanks
-john

Re: [tip:timers/core] time: Add timekeeping_inject_sleeptime

[Arve Hjønnevåg]

2011/4/29 John Stultz <john.stultz@linaro.org>:
> On Fri, 2011-04-29 at 18:12 -0700, Arve Hjønnevåg wrote:
>>         /* RTC precision is 1 second; adjust delta for avg 1/2 sec err */
>> -       set_normalized_timespec(&delta,
>> +       set_normalized_timespec(&new_delta,
>>                                 ts.tv_sec - oldtime,
>>                                 ts.tv_nsec - (NSEC_PER_SEC >> 1));
>>
>> +       /* prevent 1/2 sec errors from accumulating */
>> +       delta_delta = timespec_sub(new_delta, delta);
>> +       if (delta_delta.tv_sec < -2 || delta_delta.tv_sec >= 2)
>> +               delta = new_delta;
>>         return 0;
>
> So, the basic idea is to keep the RTC vs CLOCK_REALTIME delta constant
> (by using the same value as the last time) for each resume cycle
> assuming it stays within +/-2 seconds.
>
> If it changes more then that since the last suspend (due to user running
> hwclock or the periodic NTP sync or drift of the uncorrected RTC) then
> throw away the old value and use the new delta.
>

Yes.

>
>> @@ -80,6 +87,8 @@ static int rtc_resume(struct device *dev)
>>                 return 0;
>>         }
>>         rtc_tm_to_time(&tm, &newtime);
>> +       if (delta_delta.tv_sec < -1)
>> +               newtime++;
>
>
> So this part isn't quite clicking at the moment (forgive my brain, its a
> sunny friday!). Wouldn't this trigger even if we threw away the old
> delta (since nothing clears delta_delta)?
>
Probably.

> I'll spend some more time looking at it, but if you can clarify why we
> want to inject a second here (and why there's not a corresponding dec
> for delta_delta being > 1 sec to make it symmetric) it would help?
>

I don't remember exactly why I did it this way, but the goal is to
make sure time does not jump backwards. If the rtc runs slower than
the clock we are using when awake, then time could jump backwards
after suspend since we re-sync with the rtc. With your new code, it is
probably better to just clip the resulting sleep-time to not allow
negative values.

-- 
Arve Hjønnevåg