[RFE PATCH 0/2] x86, rtc, ntp, Enable full rtc synchronization

[RFE PATCH 0/2] x86, rtc, ntp, Enable full rtc synchronization

[Prarit Bhargava]

This patchset enables a full rtc synchronization via ntp on x86.  The current
codebase (plus http://marc.info/?l=linux-kernel&m=136036689219340&w=2, which is
queued for tip), will attempt to synchronize the rtc to the system time every
11 minutes if ntp is running.

The problem in the current code is that the synchronization will only occur
if the system time is within +/-15 minutes of the current rtc time.  ie)
we only do a "partial" synchronization of the rtc.  Other architectures do
full synchronizations and the partial sync appears to be a software limitation.

This patchset introduces a full synchronization of the rtc, and allows the
writing of the rtc date and time via sysfs (read for date and time is already
implemented).

I tested this patch by using the write capability introduced in 2/2 to
write in older and newer dates into the rtc, and then rebooting with ntpdate,
and/or ntpdate enabled and verifying the correct setting of the hwclock (and
system time) via calls to date and hwclock (all on 64-bit x86)

I have not tested the mrst/vrtc.c code, however, code inspection indicates
that the only change required is the year offset of 1972.  I booted 32-bit
Fedora 18 on an UEFI system and confirmed that the system time and hwclock
were now correct at boot.

Signed-off-by: Prarit Bhargava <prarit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Cc: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Cc: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: David Vrabel <david.vrabel-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Andi Kleen <ak-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Prarit Bhargava (2):
  x86, rtc, ntp, Do full rtc synchronization with ntp
  rtc, add write functionality to sysfs

 arch/x86/kernel/rtc.c         | 69 ++++++----------------------------
 arch/x86/platform/efi/efi.c   | 24 ++++++++----
 arch/x86/platform/mrst/vrtc.c | 41 ++++++++++++---------
 drivers/rtc/rtc-sysfs.c       | 86 ++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 136 insertions(+), 84 deletions(-)

-- 
1.8.1.2

[RFE PATCH 1/2] x86, rtc, ntp, Do full rtc synchronization with ntp

[Prarit Bhargava]

Every 11 minutes ntp attempts to update the x86 rtc with the current
system time.  Currently, the x86 code only updates the rtc if the system
time is within +/-15 minutes of the current value of the rtc.  Other
architectures do a full synchronization and there is no reason that x86
should be software limited to a 30 minute window.

This patch changes the behavior of the kernel to do a full synchronization
(year, month, day, hour, minute, and second) of the rtc when ntp requests
a synchronization between the system time and the rtc.

I've used the RTC library functions in this patchset as they do all the
required bounds checking.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Cc: x86@kernel.org
Cc: Matt Fleming <matt.fleming@intel.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: linux-efi@vger.kernel.org
---
 arch/x86/kernel/rtc.c         | 69 ++++++++-----------------------------------
 arch/x86/platform/efi/efi.c   | 24 ++++++++++-----
 arch/x86/platform/mrst/vrtc.c | 41 ++++++++++++++-----------
 3 files changed, 52 insertions(+), 82 deletions(-)

diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 801602b..44e6c3a 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -13,6 +13,7 @@
 #include <asm/x86_init.h>
 #include <asm/time.h>
 #include <asm/mrst.h>
