| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 25 Feb 2015 21:00:08 -0800
From: John Stultz <john.stultz@...aro.org>
To: Xunlei Pang <xlpang@....com>
Cc: lkml <linux-kernel@...r.kernel.org>,
"rtc-linux@...glegroups.com" <rtc-linux@...glegroups.com>,
Thomas Gleixner <tglx@...utronix.de>,
Alessandro Zummo <a.zummo@...ertech.it>,
Arnd Bergmann <arnd.bergmann@...aro.org>,
Peter Zijlstra <peterz@...radead.org>,
Xunlei Pang <pang.xunlei@...aro.org>
Subject: Re: [PATCH v4 3/4] time: rtc: Don't bother into rtc_resume() for the
nonstop clocksource
On Sun, Feb 15, 2015 at 5:09 AM, Xunlei Pang <xlpang@....com> wrote:
> From: Xunlei Pang <pang.xunlei@...aro.org>
>
> 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@...aro.org>
> ---
> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists