[PATCH v2 0/2] HID: input: do not increment usages when a duplicate is found

[PATCH v2 0/2] HID: input: do not increment usages when a duplicate is found

[Benjamin Tissoires]

Hi Jiri,

slightly modified version (to actually make it working this time).

There is not much to add, the differences are in the commit messages
and in the notes of each patch.

Cheers,
Benjamin

Benjamin Tissoires (2):
  HID: use BIT macro instead of plain integers for flags
  HID: input: do not increment usages when a duplicate is found

 drivers/hid/hid-input.c | 33 +++++++++++++++++++++++++++++++--
 include/linux/hid.h     | 14 ++++++++------
 2 files changed, 39 insertions(+), 8 deletions(-)

-- 
2.14.3

[PATCH v2 1/2] HID: use BIT macro instead of plain integers for flags

[Benjamin Tissoires]

This can lead to some hairy situation with the developer losing
a day or two realizing that 4 should be after 2, not 3.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

new in v2

 include/linux/hid.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/hid.h b/include/linux/hid.h
index a62ee4a609ac..421b62b77c69 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -494,13 +494,13 @@ struct hid_output_fifo {
 	char *raw_report;
 };
 
-#define HID_CLAIMED_INPUT	1
-#define HID_CLAIMED_HIDDEV	2
-#define HID_CLAIMED_HIDRAW	4
-#define HID_CLAIMED_DRIVER	8
+#define HID_CLAIMED_INPUT	BIT(0)
+#define HID_CLAIMED_HIDDEV	BIT(1)
+#define HID_CLAIMED_HIDRAW	BIT(2)
+#define HID_CLAIMED_DRIVER	BIT(3)
 
