* [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).