[PATCH] Handle spurious backslash key repeats on some keyboards

[PATCH] Handle spurious backslash key repeats on some keyboards

[Fredrik Hallenberg]

Here is my attempt on a fix for bug 70181, please review it. It is
tested on my nordic Corsair K70, if this solution is deemed acceptable
I can ask some people with other problematic keyboards to test it.

Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
---
 drivers/hid/hid-input.c | 14 ++++++++++++++
 include/linux/hid.h     |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 2619f7f..56429c0 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		return;
 	}
 
+	/*
+	 * Some keyboards will report both HID keys 0x31 (\ and |) and
+	 * 0x32 (Non-US # and ~) which are both mapped to
+	 * KEY_BACKSLASH. This will cause spurious events causing
+	 * repeated backslash key presses. Handle this by tracking the
+	 * active HID code and ignoring the other one.
+	 */
+	if (usage->type == EV_KEY && usage->code == KEY_BACKSLASH) {
+		if (value)
+			field->hidinput->backslash_usage = usage->hid;
+		else if (field->hidinput->backslash_usage != usage->hid)
+			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);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index f53c4a9..1c59f8f 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -448,6 +448,7 @@ struct hid_input {
 	struct list_head list;
 	struct hid_report *report;
 	struct input_dev *input;
+	unsigned backslash_usage;
 };
 
 enum hid_type {
-- 
2.1.0.rc1

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Fredrik Hallenberg]

Hi, time for a bump on this one.

On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
<megahallon@gmail.com> wrote:
> Here is my attempt on a fix for bug 70181, please review it. It is
> tested on my nordic Corsair K70, if this solution is deemed acceptable
> I can ask some people with other problematic keyboards to test it.
>
> Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
> ---
>  drivers/hid/hid-input.c | 14 ++++++++++++++
>  include/linux/hid.h     |  1 +
>  2 files changed, 15 insertions(+)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 2619f7f..56429c0 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>                 return;
>         }
>
> +       /*
> +        * Some keyboards will report both HID keys 0x31 (\ and |) and
> +        * 0x32 (Non-US # and ~) which are both mapped to
> +        * KEY_BACKSLASH. This will cause spurious events causing
> +        * repeated backslash key presses. Handle this by tracking the
> +        * active HID code and ignoring the other one.
> +        */
> +       if (usage->type == EV_KEY && usage->code == KEY_BACKSLASH) {
> +               if (value)
> +                       field->hidinput->backslash_usage = usage->hid;
> +               else if (field->hidinput->backslash_usage != usage->hid)
> +                       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);
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index f53c4a9..1c59f8f 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -448,6 +448,7 @@ struct hid_input {
>         struct list_head list;
>         struct hid_report *report;
>         struct input_dev *input;
> +       unsigned backslash_usage;
>  };
>
>  enum hid_type {
> --
> 2.1.0.rc1
>

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Jiri Kosina]

On Tue, 9 Sep 2014, Fredrik Hallenberg wrote:

> Hi, time for a bump on this one.

Do you have any idea how common this pattern is? How many keyboards have 
you encountered doing this?

I hate to be polluting generic code with such strange quirks ... we've 
spent quite some time factoring those out to specific drivers.
So knowing how commong this is could be helpful in deciding how to handle 
this.

Thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Fredrik Hallenberg]

I have seen reports for Gigabyte Osmium, Corsair Vengeance K-series
and QPAD MK-series. There are probably more. The reason I chose to not
make it a quirk specific to some specific hardware is that it feels
more like a general problem caused by the fact the there are two HID
key codes mapped to a single Linux key code.

On Tue, Sep 9, 2014 at 11:42 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Tue, 9 Sep 2014, Fredrik Hallenberg wrote:
>
>> Hi, time for a bump on this one.
>
> Do you have any idea how common this pattern is? How many keyboards have
> you encountered doing this?
>
> I hate to be polluting generic code with such strange quirks ... we've
> spent quite some time factoring those out to specific drivers.
> So knowing how commong this is could be helpful in deciding how to handle
> this.
>
> Thanks.
>
> --
> Jiri Kosina
> SUSE Labs

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Jiri Kosina]

On Tue, 9 Sep 2014, Fredrik Hallenberg wrote:

> Hi, time for a bump on this one.
> 
> On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
> <megahallon@gmail.com> wrote:
> > Here is my attempt on a fix for bug 70181, please review it. It is
> > tested on my nordic Corsair K70, if this solution is deemed acceptable
> > I can ask some people with other problematic keyboards to test it.
> >
> > Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
> > ---
> >  drivers/hid/hid-input.c | 14 ++++++++++++++
> >  include/linux/hid.h     |  1 +
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> > index 2619f7f..56429c0 100644
> > --- a/drivers/hid/hid-input.c
> > +++ b/drivers/hid/hid-input.c
> > @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
> >                 return;
> >         }
> >
> > +       /*
> > +        * Some keyboards will report both HID keys 0x31 (\ and |) and
> > +        * 0x32 (Non-US # and ~) which are both mapped to
> > +        * KEY_BACKSLASH. This will cause spurious events causing

Hmm, makes me wonder if this mapping in hid_keyboard[] is even correct. 
The footnote in HUT 1.12 specifies very different mappings for different 
national keyboards.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[David Herrmann]

Hi

(CC Dmitry for AT details)

On Wed, Sep 10, 2014 at 10:26 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Tue, 9 Sep 2014, Fredrik Hallenberg wrote:
>
>> Hi, time for a bump on this one.
>>
>> On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
>> <megahallon@gmail.com> wrote:
>> > Here is my attempt on a fix for bug 70181, please review it. It is
>> > tested on my nordic Corsair K70, if this solution is deemed acceptable
>> > I can ask some people with other problematic keyboards to test it.
>> >
>> > Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
>> > ---
>> >  drivers/hid/hid-input.c | 14 ++++++++++++++
>> >  include/linux/hid.h     |  1 +
>> >  2 files changed, 15 insertions(+)
>> >
>> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> > index 2619f7f..56429c0 100644
>> > --- a/drivers/hid/hid-input.c
>> > +++ b/drivers/hid/hid-input.c
>> > @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>> >                 return;
>> >         }
>> >
>> > +       /*
>> > +        * Some keyboards will report both HID keys 0x31 (\ and |) and
>> > +        * 0x32 (Non-US # and ~) which are both mapped to
>> > +        * KEY_BACKSLASH. This will cause spurious events causing
>
> Hmm, makes me wonder if this mapping in hid_keyboard[] is even correct.
> The footnote in HUT 1.12 specifies very different mappings for different
> national keyboards.

This is the key that's moved on European layouts due to the different
shape of the 'Enter' key. HID defines two different usage types, key
0x31 and 0x32. It doesn't make sense for a keyboard to enable both, so
this really sounds like a broken hid description to me.

We currently map both those keys to KEY_BACKSLASH, which means
regardless whether it's the European or standard layout, the same
keycodes are generated. So mapping 0x31 and 0x32 to the same key
sounds fine (or at least consistent).

@Fredrik: can you send us your HID descriptor from the broken deivce?
(cat /sys/kernel/debug/hid/<device>/desc).

I wonder why you get key-repeat events, though. I mean, the keyboard
has to send both 0x31 and 0x32 events for you to get those repeat
events. But it really doesn't make sense to me for a keyboard to
report both keys, and furthermore your report sounded like you only
pressed a single key, right?

Thanks
David

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Fredrik Hallenberg]

The keyboards in question have n-key rollover, this seems to be
implemented by sending every single key id every time the keyboard is
polled, this includes several keys that does not exist physically for
example 0x31. This may be a bit excessive but not broken. If I press
a-key and '-key in rapid succession, I get the following events
(omitting all other keys):

VALUE 1 CODE 30 HID 0x4
VALUE 0 CODE 43 HID 0x31
VALUE 0 CODE 43 HID 0x32
Output: a

VALUE 1 CODE 30 HID 0x4
VALUE 0 CODE 43 HID 0x31  - 43 is up
VALUE 1 CODE 43 HID 0x32  - 43 is down
Output: '

VALUE 0 CODE 30 HID 0x4
VALUE 0 CODE 43 HID 0x31  - 43 is up
VALUE 1 CODE 43 HID 0x32  - 43 is down
Output: '

VALUE 0 CODE 30 HID 0x4
VALUE 0 CODE 43 HID 0x31  - 43 is up
VALUE 1 CODE 43 HID 0x32  - 43 is down
Output: '

So even though the keyboard is behaving a bit weird the problem is in
the kernel as it will interpret 0x31 and 0x32 as the same key.

I don't have access to the keyboard at the moment, I will send the
descriptor later today.


On Wed, Sep 10, 2014 at 12:09 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> (CC Dmitry for AT details)
>
> On Wed, Sep 10, 2014 at 10:26 AM, Jiri Kosina <jkosina@suse.cz> wrote:
>> On Tue, 9 Sep 2014, Fredrik Hallenberg wrote:
>>
>>> Hi, time for a bump on this one.
>>>
>>> On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
>>> <megahallon@gmail.com> wrote:
>>> > Here is my attempt on a fix for bug 70181, please review it. It is
>>> > tested on my nordic Corsair K70, if this solution is deemed acceptable
>>> > I can ask some people with other problematic keyboards to test it.
>>> >
>>> > Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
>>> > ---
>>> >  drivers/hid/hid-input.c | 14 ++++++++++++++
>>> >  include/linux/hid.h     |  1 +
>>> >  2 files changed, 15 insertions(+)
>>> >
>>> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>>> > index 2619f7f..56429c0 100644
>>> > --- a/drivers/hid/hid-input.c
>>> > +++ b/drivers/hid/hid-input.c
>>> > @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>>> >                 return;
>>> >         }
>>> >
>>> > +       /*
>>> > +        * Some keyboards will report both HID keys 0x31 (\ and |) and
>>> > +        * 0x32 (Non-US # and ~) which are both mapped to
>>> > +        * KEY_BACKSLASH. This will cause spurious events causing
>>
>> Hmm, makes me wonder if this mapping in hid_keyboard[] is even correct.
>> The footnote in HUT 1.12 specifies very different mappings for different
>> national keyboards.
>
> This is the key that's moved on European layouts due to the different
> shape of the 'Enter' key. HID defines two different usage types, key
> 0x31 and 0x32. It doesn't make sense for a keyboard to enable both, so
> this really sounds like a broken hid description to me.
>
> We currently map both those keys to KEY_BACKSLASH, which means
> regardless whether it's the European or standard layout, the same
> keycodes are generated. So mapping 0x31 and 0x32 to the same key
> sounds fine (or at least consistent).
>
> @Fredrik: can you send us your HID descriptor from the broken deivce?
> (cat /sys/kernel/debug/hid/<device>/desc).
>
> I wonder why you get key-repeat events, though. I mean, the keyboard
> has to send both 0x31 and 0x32 events for you to get those repeat
> events. But it really doesn't make sense to me for a keyboard to
> report both keys, and furthermore your report sounded like you only
> pressed a single key, right?
>
> Thanks
> David

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[David Herrmann]

Hi

On Wed, Sep 10, 2014 at 1:13 PM, Fredrik Hallenberg
<megahallon@gmail.com> wrote:
> The keyboards in question have n-key rollover, this seems to be
> implemented by sending every single key id every time the keyboard is
> polled, this includes several keys that does not exist physically for
> example 0x31. This may be a bit excessive but not broken. If I press
> a-key and '-key in rapid succession, I get the following events
> (omitting all other keys):
>
> VALUE 1 CODE 30 HID 0x4
> VALUE 0 CODE 43 HID 0x31
> VALUE 0 CODE 43 HID 0x32
> Output: a
>
> VALUE 1 CODE 30 HID 0x4
> VALUE 0 CODE 43 HID 0x31  - 43 is up
> VALUE 1 CODE 43 HID 0x32  - 43 is down
> Output: '
>
> VALUE 0 CODE 30 HID 0x4
> VALUE 0 CODE 43 HID 0x31  - 43 is up
> VALUE 1 CODE 43 HID 0x32  - 43 is down
> Output: '
>
> VALUE 0 CODE 30 HID 0x4
> VALUE 0 CODE 43 HID 0x31  - 43 is up
> VALUE 1 CODE 43 HID 0x32  - 43 is down
> Output: '
>
> So even though the keyboard is behaving a bit weird the problem is in
> the kernel as it will interpret 0x31 and 0x32 as the same key.

Thanks for the information! Please include that in follow-up
commit-messages so others can see it as well. I don't think a report
descriptor is needed, anymore.

This indeed explains the problem. We only track keys on the keycode
level, not scancode level. Therefore, you get weird key-up or
key-repeat events depending on the scan-order.

The nicest fix, obviously, is to blacklist keys that are not
physically present on the keyboard. But I assume the keyboard reports
either key depending on the model (standard vs. European). Given that
this key is indeed special as we only have a single keycode for it, we
probably need some quirk like yours. I'd like to hear Dmitry's
comments on this, maybe he has had similar problems with non-HID
keyboards.

Thanks
David

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Dmitry Torokhov]

On Wed, Sep 10, 2014 at 04:22:13PM +0200, David Herrmann wrote:
> Hi
> 
> On Wed, Sep 10, 2014 at 1:13 PM, Fredrik Hallenberg
> <megahallon@gmail.com> wrote:
> > The keyboards in question have n-key rollover, this seems to be
> > implemented by sending every single key id every time the keyboard is
> > polled, this includes several keys that does not exist physically for
> > example 0x31. This may be a bit excessive but not broken. If I press
> > a-key and '-key in rapid succession, I get the following events
> > (omitting all other keys):
> >
> > VALUE 1 CODE 30 HID 0x4
> > VALUE 0 CODE 43 HID 0x31
> > VALUE 0 CODE 43 HID 0x32
> > Output: a
> >
> > VALUE 1 CODE 30 HID 0x4
> > VALUE 0 CODE 43 HID 0x31  - 43 is up
> > VALUE 1 CODE 43 HID 0x32  - 43 is down
> > Output: '
> >
> > VALUE 0 CODE 30 HID 0x4
> > VALUE 0 CODE 43 HID 0x31  - 43 is up
> > VALUE 1 CODE 43 HID 0x32  - 43 is down
> > Output: '
> >
> > VALUE 0 CODE 30 HID 0x4
> > VALUE 0 CODE 43 HID 0x31  - 43 is up
> > VALUE 1 CODE 43 HID 0x32  - 43 is down
> > Output: '
> >
> > So even though the keyboard is behaving a bit weird the problem is in
> > the kernel as it will interpret 0x31 and 0x32 as the same key.
> 
> Thanks for the information! Please include that in follow-up
> commit-messages so others can see it as well. I don't think a report
> descriptor is needed, anymore.
> 
> This indeed explains the problem. We only track keys on the keycode
> level, not scancode level. Therefore, you get weird key-up or
> key-repeat events depending on the scan-order.
> 
> The nicest fix, obviously, is to blacklist keys that are not
> physically present on the keyboard. But I assume the keyboard reports

Hmm, somebody could still load keymap with duplicate keycodes though...

>
> either key depending on the model (standard vs. European). Given that
> this key is indeed special as we only have a single keycode for it, we
> probably need some quirk like yours. I'd like to hear Dmitry's
> comments on this, maybe he has had similar problems with non-HID
> keyboards.

Hmm, I do not think we have a good story for duplicate keycodes for
drivers that send entire state as opposed to just changed bits.

Thanks.

-- 
Dmitry

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Fredrik Hallenberg]

>> The nicest fix, obviously, is to blacklist keys that are not
>> physically present on the keyboard. But I assume the keyboard reports
>
> Hmm, somebody could still load keymap with duplicate keycodes though...
>

The HID mapping is hardcoded, so only key code 43 will have this
problem regardless of user keymaps.

Regarding blacklisting it would be good but I don't think there is any
way to know in advance which key (0x31 or 0x32) the keyboard actually
uses. If anybody has ideas on how this could be done let me know.

Jiri, do you still think this should be a hardware specific quirk?

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[David Herrmann]

Hi

On Thu, Sep 11, 2014 at 10:50 AM, Fredrik Hallenberg
<megahallon@gmail.com> wrote:
>>> The nicest fix, obviously, is to blacklist keys that are not
>>> physically present on the keyboard. But I assume the keyboard reports
>>
>> Hmm, somebody could still load keymap with duplicate keycodes though...
>>
>
> The HID mapping is hardcoded, so only key code 43 will have this
> problem regardless of user keymaps.

I think Dmitry is talking about setkeycode() which can change the
keycode a scancode is mapped to. This is indeed a problem, but I
thought we implicitly tell people to not map two scancodes to the same
keycode.. we simply don't support it (as we only have 1bit state per
keycode, and 0bit per scancode).

But this reminds me: we can use udev hwdb and setkeycode() to fix your
keyboard. Simply add this to /lib/udev/hwdb.d/60-keyboard.hwdb:
    keyboard:name:*Corsair*:dmi:bvn*:bvr*:bd*:svn*:pn*:pvr*
        KEYBOARD_KEY_32=0

this will run setkeycode(0x32, KEY_RESERVED) and thus disable the key
0x32 entirely.
Adding properly crafted match-rules to udev should solve this mess, I
guess. Imho, this is the cleanest solution, but depends on udev, of
course.

Thanks
David

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Fredrik Hallenberg]

Thanks I had no idea about the hwdb but I have tried it now, it works if I use

KEYBOARD_KEY_70031=reserved

on my nordic keyboard, but a US keyboard user would have to use
scancode 70032 instead. If you chose the wrong one the backslash key
will be disabled.

I don't think this is good solution as long as there is no way to
detect which version is used with the hardware matcher string.

However I suppose udev should make it possible to do some kind of
script that does something similar to my original patch, that is,
detects which of the two keys in actually used and disables the other
one.

On Thu, Sep 11, 2014 at 2:41 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Thu, Sep 11, 2014 at 10:50 AM, Fredrik Hallenberg
> <megahallon@gmail.com> wrote:
>>>> The nicest fix, obviously, is to blacklist keys that are not
>>>> physically present on the keyboard. But I assume the keyboard reports
>>>
>>> Hmm, somebody could still load keymap with duplicate keycodes though...
>>>
>>
>> The HID mapping is hardcoded, so only key code 43 will have this
>> problem regardless of user keymaps.
>
> I think Dmitry is talking about setkeycode() which can change the
> keycode a scancode is mapped to. This is indeed a problem, but I
> thought we implicitly tell people to not map two scancodes to the same
> keycode.. we simply don't support it (as we only have 1bit state per
> keycode, and 0bit per scancode).
>
> But this reminds me: we can use udev hwdb and setkeycode() to fix your
> keyboard. Simply add this to /lib/udev/hwdb.d/60-keyboard.hwdb:
>     keyboard:name:*Corsair*:dmi:bvn*:bvr*:bd*:svn*:pn*:pvr*
>         KEYBOARD_KEY_32=0
>
> this will run setkeycode(0x32, KEY_RESERVED) and thus disable the key
> 0x32 entirely.
> Adding properly crafted match-rules to udev should solve this mess, I
> guess. Imho, this is the cleanest solution, but depends on udev, of
> course.
>
> Thanks
> David

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[David Herrmann]

Hi

On Thu, Sep 11, 2014 at 10:35 PM, Fredrik Hallenberg
<megahallon@gmail.com> wrote:
> Thanks I had no idea about the hwdb but I have tried it now, it works if I use
>
> KEYBOARD_KEY_70031=reserved

Thanks for confirming!

> on my nordic keyboard, but a US keyboard user would have to use
> scancode 70032 instead. If you chose the wrong one the backslash key
> will be disabled.
>
> I don't think this is good solution as long as there is no way to
> detect which version is used with the hardware matcher string.

Well, you can match on "country" tags. Currently, the device
identifiers contain vendor+product+version+country information. I
assume the "country" field is different between your keyboard and a
keyboard that swaps the key reports.

> However I suppose udev should make it possible to do some kind of
> script that does something similar to my original patch, that is,
> detects which of the two keys in actually used and disables the other
> one.

If you post your mod-alias line of your device (or better: all
information you can gather about the device), I can push it into hwdb.
It is up to Dmitry and Jiri to decide whether it makes sense to deal
with it in the kernel.

Thanks
David

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Fredrik Hallenberg]

Is the country tag derived from the bCountryCode seen in the HID
device descriptor seen when doing "lsusb -v"?
If so it probably wont work as it says "0 (Not supported)" on my
keyboard and a few others I have tested.

On Fri, Sep 12, 2014 at 12:29 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Thu, Sep 11, 2014 at 10:35 PM, Fredrik Hallenberg
> <megahallon@gmail.com> wrote:
>> Thanks I had no idea about the hwdb but I have tried it now, it works if I use
>>
>> KEYBOARD_KEY_70031=reserved
>
> Thanks for confirming!
>
>> on my nordic keyboard, but a US keyboard user would have to use
>> scancode 70032 instead. If you chose the wrong one the backslash key
>> will be disabled.
>>
>> I don't think this is good solution as long as there is no way to
>> detect which version is used with the hardware matcher string.
>
> Well, you can match on "country" tags. Currently, the device
> identifiers contain vendor+product+version+country information. I
> assume the "country" field is different between your keyboard and a
> keyboard that swaps the key reports.
>
>> However I suppose udev should make it possible to do some kind of
>> script that does something similar to my original patch, that is,
>> detects which of the two keys in actually used and disables the other
>> one.
>
> If you post your mod-alias line of your device (or better: all
> information you can gather about the device), I can push it into hwdb.
> It is up to Dmitry and Jiri to decide whether it makes sense to deal
> with it in the kernel.
>
> Thanks
> David

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[David Herrmann]

Hi

On Fri, Sep 12, 2014 at 1:34 PM, Fredrik Hallenberg
<megahallon@gmail.com> wrote:
> Is the country tag derived from the bCountryCode seen in the HID
> device descriptor seen when doing "lsusb -v"?

Yes, it is.

> If so it probably wont work as it says "0 (Not supported)" on my
> keyboard and a few others I have tested.

"0" might also be used as default, so it's not surprising that your
US-style keyboard uses "0".

Furthermore, the non-US style keyboards might even use different
product IDs or version numbers. So it'd be nice if you could include a
full set of device information here.

Thanks
David

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Fredrik Hallenberg]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

Here is the output from lsusb, let me know if you need more information.

Note this is the nordic version so if they are using the country code
at all it should have some other value than 0. As I mentioned I
checked a few other nordic/swedish keyboards and all of them use code
0.

Anyway as you say there could be some other value that could be used,
I will try to find someone with a US-version of this keyboard and
compare the values.

On Fri, Sep 12, 2014 at 1:47 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Fri, Sep 12, 2014 at 1:34 PM, Fredrik Hallenberg
> <megahallon@gmail.com> wrote:
>> Is the country tag derived from the bCountryCode seen in the HID
>> device descriptor seen when doing "lsusb -v"?
>
> Yes, it is.
>
>> If so it probably wont work as it says "0 (Not supported)" on my
>> keyboard and a few others I have tested.
>
> "0" might also be used as default, so it's not surprising that your
> US-style keyboard uses "0".
>
> Furthermore, the non-US style keyboards might even use different
> product IDs or version numbers. So it'd be nice if you could include a
> full set of device information here.
>
> Thanks
> David

[-- Attachment #2: corsair_k70.txt --]
[-- Type: text/plain, Size: 4187 bytes --]


Bus 001 Device 003: ID 1b1c:1b09 Corsair 
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               2.00
  bDeviceClass            0 (Defined at Interface level)
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0        64
  idVendor           0x1b1c Corsair
  idProduct          0x1b09 
  bcdDevice            1.09
  iManufacturer           1 Corsair
  iProduct                2 Corsair K70R Gaming Keyboard
  iSerial                 0 
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength           84
    bNumInterfaces          3
    bConfigurationValue     1
    iConfiguration          0 
    bmAttributes         0xa0
      (Bus Powered)
      Remote Wakeup
    MaxPower              500mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         3 Human Interface Device
      bInterfaceSubClass      1 Boot Interface Subclass
      bInterfaceProtocol      1 Keyboard
      iInterface              0 
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.11
          bCountryCode            0 Not supported
          bNumDescriptors         1
          bDescriptorType        34 Report
          wDescriptorLength      65
         Report Descriptors: 
           ** UNAVAILABLE **
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0008  1x 8 bytes
        bInterval               1
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         3 Human Interface Device
      bInterfaceSubClass      0 No Subclass
      bInterfaceProtocol      0 None
      iInterface              0 
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.11
          bCountryCode            0 Not supported
          bNumDescriptors         1
          bDescriptorType        34 Report
          wDescriptorLength      25
         Report Descriptors: 
           ** UNAVAILABLE **
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0004  1x 4 bytes
        bInterval               1
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        2
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         3 Human Interface Device
      bInterfaceSubClass      0 No Subclass
      bInterfaceProtocol      0 None
      iInterface              0 
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.11
          bCountryCode            0 Not supported
          bNumDescriptors         1
          bDescriptorType        34 Report
          wDescriptorLength      37
         Report Descriptors: 
           ** UNAVAILABLE **
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x000f  1x 15 bytes
        bInterval               1
Device Status:     0x0000
  (Bus Powered)

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[RH]

Hello,

I experience the same issue with my new keyboard:
*Corsair Raptor K30 (German version)*

The issue can be fixed/worked around with this rule in hwdb:

---------------
#keyboard:name:*Corsair*:dmi:bvn*:bvr*:bd*:svn*:pn*:pvr*
keyboard:usb:v1B1Cp1B0A*
 KEYBOARD_KEY_70031=reserved
---------------

So you think the US version won't work with this anymore? I guess they 
will send both scancodes, too.

This is the output of lsusb:
---------------
Bus 004 Device 008: ID 1b1c:1b0a Corsair 
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               2.00
  bDeviceClass            0 (Defined at Interface level)
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0        64
  idVendor           0x1b1c Corsair
  idProduct          0x1b0a 
  bcdDevice            1.03
  iManufacturer           1 Corsair
  iProduct                2 Corsair K30A Gaming Keyboard
  iSerial                 0 
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength           84
    bNumInterfaces          3
    bConfigurationValue     1
    iConfiguration          0 
    bmAttributes         0xa0
      (Bus Powered)
      Remote Wakeup
    MaxPower              500mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         3 Human Interface Device
      bInterfaceSubClass      1 Boot Interface Subclass
      bInterfaceProtocol      1 Keyboard
      iInterface              0 
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.11
          bCountryCode            0 Not supported
          bNumDescriptors         1
          bDescriptorType        34 Report
          wDescriptorLength      65
         Report Descriptors: 
           ** UNAVAILABLE **
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0008  1x 8 bytes
        bInterval               1
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         3 Human Interface Device
      bInterfaceSubClass      0 No Subclass
      bInterfaceProtocol      0 None
      iInterface              0 
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.11
          bCountryCode            0 Not supported
          bNumDescriptors         1
          bDescriptorType        34 Report
          wDescriptorLength      25
         Report Descriptors: 
           ** UNAVAILABLE **
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x82  EP 2 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0004  1x 4 bytes
        bInterval               1
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        2
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         3 Human Interface Device
      bInterfaceSubClass      0 No Subclass
      bInterfaceProtocol      0 None
      iInterface              0 
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.11
          bCountryCode            0 Not supported
          bNumDescriptors         1
          bDescriptorType        34 Report
          wDescriptorLength      37
         Report Descriptors: 
           ** UNAVAILABLE **
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x000f  1x 15 bytes
        bInterval               1
Device Status:     0x0000
  (Bus Powered)
---------------

Did you already find someone with a US keyboard? Shall I write in the 
corsair forums or to the support to find out whether US keyboards have 
the same product IDs? If this is not the case, we could safely use vendor/
product IDs for the workaround.

I'd really like to submit working hwdb lines to the udev database to save 
other users from having these troubles.

Also, do you know why

Greetings,
RH

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Fredrik Hallenberg]

> Did you already find someone with a US keyboard?

Yes as mentioned in thread it does not seem possible to write a
hwdb-rule that works for both US and non-US keyboard. Also it is a
general problem that affects other keyboards than Corsair, again see
previous mails in this thread.

David, it would be great if you could get your patch ready so this one
could be closed. If you don't have the time to do it, perhaps my
original patch could be considered again. I fully aggree that it is a
hack but it does solve the problem with little risk of bad side
effects. It can always be removed later we you have a proper fix.

Regards,

Fredrik


On Fri, Dec 19, 2014 at 5:28 PM, RH <r@hirner.at> wrote:
> Hello,
>
> I experience the same issue with my new keyboard:
> *Corsair Raptor K30 (German version)*
>
> The issue can be fixed/worked around with this rule in hwdb:
>
> ---------------
> #keyboard:name:*Corsair*:dmi:bvn*:bvr*:bd*:svn*:pn*:pvr*
> keyboard:usb:v1B1Cp1B0A*
>  KEYBOARD_KEY_70031=reserved
> ---------------
>
> So you think the US version won't work with this anymore? I guess they
> will send both scancodes, too.
>
> This is the output of lsusb:
> ---------------
> Bus 004 Device 008: ID 1b1c:1b0a Corsair
> Device Descriptor:
>   bLength                18
>   bDescriptorType         1
>   bcdUSB               2.00
>   bDeviceClass            0 (Defined at Interface level)
>   bDeviceSubClass         0
>   bDeviceProtocol         0
>   bMaxPacketSize0        64
>   idVendor           0x1b1c Corsair
>   idProduct          0x1b0a
>   bcdDevice            1.03
>   iManufacturer           1 Corsair
>   iProduct                2 Corsair K30A Gaming Keyboard
>   iSerial                 0
>   bNumConfigurations      1
>   Configuration Descriptor:
>     bLength                 9
>     bDescriptorType         2
>     wTotalLength           84
>     bNumInterfaces          3
>     bConfigurationValue     1
>     iConfiguration          0
>     bmAttributes         0xa0
>       (Bus Powered)
>       Remote Wakeup
>     MaxPower              500mA
>     Interface Descriptor:
>       bLength                 9
>       bDescriptorType         4
>       bInterfaceNumber        0
>       bAlternateSetting       0
>       bNumEndpoints           1
>       bInterfaceClass         3 Human Interface Device
>       bInterfaceSubClass      1 Boot Interface Subclass
>       bInterfaceProtocol      1 Keyboard
>       iInterface              0
>         HID Device Descriptor:
>           bLength                 9
>           bDescriptorType        33
>           bcdHID               1.11
>           bCountryCode            0 Not supported
>           bNumDescriptors         1
>           bDescriptorType        34 Report
>           wDescriptorLength      65
>          Report Descriptors:
>            ** UNAVAILABLE **
>       Endpoint Descriptor:
>         bLength                 7
>         bDescriptorType         5
>         bEndpointAddress     0x81  EP 1 IN
>         bmAttributes            3
>           Transfer Type            Interrupt
>           Synch Type               None
>           Usage Type               Data
>         wMaxPacketSize     0x0008  1x 8 bytes
>         bInterval               1
>     Interface Descriptor:
>       bLength                 9
>       bDescriptorType         4
>       bInterfaceNumber        1
>       bAlternateSetting       0
>       bNumEndpoints           1
>       bInterfaceClass         3 Human Interface Device
>       bInterfaceSubClass      0 No Subclass
>       bInterfaceProtocol      0 None
>       iInterface              0
>         HID Device Descriptor:
>           bLength                 9
>           bDescriptorType        33
>           bcdHID               1.11
>           bCountryCode            0 Not supported
>           bNumDescriptors         1
>           bDescriptorType        34 Report
>           wDescriptorLength      25
>          Report Descriptors:
>            ** UNAVAILABLE **
>       Endpoint Descriptor:
>         bLength                 7
>         bDescriptorType         5
>         bEndpointAddress     0x82  EP 2 IN
>         bmAttributes            3
>           Transfer Type            Interrupt
>           Synch Type               None
>           Usage Type               Data
>         wMaxPacketSize     0x0004  1x 4 bytes
>         bInterval               1
>     Interface Descriptor:
>       bLength                 9
>       bDescriptorType         4
>       bInterfaceNumber        2
>       bAlternateSetting       0
>       bNumEndpoints           1
>       bInterfaceClass         3 Human Interface Device
>       bInterfaceSubClass      0 No Subclass
>       bInterfaceProtocol      0 None
>       iInterface              0
>         HID Device Descriptor:
>           bLength                 9
>           bDescriptorType        33
>           bcdHID               1.11
>           bCountryCode            0 Not supported
>           bNumDescriptors         1
>           bDescriptorType        34 Report
>           wDescriptorLength      37
>          Report Descriptors:
>            ** UNAVAILABLE **
>       Endpoint Descriptor:
>         bLength                 7
>         bDescriptorType         5
>         bEndpointAddress     0x83  EP 3 IN
>         bmAttributes            3
>           Transfer Type            Interrupt
>           Synch Type               None
>           Usage Type               Data
>         wMaxPacketSize     0x000f  1x 15 bytes
>         bInterval               1
> Device Status:     0x0000
>   (Bus Powered)
> ---------------
>
> Did you already find someone with a US keyboard? Shall I write in the
> corsair forums or to the support to find out whether US keyboards have
> the same product IDs? If this is not the case, we could safely use vendor/
> product IDs for the workaround.
>
> I'd really like to submit working hwdb lines to the udev database to save
> other users from having these troubles.
>
> Also, do you know why
>
> Greetings,
> RH
>
> --
> 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] Handle spurious backslash key repeats on some keyboards

[David Herrmann]

Hi

On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
<megahallon@gmail.com> wrote:
> Here is my attempt on a fix for bug 70181, please review it. It is
> tested on my nordic Corsair K70, if this solution is deemed acceptable
> I can ask some people with other problematic keyboards to test it.
>
> Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
> ---
>  drivers/hid/hid-input.c | 14 ++++++++++++++
>  include/linux/hid.h     |  1 +
>  2 files changed, 15 insertions(+)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 2619f7f..56429c0 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>                 return;
>         }
>
> +       /*
> +        * Some keyboards will report both HID keys 0x31 (\ and |) and
> +        * 0x32 (Non-US # and ~) which are both mapped to
> +        * KEY_BACKSLASH. This will cause spurious events causing
> +        * repeated backslash key presses. Handle this by tracking the
> +        * active HID code and ignoring the other one.
> +        */
> +       if (usage->type == EV_KEY && usage->code == KEY_BACKSLASH) {
> +               if (value)
> +                       field->hidinput->backslash_usage = usage->hid;
> +               else if (field->hidinput->backslash_usage != usage->hid)
> +                       return;
> +       }
> +

