[PATCH resend] hid-magicmouse: Increase evdev buffer size

[PATCH resend] hid-magicmouse: Increase evdev buffer size

[Chase Douglas]

The evdev buffer isn't big enough when you get many fingers on the
device. Bump up the buffer to a reasonable size, matching what other
multitouch devices use. Without this change, events may be discarded in
the evdev buffer before they are read.

Reported-by: Simon Budig <simon@budig.de>
Cc: Henrik Rydberg <rydberg@euromail.se>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: stable@kernel.org
Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
Forgot to Cc the mailing lists on the first send.

 drivers/hid/hid-magicmouse.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 698e645..404dcbc 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -418,6 +418,8 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
 			input_set_abs_params(input, ABS_MT_POSITION_Y, -2456,
 				2565, 4, 0);
 		}
+
+		input_set_events_per_packet(input, 60);
 	}
 
 	if (report_undeciphered) {
-- 
1.7.4.1

Re: [PATCH resend] hid-magicmouse: Increase evdev buffer size

[Jeffrey Brown]

I've got another change in the works that fixes this problem more
systematically.

On Fri, Apr 1, 2011 at 2:03 PM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> The evdev buffer isn't big enough when you get many fingers on the
> device. Bump up the buffer to a reasonable size, matching what other
> multitouch devices use. Without this change, events may be discarded in
> the evdev buffer before they are read.
>
> Reported-by: Simon Budig <simon@budig.de>
> Cc: Henrik Rydberg <rydberg@euromail.se>
> Cc: Jiri Kosina <jkosina@suse.cz>
> Cc: stable@kernel.org
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
> Forgot to Cc the mailing lists on the first send.
>
>  drivers/hid/hid-magicmouse.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 698e645..404dcbc 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -418,6 +418,8 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
>                        input_set_abs_params(input, ABS_MT_POSITION_Y, -2456,
>                                2565, 4, 0);
>                }
> +
> +               input_set_events_per_packet(input, 60);
>        }
>
>        if (report_undeciphered) {
> --
> 1.7.4.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
>

Re: [PATCH resend] hid-magicmouse: Increase evdev buffer size

[Chase Douglas]

On 04/01/2011 07:51 PM, Jeffrey Brown wrote:
> I've got another change in the works that fixes this problem more
> systematically.

That would be great :). I still would like to see this patch added so
it's released with the 2.6.39 kernel and backported to earlier stable
releases. I hope that's not a problem.

Thanks,

-- Chase

> On Fri, Apr 1, 2011 at 2:03 PM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
>> The evdev buffer isn't big enough when you get many fingers on the
>> device. Bump up the buffer to a reasonable size, matching what other
>> multitouch devices use. Without this change, events may be discarded in
>> the evdev buffer before they are read.
>>
>> Reported-by: Simon Budig <simon@budig.de>
>> Cc: Henrik Rydberg <rydberg@euromail.se>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>> Cc: stable@kernel.org
>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>> ---
>> Forgot to Cc the mailing lists on the first send.
>>
>>  drivers/hid/hid-magicmouse.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
>> index 698e645..404dcbc 100644
>> --- a/drivers/hid/hid-magicmouse.c
>> +++ b/drivers/hid/hid-magicmouse.c
>> @@ -418,6 +418,8 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
>>                        input_set_abs_params(input, ABS_MT_POSITION_Y, -2456,
>>                                2565, 4, 0);
>>                }
>> +
>> +               input_set_events_per_packet(input, 60);
>>        }
>>
>>        if (report_undeciphered) {
>> --
>> 1.7.4.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
>>
> 

Re: [PATCH resend] hid-magicmouse: Increase evdev buffer size

[Henrik Rydberg]

On Fri, Apr 01, 2011 at 05:03:39PM -0400, Chase Douglas wrote:
> The evdev buffer isn't big enough when you get many fingers on the
> device. Bump up the buffer to a reasonable size, matching what other
> multitouch devices use. Without this change, events may be discarded in
> the evdev buffer before they are read.
> 
> Reported-by: Simon Budig <simon@budig.de>
> Cc: Henrik Rydberg <rydberg@euromail.se>
> Cc: Jiri Kosina <jkosina@suse.cz>
> Cc: stable@kernel.org
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---

Acked-by: Henrik Rydberg <rydberg@euromail.se>

Thanks, Chase.

Henrik

Re: [PATCH resend] hid-magicmouse: Increase evdev buffer size

[Simon Budig]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/02/2011 06:36 PM, Chase Douglas wrote:
> On 04/01/2011 07:51 PM, Jeffrey Brown wrote:
>> I've got another change in the works that fixes this problem more
>> systematically.
> 
> That would be great :). I still would like to see this patch added so
> it's released with the 2.6.39 kernel and backported to earlier stable
> releases. I hope that's not a problem.

