BUG: hid-multitouch causes 10 second delay and error

BUG: hid-multitouch causes 10 second delay and error

[Sean Young]

Since this commit:

	commit 0db3bfc72adf0cb70f08dfe92e4040f64e25e205

	HID: multitouch: decide if hid-multitouch needs to handle mt devices
    
I get the following when I insert a smartjoy device (hid-sjoy driver):

[ 3727.405037] usb 7-1: new low speed USB device number 2 using uhci_hcd
[ 3727.709082] usb 7-1: New USB device found, idVendor=6666, idProduct=8802
[ 3727.709087] usb 7-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 3727.709092] usb 7-1: Product: TigerGame PS/PS2 Game Controller Adapter
[ 3727.709095] usb 7-1: Manufacturer: WiseGroup.,Ltd
[ 3738.002095] hid-multitouch 0003:6666:8802.0005: timeout initializing reports
[ 3738.007861] input: WiseGroup.,Ltd TigerGame PS/PS2 Game Controller Adapter as /devices/pci0000:00/0000:00:1d.1/usb7/7-1/7-1:1.0/input/input17
[ 3738.008137] smartjoyplus 0003:6666:8802.0005: input,hidraw4: USB HID v1.00 Joystick [WiseGroup.,Ltd TigerGame PS/PS2 Game Controller Adapter] on usb-0000:00:1d.1-1/input0
[ 3738.008163] smartjoyplus 0003:6666:8802.0005: Force feedback for SmartJoy PLUS PS2/USB adapter

Note the 10 second delay caused by the hid-multitouch error.

If I understand it correctly, the problem is that hid-multitouch now has 
a catch-all usb-id field, and does a usbhid_init_reports() on the device 
without the quirk HID_QUIRK_NOGET. 

The HID_QUIRK_NOGET for this device is listed in the hid-sjoy.c driver itself 
rather than in hid-quirks.c; presumably the latter is handled correctly.

Is there a different way of handling this rather than hid-multitouch 
messing with every usb device which identifies itself as hid? Alternatively,
should all quirks for all devices be specified in hid-quirks.c and not in
individual drivers?


Sean

Re: BUG: hid-multitouch causes 10 second delay and error

[Benjamin Tissoires]

Hi Sean,


On Wed, Oct 26, 2011 at 23:37, Sean Young <sean@mess.org> wrote:
> Since this commit:
>
>        commit 0db3bfc72adf0cb70f08dfe92e4040f64e25e205
>
>        HID: multitouch: decide if hid-multitouch needs to handle mt devices
>
> I get the following when I insert a smartjoy device (hid-sjoy driver):
>
> [ 3727.405037] usb 7-1: new low speed USB device number 2 using uhci_hcd
> [ 3727.709082] usb 7-1: New USB device found, idVendor=6666, idProduct=8802
> [ 3727.709087] usb 7-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> [ 3727.709092] usb 7-1: Product: TigerGame PS/PS2 Game Controller Adapter
> [ 3727.709095] usb 7-1: Manufacturer: WiseGroup.,Ltd
> [ 3738.002095] hid-multitouch 0003:6666:8802.0005: timeout initializing reports
> [ 3738.007861] input: WiseGroup.,Ltd TigerGame PS/PS2 Game Controller Adapter as /devices/pci0000:00/0000:00:1d.1/usb7/7-1/7-1:1.0/input/input17
> [ 3738.008137] smartjoyplus 0003:6666:8802.0005: input,hidraw4: USB HID v1.00 Joystick [WiseGroup.,Ltd TigerGame PS/PS2 Game Controller Adapter] on usb-0000:00:1d.1-1/input0
> [ 3738.008163] smartjoyplus 0003:6666:8802.0005: Force feedback for SmartJoy PLUS PS2/USB adapter
>
> Note the 10 second delay caused by the hid-multitouch error.

Thanks for this bug report. It's great that this bug that concerns
only 9 devices has been reported.
To sum up, I'll have to redo my patch.

>
> If I understand it correctly, the problem is that hid-multitouch now has
> a catch-all usb-id field, and does a usbhid_init_reports() on the device
> without the quirk HID_QUIRK_NOGET.

yep

>
> The HID_QUIRK_NOGET for this device is listed in the hid-sjoy.c driver itself
> rather than in hid-quirks.c; presumably the latter is handled correctly.

It should work, but this patch was a little bit too invasive.

>
> Is there a different way of handling this rather than hid-multitouch
> messing with every usb device which identifies itself as hid? Alternatively,
> should all quirks for all devices be specified in hid-quirks.c and not in
> individual drivers?
>
>

I'm working on a better solution than this patch (I've just found it
but I need some time to format and send it...)

Cheers,
Benjamin

> Sean
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG: hid-multitouch causes 10 second delay and error

[Benjamin Tissoires]

