| Message ID | 20191021073819.18181-1-candlesea@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Benjamin Tissoires |
| Headers | show |
| Series | [v3] HID: core: check whether usage page item is after usage id item | expand |
Hi, On Mon, Oct 21, 2019 at 3:38 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 v3: > - Rework the GET_COMPLETE_USAGE macro as static complete_usage() > function > - Add some code comments for usage_page_last > > Changes in v2: > - Update patch title > - Add GET_COMPLETE_USAGE macro > - Change the logic of checking whether to concatenate usage page again > in main parsing > --- > drivers/hid/hid-core.c | 42 +++++++++++++++++++++++++++++++++++++----- > include/linux/hid.h | 1 + > 2 files changed, 38 insertions(+), 5 deletions(-) > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 3eaee2c37931..779b7798dae8 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -211,6 +211,18 @@ static unsigned hid_lookup_collection(struct hid_parser *parser, unsigned type) > return 0; /* we know nothing about this usage type */ > } > > +/* > + * Concatenate usage which defines 16 bits or less with the > + * currently defined usage page to form a 32 bit usage > + */ > + > +static void complete_usage(struct hid_parser *parser, unsigned int index) > +{ > + parser->local.usage[index] &= 0xFFFF; > + parser->local.usage[index] |= > + (parser->global.usage_page & 0xFFFF) << 16; > +} > + > /* > * Add a usage to the temporary parser table. > */ > @@ -221,7 +233,18 @@ 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 Usage item only includes usage id, concatenate it with > + * currently defined usage page and clear usage_page_last flag > + */ > + if (size <= 2) { > + parser->local.usage_page_last = false; > + complete_usage(parser, parser->local.usage_index); > + } 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 +389,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); > + /* Regard Usage Page is after Usage ID items */ > + parser->local.usage_page_last = true; > return 0; > > case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM: > @@ -543,13 +568,20 @@ 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; > > + /* > + * Concatenate usage page again only on detecting some Usage Page > + * is really after Usage ID items > + */ > + 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; > + complete_usage(parser, i); > } > > /* > @@ -561,7 +593,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 +804,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 cd41f209043f..2e0ea2f7ec5c 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 */ > }; > Hi Benjamin, Here I still use the usage_page_last flag, not using following method you provided in v2: if ((parser->local.usage[parser->local.usage_index - 1] & HID_USAGE_PAGE) >> 16 == usage_page) return 0; Because last usage maybe one Extended Usage, some logic for checking it should be added. It will make the code obscure. Using one more member in struct hid_local is straightforward and maybe better. Candle > /* > -- > 2.17.1 >
Hi Candle, On Mon, Oct 21, 2019 at 9:54 AM Candle Sun <candlesea@gmail.com> wrote: > > Hi, > > > On Mon, Oct 21, 2019 at 3:38 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 v3: > > - Rework the GET_COMPLETE_USAGE macro as static complete_usage() > > function > > - Add some code comments for usage_page_last > > > > Changes in v2: > > - Update patch title > > - Add GET_COMPLETE_USAGE macro > > - Change the logic of checking whether to concatenate usage page again > > in main parsing > > --- > > drivers/hid/hid-core.c | 42 +++++++++++++++++++++++++++++++++++++----- > > include/linux/hid.h | 1 + > > 2 files changed, 38 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > index 3eaee2c37931..779b7798dae8 100644 > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -211,6 +211,18 @@ static unsigned hid_lookup_collection(struct hid_parser *parser, unsigned type) > > return 0; /* we know nothing about this usage type */ > > } > > > > +/* > > + * Concatenate usage which defines 16 bits or less with the > > + * currently defined usage page to form a 32 bit usage > > + */ > > + > > +static void complete_usage(struct hid_parser *parser, unsigned int index) > > +{ > > + parser->local.usage[index] &= 0xFFFF; > > + parser->local.usage[index] |= > > + (parser->global.usage_page & 0xFFFF) << 16; > > +} > > + > > /* > > * Add a usage to the temporary parser table. > > */ > > @@ -221,7 +233,18 @@ 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; This broke my CI, and it turns out that installing the patch on an actual laptop, the touchpad, touchscreen were not present, nor any HID devices plugged in. Basically, complete_usage() doesn't append the usage, and now we are never assigning a usage to any field. > > + > > + /* > > + * If Usage item only includes usage id, concatenate it with > > + * currently defined usage page and clear usage_page_last flag > > + */ > > + if (size <= 2) { > > + parser->local.usage_page_last = false; > > + complete_usage(parser, parser->local.usage_index); > > + } 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 +389,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); > > + /* Regard Usage Page is after Usage ID items */ > > + parser->local.usage_page_last = true; > > return 0; > > > > case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM: > > @@ -543,13 +568,20 @@ 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; > > > > + /* > > + * Concatenate usage page again only on detecting some Usage Page > > + * is really after Usage ID items > > + */ > > + 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; > > + complete_usage(parser, i); > > } > > > > /* > > @@ -561,7 +593,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 +804,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 cd41f209043f..2e0ea2f7ec5c 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 */ > > }; > > > > Hi Benjamin, > Here I still use the usage_page_last flag, not using following method > you provided in v2: > > if ((parser->local.usage[parser->local.usage_index - 1] & > HID_USAGE_PAGE) >> 16 == usage_page) > return 0; > > Because last usage maybe one Extended Usage, some logic for checking > it should be added. > It will make the code obscure. Using one more member in struct > hid_local is straightforward > and maybe better. Still not convinced by this. Please see below for a less intrusive patch, which is also shorter. --- diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 63fdbf09b044..00ea04fb1be3 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -211,6 +211,18 @@ static unsigned hid_lookup_collection(struct hid_parser *parser, unsigned type) return 0; /* we know nothing about this usage type */ } +/* + * Concatenate usage which defines 16 bits or less with the + * currently defined usage page to form a 32 bit usage + */ + +static void complete_usage(struct hid_parser *parser, unsigned int index) +{ + parser->local.usage[index] &= 0xFFFF; + parser->local.usage[index] |= + (parser->global.usage_page & 0xFFFF) << 16; +} + /* * Add a usage to the temporary parser table. */ @@ -222,6 +234,14 @@ static int hid_add_usage(struct hid_parser *parser, unsigned usage, u8 size) return -1; } parser->local.usage[parser->local.usage_index] = usage; + + /* + * If Usage item only includes usage id, concatenate it with + * currently defined usage page + */ + if (size <= 2) + complete_usage(parser, parser->local.usage_index); + parser->local.usage_size[parser->local.usage_index] = size; parser->local.collection_index[parser->local.usage_index] = parser->collection_stack_ptr ? @@ -546,10 +566,28 @@ static int hid_parser_local(struct hid_parser *parser, struct hid_item *item) static void hid_concatenate_usage_page(struct hid_parser *parser) { int i; + uint16_t usage_page, current_page; - for (i = 0; i < parser->local.usage_index; i++) - if (parser->local.usage_size[i] <= 2) - parser->local.usage[i] += parser->global.usage_page << 16; + if (!parser->local.usage_index) + return; + + usage_page = parser->global.usage_page; + + /* + * Concatenate usage page again only if the last declared Usage Page + * has not been already used in the previous Usages + */ + for (i = parser->local.usage_index - 1; i >= 0; i--) { + if (parser->local.usage_size[i] > 2) + /* ignore extended usages */ + continue; + + current_page = parser->local.usage[i] >> 16; + if (current_page == usage_page) + break; + + complete_usage(parser, i); + } } /* --- Isn't that better? I tested this against https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/58, which you kindly submitted and it seems to do the job. Cheers, Benjamin
Hi Benjamin, On Tue, Oct 22, 2019 at 5:48 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi Candle, > > On Mon, Oct 21, 2019 at 9:54 AM Candle Sun <candlesea@gmail.com> wrote: > > > > Hi, > > > > > > On Mon, Oct 21, 2019 at 3:38 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 v3: > > > - Rework the GET_COMPLETE_USAGE macro as static complete_usage() > > > function > > > - Add some code comments for usage_page_last > > > > > > Changes in v2: > > > - Update patch title > > > - Add GET_COMPLETE_USAGE macro > > > - Change the logic of checking whether to concatenate usage page again > > > in main parsing > > > --- > > > drivers/hid/hid-core.c | 42 +++++++++++++++++++++++++++++++++++++----- > > > include/linux/hid.h | 1 + > > > 2 files changed, 38 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > > index 3eaee2c37931..779b7798dae8 100644 > > > --- a/drivers/hid/hid-core.c > > > +++ b/drivers/hid/hid-core.c > > > @@ -211,6 +211,18 @@ static unsigned hid_lookup_collection(struct hid_parser *parser, unsigned type) > > > return 0; /* we know nothing about this usage type */ > > > } > > > > > > +/* > > > + * Concatenate usage which defines 16 bits or less with the > > > + * currently defined usage page to form a 32 bit usage > > > + */ > > > + > > > +static void complete_usage(struct hid_parser *parser, unsigned int index) > > > +{ > > > + parser->local.usage[index] &= 0xFFFF; > > > + parser->local.usage[index] |= > > > + (parser->global.usage_page & 0xFFFF) << 16; > > > +} > > > + > > > /* > > > * Add a usage to the temporary parser table. > > > */ > > > @@ -221,7 +233,18 @@ 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; > > This broke my CI, and it turns out that installing the patch on an > actual laptop, the touchpad, touchscreen were not present, nor any HID > devices plugged in. > > Basically, complete_usage() doesn't append the usage, and now we are > never assigning a usage to any field. > So sorry, it is really really a BIG bug, I should be more careful. (╯﹏╰) Candle > > > + > > > + /* > > > + * If Usage item only includes usage id, concatenate it with > > > + * currently defined usage page and clear usage_page_last flag > > > + */ > > > + if (size <= 2) { > > > + parser->local.usage_page_last = false; > > > + complete_usage(parser, parser->local.usage_index); > > > + } 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 +389,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); > > > + /* Regard Usage Page is after Usage ID items */ > > > + parser->local.usage_page_last = true; > > > return 0; > > > > > > case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM: > > > @@ -543,13 +568,20 @@ 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; > > > > > > + /* > > > + * Concatenate usage page again only on detecting some Usage Page > > > + * is really after Usage ID items > > > + */ > > > + 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; > > > + complete_usage(parser, i); > > > } > > > > > > /* > > > @@ -561,7 +593,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 +804,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 cd41f209043f..2e0ea2f7ec5c 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 */ > > > }; > > > > > > > Hi Benjamin, > > Here I still use the usage_page_last flag, not using following method > > you provided in v2: > > > > if ((parser->local.usage[parser->local.usage_index - 1] & > > HID_USAGE_PAGE) >> 16 == usage_page) > > return 0; > > > > Because last usage maybe one Extended Usage, some logic for checking > > it should be added. > > It will make the code obscure. Using one more member in struct > > hid_local is straightforward > > and maybe better. > > > Still not convinced by this. Please see below for a less intrusive > patch, which is also shorter. > > --- > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 63fdbf09b044..00ea04fb1be3 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -211,6 +211,18 @@ static unsigned hid_lookup_collection(struct > hid_parser *parser, unsigned type) > return 0; /* we know nothing about this usage type */ > } > > +/* > + * Concatenate usage which defines 16 bits or less with the > + * currently defined usage page to form a 32 bit usage > + */ > + > +static void complete_usage(struct hid_parser *parser, unsigned int index) > +{ > + parser->local.usage[index] &= 0xFFFF; > + parser->local.usage[index] |= > + (parser->global.usage_page & 0xFFFF) << 16; > +} > + > /* > * Add a usage to the temporary parser table. > */ > @@ -222,6 +234,14 @@ static int hid_add_usage(struct hid_parser > *parser, unsigned usage, u8 size) > return -1; > } > parser->local.usage[parser->local.usage_index] = usage; > + > + /* > + * If Usage item only includes usage id, concatenate it with > + * currently defined usage page > + */ > + if (size <= 2) > + complete_usage(parser, parser->local.usage_index); > + > parser->local.usage_size[parser->local.usage_index] = size; > parser->local.collection_index[parser->local.usage_index] = > parser->collection_stack_ptr ? > @@ -546,10 +566,28 @@ static int hid_parser_local(struct hid_parser > *parser, struct hid_item *item) > static void hid_concatenate_usage_page(struct hid_parser *parser) > { > int i; > + uint16_t usage_page, current_page; > > - for (i = 0; i < parser->local.usage_index; i++) > - if (parser->local.usage_size[i] <= 2) > - parser->local.usage[i] += parser->global.usage_page << 16; > + if (!parser->local.usage_index) > + return; > + > + usage_page = parser->global.usage_page; > + > + /* > + * Concatenate usage page again only if the last declared Usage Page > + * has not been already used in the previous Usages > + */ > + for (i = parser->local.usage_index - 1; i >= 0; i--) { > + if (parser->local.usage_size[i] > 2) > + /* ignore extended usages */ > + continue; > + > + current_page = parser->local.usage[i] >> 16; > + if (current_page == usage_page) > + break; > + > + complete_usage(parser, i); > + } > } > > /* > --- > > Isn't that better? > > I tested this against > https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/58, > which you kindly submitted and it seems to do the job. > > Cheers, > Benjamin > OK. I will amend the patch. Best regards, Candle
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 3eaee2c37931..779b7798dae8 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -211,6 +211,18 @@ static unsigned hid_lookup_collection(struct hid_parser *parser, unsigned type) return 0; /* we know nothing about this usage type */ } +/* + * Concatenate usage which defines 16 bits or less with the + * currently defined usage page to form a 32 bit usage + */ + +static void complete_usage(struct hid_parser *parser, unsigned int index) +{ + parser->local.usage[index] &= 0xFFFF; + parser->local.usage[index] |= + (parser->global.usage_page & 0xFFFF) << 16; +} + /* * Add a usage to the temporary parser table. */ @@ -221,7 +233,18 @@ 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 Usage item only includes usage id, concatenate it with + * currently defined usage page and clear usage_page_last flag + */ + if (size <= 2) { + parser->local.usage_page_last = false; + complete_usage(parser, parser->local.usage_index); + } 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 +389,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); + /* Regard Usage Page is after Usage ID items */ + parser->local.usage_page_last = true; return 0; case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM: @@ -543,13 +568,20 @@ 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; + /* + * Concatenate usage page again only on detecting some Usage Page + * is really after Usage ID items + */ + 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; + complete_usage(parser, i); } /* @@ -561,7 +593,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 +804,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 cd41f209043f..2e0ea2f7ec5c 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 */ }; /*