| Message ID | 1473760358-31999-2-git-send-email-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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,
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;
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(+)