On 10/27/2011 11:25 AM, Benjamin Tissoires wrote:
> Hi Sean,
>
>
> On Wed, Oct 26, 2011 at 23:37, Sean Young<sean@mess.org>  wrote:
>> Since this commit:
>>
>>         commit 0db3bfc72adf0cb70f08dfe92e4040f64e25e205
>>
>>         HID: multitouch: decide if hid-multitouch needs to handle mt devices
>>
>> I get the following when I insert a smartjoy device (hid-sjoy driver):
>>
>> [ 3727.405037] usb 7-1: new low speed USB device number 2 using uhci_hcd
>> [ 3727.709082] usb 7-1: New USB device found, idVendor=6666, idProduct=8802
>> [ 3727.709087] usb 7-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
>> [ 3727.709092] usb 7-1: Product: TigerGame PS/PS2 Game Controller Adapter
>> [ 3727.709095] usb 7-1: Manufacturer: WiseGroup.,Ltd
>> [ 3738.002095] hid-multitouch 0003:6666:8802.0005: timeout initializing reports
>> [ 3738.007861] input: WiseGroup.,Ltd TigerGame PS/PS2 Game Controller Adapter as /devices/pci0000:00/0000:00:1d.1/usb7/7-1/7-1:1.0/input/input17
>> [ 3738.008137] smartjoyplus 0003:6666:8802.0005: input,hidraw4: USB HID v1.00 Joystick [WiseGroup.,Ltd TigerGame PS/PS2 Game Controller Adapter] on usb-0000:00:1d.1-1/input0
>> [ 3738.008163] smartjoyplus 0003:6666:8802.0005: Force feedback for SmartJoy PLUS PS2/USB adapter
>>
>> Note the 10 second delay caused by the hid-multitouch error.
>
> Thanks for this bug report. It's great that this bug that concerns
> only 9 devices has been reported.
> To sum up, I'll have to redo my patch.
>
>>
>> If I understand it correctly, the problem is that hid-multitouch now has
>> a catch-all usb-id field, and does a usbhid_init_reports() on the device
>> without the quirk HID_QUIRK_NOGET.
>
> yep
>
>>
>> The HID_QUIRK_NOGET for this device is listed in the hid-sjoy.c driver itself
>> rather than in hid-quirks.c; presumably the latter is handled correctly.
>
> It should work, but this patch was a little bit too invasive.
>
>>
>> Is there a different way of handling this rather than hid-multitouch
>> messing with every usb device which identifies itself as hid? Alternatively,
>> should all quirks for all devices be specified in hid-quirks.c and not in
>> individual drivers?
>>
>>
>
> I'm working on a better solution than this patch (I've just found it
> but I need some time to format and send it...)
>
> Cheers,
> Benjamin
>
>> Sean
>>

Hi Sean, can you test the following patch please:


 From 488272baf9bc95718dba2b9a0f62fe3309ca578f Mon Sep 17 00:00:00 2001
From: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Date: Thu, 27 Oct 2011 13:36:05 +0200
Subject: [PATCH 1/2] hid-multitouch: fix interaction with other hid drivers

The commit 0db3bfc72adf0cb70f08dfe92e4040f64e25e205
(HID: multitouch: decide if hid-multitouch needs to handle mt devices)
was too invasive in its relationship with other drivers.
For instance, hid-sjoy specify the quirk HID_QUIRK_NOGET
and hid-multitouch ignores it, thus adding a 10 seconds wait.

This patch allows hid-multitouch to infer how the device landed here:
* if it was manually added to the supported devices of hid-multitouch
and in hid_have_special_driver in hid-core, then it has to be taken.
* if it was rejected by hid-core due to the VID/PID in
hid_have_special_driver and is not present in the manual list,
then another driver will take care of it.
* if it was rejected by hid-core due to the presence of the CONTACT_ID
hid field, then no other driver will handle it, and hid-multitouch
can safely handle it.

The mt_have_special_driver list is also obsolete now.

Reported-by: Sean Young <sean@mess.org>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
  drivers/hid/hid-core.c       |    1 -
  drivers/hid/hid-multitouch.c |   35 +++++------------------------------
  2 files changed, 5 insertions(+), 31 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 2572db9..09e3d9b 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1230,7 +1230,6 @@ int hid_connect(struct hid_device *hdev, unsigned 
int connect_mask)
  		hdev->claimed |= HID_CLAIMED_INPUT;
  	if (hdev->quirks & HID_QUIRK_MULTITOUCH) {
  		/* this device should be handled by hid-multitouch, skip it */
-		hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
  		return -ENODEV;
  	}

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index fa5d7a1..0cc3cea 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -530,33 +530,6 @@ static void mt_set_input_mode(struct hid_device *hdev)
  	}
  }

-/* a list of devices for which there is a specialized multitouch driver */
-static const struct hid_device_id mt_have_special_driver[] = {
-	{ HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, 0x0001) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, 0x0006) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_QUANTA,
-			USB_DEVICE_ID_PIXART_IMAGING_INC_OPTICAL_TOUCH_SCREEN) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_QUANTA,
-			USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH) },
-	{ }
-};
-
-static bool mt_match_one_id(struct hid_device *hdev,
-		const struct hid_device_id *id)
-{
-	return id->bus == hdev->bus &&
-		(id->vendor == HID_ANY_ID || id->vendor == hdev->vendor) &&
-		(id->product == HID_ANY_ID || id->product == hdev->product);
-}
-
-static const struct hid_device_id *mt_match_id(struct hid_device *hdev,
-		const struct hid_device_id *id)
-{
-	for (; id->bus; id++)
-		if (mt_match_one_id(hdev, id))
-			return id;
-
-	return NULL;
  }

  static int mt_probe(struct hid_device *hdev, const struct 
hid_device_id *id)
@@ -565,7 +538,10 @@ static int mt_probe(struct hid_device *hdev, const 
struct hid_device_id *id)
  	struct mt_device *td;
  	struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */

-	if (mt_match_id(hdev, mt_have_special_driver))
+	if (!id->driver_data && !(hdev->quirks & HID_QUIRK_MULTITOUCH))
+		/* cought by HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID),
+		 * and either in hid_have_special_driver
+		 * or not detected as multitouch by hid-core */
  		return -ENODEV;

  	for (i = 0; mt_classes[i].name ; i++) {
@@ -794,8 +770,7 @@ static const struct hid_device_id mt_devices[] = {
  			USB_DEVICE_ID_XAT_CSR) },

  	/* Rest of the world */
-	{ .driver_data = MT_CLS_DEFAULT,
-		HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID) },
+	{ HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID) },

  	{ }
  };
-- 
1.7.4.4

Cheers,
Benjamin

Re: BUG: hid-multitouch causes 10 second delay and error

[Sean Young]

On Thu, Oct 27, 2011 at 01:54:23PM +0200, Benjamin Tissoires wrote:
> Hi Sean, can you test the following patch please:
> 
> 
> From 488272baf9bc95718dba2b9a0f62fe3309ca578f Mon Sep 17 00:00:00 2001
> From: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> Date: Thu, 27 Oct 2011 13:36:05 +0200
> Subject: [PATCH 1/2] hid-multitouch: fix interaction with other hid drivers

-snip-

> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 2572db9..09e3d9b 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1230,7 +1230,6 @@ int hid_connect(struct hid_device *hdev,
> unsigned int connect_mask)

Your patch got mangled -- for example, the line above got wrapped. I had
to apply it manually.

