[PATCH v4] HID: core: check whether Usage Page item is after Usage ID items

[PATCH v4] HID: core: check whether Usage Page item is after Usage ID items

[Candle Sun]

From: Candle Sun <candle.sun@unisoc.com>

Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
to Main item") adds support for Usage Page item after Usage ID items
(such as keyboards manufactured by Primax).

Usage Page concatenation in Main item works well for following report
descriptor patterns:

    USAGE_PAGE (Keyboard)                   05 07
    USAGE_MINIMUM (Keyboard LeftControl)    19 E0
    USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
    LOGICAL_MINIMUM (0)                     15 00
    LOGICAL_MAXIMUM (1)                     25 01
    REPORT_SIZE (1)                         75 01
    REPORT_COUNT (8)                        95 08
    INPUT (Data,Var,Abs)                    81 02

-------------

    USAGE_MINIMUM (Keyboard LeftControl)    19 E0
    USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
    LOGICAL_MINIMUM (0)                     15 00
    LOGICAL_MAXIMUM (1)                     25 01
    REPORT_SIZE (1)                         75 01
    REPORT_COUNT (8)                        95 08
    USAGE_PAGE (Keyboard)                   05 07
    INPUT (Data,Var,Abs)                    81 02

But it makes the parser act wrong for the following report
descriptor pattern(such as some Gamepads):

    USAGE_PAGE (Button)                     05 09
    USAGE (Button 1)                        09 01
    USAGE (Button 2)                        09 02
    USAGE (Button 4)                        09 04
    USAGE (Button 5)                        09 05
    USAGE (Button 7)                        09 07
    USAGE (Button 8)                        09 08
    USAGE (Button 14)                       09 0E
    USAGE (Button 15)                       09 0F
    USAGE (Button 13)                       09 0D
    USAGE_PAGE (Consumer Devices)           05 0C
    USAGE (Back)                            0a 24 02
    USAGE (HomePage)                        0a 23 02
    LOGICAL_MINIMUM (0)                     15 00
    LOGICAL_MAXIMUM (1)                     25 01
    REPORT_SIZE (1)                         75 01
    REPORT_COUNT (11)                       95 0B
    INPUT (Data,Var,Abs)                    81 02

With Usage Page concatenation in Main item, parser recognizes all the
11 Usages as consumer keys, it is not the HID device's real intention.

This patch checks whether Usage Page is really defined after Usage ID
items by comparing usage page using status.

Usage Page concatenation on currently defined Usage Page will always
do in local parsing when Usage ID items encountered.

When Main item is parsing, concatenation will do again with last
defined Usage Page if this page has not been used in the previous
usages concatenation.

Signed-off-by: Candle Sun <candle.sun@unisoc.com>
Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
---
Changes in v4:
- Fix v3 introduced BUG in hid_add_usage()
- Add checking logic to replace usage_page_last member
- Update patch description

Changes in v3:
- Rework the GET_COMPLETE_USAGE macro as static complete_usage()
  function
- Add some code comments for usage_page_last

Changes in v2:
- Update patch title
- Add GET_COMPLETE_USAGE macro
- Change the logic of checking whether to concatenate usage page again
  in main parsing
---
 drivers/hid/hid-core.c | 51 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3eaee2c37931..c18ed7180b07 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -211,6 +211,18 @@ static unsigned hid_lookup_collection(struct hid_parser *parser, unsigned type)
 	return 0; /* we know nothing about this usage type */
 }
 
+/*
+ * Concatenate usage which defines 16 bits or less with the
+ * currently defined usage page to form a 32 bit usage
+ */
+
+static void complete_usage(struct hid_parser *parser, unsigned int index)
+{
+	parser->local.usage[index] &= 0xFFFF;
+	parser->local.usage[index] |=
+		(parser->global.usage_page & 0xFFFF) << 16;
+}
+
 /*
  * Add a usage to the temporary parser table.
  */
@@ -222,6 +234,14 @@ static int hid_add_usage(struct hid_parser *parser, unsigned usage, u8 size)
 		return -1;
 	}
 	parser->local.usage[parser->local.usage_index] = usage;
+
+	/*
+	 * If Usage item only includes usage id, concatenate it with
+	 * currently defined usage page
+	 */
+	if (size <= 2)
+		complete_usage(parser, parser->local.usage_index);
+
 	parser->local.usage_size[parser->local.usage_index] = size;
 	parser->local.collection_index[parser->local.usage_index] =
 		parser->collection_stack_ptr ?
