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@xxxxxxxxxx>
> a Ãcrit :
> >
> > On Sep 13 2016 or thereabouts, Michel Hermier wrote:
> > > Hi,
> > >
> > > Le 13 sept. 2016 11:54 AM, "Benjamin Tissoires" <
> > > benjamin.tissoires@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxxxxxxxx
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html