diff mbox

[GIT] HID for 3.12 merge window

Message ID CANq1E4Rf4c4zj1mD1aPr93Q2_M65DCEJhuLqcBWa5uH9tcKuWA@mail.gmail.com (mailing list archive)
State New, archived
Delegated to: Jiri Kosina
Headers show

Commit Message

David Herrmann Sept. 6, 2013, 9:50 p.m. UTC
Hi

On Fri, Sep 6, 2013 at 10:20 PM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2013.09.06 at 14:00 +0200, Jiri Kosina wrote:
>>
>> David Herrmann (12):
> ...
>>       HID: wiimote: add support for Guitar-Hero drums
>
>  commit 61e00655e9cb82e034eb72b95a51072e718d14a7
>  Author: David Herrmann <dh.herrmann@gmail.com>
>  Date:   Mon Aug 26 19:14:46 2013 +0200
>
>      Input: introduce BTN/ABS bits for drums and guitars
>
> The commit above breaks my Logitech mouse. The mouse cursor just sits in
> the middle of the screen and doesn't react to movements. dmesg is
> normal, but Xorg.0.log says:

Ok, the issue is the kernel assumes ABS_MAX to be a power-of-2 minus 1
(used as mask). That wasn't really obvious to me. Attached is a patch
which should fix that. Could you apply it on top of linus/master and
give it a try?

@Dmitry: The IOC_NR part of the definition of EVIOCSABS() is now
bigger than 1-byte. I need to check how that affects the 'E' part. Any
idea what to do here?

Thanks
David

Patch is also attached as I doubt that inlining it works in that
stupid web-client:

From 653fe4d46ad368cdbf9b56a559a8468bd6f5cb3c Mon Sep 17 00:00:00 2001
From: David Herrmann <dh.herrmann@gmail.com>
Date: Fri, 6 Sep 2013 23:46:08 +0200
Subject: [PATCH] Input: evdev: don't assume ABS_MAX to be a power-of-2 minus 1

ABS_MAX is no longer a full mask. Hence, don't use it directly to get any
parameter for ioctls. Furthermore, the parameter-region and
ioctl-definition overlap, so even bumping ABS_MAX to 0x7f wouldn't help.

