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