| Message ID | 1309185013-484-5-git-send-email-dh.herrmann@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
David Herrmann <dh.herrmann@googlemail.com> wrote: >The wiimote first starts HID hardware and then registers the input >device. We need to synchronize the startup so no event handler will >start parsing events when the wiimote device is not ready, yet. > I believe this is generic HID layer problem. Would it be possible to fix this issue there? Thanks.
On Tue, Jun 28, 2011 at 7:49 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > David Herrmann <dh.herrmann@googlemail.com> wrote: > >>The wiimote first starts HID hardware and then registers the input >>device. We need to synchronize the startup so no event handler will >>start parsing events when the wiimote device is not ready, yet. >> > > I believe this is generic HID layer problem. Would it be possible to fix this issue there? The point is, I do not use HIDINPUT/HIDDEV (see commit message 2/12). The wiimote has no valid report-descriptor table so hidinput/hiddev do not make sense. I could set up a valid report-descriptor-table, use input_mapping, input_mapped, report_fixup callbacks and then use HIDINPUT. However, that would be far more complex than my current solution, which is creating my own input instance and also my own synchronization. Furthermore, the wiimote provides input/output features that cannot be handled by the generic HID handler, so I would need to extend the input_mapping, anyway. But, thanks to your hint, I looked at hid_hw_start() again and as far as I can see the synchronization issue exists there, too. The ll_driver->start() function is called first and after that, the input instance is registered. If the ll_driver reports an event before the input instance is registered, the hidinput/other-driver will call input_event() on an invalid input instance (probably even a NULL dereference). This may not be a problem for usbhid, because usbhid_open() has to be called first, however, hidp (bluetooth hid) ignores the *_open() callback and therefore may fail. I will look at this again and send a separate mail to this list, however, I don't think this is related to my driver? Or do you think I should replace my input-handling with HIDINPUT to be more homogeneous with the other hid drivers? > Thanks. > > -- > Dmitry Regards David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 29 Jun 2011, David Herrmann wrote: > But, thanks to your hint, I looked at hid_hw_start() again and as far > as I can see the synchronization issue exists there, too. > The ll_driver->start() function is called first and after that, the > input instance is registered. > If the ll_driver reports an event before the input instance is > registered, the hidinput/other-driver will call input_event() on an > invalid input instance (probably even a NULL dereference). > This may not be a problem for usbhid, because usbhid_open() has to be > called first, however, hidp (bluetooth hid) ignores the *_open() > callback and therefore may fail. Indeed, this looks like a proper analysis. Thanks a lot for looking into it. Are you planning on submitting the fix for that? Otherwise I'll add it to my TODO list, independently on the wiimote driver. Thanks,
diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c index deaf232..3416f69 100644 --- a/drivers/hid/hid-wiimote.c +++ b/drivers/hid/hid-wiimote.c @@ -10,6 +10,7 @@ * any later version. */ +#include <linux/atomic.h> #include <linux/device.h> #include <linux/hid.h> #include <linux/input.h> @@ -20,6 +21,7 @@ #define WIIMOTE_NAME "Nintendo Wii Remote" struct wiimote_data { + atomic_t ready; struct hid_device *hdev; struct input_dev *input; }; @@ -27,12 +29,26 @@ struct wiimote_data { static int wiimote_input_event(struct input_dev *dev, unsigned int type, unsigned int code, int value) { + struct wiimote_data *wdata = input_get_drvdata(dev); + + if (!atomic_read(&wdata->ready)) + return -EBUSY; + /* smp_rmb: Make sure wdata->xy is available when wdata->ready is 1 */ + smp_rmb(); + return 0; } static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report, u8 *raw_data, int size) { + struct wiimote_data *wdata = hid_get_drvdata(hdev); + + if (!atomic_read(&wdata->ready)) + return -EBUSY; + /* smp_rmb: Make sure wdata->xy is available when wdata->ready is 1 */ + smp_rmb(); + if (size < 1) return -EINVAL; @@ -103,6 +119,9 @@ static int wiimote_hid_probe(struct hid_device *hdev, goto err_stop; } + /* smp_wmb: Write wdata->xy first before wdata->ready is set to 1 */ + smp_wmb(); + atomic_set(&wdata->ready, 1); hid_info(hdev, "New device registered\n"); return 0;
The wiimote first starts HID hardware and then registers the input device. We need to synchronize the startup so no event handler will start parsing events when the wiimote device is not ready, yet. Signed-off-by: David Herrmann <dh.herrmann@googlemail.com> --- drivers/hid/hid-wiimote.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-)