Re: [PATCH 2/2] rtc: Avoid accumulating time drift in suspend/resume

From: Arve Hjønnevåg
Date: Wed Jun 01 2011 - 20:54:52 EST


On Tue, May 31, 2011 at 11:07 PM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
> Because the RTC interface is only a second granular interface,
> each time we read from the RTC for suspend/resume, we introduce a
> half second (on average) of error.
>
> In order to avoid this error accumulating as the system is suspended
> over and over, this patch measures the time delta between the RTC
> and the system CLOCK_REALTIME.
>
> If the delta is less then 2 seconds from the last suspend, we compensate
> by using the previous time delta (keeping it close). If it is larger
> then 2 seconds, we assume the clock was set or has been changed, so we
> do no correction and update the delta.
>
> Note: If NTP is running, ths could seem to "fight" with the NTP corrected
> time, where as if the system time was off by 1 second, and NTP slewed the
> value in, a suspend/resume cycle could undo this correction, by trying to
> restore the previous offset from the RTC. However, without this patch,
> since each read could cause almost a full second worth of error, its
> possible to get almost 2 seconds of error just from the suspend/resume
> cycle alone, so this about equal to any offset added by the compensation.
>
> Further on systems that suspend/resume frequently, this should keep time
> closer then NTP could compensate for if the errors were allowed to
> accumulate.
>
> Credits to Arve Hjønnevåg for suggesting this solution.
>
> This patch also improves some of the variable names and adds more clear
> comments.
>
> CC: Arve Hjønnevåg <arve@xxxxxxxxxxx>
> CC: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
>  drivers/rtc/class.c |   65 +++++++++++++++++++++++++++++++++++++-------------
>  1 files changed, 48 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index 4194e59..a619228 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -41,20 +41,41 @@ static void rtc_device_release(struct device *dev)
>  * system's wall clock; restore it on resume().
>  */
>
> -static time_t          oldtime;
> -static struct timespec oldts;
> +static struct timespec old_rtc, old_system, old_delta;
> +
>
>  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         delta, delta_delta;
>        if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
>                return 0;
>
> +       /* snapshot the current RTC and system time at suspend*/
>        rtc_read_time(rtc, &tm);
> -       ktime_get_ts(&oldts);
> -       rtc_tm_to_time(&tm, &oldtime);
> +       getnstimeofday(&old_system);
> +       rtc_tm_to_time(&tm, &old_rtc.tv_sec);
> +
> +
> +       /*
> +        * To avoid drift caused by repeated suspend/resumes,
> +        * which each can add ~1 second drift error,
> +        * try to compensate so the difference in system time
> +        * and rtc time stays close to constant.
> +        */
> +       delta = timespec_sub(old_system, old_rtc);
> +       delta_delta = timespec_sub(delta, old_delta);
> +       if (abs(delta_delta.tv_sec)  >= 2) {
> +               /*
> +                * if delta_delta is too large, assume time correction
> +                * has occured and set old_delta to the current delta.
> +                */
> +               old_delta = delta;
> +       } else {
> +               /* Otherwise try to adjust old_system to compensate */
> +               old_system = timespec_sub(old_system, delta_delta);
> +       }
>
>        return 0;
>  }
> @@ -63,32 +84,42 @@ static int rtc_resume(struct device *dev)
>  {
>        struct rtc_device       *rtc = to_rtc_device(dev);
>        struct rtc_time         tm;
> -       time_t                  newtime;
> -       struct timespec         time;
> -       struct timespec         newts;
> +       struct timespec         new_system, new_rtc;
> +       struct timespec         sleep_time;
>
>        if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
>                return 0;
>
> -       ktime_get_ts(&newts);
> +       /* snapshot the current rtc and system time at resume */
> +       getnstimeofday(&new_system);
>        rtc_read_time(rtc, &tm);
>        if (rtc_valid_tm(&tm) != 0) {
>                pr_debug("%s:  bogus resume time\n", dev_name(&rtc->dev));
>                return 0;
>        }
> -       rtc_tm_to_time(&tm, &newtime);
> -       if (newtime <= oldtime) {
> -               if (newtime < oldtime)
> +       rtc_tm_to_time(&tm, &new_rtc.tv_sec);
> +       new_rtc.tv_nsec = 0;
> +
> +       if (new_rtc.tv_sec <= old_rtc.tv_sec) {
> +               if (new_rtc.tv_sec < old_rtc.tv_sec)
>                        pr_debug("%s:  time travel!\n", dev_name(&rtc->dev));
>                return 0;
>        }
> -       /* calculate the RTC time delta */
> -       set_normalized_timespec(&time, newtime - oldtime, 0);
>
> -       /* subtract kernel time between rtc_suspend to rtc_resume */
> -       time = timespec_sub(time, timespec_sub(newts, oldts));
> +       /* calculate the RTC time delta (sleep time)*/
> +       sleep_time = timespec_sub(new_rtc, old_rtc);
> +
> +       /*
> +        * Since these RTC suspend/resume handlers are not called
> +        * at the very end of suspend or the start of resume,
> +        * some run-time may pass on either sides of the sleep time
> +        * so subtract kernel run-time between rtc_suspend to rtc_resume
> +        * to keep things accurate.
> +        */
> +       sleep_time = timespec_sub(sleep_time,
> +                       timespec_sub(new_system, old_system));

What happens if sleep_time is negative? I think this need to be
clamped to 0 to avoid backwards jumps when you wake up more than once
without the rtc advancing.

>
> -       timekeeping_inject_sleeptime(&time);
> +       timekeeping_inject_sleeptime(&sleep_time);
>        return 0;
>  }
>
> --
> 1.7.3.2.146.gca209
>
>



--
Arve Hjønnevåg
--
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/