[PATCH] HID: input: fix confusion on conflicting mappings

[PATCH] HID: input: fix confusion on conflicting mappings

[David Herrmann]

On an PC-101/103/104 keyboard (American layout) the 'Enter' key and its
neighbours look like this:

           +---+ +---+ +-------+
           | 1 | | 2 | |   5   |
           +---+ +---+ +-------+
             +---+ +-----------+
             | 3 | |     4     |
             +---+ +-----------+

On a PC-102/105 keyboard (European layout) it looks like this:

           +---+ +---+ +-------+
           | 1 | | 2 | |       |
           +---+ +---+ +-+  4  |
             +---+ +---+ |     |
             | 3 | | 5 | |     |
             +---+ +---+ +-----+

(Note that the number of keys is the same, but key '5' is moved down and
 the shape of key '4' is changed. Keys '1' to '3' are exactly the same.)

The keys 1-4 report the same scan-code in HID in both layouts, even though
the keysym they produce is usually different depending on the XKB-keymap
used by user-space.
However, key '5' (US 'backslash'/'pipe') reports 0x31 for the upper layout
and 0x32 for the lower layout, as defined by the HID spec. This is highly
confusing as the linux-input API uses a single keycode for both.

So far, this was never a problem as there never has been a keyboard with
both of those keys present at the same time. It would have to look
something like this:

           +---+ +---+ +-------+
           | 1 | | 2 | |  x31  |
           +---+ +---+ +-------+
             +---+ +---+ +-----+
             | 3 | |x32| |  4  |
             +---+ +---+ +-----+

HID can represent such a keyboard, but the linux-input API cannot.
Furthermore, any user-space mapping would be confused by this and,
luckily, no-one ever produced such hardware.

Now, the HID input layer fixed this mess by mapping both 0x31 and 0x32 to
the same keycode (KEY_BACKSLASH==0x2b). As only one of both physical keys
is present on a hardware, this works just fine.

Lets introduce hardware-vendors into this:
------------------------------------------

Unfortunately, it seems way to expensive to produce a different device for
American and European layouts. Therefore, hardware-vendors put both keys,
(0x31 and 0x32) on the same keyboard, but only one of them is hooked up
to the physical button, the other one is 'dead'.
This means, they can use the same hardware, with a different button-layout
and automatically produce the correct HID events for American *and*
European layouts. This is unproblematic for normal keyboards, as the
'dead' key will never report any KEY-DOWN events. But RollOver keyboards
send the whole matrix on each key-event, allowing n-key roll-over mode.
This means, we get a 0x31 and 0x32 event on each key-press. One of them
will always be 0, the other reports the real state. As we map both to the
same keycode, we will get spurious key-events, even though the real
key-state never changed.

The easiest way would be to blacklist 'dead' keys and never handle those.
We could simply read the 'country' tag of USB devices and blacklist either
key according to the layout. But... hardware vendors... want the same
device for all countries and thus many of them set 'country' to 0 for all
devices. Meh..

So we have to deal with this properly. As we cannot know which of the keys
is 'dead', we either need a heuristic and track those keys, or we simply
make use of our value-tracking for HID fields. We simply ignore HID events
for absolute data if the data didn't change. As HID tracks events on the
HID level, we haven't done the keycode translation, yet. Therefore, the
'dead' key is tracked independently of the real key, therefore, any events
on it will be ignored.

This patch simply discards any HID events for absolute data if it didn't
change compared to the last report. We need to ignore relative and
buffered-byte reports for obvious reasons. But those cannot be affected by
this bug, so we're fine.

Preferably, we'd do this filtering on the HID-core level. But this might
break a lot of custom drivers, if they do not follow the HID specs.
Therefore, we do this late in hid-input just before we inject it into the
input layer (which does the exact same filtering, but on the keycode
level).

If this turns out to break some devices, we might have to limit filtering
to EV_KEY events. But lets try to do the Right Thing first, and properly
filter any absolute data that didn't change.

This patch is tagged for 'stable' as it fixes a lot of n-key RollOver
hardware. We might wanna wait with backporting for a while, before we know
it doesn't break anything else, though.

