[v2] HID: core: check whether usage page item is after usage id item
diff mbox series

Message ID 1570625609-11083-1-git-send-email-candlesea@gmail.com
State Superseded
Headers show
Series
  • [v2] HID: core: check whether usage page item is after usage id item
Related show

Commit Message

Candle Sun Oct. 9, 2019, 12:53 p.m. UTC
From: Candle Sun <candle.sun@unisoc.com>

Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
to Main item") adds support for Usage Page item after Usage ID items
(such as keyboards manufactured by Primax).

Usage Page concatenation in Main item works well for following report
descriptor patterns:

    USAGE_PAGE (Keyboard)                   05 07
    USAGE_MINIMUM (Keyboard LeftControl)    19 E0
    USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
    LOGICAL_MINIMUM (0)                     15 00
    LOGICAL_MAXIMUM (1)                     25 01
    REPORT_SIZE (1)                         75 01
    REPORT_COUNT (8)                        95 08
    INPUT (Data,Var,Abs)                    81 02

-------------

    USAGE_MINIMUM (Keyboard LeftControl)    19 E0
    USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
    LOGICAL_MINIMUM (0)                     15 00
    LOGICAL_MAXIMUM (1)                     25 01
    REPORT_SIZE (1)                         75 01
    REPORT_COUNT (8)                        95 08
    USAGE_PAGE (Keyboard)                   05 07
    INPUT (Data,Var,Abs)                    81 02

But it makes the parser act wrong for the following report
descriptor pattern(such as some Gamepads):

    USAGE_PAGE (Button)                     05 09
    USAGE (Button 1)                        09 01
    USAGE (Button 2)                        09 02
    USAGE (Button 4)                        09 04
    USAGE (Button 5)                        09 05
    USAGE (Button 7)                        09 07
    USAGE (Button 8)                        09 08
    USAGE (Button 14)                       09 0E
    USAGE (Button 15)                       09 0F
    USAGE (Button 13)                       09 0D
    USAGE_PAGE (Consumer Devices)           05 0C
    USAGE (Back)                            0a 24 02
    USAGE (HomePage)                        0a 23 02
    LOGICAL_MINIMUM (0)                     15 00
    LOGICAL_MAXIMUM (1)                     25 01
    REPORT_SIZE (1)                         75 01
    REPORT_COUNT (11)                       95 0B
    INPUT (Data,Var,Abs)                    81 02

With Usage Page concatenation in Main item, parser recognizes all the
11 Usages as consumer keys, it is not the HID device's real intention.

This patch adds usage_page_last to flag whether Usage Page is after
Usage ID items. usage_page_last is false default, it is set as true
once Usage Page item is encountered and is reverted by next Usage ID
item.

Usage Page concatenation on the currently defined Usage Page will do
firstly in Local parsing when Usage ID items encountered.

When Main item is parsing, concatenation will do again with last
defined Usage Page if usage_page_last flag is true.

Signed-off-by: Candle Sun <candle.sun@unisoc.com>
Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
---
Changes in v2:
- Update patch title
- Add GET_COMPLETE_USAGE macro
- Change the logic of checking whether to concatenate usage page again
  in main parsing
---
 drivers/hid/hid-core.c | 31 +++++++++++++++++++++++++------
 include/linux/hid.h    |  1 +
 2 files changed, 26 insertions(+), 6 deletions(-)

Comments

Nicolas Saenz Julienne Oct. 9, 2019, 5:01 p.m. UTC | #1
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
Jiri Kosina Oct. 9, 2019, 5:59 p.m. UTC | #2
On Wed, 9 Oct 2019, Nicolas Saenz Julienne wrote:

> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 3eaee2c..3394222 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -35,6 +35,8 @@
> >  
> >  #include "hid-ids.h"
> >  
> > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
> 
> Not sure I like the macro. I'd rather have the explicit code. That said, lets
> see what Benjamin has to say.

Not sure about Benjamin :) but I personally would ask for putting it 
somewhere into hid.h as static inline.

And even if it's for some reason insisted on this staying macro, please at 
least put it as close to the place(s) it's being used as possible, in 
order to maintain some code sanity.

