[PATCH 0/3] HID: Win 8 multitouch panels detection in core

[PATCH 0/3] HID: Win 8 multitouch panels detection in core

[Benjamin Tissoires]

Hi guys,

This series is the following of the patch "HID: multitouch: do not init
reports for multitouch devices" which I sent on the 12th of July.

The goal is just to not call a GET_REPORT on input reports to imitate
what Win 8 is doing with the touch panels. However, to have an accurate
detection of Win 8 panels and to not introduce regressions, we need
to adapt the pre-scanning of hid devices.

The first patch drops the custom parsing and relies on the well tested
parser that we have in hid-core. This cleans up the pre-scanning and
allows us to scan the features in addition to the input reports and the
collections.

The second patch uses the pre-scanning method to detect Win 8 multitouch
panels. This allows a simplification in hid-multitouch because those
panels will now use the general path instead of having special quirks
attached during the parsing.

The third patch introduce the actual quirk which would allow us not to
have to maintain a growing list of quirks.

Cheers,
Benjamin

Benjamin Tissoires (3):
  HID: Use existing parser for pre-scanning the report descriptors
  HID: detect Win 8 multitouch devices in core
  HID: do not init input reports for Win 8 multitouch devices

 drivers/hid/hid-core.c        | 143 +++++++++++++++++++++++++++++++-----------
 drivers/hid/hid-multitouch.c  |  36 +++++++----
 drivers/hid/usbhid/hid-core.c |  11 +++-
 include/linux/hid.h           |   7 +++
 4 files changed, 143 insertions(+), 54 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] HID: Use existing parser for pre-scanning the report descriptors

[Benjamin Tissoires]

hid_scan_report() implements its own HID report descriptor parsing. It was
fine until we added the SENSOR_HUB detection. It is going to be even worse
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.

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.

Instead of directly overwriting the ->group field, this patch introduce
a ->flags field and then decide which group the device belongs to,
depending on the whole parsing (not just the local item). This will be
useful for Win 8 multitouch devices, which are multitouch devices and
Win 8 certified (so 2 flags to check).

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c | 131 +++++++++++++++++++++++++++++++++++--------------
 include/linux/hid.h    |   4 ++
 2 files changed, 97 insertions(+), 38 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3efe19f..d8cdb0a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -677,10 +677,49 @@ 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)
 {
 	if (usage == HID_DG_CONTACTID)
-		hid->group = HID_GROUP_MULTITOUCH;
+		parser->flags |= HID_FLAG_MULTITOUCH;
+}
+
+static void hid_scan_open_collection(struct hid_parser *parser, unsigned type)
+{
+	if (parser->global.usage_page == HID_UP_SENSOR &&
+	    type == HID_COLLECTION_PHYSICAL)
+		parser->flags |= HID_FLAG_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_open_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);
+	}
+
+	/* Reset the local parser environment */
+	memset(&parser->local, 0, sizeof(parser->local));
+
+	return 0;
 }
 
 /*
@@ -690,49 +729,65 @@ 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;
+	int ret;
+	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;
+	parser = vzalloc(sizeof(struct hid_parser));
+	if (!parser)
+		return -ENOMEM;
+
+	parser->device = hid;
+
+	ret = -EINVAL;
 	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;
+
+		if (item.format != HID_ITEM_FORMAT_SHORT) {
+			hid_err(hid, "unexpected long global item\n");
+			goto out;
+		}
+
+		if (dispatch_type[item.type](parser, &item)) {
+			hid_err(hid, "item %u %u %u %u parsing failed\n",
+				item.format, (unsigned)item.size,
+				(unsigned)item.type, (unsigned)item.tag);
+			goto out;
+		}
+
+		if (start == end) {
+			if (parser->local.delimiter_depth) {
+				hid_err(hid, "unbalanced delimiter at end of report description\n");
+				goto out;
 			}
-		} 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;
+			ret = 0;
+			goto out;
+		}
 	}
 
-	return 0;
+	hid_err(hid, "item fetching failed at offset %d\n", (int)(end - start));
+out:
+	switch (parser->flags) {
+	case HID_FLAG_MULTITOUCH:
+		hid->group = HID_GROUP_MULTITOUCH;
+		break;
+	case HID_FLAG_SENSOR_HUB:
+		hid->group = HID_GROUP_SENSOR_HUB;
+		break;
+	default:
+		hid->group = HID_GROUP_GENERIC;
+	}
+
+	vfree(parser);
+	return ret;
 }
 
 /**
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 5a4e789..7d823db 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -533,6 +533,9 @@ static inline void hid_set_drvdata(struct hid_device *hdev, void *data)
 #define HID_GLOBAL_STACK_SIZE 4
 #define HID_COLLECTION_STACK_SIZE 4
 
+#define HID_FLAG_MULTITOUCH		0x0001
+#define HID_FLAG_SENSOR_HUB		0x0002
+
 struct hid_parser {
 	struct hid_global     global;
 	struct hid_global     global_stack[HID_GLOBAL_STACK_SIZE];
@@ -541,6 +544,7 @@ struct hid_parser {
 	unsigned              collection_stack[HID_COLLECTION_STACK_SIZE];
 	unsigned              collection_stack_ptr;
 	struct hid_device    *device;
+	unsigned              flags;
 };
 
 struct hid_class_descriptor {
-- 
1.8.3.1

[PATCH 2/3] HID: detect Win 8 multitouch devices in core

[Benjamin Tissoires]

Detecting Win 8 multitouch devices in core allows us to set quirks
before the device is parsed through hid_hw_start().
It also simplifies the detection of those devices in hid-multitouch and
makes the handling of those devices cleaner.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c       | 12 ++++++++++++
 drivers/hid/hid-multitouch.c | 24 +++++++++++-------------
 include/linux/hid.h          |  2 ++
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index d8cdb0a..11c6f2b 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -683,6 +683,13 @@ static void hid_scan_input_usage(struct hid_parser *parser, u32 usage)
 		parser->flags |= HID_FLAG_MULTITOUCH;
 }
 
+static void hid_scan_feature_usage(struct hid_parser *parser, u32 usage)
+{
+	if (usage == 0xff0000c5 && parser->global.report_count == 256 &&
+	    parser->global.report_size == 8)
+		parser->flags |= HID_FLAG_WIN_8_CERTIFIED;
+}
+
 static void hid_scan_open_collection(struct hid_parser *parser, unsigned type)
 {
 	if (parser->global.usage_page == HID_UP_SENSOR &&
@@ -710,6 +717,8 @@ static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
 	case HID_MAIN_ITEM_TAG_OUTPUT:
 		break;
 	case HID_MAIN_ITEM_TAG_FEATURE:
+		for (i = 0; i < parser->local.usage_index; i++)
+			hid_scan_feature_usage(parser, parser->local.usage[i]);
 		break;
 	default:
 		hid_err(parser->device, "unknown main item tag 0x%x\n",
@@ -779,6 +788,9 @@ out:
 	case HID_FLAG_MULTITOUCH:
 		hid->group = HID_GROUP_MULTITOUCH;
 		break;
+	case HID_FLAG_MULTITOUCH | HID_FLAG_WIN_8_CERTIFIED:
+		hid->group = HID_GROUP_MULTITOUCH_WIN_8;
+		break;
 	case HID_FLAG_SENSOR_HUB:
 		hid->group = HID_GROUP_SENSOR_HUB;
 		break;
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 0fe00e2..c28ef86 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -133,6 +133,7 @@ static void mt_post_parse(struct mt_device *td);
 #define MT_CLS_NSMU				0x000a
 #define MT_CLS_DUAL_CONTACT_NUMBER		0x0010
 #define MT_CLS_DUAL_CONTACT_ID			0x0011
+#define MT_CLS_WIN_8				0x0012
 
 /* vendor specific classes */
 #define MT_CLS_3M				0x0101
@@ -205,6 +206,11 @@ static struct mt_class mt_classes[] = {
 			MT_QUIRK_CONTACT_CNT_ACCURATE |
 			MT_QUIRK_SLOT_IS_CONTACTID,
 		.maxcontacts = 2 },
