lkml.org 
[lkml]   [2013]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [rtc-linux] Re: [PATCH 3/9 v2] rtc: rtc-hid-sensor-time: delay registering as rtc into a work
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


\
 
 \ /
  Last update: 2013-07-06 11:41    [W:0.139 / U:4.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site