From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758936Ab1FBAys (ORCPT ); Wed, 1 Jun 2011 20:54:48 -0400 Received: from mail-px0-f179.google.com ([209.85.212.179]:62368 "EHLO mail-px0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754737Ab1FBAyr convert rfc822-to-8bit (ORCPT ); Wed, 1 Jun 2011 20:54:47 -0400 MIME-Version: 1.0 In-Reply-To: <1306908469-15275-3-git-send-email-john.stultz@linaro.org> References: <1306908469-15275-1-git-send-email-john.stultz@linaro.org> <1306908469-15275-3-git-send-email-john.stultz@linaro.org> Date: Wed, 1 Jun 2011 17:54:46 -0700 Message-ID: Subject: Re: [PATCH 2/2] rtc: Avoid accumulating time drift in suspend/resume From: =?ISO-8859-1?Q?Arve_Hj=F8nnev=E5g?= To: John Stultz Cc: linux-kernel@vger.kernel.org, Thomas Gleixner Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 31, 2011 at 11:07 PM, John Stultz 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 > CC: Thomas Gleixner > Signed-off-by: John Stultz > --- >  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