Re: [PATCH] HID: core: add usage_page_preceding flag for hid_concatenate_usage_page()

[Nicolas Saenz Julienne]

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@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 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@xxxxxxxxxx>
> > Signed-off-by: Nianfu Bai <nianfu.bai@xxxxxxxxxx>
> > ---
> >  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: signature.asc
Description: This is a digitally signed message part