I've tested it and it works fine. I do not get the error nor the delay
with this applied, which is great.

Thanks!

Sean

Re: BUG: hid-multitouch causes 10 second delay and error

[Henrik Rydberg]

Hi Benjamin,

> Hi Sean, can you test the following patch please:
> 
> 
> From 488272baf9bc95718dba2b9a0f62fe3309ca578f Mon Sep 17 00:00:00 2001
> From: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> Date: Thu, 27 Oct 2011 13:36:05 +0200
> Subject: [PATCH 1/2] hid-multitouch: fix interaction with other hid drivers

I took this patch, fixed it up, applied it to jikos/multitouch, and
did a "git diff HEAD~3" to see the actual changes applied so far for
generic hid-mt support. That diff is quite small, so I would recommend
rewinding the tree once things settle down. I have commented on the
diff below, and at the end there are three alternative (untested)
patches, as a suggestion.

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 956f849..78253ae 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1212,6 +1212,11 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
 	if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
 				connect_mask & HID_CONNECT_HIDINPUT_FORCE))
 		hdev->claimed |= HID_CLAIMED_INPUT;
+	if (hdev->quirks & HID_QUIRK_MULTITOUCH) {
+		/* this device should be handled by hid-multitouch, skip it */
+		return -ENODEV;
+	}
+
 	if ((connect_mask & HID_CONNECT_HIDDEV) && hdev->hiddev_connect &&
 			!hdev->hiddev_connect(hdev,
 				connect_mask & HID_CONNECT_HIDDEV_FORCE))

The regret here is that hid-core needs to know about hit-mt at
all. What it needs to know is whether the device should be dropped.

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 6559e2e..f333139 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -474,6 +474,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			map_key_clear(BTN_STYLUS2);
 			break;
 
+		case 0x51: /* ContactID */
+			device->quirks |= HID_QUIRK_MULTITOUCH;
+			goto unknown;
+
 		default:  goto unknown;
 		}
 		break;

So in addition to the detection here,
 
@@ -978,6 +982,13 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 		}
 	}
 
+	if (hid->quirks & HID_QUIRK_MULTITOUCH) {
+		/* generic hid does not know how to handle multitouch devices */
+		if (hidinput)
+			goto out_cleanup;
+		goto out_unwind;
+	}
+
 	if (hidinput && input_register_device(hidinput->input))
 		goto out_cleanup;

One could instead drop handling based on a quirk designed for that
purpose (HID_QUIRK_HIDINPUT_DROP).

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index f1c909f..28f8ff2 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -291,6 +291,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			td->last_slot_field = usage->hid;
 			td->last_field_index = field->index;
 			td->last_mt_collection = usage->collection_index;
+			hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
 			return 1;
 		case HID_DG_WIDTH:
 			hid_map_usage(hi, usage, bit, max,

Although correct per se, it is clearer to reset this flag in mt_probe().

@@ -535,6 +536,12 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	struct mt_device *td;
 	struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
 
+	if (!id->driver_data && !(hdev->quirks & HID_QUIRK_MULTITOUCH))
+		/* cought by HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID),
+		 * and either in hid_have_special_driver
+		 * or not detected as multitouch by hid-core */
+		return -ENODEV;
+
 	for (i = 0; mt_classes[i].name ; i++) {
 		if (id->driver_data == mt_classes[i].name) {
 			mtclass = &(mt_classes[i]);

Very neat solution indeed!

@@ -542,10 +549,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		}
 	}
 
-	/* This allows the driver to correctly support devices
-	 * that emit events over several HID messages.
-	 */
-	hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
 
 	td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
 	if (!td) {

It seems there is no longer any reason to move this line around, since
we now only come here when the device is really meant for this driver.

@@ -561,10 +564,16 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (ret != 0)
 		goto fail;
 
+	hdev->quirks |= HID_QUIRK_MULTITOUCH;

This is unnecessary and makes the logic blurred.

 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret)
 		goto fail;
 
+	/* This allows the driver to correctly support devices
+	 * that emit events over several HID messages.
+	 */
+	hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
+
 	td->slots = kzalloc(td->maxcontacts * sizeof(struct mt_slot),
 				GFP_KERNEL);
 	if (!td->slots) {

In addition to not needing to be moved, this line introduces a race
with hid-input, since the device has already started when this line is
executed.

@@ -758,6 +767,9 @@ static const struct hid_device_id mt_devices[] = {
 		HID_USB_DEVICE(USB_VENDOR_ID_XAT,
 			USB_DEVICE_ID_XAT_CSR) },
 
+	/* Rest of the world */
+	{ HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID) },
+
 	{ }
 };
 MODULE_DEVICE_TABLE(hid, mt_devices);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 9cf8e7a..6fb743d 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -312,6 +312,7 @@ struct hid_item {
 #define HID_QUIRK_BADPAD			0x00000020
 #define HID_QUIRK_MULTI_INPUT			0x00000040
 #define HID_QUIRK_HIDINPUT_FORCE		0x00000080
+#define HID_QUIRK_MULTITOUCH			0x00000100
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
 #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
 #define HID_QUIRK_NO_INIT_REPORTS		0x20000000

As an alternative, here are three untested and uncommited patches
which implements the comments above.

Cheers,
Henrik

--

>From defdac444919b99a932368ee1a8ad290dc724933 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <rydberg@euromail.se>
Date: Fri, 28 Oct 2011 11:36:36 +0200
Subject: [PATCH 1/3] hid: Allow an input device to be dropped after parsing

Some devices need a special driver based on the input mapping of the
device. This patch enables a mechanism where hidinput can set
HID_QUIRK_HIDINPUT_DROP to leave a device to be picked up by a special
driver which intercepts the input mapping.
---
 drivers/hid/hid-core.c  |    3 +++
 drivers/hid/hid-input.c |    6 ++++++
 include/linux/hid.h     |    1 +
 3 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 956f849..2628f9c 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1212,6 +1212,9 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
 	if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
 				connect_mask & HID_CONNECT_HIDINPUT_FORCE))
 		hdev->claimed |= HID_CLAIMED_INPUT;
