[PATCH 1/2] Revert "HID: dragonrise: fix HID Descriptor for 0x0006 PID"

[PATCH 1/2] Revert "HID: dragonrise: fix HID Descriptor for 0x0006 PID"

[Ioan-Adrian Ratiu]

This reverts commit 18339f59c3a6 ("HID: dragonrise: fix HID...")
because the "fix" is bogus. That report descriptor is different in
hardware (see below) and it's the way the hardware works, it can't be
fixed at this level because it reuses axes by design.

What this change tried to fix is a regression caused by commit 20aef664f139
("HID: input: force generic axis to be mapped to their user space axis") by
working around the problem and trying to change the report descriptor in
hid-dr, which obviously can't work and introduces more breakage because it
adds another unnecessary layer of multiplexing/indirection, making the
dragonrise gamepad practically unusable in userspace.

This needs to be fixed where the regression was initially introduced in
hid-input (the next patch does this).

Here's the descriptor taken directly from the device via lsusb:
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.10
          bCountryCode           33 US
          bNumDescriptors         1
          bDescriptorType        34 Report
          wDescriptorLength     101
          Report Descriptor: (length is 101)
            Item(Global): Usage Page, data= [ 0x01 ] 1
                            Generic Desktop Controls
            Item(Local ): Usage, data= [ 0x04 ] 4
                            Joystick
            Item(Main  ): Collection, data= [ 0x01 ] 1
                            Application
            Item(Main  ): Collection, data= [ 0x02 ] 2
                            Logical
            Item(Global): Report Size, data= [ 0x08 ] 8
            Item(Global): Report Count, data= [ 0x05 ] 5
            Item(Global): Logical Minimum, data= [ 0x00 ] 0
            Item(Global): Logical Maximum, data= [ 0xff 0x00 ] 255
            Item(Global): Physical Minimum, data= [ 0x00 ] 0
            Item(Global): Physical Maximum, data= [ 0xff 0x00 ] 255
            Item(Local ): Usage, data= [ 0x30 ] 48
                            Direction-X
            Item(Local ): Usage, data= [ 0x31 ] 49
                            Direction-Y
            Item(Local ): Usage, data= [ 0x32 ] 50
                            Direction-Z
            Item(Local ): Usage, data= [ 0x32 ] 50
                            Direction-Z
            Item(Local ): Usage, data= [ 0x35 ] 53
                            Rotate-Z
            Item(Main  ): Input, data= [ 0x02 ] 2
                            Data Variable Absolute No_Wrap Linear
                            Preferred_State No_Null_Position Non_Volatile Bitfield
            Item(Global): Report Size, data= [ 0x04 ] 4
            Item(Global): Report Count, data= [ 0x01 ] 1
            Item(Global): Logical Maximum, data= [ 0x07 ] 7
            Item(Global): Physical Maximum, data= [ 0x3b 0x01 ] 315
            Item(Global): Unit, data= [ 0x14 ] 20
                            System: English Rotation, Unit: Degrees
            Item(Local ): Usage, data= [ 0x39 ] 57
                            Hat Switch
            Item(Main  ): Input, data= [ 0x42 ] 66
                            Data Variable Absolute No_Wrap Linear
                            Preferred_State Null_State Non_Volatile Bitfield
            Item(Global): Unit, data= [ 0x00 ] 0
                            System: None, Unit: (None)
            Item(Global): Report Size, data= [ 0x01 ] 1
            Item(Global): Report Count, data= [ 0x0c ] 12
            Item(Global): Logical Maximum, data= [ 0x01 ] 1
            Item(Global): Physical Maximum, data= [ 0x01 ] 1
            Item(Global): Usage Page, data= [ 0x09 ] 9
                            Buttons
            Item(Local ): Usage Minimum, data= [ 0x01 ] 1
                            Button 1 (Primary)
            Item(Local ): Usage Maximum, data= [ 0x0c ] 12
                            (null)
            Item(Main  ): Input, data= [ 0x02 ] 2
                            Data Variable Absolute No_Wrap Linear
                            Preferred_State No_Null_Position Non_Volatile Bitfield
            Item(Global): Usage Page, data= [ 0x00 0xff ] 65280
                            (null)
            Item(Global): Report Size, data= [ 0x01 ] 1
            Item(Global): Report Count, data= [ 0x08 ] 8
            Item(Global): Logical Maximum, data= [ 0x01 ] 1
            Item(Global): Physical Maximum, data= [ 0x01 ] 1
            Item(Local ): Usage, data= [ 0x01 ] 1
                            (null)
            Item(Main  ): Input, data= [ 0x02 ] 2
                            Data Variable Absolute No_Wrap Linear
                            Preferred_State No_Null_Position Non_Volatile Bitfield
            Item(Main  ): End Collection, data=none
            Item(Main  ): Collection, data= [ 0x02 ] 2
                            Logical
            Item(Global): Report Size, data= [ 0x08 ] 8
            Item(Global): Report Count, data= [ 0x07 ] 7
            Item(Global): Physical Maximum, data= [ 0xff 0x00 ] 255
            Item(Global): Logical Maximum, data= [ 0xff 0x00 ] 255
            Item(Local ): Usage, data= [ 0x02 ] 2
                            (null)
            Item(Main  ): Output, data= [ 0x02 ] 2
                            Data Variable Absolute No_Wrap Linear
                            Preferred_State No_Null_Position Non_Volatile Bitfield
            Item(Main  ): End Collection, data=none
            Item(Main  ): End Collection, data=none

Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
---
 drivers/hid/hid-dr.c | 58 ----------------------------------------------------
 1 file changed, 58 deletions(-)

diff --git a/drivers/hid/hid-dr.c b/drivers/hid/hid-dr.c
index 8fd4bf7..2523f8a 100644
--- a/drivers/hid/hid-dr.c
+++ b/drivers/hid/hid-dr.c
@@ -234,58 +234,6 @@ static __u8 pid0011_rdesc_fixed[] = {
 	0xC0                /*  End Collection                  */
 };
 
-static __u8 pid0006_rdesc_fixed[] = {
-	0x05, 0x01,        /* Usage Page (Generic Desktop)	*/
-	0x09, 0x04,        /* Usage (Joystick)			*/
-	0xA1, 0x01,        /* Collection (Application)		*/
-	0xA1, 0x02,        /*   Collection (Logical)		*/
-	0x75, 0x08,        /*     Report Size (8)		*/
-	0x95, 0x05,        /*     Report Count (5)		*/
-	0x15, 0x00,        /*     Logical Minimum (0)		*/
-	0x26, 0xFF, 0x00,  /*     Logical Maximum (255)		*/
-	0x35, 0x00,        /*     Physical Minimum (0)		*/
-	0x46, 0xFF, 0x00,  /*     Physical Maximum (255)	*/
-	0x09, 0x30,        /*     Usage (X)			*/
-	0x09, 0x33,        /*     Usage (Ry)			*/
-	0x09, 0x32,        /*     Usage (Z)			*/
-	0x09, 0x31,        /*     Usage (Y)			*/
-	0x09, 0x34,        /*     Usage (Ry)			*/
-	0x81, 0x02,        /*     Input (Variable)		*/
-	0x75, 0x04,        /*     Report Size (4)		*/
-	0x95, 0x01,        /*     Report Count (1)		*/
-	0x25, 0x07,        /*     Logical Maximum (7)		*/
-	0x46, 0x3B, 0x01,  /*     Physical Maximum (315)	*/
-	0x65, 0x14,        /*     Unit (Centimeter)		*/
-	0x09, 0x39,        /*     Usage (Hat switch)		*/
-	0x81, 0x42,        /*     Input (Variable)		*/
-	0x65, 0x00,        /*     Unit (None)			*/
-	0x75, 0x01,        /*     Report Size (1)		*/
-	0x95, 0x0C,        /*     Report Count (12)		*/
-	0x25, 0x01,        /*     Logical Maximum (1)		*/
-	0x45, 0x01,        /*     Physical Maximum (1)		*/
-	0x05, 0x09,        /*     Usage Page (Button)		*/
-	0x19, 0x01,        /*     Usage Minimum (0x01)		*/
-	0x29, 0x0C,        /*     Usage Maximum (0x0C)		*/
-	0x81, 0x02,        /*     Input (Variable)		*/
-	0x06, 0x00, 0xFF,  /*     Usage Page (Vendor Defined)	*/
-	0x75, 0x01,        /*     Report Size (1)		*/
-	0x95, 0x08,        /*     Report Count (8)		*/
-	0x25, 0x01,        /*     Logical Maximum (1)		*/
-	0x45, 0x01,        /*     Physical Maximum (1)		*/
-	0x09, 0x01,        /*     Usage (0x01)			*/
-	0x81, 0x02,        /*     Input (Variable)		*/
-	0xC0,              /*   End Collection			*/
-	0xA1, 0x02,        /*   Collection (Logical)		*/
-	0x75, 0x08,        /*     Report Size (8)		*/
-	0x95, 0x07,        /*     Report Count (7)		*/
-	0x46, 0xFF, 0x00,  /*     Physical Maximum (255)	*/
-	0x26, 0xFF, 0x00,  /*     Logical Maximum (255)		*/
-	0x09, 0x02,        /*     Usage (0x02)			*/
-	0x91, 0x02,        /*     Output (Variable)		*/
-	0xC0,              /*   End Collection			*/
-	0xC0               /* End Collection			*/
-};
-
 static __u8 *dr_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 				unsigned int *rsize)
 {
@@ -296,12 +244,6 @@ static __u8 *dr_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 			*rsize = sizeof(pid0011_rdesc_fixed);
 		}
 		break;
