Re: [rtc-linux] Re: [PATCH 3/9 v2] rtc: rtc-hid-sensor-time: delayregistering as rtc into a work

[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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/