Cc: <stable@vger.kernel.org>
Reported-by: Adam Goode <adam@spicenitz.org>
Reported-by: Fredrik Hallenberg <megahallon@gmail.com>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
Hi

Fredrik, Adam, can you give this a try? It should fix your issues.

Thanks
David

 drivers/hid/hid-input.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 725f22c..8976895 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1101,6 +1101,22 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		return;
 	}
 
+	/*
+	 * Ignore reports for absolute data if the data didn't change. This is
+	 * not only an optimization but also fixes 'dead' key reports. Some
+	 * RollOver implementations for localized keys (like BACKSLASH/PIPE; HID
+	 * 0x31 and 0x32) report multiple keys, even though a localized keyboard
+	 * can only have one of them physically available. The 'dead' keys
+	 * report constant 0. As all map to the same keycode, they'd confuse
+	 * the input layer. If we filter the 'dead' keys on the HID level, we
+	 * skip the keycode translation and only forward real events.
+	 */
+	if (!(field->flags & (HID_MAIN_ITEM_RELATIVE |
+	                      HID_MAIN_ITEM_BUFFERED_BYTE)) &&
+	    usage->usage_index < field->maxusage &&
+	    value == field->value[usage->usage_index])
+		return;
+
 	/* report the usage code as scancode if the key status has changed */
 	if (usage->type == EV_KEY && !!test_bit(usage->code, input->key) != value)
 		input_event(input, EV_MSC, MSC_SCAN, usage->hid);
-- 
2.2.1

Re: [PATCH] HID: input: fix confusion on conflicting mappings

[Fredrik Hallenberg]

Thanks for completing the patch, it works fine with my Corsair K70.