-	case 0x0006:
-		if (*rsize == sizeof(pid0006_rdesc_fixed)) {
-			rdesc = pid0006_rdesc_fixed;
-			*rsize = sizeof(pid0006_rdesc_fixed);
-		}
-		break;
 	}
 	return rdesc;
 }
-- 
2.10.0

[PATCH 2/2] hid: input: add HID_QUIRK_REUSE_AXES and fix dragonrise

[Ioan-Adrian Ratiu]

Commit 79346d620e9d ("HID: input: force generic axis to be mapped to their
user space axis") made mapping generic axes to their userspace equivalents
mandatory and some lower end gamepads which were depending on the previous
behaviour suffered severe regressions because they were reusing axes and
expecting hid-input to multiplex their map to the respective userspace axis
by always searching for and using the next available axis.

Now the result is that different device axes appear on a single axis in
userspace, which is clearly a regression in the hid-input driver because it
needs to continue to handle this hardware as expected before the forcing
to provide the same interface to userspace.

Since these lower-end gamepads like 0079:0006 are definitely the exception,
create a quirk to fix them.

Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
---
 drivers/hid/hid-dr.c    |  2 ++
 drivers/hid/hid-input.c | 16 +++++++++++-----
 include/linux/hid.h     |  1 +
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-dr.c b/drivers/hid/hid-dr.c
index 2523f8a..27fc826 100644
--- a/drivers/hid/hid-dr.c
+++ b/drivers/hid/hid-dr.c
@@ -274,6 +274,8 @@ static int dr_probe(struct hid_device *hdev, const struct hid_device_id *id)
 			hid_hw_stop(hdev);
 			goto err;
 		}
+		/* has only 5 axes and reuses X, Y */
+		hdev->quirks |= HID_QUIRK_REUSE_AXES;
 		break;
 	}
 
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index fb9ace1..1cc6fe4 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -633,11 +633,17 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 		/* These usage IDs map directly to the usage codes. */
 		case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
 		case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
-			if (field->flags & HID_MAIN_ITEM_RELATIVE)
-				map_rel(usage->hid & 0xf);
-			else
-				map_abs_clear(usage->hid & 0xf);
-			break;
+
+			/* if quirk is active don't force the userspace mapping,
+			 * instead search and use the next available axis.
+			 */
+			if (!(device->quirks & HID_QUIRK_REUSE_AXES)) {
+				if (field->flags & HID_MAIN_ITEM_RELATIVE)
+					map_rel(usage->hid & 0xf);
+				else
+					map_abs_clear(usage->hid & 0xf);
+				break;
+			}
 
 		case HID_GD_SLIDER: case HID_GD_DIAL: case HID_GD_WHEEL:
 			if (field->flags & HID_MAIN_ITEM_RELATIVE)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 75b66ec..0979920 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -320,6 +320,7 @@ struct hid_item {
 #define HID_QUIRK_NO_EMPTY_INPUT		0x00000100
 #define HID_QUIRK_NO_INIT_INPUT_REPORTS		0x00000200
 #define HID_QUIRK_ALWAYS_POLL			0x00000400
+#define HID_QUIRK_REUSE_AXES			0x00000800
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
 #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		0x00020000
 #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	0x00040000
-- 
2.10.0

Re: [PATCH 2/2] hid: input: add HID_QUIRK_REUSE_AXES and fix dragonrise

[Benjamin Tissoires]

On Sep 26 2016 or thereabouts, Ioan-Adrian Ratiu wrote:
> Commit 79346d620e9d ("HID: input: force generic axis to be mapped to their
> user space axis") made mapping generic axes to their userspace equivalents
> mandatory and some lower end gamepads which were depending on the previous
> behaviour suffered severe regressions because they were reusing axes and
> expecting hid-input to multiplex their map to the respective userspace axis
> by always searching for and using the next available axis.

Yes, I apologies for the breakage and the regression, though I must say
that for now, only one hardware maker and one device (or range of devices
from the look of it) has needed to be quirked.

> 
> Now the result is that different device axes appear on a single axis in
> userspace, which is clearly a regression in the hid-input driver because it
> needs to continue to handle this hardware as expected before the forcing
> to provide the same interface to userspace.
> 
> Since these lower-end gamepads like 0079:0006 are definitely the exception,
> create a quirk to fix them.

Given that we only have this particular vendor that is an issue, I'd
rather see the fix in hid-dr.c. The reason being that you actually don't
need to have a global quirk and this simplifies the path in hid-input.
Plus for users, they can just upgrade hid-dr without having to recompile
their kernel when hid-core is not compiled as a module. 

The cleanest solution that wouldn't require any quirk in hid-core is to
simply add an .input_mapping() callback in hid-dr.c.

The code of the callback could be something like (untested):

static int dr_input_mapping(struct hid_device *hdev, struct hid_input *hi,
		struct hid_field *field, struct hid_usage *usage,
		unsigned long **bit, int *max)
{
	switch (usage->hid) {
	/* 
	 * revert the old hid-input behavior where axes
	 * can be randomly assigned when the hid usage is
	 * reused.
	 */
	case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
	case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
		if (field->flags & HID_MAIN_ITEM_RELATIVE)
			map_rel(usage->hid & 0xf);
		else
			map_abs(usage->hid & 0xf);
		return 1;
	}
	
	return 0;
}

Hopefully, something like this should revert the old behavior for all
hid-dr touchpads.

Ideally, we need to have Dragon Rise 0x0011 tested too. Nick, would you
mind checking it if you still have this particular device?

Cheers,
Benjamin

> 
> Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
> ---
>  drivers/hid/hid-dr.c    |  2 ++
>  drivers/hid/hid-input.c | 16 +++++++++++-----
>  include/linux/hid.h     |  1 +
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hid/hid-dr.c b/drivers/hid/hid-dr.c
> index 2523f8a..27fc826 100644
> --- a/drivers/hid/hid-dr.c
> +++ b/drivers/hid/hid-dr.c
> @@ -274,6 +274,8 @@ static int dr_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  			hid_hw_stop(hdev);
>  			goto err;
>  		}
> +		/* has only 5 axes and reuses X, Y */
> +		hdev->quirks |= HID_QUIRK_REUSE_AXES;
>  		break;
>  	}
>  
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index fb9ace1..1cc6fe4 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -633,11 +633,17 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>  		/* These usage IDs map directly to the usage codes. */
>  		case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
>  		case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
> -			if (field->flags & HID_MAIN_ITEM_RELATIVE)
> -				map_rel(usage->hid & 0xf);
> -			else
> -				map_abs_clear(usage->hid & 0xf);
> -			break;
> +
> +			/* if quirk is active don't force the userspace mapping,
> +			 * instead search and use the next available axis.
> +			 */
> +			if (!(device->quirks & HID_QUIRK_REUSE_AXES)) {
> +				if (field->flags & HID_MAIN_ITEM_RELATIVE)
> +					map_rel(usage->hid & 0xf);
> +				else
> +					map_abs_clear(usage->hid & 0xf);
> +				break;
> +			}
>  
>  		case HID_GD_SLIDER: case HID_GD_DIAL: case HID_GD_WHEEL:
>  			if (field->flags & HID_MAIN_ITEM_RELATIVE)
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 75b66ec..0979920 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -320,6 +320,7 @@ struct hid_item {
>  #define HID_QUIRK_NO_EMPTY_INPUT		0x00000100
>  #define HID_QUIRK_NO_INIT_INPUT_REPORTS		0x00000200
>  #define HID_QUIRK_ALWAYS_POLL			0x00000400
> +#define HID_QUIRK_REUSE_AXES			0x00000800
>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
>  #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		0x00020000
>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	0x00040000
> -- 
> 2.10.0
> 

Re: [PATCH 2/2] hid: input: add HID_QUIRK_REUSE_AXES and fix dragonrise

[Nikolai Kondrashov]

Hi Benjamin,

On 09/26/2016 12:29 PM, Benjamin Tissoires wrote:
> Ideally, we need to have Dragon Rise 0x0011 tested too. Nick, would you
> mind checking it if you still have this particular device?

I never had it, but perhaps Vladislav still has some.

Vladislav, would you be able to test a change to the kernel module for your
Dragonrise gamepads?

Please see below for context.

Thank you.

Nick

On 09/26/2016 12:29 PM, Benjamin Tissoires wrote:
> On Sep 26 2016 or thereabouts, Ioan-Adrian Ratiu wrote:
>> Commit 79346d620e9d ("HID: input: force generic axis to be mapped to their
>> user space axis") made mapping generic axes to their userspace equivalents
>> mandatory and some lower end gamepads which were depending on the previous
>> behaviour suffered severe regressions because they were reusing axes and
>> expecting hid-input to multiplex their map to the respective userspace axis
>> by always searching for and using the next available axis.
>
> Yes, I apologies for the breakage and the regression, though I must say
> that for now, only one hardware maker and one device (or range of devices
> from the look of it) has needed to be quirked.
>
>>
>> Now the result is that different device axes appear on a single axis in
>> userspace, which is clearly a regression in the hid-input driver because it
>> needs to continue to handle this hardware as expected before the forcing
>> to provide the same interface to userspace.
>>
>> Since these lower-end gamepads like 0079:0006 are definitely the exception,
>> create a quirk to fix them.
>
> Given that we only have this particular vendor that is an issue, I'd
> rather see the fix in hid-dr.c. The reason being that you actually don't
> need to have a global quirk and this simplifies the path in hid-input.
> Plus for users, they can just upgrade hid-dr without having to recompile
> their kernel when hid-core is not compiled as a module.
>
> The cleanest solution that wouldn't require any quirk in hid-core is to
> simply add an .input_mapping() callback in hid-dr.c.
>
> The code of the callback could be something like (untested):
>
> static int dr_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> 		struct hid_field *field, struct hid_usage *usage,
> 		unsigned long **bit, int *max)
> {
> 	switch (usage->hid) {
> 	/*
> 	 * revert the old hid-input behavior where axes
> 	 * can be randomly assigned when the hid usage is
> 	 * reused.
> 	 */
> 	case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
> 	case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
> 		if (field->flags & HID_MAIN_ITEM_RELATIVE)
> 			map_rel(usage->hid & 0xf);
> 		else
> 			map_abs(usage->hid & 0xf);
> 		return 1;
> 	}
> 	
> 	return 0;
> }
>
> Hopefully, something like this should revert the old behavior for all
> hid-dr touchpads.
>
> Ideally, we need to have Dragon Rise 0x0011 tested too. Nick, would you
> mind checking it if you still have this particular device?
>
> Cheers,
> Benjamin
>
>>
>> Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
>> ---
>>  drivers/hid/hid-dr.c    |  2 ++
>>  drivers/hid/hid-input.c | 16 +++++++++++-----
>>  include/linux/hid.h     |  1 +
>>  3 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hid/hid-dr.c b/drivers/hid/hid-dr.c
>> index 2523f8a..27fc826 100644
>> --- a/drivers/hid/hid-dr.c
>> +++ b/drivers/hid/hid-dr.c
>> @@ -274,6 +274,8 @@ static int dr_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>  			hid_hw_stop(hdev);
>>  			goto err;
>>  		}
>> +		/* has only 5 axes and reuses X, Y */
>> +		hdev->quirks |= HID_QUIRK_REUSE_AXES;
>>  		break;
>>  	}
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index fb9ace1..1cc6fe4 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -633,11 +633,17 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>>  		/* These usage IDs map directly to the usage codes. */
>>  		case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
>>  		case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
>> -			if (field->flags & HID_MAIN_ITEM_RELATIVE)
>> -				map_rel(usage->hid & 0xf);
>> -			else
>> -				map_abs_clear(usage->hid & 0xf);
>> -			break;
>> +
>> +			/* if quirk is active don't force the userspace mapping,
>> +			 * instead search and use the next available axis.
>> +			 */
>> +			if (!(device->quirks & HID_QUIRK_REUSE_AXES)) {
>> +				if (field->flags & HID_MAIN_ITEM_RELATIVE)
>> +					map_rel(usage->hid & 0xf);
>> +				else
>> +					map_abs_clear(usage->hid & 0xf);
>> +				break;
>> +			}
>>
>>  		case HID_GD_SLIDER: case HID_GD_DIAL: case HID_GD_WHEEL:
>>  			if (field->flags & HID_MAIN_ITEM_RELATIVE)
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 75b66ec..0979920 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -320,6 +320,7 @@ struct hid_item {
>>  #define HID_QUIRK_NO_EMPTY_INPUT		0x00000100
>>  #define HID_QUIRK_NO_INIT_INPUT_REPORTS		0x00000200
>>  #define HID_QUIRK_ALWAYS_POLL			0x00000400
>> +#define HID_QUIRK_REUSE_AXES			0x00000800
>>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
>>  #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		0x00020000
>>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	0x00040000
>> --
>> 2.10.0
>>

Re: [PATCH 2/2] hid: input: add HID_QUIRK_REUSE_AXES and fix dragonrise

[Vladislav Naumov]

Yes, I still have one of those!
0079:0011 DragonRise Inc. Gamepad
Left shift buttons are broken now, but axis and main buttons are still working.
Axis is handled properly with 3.16.0-4-686-pae #1 SMP Debian
3.16.7-ckt25-2 (2016-04-08) i686 GNU/Linux from debian/stable.
I can test what you want.
Should I apply the patch from forwarded message to upstream kernel, or
I can just pull it from some host with everything applied?

On Mon, Sep 26, 2016 at 4:53 PM, Nikolai Kondrashov <spbnick@gmail.com> wrote:
> Hi Benjamin,
>
> On 09/26/2016 12:29 PM, Benjamin Tissoires wrote:
>>
>> Ideally, we need to have Dragon Rise 0x0011 tested too. Nick, would you
>> mind checking it if you still have this particular device?
>
>
> I never had it, but perhaps Vladislav still has some.
>
> Vladislav, would you be able to test a change to the kernel module for your
> Dragonrise gamepads?
>
> Please see below for context.
>
> Thank you.
>
> Nick
>
> On 09/26/2016 12:29 PM, Benjamin Tissoires wrote:
>>
>> On Sep 26 2016 or thereabouts, Ioan-Adrian Ratiu wrote:
>>>
>>> Commit 79346d620e9d ("HID: input: force generic axis to be mapped to
>>> their
>>> user space axis") made mapping generic axes to their userspace
>>> equivalents
>>> mandatory and some lower end gamepads which were depending on the
>>> previous
>>> behaviour suffered severe regressions because they were reusing axes and
>>> expecting hid-input to multiplex their map to the respective userspace
>>> axis
>>> by always searching for and using the next available axis.
>>
>>
>> Yes, I apologies for the breakage and the regression, though I must say
>> that for now, only one hardware maker and one device (or range of devices
>> from the look of it) has needed to be quirked.
>>
>>>
>>> Now the result is that different device axes appear on a single axis in
>>> userspace, which is clearly a regression in the hid-input driver because
>>> it
>>> needs to continue to handle this hardware as expected before the forcing
>>> to provide the same interface to userspace.
>>>
>>> Since these lower-end gamepads like 0079:0006 are definitely the
>>> exception,
>>> create a quirk to fix them.
>>
>>
>> Given that we only have this particular vendor that is an issue, I'd
>> rather see the fix in hid-dr.c. The reason being that you actually don't
>> need to have a global quirk and this simplifies the path in hid-input.
>> Plus for users, they can just upgrade hid-dr without having to recompile
>> their kernel when hid-core is not compiled as a module.
>>
>> The cleanest solution that wouldn't require any quirk in hid-core is to
>> simply add an .input_mapping() callback in hid-dr.c.
>>
>> The code of the callback could be something like (untested):
>>
>> static int dr_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>                 struct hid_field *field, struct hid_usage *usage,
>>                 unsigned long **bit, int *max)
>> {
>>         switch (usage->hid) {
>>         /*
>>          * revert the old hid-input behavior where axes
>>          * can be randomly assigned when the hid usage is
>>          * reused.
>>          */
>>         case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
>>         case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
>>                 if (field->flags & HID_MAIN_ITEM_RELATIVE)
>>                         map_rel(usage->hid & 0xf);
>>                 else
>>                         map_abs(usage->hid & 0xf);
>>                 return 1;
>>         }
>>
>>         return 0;
>> }
>>
>> Hopefully, something like this should revert the old behavior for all
>> hid-dr touchpads.
>>
>> Ideally, we need to have Dragon Rise 0x0011 tested too. Nick, would you
>> mind checking it if you still have this particular device?
>>
>> Cheers,
>> Benjamin
>>
>>>
>>> Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
>>> ---
>>>  drivers/hid/hid-dr.c    |  2 ++
>>>  drivers/hid/hid-input.c | 16 +++++++++++-----
>>>  include/linux/hid.h     |  1 +
>>>  3 files changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-dr.c b/drivers/hid/hid-dr.c
>>> index 2523f8a..27fc826 100644
>>> --- a/drivers/hid/hid-dr.c
>>> +++ b/drivers/hid/hid-dr.c
>>> @@ -274,6 +274,8 @@ static int dr_probe(struct hid_device *hdev, const
>>> struct hid_device_id *id)
>>>                         hid_hw_stop(hdev);
>>>                         goto err;
>>>                 }
>>> +               /* has only 5 axes and reuses X, Y */
>>> +               hdev->quirks |= HID_QUIRK_REUSE_AXES;
>>>                 break;
>>>         }
>>>
>>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>>> index fb9ace1..1cc6fe4 100644
>>> --- a/drivers/hid/hid-input.c
>>> +++ b/drivers/hid/hid-input.c
>>> @@ -633,11 +633,17 @@ static void hidinput_configure_usage(struct
>>> hid_input *hidinput, struct hid_fiel
>>>                 /* These usage IDs map directly to the usage codes. */
>>>                 case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
>>>                 case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
>>> -                       if (field->flags & HID_MAIN_ITEM_RELATIVE)
>>> -                               map_rel(usage->hid & 0xf);
>>> -                       else
>>> -                               map_abs_clear(usage->hid & 0xf);
>>> -                       break;
>>> +
>>> +                       /* if quirk is active don't force the userspace
>>> mapping,
>>> +                        * instead search and use the next available
>>> axis.
>>> +                        */
>>> +                       if (!(device->quirks & HID_QUIRK_REUSE_AXES)) {
>>> +                               if (field->flags &
>>> HID_MAIN_ITEM_RELATIVE)
>>> +                                       map_rel(usage->hid & 0xf);
>>> +                               else
>>> +                                       map_abs_clear(usage->hid & 0xf);
>>> +                               break;
>>> +                       }
>>>
>>>                 case HID_GD_SLIDER: case HID_GD_DIAL: case HID_GD_WHEEL:
>>>                         if (field->flags & HID_MAIN_ITEM_RELATIVE)
>>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>>> index 75b66ec..0979920 100644
>>> --- a/include/linux/hid.h
>>> +++ b/include/linux/hid.h
>>> @@ -320,6 +320,7 @@ struct hid_item {
>>>  #define HID_QUIRK_NO_EMPTY_INPUT               0x00000100
>>>  #define HID_QUIRK_NO_INIT_INPUT_REPORTS                0x00000200
>>>  #define HID_QUIRK_ALWAYS_POLL                  0x00000400
>>> +#define HID_QUIRK_REUSE_AXES                   0x00000800
>>>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS          0x00010000
>>>  #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID                0x00020000
>>>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP 0x00040000
>>> --
>>> 2.10.0
>>>
>

Re: [PATCH 2/2] hid: input: add HID_QUIRK_REUSE_AXES and fix dragonrise

[Ioan-Adrian Ratiu]

On Tue, 27 Sep 2016, Vladislav Naumov <vnaum@vnaum.com> wrote:
> Yes, I still have one of those!
> 0079:0011 DragonRise Inc. Gamepad
> Left shift buttons are broken now, but axis and main buttons are still working.
> Axis is handled properly with 3.16.0-4-686-pae #1 SMP Debian
> 3.16.7-ckt25-2 (2016-04-08) i686 GNU/Linux from debian/stable.
> I can test what you want.

Can you please wait a little until I post v2 later today and test v2
directly? Because the change in it's current form has no effect on
0079:0011 (the current quirk is enabled only for 0006).

When I add the input mapping in the hid-dr driver then it will affect
both 0006 and 0011 so that's the patch really worth testing.

Thanks a lot for taking time to test this,
Ionel

> Should I apply the patch from forwarded message to upstream kernel, or
> I can just pull it from some host with everything applied?
>
> On Mon, Sep 26, 2016 at 4:53 PM, Nikolai Kondrashov <spbnick@gmail.com> wrote:
>> Hi Benjamin,
>>
>> On 09/26/2016 12:29 PM, Benjamin Tissoires wrote:
>>>
>>> Ideally, we need to have Dragon Rise 0x0011 tested too. Nick, would you
>>> mind checking it if you still have this particular device?
>>
>>
>> I never had it, but perhaps Vladislav still has some.
>>
>> Vladislav, would you be able to test a change to the kernel module for your
>> Dragonrise gamepads?
>>
>> Please see below for context.
>>
>> Thank you.
>>
>> Nick
>>
>> On 09/26/2016 12:29 PM, Benjamin Tissoires wrote:
>>>
>>> On Sep 26 2016 or thereabouts, Ioan-Adrian Ratiu wrote:
>>>>
>>>> Commit 79346d620e9d ("HID: input: force generic axis to be mapped to
>>>> their
>>>> user space axis") made mapping generic axes to their userspace
>>>> equivalents
>>>> mandatory and some lower end gamepads which were depending on the
>>>> previous
>>>> behaviour suffered severe regressions because they were reusing axes and
>>>> expecting hid-input to multiplex their map to the respective userspace
>>>> axis
>>>> by always searching for and using the next available axis.
>>>
>>>
>>> Yes, I apologies for the breakage and the regression, though I must say
>>> that for now, only one hardware maker and one device (or range of devices
>>> from the look of it) has needed to be quirked.
>>>
>>>>
>>>> Now the result is that different device axes appear on a single axis in
>>>> userspace, which is clearly a regression in the hid-input driver because
>>>> it
>>>> needs to continue to handle this hardware as expected before the forcing
>>>> to provide the same interface to userspace.
>>>>
>>>> Since these lower-end gamepads like 0079:0006 are definitely the
>>>> exception,
>>>> create a quirk to fix them.
>>>
>>>
>>> Given that we only have this particular vendor that is an issue, I'd
>>> rather see the fix in hid-dr.c. The reason being that you actually don't
>>> need to have a global quirk and this simplifies the path in hid-input.
>>> Plus for users, they can just upgrade hid-dr without having to recompile
>>> their kernel when hid-core is not compiled as a module.
>>>
>>> The cleanest solution that wouldn't require any quirk in hid-core is to
>>> simply add an .input_mapping() callback in hid-dr.c.
>>>
>>> The code of the callback could be something like (untested):
>>>
>>> static int dr_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>>                 struct hid_field *field, struct hid_usage *usage,
>>>                 unsigned long **bit, int *max)
>>> {
>>>         switch (usage->hid) {
>>>         /*
>>>          * revert the old hid-input behavior where axes
>>>          * can be randomly assigned when the hid usage is
>>>          * reused.
>>>          */
>>>         case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
>>>         case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
>>>                 if (field->flags & HID_MAIN_ITEM_RELATIVE)
>>>                         map_rel(usage->hid & 0xf);
>>>                 else
>>>                         map_abs(usage->hid & 0xf);
>>>                 return 1;
>>>         }
>>>
>>>         return 0;
>>> }
>>>
>>> Hopefully, something like this should revert the old behavior for all
>>> hid-dr touchpads.
>>>
>>> Ideally, we need to have Dragon Rise 0x0011 tested too. Nick, would you
>>> mind checking it if you still have this particular device?
>>>
>>> Cheers,
>>> Benjamin
>>>
>>>>
>>>> Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
>>>> ---
>>>>  drivers/hid/hid-dr.c    |  2 ++
>>>>  drivers/hid/hid-input.c | 16 +++++++++++-----
>>>>  include/linux/hid.h     |  1 +
>>>>  3 files changed, 14 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/hid/hid-dr.c b/drivers/hid/hid-dr.c
>>>> index 2523f8a..27fc826 100644
>>>> --- a/drivers/hid/hid-dr.c
>>>> +++ b/drivers/hid/hid-dr.c
>>>> @@ -274,6 +274,8 @@ static int dr_probe(struct hid_device *hdev, const
>>>> struct hid_device_id *id)
>>>>                         hid_hw_stop(hdev);
>>>>                         goto err;
>>>>                 }
>>>> +               /* has only 5 axes and reuses X, Y */
>>>> +               hdev->quirks |= HID_QUIRK_REUSE_AXES;
>>>>                 break;
>>>>         }
>>>>
>>>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>>>> index fb9ace1..1cc6fe4 100644
>>>> --- a/drivers/hid/hid-input.c
>>>> +++ b/drivers/hid/hid-input.c
>>>> @@ -633,11 +633,17 @@ static void hidinput_configure_usage(struct
>>>> hid_input *hidinput, struct hid_fiel
>>>>                 /* These usage IDs map directly to the usage codes. */
>>>>                 case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
>>>>                 case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
>>>> -                       if (field->flags & HID_MAIN_ITEM_RELATIVE)
>>>> -                               map_rel(usage->hid & 0xf);
>>>> -                       else
>>>> -                               map_abs_clear(usage->hid & 0xf);
>>>> -                       break;
>>>> +
>>>> +                       /* if quirk is active don't force the userspace
>>>> mapping,
>>>> +                        * instead search and use the next available
>>>> axis.
>>>> +                        */
>>>> +                       if (!(device->quirks & HID_QUIRK_REUSE_AXES)) {
>>>> +                               if (field->flags &
>>>> HID_MAIN_ITEM_RELATIVE)
>>>> +                                       map_rel(usage->hid & 0xf);
>>>> +                               else
>>>> +                                       map_abs_clear(usage->hid & 0xf);
>>>> +                               break;
>>>> +                       }
>>>>
>>>>                 case HID_GD_SLIDER: case HID_GD_DIAL: case HID_GD_WHEEL:
>>>>                         if (field->flags & HID_MAIN_ITEM_RELATIVE)
>>>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>>>> index 75b66ec..0979920 100644
>>>> --- a/include/linux/hid.h
>>>> +++ b/include/linux/hid.h
>>>> @@ -320,6 +320,7 @@ struct hid_item {
>>>>  #define HID_QUIRK_NO_EMPTY_INPUT               0x00000100
>>>>  #define HID_QUIRK_NO_INIT_INPUT_REPORTS                0x00000200
>>>>  #define HID_QUIRK_ALWAYS_POLL                  0x00000400
>>>> +#define HID_QUIRK_REUSE_AXES                   0x00000800
>>>>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS          0x00010000
>>>>  #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID                0x00020000
>>>>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP 0x00040000
>>>> --
>>>> 2.10.0
>>>>
>>

Re: [PATCH 2/2] hid: input: add HID_QUIRK_REUSE_AXES and fix dragonrise

[Vladislav Naumov]

On Tue, Sep 27, 2016 at 10:44 PM, Ioan-Adrian Ratiu <adi@adirat.com> wrote:
> Can you please wait a little until I post v2 later today and test v2
> directly? Because the change in it's current form has no effect on
> 0079:0011 (the current quirk is enabled only for 0006).

Sure thing!
Just drop me a line when it's ready.

Re: [PATCH 2/2] hid: input: add HID_QUIRK_REUSE_AXES and fix dragonrise

[Ioan-Adrian Ratiu]

On Wed, 28 Sep 2016, Vladislav Naumov <vnaum@vnaum.com> wrote:
> On Tue, Sep 27, 2016 at 10:44 PM, Ioan-Adrian Ratiu <adi@adirat.com> wrote:
>> Can you please wait a little until I post v2 later today and test v2
>> directly? Because the change in it's current form has no effect on
>> 0079:0011 (the current quirk is enabled only for 0006).
>
> Sure thing!
> Just drop me a line when it's ready.

Hi

I posted v2 last night but instead of adding you to CC now I see I've
added Nikolai by mistake.

You can use this branch [1] from github if you want to build the patches
applied directly to linus' master (they are HEAD and HEAD~1 commits).

[1] https://github.com/10ne1/linux/commits/dev/aratiu/fix-dragonrise-gamepad

Cheers,
Adrian

Re: [PATCH 2/2] hid: input: add HID_QUIRK_REUSE_AXES and fix dragonrise

[Vladislav Naumov]

I fetched `dev/aratiu/fix-dragonrise-gamepad` branch from
https://github.com/10ne1/linux
at commit a7dd8e2 "hid: hid-dr: add input mapping for axis selection"
and built 32-bit kernel (default debian config).

0079:0011 DragonRise Inc. Gamepad works properly, both axis and all buttons.

TEST PASSED.

Re: [PATCH 1/2] Revert "HID: dragonrise: fix HID Descriptor for 0x0006 PID"

[Benjamin Tissoires]

Thanks for the patch series. I am not against it, but I'd rather see the
commit message of this one amended, and the second patch changed.

On Sep 26 2016 or thereabouts, Ioan-Adrian Ratiu wrote:
> This reverts commit 18339f59c3a6 ("HID: dragonrise: fix HID...")
> because the "fix" is bogus. That report descriptor is different in

I am pretty sure this "fix" works for many. You seem to have a different
hardware (generation probably) that makes the "fix" to fail for you.
The issue is more that the manufacturer doesn't bother to reallocate a
new PID for the new device when they change something in it, so please
don't blame the author of the fix (which I am not).

> hardware (see below) and it's the way the hardware works, it can't be

Well, it's kind of hard to compare the lsusb output to the fixup in the
kernel. I'd like to know what changed, but I can't...

> fixed at this level because it reuses axes by design.

It can be fixed in hid-dr. See my comments on the next patch.

> 
> What this change tried to fix is a regression caused by commit 20aef664f139

As mentioned in your next patch in the series, the correct commit id is
79346d620e9de87912de73337f6df8b7f9a46888

> ("HID: input: force generic axis to be mapped to their user space axis") by
> working around the problem and trying to change the report descriptor in
> hid-dr, which obviously can't work and introduces more breakage because it
> adds another unnecessary layer of multiplexing/indirection, making the
> dragonrise gamepad practically unusable in userspace.

Not sure who you are blaming here. Is it me (the author of 79346d620e or
the author of the fix in hid-dr)? If it's me, I agree, the patch was a
little too aggressive, though I must say only one hardware maker has
such a crappy device that we need to care of. So this is why I just let
the patch in place without trying to have a better solution.

If the blame is on the author of the hid-dr, I must say that I find the
tone of this paragraph quite aggressive for nothing. Your device is
different than the one that was used for the original fix, so it breaks.
But I can guarantee you that the fix works for the intended device (I
happen to have one I tested recently). So please blame the hardware
maker, not the people involved in the community who are doing their
best.

> 
> This needs to be fixed where the regression was initially introduced in
> hid-input (the next patch does this).

Actually, again, I tend to disagree :)
I'll go more in details in the next patch.

Cheers,
Benjamin

> 
> Here's the descriptor taken directly from the device via lsusb:
>         HID Device Descriptor:
>           bLength                 9
>           bDescriptorType        33
>           bcdHID               1.10
>           bCountryCode           33 US
>           bNumDescriptors         1
>           bDescriptorType        34 Report
>           wDescriptorLength     101
>           Report Descriptor: (length is 101)
>             Item(Global): Usage Page, data= [ 0x01 ] 1
>                             Generic Desktop Controls
>             Item(Local ): Usage, data= [ 0x04 ] 4
>                             Joystick
>             Item(Main  ): Collection, data= [ 0x01 ] 1
>                             Application
>             Item(Main  ): Collection, data= [ 0x02 ] 2
>                             Logical
>             Item(Global): Report Size, data= [ 0x08 ] 8
>             Item(Global): Report Count, data= [ 0x05 ] 5
>             Item(Global): Logical Minimum, data= [ 0x00 ] 0
>             Item(Global): Logical Maximum, data= [ 0xff 0x00 ] 255
>             Item(Global): Physical Minimum, data= [ 0x00 ] 0
>             Item(Global): Physical Maximum, data= [ 0xff 0x00 ] 255
>             Item(Local ): Usage, data= [ 0x30 ] 48
>                             Direction-X
>             Item(Local ): Usage, data= [ 0x31 ] 49
>                             Direction-Y
>             Item(Local ): Usage, data= [ 0x32 ] 50
>                             Direction-Z
>             Item(Local ): Usage, data= [ 0x32 ] 50
>                             Direction-Z
>             Item(Local ): Usage, data= [ 0x35 ] 53
>                             Rotate-Z
>             Item(Main  ): Input, data= [ 0x02 ] 2
>                             Data Variable Absolute No_Wrap Linear
>                             Preferred_State No_Null_Position Non_Volatile Bitfield
>             Item(Global): Report Size, data= [ 0x04 ] 4
>             Item(Global): Report Count, data= [ 0x01 ] 1
>             Item(Global): Logical Maximum, data= [ 0x07 ] 7
>             Item(Global): Physical Maximum, data= [ 0x3b 0x01 ] 315
>             Item(Global): Unit, data= [ 0x14 ] 20
>                             System: English Rotation, Unit: Degrees
>             Item(Local ): Usage, data= [ 0x39 ] 57
>                             Hat Switch
>             Item(Main  ): Input, data= [ 0x42 ] 66
>                             Data Variable Absolute No_Wrap Linear
>                             Preferred_State Null_State Non_Volatile Bitfield
>             Item(Global): Unit, data= [ 0x00 ] 0
>                             System: None, Unit: (None)
>             Item(Global): Report Size, data= [ 0x01 ] 1
>             Item(Global): Report Count, data= [ 0x0c ] 12
>             Item(Global): Logical Maximum, data= [ 0x01 ] 1
>             Item(Global): Physical Maximum, data= [ 0x01 ] 1
>             Item(Global): Usage Page, data= [ 0x09 ] 9
>                             Buttons
>             Item(Local ): Usage Minimum, data= [ 0x01 ] 1
>                             Button 1 (Primary)
>             Item(Local ): Usage Maximum, data= [ 0x0c ] 12
>                             (null)
>             Item(Main  ): Input, data= [ 0x02 ] 2
>                             Data Variable Absolute No_Wrap Linear
>                             Preferred_State No_Null_Position Non_Volatile Bitfield
>             Item(Global): Usage Page, data= [ 0x00 0xff ] 65280
>                             (null)
>             Item(Global): Report Size, data= [ 0x01 ] 1
>             Item(Global): Report Count, data= [ 0x08 ] 8
>             Item(Global): Logical Maximum, data= [ 0x01 ] 1
>             Item(Global): Physical Maximum, data= [ 0x01 ] 1
>             Item(Local ): Usage, data= [ 0x01 ] 1
>                             (null)
>             Item(Main  ): Input, data= [ 0x02 ] 2
>                             Data Variable Absolute No_Wrap Linear
>                             Preferred_State No_Null_Position Non_Volatile Bitfield
>             Item(Main  ): End Collection, data=none
>             Item(Main  ): Collection, data= [ 0x02 ] 2
>                             Logical
>             Item(Global): Report Size, data= [ 0x08 ] 8
>             Item(Global): Report Count, data= [ 0x07 ] 7
>             Item(Global): Physical Maximum, data= [ 0xff 0x00 ] 255
>             Item(Global): Logical Maximum, data= [ 0xff 0x00 ] 255
>             Item(Local ): Usage, data= [ 0x02 ] 2
>                             (null)
>             Item(Main  ): Output, data= [ 0x02 ] 2
>                             Data Variable Absolute No_Wrap Linear
>                             Preferred_State No_Null_Position Non_Volatile Bitfield
>             Item(Main  ): End Collection, data=none
>             Item(Main  ): End Collection, data=none
> 
> Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
> ---
>  drivers/hid/hid-dr.c | 58 ----------------------------------------------------
>  1 file changed, 58 deletions(-)
> 
> diff --git a/drivers/hid/hid-dr.c b/drivers/hid/hid-dr.c
> index 8fd4bf7..2523f8a 100644
> --- a/drivers/hid/hid-dr.c
> +++ b/drivers/hid/hid-dr.c
> @@ -234,58 +234,6 @@ static __u8 pid0011_rdesc_fixed[] = {
>  	0xC0                /*  End Collection                  */
>  };
>  
> -static __u8 pid0006_rdesc_fixed[] = {
> -	0x05, 0x01,        /* Usage Page (Generic Desktop)	*/
> -	0x09, 0x04,        /* Usage (Joystick)			*/
> -	0xA1, 0x01,        /* Collection (Application)		*/
> -	0xA1, 0x02,        /*   Collection (Logical)		*/
> -	0x75, 0x08,        /*     Report Size (8)		*/
> -	0x95, 0x05,        /*     Report Count (5)		*/
> -	0x15, 0x00,        /*     Logical Minimum (0)		*/
> -	0x26, 0xFF, 0x00,  /*     Logical Maximum (255)		*/
> -	0x35, 0x00,        /*     Physical Minimum (0)		*/
> -	0x46, 0xFF, 0x00,  /*     Physical Maximum (255)	*/
> -	0x09, 0x30,        /*     Usage (X)			*/
> -	0x09, 0x33,        /*     Usage (Ry)			*/
> -	0x09, 0x32,        /*     Usage (Z)			*/
> -	0x09, 0x31,        /*     Usage (Y)			*/
> -	0x09, 0x34,        /*     Usage (Ry)			*/
> -	0x81, 0x02,        /*     Input (Variable)		*/
> -	0x75, 0x04,        /*     Report Size (4)		*/
> -	0x95, 0x01,        /*     Report Count (1)		*/
> -	0x25, 0x07,        /*     Logical Maximum (7)		*/
> -	0x46, 0x3B, 0x01,  /*     Physical Maximum (315)	*/
> -	0x65, 0x14,        /*     Unit (Centimeter)		*/
> -	0x09, 0x39,        /*     Usage (Hat switch)		*/
> -	0x81, 0x42,        /*     Input (Variable)		*/
> -	0x65, 0x00,        /*     Unit (None)			*/
> -	0x75, 0x01,        /*     Report Size (1)		*/
> -	0x95, 0x0C,        /*     Report Count (12)		*/
> -	0x25, 0x01,        /*     Logical Maximum (1)		*/
> -	0x45, 0x01,        /*     Physical Maximum (1)		*/
> -	0x05, 0x09,        /*     Usage Page (Button)		*/
> -	0x19, 0x01,        /*     Usage Minimum (0x01)		*/
> -	0x29, 0x0C,        /*     Usage Maximum (0x0C)		*/
> -	0x81, 0x02,        /*     Input (Variable)		*/
> -	0x06, 0x00, 0xFF,  /*     Usage Page (Vendor Defined)	*/
> -	0x75, 0x01,        /*     Report Size (1)		*/
> -	0x95, 0x08,        /*     Report Count (8)		*/
> -	0x25, 0x01,        /*     Logical Maximum (1)		*/
> -	0x45, 0x01,        /*     Physical Maximum (1)		*/
> -	0x09, 0x01,        /*     Usage (0x01)			*/
> -	0x81, 0x02,        /*     Input (Variable)		*/
> -	0xC0,              /*   End Collection			*/
> -	0xA1, 0x02,        /*   Collection (Logical)		*/
> -	0x75, 0x08,        /*     Report Size (8)		*/
> -	0x95, 0x07,        /*     Report Count (7)		*/
> -	0x46, 0xFF, 0x00,  /*     Physical Maximum (255)	*/
> -	0x26, 0xFF, 0x00,  /*     Logical Maximum (255)		*/
> -	0x09, 0x02,        /*     Usage (0x02)			*/
> -	0x91, 0x02,        /*     Output (Variable)		*/
> -	0xC0,              /*   End Collection			*/
> -	0xC0               /* End Collection			*/
> -};
> -
>  static __u8 *dr_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>  				unsigned int *rsize)
>  {
> @@ -296,12 +244,6 @@ static __u8 *dr_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>  			*rsize = sizeof(pid0011_rdesc_fixed);
>  		}
>  		break;
> -	case 0x0006:
> -		if (*rsize == sizeof(pid0006_rdesc_fixed)) {
> -			rdesc = pid0006_rdesc_fixed;
> -			*rsize = sizeof(pid0006_rdesc_fixed);
> -		}
> -		break;
>  	}
>  	return rdesc;
>  }
> -- 
> 2.10.0
> 