+	if (hdev->quirks & HID_QUIRK_HIDINPUT_DROP)
+		return -ENODEV;
+
 	if ((connect_mask & HID_CONNECT_HIDDEV) && hdev->hiddev_connect &&
 			!hdev->hiddev_connect(hdev,
 				connect_mask & HID_CONNECT_HIDDEV_FORCE))
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 6559e2e..aa3ce2e 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -978,6 +978,12 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 		}
 	}
 
+	if (hid->quirks & HID_QUIRK_HIDINPUT_DROP) {
+		if (hidinput)
+			goto out_cleanup;
+		goto out_unwind;
+	}
+
 	if (hidinput && input_register_device(hidinput->input))
 		goto out_cleanup;
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 9cf8e7a..4028a27 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -312,6 +312,7 @@ struct hid_item {
 #define HID_QUIRK_BADPAD			0x00000020
 #define HID_QUIRK_MULTI_INPUT			0x00000040
 #define HID_QUIRK_HIDINPUT_FORCE		0x00000080
+#define HID_QUIRK_HIDINPUT_DROP			0x00000100
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
 #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
 #define HID_QUIRK_NO_INIT_REPORTS		0x20000000
-- 
1.7.7

>From 5f4fe6ef4cab9721c6b277f57d5374d6b549359d Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <rydberg@euromail.se>
Date: Fri, 28 Oct 2011 11:42:39 +0200
Subject: [PATCH 2/3] hid-input: Drop generic handling of hid-mt multitouch
 devices

The hid-mt devices are recognized by the ContactID field. This patch
sets HID_QUIRK_MULTITOUCH accordingly, and leaves the device to be
picked up by any driver which intercepts the ContactID field.

All in-tree hid-mt drivers intercept the ContactID, so this patch has
no other effect than to skip generic handling of hid-mt devices.
---
 drivers/hid/hid-input.c |    6 ++++++
 include/linux/hid.h     |    1 +
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index aa3ce2e..108830fc 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -474,6 +474,12 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			map_key_clear(BTN_STYLUS2);
 			break;
 
+		case 0x51: /* ContactID */
+			device->quirks |= HID_QUIRK_MULTITOUCH;
+			/* hid-mt is not handled by generic hid */
+			device->quirks |= HID_QUIRK_HIDINPUT_DROP;
+			goto unknown;
+
 		default:  goto unknown;
 		}
 		break;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 4028a27..b9c4296 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -313,6 +313,7 @@ struct hid_item {
 #define HID_QUIRK_MULTI_INPUT			0x00000040
 #define HID_QUIRK_HIDINPUT_FORCE		0x00000080
 #define HID_QUIRK_HIDINPUT_DROP			0x00000100
+#define HID_QUIRK_MULTITOUCH			0x00000200
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
 #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
 #define HID_QUIRK_NO_INIT_REPORTS		0x20000000
-- 
1.7.7

>From 6b1ecc88b3f5cca7c9178e3900c6766544db0798 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <rydberg@euromail.se>
Date: Fri, 28 Oct 2011 12:01:08 +0200
Subject: [PATCH 3/3] hid-multitouch: Pick up all hid-mt devices

This patch adds a catch-all device entry, which "undrops" all devices
that have been identified as hid-mt by the hid core.
---
 drivers/hid/hid-multitouch.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index f1c909f..1f051eb 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -535,6 +535,15 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	struct mt_device *td;
 	struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
 
+	if (!id->driver_data && !(hdev->quirks & HID_QUIRK_MULTITOUCH))
+		/* cought by HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID),
+		 * and either in hid_have_special_driver
+		 * or not detected as multitouch by hid-core */
+		return -ENODEV;
+
+	/* pick up the device that was dropped by hid-core */
+	hdev->quirks &= ~HID_QUIRK_HIDINPUT_DROP;
+
 	for (i = 0; mt_classes[i].name ; i++) {
 		if (id->driver_data == mt_classes[i].name) {
 			mtclass = &(mt_classes[i]);
@@ -758,6 +767,9 @@ static const struct hid_device_id mt_devices[] = {
 		HID_USB_DEVICE(USB_VENDOR_ID_XAT,
 			USB_DEVICE_ID_XAT_CSR) },
 
+	/* Rest of the world */
+	{ HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID) },
+
 	{ }
 };
 MODULE_DEVICE_TABLE(hid, mt_devices);
-- 
1.7.7

Re: BUG: hid-multitouch causes 10 second delay and error

[Benjamin Tissoires]

Hi Henrik,

well, most of the comments you in-lined addressed the fact that I
tried to make the smallest patch possible.

On Fri, Oct 28, 2011 at 13:16, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> Hi Sean, can you test the following patch please:
>>
>>
>> From 488272baf9bc95718dba2b9a0f62fe3309ca578f Mon Sep 17 00:00:00 2001
>> From: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> Date: Thu, 27 Oct 2011 13:36:05 +0200
>> Subject: [PATCH 1/2] hid-multitouch: fix interaction with other hid drivers
>
> I took this patch, fixed it up, applied it to jikos/multitouch, and
> did a "git diff HEAD~3" to see the actual changes applied so far for
> generic hid-mt support. That diff is quite small, so I would recommend
> rewinding the tree once things settle down. I have commented on the
> diff below, and at the end there are three alternative (untested)
> patches, as a suggestion.
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 956f849..78253ae 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1212,6 +1212,11 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
>        if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
>                                connect_mask & HID_CONNECT_HIDINPUT_FORCE))
>                hdev->claimed |= HID_CLAIMED_INPUT;
> +       if (hdev->quirks & HID_QUIRK_MULTITOUCH) {
> +               /* this device should be handled by hid-multitouch, skip it */
> +               return -ENODEV;
> +       }
> +
>        if ((connect_mask & HID_CONNECT_HIDDEV) && hdev->hiddev_connect &&
>                        !hdev->hiddev_connect(hdev,
>                                connect_mask & HID_CONNECT_HIDDEV_FORCE))
>
> The regret here is that hid-core needs to know about hit-mt at
> all. What it needs to know is whether the device should be dropped.
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 6559e2e..f333139 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -474,6 +474,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>                        map_key_clear(BTN_STYLUS2);
>                        break;
>
> +               case 0x51: /* ContactID */
> +                       device->quirks |= HID_QUIRK_MULTITOUCH;
> +                       goto unknown;
> +
>                default:  goto unknown;
>                }
>                break;
>
> So in addition to the detection here,
>
> @@ -978,6 +982,13 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>                }
>        }
>
> +       if (hid->quirks & HID_QUIRK_MULTITOUCH) {
> +               /* generic hid does not know how to handle multitouch devices */
> +               if (hidinput)
> +                       goto out_cleanup;
> +               goto out_unwind;
> +       }
> +
>        if (hidinput && input_register_device(hidinput->input))
>                goto out_cleanup;
>
> One could instead drop handling based on a quirk designed for that
> purpose (HID_QUIRK_HIDINPUT_DROP).
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index f1c909f..28f8ff2 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -291,6 +291,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>                        td->last_slot_field = usage->hid;
>                        td->last_field_index = field->index;
>                        td->last_mt_collection = usage->collection_index;
> +                       hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
>                        return 1;
>                case HID_DG_WIDTH:
>                        hid_map_usage(hi, usage, bit, max,
>
> Although correct per se, it is clearer to reset this flag in mt_probe().
>
> @@ -535,6 +536,12 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>        struct mt_device *td;
>        struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
>
> +       if (!id->driver_data && !(hdev->quirks & HID_QUIRK_MULTITOUCH))
> +               /* cought by HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID),
> +                * and either in hid_have_special_driver
> +                * or not detected as multitouch by hid-core */
> +               return -ENODEV;
> +
>        for (i = 0; mt_classes[i].name ; i++) {
>                if (id->driver_data == mt_classes[i].name) {
>                        mtclass = &(mt_classes[i]);
>
> Very neat solution indeed!
>
> @@ -542,10 +549,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                }
>        }
>
> -       /* This allows the driver to correctly support devices
> -        * that emit events over several HID messages.
> -        */
> -       hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
>
>        td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
>        if (!td) {
>
> It seems there is no longer any reason to move this line around, since
> we now only come here when the device is really meant for this driver.
>
> @@ -561,10 +564,16 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>        if (ret != 0)
>                goto fail;
>
> +       hdev->quirks |= HID_QUIRK_MULTITOUCH;
>
> This is unnecessary and makes the logic blurred.
>
>        ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>        if (ret)
>                goto fail;
>
> +       /* This allows the driver to correctly support devices
> +        * that emit events over several HID messages.
> +        */
> +       hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
> +
>        td->slots = kzalloc(td->maxcontacts * sizeof(struct mt_slot),
>                                GFP_KERNEL);
>        if (!td->slots) {
>
> In addition to not needing to be moved, this line introduces a race
> with hid-input, since the device has already started when this line is
> executed.

Well, we should use instead the new input_register callback to avoid any races.
I've got the patch, I've tested it, but I never sent it.... shame on me.

>
> @@ -758,6 +767,9 @@ static const struct hid_device_id mt_devices[] = {
>                HID_USB_DEVICE(USB_VENDOR_ID_XAT,
>                        USB_DEVICE_ID_XAT_CSR) },
>
> +       /* Rest of the world */
> +       { HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID) },
> +
>        { }
>  };
>  MODULE_DEVICE_TABLE(hid, mt_devices);
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 9cf8e7a..6fb743d 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -312,6 +312,7 @@ struct hid_item {
>  #define HID_QUIRK_BADPAD                       0x00000020
>  #define HID_QUIRK_MULTI_INPUT                  0x00000040
>  #define HID_QUIRK_HIDINPUT_FORCE               0x00000080
> +#define HID_QUIRK_MULTITOUCH                   0x00000100
>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS          0x00010000
>  #define HID_QUIRK_FULLSPEED_INTERVAL           0x10000000
>  #define HID_QUIRK_NO_INIT_REPORTS              0x20000000
>
> As an alternative, here are three untested and uncommited patches
> which implements the comments above.
>

Except a comment in the second patch, I've tested it, and it worked
without any surprises ;-)

But I'm not sure we can rewind the tree as those patches are in
for-next since nearly a month.
I'll let you do the reverts, because on my local trees, it was quite
difficult (and I'm still on a 3.0.x, so I can not test against
for-next)


> --
>
> From defdac444919b99a932368ee1a8ad290dc724933 Mon Sep 17 00:00:00 2001
> From: Henrik Rydberg <rydberg@euromail.se>
> Date: Fri, 28 Oct 2011 11:36:36 +0200
> Subject: [PATCH 1/3] hid: Allow an input device to be dropped after parsing
>
> Some devices need a special driver based on the input mapping of the
> device. This patch enables a mechanism where hidinput can set
> HID_QUIRK_HIDINPUT_DROP to leave a device to be picked up by a special
> driver which intercepts the input mapping.
> ---
>  drivers/hid/hid-core.c  |    3 +++
>  drivers/hid/hid-input.c |    6 ++++++
>  include/linux/hid.h     |    1 +
>  3 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 956f849..2628f9c 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1212,6 +1212,9 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
>        if ((connect_mask & HID_CONNECT_HIDINPUT) && !hidinput_connect(hdev,
>                                connect_mask & HID_CONNECT_HIDINPUT_FORCE))
>                hdev->claimed |= HID_CLAIMED_INPUT;
> +       if (hdev->quirks & HID_QUIRK_HIDINPUT_DROP)
> +               return -ENODEV;
> +
>        if ((connect_mask & HID_CONNECT_HIDDEV) && hdev->hiddev_connect &&
>                        !hdev->hiddev_connect(hdev,
>                                connect_mask & HID_CONNECT_HIDDEV_FORCE))
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 6559e2e..aa3ce2e 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -978,6 +978,12 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>                }
>        }
>
> +       if (hid->quirks & HID_QUIRK_HIDINPUT_DROP) {
> +               if (hidinput)
> +                       goto out_cleanup;
> +               goto out_unwind;
> +       }
> +
>        if (hidinput && input_register_device(hidinput->input))
>                goto out_cleanup;
>
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 9cf8e7a..4028a27 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -312,6 +312,7 @@ struct hid_item {
>  #define HID_QUIRK_BADPAD                       0x00000020
>  #define HID_QUIRK_MULTI_INPUT                  0x00000040
>  #define HID_QUIRK_HIDINPUT_FORCE               0x00000080
> +#define HID_QUIRK_HIDINPUT_DROP                        0x00000100
>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS          0x00010000
>  #define HID_QUIRK_FULLSPEED_INTERVAL           0x10000000
>  #define HID_QUIRK_NO_INIT_REPORTS              0x20000000
> --
> 1.7.7
>
> From 5f4fe6ef4cab9721c6b277f57d5374d6b549359d Mon Sep 17 00:00:00 2001
> From: Henrik Rydberg <rydberg@euromail.se>
> Date: Fri, 28 Oct 2011 11:42:39 +0200
> Subject: [PATCH 2/3] hid-input: Drop generic handling of hid-mt multitouch
>  devices
>
> The hid-mt devices are recognized by the ContactID field. This patch
> sets HID_QUIRK_MULTITOUCH accordingly, and leaves the device to be
> picked up by any driver which intercepts the ContactID field.
>
> All in-tree hid-mt drivers intercept the ContactID, so this patch has
> no other effect than to skip generic handling of hid-mt devices.

Please add here the link to
http://www.microsoft.com/whdc/device/input/DigitizerDrvs_touch.mspx
to keep the origin of this statement: ContactID means multitouch

Cheers,
Benjamin

> ---
>  drivers/hid/hid-input.c |    6 ++++++
>  include/linux/hid.h     |    1 +
>  2 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index aa3ce2e..108830fc 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -474,6 +474,12 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>                        map_key_clear(BTN_STYLUS2);
>                        break;
>
> +               case 0x51: /* ContactID */
> +                       device->quirks |= HID_QUIRK_MULTITOUCH;
> +                       /* hid-mt is not handled by generic hid */
> +                       device->quirks |= HID_QUIRK_HIDINPUT_DROP;
> +                       goto unknown;
> +
>                default:  goto unknown;
>                }
>                break;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 4028a27..b9c4296 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -313,6 +313,7 @@ struct hid_item {
>  #define HID_QUIRK_MULTI_INPUT                  0x00000040
>  #define HID_QUIRK_HIDINPUT_FORCE               0x00000080
>  #define HID_QUIRK_HIDINPUT_DROP                        0x00000100
> +#define HID_QUIRK_MULTITOUCH                   0x00000200
>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS          0x00010000
>  #define HID_QUIRK_FULLSPEED_INTERVAL           0x10000000
>  #define HID_QUIRK_NO_INIT_REPORTS              0x20000000
> --
> 1.7.7
>
> From 6b1ecc88b3f5cca7c9178e3900c6766544db0798 Mon Sep 17 00:00:00 2001
> From: Henrik Rydberg <rydberg@euromail.se>
> Date: Fri, 28 Oct 2011 12:01:08 +0200
> Subject: [PATCH 3/3] hid-multitouch: Pick up all hid-mt devices
>
> This patch adds a catch-all device entry, which "undrops" all devices
> that have been identified as hid-mt by the hid core.
> ---
>  drivers/hid/hid-multitouch.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index f1c909f..1f051eb 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -535,6 +535,15 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>        struct mt_device *td;
>        struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
>
> +       if (!id->driver_data && !(hdev->quirks & HID_QUIRK_MULTITOUCH))
> +               /* cought by HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID),
> +                * and either in hid_have_special_driver
> +                * or not detected as multitouch by hid-core */
> +               return -ENODEV;
> +
> +       /* pick up the device that was dropped by hid-core */
> +       hdev->quirks &= ~HID_QUIRK_HIDINPUT_DROP;
> +
>        for (i = 0; mt_classes[i].name ; i++) {
>                if (id->driver_data == mt_classes[i].name) {
>                        mtclass = &(mt_classes[i]);
> @@ -758,6 +767,9 @@ static const struct hid_device_id mt_devices[] = {
>                HID_USB_DEVICE(USB_VENDOR_ID_XAT,
>                        USB_DEVICE_ID_XAT_CSR) },
>
> +       /* Rest of the world */
> +       { HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID) },
> +
>        { }
>  };
>  MODULE_DEVICE_TABLE(hid, mt_devices);
> --
> 1.7.7
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG: hid-multitouch causes 10 second delay and error

[Henrik Rydberg]

Hi Benjamin,

> well, most of the comments you in-lined addressed the fact that I
> tried to make the smallest patch possible.

Admittedly, HID_QUIRK_HIDINPUT_DROP makes the patch larger, but all
other comments make it smaller.

> > +       /* This allows the driver to correctly support devices
> > +        * that emit events over several HID messages.
> > +        */
> > +       hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
> > +
> >        td->slots = kzalloc(td->maxcontacts * sizeof(struct mt_slot),
> >                                GFP_KERNEL);
> >        if (!td->slots) {
> >
> > In addition to not needing to be moved, this line introduces a race
> > with hid-input, since the device has already started when this line is
> > executed.
> 
> Well, we should use instead the new input_register callback to avoid
> any races.  I've got the patch, I've tested it, but I never sent
> it.... shame on me.

I guess it is moot now anyways.

> Except a comment in the second patch, I've tested it, and it worked
> without any surprises ;-)

The code still mainly follows your initial idea, so of course it
works. ;-)

