* [PATCH] HID: core: add usage_page_preceding flag for hid_concatenate_usage_page()
@ 2019-09-30 8:09 Candle Sun
2019-09-30 8:09 ` Candle Sun
2019-09-30 9:36 ` Benjamin Tissoires
0 siblings, 2 replies; 8+ messages in thread
From: Candle Sun @ 2019-09-30 8:09 UTC (permalink / raw)
To: jikos, benjamin.tissoires
Cc: linux-input, linux-kernel, chunyan.zhang, 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 following Usage 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_preceding flag to detect the third pattern.
Usage Page concatenation is done in both Local and Main parsing.
If usage_page_preceding equals 3(the third pattern encountered),
hid_concatenate_usage_page() is jumped.
Signed-off-by: Candle Sun <candle.sun@unisoc.com>
Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
---
drivers/hid/hid-core.c | 21 +++++++++++++++++++--
include/linux/hid.h | 1 +
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3eaee2c..043a232 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -221,7 +221,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 (!parser->local.usage_index && parser->global.usage_page)
+ parser->local.usage_page_preceding = 1;
+ if (parser->local.usage_page_preceding == 2)
+ parser->local.usage_page_preceding = 3;
+ if (size <= 2 && parser->global.usage_page)
+ parser->local.usage[parser->local.usage_index] =
+ (usage & 0xffff) + (parser->global.usage_page << 16);
+ 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 +374,8 @@ 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);
+ if (parser->local.usage_page_preceding == 1)
+ parser->local.usage_page_preceding = 2;
return 0;
case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
@@ -547,9 +557,16 @@ static void hid_concatenate_usage_page(struct hid_parser *parser)
{
int i;
+ if (parser->local.usage_page_preceding == 3) {
+ dbg_hid("Using preceding usage page for final usage\n");
+ 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;
+ parser->local.usage[i] =
+ (parser->global.usage_page << 16)
+ + (parser->local.usage[i] & 0xffff);
}
/*
diff --git a/include/linux/hid.h b/include/linux/hid.h
index cd41f20..7fb6cf3 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;
+ unsigned int usage_page_preceding;
};
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH] HID: core: add usage_page_preceding flag for hid_concatenate_usage_page() 2019-09-30 8:09 [PATCH] HID: core: add usage_page_preceding flag for hid_concatenate_usage_page() Candle Sun @ 2019-09-30 8:09 ` Candle Sun 2019-09-30 9:36 ` Benjamin Tissoires 1 sibling, 0 replies; 8+ messages in thread From: Candle Sun @ 2019-09-30 8:09 UTC (permalink / raw) To: jikos, benjamin.tissoires Cc: linux-input, linux-kernel, chunyan.zhang, 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 following Usage 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_preceding flag to detect the third pattern. Usage Page concatenation is done in both Local and Main parsing. If usage_page_preceding equals 3(the third pattern encountered), hid_concatenate_usage_page() is jumped. Signed-off-by: Candle Sun <candle.sun@unisoc.com> Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com> --- drivers/hid/hid-core.c | 21 +++++++++++++++++++-- include/linux/hid.h | 1 + 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 3eaee2c..043a232 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -221,7 +221,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 (!parser->local.usage_index && parser->global.usage_page) + parser->local.usage_page_preceding = 1; + if (parser->local.usage_page_preceding == 2) + parser->local.usage_page_preceding = 3; + if (size <= 2 && parser->global.usage_page) + parser->local.usage[parser->local.usage_index] = + (usage & 0xffff) + (parser->global.usage_page << 16); + 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 +374,8 @@ 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); + if (parser->local.usage_page_preceding == 1) + parser->local.usage_page_preceding = 2; return 0; case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM: @@ -547,9 +557,16 @@ static void hid_concatenate_usage_page(struct hid_parser *parser) { int i; + if (parser->local.usage_page_preceding == 3) { + dbg_hid("Using preceding usage page for final usage\n"); + 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; + parser->local.usage[i] = + (parser->global.usage_page << 16) + + (parser->local.usage[i] & 0xffff); } /* diff --git a/include/linux/hid.h b/include/linux/hid.h index cd41f20..7fb6cf3 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; + unsigned int usage_page_preceding; }; /* -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: core: add usage_page_preceding flag for hid_concatenate_usage_page() 2019-09-30 8:09 [PATCH] HID: core: add usage_page_preceding flag for hid_concatenate_usage_page() Candle Sun 2019-09-30 8:09 ` Candle Sun @ 2019-09-30 9:36 ` Benjamin Tissoires 2019-09-30 9:36 ` Benjamin Tissoires ` (2 more replies) 1 sibling, 3 replies; 8+ messages in thread From: Benjamin Tissoires @ 2019-09-30 9:36 UTC (permalink / raw) To: Candle Sun Cc: Jiri Kosina, open list:HID CORE LAYER, lkml, chunyan.zhang, Candle Sun, Nianfu Bai, Nicolas Saenz Julienne Hi, [also addingg Nicolas, the author of 58e75155009c] On Mon, Sep 30, 2019 at 10:10 AM 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 following Usage 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_preceding flag to detect the third pattern. > Usage Page concatenation is done in both Local and Main parsing. > If usage_page_preceding equals 3(the third pattern encountered), > hid_concatenate_usage_page() is jumped. For anything core related (and especially the parsing), I am trying to enforce having regression tests. See https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/37 for the one related to 58e75155009c. So I would like to have a similar-ish MR adding the matching tests so I know we won't break this in the future. Few other comments in the code: > > Signed-off-by: Candle Sun <candle.sun@unisoc.com> > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com> > --- > drivers/hid/hid-core.c | 21 +++++++++++++++++++-- > include/linux/hid.h | 1 + > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 3eaee2c..043a232 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -221,7 +221,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 (!parser->local.usage_index && parser->global.usage_page) parser->global.usage_page is never reset, so unless I am misreading, it will always be set to a value except for the very first elements. I am just raising this in case you rely on global.usage_page being null in your algorithm. > + parser->local.usage_page_preceding = 1; > + if (parser->local.usage_page_preceding == 2) > + parser->local.usage_page_preceding = 3; Can't we use an enum at least for those 1, 2, 3 values? Unless you are counting the previous items, in which we should rename the field .usage_page_preceding with something more explicit IMO. > + if (size <= 2 && parser->global.usage_page) > + parser->local.usage[parser->local.usage_index] = > + (usage & 0xffff) + (parser->global.usage_page << 16); we could use a function as this assignment is also reused in hid_concatenate_usage_page() > + 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 +374,8 @@ 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); > + if (parser->local.usage_page_preceding == 1) > + parser->local.usage_page_preceding = 2; > return 0; > > case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM: > @@ -547,9 +557,16 @@ static void hid_concatenate_usage_page(struct hid_parser *parser) > { > int i; > > + if (parser->local.usage_page_preceding == 3) { > + dbg_hid("Using preceding usage page for final usage\n"); > + 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; > + parser->local.usage[i] = > + (parser->global.usage_page << 16) > + + (parser->local.usage[i] & 0xffff); I find the whole logic really hard to follow. I'm not saying you are wrong, but if it's hard to get the concepts behind the various states and this will make it really prone to future errors. I wonder if we should not: - store the current usage page in the current local item as they come in - then in hid_concatenate_usage_page() iterate over the usages in reverse order. We should be able to detect if the last usage page was given for the whole previous range (i.e. not assigned to any local usage) or if it has already been given to a local usage, meaning we should just keep things as it is. Cheers, Benjamin > } > > /* > diff --git a/include/linux/hid.h b/include/linux/hid.h > index cd41f20..7fb6cf3 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; > + unsigned int usage_page_preceding; > }; > > /* > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: core: add usage_page_preceding flag for hid_concatenate_usage_page() 2019-09-30 9:36 ` Benjamin Tissoires @ 2019-09-30 9:36 ` Benjamin Tissoires 2019-09-30 13:24 ` Nicolas Saenz Julienne 2019-09-30 15:20 ` Candle Sun 2 siblings, 0 replies; 8+ messages in thread From: Benjamin Tissoires @ 2019-09-30 9:36 UTC (permalink / raw) To: Candle Sun Cc: Jiri Kosina, open list:HID CORE LAYER, lkml, chunyan.zhang, Candle Sun, Nianfu Bai, Nicolas Saenz Julienne Hi, [also addingg Nicolas, the author of 58e75155009c] On Mon, Sep 30, 2019 at 10:10 AM 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 following Usage 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_preceding flag to detect the third pattern. > Usage Page concatenation is done in both Local and Main parsing. > If usage_page_preceding equals 3(the third pattern encountered), > hid_concatenate_usage_page() is jumped. For anything core related (and especially the parsing), I am trying to enforce having regression tests. See https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/37 for the one related to 58e75155009c. So I would like to have a similar-ish MR adding the matching tests so I know we won't break this in the future. Few other comments in the code: > > Signed-off-by: Candle Sun <candle.sun@unisoc.com> > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com> > --- > drivers/hid/hid-core.c | 21 +++++++++++++++++++-- > include/linux/hid.h | 1 + > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 3eaee2c..043a232 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -221,7 +221,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 (!parser->local.usage_index && parser->global.usage_page) parser->global.usage_page is never reset, so unless I am misreading, it will always be set to a value except for the very first elements. I am just raising this in case you rely on global.usage_page being null in your algorithm. > + parser->local.usage_page_preceding = 1; > + if (parser->local.usage_page_preceding == 2) > + parser->local.usage_page_preceding = 3; Can't we use an enum at least for those 1, 2, 3 values? Unless you are counting the previous items, in which we should rename the field .usage_page_preceding with something more explicit IMO. > + if (size <= 2 && parser->global.usage_page) > + parser->local.usage[parser->local.usage_index] = > + (usage & 0xffff) + (parser->global.usage_page << 16); we could use a function as this assignment is also reused in hid_concatenate_usage_page() > + 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 +374,8 @@ 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); > + if (parser->local.usage_page_preceding == 1) > + parser->local.usage_page_preceding = 2; > return 0; > > case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM: > @@ -547,9 +557,16 @@ static void hid_concatenate_usage_page(struct hid_parser *parser) > { > int i; > > + if (parser->local.usage_page_preceding == 3) { > + dbg_hid("Using preceding usage page for final usage\n"); > + 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; > + parser->local.usage[i] = > + (parser->global.usage_page << 16) > + + (parser->local.usage[i] & 0xffff); I find the whole logic really hard to follow. I'm not saying you are wrong, but if it's hard to get the concepts behind the various states and this will make it really prone to future errors. I wonder if we should not: - store the current usage page in the current local item as they come in - then in hid_concatenate_usage_page() iterate over the usages in reverse order. We should be able to detect if the last usage page was given for the whole previous range (i.e. not assigned to any local usage) or if it has already been given to a local usage, meaning we should just keep things as it is. Cheers, Benjamin > } > > /* > diff --git a/include/linux/hid.h b/include/linux/hid.h > index cd41f20..7fb6cf3 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; > + unsigned int usage_page_preceding; > }; > > /* > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: core: add usage_page_preceding flag for hid_concatenate_usage_page() 2019-09-30 9:36 ` Benjamin Tissoires 2019-09-30 9:36 ` Benjamin Tissoires @ 2019-09-30 13:24 ` Nicolas Saenz Julienne 2019-09-30 13:24 ` Nicolas Saenz Julienne 2019-09-30 15:20 ` Candle Sun 2 siblings, 1 reply; 8+ messages in thread From: Nicolas Saenz Julienne @ 2019-09-30 13:24 UTC (permalink / raw) To: Benjamin Tissoires, Candle Sun Cc: Jiri Kosina, open list:HID CORE LAYER, lkml, chunyan.zhang, Candle Sun, Nianfu Bai [-- Attachment #1: Type: text/plain, Size: 8007 bytes --] On Mon, 2019-09-30 at 11:36 +0200, Benjamin Tissoires wrote: > Hi, > > [also addingg Nicolas, the author of 58e75155009c] Thanks! > > On Mon, Sep 30, 2019 at 10:10 AM 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 following Usage 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_preceding flag to detect the third pattern. > > Usage Page concatenation is done in both Local and Main parsing. > > If usage_page_preceding equals 3(the third pattern encountered), > > hid_concatenate_usage_page() is jumped. > > For anything core related (and especially the parsing), I am trying to > enforce having regression tests. > See https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/37 > for the one related to 58e75155009c. > > So I would like to have a similar-ish MR adding the matching tests so > I know we won't break this in the future. > > Few other comments in the code: > > > Signed-off-by: Candle Sun <candle.sun@unisoc.com> > > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com> > > --- > > drivers/hid/hid-core.c | 21 +++++++++++++++++++-- > > include/linux/hid.h | 1 + > > 2 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > index 3eaee2c..043a232 100644 > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -221,7 +221,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 (!parser->local.usage_index && parser->global.usage_page) > > parser->global.usage_page is never reset, so unless I am misreading, > it will always be set to a value except for the very first elements. > I am just raising this in case you rely on global.usage_page being > null in your algorithm. > > > + parser->local.usage_page_preceding = 1; > > + if (parser->local.usage_page_preceding == 2) > > + parser->local.usage_page_preceding = 3; > > Can't we use an enum at least for those 1, 2, 3 values? > Unless you are counting the previous items, in which we should rename > the field .usage_page_preceding with something more explicit IMO. > > > > + if (size <= 2 && parser->global.usage_page) > > + parser->local.usage[parser->local.usage_index] = > > + (usage & 0xffff) + (parser->global.usage_page << > > 16); > > we could use a function as this assignment is also reused in > hid_concatenate_usage_page() > > > + 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 +374,8 @@ 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); > > + if (parser->local.usage_page_preceding == 1) > > + parser->local.usage_page_preceding = 2; > > return 0; > > > > case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM: > > @@ -547,9 +557,16 @@ static void hid_concatenate_usage_page(struct > > hid_parser *parser) > > { > > int i; > > > > + if (parser->local.usage_page_preceding == 3) { > > + dbg_hid("Using preceding usage page for final usage\n"); > > + 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; > > + parser->local.usage[i] = > > + (parser->global.usage_page << 16) > > + + (parser->local.usage[i] & 0xffff); > > I find the whole logic really hard to follow. I'm not saying you are > wrong, but if it's hard to get the concepts behind the various states > and this will make it really prone to future errors. > > I wonder if we should not: > - store the current usage page in the current local item as they come in > - then in hid_concatenate_usage_page() iterate over the usages in > reverse order. We should be able to detect if the last usage page was > given for the whole previous range (i.e. not assigned to any local > usage) or if it has already been given to a local usage, meaning we > should just keep things as it is. I agree this would be simpler to understand. All this would also fix: https://lkml.org/lkml/2019/6/14/468 I suggest we agree on a rule of thumb and add it as a code comment. My take on it would be: "Usage pages are always concatenated upon parsing a local usage. If a usage page is defined after the local usages ennumeration, we concatenate this usage page with all the local usages. This excludes the case were a usage page is set in between the local usages and then another usage page is set just before the main item. That said I doubt we'll ever see that one, as it makes no sense. FWIW we could still detect it. Just my two cents, regards, Nicolas [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: core: add usage_page_preceding flag for hid_concatenate_usage_page() 2019-09-30 13:24 ` Nicolas Saenz Julienne @ 2019-09-30 13:24 ` Nicolas Saenz Julienne 0 siblings, 0 replies; 8+ messages in thread From: Nicolas Saenz Julienne @ 2019-09-30 13:24 UTC (permalink / raw) To: Benjamin Tissoires, Candle Sun Cc: Jiri Kosina, open list:HID CORE LAYER, lkml, chunyan.zhang, Candle Sun, Nianfu Bai [-- Attachment #1: Type: text/plain, Size: 8007 bytes --] On Mon, 2019-09-30 at 11:36 +0200, Benjamin Tissoires wrote: > Hi, > > [also addingg Nicolas, the author of 58e75155009c] Thanks! > > On Mon, Sep 30, 2019 at 10:10 AM 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 following Usage 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_preceding flag to detect the third pattern. > > Usage Page concatenation is done in both Local and Main parsing. > > If usage_page_preceding equals 3(the third pattern encountered), > > hid_concatenate_usage_page() is jumped. > > For anything core related (and especially the parsing), I am trying to > enforce having regression tests. > See https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/37 > for the one related to 58e75155009c. > > So I would like to have a similar-ish MR adding the matching tests so > I know we won't break this in the future. > > Few other comments in the code: > > > Signed-off-by: Candle Sun <candle.sun@unisoc.com> > > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com> > > --- > > drivers/hid/hid-core.c | 21 +++++++++++++++++++-- > > include/linux/hid.h | 1 + > > 2 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > index 3eaee2c..043a232 100644 > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -221,7 +221,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 (!parser->local.usage_index && parser->global.usage_page) > > parser->global.usage_page is never reset, so unless I am misreading, > it will always be set to a value except for the very first elements. > I am just raising this in case you rely on global.usage_page being > null in your algorithm. > > > + parser->local.usage_page_preceding = 1; > > + if (parser->local.usage_page_preceding == 2) > > + parser->local.usage_page_preceding = 3; > > Can't we use an enum at least for those 1, 2, 3 values? > Unless you are counting the previous items, in which we should rename > the field .usage_page_preceding with something more explicit IMO. > > > > + if (size <= 2 && parser->global.usage_page) > > + parser->local.usage[parser->local.usage_index] = > > + (usage & 0xffff) + (parser->global.usage_page << > > 16); > > we could use a function as this assignment is also reused in > hid_concatenate_usage_page() > > > + 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 +374,8 @@ 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); > > + if (parser->local.usage_page_preceding == 1) > > + parser->local.usage_page_preceding = 2; > > return 0; > > > > case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM: > > @@ -547,9 +557,16 @@ static void hid_concatenate_usage_page(struct > > hid_parser *parser) > > { > > int i; > > > > + if (parser->local.usage_page_preceding == 3) { > > + dbg_hid("Using preceding usage page for final usage\n"); > > + 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; > > + parser->local.usage[i] = > > + (parser->global.usage_page << 16) > > + + (parser->local.usage[i] & 0xffff); > > I find the whole logic really hard to follow. I'm not saying you are > wrong, but if it's hard to get the concepts behind the various states > and this will make it really prone to future errors. > > I wonder if we should not: > - store the current usage page in the current local item as they come in > - then in hid_concatenate_usage_page() iterate over the usages in > reverse order. We should be able to detect if the last usage page was > given for the whole previous range (i.e. not assigned to any local > usage) or if it has already been given to a local usage, meaning we > should just keep things as it is. I agree this would be simpler to understand. All this would also fix: https://lkml.org/lkml/2019/6/14/468 I suggest we agree on a rule of thumb and add it as a code comment. My take on it would be: "Usage pages are always concatenated upon parsing a local usage. If a usage page is defined after the local usages ennumeration, we concatenate this usage page with all the local usages. This excludes the case were a usage page is set in between the local usages and then another usage page is set just before the main item. That said I doubt we'll ever see that one, as it makes no sense. FWIW we could still detect it. Just my two cents, regards, Nicolas [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: core: add usage_page_preceding flag for hid_concatenate_usage_page() 2019-09-30 9:36 ` Benjamin Tissoires 2019-09-30 9:36 ` Benjamin Tissoires 2019-09-30 13:24 ` Nicolas Saenz Julienne @ 2019-09-30 15:20 ` Candle Sun 2019-09-30 15:20 ` Candle Sun 2 siblings, 1 reply; 8+ messages in thread From: Candle Sun @ 2019-09-30 15:20 UTC (permalink / raw) To: Benjamin Tissoires Cc: Jiri Kosina, open list:HID CORE LAYER, lkml, chunyan.zhang, Candle Sun, Nianfu Bai, Nicolas Saenz Julienne Hi Benjamin, Thank you very much for the detailed review. On Mon, Sep 30, 2019 at 5:36 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi, > > [also addingg Nicolas, the author of 58e75155009c] > > On Mon, Sep 30, 2019 at 10:10 AM 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 following Usage 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_preceding flag to detect the third pattern. > > Usage Page concatenation is done in both Local and Main parsing. > > If usage_page_preceding equals 3(the third pattern encountered), > > hid_concatenate_usage_page() is jumped. > > For anything core related (and especially the parsing), I am trying to > enforce having regression tests. > See https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/37 > for the one related to 58e75155009c. > > So I would like to have a similar-ish MR adding the matching tests so > I know we won't break this in the future. > > Few other comments in the code: > Thanks. Candle > > > > Signed-off-by: Candle Sun <candle.sun@unisoc.com> > > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com> > > --- > > drivers/hid/hid-core.c | 21 +++++++++++++++++++-- > > include/linux/hid.h | 1 + > > 2 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > index 3eaee2c..043a232 100644 > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -221,7 +221,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 (!parser->local.usage_index && parser->global.usage_page) > > parser->global.usage_page is never reset, so unless I am misreading, > it will always be set to a value except for the very first elements. > I am just raising this in case you rely on global.usage_page being > null in your algorithm. > The patch doesn't rely on the global.usage_page being null. Checking whether the global.usage_page is zero here aims for ignoring the case when the first Usage Page of the whole report descriptor is after some defined Usage items: USAGE (Button 1) 09 01 USAGE_PAGE (Button) 05 09 REPORT_SIZE (1) 75 01 REPORT_COUNT (1) 95 01 LOGICAL_MINIMUM (0) 15 00 LOGICAL_MAXIMUM (1) 25 01 INPUT (Data,Var,Abs) 81 02 Of course, it maybe never occur, but it is allowed by the Spec. Regards, Candle > > + parser->local.usage_page_preceding = 1; > > + if (parser->local.usage_page_preceding == 2) > > + parser->local.usage_page_preceding = 3; > > Can't we use an enum at least for those 1, 2, 3 values? > Unless you are counting the previous items, in which we should rename > the field .usage_page_preceding with something more explicit IMO. > > Yes, using enum type for the values is much better, will add it in next version. Candle > > + if (size <= 2 && parser->global.usage_page) > > + parser->local.usage[parser->local.usage_index] = > > + (usage & 0xffff) + (parser->global.usage_page << 16); > > we could use a function as this assignment is also reused in > hid_concatenate_usage_page() > Yes, using one function is better, will amend it in next version. Candle > > + 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 +374,8 @@ 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); > > + if (parser->local.usage_page_preceding == 1) > > + parser->local.usage_page_preceding = 2; > > return 0; > > > > case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM: > > @@ -547,9 +557,16 @@ static void hid_concatenate_usage_page(struct hid_parser *parser) > > { > > int i; > > > > + if (parser->local.usage_page_preceding == 3) { > > + dbg_hid("Using preceding usage page for final usage\n"); > > + 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; > > + parser->local.usage[i] = > > + (parser->global.usage_page << 16) > > + + (parser->local.usage[i] & 0xffff); > > I find the whole logic really hard to follow. I'm not saying you are > wrong, but if it's hard to get the concepts behind the various states > and this will make it really prone to future errors. > By reading "Device Class Definition for Human Interface Devices (HID) Version 1.11" Spec more times, I think Spec regards both of the following cases legal, they are not contradictory: - Usages after some already defined Usage Page "If the bSize field = 1 or 2 then the Usage is interpreted as an unsigned value that selects a Usage ID on the *currently defined Usage Page*." - No Usage Page defined before Usages (Usage Page is *LAST") "When the parser encounters a main item it concatenates the *last declared Usage Page* with a Usage to form a complete usage value." (Here I think *last* means no Usage Page is already defined before the Usages.) HID device designer can choose either pattern for the device. So the descriptor parser must have the ability to recognize the real complete Usage value. In my opinion, the following descriptor which exsits in practice doesn't strictly conform the HID Spec: ------------------------ 05 01 Usage Page (Desktop), ; Generic desktop controls (01h) 09 06 Usage (Keyboard), ; Keyboard (06h, application collection) a1 01 Collection (Application), 05 07 Usage Page (Keyboard), ; Keyboard/keypad (07h) 19 e0 Usage Minimum (KB Leftcontrol), ; Keyboard left control (E0h, dynamic value) 29 e7 Usage Maximum (KB Right GUI), ; Keyboard right GUI (E7h, dynamic value) 15 00 Logical Minimum (0), 25 01 Logical Maximum (1), 75 01 Report Size (1), 95 08 Report Count (8), 81 02 Input (Variable), 75 08 Report Size (8), 95 01 Report Count (1), 81 01 Input (Constant), 05 08 Usage Page (LED), ; LEDs (08h) 19 01 Usage Minimum (01h), 29 03 Usage Maximum (03h), 75 01 Report Size (1), 95 03 Report Count (3), 91 02 Output (Variable), 95 01 Report Count (1), 75 05 Report Size (5), 91 01 Output (Constant), 15 00 Logical Minimum (0), 26 ff 00 Logical Maximum (255), 19 00 Usage Minimum (00h), 2a ff 00 Usage Maximum (FFh), 05 07 Usage Page (Keyboard), ; Keyboard/keypad (07h) 75 08 Report Size (8), 95 06 Report Count (6), 81 00 Input, 05 01 Usage Page (Desktop), ; Generic desktop controls (01h) 0a 68 01 Usage (0168h), 15 80 Logical Minimum (-128), 25 7f Logical Maximum (127), 95 01 Report Count (1), 75 08 Report Size (8), 81 02 Input (Variable), c0 End Collection ------------------------------------ Nicolas' patch(58e75155009c) can make parser to recognize above device's intention. But it also produces side effects on the compound Usage/Usage Page sequences, such as the following case which conforms the Spec: ------------------------------------ Usage Page (X) Usage (X.1) Usage Page (Y) Usage (Y.1) Input/Output/Feature ------------------------------------ Usage Page concatenation in Main item will always use the last Usage Page for all preceding usages. This patch is using the usage_page_preceding flag to find above compound sequence and jump hid_concatenate_usage_page() while keeping Nicolas' patch(58e75155009c) for some keyboards. Candle > I wonder if we should not: > - store the current usage page in the current local item as they come in > - then in hid_concatenate_usage_page() iterate over the usages in > reverse order. We should be able to detect if the last usage page was > given for the whole previous range (i.e. not assigned to any local > usage) or if it has already been given to a local usage, meaning we > should just keep things as it is. > > Cheers, > Benjamin > Sorry, I don't know how to do it now. Candle > > } > > > > /* > > diff --git a/include/linux/hid.h b/include/linux/hid.h > > index cd41f20..7fb6cf3 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; > > + unsigned int usage_page_preceding; > > }; > > > > /* > > -- > > 2.7.4 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: core: add usage_page_preceding flag for hid_concatenate_usage_page() 2019-09-30 15:20 ` Candle Sun @ 2019-09-30 15:20 ` Candle Sun 0 siblings, 0 replies; 8+ messages in thread From: Candle Sun @ 2019-09-30 15:20 UTC (permalink / raw) To: Benjamin Tissoires Cc: Jiri Kosina, open list:HID CORE LAYER, lkml, chunyan.zhang, Candle Sun, Nianfu Bai, Nicolas Saenz Julienne Hi Benjamin, Thank you very much for the detailed review. On Mon, Sep 30, 2019 at 5:36 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi, > > [also addingg Nicolas, the author of 58e75155009c] > > On Mon, Sep 30, 2019 at 10:10 AM 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 following Usage 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_preceding flag to detect the third pattern. > > Usage Page concatenation is done in both Local and Main parsing. > > If usage_page_preceding equals 3(the third pattern encountered), > > hid_concatenate_usage_page() is jumped. > > For anything core related (and especially the parsing), I am trying to > enforce having regression tests. > See https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/37 > for the one related to 58e75155009c. > > So I would like to have a similar-ish MR adding the matching tests so > I know we won't break this in the future. > > Few other comments in the code: > Thanks. Candle > > > > Signed-off-by: Candle Sun <candle.sun@unisoc.com> > > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com> > > --- > > drivers/hid/hid-core.c | 21 +++++++++++++++++++-- > > include/linux/hid.h | 1 + > > 2 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > index 3eaee2c..043a232 100644 > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -221,7 +221,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 (!parser->local.usage_index && parser->global.usage_page) > > parser->global.usage_page is never reset, so unless I am misreading, > it will always be set to a value except for the very first elements. > I am just raising this in case you rely on global.usage_page being > null in your algorithm. > The patch doesn't rely on the global.usage_page being null. Checking whether the global.usage_page is zero here aims for ignoring the case when the first Usage Page of the whole report descriptor is after some defined Usage items: USAGE (Button 1) 09 01 USAGE_PAGE (Button) 05 09 REPORT_SIZE (1) 75 01 REPORT_COUNT (1) 95 01 LOGICAL_MINIMUM (0) 15 00 LOGICAL_MAXIMUM (1) 25 01 INPUT (Data,Var,Abs) 81 02 Of course, it maybe never occur, but it is allowed by the Spec. Regards, Candle > > + parser->local.usage_page_preceding = 1; > > + if (parser->local.usage_page_preceding == 2) > > + parser->local.usage_page_preceding = 3; > > Can't we use an enum at least for those 1, 2, 3 values? > Unless you are counting the previous items, in which we should rename > the field .usage_page_preceding with something more explicit IMO. > > Yes, using enum type for the values is much better, will add it in next version. Candle > > + if (size <= 2 && parser->global.usage_page) > > + parser->local.usage[parser->local.usage_index] = > > + (usage & 0xffff) + (parser->global.usage_page << 16); > > we could use a function as this assignment is also reused in > hid_concatenate_usage_page() > Yes, using one function is better, will amend it in next version. Candle > > + 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 +374,8 @@ 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); > > + if (parser->local.usage_page_preceding == 1) > > + parser->local.usage_page_preceding = 2; > > return 0; > > > > case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM: > > @@ -547,9 +557,16 @@ static void hid_concatenate_usage_page(struct hid_parser *parser) > > { > > int i; > > > > + if (parser->local.usage_page_preceding == 3) { > > + dbg_hid("Using preceding usage page for final usage\n"); > > + 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; > > + parser->local.usage[i] = > > + (parser->global.usage_page << 16) > > + + (parser->local.usage[i] & 0xffff); > > I find the whole logic really hard to follow. I'm not saying you are > wrong, but if it's hard to get the concepts behind the various states > and this will make it really prone to future errors. > By reading "Device Class Definition for Human Interface Devices (HID) Version 1.11" Spec more times, I think Spec regards both of the following cases legal, they are not contradictory: - Usages after some already defined Usage Page "If the bSize field = 1 or 2 then the Usage is interpreted as an unsigned value that selects a Usage ID on the *currently defined Usage Page*." - No Usage Page defined before Usages (Usage Page is *LAST") "When the parser encounters a main item it concatenates the *last declared Usage Page* with a Usage to form a complete usage value." (Here I think *last* means no Usage Page is already defined before the Usages.) HID device designer can choose either pattern for the device. So the descriptor parser must have the ability to recognize the real complete Usage value. In my opinion, the following descriptor which exsits in practice doesn't strictly conform the HID Spec: ------------------------ 05 01 Usage Page (Desktop), ; Generic desktop controls (01h) 09 06 Usage (Keyboard), ; Keyboard (06h, application collection) a1 01 Collection (Application), 05 07 Usage Page (Keyboard), ; Keyboard/keypad (07h) 19 e0 Usage Minimum (KB Leftcontrol), ; Keyboard left control (E0h, dynamic value) 29 e7 Usage Maximum (KB Right GUI), ; Keyboard right GUI (E7h, dynamic value) 15 00 Logical Minimum (0), 25 01 Logical Maximum (1), 75 01 Report Size (1), 95 08 Report Count (8), 81 02 Input (Variable), 75 08 Report Size (8), 95 01 Report Count (1), 81 01 Input (Constant), 05 08 Usage Page (LED), ; LEDs (08h) 19 01 Usage Minimum (01h), 29 03 Usage Maximum (03h), 75 01 Report Size (1), 95 03 Report Count (3), 91 02 Output (Variable), 95 01 Report Count (1), 75 05 Report Size (5), 91 01 Output (Constant), 15 00 Logical Minimum (0), 26 ff 00 Logical Maximum (255), 19 00 Usage Minimum (00h), 2a ff 00 Usage Maximum (FFh), 05 07 Usage Page (Keyboard), ; Keyboard/keypad (07h) 75 08 Report Size (8), 95 06 Report Count (6), 81 00 Input, 05 01 Usage Page (Desktop), ; Generic desktop controls (01h) 0a 68 01 Usage (0168h), 15 80 Logical Minimum (-128), 25 7f Logical Maximum (127), 95 01 Report Count (1), 75 08 Report Size (8), 81 02 Input (Variable), c0 End Collection ------------------------------------ Nicolas' patch(58e75155009c) can make parser to recognize above device's intention. But it also produces side effects on the compound Usage/Usage Page sequences, such as the following case which conforms the Spec: ------------------------------------ Usage Page (X) Usage (X.1) Usage Page (Y) Usage (Y.1) Input/Output/Feature ------------------------------------ Usage Page concatenation in Main item will always use the last Usage Page for all preceding usages. This patch is using the usage_page_preceding flag to find above compound sequence and jump hid_concatenate_usage_page() while keeping Nicolas' patch(58e75155009c) for some keyboards. Candle > I wonder if we should not: > - store the current usage page in the current local item as they come in > - then in hid_concatenate_usage_page() iterate over the usages in > reverse order. We should be able to detect if the last usage page was > given for the whole previous range (i.e. not assigned to any local > usage) or if it has already been given to a local usage, meaning we > should just keep things as it is. > > Cheers, > Benjamin > Sorry, I don't know how to do it now. Candle > > } > > > > /* > > diff --git a/include/linux/hid.h b/include/linux/hid.h > > index cd41f20..7fb6cf3 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; > > + unsigned int usage_page_preceding; > > }; > > > > /* > > -- > > 2.7.4 > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-09-30 15:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-09-30 8:09 [PATCH] HID: core: add usage_page_preceding flag for hid_concatenate_usage_page() Candle Sun 2019-09-30 8:09 ` Candle Sun 2019-09-30 9:36 ` Benjamin Tissoires 2019-09-30 9:36 ` Benjamin Tissoires 2019-09-30 13:24 ` Nicolas Saenz Julienne 2019-09-30 13:24 ` Nicolas Saenz Julienne 2019-09-30 15:20 ` Candle Sun 2019-09-30 15:20 ` Candle Sun
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox