| Message ID | CANq1E4RVV1nEbtiAkiePtSF-K8CdnS7hmts_8hqDOi=mWsE4MA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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".. -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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;