[PATCH 0/2] "HID: redragon: Fix modifier keys for Redragon Asura Keyboard" causes regression for Acer keyboard

[PATCH 0/2] "HID: redragon: Fix modifier keys for Redragon Asura Keyboard" causes regression for Acer keyboard

[John S Gruber]

Please cc me in any replies.

The Acer branded keyboard that came with my Acer ATC-605-UB11 computer system
fails completely due to:

85455dd906d5 ("HID: redragon: Fix modifier keys for Redragon Asura Keyboard.")

That commit was merged to 4.18 on June 8.

The redragon vendor_id device_id combination 0c45:760b is shared by that
keyboard, as well as many others, it appears.
See https://linux-hardware.org/index.php?id=usb:0c45-760b
for other SONiX keyboards that don't appear to be Redragon devices.

First patch:

While the Redragon keyboard uses the second device created and not the first,
the Acer, and perhaps many others, use that first device
(the one with dev->maxapplication==1).

The hid-redragon.c driver should not cause this device to be ignored and
should not interfere with its being cleared up when disconnected.
It appears to me that this problematic change was made late in the
patch's revisions as a clean up.

I recently tried to connect another usb keyboard after the redragon driver
was activated by my Acer keyboard. It is blocked from working too
until the kernel is rebooted without my keyboard being plugged in. I
also understand that the interference with the first device stops
the keyboard LEDs from working correctly.



I'm resending this set as I haven't heard from the hid people in the two
weeks since I first sent this. Before 3.18 is released I believe either
this patch should be quickly finalized and applied, Robert's patch in the
linux-next tree should be applied instead, dc9b8e85ed9, or, as a last option,
the problematic commit should be reverted. I've tested all three
possibilities.

Second Patch:

As there are potentially many different devices being intercepted by the new
hid-redragon driver, and as the bytes x81 x00 are not rare in this context,
I also think it's advisable to improve the verification condition before
performing the report-description patch only meant for the Redragon Asura
Keyboard.



These two patches will follow. The first restores the function of my keyboard
sucessfully.

I don't have the Redragon Asura Keyboard necessary to test the second patch.
Its rdesc data come from Robert as posted to pastebin.

John S Gruber (2):
  HID: redragon: Fix regression in non-Redragon keyboard due to this new
    driver
  HID: redragon Add additional verification to rdesc modification quirk

 drivers/hid/hid-redragon.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

-- 
1.9.1

[PATCH 1/2] HID: redragon: Fix regression in non-Redragon keyboard due to this new driver

[John S Gruber]

The Redragon keyboard uses the second device being presented, but other
devices with the same vendor_id/device_id pair (0x0c45:760b) use the first.
Don't cause its deletion. Problem introduced in commit 85455dd906d5
("HID: redragon: Fix modifier keys for Redragon Asura Keyboard")

Fixes: 85455dd906d5
Signed-off-by: John S Gruber <JohnSGruber@gmail.com>
---
 drivers/hid/hid-redragon.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/hid/hid-redragon.c b/drivers/hid/hid-redragon.c
index daf5957..85a5fbb 100644
--- a/drivers/hid/hid-redragon.c
+++ b/drivers/hid/hid-redragon.c
@@ -55,10 +55,6 @@ static int redragon_probe(struct hid_device *dev,
 		return ret;
 	}