> But I'm not sure we can rewind the tree as those patches are in
> for-next since nearly a month.
> I'll let you do the reverts, because on my local trees, it was quite
> difficult (and I'm still on a 3.0.x, so I can not test against
> for-next)

Ok, let's see how Jiri wants to play it, and I can prepare either a
rewind-and-patch or a revert-and-patch or a keep-and-patch set.

> > Subject: [PATCH 2/3] hid-input: Drop generic handling of hid-mt multitouch
> >  devices
> >
> > The hid-mt devices are recognized by the ContactID field. This patch
> > sets HID_QUIRK_MULTITOUCH accordingly, and leaves the device to be
> > picked up by any driver which intercepts the ContactID field.
> >
> > All in-tree hid-mt drivers intercept the ContactID, so this patch has
> > no other effect than to skip generic handling of hid-mt devices.
> 
> Please add here the link to
> http://www.microsoft.com/whdc/device/input/DigitizerDrvs_touch.mspx
> to keep the origin of this statement: ContactID means multitouch

Thanks, I will also make sure authorship is preserved where appropriate.

Cheers,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG: hid-multitouch causes 10 second delay and error

[Jiri Kosina]

On Fri, 28 Oct 2011, Henrik Rydberg wrote:

> > for-next since nearly a month.
> > I'll let you do the reverts, because on my local trees, it was quite
> > difficult (and I'm still on a 3.0.x, so I can not test against
> > for-next)
> 
> Ok, let's see how Jiri wants to play it, and I can prepare either a
> rewind-and-patch or a revert-and-patch or a keep-and-patch set.