On Mon, Dec 29, 2014 at 3:21 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> On an PC-101/103/104 keyboard (American layout) the 'Enter' key and its
> neighbours look like this:
>
>            +---+ +---+ +-------+
>            | 1 | | 2 | |   5   |
>            +---+ +---+ +-------+
>              +---+ +-----------+
>              | 3 | |     4     |
>              +---+ +-----------+
>
> On a PC-102/105 keyboard (European layout) it looks like this:
>
>            +---+ +---+ +-------+
>            | 1 | | 2 | |       |
>            +---+ +---+ +-+  4  |
>              +---+ +---+ |     |
>              | 3 | | 5 | |     |
>              +---+ +---+ +-----+
>
> (Note that the number of keys is the same, but key '5' is moved down and
>  the shape of key '4' is changed. Keys '1' to '3' are exactly the same.)
>
> The keys 1-4 report the same scan-code in HID in both layouts, even though
> the keysym they produce is usually different depending on the XKB-keymap
> used by user-space.
> However, key '5' (US 'backslash'/'pipe') reports 0x31 for the upper layout
> and 0x32 for the lower layout, as defined by the HID spec. This is highly
> confusing as the linux-input API uses a single keycode for both.
>
> So far, this was never a problem as there never has been a keyboard with
> both of those keys present at the same time. It would have to look
> something like this:
>
>            +---+ +---+ +-------+
>            | 1 | | 2 | |  x31  |
>            +---+ +---+ +-------+
>              +---+ +---+ +-----+
>              | 3 | |x32| |  4  |
>              +---+ +---+ +-----+
>
> HID can represent such a keyboard, but the linux-input API cannot.
> Furthermore, any user-space mapping would be confused by this and,
> luckily, no-one ever produced such hardware.
>
> Now, the HID input layer fixed this mess by mapping both 0x31 and 0x32 to
> the same keycode (KEY_BACKSLASH==0x2b). As only one of both physical keys
> is present on a hardware, this works just fine.
>
> Lets introduce hardware-vendors into this:
> ------------------------------------------
>
> Unfortunately, it seems way to expensive to produce a different device for
> American and European layouts. Therefore, hardware-vendors put both keys,
> (0x31 and 0x32) on the same keyboard, but only one of them is hooked up
> to the physical button, the other one is 'dead'.
> This means, they can use the same hardware, with a different button-layout
> and automatically produce the correct HID events for American *and*
> European layouts. This is unproblematic for normal keyboards, as the
> 'dead' key will never report any KEY-DOWN events. But RollOver keyboards
> send the whole matrix on each key-event, allowing n-key roll-over mode.
> This means, we get a 0x31 and 0x32 event on each key-press. One of them
> will always be 0, the other reports the real state. As we map both to the
> same keycode, we will get spurious key-events, even though the real
> key-state never changed.
>
> The easiest way would be to blacklist 'dead' keys and never handle those.
> We could simply read the 'country' tag of USB devices and blacklist either
> key according to the layout. But... hardware vendors... want the same
> device for all countries and thus many of them set 'country' to 0 for all
> devices. Meh..
>
> So we have to deal with this properly. As we cannot know which of the keys
> is 'dead', we either need a heuristic and track those keys, or we simply
> make use of our value-tracking for HID fields. We simply ignore HID events
> for absolute data if the data didn't change. As HID tracks events on the
> HID level, we haven't done the keycode translation, yet. Therefore, the
> 'dead' key is tracked independently of the real key, therefore, any events
> on it will be ignored.
>
> This patch simply discards any HID events for absolute data if it didn't
> change compared to the last report. We need to ignore relative and
> buffered-byte reports for obvious reasons. But those cannot be affected by
> this bug, so we're fine.
>
> Preferably, we'd do this filtering on the HID-core level. But this might
> break a lot of custom drivers, if they do not follow the HID specs.
> Therefore, we do this late in hid-input just before we inject it into the
> input layer (which does the exact same filtering, but on the keycode
> level).
>
> If this turns out to break some devices, we might have to limit filtering
> to EV_KEY events. But lets try to do the Right Thing first, and properly
> filter any absolute data that didn't change.
>
> This patch is tagged for 'stable' as it fixes a lot of n-key RollOver
> hardware. We might wanna wait with backporting for a while, before we know
> it doesn't break anything else, though.
>
> Cc: <stable@vger.kernel.org>
> Reported-by: Adam Goode <adam@spicenitz.org>
> Reported-by: Fredrik Hallenberg <megahallon@gmail.com>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> Hi
>
> Fredrik, Adam, can you give this a try? It should fix your issues.
>
> Thanks
> David
>
>  drivers/hid/hid-input.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 725f22c..8976895 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1101,6 +1101,22 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>                 return;
>         }
>
> +       /*
> +        * Ignore reports for absolute data if the data didn't change. This is
> +        * not only an optimization but also fixes 'dead' key reports. Some
> +        * RollOver implementations for localized keys (like BACKSLASH/PIPE; HID
> +        * 0x31 and 0x32) report multiple keys, even though a localized keyboard
> +        * can only have one of them physically available. The 'dead' keys
> +        * report constant 0. As all map to the same keycode, they'd confuse
> +        * the input layer. If we filter the 'dead' keys on the HID level, we
> +        * skip the keycode translation and only forward real events.
> +        */
> +       if (!(field->flags & (HID_MAIN_ITEM_RELATIVE |
> +                             HID_MAIN_ITEM_BUFFERED_BYTE)) &&
> +           usage->usage_index < field->maxusage &&
> +           value == field->value[usage->usage_index])
> +               return;
> +
>         /* report the usage code as scancode if the key status has changed */
>         if (usage->type == EV_KEY && !!test_bit(usage->code, input->key) != value)
>                 input_event(input, EV_MSC, MSC_SCAN, usage->hid);
> --
> 2.2.1
>

Re: [PATCH] HID: input: fix confusion on conflicting mappings

[Christopher Head]

Hi,
I tried your patch on my North American Das Keyboard 4. In N-key
rollover mode the patch fixes the backslash problem. Unfortunately,
with the patch applied, when in 6-key rollover mode (which, if I
remember correctly, uses a one-byte DATA VAR ABS for modifiers and a
six-byte DATA ARRAY ABS for other keys, as usual for keyboards), the
last typed key repeats forever even after being released.

(Apologies for the lack of In-Reply-To header; I am not subscribed to
the list so had no message to reply to. Please CC me in replies if
necessary.)
-- 
Christopher Head