Thanks,
Candle Sun Oct. 10, 2019, 3:05 a.m. UTC | #3
On Thu, Oct 10, 2019 at 1:01 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> On Wed, 2019-10-09 at 20:53 +0800, Candle Sun wrote:
> > From: Candle Sun <candle.sun@unisoc.com>
> >
> > Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> > to Main item") adds support for Usage Page item after Usage ID items
> > (such as keyboards manufactured by Primax).
> >
> > Usage Page concatenation in Main item works well for following report
> > descriptor patterns:
> >
> >     USAGE_PAGE (Keyboard)                   05 07
> >     USAGE_MINIMUM (Keyboard LeftControl)    19 E0
> >     USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
> >     LOGICAL_MINIMUM (0)                     15 00
> >     LOGICAL_MAXIMUM (1)                     25 01
> >     REPORT_SIZE (1)                         75 01
> >     REPORT_COUNT (8)                        95 08
> >     INPUT (Data,Var,Abs)                    81 02
> >
> > -------------
> >
> >     USAGE_MINIMUM (Keyboard LeftControl)    19 E0
> >     USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
> >     LOGICAL_MINIMUM (0)                     15 00
> >     LOGICAL_MAXIMUM (1)                     25 01
> >     REPORT_SIZE (1)                         75 01
> >     REPORT_COUNT (8)                        95 08
> >     USAGE_PAGE (Keyboard)                   05 07
> >     INPUT (Data,Var,Abs)                    81 02
> >
> > But it makes the parser act wrong for the following report
> > descriptor pattern(such as some Gamepads):
> >
> >     USAGE_PAGE (Button)                     05 09
> >     USAGE (Button 1)                        09 01
> >     USAGE (Button 2)                        09 02
> >     USAGE (Button 4)                        09 04
> >     USAGE (Button 5)                        09 05
> >     USAGE (Button 7)                        09 07
> >     USAGE (Button 8)                        09 08
> >     USAGE (Button 14)                       09 0E
> >     USAGE (Button 15)                       09 0F
> >     USAGE (Button 13)                       09 0D
> >     USAGE_PAGE (Consumer Devices)           05 0C
> >     USAGE (Back)                            0a 24 02
> >     USAGE (HomePage)                        0a 23 02
> >     LOGICAL_MINIMUM (0)                     15 00
> >     LOGICAL_MAXIMUM (1)                     25 01
> >     REPORT_SIZE (1)                         75 01
> >     REPORT_COUNT (11)                       95 0B
> >     INPUT (Data,Var,Abs)                    81 02
> >
> > With Usage Page concatenation in Main item, parser recognizes all the
> > 11 Usages as consumer keys, it is not the HID device's real intention.
> >
> > This patch adds usage_page_last to flag whether Usage Page is after
> > Usage ID items. usage_page_last is false default, it is set as true
> > once Usage Page item is encountered and is reverted by next Usage ID
> > item.
> >
> > Usage Page concatenation on the currently defined Usage Page will do
> > firstly in Local parsing when Usage ID items encountered.
> >
> > When Main item is parsing, concatenation will do again with last
> > defined Usage Page if usage_page_last flag is true.
>
> Functionally I think this is the right approach. Sadly I don't have access to
> any  Primax device anymore so I can't test it. But I suggest you update
> hid-tools' parser and add a new unit test to verify we aren't missing anything.
>
> You can base your code on this:
>
> https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/37/commits
>

Thanks Nicolas. I will check and try to do it.

Candle

> > Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> > ---
> > Changes in v2:
> > - Update patch title
> > - Add GET_COMPLETE_USAGE macro
> > - Change the logic of checking whether to concatenate usage page again
> >   in main parsing
> > ---
> >  drivers/hid/hid-core.c | 31 +++++++++++++++++++++++++------
> >  include/linux/hid.h    |  1 +
> >  2 files changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 3eaee2c..3394222 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -35,6 +35,8 @@
> >
> >  #include "hid-ids.h"
> >
> > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
>
> Not sure I like the macro. I'd rather have the explicit code. That said, lets
> see what Benjamin has to say.
>
> > +
> >  /*
> >   * Version Information
> >   */
> > @@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser,
> > unsigned usage, u8 size)
> >               hid_err(parser->device, "usage index exceeded\n");
> >               return -1;
> >       }
> > -     parser->local.usage[parser->local.usage_index] = usage;
> > +
> > +     if (size <= 2) {
> > +             parser->local.usage_page_last = false;
> > +             parser->local.usage[parser->local.usage_index] =
> > +                     GET_COMPLETE_USAGE(parser->global.usage_page, usage);
> > +     } else {
> > +             parser->local.usage[parser->local.usage_index] = usage;
> > +     }
> > +
> >       parser->local.usage_size[parser->local.usage_index] = size;
> >       parser->local.collection_index[parser->local.usage_index] =
> >               parser->collection_stack_ptr ?
> > @@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser,
> > struct hid_item *item)
> >
> >       case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
> >               parser->global.usage_page = item_udata(item);
> > +             parser->local.usage_page_last = true;
> >               return 0;
> >
> >       case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
> > @@ -543,13 +554,21 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> >   * usage value."
> >   */
>
> I'd expand the comment above to further explain what we're doing here.
>