+	{ .name = MT_CLS_WIN_8,
+		.quirks = MT_QUIRK_ALWAYS_VALID |
+			MT_QUIRK_IGNORE_DUPLICATES |
+			MT_QUIRK_HOVERING |
+			MT_QUIRK_CONTACT_CNT_ACCURATE },
 
 	/*
 	 * vendor specific classes
@@ -332,19 +338,6 @@ static void mt_feature_mapping(struct hid_device *hdev,
 			td->maxcontacts = td->mtclass.maxcontacts;
 
 		break;
-	case 0xff0000c5:
-		if (field->report_count == 256 && field->report_size == 8) {
-			/* Win 8 devices need special quirks */
-			__s32 *quirks = &td->mtclass.quirks;
-			*quirks |= MT_QUIRK_ALWAYS_VALID;
-			*quirks |= MT_QUIRK_IGNORE_DUPLICATES;
-			*quirks |= MT_QUIRK_HOVERING;
-			*quirks |= MT_QUIRK_CONTACT_CNT_ACCURATE;
-			*quirks &= ~MT_QUIRK_NOT_SEEN_MEANS_UP;
-			*quirks &= ~MT_QUIRK_VALID_IS_INRANGE;
-			*quirks &= ~MT_QUIRK_VALID_IS_CONFIDENCE;
-		}
-		break;
 	}
 }
 
@@ -1346,6 +1339,11 @@ static const struct hid_device_id mt_devices[] = {
 
 	/* Generic MT device */
 	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH, HID_ANY_ID, HID_ANY_ID) },