Re: [PATCH] HID: input: fix confusion on conflicting mappings

[Fredrik Hallenberg]

I can confirm that the patch breaks things when not using n key
rollover. If using the Corsair K70 in "BIOS mode" or just using a
plain USB-keyboard keys repeat forever as reported.

David, I noticed that in the first version of your patch, where the
ignore check was done in hid-core.c, you only checked events that had
the HID_MAIN_ITEM_VARIABLE flag set. I can see that "plain" keyboards
don't have this bit set on the release event so if I keep this check
in the new patch it works in both cases.

So, the if-clause now looks like this:

...
    if (!(field->flags & (HID_MAIN_ITEM_RELATIVE |
                  HID_MAIN_ITEM_BUFFERED_BYTE)) &&
        (field->flags & HID_MAIN_ITEM_VARIABLE) &&
        usage->usage_index < field->maxusage &&
        value == field->value[usage->usage_index]) {
...

On Wed, Dec 31, 2014 at 1:57 AM, Christopher Head <chead@chead.ca> wrote:
> Hi,
> I tried your patch on my North American Das Keyboard 4. In N-key
> rollover mode the patch fixes the backslash problem. Unfortunately,
> with the patch applied, when in 6-key rollover mode (which, if I
> remember correctly, uses a one-byte DATA VAR ABS for modifiers and a
> six-byte DATA ARRAY ABS for other keys, as usual for keyboards), the
> last typed key repeats forever even after being released.
>
> (Apologies for the lack of In-Reply-To header; I am not subscribed to
> the list so had no message to reply to. Please CC me in replies if
> necessary.)
> --
> Christopher Head
> --
> 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] HID: input: fix confusion on conflicting mappings

[David Herrmann]

Hi

