rtc/hctosys.c Problem during kernel boot

rtc/hctosys.c Problem during kernel boot

[John Whitmore]

I'm having a problem with a DS3234 SPI based RTC chip and rtc/hctosys.c on the
3.10.29 kernel of the RaspberryPi. I'm not sure this is a bug or not but
thought I'd ask. I've enabled the kernel config option for HCTOSYS which, on
boot, should set the system's date/time to the value read from the RTC. I
tried tihs but it would never happen on the RPi. I eventually found in syslog
that the kernel boot is attempting to execute the hctosys functionality prior
to the SPI being initialised. As a result of this when hctosys is attempted
there is not /dev/rtc0 yet. A short time later the DS3234 RTC is initialised
but by then it's too late.

Once the system has booted and I've logged in I can read and write to the RTC
and all seems good but /sys/class/rtc/rtc0/hctosys is '0' indicating that the
system time was not set on boot.

There is a "deprecated" warning in the syslog coming from the spi of the board
file so perhaps that is the cause. So is this a bug? And if so what can I do
to resolve it. The hctosys is on a "late_initcall" so not sure of timing.

I'll include the syslog from the failed call to hctosys:

Jun 11 23:14:07 raspberrypi kernel: [    2.205432] drivers/rtc/hctosys.c: unable to open rtc device (rtc0)
Jun 11 23:14:07 raspberrypi kernel: [    2.225179] mmcblk0: mmc0:0007 SD08G 7.42 GiB 
Jun 11 23:14:07 raspberrypi kernel: [    2.234058]  mmcblk0: p1 p2
Jun 11 23:14:07 raspberrypi kernel: [    2.364955] usb 1-1: new high-speed USB device number 2 using dwc_otg
Jun 11 23:14:07 raspberrypi kernel: [    2.373061] Indeed it is in host mode hprt0 = 00001101
Jun 11 23:14:07 raspberrypi kernel: [    2.575396] usb 1-1: New USB device found, idVendor=0424, idProduct=9512
Jun 11 23:14:07 raspberrypi kernel: [    2.583608] usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
Jun 11 23:14:07 raspberrypi kernel: [    2.593237] hub 1-1:1.0: USB hub found
Jun 11 23:14:07 raspberrypi kernel: [    2.598772] hub 1-1:1.0: 3 ports detected
Jun 11 23:14:07 raspberrypi kernel: [    2.885152] usb 1-1.1: new high-speed USB device number 3 using dwc_otg
Jun 11 23:14:07 raspberrypi kernel: [    2.968801] EXT4-fs (mmcblk0p2): mounted filesystem with ordered data mode. Opts: (null)
Jun 11 23:14:07 raspberrypi kernel: [    2.980068] VFS: Mounted root (ext4 filesystem) on device 179:2.
Jun 11 23:14:07 raspberrypi kernel: [    2.991031] devtmpfs: mounted
Jun 11 23:14:07 raspberrypi kernel: [    2.996101] Freeing unused kernel memory: 132K (c0545000 - c0566000)
Jun 11 23:14:07 raspberrypi kernel: [    3.005575] usb 1-1.1: New USB device found, idVendor=0424, idProduct=ec00
Jun 11 23:14:07 raspberrypi kernel: [    3.014147] usb 1-1.1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
Jun 11 23:14:07 raspberrypi kernel: [    3.027128] smsc95xx v1.0.4
Jun 11 23:14:07 raspberrypi kernel: [    3.091322] smsc95xx 1-1.1:1.0 eth0: register 'smsc95xx' at usb-bcm2708_usb-1.1, smsc95xx USB 2.0 Ethernet, b8:27:eb:7b:7f:02
Jun 11 23:14:07 raspberrypi kernel: [    5.635302] bcm2708_spi bcm2708_spi.0: master is unqueued, this is deprecated
Jun 11 23:14:07 raspberrypi kernel: [    5.771144] ds3234 spi0.1: Control Reg: 0x1c
Jun 11 23:14:07 raspberrypi kernel: [    5.850125] ds3234 spi0.1: Ctrl/Stat Reg: 0x88
Jun 11 23:14:07 raspberrypi kernel: [    5.891108] rtc rtc0: ds3234: dev (254:0)
Jun 11 23:14:07 raspberrypi kernel: [    5.891209] ds3234 spi0.1: rtc core: registered ds3234 as rtc0
Jun 11 23:14:07 raspberrypi kernel: [    5.937290] bcm2708_spi bcm2708_spi.0: SPI Controller at 0x20204000 (irq 80)
Jun 11 23:14:07 raspberrypi kernel: [    8.031796] mcp251x spi0.0: CANSTAT 0x80 CANCTRL 0x07
Jun 11 23:14:07 raspberrypi kernel: [    8.034590] mcp251x spi0.0: probed
 

Re: rtc/hctosys.c Problem during kernel boot

[John Stultz]

On Wed, Jun 11, 2014 at 4:01 PM, John Whitmore <arigead@gmail.com> wrote:
> I'm having a problem with a DS3234 SPI based RTC chip and rtc/hctosys.c on the
> 3.10.29 kernel of the RaspberryPi. I'm not sure this is a bug or not but
> thought I'd ask. I've enabled the kernel config option for HCTOSYS which, on
> boot, should set the system's date/time to the value read from the RTC. I
> tried tihs but it would never happen on the RPi. I eventually found in syslog
> that the kernel boot is attempting to execute the hctosys functionality prior
> to the SPI being initialised. As a result of this when hctosys is attempted
> there is not /dev/rtc0 yet. A short time later the DS3234 RTC is initialised
> but by then it's too late.
>
> Once the system has booted and I've logged in I can read and write to the RTC
> and all seems good but /sys/class/rtc/rtc0/hctosys is '0' indicating that the
> system time was not set on boot.
>
> There is a "deprecated" warning in the syslog coming from the spi of the board
> file so perhaps that is the cause. So is this a bug? And if so what can I do
> to resolve it. The hctosys is on a "late_initcall" so not sure of timing.

Sigh. Yea, this issue was brought up previously, but we never got
around to a solution that could be merged.

Basically hctosys is late_init, but if the driver is a module, it
might not be loaded in time. Adding hooks at module load time when
RTCs are registered could be done, but then you have the issue that
userspace might have set the clock via something like ntpdate, so
HCTOSYS could then cause the clock to be less accurate.

So we need to make the HCTOSYS functionality happen at RTC register
time, but it needs to set the clock only if nothing has set the clock
already. This requires a new timekeeeping interface - something like
timekeeping_set_time_if_unset(), which atomically would set the time
if it has never been set.

You can read some of the previous discussion here:
https://lkml.org/lkml/2013/6/17/533

I'd be very interested in patches to resolve this!

thanks
-john

Re: rtc/hctosys.c Problem during kernel boot

[John Whitmore]