Reported-by: Markus Trippelsdorf <markus@trippelsdorf.de>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/input/evdev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

  if (copy_to_user(p, &abs, min_t(size_t,
@@ -957,12 +958,13 @@ static long evdev_do_ioctl(struct file *file,
unsigned int cmd,

  if (_IOC_DIR(cmd) == _IOC_WRITE) {

- if ((_IOC_NR(cmd) & ~ABS_MAX) == _IOC_NR(EVIOCSABS(0))) {
+ if (_IOC_NR(cmd) >= _IOC_NR(EVIOCSABS(0)) &&
+    _IOC_NR(cmd) <= _IOC_NR(EVIOCSABS(ABS_MAX))) {

  if (!dev->absinfo)
  return -EINVAL;

- t = _IOC_NR(cmd) & ABS_MAX;
+ t = _IOC_NR(cmd) - _IOC_NR(EVIOCSABS(0));

  if (copy_from_user(&abs, p, min_t(size_t,
  size, sizeof(struct input_absinfo))))

Comments

Markus Trippelsdorf Sept. 6, 2013, 9:59 p.m. UTC | #1
On 2013.09.06 at 23:50 +0200, David Herrmann wrote:
> Hi
> 
> On Fri, Sep 6, 2013 at 10:20 PM, Markus Trippelsdorf
> <markus@trippelsdorf.de> wrote:
> > On 2013.09.06 at 14:00 +0200, Jiri Kosina wrote:
> >>
> >> David Herrmann (12):
> > ...
> >>       HID: wiimote: add support for Guitar-Hero drums
> >
> >  commit 61e00655e9cb82e034eb72b95a51072e718d14a7
> >  Author: David Herrmann <dh.herrmann@gmail.com>
> >  Date:   Mon Aug 26 19:14:46 2013 +0200
> >
> >      Input: introduce BTN/ABS bits for drums and guitars
> >
> > The commit above breaks my Logitech mouse. The mouse cursor just sits in
> > the middle of the screen and doesn't react to movements. dmesg is
> > normal, but Xorg.0.log says:
> 
> Ok, the issue is the kernel assumes ABS_MAX to be a power-of-2 minus 1
> (used as mask). That wasn't really obvious to me. Attached is a patch
> which should fix that. Could you apply it on top of linus/master and
> give it a try?

Your patch fixes the issue. Thanks.
David Herrmann Sept. 6, 2013, 10:51 p.m. UTC | #2
Hi

On Fri, Sep 6, 2013 at 11:59 PM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2013.09.06 at 23:50 +0200, David Herrmann wrote:
>> Hi
>>
>> On Fri, Sep 6, 2013 at 10:20 PM, Markus Trippelsdorf
>> <markus@trippelsdorf.de> wrote:
>> > On 2013.09.06 at 14:00 +0200, Jiri Kosina wrote:
>> >>
>> >> David Herrmann (12):
>> > ...
>> >>       HID: wiimote: add support for Guitar-Hero drums
>> >
>> >  commit 61e00655e9cb82e034eb72b95a51072e718d14a7
>> >  Author: David Herrmann <dh.herrmann@gmail.com>
>> >  Date:   Mon Aug 26 19:14:46 2013 +0200
>> >
>> >      Input: introduce BTN/ABS bits for drums and guitars
>> >
>> > The commit above breaks my Logitech mouse. The mouse cursor just sits in
>> > the middle of the screen and doesn't react to movements. dmesg is
>> > normal, but Xorg.0.log says:
>>
>> Ok, the issue is the kernel assumes ABS_MAX to be a power-of-2 minus 1
>> (used as mask). That wasn't really obvious to me. Attached is a patch
>> which should fix that. Could you apply it on top of linus/master and
>> give it a try?
>
> Your patch fixes the issue. Thanks.

Thanks a lot for reporting+testing!

I am still not sure how to solve the EVIOCSABS thingy. Problem is,
it's defined as:
  #define EVIOCSABS(_abs) ...0xc0 + (_abs)...
But if (_abs > 0x3f) this will be bigger than 0xff. Unfortunately, the
upper part of the ioctl is defined as 'E' which is 0x45 in hex and
thus sets the LSB. That means we cannot extend the _IOC_TYPE field to
the upper region (which would cause endian-issues, anyway). I guess
we're screwed here and need to revert that...

Dmitry, any comment on this? Or am I missing something?

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
Dmitry Torokhov Sept. 6, 2013, 11:10 p.m. UTC | #3
On Sat, Sep 07, 2013 at 12:51:27AM +0200, David Herrmann wrote:
> Hi
> 
> On Fri, Sep 6, 2013 at 11:59 PM, Markus Trippelsdorf
> <markus@trippelsdorf.de> wrote:
> > On 2013.09.06 at 23:50 +0200, David Herrmann wrote:
> >> Hi
> >>
> >> On Fri, Sep 6, 2013 at 10:20 PM, Markus Trippelsdorf
> >> <markus@trippelsdorf.de> wrote:
> >> > On 2013.09.06 at 14:00 +0200, Jiri Kosina wrote:
> >> >>
> >> >> David Herrmann (12):
> >> > ...
> >> >>       HID: wiimote: add support for Guitar-Hero drums
> >> >
> >> >  commit 61e00655e9cb82e034eb72b95a51072e718d14a7
> >> >  Author: David Herrmann <dh.herrmann@gmail.com>
> >> >  Date:   Mon Aug 26 19:14:46 2013 +0200
> >> >
> >> >      Input: introduce BTN/ABS bits for drums and guitars
> >> >
> >> > The commit above breaks my Logitech mouse. The mouse cursor just sits in
> >> > the middle of the screen and doesn't react to movements. dmesg is
> >> > normal, but Xorg.0.log says:
> >>
> >> Ok, the issue is the kernel assumes ABS_MAX to be a power-of-2 minus 1
> >> (used as mask). That wasn't really obvious to me. Attached is a patch
> >> which should fix that. Could you apply it on top of linus/master and
> >> give it a try?
> >
> > Your patch fixes the issue. Thanks.
> 
> Thanks a lot for reporting+testing!
> 
> I am still not sure how to solve the EVIOCSABS thingy. Problem is,
> it's defined as:
>   #define EVIOCSABS(_abs) ...0xc0 + (_abs)...
> But if (_abs > 0x3f) this will be bigger than 0xff. Unfortunately, the
> upper part of the ioctl is defined as 'E' which is 0x45 in hex and
> thus sets the LSB. That means we cannot extend the _IOC_TYPE field to
> the upper region (which would cause endian-issues, anyway). I guess
> we're screwed here and need to revert that...
> 
> Dmitry, any comment on this? Or am I missing something?

We have gaps below ABS_MT constants, I think for 3.12 you could move
your whammy there and revert ABS_MAX change, but we need to plan for
expanding it in the future.

Thanks.
Linus Torvalds Sept. 6, 2013, 11:57 p.m. UTC | #4
On Fri, Sep 6, 2013 at 2:50 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Fri, Sep 6, 2013 at 10:20 PM, Markus Trippelsdorf
>>
>>  commit 61e00655e9cb82e034eb72b95a51072e718d14a7
>>  Author: David Herrmann <dh.herrmann@gmail.com>
>>  Date:   Mon Aug 26 19:14:46 2013 +0200
>>
>>      Input: introduce BTN/ABS bits for drums and guitars
>>
>> The commit above breaks my Logitech mouse. The mouse cursor just sits in
>> the middle of the screen and doesn't react to movements. dmesg is
>> normal, but Xorg.0.log says:
>
> Ok, the issue is the kernel assumes ABS_MAX to be a power-of-2 minus 1
> (used as mask). That wasn't really obvious to me. Attached is a patch
> which should fix that. Could you apply it on top of linus/master and
> give it a try?

Gah. I just wasted too much time bisecting down my logitech wireless
keyboard not working to within a few commits of this, and decided to
just try your patch.

And yes, it makes my keyboard work.

Dmitry, should I just apply the patch, or should we revert and use
other bits? Please, this needs to be resolved, I stopped merging when
I noticed this problem..

          Linus
--
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
Dmitry Torokhov Sept. 7, 2013, 12:58 a.m. UTC | #5
Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Fri, Sep 6, 2013 at 2:50 PM, David Herrmann <dh.herrmann@gmail.com>
>wrote:
>> Hi
>>
>> On Fri, Sep 6, 2013 at 10:20 PM, Markus Trippelsdorf
>>>
>>>  commit 61e00655e9cb82e034eb72b95a51072e718d14a7
>>>  Author: David Herrmann <dh.herrmann@gmail.com>
>>>  Date:   Mon Aug 26 19:14:46 2013 +0200
>>>
>>>      Input: introduce BTN/ABS bits for drums and guitars
>>>
>>> The commit above breaks my Logitech mouse. The mouse cursor just
>sits in
>>> the middle of the screen and doesn't react to movements. dmesg is
>>> normal, but Xorg.0.log says:
>>
>> Ok, the issue is the kernel assumes ABS_MAX to be a power-of-2 minus
>1
>> (used as mask). That wasn't really obvious to me. Attached is a patch
>> which should fix that. Could you apply it on top of linus/master and
>> give it a try?
>
>Gah. I just wasted too much time bisecting down my logitech wireless
>keyboard not working to within a few commits of this, and decided to
>just try your patch.
>
>And yes, it makes my keyboard work.
>
>Dmitry, should I just apply the patch, or should we revert and use
>other bits? Please, this needs to be resolved, I stopped merging when
>I noticed this problem..

The patch still had problems so I'd revert it and wii bits and try again later.


Thanks.
Linus Torvalds Sept. 7, 2013, 1 a.m. UTC | #6
On Fri, Sep 6, 2013 at 5:58 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> The patch still had problems so I'd revert it and wii bits and try again later.

Ok. Mind giving me a list of commits, so that I don't have to do a
trial-and-error thing? I know the primary commit that causes problems,
but there are commits that seem to depend on it..

             Linus
--
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
Dmitry Torokhov Sept. 7, 2013, 3:22 a.m. UTC | #7
On Fri, Sep 06, 2013 at 06:00:29PM -0700, Linus Torvalds wrote:
> On Fri, Sep 6, 2013 at 5:58 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > The patch still had problems so I'd revert it and wii bits and try again later.
> 
> Ok. Mind giving me a list of commits, so that I don't have to do a
> trial-and-error thing? I know the primary commit that causes problems,
> but there are commits that seem to depend on it..


Sorry for the delay. I believe you need to revert:

73f8645 HID: wiimote: add support for Guitar-Hero drums
61e0065 Input: introduce BTN/ABS bits for drums and guitars

Hmm... there also was "HID: wiimote: add support for Guitar-Hero
guitars" but I do not see it...

Thanks.
diff mbox

Patch

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index d2b34fb..82e0073 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -939,12 +939,13 @@  static long evdev_do_ioctl(struct file *file,
unsigned int cmd,
  _IOC_NR(cmd) & EV_MAX, size,
  p, compat_mode);

- if ((_IOC_NR(cmd) & ~ABS_MAX) == _IOC_NR(EVIOCGABS(0))) {
+ if (_IOC_NR(cmd) >= _IOC_NR(EVIOCGABS(0)) &&
+    _IOC_NR(cmd) <= _IOC_NR(EVIOCGABS(ABS_MAX))) {

  if (!dev->absinfo)
  return -EINVAL;

- t = _IOC_NR(cmd) & ABS_MAX;
+ t = _IOC_NR(cmd) - _IOC_NR(EVIOCGABS(0));
  abs = dev->absinfo[t];