OK, some comment here is better, I will add it.

Candle

> >
> > -static void hid_concatenate_usage_page(struct hid_parser *parser)
> > +static void hid_concatenate_last_usage_page(struct hid_parser *parser)
> >  {
> >       int i;
> > +     unsigned int usage;
> > +     unsigned int usage_page = parser->global.usage_page;
> > +
> > +     if (!parser->local.usage_page_last)
> > +             return;
> >
> >       for (i = 0; i < parser->local.usage_index; i++)
>
> Technically correct but it's preferred if you use braces here.
>

Nicolas, do you mean this:

@@ -563,12 +563,13 @@ static void
hid_concatenate_last_usage_page(struct hid_parser *parser)
        if (!parser->local.usage_page_last)
                return;

-       for (i = 0; i < parser->local.usage_index; i++)
+       for (i = 0; i < parser->local.usage_index; i++) {
                if (parser->local.usage_size[i] <= 2) {
                        usage = parser->local.usage[i];
                        parser->local.usage[i] =
                                GET_COMPLETE_USAGE(usage_page, usage);
                }
+       }
 }

Candle

> > -             if (parser->local.usage_size[i] <= 2)
> > -                     parser->local.usage[i] += parser->global.usage_page <<
> > 16;
> > +             if (parser->local.usage_size[i] <= 2) {
> > +                     usage = parser->local.usage[i];
> > +                     parser->local.usage[i] =
> > +                             GET_COMPLETE_USAGE(usage_page, usage);
> > +             }
> >  }
> >
> >  /*
> > @@ -561,7 +580,7 @@ static int hid_parser_main(struct hid_parser *parser,
> > struct hid_item *item)
> >       __u32 data;
> >       int ret;
> >
> > -     hid_concatenate_usage_page(parser);
> > +     hid_concatenate_last_usage_page(parser);
> >
> >       data = item_udata(item);
> >
> > @@ -772,7 +791,7 @@ static int hid_scan_main(struct hid_parser *parser, struct
> > hid_item *item)
> >       __u32 data;
> >       int i;
> >
> > -     hid_concatenate_usage_page(parser);
> > +     hid_concatenate_last_usage_page(parser);
> >
> >       data = item_udata(item);
> >
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index cd41f20..2e0ea2f7 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -412,6 +412,7 @@ struct hid_local {
> >       unsigned usage_minimum;
> >       unsigned delimiter_depth;
> >       unsigned delimiter_branch;
> > +     bool usage_page_last;      /* whether usage page is after usage id */
> >  };
> >
> >  /*
>
> Regards,
> Nicolas
>
Candle Sun Oct. 10, 2019, 3:19 a.m. UTC | #4
On Thu, Oct 10, 2019 at 2:00 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Wed, 9 Oct 2019, Nicolas Saenz Julienne wrote:
>
> > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > index 3eaee2c..3394222 100644
> > > --- a/drivers/hid/hid-core.c
> > > +++ b/drivers/hid/hid-core.c
> > > @@ -35,6 +35,8 @@
> > >
> > >  #include "hid-ids.h"
> > >
> > > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
> >
> > Not sure I like the macro. I'd rather have the explicit code. That said, lets
> > see what Benjamin has to say.
>
> Not sure about Benjamin :) but I personally would ask for putting it
> somewhere into hid.h as static inline.
>
> And even if it's for some reason insisted on this staying macro, please at
> least put it as close to the place(s) it's being used as possible, in
> order to maintain some code sanity.
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>

Thanks Nicolas and Jiri,
If macro is not good, I will change it to static function. But the
funciton is only used in hid-core.c,
maybe placing it into hid.h is not good?

Regards,
Candle
Nicolas Saenz Julienne Oct. 10, 2019, 7:42 a.m. UTC | #5
On Thu, 2019-10-10 at 11:05 +0800, Candle Sun wrote:
> > > -static void hid_concatenate_usage_page(struct hid_parser *parser)
> > > +static void hid_concatenate_last_usage_page(struct hid_parser *parser)
> > >  {
> > >       int i;
> > > +     unsigned int usage;
> > > +     unsigned int usage_page = parser->global.usage_page;
> > > +
> > > +     if (!parser->local.usage_page_last)
> > > +             return;
> > > 
> > >       for (i = 0; i < parser->local.usage_index; i++)
> > 
> > Technically correct but it's preferred if you use braces here.
> > 
> 
> Nicolas, do you mean this:
> 
> @@ -563,12 +563,13 @@ static void
> hid_concatenate_last_usage_page(struct hid_parser *parser)
>         if (!parser->local.usage_page_last)
>                 return;
> 
> -       for (i = 0; i < parser->local.usage_index; i++)
> +       for (i = 0; i < parser->local.usage_index; i++) {
>                 if (parser->local.usage_size[i] <= 2) {
>                         usage = parser->local.usage[i];
>                         parser->local.usage[i] =
>                                 GET_COMPLETE_USAGE(usage_page, usage);
>                 }
> +       }
>  }

Yes, thanks!

Regards,
Nicolas
Benjamin Tissoires Oct. 10, 2019, 12:16 p.m. UTC | #6
On Thu, Oct 10, 2019 at 5:19 AM Candle Sun <candlesea@gmail.com> wrote:
>
> On Thu, Oct 10, 2019 at 2:00 AM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > On Wed, 9 Oct 2019, Nicolas Saenz Julienne wrote:
> >
> > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > > index 3eaee2c..3394222 100644
> > > > --- a/drivers/hid/hid-core.c
> > > > +++ b/drivers/hid/hid-core.c
> > > > @@ -35,6 +35,8 @@
> > > >
> > > >  #include "hid-ids.h"
> > > >
> > > > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
> > >
> > > Not sure I like the macro. I'd rather have the explicit code. That said, lets
> > > see what Benjamin has to say.
> >
> > Not sure about Benjamin :) but I personally would ask for putting it
> > somewhere into hid.h as static inline.
> >
> > And even if it's for some reason insisted on this staying macro, please at
> > least put it as close to the place(s) it's being used as possible, in
> > order to maintain some code sanity.
> >
> > Thanks,
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >
>
> Thanks Nicolas and Jiri,
> If macro is not good, I will change it to static function. But the
> funciton is only used in hid-core.c,
> maybe placing it into hid.h is not good?

I would rather use a function too (in hid-core.c, as it's not reused
anywhere else), and we can make it simpler from the caller point of
view (if I am not mistaken):
---
static void concatenate_usage_page(struct hid_parser *parser, int index)
{
    parser->local.usage[index] &= 0xFFFF;
    parser->local.usage[index] |= (parser->global.usage_page & 0xFFFF) << 16;
}

// Which can then be called as:
+       parser->local.usage[parser->local.usage_index] = usage;
+       if (size <= 2)
+               concatenate_usage_page(parser, parser->local.usage_index);
+

// And
        for (i = 0; i < parser->local.usage_index; i++)
-               if (parser->local.usage_size[i] <= 2)
-                       parser->local.usage[i] +=
parser->global.usage_page << 16;
+               if (parser->local.usage_size[i] <= 2) {
+                       concatenate_usage_page(parser, i);
+               }
 }
---

And now that I have written this, the check on the size could also be
very well integrated in concatenate_usage_page().

Cheers,
Benjamin

>
> Regards,
> Candle
Benjamin Tissoires Oct. 10, 2019, 12:24 p.m. UTC | #7
On Wed, Oct 9, 2019 at 2:54 PM Candle Sun <candlesea@gmail.com> wrote:
>
> From: Candle Sun <candle.sun@unisoc.com>
>
> Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> to Main item") adds support for Usage Page item after Usage ID items
> (such as keyboards manufactured by Primax).
>
> Usage Page concatenation in Main item works well for following report
> descriptor patterns:
>
>     USAGE_PAGE (Keyboard)                   05 07
>     USAGE_MINIMUM (Keyboard LeftControl)    19 E0
>     USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
>     LOGICAL_MINIMUM (0)                     15 00
>     LOGICAL_MAXIMUM (1)                     25 01
>     REPORT_SIZE (1)                         75 01
>     REPORT_COUNT (8)                        95 08
>     INPUT (Data,Var,Abs)                    81 02
>
> -------------
>
>     USAGE_MINIMUM (Keyboard LeftControl)    19 E0
>     USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
>     LOGICAL_MINIMUM (0)                     15 00
>     LOGICAL_MAXIMUM (1)                     25 01
>     REPORT_SIZE (1)                         75 01
>     REPORT_COUNT (8)                        95 08
>     USAGE_PAGE (Keyboard)                   05 07
>     INPUT (Data,Var,Abs)                    81 02
>
> But it makes the parser act wrong for the following report
> descriptor pattern(such as some Gamepads):
>
>     USAGE_PAGE (Button)                     05 09
>     USAGE (Button 1)                        09 01
>     USAGE (Button 2)                        09 02
>     USAGE (Button 4)                        09 04
>     USAGE (Button 5)                        09 05
>     USAGE (Button 7)                        09 07
>     USAGE (Button 8)                        09 08
>     USAGE (Button 14)                       09 0E
>     USAGE (Button 15)                       09 0F
>     USAGE (Button 13)                       09 0D
>     USAGE_PAGE (Consumer Devices)           05 0C
>     USAGE (Back)                            0a 24 02
>     USAGE (HomePage)                        0a 23 02
>     LOGICAL_MINIMUM (0)                     15 00
>     LOGICAL_MAXIMUM (1)                     25 01
>     REPORT_SIZE (1)                         75 01
>     REPORT_COUNT (11)                       95 0B
>     INPUT (Data,Var,Abs)                    81 02
>
> With Usage Page concatenation in Main item, parser recognizes all the
> 11 Usages as consumer keys, it is not the HID device's real intention.
>
> This patch adds usage_page_last to flag whether Usage Page is after
> Usage ID items. usage_page_last is false default, it is set as true
> once Usage Page item is encountered and is reverted by next Usage ID
> item.
>
> Usage Page concatenation on the currently defined Usage Page will do
> firstly in Local parsing when Usage ID items encountered.
>
> When Main item is parsing, concatenation will do again with last
> defined Usage Page if usage_page_last flag is true.
>
> Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> ---
> Changes in v2:
> - Update patch title
> - Add GET_COMPLETE_USAGE macro
> - Change the logic of checking whether to concatenate usage page again
>   in main parsing
> ---
>  drivers/hid/hid-core.c | 31 +++++++++++++++++++++++++------
>  include/linux/hid.h    |  1 +
>  2 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 3eaee2c..3394222 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -35,6 +35,8 @@
>
>  #include "hid-ids.h"
>
> +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
> +
>  /*
>   * Version Information
>   */
> @@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser, unsigned usage, u8 size)
>                 hid_err(parser->device, "usage index exceeded\n");
>                 return -1;
>         }
> -       parser->local.usage[parser->local.usage_index] = usage;
> +
> +       if (size <= 2) {
> +               parser->local.usage_page_last = false;
> +               parser->local.usage[parser->local.usage_index] =
> +                       GET_COMPLETE_USAGE(parser->global.usage_page, usage);
> +       } else {
> +               parser->local.usage[parser->local.usage_index] = usage;
> +       }
> +
>         parser->local.usage_size[parser->local.usage_index] = size;
>         parser->local.collection_index[parser->local.usage_index] =
>                 parser->collection_stack_ptr ?
> @@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
>
>         case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
>                 parser->global.usage_page = item_udata(item);
> +               parser->local.usage_page_last = true;
>                 return 0;
>
>         case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
> @@ -543,13 +554,21 @@ static int hid_parser_local(struct hid_parser *parser, struct hid_item *item)
>   * usage value."
>   */
>
> -static void hid_concatenate_usage_page(struct hid_parser *parser)
> +static void hid_concatenate_last_usage_page(struct hid_parser *parser)
>  {
>         int i;
> +       unsigned int usage;
> +       unsigned int usage_page = parser->global.usage_page;
> +
> +       if (!parser->local.usage_page_last)
> +               return;
>
>         for (i = 0; i < parser->local.usage_index; i++)
> -               if (parser->local.usage_size[i] <= 2)
> -                       parser->local.usage[i] += parser->global.usage_page << 16;
> +               if (parser->local.usage_size[i] <= 2) {
> +                       usage = parser->local.usage[i];
> +                       parser->local.usage[i] =
> +                               GET_COMPLETE_USAGE(usage_page, usage);
> +               }
>  }
>
>  /*
> @@ -561,7 +580,7 @@ static int hid_parser_main(struct hid_parser *parser, struct hid_item *item)
>         __u32 data;
>         int ret;
>
> -       hid_concatenate_usage_page(parser);
> +       hid_concatenate_last_usage_page(parser);
>
>         data = item_udata(item);
>
> @@ -772,7 +791,7 @@ static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
>         __u32 data;
>         int i;
>
> -       hid_concatenate_usage_page(parser);
> +       hid_concatenate_last_usage_page(parser);
>
>         data = item_udata(item);
>
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index cd41f20..2e0ea2f7 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -412,6 +412,7 @@ struct hid_local {
>         unsigned usage_minimum;
>         unsigned delimiter_depth;
>         unsigned delimiter_branch;
> +       bool usage_page_last;      /* whether usage page is after usage id */

