Re: [PATCH v2] HID: core: check whether usage page item is after usage id item

[Nicolas Saenz Julienne <nsaenzjulienne@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 7129 bytes --]

On Wed, 2019-10-09 at 20:53 +0800, Candle Sun wrote:
> From: Candle Sun <candle.sun@unisoc.com>
> 
> Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> to Main item") adds support for Usage Page item after Usage ID items
> (such as keyboards manufactured by Primax).
> 
> Usage Page concatenation in Main item works well for following report
> descriptor patterns:
> 
>     USAGE_PAGE (Keyboard)                   05 07
>     USAGE_MINIMUM (Keyboard LeftControl)    19 E0
>     USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
>     LOGICAL_MINIMUM (0)                     15 00
>     LOGICAL_MAXIMUM (1)                     25 01
>     REPORT_SIZE (1)                         75 01
>     REPORT_COUNT (8)                        95 08
>     INPUT (Data,Var,Abs)                    81 02
> 
> -------------
> 
>     USAGE_MINIMUM (Keyboard LeftControl)    19 E0
>     USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
>     LOGICAL_MINIMUM (0)                     15 00
>     LOGICAL_MAXIMUM (1)                     25 01
>     REPORT_SIZE (1)                         75 01
>     REPORT_COUNT (8)                        95 08
>     USAGE_PAGE (Keyboard)                   05 07
>     INPUT (Data,Var,Abs)                    81 02
> 
> But it makes the parser act wrong for the following report
> descriptor pattern(such as some Gamepads):
> 
>     USAGE_PAGE (Button)                     05 09
>     USAGE (Button 1)                        09 01
>     USAGE (Button 2)                        09 02
>     USAGE (Button 4)                        09 04
>     USAGE (Button 5)                        09 05
>     USAGE (Button 7)                        09 07
>     USAGE (Button 8)                        09 08
>     USAGE (Button 14)                       09 0E
>     USAGE (Button 15)                       09 0F
>     USAGE (Button 13)                       09 0D
>     USAGE_PAGE (Consumer Devices)           05 0C
>     USAGE (Back)                            0a 24 02
>     USAGE (HomePage)                        0a 23 02
>     LOGICAL_MINIMUM (0)                     15 00
>     LOGICAL_MAXIMUM (1)                     25 01
>     REPORT_SIZE (1)                         75 01
>     REPORT_COUNT (11)                       95 0B
>     INPUT (Data,Var,Abs)                    81 02
> 
> With Usage Page concatenation in Main item, parser recognizes all the
> 11 Usages as consumer keys, it is not the HID device's real intention.
> 
> This patch adds usage_page_last to flag whether Usage Page is after
> Usage ID items. usage_page_last is false default, it is set as true
> once Usage Page item is encountered and is reverted by next Usage ID
> item.
> 
> Usage Page concatenation on the currently defined Usage Page will do
> firstly in Local parsing when Usage ID items encountered.
> 
> When Main item is parsing, concatenation will do again with last
> defined Usage Page if usage_page_last flag is true.

Functionally I think this is the right approach. Sadly I don't have access to
any  Primax device anymore so I can't test it. But I suggest you update
hid-tools' parser and add a new unit test to verify we aren't missing anything.

You can base your code on this:

https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/37/commits

> Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> ---
> Changes in v2:
> - Update patch title
> - Add GET_COMPLETE_USAGE macro
> - Change the logic of checking whether to concatenate usage page again
>   in main parsing
> ---
>  drivers/hid/hid-core.c | 31 +++++++++++++++++++++++++------
>  include/linux/hid.h    |  1 +
>  2 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 3eaee2c..3394222 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -35,6 +35,8 @@
>  
>  #include "hid-ids.h"
>  
> +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))

Not sure I like the macro. I'd rather have the explicit code. That said, lets
see what Benjamin has to say.

> +
>  /*
>   * Version Information
>   */
> @@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser,
> unsigned usage, u8 size)
>  		hid_err(parser->device, "usage index exceeded\n");
>  		return -1;
>  	}
> -	parser->local.usage[parser->local.usage_index] = usage;
> +
> +	if (size <= 2) {
> +		parser->local.usage_page_last = false;
> +		parser->local.usage[parser->local.usage_index] =
> +			GET_COMPLETE_USAGE(parser->global.usage_page, usage);
> +	} else {
> +		parser->local.usage[parser->local.usage_index] = usage;
> +	}
> +
>  	parser->local.usage_size[parser->local.usage_index] = size;
>  	parser->local.collection_index[parser->local.usage_index] =
>  		parser->collection_stack_ptr ?
> @@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser,
> struct hid_item *item)
>  
>  	case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
>  		parser->global.usage_page = item_udata(item);
> +		parser->local.usage_page_last = true;
>  		return 0;
>  
>  	case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
> @@ -543,13 +554,21 @@ static int hid_parser_local(struct hid_parser *parser,
> struct hid_item *item)
>   * usage value."
>   */

I'd expand the comment above to further explain what we're doing here.

>  
> -static void hid_concatenate_usage_page(struct hid_parser *parser)
> +static void hid_concatenate_last_usage_page(struct hid_parser *parser)
>  {
>  	int i;
> +	unsigned int usage;
> +	unsigned int usage_page = parser->global.usage_page;
> +
> +	if (!parser->local.usage_page_last)
> +		return;
>  
>  	for (i = 0; i < parser->local.usage_index; i++)

Technically correct but it's preferred if you use braces here.

> -		if (parser->local.usage_size[i] <= 2)
> -			parser->local.usage[i] += parser->global.usage_page <<
> 16;
> +		if (parser->local.usage_size[i] <= 2) {
> +			usage = parser->local.usage[i];
> +			parser->local.usage[i] =
> +				GET_COMPLETE_USAGE(usage_page, usage);
> +		}
>  }
>  
>  /*
> @@ -561,7 +580,7 @@ static int hid_parser_main(struct hid_parser *parser,
> struct hid_item *item)
>  	__u32 data;
>  	int ret;
>  
> -	hid_concatenate_usage_page(parser);
> +	hid_concatenate_last_usage_page(parser);
>  
>  	data = item_udata(item);
>  
> @@ -772,7 +791,7 @@ static int hid_scan_main(struct hid_parser *parser, struct
> hid_item *item)
>  	__u32 data;
>  	int i;
>  
> -	hid_concatenate_usage_page(parser);
> +	hid_concatenate_last_usage_page(parser);
>  
>  	data = item_udata(item);
>  
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index cd41f20..2e0ea2f7 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -412,6 +412,7 @@ struct hid_local {
>  	unsigned usage_minimum;
>  	unsigned delimiter_depth;
>  	unsigned delimiter_branch;
> +	bool usage_page_last;      /* whether usage page is after usage id */
>  };
>  
>  /*

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]