On Wed, Jun 11, 2014 at 04:53:55PM -0700, John Stultz wrote:
> On Wed, Jun 11, 2014 at 4:01 PM, John Whitmore <arigead@gmail.com> wrote:
> > I'm having a problem with a DS3234 SPI based RTC chip and rtc/hctosys.c on the
> > 3.10.29 kernel of the RaspberryPi. I'm not sure this is a bug or not but
> > thought I'd ask. I've enabled the kernel config option for HCTOSYS which, on
> > boot, should set the system's date/time to the value read from the RTC. I
> > tried tihs but it would never happen on the RPi. I eventually found in syslog
> > that the kernel boot is attempting to execute the hctosys functionality prior
> > to the SPI being initialised. As a result of this when hctosys is attempted
> > there is not /dev/rtc0 yet. A short time later the DS3234 RTC is initialised
> > but by then it's too late.
> >
> > Once the system has booted and I've logged in I can read and write to the RTC
> > and all seems good but /sys/class/rtc/rtc0/hctosys is '0' indicating that the
> > system time was not set on boot.
> >
> > There is a "deprecated" warning in the syslog coming from the spi of the board
> > file so perhaps that is the cause. So is this a bug? And if so what can I do
> > to resolve it. The hctosys is on a "late_initcall" so not sure of timing.
> 
> Sigh. Yea, this issue was brought up previously, but we never got
> around to a solution that could be merged.
> 
> Basically hctosys is late_init, but if the driver is a module, it
> might not be loaded in time. Adding hooks at module load time when
> RTCs are registered could be done, but then you have the issue that
> userspace might have set the clock via something like ntpdate, so
> HCTOSYS could then cause the clock to be less accurate.
> 
> So we need to make the HCTOSYS functionality happen at RTC register
> time, but it needs to set the clock only if nothing has set the clock
> already. This requires a new timekeeeping interface - something like
> timekeeping_set_time_if_unset(), which atomically would set the time
> if it has never been set.
> 
> You can read some of the previous discussion here:
> https://lkml.org/lkml/2013/6/17/533
> 

Thanks a million for that information I'll have a look, as I might try and
resolve the issue.

> I'd be very interested in patches to resolve this!
> 
> thanks
> -john

Re: rtc/hctosys.c Problem during kernel boot

[Alexander Holler]

Am 12.06.2014 01:53, schrieb John Stultz:

> Sigh. Yea, this issue was brought up previously, but we never got
> around to a solution that could be merged.
>
> Basically hctosys is late_init, but if the driver is a module, it
> might not be loaded in time. Adding hooks at module load time when
> RTCs are registered could be done, but then you have the issue that
> userspace might have set the clock via something like ntpdate, so
> HCTOSYS could then cause the clock to be less accurate.
>
> So we need to make the HCTOSYS functionality happen at RTC register
> time, but it needs to set the clock only if nothing has set the clock
> already. This requires a new timekeeeping interface - something like
> timekeeping_set_time_if_unset(), which atomically would set the time
> if it has never been set.
>
> You can read some of the previous discussion here:
> https://lkml.org/lkml/2013/6/17/533
>
> I'd be very interested in patches to resolve this!

Hmm, I'm still using this patches, now with 3.15 on a bunch of very 
different x86 and arm boxes. So they are now tested since a year.

Unfortunately my expieriences with Linux kernel maintainers became even 
worse and I'm not very eager to post patches. But if someone wants these 
patches based on 3.15, feel free to notice me.

To get rid of hctosys, I have 3 patches, and 2 more to implement 
rtc_read_timeval() for higher resolution clocks.

Regards,

Alexander Holler

Re: rtc/hctosys.c Problem during kernel boot

[Alessandro Zummo]

On Thu, 12 Jun 2014 14:01:46 +0200
Alexander Holler <holler@ahsoftware.de> wrote:

> Unfortunately my expieriences with Linux kernel maintainers became even 
> worse and I'm not very eager to post patches. But if someone wants these 
> patches based on 3.15, feel free to notice me.
> 
> To get rid of hctosys, I have 3 patches, and 2 more to implement 
> rtc_read_timeval() for higher resolution clocks.

 Yes, I know the experience might be a painful one.
 Please send the patches and let's see if they fix the issue for John too.
 
-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

[PATCH 1/3] RFC: timekeeping: introduce flag systime_was_set

[Alexander Holler]

In order to let an RTC set the time at boot without the problem that a
second RTC overwrites it, the flag systime_was_set is introduced.

systime_was_set will be true, if a persistent clock sets the time at boot,
or if do_settimeofday() is called (e.g. by the RTC subsystem or userspace).

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---

 I've based these patches on 3.15, an older version can be found at LKML,

 Besides discussing possible *real* bugs, I don't care what any person
 (including maintainers) will request. I'm fine with these patches (I'm
 using them since a year) and I don't play remote keyboard or
 patch ping-pong. If someone want changes I suggest he gets responsible
 for them himself and just puts a patch on top of my patches. And in any
 case, feel free to completely ignore these patches.

 (This note will destroy itself when using git am.)

 include/linux/time.h      |  6 ++++++
 kernel/time/timekeeping.c | 10 +++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/linux/time.h b/include/linux/time.h
index d5d229b..888280f 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct timespec now);
 void timekeeping_init(void);
 extern int timekeeping_suspended;
 
+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
 unsigned long get_seconds(void);
 struct timespec current_kernel_time(void);
 struct timespec __current_kernel_time(void); /* does not take xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index f7df8ea..6be8b72 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -43,6 +43,9 @@ int __read_mostly timekeeping_suspended;
 /* Flag for if there is a persistent clock on this platform */
 bool __read_mostly persistent_clock_exist = false;
 
+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set;
+
 static inline void tk_normalize_xtime(struct timekeeper *tk)
 {
 	while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
@@ -505,6 +508,9 @@ int do_settimeofday(const struct timespec *tv)
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
 	write_seqcount_begin(&timekeeper_seq);
 
+	systime_was_set = true;
+
+
 	timekeeping_forward_now(tk);
 
 	xt = tk_xtime(tk);
@@ -799,8 +805,10 @@ void __init timekeeping_init(void)
 			"         Check your CMOS/BIOS settings.\n");
 		now.tv_sec = 0;
 		now.tv_nsec = 0;
-	} else if (now.tv_sec || now.tv_nsec)
+	} else if (now.tv_sec || now.tv_nsec) {
 		persistent_clock_exist = true;
+		systime_was_set = true;
+	}
 
 	read_boot_clock(&boot);
 	if (!timespec_valid_strict(&boot)) {
-- 
1.8.3.2

[PATCH 2/3] RFC: timekeeping: rtc: Introduce new kernel parameter hctosys

[Alexander Holler]

hctosys= specifies the driver (RTC) name which sets the system clock at
boot, if and only if userspace hasn't set the time before the driver will
be loaded.

If hctosys will not be specified, the first available hardware clock
with a valid time will be used (again, if and only if ...).

If you don't want that the system clock will be set by any hardware clock,
just specify a non-existent RTC driver name, e.g. with hctosys=none.

Currently there exist a special name "persistent" for the persistent clock
found on some systems (e.g. the CMOS clock on x86 platforms which might be
handled by the driver named rtc_cmos too).

This will replace the existent driver/mechanism hctosys and the kernel
config options CONFIG_RTC_HCTOSYS and CONFIG_RTC_HCTOSYS_DEVICE (done
with one of the following patches)

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---

 I've based these patches on 3.15, an older version can be found at LKML,

 Besides discussing possible *real* bugs, I don't care what any person
 (including maintainers) will request. I'm fine with these patches (I'm
 using them since a year) and I don't play remote keyboard or
 patch ping-pong. If someone want changes I suggest he gets responsible
 for them himself and just puts a patch on top of my patches. And in any
 case, feel free to completely ignore these patches.

 (This note will destroy itself when using git am.)

 Documentation/kernel-parameters.txt | 12 +++++++++
 drivers/rtc/class.c                 | 42 +++++++++++++++++++++++++++++
 include/linux/time.h                |  6 +++++
 kernel/time/timekeeping.c           | 53 ++++++++++++++++++++++++++++---------
 4 files changed, 101 insertions(+), 12 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 30a8ad0d..77a36ba 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1110,6 +1110,18 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 	hcl=		[IA-64] SGI's Hardware Graph compatibility layer
 
