Re: [PATCH v4 3/4] time: rtc: Don't bother into rtc_resume() for the nonstop clocksource

[John Stultz]

On Sun, Feb 15, 2015 at 5:09 AM, Xunlei Pang <xlpang@xxxxxxx> wrote:
> From: Xunlei Pang <pang.xunlei@xxxxxxxxxx>
>
> If a system does not provide a persistent_clock(), the time
> will be updated on resume by rtc_resume(). With the addition
> of the non-stop clocksources for suspend timing, those systems
> set the time on resume in timekeeping_resume(), but may not
> provide a valid persistent_clock().
>
> This results in the rtc_resume() logic thinking no one has set
> the time and it then will over-write the suspend time again,
> which is not necessary and only increases clock error.
>
> So, fix this for rtc_resume().
>
> Signed-off-by: Xunlei Pang <pang.xunlei@xxxxxxxxxx>
> ---
>  drivers/rtc/class.c         |  4 +--
>  include/linux/timekeeping.h |  9 +++----
>  kernel/time/timekeeping.c   | 63 +++++++++++++++++++++++++++++++++------------
>  3 files changed, 52 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index 472a5ad..74a943e 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -55,7 +55,7 @@ static int rtc_suspend(struct device *dev)
>         struct timespec64       delta, delta_delta;
>         int err;
>
> -       if (has_persistent_clock())
> +       if (timekeeping_rtc_skipsuspend())
>                 return 0;
>
>         if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
> @@ -102,7 +102,7 @@ static int rtc_resume(struct device *dev)
>         struct timespec64       sleep_time;
>         int err;
>
> -       if (has_persistent_clock())
> +       if (timekeeping_rtc_skipresume())
>                 return 0;
>
>         rtc_hctosys_ret = -ENODEV;
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index 3eaae47..7cbd518 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -242,6 +242,9 @@ static inline void timekeeping_clocktai(struct timespec *ts)
>  /*
>   * RTC specific
>   */
> +extern bool timekeeping_rtc_skipsuspend(void);
> +extern bool timekeeping_rtc_skipresume(void);
> +
>  extern void timekeeping_inject_sleeptime64(struct timespec64 *delta);
>
>  /*
> @@ -253,14 +256,8 @@ extern void getnstime_raw_and_real(struct timespec *ts_raw,
>  /*
>   * Persistent clock related interfaces
>   */
> -extern bool persistent_clock_exist;
>  extern int persistent_clock_is_local;
>
> -static inline bool has_persistent_clock(void)
> -{
> -       return persistent_clock_exist;
> -}
> -
>  extern void read_persistent_clock(struct timespec *ts);
>  extern void read_boot_clock(struct timespec *ts);
>  extern int update_persistent_clock(struct timespec now);
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index ec6ee9b..276c72d 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -63,9 +63,6 @@ static struct tk_fast tk_fast_mono ____cacheline_aligned;
>  /* flag for if timekeeping is suspended */
>  int __read_mostly timekeeping_suspended;
>
> -/* Flag for if there is a persistent clock on this platform */
> -bool __read_mostly persistent_clock_exist = false;
> -
>  static inline void tk_normalize_xtime(struct timekeeper *tk)
>  {
>         while (tk->tkr.xtime_nsec >= ((u64)NSEC_PER_SEC << tk->tkr.shift)) {
> @@ -1045,6 +1042,9 @@ void __weak read_boot_clock(struct timespec *ts)
>         ts->tv_nsec = 0;
>  }
>
> +/* Flag for if there is a persistent clock on this platform */
> +static bool persistent_clock_exist;
> +

So I probably made the original mistake, but since you're modifying
it,  "persistent_clock_exists" is slightly more grammatical.


>  /*
>   * timekeeping_init - Initializes the clocksource and common timekeeping values
>   */
> @@ -1125,15 +1125,53 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
>         tk_debug_account_sleep_time(delta);
>  }
>
> +static bool sleeptime_inject;
> +

Also, maybe "sleeptime_injected".  And since these flag values are
related, can we define them next to each other (and close to their
accessor functions) rather then randomly through the file?