-	/* do not register unused input device */
-	if (dev->maxapplication == 1)
-		return 0;
-
 	ret = hid_hw_start(dev, HID_CONNECT_DEFAULT);
 	if (ret) {
 		hid_err(dev, "hw start failed\n");
-- 
1.9.1

Re: [PATCH 1/2] HID: redragon: Fix regression in non-Redragon keyboard due to this new driver

[Robert Munteanu]

Hi John,

On Sat, 2018-07-28 at 15:53 -0400, John S Gruber wrote:
> The Redragon keyboard uses the second device being presented, but
> other
> devices with the same vendor_id/device_id pair (0x0c45:760b) use the
> first.
> Don't cause its deletion. Problem introduced in commit 85455dd906d5
> ("HID: redragon: Fix modifier keys for Redragon Asura Keyboard")
> 
> Fixes: 85455dd906d5
> Signed-off-by: John S Gruber <JohnSGruber@gmail.com>
> ---
>  drivers/hid/hid-redragon.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-redragon.c b/drivers/hid/hid-redragon.c
> index daf5957..85a5fbb 100644
> --- a/drivers/hid/hid-redragon.c
> +++ b/drivers/hid/hid-redragon.c
> @@ -55,10 +55,6 @@ static int redragon_probe(struct hid_device *dev,
>  		return ret;
>  	}
> 
> -	/* do not register unused input device */
> -	if (dev->maxapplication == 1)
> -		return 0;
> -
>  	ret = hid_hw_start(dev, HID_CONNECT_DEFAULT);
>  	if (ret) {
>  		hid_err(dev, "hw start failed\n");

As I mentioned, this is already fixed by
dc9b8e85ed95cbe7e3ad0eabb5b48d617bbc365e, scheduled for 4.19, and I
suggest that we instead add that one to 4.18.

The explanation is that the block you deleted was the whole reason for
adding the redragon_probe function, so the changes are largely
equivalent.

Thanks,

Robert

[PATCH 2/2] HID: redragon Add additional verification to rdesc modification quirk

[John S Gruber]

There are many devices using the vendor_id 0c45 and device_id of 760b
combination. Also the two bytes 0x81 0x00 aren't rare for a report
description. For these reasons the report description being altered
by the quirk should be verified more completely

If I'm understanding this correctly, I believe for an array field the
report_size should be greater or equal to
ceil(log2(usage_maximum - usage_minimum + 1)). That's 3 bits for these 8
shift keys, 0xe0-0xe7. Therefore the incorrect report description can't
be valid for any device.

Check the actual count of the rdesc and compare the entire field
description to reduce the chance of patching the wrong thing by
inadvertence.

Signed-off-by: John S Gruber <JohnSGruber@gmail.com>
---
 drivers/hid/hid-redragon.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-redragon.c b/drivers/hid/hid-redragon.c
index 85a5fbb..6e506cb 100644
--- a/drivers/hid/hid-redragon.c
+++ b/drivers/hid/hid-redragon.c
@@ -18,6 +18,21 @@

 #include "hid-ids.h"

+static __u8 asura_redragon_badfield[] = {
+	0x05, 0x01,	/* Usage Page (GenericDesktop)		*/
+	0x09, 0x06,	/* Usage (Keyboard)			*/
+	0xa1, 0x01,	/* Collection (Application)		*/
+	0x85, 0x04,	/* Report ID (4)			*/
+	0x05, 0x07,	/* Usage Page (Keyboard)		*/
+	0x19, 0xe0,	/* Usage Minimum (0xe0) Left Control	*/
+	0x29, 0xe7,	/* Usage Maximum (0xe7) Right GUI	*/
+	0x15, 0x00,	/* Logical Minimum (0)			*/
+	0x25, 0x01,	/* Logical Maximum (1)			*/
+	0x75, 0x01,	/* Report Size (1)			*/
+	0x95, 0x08,	/* Report Count(8)			*/
+	0x81, 0x00	/* Input Array [Should be Input Var]	*/
+};
+

 /*
  * The Redragon Asura keyboard sends an incorrect HID descriptor.
@@ -36,7 +51,9 @@
 static __u8 *redragon_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 	unsigned int *rsize)
 {
-	if (*rsize >= 102 && rdesc[100] == 0x81 && rdesc[101] == 0x00) {
+	if (*rsize == 169 &&
+			memcmp(&rdesc[78], asura_redragon_badfield,
+			sizeof(asura_redragon_badfield)) == 0) {
 		dev_info(&hdev->dev, "Fixing Redragon ASURA report descriptor.\n");
 		rdesc[101] = 0x02;
 	}
-- 
1.9.1

Re: [PATCH 2/2] HID: redragon Add additional verification to rdesc modification quirk

[Robert Munteanu]

On Sat, 2018-07-28 at 15:53 -0400, John S Gruber wrote:
> There are many devices using the vendor_id 0c45 and device_id of 760b
> combination. Also the two bytes 0x81 0x00 aren't rare for a report
> description. For these reasons the report description being altered
> by the quirk should be verified more completely
> 
> If I'm understanding this correctly, I believe for an array field the
> report_size should be greater or equal to
> ceil(log2(usage_maximum - usage_minimum + 1)). That's 3 bits for
> these 8
> shift keys, 0xe0-0xe7. Therefore the incorrect report description
> can't
> be valid for any device.
> 
> Check the actual count of the rdesc and compare the entire field
> description to reduce the chance of patching the wrong thing by
> inadvertence.
> 
> Signed-off-by: John S Gruber <JohnSGruber@gmail.com>

I tested this on 4.17.11 with 85455dd906d5 and cbe7e3ad0eab from Jiri's
for-4.19/upstream tree and it works just fine.

Feel free to add

  Acked-By: Robert Munteanu <rombert@apache.org>

Thanks for looking into this.

Robert