Wouldn't it make sense to change the buffer logic in the input layer
that in the case of a buffer overrun the head gets pushed forward to the
next SYN_REPORT?

One symptom I witnessed with the bug was, that I got partial reports,
which especially in the case of a multitouch report (A-Type) creates
false release-events. This would be solved by the head-forwarding.

(one could even think about more fancy stuff like combining relative
events and keeping track of the absolute events, but this seems a bit
like overkill)

Thanks,
        Simon
- -- 
       Simon Budig                        kernel concepts GbR
       simon.budig@kernelconcepts.de      Sieghuetter Hauptweg 48
       +49-271-771091-17                  D-57072 Siegen

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2Zt4UACgkQO2O/RXesiHByIgCgmHsujFhsYcM0wp7qEl+HytNh
wSMAoLygXVkU1Lx+jNgBA++H8OL6dNVR
=9+3z
-----END PGP SIGNATURE-----

Re: [PATCH resend] hid-magicmouse: Increase evdev buffer size

[Jiri Kosina]

On Fri, 1 Apr 2011, Jeffrey Brown wrote:

> I've got another change in the works that fixes this problem more
> systematically.

Could you just briefly describe what your solution is?

I'd happuly take Chase's patch, but want to make sure that we don't cause 
any changes that would make backwards compatilibity an issue later.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH resend] hid-magicmouse: Increase evdev buffer size

[Jeffrey Brown]

Sorry, I originally sent a reply to the list from my phone but it was
HTML formatted for some reason and the list server rejected it.  Meh.

On Mon, Apr 4, 2011 at 5:43 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> Could you just briefly describe what your solution is?

I have sent a series of 4 patches to linux-input:

* [PATCH v2 1/4] input: Set default events per packet.

This patch modifies input_register_device to calculate the number of
events per packet if the driver did not already supply a hint.
Roughly speaking, it examines the number of MT_SLOTs or
ABS_MT_TRACKING_IDs and multiplies that by the total number of MT
axes, then adds in a bit for non-MT axes, REL axes and SYN events.

The intent is to systematically solve the problem of determining the
number of events per packet so that most drivers won't need to set
hints themselves.  This fixes the magic mouse issue and related
problems in other drivers.

* [PATCH v2 2/4] hid: hid-input: Remove obsolete default events per
packet setting.

Given the first patch, the HID driver no longer needs to hardcode a
default events per packet value.

* [PATCH v2 3/4] input: evdev: Indicate buffer overrun with SYN_DROPPED.

I first discovered that the evdev buffer was too small because of the
Apple Magic Trackpad.  With more than 3 fingers, some points would
regularly get dropped and would interrupt the gestures I was trying to
detect.  User space code got really confused since there was no
indication that anything unusual had happened and the event streams
effectively appeared to be truncated and concatenated arbitrarily.

So this patch introduces a new type of SYN event code, SYN_DROPPED, to
mark the point in the event stream where events were dropped.  The
most recent version of this patch also takes care to drop the entire
last packet, so the next event received by user-space will be the
first event of the following compete packet.

* [PATCH v2 4/4] input: evdev: Make device readable only when it
contains a complete packet.