We don't need that extra flag:
if you just check on the last element, you can guess that information:

        if ((parser->local.usage[parser->local.usage_index - 1] &
HID_USAGE_PAGE) >> 16 == usage_page)
              return 0;

Let's see the next version before requesting too many changes.

And yes, I agree I need the hid-tools patches or I will not merge this
patch (and I will advise Jiri to not take it either).

Cheers,
Benjamin
Candle Sun Oct. 11, 2019, 2:08 a.m. UTC | #8
On Thu, Oct 10, 2019 at 8:17 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Thu, Oct 10, 2019 at 5:19 AM Candle Sun <candlesea@gmail.com> wrote:
> >
> > On Thu, Oct 10, 2019 at 2:00 AM Jiri Kosina <jikos@kernel.org> wrote:
> > >
> > > On Wed, 9 Oct 2019, Nicolas Saenz Julienne wrote:
> > >
> > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > > > index 3eaee2c..3394222 100644
> > > > > --- a/drivers/hid/hid-core.c
> > > > > +++ b/drivers/hid/hid-core.c
> > > > > @@ -35,6 +35,8 @@
> > > > >
> > > > >  #include "hid-ids.h"
> > > > >
> > > > > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
> > > >
> > > > Not sure I like the macro. I'd rather have the explicit code. That said, lets
> > > > see what Benjamin has to say.
> > >
> > > Not sure about Benjamin :) but I personally would ask for putting it
> > > somewhere into hid.h as static inline.
> > >
> > > And even if it's for some reason insisted on this staying macro, please at
> > > least put it as close to the place(s) it's being used as possible, in
> > > order to maintain some code sanity.
> > >
> > > Thanks,
> > >
> > > --
> > > Jiri Kosina
> > > SUSE Labs
> > >
> >
> > Thanks Nicolas and Jiri,
> > If macro is not good, I will change it to static function. But the
> > funciton is only used in hid-core.c,
> > maybe placing it into hid.h is not good?
>
> I would rather use a function too (in hid-core.c, as it's not reused
> anywhere else), and we can make it simpler from the caller point of
> view (if I am not mistaken):
> ---
> static void concatenate_usage_page(struct hid_parser *parser, int index)
> {
>     parser->local.usage[index] &= 0xFFFF;
>     parser->local.usage[index] |= (parser->global.usage_page & 0xFFFF) << 16;
> }
>
> // Which can then be called as:
> +       parser->local.usage[parser->local.usage_index] = usage;
> +       if (size <= 2)
> +               concatenate_usage_page(parser, parser->local.usage_index);
> +
>
> // And
>         for (i = 0; i < parser->local.usage_index; i++)
> -               if (parser->local.usage_size[i] <= 2)
> -                       parser->local.usage[i] +=
> parser->global.usage_page << 16;
> +               if (parser->local.usage_size[i] <= 2) {
> +                       concatenate_usage_page(parser, i);
> +               }
>  }
> ---
>
> And now that I have written this, the check on the size could also be
> very well integrated in concatenate_usage_page().
>
> Cheers,
> Benjamin
>

