From patchwork Thu Sep 21 22:05:10 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jason Gunthorpe X-Patchwork-Id: 9964869 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id EDA78602D8 for ; Thu, 21 Sep 2017 22:06:24 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CFBEE2967D for ; Thu, 21 Sep 2017 22:06:24 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C3A79296F1; Thu, 21 Sep 2017 22:06:24 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id CDFF22967D for ; Thu, 21 Sep 2017 22:06:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=zjaS80UKqllPs4yvYtz3/DTwoNUV7XKQ8cTpi/uOpNM=; b=s8vorNLG6BHkGk vJ8WQ0aQ923BKmd5W4HrP+G72nC0XSNY8/5muCfysQjuy1k8Fw3KpKbD3DpCfkuGWoWvQdOvUq/1+ 2bGCkZ1BCHPP4lkEyksvY4osCN85+vIqLPuRjtOQp170kmP7t4NWknamhQ+7xJVaZDpXSKycMdhIF q9z/BWGVsxdVhryKWbaiTV9sKbXOX2hHKxmTnPBGbvqCVFp6/2OFV7z/YUWu6qQY9udcOQnf0lDdr uUObSe7y6KunuLUR9xPpLdvDOCZV+qAyp1OKDX32P1Axs7ry+5bhaIIY1Z4N62vxEJEGbBAb8rLWe BQlnk/MT+GLBV5+IkKHQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dv9bn-0000Cf-Sf; Thu, 21 Sep 2017 22:06:11 +0000 Received: from quartz.orcorp.ca ([184.70.90.242]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1dv9bj-0000A6-S9 for linux-arm-kernel@lists.infradead.org; Thu, 21 Sep 2017 22:06:10 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=obsidianresearch.com; s=rsa1; h=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=JimD1rMRQPdsznd3LUAxvkgBmYsztN+TSwY9S2se+R8=; b=Z+eU7rJ/a37jpndC3WPUoMOd8ftDRUtlLaQaeqIAk5EMAd4tCgMSGTlq3AHRGKSvdOEAGVSi0taZdJTo8AXdhT+QkKHnx3Bv2uStDylHwtKXIom9/IpF34fTDNQWBI/zqrgEzNWIvw/S7N1cK7ImUcfo39KtN9etNZrT/w2sPDw=; Received: from jgg by quartz.orcorp.ca with local (Exim 4.84_2) (envelope-from ) id 1dv9ao-0003CJ-Nt; Thu, 21 Sep 2017 16:05:10 -0600 Date: Thu, 21 Sep 2017 16:05:10 -0600 From: Jason Gunthorpe To: Russell King - ARM Linux Subject: Re: On NTP, RTCs and accurately setting their time Message-ID: <20170921220510.GA11395@obsidianresearch.com> References: <20170920112152.GL20805@n2100.armlinux.org.uk> <20170920162208.GB536@obsidianresearch.com> <20170920165141.GP20805@n2100.armlinux.org.uk> <20170920224522.GB20974@obsidianresearch.com> <20170921075907.GR20805@n2100.armlinux.org.uk> <20170921093218.GS20805@n2100.armlinux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170921093218.GS20805@n2100.armlinux.org.uk> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170921_150608_135449_3DA3F698 X-CRM114-Status: GOOD ( 38.37 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Alessandro Zummo , Stephen Boyd , John Stultz , Alexandre Belloni , Thomas Gleixner , linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Sep 21, 2017 at 10:32:19AM +0100, Russell King - ARM Linux wrote: I've combined several of your messages together into this reply... > static inline bool rtc_tv_nsec_ok(struct rtc_device *rtc, > struct timespec64 *now) > { > long diff; > > diff = (now->tv_nsec - rtc->time_set_nsec) % NSEC_PER_SEC; > /* diff must be within [-NSEC_PER_SEC/2..NSEC_PER_SEC/2) */ > if (diff >= NSEC_PER_SEC / 2) > diff -= NSEC_PER_SEC; > else if (diff < -NSEC_PER_SEC / 2) > diff += NSEC_PER_SEC; I like your concept of allowing a negative time_set_nsec, I was focused on a positive value and botched the first draft by not casting to unsigned for the maths.. It seems much better because it incorporates the rounding calculation into the common code instead of shifting it to the driver. I revised this idea a few times and came up with something a bit simpler to understand by changing from a time_set_nsec to its inverse the set_offset_nsec, that makes the maths simpler and we can avoid troublesome signed modulos by using time maths functions instead of open coding them. See full revised patch below, this time I did test the rtc_tv_nsec_ok() using your test vectors and it seems OK. Still have not tested the whole patch... So with v2 of this patch, the driver would specify positive 1s, and then the set will be called when tv_nsec == 0, but the tv_sec will be +1. Similarly the existing case of 0.5s offset will work more like before, where it will be called with tv_nsec == .5s and the tv_sec will be +1 - before I think this happened reliably 'by accident' due to the rounding. > Most drivers use rtc_device_register() or devm_rtc_device_register() > rather than rtc_allocate_device() (which is static.) This does not > give RTC drivers a chance to set rtc->time_set_nsec before the RTC > is registered with the kernel. You are correct about the race, of course. I left it like that deliberately, I'm not sure it is worth solving since I felt nothing should be setting the RTC within a few CPU instructions of registering it. As you say, adding a pre-registration init op will solve the issue if you think it is worth solving. Jason From 566712cb27596836469637b0e50e7fd973368df8 Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 21 Sep 2017 15:55:33 -0600 Subject: [PATCH v2] rtc: Allow rtc drivers to specify the tv_nsec value for ntp ntp is currently hardwired to try and call the rtc set when wall clock tv_nsec is 0.5 seconds. This historical behaviour works well with certain PC RTCs, but is not universal to all rtc hardware. Change how this works by introducing the driver specific concept of a required delay between current wall clock time and the target time to set (with a 0 tv_nsecs). When this delay is set to 0.5 seconds then the behaviour will be the same as today, at wall clock time 0.5 sec the RTC set will be called to set 1.0 sec. Each RTC driver should set the set_offset_nsec according to its needs. This also polices the incoming set time ts_nsec from NTP, if it is not within +-1ms of the required time then the set is deferred. The target time to set is 'snapped' to the nearest value. The calculation of the sleep time for ntp is also revised to use modern helper functions, and to more directly and safely compute a relative jiffies delay that will result in the correct tv_nsec. If for some reason the timer does not meet the requirement then it does a short sleep and tries again. Signed-off-by: Jason Gunthorpe --- drivers/rtc/class.c | 6 ++++ drivers/rtc/systohc.c | 85 ++++++++++++++++++++++++++++++++++++++++++--------- include/linux/rtc.h | 10 +++++- kernel/time/ntp.c | 62 ++++++++++++++++++++++++------------- 4 files changed, 125 insertions(+), 38 deletions(-) diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c index 2ed970d61da140..eef4123a573504 100644 --- a/drivers/rtc/class.c +++ b/drivers/rtc/class.c @@ -161,6 +161,12 @@ static struct rtc_device *rtc_allocate_device(void) device_initialize(&rtc->dev); + /* Drivers can revise this default after allocating the device. It + * should be the value of wallclock tv_nsec that the driver needs in + * order to synchronize the second tick over during set. + */ + rtc->set_offset_nsec = NSEC_PER_SEC / 2; + rtc->irq_freq = 1; rtc->max_user_freq = 64; rtc->dev.class = rtc_class; diff --git a/drivers/rtc/systohc.c b/drivers/rtc/systohc.c index b4a68ffcd06bb8..a7d0bbe2577110 100644 --- a/drivers/rtc/systohc.c +++ b/drivers/rtc/systohc.c @@ -7,9 +7,42 @@ #include #include +/* Determine if we can call to driver to set the time. Drivers can only be + * called to set a second aligned time value, and the field set_offset_nsec + * specifies how far away from the second aligned time to call the driver. + * + * This also compute 'to_set' which is the time we are trying to set, and has + * a zero in tv_nsecs, such that: + * to_set - set_delay_nsec == now +/- FUZZ + * + */ +#define TIME_SET_NSEC_FUZZ (1000 * 1000) +static inline bool rtc_tv_nsec_ok(struct rtc_device *rtc, + struct timespec64 *to_set, + const struct timespec64 *now) +{ + struct timespec64 delay = {.tv_sec = 0, + .tv_nsec = rtc->set_offset_nsec}; + + *to_set = timespec64_add(*now, delay); + + if (to_set->tv_nsec < TIME_SET_NSEC_FUZZ) { + to_set->tv_nsec = 0; + return true; + } + + if (to_set->tv_nsec > NSEC_PER_SEC - TIME_SET_NSEC_FUZZ) { + to_set->tv_sec++; + to_set->tv_nsec = 0; + return true; + } + return false; +} + /** * rtc_set_ntp_time - Save NTP synchronized time to the RTC * @now: Current time of day + * @target_nsec: pointer for desired now->tv_nsec value * * Replacement for the NTP platform function update_persistent_clock64 * that stores time for later retrieval by rtc_hctosys. @@ -18,30 +51,52 @@ * possible at all, and various other -errno for specific temporary failure * cases. * + * -EPROTO is returned if now.tv_nsec is not close enough to *target_nsec. + ( * If temporary failure is indicated the caller should try again 'soon' */ -int rtc_set_ntp_time(struct timespec64 now) +int rtc_set_ntp_time(struct timespec64 now, long *target_nsec) { struct rtc_device *rtc; struct rtc_time tm; + struct timespec64 to_set; int err = -ENODEV; - - if (now.tv_nsec < (NSEC_PER_SEC >> 1)) - rtc_time64_to_tm(now.tv_sec, &tm); - else - rtc_time64_to_tm(now.tv_sec + 1, &tm); + bool ok; rtc = rtc_class_open(CONFIG_RTC_SYSTOHC_DEVICE); - if (rtc) { - /* rtc_hctosys exclusively uses UTC, so we call set_time here, - * not set_mmss. */ - if (rtc->ops && - (rtc->ops->set_time || - rtc->ops->set_mmss64 || - rtc->ops->set_mmss)) - err = rtc_set_time(rtc, &tm); - rtc_class_close(rtc); + if (!rtc) + goto out_err; + + if (!rtc->ops || (!rtc->ops->set_time && !rtc->ops->set_mmss64 && + !rtc->ops->set_mmss)) + goto out_close; + + /* Compute the value of tv_nsec we require the caller to supply in + * now.tv_nsec. This is the value such that (now + + * set_offset_nsec).tv_nsec == 0. + */ + set_normalized_timespec64(&to_set, 0, -rtc->set_offset_nsec); + *target_nsec = to_set.tv_nsec; + + /* The ntp code must call this with the correct value in tv_nsec, if + * it does not we update target_nsec and return EPROTO to make the ntp + * code try again later. + */ + ok = rtc_tv_nsec_ok(rtc, &to_set, &now); + if (!ok) { + err = -EPROTO; + goto out_close; } + rtc_time64_to_tm(to_set.tv_sec, &tm); + + /* rtc_hctosys exclusively uses UTC, so we call set_time here, not + * set_mmss. + */ + err = rtc_set_time(rtc, &tm); + +out_close: + rtc_class_close(rtc); +out_err: return err; } diff --git a/include/linux/rtc.h b/include/linux/rtc.h index 0a0f0d14a5fba5..7d526550a700f8 100644 --- a/include/linux/rtc.h +++ b/include/linux/rtc.h @@ -137,6 +137,14 @@ struct rtc_device { /* Some hardware can't support UIE mode */ int uie_unsupported; + /* Number of nsec it takes to set the RTC clock. This influences when + * the set ops are called. An offset: + * - of 0.5 s will call RTC set for wall clock time 10.0 s at 9.5 s + * - of 1.5 s will call RTC set for wall clock time 10.0 s at 8.5 s + * - of -0.5 s will call RTC set for wall clock time 10.0 s at 10.5 s + */ + long set_offset_nsec; + bool registered; struct nvmem_config *nvmem_config; @@ -174,7 +182,7 @@ extern void devm_rtc_device_unregister(struct device *dev, extern int rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm); extern int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm); -extern int rtc_set_ntp_time(struct timespec64 now); +extern int rtc_set_ntp_time(struct timespec64 now, long *target_nsec); int __rtc_read_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm); extern int rtc_read_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alrm); diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c index edf19cc5314043..6d2f521dd97748 100644 --- a/kernel/time/ntp.c +++ b/kernel/time/ntp.c @@ -514,17 +514,19 @@ static DECLARE_DELAYED_WORK(sync_cmos_work, sync_cmos_clock); static void sync_cmos_clock(struct work_struct *work) { - struct timespec64 now; - struct timespec64 next; + struct timespec64 now, next, delta; int fail = 1; + long target_nsec = NSEC_PER_SEC / 2; /* - * If we have an externally synchronized Linux clock, then update - * CMOS clock accordingly every ~11 minutes. Set_rtc_mmss() has to be - * called as close as possible to 500 ms before the new second starts. - * This code is run on a timer. If the clock is set, that timer - * may not expire at the correct time. Thus, we adjust... - * We want the clock to be within a couple of ticks from the target. + * If we have an externally synchronized Linux clock, then update CMOS + * clock accordingly every ~11 minutes. Histiocally Set_rtc_mmss() + * has to be called as close as possible to 500 ms (target_nsec) + * before the new second starts, but new RTC drivers can select a + * different value. This code is run on a timer. If the clock is + * set, that timer may not expire at the correct time. Thus, we + * adjust... We want the clock to be within a couple of ticks from + * the target. */ if (!ntp_synced()) { /* @@ -547,25 +549,41 @@ static void sync_cmos_clock(struct work_struct *work) #ifdef CONFIG_RTC_SYSTOHC if (fail == -ENODEV) - fail = rtc_set_ntp_time(adjust); + fail = rtc_set_ntp_time(adjust, &target_nsec); #endif } - next.tv_nsec = (NSEC_PER_SEC / 2) - now.tv_nsec - (TICK_NSEC / 2); - if (next.tv_nsec <= 0) - next.tv_nsec += NSEC_PER_SEC; + do { + /* + * Compute the next wall clock time to try and set the + * clock + */ + next = now; + if (!fail || fail == -ENODEV) + timespec64_add_ns(&next, 659ULL * NSEC_PER_SEC); + else + /* Update failed, try again in about 10 seconds */ + timespec64_add_ns(&next, 10ULL * NSEC_PER_SEC); - if (!fail || fail == -ENODEV) - next.tv_sec = 659; - else - next.tv_sec = 0; + /* + * The next call to sync_cmos_clock needs to have have a wall + * clock tv_nsec value equal to target_nsec. + */ + if (next.tv_nsec > target_nsec) + next.tv_sec++; + next.tv_nsec = target_nsec; - if (next.tv_nsec >= NSEC_PER_SEC) { - next.tv_sec++; - next.tv_nsec -= NSEC_PER_SEC; - } - queue_delayed_work(system_power_efficient_wq, - &sync_cmos_work, timespec64_to_jiffies(&next)); + /* + * Convert to a relative delay. If time set took a really long + * time, or the wall clock was changed, this might become + * negative, so try again. + */ + getnstimeofday64(&now); + delta = timespec64_sub(next, now); + } while (delta.tv_sec <= 0); + + queue_delayed_work(system_power_efficient_wq, &sync_cmos_work, + timespec64_to_jiffies(&delta)); } void ntp_notify_cmos_timer(void)