+	hctosys=	[KNL] Specifies the driver (RTC) name which sets the
+			time at	boot, if and only if userspace hasn't set the
+			time before the driver will be loaded. If hctosys will
+			not be specified, the first available hardware clock
+			with a valid time will be used.
+			Use a non-existent name (e.g. hctosys=none) if you want
+			to avoid that a hardware clock will set the time.
+			Currently there exist a special name "persistent" for
+			the persistent clock found on some systems (e.g. the
+			CMOS clock on x86 platforms which might be handled
+			by the driver named rtc_cmos too).
+
 	hd=		[EIDE] (E)IDE hard drive subsystem geometry
 			Format: <cyl>,<head>,<sect>
 
diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 589351e..18c47b0 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -143,6 +143,43 @@ static SIMPLE_DEV_PM_OPS(rtc_class_dev_pm_ops, rtc_suspend, rtc_resume);
 #endif
 
 
+static void hctosys(struct rtc_device *rtc)
+{
+	struct rtc_time now;
+	struct timespec ts = {
+		.tv_nsec = NSEC_PER_SEC >> 1,
+	};
+	int rc;
+
+	rc = rtc_read_time(rtc, &now);
+	if (unlikely(rc)) {
+		dev_err(rtc->dev.parent,
+			"rtc core: error reading time from RTC: %d\n", rc);
+		return;
+	}
+	rc = rtc_valid_tm(&now);
+	if (unlikely(rc)) {
+		dev_err(rtc->dev.parent,
+			"rtc core: timestamp from RTC is invalid\n");
+		return;
+	}
+	rtc_tm_to_time(&now, &ts.tv_sec);
+	if (systime_was_set)
+		return;
+	rc = do_settimeofday(&ts);
+	if (unlikely(rc)) {
+		dev_err(rtc->dev.parent,
+			"rtc core: error setting system clock: %d\n", rc);
+		return;
+	} else if (systime_was_set)
+		dev_info(rtc->dev.parent,
+			"rtc core: setting system clock to "
+			"%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
+			now.tm_year + 1900, now.tm_mon + 1, now.tm_mday,
+			now.tm_hour, now.tm_min, now.tm_sec,
+			(unsigned int) ts.tv_sec);
+}
+
 /**
  * rtc_device_register - register w/ RTC class
  * @dev: the device to register
@@ -159,6 +196,7 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
 	struct rtc_device *rtc;
 	struct rtc_wkalrm alrm;
 	int of_id = -1, id = -1, err;
+	const char *hctosys_name = get_hctosys_name();
 
 	if (dev->of_node)
 		of_id = of_alias_get_id(dev->of_node, "rtc");
@@ -213,6 +251,10 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
 	rtc->pie_timer.function = rtc_pie_update_irq;
 	rtc->pie_enabled = 0;
 
+	if (!systime_was_set &&
+		(!hctosys_name[0] || !strcasecmp(name, hctosys_name)))
+		hctosys(rtc);
+
 	/* Check to see if there is an ALARM already set in hw */
 	err = __rtc_read_alarm(rtc, &alrm);
 
diff --git a/include/linux/time.h b/include/linux/time.h
index 888280f..e5f644c 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -135,6 +135,12 @@ extern int timekeeping_suspended;
  */
 extern bool systime_was_set;
 
+/*
+ * Returns a pointer to the string specified with
+ * hctosys= at the kernel command line.
+ */
+const char *get_hctosys_name(void);
+
 unsigned long get_seconds(void);
 struct timespec current_kernel_time(void);
 struct timespec __current_kernel_time(void); /* does not take xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 6be8b72..c8992e4 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -18,6 +18,7 @@
 #include <linux/syscore_ops.h>
 #include <linux/clocksource.h>
 #include <linux/jiffies.h>
+#include <linux/rtc.h>
 #include <linux/time.h>
 #include <linux/tick.h>
 #include <linux/stop_machine.h>
@@ -46,6 +47,22 @@ bool __read_mostly persistent_clock_exist = false;
 /* Flag for if the system time was set at least once */
 bool __read_mostly systime_was_set;
 
+static char hctosys_name[RTC_DEVICE_NAME_SIZE];
+
+static int __init hctosys_setup(char *line)
+{
+	strlcpy(hctosys_name, line, sizeof(hctosys_name));
+	return 1;
+}
+
+__setup("hctosys=", hctosys_setup);
+
+const char *get_hctosys_name(void)
+{
+	return hctosys_name;
+}
+EXPORT_SYMBOL_GPL(get_hctosys_name);
+
 static inline void tk_normalize_xtime(struct timekeeper *tk)
 {
 	while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
@@ -796,18 +813,19 @@ void __init timekeeping_init(void)
 	struct timekeeper *tk = &timekeeper;
 	struct clocksource *clock;
 	unsigned long flags;
-	struct timespec now, boot, tmp;
-
-	read_persistent_clock(&now);
-
-	if (!timespec_valid_strict(&now)) {
-		pr_warn("WARNING: Persistent clock returned invalid value!\n"
-			"         Check your CMOS/BIOS settings.\n");
-		now.tv_sec = 0;
-		now.tv_nsec = 0;
-	} else if (now.tv_sec || now.tv_nsec) {
-		persistent_clock_exist = true;
-		systime_was_set = true;
+	struct timespec now = {0, 0}, boot, tmp;
+
+	if (!hctosys_name[0] || !strcasecmp(hctosys_name, "persistent")) {
+		read_persistent_clock(&now);
+		if (!timespec_valid_strict(&now)) {
+			pr_warn("WARNING: Persistent clock returned invalid value!\n"
+				"         Check your CMOS/BIOS settings.\n");
+			now.tv_sec = 0;
+			now.tv_nsec = 0;
+		} else if (now.tv_sec || now.tv_nsec) {
+			persistent_clock_exist = true;
+			systime_was_set = true;
+		}
 	}
 
 	read_boot_clock(&boot);
@@ -844,6 +862,17 @@ void __init timekeeping_init(void)
 
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+
+	if (systime_was_set) {
+		/* print a msg like if the time was set by a RTC */
+		struct tm now_tm;
+		time_to_tm(now.tv_sec, 0, &now_tm);
+		pr_info("persistent clock: setting system clock to "
+			"%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
+			(int)now_tm.tm_year + 1900, now_tm.tm_mon + 1,
+			now_tm.tm_mday,	now_tm.tm_hour, now_tm.tm_min,
+			now_tm.tm_sec, (unsigned int) now.tv_sec);
+	}
 }
 
 /* time in seconds when suspend began */
-- 
1.8.3.2

[PATCH 3/3] RFC: timekeeping: rtc: remove CONFIG_RTC_HCTOSYS and RTC_HCTOSYS_DEVICE

[Alexander Holler]

Those config options don't make sense anymore with the new hctosys
mechanism introduced with the previous patch.

That means two things:

- If a (hardware) clock is available it will be used to set the time at
  boot. This was already the case for system which have a "persistent"
  clock, e.g. most x86 systems. The only way to specify the device used
  for hctosys is now by using the kernel parameter hctosys= introduced
  with a previous patch.

- If a hardware clock was used for hctosys before suspend, this clock
  will be used to adjust the clock at resume. Again, this doesn't change
  anything on systems with a "persistent" clock.

What's missing:

I don't know much about those "persistent" clocks and I haven't had a
deep look at them. That's especially true for the suspend/resume
mechanism used by them. The mechanism I want to use is the following:
The RTC subsystem now maintains the ID of the RTC device which was used
for hctosys (in rtc_hctosys_dev_id) and therefor specifies the device
which should be used to adjust the time after resume. Additionaly the
(new) flag systime_was_set will be set to false at suspend and on resume
this flag will be set to true if either the clock will be adjusted by
the device used for hctosys or by userspace (through do_settimeofday()).

That all should already work as expected for RTCs, what's missing for
"persistent" clocks is that the flag systime_was_set is set to false on
suspend and set to true on resume. Currently it just stays at true
(which is set through hctosys if a "persistent" clock is found.
But because "persistent" clocks don't go away (as it is possible with
RTCs by removing the driver or the RTC itself), nor do "persistent"
clocks might have two instances, this shouldn't be a problem at all.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---

 I've based these patches on 3.15, an older version can be found at LKML,

 Besides discussing possible *real* bugs, I don't care what any person
 (including maintainers) will request. I'm fine with these patches (I'm
 using them since a year) and I don't play remote keyboard or
 patch ping-pong. If someone want changes I suggest he gets responsible
 for them himself and just puts a patch on top of my patches. And in any
 case, feel free to completely ignore these patches.

 (This note will destroy itself when using git am.)

 Documentation/rtc.txt   |  8 +++---
 drivers/rtc/Kconfig     | 35 ++---------------------
 drivers/rtc/Makefile    |  1 -
 drivers/rtc/class.c     | 30 ++++++++++++-------
 drivers/rtc/hctosys.c   | 76 -------------------------------------------------
 drivers/rtc/rtc-proc.c  | 25 ++--------------
 drivers/rtc/rtc-sysfs.c |  6 +---
 drivers/rtc/systohc.c   |  5 +++-
 include/linux/rtc.h     |  7 ++---
 9 files changed, 34 insertions(+), 159 deletions(-)
 delete mode 100644 drivers/rtc/hctosys.c

