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

Hi Benjamin,

quoted

Hi Sean, can you test the following patch please:


From 488272baf9bc95718dba2b9a0f62fe3309ca578f Mon Sep 17 00:00:00 2001
From: Benjamin Tissoires <redacted>
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.

--

From defdac444919b99a932368ee1a8ad290dc724933 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <redacted>
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 <redacted>
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 <redacted>
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