Thanks Benjamin's detailed directions. I will amend the patch.

Candle

> >
> > Regards,
> > Candle
>
Candle Sun Oct. 11, 2019, 2:38 a.m. UTC | #9
On Thu, Oct 10, 2019 at 8:24 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Oct 9, 2019 at 2:54 PM Candle Sun <candlesea@gmail.com> wrote:
> >
> > From: Candle Sun <candle.sun@unisoc.com>
> >
> > Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> > to Main item") adds support for Usage Page item after Usage ID items
> > (such as keyboards manufactured by Primax).
> >
> > Usage Page concatenation in Main item works well for following report
> > descriptor patterns:
> >
> >     USAGE_PAGE (Keyboard)                   05 07
> >     USAGE_MINIMUM (Keyboard LeftControl)    19 E0
> >     USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
> >     LOGICAL_MINIMUM (0)                     15 00
> >     LOGICAL_MAXIMUM (1)                     25 01
> >     REPORT_SIZE (1)                         75 01
> >     REPORT_COUNT (8)                        95 08
> >     INPUT (Data,Var,Abs)                    81 02
> >
> > -------------
> >
> >     USAGE_MINIMUM (Keyboard LeftControl)    19 E0
> >     USAGE_MAXIMUM (Keyboard Right GUI)      29 E7
> >     LOGICAL_MINIMUM (0)                     15 00
> >     LOGICAL_MAXIMUM (1)                     25 01
> >     REPORT_SIZE (1)                         75 01
> >     REPORT_COUNT (8)                        95 08
> >     USAGE_PAGE (Keyboard)                   05 07
> >     INPUT (Data,Var,Abs)                    81 02
> >
> > But it makes the parser act wrong for the following report
> > descriptor pattern(such as some Gamepads):
> >
> >     USAGE_PAGE (Button)                     05 09
> >     USAGE (Button 1)                        09 01
> >     USAGE (Button 2)                        09 02
> >     USAGE (Button 4)                        09 04
> >     USAGE (Button 5)                        09 05
> >     USAGE (Button 7)                        09 07
> >     USAGE (Button 8)                        09 08
> >     USAGE (Button 14)                       09 0E
> >     USAGE (Button 15)                       09 0F
> >     USAGE (Button 13)                       09 0D
> >     USAGE_PAGE (Consumer Devices)           05 0C
> >     USAGE (Back)                            0a 24 02
> >     USAGE (HomePage)                        0a 23 02
> >     LOGICAL_MINIMUM (0)                     15 00
> >     LOGICAL_MAXIMUM (1)                     25 01
> >     REPORT_SIZE (1)                         75 01
> >     REPORT_COUNT (11)                       95 0B
> >     INPUT (Data,Var,Abs)                    81 02
> >
> > With Usage Page concatenation in Main item, parser recognizes all the
> > 11 Usages as consumer keys, it is not the HID device's real intention.
> >
> > This patch adds usage_page_last to flag whether Usage Page is after
> > Usage ID items. usage_page_last is false default, it is set as true
> > once Usage Page item is encountered and is reverted by next Usage ID
> > item.
> >
> > Usage Page concatenation on the currently defined Usage Page will do
> > firstly in Local parsing when Usage ID items encountered.
> >
> > When Main item is parsing, concatenation will do again with last
> > defined Usage Page if usage_page_last flag is true.
> >
> > Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> > ---
> > Changes in v2:
> > - Update patch title
> > - Add GET_COMPLETE_USAGE macro
> > - Change the logic of checking whether to concatenate usage page again
> >   in main parsing
> > ---
> >  drivers/hid/hid-core.c | 31 +++++++++++++++++++++++++------
> >  include/linux/hid.h    |  1 +
> >  2 files changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 3eaee2c..3394222 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -35,6 +35,8 @@
> >
> >  #include "hid-ids.h"
> >
> > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
> > +
> >  /*
> >   * Version Information
> >   */
> > @@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser, unsigned usage, u8 size)
> >                 hid_err(parser->device, "usage index exceeded\n");
> >                 return -1;
> >         }
> > -       parser->local.usage[parser->local.usage_index] = usage;
> > +
> > +       if (size <= 2) {
> > +               parser->local.usage_page_last = false;
> > +               parser->local.usage[parser->local.usage_index] =
> > +                       GET_COMPLETE_USAGE(parser->global.usage_page, usage);
> > +       } else {
> > +               parser->local.usage[parser->local.usage_index] = usage;
> > +       }
> > +
> >         parser->local.usage_size[parser->local.usage_index] = size;
> >         parser->local.collection_index[parser->local.usage_index] =
> >                 parser->collection_stack_ptr ?
> > @@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
> >
> >         case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
> >                 parser->global.usage_page = item_udata(item);
> > +               parser->local.usage_page_last = true;
> >                 return 0;
> >
> >         case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
> > @@ -543,13 +554,21 @@ static int hid_parser_local(struct hid_parser *parser, struct hid_item *item)
> >   * usage value."
> >   */
> >
> > -static void hid_concatenate_usage_page(struct hid_parser *parser)
> > +static void hid_concatenate_last_usage_page(struct hid_parser *parser)
> >  {
> >         int i;
> > +       unsigned int usage;
> > +       unsigned int usage_page = parser->global.usage_page;
> > +
> > +       if (!parser->local.usage_page_last)
> > +               return;
> >
> >         for (i = 0; i < parser->local.usage_index; i++)
> > -               if (parser->local.usage_size[i] <= 2)
> > -                       parser->local.usage[i] += parser->global.usage_page << 16;
> > +               if (parser->local.usage_size[i] <= 2) {
> > +                       usage = parser->local.usage[i];
> > +                       parser->local.usage[i] =
> > +                               GET_COMPLETE_USAGE(usage_page, usage);
> > +               }
> >  }
> >
> >  /*
> > @@ -561,7 +580,7 @@ static int hid_parser_main(struct hid_parser *parser, struct hid_item *item)
> >         __u32 data;
> >         int ret;
> >
> > -       hid_concatenate_usage_page(parser);
> > +       hid_concatenate_last_usage_page(parser);
> >
> >         data = item_udata(item);
> >
> > @@ -772,7 +791,7 @@ static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
> >         __u32 data;
> >         int i;
> >
> > -       hid_concatenate_usage_page(parser);
> > +       hid_concatenate_last_usage_page(parser);
> >
> >         data = item_udata(item);
> >
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index cd41f20..2e0ea2f7 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -412,6 +412,7 @@ struct hid_local {
> >         unsigned usage_minimum;
> >         unsigned delimiter_depth;
> >         unsigned delimiter_branch;
> > +       bool usage_page_last;      /* whether usage page is after usage id */
>
> We don't need that extra flag:
> if you just check on the last element, you can guess that information:
>
>         if ((parser->local.usage[parser->local.usage_index - 1] &
> HID_USAGE_PAGE) >> 16 == usage_page)
>               return 0;
>
> Let's see the next version before requesting too many changes.
>
> And yes, I agree I need the hid-tools patches or I will not merge this
> patch (and I will advise Jiri to not take it either).
>
> Cheers,
> Benjamin
>