@@ -543,13 +563,32 @@ static int hid_parser_local(struct hid_parser *parser, struct hid_item *item)
  * usage value."
  */
 
-static void hid_concatenate_usage_page(struct hid_parser *parser)
+static void hid_concatenate_last_usage_page(struct hid_parser *parser)
 {
 	int i;
+	unsigned int usage_page;
+	unsigned int current_page;
 
-	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;
+	if (!parser->local.usage_index)
+		return;
+
+	usage_page = parser->global.usage_page;
+
+	/*
+	 * Concatenate usage page again only if last declared Usage Page
+	 * has not been already used in previous usages concatenation
+	 */
+	for (i = parser->local.usage_index - 1; i >= 0; i--) {
+		if (parser->local.usage_size[i] > 2)
+			/* Ignore extended usages */
+			continue;
+
+		current_page = parser->local.usage[i] >> 16;
+		if (current_page == usage_page)
+			break;
+
+		complete_usage(parser, i);
+	}
 }
 
 /*
@@ -561,7 +600,7 @@ static int hid_parser_main(struct hid_parser *parser, struct hid_item *item)
 	__u32 data;
 	int ret;
 
-	hid_concatenate_usage_page(parser);
+	hid_concatenate_last_usage_page(parser);
 
 	data = item_udata(item);
 
@@ -772,7 +811,7 @@ static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
 	__u32 data;
 	int i;
 
-	hid_concatenate_usage_page(parser);
+	hid_concatenate_last_usage_page(parser);
 
 	data = item_udata(item);
 
-- 
2.17.1

Re: [PATCH v4] HID: core: check whether Usage Page item is after Usage ID items

[Candle Sun]

Jiri & Benjamin,
Do you have some futher advice on this patch?
Can the patch be merged?

Regards,
Candle

On Tue, Oct 22, 2019 at 10:22 PM Candle Sun <candlesea@gmail.com> wrote:
>
> From: Candle Sun <candle.sun@unisoc.com>
>
> Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> to Main item") adds support for Usage Page item after Usage ID items
> (such as keyboards manufactured by Primax).
>
> Usage Page concatenation in Main item works well for following report
> descriptor patterns:
>
>     USAGE_PAGE (Keyboard)                   05 07
>     USAGE_MINIMUM (Keyboard LeftControl)    19 E0
>     USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
>     LOGICAL_MINIMUM (0)                     15 00
>     LOGICAL_MAXIMUM (1)                     25 01
>     REPORT_SIZE (1)                         75 01
>     REPORT_COUNT (8)                        95 08
>     INPUT (Data,Var,Abs)                    81 02
>
> -------------
>
>     USAGE_MINIMUM (Keyboard LeftControl)    19 E0
>     USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
>     LOGICAL_MINIMUM (0)                     15 00
>     LOGICAL_MAXIMUM (1)                     25 01
>     REPORT_SIZE (1)                         75 01
>     REPORT_COUNT (8)                        95 08
>     USAGE_PAGE (Keyboard)                   05 07
>     INPUT (Data,Var,Abs)                    81 02
>
> But it makes the parser act wrong for the following report
> descriptor pattern(such as some Gamepads):
>
>     USAGE_PAGE (Button)                     05 09
>     USAGE (Button 1)                        09 01
>     USAGE (Button 2)                        09 02
>     USAGE (Button 4)                        09 04
>     USAGE (Button 5)                        09 05
>     USAGE (Button 7)                        09 07
>     USAGE (Button 8)                        09 08
>     USAGE (Button 14)                       09 0E
>     USAGE (Button 15)                       09 0F
>     USAGE (Button 13)                       09 0D
>     USAGE_PAGE (Consumer Devices)           05 0C
>     USAGE (Back)                            0a 24 02
>     USAGE (HomePage)                        0a 23 02
>     LOGICAL_MINIMUM (0)                     15 00
>     LOGICAL_MAXIMUM (1)                     25 01
>     REPORT_SIZE (1)                         75 01
>     REPORT_COUNT (11)                       95 0B
>     INPUT (Data,Var,Abs)                    81 02
>
> With Usage Page concatenation in Main item, parser recognizes all the
> 11 Usages as consumer keys, it is not the HID device's real intention.
>
> This patch checks whether Usage Page is really defined after Usage ID
> items by comparing usage page using status.
>
> Usage Page concatenation on currently defined Usage Page will always
> do in local parsing when Usage ID items encountered.
>
> When Main item is parsing, concatenation will do again with last
> defined Usage Page if this page has not been used in the previous
> usages concatenation.
>
> Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> ---
> Changes in v4:
> - Fix v3 introduced BUG in hid_add_usage()
> - Add checking logic to replace usage_page_last member
> - Update patch description
>
> Changes in v3:
> - Rework the GET_COMPLETE_USAGE macro as static complete_usage()
>   function
> - Add some code comments for usage_page_last
>
> Changes in v2:
> - Update patch title
> - Add GET_COMPLETE_USAGE macro
> - Change the logic of checking whether to concatenate usage page again
>   in main parsing
> ---
>  drivers/hid/hid-core.c | 51 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 3eaee2c37931..c18ed7180b07 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -211,6 +211,18 @@ static unsigned hid_lookup_collection(struct hid_parser *parser, unsigned type)
>         return 0; /* we know nothing about this usage type */
>  }
>
> +/*
> + * Concatenate usage which defines 16 bits or less with the
> + * currently defined usage page to form a 32 bit usage
> + */
> +
> +static void complete_usage(struct hid_parser *parser, unsigned int index)
> +{
> +       parser->local.usage[index] &= 0xFFFF;
> +       parser->local.usage[index] |=
> +               (parser->global.usage_page & 0xFFFF) << 16;
> +}
> +
>  /*
>   * Add a usage to the temporary parser table.
>   */
> @@ -222,6 +234,14 @@ static int hid_add_usage(struct hid_parser *parser, unsigned usage, u8 size)
>                 return -1;
>         }
>         parser->local.usage[parser->local.usage_index] = usage;
> +
> +       /*
> +        * If Usage item only includes usage id, concatenate it with
> +        * currently defined usage page
> +        */
> +       if (size <= 2)
> +               complete_usage(parser, parser->local.usage_index);
> +
>         parser->local.usage_size[parser->local.usage_index] = size;
>         parser->local.collection_index[parser->local.usage_index] =
>                 parser->collection_stack_ptr ?
> @@ -543,13 +563,32 @@ static int hid_parser_local(struct hid_parser *parser, struct hid_item *item)
>   * usage value."
>   */
>
> -static void hid_concatenate_usage_page(struct hid_parser *parser)
> +static void hid_concatenate_last_usage_page(struct hid_parser *parser)
>  {
>         int i;
> +       unsigned int usage_page;
> +       unsigned int current_page;
>
> -       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;
> +       if (!parser->local.usage_index)
> +               return;
> +
> +       usage_page = parser->global.usage_page;
> +
> +       /*
> +        * Concatenate usage page again only if last declared Usage Page
> +        * has not been already used in previous usages concatenation
> +        */
> +       for (i = parser->local.usage_index - 1; i >= 0; i--) {
> +               if (parser->local.usage_size[i] > 2)
> +                       /* Ignore extended usages */
> +                       continue;
> +
> +               current_page = parser->local.usage[i] >> 16;
> +               if (current_page == usage_page)
> +                       break;
> +
> +               complete_usage(parser, i);
> +       }
>  }
>
>  /*
> @@ -561,7 +600,7 @@ static int hid_parser_main(struct hid_parser *parser, struct hid_item *item)
>         __u32 data;
>         int ret;
>
> -       hid_concatenate_usage_page(parser);
> +       hid_concatenate_last_usage_page(parser);
>
>         data = item_udata(item);
>
> @@ -772,7 +811,7 @@ static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
>         __u32 data;
>         int i;
>
> -       hid_concatenate_usage_page(parser);
> +       hid_concatenate_last_usage_page(parser);
>
>         data = item_udata(item);
>
> --
> 2.17.1
>

Re: [PATCH v4] HID: core: check whether Usage Page item is after Usage ID items

[Jiri Kosina]

On Tue, 22 Oct 2019, Candle Sun wrote:

> From: Candle Sun <candle.sun@unisoc.com>
> 
> Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> to Main item") adds support for Usage Page item after Usage ID items
> (such as keyboards manufactured by Primax).
[ ... snip ... ]

Benjamin,

are you planning to run this through your testsuite against regressions?

I believe that's the last missing step, otherwise I'd be fine merging 
this.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4] HID: core: check whether Usage Page item is after Usage ID items

[Benjamin Tissoires]

Hi all,

On Tue, Nov 12, 2019 at 2:57 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Tue, 22 Oct 2019, Candle Sun wrote:
>
> > From: Candle Sun <candle.sun@unisoc.com>
> >
> > Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> > to Main item") adds support for Usage Page item after Usage ID items
> > (such as keyboards manufactured by Primax).
> [ ... snip ... ]
>
> Benjamin,
>
> are you planning to run this through your testsuite against regressions?
>
> I believe that's the last missing step, otherwise I'd be fine merging
> this.

Sorry I had to deal with family issues 2 weeks ago, and now RHEL is
coming back at me and eating all my time.

The kernel patch is now OK, so we can grab it now (either you take it
Jiri, and add my acked-by or I'll push it later...)

Candle, can you rework
https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/58 so
that it mirrors the kernel code (and get rid of the
self.local.usage_page_last logic)?

Cheers,
Benjamin

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>

Re: [PATCH v4] HID: core: check whether Usage Page item is after Usage ID items

[Jiri Kosina]

On Tue, 12 Nov 2019, Benjamin Tissoires wrote:

> The kernel patch is now OK, so we can grab it now (either you take it
> Jiri, and add my acked-by or I'll push it later...)

I've just queued it in for-5.5/core. Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4] HID: core: check whether Usage Page item is after Usage ID items

[Candle Sun]

On Tue, Nov 12, 2019 at 11:18 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi all,
>
> On Tue, Nov 12, 2019 at 2:57 PM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > On Tue, 22 Oct 2019, Candle Sun wrote:
> >
> > > From: Candle Sun <candle.sun@unisoc.com>
> > >
> > > Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> > > to Main item") adds support for Usage Page item after Usage ID items
> > > (such as keyboards manufactured by Primax).
> > [ ... snip ... ]
> >
> > Benjamin,
> >
> > are you planning to run this through your testsuite against regressions?
> >
> > I believe that's the last missing step, otherwise I'd be fine merging
> > this.
>
> Sorry I had to deal with family issues 2 weeks ago, and now RHEL is
> coming back at me and eating all my time.
>
> The kernel patch is now OK, so we can grab it now (either you take it
> Jiri, and add my acked-by or I'll push it later...)
>
> Candle, can you rework
> https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/58 so
> that it mirrors the kernel code (and get rid of the
> self.local.usage_page_last logic)?
>
> Cheers,
> Benjamin
>

Thanks Jiri and Benjamin.
I will rework the hid-tools patch ASAP.

Regards,
Candle

> >
> > Thanks,
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >
>

Re: [PATCH v4] HID: core: check whether Usage Page item is after Usage ID items

[Candle Sun]

On Wed, Nov 13, 2019 at 9:08 AM Candle Sun <candlesea@gmail.com> wrote:
>
> On Tue, Nov 12, 2019 at 11:18 PM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > Hi all,
> >
> > On Tue, Nov 12, 2019 at 2:57 PM Jiri Kosina <jikos@kernel.org> wrote:
> > >
> > > On Tue, 22 Oct 2019, Candle Sun wrote:
> > >
> > > > From: Candle Sun <candle.sun@unisoc.com>
> > > >
> > > > Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> > > > to Main item") adds support for Usage Page item after Usage ID items
> > > > (such as keyboards manufactured by Primax).
> > > [ ... snip ... ]
> > >
> > > Benjamin,
> > >
> > > are you planning to run this through your testsuite against regressions?
> > >
> > > I believe that's the last missing step, otherwise I'd be fine merging
> > > this.
> >
> > Sorry I had to deal with family issues 2 weeks ago, and now RHEL is
> > coming back at me and eating all my time.
> >
> > The kernel patch is now OK, so we can grab it now (either you take it
> > Jiri, and add my acked-by or I'll push it later...)
> >
> > Candle, can you rework
> > https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/58 so
> > that it mirrors the kernel code (and get rid of the
> > self.local.usage_page_last logic)?
> >
> > Cheers,
> > Benjamin
> >
>
> Thanks Jiri and Benjamin.
> I will rework the hid-tools patch ASAP.
>
> Regards,
> Candle
>

Hi Benjamin,
The MR for hid-tools is updated:
https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/62

Thanks,
Candle

> > >
> > > Thanks,
> > >
> > > --
> > > Jiri Kosina
> > > SUSE Labs
> > >
> >