There is a significant performance issue on SMP systems where the
driver and user-space code can race against one another to fill and
drain the evdev buffer in parallel.  It is possible for a client
waiting on poll() to wake up and read each event published by the
driver immediately as it becomes available.  That could mean 60 or
more calls to poll() and read() to read an entire packet.  It would be
preferable if the client were only awoken when the entire packet were
available so it could just make 1 call to poll() and read() everything
all at once.

No changes are required on the part of the client.  This patch changes
when evdev considers itself readable.  Essentially evdev is made to be
readable only up to the last non-MT SYN event it contains.  That way
the client can only read events that belong to packets which have been
completely buffered.  The client can still read events one at a time
if it likes.  The client will only wake from poll() when the buffer
contains a non-MT SYN.

This patch works well in combination with the SYN_DROPPED change above.

> I'd happuly take Chase's patch, but want to make sure that we don't cause
> any changes that would make backwards compatilibity an issue later.

There should be no compatibility issues.  However, we might be better
off in the long term taking (some variation of) these patches instead.

Jeff.

Re: [PATCH resend] hid-magicmouse: Increase evdev buffer size

[Chase Douglas]

On 04/04/2011 02:13 PM, Jeffrey Brown wrote:
> On Mon, Apr 4, 2011 at 5:43 AM, Jiri Kosina <jkosina@suse.cz> wrote:
>> I'd happuly take Chase's patch, but want to make sure that we don't cause
>> any changes that would make backwards compatilibity an issue later.
> 
> There should be no compatibility issues.  However, we might be better
> off in the long term taking (some variation of) these patches instead.

I like the proposed changes, but I want to ensure stable kernel releases
aren't left out of the fix for hid-magicmouse. I don't know the best way
forward, but here's one possibility:

1. Apply my patch to manually set the buffer size hint
2. It gets sent to stable trees due to the 'Cc: stable@kernel.org' line
3. Apply Jeffrey's patches, including a reversion of my buffer size hint

Obviously the extra application and reversion is odd, but this seems the
easiest way forward given that the patches already exist and can be
applied without issue.

Thanks,

-- Chase

Re: [PATCH resend] hid-magicmouse: Increase evdev buffer size

[Dmitry Torokhov]

On Mon, Apr 04, 2011 at 02:55:03PM -0400, Chase Douglas wrote:
> On 04/04/2011 02:13 PM, Jeffrey Brown wrote:
> > On Mon, Apr 4, 2011 at 5:43 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> >> I'd happuly take Chase's patch, but want to make sure that we don't cause
> >> any changes that would make backwards compatilibity an issue later.
> > 
> > There should be no compatibility issues.  However, we might be better
> > off in the long term taking (some variation of) these patches instead.
> 
> I like the proposed changes, but I want to ensure stable kernel releases
> aren't left out of the fix for hid-magicmouse. I don't know the best way
> forward, but here's one possibility:
> 
> 1. Apply my patch to manually set the buffer size hint
> 2. It gets sent to stable trees due to the 'Cc: stable@kernel.org' line
> 3. Apply Jeffrey's patches, including a reversion of my buffer size hint
> 
> Obviously the extra application and reversion is odd, but this seems the
> easiest way forward given that the patches already exist and can be
> applied without issue.
> 

Or, once Jeffrey's patches hit mainline, send your change to stable with
the explanation why it is needed for stable but not for mainline.

Thanks.

-- 
Dmitry

Re: [PATCH resend] hid-magicmouse: Increase evdev buffer size

[Chase Douglas]

