[PATCH 0/2] Fix spurious joydev node on some unrelated devices

[PATCH 0/2] Fix spurious joydev node on some unrelated devices

[Benjamin Tissoires]

Hi,

It looks like Steam is suffering from having spurious joydev nodes not
properly handled by the kernel. This appears on nearly all Microsoft devices
and all Wacom tablets (though I guess the interference between Wacom tablet
and Steam is not much of a problem :-P ).

Anyway, this should fix a long standing issue (since 2011 apparently).

It's still unclear whether this will fix all the spurious joydev nodes for
other vendors too (A4Tech mice notably).

Cheers,
Benjamin

Benjamin Tissoires (2):
  HID: input: ignore System Control application usages if not System
    Controls
  HID: wacom: do not detect Pad devices as joysticks

 drivers/hid/hid-input.c | 9 +++++++++
 drivers/hid/wacom_wac.c | 3 +++
 2 files changed, 12 insertions(+)

-- 
2.7.4

[PATCH 1/2] HID: input: ignore System Control application usages if not System Controls

[Benjamin Tissoires]

Microsoft is reusing its report descriptor again and again, and part of it
looks like this:

0x05, 0x01,                    // Usage Page (Generic Desktop)        299
0x09, 0x80,                    // Usage (System Control)              301
0xa1, 0x01,                    // Collection (Application)            303
0x85, 0x03,                    //  Report ID (3)                      305
0x19, 0x00,                    //  Usage Minimum (0)                  307
0x29, 0xff,                    //  Usage Maximum (255)                309
0x15, 0x00,                    //  Logical Minimum (0)                311
0x26, 0xff, 0x00,              //  Logical Maximum (255)              313
0x81, 0x00,                    //  Input (Data,Arr,Abs)               316
0xc0,                          // End Collection                      318

While there is nothing wrong in term of processing, we do however blindly
map the full usage range (it's an array) from 0x00 to 0xff, which creates
some interesting axis, like ABS_X|Y, and a bunch of ABS_MISC + n.

While libinput and other stacks don't care that much (we can detect them),
joydev is very happy and attaches itself to the mouse or keyboard.

The problem is that joydev now handles the device as a joystick, but given
that we have a HID array, it sets all the ABS_* values to 0. And in its
world, 0 means -32767 (minimum value), which sends spurious events to games
(think Steam).

It looks like hid-microsoft tries to tackle the very same problem with its
.report_fixup callback. But fixing the report descriptor is an endless task
and is quite obfuscated.

So take the hammer, and decide that if the application is meant to be
System Control, any other usage not in the System Control range should
be ignored.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=1325354
Link: https://bugzilla.kernel.org/show_bug.cgi?id=28912
Link: https://github.com/ValveSoftware/steam-for-linux/issues/3384
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1325354
Link: https://bugzilla.kernel.org/show_bug.cgi?id=37982

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 14dd974..55db584 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -604,6 +604,15 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			break;
 		}
 