As I have already flushed my initial 3.1 queue to Linus already, I'd like 
to ask you guys to send me a patch based on Linus' tree (or my 
'upstream-fixes' branch, which is what I am going to be sending as fixes 
for 3.2 still).

I will then make sure that it reaches Linus before 3.2 has been released.

> > Please add here the link to
> > http://www.microsoft.com/whdc/device/input/DigitizerDrvs_touch.mspx
> > to keep the origin of this statement: ContactID means multitouch
> 
> Thanks, I will also make sure authorship is preserved where appropriate.

Thanks a lot to both of you, and also an excellent bugreport by Sean.

-- 
Jiri Kosina
SUSE Labs

Re: BUG: hid-multitouch causes 10 second delay and error

[Henrik Rydberg]

>From bc81ba588149a0058ca083f673cb22ff543e5af8 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <rydberg@euromail.se>
Date: Tue, 1 Nov 2011 13:20:59 +0100
Subject: [PATCH 1/2] Revert "HID: multitouch: decide if hid-multitouch needs
 to handle mt devices"

This reverts commit 0db3bfc72adf0cb70f08dfe92e4040f64e25e205.

The generic detection of hid-mt devices has two major flaws, and was
merged prematurely. Firstly, the hid-multitouch gets loaded even when
the device is handled by a special device. Secondly, the patch only
partially duplicates the device whitelist already present in hid-core,
effectively rendering a number of devices non-functional.