>  #if defined(CONFIG_RTC_CLASS) && \
>         defined(CONFIG_PM_SLEEP) && \
>         defined(CONFIG_RTC_HCTOSYS_DEVICE)
>  /**
> + * We have three kinds of time sources to use for sleep time
> + * injection, the preference order is:
> + * 1) non-stop clocksource
> + * 2) persistent clock (ie: RTC accessible when irqs are off)
> + * 3) RTC
> + *
> + * 1) and 2) are used by timekeeping, 3) by RTC subsystem.
> + * If system has neither 1) nor 2), 3) will be used finally.
> + *
> + *
> + * If timekeeping has injected sleeptime via either 1) or 2),
> + * 3) becomes needless, so in this case we don't need to call
> + * rtc_resume(), and this is what timekeeping_rtc_skipresume()
> + * means.
> + */
> +bool timekeeping_rtc_skipresume(void)
> +{
> +       return sleeptime_inject;
> +}
> +
> +/**
> + * 1) can be determined whether to use or not only when doing
> + * timekeeping_resume() which is invoked after rtc_suspend(),
> + * so we can't skip rtc_suspend() surely if system has 1).
> + *
> + * But if system has 2), 2) will definitely be used, so in this
> + * case we don't need to call rtc_suspend(), and this is what
> + * timekeeping_rtc_skipsuspend() means.
> + */
> +bool timekeeping_rtc_skipsuspend(void)
> +{
> +       return persistent_clock_exist;
> +}
> +
> +/**
>   * timekeeping_inject_sleeptime64 - Adds suspend interval to timeekeeping values
>   * @delta: pointer to a timespec64 delta value
>   *
>   * This hook is for architectures that cannot support read_persistent_clock
> - * because their RTC/persistent clock is only accessible when irqs are enabled.
> + * because their RTC/persistent clock is only accessible when irqs are enabled,
> + * and also don't have an effective nonstop clocksource.
>   *
>   * This function should only be called by rtc_resume(), and allows
>   * a suspend offset to be injected into the timekeeping values.
> @@ -1143,13 +1181,6 @@ void timekeeping_inject_sleeptime64(struct timespec64 *delta)
>         struct timekeeper *tk = &tk_core.timekeeper;
>         unsigned long flags;
>
> -       /*
> -        * Make sure we don't set the clock twice, as timekeeping_resume()
> -        * already did it
> -        */
> -       if (has_persistent_clock())
> -               return;
> -
>         raw_spin_lock_irqsave(&timekeeper_lock, flags);
>         write_seqcount_begin(&tk_core.seq);
>
> @@ -1182,8 +1213,8 @@ static void timekeeping_resume(void)
>         struct timespec64 ts_new, ts_delta;
>         struct timespec tmp;
>         cycle_t cycle_now, cycle_delta;
> -       bool suspendtime_found = false;
>
> +       sleeptime_inject = false;
>         read_persistent_clock(&tmp);
>         ts_new = timespec_to_timespec64(tmp);
>
> @@ -1230,14 +1261,14 @@ static void timekeeping_resume(void)
>                 nsec += ((u64) cycle_delta * mult) >> shift;
>
>                 ts_delta = ns_to_timespec64(nsec);
> -               suspendtime_found = true;
> +               sleeptime_inject = true;
>         } else if (timespec64_compare(&ts_new,
>                                     &persistent_clock_suspendtime) > 0) {
>                 ts_delta = timespec64_sub(ts_new, persistent_clock_suspendtime);
> -               suspendtime_found = true;
> +               sleeptime_inject = true;
>         }
>
> -       if (suspendtime_found)
> +       if (sleeptime_inject)
>                 __timekeeping_inject_sleeptime(tk, &ts_delta);
>
>         /* Re-base the last cycle value */
> @@ -1281,7 +1312,7 @@ static int timekeeping_suspend(void)
>         timekeeping_forward_now(tk);
>         timekeeping_suspended = 1;
>
> -       if (has_persistent_clock()) {
> +       if (persistent_clock_exist) {
>                 /*
>                  * To avoid drift caused by repeated suspend/resumes,
>                  * which each can add ~1 second drift error,


This is looking better! Thanks for the respin!
-john
--
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/