diff --git a/Documentation/rtc.txt b/Documentation/rtc.txt
index 596b60c..76e031b 100644
--- a/Documentation/rtc.txt
+++ b/Documentation/rtc.txt
@@ -121,8 +121,9 @@ three different userspace interfaces:
 
     *	/proc/driver/rtc ... the system clock RTC may expose itself
 	using a procfs interface. If there is no RTC for the system clock,
-	rtc0 is used by default. More information is (currently) shown
-	here than through sysfs.
+	or a "persistent" clock is used to set the system clock,
+	/proc/driver/rtc will not exist.
+	More information is (currently) shown here than through sysfs.
 
 The RTC Class framework supports a wide variety of RTCs, ranging from those
 integrated into embeddable system-on-chip (SOC) processors to discrete chips
@@ -144,8 +145,7 @@ rtc attributes without requiring the use of ioctls. All dates and times
 are in the RTC's timezone, rather than in system time.
 
 date:  	   	 RTC-provided date
-hctosys:   	 1 if the RTC provided the system time at boot via the
-		 CONFIG_RTC_HCTOSYS kernel option, 0 otherwise
+hctosys:   	 1 if the RTC provided the system time at boot, 0 otherwise
 max_user_freq:	 The maximum interrupt rate an unprivileged user may request
 		 from this RTC.
 name:		 The name of the RTC corresponding to this sysfs directory
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 2e565f8..bce8625 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -17,44 +17,13 @@ menuconfig RTC_CLASS
 
 if RTC_CLASS
 
-config RTC_HCTOSYS
-	bool "Set system time from RTC on startup and resume"
-	default y
-	help
-	  If you say yes here, the system time (wall clock) will be set using
-	  the value read from a specified RTC device. This is useful to avoid
-	  unnecessary fsck runs at boot time, and to network better.
-
 config RTC_SYSTOHC
 	bool "Set the RTC time based on NTP synchronization"
 	default y
 	help
 	  If you say yes here, the system time (wall clock) will be stored
-	  in the RTC specified by RTC_HCTOSYS_DEVICE approximately every 11
-	  minutes if userspace reports synchronized NTP status.
-
-config RTC_HCTOSYS_DEVICE
-	string "RTC used to set the system time"
-	depends on RTC_HCTOSYS = y || RTC_SYSTOHC = y
-	default "rtc0"
-	help
-	  The RTC device that will be used to (re)initialize the system
-	  clock, usually rtc0. Initialization is done when the system
-	  starts up, and when it resumes from a low power state. This
-	  device should record time in UTC, since the kernel won't do
-	  timezone correction.
-
-	  The driver for this RTC device must be loaded before late_initcall
-	  functions run, so it must usually be statically linked.
-
-	  This clock should be battery-backed, so that it reads the correct
-	  time when the system boots from a power-off state. Otherwise, your
-	  system will need an external clock source (like an NTP server).
-
-	  If the clock you specify here is not battery backed, it may still
-	  be useful to reinitialize system time when resuming from system
-	  sleep states. Do not specify an RTC here unless it stays powered
-	  during all this system's supported sleep states.
+	  in the RTC used to set the time at boot or resume approximately
+	  every 11 minutes if userspace reports synchronized NTP status.
 
 config RTC_DEBUG
 	bool "RTC debug support"
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 40a0991..9ef9ad1 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -5,7 +5,6 @@
 ccflags-$(CONFIG_RTC_DEBUG)	:= -DDEBUG
 
 obj-$(CONFIG_RTC_LIB)		+= rtc-lib.o
-obj-$(CONFIG_RTC_HCTOSYS)	+= hctosys.o
 obj-$(CONFIG_RTC_SYSTOHC)	+= systohc.o
 obj-$(CONFIG_RTC_CLASS)		+= rtc-core.o
 rtc-core-y			:= class.o interface.o
diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 18c47b0..c1bc700 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -27,6 +27,9 @@
 static DEFINE_IDA(rtc_ida);
 struct class *rtc_class;
 
+/* The ID of the RTC device used to set the system clock */
+int __read_mostly rtc_hctosys_dev_id = -1;
+
 static void rtc_device_release(struct device *dev)
 {
 	struct rtc_device *rtc = to_rtc_device(dev);
@@ -34,12 +37,7 @@ static void rtc_device_release(struct device *dev)
 	kfree(rtc);
 }
 
-#ifdef CONFIG_RTC_HCTOSYS_DEVICE
-/* Result of the last RTC to system clock attempt. */
-int rtc_hctosys_ret = -ENODEV;
-#endif
-
-#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE)
+#if defined(CONFIG_PM_SLEEP)
 /*
  * On suspend(), measure the delta between one RTC and the
  * system's wall clock; restore it on resume().
@@ -54,12 +52,18 @@ static int rtc_suspend(struct device *dev)
 	struct rtc_time		tm;
 	struct timespec		delta, delta_delta;
 
+
+	if (!systime_was_set)
+		return 0;
+
 	if (has_persistent_clock())
 		return 0;
 
-	if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
+	if (rtc->id != rtc_hctosys_dev_id)
 		return 0;
 
+	systime_was_set = false;
+
 	/* snapshot the current RTC and system time at suspend*/
 	rtc_read_time(rtc, &tm);
 	getnstimeofday(&old_system);
@@ -95,11 +99,13 @@ static int rtc_resume(struct device *dev)
 	struct timespec		new_system, new_rtc;
 	struct timespec		sleep_time;
 
+	if (systime_was_set)
+		return 0;
+
 	if (has_persistent_clock())
 		return 0;
 
-	rtc_hctosys_ret = -ENODEV;
-	if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
+	if (rtc->id != rtc_hctosys_dev_id)
 		return 0;
 
 	/* snapshot the current rtc and system time at resume */
@@ -132,7 +138,7 @@ static int rtc_resume(struct device *dev)
 
 	if (sleep_time.tv_sec >= 0)
 		timekeeping_inject_sleeptime(&sleep_time);
