* [PATCH v3] HID: core: check whether usage page item is after usage id item
@ 2019-10-21 7:38 Candle Sun
2019-10-21 7:54 ` Candle Sun
0 siblings, 1 reply; 4+ messages in thread
From: Candle Sun @ 2019-10-21 7:38 UTC (permalink / raw)
To: jikos, benjamin.tissoires, nsaenzjulienne
Cc: orson.zhai, linux-input, linux-kernel, Candle Sun, Nianfu Bai
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 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;
+
+ /*
+ * 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 */
};
/*
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] HID: core: check whether usage page item is after usage id item
2019-10-21 7:38 [PATCH v3] HID: core: check whether usage page item is after usage id item Candle Sun
@ 2019-10-21 7:54 ` Candle Sun
2019-10-22 9:48 ` Benjamin Tissoires
0 siblings, 1 reply; 4+ messages in thread
From: Candle Sun @ 2019-10-21 7:54 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Nicolas Saenz Julienne
Cc: 翟京 (Orson Zhai),
open list:HID CORE LAYER, lkml, Candle Sun, Nianfu Bai
Hi,
On Mon, Oct 21, 2019 at 3:38 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 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;
> +
> + /*
> + * 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.
Candle
> /*
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] HID: core: check whether usage page item is after usage id item
2019-10-21 7:54 ` Candle Sun
@ 2019-10-22 9:48 ` Benjamin Tissoires
2019-10-22 10:33 ` Candle Sun
0 siblings, 1 reply; 4+ messages in thread
From: Benjamin Tissoires @ 2019-10-22 9:48 UTC (permalink / raw)
To: Candle Sun
Cc: Jiri Kosina, Nicolas Saenz Julienne,
翟京 (Orson Zhai),
open list:HID CORE LAYER, lkml, Candle Sun, Nianfu Bai
Hi Candle,
On Mon, Oct 21, 2019 at 9:54 AM Candle Sun <candlesea@gmail.com> wrote:
>
> Hi,
>
>
> On Mon, Oct 21, 2019 at 3:38 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 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.
> > +
> > + /*
> > + * 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] HID: core: check whether usage page item is after usage id item
2019-10-22 9:48 ` Benjamin Tissoires
@ 2019-10-22 10:33 ` Candle Sun
0 siblings, 0 replies; 4+ messages in thread
From: Candle Sun @ 2019-10-22 10:33 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Nicolas Saenz Julienne,
翟京 (Orson Zhai),
open list:HID CORE LAYER, lkml, Candle Sun, Nianfu Bai
Hi Benjamin,
On Tue, Oct 22, 2019 at 5:48 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi Candle,
>
> On Mon, Oct 21, 2019 at 9:54 AM Candle Sun <candlesea@gmail.com> wrote:
> >
> > Hi,
> >
> >
> > On Mon, Oct 21, 2019 at 3:38 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 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-22 10:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-21 7:38 [PATCH v3] HID: core: check whether usage page item is after usage id item Candle Sun
2019-10-21 7:54 ` Candle Sun
2019-10-22 9:48 ` Benjamin Tissoires
2019-10-22 10:33 ` Candle Sun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).