Re: [PATCH] HID: hid-input: avoid splitting keyboard, system and consumer controls
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 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 > --- Queued in for-5.12/core. Thanks, -- Jiri Kosina SUSE Labs
Re: [PATCH] HID: hid-input: avoid splitting keyboard, system and consumer controls
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 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 Acked-by: Peter Hutterer I think, let's see if there's any fallout from that :) Cheers, Peter
Re: [PATCH] HID: hid-input: avoid splitting keyboard, system and consumer controls
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 > wrote: > > > > A typical USB keyboard usually splits its keys into several reports: > > > > - one for the basic alphanumeric keys, modifier keys, F 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 > > --- > > 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, >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! -- Dmitry
Re: [PATCH] HID: hid-input: avoid splitting keyboard, system and consumer controls
Hi Dmitry, On Thu, Jan 14, 2021 at 7:24 AM Dmitry Torokhov wrote: > > A typical USB keyboard usually splits its keys into several reports: > > - one for the basic alphanumeric keys, modifier keys, F 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 > --- > 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, >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 >