Thanks Benjamin.
I will rework the patch and add changes on hid-tools to test it more.

Candle

Patch
diff mbox series

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3eaee2c..3394222 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -35,6 +35,8 @@ 
 
 #include "hid-ids.h"
 
+#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0xffff))
+
 /*
  * Version Information
  */
@@ -221,7 +223,15 @@  static int hid_add_usage(struct hid_parser *parser, unsigned usage, u8 size)
 		hid_err(parser->device, "usage index exceeded\n");
 		return -1;
 	}
-	parser->local.usage[parser->local.usage_index] = usage;
+
+	if (size <= 2) {
+		parser->local.usage_page_last = false;
+		parser->local.usage[parser->local.usage_index] =
+			GET_COMPLETE_USAGE(parser->global.usage_page, usage);
+	} else {
+		parser->local.usage[parser->local.usage_index] = usage;
+	}
+
 	parser->local.usage_size[parser->local.usage_index] = size;
 	parser->local.collection_index[parser->local.usage_index] =
 		parser->collection_stack_ptr ?
@@ -366,6 +376,7 @@  static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
 
 	case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
 		parser->global.usage_page = item_udata(item);
+		parser->local.usage_page_last = true;
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
@@ -543,13 +554,21 @@  static int hid_parser_local(struct hid_parser *parser, struct hid_item *item)
  * usage value."
  */
 