-#define HID_STAT_ADDED		1
-#define HID_STAT_PARSED		2
+#define HID_STAT_ADDED		BIT(0)
+#define HID_STAT_PARSED		BIT(1)
 
 struct hid_input {
 	struct list_head list;
-- 
2.14.3

[PATCH v2 2/2] HID: input: do not increment usages when a duplicate is found

[Benjamin Tissoires]

This is something that bothered us from a long time. When hid-input
doesn't know how to map a usage, it uses *_MISC. But there is something
else which increments the usage if the evdev code is already used.

This leads to few issues:
- some devices may have their ABS_X mapped to ABS_Y if they export a bad
  set of usages (see the DragonRise joysticks IIRC -> fixed in a specific
  HID driver)
- *_MISC + N might (will) conflict with other defined axes (my Logitech
  H800 exports some multitouch axes because of that)
- this prevents to freely add some new evdev usages, because "hey, my
  headset will now report ABS_COFFEE, and it's not coffee capable".

So let's try to kill this nonsense, and hope we won't break too many
devices.

I my headset case, the ABS_MISC axes are created because of some
proprietary usages, so we might not break that many devices.

For backward compatibility, a quirk HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE
is created and can be applied to any device that needs this behavior.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

changes in v2:
- fixed a bug where the flag was not used properly and prevented to
  remove devices
- downgrade the error message from info to debug, given that when
  hid-generic binds first, it will output such kernel log for every
  multitouch device.

 drivers/hid/hid-input.c | 33 +++++++++++++++++++++++++++++++--
 include/linux/hid.h     |  2 ++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 04d01b57d94c..31bbeb7019bd 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1100,8 +1100,31 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 
 	set_bit(usage->type, input->evbit);
 
-	while (usage->code <= max && test_and_set_bit(usage->code, bit))
-		usage->code = find_next_zero_bit(bit, max + 1, usage->code);
+	/*
+	 * This part is *really* controversial:
+	 * - HID aims at being generic so we should do our best to export
+	 *   all incoming events
+	 * - HID describes what events are, so there is no reason for ABS_X
+	 *   to be mapped to ABS_Y
+	 * - HID is using *_MISC+N as a default value, but nothing prevents
+	 *   *_MISC+N to overwrite a legitimate even, which confuses userspace
+	 *   (for instance ABS_MISC + 7 is ABS_MT_SLOT, which has a different
+	 *   processing)
+	 *
+	 * If devices still want to use this (at their own risk), they will
+	 * have to use the quirk HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE, but
+	 * the default should be a reliable mapping.
+	 */
+	while (usage->code <= max && test_and_set_bit(usage->code, bit)) {
+		if (device->quirks & HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE) {
+			usage->code = find_next_zero_bit(bit,
+							 max + 1,
+							 usage->code);
+		} else {
+			device->status |= HID_STAT_DUP_DETECTED;
+			goto ignore;
+		}
+	}
 
 	if (usage->code > max)
 		goto ignore;
@@ -1610,6 +1633,8 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 	INIT_LIST_HEAD(&hid->inputs);
 	INIT_WORK(&hid->led_work, hidinput_led_worker);
 
+	hid->status &= ~HID_STAT_DUP_DETECTED;
+
 	if (!force) {
 		for (i = 0; i < hid->maxcollection; i++) {
 			struct hid_collection *col = &hid->collection[i];
@@ -1676,6 +1701,10 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 		goto out_unwind;
 	}
 
+	if (hid->status & HID_STAT_DUP_DETECTED)
+		hid_dbg(hid,
+			"Some usages could not be mapped, please use HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE if this is legitimate.\n");
+
 	return 0;
 
 out_unwind:
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 421b62b77c69..de3a8700ccf1 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -344,6 +344,7 @@ struct hid_item {
 #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		0x00020000
 #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	0x00040000
 #define HID_QUIRK_HAVE_SPECIAL_DRIVER		0x00080000
+#define HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE	0x00100000
 #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
 #define HID_QUIRK_NO_INIT_REPORTS		0x20000000
 #define HID_QUIRK_NO_IGNORE			0x40000000
@@ -501,6 +502,7 @@ struct hid_output_fifo {
 
 #define HID_STAT_ADDED		BIT(0)
 #define HID_STAT_PARSED		BIT(1)
+#define HID_STAT_DUP_DETECTED	BIT(2)
 
 struct hid_input {
 	struct list_head list;
-- 
2.14.3

Re: [PATCH v2 1/2] HID: use BIT macro instead of plain integers for flags

[Dmitry Torokhov]

On Fri, Dec 08, 2017 at 03:28:17PM +0100, Benjamin Tissoires wrote:
> This can lead to some hairy situation with the developer losing
> a day or two realizing that 4 should be after 2, not 3.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---

Please add

#include <linux/bitops.h>

to make sure we have definition if BIT(), otherwise

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks!

> 
> new in v2
> 
>  include/linux/hid.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index a62ee4a609ac..421b62b77c69 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -494,13 +494,13 @@ struct hid_output_fifo {
>  	char *raw_report;
>  };
>  
> -#define HID_CLAIMED_INPUT	1
> -#define HID_CLAIMED_HIDDEV	2
> -#define HID_CLAIMED_HIDRAW	4
> -#define HID_CLAIMED_DRIVER	8
> +#define HID_CLAIMED_INPUT	BIT(0)
> +#define HID_CLAIMED_HIDDEV	BIT(1)
> +#define HID_CLAIMED_HIDRAW	BIT(2)
> +#define HID_CLAIMED_DRIVER	BIT(3)
>  
> -#define HID_STAT_ADDED		1
> -#define HID_STAT_PARSED		2
> +#define HID_STAT_ADDED		BIT(0)
> +#define HID_STAT_PARSED		BIT(1)
>  
>  struct hid_input {
>  	struct list_head list;
> -- 
> 2.14.3
> 

-- 
Dmitry

Re: [PATCH v2 2/2] HID: input: do not increment usages when a duplicate is found

[Dmitry Torokhov]

On Fri, Dec 08, 2017 at 03:28:18PM +0100, Benjamin Tissoires wrote:
> This is something that bothered us from a long time. When hid-input
> doesn't know how to map a usage, it uses *_MISC. But there is something
> else which increments the usage if the evdev code is already used.
> 
> This leads to few issues:
> - some devices may have their ABS_X mapped to ABS_Y if they export a bad
>   set of usages (see the DragonRise joysticks IIRC -> fixed in a specific
>   HID driver)
> - *_MISC + N might (will) conflict with other defined axes (my Logitech
>   H800 exports some multitouch axes because of that)
> - this prevents to freely add some new evdev usages, because "hey, my
>   headset will now report ABS_COFFEE, and it's not coffee capable".
> 
> So let's try to kill this nonsense, and hope we won't break too many
> devices.
> 
> I my headset case, the ABS_MISC axes are created because of some
> proprietary usages, so we might not break that many devices.
> 
> For backward compatibility, a quirk HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE
> is created and can be applied to any device that needs this behavior.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> 
> changes in v2:
> - fixed a bug where the flag was not used properly and prevented to
>   remove devices
> - downgrade the error message from info to debug, given that when
>   hid-generic binds first, it will output such kernel log for every
>   multitouch device.
> 
>  drivers/hid/hid-input.c | 33 +++++++++++++++++++++++++++++++--
>  include/linux/hid.h     |  2 ++
>  2 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 04d01b57d94c..31bbeb7019bd 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1100,8 +1100,31 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>  
>  	set_bit(usage->type, input->evbit);
>  
> -	while (usage->code <= max && test_and_set_bit(usage->code, bit))
> -		usage->code = find_next_zero_bit(bit, max + 1, usage->code);
> +	/*
> +	 * This part is *really* controversial:
> +	 * - HID aims at being generic so we should do our best to export
> +	 *   all incoming events
> +	 * - HID describes what events are, so there is no reason for ABS_X
> +	 *   to be mapped to ABS_Y
> +	 * - HID is using *_MISC+N as a default value, but nothing prevents
> +	 *   *_MISC+N to overwrite a legitimate even, which confuses userspace
> +	 *   (for instance ABS_MISC + 7 is ABS_MT_SLOT, which has a different
> +	 *   processing)
> +	 *
> +	 * If devices still want to use this (at their own risk), they will
> +	 * have to use the quirk HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE, but
> +	 * the default should be a reliable mapping.
> +	 */
> +	while (usage->code <= max && test_and_set_bit(usage->code, bit)) {
> +		if (device->quirks & HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE) {
> +			usage->code = find_next_zero_bit(bit,
> +							 max + 1,
> +							 usage->code);
> +		} else {
> +			device->status |= HID_STAT_DUP_DETECTED;
> +			goto ignore;
> +		}
> +	}
>  
>  	if (usage->code > max)
>  		goto ignore;
> @@ -1610,6 +1633,8 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  	INIT_LIST_HEAD(&hid->inputs);
>  	INIT_WORK(&hid->led_work, hidinput_led_worker);
>  
> +	hid->status &= ~HID_STAT_DUP_DETECTED;
> +
>  	if (!force) {
>  		for (i = 0; i < hid->maxcollection; i++) {
>  			struct hid_collection *col = &hid->collection[i];
> @@ -1676,6 +1701,10 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  		goto out_unwind;
>  	}
>  
> +	if (hid->status & HID_STAT_DUP_DETECTED)
> +		hid_dbg(hid,
> +			"Some usages could not be mapped, please use HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE if this is legitimate.\n");
> +
>  	return 0;
>  
>  out_unwind:
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 421b62b77c69..de3a8700ccf1 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -344,6 +344,7 @@ struct hid_item {
>  #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		0x00020000
>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	0x00040000
>  #define HID_QUIRK_HAVE_SPECIAL_DRIVER		0x00080000
> +#define HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE	0x00100000
>  #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
>  #define HID_QUIRK_NO_INIT_REPORTS		0x20000000
>  #define HID_QUIRK_NO_IGNORE			0x40000000

Maybe these should be expressed as BIT() also?

> @@ -501,6 +502,7 @@ struct hid_output_fifo {
>  
>  #define HID_STAT_ADDED		BIT(0)
>  #define HID_STAT_PARSED		BIT(1)
> +#define HID_STAT_DUP_DETECTED	BIT(2)
>  
>  struct hid_input {
>  	struct list_head list;
> -- 
> 2.14.3
> 

-- 
Dmitry

Re: [PATCH v2 2/2] HID: input: do not increment usages when a duplicate is found

[Peter Hutterer]

On Fri, Dec 08, 2017 at 03:28:18PM +0100, Benjamin Tissoires wrote:
> This is something that bothered us from a long time. When hid-input
> doesn't know how to map a usage, it uses *_MISC. But there is something
> else which increments the usage if the evdev code is already used.
> 
> This leads to few issues:
> - some devices may have their ABS_X mapped to ABS_Y if they export a bad
>   set of usages (see the DragonRise joysticks IIRC -> fixed in a specific
>   HID driver)
> - *_MISC + N might (will) conflict with other defined axes (my Logitech
>   H800 exports some multitouch axes because of that)
> - this prevents to freely add some new evdev usages, because "hey, my
>   headset will now report ABS_COFFEE, and it's not coffee capable".
> 
> So let's try to kill this nonsense, and hope we won't break too many
> devices.
> 
> I my headset case, the ABS_MISC axes are created because of some
> proprietary usages, so we might not break that many devices.
> 
> For backward compatibility, a quirk HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE
> is created and can be applied to any device that needs this behavior.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Acked-by: Peter Hutterer <peter.hutterer@who-t.net>

Cheers,
   Peter

> ---
> 
> changes in v2:
> - fixed a bug where the flag was not used properly and prevented to
>   remove devices
> - downgrade the error message from info to debug, given that when
>   hid-generic binds first, it will output such kernel log for every
>   multitouch device.
> 
>  drivers/hid/hid-input.c | 33 +++++++++++++++++++++++++++++++--
>  include/linux/hid.h     |  2 ++
>  2 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 04d01b57d94c..31bbeb7019bd 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1100,8 +1100,31 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>  
>  	set_bit(usage->type, input->evbit);
>  
> -	while (usage->code <= max && test_and_set_bit(usage->code, bit))
> -		usage->code = find_next_zero_bit(bit, max + 1, usage->code);
> +	/*
> +	 * This part is *really* controversial:
> +	 * - HID aims at being generic so we should do our best to export
> +	 *   all incoming events
> +	 * - HID describes what events are, so there is no reason for ABS_X
> +	 *   to be mapped to ABS_Y
> +	 * - HID is using *_MISC+N as a default value, but nothing prevents
> +	 *   *_MISC+N to overwrite a legitimate even, which confuses userspace
> +	 *   (for instance ABS_MISC + 7 is ABS_MT_SLOT, which has a different
> +	 *   processing)
> +	 *
> +	 * If devices still want to use this (at their own risk), they will
> +	 * have to use the quirk HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE, but
> +	 * the default should be a reliable mapping.
> +	 */
> +	while (usage->code <= max && test_and_set_bit(usage->code, bit)) {
> +		if (device->quirks & HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE) {
> +			usage->code = find_next_zero_bit(bit,
> +							 max + 1,
> +							 usage->code);
> +		} else {
> +			device->status |= HID_STAT_DUP_DETECTED;
> +			goto ignore;
> +		}
> +	}
>  
>  	if (usage->code > max)
>  		goto ignore;
> @@ -1610,6 +1633,8 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  	INIT_LIST_HEAD(&hid->inputs);
>  	INIT_WORK(&hid->led_work, hidinput_led_worker);
>  
> +	hid->status &= ~HID_STAT_DUP_DETECTED;
> +
>  	if (!force) {
>  		for (i = 0; i < hid->maxcollection; i++) {
>  			struct hid_collection *col = &hid->collection[i];
> @@ -1676,6 +1701,10 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>  		goto out_unwind;
>  	}
>  
> +	if (hid->status & HID_STAT_DUP_DETECTED)
> +		hid_dbg(hid,
> +			"Some usages could not be mapped, please use HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE if this is legitimate.\n");
> +
>  	return 0;
>  
>  out_unwind:
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 421b62b77c69..de3a8700ccf1 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -344,6 +344,7 @@ struct hid_item {
>  #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		0x00020000
>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	0x00040000
>  #define HID_QUIRK_HAVE_SPECIAL_DRIVER		0x00080000
> +#define HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE	0x00100000
>  #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
>  #define HID_QUIRK_NO_INIT_REPORTS		0x20000000
>  #define HID_QUIRK_NO_IGNORE			0x40000000
> @@ -501,6 +502,7 @@ struct hid_output_fifo {
>  
>  #define HID_STAT_ADDED		BIT(0)
>  #define HID_STAT_PARSED		BIT(1)
> +#define HID_STAT_DUP_DETECTED	BIT(2)
>  
>  struct hid_input {
>  	struct list_head list;
> -- 
> 2.14.3
> 

Re: [PATCH v2 2/2] HID: input: do not increment usages when a duplicate is found

[Dmitry Torokhov]

On Mon, Dec 11, 2017 at 08:29:26AM +1000, Peter Hutterer wrote:
> On Fri, Dec 08, 2017 at 03:28:18PM +0100, Benjamin Tissoires wrote:
> > This is something that bothered us from a long time. When hid-input
> > doesn't know how to map a usage, it uses *_MISC. But there is something
> > else which increments the usage if the evdev code is already used.
> > 
> > This leads to few issues:
> > - some devices may have their ABS_X mapped to ABS_Y if they export a bad
> >   set of usages (see the DragonRise joysticks IIRC -> fixed in a specific
> >   HID driver)
> > - *_MISC + N might (will) conflict with other defined axes (my Logitech
> >   H800 exports some multitouch axes because of that)
> > - this prevents to freely add some new evdev usages, because "hey, my
> >   headset will now report ABS_COFFEE, and it's not coffee capable".
> > 
> > So let's try to kill this nonsense, and hope we won't break too many
> > devices.
> > 
> > I my headset case, the ABS_MISC axes are created because of some
> > proprietary usages, so we might not break that many devices.
> > 
> > For backward compatibility, a quirk HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE
> > is created and can be applied to any device that needs this behavior.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> Acked-by: Peter Hutterer <peter.hutterer@who-t.net>

So what is happening with this series? I think we should get it them
in; there is really no reason for bumping ABS_MISC till it gets into
ABS_MT_* range on some devices that are out there.

FWIW

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/2] HID: input: do not increment usages when a duplicate is found

[Jiri Kosina]

On Mon, 16 Apr 2018, Dmitry Torokhov wrote:

> So what is happening with this series? I think we should get it them
> in; there is really no reason for bumping ABS_MISC till it gets into
> ABS_MT_* range on some devices that are out there.
> 
> FWIW
> 
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Sorry, I somehow lost this one. Now queued for 4.18.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 2/2] HID: input: do not increment usages when a duplicate is found

[Benjamin Tissoires]

On Tue, Apr 17, 2018 at 2:11 PM, Jiri Kosina <jikos@kernel.org> wrote:
> On Mon, 16 Apr 2018, Dmitry Torokhov wrote:
>
>> So what is happening with this series? I think we should get it them
>> in; there is really no reason for bumping ABS_MISC till it gets into
>> ABS_MT_* range on some devices that are out there.
>>
>> FWIW
>>
>> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Sorry, I somehow lost this one. Now queued for 4.18.

Thanks Jiri, and thanks Dmitry for the reminder.

Jiri, BTW, I have 2 patch locally that should mitigate the negative
impact from this patch if there is any,

While trying to merging hid-multitouch into hid-core, I came to
realize that hid-generic should probably split the HID collections by
applications. If a device has a joystick and a mouse collection, there
is no point merging these two collections together. This is the most
common use case for reusing the same ABS_X value in one device.

I'll try to post the patches today, though I have some internal deadline :(

Cheers,
Benjamin

Re: [PATCH v2 2/2] HID: input: do not increment usages when a duplicate is found

[Benjamin Tissoires]

FYI, these are the two patches I mentioned earlier.

checkpatch.pl still complains about them so do not merge them right away, but
this should give you a better idea.
Also, this is the tip of my local tree, so there is a high chance it doesn't
apply cleanly on your for-next branch.

Cheers,
Benjamin

Benjamin Tissoires (2):
  HID: generic: create one input report per application type
  HID: input: append a suffix matching the application

 drivers/hid/hid-core.c       | 10 ++++--
 drivers/hid/hid-generic.c    | 15 ++++++++
 drivers/hid/hid-gfrm.c       |  2 +-
 drivers/hid/hid-input.c      | 84 +++++++++++++++++++++++++++++++++++++++-----
 drivers/hid/hid-magicmouse.c |  6 ++--
 drivers/hid/hid-multitouch.c | 34 ++++++------------
 include/linux/hid.h          |  6 +++-
 7 files changed, 117 insertions(+), 40 deletions(-)

-- 
2.14.3

[PATCH 1/2] HID: generic: create one input report per application type

[Benjamin Tissoires]

It is not a good idea to try to fit all types of applications in the
same input report. There are a lot of devices that are needing
the quirk HID_MULTI_INPUT but this quirk doesn't match the actual HID
description as it is based on the report ID.

Given that most devices with MULTI_INPUT I can think of split nicely
the devices inputs into application, it is a good thing to split the
devices by default based on this assumption.

Also make hid-multitouch following this rule, to not have to deal
with too many input created

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c       | 10 +++++++---
 drivers/hid/hid-generic.c    | 15 +++++++++++++++
 drivers/hid/hid-gfrm.c       |  2 +-
 drivers/hid/hid-input.c      | 20 +++++++++++++++++++-
 drivers/hid/hid-magicmouse.c |  6 +++---
 drivers/hid/hid-multitouch.c |  4 ++--
 include/linux/hid.h          |  5 ++++-
 7 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 5d7cc6bbbac6..ac4799abcc4d 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -57,7 +57,8 @@ MODULE_PARM_DESC(ignore_special_drivers, "Ignore any special drivers and handle
  * Register a new report for a device.
  */
 
-struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id)
+struct hid_report *hid_register_report(struct hid_device *device, unsigned type,
+				       unsigned id, unsigned application)
 {
 	struct hid_report_enum *report_enum = device->report_enum + type;
 	struct hid_report *report;
@@ -78,6 +79,7 @@ struct hid_report *hid_register_report(struct hid_device *device, unsigned type,
 	report->type = type;
 	report->size = 0;
 	report->device = device;
+	report->application = application;
 	report_enum->report_id_hash[id] = report;
 
 	list_add_tail(&report->list, &report_enum->report_list);
@@ -224,8 +226,10 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
 	unsigned usages;
 	unsigned offset;
 	unsigned i;
+	unsigned application = hid_lookup_collection(parser, HID_COLLECTION_APPLICATION);
 
-	report = hid_register_report(parser->device, report_type, parser->global.report_id);
+	report = hid_register_report(parser->device, report_type,
+				     parser->global.report_id, application);
 	if (!report) {
 		hid_err(parser->device, "hid_register_report failed\n");
 		return -1;
@@ -259,7 +263,7 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
 
 	field->physical = hid_lookup_collection(parser, HID_COLLECTION_PHYSICAL);
 	field->logical = hid_lookup_collection(parser, HID_COLLECTION_LOGICAL);
-	field->application = hid_lookup_collection(parser, HID_COLLECTION_APPLICATION);
+	field->application = application;
 
 	for (i = 0; i < usages; i++) {
 		unsigned j = i;
diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
index c25b4718de44..3b6eccbc2519 100644
--- a/drivers/hid/hid-generic.c
+++ b/drivers/hid/hid-generic.c
@@ -56,6 +56,20 @@ static bool hid_generic_match(struct hid_device *hdev,
 	return true;
 }
 
+static int hid_generic_probe(struct hid_device *hdev,
+			     const struct hid_device_id *id)
+{
+	int ret;
+
+	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
+
+	ret = hid_parse(hdev);
+	if (ret)
+		return ret;
+
+	return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+}
+
 static const struct hid_device_id hid_table[] = {
 	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID) },
 	{ }
@@ -66,6 +80,7 @@ static struct hid_driver hid_generic = {
 	.name = "hid-generic",
 	.id_table = hid_table,
 	.match = hid_generic_match,
+	.probe = hid_generic_probe,
 };
 module_hid_driver(hid_generic);
 
diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
index 075b1c020846..cf477f8c8f4c 100644
--- a/drivers/hid/hid-gfrm.c
+++ b/drivers/hid/hid-gfrm.c
@@ -116,7 +116,7 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		 * those reports reach gfrm_raw_event() from hid_input_report().
 		 */
 		if (!hid_register_report(hdev, HID_INPUT_REPORT,
-					 GFRM100_SEARCH_KEY_REPORT_ID)) {
+					 GFRM100_SEARCH_KEY_REPORT_ID, 0)) {
 			ret = -ENOMEM;
 			goto done;
 		}
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 04056773102e..0803d5adefa7 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1607,6 +1607,20 @@ static struct hid_input *hidinput_match(struct hid_report *report)
 	return NULL;
 }
 
+static struct hid_input *hidinput_match_application(struct hid_report *report)
+{
+	struct hid_device *hid = report->device;
+	struct hid_input *hidinput;
+
+	list_for_each_entry(hidinput, &hid->inputs, list) {
+		if (hidinput->report &&
+		    hidinput->report->application == report->application)
+			return hidinput;
+	}
+
+	return NULL;
+}
+
 static inline void hidinput_configure_usages(struct hid_input *hidinput,
 					     struct hid_report *report)
 {
@@ -1667,6 +1681,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 			 */
 			if (hid->quirks & HID_QUIRK_MULTI_INPUT)
 				hidinput = hidinput_match(report);
+			else if (hid->maxapplication > 1 &&
+				 (hid->quirks & HID_QUIRK_INPUT_PER_APP))
+				hidinput = hidinput_match_application(report);
 
 			if (!hidinput) {
 				hidinput = hidinput_allocate(hid);
@@ -1676,7 +1693,8 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 
 			hidinput_configure_usages(hidinput, report);
 
-			if (hid->quirks & HID_QUIRK_MULTI_INPUT)
+			if (hid->quirks &
+			    (HID_QUIRK_MULTI_INPUT | HID_QUIRK_INPUT_PER_APP))
 				hidinput->report = report;
 		}
 	}
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 42ed887ba0be..b454c4386157 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -531,12 +531,12 @@ static int magicmouse_probe(struct hid_device *hdev,
 
 	if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE)
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
-			MOUSE_REPORT_ID);
+			MOUSE_REPORT_ID, 0);
 	else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
-			TRACKPAD_REPORT_ID);
+			TRACKPAD_REPORT_ID, 0);
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
-			DOUBLE_REPORT_ID);
+			DOUBLE_REPORT_ID, 0);
 	}
 
 	if (!report) {
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 8551766b75fa..2a0caf2e24ce 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1458,10 +1458,10 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	/*
 	 * This allows the driver to handle different input sensors
-	 * that emits events through different reports on the same HID
+	 * that emits events through different applications on the same HID
 	 * device.
 	 */
-	hdev->quirks |= HID_QUIRK_MULTI_INPUT;
+	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
 
 	timer_setup(&td->release_timer, mt_expired_timeout, 0);
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 0267aa5c1ea3..8d52cefedfe5 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -341,6 +341,7 @@ struct hid_item {
 /* BIT(8) reserved for backward compatibility, was HID_QUIRK_NO_EMPTY_INPUT */
 /* BIT(9) reserved for backward compatibility, was NO_INIT_INPUT_REPORTS */
 #define HID_QUIRK_ALWAYS_POLL			BIT(10)
+#define HID_QUIRK_INPUT_PER_APP			BIT(11)
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		BIT(16)
 #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		BIT(17)
 #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	BIT(18)
@@ -466,6 +467,7 @@ struct hid_report {
 	struct list_head list;
 	unsigned id;					/* id of this report */
 	unsigned type;					/* report type */
+	unsigned application;				/* application usage for this report */
 	struct hid_field *field[HID_MAX_FIELDS];	/* fields of the report */
 	unsigned maxfield;				/* maximum valid field index */
 	unsigned size;					/* size of the report (bits) */
@@ -859,7 +861,8 @@ void hid_output_report(struct hid_report *report, __u8 *data);
 void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
 u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
 struct hid_device *hid_allocate_device(void);
-struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
+struct hid_report *hid_register_report(struct hid_device *device, unsigned type,
+				       unsigned id, unsigned application);
 int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size);
 struct hid_report *hid_validate_values(struct hid_device *hid,
 				       unsigned int type, unsigned int id,
-- 
2.14.3

[PATCH 2/2] HID: input: append a suffix matching the application

[Benjamin Tissoires]

Given that we create one input node per application, we should name
the input node accordingly to not lose userspace.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c      | 64 ++++++++++++++++++++++++++++++++++++++------
 drivers/hid/hid-multitouch.c | 30 +++++++--------------
 include/linux/hid.h          |  1 +
 3 files changed, 66 insertions(+), 29 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 0803d5adefa7..886d81df50d4 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1500,15 +1500,56 @@ static void report_features(struct hid_device *hid)
 		}
 }
 
-static struct hid_input *hidinput_allocate(struct hid_device *hid)
+static struct hid_input *hidinput_allocate(struct hid_device *hid,
+					   unsigned application)
 {
 	struct hid_input *hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
 	struct input_dev *input_dev = input_allocate_device();
-	if (!hidinput || !input_dev) {
-		kfree(hidinput);
-		input_free_device(input_dev);
-		hid_err(hid, "Out of memory during hid input probe\n");
-		return NULL;
+	const char *suffix = NULL;
+
+	if (!hidinput || !input_dev)
+		goto fail;
+
+	if ((hid->quirks & HID_QUIRK_INPUT_PER_APP) &&
+	    hid->maxapplication > 1) {
+		switch (application) {
+		case HID_GD_KEYBOARD:
+			suffix = "Keyboard";
+			break;
+		case HID_GD_KEYPAD:
+			suffix = "Keypad";
+			break;
+		case HID_GD_MOUSE:
+			suffix = "Mouse";
+			break;
+		case HID_DG_STYLUS:
+			suffix = "Pen";
+			break;
+		case HID_DG_TOUCHSCREEN:
+			suffix = "Touchscreen";
+			break;
+		case HID_DG_TOUCHPAD:
+			suffix = "Touchpad";
+			break;
+		case HID_GD_SYSTEM_CONTROL:
+			suffix = "System Control";
+			break;
+		case HID_CP_CONSUMER_CONTROL:
+			suffix = "Consumer Control";
+			break;
+		case HID_GD_WIRELESS_RADIO_CTLS:
+			suffix = "Wireless Radio Control";
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (suffix) {
+		hidinput->name = kasprintf(GFP_KERNEL, "%s %s",
+					   hid->name, suffix);
+		if (!hidinput->name)
+			goto fail;
 	}
 
 	input_set_drvdata(input_dev, hid);
@@ -1518,7 +1559,7 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	input_dev->setkeycode = hidinput_setkeycode;
 	input_dev->getkeycode = hidinput_getkeycode;
 
-	input_dev->name = hid->name;
+	input_dev->name = hidinput->name ? hidinput->name : hid->name;
 	input_dev->phys = hid->phys;
 	input_dev->uniq = hid->uniq;
 	input_dev->id.bustype = hid->bus;
@@ -1530,6 +1571,12 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	list_add_tail(&hidinput->list, &hid->inputs);
 
 	return hidinput;
+
+fail:
+	kfree(hidinput);
+	input_free_device(input_dev);
+	hid_err(hid, "Out of memory during hid input probe\n");
+	return NULL;
 }
 
 static bool hidinput_has_been_populated(struct hid_input *hidinput)
@@ -1575,6 +1622,7 @@ static void hidinput_cleanup_hidinput(struct hid_device *hid,
 
 	list_del(&hidinput->list);
 	input_free_device(hidinput->input);
+	kfree(hidinput->name);
 
 	for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) {
 		if (k == HID_OUTPUT_REPORT &&
@@ -1686,7 +1734,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 				hidinput = hidinput_match_application(report);
 
 			if (!hidinput) {
-				hidinput = hidinput_allocate(hid);
+				hidinput = hidinput_allocate(hid, report->application);
 				if (!hidinput)
 					goto out_unwind;
 			}
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 2a0caf2e24ce..cfbaedc55e02 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1294,33 +1294,21 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 	} else {
 		switch (field->application) {
 		case HID_GD_KEYBOARD:
-			suffix = "Keyboard";
-			break;
 		case HID_GD_KEYPAD:
-			suffix = "Keypad";
-			break;
 		case HID_GD_MOUSE:
-			suffix = "Mouse";
-			break;
-		case HID_DG_STYLUS:
-			suffix = "Pen";
-			/* force BTN_STYLUS to allow tablet matching in udev */
-			__set_bit(BTN_STYLUS, hi->input->keybit);
-			break;
-		case HID_DG_TOUCHSCREEN:
-			/* we do not set suffix = "Touchscreen" */
-			break;
 		case HID_DG_TOUCHPAD:
-			suffix = "Touchpad";
-			break;
 		case HID_GD_SYSTEM_CONTROL:
-			suffix = "System Control";
-			break;
 		case HID_CP_CONSUMER_CONTROL:
-			suffix = "Consumer Control";
-			break;
 		case HID_GD_WIRELESS_RADIO_CTLS:
-			suffix = "Wireless Radio Control";
+			/* already handled by hid core */
+			break;
+		case HID_DG_TOUCHSCREEN:
+			/* we do not set suffix = "Touchscreen" */
+			hi->input->name = hdev->name;
+			break;
+		case HID_DG_STYLUS:
+			/* force BTN_STYLUS to allow tablet matching in udev */
+			__set_bit(BTN_STYLUS, hi->input->keybit);
 			break;
 		case HID_VD_ASUS_CUSTOM_MEDIA_KEYS:
 			suffix = "Custom Media Keys";
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 8d52cefedfe5..1d1387773a7c 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -511,6 +511,7 @@ struct hid_input {
 	struct list_head list;
 	struct hid_report *report;
 	struct input_dev *input;
+	const char *name;
 	bool registered;
 };
 
-- 
2.14.3