[PATCH] HID: input: fix a4tech horizontal wheel custom usage id

[PATCH] HID: input: fix a4tech horizontal wheel custom usage id

[Nicolas Saenz Julienne]

Some a4tech mice use the 'GenericDesktop.00b8' usage id to inform
whether the previous wheel report was horizontal or vertical. Before
c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key") this
usage id was being mapped to 'Relative.Misc'. After the patch it's
simply ignored (usage->type == 0 & usage->code == 0). Checking the HID
Usage Tables it turns out it's a reserved usage_id, so it makes sense to
map it the way it was. Ultimately this makes hid-a4tech ignore the
WHEEL/HWHEEL selection event, as it has no usage->type.

The patch reverts the handling of the usage id back to it's previous
behavior.

Fixes: c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key")
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/hid/hid-input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 63855f275a38..6a956d5a195e 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -671,7 +671,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 		if ((usage->hid & 0xf0) == 0xb0) {	/* SC - Display */
 			switch (usage->hid & 0xf) {
 			case 0x05: map_key_clear(KEY_SWITCHVIDEOMODE); break;
-			default: goto ignore;
+			default: goto unknown;
 			}
 			break;
 		}
-- 
2.21.0

Re: [PATCH] HID: input: fix a4tech horizontal wheel custom usage id

[Benjamin Tissoires]

Hi Nicolas,

On Mon, Jun 10, 2019 at 8:54 PM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Some a4tech mice use the 'GenericDesktop.00b8' usage id to inform
> whether the previous wheel report was horizontal or vertical. Before
> c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key") this
> usage id was being mapped to 'Relative.Misc'. After the patch it's
> simply ignored (usage->type == 0 & usage->code == 0). Checking the HID
> Usage Tables it turns out it's a reserved usage_id, so it makes sense to
> map it the way it was. Ultimately this makes hid-a4tech ignore the
> WHEEL/HWHEEL selection event, as it has no usage->type.
>
> The patch reverts the handling of the usage id back to it's previous
> behavior.

Hmm, if A4Tech is using a reserved usage, we shouldn't fix this in
hid-input.c but in hid-a4tech instead.
Because you won't know when someone else in the HID consortium will
remap this usage to some other random axis, and your mouse will be
broken again.

How about you add a .input_mapping callback in hid-a4tech and map this
usage there to your needs? This way you will be sure that such a
situation will not happen again.

Cheers,
Benjamin

>
> Fixes: c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key")
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>  drivers/hid/hid-input.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 63855f275a38..6a956d5a195e 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -671,7 +671,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>                 if ((usage->hid & 0xf0) == 0xb0) {      /* SC - Display */
>                         switch (usage->hid & 0xf) {
>                         case 0x05: map_key_clear(KEY_SWITCHVIDEOMODE); break;
> -                       default: goto ignore;
> +                       default: goto unknown;
>                         }
>                         break;
>                 }
> --
> 2.21.0
>

Re: [PATCH] HID: input: fix a4tech horizontal wheel custom usage id

[Nicolas Saenz Julienne]

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

On Tue, 2019-06-11 at 10:43 +0200, Benjamin Tissoires wrote:
> Hi Nicolas,
> 
> On Mon, Jun 10, 2019 at 8:54 PM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > Some a4tech mice use the 'GenericDesktop.00b8' usage id to inform
> > whether the previous wheel report was horizontal or vertical. Before
> > c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key") this
> > usage id was being mapped to 'Relative.Misc'. After the patch it's
> > simply ignored (usage->type == 0 & usage->code == 0). Checking the HID
> > Usage Tables it turns out it's a reserved usage_id, so it makes sense to
> > map it the way it was. Ultimately this makes hid-a4tech ignore the
> > WHEEL/HWHEEL selection event, as it has no usage->type.
> > 
> > The patch reverts the handling of the usage id back to it's previous
> > behavior.
> 
> Hmm, if A4Tech is using a reserved usage, we shouldn't fix this in
> hid-input.c but in hid-a4tech instead.
> Because you won't know when someone else in the HID consortium will
> remap this usage to some other random axis, and your mouse will be
> broken again.
> 
> How about you add a .input_mapping callback in hid-a4tech and map this
> usage there to your needs? This way you will be sure that such a
> situation will not happen again.

I agree it would be a cleaner solution.

In summary the first report indicates the wheel relative value, the second the
orientation. The first report is already being mapped to REL_WHEEL and
REL_WHEEL (or the high-res versions), but what would be a correct code for the
second report? The way I see it, we shouldn't map it to anything. And then
catch both events in the custom driver to build the input_event() accordinlgy
(as it's almost being done already). Is this somewhat correct? I'll send a
followup patch anyway so we have something more tangible comment on.

> > Fixes: c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key")
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > ---
> >  drivers/hid/hid-input.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> > index 63855f275a38..6a956d5a195e 100644
> > --- a/drivers/hid/hid-input.c
> > +++ b/drivers/hid/hid-input.c
> > @@ -671,7 +671,7 @@ static void hidinput_configure_usage(struct hid_input
> > *hidinput, struct hid_fiel
> >                 if ((usage->hid & 0xf0) == 0xb0) {      /* SC - Display */
> >                         switch (usage->hid & 0xf) {
> >                         case 0x05: map_key_clear(KEY_SWITCHVIDEOMODE);
> > break;
> > -                       default: goto ignore;
> > +                       default: goto unknown;
> >                         }
> >                         break;
> >                 }
> > --
> > 2.21.0
> > 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]