+		/*
+		 * Some lazy vendors declare 255 usages for System Control,
+		 * leading to the creation of ABS_X|Y axis and too many others.
+		 * It wouldn't be a problem if joydev doesn't consider the
+		 * device as a joystick then.
+		 */
+		if (field->application == HID_GD_SYSTEM_CONTROL)
+			goto ignore;
+
 		if ((usage->hid & 0xf0) == 0x90) {	/* D-pad */
 			switch (usage->hid) {
 			case HID_GD_UP:	   usage->hat_dir = 1; break;
-- 
2.7.4

Re: [PATCH 1/2] HID: input: ignore System Control application usages if not System Controls

[Benjamin Tissoires]

On Sep 13 2016 or thereabouts, Michel Hermier wrote:
> Hi,
> 
> Le 13 sept. 2016 11:54 AM, "Benjamin Tissoires" <
> benjamin.tissoires@redhat.com> a écrit :
> >
> > Microsoft is reusing its report descriptor again and again, and part of it
> > looks like this:
> >
> > 0x05, 0x01,                    // Usage Page (Generic Desktop)        299
> > 0x09, 0x80,                    // Usage (System Control)              301
> > 0xa1, 0x01,                    // Collection (Application)            303
> > 0x85, 0x03,                    //  Report ID (3)                      305
> > 0x19, 0x00,                    //  Usage Minimum (0)                  307
> > 0x29, 0xff,                    //  Usage Maximum (255)                309
> > 0x15, 0x00,                    //  Logical Minimum (0)                311
> > 0x26, 0xff, 0x00,              //  Logical Maximum (255)              313
> > 0x81, 0x00,                    //  Input (Data,Arr,Abs)               316
> > 0xc0,                          // End Collection                      318
> >
> > While there is nothing wrong in term of processing, we do however blindly
> > map the full usage range (it's an array) from 0x00 to 0xff, which creates
> > some interesting axis, like ABS_X|Y, and a bunch of ABS_MISC + n.
> >
> > While libinput and other stacks don't care that much (we can detect them),
> > joydev is very happy and attaches itself to the mouse or keyboard.
> >
> > The problem is that joydev now handles the device as a joystick, but given
> > that we have a HID array, it sets all the ABS_* values to 0. And in its
> > world, 0 means -32767 (minimum value), which sends spurious events to
> games
> > (think Steam).
> >
> > It looks like hid-microsoft tries to tackle the very same problem with its
> > .report_fixup callback. But fixing the report descriptor is an endless
> task
> > and is quite obfuscated.
> 
> Do you plan to remove those various fixup in a later patch series, or do
> you have other plans?

Well, I don't have the affected hardware on hid-microsoft, so I was planing
on just leaving the code as it is.

If you have some and can test/verify, then do not hesitate to remove
those quirks.

> 
> >
> > So take the hammer, and decide that if the application is meant to be
> > System Control, any other usage not in the System Control range should
> > be ignored.
> 
> To be on a safe side, shouldn't there be a flag to bypass that change in
> case it makes some situations worse? (Thought I agree we should be pretty
> safe here)

Well, we are safe unless Hardware makers start doing weird things. But
hardware makers always like doing weird things.

I think I am more willing to try fixing this one out, and if somebody
else starts complaining about, then we can always add more guards (one
could be to check if the wrong usage min/max are 0-255). We could also
simply add "if vendor == MICROSOFT" or something in that spirit.

Right now I'd better try this just in case this wrong report descriptor
is used in different hardware.

Regarding the flag solution, if you are thinking at a kernel module
parameter,  I'd better not to. The reason being that once the word is
spread that you can "fix" your device by adding a simple module
parameter, people tend to forget to report bugs. I have seen countless
of forums/threads saying that you have to disable i2c-hid to have your
touchpad working, even when this is just not required.

Well, if Jiri is willing to add more checks and flags, we can always,
but I'd rather not.

Last, I'll request those patches to be included in Fedora when Jiri
takes them (or a future revision). We should have enough angry users
once it hits Fedora if there are some :)

Cheers,
Benjamin

> 
> Cheers,
>    Michek
> 
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1325354
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=28912
> > Link: https://github.com/ValveSoftware/steam-for-linux/issues/3384
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1325354
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=37982
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  drivers/hid/hid-input.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> > index 14dd974..55db584 100644
> > --- a/drivers/hid/hid-input.c
> > +++ b/drivers/hid/hid-input.c
> > @@ -604,6 +604,15 @@ static void hidinput_configure_usage(struct
> hid_input *hidinput, struct hid_fiel
> >                         break;
> >                 }
> >
> > +               /*
> > +                * Some lazy vendors declare 255 usages for System
> Control,
> > +                * leading to the creation of ABS_X|Y axis and too many
> others.
> > +                * It wouldn't be a problem if joydev doesn't consider the
> > +                * device as a joystick then.
> > +                */
> > +               if (field->application == HID_GD_SYSTEM_CONTROL)
> > +                       goto ignore;
> > +
> >                 if ((usage->hid & 0xf0) == 0x90) {      /* D-pad */
> >                         switch (usage->hid) {
> >                         case HID_GD_UP:    usage->hat_dir = 1; break;
> > --
> > 2.7.4
> >
> > --
> > 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 1/2] HID: input: ignore System Control application usages if not System Controls

[Benjamin Tissoires]

On Sep 13 2016 or thereabouts, Michel Hermier wrote:
> Le 13 sept. 2016 15:58, "Benjamin Tissoires" <benjamin.tissoires@redhat.com>
> a écrit :
> >
> > On Sep 13 2016 or thereabouts, Michel Hermier wrote:
> > > Hi,
> > >
> > > Le 13 sept. 2016 11:54 AM, "Benjamin Tissoires" <
> > > benjamin.tissoires@redhat.com> a écrit :
> > > >
> > > > Microsoft is reusing its report descriptor again and again, and part
> of it
> > > > looks like this:
> > > >
> > > > 0x05, 0x01,                    // Usage Page (Generic Desktop)
> 299
> > > > 0x09, 0x80,                    // Usage (System Control)
> 301
> > > > 0xa1, 0x01,                    // Collection (Application)
> 303
> > > > 0x85, 0x03,                    //  Report ID (3)
> 305
> > > > 0x19, 0x00,                    //  Usage Minimum (0)
> 307
> > > > 0x29, 0xff,                    //  Usage Maximum (255)
> 309
> > > > 0x15, 0x00,                    //  Logical Minimum (0)
> 311
> > > > 0x26, 0xff, 0x00,              //  Logical Maximum (255)
> 313
> > > > 0x81, 0x00,                    //  Input (Data,Arr,Abs)
>  316
> > > > 0xc0,                          // End Collection
> 318
> > > >
> > > > While there is nothing wrong in term of processing, we do however
> blindly
> > > > map the full usage range (it's an array) from 0x00 to 0xff, which
> creates
> > > > some interesting axis, like ABS_X|Y, and a bunch of ABS_MISC + n.
> > > >
> > > > While libinput and other stacks don't care that much (we can detect
> them),
> > > > joydev is very happy and attaches itself to the mouse or keyboard.
> > > >
> > > > The problem is that joydev now handles the device as a joystick, but
> given
> > > > that we have a HID array, it sets all the ABS_* values to 0. And in
> its
> > > > world, 0 means -32767 (minimum value), which sends spurious events to
> > > games
> > > > (think Steam).
> > > >
> > > > It looks like hid-microsoft tries to tackle the very same problem
> with its
> > > > .report_fixup callback. But fixing the report descriptor is an endless
> > > task
> > > > and is quite obfuscated.
> > >
> > > Do you plan to remove those various fixup in a later patch series, or do
> > > you have other plans?
> >
> > Well, I don't have the affected hardware on hid-microsoft, so I was
> planing
> > on just leaving the code as it is.
> >
> > If you have some and can test/verify, then do not hesitate to remove
> > those quirks.
> 
> I'm the original author of the 2011 bug, and I still own one of the
> Microsoft keyboard with the issue. But not one that has a fix, but one that
> have the exact same broken report descriptor. When I'll get some time to
> test the patch, on success I think it would be safe to remove one of the 2
> fixup available, since they seems to have reused the exact same descriptor
> for the whole family.

TBH, I think it should be safe to remove the whole report_fixup in
hid-microsoft. If there is enough push, I'll do it.

> 
> > >
> > > >
> > > > So take the hammer, and decide that if the application is meant to be
> > > > System Control, any other usage not in the System Control range should
> > > > be ignored.
> > >
> > > To be on a safe side, shouldn't there be a flag to bypass that change in
> > > case it makes some situations worse? (Thought I agree we should be
> pretty
> > > safe here)
> >
> > Well, we are safe unless Hardware makers start doing weird things. But
> > hardware makers always like doing weird things.
> >
> > I think I am more willing to try fixing this one out, and if somebody
> > else starts complaining about, then we can always add more guards (one
> > could be to check if the wrong usage min/max are 0-255). We could also
> > simply add "if vendor == MICROSOFT" or something in that spirit.
> >
> > Right now I'd better try this just in case this wrong report descriptor
> > is used in different hardware.
> >
> > Regarding the flag solution, if you are thinking at a kernel module
> > parameter,  I'd better not to. The reason being that once the word is
> > spread that you can "fix" your device by adding a simple module
> > parameter, people tend to forget to report bugs. I have seen countless
> > of forums/threads saying that you have to disable i2c-hid to have your
> > touchpad working, even when this is just not required.
> >
> > Well, if Jiri is willing to add more checks and flags, we can always,
> > but I'd rather not.
> >
> > Last, I'll request those patches to be included in Fedora when Jiri
> > takes them (or a future revision). We should have enough angry users
> > once it hits Fedora if there are some :)
> 
> Nice, what is the window, so I can plan testing the patch and report if you
> can remove one of the quirk? Unfortunately this is the period where I get
> really busy at work, this is why I ask.

There is already a scratch kernel available in the Red Hat bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1325354#c8

If the device is supported already by hid-microsoft, starting the kernel
with hid.ignore_special_drivers=1 should force hid-generic to handle the
keyboard.

I can't really give an estimate on when this will land Fedora stable
though. It depends on when Jiri picks up the patch, when I remember to
ask for the inclusion in Fedora, then the maintainers have to take the
patch and build a new build. Everything is quite quick, but there are
some uncertainties that prevent me to give you a good estimate.

> 
> Also maybe, attach the patch to my bug so it tries to get a little bit
> more  spread/visibility.

k, will do.

Cheers,
Benjamin

> 
> Cheers,
>    Michel
> 
> >
> > Cheers,
> > Benjamin
> >
> > >
> > > Cheers,
> > >    Michek
> > >
> > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1325354
> > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=28912
> > > > Link: https://github.com/ValveSoftware/steam-for-linux/issues/3384
> > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1325354
> > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=37982
> > > >
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > ---
> > > >  drivers/hid/hid-input.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> > > > index 14dd974..55db584 100644
> > > > --- a/drivers/hid/hid-input.c
> > > > +++ b/drivers/hid/hid-input.c
> > > > @@ -604,6 +604,15 @@ static void hidinput_configure_usage(struct
> > > hid_input *hidinput, struct hid_fiel
> > > >                         break;
> > > >                 }
> > > >
> > > > +               /*
> > > > +                * Some lazy vendors declare 255 usages for System
> > > Control,
> > > > +                * leading to the creation of ABS_X|Y axis and too
> many
> > > others.
> > > > +                * It wouldn't be a problem if joydev doesn't
> consider the
> > > > +                * device as a joystick then.
> > > > +                */
> > > > +               if (field->application == HID_GD_SYSTEM_CONTROL)
> > > > +                       goto ignore;
> > > > +
> > > >                 if ((usage->hid & 0xf0) == 0x90) {      /* D-pad */
> > > >                         switch (usage->hid) {
> > > >                         case HID_GD_UP:    usage->hat_dir = 1; break;
> > > > --
> > > > 2.7.4
> > > >
> > > > --
> > > > 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 1/2] HID: input: ignore System Control application usages if not System Controls

[Jiri Kosina]

On Tue, 13 Sep 2016, Michel Hermier wrote:

> I'm the original author of the 2011 bug, and I still own one of the 
> Microsoft keyboard with the issue. But not one that has a fix, but one 
> that have the exact same broken report descriptor. When I'll get some 
> time to test the patch, on success I think it would be safe to remove 
> one of the 2 fixup available, since they seems to have reused the exact 
> same descriptor for the whole family.

I'd be interested in the result of this test.

In any case, to get as much testing coverage as possible, I've now applied 
this one (1/2) to hid.git#for-4.9.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[PATCH 2/2] HID: wacom: do not detect Pad devices as joysticks

[Benjamin Tissoires]

Well, joydev consider our Pad devices as joysticks and associate a
/dev/input/jsX node for them.

The problem is that it consider the ABS_X value as 0, and sends some
spurious joystick events saying the X axis is full left (Y too, but full
up). Given that we are basically screwed, add one more BTN which tells
joydev to ignore the Pad device and doesn't interfere with the rest of
the udev rules and the processing.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_wac.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 8fd115f..498f4a8 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -2920,6 +2920,9 @@ int wacom_setup_pad_input_capabilities(struct input_dev *input_dev,
 	/* kept for making udev and libwacom accepting the pad */
 	__set_bit(BTN_STYLUS, input_dev->keybit);
 
+	/* added to be ignored by joydev */
+	__set_bit(BTN_TOOL_PEN, input_dev->keybit);
+
 	wacom_setup_numbered_buttons(input_dev, features->numbered_buttons);
 
 	switch (features->type) {
-- 
2.7.4

Re: [PATCH 2/2] HID: wacom: do not detect Pad devices as joysticks

[Benjamin Tissoires]

On Sep 13 2016 or thereabouts, Ping Cheng wrote:
> Hi Benjamin,
> 
> On Tuesday, September 13, 2016, Benjamin Tissoires <
> benjamin.tissoires@redhat.com> wrote:
> 
> > Well, joydev consider our Pad devices as joysticks and associate a
> > /dev/input/jsX node for them.
> 
> 
> Indeed, this issue has annoyed me for many years. I'm happy to see someone
> is putting effort on it. Thank you so much!

I thought this would be not so much effort, but I see you are willing to
make me do some more effort :)

> 
> The problem is that it consider the ABS_X value as 0, and sends some
> > spurious joystick events saying the X axis is full left (Y too, but full
> > up). Given that we are basically screwed, add one more BTN which tells
> > joydev to ignore the Pad device and doesn't interfere with the rest of
> > the udev rules and the processing.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com
> > <javascript:;>>
> > ---
> >  drivers/hid/wacom_wac.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> > index 8fd115f..498f4a8 100644
> > --- a/drivers/hid/wacom_wac.c
> > +++ b/drivers/hid/wacom_wac.c
> > @@ -2920,6 +2920,9 @@ int wacom_setup_pad_input_capabilities(struct
> > input_dev *input_dev,
> >         /* kept for making udev and libwacom accepting the pad */
> >         __set_bit(BTN_STYLUS, input_dev->keybit);
> >
> > +       /* added to be ignored by joydev */
> > +       __set_bit(BTN_TOOL_PEN, input_dev->keybit);
> > +
> 
> 
> I am thinking this could be confusing to those 3rd party developers who
> extract tool/device types directly from the kernel. Since we are on this
> page, can we add some types to distinguish a PAD from a JOYSTICK, such as
> BTN_TOOL_JOYSTICK and BTN_TOOL_PAD. A clear upstream definition goes a long
> way to userland.

It's going to be difficult to convince Dmitry on this. Joysticks and
Pads are usually static configuration per device node, so it would make
more sense to have this set as an INPUT_PROP instead.

And given we already have INPUT_PROP_POINTER and INPUT_PROP_DIRECT, it
looks like we can infer the Pad/Joysitck easily from userspace without
adding more API.

Anyway, I think I'll turn the patch into one against joydev that will
add an exception for BTN_STYLUS, like we have for BTN_DIGI (a.k.a.
BTN_TOOL_PEN). However, I foresee a more difficult inclusion because the
heuristic in joydev is quite interesting to the least.

Cheers,
Benjamin

> 
> Cheers,
> 
> Ping
> 
>         wacom_setup_numbered_buttons(input_dev, features->numbered_buttons);
> >
> >         switch (features->type) {
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-input" in
> > the body of a message to majordomo@vger.kernel.org <javascript:;>
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >