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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> Reported-by: Adam Goode <adam@xxxxxxxxxxxxx>
> Reported-by: Fredrik Hallenberg <megahallon@xxxxxxxxx>
> Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxx>
> ---
> 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
>
--
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



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux