Hi Henrik,

On Wed, Aug 21, 2013 at 8:15 PM, Henrik Rydberg [off-list ref] wrote:

Hi Benjamin,

this looks pretty good to me, just a few nitpicks below.

quoted

hid_scan_report() implements its own HID report descriptor parsing. It is
going to be really bad with the detection of Win 8 certified touchscreen,
as this detection relies on a special feature and on the report_size and
report_count fields.

How about 'The Win 8 detection is sufficiently complex to warrant use
of the full parser code, in spite of the inferred memory
usage. Therefore...'

Sure. Will change in v3.

quoted

We can use the existing HID parser in hid-core for hid_scan_report()
by re-using the code from hid_open_report(). hid_parser_global,
hid_parser_local and hid_parser_reserved does not have any side effects.
We just need to reimplement the MAIN_ITEM callback to have a proper
parsing without side effects.

Signed-off-by: Benjamin Tissoires <redacted>
---

changes in v2:
- moved "flags" processing in patch 2/3 (so use hid->group in this patch)
- hid_scan_report() is less verbose when errors are found in the descriptor
- hid_scan_report() is tolerant to parsing errors
- fixed usage_page handling in hid_scan_collection(), which fixes sensors detection
- amended commit message

 drivers/hid/hid-core.c | 106 +++++++++++++++++++++++++++++++------------------
 1 file changed, 67 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3efe19f..e072b15 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c

@@ -677,12 +677,55 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
      return NULL;
 }

-static void hid_scan_usage(struct hid_device *hid, u32 usage)
+static void hid_scan_input_usage(struct hid_parser *parser, u32 usage)
 {
+     struct hid_device *hid = parser->device;
+
      if (usage == HID_DG_CONTACTID)
              hid->group = HID_GROUP_MULTITOUCH;
 }

+static void hid_scan_collection(struct hid_parser *parser, unsigned type)
+{
+     struct hid_device *hid = parser->device;
+
+     if (((parser->global.usage_page << 16) == HID_UP_SENSOR) &&
+         type == HID_COLLECTION_PHYSICAL)
+             hid->group = HID_GROUP_SENSOR_HUB;
+}
+
+static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
+{
+     __u32 data;
+     int i;
+
+     data = item_udata(item);
+
+     switch (item->tag) {
+     case HID_MAIN_ITEM_TAG_BEGIN_COLLECTION:
+             hid_scan_collection(parser, data & 0xff);
+             break;
+     case HID_MAIN_ITEM_TAG_END_COLLECTION:
+             break;
+     case HID_MAIN_ITEM_TAG_INPUT:
+             for (i = 0; i < parser->local.usage_index; i++)
+                     hid_scan_input_usage(parser, parser->local.usage[i]);
+             break;
+     case HID_MAIN_ITEM_TAG_OUTPUT:
+             break;
+     case HID_MAIN_ITEM_TAG_FEATURE:
+             break;
+     default:
+             hid_err(parser->device, "unknown main item tag 0x%x\n",
+                     item->tag);

Looks this this message is a duplicate as well.

oops, forgot to remove it :)

Cheers,
Benjamin

quoted

+     }
+
+     /* Reset the local parser environment */
+     memset(&parser->local, 0, sizeof(parser->local));
+
+     return 0;
+}
+
 /*
  * Scan a report descriptor before the device is added to the bus.
  * Sets device groups and other properties that determine what driver

@@ -690,48 +733,33 @@ static void hid_scan_usage(struct hid_device *hid, u32 usage)
  */
 static int hid_scan_report(struct hid_device *hid)
 {
-     unsigned int page = 0, delim = 0;
+     struct hid_parser *parser;
+     struct hid_item item;
      __u8 *start = hid->dev_rdesc;
      __u8 *end = start + hid->dev_rsize;
-     unsigned int u, u_min = 0, u_max = 0;
-     struct hid_item item;
+     static int (*dispatch_type[])(struct hid_parser *parser,
+                                   struct hid_item *item) = {
+             hid_scan_main,
+             hid_parser_global,
+             hid_parser_local,
+             hid_parser_reserved
+     };

-     hid->group = HID_GROUP_GENERIC;
-     while ((start = fetch_item(start, end, &item)) != NULL) {
-             if (item.format != HID_ITEM_FORMAT_SHORT)
-                     return -EINVAL;
-             if (item.type == HID_ITEM_TYPE_GLOBAL) {
-                     if (item.tag == HID_GLOBAL_ITEM_TAG_USAGE_PAGE)
-                             page = item_udata(&item) << 16;
-             } else if (item.type == HID_ITEM_TYPE_LOCAL) {
-                     if (delim > 1)
-                             break;
-                     u = item_udata(&item);
-                     if (item.size <= 2)
-                             u += page;
-                     switch (item.tag) {
-                     case HID_LOCAL_ITEM_TAG_DELIMITER:
-                             delim += !!u;
-                             break;
-                     case HID_LOCAL_ITEM_TAG_USAGE:
-                             hid_scan_usage(hid, u);
-                             break;
-                     case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
-                             u_min = u;
-                             break;
-                     case HID_LOCAL_ITEM_TAG_USAGE_MAXIMUM:
-                             u_max = u;
-                             for (u = u_min; u <= u_max; u++)
-                                     hid_scan_usage(hid, u);
-                             break;
-                     }
-             } else if (page == HID_UP_SENSOR &&
-                     item.type == HID_ITEM_TYPE_MAIN &&
-                     item.tag == HID_MAIN_ITEM_TAG_BEGIN_COLLECTION &&
-                     (item_udata(&item) & 0xff) == HID_COLLECTION_PHYSICAL)
-                     hid->group = HID_GROUP_SENSOR_HUB;
-     }
+     parser = vzalloc(sizeof(struct hid_parser));
+     if (!parser)
+             return -ENOMEM;
+
+     parser->device = hid;
+
+     /*
+      * The parsing is simpler than the one in hid_open_report() as we should
+      * be robust against hid errors. Those errors will be raised by
+      * hid_open_report() anyway.
+      */
+     while ((start = fetch_item(start, end, &item)) != NULL)
+             dispatch_type[item.type](parser, &item);

+     vfree(parser);
      return 0;
 }

--

1.8.3.1

Thanks,
Henrik