-	rtc_hctosys_ret = 0;
+	systime_was_set = true;
 	return 0;
 }
 
@@ -171,13 +177,15 @@ static void hctosys(struct rtc_device *rtc)
 		dev_err(rtc->dev.parent,
 			"rtc core: error setting system clock: %d\n", rc);
 		return;
-	} else if (systime_was_set)
+	} else if (systime_was_set) {
+		rtc_hctosys_dev_id = rtc->id;
 		dev_info(rtc->dev.parent,
 			"rtc core: setting system clock to "
 			"%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
 			now.tm_year + 1900, now.tm_mon + 1, now.tm_mday,
 			now.tm_hour, now.tm_min, now.tm_sec,
 			(unsigned int) ts.tv_sec);
+	}
 }
 
 /**
diff --git a/drivers/rtc/hctosys.c b/drivers/rtc/hctosys.c
deleted file mode 100644
index 4aa60d7..0000000
--- a/drivers/rtc/hctosys.c
+++ /dev/null
@@ -1,76 +0,0 @@
-/*
- * RTC subsystem, initialize system time on startup
- *
- * Copyright (C) 2005 Tower Technologies
- * Author: Alessandro Zummo <a.zummo@towertech.it>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
-*/
-
-#include <linux/rtc.h>
-
-/* IMPORTANT: the RTC only stores whole seconds. It is arbitrary
- * whether it stores the most close value or the value with partial
- * seconds truncated. However, it is important that we use it to store
- * the truncated value. This is because otherwise it is necessary,
- * in an rtc sync function, to read both xtime.tv_sec and
- * xtime.tv_nsec. On some processors (i.e. ARM), an atomic read
- * of >32bits is not possible. So storing the most close value would
- * slow down the sync API. So here we have the truncated value and
- * the best guess is to add 0.5s.
- */
-
-static int __init rtc_hctosys(void)
-{
-	int err = -ENODEV;
-	struct rtc_time tm;
-	struct timespec tv = {
-		.tv_nsec = NSEC_PER_SEC >> 1,
-	};
-	struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE);
-
-	if (rtc == NULL) {
-		pr_err("%s: unable to open rtc device (%s)\n",
-			__FILE__, CONFIG_RTC_HCTOSYS_DEVICE);
-		goto err_open;
-	}
-
-	err = rtc_read_time(rtc, &tm);
-	if (err) {
-		dev_err(rtc->dev.parent,
-			"hctosys: unable to read the hardware clock\n");
-		goto err_read;
-
-	}
-
-	err = rtc_valid_tm(&tm);
-	if (err) {
-		dev_err(rtc->dev.parent,
-			"hctosys: invalid date/time\n");
-		goto err_invalid;
-	}
-
-	rtc_tm_to_time(&tm, &tv.tv_sec);
-
-	err = do_settimeofday(&tv);
-
-	dev_info(rtc->dev.parent,
-		"setting system clock to "
-		"%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
-		tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
-		tm.tm_hour, tm.tm_min, tm.tm_sec,
-		(unsigned int) tv.tv_sec);
-
-err_invalid:
-err_read:
-	rtc_class_close(rtc);
-
-err_open:
-	rtc_hctosys_ret = err;
-
-	return err;
-}
-
-late_initcall(rtc_hctosys);
diff --git a/drivers/rtc/rtc-proc.c b/drivers/rtc/rtc-proc.c
index ffa69e1..fd59c1b 100644
--- a/drivers/rtc/rtc-proc.c
+++ b/drivers/rtc/rtc-proc.c
@@ -18,27 +18,6 @@
 
 #include "rtc-core.h"
 
-#define NAME_SIZE	10
-
-#if defined(CONFIG_RTC_HCTOSYS_DEVICE)
-static bool is_rtc_hctosys(struct rtc_device *rtc)
-{
-	int size;
-	char name[NAME_SIZE];
-
-	size = scnprintf(name, NAME_SIZE, "rtc%d", rtc->id);
-	if (size > NAME_SIZE)
-		return false;
-
-	return !strncmp(name, CONFIG_RTC_HCTOSYS_DEVICE, NAME_SIZE);
-}
-#else
-static bool is_rtc_hctosys(struct rtc_device *rtc)
-{
-	return (rtc->id == 0);
-}
-#endif
-
 static int rtc_proc_show(struct seq_file *seq, void *offset)
 {
 	int err;
@@ -137,12 +116,12 @@ static const struct file_operations rtc_proc_fops = {
 
 void rtc_proc_add_device(struct rtc_device *rtc)
 {
-	if (is_rtc_hctosys(rtc))
+	if (rtc->id == rtc_hctosys_dev_id)
 		proc_create_data("driver/rtc", 0, NULL, &rtc_proc_fops, rtc);
 }
 
 void rtc_proc_del_device(struct rtc_device *rtc)
 {
-	if (is_rtc_hctosys(rtc))
+	if (rtc->id == rtc_hctosys_dev_id)
 		remove_proc_entry("driver/rtc", NULL);
 }
diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
index babd43b..c15ee78 100644
--- a/drivers/rtc/rtc-sysfs.c
+++ b/drivers/rtc/rtc-sysfs.c
@@ -111,13 +111,9 @@ static DEVICE_ATTR_RW(max_user_freq);
 static ssize_t
 hctosys_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-#ifdef CONFIG_RTC_HCTOSYS_DEVICE
-	if (rtc_hctosys_ret == 0 &&
-			strcmp(dev_name(&to_rtc_device(dev)->dev),
-				CONFIG_RTC_HCTOSYS_DEVICE) == 0)
+	if (to_rtc_device(dev)->id == rtc_hctosys_dev_id)
 		return sprintf(buf, "1\n");
 	else
-#endif
 		return sprintf(buf, "0\n");
 }
 static DEVICE_ATTR_RO(hctosys);
diff --git a/drivers/rtc/systohc.c b/drivers/rtc/systohc.c
index bf3e242..1962f4c 100644
--- a/drivers/rtc/systohc.c
+++ b/drivers/rtc/systohc.c
@@ -25,13 +25,16 @@ int rtc_set_ntp_time(struct timespec now)
 	struct rtc_device *rtc;
 	struct rtc_time tm;
 	int err = -ENODEV;
+	char rtc_hctosys_name[10];
 
 	if (now.tv_nsec < (NSEC_PER_SEC >> 1))
 		rtc_time_to_tm(now.tv_sec, &tm);
 	else
 		rtc_time_to_tm(now.tv_sec + 1, &tm);
 
-	rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE);
+	scnprintf(rtc_hctosys_name, sizeof(rtc_hctosys_name), "rtc%d",
+		rtc_hctosys_dev_id);
+	rtc = rtc_class_open(rtc_hctosys_name);
 	if (rtc) {
 		/* rtc_hctosys exclusively uses UTC, so we call set_time here,
 		 * not set_mmss. */
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index c2c2897..50caf0d 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -191,10 +191,7 @@ static inline bool is_leap_year(unsigned int year)
 	return (!(year % 4) && (year % 100)) || !(year % 400);
 }
 
-#ifdef CONFIG_RTC_HCTOSYS_DEVICE
-extern int rtc_hctosys_ret;
-#else
-#define rtc_hctosys_ret -ENODEV
-#endif
+/* The ID of the RTC device used to set the system clock */
+extern int rtc_hctosys_dev_id;
 
 #endif /* _LINUX_RTC_H_ */
-- 
1.8.3.2

Re: rtc/hctosys.c Problem during kernel boot

[Alexander Holler]

Am 12.06.2014 01:53, schrieb John Stultz:

