On Fri, Apr 29, 2011 at 10:31 AM, tip-bot for John Stultz
<[email protected]> 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 <[email protected]>
> CC: Thomas Gleixner <[email protected]>
> Acked-by: Arnd Bergmann <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> ?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
On Fri, 29 Apr 2011, Arve Hj?nnev?g wrote:
> On Fri, Apr 29, 2011 at 10:31 AM, tip-bot for John Stultz
> <[email protected]> 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
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
2011/4/29 John Stultz <[email protected]>:
> 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
2011/4/29 Thomas Gleixner <[email protected]>:
> On Fri, 29 Apr 2011, Arve Hj?nnev?g wrote:
>> On Fri, Apr 29, 2011 at 10:31 AM, tip-bot for John Stultz
>> <[email protected]> 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
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
2011/4/29 John Stultz <[email protected]>:
> 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