RE: [PATCH v3] HID: core: move Usage Page concatenation to Main item

[Junge, Terry]

Hi Nicolas,

This patch looks good except for one comment/question below.

Thanks,
Terry

On Tuesday, March 26, 2019 1:04 PM Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx> wrote:
>
>As seen on some USB wireless keyboards manufactured by Primax, the HID
>parser was using some assumptions that are not always true. In this case it's s
>the fact that, inside the scope of a main item, an Usage Page will always
>precede an Usage.
>
>The spec is not pretty clear as 6.2.2.7 states "Any usage that follows is
>interpreted as a Usage ID and concatenated with the Usage Page".
>While 6.2.2.8 states "When the parser encounters a main item it concatenates
>the last declared Usage Page with a Usage to form a complete usage value."
>Being somewhat contradictory it was decided to match Window's
>implementation, which follows 6.2.2.8.
>
>In summary, the patch moves the Usage Page concatenation from the local
>item parsing function to the main item parsing function.
>
>Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>
>---
>
>v2->v3: - Update patch title
>
>v1->v2: - Add usage concatenation to hid_scan_main()
>	- Rework tests in hid-tools, making sure no-one is failing
>
> drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++------------
> include/linux/hid.h    |  1 +
> 2 files changed, 29 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index
>9993b692598f..40c836ce3248 100644
>--- a/drivers/hid/hid-core.c
>+++ b/drivers/hid/hid-core.c
>@@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct
>hid_parser *parser, unsigned type)
>  * Add a usage to the temporary parser table.
>  */
>
>-static int hid_add_usage(struct hid_parser *parser, unsigned usage)
>+static int hid_add_usage(struct hid_parser *parser, unsigned usage,
>+__u8 size)
> {
> 	if (parser->local.usage_index >= HID_MAX_USAGES) {
> 		hid_err(parser->device, "usage index exceeded\n");
> 		return -1;
> 	}
> 	parser->local.usage[parser->local.usage_index] = usage;
>+	parser->local.usage_size[parser->local.usage_index] = size;
> 	parser->local.collection_index[parser->local.usage_index] =
> 		parser->collection_stack_ptr ?
> 		parser->collection_stack[parser->collection_stack_ptr - 1] : 0;
>@@ -486,10 +487,7 @@ static int hid_parser_local(struct hid_parser *parser,
>struct hid_item *item)
> 			return 0;
> 		}
>
>-		if (item->size <= 2)
>-			data = (parser->global.usage_page << 16) + data;
>-
>-		return hid_add_usage(parser, data);
>+		return hid_add_usage(parser, data, item->size);
>
> 	case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
>
>@@ -498,9 +496,6 @@ static int hid_parser_local(struct hid_parser *parser,
>struct hid_item *item)
> 			return 0;
> 		}
>
>-		if (item->size <= 2)
>-			data = (parser->global.usage_page << 16) + data;
>-
> 		parser->local.usage_minimum = data;
> 		return 0;
>
>@@ -511,9 +506,6 @@ static int hid_parser_local(struct hid_parser *parser,
>struct hid_item *item)
> 			return 0;
> 		}
>
>-		if (item->size <= 2)
>-			data = (parser->global.usage_page << 16) + data;
>-
> 		count = data - parser->local.usage_minimum;
> 		if (count + parser->local.usage_index >= HID_MAX_USAGES) {
> 			/*
>@@ -533,7 +525,7 @@ static int hid_parser_local(struct hid_parser *parser,
>struct hid_item *item)
> 		}
>
> 		for (n = parser->local.usage_minimum; n <= data; n++)
>-			if (hid_add_usage(parser, n)) {
>+			if (hid_add_usage(parser, n, item->size)) {
> 				dbg_hid("hid_add_usage failed\n");
> 				return -1;
> 			}
>@@ -547,6 +539,26 @@ static int hid_parser_local(struct hid_parser *parser,
>struct hid_item *item)
> 	return 0;
> }
>
>+/*
>+ * Concatenate Usage Pages into Usages where relevant:
>+ * As per specification, 6.2.2.8: "When the parser encounters a main
>+item it
>+ * concatenates the last declared Usage Page with a Usage to form a
>+complete
>+ * usage value."
>+ */
>+
>+static void hid_concatenate_usage_page(struct hid_parser *parser) {
>+	unsigned usages;
>+	int i;
>+
>+	usages = max_t(unsigned, parser->local.usage_index,
>+				 parser->global.report_count);

I don't think we need to worry about global.report_count here,
just concatenate for the usages currently in the local queue so could
this be simplified by removing usages and just using local.usage_index?

        for (i = 0; i < local.usage_index; i++)

>+
>+	for (i = 0; i < usages; i++)
>+		if (parser->local.usage_size[i] <= 2)
>+			parser->local.usage[i] += parser->global.usage_page
><< 16; }
>+
> /*
>  * Process a main item.
>  */
>@@ -556,6 +568,8 @@ static int hid_parser_main(struct hid_parser *parser,
>struct hid_item *item)
> 	__u32 data;
> 	int ret;
>
>+	hid_concatenate_usage_page(parser);
>+
> 	data = item_udata(item);
>
> 	switch (item->tag) {
>@@ -765,6 +779,8 @@ static int hid_scan_main(struct hid_parser *parser,
>struct hid_item *item)
> 	__u32 data;
> 	int i;
>
>+	hid_concatenate_usage_page(parser);
>+
> 	data = item_udata(item);
>
> 	switch (item->tag) {
>diff --git a/include/linux/hid.h b/include/linux/hid.h index
>f9707d1dcb58..d1fb4b678873 100644
>--- a/include/linux/hid.h
>+++ b/include/linux/hid.h
>@@ -417,6 +417,7 @@ struct hid_global {
>
> struct hid_local {
> 	unsigned usage[HID_MAX_USAGES]; /* usage array */
>+	__u8 usage_size[HID_MAX_USAGES]; /* usage size array */
> 	unsigned collection_index[HID_MAX_USAGES]; /* collection index
>array */
> 	unsigned usage_index;
> 	unsigned usage_minimum;
>--
>2.21.0