Re: [PATCH 1/2] Revert "HID: dragonrise: fix HID Descriptor for 0x0006 PID"

[Ioan-Adrian Ratiu]

Hi

On Mon, 26 Sep 2016, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> Thanks for the patch series. I am not against it, but I'd rather see the
> commit message of this one amended, and the second patch changed.

Sorry if I came out too aggresive (I'll amend), I'm just annoyed that I
had to spend a weekend night digging through this crap because my fiancee
came crying to me that her gamepad stopped working after a kernel update.

>
> On Sep 26 2016 or thereabouts, Ioan-Adrian Ratiu wrote:
>> This reverts commit 18339f59c3a6 ("HID: dragonrise: fix HID...")
>> because the "fix" is bogus. That report descriptor is different in
>
> I am pretty sure this "fix" works for many. You seem to have a different
> hardware (generation probably) that makes the "fix" to fail for you.
> The issue is more that the manufacturer doesn't bother to reallocate a
> new PID for the new device when they change something in it, so please
> don't blame the author of the fix (which I am not).

We don't actually know that it's a new generation of hardware without a
new PID and I really doubt manufacturers are *that* stupid, they should
at least have increased the device revision number, but... China.

>
>> hardware (see below) and it's the way the hardware works, it can't be
>
> Well, it's kind of hard to compare the lsusb output to the fixup in the
> kernel. I'd like to know what changed, but I can't...
>
>> fixed at this level because it reuses axes by design.
>
> It can be fixed in hid-dr. See my comments on the next patch.

Thanks, I'll try to add an input mapping in the driver. However if
indeed there are different hardware with the same PID and this "fix" is
for another issue than the one I'm having then I'd really rather not
revert this if possible to not break other people's hardware. But I also
can't keep it because it breaks my hardware.

Does anyone have any suggestions what to do in this case?

>
>> 
>> What this change tried to fix is a regression caused by commit 20aef664f139
>
> As mentioned in your next patch in the series, the correct commit id is
> 79346d620e9de87912de73337f6df8b7f9a46888

Thanks, this was a slip-up on my part.

>
>> ("HID: input: force generic axis to be mapped to their user space axis") by
>> working around the problem and trying to change the report descriptor in
>> hid-dr, which obviously can't work and introduces more breakage because it
>> adds another unnecessary layer of multiplexing/indirection, making the
>> dragonrise gamepad practically unusable in userspace.
>
> Not sure who you are blaming here. Is it me (the author of 79346d620e or
> the author of the fix in hid-dr)? If it's me, I agree, the patch was a
> little too aggressive, though I must say only one hardware maker has
> such a crappy device that we need to care of. So this is why I just let
> the patch in place without trying to have a better solution.
>
> If the blame is on the author of the hid-dr, I must say that I find the
> tone of this paragraph quite aggressive for nothing. Your device is
> different than the one that was used for the original fix, so it breaks.
> But I can guarantee you that the fix works for the intended device (I
> happen to have one I tested recently). So please blame the hardware
> maker, not the people involved in the community who are doing their
> best.

I'm not assigning blame because it's counterproductive. Of course, I
agree if we are to find a scapegoat it's the crappy manufacturers
because they make the crappy hardware with crappy or no Linux support
(btw I am in no way affiliated with dragonrise or any other vendor).

The only thing that annoys me is that this known hid-input kernel
regression has been ignored for all this time, leaving users like me
dead in the watter with a broken driver upon kernel update.

>
>> 
>> This needs to be fixed where the regression was initially introduced in
>> hid-input (the next patch does this).
>
> Actually, again, I tend to disagree :)
> I'll go more in details in the next patch.
>
> Cheers,
> Benjamin
>
>> 
>> Here's the descriptor taken directly from the device via lsusb:
>>         HID Device Descriptor:
>>           bLength                 9
>>           bDescriptorType        33
>>           bcdHID               1.10
>>           bCountryCode           33 US
>>           bNumDescriptors         1
>>           bDescriptorType        34 Report
>>           wDescriptorLength     101
>>           Report Descriptor: (length is 101)
>>             Item(Global): Usage Page, data= [ 0x01 ] 1
>>                             Generic Desktop Controls
>>             Item(Local ): Usage, data= [ 0x04 ] 4
>>                             Joystick
>>             Item(Main  ): Collection, data= [ 0x01 ] 1
>>                             Application
>>             Item(Main  ): Collection, data= [ 0x02 ] 2
>>                             Logical
>>             Item(Global): Report Size, data= [ 0x08 ] 8
>>             Item(Global): Report Count, data= [ 0x05 ] 5
>>             Item(Global): Logical Minimum, data= [ 0x00 ] 0
>>             Item(Global): Logical Maximum, data= [ 0xff 0x00 ] 255
>>             Item(Global): Physical Minimum, data= [ 0x00 ] 0
>>             Item(Global): Physical Maximum, data= [ 0xff 0x00 ] 255
>>             Item(Local ): Usage, data= [ 0x30 ] 48
>>                             Direction-X
>>             Item(Local ): Usage, data= [ 0x31 ] 49
>>                             Direction-Y
>>             Item(Local ): Usage, data= [ 0x32 ] 50
>>                             Direction-Z
>>             Item(Local ): Usage, data= [ 0x32 ] 50
>>                             Direction-Z
>>             Item(Local ): Usage, data= [ 0x35 ] 53
>>                             Rotate-Z
>>             Item(Main  ): Input, data= [ 0x02 ] 2
>>                             Data Variable Absolute No_Wrap Linear
>>                             Preferred_State No_Null_Position Non_Volatile Bitfield
>>             Item(Global): Report Size, data= [ 0x04 ] 4
>>             Item(Global): Report Count, data= [ 0x01 ] 1
>>             Item(Global): Logical Maximum, data= [ 0x07 ] 7
>>             Item(Global): Physical Maximum, data= [ 0x3b 0x01 ] 315
>>             Item(Global): Unit, data= [ 0x14 ] 20
>>                             System: English Rotation, Unit: Degrees
>>             Item(Local ): Usage, data= [ 0x39 ] 57
>>                             Hat Switch
>>             Item(Main  ): Input, data= [ 0x42 ] 66
>>                             Data Variable Absolute No_Wrap Linear
>>                             Preferred_State Null_State Non_Volatile Bitfield
>>             Item(Global): Unit, data= [ 0x00 ] 0
>>                             System: None, Unit: (None)
>>             Item(Global): Report Size, data= [ 0x01 ] 1
>>             Item(Global): Report Count, data= [ 0x0c ] 12
>>             Item(Global): Logical Maximum, data= [ 0x01 ] 1
>>             Item(Global): Physical Maximum, data= [ 0x01 ] 1
>>             Item(Global): Usage Page, data= [ 0x09 ] 9
>>                             Buttons
>>             Item(Local ): Usage Minimum, data= [ 0x01 ] 1
>>                             Button 1 (Primary)
>>             Item(Local ): Usage Maximum, data= [ 0x0c ] 12
>>                             (null)
>>             Item(Main  ): Input, data= [ 0x02 ] 2
>>                             Data Variable Absolute No_Wrap Linear
>>                             Preferred_State No_Null_Position Non_Volatile Bitfield
>>             Item(Global): Usage Page, data= [ 0x00 0xff ] 65280
>>                             (null)
>>             Item(Global): Report Size, data= [ 0x01 ] 1
>>             Item(Global): Report Count, data= [ 0x08 ] 8
>>             Item(Global): Logical Maximum, data= [ 0x01 ] 1
>>             Item(Global): Physical Maximum, data= [ 0x01 ] 1
>>             Item(Local ): Usage, data= [ 0x01 ] 1
>>                             (null)
>>             Item(Main  ): Input, data= [ 0x02 ] 2
>>                             Data Variable Absolute No_Wrap Linear
>>                             Preferred_State No_Null_Position Non_Volatile Bitfield
>>             Item(Main  ): End Collection, data=none
>>             Item(Main  ): Collection, data= [ 0x02 ] 2
>>                             Logical
>>             Item(Global): Report Size, data= [ 0x08 ] 8
>>             Item(Global): Report Count, data= [ 0x07 ] 7
>>             Item(Global): Physical Maximum, data= [ 0xff 0x00 ] 255
>>             Item(Global): Logical Maximum, data= [ 0xff 0x00 ] 255
>>             Item(Local ): Usage, data= [ 0x02 ] 2
>>                             (null)
>>             Item(Main  ): Output, data= [ 0x02 ] 2
>>                             Data Variable Absolute No_Wrap Linear
>>                             Preferred_State No_Null_Position Non_Volatile Bitfield
>>             Item(Main  ): End Collection, data=none
>>             Item(Main  ): End Collection, data=none
>> 
>> Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
>> ---
>>  drivers/hid/hid-dr.c | 58 ----------------------------------------------------
>>  1 file changed, 58 deletions(-)
>> 
>> diff --git a/drivers/hid/hid-dr.c b/drivers/hid/hid-dr.c
>> index 8fd4bf7..2523f8a 100644
>> --- a/drivers/hid/hid-dr.c
>> +++ b/drivers/hid/hid-dr.c
>> @@ -234,58 +234,6 @@ static __u8 pid0011_rdesc_fixed[] = {
>>  	0xC0                /*  End Collection                  */
>>  };
>>  
>> -static __u8 pid0006_rdesc_fixed[] = {
>> -	0x05, 0x01,        /* Usage Page (Generic Desktop)	*/
>> -	0x09, 0x04,        /* Usage (Joystick)			*/
>> -	0xA1, 0x01,        /* Collection (Application)		*/
>> -	0xA1, 0x02,        /*   Collection (Logical)		*/
>> -	0x75, 0x08,        /*     Report Size (8)		*/
>> -	0x95, 0x05,        /*     Report Count (5)		*/
>> -	0x15, 0x00,        /*     Logical Minimum (0)		*/
>> -	0x26, 0xFF, 0x00,  /*     Logical Maximum (255)		*/
>> -	0x35, 0x00,        /*     Physical Minimum (0)		*/
>> -	0x46, 0xFF, 0x00,  /*     Physical Maximum (255)	*/
>> -	0x09, 0x30,        /*     Usage (X)			*/
>> -	0x09, 0x33,        /*     Usage (Ry)			*/
>> -	0x09, 0x32,        /*     Usage (Z)			*/
>> -	0x09, 0x31,        /*     Usage (Y)			*/
>> -	0x09, 0x34,        /*     Usage (Ry)			*/
>> -	0x81, 0x02,        /*     Input (Variable)		*/
>> -	0x75, 0x04,        /*     Report Size (4)		*/
>> -	0x95, 0x01,        /*     Report Count (1)		*/
>> -	0x25, 0x07,        /*     Logical Maximum (7)		*/
>> -	0x46, 0x3B, 0x01,  /*     Physical Maximum (315)	*/
>> -	0x65, 0x14,        /*     Unit (Centimeter)		*/
>> -	0x09, 0x39,        /*     Usage (Hat switch)		*/
>> -	0x81, 0x42,        /*     Input (Variable)		*/
>> -	0x65, 0x00,        /*     Unit (None)			*/
>> -	0x75, 0x01,        /*     Report Size (1)		*/
>> -	0x95, 0x0C,        /*     Report Count (12)		*/
>> -	0x25, 0x01,        /*     Logical Maximum (1)		*/
>> -	0x45, 0x01,        /*     Physical Maximum (1)		*/
>> -	0x05, 0x09,        /*     Usage Page (Button)		*/
>> -	0x19, 0x01,        /*     Usage Minimum (0x01)		*/
>> -	0x29, 0x0C,        /*     Usage Maximum (0x0C)		*/
>> -	0x81, 0x02,        /*     Input (Variable)		*/
>> -	0x06, 0x00, 0xFF,  /*     Usage Page (Vendor Defined)	*/
>> -	0x75, 0x01,        /*     Report Size (1)		*/
>> -	0x95, 0x08,        /*     Report Count (8)		*/
>> -	0x25, 0x01,        /*     Logical Maximum (1)		*/
>> -	0x45, 0x01,        /*     Physical Maximum (1)		*/
>> -	0x09, 0x01,        /*     Usage (0x01)			*/
>> -	0x81, 0x02,        /*     Input (Variable)		*/
>> -	0xC0,              /*   End Collection			*/
>> -	0xA1, 0x02,        /*   Collection (Logical)		*/
>> -	0x75, 0x08,        /*     Report Size (8)		*/
>> -	0x95, 0x07,        /*     Report Count (7)		*/
>> -	0x46, 0xFF, 0x00,  /*     Physical Maximum (255)	*/
>> -	0x26, 0xFF, 0x00,  /*     Logical Maximum (255)		*/
>> -	0x09, 0x02,        /*     Usage (0x02)			*/
>> -	0x91, 0x02,        /*     Output (Variable)		*/
>> -	0xC0,              /*   End Collection			*/
>> -	0xC0               /* End Collection			*/
>> -};
>> -
>>  static __u8 *dr_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>>  				unsigned int *rsize)
>>  {
>> @@ -296,12 +244,6 @@ static __u8 *dr_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>>  			*rsize = sizeof(pid0011_rdesc_fixed);
>>  		}
>>  		break;
>> -	case 0x0006:
>> -		if (*rsize == sizeof(pid0006_rdesc_fixed)) {
>> -			rdesc = pid0006_rdesc_fixed;
>> -			*rsize = sizeof(pid0006_rdesc_fixed);
>> -		}
>> -		break;
>>  	}
>>  	return rdesc;
>>  }
>> -- 
>> 2.10.0
>>