| Message ID | X//jjawwbm8FxbQU@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 7c7d7ac7cebbf64a256b40ac7eb198cef8fd0642 |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: hid-input: avoid splitting keyboard, system and consumer controls | expand |
Hi Dmitry, On Thu, Jan 14, 2021 at 7:24 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > A typical USB keyboard usually splits its keys into several reports: > > - one for the basic alphanumeric keys, modifier keys, F<n> keys, six pack > keys and keypad. This report's application is normally listed as > GenericDesktop.Keyboard > - a GenericDesktop.SystemControl report for the system control keys, such > as power and sleep > - Consumer.ConsumerControl report for multimedia (forward, rewind, > play/pause, mute, etc) and other extended keys. > - additional output, vendor specific, and feature reports > > Splitting each report into a separate input device is wasteful and even > hurts userspace as it makes it harder to determine the true capabilities > (set of available keys) of a keyboard, so let's adjust application > matching to merge system control and consumer control reports with > keyboard report, if one has already been processed. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/hid/hid-input.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > index f797659cb9d9..df45d8d07dc2 100644 > --- a/drivers/hid/hid-input.c > +++ b/drivers/hid/hid-input.c > @@ -1851,6 +1851,16 @@ static struct hid_input *hidinput_match_application(struct hid_report *report) > list_for_each_entry(hidinput, &hid->inputs, list) { > if (hidinput->application == report->application) > return hidinput; > + > + /* > + * Keep SystemControl and ConsumerControl applications together > + * with the main keyboard, if present. > + */ > + if ((report->application == HID_GD_SYSTEM_CONTROL || > + report->application == HID_CP_CONSUMER_CONTROL) && > + hidinput->application == HID_GD_KEYBOARD) { I am not fundamentally against the patch, but I think that if the device exposes first a HID_CP_CONSUMER_CONTROL and then a HID_GD_KEYBOARD we will end up with 2 different input nodes. We likely need to "convert" HID_GD_SYSTEM_CONTROL and HID_CP_CONSUMER_CONTROL to HID_GD_KEYBOARD when creating the hidinput. Cheers, Benjamin > + return hidinput; > + } > } > > return NULL; > -- > 2.30.0.284.gd98b1dd5eaa7-goog > > > -- > Dmitry >
Hi Benjamin, On Thu, Jan 14, 2021 at 10:23:02AM +0100, Benjamin Tissoires wrote: > Hi Dmitry, > > On Thu, Jan 14, 2021 at 7:24 AM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > A typical USB keyboard usually splits its keys into several reports: > > > > - one for the basic alphanumeric keys, modifier keys, F<n> keys, six pack > > keys and keypad. This report's application is normally listed as > > GenericDesktop.Keyboard > > - a GenericDesktop.SystemControl report for the system control keys, such > > as power and sleep > > - Consumer.ConsumerControl report for multimedia (forward, rewind, > > play/pause, mute, etc) and other extended keys. > > - additional output, vendor specific, and feature reports > > > > Splitting each report into a separate input device is wasteful and even > > hurts userspace as it makes it harder to determine the true capabilities > > (set of available keys) of a keyboard, so let's adjust application > > matching to merge system control and consumer control reports with > > keyboard report, if one has already been processed. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/hid/hid-input.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > > index f797659cb9d9..df45d8d07dc2 100644 > > --- a/drivers/hid/hid-input.c > > +++ b/drivers/hid/hid-input.c > > @@ -1851,6 +1851,16 @@ static struct hid_input *hidinput_match_application(struct hid_report *report) > > list_for_each_entry(hidinput, &hid->inputs, list) { > > if (hidinput->application == report->application) > > return hidinput; > > + > > + /* > > + * Keep SystemControl and ConsumerControl applications together > > + * with the main keyboard, if present. > > + */ > > + if ((report->application == HID_GD_SYSTEM_CONTROL || > > + report->application == HID_CP_CONSUMER_CONTROL) && > > + hidinput->application == HID_GD_KEYBOARD) { > > I am not fundamentally against the patch, but I think that if the > device exposes first a HID_CP_CONSUMER_CONTROL and then a > HID_GD_KEYBOARD we will end up with 2 different input nodes. We likely > need to "convert" HID_GD_SYSTEM_CONTROL and HID_CP_CONSUMER_CONTROL to > HID_GD_KEYBOARD when creating the hidinput. While it is technically possible that consumer control or system control comes first, before main keyboard report, in reality all keyboards that I have seen so far have the main keyboard report first, so I opted not to handle the uncommon case to keep the code simple. I could add the above as a comment, and we could wait to see if there are devices that are exceptions to the common practice, or I can go and try to implement the conversion if you feel it is required. Please let me know. Note that we will still end up with 2+ input nodes if a device uses several USB interfaces, but we can't really do much about such cases (well, short of having a specialized driver claiming additional interfaces while probing the first one, but that is really outside of scope of this patch). Thanks!
On Wed, Jan 13, 2021 at 10:24:13PM -0800, Dmitry Torokhov wrote: > A typical USB keyboard usually splits its keys into several reports: > > - one for the basic alphanumeric keys, modifier keys, F<n> keys, six pack > keys and keypad. This report's application is normally listed as > GenericDesktop.Keyboard > - a GenericDesktop.SystemControl report for the system control keys, such > as power and sleep > - Consumer.ConsumerControl report for multimedia (forward, rewind, > play/pause, mute, etc) and other extended keys. > - additional output, vendor specific, and feature reports > > Splitting each report into a separate input device is wasteful and even > hurts userspace as it makes it harder to determine the true capabilities > (set of available keys) of a keyboard, so let's adjust application > matching to merge system control and consumer control reports with > keyboard report, if one has already been processed. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Acked-by: Peter Hutterer <peter.hutterer@who-t.net> I think, let's see if there's any fallout from that :) Cheers, Peter
On Wed, 13 Jan 2021, Dmitry Torokhov wrote: > A typical USB keyboard usually splits its keys into several reports: > > - one for the basic alphanumeric keys, modifier keys, F<n> keys, six pack > keys and keypad. This report's application is normally listed as > GenericDesktop.Keyboard > - a GenericDesktop.SystemControl report for the system control keys, such > as power and sleep > - Consumer.ConsumerControl report for multimedia (forward, rewind, > play/pause, mute, etc) and other extended keys. > - additional output, vendor specific, and feature reports > > Splitting each report into a separate input device is wasteful and even > hurts userspace as it makes it harder to determine the true capabilities > (set of available keys) of a keyboard, so let's adjust application > matching to merge system control and consumer control reports with > keyboard report, if one has already been processed. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- Queued in for-5.12/core. Thanks,
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index f797659cb9d9..df45d8d07dc2 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -1851,6 +1851,16 @@ static struct hid_input *hidinput_match_application(struct hid_report *report) list_for_each_entry(hidinput, &hid->inputs, list) { if (hidinput->application == report->application) return hidinput; + + /* + * Keep SystemControl and ConsumerControl applications together + * with the main keyboard, if present. + */ + if ((report->application == HID_GD_SYSTEM_CONTROL || + report->application == HID_CP_CONSUMER_CONTROL) && + hidinput->application == HID_GD_KEYBOARD) { + return hidinput; + } } return NULL;
A typical USB keyboard usually splits its keys into several reports: - one for the basic alphanumeric keys, modifier keys, F<n> keys, six pack keys and keypad. This report's application is normally listed as GenericDesktop.Keyboard - a GenericDesktop.SystemControl report for the system control keys, such as power and sleep - Consumer.ConsumerControl report for multimedia (forward, rewind, play/pause, mute, etc) and other extended keys. - additional output, vendor specific, and feature reports Splitting each report into a separate input device is wasteful and even hurts userspace as it makes it harder to determine the true capabilities (set of available keys) of a keyboard, so let's adjust application matching to merge system control and consumer control reports with keyboard report, if one has already been processed. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/hid/hid-input.c | 10 ++++++++++ 1 file changed, 10 insertions(+)