> You can read some of the previous discussion here:
> https://lkml.org/lkml/2013/6/17/533
>
> I'd be very interested in patches to resolve this!

And the silence as response to my repost of my already working patches 
just proved that isn't true.

So (John Whitmore), I suggest to not post patches, Linux kernel 
maintainers don't really have an interest in getting things done or to 
fix bugs, they just need fresh meat they can review in order to have job 
security and prove their status.

I really wonder what's their expectation. Do they really think other 
people have to incorporate their (often silly) requests, making the 
maintainers themself not responsible for their requested changes? Do 
they think other people have fun and time to write and post patches 
again and again just to make some arbitrary maintainer happy?

If there really would be an interest, a reasonable approach would be to 
just take my patches and put a patch on top with whatever changes 
someone thinks are needed. As I don't think there are changes needed, I 
will not add such changes using my name as author.

And don't try to tell me I'm uncooperative. I've spend time to write 
these patches and even have written documentation I don't need myself. 
The uncooperative people which are blocking almost everthing and which 
do ignore bugs have become these people which are calling themself Linux 
kernel maintainers which do expect other people have to play remote 
keyboard and have to take responsibility for changes maintainers don't 
want to be responsible for themself.

So the above quoted sentence is just another "marketing" verbiage, at 
least in my point of view.

Alexander Holler

Re: rtc/hctosys.c Problem during kernel boot

[Alessandro Zummo]

I'm testing them and they're working fine so far. Will handle them the next week. 

--
 Best regards,
  Alessandro Zummo
     Tower Technologies

Sent from my iPhone, please excuse my brevity. 

> On 21/giu/2014, at 15:08, Alexander Holler <holler@ahsoftware.de> wrote:
> 
> Am 12.06.2014 01:53, schrieb John Stultz:
> 
>> You can read some of the previous discussion here:
>> https://lkml.org/lkml/2013/6/17/533
>> 
>> I'd be very interested in patches to resolve this!
> 
> And the silence as response to my repost of my already working patches just proved that isn't true.
> 
> So (John Whitmore), I suggest to not post patches, Linux kernel maintainers don't really have an interest in getting things done or to fix bugs, they just need fresh meat they can review in order to have job security and prove their status.
> 
> I really wonder what's their expectation. Do they really think other people have to incorporate their (often silly) requests, making the maintainers themself not responsible for their requested changes? Do they think other people have fun and time to write and post patches again and again just to make some arbitrary maintainer happy?
> 
> If there really would be an interest, a reasonable approach would be to just take my patches and put a patch on top with whatever changes someone thinks are needed. As I don't think there are changes needed, I will not add such changes using my name as author.
> 
> And don't try to tell me I'm uncooperative. I've spend time to write these patches and even have written documentation I don't need myself. The uncooperative people which are blocking almost everthing and which do ignore bugs have become these people which are calling themself Linux kernel maintainers which do expect other people have to play remote keyboard and have to take responsibility for changes maintainers don't want to be responsible for themself.
> 
> So the above quoted sentence is just another "marketing" verbiage, at least in my point of view.
> 
> Alexander Holler
> 

Re: rtc/hctosys.c Problem during kernel boot

[John Stultz]

On Sat, Jun 21, 2014 at 6:08 AM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 12.06.2014 01:53, schrieb John Stultz:
>
>> You can read some of the previous discussion here:
>> https://lkml.org/lkml/2013/6/17/533
>>
>> I'd be very interested in patches to resolve this!
>
>
> And the silence as response to my repost of my already working patches just
> proved that isn't true.

You put in your patches the following:
"Besides discussing possible *real* bugs, I don't care what any person
 (including maintainers) will request. I'm fine with these patches (I'm
 using them since a year) and I don't play remote keyboard or
 patch ping-pong. If someone want changes I suggest he gets responsible
 for them himself and just puts a patch on top of my patches. And in any
 case, feel free to completely ignore these patches."

I've pointed out problems with your patchset earlier, and you refuse
to address them. That's totally your prerogative, and that's fine, but
I don't know how I should respond here.

I agree that there is an issue here that your patches try to address,
which needs to be fixed, but I'm hoping John Whitmore might be able to
read the discussion and either rework your patches or write his own to
address the issue without the problems in your patch I pointed out.

I've removed the rest of your anti-maintainer rant here, but I will
say that I do very much understand that the upstream kernel community
process can be frustrating at times. I have myself had to start over
many many times on patches when maintainers request, and sometimes my
patches don't ever make it past the bar for acceptance. So I very much
do see this from both sides, and despite my frustration, I appreciate
that folks are looking over my patches carefully for design and
maintenance issues, because without the high standards, the kernel
code would be in much worse shape.

Similarly I appreciate your continued participation here, even if its
just to resend your patches and provide more context to others wanting
to solve the issue properly. But it might be better not get into
personal tangents, and instead focus on the technical merits and
issues with the potential approaches.

thanks
-john

Re: rtc/hctosys.c Problem during kernel boot

[Alexander Holler]

Am 23.06.2014 23:36, schrieb John Stultz:

> do see this from both sides, and despite my frustration, I appreciate
> that folks are looking over my patches carefully for design and
> maintenance issues, because without the high standards, the kernel
> code would be in much worse shape.

I wouldn't say that "looks good but doesn't work" is a high standard, 
but maybe I'm old school.

Regards,

Alexander Holler

Re: rtc/hctosys.c Problem during kernel boot

[Alexander Holler]

Am 23.06.2014 23:36, schrieb John Stultz:
> On Sat, Jun 21, 2014 at 6:08 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>> Am 12.06.2014 01:53, schrieb John Stultz:
>>
>>> You can read some of the previous discussion here:
>>> https://lkml.org/lkml/2013/6/17/533
>>>
>>> I'd be very interested in patches to resolve this!
>>
>>
>> And the silence as response to my repost of my already working patches just
>> proved that isn't true.
>
> You put in your patches the following:
> "Besides discussing possible *real* bugs, I don't care what any person
>   (including maintainers) will request. I'm fine with these patches (I'm
>   using them since a year) and I don't play remote keyboard or
>   patch ping-pong. If someone want changes I suggest he gets responsible
>   for them himself and just puts a patch on top of my patches. And in any
>   case, feel free to completely ignore these patches."
>
> I've pointed out problems with your patchset earlier, and you refuse
> to address them. That's totally your prerogative, and that's fine, but
> I don't know how I should respond here.

I've written that because I will not add a totally unnecessary lock in a 
code path which does set the system time. If multiple RTCs do race to 
set the system time, the system was configured wrong and is already 
untrustworthy and a lock inside the kernel won't help. Especially 
because it will not remove the race, and just solve it somehow. And 
that's happen without a lock (which might block or even dead lock if 
done wrong) too.

So if you want a lock there, please add such a patch yourself and be 
responsible for it. I will not do such.

> I agree that there is an issue here that your patches try to address,
> which needs to be fixed, but I'm hoping John Whitmore might be able to
> read the discussion and either rework your patches or write his own to
> address the issue without the problems in your patch I pointed out.
>
> I've removed the rest of your anti-maintainer rant here, but I will
> say that I do very much understand that the upstream kernel community
> process can be frustrating at times. I have myself had to start over
> many many times on patches when maintainers request, and sometimes my
> patches don't ever make it past the bar for acceptance. So I very much
> do see this from both sides, and despite my frustration, I appreciate
> that folks are looking over my patches carefully for design and
> maintenance issues, because without the high standards, the kernel
> code would be in much worse shape.

Unfortunately I really think you believe what you are writing.

