| Message ID | 1371724776-5572-1-git-send-email-holler@ahsoftware.de |
|---|---|
| State | Rejected |
| Headers | show |
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?
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
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
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;
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(-)