* [PATCH v2] HID: core: check whether usage page item is after usage id item
@ 2019-10-09 12:53 Candle Sun
2019-10-09 17:01 ` Nicolas Saenz Julienne
2019-10-10 12:24 ` Benjamin Tissoires
0 siblings, 2 replies; 10+ messages in thread
From: Candle Sun @ 2019-10-09 12:53 UTC (permalink / raw)
To: jikos, benjamin.tissoires, nsaenzjulienne
Cc: orson.zhai, linux-input, linux-kernel, Candle Sun, Nianfu Bai
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 adds usage_page_last to flag whether Usage Page is after
Usage ID items. usage_page_last is false default, it is set as true
once Usage Page item is encountered and is reverted by next Usage ID
item.
Usage Page concatenation on the currently defined Usage Page will do
firstly in Local parsing when Usage ID items encountered.
When Main item is parsing, concatenation will do again with last
defined Usage Page if usage_page_last flag is true.
Signed-off-by: Candle Sun <candle.sun@unisoc.com>
Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
---
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 | 31 +++++++++++++++++++++++++------
include/linux/hid.h | 1 +
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3eaee2c..3394222 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -35,6 +35,8 @@
#include "hid-ids.h"
+#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
+
/*
* Version Information
*/
@@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser, unsigned usage, u8 size)
hid_err(parser->device, "usage index exceeded\n");
return -1;
}
- parser->local.usage[parser->local.usage_index] = usage;
+
+ if (size <= 2) {
+ parser->local.usage_page_last = false;
+ parser->local.usage[parser->local.usage_index] =
+ GET_COMPLETE_USAGE(parser->global.usage_page, usage);
+ } else {
+ 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 ?
@@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
parser->global.usage_page = item_udata(item);
+ parser->local.usage_page_last = true;
return 0;
case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
@@ -543,13 +554,21 @@ 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;
+ unsigned int usage_page = parser->global.usage_page;
+
+ if (!parser->local.usage_page_last)
+ return;
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_size[i] <= 2) {
+ usage = parser->local.usage[i];
+ parser->local.usage[i] =
+ GET_COMPLETE_USAGE(usage_page, usage);
+ }
}
/*
@@ -561,7 +580,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 +791,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);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index cd41f20..2e0ea2f7 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -412,6 +412,7 @@ struct hid_local {
unsigned usage_minimum;
unsigned delimiter_depth;
unsigned delimiter_branch;
+ bool usage_page_last; /* whether usage page is after usage id */
};
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
2019-10-09 12:53 [PATCH v2] HID: core: check whether usage page item is after usage id item Candle Sun
@ 2019-10-09 17:01 ` Nicolas Saenz Julienne
2019-10-09 17:59 ` Jiri Kosina
2019-10-10 3:05 ` Candle Sun
2019-10-10 12:24 ` Benjamin Tissoires
1 sibling, 2 replies; 10+ messages in thread
From: Nicolas Saenz Julienne @ 2019-10-09 17:01 UTC (permalink / raw)
To: Candle Sun, jikos, benjamin.tissoires
Cc: orson.zhai, linux-input, linux-kernel, Candle Sun, Nianfu Bai
[-- Attachment #1: Type: text/plain, Size: 7129 bytes --]
On Wed, 2019-10-09 at 20:53 +0800, 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).
>
> 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 adds usage_page_last to flag whether Usage Page is after
> Usage ID items. usage_page_last is false default, it is set as true
> once Usage Page item is encountered and is reverted by next Usage ID
> item.
>
> Usage Page concatenation on the currently defined Usage Page will do
> firstly in Local parsing when Usage ID items encountered.
>
> When Main item is parsing, concatenation will do again with last
> defined Usage Page if usage_page_last flag is true.
Functionally I think this is the right approach. Sadly I don't have access to
any Primax device anymore so I can't test it. But I suggest you update
hid-tools' parser and add a new unit test to verify we aren't missing anything.
You can base your code on this:
https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/37/commits
> Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> ---
> 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 | 31 +++++++++++++++++++++++++------
> include/linux/hid.h | 1 +
> 2 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 3eaee2c..3394222 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -35,6 +35,8 @@
>
> #include "hid-ids.h"
>
> +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
Not sure I like the macro. I'd rather have the explicit code. That said, lets
see what Benjamin has to say.
> +
> /*
> * Version Information
> */
> @@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser,
> unsigned usage, u8 size)
> hid_err(parser->device, "usage index exceeded\n");
> return -1;
> }
> - parser->local.usage[parser->local.usage_index] = usage;
> +
> + if (size <= 2) {
> + parser->local.usage_page_last = false;
> + parser->local.usage[parser->local.usage_index] =
> + GET_COMPLETE_USAGE(parser->global.usage_page, usage);
> + } else {
> + 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 ?
> @@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser,
> struct hid_item *item)
>
> case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
> parser->global.usage_page = item_udata(item);
> + parser->local.usage_page_last = true;
> return 0;
>
> case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
> @@ -543,13 +554,21 @@ static int hid_parser_local(struct hid_parser *parser,
> struct hid_item *item)
> * usage value."
> */
I'd expand the comment above to further explain what we're doing here.
>
> -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;
> + unsigned int usage_page = parser->global.usage_page;
> +
> + if (!parser->local.usage_page_last)
> + return;
>
> for (i = 0; i < parser->local.usage_index; i++)
Technically correct but it's preferred if you use braces here.
> - if (parser->local.usage_size[i] <= 2)
> - parser->local.usage[i] += parser->global.usage_page <<
> 16;
> + if (parser->local.usage_size[i] <= 2) {
> + usage = parser->local.usage[i];
> + parser->local.usage[i] =
> + GET_COMPLETE_USAGE(usage_page, usage);
> + }
> }
>
> /*
> @@ -561,7 +580,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 +791,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);
>
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index cd41f20..2e0ea2f7 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -412,6 +412,7 @@ struct hid_local {
> unsigned usage_minimum;
> unsigned delimiter_depth;
> unsigned delimiter_branch;
> + bool usage_page_last; /* whether usage page is after usage id */
> };
>
> /*
Regards,
Nicolas
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
2019-10-09 17:01 ` Nicolas Saenz Julienne
@ 2019-10-09 17:59 ` Jiri Kosina
2019-10-10 3:19 ` Candle Sun
2019-10-10 3:05 ` Candle Sun
1 sibling, 1 reply; 10+ messages in thread
From: Jiri Kosina @ 2019-10-09 17:59 UTC (permalink / raw)
To: Nicolas Saenz Julienne
Cc: Candle Sun, benjamin.tissoires, orson.zhai, linux-input,
linux-kernel, Candle Sun, Nianfu Bai
On Wed, 9 Oct 2019, Nicolas Saenz Julienne wrote:
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 3eaee2c..3394222 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -35,6 +35,8 @@
> >
> > #include "hid-ids.h"
> >
> > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
>
> Not sure I like the macro. I'd rather have the explicit code. That said, lets
> see what Benjamin has to say.
Not sure about Benjamin :) but I personally would ask for putting it
somewhere into hid.h as static inline.
And even if it's for some reason insisted on this staying macro, please at
least put it as close to the place(s) it's being used as possible, in
order to maintain some code sanity.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
2019-10-09 17:01 ` Nicolas Saenz Julienne
2019-10-09 17:59 ` Jiri Kosina
@ 2019-10-10 3:05 ` Candle Sun
2019-10-10 7:42 ` Nicolas Saenz Julienne
1 sibling, 1 reply; 10+ messages in thread
From: Candle Sun @ 2019-10-10 3:05 UTC (permalink / raw)
To: Nicolas Saenz Julienne
Cc: Jiri Kosina, Benjamin Tissoires, orson.zhai,
open list:HID CORE LAYER, lkml, Candle Sun, Nianfu Bai
On Thu, Oct 10, 2019 at 1:01 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> On Wed, 2019-10-09 at 20:53 +0800, 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).
> >
> > 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 adds usage_page_last to flag whether Usage Page is after
> > Usage ID items. usage_page_last is false default, it is set as true
> > once Usage Page item is encountered and is reverted by next Usage ID
> > item.
> >
> > Usage Page concatenation on the currently defined Usage Page will do
> > firstly in Local parsing when Usage ID items encountered.
> >
> > When Main item is parsing, concatenation will do again with last
> > defined Usage Page if usage_page_last flag is true.
>
> Functionally I think this is the right approach. Sadly I don't have access to
> any Primax device anymore so I can't test it. But I suggest you update
> hid-tools' parser and add a new unit test to verify we aren't missing anything.
>
> You can base your code on this:
>
> https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/37/commits
>
Thanks Nicolas. I will check and try to do it.
Candle
> > Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> > ---
> > 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 | 31 +++++++++++++++++++++++++------
> > include/linux/hid.h | 1 +
> > 2 files changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 3eaee2c..3394222 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -35,6 +35,8 @@
> >
> > #include "hid-ids.h"
> >
> > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
>
> Not sure I like the macro. I'd rather have the explicit code. That said, lets
> see what Benjamin has to say.
>
> > +
> > /*
> > * Version Information
> > */
> > @@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser,
> > unsigned usage, u8 size)
> > hid_err(parser->device, "usage index exceeded\n");
> > return -1;
> > }
> > - parser->local.usage[parser->local.usage_index] = usage;
> > +
> > + if (size <= 2) {
> > + parser->local.usage_page_last = false;
> > + parser->local.usage[parser->local.usage_index] =
> > + GET_COMPLETE_USAGE(parser->global.usage_page, usage);
> > + } else {
> > + 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 ?
> > @@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser,
> > struct hid_item *item)
> >
> > case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
> > parser->global.usage_page = item_udata(item);
> > + parser->local.usage_page_last = true;
> > return 0;
> >
> > case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
> > @@ -543,13 +554,21 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > * usage value."
> > */
>
> I'd expand the comment above to further explain what we're doing here.
>
OK, some comment here is better, I will add it.
Candle
> >
> > -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;
> > + unsigned int usage_page = parser->global.usage_page;
> > +
> > + if (!parser->local.usage_page_last)
> > + return;
> >
> > for (i = 0; i < parser->local.usage_index; i++)
>
> Technically correct but it's preferred if you use braces here.
>
Nicolas, do you mean this:
@@ -563,12 +563,13 @@ static void
hid_concatenate_last_usage_page(struct hid_parser *parser)
if (!parser->local.usage_page_last)
return;
- for (i = 0; i < parser->local.usage_index; i++)
+ for (i = 0; i < parser->local.usage_index; i++) {
if (parser->local.usage_size[i] <= 2) {
usage = parser->local.usage[i];
parser->local.usage[i] =
GET_COMPLETE_USAGE(usage_page, usage);
}
+ }
}
Candle
> > - if (parser->local.usage_size[i] <= 2)
> > - parser->local.usage[i] += parser->global.usage_page <<
> > 16;
> > + if (parser->local.usage_size[i] <= 2) {
> > + usage = parser->local.usage[i];
> > + parser->local.usage[i] =
> > + GET_COMPLETE_USAGE(usage_page, usage);
> > + }
> > }
> >
> > /*
> > @@ -561,7 +580,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 +791,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);
> >
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index cd41f20..2e0ea2f7 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -412,6 +412,7 @@ struct hid_local {
> > unsigned usage_minimum;
> > unsigned delimiter_depth;
> > unsigned delimiter_branch;
> > + bool usage_page_last; /* whether usage page is after usage id */
> > };
> >
> > /*
>
> Regards,
> Nicolas
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
2019-10-09 17:59 ` Jiri Kosina
@ 2019-10-10 3:19 ` Candle Sun
2019-10-10 12:16 ` Benjamin Tissoires
0 siblings, 1 reply; 10+ messages in thread
From: Candle Sun @ 2019-10-10 3:19 UTC (permalink / raw)
To: Jiri Kosina
Cc: Nicolas Saenz Julienne, Benjamin Tissoires, orson.zhai,
open list:HID CORE LAYER, lkml, Candle Sun, Nianfu Bai
On Thu, Oct 10, 2019 at 2:00 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Wed, 9 Oct 2019, Nicolas Saenz Julienne wrote:
>
> > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > index 3eaee2c..3394222 100644
> > > --- a/drivers/hid/hid-core.c
> > > +++ b/drivers/hid/hid-core.c
> > > @@ -35,6 +35,8 @@
> > >
> > > #include "hid-ids.h"
> > >
> > > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
> >
> > Not sure I like the macro. I'd rather have the explicit code. That said, lets
> > see what Benjamin has to say.
>
> Not sure about Benjamin :) but I personally would ask for putting it
> somewhere into hid.h as static inline.
>
> And even if it's for some reason insisted on this staying macro, please at
> least put it as close to the place(s) it's being used as possible, in
> order to maintain some code sanity.
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>
Thanks Nicolas and Jiri,
If macro is not good, I will change it to static function. But the
funciton is only used in hid-core.c,
maybe placing it into hid.h is not good?
Regards,
Candle
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
2019-10-10 3:05 ` Candle Sun
@ 2019-10-10 7:42 ` Nicolas Saenz Julienne
0 siblings, 0 replies; 10+ messages in thread
From: Nicolas Saenz Julienne @ 2019-10-10 7:42 UTC (permalink / raw)
To: Candle Sun
Cc: Jiri Kosina, Benjamin Tissoires, orson.zhai,
open list:HID CORE LAYER, lkml, Candle Sun, Nianfu Bai
[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]
On Thu, 2019-10-10 at 11:05 +0800, Candle Sun wrote:
> > > -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;
> > > + unsigned int usage_page = parser->global.usage_page;
> > > +
> > > + if (!parser->local.usage_page_last)
> > > + return;
> > >
> > > for (i = 0; i < parser->local.usage_index; i++)
> >
> > Technically correct but it's preferred if you use braces here.
> >
>
> Nicolas, do you mean this:
>
> @@ -563,12 +563,13 @@ static void
> hid_concatenate_last_usage_page(struct hid_parser *parser)
> if (!parser->local.usage_page_last)
> return;
>
> - for (i = 0; i < parser->local.usage_index; i++)
> + for (i = 0; i < parser->local.usage_index; i++) {
> if (parser->local.usage_size[i] <= 2) {
> usage = parser->local.usage[i];
> parser->local.usage[i] =
> GET_COMPLETE_USAGE(usage_page, usage);
> }
> + }
> }
Yes, thanks!
Regards,
Nicolas
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
2019-10-10 3:19 ` Candle Sun
@ 2019-10-10 12:16 ` Benjamin Tissoires
2019-10-11 2:08 ` Candle Sun
0 siblings, 1 reply; 10+ messages in thread
From: Benjamin Tissoires @ 2019-10-10 12:16 UTC (permalink / raw)
To: Candle Sun
Cc: Jiri Kosina, Nicolas Saenz Julienne, orson.zhai,
open list:HID CORE LAYER, lkml, Candle Sun, Nianfu Bai
On Thu, Oct 10, 2019 at 5:19 AM Candle Sun <candlesea@gmail.com> wrote:
>
> On Thu, Oct 10, 2019 at 2:00 AM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > On Wed, 9 Oct 2019, Nicolas Saenz Julienne wrote:
> >
> > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > > index 3eaee2c..3394222 100644
> > > > --- a/drivers/hid/hid-core.c
> > > > +++ b/drivers/hid/hid-core.c
> > > > @@ -35,6 +35,8 @@
> > > >
> > > > #include "hid-ids.h"
> > > >
> > > > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
> > >
> > > Not sure I like the macro. I'd rather have the explicit code. That said, lets
> > > see what Benjamin has to say.
> >
> > Not sure about Benjamin :) but I personally would ask for putting it
> > somewhere into hid.h as static inline.
> >
> > And even if it's for some reason insisted on this staying macro, please at
> > least put it as close to the place(s) it's being used as possible, in
> > order to maintain some code sanity.
> >
> > Thanks,
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >
>
> Thanks Nicolas and Jiri,
> If macro is not good, I will change it to static function. But the
> funciton is only used in hid-core.c,
> maybe placing it into hid.h is not good?
I would rather use a function too (in hid-core.c, as it's not reused
anywhere else), and we can make it simpler from the caller point of
view (if I am not mistaken):
---
static void concatenate_usage_page(struct hid_parser *parser, int index)
{
parser->local.usage[index] &= 0xFFFF;
parser->local.usage[index] |= (parser->global.usage_page & 0xFFFF) << 16;
}
// Which can then be called as:
+ parser->local.usage[parser->local.usage_index] = usage;
+ if (size <= 2)
+ concatenate_usage_page(parser, parser->local.usage_index);
+
// And
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_size[i] <= 2) {
+ concatenate_usage_page(parser, i);
+ }
}
---
And now that I have written this, the check on the size could also be
very well integrated in concatenate_usage_page().
Cheers,
Benjamin
>
> Regards,
> Candle
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
2019-10-09 12:53 [PATCH v2] HID: core: check whether usage page item is after usage id item Candle Sun
2019-10-09 17:01 ` Nicolas Saenz Julienne
@ 2019-10-10 12:24 ` Benjamin Tissoires
2019-10-11 2:38 ` Candle Sun
1 sibling, 1 reply; 10+ messages in thread
From: Benjamin Tissoires @ 2019-10-10 12:24 UTC (permalink / raw)
To: Candle Sun
Cc: Jiri Kosina, Nicolas Saenz Julienne, orson.zhai,
open list:HID CORE LAYER, lkml, Candle Sun, Nianfu Bai
On Wed, Oct 9, 2019 at 2:54 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 adds usage_page_last to flag whether Usage Page is after
> Usage ID items. usage_page_last is false default, it is set as true
> once Usage Page item is encountered and is reverted by next Usage ID
> item.
>
> Usage Page concatenation on the currently defined Usage Page will do
> firstly in Local parsing when Usage ID items encountered.
>
> When Main item is parsing, concatenation will do again with last
> defined Usage Page if usage_page_last flag is true.
>
> Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> ---
> 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 | 31 +++++++++++++++++++++++++------
> include/linux/hid.h | 1 +
> 2 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 3eaee2c..3394222 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -35,6 +35,8 @@
>
> #include "hid-ids.h"
>
> +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
> +
> /*
> * Version Information
> */
> @@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser, unsigned usage, u8 size)
> hid_err(parser->device, "usage index exceeded\n");
> return -1;
> }
> - parser->local.usage[parser->local.usage_index] = usage;
> +
> + if (size <= 2) {
> + parser->local.usage_page_last = false;
> + parser->local.usage[parser->local.usage_index] =
> + GET_COMPLETE_USAGE(parser->global.usage_page, usage);
> + } else {
> + 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 ?
> @@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
>
> case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
> parser->global.usage_page = item_udata(item);
> + parser->local.usage_page_last = true;
> return 0;
>
> case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
> @@ -543,13 +554,21 @@ 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;
> + unsigned int usage_page = parser->global.usage_page;
> +
> + if (!parser->local.usage_page_last)
> + return;
>
> 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_size[i] <= 2) {
> + usage = parser->local.usage[i];
> + parser->local.usage[i] =
> + GET_COMPLETE_USAGE(usage_page, usage);
> + }
> }
>
> /*
> @@ -561,7 +580,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 +791,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);
>
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index cd41f20..2e0ea2f7 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -412,6 +412,7 @@ struct hid_local {
> unsigned usage_minimum;
> unsigned delimiter_depth;
> unsigned delimiter_branch;
> + bool usage_page_last; /* whether usage page is after usage id */
We don't need that extra flag:
if you just check on the last element, you can guess that information:
if ((parser->local.usage[parser->local.usage_index - 1] &
HID_USAGE_PAGE) >> 16 == usage_page)
return 0;
Let's see the next version before requesting too many changes.
And yes, I agree I need the hid-tools patches or I will not merge this
patch (and I will advise Jiri to not take it either).
Cheers,
Benjamin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
2019-10-10 12:16 ` Benjamin Tissoires
@ 2019-10-11 2:08 ` Candle Sun
0 siblings, 0 replies; 10+ messages in thread
From: Candle Sun @ 2019-10-11 2:08 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Nicolas Saenz Julienne, orson.zhai,
open list:HID CORE LAYER, lkml, Candle Sun, Nianfu Bai
On Thu, Oct 10, 2019 at 8:17 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Thu, Oct 10, 2019 at 5:19 AM Candle Sun <candlesea@gmail.com> wrote:
> >
> > On Thu, Oct 10, 2019 at 2:00 AM Jiri Kosina <jikos@kernel.org> wrote:
> > >
> > > On Wed, 9 Oct 2019, Nicolas Saenz Julienne wrote:
> > >
> > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > > > index 3eaee2c..3394222 100644
> > > > > --- a/drivers/hid/hid-core.c
> > > > > +++ b/drivers/hid/hid-core.c
> > > > > @@ -35,6 +35,8 @@
> > > > >
> > > > > #include "hid-ids.h"
> > > > >
> > > > > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
> > > >
> > > > Not sure I like the macro. I'd rather have the explicit code. That said, lets
> > > > see what Benjamin has to say.
> > >
> > > Not sure about Benjamin :) but I personally would ask for putting it
> > > somewhere into hid.h as static inline.
> > >
> > > And even if it's for some reason insisted on this staying macro, please at
> > > least put it as close to the place(s) it's being used as possible, in
> > > order to maintain some code sanity.
> > >
> > > Thanks,
> > >
> > > --
> > > Jiri Kosina
> > > SUSE Labs
> > >
> >
> > Thanks Nicolas and Jiri,
> > If macro is not good, I will change it to static function. But the
> > funciton is only used in hid-core.c,
> > maybe placing it into hid.h is not good?
>
> I would rather use a function too (in hid-core.c, as it's not reused
> anywhere else), and we can make it simpler from the caller point of
> view (if I am not mistaken):
> ---
> static void concatenate_usage_page(struct hid_parser *parser, int index)
> {
> parser->local.usage[index] &= 0xFFFF;
> parser->local.usage[index] |= (parser->global.usage_page & 0xFFFF) << 16;
> }
>
> // Which can then be called as:
> + parser->local.usage[parser->local.usage_index] = usage;
> + if (size <= 2)
> + concatenate_usage_page(parser, parser->local.usage_index);
> +
>
> // And
> 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_size[i] <= 2) {
> + concatenate_usage_page(parser, i);
> + }
> }
> ---
>
> And now that I have written this, the check on the size could also be
> very well integrated in concatenate_usage_page().
>
> Cheers,
> Benjamin
>
Thanks Benjamin's detailed directions. I will amend the patch.
Candle
> >
> > Regards,
> > Candle
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
2019-10-10 12:24 ` Benjamin Tissoires
@ 2019-10-11 2:38 ` Candle Sun
0 siblings, 0 replies; 10+ messages in thread
From: Candle Sun @ 2019-10-11 2:38 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Nicolas Saenz Julienne,
翟京 (Orson Zhai),
open list:HID CORE LAYER, lkml, Candle Sun, Nianfu Bai
On Thu, Oct 10, 2019 at 8:24 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Oct 9, 2019 at 2:54 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 adds usage_page_last to flag whether Usage Page is after
> > Usage ID items. usage_page_last is false default, it is set as true
> > once Usage Page item is encountered and is reverted by next Usage ID
> > item.
> >
> > Usage Page concatenation on the currently defined Usage Page will do
> > firstly in Local parsing when Usage ID items encountered.
> >
> > When Main item is parsing, concatenation will do again with last
> > defined Usage Page if usage_page_last flag is true.
> >
> > Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> > ---
> > 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 | 31 +++++++++++++++++++++++++------
> > include/linux/hid.h | 1 +
> > 2 files changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 3eaee2c..3394222 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -35,6 +35,8 @@
> >
> > #include "hid-ids.h"
> >
> > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
> > +
> > /*
> > * Version Information
> > */
> > @@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser, unsigned usage, u8 size)
> > hid_err(parser->device, "usage index exceeded\n");
> > return -1;
> > }
> > - parser->local.usage[parser->local.usage_index] = usage;
> > +
> > + if (size <= 2) {
> > + parser->local.usage_page_last = false;
> > + parser->local.usage[parser->local.usage_index] =
> > + GET_COMPLETE_USAGE(parser->global.usage_page, usage);
> > + } else {
> > + 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 ?
> > @@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
> >
> > case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
> > parser->global.usage_page = item_udata(item);
> > + parser->local.usage_page_last = true;
> > return 0;
> >
> > case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
> > @@ -543,13 +554,21 @@ 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;
> > + unsigned int usage_page = parser->global.usage_page;
> > +
> > + if (!parser->local.usage_page_last)
> > + return;
> >
> > 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_size[i] <= 2) {
> > + usage = parser->local.usage[i];
> > + parser->local.usage[i] =
> > + GET_COMPLETE_USAGE(usage_page, usage);
> > + }
> > }
> >
> > /*
> > @@ -561,7 +580,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 +791,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);
> >
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index cd41f20..2e0ea2f7 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -412,6 +412,7 @@ struct hid_local {
> > unsigned usage_minimum;
> > unsigned delimiter_depth;
> > unsigned delimiter_branch;
> > + bool usage_page_last; /* whether usage page is after usage id */
>
> We don't need that extra flag:
> if you just check on the last element, you can guess that information:
>
> if ((parser->local.usage[parser->local.usage_index - 1] &
> HID_USAGE_PAGE) >> 16 == usage_page)
> return 0;
>
> Let's see the next version before requesting too many changes.
>
> And yes, I agree I need the hid-tools patches or I will not merge this
> patch (and I will advise Jiri to not take it either).
>
> Cheers,
> Benjamin
>
Thanks Benjamin.
I will rework the patch and add changes on hid-tools to test it more.
Candle
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-11 2:38 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09 12:53 [PATCH v2] HID: core: check whether usage page item is after usage id item Candle Sun
2019-10-09 17:01 ` Nicolas Saenz Julienne
2019-10-09 17:59 ` Jiri Kosina
2019-10-10 3:19 ` Candle Sun
2019-10-10 12:16 ` Benjamin Tissoires
2019-10-11 2:08 ` Candle Sun
2019-10-10 3:05 ` Candle Sun
2019-10-10 7:42 ` Nicolas Saenz Julienne
2019-10-10 12:24 ` Benjamin Tissoires
2019-10-11 2:38 ` Candle Sun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).