* [PATCH] HID: magicmouse: Fix race in input mapping. @ 2011-09-16 22:12 Jaikumar Ganesh 2011-09-20 20:58 ` Jaikumar Ganesh 2011-09-20 22:43 ` David Herrmann 0 siblings, 2 replies; 5+ messages in thread From: Jaikumar Ganesh @ 2011-09-16 22:12 UTC (permalink / raw) To: linux-input; +Cc: Jaikumar Ganesh magicmouse_select_input was being called after device registration. Hence, any thread scanning "/dev" for new devices will see the input bits change on an open file descriptor. Fix this by calling select_input inside input_mapping. Based on discussions with Michael Poole <mdpoole@troilus.org> Change-Id: I289a37a850977c2cbbc9eb76fbf6c9a28d1833c5 Signed-off-by: Jaikumar Ganesh <jaikumarg@android.com> --- drivers/hid/hid-magicmouse.c | 13 +++++++------ 1 files changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c index 56d0539..1175452 100644 --- a/drivers/hid/hid-magicmouse.c +++ b/drivers/hid/hid-magicmouse.c @@ -98,6 +98,7 @@ struct magicmouse_sc { int ntouches; int scroll_accel; unsigned long scroll_jiffies; + bool setup_input; struct { short x; @@ -437,6 +438,11 @@ static int magicmouse_input_mapping(struct hid_device *hdev, if (!msc->input) msc->input = hi->input; + if (!msc->setup_input) { + magicmouse_setup_input(msc->input, hdev); + msc->setup_input = true; + } + /* Magic Trackpad does not give relative data after switching to MT */ if (hi->input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD && field->flags & HID_MAIN_ITEM_RELATIVE) @@ -465,6 +471,7 @@ static int magicmouse_probe(struct hid_device *hdev, hid_set_drvdata(hdev, msc); msc->single_touch_id = NO_TOUCHES; + msc->setup_input = false; ret = hid_parse(hdev); if (ret) { @@ -478,12 +485,6 @@ static int magicmouse_probe(struct hid_device *hdev, goto err_free; } - /* We do this after hid-input is done parsing reports so that - * hid-input uses the most natural button and axis IDs. - */ - if (msc->input) - magicmouse_setup_input(msc->input, hdev); - if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE) report = hid_register_report(hdev, HID_INPUT_REPORT, MOUSE_REPORT_ID); -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] HID: magicmouse: Fix race in input mapping. 2011-09-16 22:12 [PATCH] HID: magicmouse: Fix race in input mapping Jaikumar Ganesh @ 2011-09-20 20:58 ` Jaikumar Ganesh 2011-09-20 22:43 ` David Herrmann 1 sibling, 0 replies; 5+ messages in thread From: Jaikumar Ganesh @ 2011-09-20 20:58 UTC (permalink / raw) To: Jaikumar Ganesh; +Cc: linux-input, mdpoole Hello On Fri, Sep 16, 2011 at 3:12 PM, Jaikumar Ganesh <jaikumarg@android.com> wrote: > magicmouse_select_input was being called after device > registration. Hence, any thread scanning "/dev" for new > devices will see the input bits change on an open file > descriptor. Fix this by calling select_input inside > input_mapping. > > Based on discussions with Michael Poole <mdpoole@troilus.org> > > Change-Id: I289a37a850977c2cbbc9eb76fbf6c9a28d1833c5 > Signed-off-by: Jaikumar Ganesh <jaikumarg@android.com> > --- > drivers/hid/hid-magicmouse.c | 13 +++++++------ > 1 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c > index 56d0539..1175452 100644 > --- a/drivers/hid/hid-magicmouse.c > +++ b/drivers/hid/hid-magicmouse.c > @@ -98,6 +98,7 @@ struct magicmouse_sc { > int ntouches; > int scroll_accel; > unsigned long scroll_jiffies; > + bool setup_input; > > struct { > short x; > @@ -437,6 +438,11 @@ static int magicmouse_input_mapping(struct hid_device *hdev, > if (!msc->input) > msc->input = hi->input; > > + if (!msc->setup_input) { > + magicmouse_setup_input(msc->input, hdev); > + msc->setup_input = true; > + } > + > /* Magic Trackpad does not give relative data after switching to MT */ > if (hi->input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD && > field->flags & HID_MAIN_ITEM_RELATIVE) > @@ -465,6 +471,7 @@ static int magicmouse_probe(struct hid_device *hdev, > hid_set_drvdata(hdev, msc); > > msc->single_touch_id = NO_TOUCHES; > + msc->setup_input = false; > > ret = hid_parse(hdev); > if (ret) { > @@ -478,12 +485,6 @@ static int magicmouse_probe(struct hid_device *hdev, > goto err_free; > } > > - /* We do this after hid-input is done parsing reports so that > - * hid-input uses the most natural button and axis IDs. > - */ > - if (msc->input) > - magicmouse_setup_input(msc->input, hdev); > - > if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE) > report = hid_register_report(hdev, HID_INPUT_REPORT, > MOUSE_REPORT_ID); > -- > 1.7.3.1 > Ping for review. > -- > 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 > -- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] HID: magicmouse: Fix race in input mapping. 2011-09-16 22:12 [PATCH] HID: magicmouse: Fix race in input mapping Jaikumar Ganesh 2011-09-20 20:58 ` Jaikumar Ganesh @ 2011-09-20 22:43 ` David Herrmann 2011-09-20 22:57 ` Jaikumar Ganesh 1 sibling, 1 reply; 5+ messages in thread From: David Herrmann @ 2011-09-20 22:43 UTC (permalink / raw) To: Jaikumar Ganesh; +Cc: linux-input, Jiri Kosina Hi Jaikumar On Sat, Sep 17, 2011 at 12:12 AM, Jaikumar Ganesh <jaikumarg@android.com> wrote: > magicmouse_select_input was being called after device This should be "*setup_input" not "*select_input". > registration. Hence, any thread scanning "/dev" for new > devices will see the input bits change on an open file > descriptor. Fix this by calling select_input inside > input_mapping. Isn't input_mapped more appropriate here? Otherwise hidinput_configure_usage could probably overwrite your abs values. > Based on discussions with Michael Poole <mdpoole@troilus.org> > > Change-Id: I289a37a850977c2cbbc9eb76fbf6c9a28d1833c5 > Signed-off-by: Jaikumar Ganesh <jaikumarg@android.com> > --- > drivers/hid/hid-magicmouse.c | 13 +++++++------ > 1 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c > index 56d0539..1175452 100644 > --- a/drivers/hid/hid-magicmouse.c > +++ b/drivers/hid/hid-magicmouse.c > @@ -98,6 +98,7 @@ struct magicmouse_sc { > int ntouches; > int scroll_accel; > unsigned long scroll_jiffies; > + bool setup_input; > > struct { > short x; > @@ -437,6 +438,11 @@ static int magicmouse_input_mapping(struct hid_device *hdev, > if (!msc->input) > msc->input = hi->input; > > + if (!msc->setup_input) { > + magicmouse_setup_input(msc->input, hdev); > + msc->setup_input = true; > + } > + > /* Magic Trackpad does not give relative data after switching to MT */ > if (hi->input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD && > field->flags & HID_MAIN_ITEM_RELATIVE) > @@ -465,6 +471,7 @@ static int magicmouse_probe(struct hid_device *hdev, > hid_set_drvdata(hdev, msc); > > msc->single_touch_id = NO_TOUCHES; > + msc->setup_input = false; > > ret = hid_parse(hdev); > if (ret) { > @@ -478,12 +485,6 @@ static int magicmouse_probe(struct hid_device *hdev, > goto err_free; > } > > - /* We do this after hid-input is done parsing reports so that > - * hid-input uses the most natural button and axis IDs. > - */ > - if (msc->input) > - magicmouse_setup_input(msc->input, hdev); > - > if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE) > report = hid_register_report(hdev, HID_INPUT_REPORT, > MOUSE_REPORT_ID); > -- > 1.7.3.1 > > -- > 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 > I think this is the same problem as: http://thread.gmane.org/gmane.linux.kernel.input/21660/focus=21669 Several drivers need to set additional flags on the input device. I'd recommend an "input_register" callback that is called after hidinput_configure_usage() is called but before the input device is registered. But it is only called once and not for every hid-descr field. Then we could skip the "msc->setup_input" boolean flag. However, until then this fix seems fine, although I'd use input_mapped instead of input_mapping. 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] HID: magicmouse: Fix race in input mapping. 2011-09-20 22:43 ` David Herrmann @ 2011-09-20 22:57 ` Jaikumar Ganesh 2011-09-21 0:14 ` Jaikumar Ganesh 0 siblings, 1 reply; 5+ messages in thread From: Jaikumar Ganesh @ 2011-09-20 22:57 UTC (permalink / raw) To: David Herrmann; +Cc: linux-input, Jiri Kosina Hi David: On Tue, Sep 20, 2011 at 3:43 PM, David Herrmann <dh.herrmann@googlemail.com> wrote: > Hi Jaikumar > > On Sat, Sep 17, 2011 at 12:12 AM, Jaikumar Ganesh <jaikumarg@android.com> wrote: >> magicmouse_select_input was being called after device > > This should be "*setup_input" not "*select_input". > >> registration. Hence, any thread scanning "/dev" for new >> devices will see the input bits change on an open file >> descriptor. Fix this by calling select_input inside >> input_mapping. > > Isn't input_mapped more appropriate here? Otherwise > hidinput_configure_usage could probably overwrite your abs values. > >> Based on discussions with Michael Poole <mdpoole@troilus.org> >> >> Change-Id: I289a37a850977c2cbbc9eb76fbf6c9a28d1833c5 >> Signed-off-by: Jaikumar Ganesh <jaikumarg@android.com> >> --- >> drivers/hid/hid-magicmouse.c | 13 +++++++------ >> 1 files changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c >> index 56d0539..1175452 100644 >> --- a/drivers/hid/hid-magicmouse.c >> +++ b/drivers/hid/hid-magicmouse.c >> @@ -98,6 +98,7 @@ struct magicmouse_sc { >> int ntouches; >> int scroll_accel; >> unsigned long scroll_jiffies; >> + bool setup_input; >> >> struct { >> short x; >> @@ -437,6 +438,11 @@ static int magicmouse_input_mapping(struct hid_device *hdev, >> if (!msc->input) >> msc->input = hi->input; >> >> + if (!msc->setup_input) { >> + magicmouse_setup_input(msc->input, hdev); >> + msc->setup_input = true; >> + } >> + >> /* Magic Trackpad does not give relative data after switching to MT */ >> if (hi->input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD && >> field->flags & HID_MAIN_ITEM_RELATIVE) >> @@ -465,6 +471,7 @@ static int magicmouse_probe(struct hid_device *hdev, >> hid_set_drvdata(hdev, msc); >> >> msc->single_touch_id = NO_TOUCHES; >> + msc->setup_input = false; >> >> ret = hid_parse(hdev); >> if (ret) { >> @@ -478,12 +485,6 @@ static int magicmouse_probe(struct hid_device *hdev, >> goto err_free; >> } >> >> - /* We do this after hid-input is done parsing reports so that >> - * hid-input uses the most natural button and axis IDs. >> - */ >> - if (msc->input) >> - magicmouse_setup_input(msc->input, hdev); >> - >> if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE) >> report = hid_register_report(hdev, HID_INPUT_REPORT, >> MOUSE_REPORT_ID); >> -- >> 1.7.3.1 >> >> -- >> 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 >> > > I think this is the same problem as: > http://thread.gmane.org/gmane.linux.kernel.input/21660/focus=21669 > > Several drivers need to set additional flags on the input device. I'd > recommend an "input_register" callback that is called after > hidinput_configure_usage() is called but before the input device is > registered. But it is only called once and not for every hid-descr > field. Then we could skip the "msc->setup_input" boolean flag. > However, until then this fix seems fine, although I'd use input_mapped > instead of input_mapping. This was one of my suggestions too in this thread" "http://thread.gmane.org/gmane.linux.kernel.input/21604/" This needs to be discussed on a more general basis since it will affect many drivers. > > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] HID: magicmouse: Fix race in input mapping. 2011-09-20 22:57 ` Jaikumar Ganesh @ 2011-09-21 0:14 ` Jaikumar Ganesh 0 siblings, 0 replies; 5+ messages in thread From: Jaikumar Ganesh @ 2011-09-21 0:14 UTC (permalink / raw) To: Jaikumar Ganesh; +Cc: David Herrmann, linux-input, Jiri Kosina Hi David, On Tue, Sep 20, 2011 at 3:57 PM, Jaikumar Ganesh <jaikumarg@android.com> wrote: > Hi David: > > On Tue, Sep 20, 2011 at 3:43 PM, David Herrmann > <dh.herrmann@googlemail.com> wrote: >> Hi Jaikumar >> >> On Sat, Sep 17, 2011 at 12:12 AM, Jaikumar Ganesh <jaikumarg@android.com> wrote: >>> magicmouse_select_input was being called after device >> >> This should be "*setup_input" not "*select_input". >> >>> registration. Hence, any thread scanning "/dev" for new >>> devices will see the input bits change on an open file >>> descriptor. Fix this by calling select_input inside >>> input_mapping. >> >> Isn't input_mapped more appropriate here? Otherwise >> hidinput_configure_usage could probably overwrite your abs values. >> >>> Based on discussions with Michael Poole <mdpoole@troilus.org> >>> >>> Change-Id: I289a37a850977c2cbbc9eb76fbf6c9a28d1833c5 >>> Signed-off-by: Jaikumar Ganesh <jaikumarg@android.com> >>> --- >>> drivers/hid/hid-magicmouse.c | 13 +++++++------ >>> 1 files changed, 7 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c >>> index 56d0539..1175452 100644 >>> --- a/drivers/hid/hid-magicmouse.c >>> +++ b/drivers/hid/hid-magicmouse.c >>> @@ -98,6 +98,7 @@ struct magicmouse_sc { >>> int ntouches; >>> int scroll_accel; >>> unsigned long scroll_jiffies; >>> + bool setup_input; >>> >>> struct { >>> short x; >>> @@ -437,6 +438,11 @@ static int magicmouse_input_mapping(struct hid_device *hdev, >>> if (!msc->input) >>> msc->input = hi->input; >>> >>> + if (!msc->setup_input) { >>> + magicmouse_setup_input(msc->input, hdev); >>> + msc->setup_input = true; >>> + } >>> + >>> /* Magic Trackpad does not give relative data after switching to MT */ >>> if (hi->input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD && >>> field->flags & HID_MAIN_ITEM_RELATIVE) >>> @@ -465,6 +471,7 @@ static int magicmouse_probe(struct hid_device *hdev, >>> hid_set_drvdata(hdev, msc); >>> >>> msc->single_touch_id = NO_TOUCHES; >>> + msc->setup_input = false; >>> >>> ret = hid_parse(hdev); >>> if (ret) { >>> @@ -478,12 +485,6 @@ static int magicmouse_probe(struct hid_device *hdev, >>> goto err_free; >>> } >>> >>> - /* We do this after hid-input is done parsing reports so that >>> - * hid-input uses the most natural button and axis IDs. >>> - */ >>> - if (msc->input) >>> - magicmouse_setup_input(msc->input, hdev); >>> - >>> if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE) >>> report = hid_register_report(hdev, HID_INPUT_REPORT, >>> MOUSE_REPORT_ID); >>> -- >>> 1.7.3.1 >>> >>> -- >>> 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 >>> >> >> I think this is the same problem as: >> http://thread.gmane.org/gmane.linux.kernel.input/21660/focus=21669 >> >> Several drivers need to set additional flags on the input device. I'd >> recommend an "input_register" callback that is called after >> hidinput_configure_usage() is called but before the input device is >> registered. But it is only called once and not for every hid-descr >> field. Then we could skip the "msc->setup_input" boolean flag. >> However, until then this fix seems fine, although I'd use input_mapped >> instead of input_mapping. > > This was one of my suggestions too in this thread" > "http://thread.gmane.org/gmane.linux.kernel.input/21604/" > > This needs to be discussed on a more general basis since it will > affect many drivers. > It will be good to have this callback added now. I have sent another patch to the mailing list, Have a look. Thanks Jaikumar >> >> 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 > -- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-09-21 0:14 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-09-16 22:12 [PATCH] HID: magicmouse: Fix race in input mapping Jaikumar Ganesh 2011-09-20 20:58 ` Jaikumar Ganesh 2011-09-20 22:43 ` David Herrmann 2011-09-20 22:57 ` Jaikumar Ganesh 2011-09-21 0:14 ` Jaikumar Ganesh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).