On Tue, Jan 6, 2015 at 1:37 PM, Fredrik Hallenberg <megahallon@gmail.com> wrote:
> I can confirm that the patch breaks things when not using n key
> rollover. If using the Corsair K70 in "BIOS mode" or just using a
> plain USB-keyboard keys repeat forever as reported.
>
> David, I noticed that in the first version of your patch, where the
> ignore check was done in hid-core.c, you only checked events that had
> the HID_MAIN_ITEM_VARIABLE flag set. I can see that "plain" keyboards
> don't have this bit set on the release event so if I keep this check
> in the new patch it works in both cases.
>
> So, the if-clause now looks like this:
>
> ...
>     if (!(field->flags & (HID_MAIN_ITEM_RELATIVE |
>                   HID_MAIN_ITEM_BUFFERED_BYTE)) &&
>         (field->flags & HID_MAIN_ITEM_VARIABLE) &&
>         usage->usage_index < field->maxusage &&
>         value == field->value[usage->usage_index]) {
> ...

Nice catch! Of course, we must not apply this to ARRAY reports. I will
send v2 tomorrow (still on the road..).

Thanks
David

Re: [PATCH] HID: input: fix confusion on conflicting mappings

[Jiri Kosina]

On Mon, 29 Dec 2014, David Herrmann wrote:

> On an PC-101/103/104 keyboard (American layout) the 'Enter' key and its
> neighbours look like this:
> 
>            +---+ +---+ +-------+
>            | 1 | | 2 | |   5   |
>            +---+ +---+ +-------+
>              +---+ +-----------+
>              | 3 | |     4     |
>              +---+ +-----------+

[ ... snip ... ]

Alright, I can't really see this introducing any regressions. I have now 
queued this for 3.20.

Thanks!

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: input: fix confusion on conflicting mappings

[David Herrmann]

Hi

On Tue, Jan 6, 2015 at 10:19 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 29 Dec 2014, David Herrmann wrote:
>
>> On an PC-101/103/104 keyboard (American layout) the 'Enter' key and its
>> neighbours look like this:
>>
>>            +---+ +---+ +-------+
>>            | 1 | | 2 | |   5   |
>>            +---+ +---+ +-------+
>>              +---+ +-----------+
>>              | 3 | |     4     |
>>              +---+ +-----------+
>
> [ ... snip ... ]
>
> Alright, I can't really see this introducing any regressions. I have now
> queued this for 3.20.

As Fredrik wrote, we need to also check for VARIABLE:

    if (!(field->flags & (HID_MAIN_ITEM_RELATIVE |
                  HID_MAIN_ITEM_BUFFERED_BYTE)) &&
        (field->flags & HID_MAIN_ITEM_VARIABLE) &&
        usage->usage_index < field->maxusage &&
        value == field->value[usage->usage_index]) {

the added line is:

            (field->flags & HID_MAIN_ITEM_VARIABLE) &&

I'll not be at home until tomorrow. Feel free to add it yourself,
otherwise, basic HID keyboards that use ARRAY reports are broken.

Thanks
David

Re: [PATCH] HID: input: fix confusion on conflicting mappings

[Jiri Kosina]

On Tue, 6 Jan 2015, David Herrmann wrote:

> >> On an PC-101/103/104 keyboard (American layout) the 'Enter' key and its
> >> neighbours look like this:
> >>
> >>            +---+ +---+ +-------+
> >>            | 1 | | 2 | |   5   |
> >>            +---+ +---+ +-------+
> >>              +---+ +-----------+
> >>              | 3 | |     4     |
> >>              +---+ +-----------+
> >
> > [ ... snip ... ]
> >
> > Alright, I can't really see this introducing any regressions. I have now
> > queued this for 3.20.
> 
> As Fredrik wrote, we need to also check for VARIABLE:

Gah, that's indeed a good catch. For some reason I don't seem to be CCed 
on Fredrik's mail :/

I have now applied the patch below on top of for-3.20/upstream, so that we 
don't keep -next unnecessarily broken.



From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] HID: fixup the conflicting keyboard mappings quirk

The ignore check that got added in 6ce901eb61 ("HID: input: fix confusion
on conflicting mappings") needs to properly check for VARIABLE reports
as well (ARRAY reports should be ignored), otherwise legitimate keyboards
might break.

Cc: <stable@vger.kernel.org>
Fixes: 6ce901eb61 ("HID: input: fix confusion on conflicting mappings")
Reported-by: Fredrik Hallenberg <megahallon@gmail.com>
Reported-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/hid/hid-input.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 84b6899..a758900 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1113,6 +1113,7 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 	 */
 	if (!(field->flags & (HID_MAIN_ITEM_RELATIVE |
 	                      HID_MAIN_ITEM_BUFFERED_BYTE)) &&
+			      (field->flags & HID_MAIN_ITEM_VARIABLE) &&
 	    usage->usage_index < field->maxusage &&
 	    value == field->value[usage->usage_index])
 		return;
-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: input: fix confusion on conflicting mappings

[Christopher Head]

On Tue, 6 Jan 2015 13:37:26 +0100
Fredrik Hallenberg <megahallon@gmail.com> wrote:

> ...
>     if (!(field->flags & (HID_MAIN_ITEM_RELATIVE |
>                   HID_MAIN_ITEM_BUFFERED_BYTE)) &&
>         (field->flags & HID_MAIN_ITEM_VARIABLE) &&
>         usage->usage_index < field->maxusage &&
>         value == field->value[usage->usage_index]) {
> ...

This works perfectly for me, thanks.
-- 
Christopher Head

Re: [PATCH] HID: input: fix confusion on conflicting mappings

[Fredrik Hallenberg]

Jiri, I suppose you can close bugzilla bug 70181?

On Thu, Jan 8, 2015 at 6:50 AM, Christopher Head <chead@chead.ca> wrote:
> On Tue, 6 Jan 2015 13:37:26 +0100
> Fredrik Hallenberg <megahallon@gmail.com> wrote:
>
>> ...
>>     if (!(field->flags & (HID_MAIN_ITEM_RELATIVE |
>>                   HID_MAIN_ITEM_BUFFERED_BYTE)) &&
>>         (field->flags & HID_MAIN_ITEM_VARIABLE) &&
>>         usage->usage_index < field->maxusage &&
>>         value == field->value[usage->usage_index]) {
>> ...
>
> This works perfectly for me, thanks.
> --
> Christopher Head