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

2018-04-17 Thread 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



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

2018-04-17 Thread 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



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

2018-04-17 Thread Benjamin Tissoires
On Tue, Apr 17, 2018 at 2:11 PM, Jiri Kosina  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 
>
> 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

2018-04-17 Thread Benjamin Tissoires
On Tue, Apr 17, 2018 at 2:11 PM, Jiri Kosina  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 
>
> 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

2018-04-17 Thread 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 

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

2018-04-17 Thread 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 

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

2018-04-16 Thread 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 
> 
> Acked-by: Peter Hutterer 

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 

Thanks.

-- 
Dmitry


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

2018-04-16 Thread 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 
> 
> Acked-by: Peter Hutterer 

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 

Thanks.

-- 
Dmitry


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

2017-12-10 Thread 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 

Acked-by: Peter Hutterer 

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(>inputs);
>   INIT_WORK(>led_work, hidinput_led_worker);
>  
> + hid->status &= ~HID_STAT_DUP_DETECTED;
> +
>   if (!force) {
>   for (i = 0; i < hid->maxcollection; i++) {
>   struct hid_collection *col = >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  0x0002
>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP   0x0004
>  #define HID_QUIRK_HAVE_SPECIAL_DRIVER0x0008
> +#define HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE   0x0010
>  #define HID_QUIRK_FULLSPEED_INTERVAL 0x1000
>  #define HID_QUIRK_NO_INIT_REPORTS0x2000
>  #define HID_QUIRK_NO_IGNORE  0x4000
> @@ -501,6 +502,7 @@ struct hid_output_fifo {
>  
>  #define 

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

2017-12-10 Thread 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 

Acked-by: Peter Hutterer 

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(>inputs);
>   INIT_WORK(>led_work, hidinput_led_worker);
>  
> + hid->status &= ~HID_STAT_DUP_DETECTED;
> +
>   if (!force) {
>   for (i = 0; i < hid->maxcollection; i++) {
>   struct hid_collection *col = >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  0x0002
>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP   0x0004
>  #define HID_QUIRK_HAVE_SPECIAL_DRIVER0x0008
> +#define HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE   0x0010
>  #define HID_QUIRK_FULLSPEED_INTERVAL 0x1000
>  #define HID_QUIRK_NO_INIT_REPORTS0x2000
>  #define HID_QUIRK_NO_IGNORE  0x4000
> @@ -501,6 +502,7 @@ struct hid_output_fifo {
>  
>  #define HID_STAT_ADDED   BIT(0)
>  #define 

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

2017-12-08 Thread 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 
> ---
> 
> 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(>inputs);
>   INIT_WORK(>led_work, hidinput_led_worker);
>  
> + hid->status &= ~HID_STAT_DUP_DETECTED;
> +
>   if (!force) {
>   for (i = 0; i < hid->maxcollection; i++) {
>   struct hid_collection *col = >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  0x0002
>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP   0x0004
>  #define HID_QUIRK_HAVE_SPECIAL_DRIVER0x0008
> +#define HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE   0x0010
>  #define HID_QUIRK_FULLSPEED_INTERVAL 0x1000
>  #define HID_QUIRK_NO_INIT_REPORTS0x2000
>  #define HID_QUIRK_NO_IGNORE  0x4000

Maybe these should be expressed as BIT() also?

> @@ -501,6 +502,7 @@ struct hid_output_fifo {
>  
>  #define HID_STAT_ADDED  

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

2017-12-08 Thread 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 
> ---
> 
> 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(>inputs);
>   INIT_WORK(>led_work, hidinput_led_worker);
>  
> + hid->status &= ~HID_STAT_DUP_DETECTED;
> +
>   if (!force) {
>   for (i = 0; i < hid->maxcollection; i++) {
>   struct hid_collection *col = >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  0x0002
>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP   0x0004
>  #define HID_QUIRK_HAVE_SPECIAL_DRIVER0x0008
> +#define HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE   0x0010
>  #define HID_QUIRK_FULLSPEED_INTERVAL 0x1000
>  #define HID_QUIRK_NO_INIT_REPORTS0x2000
>  #define HID_QUIRK_NO_IGNORE  0x4000

Maybe these should be expressed as BIT() also?

> @@ -501,6 +502,7 @@ struct hid_output_fifo {
>  
>  #define HID_STAT_ADDED   BIT(0)
>  #define