Comments

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(-)

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

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

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

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

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.

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