Messages in this thread | | | Date | Sat, 06 Jul 2013 10:55:10 +0200 | From | Alexander Holler <> | Subject | Re: [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
| |