Patchwork [3/9,v2] rtc: rtc-hid-sensor-time: delay registering as rtc into a work

login
register
mail settings
Submitter Alexander Holler
Date June 20, 2013, 10:39 a.m.
Message ID <1371724776-5572-1-git-send-email-holler@ahsoftware.de>
Download mbox | patch
Permalink /patch/252866/
State New
Headers show

Comments

Alexander Holler - June 20, 2013, 10:39 a.m.
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 <holler@ahsoftware.de>
---
 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(-)
Andrew Morton - June 26, 2013, 7:55 p.m.
On Thu, 20 Jun 2013 12:39:36 +0200 Alexander Holler <holler@ahsoftware.de> 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?
Alexander Holler - June 26, 2013, 9:34 p.m.
Am 26.06.2013 21:55, schrieb Andrew Morton:
> On Thu, 20 Jun 2013 12:39:36 +0200 Alexander Holler <holler@ahsoftware.de> 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
Greg KH - June 26, 2013, 10:07 p.m.
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
Alexander Holler - June 26, 2013, 11:51 p.m.
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
Alexander Holler - June 28, 2013, 1:29 a.m.
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 <holler@ahsoftware.de> 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
Alexander Holler - July 6, 2013, 8:55 a.m.
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
Jiri Kosina - July 6, 2013, 6:21 p.m.
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.
Alexander Holler - July 7, 2013, 7:35 a.m.
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

Patch

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;