| Message ID | 1366384452-14367-4-git-send-email-holler@ahsoftware.de |
|---|---|
| State | Rejected |
| Headers | show |
On Fri, 19 Apr 2013 17:14:12 +0200 Alexander Holler <holler@ahsoftware.de> 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 <holler@ahsoftware.de> 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 >> <holler@ahsoftware.de> 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 >>> <holler@ahsoftware.de> 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 >>>> <holler@ahsoftware.de> 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 >>>>> <holler@ahsoftware.de> 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 <holler@ahsoftware.de> 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 <holler@ahsoftware.de> 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
On Sun, 5 May 2013 13:21:26 +0200 Alexander Holler <holler@ahsoftware.de> 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
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
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; }
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 <holler@ahsoftware.de> --- drivers/rtc/rtc-hid-sensor-time.c | 72 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+)