Hi Benjamin,
thanks for the patches, things are looking a lot better this way.
> hid_scan_report() implements its own HID report descriptor parsing. It was
> fine until we added the SENSOR_HUB detection. It is going to be even worse
> with the detection of Win 8 certified touchscreen, as this detection
> relies on a special feature and on the report_size and report_count fields.
It was fine with sensors added as well. You seem to have found a
reasonable way to add support for all the tags, but there is a
rationale for the current scanner that may not have been addressed in
this patch: it is robust against parse errors. This is particularly
important for devices which later tweak the report, often in order to
parse properly.
Please find some further comments inline.
> We can use the existing HID parser in hid-core for hid_scan_report()
> by re-using the code from hid_open_report(). hid_parser_global,
> hid_parser_local and hid_parser_reserved does not have any side effects.
> We just need to reimplement the MAIN_ITEM callback to have a proper
> parsing without side effects.
>
> Instead of directly overwriting the ->group field, this patch introduce
> a ->flags field and then decide which group the device belongs to,
> depending on the whole parsing (not just the local item). This will be
> useful for Win 8 multitouch devices, which are multitouch devices and
> Win 8 certified (so 2 flags to check).
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoi...@redhat.com>
> ---
> drivers/hid/hid-core.c | 131
> +++++++++++++++++++++++++++++++++++--------------
> include/linux/hid.h | 4 ++
> 2 files changed, 97 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 3efe19f..d8cdb0a 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -677,10 +677,49 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct
> hid_item *item)
> return NULL;
> }
>
> -static void hid_scan_usage(struct hid_device *hid, u32 usage)
> +static void hid_scan_input_usage(struct hid_parser *parser, u32 usage)
> {
> if (usage == HID_DG_CONTACTID)
> - hid->group = HID_GROUP_MULTITOUCH;
> + parser->flags |= HID_FLAG_MULTITOUCH;
Did you consider reusing the group flags, e.g., parser->groups |= (1
<< HID_GROUP_MULTITOUCH)? This change could be made regardless of the
parser logic.
> +}
> +
> +static void hid_scan_open_collection(struct hid_parser *parser, unsigned
> type)
We are not really opening anything here, so perhaps
hid_scan_collection would suffice.
> +{
> + if (parser->global.usage_page == HID_UP_SENSOR &&
> + type == HID_COLLECTION_PHYSICAL)
> + parser->flags |= HID_FLAG_SENSOR_HUB;
> +}
> +
> +static int hid_scan_main(struct hid_parser *parser, struct hid_item *item)
> +{
> + __u32 data;
> + int i;
> +
> + data = item_udata(item);
> +
> + switch (item->tag) {
> + case HID_MAIN_ITEM_TAG_BEGIN_COLLECTION:
> + hid_scan_open_collection(parser, data & 0xff);
> + break;
> + case HID_MAIN_ITEM_TAG_END_COLLECTION:
> + break;
> + case HID_MAIN_ITEM_TAG_INPUT:
> + for (i = 0; i < parser->local.usage_index; i++)
> + hid_scan_input_usage(parser, parser->local.usage[i]);
> + break;
> + case HID_MAIN_ITEM_TAG_OUTPUT:
> + break;
> + case HID_MAIN_ITEM_TAG_FEATURE:
> + break;
> + default:
> + hid_err(parser->device, "unknown main item tag 0x%x\n",
> + item->tag);
> + }
> +
> + /* Reset the local parser environment */
> + memset(&parser->local, 0, sizeof(parser->local));
> +
> + return 0;
> }
>
> /*
> @@ -690,49 +729,65 @@ static void hid_scan_usage(struct hid_device *hid, u32
> usage)
> */
> static int hid_scan_report(struct hid_device *hid)
> {
> - unsigned int page = 0, delim = 0;
> + struct hid_parser *parser;
> + struct hid_item item;
> __u8 *start = hid->dev_rdesc;
> __u8 *end = start + hid->dev_rsize;
> - unsigned int u, u_min = 0, u_max = 0;
> - struct hid_item item;
> + int ret;
> + static int (*dispatch_type[])(struct hid_parser *parser,
> + struct hid_item *item) = {
> + hid_scan_main,
> + hid_parser_global,
> + hid_parser_local,
> + hid_parser_reserved
> + };
>
> - hid->group = HID_GROUP_GENERIC;
> + parser = vzalloc(sizeof(struct hid_parser));
Argh, I realize it is inevitable for this patch, but it still makes my
eyes bleed. The parser takes quite a bit of memory...
> + if (!parser)
> + return -ENOMEM;
> +
> + parser->device = hid;
> +
> + ret = -EINVAL;
> while ((start = fetch_item(start, end, &item)) != NULL) {
> - if (item.format != HID_ITEM_FORMAT_SHORT)
> - return -EINVAL;
> - if (item.type == HID_ITEM_TYPE_GLOBAL) {
> - if (item.tag == HID_GLOBAL_ITEM_TAG_USAGE_PAGE)
> - page = item_udata(&item) << 16;
> - } else if (item.type == HID_ITEM_TYPE_LOCAL) {
> - if (delim > 1)
> - break;
> - u = item_udata(&item);
> - if (item.size <= 2)
> - u += page;
> - switch (item.tag) {
> - case HID_LOCAL_ITEM_TAG_DELIMITER:
> - delim += !!u;
> - break;
> - case HID_LOCAL_ITEM_TAG_USAGE:
> - hid_scan_usage(hid, u);
> - break;
> - case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
> - u_min = u;
> - break;
> - case HID_LOCAL_ITEM_TAG_USAGE_MAXIMUM:
> - u_max = u;
> - for (u = u_min; u <= u_max; u++)
> - hid_scan_usage(hid, u);
> - break;
> +
> + if (item.format != HID_ITEM_FORMAT_SHORT) {
> + hid_err(hid, "unexpected long global item\n");
I do not think we should be verbose on errors during scan, for the
reason stated at the top. Since this goes also for the global parser
functions, we might have a problem.
> + goto out;
> + }
> +
> + if (dispatch_type[item.type](parser, &item)) {
> + hid_err(hid, "item %u %u %u %u parsing failed\n",
> + item.format, (unsigned)item.size,
> + (unsigned)item.type, (unsigned)item.tag);
Ditto.
> + goto out;
> + }
> +
> + if (start == end) {
> + if (parser->local.delimiter_depth) {
> + hid_err(hid, "unbalanced delimiter at end of
> report description\n");
Robustness, see top.
> + goto out;
> }
> - } else if (page == HID_UP_SENSOR &&
> - item.type == HID_ITEM_TYPE_MAIN &&
> - item.tag == HID_MAIN_ITEM_TAG_BEGIN_COLLECTION &&
> - (item_udata(&item) & 0xff) == HID_COLLECTION_PHYSICAL)
> - hid->group = HID_GROUP_SENSOR_HUB;
At the end of the day, It may be best to simply extend this branch as
the main item type and add whatever you need to detect win8 from
there.
> + ret = 0;
> + goto out;
> + }
> }
>
> - return 0;
> + hid_err(hid, "item fetching failed at offset %d\n", (int)(end - start));
> +out:
> + switch (parser->flags) {
> + case HID_FLAG_MULTITOUCH:
> + hid->group = HID_GROUP_MULTITOUCH;
> + break;
> + case HID_FLAG_SENSOR_HUB:
> + hid->group = HID_GROUP_SENSOR_HUB;
> + break;
> + default:
> + hid->group = HID_GROUP_GENERIC;
> + }
Looks odd to switch on flags, but it works pretty well with the rest
of the patches in the series.
> +
> + vfree(parser);
> + return ret;
> }
>
> /**
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 5a4e789..7d823db 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -533,6 +533,9 @@ static inline void hid_set_drvdata(struct hid_device
> *hdev, void *data)
> #define HID_GLOBAL_STACK_SIZE 4
> #define HID_COLLECTION_STACK_SIZE 4
>
> +#define HID_FLAG_MULTITOUCH 0x0001
> +#define HID_FLAG_SENSOR_HUB 0x0002
> +
> struct hid_parser {
> struct hid_global global;
> struct hid_global global_stack[HID_GLOBAL_STACK_SIZE];
> @@ -541,6 +544,7 @@ struct hid_parser {
> unsigned collection_stack[HID_COLLECTION_STACK_SIZE];
> unsigned collection_stack_ptr;
> struct hid_device *device;
> + unsigned flags;
> };
>
> struct hid_class_descriptor {
> --
> 1.8.3.1
>
Thanks,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