But my experience is that even totally silly and very small bugfixes 
like oneliners are discussed to death and do need years to end up in a 
mainline kernel, if they end up there at all.

And it doesn't help if maintainers do request silly things. My 
experience here is that they always will request a change, even for 
perfect patches, just to request something. If a patch contains a 
variable named red, they would request to change it to green and if it 
would have a name green, they would request to change it to red.

So I'm sorry, but in my humble opinion that isn't how a high standard 
does look like. Many request from maintainer are just capriciousness or 
despotism and often maintainers are totally wrong. And discussing with 
them just leads to ignored patches/bugfixes.

Regards,

Alexander Holler

Re: [PATCH 3/4] rtc: rtc-hid-sensor-time: add option hctosys to set time at boot

[Alexander Holler]

Am 29.05.2013 06:42, schrieb Alexander Holler:
> Am 28.05.2013 21:37, schrieb John Stultz:
>> On 05/21/2013 04:15 PM, Alexander Holler wrote:

>>> Implementation could be as easy as a bool "time_set_at_least_once" in
>>> the timer subsystem itself (e.g. in do_settimeofday() and whatever
>>> similiar is available).
>>
>> If it were to be done this way, it would be good to have the RTC layer
>> check when RTC devices are registered and call the internal hctosys
>> functionality then (rather then just at late_init). Do you want to try
>> to rework the patch in this way?
> 
> That sounds like what Andrew Morton wanted to trick me to do. ;)

Just in case that was misunderstood. Of course, I would rework the patch
if we reach a consensus about how I can do that. I would also volunteer
and would remove hctosys and replace it by a better approach (imho)
which sets the system time when a rtc registers. That doesn't look like
much work.

I've just did a small test and I think the mentioned flag could be
implemented by the patch below (pasted => wrong format).

I don't think stuff like atomic_*, a spinlock, mutex or similiar is
necessary to make sure that the system time will only be set by one rtc,
because I think it is extremly unlikely that more than one rtc will
register at the same time to make such necessary. That means I think
it's ok to just use "if (!systime_was_set) do_settimeofday()" in the rtc
subsystem afterwards.

I would also implement a kernel parameter like systime_from which could
not be set (== use the first clock which offers a valid time) or could
be set to "persistent" or the name of a RTC module in which case either
the persistent clock or the named RTC would be used to set the time, if
and only if the system time wasn't be set by something else (most likely
userspace).

Regards,

Alexander Holler


diff --git a/include/linux/time.h b/include/linux/time.h
index afcdc4b..9c488f3 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -129,6 +129,12 @@ extern int update_persistent_clock(struct timespec
now);
 void timekeeping_init(void);
 extern int timekeeping_suspended;

+/*
+ * Will be true if the system time was set at least once by
+ * a persistent clock, RTC or userspace.
+ */
+extern bool systime_was_set;
+
 unsigned long get_seconds(void);
 struct timespec current_kernel_time(void);
 struct timespec __current_kernel_time(void); /* does not take xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 9a0bc98..442e03c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -32,6 +32,9 @@ int __read_mostly timekeeping_suspended;
 /* Flag for if there is a persistent clock on this platform */
 bool __read_mostly persistent_clock_exist = false;

+/* Flag for if the system time was set at least once */
+bool __read_mostly systime_was_set = false;
+
 static inline void tk_normalize_xtime(struct timekeeper *tk)
 {
        while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
@@ -448,6 +451,10 @@ int do_settimeofday(const struct timespec *tv)
        if (!timespec_valid_strict(tv))
                return -EINVAL;

+       systime_was_set = true;
+
        write_seqlock_irqsave(&tk->lock, flags);

        timekeeping_forward_now(tk);
@@ -682,8 +689,11 @@ void __init timekeeping_init(void)
                        "         Check your CMOS/BIOS settings.\n");
                now.tv_sec = 0;
                now.tv_nsec = 0;
-       } else if (now.tv_sec || now.tv_nsec)
+       } else if (now.tv_sec || now.tv_nsec) {
                persistent_clock_exist = true;
+               systime_was_set = true;
+       }

        read_boot_clock(&boot);
        if (!timespec_valid_strict(&boot)) {

[PATCH 0/3] RFC: timekeeping: rtc: change hctosys mechanism

[Alexander Holler]

Hello,

because it wasn't that much work, I've already rewritten the hctosys
mechanism without discussing the changes further before.

So the following patches are RFC (and incomplete but imho perfectly,
usable) see below under missing) and I hope they will find friends and at
least one reviewer which doesn't had a bad day.


To describe the changes, I first quote what I've added to
Documentation/kernel-parameters.txt:

	hctosys=	[KNL] Specifies the driver (RTC) name which sets the
			time at	boot, if and only if userspace hasn't set the
			time before the driver will be loaded. If hctosys will
			not be specified, the first available hardware clock
			with a valid time will be used.
			Use a non-existent name (e.g. hctosys=none) if you want
			to avoid that a hardware clock will set the time.
			Currently there exist a special name "persistent" for
			the persistent clock found on some systems (e.g. the
			CMOS clock on x86 platforms which might be handled
			by the driver named rtc_cmos too).

What the patches do change:

- Default functionality: hopefully nothing changes.
  The only user visible change is that /proc/dev/rtc doesn't appear on
  systems on which a "persistent" clock still is used. But I think this
  is logically more correct, than what currently happens:
  /proc/dev/rtc outputs data from an RTC which doesn't have to be what
  is really used (persistent clock).
  Therefor /proc/dev/rtc now only appears, if a RTC-driver is used for
  hctosys and not if a "persistent" clock is used. Unfortunately the later
  is still true on most systems (at least for x86*).

- CONFIG_RTC_HCTOSYS is gone.
  On systems with a "persistent" clock, CONFIG_RTC_HCTOSYS was meaningless,
  because those systems always used the "persistent" clock and so already
  ignored CONFIG_RTC_HCTOSYS. Now this functionality is always on, if a
  hardware clock ("persistent" clock or RTC-driver) is used for hctosys.

- CONFIG_RTC_HCTOSYS_DEVICE is gone.
  This config option never really specified which RTC is used, it only
  specified which RTC is used in kind of the order of their appearance
  to the system. With the new kernel-parameter hctosys= it is now at least
  possible to specify the type of the used RTC. Of course, if more RTCs of
  the same type (same driver) are available, the first of them will be used.

- The hctosys functionality works even when userspace alreads has started.
  Without these changes, hctosys was invoked either at initialization of
  the timekeeping system (for "persistent" clock) or at late_init, both
  happens before userspace is started.
  To avoid problems with that change there is now an additional rule:
  Userspace comes first. If the userspace sets the system clock before
  any hardware clock was available, the hardware clock will not change
  the system clock.

The last point above is also the reason why I did those changes at all
(to support USB RTCs).


What's missing:

I don't know much about those "persistent" clocks and I haven't had a
deep look at them. That's especially true for the suspend/resume
mechanism used by them. The mechanism I want to use is the following:
The RTC subsystem now maintains the ID of the RTC device which was used
for hctosys (in rtc_hctosys_dev_id) and therefor specifies the device
which should be used to adjust the time after resume. Additionaly the
(new) flag systime_was_set will be set to false at suspend and on resume
this flag will be set to true if either the clock will be adjusted by
the device used for hctosys or by userspace (through do_settimeofday()).

That all should already work as expected for RTCs, what's missing for
"persistent" clocks is that the flag systime_was_set is set to false on
suspend and set to true on resume. Currently it just stays at true
(which is set through hctosys if a "persistent" clock is found.
But because "persistent" clocks don't go away (as it is possible with
RTCs by removing the driver or the RTC itself), nor do "persistent"
clocks might have two instances, this shouldn't be a problem at all.