Reported-by: Sean Young <sean@mess.org>
Tested-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
Hi Jiri,

Unfortunately, the recently discussed solution does not seem to work,
so there is only a revert for now.

I was planning to send a second patch, very small and concise, based
on Benjamins patch, but

1. It still does not resolve the
hid-multitouch-gets-loaded-for-every-hid-device problem, and

2. It did not work after removing the tested device from the hid core
whitelist; the device quirk seemed to get lost in the process.

In other words, there is currently no viable solution.

Thanks,
Henrik

 drivers/hid/hid-multitouch.c |   47 +++--------------------------------------
 1 files changed, 4 insertions(+), 43 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index fa5d7a1..f1c909f 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -291,7 +291,6 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			td->last_slot_field = usage->hid;
 			td->last_field_index = field->index;
 			td->last_mt_collection = usage->collection_index;
-			hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
 			return 1;
 		case HID_DG_WIDTH:
 			hid_map_usage(hi, usage, bit, max,
@@ -530,44 +529,12 @@ static void mt_set_input_mode(struct hid_device *hdev)
 	}
 }
 
-/* a list of devices for which there is a specialized multitouch driver */
-static const struct hid_device_id mt_have_special_driver[] = {
-	{ HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, 0x0001) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, 0x0006) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_QUANTA,
-			USB_DEVICE_ID_PIXART_IMAGING_INC_OPTICAL_TOUCH_SCREEN) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_QUANTA,
-			USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH) },
-	{ }
-};
-
-static bool mt_match_one_id(struct hid_device *hdev,
-		const struct hid_device_id *id)
-{
-	return id->bus == hdev->bus &&
-		(id->vendor == HID_ANY_ID || id->vendor == hdev->vendor) &&
-		(id->product == HID_ANY_ID || id->product == hdev->product);
-}
-
-static const struct hid_device_id *mt_match_id(struct hid_device *hdev,
-		const struct hid_device_id *id)
-{
-	for (; id->bus; id++)
-		if (mt_match_one_id(hdev, id))
-			return id;
-
-	return NULL;
-}
-
 static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	int ret, i;
 	struct mt_device *td;
 	struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
 
-	if (mt_match_id(hdev, mt_have_special_driver))
-		return -ENODEV;
-
 	for (i = 0; mt_classes[i].name ; i++) {
 		if (id->driver_data == mt_classes[i].name) {
 			mtclass = &(mt_classes[i]);
@@ -575,6 +542,10 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		}
 	}
 
+	/* This allows the driver to correctly support devices
+	 * that emit events over several HID messages.
+	 */
+	hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
 
 	td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
 	if (!td) {
@@ -590,16 +561,10 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (ret != 0)
 		goto fail;
 
-	hdev->quirks |= HID_QUIRK_MULTITOUCH;
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret)
 		goto fail;
 
-	/* This allows the driver to correctly support devices
-	 * that emit events over several HID messages.
-	 */
-	hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
-
 	td->slots = kzalloc(td->maxcontacts * sizeof(struct mt_slot),
 				GFP_KERNEL);
 	if (!td->slots) {
@@ -793,10 +758,6 @@ static const struct hid_device_id mt_devices[] = {
 		HID_USB_DEVICE(USB_VENDOR_ID_XAT,
 			USB_DEVICE_ID_XAT_CSR) },
 
-	/* Rest of the world */
-	{ .driver_data = MT_CLS_DEFAULT,
-		HID_USB_DEVICE(HID_ANY_ID, HID_ANY_ID) },
-
 	{ }
 };
 MODULE_DEVICE_TABLE(hid, mt_devices);
-- 
1.7.7.1

Re: BUG: hid-multitouch causes 10 second delay and error

[Jiri Kosina]

On Tue, 1 Nov 2011, Henrik Rydberg wrote:

