Hello,
as I'm not sure if the maintainer of the RTC subsystem is active again,
I've added the people which where involved with rtc-hid-sensor-time
before to cc too. That might be a good idea even he is active again,
as the driver depends on hid-sensor-hub and as such works a bit
outside the stuff other RTC drivers do use.
This small series of patches do extend the driver a bit. They make
the driver more standard (draft) compliant and I've added a replacement
for the usual hctosys, because that isn't usable with this driver.
I've already send the first patch two weeks before (unchanged) therefor
I've marked it as RESEND (see https://lkml.org/lkml/2013/4/5/177 ).
Actually I wanted to have added another patch to adjust the system time
too (when the clock sends HID reports which are including milliseconds
regulary by itself), but I have to read about the in-kernel timekeeping
(kernel/time/timekeeping.c, kernel/time/ntp.c) for that and I don't know
if I will find the time without missing the next merge window.
Personally I would like if they went into 3.9 too, but beeing realistic,
I'm happy if they end up in 3.11. ;)
If someone is curious about my other side of that driver, I've setup a
website with explanations and source which should make it easy build a
similiar hot-pluggable RTC: http://ahsoftware.de/dcf77-hid-usb-rtc/
Regards,
Alexander
The draft for HID-sensors (HUTRR39) currently doesn't define the range
for the attribute year. Asking one of the authors revealed that full years
(e.g. 2013 instead of just 13) were meant.
So we now allow both, 8 bit and 16 bit values for the attribute year and
assuming full years when the value is 16 bits wide.
We will still support 8 bit values until the specification gets final
(and maybe defines a way to set the time too).
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 33 ++++++++++++++++++++++++++-------
1 file changed, 26 insertions(+), 7 deletions(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index 31c5728..f3ef38a 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -85,10 +85,13 @@ static int hid_time_capture_sample(struct hid_sensor_hub_device *hsdev,
switch (usage_id) {
case HID_USAGE_SENSOR_TIME_YEAR:
- time_buf->tm_year = *(u8 *)raw_data;
- if (time_buf->tm_year < 70)
- /* assume we are in 1970...2069 */
- time_buf->tm_year += 100;
+ if (raw_len == 1) {
+ time_buf->tm_year = *(u8 *)raw_data;
+ if (time_buf->tm_year < 70)
+ /* assume we are in 1970...2069 */
+ time_buf->tm_year += 100;
+ } else
+ time_buf->tm_year = *(u16 *)raw_data-1900;
break;
case HID_USAGE_SENSOR_TIME_MONTH:
/* sensor sending the month as 1-12, we need 0-11 */
@@ -151,11 +154,27 @@ static int hid_time_parse_report(struct platform_device *pdev,
return -EINVAL;
}
if (time_state->info[i].size != 1) {
- dev_err(&pdev->dev,
- "attribute '%s' not 8 bits wide!\n",
+ /*
+ * The draft for HID-sensors (HUTRR39) currently
+ * doesn't define the range for the year attribute.
+ * Therefor we support both 8 bit (0-99) and 16 bit
+ * (full) as size for the year.
+ */
+ if (time_state->info[i].attrib_id !=
+ HID_USAGE_SENSOR_TIME_YEAR) {
+ dev_err(&pdev->dev,
+ "attribute '%s' not 8 bits wide!\n",
hid_time_attrib_name(
time_state->info[i].attrib_id));
- return -EINVAL;
+ return -EINVAL;
+ }
+ if (time_state->info[i].size != 2) {
+ dev_err(&pdev->dev,
+ "attribute '%s' not 8 or 16 bits wide!\n",
+ hid_time_attrib_name(
+ time_state->info[i].attrib_id));
+ return -EINVAL;
+ }
}
if (time_state->info[i].units !=
HID_USAGE_SENSOR_UNITS_NOT_SPECIFIED &&
--
1.8.1.4
There is no real reason to not support 16 or 32 bit values too.
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 59 +++++++++++++++++++++------------------
1 file changed, 32 insertions(+), 27 deletions(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index f3ef38a..10fc0f0 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -76,6 +76,20 @@ static int hid_time_proc_event(struct hid_sensor_hub_device *hsdev,
return 0;
}
+static u32 hid_time_value(size_t raw_len, char *raw_data)
+{
+ switch (raw_len) {
+ case 1:
+ return *(u8 *)raw_data;
+ case 2:
+ return *(u16 *)raw_data;
+ case 4:
+ return *(u32 *)raw_data;
+ default:
+ return (u32)(~0U); /* 0xff... or -1 to denote an error */
+ }
+}
+
static int hid_time_capture_sample(struct hid_sensor_hub_device *hsdev,
unsigned usage_id, size_t raw_len,
char *raw_data, void *priv)
@@ -85,29 +99,35 @@ static int hid_time_capture_sample(struct hid_sensor_hub_device *hsdev,
switch (usage_id) {
case HID_USAGE_SENSOR_TIME_YEAR:
+ /*
+ * The draft for HID-sensors (HUTRR39) currently doesn't define
+ * the range for the year attribute. Therefor we support
+ * 8 bit (0-99) and 16 or 32 bits (full) as size for the year.
+ */
if (raw_len == 1) {
time_buf->tm_year = *(u8 *)raw_data;
if (time_buf->tm_year < 70)
/* assume we are in 1970...2069 */
time_buf->tm_year += 100;
} else
- time_buf->tm_year = *(u16 *)raw_data-1900;
+ time_buf->tm_year =
+ (int)hid_time_value(raw_len, raw_data)-1900;
break;
case HID_USAGE_SENSOR_TIME_MONTH:
- /* sensor sending the month as 1-12, we need 0-11 */
- time_buf->tm_mon = *(u8 *)raw_data-1;
+ /* sensors are sending the month as 1-12, we need 0-11 */
+ time_buf->tm_mon = (int)hid_time_value(raw_len, raw_data)-1;
break;
case HID_USAGE_SENSOR_TIME_DAY:
- time_buf->tm_mday = *(u8 *)raw_data;
+ time_buf->tm_mday = (int)hid_time_value(raw_len, raw_data);
break;
case HID_USAGE_SENSOR_TIME_HOUR:
- time_buf->tm_hour = *(u8 *)raw_data;
+ time_buf->tm_hour = (int)hid_time_value(raw_len, raw_data);
break;
case HID_USAGE_SENSOR_TIME_MINUTE:
- time_buf->tm_min = *(u8 *)raw_data;
+ time_buf->tm_min = (int)hid_time_value(raw_len, raw_data);
break;
case HID_USAGE_SENSOR_TIME_SECOND:
- time_buf->tm_sec = *(u8 *)raw_data;
+ time_buf->tm_sec = (int)hid_time_value(raw_len, raw_data);
break;
default:
return -EINVAL;
@@ -153,28 +173,13 @@ static int hid_time_parse_report(struct platform_device *pdev,
"not all needed attributes inside the same report!\n");
return -EINVAL;
}
- if (time_state->info[i].size != 1) {
- /*
- * The draft for HID-sensors (HUTRR39) currently
- * doesn't define the range for the year attribute.
- * Therefor we support both 8 bit (0-99) and 16 bit
- * (full) as size for the year.
- */
- if (time_state->info[i].attrib_id !=
- HID_USAGE_SENSOR_TIME_YEAR) {
- dev_err(&pdev->dev,
- "attribute '%s' not 8 bits wide!\n",
- hid_time_attrib_name(
- time_state->info[i].attrib_id));
- return -EINVAL;
- }
- if (time_state->info[i].size != 2) {
- dev_err(&pdev->dev,
- "attribute '%s' not 8 or 16 bits wide!\n",
+ if (time_state->info[i].size == 3 ||
+ time_state->info[i].size > 4) {
+ dev_err(&pdev->dev,
+ "attribute '%s' not 8, 16 or 32 bits wide!\n",
hid_time_attrib_name(
time_state->info[i].attrib_id));
- return -EINVAL;
- }
+ return -EINVAL;
}
if (time_state->info[i].units !=
HID_USAGE_SENSOR_UNITS_NOT_SPECIFIED &&
--
1.8.1.4
drivers/rtc/hctosys (CONFIG_RTC_HCTOSYS) doesn't work for
rtc-hid-sensor-time because it will be called in late_init, and thus before
rtc-hid-sensor-time gets loaded. To set the time through rtc-hid-sensor-time
at startup, the module now checks by default if the system time is before
1970-01-02 and sets the system time (once) if this is the case.
To disable this behaviour, set the module option hctosys to zero, e.g. by
using rtc-hid-sensor-time.hctosys=0 at the kernel command line if the
driver is statically linked into the kernel.
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 72 +++++++++++++++++++++++++++++++++++++++
1 file changed, 72 insertions(+)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index 10fc0f0..3184729 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -27,6 +27,12 @@
/* Usage ID from spec for Time: 0x2000A0 */
#define DRIVER_NAME "HID-SENSOR-2000a0" /* must be lowercase */
+static bool hid_time_hctosys_enabled = 1;
+module_param_named(hctosys, hid_time_hctosys_enabled, bool, 0644);
+MODULE_PARM_DESC(hctosys,
+ "set the system time (once) if it is before 1970-01-02");
+static bool hid_time_time_set_once;
+
enum hid_time_channel {
CHANNEL_SCAN_INDEX_YEAR,
CHANNEL_SCAN_INDEX_MONTH,
@@ -62,6 +68,38 @@ static const char * const hid_time_channel_names[TIME_RTC_CHANNEL_MAX] = {
"year", "month", "day", "hour", "minute", "second",
};
+static void hid_time_hctosys(struct hid_time_state *time_state)
+{
+ struct timeval tv;
+ struct timespec ts;
+ int rc = rtc_valid_tm(&time_state->last_time);
+ if (rc) {
+ dev_err(time_state->rtc->dev.parent,
+ "hctosys: invalid date/time\n");
+ return;
+ }
+ do_gettimeofday(&tv);
+ if (tv.tv_sec >= 86400) /* 1970-01-02 00:00:00 UTC */
+ /*
+ * Security: don't let a hot-pluggable device change
+ * a valid system time.
+ */
+ return;
+ ts.tv_nsec = NSEC_PER_SEC >> 1;
+ rtc_tm_to_time(&time_state->last_time, &ts.tv_sec);
+ rc = do_settimeofday(&ts);
+ dev_info(time_state->rtc->dev.parent,
+ "hctosys: setting system clock to "
+ "%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
+ time_state->last_time.tm_year + 1900,
+ time_state->last_time.tm_mon + 1,
+ time_state->last_time.tm_mday,
+ time_state->last_time.tm_hour,
+ time_state->last_time.tm_min,
+ time_state->last_time.tm_sec,
+ (unsigned int) ts.tv_sec);
+}
+
/* Callback handler to send event after all samples are received and captured */
static int hid_time_proc_event(struct hid_sensor_hub_device *hsdev,
unsigned usage_id, void *priv)
@@ -73,6 +111,10 @@ static int hid_time_proc_event(struct hid_sensor_hub_device *hsdev,
time_state->last_time = time_state->time_buf;
spin_unlock_irqrestore(&time_state->lock_last_time, flags);
complete(&time_state->comp_last_time);
+ if (unlikely(!hid_time_time_set_once && hid_time_hctosys_enabled)) {
+ hid_time_time_set_once = 1;
+ hid_time_hctosys(time_state);
+ }
return 0;
}
@@ -237,6 +279,22 @@ static const struct rtc_class_ops hid_time_rtc_ops = {
.read_time = hid_rtc_read_time,
};
+struct hid_time_work_time_state {
+ struct work_struct work;
+ struct hid_time_state *time_state;
+};
+
+static void hid_time_request_report_work(struct work_struct *work)
+{
+ struct hid_time_state *time_state =
+ ((struct hid_time_work_time_state *)work)->time_state;
+ /* get a report with all values through requesting one value */
+ sensor_hub_input_attr_get_raw_value(
+ time_state->common_attributes.hsdev, HID_USAGE_SENSOR_TIME,
+ hid_time_addresses[0], time_state->info[0].report_id);
+ kfree(work);
+}
+
static int hid_time_probe(struct platform_device *pdev)
{
int ret = 0;
@@ -287,6 +345,20 @@ static int hid_time_probe(struct platform_device *pdev)
return PTR_ERR(time_state->rtc);
}
+ if (!hid_time_time_set_once && hid_time_hctosys_enabled) {
+ /*
+ * Request a HID report to set the time.
+ * Calling sensor_hub_..._get_raw_value() here directly
+ * doesn't work, therefor we have to use a work.
+ */
+ struct hid_time_work_time_state *hdwork =
+ kmalloc(sizeof(struct hid_time_work_time_state),
+ GFP_KERNEL);
+ hdwork->time_state = time_state;
+ INIT_WORK(&hdwork->work, hid_time_request_report_work);
+ schedule_work(&hdwork->work);
+ }
+
return ret;
}
--
1.8.1.4
On Fri, 19 Apr 2013, Alexander Holler wrote:
> as I'm not sure if the maintainer of the RTC subsystem is active again,
> I've added the people which where involved with rtc-hid-sensor-time
> before to cc too. That might be a good idea even he is active again,
> as the driver depends on hid-sensor-hub and as such works a bit
> outside the stuff other RTC drivers do use.
Agreed. I'd say let's wait if RTC maintainer(s) step up and have anything
to say to this patchset.
If there's a silence, it'll probably be ok taking it through my review
process and my tree, but at least for 3/3 I'd definitely like to have Ack
from someone else as well.
For this reason, this is likely not be 3.10 material, but 3.11.
Thanks,
--
Jiri Kosina
SUSE Labs
Am 21.04.2013 01:46, schrieb Jiri Kosina:
> On Fri, 19 Apr 2013, Alexander Holler wrote:
>
>> as I'm not sure if the maintainer of the RTC subsystem is active again,
>> I've added the people which where involved with rtc-hid-sensor-time
>> before to cc too. That might be a good idea even he is active again,
>> as the driver depends on hid-sensor-hub and as such works a bit
>> outside the stuff other RTC drivers do use.
>
> Agreed. I'd say let's wait if RTC maintainer(s) step up and have anything
> to say to this patchset.
>
> If there's a silence, it'll probably be ok taking it through my review
> process and my tree, but at least for 3/3 I'd definitely like to have Ack
> from someone else as well.
>
> For this reason, this is likely not be 3.10 material, but 3.11.
Thanks a lot.
I already have another patch here to support milliseconds (for hctosys),
and I might have another patch ready in a few days, which adjusts the
system time whenever the HID device sends a timestamp which includes
milliseconds (enabled by a new module option named adjtime for
rtc-hid-sensor-time, which is by default off). The basics do already
work, but I'm just starting to look at how to adjust the system time
best, so suggestions are welcome. ;)
(Therefor I've now added the maintainers for kernel/time/timekeeping.c
to cc).
Regards,
Alexander
On Fri, 19 Apr 2013 17:14:12 +0200 Alexander Holler <[email protected]> wrote:
> drivers/rtc/hctosys (CONFIG_RTC_HCTOSYS) doesn't work for
> rtc-hid-sensor-time because it will be called in late_init, and thus before
> rtc-hid-sensor-time gets loaded.
Isn't that true of all RTC drivers which are built as modules? There's
nothing special about hid-sensor-time here?
I assume the standard answer here is "your RTC driver should be built
into vmlinux". If we wish to make things work for modular RTC drivers
then we should find a solution which addresses *all* RTC drivers?
> To set the time through rtc-hid-sensor-time
> at startup, the module now checks by default if the system time is before
> 1970-01-02 and sets the system time (once) if this is the case.
>
> To disable this behaviour, set the module option hctosys to zero, e.g. by
> using rtc-hid-sensor-time.hctosys=0 at the kernel command line if the
> driver is statically linked into the kernel.
Is a bit hacky, no?
> @@ -237,6 +279,22 @@ static const struct rtc_class_ops hid_time_rtc_ops = {
> .read_time = hid_rtc_read_time,
> };
>
> +struct hid_time_work_time_state {
> + struct work_struct work;
> + struct hid_time_state *time_state;
> +};
> +
> +static void hid_time_request_report_work(struct work_struct *work)
> +{
> + struct hid_time_state *time_state =
> + ((struct hid_time_work_time_state *)work)->time_state;
Yikes. Use container_of() here.
Also, you don't *have* to initialise things at their definition site. So
struct hid_time_state *time_state =
some-ginormous-expression-which-overflows-80-columns;
becomes
struct hid_time_state *time_state;
time_state = some-ginormous-expression-which-no-longer-overflows-80-columns;
Simple, no?
> + /* get a report with all values through requesting one value */
> + sensor_hub_input_attr_get_raw_value(
> + time_state->common_attributes.hsdev, HID_USAGE_SENSOR_TIME,
> + hid_time_addresses[0], time_state->info[0].report_id);
> + kfree(work);
> +}
> +
> static int hid_time_probe(struct platform_device *pdev)
> {
> int ret = 0;
> @@ -287,6 +345,20 @@ static int hid_time_probe(struct platform_device *pdev)
> return PTR_ERR(time_state->rtc);
> }
>
> + if (!hid_time_time_set_once && hid_time_hctosys_enabled) {
> + /*
> + * Request a HID report to set the time.
> + * Calling sensor_hub_..._get_raw_value() here directly
> + * doesn't work, therefor we have to use a work.
> + */
> + struct hid_time_work_time_state *hdwork =
> + kmalloc(sizeof(struct hid_time_work_time_state),
> + GFP_KERNEL);
looky:
struct hid_time_work_time_state *hdwork;
hdwork = kmalloc(sizeof(*hdwork), GFP_KERNEL);
> + hdwork->time_state = time_state;
Forgot to check for kmalloc() failure.
> + INIT_WORK(&hdwork->work, hid_time_request_report_work);
> + schedule_work(&hdwork->work);
The patch adds a schedule_work() but no flush_scheduled_work(), etc.
So if the driver is shut down or rmmodded while the work is still
pending, the kernel will go kapow.
> + }
> +
> return ret;
> }
Am 23.04.2013 01:38, schrieb Andrew Morton:
> On Fri, 19 Apr 2013 17:14:12 +0200 Alexander Holler <[email protected]> wrote:
>
>> drivers/rtc/hctosys (CONFIG_RTC_HCTOSYS) doesn't work for
>> rtc-hid-sensor-time because it will be called in late_init, and thus before
>> rtc-hid-sensor-time gets loaded.
>
> Isn't that true of all RTC drivers which are built as modules? There's
> nothing special about hid-sensor-time here?
>
> I assume the standard answer here is "your RTC driver should be built
> into vmlinux". If we wish to make things work for modular RTC drivers
> then we should find a solution which addresses *all* RTC drivers?
No. I having rtc-hid-sensor-time, hid-sensor-hub (and USB) statically
linked in doesn't help. Here is what happens here with such an
configuration:
--
[ 7.638970] drivers/rtc/hctosys.c: unable to open rtc device (rtc0)
[ 7.645639] Waiting 180sec before mounting root device...
[ 16.598759] HID-SENSOR-2000a0 HID-SENSOR-2000a0.0: rtc core:
registered hid-sensor-time as rtc0
[ 16.608712] HID-SENSOR-2000a0 HID-SENSOR-2000a0.0: hctosys: setting
system clock to 2013-04-19 16:45:06 UTC (1366389906)
--
I havent't looked in detail at why rtc-hid-sensor-time gets loaded that
late, but I assume it's because the USB stack (and/or the device or the
communication inbetween) needs some time (and I assume that's why
rootwait and rootdelay got invented too).
>
>> To set the time through rtc-hid-sensor-time
>> at startup, the module now checks by default if the system time is before
>> 1970-01-02 and sets the system time (once) if this is the case.
>>
>> To disable this behaviour, set the module option hctosys to zero, e.g. by
>> using rtc-hid-sensor-time.hctosys=0 at the kernel command line if the
>> driver is statically linked into the kernel.
>
> Is a bit hacky, no?
I didn't have any other idea to prevent an USB (or any other
hot-pluggable HID) device to change the time while still beeing able (by
default) to set the time by such an device at boot. But I'm open to
suggestions. (E.g. one of the scenarios I want to prevent is, that a
computer gets it's time by NTP and someone is able to change the time
later on by simply plugging in some HID device.)
>
>> @@ -237,6 +279,22 @@ static const struct rtc_class_ops hid_time_rtc_ops = {
>> .read_time = hid_rtc_read_time,
>> };
>>
>> +struct hid_time_work_time_state {
>> + struct work_struct work;
>> + struct hid_time_state *time_state;
>> +};
>> +
>> +static void hid_time_request_report_work(struct work_struct *work)
>> +{
>> + struct hid_time_state *time_state =
>> + ((struct hid_time_work_time_state *)work)->time_state;
>
> Yikes. Use container_of() here.
Ok, will do.
>
> Also, you don't *have* to initialise things at their definition site. So
>
> struct hid_time_state *time_state =
> some-ginormous-expression-which-overflows-80-columns;
>
> becomes
>
> struct hid_time_state *time_state;
> time_state = some-ginormous-expression-which-no-longer-overflows-80-columns;
>
> Simple, no?
Depends on which maintainer one might end up. Everyone has it's own
preferred style and I've seen discussions about more silly things. ;)
Will change it.
>
>> + /* get a report with all values through requesting one value */
>> + sensor_hub_input_attr_get_raw_value(
>> + time_state->common_attributes.hsdev, HID_USAGE_SENSOR_TIME,
>> + hid_time_addresses[0], time_state->info[0].report_id);
>> + kfree(work);
>> +}
>> +
>> static int hid_time_probe(struct platform_device *pdev)
>> {
>> int ret = 0;
>> @@ -287,6 +345,20 @@ static int hid_time_probe(struct platform_device *pdev)
>> return PTR_ERR(time_state->rtc);
>> }
>>
>> + if (!hid_time_time_set_once && hid_time_hctosys_enabled) {
>> + /*
>> + * Request a HID report to set the time.
>> + * Calling sensor_hub_..._get_raw_value() here directly
>> + * doesn't work, therefor we have to use a work.
>> + */
>> + struct hid_time_work_time_state *hdwork =
>> + kmalloc(sizeof(struct hid_time_work_time_state),
>> + GFP_KERNEL);
>
> looky:
>
> struct hid_time_work_time_state *hdwork;
>
> hdwork = kmalloc(sizeof(*hdwork), GFP_KERNEL);
>
>> + hdwork->time_state = time_state;
>
> Forgot to check for kmalloc() failure.
>
>> + INIT_WORK(&hdwork->work, hid_time_request_report_work);
>> + schedule_work(&hdwork->work);
>
> The patch adds a schedule_work() but no flush_scheduled_work(), etc.
> So if the driver is shut down or rmmodded while the work is still
> pending, the kernel will go kapow.
>
Yes, I've forgotten those two things. Will fix them with a v2, if there
will be an agreement about the first two points.
Thanks for the review.
Alexander
Am 23.04.2013 10:51, schrieb Alexander Holler:
> Am 23.04.2013 01:38, schrieb Andrew Morton:
>> On Fri, 19 Apr 2013 17:14:12 +0200 Alexander Holler
>> <[email protected]> wrote:
>>
>>> drivers/rtc/hctosys (CONFIG_RTC_HCTOSYS) doesn't work for
>>> rtc-hid-sensor-time because it will be called in late_init, and thus
>>> before
>>> rtc-hid-sensor-time gets loaded.
>>
>> Isn't that true of all RTC drivers which are built as modules? There's
>> nothing special about hid-sensor-time here?
>>
>> I assume the standard answer here is "your RTC driver should be built
>> into vmlinux". If we wish to make things work for modular RTC drivers
>> then we should find a solution which addresses *all* RTC drivers?
>
> No. I having rtc-hid-sensor-time, hid-sensor-hub (and USB) statically
> linked in doesn't help. Here is what happens here with such an
> configuration:
>
> --
> [ 7.638970] drivers/rtc/hctosys.c: unable to open rtc device (rtc0)
> [ 7.645639] Waiting 180sec before mounting root device...
> [ 16.598759] HID-SENSOR-2000a0 HID-SENSOR-2000a0.0: rtc core:
> registered hid-sensor-time as rtc0
> [ 16.608712] HID-SENSOR-2000a0 HID-SENSOR-2000a0.0: hctosys: setting
> system clock to 2013-04-19 16:45:06 UTC (1366389906)
> --
>
> I havent't looked in detail at why rtc-hid-sensor-time gets loaded that
> late, but I assume it's because the USB stack (and/or the device or the
> communication inbetween) needs some time (and I assume that's why
> rootwait and rootdelay got invented too).
>
>>
>>> To set the time through rtc-hid-sensor-time
>>> at startup, the module now checks by default if the system time is
>>> before
>>> 1970-01-02 and sets the system time (once) if this is the case.
>>>
>>> To disable this behaviour, set the module option hctosys to zero,
>>> e.g. by
>>> using rtc-hid-sensor-time.hctosys=0 at the kernel command line if the
>>> driver is statically linked into the kernel.
>>
>> Is a bit hacky, no?
>
> I didn't have any other idea to prevent an USB (or any other
> hot-pluggable HID) device to change the time while still beeing able (by
> default) to set the time by such an device at boot. But I'm open to
> suggestions. (E.g. one of the scenarios I want to prevent is, that a
> computer gets it's time by NTP and someone is able to change the time
> later on by simply plugging in some HID device.)
>
To add something more: I use the system time as a bool
"time_was_set_once" and have choosen one day just in case something
needs really long to boot (e.g. because of some lengthy fsck or whatever
else).
A solution to both problems might be to change the logic for hctosys
completly to read the time when the first RTC device appears (or when
the device mentioned in CONFIG_RTC_HCTOSYS_DEVICE appears). But that
would require a change to hctosys or the RTC subsystem, which would
involve more patches and discussion. As rtc-hid-sensor-time currently
seems to be the only RTC with the above problems, I've gone the easy
route and only modified this driver.
Regards,
Alexander
Am 23.04.2013 12:08, schrieb Alexander Holler:
> Am 23.04.2013 10:51, schrieb Alexander Holler:
>> Am 23.04.2013 01:38, schrieb Andrew Morton:
>>> On Fri, 19 Apr 2013 17:14:12 +0200 Alexander Holler
>>> <[email protected]> wrote:
>>>
>>>> drivers/rtc/hctosys (CONFIG_RTC_HCTOSYS) doesn't work for
>>>> rtc-hid-sensor-time because it will be called in late_init, and thus
>>>> before
>>>> rtc-hid-sensor-time gets loaded.
>>>
>>> Isn't that true of all RTC drivers which are built as modules? There's
>>> nothing special about hid-sensor-time here?
>>>
>>> I assume the standard answer here is "your RTC driver should be built
>>> into vmlinux". If we wish to make things work for modular RTC drivers
>>> then we should find a solution which addresses *all* RTC drivers?
>>
>> No. I having rtc-hid-sensor-time, hid-sensor-hub (and USB) statically
>> linked in doesn't help. Here is what happens here with such an
>> configuration:
>>
>> --
>> [ 7.638970] drivers/rtc/hctosys.c: unable to open rtc device (rtc0)
>> [ 7.645639] Waiting 180sec before mounting root device...
>> [ 16.598759] HID-SENSOR-2000a0 HID-SENSOR-2000a0.0: rtc core:
>> registered hid-sensor-time as rtc0
>> [ 16.608712] HID-SENSOR-2000a0 HID-SENSOR-2000a0.0: hctosys: setting
>> system clock to 2013-04-19 16:45:06 UTC (1366389906)
>> --
>>
>> I havent't looked in detail at why rtc-hid-sensor-time gets loaded that
>> late, but I assume it's because the USB stack (and/or the device or the
>> communication inbetween) needs some time (and I assume that's why
>> rootwait and rootdelay got invented too).
>>
>>>
>>>> To set the time through rtc-hid-sensor-time
>>>> at startup, the module now checks by default if the system time is
>>>> before
>>>> 1970-01-02 and sets the system time (once) if this is the case.
>>>>
>>>> To disable this behaviour, set the module option hctosys to zero,
>>>> e.g. by
>>>> using rtc-hid-sensor-time.hctosys=0 at the kernel command line if the
>>>> driver is statically linked into the kernel.
>>>
>>> Is a bit hacky, no?
>>
>> I didn't have any other idea to prevent an USB (or any other
>> hot-pluggable HID) device to change the time while still beeing able (by
>> default) to set the time by such an device at boot. But I'm open to
>> suggestions. (E.g. one of the scenarios I want to prevent is, that a
>> computer gets it's time by NTP and someone is able to change the time
>> later on by simply plugging in some HID device.)
>>
>
> To add something more: I use the system time as a bool
> "time_was_set_once" and have choosen one day just in case something
> needs really long to boot (e.g. because of some lengthy fsck or whatever
> else).
>
> A solution to both problems might be to change the logic for hctosys
> completly to read the time when the first RTC device appears (or when
> the device mentioned in CONFIG_RTC_HCTOSYS_DEVICE appears). But that
> would require a change to hctosys or the RTC subsystem, which would
> involve more patches and discussion. As rtc-hid-sensor-time currently
> seems to be the only RTC with the above problems, I've gone the easy
> route and only modified this driver.
Oh, damn. I've forgotten my example above with NTP. In that case setting
the time when the first RTC appears doesn't work.
Regards,
Alexander
Am 23.04.2013 12:13, schrieb Alexander Holler:
> Am 23.04.2013 12:08, schrieb Alexander Holler:
>> Am 23.04.2013 10:51, schrieb Alexander Holler:
>>> Am 23.04.2013 01:38, schrieb Andrew Morton:
>>>> On Fri, 19 Apr 2013 17:14:12 +0200 Alexander Holler
>>>> <[email protected]> wrote:
>>>>
>>>>> drivers/rtc/hctosys (CONFIG_RTC_HCTOSYS) doesn't work for
>>>>> rtc-hid-sensor-time because it will be called in late_init, and thus
>>>>> before
>>>>> rtc-hid-sensor-time gets loaded.
>>>>
>>>> Isn't that true of all RTC drivers which are built as modules? There's
>>>> nothing special about hid-sensor-time here?
>>>>
>>>> I assume the standard answer here is "your RTC driver should be built
>>>> into vmlinux". If we wish to make things work for modular RTC drivers
>>>> then we should find a solution which addresses *all* RTC drivers?
>>>
>>> No. I having rtc-hid-sensor-time, hid-sensor-hub (and USB) statically
>>> linked in doesn't help. Here is what happens here with such an
>>> configuration:
>>>
>>> --
>>> [ 7.638970] drivers/rtc/hctosys.c: unable to open rtc device (rtc0)
>>> [ 7.645639] Waiting 180sec before mounting root device...
>>> [ 16.598759] HID-SENSOR-2000a0 HID-SENSOR-2000a0.0: rtc core:
>>> registered hid-sensor-time as rtc0
>>> [ 16.608712] HID-SENSOR-2000a0 HID-SENSOR-2000a0.0: hctosys: setting
>>> system clock to 2013-04-19 16:45:06 UTC (1366389906)
>>> --
>>>
>>> I havent't looked in detail at why rtc-hid-sensor-time gets loaded that
>>> late, but I assume it's because the USB stack (and/or the device or the
>>> communication inbetween) needs some time (and I assume that's why
>>> rootwait and rootdelay got invented too).
>>>
>>>>
>>>>> To set the time through rtc-hid-sensor-time
>>>>> at startup, the module now checks by default if the system time is
>>>>> before
>>>>> 1970-01-02 and sets the system time (once) if this is the case.
>>>>>
>>>>> To disable this behaviour, set the module option hctosys to zero,
>>>>> e.g. by
>>>>> using rtc-hid-sensor-time.hctosys=0 at the kernel command line if the
>>>>> driver is statically linked into the kernel.
>>>>
>>>> Is a bit hacky, no?
>>>
>>> I didn't have any other idea to prevent an USB (or any other
>>> hot-pluggable HID) device to change the time while still beeing able (by
>>> default) to set the time by such an device at boot. But I'm open to
>>> suggestions. (E.g. one of the scenarios I want to prevent is, that a
>>> computer gets it's time by NTP and someone is able to change the time
>>> later on by simply plugging in some HID device.)
>>>
>>
>> To add something more: I use the system time as a bool
>> "time_was_set_once" and have choosen one day just in case something
>> needs really long to boot (e.g. because of some lengthy fsck or whatever
>> else).
>>
>> A solution to both problems might be to change the logic for hctosys
>> completly to read the time when the first RTC device appears (or when
>> the device mentioned in CONFIG_RTC_HCTOSYS_DEVICE appears). But that
>> would require a change to hctosys or the RTC subsystem, which would
>> involve more patches and discussion. As rtc-hid-sensor-time currently
>> seems to be the only RTC with the above problems, I've gone the easy
>> route and only modified this driver.
>
> Oh, damn. I've forgotten my example above with NTP. In that case setting
> the time when the first RTC appears doesn't work.
So a general solution might be to set the system time when the first RTC
(or the one mentioned in CONFIG_RTC_HCTOSYS_DEVICE) appears AND nothing
else did set the time before.
Regards,
Alexander
Am 23.04.2013 12:17, schrieb Alexander Holler:
> Am 23.04.2013 12:13, schrieb Alexander Holler:
>> Am 23.04.2013 12:08, schrieb Alexander Holler:
>>> Am 23.04.2013 10:51, schrieb Alexander Holler:
>>>> Am 23.04.2013 01:38, schrieb Andrew Morton:
>>>>> On Fri, 19 Apr 2013 17:14:12 +0200 Alexander Holler
>>>>> <[email protected]> wrote:
>>>>>
>>>>>> drivers/rtc/hctosys (CONFIG_RTC_HCTOSYS) doesn't work for
>>>>>> rtc-hid-sensor-time because it will be called in late_init, and thus
>>>>>> before
>>>>>> rtc-hid-sensor-time gets loaded.
>>>>>
>>>>> Isn't that true of all RTC drivers which are built as modules?
>>>>> There's
>>>>> nothing special about hid-sensor-time here?
>>>>>
>>>>> I assume the standard answer here is "your RTC driver should be built
>>>>> into vmlinux". If we wish to make things work for modular RTC drivers
>>>>> then we should find a solution which addresses *all* RTC drivers?
>>>>
>>>> No. I having rtc-hid-sensor-time, hid-sensor-hub (and USB) statically
>>>> linked in doesn't help. Here is what happens here with such an
>>>> configuration:
>>>>
>>>> --
>>>> [ 7.638970] drivers/rtc/hctosys.c: unable to open rtc device (rtc0)
>>>> [ 7.645639] Waiting 180sec before mounting root device...
>>>> [ 16.598759] HID-SENSOR-2000a0 HID-SENSOR-2000a0.0: rtc core:
>>>> registered hid-sensor-time as rtc0
>>>> [ 16.608712] HID-SENSOR-2000a0 HID-SENSOR-2000a0.0: hctosys: setting
>>>> system clock to 2013-04-19 16:45:06 UTC (1366389906)
>>>> --
>>>>
>>>> I havent't looked in detail at why rtc-hid-sensor-time gets loaded that
>>>> late, but I assume it's because the USB stack (and/or the device or the
>>>> communication inbetween) needs some time (and I assume that's why
>>>> rootwait and rootdelay got invented too).
>>>>
>>>>>
>>>>>> To set the time through rtc-hid-sensor-time
>>>>>> at startup, the module now checks by default if the system time is
>>>>>> before
>>>>>> 1970-01-02 and sets the system time (once) if this is the case.
>>>>>>
>>>>>> To disable this behaviour, set the module option hctosys to zero,
>>>>>> e.g. by
>>>>>> using rtc-hid-sensor-time.hctosys=0 at the kernel command line if the
>>>>>> driver is statically linked into the kernel.
>>>>>
>>>>> Is a bit hacky, no?
>>>>
>>>> I didn't have any other idea to prevent an USB (or any other
>>>> hot-pluggable HID) device to change the time while still beeing able
>>>> (by
>>>> default) to set the time by such an device at boot. But I'm open to
>>>> suggestions. (E.g. one of the scenarios I want to prevent is, that a
>>>> computer gets it's time by NTP and someone is able to change the time
>>>> later on by simply plugging in some HID device.)
>>>>
>>>
>>> To add something more: I use the system time as a bool
>>> "time_was_set_once" and have choosen one day just in case something
>>> needs really long to boot (e.g. because of some lengthy fsck or whatever
>>> else).
>>>
>>> A solution to both problems might be to change the logic for hctosys
>>> completly to read the time when the first RTC device appears (or when
>>> the device mentioned in CONFIG_RTC_HCTOSYS_DEVICE appears). But that
>>> would require a change to hctosys or the RTC subsystem, which would
>>> involve more patches and discussion. As rtc-hid-sensor-time currently
>>> seems to be the only RTC with the above problems, I've gone the easy
>>> route and only modified this driver.
>>
>> Oh, damn. I've forgotten my example above with NTP. In that case setting
>> the time when the first RTC appears doesn't work.
>
> So a general solution might be to set the system time when the first RTC
> (or the one mentioned in CONFIG_RTC_HCTOSYS_DEVICE) appears AND nothing
> else did set the time before.
To extend that idea a bit further, I think an option timewait (similiar
to rootwait) would be a nice to have.
If just time wouldn't be that rare ... ;)
Regards,
Alexander
On Tue, 23 Apr 2013 17:47:20 +0200 Alexander Holler <[email protected]> wrote:
> >>> "time_was_set_once" and have choosen one day just in case something
> >>> needs really long to boot (e.g. because of some lengthy fsck or whatever
> >>> else).
> >>>
> >>> A solution to both problems might be to change the logic for hctosys
> >>> completly to read the time when the first RTC device appears (or when
> >>> the device mentioned in CONFIG_RTC_HCTOSYS_DEVICE appears). But that
> >>> would require a change to hctosys or the RTC subsystem, which would
> >>> involve more patches and discussion. As rtc-hid-sensor-time currently
> >>> seems to be the only RTC with the above problems, I've gone the easy
> >>> route and only modified this driver.
> >>
> >> Oh, damn. I've forgotten my example above with NTP. In that case setting
> >> the time when the first RTC appears doesn't work.
> >
> > So a general solution might be to set the system time when the first RTC
> > (or the one mentioned in CONFIG_RTC_HCTOSYS_DEVICE) appears AND nothing
> > else did set the time before.
>
> To extend that idea a bit further, I think an option timewait (similiar
> to rootwait) would be a nice to have.
>
> If just time wouldn't be that rare ... ;)
Yes, some sort of notifier callout when the CONFIG_RTC_HCTOSYS_DEVICE
device is registered seems appropriate. I didn't understand why that
is problematic for NTP.
Getting all the lifetime/reference stuff right will be tricky :( Can't
shut down or unload a driver when it is on that notification list.
And it assumes that the CONFIG_RTC_HCTOSYS_DEVICE driver is all ready
to go when it registers itself. Hopefully that is true, but they
should be reviewed.
Adding the timewait thing sounds unpleasant - how to reliably tune the
interval for all possible users of your distro? We should aim to make
things synchronous, deterministic and
stuff-happens-in-the-correct-order.
It all sounds like a moderate amount of work, but it would be great
to be able to fix this for all drivers, not just hid-sensor-time. That's
assuming that other drivers have the same issue - perhaps they don't,
but perhaps things can go wrong if the module loading order is wrong?
Am 24.04.2013 23:14, schrieb Andrew Morton:
> On Tue, 23 Apr 2013 17:47:20 +0200 Alexander Holler <[email protected]> wrote:
>
>>>>> "time_was_set_once" and have choosen one day just in case something
>>>>> needs really long to boot (e.g. because of some lengthy fsck or whatever
>>>>> else).
>>>>>
>>>>> A solution to both problems might be to change the logic for hctosys
>>>>> completly to read the time when the first RTC device appears (or when
>>>>> the device mentioned in CONFIG_RTC_HCTOSYS_DEVICE appears). But that
>>>>> would require a change to hctosys or the RTC subsystem, which would
>>>>> involve more patches and discussion. As rtc-hid-sensor-time currently
>>>>> seems to be the only RTC with the above problems, I've gone the easy
>>>>> route and only modified this driver.
>>>>
>>>> Oh, damn. I've forgotten my example above with NTP. In that case setting
>>>> the time when the first RTC appears doesn't work.
>>>
>>> So a general solution might be to set the system time when the first RTC
>>> (or the one mentioned in CONFIG_RTC_HCTOSYS_DEVICE) appears AND nothing
>>> else did set the time before.
>>
>> To extend that idea a bit further, I think an option timewait (similiar
>> to rootwait) would be a nice to have.
>>
>> If just time wouldn't be that rare ... ;)
>
> Yes, some sort of notifier callout when the CONFIG_RTC_HCTOSYS_DEVICE
> device is registered seems appropriate. I didn't understand why that
> is problematic for NTP.
>
> Getting all the lifetime/reference stuff right will be tricky :( Can't
> shut down or unload a driver when it is on that notification list.
>
> And it assumes that the CONFIG_RTC_HCTOSYS_DEVICE driver is all ready
> to go when it registers itself. Hopefully that is true, but they
> should be reviewed.
>
> Adding the timewait thing sounds unpleasant - how to reliably tune the
> interval for all possible users of your distro? We should aim to make
> things synchronous, deterministic and
> stuff-happens-in-the-correct-order.
>
> It all sounds like a moderate amount of work, but it would be great
> to be able to fix this for all drivers, not just hid-sensor-time. That's
> assuming that other drivers have the same issue - perhaps they don't,
> but perhaps things can go wrong if the module loading order is wrong?
>
I've added John Stultz to cc as he seems to work on a similiar problem
(to not set the time twice on resume).
I'm not sure what you meant with the notifiers, but wouldn't be a
function which sets the time if nothing else did that before a solution?
I think about something like that (not real code):
static bool timeWasSet; // = 0
int setTimeIfNotSet(time, devicename)
{
if (timeWasSet || (devicename && CONFIG_RTC_HCTOSYS_DEVICE &&
devicename != CONFIG_RTC_HCTOSYS_DEVICE )
return -1;
timeWasSet = 1;
do_settimeofday(time);
return 0;
}
That "timewait" kernel option I mentioned above then would be easy too:
void timewait(void)
{
while (!timeWasSet)
sleepOrSimiliar();
}
That setTimeIfNotSet() function could be called by the RTC subsystem
whenever a new RTC will be registered and CONFIG_RTC_HCTOSYS is enabled
(or on resume).
In regard to the persistent_clocks on x86 I know almost nothing. I
didn't even knew they do exist before I've seen a discussion about
HCTOSYS yesterday. I thought the RTC_CMOS driver is responsible for the
time on x86. ;) Therfor I can't say anything about how things do work there.
Whats missing above is something which sets timeWasSet to 1 if userland
(NTP or similiar) does set the time. That could be done always (in
do_settimeofday) or e.g. by using a function pointer to
do_settimeofday() which first points to a function which sets timeWasSet
and later on points to do_settimeofday() directly.
Maybe it's all silly (I'm missing the big overview over time in the
kernel), but thats what first entered my mind while thinking about how
to avoid changing a time by an HID device if it was already set by
userland/NTP or another clock (besides that trick in testing for system
date < 1970-01-02 I've then used in my patch because it was easy to
implement while not doing changes to timekeeping itself).
Regards,
Alexander
Changes to the first series:
- Patch 1 and 2 are unchanged,
- Patch 3 uses getnstimeofday() instead of do_gettimeofday() which spares
a variable and it should include all changes requested through the
review by Andrew Morton. I left in the "hacky" part but included a more
verbose comment in the source why the "hacky" looking part is there,
- Patch 4 is new and adds support for milliseconds (currently only used
if the system time will be set by the driver).
Regards,
Alexander Holler
The draft for HID-sensors (HUTRR39) currently doesn't define the range
for the attribute year. Asking one of the authors revealed that full years
(e.g. 2013 instead of just 13) were meant.
So we now allow both, 8 bit and 16 bit values for the attribute year and
assuming full years when the value is 16 bits wide.
We will still support 8 bit values until the specification gets final
(and maybe defines a way to set the time too).
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 33 ++++++++++++++++++++++++++-------
1 file changed, 26 insertions(+), 7 deletions(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index 6302450..b8c1b10 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -85,10 +85,13 @@ static int hid_time_capture_sample(struct hid_sensor_hub_device *hsdev,
switch (usage_id) {
case HID_USAGE_SENSOR_TIME_YEAR:
- time_buf->tm_year = *(u8 *)raw_data;
- if (time_buf->tm_year < 70)
- /* assume we are in 1970...2069 */
- time_buf->tm_year += 100;
+ if (raw_len == 1) {
+ time_buf->tm_year = *(u8 *)raw_data;
+ if (time_buf->tm_year < 70)
+ /* assume we are in 1970...2069 */
+ time_buf->tm_year += 100;
+ } else
+ time_buf->tm_year = *(u16 *)raw_data-1900;
break;
case HID_USAGE_SENSOR_TIME_MONTH:
/* sensor sending the month as 1-12, we need 0-11 */
@@ -151,11 +154,27 @@ static int hid_time_parse_report(struct platform_device *pdev,
return -EINVAL;
}
if (time_state->info[i].size != 1) {
- dev_err(&pdev->dev,
- "attribute '%s' not 8 bits wide!\n",
+ /*
+ * The draft for HID-sensors (HUTRR39) currently
+ * doesn't define the range for the year attribute.
+ * Therefor we support both 8 bit (0-99) and 16 bit
+ * (full) as size for the year.
+ */
+ if (time_state->info[i].attrib_id !=
+ HID_USAGE_SENSOR_TIME_YEAR) {
+ dev_err(&pdev->dev,
+ "attribute '%s' not 8 bits wide!\n",
hid_time_attrib_name(
time_state->info[i].attrib_id));
- return -EINVAL;
+ return -EINVAL;
+ }
+ if (time_state->info[i].size != 2) {
+ dev_err(&pdev->dev,
+ "attribute '%s' not 8 or 16 bits wide!\n",
+ hid_time_attrib_name(
+ time_state->info[i].attrib_id));
+ return -EINVAL;
+ }
}
if (time_state->info[i].units !=
HID_USAGE_SENSOR_UNITS_NOT_SPECIFIED &&
--
1.8.1.5
There is no real reason to not support 16 or 32 bit values too.
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 59 +++++++++++++++++++++------------------
1 file changed, 32 insertions(+), 27 deletions(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index b8c1b10..7273b01 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -76,6 +76,20 @@ static int hid_time_proc_event(struct hid_sensor_hub_device *hsdev,
return 0;
}
+static u32 hid_time_value(size_t raw_len, char *raw_data)
+{
+ switch (raw_len) {
+ case 1:
+ return *(u8 *)raw_data;
+ case 2:
+ return *(u16 *)raw_data;
+ case 4:
+ return *(u32 *)raw_data;
+ default:
+ return (u32)(~0U); /* 0xff... or -1 to denote an error */
+ }
+}
+
static int hid_time_capture_sample(struct hid_sensor_hub_device *hsdev,
unsigned usage_id, size_t raw_len,
char *raw_data, void *priv)
@@ -85,29 +99,35 @@ static int hid_time_capture_sample(struct hid_sensor_hub_device *hsdev,
switch (usage_id) {
case HID_USAGE_SENSOR_TIME_YEAR:
+ /*
+ * The draft for HID-sensors (HUTRR39) currently doesn't define
+ * the range for the year attribute. Therefor we support
+ * 8 bit (0-99) and 16 or 32 bits (full) as size for the year.
+ */
if (raw_len == 1) {
time_buf->tm_year = *(u8 *)raw_data;
if (time_buf->tm_year < 70)
/* assume we are in 1970...2069 */
time_buf->tm_year += 100;
} else
- time_buf->tm_year = *(u16 *)raw_data-1900;
+ time_buf->tm_year =
+ (int)hid_time_value(raw_len, raw_data)-1900;
break;
case HID_USAGE_SENSOR_TIME_MONTH:
- /* sensor sending the month as 1-12, we need 0-11 */
- time_buf->tm_mon = *(u8 *)raw_data-1;
+ /* sensors are sending the month as 1-12, we need 0-11 */
+ time_buf->tm_mon = (int)hid_time_value(raw_len, raw_data)-1;
break;
case HID_USAGE_SENSOR_TIME_DAY:
- time_buf->tm_mday = *(u8 *)raw_data;
+ time_buf->tm_mday = (int)hid_time_value(raw_len, raw_data);
break;
case HID_USAGE_SENSOR_TIME_HOUR:
- time_buf->tm_hour = *(u8 *)raw_data;
+ time_buf->tm_hour = (int)hid_time_value(raw_len, raw_data);
break;
case HID_USAGE_SENSOR_TIME_MINUTE:
- time_buf->tm_min = *(u8 *)raw_data;
+ time_buf->tm_min = (int)hid_time_value(raw_len, raw_data);
break;
case HID_USAGE_SENSOR_TIME_SECOND:
- time_buf->tm_sec = *(u8 *)raw_data;
+ time_buf->tm_sec = (int)hid_time_value(raw_len, raw_data);
break;
default:
return -EINVAL;
@@ -153,28 +173,13 @@ static int hid_time_parse_report(struct platform_device *pdev,
"not all needed attributes inside the same report!\n");
return -EINVAL;
}
- if (time_state->info[i].size != 1) {
- /*
- * The draft for HID-sensors (HUTRR39) currently
- * doesn't define the range for the year attribute.
- * Therefor we support both 8 bit (0-99) and 16 bit
- * (full) as size for the year.
- */
- if (time_state->info[i].attrib_id !=
- HID_USAGE_SENSOR_TIME_YEAR) {
- dev_err(&pdev->dev,
- "attribute '%s' not 8 bits wide!\n",
- hid_time_attrib_name(
- time_state->info[i].attrib_id));
- return -EINVAL;
- }
- if (time_state->info[i].size != 2) {
- dev_err(&pdev->dev,
- "attribute '%s' not 8 or 16 bits wide!\n",
+ if (time_state->info[i].size == 3 ||
+ time_state->info[i].size > 4) {
+ dev_err(&pdev->dev,
+ "attribute '%s' not 8, 16 or 32 bits wide!\n",
hid_time_attrib_name(
time_state->info[i].attrib_id));
- return -EINVAL;
- }
+ return -EINVAL;
}
if (time_state->info[i].units !=
HID_USAGE_SENSOR_UNITS_NOT_SPECIFIED &&
--
1.8.1.5
drivers/rtc/hctosys (CONFIG_RTC_HCTOSYS) doesn't work for
rtc-hid-sensor-time because it will be called in late_init, and thus before
rtc-hid-sensor-time gets loaded. To set the time through
rtc-hid-sensor-time at startup, the module now checks by default if the
system time is before 1970-01-02 and sets the system time (once) if this is
the case.
To disable this behaviour, set the module option hctosys to zero, e.g. by
using rtc-hid-sensor-time.hctosys=0 at the kernel command line if the
driver is statically linked into the kernel.
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 94 +++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index 7273b01..db8503a 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -27,6 +27,12 @@
/* Usage ID from spec for Time: 0x2000A0 */
#define DRIVER_NAME "HID-SENSOR-2000a0" /* must be lowercase */
+static bool hid_time_hctosys_enabled = 1;
+module_param_named(hctosys, hid_time_hctosys_enabled, bool, 0644);
+MODULE_PARM_DESC(hctosys,
+ "set the system time (once) if it is before 1970-01-02");
+static bool hid_time_time_set_once;
+
enum hid_time_channel {
CHANNEL_SCAN_INDEX_YEAR,
CHANNEL_SCAN_INDEX_MONTH,
@@ -37,6 +43,11 @@ enum hid_time_channel {
TIME_RTC_CHANNEL_MAX,
};
+struct hid_time_workts {
+ struct work_struct work;
+ struct hid_time_state *time_state;
+};
+
struct hid_time_state {
struct hid_sensor_hub_callbacks callbacks;
struct hid_sensor_common common_attributes;
@@ -46,6 +57,7 @@ struct hid_time_state {
struct completion comp_last_time;
struct rtc_time time_buf;
struct rtc_device *rtc;
+ struct hid_time_workts *workts;
};
static const u32 hid_time_addresses[TIME_RTC_CHANNEL_MAX] = {
@@ -62,6 +74,49 @@ static const char * const hid_time_channel_names[TIME_RTC_CHANNEL_MAX] = {
"year", "month", "day", "hour", "minute", "second",
};
+static void hid_time_hctosys(struct hid_time_state *time_state)
+{
+ struct timespec ts;
+ int rc = rtc_valid_tm(&time_state->last_time);
+
+ if (rc) {
+ dev_err(time_state->rtc->dev.parent,
+ "hctosys: invalid date/time\n");
+ return;
+ }
+
+ getnstimeofday(&ts);
+ if (ts.tv_sec >= 86400) /* 1970-01-02 00:00:00 UTC */
+ /*
+ * Security: don't let a hot-pluggable device change
+ * a valid system time.
+ * In absence of a kernel-wide flag like 'systime_was_set',
+ * we check if the system time is earlier than 1970-01-02.
+ * Just using a flag inside the RTC subsystem (e.g. from
+ * hctosys) doesn't work, because the time could be set
+ * by userspace (NTP, date, user, ...) or something else
+ * in the kernel too.
+ * Using a whole day to decide if something else has set the
+ * time should be enough for even the longest boot to complete
+ * (including possible forced fscks) and load this module.
+ */
+ return;
+
+ ts.tv_nsec = NSEC_PER_SEC >> 1;
+ rtc_tm_to_time(&time_state->last_time, &ts.tv_sec);
+ rc = do_settimeofday(&ts);
+ dev_info(time_state->rtc->dev.parent,
+ "hctosys: setting system clock to "
+ "%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
+ time_state->last_time.tm_year + 1900,
+ time_state->last_time.tm_mon + 1,
+ time_state->last_time.tm_mday,
+ time_state->last_time.tm_hour,
+ time_state->last_time.tm_min,
+ time_state->last_time.tm_sec,
+ (unsigned int) ts.tv_sec);
+}
+
/* Callback handler to send event after all samples are received and captured */
static int hid_time_proc_event(struct hid_sensor_hub_device *hsdev,
unsigned usage_id, void *priv)
@@ -73,6 +128,10 @@ static int hid_time_proc_event(struct hid_sensor_hub_device *hsdev,
time_state->last_time = time_state->time_buf;
spin_unlock_irqrestore(&time_state->lock_last_time, flags);
complete(&time_state->comp_last_time);
+ if (unlikely(!hid_time_time_set_once && hid_time_hctosys_enabled)) {
+ hid_time_time_set_once = 1;
+ hid_time_hctosys(time_state);
+ }
return 0;
}
@@ -237,6 +296,19 @@ static const struct rtc_class_ops hid_time_rtc_ops = {
.read_time = hid_rtc_read_time,
};
+static void hid_time_request_report_work(struct work_struct *work)
+{
+ struct hid_time_state *time_state =
+ container_of(work, struct hid_time_workts, work)
+ ->time_state;
+ /* get a report with all values through requesting one value */
+ sensor_hub_input_attr_get_raw_value(
+ time_state->common_attributes.hsdev, HID_USAGE_SENSOR_TIME,
+ hid_time_addresses[0], time_state->info[0].report_id);
+ time_state->workts = NULL;
+ kfree(work);
+}
+
static int hid_time_probe(struct platform_device *pdev)
{
int ret = 0;
@@ -288,13 +360,35 @@ static int hid_time_probe(struct platform_device *pdev)
return PTR_ERR(time_state->rtc);
}
+ if (!hid_time_time_set_once && hid_time_hctosys_enabled) {
+ /*
+ * Request a HID report to set the time.
+ * Calling sensor_hub_..._get_raw_value() here directly
+ * doesn't work, therefor we have to use a work.
+ */
+ time_state->workts = kmalloc(sizeof(struct hid_time_workts),
+ GFP_KERNEL);
+ if (time_state->workts == NULL)
+ return -ENOMEM;
+ time_state->workts->time_state = time_state;
+ INIT_WORK(&time_state->workts->work,
+ hid_time_request_report_work);
+ schedule_work(&time_state->workts->work);
+ }
+
return ret;
}
static int hid_time_remove(struct platform_device *pdev)
{
struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
+ struct hid_time_state *time_state = platform_get_drvdata(pdev);
+ if (time_state->workts) {
+ cancel_work_sync(&time_state->workts->work);
+ kfree(time_state->workts);
+ time_state->workts = NULL;
+ }
sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
return 0;
--
1.8.1.5
If a device sends milliseconds too, the driver will use them if it sets
the system clock at startup (through module option hctosys).
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 37 +++++++++++++++++++++++++++++++++----
include/linux/hid-sensor-ids.h | 1 +
2 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index db8503a..4aca12d 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -40,6 +40,7 @@ enum hid_time_channel {
CHANNEL_SCAN_INDEX_HOUR,
CHANNEL_SCAN_INDEX_MINUTE,
CHANNEL_SCAN_INDEX_SECOND,
+ CHANNEL_SCAN_INDEX_MILLISECOND, /* optional */
TIME_RTC_CHANNEL_MAX,
};
@@ -53,9 +54,11 @@ struct hid_time_state {
struct hid_sensor_common common_attributes;
struct hid_sensor_hub_attribute_info info[TIME_RTC_CHANNEL_MAX];
struct rtc_time last_time;
+ unsigned last_ms; /* == UINT_MAX to indicate ms aren't supported */
spinlock_t lock_last_time;
struct completion comp_last_time;
struct rtc_time time_buf;
+ unsigned ms_buf;
struct rtc_device *rtc;
struct hid_time_workts *workts;
};
@@ -67,16 +70,18 @@ static const u32 hid_time_addresses[TIME_RTC_CHANNEL_MAX] = {
HID_USAGE_SENSOR_TIME_HOUR,
HID_USAGE_SENSOR_TIME_MINUTE,
HID_USAGE_SENSOR_TIME_SECOND,
+ HID_USAGE_SENSOR_TIME_MILLISECOND, /* optional */
};
/* Channel names for verbose error messages */
static const char * const hid_time_channel_names[TIME_RTC_CHANNEL_MAX] = {
- "year", "month", "day", "hour", "minute", "second",
+ "year", "month", "day", "hour", "minute", "second", "millisecond",
};
static void hid_time_hctosys(struct hid_time_state *time_state)
{
struct timespec ts;
+ char msbuf[5];
int rc = rtc_valid_tm(&time_state->last_time);
if (rc) {
@@ -102,18 +107,25 @@ static void hid_time_hctosys(struct hid_time_state *time_state)
*/
return;
- ts.tv_nsec = NSEC_PER_SEC >> 1;
+ if (time_state->last_ms != UINT_MAX) {
+ ts.tv_nsec = time_state->last_ms * NSEC_PER_MSEC;
+ snprintf(msbuf, sizeof(msbuf), ":%03d", time_state->last_ms);
+ } else {
+ ts.tv_nsec = NSEC_PER_SEC >> 1;
+ *msbuf = 0;
+ }
rtc_tm_to_time(&time_state->last_time, &ts.tv_sec);
rc = do_settimeofday(&ts);
dev_info(time_state->rtc->dev.parent,
"hctosys: setting system clock to "
- "%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
+ "%d-%02d-%02d %02d:%02d:%02d%s UTC (%u)\n",
time_state->last_time.tm_year + 1900,
time_state->last_time.tm_mon + 1,
time_state->last_time.tm_mday,
time_state->last_time.tm_hour,
time_state->last_time.tm_min,
time_state->last_time.tm_sec,
+ msbuf,
(unsigned int) ts.tv_sec);
}
@@ -126,6 +138,8 @@ static int hid_time_proc_event(struct hid_sensor_hub_device *hsdev,
spin_lock_irqsave(&time_state->lock_last_time, flags);
time_state->last_time = time_state->time_buf;
+ if (time_state->last_ms != UINT_MAX)
+ time_state->last_ms = time_state->ms_buf;
spin_unlock_irqrestore(&time_state->lock_last_time, flags);
complete(&time_state->comp_last_time);
if (unlikely(!hid_time_time_set_once && hid_time_hctosys_enabled)) {
@@ -188,6 +202,9 @@ static int hid_time_capture_sample(struct hid_sensor_hub_device *hsdev,
case HID_USAGE_SENSOR_TIME_SECOND:
time_buf->tm_sec = (int)hid_time_value(raw_len, raw_data);
break;
+ case HID_USAGE_SENSOR_TIME_MILLISECOND:
+ time_state->ms_buf = hid_time_value(raw_len, raw_data);
+ break;
default:
return -EINVAL;
}
@@ -214,12 +231,18 @@ static int hid_time_parse_report(struct platform_device *pdev,
{
int report_id, i;
- for (i = 0; i < TIME_RTC_CHANNEL_MAX; ++i)
+ /* Check if all required attributes are available */
+ for (i = 0; i < CHANNEL_SCAN_INDEX_MILLISECOND; ++i)
if (sensor_hub_input_get_attribute_info(hsdev,
HID_INPUT_REPORT, usage_id,
hid_time_addresses[i],
&time_state->info[i]) < 0)
return -EINVAL;
+ if (!sensor_hub_input_get_attribute_info(hsdev, HID_INPUT_REPORT,
+ usage_id, hid_time_addresses[i], &time_state->info[i])) {
+ dev_info(&pdev->dev, "milliseconds supported\n");
+ time_state->last_ms = 0;
+ }
/* Check the (needed) attributes for sanity */
report_id = time_state->info[0].report_id;
if (report_id < 0) {
@@ -227,6 +250,10 @@ static int hid_time_parse_report(struct platform_device *pdev,
return -EINVAL;
}
for (i = 0; i < TIME_RTC_CHANNEL_MAX; ++i) {
+ if (time_state->info[i].attrib_id ==
+ HID_USAGE_SENSOR_TIME_MILLISECOND &&
+ time_state->last_ms == UINT_MAX)
+ continue;
if (time_state->info[i].report_id != report_id) {
dev_err(&pdev->dev,
"not all needed attributes inside the same report!\n");
@@ -319,6 +346,8 @@ static int hid_time_probe(struct platform_device *pdev)
if (time_state == NULL)
return -ENOMEM;
+ time_state->last_ms = UINT_MAX;
+
platform_set_drvdata(pdev, time_state);
spin_lock_init(&time_state->lock_last_time);
diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
index 6f24446..b519c47 100644
--- a/include/linux/hid-sensor-ids.h
+++ b/include/linux/hid-sensor-ids.h
@@ -74,6 +74,7 @@
#define HID_USAGE_SENSOR_TIME_HOUR 0x200525
#define HID_USAGE_SENSOR_TIME_MINUTE 0x200526
#define HID_USAGE_SENSOR_TIME_SECOND 0x200527
+#define HID_USAGE_SENSOR_TIME_MILLISECOND 0x200528
/* Units */
#define HID_USAGE_SENSOR_UNITS_NOT_SPECIFIED 0x00
--
1.8.1.5
On Sun, 5 May 2013 13:21:26 +0200 Alexander Holler <[email protected]> wrote:
> drivers/rtc/hctosys (CONFIG_RTC_HCTOSYS) doesn't work for
> rtc-hid-sensor-time because it will be called in late_init, and thus before
> rtc-hid-sensor-time gets loaded. To set the time through
> rtc-hid-sensor-time at startup, the module now checks by default if the
> system time is before 1970-01-02 and sets the system time (once) if this is
> the case.
>
> To disable this behaviour, set the module option hctosys to zero, e.g. by
> using rtc-hid-sensor-time.hctosys=0 at the kernel command line if the
> driver is statically linked into the kernel.
I still find this rather unpleasant. Partly because it's hacky, mainly
because it only solves the problem for one driver.
Can we please try harder to find a more general fix?
For example: if hctosys finds there are no drivers available, it sets a
flag. Later when drivers are registered(?), that flag is queried and,
if set, we set the system time at this time.
Or something.
On 05/05/2013 04:21 AM, Alexander Holler wrote:
> drivers/rtc/hctosys (CONFIG_RTC_HCTOSYS) doesn't work for
> rtc-hid-sensor-time because it will be called in late_init, and thus before
> rtc-hid-sensor-time gets loaded. To set the time through
> rtc-hid-sensor-time at startup, the module now checks by default if the
> system time is before 1970-01-02 and sets the system time (once) if this is
> the case.
>
> To disable this behaviour, set the module option hctosys to zero, e.g. by
> using rtc-hid-sensor-time.hctosys=0 at the kernel command line if the
> driver is statically linked into the kernel.
Sorry I missed this earlier, it fell into my spam box for some reason.
Like Andrew, I think this feels particularly hacky.
Why exactly is late_init too early? (I'm unfamiliar with the
rtc-hid-sensor-time driver)
If this is a hotplug rtc device (why I'm guessing its not available at
late_init), would it not be better to leave the setting of time using
hwclock --hctosys via a udev rule or something?
thanks
-john
Am 22.05.2013 00:02, schrieb John Stultz:
> On 05/05/2013 04:21 AM, Alexander Holler wrote:
>> drivers/rtc/hctosys (CONFIG_RTC_HCTOSYS) doesn't work for
>> rtc-hid-sensor-time because it will be called in late_init, and thus
>> before
>> rtc-hid-sensor-time gets loaded. To set the time through
>> rtc-hid-sensor-time at startup, the module now checks by default if the
>> system time is before 1970-01-02 and sets the system time (once) if
>> this is
>> the case.
>>
>> To disable this behaviour, set the module option hctosys to zero, e.g. by
>> using rtc-hid-sensor-time.hctosys=0 at the kernel command line if the
>> driver is statically linked into the kernel.
>
> Sorry I missed this earlier, it fell into my spam box for some reason.
>
I've recently heard that gmail seems to move my mails to a SPAM folder
(too). As I don't know why, I can't help with that and I have to live
with the fact that I seem to get censored by Google (if it isn't a
problem of my mail setup, but I'm not aware of such). Anyway, I'm sure
some people like that I'm silenced this way. ;)
>
> Like Andrew, I think this feels particularly hacky.
>
> Why exactly is late_init too early? (I'm unfamiliar with the
> rtc-hid-sensor-time driver)
Currently it can be an USB device (and maybe Bluetooth or even i2c in
the future, depends on hid-sensor-hub). That has some implications:
(1) Initialization might need longer (or happens later) than late_init,
even if everything is linked into the kernel (same problem as with a
boot from USB-storage)
(2) It might not even be available at boot, but it should work if a user
plugs it in afterwards.
(3) To accomplish (2) it should set the system time (by default) IFF
nothing else did set the time.
That "nothing else" in (3) is for security reasons, because no plugable
HID device should be able to change the system time by default.
The check if something else did set the system time can't be
accomplished only by the RTC subsystem because userspace, network or
whatever else is able to set the system time most likely doesn't use the
RTC subsystem (or hctosys).
E.g. one of those setups could be:
boot
hctosys (fails because of no RTC)
ntpdate/rdate/date < whatever
load modules (rtc-hid-sensor-time)
If we would use a flag in the hctosys module then rtc-hid-sensor-time
would be able to change the time (in the setup above).
Using a module option which is by default off doesn't help too. Users
(or even distros) which would turn it on, might forget it and systems
would be at risk if no HID clock will be found at boot (but later
plugged in by some blackhat).
A flag in the time subsystem itself would do the trick. Such a flag
might help with the problem if the RTC subsystem or the persistent clock
code did set the time too. You've mentioned in another thread that you
had to solve such a problem, but I'm not aware how you did that.
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 this is a hotplug rtc device (why I'm guessing its not available at
> late_init), would it not be better to leave the setting of time using
> hwclock --hctosys via a udev rule or something?
I want to set the time with millisecond precision (if the HID clock
offers that), which currently isn't available through the RTC subsystem.
But even if milliseconds would be available through /dev/rtcN, the
problem if something else did set the time still would be the same, just
that an udev-rule now would have that problem.
Regards,
Alexander Holler
On 05/21/2013 04:15 PM, Alexander Holler wrote:
> Am 22.05.2013 00:02, schrieb John Stultz:
>
>>
>> Like Andrew, I think this feels particularly hacky.
>>
>> Why exactly is late_init too early? (I'm unfamiliar with the
>> rtc-hid-sensor-time driver)
>
> Currently it can be an USB device (and maybe Bluetooth or even i2c in
> the future, depends on hid-sensor-hub). That has some implications:
>
> (1) Initialization might need longer (or happens later) than
> late_init, even if everything is linked into the kernel (same problem
> as with a boot from USB-storage)
> (2) It might not even be available at boot, but it should work if a
> user plugs it in afterwards.
> (3) To accomplish (2) it should set the system time (by default) IFF
> nothing else did set the time.
>
> That "nothing else" in (3) is for security reasons, because no
> plugable HID device should be able to change the system time by default.
>
> The check if something else did set the system time can't be
> accomplished only by the RTC subsystem because userspace, network or
> whatever else is able to set the system time most likely doesn't use
> the RTC subsystem (or hctosys).
>
> E.g. one of those setups could be:
>
> boot
> hctosys (fails because of no RTC)
> ntpdate/rdate/date < whatever
> load modules (rtc-hid-sensor-time)
>
> If we would use a flag in the hctosys module then rtc-hid-sensor-time
> would be able to change the time (in the setup above).
>
> Using a module option which is by default off doesn't help too. Users
> (or even distros) which would turn it on, might forget it and systems
> would be at risk if no HID clock will be found at boot (but later
> plugged in by some blackhat).
>
> A flag in the time subsystem itself would do the trick. Such a flag
> might help with the problem if the RTC subsystem or the persistent
> clock code did set the time too. You've mentioned in another thread
> that you had to solve such a problem, but I'm not aware how you did that.
>
> 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?
I'm not totally sure I'd agree that it would be better over leaving it
to userspace, but if we're going to go with an in-kernel policy for
this, then it seems like a better approach then the current patch.
>
> >
> > If this is a hotplug rtc device (why I'm guessing its not available at
> > late_init), would it not be better to leave the setting of time using
> > hwclock --hctosys via a udev rule or something?
>
> I want to set the time with millisecond precision (if the HID clock
> offers that), which currently isn't available through the RTC subsystem.
True. This is one area I'd like to see addressed at some point. A few
RTC devices have sub-second granularity and we're not exposing it via
the RTC subsystem to either kernel or userland consumers.
> But even if milliseconds would be available through /dev/rtcN, the
> problem if something else did set the time still would be the same,
> just that an udev-rule now would have that problem.
Though a udev hack to check if its 1970 would be fairly simple. And
pushes the policy of setting or not setting clearly off to userland
(which allows for less compile-time or boot option tweaks to manipulate).
thanks
-john
Am 28.05.2013 21:37, schrieb John Stultz:
> On 05/21/2013 04:15 PM, Alexander Holler wrote:
>> Am 22.05.2013 00:02, schrieb John Stultz:
>>
>>>
>>> Like Andrew, I think this feels particularly hacky.
>>>
>>> Why exactly is late_init too early? (I'm unfamiliar with the
>>> rtc-hid-sensor-time driver)
>>
>> Currently it can be an USB device (and maybe Bluetooth or even i2c in
>> the future, depends on hid-sensor-hub). That has some implications:
>>
>> (1) Initialization might need longer (or happens later) than
>> late_init, even if everything is linked into the kernel (same problem
>> as with a boot from USB-storage)
>> (2) It might not even be available at boot, but it should work if a
>> user plugs it in afterwards.
>> (3) To accomplish (2) it should set the system time (by default) IFF
>> nothing else did set the time.
>>
>> That "nothing else" in (3) is for security reasons, because no
>> plugable HID device should be able to change the system time by default.
>>
>> The check if something else did set the system time can't be
>> accomplished only by the RTC subsystem because userspace, network or
>> whatever else is able to set the system time most likely doesn't use
>> the RTC subsystem (or hctosys).
>>
>> E.g. one of those setups could be:
>>
>> boot
>> hctosys (fails because of no RTC)
>> ntpdate/rdate/date < whatever
>> load modules (rtc-hid-sensor-time)
>>
>> If we would use a flag in the hctosys module then rtc-hid-sensor-time
>> would be able to change the time (in the setup above).
>>
>> Using a module option which is by default off doesn't help too. Users
>> (or even distros) which would turn it on, might forget it and systems
>> would be at risk if no HID clock will be found at boot (but later
>> plugged in by some blackhat).
>>
>> A flag in the time subsystem itself would do the trick. Such a flag
>> might help with the problem if the RTC subsystem or the persistent
>> clock code did set the time too. You've mentioned in another thread
>> that you had to solve such a problem, but I'm not aware how you did that.
>>
>> 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. ;)
Of course, if the time subsystem would offer such a flag, it would be
easy to get rid of the hctosys module/driver. The RTC subsystem just
could check that flag and could set the system time whenever a (usually
the first) RTC driver would register (and offers a valid time).
In addition that would offer the possibility to add a kernel parameter
which describes the driver/module (by name) which should be used to set
the time. I never really liked that rtcN from hctosys, because it
doesn't really specify the RTC but depends on the order drivers get loaded.
And I still like the idea to have a timewait or clockwait kernel
parameter which prevents booting until the system has a valid time.
Booting without having a valid time, can have serious implications (see
e.g. https://lkml.org/lkml/2013/5/2/260 for which I never got feedback).
> I'm not totally sure I'd agree that it would be better over leaving it
> to userspace, but if we're going to go with an in-kernel policy for
> this, then it seems like a better approach then the current patch.
The only way I see to do such in userspace would be with that hacky
looking trick I'm using in this patch, because userspace can't reliable
manage a flag "time_set_at_least_once".
Regards,
Alexander
PS: I left out what happens on resume, because I just don't know how the
kernel gets the time after a resume (and never looked at it). That might
especially be tricky with a RTC which needs some time before it can
offer the time itself (e.g. gps or radio) or is only available after
some other subsystem like USB is available.
The work we schedule on register deletes himself. Therefor we cannot
use cancel_work_sync() because that calls flush_work() but still uses
the pointer to the (now deleted) work afterwards (for clear_work_data)
which ends up in a bug.
Replacing cancel_work_sync() with flush_work() fixes that.
Signed-off-by: Alexander Holler <[email protected]>
---
This patch applies on top of PATCH 3/4 or 4/4 of the small series of 4 patches
for rtc-hid-sensor-time, which was already merged into the -mm tree.
drivers/rtc/rtc-hid-sensor-time.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index 4aca12d..acb2bc2 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -414,9 +414,8 @@ static int hid_time_remove(struct platform_device *pdev)
struct hid_time_state *time_state = platform_get_drvdata(pdev);
if (time_state->workts) {
- cancel_work_sync(&time_state->workts->work);
- kfree(time_state->workts);
- time_state->workts = NULL;
+ flush_work(&time_state->workts->work);
+ BUG_ON(time_state->workts != NULL);
}
sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
--
1.8.1.5
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)) {
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.
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 <[email protected]>
---
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 98cd470..07f151f 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,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)) {
@@ -498,6 +501,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);
@@ -781,8 +787,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.1.5
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 <[email protected]>
---
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
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 <[email protected]>
---
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 32aa400..a1abfd8 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 b983813..b1d6691 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 c33f86f..b699504 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 841d4e9..8ec4a0b 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -26,6 +26,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);
@@ -33,12 +36,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) && defined(CONFIG_RTC_HCTOSYS_DEVICE)
+#if defined(CONFIG_PM)
/*
* On suspend(), measure the delta between one RTC and the
* system's wall clock; restore it on resume().
@@ -53,12 +51,18 @@ static int rtc_suspend(struct device *dev, pm_message_t mesg)
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);
@@ -94,11 +98,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 */
@@ -131,7 +137,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;
}
@@ -159,13 +165,15 @@ static void hctosys(struct rtc_device *rtc)
if (systime_was_set)
return;
err = do_settimeofday(&tv);
- if (!err && systime_was_set)
+ if (!err && systime_was_set) {
+ rtc_hctosys_dev_id = rtc->id;
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);
+ }
}
/**
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 <[email protected]>
- *
- * 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 b70e2bb..6b8a41b 100644
--- a/drivers/rtc/rtc-sysfs.c
+++ b/drivers/rtc/rtc-sysfs.c
@@ -112,13 +112,9 @@ static ssize_t
rtc_sysfs_show_hctosys(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");
}
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.1.5
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
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 <[email protected]>
---
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 98cd470..07f151f 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,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)) {
@@ -498,6 +501,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);
@@ -781,8 +787,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.1.5
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 <[email protected]>
---
Documentation/kernel-parameters.txt | 12 +++++++++
drivers/rtc/class.c | 39 +++++++++++++++++++++++++++++
include/linux/time.h | 6 +++++
kernel/time/timekeeping.c | 49 +++++++++++++++++++++++++++++--------
4 files changed, 96 insertions(+), 10 deletions(-)
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..a159d1f 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -141,6 +141,40 @@ 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 rc;
+
+ rc = rtc_read_time(rtc, &now);
+ if (rc) {
+ dev_err(rtc->dev.parent, "rtc core: error reading time from RTC: %d\n", rc);
+ return;
+ }
+ rc = rtc_valid_tm(&now);
+ if (rc) {
+ dev_err(rtc->dev.parent, "rtc core: date/time from RTC is invalid\n");
+ return;
+ }
+ rtc_tm_to_time(&now, &tv.tv_sec);
+ if (systime_was_set)
+ return;
+ rc = do_settimeofday(&tv);
+ if (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,
+ "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 +191,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 +231,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..fdbe3ab 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,16 +797,17 @@ void __init timekeeping_init(void)
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;
+ 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);
@@ -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
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 <[email protected]>
---
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 32aa400..a1abfd8 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 b983813..b1d6691 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 c33f86f..b699504 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 a159d1f..9ea5bdc 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -26,6 +26,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);
@@ -33,12 +36,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) && defined(CONFIG_RTC_HCTOSYS_DEVICE)
+#if defined(CONFIG_PM)
/*
* On suspend(), measure the delta between one RTC and the
* system's wall clock; restore it on resume().
@@ -53,12 +51,18 @@ static int rtc_suspend(struct device *dev, pm_message_t mesg)
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);
@@ -94,11 +98,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 */
@@ -131,7 +137,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;
}
@@ -166,13 +172,15 @@ static void hctosys(struct rtc_device *rtc)
if (rc) {
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,
"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);
+ }
}
/**
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 <[email protected]>
- *
- * 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 b70e2bb..6b8a41b 100644
--- a/drivers/rtc/rtc-sysfs.c
+++ b/drivers/rtc/rtc-sysfs.c
@@ -112,13 +112,9 @@ static ssize_t
rtc_sysfs_show_hctosys(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");
}
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.1.5
Hello,
sorry to bother again. I've now had the time to play with the new
hctosys I proposed patches for and rtc-hid-sensor-time.
If the new hctosys mechanism will end up in the kernel, the following
patches I've recently send and which already got merged to -mm won't
work anymore and should disappear:
[PATCH 3/4] rtc: rtc-hid-sensor-time: add option hctosys to set time at boot
[PATCH 4/4] rtc: rtc-hid-sensor-time: add support for milliseconds
[PATCH] rtc: rtc-hid-sensor-time: fix possible bug on driver_remove
The other two patches
[PATCH 1/4] rtc: rtc-hid-sensor-time: allow full years (16bit) in HID
reports
[PATCH 2/4] rtc: rtc-hid-sensor-time: allow 16 and 32 bit values for all
attributes.
which got merged too are still fine.
I don't know if I should send reverts for those 3 patches, or ...
I've already have a patch which would be needed if the new hctosys would
be used, but I will delay sending them until we figured out if and how
that new hctosys will become real. But you might already revert or
delete the above 3 patches in your tree.
That silly looking driver rtc-hid-sensor-time is really an endless story. :/
Besides writing the driver and an example firmware implementation I had
to make patches for iio, hid-sensor-hub and hid, and now for timekeeping
and rtc. All for nothing but the fun having to convince maintainers of
all sorts and bothering them. ;)
And the todo-list still contains some stuff:
- rtc-hid-sensor-time still does not get loaded automatically if a
matching hid-sensor-hub appears to the system. That needs changes to hid
and/or hid-sensor-hub. Problem is that I don't have a real hid sensor
hub and I don't know why the people which do produce such products and
have written hid-sensor-hub didn't have implemented a way to do such. Or
I've just missed something.
- Have to redo millisecond support.
- ntp functionality (e.g. module parameter adjtime) to make it possible
to use a hardware clock with support for milliseconds to turn the system
clock to a stratum 1 clock.
- and last but not least I hope the standard for those HID sensors will
become a standardized way to set the time. I still wouldn't like it if I
had to "invent" something myself (easy, but it would be linux-specific
and "invented" by some fool like me who just wrote it's own firmware
without having any knowledge about some real hid-sensor products ;) ),
but ...
Regards,
Alexander Holler
Am 06.06.2013 12:51, schrieb 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 <[email protected]>
While implementing support for high resolution (read-only) clocks (e.g.
radio or GPS) for the new hctosys, I've found another bug in my (quickly
done) RFC hctosys implementation.
Instead of posting the whole patch or the whole series again, here is
the small diff which should be added to patch2/3:
----------
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 49dd97c..e101468 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -795,7 +795,7 @@ void __init timekeeping_init(void)
struct timekeeper *tk = &timekeeper;
struct clocksource *clock;
unsigned long flags;
- struct timespec now, boot, tmp;
+ struct timespec now = {0, 0}, boot, tmp;
if (!hctosys_name[0] || !strcasecmp(hctosys_name, "persistent")) {
read_persistent_clock(&now);
----------
I now have 4 additional patches here:
rtc: rtc-hid-sensor-time: delay registering as rtc into a work
rtc: implement rtc_read_timeval()
rtc: hctosys: support rtc_read_timeval() for high precision clocks
rtc: rtc-hid-sensor-time: add support for rtc_read_timeval()
The first oatch is necessary because registering a rtc wants to read the
clock (for hctosys) and in case of the HID RTC this only works after the
HID device is actually registered. Therefor registering should not
happen in probe().
The other 3 new patches do add a read_timeval to rtc_ops and use it, if
available, instead of read_time to retrieve and set the system clock
with hctosys using a higher precision timestamp (us).
I requested, I will post the whole series or the just the
additional/fixed patches.
Regards,
Alexander Holler
Hello,
Because I've become a request which made me slightly more optimistic about
the RFC patches, I've spend some more time on the patches doing cosmetic.
I know think the "quickly done" is gone and they might be ready for
submission.
The whole series consists more or less of 3 mini-series.
The first two patches are already in -mm, the 3. should end there too.
Then there are 3 patches to change the hctosys mechanism and another
3 patches to implement read_timeval for rtc_ops, together with an example
implementation for read_timeval in rtc-hid-sensor-time.
The introductory message of my first post of this series still describes
all changes to hctosys, what the additional 3 patches with read_timeval
are doing should be obvious.
Regards,
Alexander Holler
BTW: rtc_device_unregister is broken in 3.10-rc5, most likely because
of the switch to devm*. But I will start another thread for that as this
is unrelated to this series of patches.
The draft for HID-sensors (HUTRR39) currently doesn't define the range
for the attribute year. Asking one of the authors revealed that full years
(e.g. 2013 instead of just 13) were meant.
So we now allow both, 8 bit and 16 bit values for the attribute year and
assuming full years when the value is 16 bits wide.
We will still support 8 bit values until the specification gets final
(and maybe defines a way to set the time too).
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 33 ++++++++++++++++++++++++++-------
1 file changed, 26 insertions(+), 7 deletions(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index 6302450..b8c1b10 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -85,10 +85,13 @@ static int hid_time_capture_sample(struct hid_sensor_hub_device *hsdev,
switch (usage_id) {
case HID_USAGE_SENSOR_TIME_YEAR:
- time_buf->tm_year = *(u8 *)raw_data;
- if (time_buf->tm_year < 70)
- /* assume we are in 1970...2069 */
- time_buf->tm_year += 100;
+ if (raw_len == 1) {
+ time_buf->tm_year = *(u8 *)raw_data;
+ if (time_buf->tm_year < 70)
+ /* assume we are in 1970...2069 */
+ time_buf->tm_year += 100;
+ } else
+ time_buf->tm_year = *(u16 *)raw_data-1900;
break;
case HID_USAGE_SENSOR_TIME_MONTH:
/* sensor sending the month as 1-12, we need 0-11 */
@@ -151,11 +154,27 @@ static int hid_time_parse_report(struct platform_device *pdev,
return -EINVAL;
}
if (time_state->info[i].size != 1) {
- dev_err(&pdev->dev,
- "attribute '%s' not 8 bits wide!\n",
+ /*
+ * The draft for HID-sensors (HUTRR39) currently
+ * doesn't define the range for the year attribute.
+ * Therefor we support both 8 bit (0-99) and 16 bit
+ * (full) as size for the year.
+ */
+ if (time_state->info[i].attrib_id !=
+ HID_USAGE_SENSOR_TIME_YEAR) {
+ dev_err(&pdev->dev,
+ "attribute '%s' not 8 bits wide!\n",
hid_time_attrib_name(
time_state->info[i].attrib_id));
- return -EINVAL;
+ return -EINVAL;
+ }
+ if (time_state->info[i].size != 2) {
+ dev_err(&pdev->dev,
+ "attribute '%s' not 8 or 16 bits wide!\n",
+ hid_time_attrib_name(
+ time_state->info[i].attrib_id));
+ return -EINVAL;
+ }
}
if (time_state->info[i].units !=
HID_USAGE_SENSOR_UNITS_NOT_SPECIFIED &&
--
1.8.1.4
There is no real reason to not support 16 or 32 bit values too.
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 59 +++++++++++++++++++++------------------
1 file changed, 32 insertions(+), 27 deletions(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index b8c1b10..7273b01 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -76,6 +76,20 @@ static int hid_time_proc_event(struct hid_sensor_hub_device *hsdev,
return 0;
}
+static u32 hid_time_value(size_t raw_len, char *raw_data)
+{
+ switch (raw_len) {
+ case 1:
+ return *(u8 *)raw_data;
+ case 2:
+ return *(u16 *)raw_data;
+ case 4:
+ return *(u32 *)raw_data;
+ default:
+ return (u32)(~0U); /* 0xff... or -1 to denote an error */
+ }
+}
+
static int hid_time_capture_sample(struct hid_sensor_hub_device *hsdev,
unsigned usage_id, size_t raw_len,
char *raw_data, void *priv)
@@ -85,29 +99,35 @@ static int hid_time_capture_sample(struct hid_sensor_hub_device *hsdev,
switch (usage_id) {
case HID_USAGE_SENSOR_TIME_YEAR:
+ /*
+ * The draft for HID-sensors (HUTRR39) currently doesn't define
+ * the range for the year attribute. Therefor we support
+ * 8 bit (0-99) and 16 or 32 bits (full) as size for the year.
+ */
if (raw_len == 1) {
time_buf->tm_year = *(u8 *)raw_data;
if (time_buf->tm_year < 70)
/* assume we are in 1970...2069 */
time_buf->tm_year += 100;
} else
- time_buf->tm_year = *(u16 *)raw_data-1900;
+ time_buf->tm_year =
+ (int)hid_time_value(raw_len, raw_data)-1900;
break;
case HID_USAGE_SENSOR_TIME_MONTH:
- /* sensor sending the month as 1-12, we need 0-11 */
- time_buf->tm_mon = *(u8 *)raw_data-1;
+ /* sensors are sending the month as 1-12, we need 0-11 */
+ time_buf->tm_mon = (int)hid_time_value(raw_len, raw_data)-1;
break;
case HID_USAGE_SENSOR_TIME_DAY:
- time_buf->tm_mday = *(u8 *)raw_data;
+ time_buf->tm_mday = (int)hid_time_value(raw_len, raw_data);
break;
case HID_USAGE_SENSOR_TIME_HOUR:
- time_buf->tm_hour = *(u8 *)raw_data;
+ time_buf->tm_hour = (int)hid_time_value(raw_len, raw_data);
break;
case HID_USAGE_SENSOR_TIME_MINUTE:
- time_buf->tm_min = *(u8 *)raw_data;
+ time_buf->tm_min = (int)hid_time_value(raw_len, raw_data);
break;
case HID_USAGE_SENSOR_TIME_SECOND:
- time_buf->tm_sec = *(u8 *)raw_data;
+ time_buf->tm_sec = (int)hid_time_value(raw_len, raw_data);
break;
default:
return -EINVAL;
@@ -153,28 +173,13 @@ static int hid_time_parse_report(struct platform_device *pdev,
"not all needed attributes inside the same report!\n");
return -EINVAL;
}
- if (time_state->info[i].size != 1) {
- /*
- * The draft for HID-sensors (HUTRR39) currently
- * doesn't define the range for the year attribute.
- * Therefor we support both 8 bit (0-99) and 16 bit
- * (full) as size for the year.
- */
- if (time_state->info[i].attrib_id !=
- HID_USAGE_SENSOR_TIME_YEAR) {
- dev_err(&pdev->dev,
- "attribute '%s' not 8 bits wide!\n",
- hid_time_attrib_name(
- time_state->info[i].attrib_id));
- return -EINVAL;
- }
- if (time_state->info[i].size != 2) {
- dev_err(&pdev->dev,
- "attribute '%s' not 8 or 16 bits wide!\n",
+ if (time_state->info[i].size == 3 ||
+ time_state->info[i].size > 4) {
+ dev_err(&pdev->dev,
+ "attribute '%s' not 8, 16 or 32 bits wide!\n",
hid_time_attrib_name(
time_state->info[i].attrib_id));
- return -EINVAL;
- }
+ return -EINVAL;
}
if (time_state->info[i].units !=
HID_USAGE_SENSOR_UNITS_NOT_SPECIFIED &&
--
1.8.1.4
rtc_device_register() might want to read the clock which doesn't work
before the hid device is registered. Therefor we delay the registration of
the rtc driver by moving it to a work.
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 66 ++++++++++++++++++++++++++++++++++-----
1 file changed, 58 insertions(+), 8 deletions(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index 7273b01..b842730 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -37,6 +37,11 @@ enum hid_time_channel {
TIME_RTC_CHANNEL_MAX,
};
+struct hid_time_workts {
+ struct work_struct work;
+ struct hid_time_state *time_state;
+};
+
struct hid_time_state {
struct hid_sensor_hub_callbacks callbacks;
struct hid_sensor_common common_attributes;
@@ -46,6 +51,7 @@ struct hid_time_state {
struct completion comp_last_time;
struct rtc_time time_buf;
struct rtc_device *rtc;
+ struct hid_time_workts *workts;
};
static const u32 hid_time_addresses[TIME_RTC_CHANNEL_MAX] = {
@@ -237,6 +243,36 @@ static const struct rtc_class_ops hid_time_rtc_ops = {
.read_time = hid_rtc_read_time,
};
+static void hid_time_register_rtc_work(struct work_struct *work)
+{
+ struct hid_time_state *time_state =
+ container_of(work, struct hid_time_workts, work)
+ ->time_state;
+ struct platform_device *pdev = time_state->callbacks.pdev;
+
+ time_state->rtc = devm_rtc_device_register(&pdev->dev,
+ "hid-sensor-time", &hid_time_rtc_ops,
+ THIS_MODULE);
+ if (IS_ERR_OR_NULL(time_state->rtc)) {
+ struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
+ sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
+ time_state->rtc = NULL;
+ dev_err(&pdev->dev, "rtc device register failed!\n");
+ /*
+ * I haven't a found a way to remove only this device from
+ * hid-sensor-hub. Removing the device a level above (the
+ * complete HID device) doesn't work, because a sensor-hub
+ * might provide more than just a time-sensor and thus we
+ * would remove all sensors not just this one.
+ * So we just leave this driver idling around until I or
+ * someone else has figured out how to remove this device
+ * from hid-sensor-hub.
+ */
+ }
+ time_state->workts = NULL;
+ kfree(work);
+}
+
static int hid_time_probe(struct platform_device *pdev)
{
int ret = 0;
@@ -279,22 +315,36 @@ static int hid_time_probe(struct platform_device *pdev)
return ret;
}
- time_state->rtc = devm_rtc_device_register(&pdev->dev,
- "hid-sensor-time", &hid_time_rtc_ops,
- THIS_MODULE);
-
- if (IS_ERR(time_state->rtc)) {
- dev_err(&pdev->dev, "rtc device register failed!\n");
- return PTR_ERR(time_state->rtc);
+ /*
+ * The HID device has to be registered to read the clock.
+ * Because rtc_device_register() might read the time, we have to delay
+ * rtc_device_register() to a work in order to finish the probe before.
+ */
+ time_state->workts = kmalloc(sizeof(struct hid_time_workts),
+ GFP_KERNEL);
+ if (time_state->workts == NULL) {
+ sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
+ return -ENOMEM;
}
+ time_state->workts->time_state = time_state;
+ INIT_WORK(&time_state->workts->work,
+ hid_time_register_rtc_work);
+ schedule_work(&time_state->workts->work);
- return ret;
+ return 0;
}
static int hid_time_remove(struct platform_device *pdev)
{
struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
+ struct hid_time_state *time_state = platform_get_drvdata(pdev);
+ if (time_state->workts) {
+ flush_work(&time_state->workts->work);
+ WARN_ON(time_state->workts != NULL);
+ }
+ if (time_state->rtc)
+ rtc_device_unregister(time_state->rtc);
sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
return 0;
--
1.8.1.4
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 <[email protected]>
---
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 baeeb5c..07d8531 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -37,6 +37,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)) {
@@ -498,6 +501,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);
@@ -781,8 +787,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.1.4
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 <[email protected]>
---
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 2fe6e76..0b8f101 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..37a1647 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -141,6 +141,43 @@ static int rtc_resume(struct device *dev)
#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
@@ -157,6 +194,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 +234,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 07d8531..e101468 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)) {
@@ -778,18 +795,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);
@@ -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.4
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 <[email protected]>
---
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 32aa400..a1abfd8 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 b983813..b1d6691 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 c33f86f..b699504 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 37a1647..625076b 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -26,6 +26,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);
@@ -33,12 +36,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) && defined(CONFIG_RTC_HCTOSYS_DEVICE)
+#if defined(CONFIG_PM)
/*
* On suspend(), measure the delta between one RTC and the
* system's wall clock; restore it on resume().
@@ -53,12 +51,18 @@ static int rtc_suspend(struct device *dev, pm_message_t mesg)
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);
@@ -94,11 +98,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 */
@@ -131,7 +137,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;
}
@@ -169,13 +175,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 <[email protected]>
- *
- * 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 b70e2bb..6b8a41b 100644
--- a/drivers/rtc/rtc-sysfs.c
+++ b/drivers/rtc/rtc-sysfs.c
@@ -112,13 +112,9 @@ static ssize_t
rtc_sysfs_show_hctosys(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");
}
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.1.4
Some RTCs offer a higher resolution than seconds. To support reading such
high resolution timestamps from inside the kernel implement
rtc_read_timeval() and add a read_timeval to the rtc-ops.
This is done to support high precision read-only clocks (like radio- or
GPS-clocks) from inside the kernel (mainly hctosys). Therfor there is
currently no need to set the clock or to extend the (rtc-)userspace api to
support high precision timestamps.
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/interface.c | 28 ++++++++++++++++++++++++++++
include/linux/rtc.h | 3 +++
2 files changed, 31 insertions(+)
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 42bd57d..f09f384 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -48,6 +48,34 @@ int rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm)
}
EXPORT_SYMBOL_GPL(rtc_read_time);
+static int __rtc_read_timeval(struct rtc_device *rtc, struct timeval *tv)
+{
+ int err;
+ if (!rtc->ops)
+ err = -ENODEV;
+ else if (!rtc->ops->read_timeval)
+ err = -EINVAL;
+ else {
+ memset(tv, 0, sizeof(struct timeval));
+ err = rtc->ops->read_timeval(rtc->dev.parent, tv);
+ }
+ return err;
+}
+
+int rtc_read_timeval(struct rtc_device *rtc, struct timeval *tv)
+{
+ int err;
+
+ err = mutex_lock_interruptible(&rtc->ops_lock);
+ if (err)
+ return err;
+
+ err = __rtc_read_timeval(rtc, tv);
+ mutex_unlock(&rtc->ops_lock);
+ return err;
+}
+EXPORT_SYMBOL_GPL(rtc_read_timeval);
+
int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
{
int err;
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 50caf0d..863f916 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -15,6 +15,7 @@
#include <linux/types.h>
#include <linux/interrupt.h>
#include <uapi/linux/rtc.h>
+#include <uapi/linux/time.h>
extern int rtc_month_days(unsigned int month, unsigned int year);
extern int rtc_year_days(unsigned int day, unsigned int month, unsigned int year);
@@ -63,6 +64,7 @@ struct rtc_class_ops {
int (*set_mmss)(struct device *, unsigned long secs);
int (*read_callback)(struct device *, int data);
int (*alarm_irq_enable)(struct device *, unsigned int enabled);
+ int (*read_timeval)(struct device *, struct timeval *);
};
#define RTC_DEVICE_NAME_SIZE 20
@@ -143,6 +145,7 @@ extern void devm_rtc_device_unregister(struct device *dev,
extern int rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm);
extern int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm);
+extern int rtc_read_timeval(struct rtc_device *rtc, struct timeval *tv);
extern int rtc_set_mmss(struct rtc_device *rtc, unsigned long secs);
extern int rtc_set_ntp_time(struct timespec now);
int __rtc_read_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm);
--
1.8.1.4
Some RTCs do provide a higher precision than seconds. Add support for them
by trying rtc_read_timeval() before using rtc_read_time() to get the time
in the hctosys mechanism.
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/class.c | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 625076b..1cdd506 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -150,16 +150,25 @@ static int rtc_resume(struct device *dev)
static void hctosys(struct rtc_device *rtc)
{
struct rtc_time now;
- struct timespec ts = {
- .tv_nsec = NSEC_PER_SEC >> 1,
- };
+ struct timespec ts;
+ struct timeval tv = { 0, 0 };
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_read_timeval(rtc, &tv);
+ if (rc || (!tv.tv_sec && !tv.tv_usec)) {
+ 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;
+ }
+ rtc_tm_to_time(&now, &ts.tv_sec);
+ ts.tv_nsec = NSEC_PER_SEC >> 1;
+ } else {
+ rtc_time_to_tm(tv.tv_sec, &now);
+ ts.tv_sec = tv.tv_sec;
+ ts.tv_nsec = tv.tv_usec*NSEC_PER_USEC;
}
rc = rtc_valid_tm(&now);
if (unlikely(rc)) {
@@ -167,7 +176,6 @@ static void hctosys(struct rtc_device *rtc)
"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);
@@ -176,12 +184,17 @@ static void hctosys(struct rtc_device *rtc)
"rtc core: error setting system clock: %d\n", rc);
return;
} else if (systime_was_set) {
+ char usbuf[8];
rtc_hctosys_dev_id = rtc->id;
+ if (tv.tv_sec)
+ snprintf(usbuf, sizeof(usbuf), ":%06ld", tv.tv_usec);
+ else
+ *usbuf = 0;
dev_info(rtc->dev.parent,
"rtc core: setting system clock to "
- "%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
+ "%d-%02d-%02d %02d:%02d:%02d%s UTC (%u)\n",
now.tm_year + 1900, now.tm_mon + 1, now.tm_mday,
- now.tm_hour, now.tm_min, now.tm_sec,
+ now.tm_hour, now.tm_min, now.tm_sec, usbuf,
(unsigned int) ts.tv_sec);
}
}
--
1.8.1.4
Some HID clocks do provide milliseconds. Make it possible to read a high
precision timestamp by supporting rtc_read_timeval().
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 74 +++++++++++++++++++++++++++++++--------
include/linux/hid-sensor-ids.h | 1 +
2 files changed, 61 insertions(+), 14 deletions(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index b842730..3e6d874 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -34,6 +34,7 @@ enum hid_time_channel {
CHANNEL_SCAN_INDEX_HOUR,
CHANNEL_SCAN_INDEX_MINUTE,
CHANNEL_SCAN_INDEX_SECOND,
+ CHANNEL_SCAN_INDEX_MILLISECOND, /* optional */
TIME_RTC_CHANNEL_MAX,
};
@@ -47,11 +48,14 @@ struct hid_time_state {
struct hid_sensor_common common_attributes;
struct hid_sensor_hub_attribute_info info[TIME_RTC_CHANNEL_MAX];
struct rtc_time last_time;
+ unsigned last_ms; /* == UINT_MAX to indicate ms aren't supported */
spinlock_t lock_last_time;
struct completion comp_last_time;
struct rtc_time time_buf;
+ unsigned ms_buf;
struct rtc_device *rtc;
struct hid_time_workts *workts;
+ struct rtc_class_ops rtc_ops;
};
static const u32 hid_time_addresses[TIME_RTC_CHANNEL_MAX] = {
@@ -61,11 +65,12 @@ static const u32 hid_time_addresses[TIME_RTC_CHANNEL_MAX] = {
HID_USAGE_SENSOR_TIME_HOUR,
HID_USAGE_SENSOR_TIME_MINUTE,
HID_USAGE_SENSOR_TIME_SECOND,
+ HID_USAGE_SENSOR_TIME_MILLISECOND, /* optional */
};
/* Channel names for verbose error messages */
static const char * const hid_time_channel_names[TIME_RTC_CHANNEL_MAX] = {
- "year", "month", "day", "hour", "minute", "second",
+ "year", "month", "day", "hour", "minute", "second", "millisecond",
};
/* Callback handler to send event after all samples are received and captured */
@@ -77,6 +82,8 @@ static int hid_time_proc_event(struct hid_sensor_hub_device *hsdev,
spin_lock_irqsave(&time_state->lock_last_time, flags);
time_state->last_time = time_state->time_buf;
+ if (time_state->last_ms != UINT_MAX)
+ time_state->last_ms = time_state->ms_buf;
spin_unlock_irqrestore(&time_state->lock_last_time, flags);
complete(&time_state->comp_last_time);
return 0;
@@ -135,6 +142,9 @@ static int hid_time_capture_sample(struct hid_sensor_hub_device *hsdev,
case HID_USAGE_SENSOR_TIME_SECOND:
time_buf->tm_sec = (int)hid_time_value(raw_len, raw_data);
break;
+ case HID_USAGE_SENSOR_TIME_MILLISECOND:
+ time_state->ms_buf = hid_time_value(raw_len, raw_data);
+ break;
default:
return -EINVAL;
}
@@ -161,12 +171,18 @@ static int hid_time_parse_report(struct platform_device *pdev,
{
int report_id, i;
- for (i = 0; i < TIME_RTC_CHANNEL_MAX; ++i)
+ /* Check if all required attributes are available */
+ for (i = 0; i < CHANNEL_SCAN_INDEX_MILLISECOND; ++i)
if (sensor_hub_input_get_attribute_info(hsdev,
HID_INPUT_REPORT, usage_id,
hid_time_addresses[i],
&time_state->info[i]) < 0)
return -EINVAL;
+ if (!sensor_hub_input_get_attribute_info(hsdev, HID_INPUT_REPORT,
+ usage_id, hid_time_addresses[i], &time_state->info[i])) {
+ dev_info(&pdev->dev, "milliseconds supported\n");
+ time_state->last_ms = 0;
+ }
/* Check the (needed) attributes for sanity */
report_id = time_state->info[0].report_id;
if (report_id < 0) {
@@ -174,6 +190,10 @@ static int hid_time_parse_report(struct platform_device *pdev,
return -EINVAL;
}
for (i = 0; i < TIME_RTC_CHANNEL_MAX; ++i) {
+ if (time_state->info[i].attrib_id ==
+ HID_USAGE_SENSOR_TIME_MILLISECOND &&
+ time_state->last_ms == UINT_MAX)
+ continue;
if (time_state->info[i].report_id != report_id) {
dev_err(&pdev->dev,
"not all needed attributes inside the same report!\n");
@@ -212,21 +232,25 @@ static int hid_time_parse_report(struct platform_device *pdev,
return 0;
}
-static int hid_rtc_read_time(struct device *dev, struct rtc_time *tm)
+static int hid_time_get_report(struct hid_time_state *time_state)
{
- unsigned long flags;
- struct hid_time_state *time_state =
- platform_get_drvdata(to_platform_device(dev));
- int ret;
-
INIT_COMPLETION(time_state->comp_last_time);
/* get a report with all values through requesting one value */
sensor_hub_input_attr_get_raw_value(time_state->common_attributes.hsdev,
HID_USAGE_SENSOR_TIME, hid_time_addresses[0],
time_state->info[0].report_id);
/* wait for all values (event) */
- ret = wait_for_completion_killable_timeout(
- &time_state->comp_last_time, HZ*6);
+ return wait_for_completion_killable_timeout(&time_state->comp_last_time,
+ HZ*6);
+}
+
+static int hid_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+ unsigned long flags;
+ struct hid_time_state *time_state =
+ platform_get_drvdata(to_platform_device(dev));
+ int ret = hid_time_get_report(time_state);
+
if (ret > 0) {
/* no error */
spin_lock_irqsave(&time_state->lock_last_time, flags);
@@ -239,9 +263,25 @@ static int hid_rtc_read_time(struct device *dev, struct rtc_time *tm)
return ret; /* killed (-ERESTARTSYS) */
}
-static const struct rtc_class_ops hid_time_rtc_ops = {
- .read_time = hid_rtc_read_time,
-};
+static int hid_rtc_read_timeval(struct device *dev, struct timeval *tv)
+{
+ unsigned long flags;
+ struct hid_time_state *time_state =
+ platform_get_drvdata(to_platform_device(dev));
+ int ret = hid_time_get_report(time_state);
+
+ if (ret > 0) {
+ /* no error */
+ spin_lock_irqsave(&time_state->lock_last_time, flags);
+ rtc_tm_to_time(&time_state->last_time, &tv->tv_sec);
+ tv->tv_usec = time_state->last_ms * USEC_PER_MSEC;
+ spin_unlock_irqrestore(&time_state->lock_last_time, flags);
+ return 0;
+ }
+ if (!ret)
+ return -EIO; /* timeouted */
+ return ret; /* killed (-ERESTARTSYS) */
+}
static void hid_time_register_rtc_work(struct work_struct *work)
{
@@ -251,7 +291,7 @@ static void hid_time_register_rtc_work(struct work_struct *work)
struct platform_device *pdev = time_state->callbacks.pdev;
time_state->rtc = devm_rtc_device_register(&pdev->dev,
- "hid-sensor-time", &hid_time_rtc_ops,
+ "hid-sensor-time", &time_state->rtc_ops,
THIS_MODULE);
if (IS_ERR_OR_NULL(time_state->rtc)) {
struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
@@ -283,6 +323,8 @@ static int hid_time_probe(struct platform_device *pdev)
if (time_state == NULL)
return -ENOMEM;
+ time_state->last_ms = UINT_MAX;
+
platform_set_drvdata(pdev, time_state);
spin_lock_init(&time_state->lock_last_time);
@@ -315,6 +357,10 @@ static int hid_time_probe(struct platform_device *pdev)
return ret;
}
+ time_state->rtc_ops.read_time = hid_rtc_read_time;
+ if (time_state->last_ms != UINT_MAX)
+ time_state->rtc_ops.read_timeval = hid_rtc_read_timeval;
+
/*
* The HID device has to be registered to read the clock.
* Because rtc_device_register() might read the time, we have to delay
diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
index 6f24446..b519c47 100644
--- a/include/linux/hid-sensor-ids.h
+++ b/include/linux/hid-sensor-ids.h
@@ -74,6 +74,7 @@
#define HID_USAGE_SENSOR_TIME_HOUR 0x200525
#define HID_USAGE_SENSOR_TIME_MINUTE 0x200526
#define HID_USAGE_SENSOR_TIME_SECOND 0x200527
+#define HID_USAGE_SENSOR_TIME_MILLISECOND 0x200528
/* Units */
#define HID_USAGE_SENSOR_UNITS_NOT_SPECIFIED 0x00
--
1.8.1.4
On 06/14/2013 09:52 AM, Alexander Holler wrote:
> Some RTCs offer a higher resolution than seconds. To support reading such
> high resolution timestamps from inside the kernel implement
> rtc_read_timeval() and add a read_timeval to the rtc-ops.
So I like the direction this patch is going. But if we're going to add a
new interface, lets not use an already out-dated structure (timeval).
Instead could you rework this to be timepsec based? Or ktime_t if its
really internal only?
thanks
-john
On 06/14/2013 09:52 AM, Alexander Holler wrote:
> Hello,
>
> Because I've become a request which made me slightly more optimistic about
> the RFC patches, I've spend some more time on the patches doing cosmetic.
> I know think the "quickly done" is gone and they might be ready for
> submission.
>
> The whole series consists more or less of 3 mini-series.
In the future, it might be easier if you split the series up into
separate patchsets. That way it makes the dependencies clear.
For instance, 1-3 I've got no objection to, and 7-9 are getting there,
but the middle three patches are probably going to need a good bit more
work.
thanks
-john
On 06/14/2013 09:52 AM, Alexander Holler wrote:
> Some RTCs offer a higher resolution than seconds. To support reading such
> high resolution timestamps from inside the kernel implement
> rtc_read_timeval() and add a read_timeval to the rtc-ops.
>
> This is done to support high precision read-only clocks (like radio- or
> GPS-clocks) from inside the kernel (mainly hctosys). Therfor there is
> currently no need to set the clock or to extend the (rtc-)userspace api to
> support high precision timestamps.
And is there really no reason to eventually pass this out to userland?
Also won't we need an equivalent resolution settime method?
thanks
-john
On 06/14/2013 09:52 AM, Alexander Holler wrote:
> 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 <[email protected]>
> ---
> 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;
> +
Probably should make this static to timekeeping.c and create an accessor
function so you don't have to export locking rules on this.
> 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 baeeb5c..07d8531 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -37,6 +37,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;
> +
Probably should also move this to be part of the timekeeper structure
(since it will be protected by the timekeeper lock.
> static inline void tk_normalize_xtime(struct timekeeper *tk)
> {
> while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
> @@ -498,6 +501,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);
Might also want to add the flag to inject_offset as well, since that
could be used to set the time.
thanks
-john
Am 14.06.2013 19:23, schrieb John Stultz:
> On 06/14/2013 09:52 AM, Alexander Holler wrote:
>> Some RTCs offer a higher resolution than seconds. To support reading such
>> high resolution timestamps from inside the kernel implement
>> rtc_read_timeval() and add a read_timeval to the rtc-ops.
>
> So I like the direction this patch is going. But if we're going to add a
> new interface, lets not use an already out-dated structure (timeval).
>
> Instead could you rework this to be timepsec based? Or ktime_t if its
> really internal only?
Sure, no problem. I wasn't aware timeval is out-dated and I've read
somewhere in the sources a comment that ktime_t has to disappear. I had
no clue if I should use timeval or timespec, and just have roled a dice
to decide between timeval and timespec.
That "internal only" is only because I don't want to write changes for
the userspace api to handle RTCs with greater precision than seconds. I
don't have a writable RTC supporting such a precision and I want to
leave this for people which are developing such devices and earning
money with them.
Besides that, I have almost no knowledge about the userspace api for
RTCs. If just looked a bit around in the in-kernel-ntp-sources for
adjtimex, but that's all.
Please don't forget, my only motivation is to use an USB-RTC I've build
myself to use with developer-boards without RTCs. All the other work
like the support for higher precisions, the new hctosys or similiar is
just because I let me trick to do that ;)
Regards,
Alexander Holler
Am 14.06.2013 19:41, schrieb John Stultz:
> On 06/14/2013 09:52 AM, Alexander Holler wrote:
>> 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 <[email protected]>
>> ---
>> 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;
>> +
>
> Probably should make this static to timekeeping.c and create an accessor
> function so you don't have to export locking rules on this.
>
>
>> 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 baeeb5c..07d8531 100644
>> --- a/kernel/time/timekeeping.c
>> +++ b/kernel/time/timekeeping.c
>> @@ -37,6 +37,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;
>> +
> Probably should also move this to be part of the timekeeper structure
> (since it will be protected by the timekeeper lock.
>
I wanted to avoid locks for this silly flag at all. It is only set once
at boot (and resume) and set to 0 at suspend. And I don't see any
possible race condition which could make a lock necessary. Therefor I've
decided to not use a lock or atomic_* in order to skip any delay in
setting the time.
Of course, I might be wrong and there might be a use case where multiple
things do set the system time concurrently and nothing else did set
system time before, but I found that extremly unlikely.
>> static inline void tk_normalize_xtime(struct timekeeper *tk)
>> {
>> while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
>> @@ -498,6 +501,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);
>
> Might also want to add the flag to inject_offset as well, since that
> could be used to set the time.
I wasn't sure about that because I had only a quick look at
inject_offset() and had the impression it's only able to inject a
relative small offset (so not usable at boot). And, as written sometimes
before, I haven't had a deep look at suspend/resume, which might be the
only place where it is really used to set the clock when
systime_was_set is false.
Regards,
Alexander
On 06/14/2013 11:05 AM, Alexander Holler wrote:
> Am 14.06.2013 19:41, schrieb John Stultz:
>> On 06/14/2013 09:52 AM, Alexander Holler wrote:
>>> 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 <[email protected]>
>>> ---
>>> 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;
>>> +
>>
>> Probably should make this static to timekeeping.c and create an accessor
>> function so you don't have to export locking rules on this.
>>
>>
>>> 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 baeeb5c..07d8531 100644
>>> --- a/kernel/time/timekeeping.c
>>> +++ b/kernel/time/timekeeping.c
>>> @@ -37,6 +37,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;
>>> +
>> Probably should also move this to be part of the timekeeper structure
>> (since it will be protected by the timekeeper lock.
>>
>
> I wanted to avoid locks for this silly flag at all. It is only set
> once at boot (and resume) and set to 0 at suspend. And I don't see any
> possible race condition which could make a lock necessary. Therefor
> I've decided to not use a lock or atomic_* in order to skip any delay
> in setting the time.
Even so, having random flag variables with special rules being exported
out is likely to cause eventual trouble (someone will mis-use or
overload some meaning on it).
So at least providing a accessor function for non-timekeeping.c uses
would be good.
> Of course, I might be wrong and there might be a use case where
> multiple things do set the system time concurrently and nothing else
> did set system time before, but I found that extremly unlikely.
Yea, the condition check and the action won't be both be done under a
lock, so its likely going to be racy anyway.
>>> static inline void tk_normalize_xtime(struct timekeeper *tk)
>>> {
>>> while (tk->xtime_nsec >= ((u64)NSEC_PER_SEC << tk->shift)) {
>>> @@ -498,6 +501,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);
>>
>> Might also want to add the flag to inject_offset as well, since that
>> could be used to set the time.
>
> I wasn't sure about that because I had only a quick look at
> inject_offset() and had the impression it's only able to inject a
> relative small offset (so not usable at boot). And, as written
> sometimes before, I haven't had a deep look at suspend/resume, which
> might be the only place where it is really used to set the clock when
> systime_was_set is false.
Not via suspend/resume, since those modify the boottime to account for
the sleep time that has past.
I'm thinking via adjtimex ADJ_SETOFFSET (which is relatively new, and
not widely used). See the do_adjtimex() path in timekeeping.c
thanks
-john
On 06/14/2013 09:52 AM, Alexander Holler wrote:
> 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.
This one concerns me a bit. Since you're removing quite a bit and it
looks like it may break userland expectations.
I ran into this myself recently, when I found some distros look for
/sys/class/rtc/rtcN/hctosys in order to determine which rtc device
should be synced with from userland.
So I'd probably suggest instead to re-factor this so you leave all the
hctosys bits alone, but just change it from being called by a
late_initcall() and instead have it called when we register the RTC that
matches CONFIG_RTC_HCTOSYS_DEVICE.
I suspect it will end up being a much smaller change that way.
Then the last bit is just a matter of adding the
timekeeping_systimeset() check to the hctosys bits.
thanks
-john
On 06/14/2013 10:43 AM, Alexander Holler wrote:
> Am 14.06.2013 19:23, schrieb John Stultz:
>> On 06/14/2013 09:52 AM, Alexander Holler wrote:
>>> Some RTCs offer a higher resolution than seconds. To support reading
>>> such
>>> high resolution timestamps from inside the kernel implement
>>> rtc_read_timeval() and add a read_timeval to the rtc-ops.
>>
>> So I like the direction this patch is going. But if we're going to add a
>> new interface, lets not use an already out-dated structure (timeval).
>>
>> Instead could you rework this to be timepsec based? Or ktime_t if its
>> really internal only?
>
> Sure, no problem. I wasn't aware timeval is out-dated and I've read
> somewhere in the sources a comment that ktime_t has to disappear. I
> had no clue if I should use timeval or timespec, and just have roled a
> dice to decide between timeval and timespec.
>
> That "internal only" is only because I don't want to write changes for
> the userspace api to handle RTCs with greater precision than seconds.
> I don't have a writable RTC supporting such a precision and I want to
> leave this for people which are developing such devices and earning
> money with them.
>
Ok. Pushing it out to userland and the settime interfaces are probably
logically separate enough.
That said, the the pain with this change is it leaves us with duplicate
internal interfaces to read the time. So at some point we'll have to go
through all the clocksource drivers and convert them to provide the
timespec granular method, even if they just return the second portion.
Otherwise, you end up with a mishmash of some users using the second
granular and some users using the highres and not all RTCs working for
all users (or the users have to do their own fallback logic).
That said, I think such a transition is needed eventually, and this gets
us at least started in that direction.
Alessandro: You have any thoughts here on how to best approach this sort
of eventually far-reaching change?
thanks
-john
On 06/14/2013 09:52 AM, Alexander Holler wrote:
> Some RTCs do provide a higher precision than seconds. Add support for them
> by trying rtc_read_timeval() before using rtc_read_time() to get the time
> in the hctosys mechanism.
>
>
[snip]
> + rc = rtc_read_timeval(rtc, &tv);
> + if (rc || (!tv.tv_sec && !tv.tv_usec)) {
> + 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;
> + }
> + rtc_tm_to_time(&now, &ts.tv_sec);
> + ts.tv_nsec = NSEC_PER_SEC >> 1;
> + } else {
> + rtc_time_to_tm(tv.tv_sec, &now);
> + ts.tv_sec = tv.tv_sec;
> + ts.tv_nsec = tv.tv_usec*NSEC_PER_USEC;
Yea, this sort of fallback logic should be centralized in the RTC layer
rather then in the individual users.
Might be easiest to modify the rtc_read_timeval() interface to try the
ops->read_timeval() operation and do the fallback to rtc_read_time()
internally.
thanks
-john
On 06/14/2013 09:52 AM, Alexander Holler wrote:
> 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.
So what was the rational for this to be a boot time argument instead of
using the existing compile time CONFIG_RTC_HCTOSYS_DEVICE?
If we need a boot argument, that's still doable, but it probably should
fall back to CONFIG_RTC_HCTOSYS_DEVICE if not specified.
thanks
-john
Am 14.06.2013 20:28, schrieb John Stultz:
> On 06/14/2013 11:05 AM, Alexander Holler wrote:
>> Am 14.06.2013 19:41, schrieb John Stultz:
>>> On 06/14/2013 09:52 AM, Alexander Holler wrote:
>>>> 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 <[email protected]>
>>>> ---
>>>> 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;
>>>> +
>>>
>>> Probably should make this static to timekeeping.c and create an accessor
>>> function so you don't have to export locking rules on this.
>>>
>>>
>>>> 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 baeeb5c..07d8531 100644
>>>> --- a/kernel/time/timekeeping.c
>>>> +++ b/kernel/time/timekeeping.c
>>>> @@ -37,6 +37,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;
>>>> +
>>> Probably should also move this to be part of the timekeeper structure
>>> (since it will be protected by the timekeeper lock.
>>>
>>
>> I wanted to avoid locks for this silly flag at all. It is only set
>> once at boot (and resume) and set to 0 at suspend. And I don't see any
>> possible race condition which could make a lock necessary. Therefor
>> I've decided to not use a lock or atomic_* in order to skip any delay
>> in setting the time.
>
> Even so, having random flag variables with special rules being exported
> out is likely to cause eventual trouble (someone will mis-use or
> overload some meaning on it).
>
> So at least providing a accessor function for non-timekeeping.c uses
> would be good.
It's rather hard to misuse a bool (even if a bool in C is just a define).
What do you think I should write?
void set_systime_was_set(void) and void clear_systime_was_set(void)?
And both functions would have to be exported in order to be usable from
modules?
Or do you think I should write something like that:
extern bool foo;
inline void set_foo(void) { foo = true};
inline void clear_foo(void) { foo = false };
That's just silly, sorry to call it such.
So now I'm unsure if I'm able to continue work on this series. I seem to
be unable to think and code like maintainers do want I should think and
code, whatever that might be.
>> Of course, I might be wrong and there might be a use case where
>> multiple things do set the system time concurrently and nothing else
>> did set system time before, but I found that extremly unlikely.
>
> Yea, the condition check and the action won't be both be done under a
> lock, so its likely going to be racy anyway.
And if there ever will be a race for the first timesource to set this
flag (the first time), and something does care about the outtake, the
system would be completly broken.
In order to keep it simple, I just tread userspace like a RTC of type X
and will call them all timesources of type x where a the type is defined
by the driver.
Let us go through the possible cases:
- 2 or more timesources of different type:
If the order is undefined and they have to race for which clock might be
used for hctosys (and thus for adjusting the time after resume too), the
only reason one would want such is for HA purposes. And in case of HA,
both clocks must have the same time, so nobody does care about which one
will win the race (=> no race, no lock or atomic_* needed).
If the purpose isn't for HA and someone does care about which timesource
should be used, the way to do this is to use hctosys=type (or
hctosys=none in case of userspace) to define which timesource should be
used for hctosys (=> no race, no lock or atomic_* needed).
- 2 or more timesources of the same type:
There is no possibility to define which one should win the race. Such a
system configuration is only usable for HA purposes, so if such exists,
nobody cares about the outtake of the race (=> no race, no lock or
atomic_* needed).
Regards,
Alexander Holler
On 06/14/2013 11:01 PM, Alexander Holler wrote:
> Am 14.06.2013 20:28, schrieb John Stultz:
>> On 06/14/2013 11:05 AM, Alexander Holler wrote:
>>> Am 14.06.2013 19:41, schrieb John Stultz:
>>>> On 06/14/2013 09:52 AM, Alexander Holler wrote:
>>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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;
>>>>> +
>>>>
>>>> Probably should make this static to timekeeping.c and create an
>>>> accessor
>>>> function so you don't have to export locking rules on this.
>>>>
>>>>
>>>>> 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 baeeb5c..07d8531 100644
>>>>> --- a/kernel/time/timekeeping.c
>>>>> +++ b/kernel/time/timekeeping.c
>>>>> @@ -37,6 +37,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;
>>>>> +
>>>> Probably should also move this to be part of the timekeeper structure
>>>> (since it will be protected by the timekeeper lock.
>>>>
>>>
>>> I wanted to avoid locks for this silly flag at all. It is only set
>>> once at boot (and resume) and set to 0 at suspend. And I don't see any
>>> possible race condition which could make a lock necessary. Therefor
>>> I've decided to not use a lock or atomic_* in order to skip any delay
>>> in setting the time.
>>
>> Even so, having random flag variables with special rules being exported
>> out is likely to cause eventual trouble (someone will mis-use or
>> overload some meaning on it).
>>
>> So at least providing a accessor function for non-timekeeping.c uses
>> would be good.
>
>
> It's rather hard to misuse a bool (even if a bool in C is just a define).
I'm trying to avoid allowing non-timekeeping users of the value to be
able to set it.
By putting the value behind a timekeeping_systime_was_set() accessor,
and making the boolean value static, we can make sure its properly
managed by the timekeeping code alone.
> What do you think I should write?
>
> void set_systime_was_set(void) and void clear_systime_was_set(void)?
>
> And both functions would have to be exported in order to be usable
> from modules?
>
> Or do you think I should write something like that:
>
> extern bool foo;
> inline void set_foo(void) { foo = true};
> inline void clear_foo(void) { foo = false };
>
> That's just silly, sorry to call it such.
No no. I'm only asking that the boolean be static to timekeeping.c and
an accessor function be used to read it. Since the timekeeping core
should be managing this value, there should be no reason for any other
users to be setting or clearing the value.
>>> Of course, I might be wrong and there might be a use case where
>>> multiple things do set the system time concurrently and nothing else
>>> did set system time before, but I found that extremly unlikely.
>>
>> Yea, the condition check and the action won't be both be done under a
>> lock, so its likely going to be racy anyway.
>
> And if there ever will be a race for the first timesource to set this
> flag (the first time), and something does care about the outtake, the
> system would be completly broken.
>
> In order to keep it simple, I just tread userspace like a RTC of type
> X and will call them all timesources of type x where a the type is
> defined by the driver.
>
> Let us go through the possible cases:
>
> - 2 or more timesources of different type:
>
> If the order is undefined and they have to race for which clock might
> be used for hctosys (and thus for adjusting the time after resume
> too), the only reason one would want such is for HA purposes. And in
> case of HA, both clocks must have the same time, so nobody does care
> about which one will win the race (=> no race, no lock or atomic_*
> needed).
>
> If the purpose isn't for HA and someone does care about which
> timesource should be used, the way to do this is to use hctosys=type
> (or hctosys=none in case of userspace) to define which timesource
> should be used for hctosys (=> no race, no lock or atomic_* needed).
>
> - 2 or more timesources of the same type:
> There is no possibility to define which one should win the race. Such
> a system configuration is only usable for HA purposes, so if such
> exists, nobody cares about the outtake of the race (=> no race, no
> lock or atomic_* needed).
>
The race I'm thinking of is you have a system that normally sets the
time via ntpdate at bootup. Thus they expect the system to always be
started w/ NTP time (even if the system time was initially set via
hctosys).
Then because of of some delay in the driver (or because the RTC device
was plugged in during boot), the hctosys functionality runs just as
ntpdate is being called. hctosys sees time has not yet been set and
reads the RTC hardware time. At this point, ntpdate sets the time to NTP
time. Then hctosys completes, setting the time to the RTC time. This
results in the system clock being wrong from the user's perspective (as
they expect it to be set to NTP time).
This is basically what this code is trying to avoid in the first place.
And I'll grant that its a small race window, but it may lead to
irregular behavior.
So either we need to document that this race is theoretically possible,
and explain *why* its safe to ignore it. Or if we really want to do
something like this properly, we need to drop the accessor function to
the boolean, and instead provide a special
timekeeping_set_time_if_unset() (with hopefully a better name then what
I just came up with ;) function which checks the boolean and sets the
clock all while holding the same lock. That way we really can be sure
that if userland sets the time, we don't accidentally over-write that time.
thanks
-john
Am 17.06.2013 20:10, schrieb John Stultz:
> On 06/14/2013 11:01 PM, Alexander Holler wrote:
>> Am 14.06.2013 20:28, schrieb John Stultz:
>>> On 06/14/2013 11:05 AM, Alexander Holler wrote:
>>>> Am 14.06.2013 19:41, schrieb John Stultz:
>>>>> On 06/14/2013 09:52 AM, Alexander Holler wrote:
>>>>>> 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 <[email protected]>
>>>>>> ---
>>>>>> 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;
>>>>>> +
>>>>>
>>>>> Probably should make this static to timekeeping.c and create an
>>>>> accessor
>>>>> function so you don't have to export locking rules on this.
>>>>>
>>>>>
>>>>>> 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 baeeb5c..07d8531 100644
>>>>>> --- a/kernel/time/timekeeping.c
>>>>>> +++ b/kernel/time/timekeeping.c
>>>>>> @@ -37,6 +37,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;
>>>>>> +
>>>>> Probably should also move this to be part of the timekeeper structure
>>>>> (since it will be protected by the timekeeper lock.
>>>>>
>>>>
>>>> I wanted to avoid locks for this silly flag at all. It is only set
>>>> once at boot (and resume) and set to 0 at suspend. And I don't see any
>>>> possible race condition which could make a lock necessary. Therefor
>>>> I've decided to not use a lock or atomic_* in order to skip any delay
>>>> in setting the time.
>>>
>>> Even so, having random flag variables with special rules being exported
>>> out is likely to cause eventual trouble (someone will mis-use or
>>> overload some meaning on it).
>>>
>>> So at least providing a accessor function for non-timekeeping.c uses
>>> would be good.
>>
>>
>> It's rather hard to misuse a bool (even if a bool in C is just a define).
>
> I'm trying to avoid allowing non-timekeeping users of the value to be
> able to set it.
> By putting the value behind a timekeeping_systime_was_set() accessor,
> and making the boolean value static, we can make sure its properly
> managed by the timekeeping code alone.
>
>> What do you think I should write?
>>
>> void set_systime_was_set(void) and void clear_systime_was_set(void)?
>>
>> And both functions would have to be exported in order to be usable
>> from modules?
>>
>> Or do you think I should write something like that:
>>
>> extern bool foo;
>> inline void set_foo(void) { foo = true};
>> inline void clear_foo(void) { foo = false };
>>
>> That's just silly, sorry to call it such.
>
> No no. I'm only asking that the boolean be static to timekeeping.c and
> an accessor function be used to read it. Since the timekeeping core
> should be managing this value, there should be no reason for any other
> users to be setting or clearing the value.
First you can't make the value static (semantically) because it has to
be set and cleared from different parts of the kernel. And adding
accessor functions doesn't help in any way, everyone will still be able
to set or clear the value (this still is C and no C++ with classes or
other encapsulation features). The only thing what will happen with such
an accessor function is that a small overhead through the then necessary
function call is introduced.
>
>
>>>> Of course, I might be wrong and there might be a use case where
>>>> multiple things do set the system time concurrently and nothing else
>>>> did set system time before, but I found that extremly unlikely.
>>>
>>> Yea, the condition check and the action won't be both be done under a
>>> lock, so its likely going to be racy anyway.
>>
>> And if there ever will be a race for the first timesource to set this
>> flag (the first time), and something does care about the outtake, the
>> system would be completly broken.
>>
>> In order to keep it simple, I just tread userspace like a RTC of type
>> X and will call them all timesources of type x where a the type is
>> defined by the driver.
>>
>> Let us go through the possible cases:
>>
>> - 2 or more timesources of different type:
>>
>> If the order is undefined and they have to race for which clock might
>> be used for hctosys (and thus for adjusting the time after resume
>> too), the only reason one would want such is for HA purposes. And in
>> case of HA, both clocks must have the same time, so nobody does care
>> about which one will win the race (=> no race, no lock or atomic_*
>> needed).
>>
>> If the purpose isn't for HA and someone does care about which
>> timesource should be used, the way to do this is to use hctosys=type
>> (or hctosys=none in case of userspace) to define which timesource
>> should be used for hctosys (=> no race, no lock or atomic_* needed).
>>
>> - 2 or more timesources of the same type:
>> There is no possibility to define which one should win the race. Such
>> a system configuration is only usable for HA purposes, so if such
>> exists, nobody cares about the outtake of the race (=> no race, no
>> lock or atomic_* needed).
>>
>
> The race I'm thinking of is you have a system that normally sets the
> time via ntpdate at bootup. Thus they expect the system to always be
> started w/ NTP time (even if the system time was initially set via
> hctosys).
>
> Then because of of some delay in the driver (or because the RTC device
> was plugged in during boot), the hctosys functionality runs just as
> ntpdate is being called. hctosys sees time has not yet been set and
> reads the RTC hardware time. At this point, ntpdate sets the time to NTP
> time. Then hctosys completes, setting the time to the RTC time. This
> results in the system clock being wrong from the user's perspective (as
> they expect it to be set to NTP time).
Therefor there now will be hctosys as a kernel command line parameter.
Instead of a kernel config option which can't be changed by 99% of all
Linux users, that option allows ordinary (non kernel compiling) users to
disable hctosys at all. Something which wasn't possible before without
recompiling the kernel. And, like before, most RTC drivers will be
loaded before userspace calls ntp/ntpdate. If not, the system is already
broken.
And just in case, I've made that possible window for the above race very
small by checking the flag systime_was_set twice, once before starting
to read the time and a second time right before the time is set. Ok, the
race is still there, but as said before, if that problem does exist at
all, the system would be setup wrong at all.
>
> This is basically what this code is trying to avoid in the first place.
> And I'll grant that its a small race window, but it may lead to
> irregular behavior.
>
> So either we need to document that this race is theoretically possible,
> and explain *why* its safe to ignore it. Or if we really want to do
I would think that documenting hctosys=none should be enough.
All systems I've seen do load modules very early (before they would
start anything ntp related). And the new hctosys is done inside the
registration of the RTC. So if a system is configured in such a way that
the race really might happen, the system would be broken because the RTC
would not be there when starting ntp. To conclude, I think the problem
is highly academic/artificial and, if possible at all, only possible on
already misconfigured systems during a very, very small window. Therfor
I still believe that no locking mechanism is needed.
Anyway, I don't care, if you want, I will make an accessor-function with
locks and an return code for "set" to indicate if it was already set.
We could also request and wait for a third opinion on that topic. As it
most likely will not end up in any kernel before the end of this year
(if at all), there is enough time to do so.
Regards,
Alexander Holler
rtc_device_register() might want to read the clock which doesn't work
before the hid device is registered. Therefor we delay the registration of
the rtc driver by moving it to a work.
Signed-off-by: Alexander Holler <[email protected]>
---
Changes to v1:
I've fixed a bug calling rtc_device_unregister() in remove() which was
necessary in 3.9 but leads to a crash in 3.10 (through the use of devm_*).
Sorry for that, I assume I've resolved a conflict wrong while moving this
patch from 3.9.x to 3.10.
This patch can already be added to -mm. It is only necessary if
rtc_device_register() reads the time (which would fail without this patch).
I've left the "3/9" in the subject only there, because the patch was named
as such before. But the patch is independ if hctosys will be changed or not.
1/9 and 2/9 are already in -mm, 4-9 are in discussion which will need some
time.
Thanks,
Alexander Holler
drivers/rtc/rtc-hid-sensor-time.c | 64 ++++++++++++++++++++++++++++++++++-----
1 file changed, 56 insertions(+), 8 deletions(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index 7273b01..0767395 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -37,6 +37,11 @@ enum hid_time_channel {
TIME_RTC_CHANNEL_MAX,
};
+struct hid_time_workts {
+ struct work_struct work;
+ struct hid_time_state *time_state;
+};
+
struct hid_time_state {
struct hid_sensor_hub_callbacks callbacks;
struct hid_sensor_common common_attributes;
@@ -46,6 +51,7 @@ struct hid_time_state {
struct completion comp_last_time;
struct rtc_time time_buf;
struct rtc_device *rtc;
+ struct hid_time_workts *workts;
};
static const u32 hid_time_addresses[TIME_RTC_CHANNEL_MAX] = {
@@ -237,6 +243,36 @@ static const struct rtc_class_ops hid_time_rtc_ops = {
.read_time = hid_rtc_read_time,
};
+static void hid_time_register_rtc_work(struct work_struct *work)
+{
+ struct hid_time_state *time_state =
+ container_of(work, struct hid_time_workts, work)
+ ->time_state;
+ struct platform_device *pdev = time_state->callbacks.pdev;
+
+ time_state->rtc = devm_rtc_device_register(&pdev->dev,
+ "hid-sensor-time", &hid_time_rtc_ops,
+ THIS_MODULE);
+ if (IS_ERR_OR_NULL(time_state->rtc)) {
+ struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
+ sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
+ time_state->rtc = NULL;
+ dev_err(&pdev->dev, "rtc device register failed!\n");
+ /*
+ * I haven't a found a way to remove only this device from
+ * hid-sensor-hub. Removing the device a level above (the
+ * complete HID device) doesn't work, because a sensor-hub
+ * might provide more than just a time-sensor and thus we
+ * would remove all sensors not just this one.
+ * So we just leave this driver idling around until I or
+ * someone else has figured out how to remove this device
+ * from hid-sensor-hub.
+ */
+ }
+ time_state->workts = NULL;
+ kfree(work);
+}
+
static int hid_time_probe(struct platform_device *pdev)
{
int ret = 0;
@@ -279,22 +315,34 @@ static int hid_time_probe(struct platform_device *pdev)
return ret;
}
- time_state->rtc = devm_rtc_device_register(&pdev->dev,
- "hid-sensor-time", &hid_time_rtc_ops,
- THIS_MODULE);
-
- if (IS_ERR(time_state->rtc)) {
- dev_err(&pdev->dev, "rtc device register failed!\n");
- return PTR_ERR(time_state->rtc);
+ /*
+ * The HID device has to be registered to read the clock.
+ * Because rtc_device_register() might read the time, we have to delay
+ * rtc_device_register() to a work in order to finish the probe before.
+ */
+ time_state->workts = kmalloc(sizeof(struct hid_time_workts),
+ GFP_KERNEL);
+ if (time_state->workts == NULL) {
+ sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
+ return -ENOMEM;
}
+ time_state->workts->time_state = time_state;
+ INIT_WORK(&time_state->workts->work,
+ hid_time_register_rtc_work);
+ schedule_work(&time_state->workts->work);
- return ret;
+ return 0;
}
static int hid_time_remove(struct platform_device *pdev)
{
struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
+ struct hid_time_state *time_state = platform_get_drvdata(pdev);
+ if (time_state->workts) {
+ flush_work(&time_state->workts->work);
+ WARN_ON(time_state->workts != NULL);
+ }
sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
return 0;
--
1.8.1.4
On 06/20/2013 03:15 AM, Alexander Holler wrote:
> Am 17.06.2013 20:10, schrieb John Stultz:
>> On 06/14/2013 11:01 PM, Alexander Holler wrote:
>>
>>> What do you think I should write?
>>>
>>> void set_systime_was_set(void) and void clear_systime_was_set(void)?
>>>
>>> And both functions would have to be exported in order to be usable
>>> from modules?
>>>
>>> Or do you think I should write something like that:
>>>
>>> extern bool foo;
>>> inline void set_foo(void) { foo = true};
>>> inline void clear_foo(void) { foo = false };
>>>
>>> That's just silly, sorry to call it such.
>>
>> No no. I'm only asking that the boolean be static to timekeeping.c and
>> an accessor function be used to read it. Since the timekeeping core
>> should be managing this value, there should be no reason for any other
>> users to be setting or clearing the value.
>
> First you can't make the value static (semantically) because it has to
> be set and cleared from different parts of the kernel. And adding
> accessor functions doesn't help in any way, everyone will still be
> able to set or clear the value (this still is C and no C++ with
> classes or other encapsulation features). The only thing what will
> happen with such an accessor function is that a small overhead through
> the then necessary function call is introduced.
Why would it be set and cleared from different parts of the kernel?
We're checking if the system time was set. The system time can be set
only from a limited number functions in timekeeping.c. It seems
reasonable it should be static to timekeeping.c
Even so, this is all a tangent. I really think the flag value is racy
and should be dropped for a timekeeping_setime_if_not_set() - or better
named - function that can act atomically.
>>
>>>>> Of course, I might be wrong and there might be a use case where
>>>>> multiple things do set the system time concurrently and nothing else
>>>>> did set system time before, but I found that extremly unlikely.
>>>>
>>>> Yea, the condition check and the action won't be both be done under a
>>>> lock, so its likely going to be racy anyway.
>>>
>>> And if there ever will be a race for the first timesource to set this
>>> flag (the first time), and something does care about the outtake, the
>>> system would be completly broken.
>>>
>>> In order to keep it simple, I just tread userspace like a RTC of type
>>> X and will call them all timesources of type x where a the type is
>>> defined by the driver.
>>>
>>> Let us go through the possible cases:
>>>
>>> - 2 or more timesources of different type:
>>>
>>> If the order is undefined and they have to race for which clock might
>>> be used for hctosys (and thus for adjusting the time after resume
>>> too), the only reason one would want such is for HA purposes. And in
>>> case of HA, both clocks must have the same time, so nobody does care
>>> about which one will win the race (=> no race, no lock or atomic_*
>>> needed).
>>>
>>> If the purpose isn't for HA and someone does care about which
>>> timesource should be used, the way to do this is to use hctosys=type
>>> (or hctosys=none in case of userspace) to define which timesource
>>> should be used for hctosys (=> no race, no lock or atomic_* needed).
>>>
>>> - 2 or more timesources of the same type:
>>> There is no possibility to define which one should win the race. Such
>>> a system configuration is only usable for HA purposes, so if such
>>> exists, nobody cares about the outtake of the race (=> no race, no
>>> lock or atomic_* needed).
>>>
>>
>> The race I'm thinking of is you have a system that normally sets the
>> time via ntpdate at bootup. Thus they expect the system to always be
>> started w/ NTP time (even if the system time was initially set via
>> hctosys).
>>
>> Then because of of some delay in the driver (or because the RTC device
>> was plugged in during boot), the hctosys functionality runs just as
>> ntpdate is being called. hctosys sees time has not yet been set and
>> reads the RTC hardware time. At this point, ntpdate sets the time to NTP
>> time. Then hctosys completes, setting the time to the RTC time. This
>> results in the system clock being wrong from the user's perspective (as
>> they expect it to be set to NTP time).
>
> Therefor there now will be hctosys as a kernel command line parameter.
> Instead of a kernel config option which can't be changed by 99% of all
> Linux users, that option allows ordinary (non kernel compiling) users
> to disable hctosys at all.
I agree your suggestion of having a hctosys= boot option (to override
the CONFIG_HCTOSYS_DEVICE value) could be a useful extension.
But we shouldn't expect users to set magic boot flags in order to have a
reliably functioning system. If userland sets the time during init, and
the hctosys functionality isn't supposed to overwrite that value, then
there should be no case where userland sets the time at boot, but we end
up with the RTC time after boot. But currently that race is possible
(though small).
A more concrete case:
On many distros ntpd isn't installed by default. Instead they leave the
kernel to initialize the time from the RTC.
But ntpd can be installed afterwards, and it would be silly to require
users edit their boot arguments when installing the ntp package.
> Something which wasn't possible before without recompiling the kernel.
> And, like before, most RTC drivers will be loaded before userspace
> calls ntp/ntpdate. If not, the system is already broken.
I'm not sure I'm following how the system is already broken?
> And just in case, I've made that possible window for the above race
> very small by checking the flag systime_was_set twice, once before
> starting to read the time and a second time right before the time is
> set. Ok, the race is still there, but as said before, if that problem
> does exist at all, the system would be setup wrong at all.
It just seems that if we really want a to do this, we might as well do
it right, using the timekeeping_settime_first() or whatever function
that can properly check the value and complete the action atomically
while holding the lock.
>>
>> This is basically what this code is trying to avoid in the first place.
>> And I'll grant that its a small race window, but it may lead to
>> irregular behavior.
>>
>> So either we need to document that this race is theoretically possible,
>> and explain *why* its safe to ignore it. Or if we really want to do
>
> I would think that documenting hctosys=none should be enough.
Again, I don't think users who install ntpd should have to also change
their boot parameters.
> All systems I've seen do load modules very early (before they would
> start anything ntp related). And the new hctosys is done inside the
> registration of the RTC. So if a system is configured in such a way
> that the race really might happen, the system would be broken because
> the RTC would not be there when starting ntp. To conclude, I think the
> problem is highly academic/artificial and, if possible at all, only
> possible on already misconfigured systems during a very, very small
> window. Therfor I still believe that no locking mechanism is needed.
>
> Anyway, I don't care, if you want, I will make an accessor-function
> with locks and an return code for "set" to indicate if it was already
> set.
Sorry if I'm frustrating you here, that's not my intent.
>
> We could also request and wait for a third opinion on that topic. As
> it most likely will not end up in any kernel before the end of this
> year (if at all), there is enough time to do so.
>
I'm in no rush. I think the changes you're proposing are interesting and
novel cases that we should handle. But I also do think we should do it
properly, especially since its relatively easy to do in this case.
thanks
-john
Am 20.06.2013 19:27, schrieb John Stultz:
> On 06/20/2013 03:15 AM, Alexander Holler wrote:
>> Am 17.06.2013 20:10, schrieb John Stultz:
>>> On 06/14/2013 11:01 PM, Alexander Holler wrote:
>>>
>>>> What do you think I should write?
>>>>
>>>> void set_systime_was_set(void) and void clear_systime_was_set(void)?
>>>>
>>>> And both functions would have to be exported in order to be usable
>>>> from modules?
>>>>
>>>> Or do you think I should write something like that:
>>>>
>>>> extern bool foo;
>>>> inline void set_foo(void) { foo = true};
>>>> inline void clear_foo(void) { foo = false };
>>>>
>>>> That's just silly, sorry to call it such.
>>>
>>> No no. I'm only asking that the boolean be static to timekeeping.c and
>>> an accessor function be used to read it. Since the timekeeping core
>>> should be managing this value, there should be no reason for any other
>>> users to be setting or clearing the value.
>>
>> First you can't make the value static (semantically) because it has to
>> be set and cleared from different parts of the kernel. And adding
>> accessor functions doesn't help in any way, everyone will still be
>> able to set or clear the value (this still is C and no C++ with
>> classes or other encapsulation features). The only thing what will
>> happen with such an accessor function is that a small overhead through
>> the then necessary function call is introduced.
>
>
>
>
> Why would it be set and cleared from different parts of the kernel?
>
> We're checking if the system time was set. The system time can be set
> only from a limited number functions in timekeeping.c. It seems
> reasonable it should be static to timekeeping.c
>
> Even so, this is all a tangent. I really think the flag value is racy
> and should be dropped for a timekeeping_setime_if_not_set() - or better
> named - function that can act atomically.
>
>
>
>
>>>
>>>>>> Of course, I might be wrong and there might be a use case where
>>>>>> multiple things do set the system time concurrently and nothing else
>>>>>> did set system time before, but I found that extremly unlikely.
>>>>>
>>>>> Yea, the condition check and the action won't be both be done under a
>>>>> lock, so its likely going to be racy anyway.
>>>>
>>>> And if there ever will be a race for the first timesource to set this
>>>> flag (the first time), and something does care about the outtake, the
>>>> system would be completly broken.
>>>>
>>>> In order to keep it simple, I just tread userspace like a RTC of type
>>>> X and will call them all timesources of type x where a the type is
>>>> defined by the driver.
>>>>
>>>> Let us go through the possible cases:
>>>>
>>>> - 2 or more timesources of different type:
>>>>
>>>> If the order is undefined and they have to race for which clock might
>>>> be used for hctosys (and thus for adjusting the time after resume
>>>> too), the only reason one would want such is for HA purposes. And in
>>>> case of HA, both clocks must have the same time, so nobody does care
>>>> about which one will win the race (=> no race, no lock or atomic_*
>>>> needed).
>>>>
>>>> If the purpose isn't for HA and someone does care about which
>>>> timesource should be used, the way to do this is to use hctosys=type
>>>> (or hctosys=none in case of userspace) to define which timesource
>>>> should be used for hctosys (=> no race, no lock or atomic_* needed).
>>>>
>>>> - 2 or more timesources of the same type:
>>>> There is no possibility to define which one should win the race. Such
>>>> a system configuration is only usable for HA purposes, so if such
>>>> exists, nobody cares about the outtake of the race (=> no race, no
>>>> lock or atomic_* needed).
>>>>
>>>
>>> The race I'm thinking of is you have a system that normally sets the
>>> time via ntpdate at bootup. Thus they expect the system to always be
>>> started w/ NTP time (even if the system time was initially set via
>>> hctosys).
>>>
>>> Then because of of some delay in the driver (or because the RTC device
>>> was plugged in during boot), the hctosys functionality runs just as
>>> ntpdate is being called. hctosys sees time has not yet been set and
>>> reads the RTC hardware time. At this point, ntpdate sets the time to NTP
>>> time. Then hctosys completes, setting the time to the RTC time. This
>>> results in the system clock being wrong from the user's perspective (as
>>> they expect it to be set to NTP time).
>>
>> Therefor there now will be hctosys as a kernel command line parameter.
>> Instead of a kernel config option which can't be changed by 99% of all
>> Linux users, that option allows ordinary (non kernel compiling) users
>> to disable hctosys at all.
>
>
> I agree your suggestion of having a hctosys= boot option (to override
> the CONFIG_HCTOSYS_DEVICE value) could be a useful extension.
>
> But we shouldn't expect users to set magic boot flags in order to have a
> reliably functioning system. If userland sets the time during init, and
> the hctosys functionality isn't supposed to overwrite that value, then
> there should be no case where userland sets the time at boot, but we end
> up with the RTC time after boot. But currently that race is possible
> (though small).
>
> A more concrete case:
> On many distros ntpd isn't installed by default. Instead they leave the
> kernel to initialize the time from the RTC.
Which still is done, even earlier with the new hctosys (if a RTC is used
instead of a persistent clock). Nothing changed there. And if the
persistent clock is used, which is true on almost all x86 systems, the
race doesn't exist at all, at least as long as the persistent clock
still exists and will be used (instead of rtc-cmos).
>
> But ntpd can be installed afterwards, and it would be silly to require
> users edit their boot arguments when installing the ntp package.
>
>
>> Something which wasn't possible before without recompiling the kernel.
>> And, like before, most RTC drivers will be loaded before userspace
>> calls ntp/ntpdate. If not, the system is already broken.
>
> I'm not sure I'm following how the system is already broken?
Because it isn't determined what does set the time. The race you've
described happens because someone wants to use ntp for the hctosys
functionality but he doesn't want it, if the date might come first from
a RTC (in case the race window would be even hit). So the configuration
is broken because it is non-deterministic while someone wants deterministic.
>
>
>> And just in case, I've made that possible window for the above race
>> very small by checking the flag systime_was_set twice, once before
>> starting to read the time and a second time right before the time is
>> set. Ok, the race is still there, but as said before, if that problem
>> does exist at all, the system would be setup wrong at all.
>
> It just seems that if we really want a to do this, we might as well do
> it right, using the timekeeping_settime_first() or whatever function
> that can properly check the value and complete the action atomically
> while holding the lock.
>
>
>>>
>>> This is basically what this code is trying to avoid in the first place.
>>> And I'll grant that its a small race window, but it may lead to
>>> irregular behavior.
>>>
>>> So either we need to document that this race is theoretically possible,
>>> and explain *why* its safe to ignore it. Or if we really want to do
>>
>> I would think that documenting hctosys=none should be enough.
>
> Again, I don't think users who install ntpd should have to also change
> their boot parameters.
>
>
>> All systems I've seen do load modules very early (before they would
>> start anything ntp related). And the new hctosys is done inside the
>> registration of the RTC. So if a system is configured in such a way
>> that the race really might happen, the system would be broken because
>> the RTC would not be there when starting ntp. To conclude, I think the
>> problem is highly academic/artificial and, if possible at all, only
>> possible on already misconfigured systems during a very, very small
>> window. Therfor I still believe that no locking mechanism is needed.
>>
>> Anyway, I don't care, if you want, I will make an accessor-function
>> with locks and an return code for "set" to indicate if it was already
>> set.
>
> Sorry if I'm frustrating you here, that's not my intent.
I'm not frustrated, I'm just annoyed. I don't know why everyone seems to
assume that I'm getting frustrated while I just find it totally annoying
to invest time to make checkpatch-clean max. 80 chars per line wide
patches while having to use 8 chars intendation and meaningfull variable
and function names. Besides having to explain and discuss every single
line and often get called a fool or similiar. I might become frustrated
if I would need to get patches into the mainline kernel, but happily I'm
able to manage my own tree of patches and don't have to care if patches
from me will end up in the mainline kernel. After all I still find it
much less time consuming to fix bugs and add additional functionalities
myself instead of just writing bug reports or requesting features. And
it becomes easier with every patch I write for subsystem xy. ;)
>
>>
>> We could also request and wait for a third opinion on that topic. As
>> it most likely will not end up in any kernel before the end of this
>> year (if at all), there is enough time to do so.
>>
>
> I'm in no rush. I think the changes you're proposing are interesting and
> novel cases that we should handle. But I also do think we should do it
> properly, especially since its relatively easy to do in this case.
Anyway, my intention was to avoid locking stuff in the time-setting
functions but it looks like that isn't wanted. So I will implement the
locking foo, after we finished the discussion about the other parts and
I will have come to the impression that I don't write stuff just for
nothing. A quick look at your other comments revealed that I have to
explain a lot more in order to not get accused that I want to kill the
timekeeping system. ;)
I will answer the next comment shortly.
Regards,
Alexander Holler
On 06/20/2013 11:45 AM, Alexander Holler wrote:
> Am 20.06.2013 19:27, schrieb John Stultz:
>> On 06/20/2013 03:15 AM, Alexander Holler wrote:
>>> Therefor there now will be hctosys as a kernel command line parameter.
>>> Instead of a kernel config option which can't be changed by 99% of all
>>> Linux users, that option allows ordinary (non kernel compiling) users
>>> to disable hctosys at all.
>>
>>
>> I agree your suggestion of having a hctosys= boot option (to override
>> the CONFIG_HCTOSYS_DEVICE value) could be a useful extension.
>>
>> But we shouldn't expect users to set magic boot flags in order to have a
>> reliably functioning system. If userland sets the time during init, and
>> the hctosys functionality isn't supposed to overwrite that value, then
>> there should be no case where userland sets the time at boot, but we end
>> up with the RTC time after boot. But currently that race is possible
>> (though small).
>>
>> A more concrete case:
>> On many distros ntpd isn't installed by default. Instead they leave the
>> kernel to initialize the time from the RTC.
>
> Which still is done, even earlier with the new hctosys (if a RTC is
> used instead of a persistent clock). Nothing changed there. And if the
> persistent clock is used, which is true on almost all x86 systems, the
> race doesn't exist at all, at least as long as the persistent clock
> still exists and will be used (instead of rtc-cmos).
Yea, eventually I'd like to push the persistent clock functionality into
the RTC core, or remove its use for time initialization and only use the
persistent clock for suspend/resume timing.
But just because the race doesn't exist on x86, doesn't mean we can
ignore it for all the various other arches.
>
>>
>> But ntpd can be installed afterwards, and it would be silly to require
>> users edit their boot arguments when installing the ntp package.
>>
This point you left un-addressed, and is the key problem I see with
requiring boot arguments.
>>
>>> Something which wasn't possible before without recompiling the kernel.
>>> And, like before, most RTC drivers will be loaded before userspace
>>> calls ntp/ntpdate. If not, the system is already broken.
>>
>> I'm not sure I'm following how the system is already broken?
>
> Because it isn't determined what does set the time. The race you've
> described happens because someone wants to use ntp for the hctosys
> functionality but he doesn't want it, if the date might come first
> from a RTC (in case the race window would be even hit). So the
> configuration is broken because it is non-deterministic while someone
> wants deterministic.
I still don't see this. As it stands with the current kernel, the
HCTOSYS functionality runs prior to init starting, so it may initialize
time, but any userland setting of time will have the final say.
You're patches allow for the HCTOSYS functionality to happen after init
starts. And the systime_was_set flag you proposed seems to address this
change in behavior the following way: Assuming userland has not set the
clock, allow the HCTOSYS functionality to set the clock after userland
has run.
This seems like a reasonable balance. However, with your implementation
there is a small race possible, such that the hctosys might set the time
to RTC time after userland has set the time.
You seem to be saying the race is only possible if the system doesn't
use the hctosys= boot argument you're also proposing.
But machines don't need a boot argument now, and aren't broken, so why
do we want to add an option that requires everyone to use it? Personally
I think requiring a boot argument is unnecessary (though having a boot
option can be helpful in some cases - don't mistake me for thinking the
option is a bad idea, I just don't think it should be required) and is
actually problematic for distros to handle properly.
>
>>
>>
>>> And just in case, I've made that possible window for the above race
>>> very small by checking the flag systime_was_set twice, once before
>>> starting to read the time and a second time right before the time is
>>> set. Ok, the race is still there, but as said before, if that problem
>>> does exist at all, the system would be setup wrong at all.
>>
>> It just seems that if we really want a to do this, we might as well do
>> it right, using the timekeeping_settime_first() or whatever function
>> that can properly check the value and complete the action atomically
>> while holding the lock.
>>
>>
>>>>
>>>> This is basically what this code is trying to avoid in the first
>>>> place.
>>>> And I'll grant that its a small race window, but it may lead to
>>>> irregular behavior.
>>>>
>>>> So either we need to document that this race is theoretically
>>>> possible,
>>>> and explain *why* its safe to ignore it. Or if we really want to do
>>>
>>> I would think that documenting hctosys=none should be enough.
>>
>> Again, I don't think users who install ntpd should have to also change
>> their boot parameters.
>>
>>
>>> All systems I've seen do load modules very early (before they would
>>> start anything ntp related). And the new hctosys is done inside the
>>> registration of the RTC. So if a system is configured in such a way
>>> that the race really might happen, the system would be broken because
>>> the RTC would not be there when starting ntp. To conclude, I think the
>>> problem is highly academic/artificial and, if possible at all, only
>>> possible on already misconfigured systems during a very, very small
>>> window. Therfor I still believe that no locking mechanism is needed.
>>>
>>> Anyway, I don't care, if you want, I will make an accessor-function
>>> with locks and an return code for "set" to indicate if it was already
>>> set.
>>
>> Sorry if I'm frustrating you here, that's not my intent.
>
> I'm not frustrated, I'm just annoyed.
Well, I'm sorry if I'm annoying you. Again, not my intent.
> I don't know why everyone seems to assume that I'm getting frustrated
> while I just find it totally annoying to invest time to make
> checkpatch-clean max. 80 chars per line wide patches while having to
> use 8 chars intendation and meaningfull variable and function names.
> Besides having to explain and discuss every single line and often get
> called a fool or similiar.
Just to be clear, I'm in no way calling you a fool. Part of the review
process is that we have to understand not only what is done but *why*
its being done. So I have to ask (sometimes dumb) questions so that
*why* is clearly established. Please don't take these inquiries as
questioning your ability.
>>>
>>> We could also request and wait for a third opinion on that topic. As
>>> it most likely will not end up in any kernel before the end of this
>>> year (if at all), there is enough time to do so.
>>>
>>
>> I'm in no rush. I think the changes you're proposing are interesting and
>> novel cases that we should handle. But I also do think we should do it
>> properly, especially since its relatively easy to do in this case.
>
> Anyway, my intention was to avoid locking stuff in the time-setting
> functions but it looks like that isn't wanted. So I will implement the
> locking foo, after we finished the discussion about the other parts
> and I will have come to the impression that I don't write stuff just
> for nothing. A quick look at your other comments revealed that I have
> to explain a lot more in order to not get accused that I want to kill
> the timekeeping system. ;)
To be clear, we already do locking in the time-setting functions. I'm
just proposing we introduce a new time setting function that uses the
same locking to allow for the flag to be tested and the time to be set
atomically (returning an error code if the time had been previously set).
thanks
-john
Am 20.06.2013 21:28, schrieb John Stultz:
>>>
>>> But ntpd can be installed afterwards, and it would be silly to require
>>> users edit their boot arguments when installing the ntp package.
>>>
>
> This point you left un-addressed, and is the key problem I see with
> requiring boot arguments.
There is no requirement for an additional boot argument.
That would only be necessary if you start ntpdate without using a
persistent clock and before loading of (working) rtc-modules would have
finished. And even then it would only be necessary if you use ntpdate
(once) while a driver for the rtc is in it's registration phase and
wants to set the clock or if you use ntp and the rtc which registers
while ntp sets the time, has a time which would force ntp to refuse
further adjustments. And in both cases only, if you don't have disabled
the proposed hctosys with using a boot argument. Just because a boot
argument makes it possible to disable hctosys by RTC doesn't mean it's
necessary.
Sorry for beeing pedantic, but I have to defend my decision to avoid
locking just because of that possibility. I was fully aware of that
unlikely race condition.
Anyway, I've already accomplished to add locking to prevent that case.
Regards,
Alexander Holler
Am 14.06.2013 21:11, schrieb John Stultz:
> On 06/14/2013 09:52 AM, Alexander Holler wrote:
>> 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.
>
> This one concerns me a bit. Since you're removing quite a bit and it
> looks like it may break userland expectations.
The only thing I remove from a user point of view is a broken (because
undetermined) informational entry (/proc/driver/rtc) if persistent
clocks are used.
>
> I ran into this myself recently, when I found some distros look for
> /sys/class/rtc/rtcN/hctosys in order to determine which rtc device
> should be synced with from userland.
The patches don't change anything in /sys.
>
> So I'd probably suggest instead to re-factor this so you leave all the
> hctosys bits alone, but just change it from being called by a
> late_initcall() and instead have it called when we register the RTC that
> matches CONFIG_RTC_HCTOSYS_DEVICE.
>
> I suspect it will end up being a much smaller change that way.
>
> Then the last bit is just a matter of adding the
> timekeeping_systimeset() check to the hctosys bits.
This doesn't work because the current hctosys approach is (imho) broken
by design, at least how it currently works. Of course, this might be the
result of changes, refused patches or whatever, I don't know the history
of the current hctosys.
First it's only a kernel configuration, that means most users can't even
change it. Second, it just defines the N in rtcN, which is based on the
order RTC drivers are loaded (undefined). Third, it ignores persistent
clocks, and if persistent clocks are used, the informational entry in
/proc is misleading and just might fit because of some lucky circumstance.
As it seems hard to understand the changes, here they are again, maybe
my second description is more understandable.
1. Replacment of the Kernel config option CONFIG_RTC_HCTOSYS by a kernel
command line parameter hctosys=.
- Kernel config options are unavailable for most users.
A kernel command line parameter gives them the possibility to
change it.
- The current config option ignores persistent clocks at all. It
doesn't allow to disable persistent clocks and therefor it is
misleading. I assume most users don't even know persistent clocks
do exist. Fixed with the new kernel command line parameter.
2. Replacement of RTC_HCTOSYS_DEVICE by a kernel command line parameter
hctosys=.
- Again, it's a kernel config option only, not changeable by most
users. The new hctosys= allows ordinary (non-kernel-compiling) users
to change that.
- The current config option is based on the order RTC drivers are
loaded, which is non-deterministic thus undefined. The new hctosys=
allows to choose the used RTC driver by name.
- The config option ignores persistent clocks, the new hctosys=
kernel commandline parameter doesn't.
3. Removal of /proc/driver/rtc if and only if a persistent clock is used.
- If a persistent clock is used, the informational entry in /proc
might be totally wrong because it is based on the first loaded RTC
driver. The first loaded RTC driver doesn't have to be the
corresponding rtc-driver which uses the same hardware clock as the
persistent clock and might describe a totally different hardware
clock.
- If a persistent clock is used, it describes the abilities of the
RTC driver, but not those of the persistent clock driver. This
might be misleading because only the persistent clock driver is
used and not the corresponding (described) RTC driver.
- No changes for users of any RTC driver. The entry in /proc will
still be there.
Regards,
Alexander Holler
On Thu, 20 Jun 2013 12:39:36 +0200 Alexander Holler <[email protected]> wrote:
> rtc_device_register() might want to read the clock which doesn't work
> before the hid device is registered. Therefor we delay the registration of
> the rtc driver by moving it to a work.
>
> --- a/drivers/rtc/rtc-hid-sensor-time.c
> +++ b/drivers/rtc/rtc-hid-sensor-time.c
> @@ -37,6 +37,11 @@ enum hid_time_channel {
> TIME_RTC_CHANNEL_MAX,
> };
>
> +struct hid_time_workts {
Strange name. I can't work out what the "ts" means.
> + struct work_struct work;
> + struct hid_time_state *time_state;
> +};
> +
> struct hid_time_state {
> struct hid_sensor_hub_callbacks callbacks;
> struct hid_sensor_common common_attributes;
>
> ...
>
> @@ -237,6 +243,36 @@ static const struct rtc_class_ops hid_time_rtc_ops = {
> .read_time = hid_rtc_read_time,
> };
>
> +static void hid_time_register_rtc_work(struct work_struct *work)
> +{
> + struct hid_time_state *time_state =
> + container_of(work, struct hid_time_workts, work)
> + ->time_state;
> + struct platform_device *pdev = time_state->callbacks.pdev;
Ick. When the initialisers overflow 80 cols, the fix is easy: don't
use initalisers:
struct hid_time_state *time_state;
struct platform_device *pdev;
time_state = container_of(work, struct hid_time_workts, work)->time_state;
pdev = time_state->callbacks.pdev;
> + time_state->rtc = devm_rtc_device_register(&pdev->dev,
> + "hid-sensor-time", &hid_time_rtc_ops,
> + THIS_MODULE);
> + if (IS_ERR_OR_NULL(time_state->rtc)) {
> + struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
Newline after end-of-definitions and before start-of-code, please.
> + sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
> + time_state->rtc = NULL;
> + dev_err(&pdev->dev, "rtc device register failed!\n");
> + /*
> + * I haven't a found a way to remove only this device from
> + * hid-sensor-hub. Removing the device a level above (the
> + * complete HID device) doesn't work, because a sensor-hub
> + * might provide more than just a time-sensor and thus we
> + * would remove all sensors not just this one.
> + * So we just leave this driver idling around until I or
> + * someone else has figured out how to remove this device
> + * from hid-sensor-hub.
> + */
> + }
> + time_state->workts = NULL;
> + kfree(work);
> +}
> +
> static int hid_time_probe(struct platform_device *pdev)
> {
> int ret = 0;
> @@ -279,22 +315,34 @@ static int hid_time_probe(struct platform_device *pdev)
> return ret;
> }
>
> - time_state->rtc = devm_rtc_device_register(&pdev->dev,
> - "hid-sensor-time", &hid_time_rtc_ops,
> - THIS_MODULE);
> -
> - if (IS_ERR(time_state->rtc)) {
> - dev_err(&pdev->dev, "rtc device register failed!\n");
> - return PTR_ERR(time_state->rtc);
> + /*
> + * The HID device has to be registered to read the clock.
> + * Because rtc_device_register() might read the time, we have to delay
> + * rtc_device_register() to a work in order to finish the probe before.
> + */
> + time_state->workts = kmalloc(sizeof(struct hid_time_workts),
> + GFP_KERNEL);
> + if (time_state->workts == NULL) {
> + sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
> + return -ENOMEM;
> }
> + time_state->workts->time_state = time_state;
> + INIT_WORK(&time_state->workts->work,
> + hid_time_register_rtc_work);
> + schedule_work(&time_state->workts->work);
This seems unreliable. The scheduled work can run one nanosecond
later, on this or a different CPU. What guarantees that the HID device
will then be fully registered?
Am 26.06.2013 21:55, schrieb Andrew Morton:
> On Thu, 20 Jun 2013 12:39:36 +0200 Alexander Holler <[email protected]> wrote:
>
>> rtc_device_register() might want to read the clock which doesn't work
>> before the hid device is registered. Therefor we delay the registration of
>> the rtc driver by moving it to a work.
>>
>
>
>> --- a/drivers/rtc/rtc-hid-sensor-time.c
>> +++ b/drivers/rtc/rtc-hid-sensor-time.c
>> @@ -37,6 +37,11 @@ enum hid_time_channel {
>> TIME_RTC_CHANNEL_MAX,
>> };
>>
>> +struct hid_time_workts {
>
> Strange name. I can't work out what the "ts" means.
It's just a name
>
>> + struct work_struct work;
>> + struct hid_time_state *time_state;
>> +};
and stands for work + time_state. Peronally I would use
hid_time_work_time_state, but then I would get even more problems to go
conform with CGA restrictions on line widths.
>> +
>> struct hid_time_state {
>> struct hid_sensor_hub_callbacks callbacks;
>> struct hid_sensor_common common_attributes;
>>
>> ...
>>
>> @@ -237,6 +243,36 @@ static const struct rtc_class_ops hid_time_rtc_ops = {
>> .read_time = hid_rtc_read_time,
>> };
>>
>> +static void hid_time_register_rtc_work(struct work_struct *work)
>> +{
>> + struct hid_time_state *time_state =
>> + container_of(work, struct hid_time_workts, work)
>> + ->time_state;
>> + struct platform_device *pdev = time_state->callbacks.pdev;
>
> Ick. When the initialisers overflow 80 cols, the fix is easy: don't
> use initalisers:
>
> struct hid_time_state *time_state;
> struct platform_device *pdev;
>
> time_state = container_of(work, struct hid_time_workts, work)->time_state;
> pdev = time_state->callbacks.pdev;
>
Sorry, but it's long ago since I had to use a DOS machine and I still
don't use a phone to write source, therefor I'm not very skilled in
writing readable source with meaningfull names in max. 72 (80-8) chars
per line. But I will work hard to relearn those long forgotten skills,
they might become handy again, when PCs with monitors got finally
replaced by phones and tablets with small screens. ;)
>> + time_state->rtc = devm_rtc_device_register(&pdev->dev,
>> + "hid-sensor-time", &hid_time_rtc_ops,
>> + THIS_MODULE);
>> + if (IS_ERR_OR_NULL(time_state->rtc)) {
>> + struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>
> Newline after end-of-definitions and before start-of-code, please.
>
>> + sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
>> + time_state->rtc = NULL;
>> + dev_err(&pdev->dev, "rtc device register failed!\n");
>> + /*
>> + * I haven't a found a way to remove only this device from
>> + * hid-sensor-hub. Removing the device a level above (the
>> + * complete HID device) doesn't work, because a sensor-hub
>> + * might provide more than just a time-sensor and thus we
>> + * would remove all sensors not just this one.
>> + * So we just leave this driver idling around until I or
>> + * someone else has figured out how to remove this device
>> + * from hid-sensor-hub.
>> + */
>> + }
>> + time_state->workts = NULL;
>> + kfree(work);
>> +}
>> +
>> static int hid_time_probe(struct platform_device *pdev)
>> {
>> int ret = 0;
>> @@ -279,22 +315,34 @@ static int hid_time_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> - time_state->rtc = devm_rtc_device_register(&pdev->dev,
>> - "hid-sensor-time", &hid_time_rtc_ops,
>> - THIS_MODULE);
>> -
>> - if (IS_ERR(time_state->rtc)) {
>> - dev_err(&pdev->dev, "rtc device register failed!\n");
>> - return PTR_ERR(time_state->rtc);
>> + /*
>> + * The HID device has to be registered to read the clock.
>> + * Because rtc_device_register() might read the time, we have to delay
>> + * rtc_device_register() to a work in order to finish the probe before.
>> + */
>> + time_state->workts = kmalloc(sizeof(struct hid_time_workts),
>> + GFP_KERNEL);
>> + if (time_state->workts == NULL) {
>> + sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
>> + return -ENOMEM;
>> }
>> + time_state->workts->time_state = time_state;
>> + INIT_WORK(&time_state->workts->work,
>> + hid_time_register_rtc_work);
>> + schedule_work(&time_state->workts->work);
>
> This seems unreliable. The scheduled work can run one nanosecond
> later, on this or a different CPU. What guarantees that the HID device
> will then be fully registered?
Nothing, but schedule_delayed_work() is as unreliable as without delay
and I don't know of any callback after registration has happened. I have
to dig through the hid-(sensor-)code, maybe I will find a callback I can
(mis)use to register the rtc driver after the hid driver was registered.
I will write a v3 if I've found something.
Regards,
Alexander Holler
On Wed, Jun 26, 2013 at 11:34:35PM +0200, Alexander Holler wrote:
> >> + /*
> >> + * The HID device has to be registered to read the clock.
> >> + * Because rtc_device_register() might read the time, we have to delay
> >> + * rtc_device_register() to a work in order to finish the probe before.
> >> + */
> >> + time_state->workts = kmalloc(sizeof(struct hid_time_workts),
> >> + GFP_KERNEL);
> >> + if (time_state->workts == NULL) {
> >> + sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
> >> + return -ENOMEM;
> >> }
> >> + time_state->workts->time_state = time_state;
> >> + INIT_WORK(&time_state->workts->work,
> >> + hid_time_register_rtc_work);
> >> + schedule_work(&time_state->workts->work);
> >
> > This seems unreliable. The scheduled work can run one nanosecond
> > later, on this or a different CPU. What guarantees that the HID device
> > will then be fully registered?
>
> Nothing, but schedule_delayed_work() is as unreliable as without delay
> and I don't know of any callback after registration has happened. I have
> to dig through the hid-(sensor-)code, maybe I will find a callback I can
> (mis)use to register the rtc driver after the hid driver was registered.
Why not use the deferred_probe code, which is there just for this type
of thing (i.e. your other drivers/devices aren't present/initialized
yet.)? Just return -EPROBE_DEFER from your probe function if you don't
find everything already set up properly, the driver core will call you
again later after it has initialized everything it has found.
Hope this helps,
greg k-h
Am 27.06.2013 00:07, schrieb Greg KH:
> On Wed, Jun 26, 2013 at 11:34:35PM +0200, Alexander Holler wrote:
>>>> + /*
>>>> + * The HID device has to be registered to read the clock.
>>>> + * Because rtc_device_register() might read the time, we have to delay
>>>> + * rtc_device_register() to a work in order to finish the probe before.
>>>> + */
>>>> + time_state->workts = kmalloc(sizeof(struct hid_time_workts),
>>>> + GFP_KERNEL);
>>>> + if (time_state->workts == NULL) {
>>>> + sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
>>>> + return -ENOMEM;
>>>> }
>>>> + time_state->workts->time_state = time_state;
>>>> + INIT_WORK(&time_state->workts->work,
>>>> + hid_time_register_rtc_work);
>>>> + schedule_work(&time_state->workts->work);
>>>
>>> This seems unreliable. The scheduled work can run one nanosecond
>>> later, on this or a different CPU. What guarantees that the HID device
>>> will then be fully registered?
>>
>> Nothing, but schedule_delayed_work() is as unreliable as without delay
>> and I don't know of any callback after registration has happened. I have
>> to dig through the hid-(sensor-)code, maybe I will find a callback I can
>> (mis)use to register the rtc driver after the hid driver was registered.
>
> Why not use the deferred_probe code, which is there just for this type
> of thing (i.e. your other drivers/devices aren't present/initialized
> yet.)? Just return -EPROBE_DEFER from your probe function if you don't
> find everything already set up properly, the driver core will call you
> again later after it has initialized everything it has found.
Hmm, didn't know about the deferred_probe stuff. Thanks.
Unfortunately I currently don't see how this might help here. The
rtc-device will not be probed, so it can't be deferred. And even if I
will find or implement a way to add a probe for the rtc device, I still
have to search how to find out if the HID device is registered.
Anyway, back to reading to sources. Maybe there already is some callback
from hid-sensor-hub or the hid-subsystem I can use. I haven't searched
in deep for such because using a work worked just fine here on several
machines (besides that it was a quick hack which got only necessary with
the changed hctosys which reads the time in rtc_device_register()).
I already wondered why using a work (even without delay) did work, but I
explained it with some (maybe imaginary) locality of works, such that
they get called after the scheduling thread gives up his timeslice which
happily happened after the hid-device was registered. So I hoped (hope
dies last ;) ) to only have to fix it, if it actually doesn't work
somewhere or sometimes after the foreseen discussion about hctosys has
come to an end.
Regards,
Alexander Holler
Am 26.06.2013 23:34, schrieb Alexander Holler:
> Am 26.06.2013 21:55, schrieb Andrew Morton:
>> On Thu, 20 Jun 2013 12:39:36 +0200 Alexander Holler <[email protected]> wrote:
>>> +static void hid_time_register_rtc_work(struct work_struct *work)
>>> +{
>>> + struct hid_time_state *time_state =
>>> + container_of(work, struct hid_time_workts, work)
>>> + ->time_state;
>>> + struct platform_device *pdev = time_state->callbacks.pdev;
>>
>> Ick. When the initialisers overflow 80 cols, the fix is easy: don't
>> use initalisers:
>>
>> struct hid_time_state *time_state;
>> struct platform_device *pdev;
>>
>> time_state = container_of(work, struct hid_time_workts, work)->time_state;
>> pdev = time_state->callbacks.pdev;
>>
>
> Sorry, but it's long ago since I had to use a DOS machine and I still
> don't use a phone to write source, therefor I'm not very skilled in
> writing readable source with meaningfull names in max. 72 (80-8) chars
> per line. But I will work hard to relearn those long forgotten skills,
> they might become handy again, when PCs with monitors got finally
> replaced by phones and tablets with small screens. ;)
To do other poor patch submitters which don't use a video terminal too a
favor, I've decided it might make sense to describe the workflow which
is responsible for the above stuff which makes you cry so often. It
isn't that I don't know that I don't have to use initializers, I know C
since 3 decades.
The following happens here:
- I'm writing source/patches without limiting myself to 80x25 chars.
- Then I'm executing the insulting and must be one of the most hated
piece of software callled checkpatch.pl.
- It tells me to place or delete spaces here and cut lines there.
- I do exactly that, while having my brain turned off (self-protection)
Therefor stuff the like the above happens. It isn't badwill,
incompetence or inability.
Regards,
Alexander Holler
Am 27.06.2013 01:51, schrieb Alexander Holler:
> Am 27.06.2013 00:07, schrieb Greg KH:
>> On Wed, Jun 26, 2013 at 11:34:35PM +0200, Alexander Holler wrote:
>>>>> + /*
>>>>> + * The HID device has to be registered to read the clock.
>>>>> + * Because rtc_device_register() might read the time, we have to delay
>>>>> + * rtc_device_register() to a work in order to finish the probe before.
>>>>> + */
>>>>> + time_state->workts = kmalloc(sizeof(struct hid_time_workts),
>>>>> + GFP_KERNEL);
>>>>> + if (time_state->workts == NULL) {
>>>>> + sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
>>>>> + return -ENOMEM;
>>>>> }
>>>>> + time_state->workts->time_state = time_state;
>>>>> + INIT_WORK(&time_state->workts->work,
>>>>> + hid_time_register_rtc_work);
>>>>> + schedule_work(&time_state->workts->work);
>>>>
>>>> This seems unreliable. The scheduled work can run one nanosecond
>>>> later, on this or a different CPU. What guarantees that the HID device
>>>> will then be fully registered?
>>>
>>> Nothing, but schedule_delayed_work() is as unreliable as without delay
>>> and I don't know of any callback after registration has happened. I have
>>> to dig through the hid-(sensor-)code, maybe I will find a callback I can
>>> (mis)use to register the rtc driver after the hid driver was registered.
>>
>> Why not use the deferred_probe code, which is there just for this type
>> of thing (i.e. your other drivers/devices aren't present/initialized
>> yet.)? Just return -EPROBE_DEFER from your probe function if you don't
>> find everything already set up properly, the driver core will call you
>> again later after it has initialized everything it has found.
>
> Hmm, didn't know about the deferred_probe stuff. Thanks.
>
> Unfortunately I currently don't see how this might help here. The
> rtc-device will not be probed, so it can't be deferred. And even if I
> will find or implement a way to add a probe for the rtc device, I still
> have to search how to find out if the HID device is registered.
>
> Anyway, back to reading to sources. Maybe there already is some callback
> from hid-sensor-hub or the hid-subsystem I can use. I haven't searched
> in deep for such because using a work worked just fine here on several
> machines (besides that it was a quick hack which got only necessary with
> the changed hctosys which reads the time in rtc_device_register()).
>
> I already wondered why using a work (even without delay) did work, but I
> explained it with some (maybe imaginary) locality of works, such that
> they get called after the scheduling thread gives up his timeslice which
> happily happened after the hid-device was registered. So I hoped (hope
> dies last ;) ) to only have to fix it, if it actually doesn't work
> somewhere or sometimes after the foreseen discussion about hctosys has
> come to an end.
>
I've now traced down why the above patch _really_ is needed: it's
because of how the locking is done in the hid-subsystem. So my intuition
to use a work was ok, but my assumption that it's because the HID device
driver wasn't registered before probe() finished was wrong.
What happens without using a work is the following (shortened a lot):
hid_device_probe() // hid subsystem locks hdev->driver_input_lock
hid_time_probe()
devm_rtc_device_register() // wants to read the clock
hid_rtc_read_time() // submits GET_REPORT and waits for the answer
// (the timestamp from the clock)
hid_input_report()
And there we have something like a deadlock situation because
hid_input_report() needs hdev->driver_input_lock to submit the report to
the HID driver.
So in short, it's currently impossible for a HID driver to read input
from a HID device from inside it's probe function.
That means the patch is fine, but the comment is wrong.
Because I think it would be better to change the locking inside the HID
subsystem (drivers/hid/hid-core.c) in order to allow the probe function
of HID drivers to read input, I will first have a look if I see a way to
change the locking in hid-core.c, before I will post an update to the
above patch with a corrected comment. But this might need some time as
I'm not familiar with the locking in the HID subsystem and my motivation
currently isn't very high.
Regards,
Alexander Holler
On Sat, 6 Jul 2013, Alexander Holler wrote:
> I've now traced down why the above patch _really_ is needed: it's because of
> how the locking is done in the hid-subsystem. So my intuition to use a work
> was ok, but my assumption that it's because the HID device driver wasn't
> registered before probe() finished was wrong.
>
> What happens without using a work is the following (shortened a lot):
>
> hid_device_probe() // hid subsystem locks hdev->driver_input_lock
> hid_time_probe()
> devm_rtc_device_register() // wants to read the clock
> hid_rtc_read_time() // submits GET_REPORT and waits for the answer
> // (the timestamp from the clock)
> hid_input_report()
>
> And there we have something like a deadlock situation because
> hid_input_report() needs hdev->driver_input_lock to submit the report to the
> HID driver.
>
> So in short, it's currently impossible for a HID driver to read input from a
> HID device from inside it's probe function.
Please see commit c849a6143b ("HID: Separate struct hid_device's
driver_lock into two locks"). It's done exactly in order to allow for
receiving input inside of the probe() function of the driver.
--
Jiri Kosina
SUSE Labs
Am 06.07.2013 20:21, schrieb Jiri Kosina:
> On Sat, 6 Jul 2013, Alexander Holler wrote:
>
>> I've now traced down why the above patch _really_ is needed: it's because of
>> how the locking is done in the hid-subsystem. So my intuition to use a work
>> was ok, but my assumption that it's because the HID device driver wasn't
>> registered before probe() finished was wrong.
>>
>> What happens without using a work is the following (shortened a lot):
>>
>> hid_device_probe() // hid subsystem locks hdev->driver_input_lock
>> hid_time_probe()
>> devm_rtc_device_register() // wants to read the clock
>> hid_rtc_read_time() // submits GET_REPORT and waits for the answer
>> // (the timestamp from the clock)
>> hid_input_report()
>>
>> And there we have something like a deadlock situation because
>> hid_input_report() needs hdev->driver_input_lock to submit the report to the
>> HID driver.
>>
>> So in short, it's currently impossible for a HID driver to read input from a
>> HID device from inside it's probe function.
>
> Please see commit c849a6143b ("HID: Separate struct hid_device's
> driver_lock into two locks"). It's done exactly in order to allow for
> receiving input inside of the probe() function of the driver.
So that would have been likely my next discovery. ;)
Very nice, this will reduce the whole patch to one line.
I will test it and send an updated patch.
Thanks a lot,
Alexander Holler
The following 2 patches together replace the previous send patch
"rtc: rtc-hid-sensor-time: delay registering as rtc into a work".
They are based on the current master, so they might become included in 3.11.
I've splitted the changes into two patches because the patch changed such, that
combining them into just one patch doesn't make sense anymore.
Regards,
Alexander Holler
Stop processing hid input when registering the RTC fails and handle
a NULL returned from devm_rtc_device_register() as a failure too.
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index 7273b01..1ab3d13 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -283,9 +283,11 @@ static int hid_time_probe(struct platform_device *pdev)
"hid-sensor-time", &hid_time_rtc_ops,
THIS_MODULE);
- if (IS_ERR(time_state->rtc)) {
+ if (IS_ERR_OR_NULL(time_state->rtc)) {
+ ret = time_state->rtc ? PTR_ERR(time_state->rtc) : -ENODEV;
+ time_state->rtc = NULL;
+ sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
dev_err(&pdev->dev, "rtc device register failed!\n");
- return PTR_ERR(time_state->rtc);
}
return ret;
--
1.8.1.4
Enable the processing of HID input records before the RTC will be registered,
in order to allow the RTC register function to read clock. Without doing
that the clock can only be read after the probe function has finished.
Signed-off-by: Alexander Holler <[email protected]>
---
drivers/rtc/rtc-hid-sensor-time.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-sensor-time.c
index 1ab3d13..1006a62 100644
--- a/drivers/rtc/rtc-hid-sensor-time.c
+++ b/drivers/rtc/rtc-hid-sensor-time.c
@@ -279,11 +279,18 @@ static int hid_time_probe(struct platform_device *pdev)
return ret;
}
+ /*
+ * Enable HID input processing early in order to be able to read the
+ * clock already in devm_rtc_device_register().
+ */
+ hid_device_io_start(hsdev->hdev);
+
time_state->rtc = devm_rtc_device_register(&pdev->dev,
"hid-sensor-time", &hid_time_rtc_ops,
THIS_MODULE);
if (IS_ERR_OR_NULL(time_state->rtc)) {
+ hid_device_io_stop(hsdev->hdev);
ret = time_state->rtc ? PTR_ERR(time_state->rtc) : -ENODEV;
time_state->rtc = NULL;
sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
--
1.8.1.4