And last but not least, I have to add a disclaimer:

This changes aren't company driven. That means until now I was the only
person who has seen, reviewed and tested them and I spend only a limited
amount of (my spare) time for that. I did (quick) tests with x86 and ARM
systems, with and without RTCs, with and without "persistent" clocks.
I also tested suspend/resume on x86. But in any case, don't expect I
didn't make failures. I did those changes in my "private" mode and not in
my "professional" mode, which makes a difference (at least in the amount
of time I spend for review and testing). But don't be scared, even my
"private" mode might spend more time for QA than many companies (are able
and willing to) do. ;)


Regards,

Alexander Holler

PS: Parts of the above documentation might be added to one of the following
patches to document them better in git too. I've written the above
documentation after I've done the patches and will wait for feedback before
I change them again. I've already done more than I initially wanted to do.

PPS: The new hctosys mechanism provides an additional feature some people
might like: HA for RTCs. If a system has two hardware clocks and one of
them will fail such that it provides an invalid time (in regard to
rtc_valid_tm()), the second one will be used.

[PATCH 2/3] RFC: timekeeping: rtc: Introduce new kernel parameter hctosys

[Alexander Holler]

hctosys= specifies the driver (RTC) name which sets the system clock at
boot, if and only if userspace hasn't set the time before the driver will
be loaded.

If hctosys will not be specified, the first available hardware clock
with a valid time will be used (again, if and only if ...).

If you don't want that the system clock will be set by any hardware clock,
just specify a non-existent RTC driver name, e.g. with hctosys=none.

Currently there exist a special name "persistent" for the persistent clock
found on some systems (e.g. the CMOS clock on x86 platforms which might be
handled by the driver named rtc_cmos too).

This will replace the existent driver/mechanism hctosys and the kernel
config options CONFIG_RTC_HCTOSYS and CONFIG_RTC_HCTOSYS_DEVICE (done
with one of the following patches)

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 Documentation/kernel-parameters.txt | 12 ++++++++++++
 drivers/rtc/class.c                 | 32 ++++++++++++++++++++++++++++++++
 include/linux/time.h                |  6 ++++++
 kernel/time/timekeeping.c           | 31 ++++++++++++++++++++++++++++++-
 4 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 6e3b18a..a8b7a9c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -983,6 +983,18 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 	hcl=		[IA-64] SGI's Hardware Graph compatibility layer
 
+	hctosys=	[KNL] Specifies the driver (RTC) name which sets the
+			time at	boot, if and only if userspace hasn't set the
+			time before the driver will be loaded. If hctosys will
+			not be specified, the first available hardware clock
+			with a valid time will be used.
+			Use a non-existent name (e.g. hctosys=none) if you want
+			to avoid that a hardware clock will set the time.
+			Currently there exist a special name "persistent" for
+			the persistent clock found on some systems (e.g. the
+			CMOS clock on x86 platforms which might be handled
+			by the driver named rtc_cmos too).
+
 	hd=		[EIDE] (E)IDE hard drive subsystem geometry
 			Format: <cyl>,<head>,<sect>
 
diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 6638540..841d4e9 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -141,6 +141,33 @@ static int rtc_resume(struct device *dev)
 #endif
 
 
+static void hctosys(struct rtc_device *rtc)
+{
+	struct rtc_time now;
+	struct timespec tv = {
+		.tv_nsec = NSEC_PER_SEC >> 1,
+	};
+	int err;
+
+	err = rtc_read_time(rtc, &now);
+	if (err)
+		return;
+	err = rtc_valid_tm(&now);
+	if (err)
+		return;
+	rtc_tm_to_time(&now, &tv.tv_sec);
+	if (systime_was_set)
+		return;
+	err = do_settimeofday(&tv);
+	if (!err && systime_was_set)
+		dev_info(rtc->dev.parent,
+			"setting system clock to "
+			"%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
+			now.tm_year + 1900, now.tm_mon + 1, now.tm_mday,
+			now.tm_hour, now.tm_min, now.tm_sec,
+			(unsigned int) tv.tv_sec);
+}
+
 /**
  * rtc_device_register - register w/ RTC class
  * @dev: the device to register
@@ -157,6 +184,7 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
 	struct rtc_device *rtc;
 	struct rtc_wkalrm alrm;
 	int id, err;
+	const char *hctosys_name = get_hctosys_name();
 
 	id = ida_simple_get(&rtc_ida, 0, 0, GFP_KERNEL);
 	if (id < 0) {
@@ -196,6 +224,10 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
 	rtc->pie_timer.function = rtc_pie_update_irq;
 	rtc->pie_enabled = 0;
 
+	if (!systime_was_set &&
+		(!hctosys_name[0] || !strcasecmp(name, hctosys_name)))
+		hctosys(rtc);
+
 	/* Check to see if there is an ALARM already set in hw */
 	err = __rtc_read_alarm(rtc, &alrm);
 
diff --git a/include/linux/time.h b/include/linux/time.h
index 888280f..e5f644c 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -135,6 +135,12 @@ extern int timekeeping_suspended;
  */
 extern bool systime_was_set;
 
+/*
+ * Returns a pointer to the string specified with
+ * hctosys= at the kernel command line.
+ */
+const char *get_hctosys_name(void);
+
 unsigned long get_seconds(void);
 struct timespec current_kernel_time(void);
 struct timespec __current_kernel_time(void); /* does not take xtime_lock */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 07f151f..1de734c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -22,6 +22,7 @@
 #include <linux/tick.h>
 #include <linux/stop_machine.h>
 #include <linux/pvclock_gtod.h>
+#include <linux/rtc.h>
 
 #include "tick-internal.h"
 #include "ntp_internal.h"
@@ -40,6 +41,22 @@ bool __read_mostly persistent_clock_exist = false;
 /* Flag for if the system time was set at least once */
 bool __read_mostly systime_was_set;
 
+static char hctosys_name[RTC_DEVICE_NAME_SIZE];
+
+static int __init hctosys_setup(char *line)
+{
+	strlcpy(hctosys_name, line, sizeof(hctosys_name));
+	return 1;
+}
+
+__setup("hctosys=", hctosys_setup);
+
+const char *get_hctosys_name(void)
+{
+	return hctosys_name;
+}
+EXPORT_SYMBOL_GPL(get_hctosys_name);
+
 static inline void tk_normalize_xtime(struct timekeeper *tk)
 {
 	while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
@@ -780,7 +797,8 @@ void __init timekeeping_init(void)
 	unsigned long flags;
 	struct timespec now, boot, tmp;
 
-	read_persistent_clock(&now);
+	if (!hctosys_name[0] || !strcasecmp(hctosys_name, "persistent"))
+		read_persistent_clock(&now);
 
 	if (!timespec_valid_strict(&now)) {
 		pr_warn("WARNING: Persistent clock returned invalid value!\n"
@@ -826,6 +844,17 @@ void __init timekeeping_init(void)
 
 	write_seqcount_end(&timekeeper_seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+
+	if (systime_was_set) {
+		/* print a msg like if the time was set by a RTC */
+		struct tm now_tm;
+		time_to_tm(now.tv_sec, 0, &now_tm);
+		pr_info("persistent clock: setting system clock to "
+			"%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
+			(int)now_tm.tm_year + 1900, now_tm.tm_mon + 1,
+			now_tm.tm_mday,	now_tm.tm_hour, now_tm.tm_min,
+			now_tm.tm_sec, (unsigned int) now.tv_sec);
+	}
 }
 
 /* time in seconds when suspend began */
-- 
1.8.1.5