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

From: Candle Sun
Date: Tue Oct 22 2019 - 06:33:41 EST

Next message: Gautham R. Shenoy: "[PATCH v2 1/1] pseries/hotplug-cpu: Change default behaviour of cede_offline to "off""
Previous message: Gautham R. Shenoy: "[PATCH v2 0/1] pseries/hotplug: Change the default behaviour of cede_offline"
In reply to: Benjamin Tissoires: "Re: [PATCH v3] HID: core: check whether usage page item is after usage id item"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Benjamin,

On Tue, Oct 22, 2019 at 5:48 PM Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> wrote:
>
> Hi Candle,
>
> On Mon, Oct 21, 2019 at 9:54 AM Candle Sun <candlesea@xxxxxxxxx> wrote:
> >
> > Hi,
> >
> >
> > On Mon, Oct 21, 2019 at 3:38 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 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@xxxxxxxxxx>
> > > Signed-off-by: Nianfu Bai <nianfu.bai@xxxxxxxxxx>
> > > ---
> > > 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 | 42 +++++++++++++++++++++++++++++++++++++-----
> > > include/linux/hid.h | 1 +
> > > 2 files changed, 38 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > index 3eaee2c37931..779b7798dae8 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.
> > > */
> > > @@ -221,7 +233,18 @@ 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;
>
> This broke my CI, and it turns out that installing the patch on an
> actual laptop, the touchpad, touchscreen were not present, nor any HID
> devices plugged in.
>
> Basically, complete_usage() doesn't append the usage, and now we are
> never assigning a usage to any field.
>

So sorry, it is really really a BIG bug, I should be more careful. (âïâ)

Candle

> > > +
> > > + /*
> > > + * If Usage item only includes usage id, concatenate it with
> > > + * currently defined usage page and clear usage_page_last flag
> > > + */
> > > + if (size <= 2) {
> > > + parser->local.usage_page_last = false;
> > > + complete_usage(parser, parser->local.usage_index);
> > > + } 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 +389,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);
> > > + /* Regard Usage Page is after Usage ID items */
> > > + parser->local.usage_page_last = true;
> > > return 0;
> > >
> > > case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
> > > @@ -543,13 +568,20 @@ 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;
> > >
> > > + /*
> > > + * Concatenate usage page again only on detecting some Usage Page
> > > + * is really after Usage ID items
> > > + */
> > > + 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;
> > > + complete_usage(parser, i);
> > > }
> > >
> > > /*
> > > @@ -561,7 +593,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 +804,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 cd41f209043f..2e0ea2f7ec5c 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 */
> > > };
> > >
> >
> > Hi Benjamin,
> > Here I still use the usage_page_last flag, not using following method
> > you provided in v2:
> >
> > if ((parser->local.usage[parser->local.usage_index - 1] &
> > HID_USAGE_PAGE) >> 16 == usage_page)
> > return 0;
> >
> > Because last usage maybe one Extended Usage, some logic for checking
> > it should be added.
> > It will make the code obscure. Using one more member in struct
> > hid_local is straightforward
> > and maybe better.
>
>
> Still not convinced by this. Please see below for a less intrusive
> patch, which is also shorter.
>
> ---
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 63fdbf09b044..00ea04fb1be3 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 ?
> @@ -546,10 +566,28 @@ static int hid_parser_local(struct hid_parser
> *parser, struct hid_item *item)
> static void hid_concatenate_usage_page(struct hid_parser *parser)
> {
> int i;
> + uint16_t usage_page, 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 the last declared Usage Page
> + * has not been already used in the previous Usages
> + */
> + 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);
> + }
> }
>
> /*
> ---
>
> Isn't that better?
>
> I tested this against
> https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/58,
> which you kindly submitted and it seems to do the job.
>
> Cheers,
> Benjamin
>

OK. I will amend the patch.

Best regards,
Candle

Next message: Gautham R. Shenoy: "[PATCH v2 1/1] pseries/hotplug-cpu: Change default behaviour of cede_offline to "off""
Previous message: Gautham R. Shenoy: "[PATCH v2 0/1] pseries/hotplug: Change the default behaviour of cede_offline"
In reply to: Benjamin Tissoires: "Re: [PATCH v3] HID: core: check whether usage page item is after usage id item"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]