[v4] HID: core: move Usage Page concatenation to Main item
diff mbox series

Message ID 20190327101848.25477-1-nsaenzjulienne@suse.de
State Accepted
Commit 58e75155009cc800005629955d3482f36a1e0eec
Headers show
Series
  • [v4] HID: core: move Usage Page concatenation to Main item
Related show

Commit Message

Nicolas Saenz Julienne March 27, 2019, 10:18 a.m. UTC
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@suse.de>
---

v3->v4: - Use u8 instead of __u8
        - Remove overly complex usage size calculation

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 | 36 ++++++++++++++++++++++++------------
 include/linux/hid.h    |  1 +
 2 files changed, 25 insertions(+), 12 deletions(-)

Comments

Junge, Terry March 28, 2019, 12:27 a.m. UTC | #1
Hi Nicolas,

V4 looks good to me.

Thanks,
Terry

>-----Original Message-----
>Subject: [PATCH v4] HID: core: move Usage Page concatenation to Main item
>
Benjamin Tissoires March 28, 2019, 1:48 p.m. UTC | #2
On Thu, Mar 28, 2019 at 1:27 AM Junge, Terry <terry.junge@poly.com> wrote:
>
> Hi Nicolas,
>
> V4 looks good to me.

Looks good to me too.

Terry, can I consider this as a formal Rev-by you?

Cheers,
Benjamin

>
> Thanks,
> Terry
>
> >-----Original Message-----
> >Subject: [PATCH v4] HID: core: move Usage Page concatenation to Main item
> >
Junge, Terry March 29, 2019, 12:48 a.m. UTC | #3
On Thursday, March 28, 2019 6:49 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>>
>> Hi Nicolas,
>>
>> V4 looks good to me.
>
>Looks good to me too.
>
>Terry, can I consider this as a formal Rev-by you?

Benjamin,

Yes, thank you. What is the normal way of formally indicating Rev-by?

Regards,
Terry

>
>Cheers,
>Benjamin
>
>>
>> Thanks,
>> Terry
>>
>> >-----Original Message-----
>> >Subject: [PATCH v4] HID: core: move Usage Page concatenation to Main
>item
>> >
Benjamin Tissoires March 29, 2019, 7:51 a.m. UTC | #4
On Fri, Mar 29, 2019 at 1:48 AM Junge, Terry <terry.junge@poly.com> wrote:
>
> On Thursday, March 28, 2019 6:49 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> >>
> >> Hi Nicolas,
> >>
> >> V4 looks good to me.
> >
> >Looks good to me too.
> >
> >Terry, can I consider this as a formal Rev-by you?
>
> Benjamin,
>
> Yes, thank you. What is the normal way of formally indicating Rev-by?
>

Thanks.

So giving a rev-by is described in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst
section 13.

Basically, you just answer a patch with: "Reviewed-by: Your Name
<your@address.sth>" (without quotes).

This has 2 benefits: it's formal, and everybody understands it, and
patchwork (at https://patchwork.kernel.org/patch/10873179/ for this
patch) will catch it. Then with the patchwork client, I can apply the
patch without having to manually add your tag :) (no need to resend
one here, unless you want to  give a try, I'll apply it manually).

Cheers,
Benjamin
Benjamin Tissoires April 2, 2019, 2:34 p.m. UTC | #5
On Wed, Mar 27, 2019 at 11:19 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> 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@suse.de>
> ---

Applied to for-5.2/core with Terry's reviewed-by.

Thanks everybody.

Cheers,
Benjamin

>
> v3->v4: - Use u8 instead of __u8
>         - Remove overly complex usage size calculation
>
> 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 | 36 ++++++++++++++++++++++++------------
>  include/linux/hid.h    |  1 +
>  2 files changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 9993b692598f..852bbd303d9a 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,22 @@ 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)
> +{
> +       int i;
> +
> +       for (i = 0; i < parser->local.usage_index; i++)
> +               if (parser->local.usage_size[i] <= 2)
> +                       parser->local.usage[i] += parser->global.usage_page << 16;
> +}
> +
>  /*
>   * Process a main item.
>   */
> @@ -556,6 +564,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 +775,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..ac0c70b4ce10 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
>

Patch
diff mbox series

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9993b692598f..852bbd303d9a 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,22 @@  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)
+{
+	int i;
+
+	for (i = 0; i < parser->local.usage_index; i++)
+		if (parser->local.usage_size[i] <= 2)
+			parser->local.usage[i] += parser->global.usage_page << 16;
+}
+
 /*
  * Process a main item.
  */
@@ -556,6 +564,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 +775,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..ac0c70b4ce10 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;