+#include <asm/rtc.h>
 
 #ifdef CONFIG_X86_32
 /*
@@ -36,70 +37,24 @@ EXPORT_SYMBOL(rtc_lock);
  * nowtime is written into the registers of the CMOS clock, it will
  * jump to the next second precisely 500 ms later. Check the Motorola
  * MC146818A or Dallas DS12887 data sheet for details.
- *
- * BUG: This routine does not handle hour overflow properly; it just
- *      sets the minutes. Usually you'll only notice that after reboot!
  */
 int mach_set_rtc_mmss(unsigned long nowtime)
 {
-	int real_seconds, real_minutes, cmos_minutes;
-	unsigned char save_control, save_freq_select;
-	unsigned long flags;
+	struct rtc_time tm;
 	int retval = 0;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-
-	 /* tell the clock it's being set */
-	save_control = CMOS_READ(RTC_CONTROL);
-	CMOS_WRITE((save_control|RTC_SET), RTC_CONTROL);
-
-	/* stop and reset prescaler */
-	save_freq_select = CMOS_READ(RTC_FREQ_SELECT);
-	CMOS_WRITE((save_freq_select|RTC_DIV_RESET2), RTC_FREQ_SELECT);
-
-	cmos_minutes = CMOS_READ(RTC_MINUTES);
-	if (!(save_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD)
-		cmos_minutes = bcd2bin(cmos_minutes);
-
-	/*
-	 * since we're only adjusting minutes and seconds,
-	 * don't interfere with hour overflow. This avoids
-	 * messing with unknown time zones but requires your
-	 * RTC not to be off by more than 15 minutes
-	 */
-	real_seconds = nowtime % 60;
-	real_minutes = nowtime / 60;
-	/* correct for half hour time zone */
-	if (((abs(real_minutes - cmos_minutes) + 15)/30) & 1)
-		real_minutes += 30;
-	real_minutes %= 60;
-
-	if (abs(real_minutes - cmos_minutes) < 30) {
-		if (!(save_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD) {
-			real_seconds = bin2bcd(real_seconds);
-			real_minutes = bin2bcd(real_minutes);
-		}
-		CMOS_WRITE(real_seconds, RTC_SECONDS);
-		CMOS_WRITE(real_minutes, RTC_MINUTES);
+	rtc_time_to_tm(nowtime, &tm);
+	if (!rtc_valid_tm(&tm)) {
+		retval = set_rtc_time(&tm);
+		if (retval)
+			printk(KERN_ERR "%s: RTC write failed with error %d\n",
+			       __FUNCTION__, retval);
 	} else {
-		printk_once(KERN_NOTICE
-		       "set_rtc_mmss: can't update from %d to %d\n",
-		       cmos_minutes, real_minutes);
-		retval = -1;
+		printk(KERN_ERR
+		       "%s: Invalid RTC value: write of %lx to RTC failed\n",
+			__FUNCTION__, nowtime);
+		retval = -EINVAL;
 	}
-
-	/* The following flags have to be released exactly in this order,
-	 * otherwise the DS12887 (popular MC146818A clone with integrated
-	 * battery and quartz) will not reset the oscillator and will not
-	 * update precisely 500 ms later. You won't find this mentioned in
-	 * the Dallas Semiconductor data sheets, but who believes data
-	 * sheets anyway ...                           -- Markus Kuhn
-	 */
-	CMOS_WRITE(save_control, RTC_CONTROL);
-	CMOS_WRITE(save_freq_select, RTC_FREQ_SELECT);
-
-	spin_unlock_irqrestore(&rtc_lock, flags);
-
 	return retval;
 }
 
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 77cf009..6ca930e 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -48,6 +48,7 @@
 #include <asm/cacheflush.h>
 #include <asm/tlbflush.h>
 #include <asm/x86_init.h>
+#include <asm/rtc.h>
 
 #define EFI_DEBUG	1
 
@@ -262,10 +263,10 @@ static efi_status_t __init phys_efi_get_time(efi_time_t *tm,
 
 int efi_set_rtc_mmss(unsigned long nowtime)
 {
-	int real_seconds, real_minutes;
 	efi_status_t 	status;
 	efi_time_t 	eft;
 	efi_time_cap_t 	cap;
+	struct rtc_time	tm;
 
 	status = efi.get_time(&eft, &cap);
 	if (status != EFI_SUCCESS) {
@@ -273,13 +274,20 @@ int efi_set_rtc_mmss(unsigned long nowtime)
 		return -1;
 	}
 
-	real_seconds = nowtime % 60;
-	real_minutes = nowtime / 60;
-	if (((abs(real_minutes - eft.minute) + 15)/30) & 1)
-		real_minutes += 30;
-	real_minutes %= 60;
-	eft.minute = real_minutes;
-	eft.second = real_seconds;
+	rtc_time_to_tm(nowtime, &tm);
+	if (!rtc_valid_tm(&tm)) {
+		eft.year = tm.tm_year + 1900;
+		eft.month = tm.tm_mon + 1;
+		eft.day = tm.tm_mday;
+		eft.minute = tm.tm_min;
+		eft.second = tm.tm_sec;
+		eft.nanosecond = 0;
+	} else {
+		printk(KERN_ERR
+		       "%s: Invalid EFI RTC value: write of %lx to EFI RTC failed\n",
+		       __FUNCTION__, nowtime);
+		return -1;
+	}
 
 	status = efi.set_time(&eft);
 	if (status != EFI_SUCCESS) {
diff --git a/arch/x86/platform/mrst/vrtc.c b/arch/x86/platform/mrst/vrtc.c
index 225bd0f..205b3d0 100644
--- a/arch/x86/platform/mrst/vrtc.c
+++ b/arch/x86/platform/mrst/vrtc.c
@@ -85,26 +85,33 @@ unsigned long vrtc_get_time(void)
 	return mktime(year, mon, mday, hour, min, sec);
 }
 
-/* Only care about the minutes and seconds */
 int vrtc_set_mmss(unsigned long nowtime)
 {
-	int real_sec, real_min;
 	unsigned long flags;
-	int vrtc_min;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	vrtc_min = vrtc_cmos_read(RTC_MINUTES);
-
-	real_sec = nowtime % 60;
-	real_min = nowtime / 60;
-	if (((abs(real_min - vrtc_min) + 15)/30) & 1)
-		real_min += 30;
-	real_min %= 60;
-
-	vrtc_cmos_write(real_sec, RTC_SECONDS);
-	vrtc_cmos_write(real_min, RTC_MINUTES);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-
+	struct rtc_time tm;
+	int year;
+
+	rtc_time_to_tm(nowtime, &tm);
+	if (!rtc_valid_tm(&tm) && tm.year >= 72) {
+		/*
+		 * tm.year is the number of years since 1900, and the
+		 * vrtc need the years since 1972.
+		 */
+		year = tm.year - 72;
+		spin_lock_irqsave(&rtc_lock, flags);
+		vrtc_cmos_write(year, RTC_YEAR);
+		vrtc_cmos_write(tm.mon, RTC_MONTH);
+		vrtc_cmos_write(tm.mday, RTC_DAY_OF_MONTH);
+		vrtc_cmos_write(tm.hour, RTC_HOURS);
+		vrtc_cmos_write(tm.min, RTC_MINUTES);
+		vrtc_cmos_write(tm.sec, RTC_SECONDS);
+		spin_unlock_irqrestore(&rtc_lock, flags);
+	} else {
+		printk(KERN_ERR
+		       "%s: Invalid vRTC value: write of %lx to vRTC failed\n",
+			__FUNCTION__, nowtime);
+		retval = -EINVAL;
+	}
 	return 0;
 }
 
-- 
1.8.1.2

Re: [RFE PATCH 1/2] x86, rtc, ntp, Do full rtc synchronization with ntp

[John Stultz]

On 02/14/2013 09:02 AM, Prarit Bhargava wrote:
> Every 11 minutes ntp attempts to update the x86 rtc with the current
> system time.  Currently, the x86 code only updates the rtc if the system
> time is within +/-15 minutes of the current value of the rtc.  Other
> architectures do a full synchronization and there is no reason that x86
> should be software limited to a 30 minute window.
>
> This patch changes the behavior of the kernel to do a full synchronization
> (year, month, day, hour, minute, and second) of the rtc when ntp requests
> a synchronization between the system time and the rtc.
>
> I've used the RTC library functions in this patchset as they do all the
> required bounds checking.
>
> Signed-off-by: Prarit Bhargava <prarit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Cc: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: David Vrabel <david.vrabel-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Andi Kleen <ak-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org


This looks reasonable to me.

Though I want to make sure we get this thoroughly tested by the various 
distros so we don't surprise anyone, since it has to potential to cause 
problems where folks are dualbooting windows (using a localtime RTC) and 
do not have their OS setup to trigger warp_clock to adjust for the 
localtime rtc (instead getting a time correction later via NTP).

I'll queue it and see about getting it merged to -tip & -next. Then 
we'll have to decide if 3.10 or 3.11 is the right time frame to land it.

thanks
-john

Re: [RFE PATCH 1/2] x86, rtc, ntp, Do full rtc synchronization with ntp

[Prarit Bhargava]

On 02/22/2013 03:42 PM, John Stultz wrote:
> 
> 
> This looks reasonable to me.
> 
> Though I want to make sure we get this thoroughly tested by the various distros
> so we don't surprise anyone, since it has to potential to cause problems where
> folks are dualbooting windows (using a localtime RTC) and do not have their OS
> setup to trigger warp_clock to adjust for the localtime rtc (instead getting a
> time correction later via NTP).
> 
> I'll queue it and see about getting it merged to -tip & -next. Then we'll have
> to decide if 3.10 or 3.11 is the right time frame to land it.
> 

cc'ing Alessandro as well.

John, I've been testing this across various systems (including those known to
have some wonkiness with the RTC in BIOS ... see comment below) to see if this
code impacts anything.

I've tested mainly using Fedora 18 (with the latest kernel -tip obviously), but
I also installed Ubuntu on a system to see if there was any noticeable impact
there too.  I have not seen any unusual testing failures on AMD or Intel systems.

On the one system which I know to have "weak" battery such that the RTC doesn't
"stick" on the system shutdown the clock resets itself in BIOS reboot to "Jan 1
1970".  When I tested previously I could not get the RTC written to the current
date; after my changes, the RTC does at least reflect the current date through a
reboot.  It should be noted that if I do replace the battery on this system I
can get the RTC to properly "stick" through a reboot.

Given the test results I think this should go in earlier rather than later;  I'd
like to target 3.10 for the full sync, and possibly 3.11 for the HCTOSYS stuff.

... unless anyone has a strenuous objection ;)

P.

Re: [RFE PATCH 1/2] x86, rtc, ntp, Do full rtc synchronization with ntp

[Alessandro Zummo]

On Sun, 24 Feb 2013 12:12:57 -0500
Prarit Bhargava <prarit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> Given the test results I think this should go in earlier rather than later;  I'd
> like to target 3.10 for the full sync, and possibly 3.11 for the HCTOSYS stuff.
> 
> ... unless anyone has a strenuous objection ;)

 looks reasonable.

 I prefer == 0 instead of !rtc_valid_tm(&tm),
 but it's just a cosmetic issue.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

[RFE PATCH 2/2] rtc, add write functionality to sysfs

[Prarit Bhargava]

/sys/class/rtc/rtcX/date and /sys/class/rtc/rtcX/time currently have
read-only access.  This patch introduces write functionality which will
set the rtc time.

Usage: echo YYYY-MM-DD > /sys/class/rtc/rtcX/date
       echo HH:MM:SS > /sys/class/rtc/rtcX/time

Signed-off-by: Prarit Bhargava <prarit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Cc: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Cc: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: David Vrabel <david.vrabel-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Andi Kleen <ak-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 drivers/rtc/rtc-sysfs.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
index b70e2bb..87d63d7 100644
--- a/drivers/rtc/rtc-sysfs.c
+++ b/drivers/rtc/rtc-sysfs.c
@@ -48,6 +48,46 @@ rtc_sysfs_show_date(struct device *dev, struct device_attribute *attr,
 }
 
 static ssize_t
+rtc_sysfs_store_date(struct device *dev, struct device_attribute *attr,
+		     const char *buf, size_t count)
+{
+	ssize_t retval;
+	struct rtc_time tm;
+	char tmp[11] = "\0", *tmpptr = tmp;
+	char *cyear, *cmonth, *cday;
+	int year, month, day;
+
+	if (!capable(CAP_SYS_TIME))
+		return -EPERM;
+
+	/* from rtc_sysfs_show_date(): date string format is YYYY-MM-DD */
+	if (strlen(buf) != 11)
+		return -EINVAL;
+
+	strncpy(tmp, buf, 11);
+	cyear = strsep(&tmpptr, "-");
+	sscanf(cyear, "%i", &year);
+	cmonth = strsep(&tmpptr, "-");
+	sscanf(cmonth, "%i", &month);
+	cday = strsep(&tmpptr, "-");
+	sscanf(cday, "%i", &day);
+
+	retval = rtc_read_time(to_rtc_device(dev), &tm);
+	if (retval)
+		return -ENODEV;
+
+	tm.tm_year = year - 1900;
+	tm.tm_mon = month - 1;
+	tm.tm_mday = day;
+
+	retval = rtc_set_time(to_rtc_device(dev), &tm);
+	if (retval)
+		return retval;
+
+	return count;
+}
+
+static ssize_t
 rtc_sysfs_show_time(struct device *dev, struct device_attribute *attr,
 		char *buf)
 {
@@ -64,6 +104,46 @@ rtc_sysfs_show_time(struct device *dev, struct device_attribute *attr,
 }
 
 static ssize_t
+rtc_sysfs_store_time(struct device *dev, struct device_attribute *attr,
+		     const char *buf, size_t count)
+{
+	ssize_t retval;
+	struct rtc_time tm;
+	char tmp[9] = "\0", *tmpptr = tmp;
+	char *chour, *cmin, *csec;
+	int hour, min, sec;
+
+	if (!capable(CAP_SYS_TIME))
+		return -EPERM;
+
+	/* from rtc_sysfs_show_time(): time string format is HH:MM:SS */
+	if (strlen(buf) != 9)
+		return -EINVAL;
+
+	strncpy(tmp, buf, 9);
+	chour = strsep(&tmpptr, ":");
+	sscanf(chour, "%i", &hour);
+	cmin = strsep(&tmpptr, ":");
+	sscanf(cmin, "%i", &min);
+	csec = strsep(&tmpptr, ":");
+	sscanf(csec, "%i", &sec);
+
+	retval = rtc_read_time(to_rtc_device(dev), &tm);
+	if (retval)
+		return -ENODEV;
+
+	tm.tm_hour = hour;
+	tm.tm_min = min;
+	tm.tm_sec = sec;
+
+	retval = rtc_set_time(to_rtc_device(dev), &tm);
+	if (retval)
+		return retval;
+
+	return count;
+}
+
+static ssize_t
 rtc_sysfs_show_since_epoch(struct device *dev, struct device_attribute *attr,
 		char *buf)
 {
@@ -124,8 +204,10 @@ rtc_sysfs_show_hctosys(struct device *dev, struct device_attribute *attr,
 
 static struct device_attribute rtc_attrs[] = {
 	__ATTR(name, S_IRUGO, rtc_sysfs_show_name, NULL),
-	__ATTR(date, S_IRUGO, rtc_sysfs_show_date, NULL),
-	__ATTR(time, S_IRUGO, rtc_sysfs_show_time, NULL),
+	__ATTR(date, S_IRUGO | S_IWUGO, rtc_sysfs_show_date,
+	       rtc_sysfs_store_date),
+	__ATTR(time, S_IRUGO | S_IWUGO, rtc_sysfs_show_time,
+	       rtc_sysfs_store_time),
 	__ATTR(since_epoch, S_IRUGO, rtc_sysfs_show_since_epoch, NULL),
 	__ATTR(max_user_freq, S_IRUGO | S_IWUSR, rtc_sysfs_show_max_user_freq,
 			rtc_sysfs_set_max_user_freq),
-- 
1.8.1.2

Re: [RFE PATCH 2/2] rtc, add write functionality to sysfs

[John Stultz]

On 02/14/2013 09:02 AM, Prarit Bhargava wrote:
> /sys/class/rtc/rtcX/date and /sys/class/rtc/rtcX/time currently have
> read-only access.  This patch introduces write functionality which will
> set the rtc time.
>
> Usage: echo YYYY-MM-DD > /sys/class/rtc/rtcX/date
>         echo HH:MM:SS > /sys/class/rtc/rtcX/time

Why do we want to add a new interface here?

thanks
-john

Re: [RFE PATCH 2/2] rtc, add write functionality to sysfs

[Prarit Bhargava]

On 02/22/2013 03:43 PM, John Stultz wrote:
> On 02/14/2013 09:02 AM, Prarit Bhargava wrote:
>> /sys/class/rtc/rtcX/date and /sys/class/rtc/rtcX/time currently have
>> read-only access.  This patch introduces write functionality which will
>> set the rtc time.
>>
>> Usage: echo YYYY-MM-DD > /sys/class/rtc/rtcX/date
>>         echo HH:MM:SS > /sys/class/rtc/rtcX/time
> 
> Why do we want to add a new interface here?

John,

I'm not adding a new interface.  The current date/time interface only handles
read and I'm introducing write.

Sorry -- maybe my description was too short ...

P.

> 
> thanks
> -john
> 

Re: [RFE PATCH 2/2] rtc, add write functionality to sysfs

[John Stultz]

On 02/22/2013 12:55 PM, Prarit Bhargava wrote:
>
> On 02/22/2013 03:43 PM, John Stultz wrote:
>> On 02/14/2013 09:02 AM, Prarit Bhargava wrote:
>>> /sys/class/rtc/rtcX/date and /sys/class/rtc/rtcX/time currently have
>>> read-only access.  This patch introduces write functionality which will
>>> set the rtc time.
>>>
>>> Usage: echo YYYY-MM-DD > /sys/class/rtc/rtcX/date
>>>          echo HH:MM:SS > /sys/class/rtc/rtcX/time
>> Why do we want to add a new interface here?
> John,
>
> I'm not adding a new interface.  The current date/time interface only handles
> read and I'm introducing write.
>

Right, but what benefit does that provide?
(I'm not saying there isn't any, its just not clear from your patch why 
this is a good thing.)

Also CC'ing Alessandro for his input.

thanks
-john

Re: [RFE PATCH 2/2] rtc, add write functionality to sysfs

[Alessandro Zummo]

On 22/feb/2013, at 22:05, John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 02/22/2013 12:55 PM, Prarit Bhargava wrote:
>> 
>> On 02/22/2013 03:43 PM, John Stultz wrote:
>>> On 02/14/2013 09:02 AM, Prarit Bhargava wrote:
>>>> /sys/class/rtc/rtcX/date and /sys/class/rtc/rtcX/time currently have
>>>> read-only access.  This patch introduces write functionality which will
>>>> set the rtc time.
>>>> 
>>>> Usage: echo YYYY-MM-DD > /sys/class/rtc/rtcX/date
>>>>         echo HH:MM:SS > /sys/class/rtc/rtcX/time
>>> Why do we want to add a new interface here?
>> John,
>> 
>> I'm not adding a new interface.  The current date/time interface only handles
>> read and I'm introducing write.
>> 
> 
> Right, but what benefit does that provide?
> (I'm not saying there isn't any, its just not clear from your patch why this is a good thing.)
> 
> Also CC'ing Alessandro for his input.

I'd like to keep the interfaces as simple as possible but I'm open to improvements if there are good use cases.

Re: [RFE PATCH 2/2] rtc, add write functionality to sysfs

[Prarit Bhargava]

On 02/23/2013 06:11 PM, Alessandro Zummo wrote:
> On 22/feb/2013, at 22:05, John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
>> On 02/22/2013 12:55 PM, Prarit Bhargava wrote:
>>>
>>> On 02/22/2013 03:43 PM, John Stultz wrote:
>>>> On 02/14/2013 09:02 AM, Prarit Bhargava wrote:
>>>>> /sys/class/rtc/rtcX/date and /sys/class/rtc/rtcX/time currently have
>>>>> read-only access.  This patch introduces write functionality which will
>>>>> set the rtc time.
>>>>>
>>>>> Usage: echo YYYY-MM-DD > /sys/class/rtc/rtcX/date
>>>>>         echo HH:MM:SS > /sys/class/rtc/rtcX/time
>>>> Why do we want to add a new interface here?
>>> John,
>>>
>>> I'm not adding a new interface.  The current date/time interface only handles
>>> read and I'm introducing write.
>>>
>>
>> Right, but what benefit does that provide?
>> (I'm not saying there isn't any, its just not clear from your patch why this is a good thing.)
>>

Sorry John, I misunderstood your question.

>> Also CC'ing Alessandro for his input.
> 
> I'd like to keep the interfaces as simple as possible but I'm open to improvements if there are good use cases.
> 

AFAICT there is no way for me to "test" or use the write from userspace.
hwclock uses the SET_TIME ioctl, which is a different code path AFAICT.

I'd like to be at least able to test this stuff when we make changes to it so I
think having write functionality for date & time is worthwhile.

For me, I'm using these to heavily test ntp and ntpdate over system reboots.

OOC, Alessandro, why is the date & time split into two fields?

P.

Re: [RFE PATCH 2/2] rtc, add write functionality to sysfs

[Alessandro Zummo]

On Sun, 24 Feb 2013 12:03:01 -0500
Prarit Bhargava <prarit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> 
> AFAICT there is no way for me to "test" or use the write from userspace.
> hwclock uses the SET_TIME ioctl, which is a different code path AFAICT.
> 
> I'd like to be at least able to test this stuff when we make changes to it so I
> think having write functionality for date & time is worthwhile.
> 
> For me, I'm using these to heavily test ntp and ntpdate over system reboots.

 the point is: who will benefit from this patch? users? distributions?
 embedded distributions? if it's useful, then just go for it.


> OOC, Alessandro, why is the date & time split into two fields?

 because date and time are two different things and we expect
 sysfs to preferably have one value for each entry.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: [RFE PATCH 2/2] rtc, add write functionality to sysfs

[Prarit Bhargava]

On 02/25/2013 09:58 AM, Alessandro Zummo wrote:
> On Sun, 24 Feb 2013 12:03:01 -0500
> Prarit Bhargava <prarit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
>>
>> AFAICT there is no way for me to "test" or use the write from userspace.
>> hwclock uses the SET_TIME ioctl, which is a different code path AFAICT.
>>
>> I'd like to be at least able to test this stuff when we make changes to it so I
>> think having write functionality for date & time is worthwhile.
>>
>> For me, I'm using these to heavily test ntp and ntpdate over system reboots.
> 
>  the point is: who will benefit from this patch? users? distributions?
>  embedded distributions? if it's useful, then just go for it.

This can be dropped IMO.

P.

> 
> 
>> OOC, Alessandro, why is the date & time split into two fields?
> 
>  because date and time are two different things and we expect
>  sysfs to preferably have one value for each entry.
> 

Re: [RFE PATCH 2/2] rtc, add write functionality to sysfs

[John Stultz]

On 02/24/2013 09:03 AM, Prarit Bhargava wrote:
>
> On 02/23/2013 06:11 PM, Alessandro Zummo wrote:
>> On 22/feb/2013, at 22:05, John Stultz <john.stultz@linaro.org> wrote:
>>
>>> On 02/22/2013 12:55 PM, Prarit Bhargava wrote:
>>>> On 02/22/2013 03:43 PM, John Stultz wrote:
>>>>> On 02/14/2013 09:02 AM, Prarit Bhargava wrote:
>>>>>> /sys/class/rtc/rtcX/date and /sys/class/rtc/rtcX/time currently have
>>>>>> read-only access.  This patch introduces write functionality which will
>>>>>> set the rtc time.
>>>>>>
>>>>>> Usage: echo YYYY-MM-DD > /sys/class/rtc/rtcX/date
>>>>>>          echo HH:MM:SS > /sys/class/rtc/rtcX/time
>>>>> Why do we want to add a new interface here?
>>>> John,
>>>>
>>>> I'm not adding a new interface.  The current date/time interface only handles
>>>> read and I'm introducing write.
>>>>
>>> Right, but what benefit does that provide?
>>> (I'm not saying there isn't any, its just not clear from your patch why this is a good thing.)
>>>
> Sorry John, I misunderstood your question.
>
>>> Also CC'ing Alessandro for his input.
>> I'd like to keep the interfaces as simple as possible but I'm open to improvements if there are good use cases.
>>
> AFAICT there is no way for me to "test" or use the write from userspace.
> hwclock uses the SET_TIME ioctl, which is a different code path AFAICT.

Yea, per your other email, I'd rather we drop this change for now.

If possible, instead of adding redundant userland interfaces, I'd rather 
we consolidate the redundant internal code paths.

thanks
-john