-static void hid_concatenate_usage_page(struct hid_parser *parser)
+static void hid_concatenate_last_usage_page(struct hid_parser *parser)
 {
 	int i;
+	unsigned int usage;
+	unsigned int usage_page = parser->global.usage_page;
+
+	if (!parser->local.usage_page_last)
+		return;
 
 	for (i = 0; i < parser->local.usage_index; i++)
-		if (parser->local.usage_size[i] <= 2)
-			parser->local.usage[i] += parser->global.usage_page << 16;
+		if (parser->local.usage_size[i] <= 2) {
+			usage = parser->local.usage[i];
+			parser->local.usage[i] =
+				GET_COMPLETE_USAGE(usage_page, usage);
+		}
 }
 
 /*
@@ -561,7 +580,7 @@  static int hid_parser_main(struct hid_parser *parser, struct hid_item *item)
 	__u32 data;
 	int ret;
 
-	hid_concatenate_usage_page(parser);
+	hid_concatenate_last_usage_page(parser);
 
 	data = item_udata(item);
 
@@ -772,7 +791,7 @@  static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
 	__u32 data;
 	int i;
 
-	hid_concatenate_usage_page(parser);
+	hid_concatenate_last_usage_page(parser);
 
 	data = item_udata(item);
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index cd41f20..2e0ea2f7 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -412,6 +412,7 @@  struct hid_local {
 	unsigned usage_minimum;
 	unsigned delimiter_depth;
 	unsigned delimiter_branch;
+	bool usage_page_last;      /* whether usage page is after usage id */
 };
 
 /*