> This reverts commit 0db3bfc72adf0cb70f08dfe92e4040f64e25e205.
> 
> The generic detection of hid-mt devices has two major flaws, and was
> merged prematurely. Firstly, the hid-multitouch gets loaded even when
> the device is handled by a special device. Secondly, the patch only
> partially duplicates the device whitelist already present in hid-core,
> effectively rendering a number of devices non-functional.
> 
> Reported-by: Sean Young <sean@mess.org>
> Tested-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
> Hi Jiri,
> 
> Unfortunately, the recently discussed solution does not seem to work,
> so there is only a revert for now.
> 
> I was planning to send a second patch, very small and concise, based
> on Benjamins patch, but
> 
> 1. It still does not resolve the
> hid-multitouch-gets-loaded-for-every-hid-device problem, and

Hmm, I thought it should work. What is the catch?

> In other words, there is currently no viable solution.

Thanks, I will be including the patch in my next pile of fixes for 3.2 
that will go to Linus.

-- 
Jiri Kosina
SUSE Labs

Re: BUG: hid-multitouch causes 10 second delay and error

[Henrik Rydberg]

> > 1. It still does not resolve the
> > hid-multitouch-gets-loaded-for-every-hid-device problem, and
> 
> Hmm, I thought it should work. What is the catch?

Everyone with an umatched hid device, even completely unrelated to
touch, will be surprised to find the hid-multitouch module loaded.

Thanks,
Henrik

Re: BUG: hid-multitouch causes 10 second delay and error

[Benjamin Tissoires]

Hi Henrik

On Tue, Nov 1, 2011 at 16:33, Henrik Rydberg <rydberg@euromail.se> wrote:
>> > 1. It still does not resolve the
>> > hid-multitouch-gets-loaded-for-every-hid-device problem, and
>>
>> Hmm, I thought it should work. What is the catch?
>
> Everyone with an umatched hid device, even completely unrelated to
> touch, will be surprised to find the hid-multitouch module loaded.
>

Well, this is a problem that can not be easily solved: IIRC, we can
not force the load of an external driver from within the kernel.
The best solution would be to merge hid-input and hid-multitouch.
Indeed both systems aim at handling generic devices.However, I'd
rather not doing it now as we are not as "good" as the Win 7 driver
(i.e. there are some fallback modes that allow every devices to be
handled even if they don't send clean hid reports).

And, even if hid-multitouch is loaded, only 2 or 3 lines of codes will
be executed to reject the driver in mt_probe, which won't be very time
consuming for end user.

For your second point:
>> > 2. It did not work after removing the tested device from the hid core
>> > whitelist; the device quirk seemed to get lost in the process.

Didn't you forget to remove the following line?

--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1230,7 +1230,6 @@ int hid_connect(struct hid_device *hdev,
unsigned int connect_mask)
               hdev->claimed |= HID_CLAIMED_INPUT;
       if (hdev->quirks & HID_QUIRK_MULTITOUCH) {
               /* this device should be handled by hid-multitouch, skip it */
-               hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
               return -ENODEV;
       }


If not, that's maybe that you encountered the only case that is not
correctly handled:
if you register hid-multitouch before hid, then it will be the first
driver tested, and hid-input won't set the quirk correctly.
BTW, it's not a big deal, because if systems do have this behavior, we
can easily put the device from the user space by using
/sys/module/hid_multitouch/drivers/hid\:hid-multitouch/new_id

Cheers,
Benjamin

Re: BUG: hid-multitouch causes 10 second delay and error

[Henrik Rydberg]

Hi Benjamin,

> > Everyone with an umatched hid device, even completely unrelated to
> > touch, will be surprised to find the hid-multitouch module loaded.
> >
> 
> Well, this is a problem that can not be easily solved: IIRC, we can
> not force the load of an external driver from within the kernel.
> The best solution would be to merge hid-input and hid-multitouch.

Yes, or actually adding a dynamic mechanism.  With hid, it would
clearly be beneficial to be able to load modules based on the result
of the report parsing.

> Indeed both systems aim at handling generic devices.However, I'd
> rather not doing it now as we are not as "good" as the Win 7 driver
> (i.e. there are some fallback modes that allow every devices to be
> handled even if they don't send clean hid reports).

What's wrong with having a generic handling in addition to the
specific device list in hid-multitouch?

> And, even if hid-multitouch is loaded, only 2 or 3 lines of codes will
> be executed to reject the driver in mt_probe, which won't be very time
> consuming for end user.

The time to load the module will hit _every_ user, which is worse than
having the code merged with hid-input. Not to mention the annoyance,
it is simply unacceptable.

> For your second point:
> >> > 2. It did not work after removing the tested device from the hid core
> >> > whitelist; the device quirk seemed to get lost in the process.
> 
> Didn't you forget to remove the following line?
> 
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1230,7 +1230,6 @@ int hid_connect(struct hid_device *hdev,
> unsigned int connect_mask)
>                hdev->claimed |= HID_CLAIMED_INPUT;
>        if (hdev->quirks & HID_QUIRK_MULTITOUCH) {
>                /* this device should be handled by hid-multitouch, skip it */
> -               hdev->quirks &= ~HID_QUIRK_MULTITOUCH;
>                return -ENODEV;
>        }
> 
>

Gah, how did that end up there? Yes, I missed that line in my testing,
which explains it (although I won't test again right now).

> If not, that's maybe that you encountered the only case that is not
> correctly handled:
> if you register hid-multitouch before hid, then it will be the first
> driver tested, and hid-input won't set the quirk correctly.
> BTW, it's not a big deal, because if systems do have this behavior, we
> can easily put the device from the user space by using
> /sys/module/hid_multitouch/drivers/hid\:hid-multitouch/new_id

This is also hackish. I _do_ understand the benefits of what we are
aiming at here, but we are piling up crap in the kernel.

To summarize, the idea looked good at first glance, but I think it
creates unacceptable dependencies between modules.

To try again later on, at the very least one should move the essence
of hid-multitouch to something like hid-input-mt, and have hid-input
either include it, select it or depend dynamically on it, in a
try_module_get fashion. The hid_multitouch would then simply contain
the device white list, leaving the general case to hid-input.

Henrik