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