+
+	/* Generic Win 8 certified MT device */
+	{  .driver_data = MT_CLS_WIN_8,
+		HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8,
+			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 7d823db..28ee7dd 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -295,6 +295,7 @@ struct hid_item {
 #define HID_GROUP_GENERIC			0x0001
 #define HID_GROUP_MULTITOUCH			0x0002
 #define HID_GROUP_SENSOR_HUB			0x0003
+#define HID_GROUP_MULTITOUCH_WIN_8		0x0004
 
 /*
  * This is the global environment of the parser. This information is
@@ -535,6 +536,7 @@ static inline void hid_set_drvdata(struct hid_device *hdev, void *data)
 
 #define HID_FLAG_MULTITOUCH		0x0001
 #define HID_FLAG_SENSOR_HUB		0x0002
+#define HID_FLAG_WIN_8_CERTIFIED	0x0004
 
 struct hid_parser {
 	struct hid_global     global;
-- 
1.8.3.1

[PATCH 3/3] HID: do not init input reports for Win 8 multitouch devices

[Benjamin Tissoires]

Some multitouch screens do not like to be polled for input reports.
However, the Win8 spec says that all touches should be sent during
each report, making the initialization of reports unnecessary.
The Win7 spec is less precise, so do not use this for those devices.

Add the quirk HID_QUIRK_NO_INIT_INPUT_REPORTS so that we do not have to
introduce a quirk for each problematic device. This quirk makes the driver
behave the same way the Win 8 does. It actually retrieves the features,
but not the inputs.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-multitouch.c  | 12 ++++++++++++
 drivers/hid/usbhid/hid-core.c | 11 ++++++++---
 include/linux/hid.h           |  1 +
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c28ef86..ac28f08 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -951,6 +951,18 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	hdev->quirks |= HID_QUIRK_MULTI_INPUT;
 	hdev->quirks |= HID_QUIRK_NO_EMPTY_INPUT;
 
+	/*
+	 * Handle special quirks for Windows 8 certified devices.
+	 */
+	if (id->group == HID_GROUP_MULTITOUCH_WIN_8)
+		/*
+		 * Some multitouch screens do not like to be polled for input
+		 * reports. Fortunately, the Win8 spec says that all touches
+		 * should be sent during each report, making the initialization
+		 * of input reports unnecessary.
+		 */
+		hdev->quirks |= HID_QUIRK_NO_INIT_INPUT_REPORTS;
+
 	td = devm_kzalloc(&hdev->dev, sizeof(struct mt_device), GFP_KERNEL);
 	if (!td) {
 		dev_err(&hdev->dev, "cannot allocate multitouch data\n");
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index bd38cdf..44df131 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -750,12 +750,17 @@ void usbhid_init_reports(struct hid_device *hid)
 {
 	struct hid_report *report;
 	struct usbhid_device *usbhid = hid->driver_data;
+	struct hid_report_enum *report_enum;
 	int err, ret;
 
-	list_for_each_entry(report, &hid->report_enum[HID_INPUT_REPORT].report_list, list)
-		usbhid_submit_report(hid, report, USB_DIR_IN);
+	if (!(hid->quirks & HID_QUIRK_NO_INIT_INPUT_REPORTS)) {
+		report_enum = &hid->report_enum[HID_INPUT_REPORT];
+		list_for_each_entry(report, &report_enum->report_list, list)
+			usbhid_submit_report(hid, report, USB_DIR_IN);
+	}
 
-	list_for_each_entry(report, &hid->report_enum[HID_FEATURE_REPORT].report_list, list)
+	report_enum = &hid->report_enum[HID_FEATURE_REPORT];
+	list_for_each_entry(report, &report_enum->report_list, list)
 		usbhid_submit_report(hid, report, USB_DIR_IN);
 
 	err = 0;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 28ee7dd..d55f244 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -283,6 +283,7 @@ struct hid_item {
 #define HID_QUIRK_MULTI_INPUT			0x00000040
 #define HID_QUIRK_HIDINPUT_FORCE		0x00000080
 #define HID_QUIRK_NO_EMPTY_INPUT		0x00000100
+#define HID_QUIRK_NO_INIT_INPUT_REPORTS		0x00000200
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
 #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
 #define HID_QUIRK_NO_INIT_REPORTS		0x20000000
-- 
1.8.3.1

Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the report descriptors

[Alexander Holler]

Am 13.08.2013 16:58, schrieb Benjamin Tissoires:
> hid_scan_report() implements its own HID report descriptor parsing. It was
> fine until we added the SENSOR_HUB detection. It is going to be even worse
> 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.

Sorry, but I can't agree with your wording here.

If you look at the if expression I've added to support HID sensor hubs, 
you will notice that the first expression was a test for the usage page 
of sensor hubs. That wasn't the first test by accident, instead I've 
choosen the order of those 4 tests very carefully to make the impact on 
the existing parsing of other HID devices very small. So it might not 
have be pleasing to your eyes, but it was for sure an appropriate solution.

Anyway, I've tested your patch 1/3 on top of 3.11-rc5 here and the 
detection of sensor hubs doesn't work anymore with your patch applied. 
Up to now I only had a quick look at it, but it looks like the test in 
hid_scan_open_collection() isn't hit for my device.

Another problem is that I don't have any commercial sensor hub and I'm 
therefor not a very relvant as tester (I've implemented the firmware for 
my HID (sensor hub) device too). Therefor I've added Srinivas Pandruvada 
to cc, because he's the only one I know who has HID sensor hubs.

And, as said, I've implemented the other side here too, therefor I've 
added the descriptor I'm using below.

Regards,

Alexander Holler


--my-descriptor--
	0x05, 0x20, // HID_USAGE_PAGE_SENSOR
	0xa1, 0x01, // COLLECTION (Application)
		0x09, 0xa0, // HID_USAGE_SENSOR_CATEGORY_TIME
		0xa1, 0x00, // COLLECTION (Physical)
#ifndef USE_FULL_YEAR
			0x75, 0x08, // REPORT_SIZE (8 bits)
#endif
			0x95, 0x01, // REPORT_COUNT (1)
			0x85, HID_REPORT_ID_TIME, // REPORT_ID

			0x0a, 0x21, 0x05, // USAGE (Year)
#ifdef USE_FULL_YEAR
			0x75, 0x10, // REPORT_SIZE (16 bits)
#else
			0x15, 0x00, // LOGICAL_MINIMUM (0) (Range is currently not specified 
in HUTRR39)
			0x25, 0x63, // LOGICAL_MAXIMUM (99)
#endif
			0x65, 0x00, // UNIT (None)
			0x81, 0x02, // INPUT (Data,Var,Abs)

			0x0a, 0x22, 0x05, // USAGE (Month)
#ifdef USE_FULL_YEAR
			0x75, 0x08, // REPORT_SIZE (8 bits)
#endif
			0x15, 0x01, // LOGICAL_MINIMUM (1)
			0x25, 0x0c, // LOGICAL_MAXIMUM (12)
			0x65, 0x00, // UNIT (None)
			0x81, 0x02, // INPUT (Data,Var,Abs)

			0x0a, 0x23, 0x05, // USAGE (Day)
			0x15, 0x01, // LOGICAL_MINIMUM (1)
			0x25, 0x1f, // LOGICAL_MAXIMUM (31)
			0x65, 0x00, // UNIT (None)
			0x81, 0x02, // INPUT (Data,Var,Abs)

			0x0a, 0x24, 0x05, // USAGE (Day of Week)
			0x15, 0x00, // LOGICAL_MINIMUM (0)
			0x25, 0x06, // LOGICAL_MAXIMUM (6)
			0xa1, 0x02, // COLLECTION (Logical)
				0x0a, 0xc0, 0x08, // Day of Week: Sunday
				0x0a, 0xc1, 0x08, // Day of Week: Monday
				0x0a, 0xc2, 0x08, // Day of Week: Tuesday
				0x0a, 0xc3, 0x08, // Day of Week: Wednesday
				0x0a, 0xc4, 0x08, // Day of Week: Thursday
				0x0a, 0xc5, 0x08, // Day of Week: Friday
				0x0a, 0xc6, 0x08, // Day of Week: Saturday
				0x81, 0x02, // INPUT (Const,Arr,Abs)
			0xc0, // END_COLLECTION

			0x0a, 0x25, 0x05, // USAGE (Hour)
			0x15, 0x00, // LOGICAL_MINIMUM (0)
			0x25, 0x17, // LOGICAL_MAXIMUM (23)
			0x65, 0x00, // UNIT (None)
			0x81, 0x02, // INPUT (Data,Var,Abs)

			0x0a, 0x26, 0x05, // USAGE (Minute)
			0x15, 0x00, // LOGICAL_MINIMUM (0)
			0x25, 0x3b, // LOGICAL_MAXIMUM (59)
			0x65, 0x00, // UNIT (None)
			0x81, 0x02, // INPUT (Data,Var,Abs)

			0x0a, 0x27, 0x05, // USAGE (Second)
			0x15, 0x00, // LOGICAL_MINIMUM (0)
			0x25, 0x3b, // LOGICAL_MAXIMUM (59)
			0x65, 0x00, // UNIT (None)
			//0x66, 0x10, 0x01, // UNIT (second)
			0x81, 0x02, // INPUT (Data,Var,Abs)

			0x0a, 0x28, 0x05, // USAGE (Millisecond)
			0x75, 0x10, // REPORT_SIZE (16 bits)
			0x65, 0x00, // UNIT (None)
			0x81, 0x02, // INPUT (Data,Var,Abs)

		0xc0, // END_COLLECTION
	0xc0, // END_COLLECTION
--my-descriptor--

>
> 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.
>
> Instead of directly overwriting the ->group field, this patch introduce
> a ->flags field and then decide which group the device belongs to,
> depending on the whole parsing (not just the local item). This will be
> useful for Win 8 multitouch devices, which are multitouch devices and
> Win 8 certified (so 2 flags to check).
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>   drivers/hid/hid-core.c | 131 +++++++++++++++++++++++++++++++++++--------------
>   include/linux/hid.h    |   4 ++
>   2 files changed, 97 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 3efe19f..d8cdb0a 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -677,10 +677,49 @@ 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)
>   {
>   	if (usage == HID_DG_CONTACTID)
> -		hid->group = HID_GROUP_MULTITOUCH;
> +		parser->flags |= HID_FLAG_MULTITOUCH;
> +}
> +
> +static void hid_scan_open_collection(struct hid_parser *parser, unsigned type)
> +{
> +	if (parser->global.usage_page == HID_UP_SENSOR &&
> +	    type == HID_COLLECTION_PHYSICAL)
> +		parser->flags |= HID_FLAG_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_open_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);
> +	}
> +
> +	/* Reset the local parser environment */
> +	memset(&parser->local, 0, sizeof(parser->local));
> +
> +	return 0;
>   }
>
>   /*
> @@ -690,49 +729,65 @@ 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;
> +	int ret;
> +	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;
> +	parser = vzalloc(sizeof(struct hid_parser));
> +	if (!parser)
> +		return -ENOMEM;
> +
> +	parser->device = hid;
> +
> +	ret = -EINVAL;
>   	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;
> +
> +		if (item.format != HID_ITEM_FORMAT_SHORT) {
> +			hid_err(hid, "unexpected long global item\n");
> +			goto out;
> +		}
> +
> +		if (dispatch_type[item.type](parser, &item)) {
> +			hid_err(hid, "item %u %u %u %u parsing failed\n",
> +				item.format, (unsigned)item.size,
> +				(unsigned)item.type, (unsigned)item.tag);
> +			goto out;
> +		}
> +
> +		if (start == end) {
> +			if (parser->local.delimiter_depth) {
> +				hid_err(hid, "unbalanced delimiter at end of report description\n");
> +				goto out;
>   			}
> -		} 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;
> +			ret = 0;
> +			goto out;
> +		}
>   	}
>
> -	return 0;
> +	hid_err(hid, "item fetching failed at offset %d\n", (int)(end - start));
> +out:
> +	switch (parser->flags) {
> +	case HID_FLAG_MULTITOUCH:
> +		hid->group = HID_GROUP_MULTITOUCH;
> +		break;
> +	case HID_FLAG_SENSOR_HUB:
> +		hid->group = HID_GROUP_SENSOR_HUB;
> +		break;
> +	default:
> +		hid->group = HID_GROUP_GENERIC;
> +	}
> +
> +	vfree(parser);
> +	return ret;
>   }
>
>   /**
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 5a4e789..7d823db 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -533,6 +533,9 @@ static inline void hid_set_drvdata(struct hid_device *hdev, void *data)
>   #define HID_GLOBAL_STACK_SIZE 4
>   #define HID_COLLECTION_STACK_SIZE 4
>
> +#define HID_FLAG_MULTITOUCH		0x0001
> +#define HID_FLAG_SENSOR_HUB		0x0002
> +
>   struct hid_parser {
>   	struct hid_global     global;
>   	struct hid_global     global_stack[HID_GLOBAL_STACK_SIZE];
> @@ -541,6 +544,7 @@ struct hid_parser {
>   	unsigned              collection_stack[HID_COLLECTION_STACK_SIZE];
>   	unsigned              collection_stack_ptr;
>   	struct hid_device    *device;
> +	unsigned              flags;
>   };
>
>   struct hid_class_descriptor {
>

Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the report descriptors

[Benjamin Tissoires]

On Tue, Aug 13, 2013 at 8:37 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 13.08.2013 16:58, schrieb Benjamin Tissoires:
>
>> hid_scan_report() implements its own HID report descriptor parsing. It was
>> fine until we added the SENSOR_HUB detection. It is going to be even worse
>> 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.
>
>
> Sorry, but I can't agree with your wording here.
>
> If you look at the if expression I've added to support HID sensor hubs, you
> will notice that the first expression was a test for the usage page of
> sensor hubs. That wasn't the first test by accident, instead I've choosen
> the order of those 4 tests very carefully to make the impact on the existing
> parsing of other HID devices very small. So it might not have be pleasing to
> your eyes, but it was for sure an appropriate solution.

I'm sorry if you feel like it was a personal attack. I'm sure the
upstream code is working good and that there is no extra tests and
that you implemented it that way without proper testing and
validating. What I meant was that the initial code was not exactly
meant to address the particular problem of sensor hub (at that time,
we only needed to check for one input report). However, I need now to
add a test against a feature report, which would need a new custom
detection, and a heavy modification of the existing code.

>
> Anyway, I've tested your patch 1/3 on top of 3.11-rc5 here and the detection
> of sensor hubs doesn't work anymore with your patch applied. Up to now I
> only had a quick look at it, but it looks like the test in
> hid_scan_open_collection() isn't hit for my device.

oops, this is not intentional. I'll try to fix this in v2.

>
> Another problem is that I don't have any commercial sensor hub and I'm
> therefor not a very relvant as tester (I've implemented the firmware for my
> HID (sensor hub) device too). Therefor I've added Srinivas Pandruvada to cc,
> because he's the only one I know who has HID sensor hubs.

It is not a problem if you don't have a commercial sensor hub :)
Thanks for adding Srinivas in CC.

>
> And, as said, I've implemented the other side here too, therefor I've added
> the descriptor I'm using below.
>

Thanks, this will help me to test against your report descriptors.
Can I also ask you to send me some hid-recorder[1] traces of your
sensor? With hid-replay, I can then re-inject them in the hid
subsystem, and then I include the results in a regression test suite.

Cheers,
Benjamin

[1] http://bentiss.github.io/hid-replay-docs/

>
> --my-descriptor--
>         0x05, 0x20, // HID_USAGE_PAGE_SENSOR
>         0xa1, 0x01, // COLLECTION (Application)
>                 0x09, 0xa0, // HID_USAGE_SENSOR_CATEGORY_TIME
>                 0xa1, 0x00, // COLLECTION (Physical)
> #ifndef USE_FULL_YEAR
>                         0x75, 0x08, // REPORT_SIZE (8 bits)
> #endif
>                         0x95, 0x01, // REPORT_COUNT (1)
>                         0x85, HID_REPORT_ID_TIME, // REPORT_ID
>
>                         0x0a, 0x21, 0x05, // USAGE (Year)
> #ifdef USE_FULL_YEAR
>                         0x75, 0x10, // REPORT_SIZE (16 bits)
> #else
>                         0x15, 0x00, // LOGICAL_MINIMUM (0) (Range is
> currently not specified in HUTRR39)
>                         0x25, 0x63, // LOGICAL_MAXIMUM (99)
> #endif
>                         0x65, 0x00, // UNIT (None)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                         0x0a, 0x22, 0x05, // USAGE (Month)
> #ifdef USE_FULL_YEAR
>                         0x75, 0x08, // REPORT_SIZE (8 bits)
> #endif
>                         0x15, 0x01, // LOGICAL_MINIMUM (1)
>                         0x25, 0x0c, // LOGICAL_MAXIMUM (12)
>                         0x65, 0x00, // UNIT (None)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                         0x0a, 0x23, 0x05, // USAGE (Day)
>                         0x15, 0x01, // LOGICAL_MINIMUM (1)
>                         0x25, 0x1f, // LOGICAL_MAXIMUM (31)
>                         0x65, 0x00, // UNIT (None)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                         0x0a, 0x24, 0x05, // USAGE (Day of Week)
>                         0x15, 0x00, // LOGICAL_MINIMUM (0)
>                         0x25, 0x06, // LOGICAL_MAXIMUM (6)
>                         0xa1, 0x02, // COLLECTION (Logical)
>                                 0x0a, 0xc0, 0x08, // Day of Week: Sunday
>                                 0x0a, 0xc1, 0x08, // Day of Week: Monday
>                                 0x0a, 0xc2, 0x08, // Day of Week: Tuesday
>                                 0x0a, 0xc3, 0x08, // Day of Week: Wednesday
>                                 0x0a, 0xc4, 0x08, // Day of Week: Thursday
>                                 0x0a, 0xc5, 0x08, // Day of Week: Friday
>                                 0x0a, 0xc6, 0x08, // Day of Week: Saturday
>                                 0x81, 0x02, // INPUT (Const,Arr,Abs)
>                         0xc0, // END_COLLECTION
>
>                         0x0a, 0x25, 0x05, // USAGE (Hour)
>                         0x15, 0x00, // LOGICAL_MINIMUM (0)
>                         0x25, 0x17, // LOGICAL_MAXIMUM (23)
>                         0x65, 0x00, // UNIT (None)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                         0x0a, 0x26, 0x05, // USAGE (Minute)
>                         0x15, 0x00, // LOGICAL_MINIMUM (0)
>                         0x25, 0x3b, // LOGICAL_MAXIMUM (59)
>                         0x65, 0x00, // UNIT (None)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                         0x0a, 0x27, 0x05, // USAGE (Second)
>                         0x15, 0x00, // LOGICAL_MINIMUM (0)
>                         0x25, 0x3b, // LOGICAL_MAXIMUM (59)
>                         0x65, 0x00, // UNIT (None)
>                         //0x66, 0x10, 0x01, // UNIT (second)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                         0x0a, 0x28, 0x05, // USAGE (Millisecond)
>                         0x75, 0x10, // REPORT_SIZE (16 bits)
>                         0x65, 0x00, // UNIT (None)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                 0xc0, // END_COLLECTION
>         0xc0, // END_COLLECTION
> --my-descriptor--
>
>

Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the report descriptors

[rydberg]

Hi Benjamin,

thanks for the patches, things are looking a lot better this way.

> hid_scan_report() implements its own HID report descriptor parsing. It was
> fine until we added the SENSOR_HUB detection. It is going to be even worse
> 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.

It was fine with sensors added as well. You seem to have found a
reasonable way to add support for all the tags, but there is a
rationale for the current scanner that may not have been addressed in
this patch: it is robust against parse errors. This is particularly
important for devices which later tweak the report, often in order to
parse properly.

Please find some further comments inline.

> 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.
> 
> Instead of directly overwriting the ->group field, this patch introduce
> a ->flags field and then decide which group the device belongs to,
> depending on the whole parsing (not just the local item). This will be
> useful for Win 8 multitouch devices, which are multitouch devices and
> Win 8 certified (so 2 flags to check).
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-core.c | 131 +++++++++++++++++++++++++++++++++++--------------
>  include/linux/hid.h    |   4 ++
>  2 files changed, 97 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 3efe19f..d8cdb0a 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -677,10 +677,49 @@ 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)
>  {
>  	if (usage == HID_DG_CONTACTID)
> -		hid->group = HID_GROUP_MULTITOUCH;
> +		parser->flags |= HID_FLAG_MULTITOUCH;

Did you consider reusing the group flags, e.g., parser->groups |= (1
<< HID_GROUP_MULTITOUCH)? This change could be made regardless of the
parser logic.

> +}
> +
> +static void hid_scan_open_collection(struct hid_parser *parser, unsigned type)

We are not really opening anything here, so perhaps
hid_scan_collection would suffice.

> +{
> +	if (parser->global.usage_page == HID_UP_SENSOR &&
> +	    type == HID_COLLECTION_PHYSICAL)
> +		parser->flags |= HID_FLAG_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_open_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);
> +	}
> +
> +	/* Reset the local parser environment */
> +	memset(&parser->local, 0, sizeof(parser->local));
> +
> +	return 0;
>  }
>  
>  /*
> @@ -690,49 +729,65 @@ 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;
> +	int ret;
> +	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;
> +	parser = vzalloc(sizeof(struct hid_parser));

Argh, I realize it is inevitable for this patch, but it still makes my
eyes bleed. The parser takes quite a bit of memory...

> +	if (!parser)
> +		return -ENOMEM;
> +
> +	parser->device = hid;
> +
> +	ret = -EINVAL;
>  	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;
> +
> +		if (item.format != HID_ITEM_FORMAT_SHORT) {
> +			hid_err(hid, "unexpected long global item\n");

I do not think we should be verbose on errors during scan, for the
reason stated at the top. Since this goes also for the global parser
functions, we might have a problem.

> +			goto out;
> +		}
> +
> +		if (dispatch_type[item.type](parser, &item)) {
> +			hid_err(hid, "item %u %u %u %u parsing failed\n",
> +				item.format, (unsigned)item.size,
> +				(unsigned)item.type, (unsigned)item.tag);

Ditto.

> +			goto out;
> +		}
> +
> +		if (start == end) {
> +			if (parser->local.delimiter_depth) {
> +				hid_err(hid, "unbalanced delimiter at end of report description\n");

Robustness, see top.

> +				goto out;
>  			}
> -		} 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;

At the end of the day, It may be best to simply extend this branch as
the main item type and add whatever you need to detect win8 from
there.

> +			ret = 0;
> +			goto out;
> +		}
>  	}
>  
> -	return 0;
> +	hid_err(hid, "item fetching failed at offset %d\n", (int)(end - start));
> +out:
> +	switch (parser->flags) {
> +	case HID_FLAG_MULTITOUCH:
> +		hid->group = HID_GROUP_MULTITOUCH;
> +		break;
> +	case HID_FLAG_SENSOR_HUB:
> +		hid->group = HID_GROUP_SENSOR_HUB;
> +		break;
> +	default:
> +		hid->group = HID_GROUP_GENERIC;
> +	}

Looks odd to switch on flags, but it works pretty well with the rest
of the patches in the series.

> +
> +	vfree(parser);
> +	return ret;
>  }
>  
>  /**
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 5a4e789..7d823db 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -533,6 +533,9 @@ static inline void hid_set_drvdata(struct hid_device *hdev, void *data)
>  #define HID_GLOBAL_STACK_SIZE 4
>  #define HID_COLLECTION_STACK_SIZE 4
>  
> +#define HID_FLAG_MULTITOUCH		0x0001
> +#define HID_FLAG_SENSOR_HUB		0x0002
> +
>  struct hid_parser {
>  	struct hid_global     global;
>  	struct hid_global     global_stack[HID_GLOBAL_STACK_SIZE];
> @@ -541,6 +544,7 @@ struct hid_parser {
>  	unsigned              collection_stack[HID_COLLECTION_STACK_SIZE];
>  	unsigned              collection_stack_ptr;
>  	struct hid_device    *device;
> +	unsigned              flags;
>  };
>  
>  struct hid_class_descriptor {
> -- 
> 1.8.3.1
> 

Thanks,
Henrik

Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the report descriptors

[Alexander Holler]

Am 13.08.2013 21:15, schrieb Benjamin Tissoires:
> On Tue, Aug 13, 2013 at 8:37 PM, Alexander Holler <holler@ahsoftware.de> wrote:

>>
>> Another problem is that I don't have any commercial sensor hub and I'm
>> therefor not a very relvant as tester (I've implemented the firmware for my
>> HID (sensor hub) device too). Therefor I've added Srinivas Pandruvada to cc,
>> because he's the only one I know who has HID sensor hubs.
>
> It is not a problem if you don't have a commercial sensor hub :)
> Thanks for adding Srinivas in CC.

The problem is that this invalidates me as a tester because everything I 
can test is something untested. As I've written both ends of the pipe, I 
could have understood something totally wrong but as I've implemented 
both sides (the device- and the kernel-part) it still might work without 
problems. Therefor I currently have to bother Srinivas whenever I do 
changes in the kernel in regard to hid-sensor-hub. ;)

>>
>> And, as said, I've implemented the other side here too, therefor I've added
>> the descriptor I'm using below.
>>
>
> Thanks, this will help me to test against your report descriptors.
> Can I also ask you to send me some hid-recorder[1] traces of your
> sensor? With hid-replay, I can then re-inject them in the hid
> subsystem, and then I include the results in a regression test suite.

Unfortunately hid-record doesn't list my device because it doesn't show 
up as an hidraw-device.

But maybe you have an Arduino Leonardo or a clone. If so you can get the 
description and firmware here:

http://ahsoftware.de/dcf77-hid-usb-rtc/

Regards,

Alexander Holler

Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the report descriptors

[Benjamin Tissoires]

On 13/08/13 20:37, Alexander Holler wrote:
> Am 13.08.2013 16:58, schrieb Benjamin Tissoires:
>> hid_scan_report() implements its own HID report descriptor parsing. It
>> was
>> fine until we added the SENSOR_HUB detection. It is going to be even
>> worse
>> 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.
> 
> Sorry, but I can't agree with your wording here.
> 
> If you look at the if expression I've added to support HID sensor hubs,
> you will notice that the first expression was a test for the usage page
> of sensor hubs. That wasn't the first test by accident, instead I've
> choosen the order of those 4 tests very carefully to make the impact on
> the existing parsing of other HID devices very small. So it might not
> have be pleasing to your eyes, but it was for sure an appropriate solution.
> 
> Anyway, I've tested your patch 1/3 on top of 3.11-rc5 here and the
> detection of sensor hubs doesn't work anymore with your patch applied.
> Up to now I only had a quick look at it, but it looks like the test in
> hid_scan_open_collection() isn't hit for my device.

Ok, I found the culprit (see below). Thanks for providing your report
descriptors (and sources), I managed to test the detection of your device.


> 
> Another problem is that I don't have any commercial sensor hub and I'm
> therefor not a very relvant as tester (I've implemented the firmware for
> my HID (sensor hub) device too). Therefor I've added Srinivas Pandruvada
> to cc, because he's the only one I know who has HID sensor hubs.
> 
> And, as said, I've implemented the other side here too, therefor I've
> added the descriptor I'm using below.
> 
> Regards,
> 
> Alexander Holler
> 
> 
> --my-descriptor--
>     0x05, 0x20, // HID_USAGE_PAGE_SENSOR
>     0xa1, 0x01, // COLLECTION (Application)
>         0x09, 0xa0, // HID_USAGE_SENSOR_CATEGORY_TIME
>         0xa1, 0x00, // COLLECTION (Physical)
> #ifndef USE_FULL_YEAR
>             0x75, 0x08, // REPORT_SIZE (8 bits)
> #endif
>             0x95, 0x01, // REPORT_COUNT (1)
>             0x85, HID_REPORT_ID_TIME, // REPORT_ID
> 
>             0x0a, 0x21, 0x05, // USAGE (Year)
> #ifdef USE_FULL_YEAR
>             0x75, 0x10, // REPORT_SIZE (16 bits)
> #else
>             0x15, 0x00, // LOGICAL_MINIMUM (0) (Range is currently not
> specified in HUTRR39)
>             0x25, 0x63, // LOGICAL_MAXIMUM (99)
> #endif
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>             0x0a, 0x22, 0x05, // USAGE (Month)
> #ifdef USE_FULL_YEAR
>             0x75, 0x08, // REPORT_SIZE (8 bits)
> #endif
>             0x15, 0x01, // LOGICAL_MINIMUM (1)
>             0x25, 0x0c, // LOGICAL_MAXIMUM (12)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>             0x0a, 0x23, 0x05, // USAGE (Day)
>             0x15, 0x01, // LOGICAL_MINIMUM (1)
>             0x25, 0x1f, // LOGICAL_MAXIMUM (31)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>             0x0a, 0x24, 0x05, // USAGE (Day of Week)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x06, // LOGICAL_MAXIMUM (6)
>             0xa1, 0x02, // COLLECTION (Logical)
>                 0x0a, 0xc0, 0x08, // Day of Week: Sunday
>                 0x0a, 0xc1, 0x08, // Day of Week: Monday
>                 0x0a, 0xc2, 0x08, // Day of Week: Tuesday
>                 0x0a, 0xc3, 0x08, // Day of Week: Wednesday
>                 0x0a, 0xc4, 0x08, // Day of Week: Thursday
>                 0x0a, 0xc5, 0x08, // Day of Week: Friday
>                 0x0a, 0xc6, 0x08, // Day of Week: Saturday
>                 0x81, 0x02, // INPUT (Const,Arr,Abs)
>             0xc0, // END_COLLECTION
> 
>             0x0a, 0x25, 0x05, // USAGE (Hour)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x17, // LOGICAL_MAXIMUM (23)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>             0x0a, 0x26, 0x05, // USAGE (Minute)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x3b, // LOGICAL_MAXIMUM (59)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>             0x0a, 0x27, 0x05, // USAGE (Second)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x3b, // LOGICAL_MAXIMUM (59)
>             0x65, 0x00, // UNIT (None)
>             //0x66, 0x10, 0x01, // UNIT (second)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>             0x0a, 0x28, 0x05, // USAGE (Millisecond)
>             0x75, 0x10, // REPORT_SIZE (16 bits)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>         0xc0, // END_COLLECTION
>     0xc0, // END_COLLECTION
> --my-descriptor--
> 
>>
>> 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.
>>
>> Instead of directly overwriting the ->group field, this patch introduce
>> a ->flags field and then decide which group the device belongs to,
>> depending on the whole parsing (not just the local item). This will be
>> useful for Win 8 multitouch devices, which are multitouch devices and
>> Win 8 certified (so 2 flags to check).
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>   drivers/hid/hid-core.c | 131
>> +++++++++++++++++++++++++++++++++++--------------
>>   include/linux/hid.h    |   4 ++
>>   2 files changed, 97 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 3efe19f..d8cdb0a 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -677,10 +677,49 @@ 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)
>>   {
>>       if (usage == HID_DG_CONTACTID)
>> -        hid->group = HID_GROUP_MULTITOUCH;
>> +        parser->flags |= HID_FLAG_MULTITOUCH;
>> +}
>> +
>> +static void hid_scan_open_collection(struct hid_parser *parser,
>> unsigned type)
>> +{
>> +    if (parser->global.usage_page == HID_UP_SENSOR &&

HID_UP_SENSOR is actually 0x00200000, and parser->global.usage_page
contains the raw value 0x0020... Moving HID_UP_SENSOR 16 bits to the
right makes the test working, and your sensor device correctly tagged.

Cheers,
Benjamin

>> +        type == HID_COLLECTION_PHYSICAL)
>> +        parser->flags |= HID_FLAG_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_open_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);
>> +    }
>> +
>> +    /* Reset the local parser environment */
>> +    memset(&parser->local, 0, sizeof(parser->local));
>> +
>> +    return 0;
>>   }
>>
>>   /*
>> @@ -690,49 +729,65 @@ 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;
>> +    int ret;
>> +    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;
>> +    parser = vzalloc(sizeof(struct hid_parser));
>> +    if (!parser)
>> +        return -ENOMEM;
>> +
>> +    parser->device = hid;
>> +
>> +    ret = -EINVAL;
>>       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;
>> +
>> +        if (item.format != HID_ITEM_FORMAT_SHORT) {
>> +            hid_err(hid, "unexpected long global item\n");
>> +            goto out;
>> +        }
>> +
>> +        if (dispatch_type[item.type](parser, &item)) {
>> +            hid_err(hid, "item %u %u %u %u parsing failed\n",
>> +                item.format, (unsigned)item.size,
>> +                (unsigned)item.type, (unsigned)item.tag);
>> +            goto out;
>> +        }
>> +
>> +        if (start == end) {
>> +            if (parser->local.delimiter_depth) {
>> +                hid_err(hid, "unbalanced delimiter at end of report
>> description\n");
>> +                goto out;
>>               }
>> -        } 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;
>> +            ret = 0;
>> +            goto out;
>> +        }
>>       }
>>
>> -    return 0;
>> +    hid_err(hid, "item fetching failed at offset %d\n", (int)(end -
>> start));
>> +out:
>> +    switch (parser->flags) {
>> +    case HID_FLAG_MULTITOUCH:
>> +        hid->group = HID_GROUP_MULTITOUCH;
>> +        break;
>> +    case HID_FLAG_SENSOR_HUB:
>> +        hid->group = HID_GROUP_SENSOR_HUB;
>> +        break;
>> +    default:
>> +        hid->group = HID_GROUP_GENERIC;
>> +    }
>> +
>> +    vfree(parser);
>> +    return ret;
>>   }
>>
>>   /**
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 5a4e789..7d823db 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -533,6 +533,9 @@ static inline void hid_set_drvdata(struct
>> hid_device *hdev, void *data)
>>   #define HID_GLOBAL_STACK_SIZE 4
>>   #define HID_COLLECTION_STACK_SIZE 4
>>
>> +#define HID_FLAG_MULTITOUCH        0x0001
>> +#define HID_FLAG_SENSOR_HUB        0x0002
>> +
>>   struct hid_parser {
>>       struct hid_global     global;
>>       struct hid_global     global_stack[HID_GLOBAL_STACK_SIZE];
>> @@ -541,6 +544,7 @@ struct hid_parser {
>>       unsigned              collection_stack[HID_COLLECTION_STACK_SIZE];
>>       unsigned              collection_stack_ptr;
>>       struct hid_device    *device;
>> +    unsigned              flags;
>>   };
>>
>>   struct hid_class_descriptor {
>>
> 

Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the report descriptors

[Benjamin Tissoires]

Hi Henrik,

On 13/08/13 21:17, rydberg@euromail.se wrote:
> Hi Benjamin,
> 
> thanks for the patches, things are looking a lot better this way.

thanks for the review :)

> 
>> hid_scan_report() implements its own HID report descriptor parsing. It was
>> fine until we added the SENSOR_HUB detection. It is going to be even worse
>> 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.
> 
> It was fine with sensors added as well. You seem to have found a
> reasonable way to add support for all the tags, but there is a
> rationale for the current scanner that may not have been addressed in
> this patch: it is robust against parse errors. This is particularly
> important for devices which later tweak the report, often in order to
> parse properly.

Hmm, I disagree: if a driver needs to tweak a report, then it will
register itself in hid_have_special_driver. The pre-scanning pass is
done only for devices which are not in hid_have_special_driver. So I
consider it is safe to assume that the report descriptor does not
contain errors. If it does, then it will just leave the current
hid-group state, and will just pop an debug information.

> 
> Please find some further comments inline.
> 
>> 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.
>>
>> Instead of directly overwriting the ->group field, this patch introduce
>> a ->flags field and then decide which group the device belongs to,
>> depending on the whole parsing (not just the local item). This will be
>> useful for Win 8 multitouch devices, which are multitouch devices and
>> Win 8 certified (so 2 flags to check).
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>  drivers/hid/hid-core.c | 131 +++++++++++++++++++++++++++++++++++--------------
>>  include/linux/hid.h    |   4 ++
>>  2 files changed, 97 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 3efe19f..d8cdb0a 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -677,10 +677,49 @@ 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)
>>  {
>>  	if (usage == HID_DG_CONTACTID)
>> -		hid->group = HID_GROUP_MULTITOUCH;
>> +		parser->flags |= HID_FLAG_MULTITOUCH;
> 
> Did you consider reusing the group flags, e.g., parser->groups |= (1
> << HID_GROUP_MULTITOUCH)? This change could be made regardless of the
> parser logic.

If nobody is against changing the values of the different groups across
kernel version (which should be harmless), then I fully agree, we can
use the group item as a bit field (but we would be able to only have 16
different device groups).

> 
>> +}
>> +
>> +static void hid_scan_open_collection(struct hid_parser *parser, unsigned type)
> 
> We are not really opening anything here, so perhaps
> hid_scan_collection would suffice.

Ok, will change in v2

> 
>> +{
>> +	if (parser->global.usage_page == HID_UP_SENSOR &&
>> +	    type == HID_COLLECTION_PHYSICAL)
>> +		parser->flags |= HID_FLAG_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_open_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);
>> +	}
>> +
>> +	/* Reset the local parser environment */
>> +	memset(&parser->local, 0, sizeof(parser->local));
>> +
>> +	return 0;
>>  }
>>  
>>  /*
>> @@ -690,49 +729,65 @@ 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;
>> +	int ret;
>> +	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;
>> +	parser = vzalloc(sizeof(struct hid_parser));
> 
> Argh, I realize it is inevitable for this patch, but it still makes my
> eyes bleed. The parser takes quite a bit of memory...

Yep, my first attempt was to remove it, then I re-added it with a small
tear...

> 
>> +	if (!parser)
>> +		return -ENOMEM;
>> +
>> +	parser->device = hid;
>> +
>> +	ret = -EINVAL;
>>  	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;
>> +
>> +		if (item.format != HID_ITEM_FORMAT_SHORT) {
>> +			hid_err(hid, "unexpected long global item\n");
> 
> I do not think we should be verbose on errors during scan, for the
> reason stated at the top. Since this goes also for the global parser
> functions, we might have a problem.

yes, I kept it for documenting purposes, but this will double the amount
of logs in the kernel debug output. I'll just convert them into comments
in v2.

> 
>> +			goto out;
>> +		}
>> +
>> +		if (dispatch_type[item.type](parser, &item)) {
>> +			hid_err(hid, "item %u %u %u %u parsing failed\n",
>> +				item.format, (unsigned)item.size,
>> +				(unsigned)item.type, (unsigned)item.tag);
> 
> Ditto.

Ditto

> 
>> +			goto out;
>> +		}
>> +
>> +		if (start == end) {
>> +			if (parser->local.delimiter_depth) {
>> +				hid_err(hid, "unbalanced delimiter at end of report description\n");
> 
> Robustness, see top.
> 
>> +				goto out;
>>  			}
>> -		} 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;
> 
> At the end of the day, It may be best to simply extend this branch as
> the main item type and add whatever you need to detect win8 from
> there.
> 

I don't think this would be that simple:
- the usage is given by the HID_ITEM_TYPE_LOCAL branch
- I also need to check the fact that the usage is used as a feature:
this would require setting temporary flags and involve a logic which
would not be very easy to understand.

Furthermore, this makes me think that if a device describes in the
reports a ContactID as a feature, then the current parsing will say that
this is a multitouch device, whereas it should not. (ok, this has no
reasons to appear because it would be dumb, but still...)

>> +			ret = 0;
>> +			goto out;
>> +		}
>>  	}
>>  
>> -	return 0;
>> +	hid_err(hid, "item fetching failed at offset %d\n", (int)(end - start));
>> +out:
>> +	switch (parser->flags) {
>> +	case HID_FLAG_MULTITOUCH:
>> +		hid->group = HID_GROUP_MULTITOUCH;
>> +		break;
>> +	case HID_FLAG_SENSOR_HUB:
>> +		hid->group = HID_GROUP_SENSOR_HUB;
>> +		break;
>> +	default:
>> +		hid->group = HID_GROUP_GENERIC;
>> +	}
> 
> Looks odd to switch on flags, but it works pretty well with the rest
> of the patches in the series.

Yep, it's odd. If nobody is against changing the current groups
definitions, we can just keep the group as a bitfield, and it will work
(we just need to sanitize the group at the end).

Cheers,
Benjamin

> 
>> +
>> +	vfree(parser);
>> +	return ret;
>>  }
>>  
>>  /**
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 5a4e789..7d823db 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -533,6 +533,9 @@ static inline void hid_set_drvdata(struct hid_device *hdev, void *data)
>>  #define HID_GLOBAL_STACK_SIZE 4
>>  #define HID_COLLECTION_STACK_SIZE 4
>>  
>> +#define HID_FLAG_MULTITOUCH		0x0001
>> +#define HID_FLAG_SENSOR_HUB		0x0002
>> +
>>  struct hid_parser {
>>  	struct hid_global     global;
>>  	struct hid_global     global_stack[HID_GLOBAL_STACK_SIZE];
>> @@ -541,6 +544,7 @@ struct hid_parser {
>>  	unsigned              collection_stack[HID_COLLECTION_STACK_SIZE];
>>  	unsigned              collection_stack_ptr;
>>  	struct hid_device    *device;
>> +	unsigned              flags;
>>  };
>>  
>>  struct hid_class_descriptor {
>> -- 
>> 1.8.3.1
>>
> 
> Thanks,
> Henrik
> 

Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the report descriptors

[Srinivas Pandruvada]

On 08/13/2013 11:37 AM, Alexander Holler wrote:
> Am 13.08.2013 16:58, schrieb Benjamin Tissoires:
>> hid_scan_report() implements its own HID report descriptor parsing. 
>> It was
>> fine until we added the SENSOR_HUB detection. It is going to be even 
>> worse
>> 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.
>
> Sorry, but I can't agree with your wording here.
>
> If you look at the if expression I've added to support HID sensor 
> hubs, you will notice that the first expression was a test for the 
> usage page of sensor hubs. That wasn't the first test by accident, 
> instead I've choosen the order of those 4 tests very carefully to make 
> the impact on the existing parsing of other HID devices very small. So 
> it might not have be pleasing to your eyes, but it was for sure an 
> appropriate solution.
>
> Anyway, I've tested your patch 1/3 on top of 3.11-rc5 here and the 
> detection of sensor hubs doesn't work anymore with your patch applied. 
> Up to now I only had a quick look at it, but it looks like the test in 
> hid_scan_open_collection() isn't hit for my device.
>
> Another problem is that I don't have any commercial sensor hub and I'm 
> therefor not a very relvant as tester (I've implemented the firmware 
> for my HID (sensor hub) device too). Therefor I've added Srinivas 
> Pandruvada to cc, because he's the only one I know who has HID sensor 
> hubs.
>
Tested on 3.11-rc5. Sensor hubs are no longer detected. If you have some 
patch to fix this, I can give a try.
> And, as said, I've implemented the other side here too, therefor I've 
> added the descriptor I'm using below.
>
> Regards,
>
> Alexander Holler
>
>
> --my-descriptor--
>     0x05, 0x20, // HID_USAGE_PAGE_SENSOR
>     0xa1, 0x01, // COLLECTION (Application)
>         0x09, 0xa0, // HID_USAGE_SENSOR_CATEGORY_TIME
>         0xa1, 0x00, // COLLECTION (Physical)
> #ifndef USE_FULL_YEAR
>             0x75, 0x08, // REPORT_SIZE (8 bits)
> #endif
>             0x95, 0x01, // REPORT_COUNT (1)
>             0x85, HID_REPORT_ID_TIME, // REPORT_ID
>
>             0x0a, 0x21, 0x05, // USAGE (Year)
> #ifdef USE_FULL_YEAR
>             0x75, 0x10, // REPORT_SIZE (16 bits)
> #else
>             0x15, 0x00, // LOGICAL_MINIMUM (0) (Range is currently not 
> specified in HUTRR39)
>             0x25, 0x63, // LOGICAL_MAXIMUM (99)
> #endif
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>             0x0a, 0x22, 0x05, // USAGE (Month)
> #ifdef USE_FULL_YEAR
>             0x75, 0x08, // REPORT_SIZE (8 bits)
> #endif
>             0x15, 0x01, // LOGICAL_MINIMUM (1)
>             0x25, 0x0c, // LOGICAL_MAXIMUM (12)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>             0x0a, 0x23, 0x05, // USAGE (Day)
>             0x15, 0x01, // LOGICAL_MINIMUM (1)
>             0x25, 0x1f, // LOGICAL_MAXIMUM (31)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>             0x0a, 0x24, 0x05, // USAGE (Day of Week)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x06, // LOGICAL_MAXIMUM (6)
>             0xa1, 0x02, // COLLECTION (Logical)
>                 0x0a, 0xc0, 0x08, // Day of Week: Sunday
>                 0x0a, 0xc1, 0x08, // Day of Week: Monday
>                 0x0a, 0xc2, 0x08, // Day of Week: Tuesday
>                 0x0a, 0xc3, 0x08, // Day of Week: Wednesday
>                 0x0a, 0xc4, 0x08, // Day of Week: Thursday
>                 0x0a, 0xc5, 0x08, // Day of Week: Friday
>                 0x0a, 0xc6, 0x08, // Day of Week: Saturday
>                 0x81, 0x02, // INPUT (Const,Arr,Abs)
>             0xc0, // END_COLLECTION
>
>             0x0a, 0x25, 0x05, // USAGE (Hour)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x17, // LOGICAL_MAXIMUM (23)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>             0x0a, 0x26, 0x05, // USAGE (Minute)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x3b, // LOGICAL_MAXIMUM (59)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>             0x0a, 0x27, 0x05, // USAGE (Second)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x3b, // LOGICAL_MAXIMUM (59)
>             0x65, 0x00, // UNIT (None)
>             //0x66, 0x10, 0x01, // UNIT (second)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>             0x0a, 0x28, 0x05, // USAGE (Millisecond)
>             0x75, 0x10, // REPORT_SIZE (16 bits)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>         0xc0, // END_COLLECTION
>     0xc0, // END_COLLECTION
> --my-descriptor--
>
>>
>> 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.
>>
>> Instead of directly overwriting the ->group field, this patch introduce
>> a ->flags field and then decide which group the device belongs to,
>> depending on the whole parsing (not just the local item). This will be
>> useful for Win 8 multitouch devices, which are multitouch devices and
>> Win 8 certified (so 2 flags to check).
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>   drivers/hid/hid-core.c | 131 
>> +++++++++++++++++++++++++++++++++++--------------
>>   include/linux/hid.h    |   4 ++
>>   2 files changed, 97 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 3efe19f..d8cdb0a 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -677,10 +677,49 @@ 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)
>>   {
>>       if (usage == HID_DG_CONTACTID)
>> -        hid->group = HID_GROUP_MULTITOUCH;
>> +        parser->flags |= HID_FLAG_MULTITOUCH;
>> +}
>> +
>> +static void hid_scan_open_collection(struct hid_parser *parser, 
>> unsigned type)
>> +{
>> +    if (parser->global.usage_page == HID_UP_SENSOR &&
>> +        type == HID_COLLECTION_PHYSICAL)
>> +        parser->flags |= HID_FLAG_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_open_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);
>> +    }
>> +
>> +    /* Reset the local parser environment */
>> +    memset(&parser->local, 0, sizeof(parser->local));
>> +
>> +    return 0;
>>   }
>>
>>   /*
>> @@ -690,49 +729,65 @@ 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;
>> +    int ret;
>> +    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;
>> +    parser = vzalloc(sizeof(struct hid_parser));
>> +    if (!parser)
>> +        return -ENOMEM;
>> +
>> +    parser->device = hid;
>> +
>> +    ret = -EINVAL;
>>       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;
>> +
>> +        if (item.format != HID_ITEM_FORMAT_SHORT) {
>> +            hid_err(hid, "unexpected long global item\n");
>> +            goto out;
>> +        }
>> +
>> +        if (dispatch_type[item.type](parser, &item)) {
>> +            hid_err(hid, "item %u %u %u %u parsing failed\n",
>> +                item.format, (unsigned)item.size,
>> +                (unsigned)item.type, (unsigned)item.tag);
>> +            goto out;
>> +        }
>> +
>> +        if (start == end) {
>> +            if (parser->local.delimiter_depth) {
>> +                hid_err(hid, "unbalanced delimiter at end of report 
>> description\n");
>> +                goto out;
>>               }
>> -        } 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;
>> +            ret = 0;
>> +            goto out;
>> +        }
>>       }
>>
>> -    return 0;
>> +    hid_err(hid, "item fetching failed at offset %d\n", (int)(end - 
>> start));
>> +out:
>> +    switch (parser->flags) {
>> +    case HID_FLAG_MULTITOUCH:
>> +        hid->group = HID_GROUP_MULTITOUCH;
>> +        break;
>> +    case HID_FLAG_SENSOR_HUB:
>> +        hid->group = HID_GROUP_SENSOR_HUB;
>> +        break;
>> +    default:
>> +        hid->group = HID_GROUP_GENERIC;
>> +    }
>> +
>> +    vfree(parser);
>> +    return ret;
>>   }
>>
>>   /**
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 5a4e789..7d823db 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -533,6 +533,9 @@ static inline void hid_set_drvdata(struct 
>> hid_device *hdev, void *data)
>>   #define HID_GLOBAL_STACK_SIZE 4
>>   #define HID_COLLECTION_STACK_SIZE 4
>>
>> +#define HID_FLAG_MULTITOUCH        0x0001
>> +#define HID_FLAG_SENSOR_HUB        0x0002
>> +
>>   struct hid_parser {
>>       struct hid_global     global;
>>       struct hid_global     global_stack[HID_GLOBAL_STACK_SIZE];
>> @@ -541,6 +544,7 @@ struct hid_parser {
>>       unsigned collection_stack[HID_COLLECTION_STACK_SIZE];
>>       unsigned              collection_stack_ptr;
>>       struct hid_device    *device;
>> +    unsigned              flags;
>>   };
>>
>>   struct hid_class_descriptor {
>>
>
>
Thanks,
Srinivas

Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the report descriptors

[Alexander Holler]

Am 14.08.2013 17:38, schrieb Benjamin Tissoires:

>>>   {
>>>   	if (usage == HID_DG_CONTACTID)
>>> -		hid->group = HID_GROUP_MULTITOUCH;
>>> +		parser->flags |= HID_FLAG_MULTITOUCH;
>>
>> Did you consider reusing the group flags, e.g., parser->groups |= (1
>> << HID_GROUP_MULTITOUCH)? This change could be made regardless of the
>> parser logic.
>
> If nobody is against changing the values of the different groups across
> kernel version (which should be harmless), then I fully agree, we can
> use the group item as a bit field (but we would be able to only have 16
> different device groups).

Hmm, that might become a problem. E.g. all the HID sensors might be used 
stand alone (without a sensor-hub, if someone modifies the drivers). But 
I agree that currently the flags are just confusing and would introduce 
them only if the number of groups reaches the limit.


>>> -	hid->group = HID_GROUP_GENERIC;
>>> +	parser = vzalloc(sizeof(struct hid_parser));
>>
>> Argh, I realize it is inevitable for this patch, but it still makes my
>> eyes bleed. The parser takes quite a bit of memory...
>
> Yep, my first attempt was to remove it, then I re-added it with a small
> tear...

So you actually create a new parser and the subject (that existing) of 
this patch is misleading.

Regards,

Alexander Holler

Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the report descriptors

[Benjamin Tissoires]

On Wed, Aug 14, 2013 at 10:03 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 14.08.2013 17:38, schrieb Benjamin Tissoires:
>
>
>>>>   {
>>>>         if (usage == HID_DG_CONTACTID)
>>>> -               hid->group = HID_GROUP_MULTITOUCH;
>>>> +               parser->flags |= HID_FLAG_MULTITOUCH;
>>>
>>>
>>> Did you consider reusing the group flags, e.g., parser->groups |= (1
>>> << HID_GROUP_MULTITOUCH)? This change could be made regardless of the
>>> parser logic.
>>
>>
>> If nobody is against changing the values of the different groups across
>> kernel version (which should be harmless), then I fully agree, we can
>> use the group item as a bit field (but we would be able to only have 16
>> different device groups).
>
>
> Hmm, that might become a problem. E.g. all the HID sensors might be used
> stand alone (without a sensor-hub, if someone modifies the drivers). But I
> agree that currently the flags are just confusing and would introduce them
> only if the number of groups reaches the limit.
>
>
>
>>>> -       hid->group = HID_GROUP_GENERIC;
>>>> +       parser = vzalloc(sizeof(struct hid_parser));
>>>
>>>
>>> Argh, I realize it is inevitable for this patch, but it still makes my
>>> eyes bleed. The parser takes quite a bit of memory...
>>
>>
>> Yep, my first attempt was to remove it, then I re-added it with a small
>> tear...
>
>
> So you actually create a new parser and the subject (that existing) of this
> patch is misleading.

Hi Alexander,

I think you misread what Henrik and I were discussing about:
Henrik complained about using the heap for something which is just
used in this function, and using the stack would have been better.
However, the size of the parser is too big that the compiler complains
when we declare it on the stack.

So this line is just a copy/paste from the function hid_open_report(),
and thus, you can agree that I did not create a new parser.

Cheers,
Benjamin

Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the report descriptors

[Alexander Holler]

Am 15.08.2013 19:36, schrieb Benjamin Tissoires:
> On Wed, Aug 14, 2013 at 10:03 PM, Alexander Holler <holler@ahsoftware.de> wrote:
>>>>> -       hid->group = HID_GROUP_GENERIC;
>>>>> +       parser = vzalloc(sizeof(struct hid_parser));
>>>>
>>>>
>>>> Argh, I realize it is inevitable for this patch, but it still makes my
>>>> eyes bleed. The parser takes quite a bit of memory...
>>>
>>>
>>> Yep, my first attempt was to remove it, then I re-added it with a small
>>> tear...
>>
>>
>> So you actually create a new parser and the subject (that existing) of this
>> patch is misleading.
> 
> Hi Alexander,
> 
> I think you misread what Henrik and I were discussing about:
> Henrik complained about using the heap for something which is just
> used in this function, and using the stack would have been better.
> However, the size of the parser is too big that the compiler complains
> when we declare it on the stack.
> 
> So this line is just a copy/paste from the function hid_open_report(),
> and thus, you can agree that I did not create a new parser.

Hmm, not really, you do instantiate a new parser, wherever that one lives.

Of course, the code for the parser already exists, but at first I've
read the subject such, that something will be used which already was in
use. That "existing" did mislead me.

I think "Use hid_parser for ..." would describe the change more verbose
as the patch changes runtime behaviour quite a bit (by dispatching
everything through the parser).

Regards,

Alexander Holler