On 04/04/2011 05:39 PM, Dmitry Torokhov wrote:
> On Mon, Apr 04, 2011 at 02:55:03PM -0400, Chase Douglas wrote:
>> On 04/04/2011 02:13 PM, Jeffrey Brown wrote:
>>> On Mon, Apr 4, 2011 at 5:43 AM, Jiri Kosina <jkosina@suse.cz> wrote:
>>>> I'd happuly take Chase's patch, but want to make sure that we don't cause
>>>> any changes that would make backwards compatilibity an issue later.
>>>
>>> There should be no compatibility issues.  However, we might be better
>>> off in the long term taking (some variation of) these patches instead.
>>
>> I like the proposed changes, but I want to ensure stable kernel releases
>> aren't left out of the fix for hid-magicmouse. I don't know the best way
>> forward, but here's one possibility:
>>
>> 1. Apply my patch to manually set the buffer size hint
>> 2. It gets sent to stable trees due to the 'Cc: stable@kernel.org' line
>> 3. Apply Jeffrey's patches, including a reversion of my buffer size hint
>>
>> Obviously the extra application and reversion is odd, but this seems the
>> easiest way forward given that the patches already exist and can be
>> applied without issue.
>>
> 
> Or, once Jeffrey's patches hit mainline, send your change to stable with
> the explanation why it is needed for stable but not for mainline.

I'm fine with this too. I'll watch the list to see what happens.

Thanks,

-- Chase

Re: [PATCH resend] hid-magicmouse: Increase evdev buffer size

[Chase Douglas]

On 04/04/2011 05:39 PM, Dmitry Torokhov wrote:
> On Mon, Apr 04, 2011 at 02:55:03PM -0400, Chase Douglas wrote:
>> On 04/04/2011 02:13 PM, Jeffrey Brown wrote:
>>> On Mon, Apr 4, 2011 at 5:43 AM, Jiri Kosina <jkosina@suse.cz> wrote:
>>>> I'd happuly take Chase's patch, but want to make sure that we don't cause
>>>> any changes that would make backwards compatilibity an issue later.
>>>
>>> There should be no compatibility issues.  However, we might be better
>>> off in the long term taking (some variation of) these patches instead.
>>
>> I like the proposed changes, but I want to ensure stable kernel releases
>> aren't left out of the fix for hid-magicmouse. I don't know the best way
>> forward, but here's one possibility:
>>
>> 1. Apply my patch to manually set the buffer size hint
>> 2. It gets sent to stable trees due to the 'Cc: stable@kernel.org' line
>> 3. Apply Jeffrey's patches, including a reversion of my buffer size hint
>>
>> Obviously the extra application and reversion is odd, but this seems the
>> easiest way forward given that the patches already exist and can be
>> applied without issue.
>>
> 
> Or, once Jeffrey's patches hit mainline, send your change to stable with
> the explanation why it is needed for stable but not for mainline.

One thing that crossed my mind is that Jeffrey's patches wouldn't be
merged until 2.6.40, right? If so, even 2.6.39 will be released with
this bug, which makes me want to go with the plan I outlined above.

Thanks,

-- Chase

Re: [PATCH resend] hid-magicmouse: Increase evdev buffer size

[Jiri Kosina]

On Tue, 5 Apr 2011, Chase Douglas wrote:

> >>>> I'd happuly take Chase's patch, but want to make sure that we don't cause
> >>>> any changes that would make backwards compatilibity an issue later.
> >>>
> >>> There should be no compatibility issues.  However, we might be better
> >>> off in the long term taking (some variation of) these patches instead.
> >>
> >> I like the proposed changes, but I want to ensure stable kernel releases
> >> aren't left out of the fix for hid-magicmouse. I don't know the best way
> >> forward, but here's one possibility:
> >>
> >> 1. Apply my patch to manually set the buffer size hint
> >> 2. It gets sent to stable trees due to the 'Cc: stable@kernel.org' line
> >> 3. Apply Jeffrey's patches, including a reversion of my buffer size hint
> >>
> >> Obviously the extra application and reversion is odd, but this seems the
> >> easiest way forward given that the patches already exist and can be
> >> applied without issue.
> >>
> > 
> > Or, once Jeffrey's patches hit mainline, send your change to stable with
> > the explanation why it is needed for stable but not for mainline.
> 
> One thing that crossed my mind is that Jeffrey's patches wouldn't be
> merged until 2.6.40, right? If so, even 2.6.39 will be released with
> this bug, which makes me want to go with the plan I outlined above.

OK, applied. Thanks everybody.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.