diff mbox

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

Message ID 51ADEE76.7090504@ahsoftware.de
State Superseded
Headers show

Commit Message

Alexander Holler June 4, 2013, 1:41 p.m. UTC
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


                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)) {

Comments

Alexander Holler June 5, 2013, 5:15 p.m. UTC | #1
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.
Alexander Holler June 6, 2013, 10:51 a.m. UTC | #2
As already assumed, there are was at least one silly failure I made,
a wrong warning if the persistent was disabled by purpose which I seem
to have missed while looking at the output during my tests:

WARNING: Persistent clock returned invalid value!

I've fixed that in patch 2/3 and also have added some error messages
to indicate why reading the time from a misfunctional RTC fails to the
same patch. Because patch 3/3 didn't apply afterwards, I've solved the
conflict and resend it too. And to keep the small series together, I
resend patch 1/3 too.

So no changes in functionality, just some "cosmetic" changes in patch 2/3.

Regards,

Alexander Holler
diff mbox

Patch

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");