Hi John, On 11 February 2015 at 08:01, John Stultz <john.stu...@linaro.org> wrote: > On Thu, Jan 29, 2015 at 11:59 PM, Xunlei Pang <xlp...@126.com> wrote: >> From: Xunlei Pang <pang.xun...@linaro.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.xun...@linaro.org> >> --- >> v2->v3: >> Refine according to John's comments using internal variable. >> >> drivers/rtc/class.c | 2 +- >> include/linux/timekeeping.h | 1 + >> kernel/time/timekeeping.c | 32 ++++++++++++++++++++------------ >> 3 files changed, 22 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c >> index 472a5ad..6100af5 100644 >> --- a/drivers/rtc/class.c >> +++ b/drivers/rtc/class.c >> @@ -102,7 +102,7 @@ static int rtc_resume(struct device *dev) >> struct timespec64 sleep_time; >> int err; >> >> - if (has_persistent_clock()) >> + if (timekeeping_sleeptime_injected()) >> return 0; > > Took a closer look here.. So you're replacing has_persistent_clock() > in the resume side, but not the suspend... Can we not cleanup > has_persistent_clock and consolidate to one accessor for both sides of > the suspend?
The sequential calls when the system is suspended are: rtc_suspend(), then timekeeping_suspend(). The sequential calls when the system is resumed are: timekeeping_resume(), then rtc_resume(). Obviously, timekeeping_sleeptime_injected() used by rtc_resume() can be easily determined at timekeeping_resume(). And for nonstop clocksources, currently we have code below: timekeeping_resume(): cycle_now = tk->tkr.read(clock); if ((clock->flags & CLOCK_SOURCE_SUSPEND_NONSTOP) && cycle_now > tk->tkr.cycle_last) { Here comes the confusing thing: "cycle_now > tk->tkr.cycle_last". I can't quite catch what's the purpose by judging cycle_now and cycle_last here, May Nonstop clocksource get wrapped or still can get disfunctional during system suspend? If so, I think it would be hard to judge exactly whether rtc_suspend() is needed for nonstop clocksource cases. If we can get rid of this judgement, I think it would be easy to consolidate this just using read_persistent_clock() and CLOCK_SOURCE_SUSPEND_NONSTOP flag. Any suggestion for this? Thanks, Xunlei > >> >> rtc_hctosys_ret = -ENODEV; >> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h >> index 9b63d13..17a460d 100644 >> --- a/include/linux/timekeeping.h >> +++ b/include/linux/timekeeping.h >> @@ -225,6 +225,7 @@ static inline void timekeeping_clocktai(struct timespec >> *ts) >> /* >> * RTC specific >> */ >> +extern bool timekeeping_sleeptime_injected(void); >> extern void timekeeping_inject_sleeptime64(struct timespec64 *delta); >> >> /* >> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c >> index 6a93185..b02133e 100644 >> --- a/kernel/time/timekeeping.c >> +++ b/kernel/time/timekeeping.c >> @@ -1125,12 +1125,26 @@ static void __timekeeping_inject_sleeptime(struct >> timekeeper *tk, >> tk_debug_account_sleep_time(delta); >> } >> >> +static bool sleeptime_inject; >> + >> +#if defined(CONFIG_RTC_CLASS) && \ >> + defined(CONFIG_PM_SLEEP) && \ >> + defined(CONFIG_RTC_HCTOSYS_DEVICE) > > This change wasn't explained in the commit message. Its fine as a > small optimization, but probably should be split into its own patch. > > thanks > -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/