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 Content-Length: 6295 Lines: 163 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/