From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751513AbbBKGw7 (ORCPT ); Wed, 11 Feb 2015 01:52:59 -0500 Received: from mail-yh0-f49.google.com ([209.85.213.49]:40094 "EHLO mail-yh0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750906AbbBKGw6 (ORCPT ); Wed, 11 Feb 2015 01:52:58 -0500 MIME-Version: 1.0 In-Reply-To: References: <1422547147-3073-1-git-send-email-xlpang@126.com> Date: Wed, 11 Feb 2015 14:52:57 +0800 Message-ID: Subject: Re: [PATCH v3 1/2] time: Don't bother to run rtc_resume() for the nonstop clocksource From: Xunlei Pang To: John Stultz Cc: Xunlei Pang , lkml , "rtc-linux@googlegroups.com" , Thomas Gleixner , Alessandro Zummo , Arnd Bergmann Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi John, On 11 February 2015 at 08:01, John Stultz wrote: > On Thu, Jan 29, 2015 at 11:59 PM, Xunlei Pang wrote: >> From: Xunlei Pang >> >> 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 >> --- >> 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