Ok, I have looked through some more reports and at a bunch of AT
keyboards and how the HID standard maps them. The backslash/pipe key
is the only key affected by this. There _might_ be some similar
overlap with Korean 106 and Japanese 109 layouts, but as there haven't
been any reports, we probably don't care right now.

And indeed, I'm kinda reluctant to add an hwdb entry for the reported
keyboards as I couldn't find anyone with non-105 keyboards to compare
VID/PID. Furthermore, similar issues will probably show up again in
the future. However, I still dislike the quick hack in this patch. I
especially dislike matching on KEY_BACKSLASH while we really want to
match on 0x31 and 0x32. If users use setkeycode() to change a mapping,
this should not trigger our quirk. We could easily change the patch to
match on usage, but I think there's a better way:

hid-core.c: hid_input_field()
This is the main entry point for data that is matched on HID fields.
We already have a RollOver quirk in there, which is extremely bad
documented and I have no idea what it does.. However, this function
saves the reported values so we can retrieve them on a following
report. Why don't we skip events that have the same value reported as
before? Obviously, we should do this only for absolute data, not
relative. So I think something like this should work (sorry for
line-wraps):

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 12b6e67..000a768 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1220,7 +1220,10 @@
        for (n = 0; n < count; n++) {

                if (HID_MAIN_ITEM_VARIABLE & field->flags) {
-                       hid_process_event(hid, field,
&field->usage[n], value[n], interrupt);
+                       /* ignore absolute values if they didn't change */
+                       if (HID_MAIN_ITEM_RELATIVE & field->flags ||
+                           value[n] != field->value[n])
+                               hid_process_event(hid, field,
&field->usage[n], value[n], interrupt);
                        continue;
                }


@Jiri: Any comments on this? I now agree with Fredrik that this is
better solved in HID core. It is a generic problem..

In HID, we save values per scancode / HID-field, additionally to
input-core saving them per keycode / input-code. This patch basically
makes use of this and avoids sending events for scan-codes that didn't
change, but accidentally map to the same keycode as another key (which
might have a different state than this one).

If this looks reasonable, I can prepare a patch with a proper
commit-log and give it a spin here.

Thanks
David

>         /* 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);
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index f53c4a9..1c59f8f 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -448,6 +448,7 @@ struct hid_input {
>         struct list_head list;
>         struct hid_report *report;
>         struct input_dev *input;
> +       unsigned backslash_usage;
>  };
>
>  enum hid_type {
> --
> 2.1.0.rc1
>
> --
> 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] Handle spurious backslash key repeats on some keyboards

[David Herrmann]

Hi

On Sun, Sep 14, 2014 at 6:45 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
> <megahallon@gmail.com> wrote:
>> Here is my attempt on a fix for bug 70181, please review it. It is
>> tested on my nordic Corsair K70, if this solution is deemed acceptable
>> I can ask some people with other problematic keyboards to test it.
>>
>> Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
>> ---
>>  drivers/hid/hid-input.c | 14 ++++++++++++++
>>  include/linux/hid.h     |  1 +
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 2619f7f..56429c0 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>>                 return;
>>         }
>>
>> +       /*
>> +        * Some keyboards will report both HID keys 0x31 (\ and |) and
>> +        * 0x32 (Non-US # and ~) which are both mapped to
>> +        * KEY_BACKSLASH. This will cause spurious events causing
>> +        * repeated backslash key presses. Handle this by tracking the
>> +        * active HID code and ignoring the other one.
>> +        */
>> +       if (usage->type == EV_KEY && usage->code == KEY_BACKSLASH) {
>> +               if (value)
>> +                       field->hidinput->backslash_usage = usage->hid;
>> +               else if (field->hidinput->backslash_usage != usage->hid)
>> +                       return;
>> +       }
>> +
>
> Ok, I have looked through some more reports and at a bunch of AT
> keyboards and how the HID standard maps them. The backslash/pipe key
> is the only key affected by this. There _might_ be some similar
> overlap with Korean 106 and Japanese 109 layouts, but as there haven't
> been any reports, we probably don't care right now.
>
> And indeed, I'm kinda reluctant to add an hwdb entry for the reported
> keyboards as I couldn't find anyone with non-105 keyboards to compare
> VID/PID. Furthermore, similar issues will probably show up again in
> the future. However, I still dislike the quick hack in this patch. I
> especially dislike matching on KEY_BACKSLASH while we really want to
> match on 0x31 and 0x32. If users use setkeycode() to change a mapping,
> this should not trigger our quirk. We could easily change the patch to
> match on usage, but I think there's a better way:
>
> hid-core.c: hid_input_field()
> This is the main entry point for data that is matched on HID fields.
> We already have a RollOver quirk in there, which is extremely bad
> documented and I have no idea what it does.. However, this function
> saves the reported values so we can retrieve them on a following
> report. Why don't we skip events that have the same value reported as
> before? Obviously, we should do this only for absolute data, not
> relative. So I think something like this should work (sorry for
> line-wraps):
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 12b6e67..000a768 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1220,7 +1220,10 @@
>         for (n = 0; n < count; n++) {
>
>                 if (HID_MAIN_ITEM_VARIABLE & field->flags) {
> -                       hid_process_event(hid, field,
> &field->usage[n], value[n], interrupt);
> +                       /* ignore absolute values if they didn't change */
> +                       if (HID_MAIN_ITEM_RELATIVE & field->flags ||
> +                           value[n] != field->value[n])
> +                               hid_process_event(hid, field,
> &field->usage[n], value[n], interrupt);

This should probably be:
  (HID_MAIN_ITEM_RELATIVE | HID_MAIN_ITEM_BUFFERED_BYTE)
..though I wonder whether buffered-byte makes much sense without "relative"..

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Fredrik Hallenberg]

Thank you for this, I felt that it would be difficult to do a good
solution with hwdb but could not really back this up. It is also nice
to see a much better patch than mine, I did not have enough knowledge
on the input system to do a proper one.

On Sun, Sep 14, 2014 at 6:45 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
> <megahallon@gmail.com> wrote:
>> Here is my attempt on a fix for bug 70181, please review it. It is
>> tested on my nordic Corsair K70, if this solution is deemed acceptable
>> I can ask some people with other problematic keyboards to test it.
>>
>> Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
>> ---
>>  drivers/hid/hid-input.c | 14 ++++++++++++++
>>  include/linux/hid.h     |  1 +
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 2619f7f..56429c0 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>>                 return;
>>         }
>>
>> +       /*
>> +        * Some keyboards will report both HID keys 0x31 (\ and |) and
>> +        * 0x32 (Non-US # and ~) which are both mapped to
>> +        * KEY_BACKSLASH. This will cause spurious events causing
>> +        * repeated backslash key presses. Handle this by tracking the
>> +        * active HID code and ignoring the other one.
>> +        */
>> +       if (usage->type == EV_KEY && usage->code == KEY_BACKSLASH) {
>> +               if (value)
>> +                       field->hidinput->backslash_usage = usage->hid;
>> +               else if (field->hidinput->backslash_usage != usage->hid)
>> +                       return;
>> +       }
>> +
>
> Ok, I have looked through some more reports and at a bunch of AT
> keyboards and how the HID standard maps them. The backslash/pipe key
> is the only key affected by this. There _might_ be some similar
> overlap with Korean 106 and Japanese 109 layouts, but as there haven't
> been any reports, we probably don't care right now.
>
> And indeed, I'm kinda reluctant to add an hwdb entry for the reported
> keyboards as I couldn't find anyone with non-105 keyboards to compare
> VID/PID. Furthermore, similar issues will probably show up again in
> the future. However, I still dislike the quick hack in this patch. I
> especially dislike matching on KEY_BACKSLASH while we really want to
> match on 0x31 and 0x32. If users use setkeycode() to change a mapping,
> this should not trigger our quirk. We could easily change the patch to
> match on usage, but I think there's a better way:
>
> hid-core.c: hid_input_field()
> This is the main entry point for data that is matched on HID fields.
> We already have a RollOver quirk in there, which is extremely bad
> documented and I have no idea what it does.. However, this function
> saves the reported values so we can retrieve them on a following
> report. Why don't we skip events that have the same value reported as
> before? Obviously, we should do this only for absolute data, not
> relative. So I think something like this should work (sorry for
> line-wraps):
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 12b6e67..000a768 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1220,7 +1220,10 @@
>         for (n = 0; n < count; n++) {
>
>                 if (HID_MAIN_ITEM_VARIABLE & field->flags) {
> -                       hid_process_event(hid, field,
> &field->usage[n], value[n], interrupt);
> +                       /* ignore absolute values if they didn't change */
> +                       if (HID_MAIN_ITEM_RELATIVE & field->flags ||
> +                           value[n] != field->value[n])
> +                               hid_process_event(hid, field,
> &field->usage[n], value[n], interrupt);
>                         continue;
>                 }
>
>
> @Jiri: Any comments on this? I now agree with Fredrik that this is
> better solved in HID core. It is a generic problem..
>
> In HID, we save values per scancode / HID-field, additionally to
> input-core saving them per keycode / input-code. This patch basically
> makes use of this and avoids sending events for scan-codes that didn't
> change, but accidentally map to the same keycode as another key (which
> might have a different state than this one).
>
> If this looks reasonable, I can prepare a patch with a proper
> commit-log and give it a spin here.
>
> Thanks
> David
>
>>         /* 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);
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index f53c4a9..1c59f8f 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -448,6 +448,7 @@ struct hid_input {
>>         struct list_head list;
>>         struct hid_report *report;
>>         struct input_dev *input;
>> +       unsigned backslash_usage;
>>  };
>>
>>  enum hid_type {
>> --
>> 2.1.0.rc1
>>
>> --
>> 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] Handle spurious backslash key repeats on some keyboards

[Fredrik Hallenberg]

FYI I just got a "lsusb -v" dump from a US version of my keyboard, and
it is exactly the same as the one from my nordic version.

On Mon, Sep 15, 2014 at 12:53 AM, Fredrik Hallenberg
<megahallon@gmail.com> wrote:
> Thank you for this, I felt that it would be difficult to do a good
> solution with hwdb but could not really back this up. It is also nice
> to see a much better patch than mine, I did not have enough knowledge
> on the input system to do a proper one.
>
> On Sun, Sep 14, 2014 at 6:45 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> Hi
>>
>> On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
>> <megahallon@gmail.com> wrote:
>>> Here is my attempt on a fix for bug 70181, please review it. It is
>>> tested on my nordic Corsair K70, if this solution is deemed acceptable
>>> I can ask some people with other problematic keyboards to test it.
>>>
>>> Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
>>> ---
>>>  drivers/hid/hid-input.c | 14 ++++++++++++++
>>>  include/linux/hid.h     |  1 +
>>>  2 files changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>>> index 2619f7f..56429c0 100644
>>> --- a/drivers/hid/hid-input.c
>>> +++ b/drivers/hid/hid-input.c
>>> @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>>>                 return;
>>>         }
>>>
>>> +       /*
>>> +        * Some keyboards will report both HID keys 0x31 (\ and |) and
>>> +        * 0x32 (Non-US # and ~) which are both mapped to
>>> +        * KEY_BACKSLASH. This will cause spurious events causing
>>> +        * repeated backslash key presses. Handle this by tracking the
>>> +        * active HID code and ignoring the other one.
>>> +        */
>>> +       if (usage->type == EV_KEY && usage->code == KEY_BACKSLASH) {
>>> +               if (value)
>>> +                       field->hidinput->backslash_usage = usage->hid;
>>> +               else if (field->hidinput->backslash_usage != usage->hid)
>>> +                       return;
>>> +       }
>>> +
>>
>> Ok, I have looked through some more reports and at a bunch of AT
>> keyboards and how the HID standard maps them. The backslash/pipe key
>> is the only key affected by this. There _might_ be some similar
>> overlap with Korean 106 and Japanese 109 layouts, but as there haven't
>> been any reports, we probably don't care right now.
>>
>> And indeed, I'm kinda reluctant to add an hwdb entry for the reported
>> keyboards as I couldn't find anyone with non-105 keyboards to compare
>> VID/PID. Furthermore, similar issues will probably show up again in
>> the future. However, I still dislike the quick hack in this patch. I
>> especially dislike matching on KEY_BACKSLASH while we really want to
>> match on 0x31 and 0x32. If users use setkeycode() to change a mapping,
>> this should not trigger our quirk. We could easily change the patch to
>> match on usage, but I think there's a better way:
>>
>> hid-core.c: hid_input_field()
>> This is the main entry point for data that is matched on HID fields.
>> We already have a RollOver quirk in there, which is extremely bad
>> documented and I have no idea what it does.. However, this function
>> saves the reported values so we can retrieve them on a following
>> report. Why don't we skip events that have the same value reported as
>> before? Obviously, we should do this only for absolute data, not
>> relative. So I think something like this should work (sorry for
>> line-wraps):
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 12b6e67..000a768 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1220,7 +1220,10 @@
>>         for (n = 0; n < count; n++) {
>>
>>                 if (HID_MAIN_ITEM_VARIABLE & field->flags) {
>> -                       hid_process_event(hid, field,
>> &field->usage[n], value[n], interrupt);
>> +                       /* ignore absolute values if they didn't change */
>> +                       if (HID_MAIN_ITEM_RELATIVE & field->flags ||
>> +                           value[n] != field->value[n])
>> +                               hid_process_event(hid, field,
>> &field->usage[n], value[n], interrupt);
>>                         continue;
>>                 }
>>
>>
>> @Jiri: Any comments on this? I now agree with Fredrik that this is
>> better solved in HID core. It is a generic problem..
>>
>> In HID, we save values per scancode / HID-field, additionally to
>> input-core saving them per keycode / input-code. This patch basically
>> makes use of this and avoids sending events for scan-codes that didn't
>> change, but accidentally map to the same keycode as another key (which
>> might have a different state than this one).
>>
>> If this looks reasonable, I can prepare a patch with a proper
>> commit-log and give it a spin here.
>>
>> Thanks
>> David
>>
>>>         /* 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);
>>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>>> index f53c4a9..1c59f8f 100644
>>> --- a/include/linux/hid.h
>>> +++ b/include/linux/hid.h
>>> @@ -448,6 +448,7 @@ struct hid_input {
>>>         struct list_head list;
>>>         struct hid_report *report;
>>>         struct input_dev *input;
>>> +       unsigned backslash_usage;
>>>  };
>>>
>>>  enum hid_type {
>>> --
>>> 2.1.0.rc1
>>>
>>> --
>>> 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] Handle spurious backslash key repeats on some keyboards

[Benjamin Tissoires]

On Sun, Sep 14, 2014 at 12:45 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
> <megahallon@gmail.com> wrote:
>> Here is my attempt on a fix for bug 70181, please review it. It is
>> tested on my nordic Corsair K70, if this solution is deemed acceptable
>> I can ask some people with other problematic keyboards to test it.
>>
>> Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
>> ---
>>  drivers/hid/hid-input.c | 14 ++++++++++++++
>>  include/linux/hid.h     |  1 +
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 2619f7f..56429c0 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>>                 return;
>>         }
>>
>> +       /*
>> +        * Some keyboards will report both HID keys 0x31 (\ and |) and
>> +        * 0x32 (Non-US # and ~) which are both mapped to
>> +        * KEY_BACKSLASH. This will cause spurious events causing
>> +        * repeated backslash key presses. Handle this by tracking the
>> +        * active HID code and ignoring the other one.
>> +        */
>> +       if (usage->type == EV_KEY && usage->code == KEY_BACKSLASH) {
>> +               if (value)
>> +                       field->hidinput->backslash_usage = usage->hid;
>> +               else if (field->hidinput->backslash_usage != usage->hid)
>> +                       return;
>> +       }
>> +
>
> Ok, I have looked through some more reports and at a bunch of AT
> keyboards and how the HID standard maps them. The backslash/pipe key
> is the only key affected by this. There _might_ be some similar
> overlap with Korean 106 and Japanese 109 layouts, but as there haven't
> been any reports, we probably don't care right now.
>
> And indeed, I'm kinda reluctant to add an hwdb entry for the reported
> keyboards as I couldn't find anyone with non-105 keyboards to compare
> VID/PID. Furthermore, similar issues will probably show up again in
> the future. However, I still dislike the quick hack in this patch. I
> especially dislike matching on KEY_BACKSLASH while we really want to
> match on 0x31 and 0x32. If users use setkeycode() to change a mapping,
> this should not trigger our quirk. We could easily change the patch to
> match on usage, but I think there's a better way:
>
> hid-core.c: hid_input_field()
> This is the main entry point for data that is matched on HID fields.
> We already have a RollOver quirk in there, which is extremely bad
> documented and I have no idea what it does.. However, this function
> saves the reported values so we can retrieve them on a following
> report. Why don't we skip events that have the same value reported as
> before? Obviously, we should do this only for absolute data, not
> relative. So I think something like this should work (sorry for
> line-wraps):
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 12b6e67..000a768 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1220,7 +1220,10 @@
>         for (n = 0; n < count; n++) {
>
>                 if (HID_MAIN_ITEM_VARIABLE & field->flags) {
> -                       hid_process_event(hid, field,
> &field->usage[n], value[n], interrupt);
> +                       /* ignore absolute values if they didn't change */
> +                       if (HID_MAIN_ITEM_RELATIVE & field->flags ||
> +                           value[n] != field->value[n])
> +                               hid_process_event(hid, field,
> &field->usage[n], value[n], interrupt);
>                         continue;
>                 }
>
>
> @Jiri: Any comments on this? I now agree with Fredrik that this is
> better solved in HID core. It is a generic problem..

Jumping in the conversation here, but this is a strong NACK.

The multitouch handling sometimes suppose that there is a keep alive
on each touch, and when the keep alive stops in the report, then the
mt library releases the touch. If you stop sending these events, the
tip switch will not be sent twice, and then the slot will be
considered as released, and you broke many touchscreens.

Cheers,
Benjamin

>
> In HID, we save values per scancode / HID-field, additionally to
> input-core saving them per keycode / input-code. This patch basically
> makes use of this and avoids sending events for scan-codes that didn't
> change, but accidentally map to the same keycode as another key (which
> might have a different state than this one).
>
> If this looks reasonable, I can prepare a patch with a proper
> commit-log and give it a spin here.
>
> Thanks
> David
>
>>         /* 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);
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index f53c4a9..1c59f8f 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -448,6 +448,7 @@ struct hid_input {
>>         struct list_head list;
>>         struct hid_report *report;
>>         struct input_dev *input;
>> +       unsigned backslash_usage;
>>  };
>>
>>  enum hid_type {
>> --
>> 2.1.0.rc1
>>
>> --
>> 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

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Benjamin Tissoires]

On Mon, Sep 15, 2014 at 10:39 AM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Sun, Sep 14, 2014 at 12:45 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> Hi
>>
>> On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
>> <megahallon@gmail.com> wrote:
>>> Here is my attempt on a fix for bug 70181, please review it. It is
>>> tested on my nordic Corsair K70, if this solution is deemed acceptable
>>> I can ask some people with other problematic keyboards to test it.
>>>
>>> Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
>>> ---
>>>  drivers/hid/hid-input.c | 14 ++++++++++++++
>>>  include/linux/hid.h     |  1 +
>>>  2 files changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>>> index 2619f7f..56429c0 100644
>>> --- a/drivers/hid/hid-input.c
>>> +++ b/drivers/hid/hid-input.c
>>> @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>>>                 return;
>>>         }
>>>
>>> +       /*
>>> +        * Some keyboards will report both HID keys 0x31 (\ and |) and
>>> +        * 0x32 (Non-US # and ~) which are both mapped to
>>> +        * KEY_BACKSLASH. This will cause spurious events causing
>>> +        * repeated backslash key presses. Handle this by tracking the
>>> +        * active HID code and ignoring the other one.
>>> +        */
>>> +       if (usage->type == EV_KEY && usage->code == KEY_BACKSLASH) {
>>> +               if (value)
>>> +                       field->hidinput->backslash_usage = usage->hid;
>>> +               else if (field->hidinput->backslash_usage != usage->hid)
>>> +                       return;
>>> +       }
>>> +
>>
>> Ok, I have looked through some more reports and at a bunch of AT
>> keyboards and how the HID standard maps them. The backslash/pipe key
>> is the only key affected by this. There _might_ be some similar
>> overlap with Korean 106 and Japanese 109 layouts, but as there haven't
>> been any reports, we probably don't care right now.
>>
>> And indeed, I'm kinda reluctant to add an hwdb entry for the reported
>> keyboards as I couldn't find anyone with non-105 keyboards to compare
>> VID/PID. Furthermore, similar issues will probably show up again in
>> the future. However, I still dislike the quick hack in this patch. I
>> especially dislike matching on KEY_BACKSLASH while we really want to
>> match on 0x31 and 0x32. If users use setkeycode() to change a mapping,
>> this should not trigger our quirk. We could easily change the patch to
>> match on usage, but I think there's a better way:
>>
>> hid-core.c: hid_input_field()
>> This is the main entry point for data that is matched on HID fields.
>> We already have a RollOver quirk in there, which is extremely bad
>> documented and I have no idea what it does.. However, this function
>> saves the reported values so we can retrieve them on a following
>> report. Why don't we skip events that have the same value reported as
>> before? Obviously, we should do this only for absolute data, not
>> relative. So I think something like this should work (sorry for
>> line-wraps):
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 12b6e67..000a768 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1220,7 +1220,10 @@
>>         for (n = 0; n < count; n++) {
>>
>>                 if (HID_MAIN_ITEM_VARIABLE & field->flags) {
>> -                       hid_process_event(hid, field,
>> &field->usage[n], value[n], interrupt);
>> +                       /* ignore absolute values if they didn't change */
>> +                       if (HID_MAIN_ITEM_RELATIVE & field->flags ||
>> +                           value[n] != field->value[n])
>> +                               hid_process_event(hid, field,
>> &field->usage[n], value[n], interrupt);
>>                         continue;
>>                 }
>>
>>
>> @Jiri: Any comments on this? I now agree with Fredrik that this is
>> better solved in HID core. It is a generic problem..
>
> Jumping in the conversation here, but this is a strong NACK.
>
> The multitouch handling sometimes suppose that there is a keep alive
> on each touch, and when the keep alive stops in the report, then the
> mt library releases the touch. If you stop sending these events, the
> tip switch will not be sent twice, and then the slot will be
> considered as released, and you broke many touchscreens.
>

To be fair, this will not impact hid-multitouch actually. This driver
does not uses the mt_event directly, so it should be fine. Still,
changing this behaviour is IMO likely to introduce regressions, unless
you conduct a very thorough audit of all the hid drivers and check
that they are not relying on the fact that they get all the events in
each report.

Cheers,
Benjamin

>>
>> In HID, we save values per scancode / HID-field, additionally to
>> input-core saving them per keycode / input-code. This patch basically
>> makes use of this and avoids sending events for scan-codes that didn't
>> change, but accidentally map to the same keycode as another key (which
>> might have a different state than this one).
>>
>> If this looks reasonable, I can prepare a patch with a proper
>> commit-log and give it a spin here.
>>
>> Thanks
>> David
>>
>>>         /* 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);
>>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>>> index f53c4a9..1c59f8f 100644
>>> --- a/include/linux/hid.h
>>> +++ b/include/linux/hid.h
>>> @@ -448,6 +448,7 @@ struct hid_input {
>>>         struct list_head list;
>>>         struct hid_report *report;
>>>         struct input_dev *input;
>>> +       unsigned backslash_usage;
>>>  };
>>>
>>>  enum hid_type {
>>> --
>>> 2.1.0.rc1
>>>
>>> --
>>> 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

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[David Herrmann]

Hi

On Mon, Sep 15, 2014 at 4:47 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Mon, Sep 15, 2014 at 10:39 AM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> On Sun, Sep 14, 2014 at 12:45 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>> Hi
>>>
>>> On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
>>> <megahallon@gmail.com> wrote:
>>>> Here is my attempt on a fix for bug 70181, please review it. It is
>>>> tested on my nordic Corsair K70, if this solution is deemed acceptable
>>>> I can ask some people with other problematic keyboards to test it.
>>>>
>>>> Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
>>>> ---
>>>>  drivers/hid/hid-input.c | 14 ++++++++++++++
>>>>  include/linux/hid.h     |  1 +
>>>>  2 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>>>> index 2619f7f..56429c0 100644
>>>> --- a/drivers/hid/hid-input.c
>>>> +++ b/drivers/hid/hid-input.c
>>>> @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>>>>                 return;
>>>>         }
>>>>
>>>> +       /*
>>>> +        * Some keyboards will report both HID keys 0x31 (\ and |) and
>>>> +        * 0x32 (Non-US # and ~) which are both mapped to
>>>> +        * KEY_BACKSLASH. This will cause spurious events causing
>>>> +        * repeated backslash key presses. Handle this by tracking the
>>>> +        * active HID code and ignoring the other one.
>>>> +        */
>>>> +       if (usage->type == EV_KEY && usage->code == KEY_BACKSLASH) {
>>>> +               if (value)
>>>> +                       field->hidinput->backslash_usage = usage->hid;
>>>> +               else if (field->hidinput->backslash_usage != usage->hid)
>>>> +                       return;
>>>> +       }
>>>> +
>>>
>>> Ok, I have looked through some more reports and at a bunch of AT
>>> keyboards and how the HID standard maps them. The backslash/pipe key
>>> is the only key affected by this. There _might_ be some similar
>>> overlap with Korean 106 and Japanese 109 layouts, but as there haven't
>>> been any reports, we probably don't care right now.
>>>
>>> And indeed, I'm kinda reluctant to add an hwdb entry for the reported
>>> keyboards as I couldn't find anyone with non-105 keyboards to compare
>>> VID/PID. Furthermore, similar issues will probably show up again in
>>> the future. However, I still dislike the quick hack in this patch. I
>>> especially dislike matching on KEY_BACKSLASH while we really want to
>>> match on 0x31 and 0x32. If users use setkeycode() to change a mapping,
>>> this should not trigger our quirk. We could easily change the patch to
>>> match on usage, but I think there's a better way:
>>>
>>> hid-core.c: hid_input_field()
>>> This is the main entry point for data that is matched on HID fields.
>>> We already have a RollOver quirk in there, which is extremely bad
>>> documented and I have no idea what it does.. However, this function
>>> saves the reported values so we can retrieve them on a following
>>> report. Why don't we skip events that have the same value reported as
>>> before? Obviously, we should do this only for absolute data, not
>>> relative. So I think something like this should work (sorry for
>>> line-wraps):
>>>
>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> index 12b6e67..000a768 100644
>>> --- a/drivers/hid/hid-core.c
>>> +++ b/drivers/hid/hid-core.c
>>> @@ -1220,7 +1220,10 @@
>>>         for (n = 0; n < count; n++) {
>>>
>>>                 if (HID_MAIN_ITEM_VARIABLE & field->flags) {
>>> -                       hid_process_event(hid, field,
>>> &field->usage[n], value[n], interrupt);
>>> +                       /* ignore absolute values if they didn't change */
>>> +                       if (HID_MAIN_ITEM_RELATIVE & field->flags ||
>>> +                           value[n] != field->value[n])
>>> +                               hid_process_event(hid, field,
>>> &field->usage[n], value[n], interrupt);
>>>                         continue;
>>>                 }
>>>
>>>
>>> @Jiri: Any comments on this? I now agree with Fredrik that this is
>>> better solved in HID core. It is a generic problem..
>>
>> Jumping in the conversation here, but this is a strong NACK.
>>
>> The multitouch handling sometimes suppose that there is a keep alive
>> on each touch, and when the keep alive stops in the report, then the
>> mt library releases the touch. If you stop sending these events, the
>> tip switch will not be sent twice, and then the slot will be
>> considered as released, and you broke many touchscreens.
>>
>
> To be fair, this will not impact hid-multitouch actually. This driver
> does not uses the mt_event directly, so it should be fine. Still,
> changing this behaviour is IMO likely to introduce regressions, unless
> you conduct a very thorough audit of all the hid drivers and check
> that they are not relying on the fact that they get all the events in
> each report.

I'm fine with reducing this to a minimum (like matching on
HID_UP_KEYBOARD). I haven't sent a patch, yet, as I'm looking for
exactly those cases. So thanks for pointing it out.

Anything going though the input layer is already discarded if it
doesn't change (and is absolute data). So it's really just about
quirks and special HID drivers. We could move this comparison into
hid-input.c *after* any driver quirk has been called?

Thanks
David

Re: [PATCH] Handle spurious backslash key repeats on some keyboards

[Benjamin Tissoires]

On Mon, Sep 15, 2014 at 10:52 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Sep 15, 2014 at 4:47 PM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> On Mon, Sep 15, 2014 at 10:39 AM, Benjamin Tissoires
>> <benjamin.tissoires@gmail.com> wrote:
>>> On Sun, Sep 14, 2014 at 12:45 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>>> Hi
>>>>
>>>> On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
>>>> <megahallon@gmail.com> wrote:
>>>>> Here is my attempt on a fix for bug 70181, please review it. It is
>>>>> tested on my nordic Corsair K70, if this solution is deemed acceptable
>>>>> I can ask some people with other problematic keyboards to test it.
>>>>>
>>>>> Signed-off-by: Fredrik Hallenberg <megahallon@gmail.com>
>>>>> ---
>>>>>  drivers/hid/hid-input.c | 14 ++++++++++++++
>>>>>  include/linux/hid.h     |  1 +
>>>>>  2 files changed, 15 insertions(+)
>>>>>
>>>>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>>>>> index 2619f7f..56429c0 100644
>>>>> --- a/drivers/hid/hid-input.c
>>>>> +++ b/drivers/hid/hid-input.c
>>>>> @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>>>>>                 return;
>>>>>         }
>>>>>
>>>>> +       /*
>>>>> +        * Some keyboards will report both HID keys 0x31 (\ and |) and
>>>>> +        * 0x32 (Non-US # and ~) which are both mapped to
>>>>> +        * KEY_BACKSLASH. This will cause spurious events causing
>>>>> +        * repeated backslash key presses. Handle this by tracking the
>>>>> +        * active HID code and ignoring the other one.
>>>>> +        */
>>>>> +       if (usage->type == EV_KEY && usage->code == KEY_BACKSLASH) {
>>>>> +               if (value)
>>>>> +                       field->hidinput->backslash_usage = usage->hid;
>>>>> +               else if (field->hidinput->backslash_usage != usage->hid)
>>>>> +                       return;
>>>>> +       }
>>>>> +
>>>>
>>>> Ok, I have looked through some more reports and at a bunch of AT
>>>> keyboards and how the HID standard maps them. The backslash/pipe key
>>>> is the only key affected by this. There _might_ be some similar
>>>> overlap with Korean 106 and Japanese 109 layouts, but as there haven't
>>>> been any reports, we probably don't care right now.
>>>>
>>>> And indeed, I'm kinda reluctant to add an hwdb entry for the reported
>>>> keyboards as I couldn't find anyone with non-105 keyboards to compare
>>>> VID/PID. Furthermore, similar issues will probably show up again in
>>>> the future. However, I still dislike the quick hack in this patch. I
>>>> especially dislike matching on KEY_BACKSLASH while we really want to
>>>> match on 0x31 and 0x32. If users use setkeycode() to change a mapping,
>>>> this should not trigger our quirk. We could easily change the patch to
>>>> match on usage, but I think there's a better way:
>>>>
>>>> hid-core.c: hid_input_field()
>>>> This is the main entry point for data that is matched on HID fields.
>>>> We already have a RollOver quirk in there, which is extremely bad
>>>> documented and I have no idea what it does.. However, this function
>>>> saves the reported values so we can retrieve them on a following
>>>> report. Why don't we skip events that have the same value reported as
>>>> before? Obviously, we should do this only for absolute data, not
>>>> relative. So I think something like this should work (sorry for
>>>> line-wraps):
>>>>
>>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>>> index 12b6e67..000a768 100644
>>>> --- a/drivers/hid/hid-core.c
>>>> +++ b/drivers/hid/hid-core.c
>>>> @@ -1220,7 +1220,10 @@
>>>>         for (n = 0; n < count; n++) {
>>>>
>>>>                 if (HID_MAIN_ITEM_VARIABLE & field->flags) {
>>>> -                       hid_process_event(hid, field,
>>>> &field->usage[n], value[n], interrupt);
>>>> +                       /* ignore absolute values if they didn't change */
>>>> +                       if (HID_MAIN_ITEM_RELATIVE & field->flags ||
>>>> +                           value[n] != field->value[n])
>>>> +                               hid_process_event(hid, field,
>>>> &field->usage[n], value[n], interrupt);
>>>>                         continue;
>>>>                 }
>>>>
>>>>
>>>> @Jiri: Any comments on this? I now agree with Fredrik that this is
>>>> better solved in HID core. It is a generic problem..
>>>
>>> Jumping in the conversation here, but this is a strong NACK.
>>>
>>> The multitouch handling sometimes suppose that there is a keep alive
>>> on each touch, and when the keep alive stops in the report, then the
>>> mt library releases the touch. If you stop sending these events, the
>>> tip switch will not be sent twice, and then the slot will be
>>> considered as released, and you broke many touchscreens.
>>>
>>
>> To be fair, this will not impact hid-multitouch actually. This driver
>> does not uses the mt_event directly, so it should be fine. Still,
>> changing this behaviour is IMO likely to introduce regressions, unless
>> you conduct a very thorough audit of all the hid drivers and check
>> that they are not relying on the fact that they get all the events in
>> each report.
>
> I'm fine with reducing this to a minimum (like matching on
> HID_UP_KEYBOARD). I haven't sent a patch, yet, as I'm looking for
> exactly those cases. So thanks for pointing it out.
>
> Anything going though the input layer is already discarded if it
> doesn't change (and is absolute data). So it's really just about
> quirks and special HID drivers. We could move this comparison into
> hid-input.c *after* any driver quirk has been called?
>

At first sight, yes, it can work. hidinput_hid_event() contains all
the tablet (PEN) semantic which does not seem to be impacted by not
having all the reports.

IMO, you can just put this before the input_event() call in
hidinput_hid_event().

Side note: this *might* also fix a pb we have been reported
(https://bugzilla.kernel.org/show_bug.cgi?id=76461 and
http://www.spinics.net/lists/linux-input/msg31506.html)

Cheers,
Benjamin