diff mbox

[1/3] HID: Use existing parser for pre-scanning the report descriptors

Message ID 1376405889-12378-2-git-send-email-benjamin.tissoires@redhat.com
State New, archived
Delegated to: Jiri Kosina
Headers show

Commit Message

Benjamin Tissoires Aug. 13, 2013, 2:58 p.m. UTC
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.

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.tissoires@redhat.com>
---
 drivers/hid/hid-core.c | 131 +++++++++++++++++++++++++++++++++++--------------
 include/linux/hid.h    |   4 ++
 2 files changed, 97 insertions(+), 38 deletions(-)

Comments

Alexander Holler Aug. 13, 2013, 6:37 p.m. UTC | #1
Am 13.08.2013 16:58, schrieb Benjamin Tissoires:
> 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.

Sorry, but I can't agree with your wording here.

If you look at the if expression I've added to support HID sensor hubs, 
you will notice that the first expression was a test for the usage page 
of sensor hubs. That wasn't the first test by accident, instead I've 
choosen the order of those 4 tests very carefully to make the impact on 
the existing parsing of other HID devices very small. So it might not 
have be pleasing to your eyes, but it was for sure an appropriate solution.

Anyway, I've tested your patch 1/3 on top of 3.11-rc5 here and the 
detection of sensor hubs doesn't work anymore with your patch applied. 
Up to now I only had a quick look at it, but it looks like the test in 
hid_scan_open_collection() isn't hit for my device.

Another problem is that I don't have any commercial sensor hub and I'm 
therefor not a very relvant as tester (I've implemented the firmware for 
my HID (sensor hub) device too). Therefor I've added Srinivas Pandruvada 
to cc, because he's the only one I know who has HID sensor hubs.

And, as said, I've implemented the other side here too, therefor I've 
added the descriptor I'm using below.

Regards,

Alexander Holler


--my-descriptor--
	0x05, 0x20, // HID_USAGE_PAGE_SENSOR
	0xa1, 0x01, // COLLECTION (Application)
		0x09, 0xa0, // HID_USAGE_SENSOR_CATEGORY_TIME
		0xa1, 0x00, // COLLECTION (Physical)
#ifndef USE_FULL_YEAR
			0x75, 0x08, // REPORT_SIZE (8 bits)
#endif
			0x95, 0x01, // REPORT_COUNT (1)
			0x85, HID_REPORT_ID_TIME, // REPORT_ID

			0x0a, 0x21, 0x05, // USAGE (Year)
#ifdef USE_FULL_YEAR
			0x75, 0x10, // REPORT_SIZE (16 bits)
#else
			0x15, 0x00, // LOGICAL_MINIMUM (0) (Range is currently not specified 
in HUTRR39)
			0x25, 0x63, // LOGICAL_MAXIMUM (99)
#endif
			0x65, 0x00, // UNIT (None)
			0x81, 0x02, // INPUT (Data,Var,Abs)

			0x0a, 0x22, 0x05, // USAGE (Month)
#ifdef USE_FULL_YEAR
			0x75, 0x08, // REPORT_SIZE (8 bits)
#endif
			0x15, 0x01, // LOGICAL_MINIMUM (1)
			0x25, 0x0c, // LOGICAL_MAXIMUM (12)
			0x65, 0x00, // UNIT (None)
			0x81, 0x02, // INPUT (Data,Var,Abs)

			0x0a, 0x23, 0x05, // USAGE (Day)
			0x15, 0x01, // LOGICAL_MINIMUM (1)
			0x25, 0x1f, // LOGICAL_MAXIMUM (31)
			0x65, 0x00, // UNIT (None)
			0x81, 0x02, // INPUT (Data,Var,Abs)

			0x0a, 0x24, 0x05, // USAGE (Day of Week)
			0x15, 0x00, // LOGICAL_MINIMUM (0)
			0x25, 0x06, // LOGICAL_MAXIMUM (6)
			0xa1, 0x02, // COLLECTION (Logical)
				0x0a, 0xc0, 0x08, // Day of Week: Sunday
				0x0a, 0xc1, 0x08, // Day of Week: Monday
				0x0a, 0xc2, 0x08, // Day of Week: Tuesday
				0x0a, 0xc3, 0x08, // Day of Week: Wednesday
				0x0a, 0xc4, 0x08, // Day of Week: Thursday
				0x0a, 0xc5, 0x08, // Day of Week: Friday
				0x0a, 0xc6, 0x08, // Day of Week: Saturday
				0x81, 0x02, // INPUT (Const,Arr,Abs)
			0xc0, // END_COLLECTION

			0x0a, 0x25, 0x05, // USAGE (Hour)
			0x15, 0x00, // LOGICAL_MINIMUM (0)
			0x25, 0x17, // LOGICAL_MAXIMUM (23)
			0x65, 0x00, // UNIT (None)
			0x81, 0x02, // INPUT (Data,Var,Abs)

			0x0a, 0x26, 0x05, // USAGE (Minute)
			0x15, 0x00, // LOGICAL_MINIMUM (0)
			0x25, 0x3b, // LOGICAL_MAXIMUM (59)
			0x65, 0x00, // UNIT (None)
			0x81, 0x02, // INPUT (Data,Var,Abs)

			0x0a, 0x27, 0x05, // USAGE (Second)
			0x15, 0x00, // LOGICAL_MINIMUM (0)
			0x25, 0x3b, // LOGICAL_MAXIMUM (59)
			0x65, 0x00, // UNIT (None)
			//0x66, 0x10, 0x01, // UNIT (second)
			0x81, 0x02, // INPUT (Data,Var,Abs)

			0x0a, 0x28, 0x05, // USAGE (Millisecond)
			0x75, 0x10, // REPORT_SIZE (16 bits)
			0x65, 0x00, // UNIT (None)
			0x81, 0x02, // INPUT (Data,Var,Abs)

		0xc0, // END_COLLECTION
	0xc0, // END_COLLECTION
--my-descriptor--

>
> 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.tissoires@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;
> +}
> +
> +static void hid_scan_open_collection(struct hid_parser *parser, unsigned type)
> +{
> +	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));
> +	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");
> +			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);
> +			goto out;
> +		}
> +
> +		if (start == end) {
> +			if (parser->local.delimiter_depth) {
> +				hid_err(hid, "unbalanced delimiter at end of report description\n");
> +				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;
> +			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;
> +	}
> +
> +	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 {
>

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Tissoires Aug. 13, 2013, 7:15 p.m. UTC | #2
On Tue, Aug 13, 2013 at 8:37 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 13.08.2013 16:58, schrieb Benjamin Tissoires:
>
>> 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.
>
>
> Sorry, but I can't agree with your wording here.
>
> If you look at the if expression I've added to support HID sensor hubs, you
> will notice that the first expression was a test for the usage page of
> sensor hubs. That wasn't the first test by accident, instead I've choosen
> the order of those 4 tests very carefully to make the impact on the existing
> parsing of other HID devices very small. So it might not have be pleasing to
> your eyes, but it was for sure an appropriate solution.

I'm sorry if you feel like it was a personal attack. I'm sure the
upstream code is working good and that there is no extra tests and
that you implemented it that way without proper testing and
validating. What I meant was that the initial code was not exactly
meant to address the particular problem of sensor hub (at that time,
we only needed to check for one input report). However, I need now to
add a test against a feature report, which would need a new custom
detection, and a heavy modification of the existing code.

>
> Anyway, I've tested your patch 1/3 on top of 3.11-rc5 here and the detection
> of sensor hubs doesn't work anymore with your patch applied. Up to now I
> only had a quick look at it, but it looks like the test in
> hid_scan_open_collection() isn't hit for my device.

oops, this is not intentional. I'll try to fix this in v2.

>
> Another problem is that I don't have any commercial sensor hub and I'm
> therefor not a very relvant as tester (I've implemented the firmware for my
> HID (sensor hub) device too). Therefor I've added Srinivas Pandruvada to cc,
> because he's the only one I know who has HID sensor hubs.

It is not a problem if you don't have a commercial sensor hub :)
Thanks for adding Srinivas in CC.

>
> And, as said, I've implemented the other side here too, therefor I've added
> the descriptor I'm using below.
>

Thanks, this will help me to test against your report descriptors.
Can I also ask you to send me some hid-recorder[1] traces of your
sensor? With hid-replay, I can then re-inject them in the hid
subsystem, and then I include the results in a regression test suite.

Cheers,
Benjamin

[1] http://bentiss.github.io/hid-replay-docs/

>
> --my-descriptor--
>         0x05, 0x20, // HID_USAGE_PAGE_SENSOR
>         0xa1, 0x01, // COLLECTION (Application)
>                 0x09, 0xa0, // HID_USAGE_SENSOR_CATEGORY_TIME
>                 0xa1, 0x00, // COLLECTION (Physical)
> #ifndef USE_FULL_YEAR
>                         0x75, 0x08, // REPORT_SIZE (8 bits)
> #endif
>                         0x95, 0x01, // REPORT_COUNT (1)
>                         0x85, HID_REPORT_ID_TIME, // REPORT_ID
>
>                         0x0a, 0x21, 0x05, // USAGE (Year)
> #ifdef USE_FULL_YEAR
>                         0x75, 0x10, // REPORT_SIZE (16 bits)
> #else
>                         0x15, 0x00, // LOGICAL_MINIMUM (0) (Range is
> currently not specified in HUTRR39)
>                         0x25, 0x63, // LOGICAL_MAXIMUM (99)
> #endif
>                         0x65, 0x00, // UNIT (None)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                         0x0a, 0x22, 0x05, // USAGE (Month)
> #ifdef USE_FULL_YEAR
>                         0x75, 0x08, // REPORT_SIZE (8 bits)
> #endif
>                         0x15, 0x01, // LOGICAL_MINIMUM (1)
>                         0x25, 0x0c, // LOGICAL_MAXIMUM (12)
>                         0x65, 0x00, // UNIT (None)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                         0x0a, 0x23, 0x05, // USAGE (Day)
>                         0x15, 0x01, // LOGICAL_MINIMUM (1)
>                         0x25, 0x1f, // LOGICAL_MAXIMUM (31)
>                         0x65, 0x00, // UNIT (None)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                         0x0a, 0x24, 0x05, // USAGE (Day of Week)
>                         0x15, 0x00, // LOGICAL_MINIMUM (0)
>                         0x25, 0x06, // LOGICAL_MAXIMUM (6)
>                         0xa1, 0x02, // COLLECTION (Logical)
>                                 0x0a, 0xc0, 0x08, // Day of Week: Sunday
>                                 0x0a, 0xc1, 0x08, // Day of Week: Monday
>                                 0x0a, 0xc2, 0x08, // Day of Week: Tuesday
>                                 0x0a, 0xc3, 0x08, // Day of Week: Wednesday
>                                 0x0a, 0xc4, 0x08, // Day of Week: Thursday
>                                 0x0a, 0xc5, 0x08, // Day of Week: Friday
>                                 0x0a, 0xc6, 0x08, // Day of Week: Saturday
>                                 0x81, 0x02, // INPUT (Const,Arr,Abs)
>                         0xc0, // END_COLLECTION
>
>                         0x0a, 0x25, 0x05, // USAGE (Hour)
>                         0x15, 0x00, // LOGICAL_MINIMUM (0)
>                         0x25, 0x17, // LOGICAL_MAXIMUM (23)
>                         0x65, 0x00, // UNIT (None)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                         0x0a, 0x26, 0x05, // USAGE (Minute)
>                         0x15, 0x00, // LOGICAL_MINIMUM (0)
>                         0x25, 0x3b, // LOGICAL_MAXIMUM (59)
>                         0x65, 0x00, // UNIT (None)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                         0x0a, 0x27, 0x05, // USAGE (Second)
>                         0x15, 0x00, // LOGICAL_MINIMUM (0)
>                         0x25, 0x3b, // LOGICAL_MAXIMUM (59)
>                         0x65, 0x00, // UNIT (None)
>                         //0x66, 0x10, 0x01, // UNIT (second)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                         0x0a, 0x28, 0x05, // USAGE (Millisecond)
>                         0x75, 0x10, // REPORT_SIZE (16 bits)
>                         0x65, 0x00, // UNIT (None)
>                         0x81, 0x02, // INPUT (Data,Var,Abs)
>
>                 0xc0, // END_COLLECTION
>         0xc0, // END_COLLECTION
> --my-descriptor--
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Henrik Rydberg Aug. 13, 2013, 7:17 p.m. UTC | #3
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.tissoires@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-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Holler Aug. 14, 2013, 6:46 a.m. UTC | #4
Am 13.08.2013 21:15, schrieb Benjamin Tissoires:
> On Tue, Aug 13, 2013 at 8:37 PM, Alexander Holler <holler@ahsoftware.de> wrote:

>>
>> Another problem is that I don't have any commercial sensor hub and I'm
>> therefor not a very relvant as tester (I've implemented the firmware for my
>> HID (sensor hub) device too). Therefor I've added Srinivas Pandruvada to cc,
>> because he's the only one I know who has HID sensor hubs.
>
> It is not a problem if you don't have a commercial sensor hub :)
> Thanks for adding Srinivas in CC.

The problem is that this invalidates me as a tester because everything I 
can test is something untested. As I've written both ends of the pipe, I 
could have understood something totally wrong but as I've implemented 
both sides (the device- and the kernel-part) it still might work without 
problems. Therefor I currently have to bother Srinivas whenever I do 
changes in the kernel in regard to hid-sensor-hub. ;)

>>
>> And, as said, I've implemented the other side here too, therefor I've added
>> the descriptor I'm using below.
>>
>
> Thanks, this will help me to test against your report descriptors.
> Can I also ask you to send me some hid-recorder[1] traces of your
> sensor? With hid-replay, I can then re-inject them in the hid
> subsystem, and then I include the results in a regression test suite.

Unfortunately hid-record doesn't list my device because it doesn't show 
up as an hidraw-device.

But maybe you have an Arduino Leonardo or a clone. If so you can get the 
description and firmware here:

http://ahsoftware.de/dcf77-hid-usb-rtc/

Regards,

Alexander Holler

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Tissoires Aug. 14, 2013, 3:08 p.m. UTC | #5
On 13/08/13 20:37, Alexander Holler wrote:
> Am 13.08.2013 16:58, schrieb Benjamin Tissoires:
>> 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.
> 
> Sorry, but I can't agree with your wording here.
> 
> If you look at the if expression I've added to support HID sensor hubs,
> you will notice that the first expression was a test for the usage page
> of sensor hubs. That wasn't the first test by accident, instead I've
> choosen the order of those 4 tests very carefully to make the impact on
> the existing parsing of other HID devices very small. So it might not
> have be pleasing to your eyes, but it was for sure an appropriate solution.
> 
> Anyway, I've tested your patch 1/3 on top of 3.11-rc5 here and the
> detection of sensor hubs doesn't work anymore with your patch applied.
> Up to now I only had a quick look at it, but it looks like the test in
> hid_scan_open_collection() isn't hit for my device.

Ok, I found the culprit (see below). Thanks for providing your report
descriptors (and sources), I managed to test the detection of your device.


> 
> Another problem is that I don't have any commercial sensor hub and I'm
> therefor not a very relvant as tester (I've implemented the firmware for
> my HID (sensor hub) device too). Therefor I've added Srinivas Pandruvada
> to cc, because he's the only one I know who has HID sensor hubs.
> 
> And, as said, I've implemented the other side here too, therefor I've
> added the descriptor I'm using below.
> 
> Regards,
> 
> Alexander Holler
> 
> 
> --my-descriptor--
>     0x05, 0x20, // HID_USAGE_PAGE_SENSOR
>     0xa1, 0x01, // COLLECTION (Application)
>         0x09, 0xa0, // HID_USAGE_SENSOR_CATEGORY_TIME
>         0xa1, 0x00, // COLLECTION (Physical)
> #ifndef USE_FULL_YEAR
>             0x75, 0x08, // REPORT_SIZE (8 bits)
> #endif
>             0x95, 0x01, // REPORT_COUNT (1)
>             0x85, HID_REPORT_ID_TIME, // REPORT_ID
> 
>             0x0a, 0x21, 0x05, // USAGE (Year)
> #ifdef USE_FULL_YEAR
>             0x75, 0x10, // REPORT_SIZE (16 bits)
> #else
>             0x15, 0x00, // LOGICAL_MINIMUM (0) (Range is currently not
> specified in HUTRR39)
>             0x25, 0x63, // LOGICAL_MAXIMUM (99)
> #endif
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>             0x0a, 0x22, 0x05, // USAGE (Month)
> #ifdef USE_FULL_YEAR
>             0x75, 0x08, // REPORT_SIZE (8 bits)
> #endif
>             0x15, 0x01, // LOGICAL_MINIMUM (1)
>             0x25, 0x0c, // LOGICAL_MAXIMUM (12)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>             0x0a, 0x23, 0x05, // USAGE (Day)
>             0x15, 0x01, // LOGICAL_MINIMUM (1)
>             0x25, 0x1f, // LOGICAL_MAXIMUM (31)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>             0x0a, 0x24, 0x05, // USAGE (Day of Week)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x06, // LOGICAL_MAXIMUM (6)
>             0xa1, 0x02, // COLLECTION (Logical)
>                 0x0a, 0xc0, 0x08, // Day of Week: Sunday
>                 0x0a, 0xc1, 0x08, // Day of Week: Monday
>                 0x0a, 0xc2, 0x08, // Day of Week: Tuesday
>                 0x0a, 0xc3, 0x08, // Day of Week: Wednesday
>                 0x0a, 0xc4, 0x08, // Day of Week: Thursday
>                 0x0a, 0xc5, 0x08, // Day of Week: Friday
>                 0x0a, 0xc6, 0x08, // Day of Week: Saturday
>                 0x81, 0x02, // INPUT (Const,Arr,Abs)
>             0xc0, // END_COLLECTION
> 
>             0x0a, 0x25, 0x05, // USAGE (Hour)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x17, // LOGICAL_MAXIMUM (23)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>             0x0a, 0x26, 0x05, // USAGE (Minute)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x3b, // LOGICAL_MAXIMUM (59)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>             0x0a, 0x27, 0x05, // USAGE (Second)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x3b, // LOGICAL_MAXIMUM (59)
>             0x65, 0x00, // UNIT (None)
>             //0x66, 0x10, 0x01, // UNIT (second)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>             0x0a, 0x28, 0x05, // USAGE (Millisecond)
>             0x75, 0x10, // REPORT_SIZE (16 bits)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
> 
>         0xc0, // END_COLLECTION
>     0xc0, // END_COLLECTION
> --my-descriptor--
> 
>>
>> 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.tissoires@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;
>> +}
>> +
>> +static void hid_scan_open_collection(struct hid_parser *parser,
>> unsigned type)
>> +{
>> +    if (parser->global.usage_page == HID_UP_SENSOR &&

HID_UP_SENSOR is actually 0x00200000, and parser->global.usage_page
contains the raw value 0x0020... Moving HID_UP_SENSOR 16 bits to the
right makes the test working, and your sensor device correctly tagged.

Cheers,
Benjamin

>> +        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));
>> +    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");
>> +            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);
>> +            goto out;
>> +        }
>> +
>> +        if (start == end) {
>> +            if (parser->local.delimiter_depth) {
>> +                hid_err(hid, "unbalanced delimiter at end of report
>> description\n");
>> +                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;
>> +            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;
>> +    }
>> +
>> +    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 {
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Tissoires Aug. 14, 2013, 3:38 p.m. UTC | #6
Hi Henrik,

On 13/08/13 21:17, rydberg@euromail.se wrote:
> Hi Benjamin,
> 
> thanks for the patches, things are looking a lot better this way.

thanks for the review :)

> 
>> 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.

Hmm, I disagree: if a driver needs to tweak a report, then it will
register itself in hid_have_special_driver. The pre-scanning pass is
done only for devices which are not in hid_have_special_driver. So I
consider it is safe to assume that the report descriptor does not
contain errors. If it does, then it will just leave the current
hid-group state, and will just pop an debug information.

> 
> 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.tissoires@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.

If nobody is against changing the values of the different groups across
kernel version (which should be harmless), then I fully agree, we can
use the group item as a bit field (but we would be able to only have 16
different device groups).

> 
>> +}
>> +
>> +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.

Ok, will change in v2

> 
>> +{
>> +	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...

Yep, my first attempt was to remove it, then I re-added it with a small
tear...

> 
>> +	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.

yes, I kept it for documenting purposes, but this will double the amount
of logs in the kernel debug output. I'll just convert them into comments
in v2.

> 
>> +			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.

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.
> 

I don't think this would be that simple:
- the usage is given by the HID_ITEM_TYPE_LOCAL branch
- I also need to check the fact that the usage is used as a feature:
this would require setting temporary flags and involve a logic which
would not be very easy to understand.

Furthermore, this makes me think that if a device describes in the
reports a ContactID as a feature, then the current parsing will say that
this is a multitouch device, whereas it should not. (ok, this has no
reasons to appear because it would be dumb, but still...)

>> +			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.

Yep, it's odd. If nobody is against changing the current groups
definitions, we can just keep the group as a bitfield, and it will work
(we just need to sanitize the group at the end).

Cheers,
Benjamin

> 
>> +
>> +	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-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srinivas Pandruvada Aug. 14, 2013, 4:07 p.m. UTC | #7
On 08/13/2013 11:37 AM, Alexander Holler wrote:
> Am 13.08.2013 16:58, schrieb Benjamin Tissoires:
>> 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.
>
> Sorry, but I can't agree with your wording here.
>
> If you look at the if expression I've added to support HID sensor 
> hubs, you will notice that the first expression was a test for the 
> usage page of sensor hubs. That wasn't the first test by accident, 
> instead I've choosen the order of those 4 tests very carefully to make 
> the impact on the existing parsing of other HID devices very small. So 
> it might not have be pleasing to your eyes, but it was for sure an 
> appropriate solution.
>
> Anyway, I've tested your patch 1/3 on top of 3.11-rc5 here and the 
> detection of sensor hubs doesn't work anymore with your patch applied. 
> Up to now I only had a quick look at it, but it looks like the test in 
> hid_scan_open_collection() isn't hit for my device.
>
> Another problem is that I don't have any commercial sensor hub and I'm 
> therefor not a very relvant as tester (I've implemented the firmware 
> for my HID (sensor hub) device too). Therefor I've added Srinivas 
> Pandruvada to cc, because he's the only one I know who has HID sensor 
> hubs.
>
Tested on 3.11-rc5. Sensor hubs are no longer detected. If you have some 
patch to fix this, I can give a try.
> And, as said, I've implemented the other side here too, therefor I've 
> added the descriptor I'm using below.
>
> Regards,
>
> Alexander Holler
>
>
> --my-descriptor--
>     0x05, 0x20, // HID_USAGE_PAGE_SENSOR
>     0xa1, 0x01, // COLLECTION (Application)
>         0x09, 0xa0, // HID_USAGE_SENSOR_CATEGORY_TIME
>         0xa1, 0x00, // COLLECTION (Physical)
> #ifndef USE_FULL_YEAR
>             0x75, 0x08, // REPORT_SIZE (8 bits)
> #endif
>             0x95, 0x01, // REPORT_COUNT (1)
>             0x85, HID_REPORT_ID_TIME, // REPORT_ID
>
>             0x0a, 0x21, 0x05, // USAGE (Year)
> #ifdef USE_FULL_YEAR
>             0x75, 0x10, // REPORT_SIZE (16 bits)
> #else
>             0x15, 0x00, // LOGICAL_MINIMUM (0) (Range is currently not 
> specified in HUTRR39)
>             0x25, 0x63, // LOGICAL_MAXIMUM (99)
> #endif
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>             0x0a, 0x22, 0x05, // USAGE (Month)
> #ifdef USE_FULL_YEAR
>             0x75, 0x08, // REPORT_SIZE (8 bits)
> #endif
>             0x15, 0x01, // LOGICAL_MINIMUM (1)
>             0x25, 0x0c, // LOGICAL_MAXIMUM (12)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>             0x0a, 0x23, 0x05, // USAGE (Day)
>             0x15, 0x01, // LOGICAL_MINIMUM (1)
>             0x25, 0x1f, // LOGICAL_MAXIMUM (31)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>             0x0a, 0x24, 0x05, // USAGE (Day of Week)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x06, // LOGICAL_MAXIMUM (6)
>             0xa1, 0x02, // COLLECTION (Logical)
>                 0x0a, 0xc0, 0x08, // Day of Week: Sunday
>                 0x0a, 0xc1, 0x08, // Day of Week: Monday
>                 0x0a, 0xc2, 0x08, // Day of Week: Tuesday
>                 0x0a, 0xc3, 0x08, // Day of Week: Wednesday
>                 0x0a, 0xc4, 0x08, // Day of Week: Thursday
>                 0x0a, 0xc5, 0x08, // Day of Week: Friday
>                 0x0a, 0xc6, 0x08, // Day of Week: Saturday
>                 0x81, 0x02, // INPUT (Const,Arr,Abs)
>             0xc0, // END_COLLECTION
>
>             0x0a, 0x25, 0x05, // USAGE (Hour)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x17, // LOGICAL_MAXIMUM (23)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>             0x0a, 0x26, 0x05, // USAGE (Minute)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x3b, // LOGICAL_MAXIMUM (59)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>             0x0a, 0x27, 0x05, // USAGE (Second)
>             0x15, 0x00, // LOGICAL_MINIMUM (0)
>             0x25, 0x3b, // LOGICAL_MAXIMUM (59)
>             0x65, 0x00, // UNIT (None)
>             //0x66, 0x10, 0x01, // UNIT (second)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>             0x0a, 0x28, 0x05, // USAGE (Millisecond)
>             0x75, 0x10, // REPORT_SIZE (16 bits)
>             0x65, 0x00, // UNIT (None)
>             0x81, 0x02, // INPUT (Data,Var,Abs)
>
>         0xc0, // END_COLLECTION
>     0xc0, // END_COLLECTION
> --my-descriptor--
>
>>
>> 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.tissoires@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;
>> +}
>> +
>> +static void hid_scan_open_collection(struct hid_parser *parser, 
>> unsigned type)
>> +{
>> +    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));
>> +    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");
>> +            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);
>> +            goto out;
>> +        }
>> +
>> +        if (start == end) {
>> +            if (parser->local.delimiter_depth) {
>> +                hid_err(hid, "unbalanced delimiter at end of report 
>> description\n");
>> +                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;
>> +            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;
>> +    }
>> +
>> +    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 {
>>
>
>
Thanks,
Srinivas
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Holler Aug. 14, 2013, 8:03 p.m. UTC | #8
Am 14.08.2013 17:38, schrieb Benjamin Tissoires:

>>>   {
>>>   	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.
>
> If nobody is against changing the values of the different groups across
> kernel version (which should be harmless), then I fully agree, we can
> use the group item as a bit field (but we would be able to only have 16
> different device groups).

Hmm, that might become a problem. E.g. all the HID sensors might be used 
stand alone (without a sensor-hub, if someone modifies the drivers). But 
I agree that currently the flags are just confusing and would introduce 
them only if the number of groups reaches the limit.


>>> -	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...
>
> Yep, my first attempt was to remove it, then I re-added it with a small
> tear...

So you actually create a new parser and the subject (that existing) of 
this patch is misleading.

Regards,

Alexander Holler
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Tissoires Aug. 15, 2013, 5:36 p.m. UTC | #9
On Wed, Aug 14, 2013 at 10:03 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 14.08.2013 17:38, schrieb Benjamin Tissoires:
>
>
>>>>   {
>>>>         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.
>>
>>
>> If nobody is against changing the values of the different groups across
>> kernel version (which should be harmless), then I fully agree, we can
>> use the group item as a bit field (but we would be able to only have 16
>> different device groups).
>
>
> Hmm, that might become a problem. E.g. all the HID sensors might be used
> stand alone (without a sensor-hub, if someone modifies the drivers). But I
> agree that currently the flags are just confusing and would introduce them
> only if the number of groups reaches the limit.
>
>
>
>>>> -       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...
>>
>>
>> Yep, my first attempt was to remove it, then I re-added it with a small
>> tear...
>
>
> So you actually create a new parser and the subject (that existing) of this
> patch is misleading.

Hi Alexander,

I think you misread what Henrik and I were discussing about:
Henrik complained about using the heap for something which is just
used in this function, and using the stack would have been better.
However, the size of the parser is too big that the compiler complains
when we declare it on the stack.

So this line is just a copy/paste from the function hid_open_report(),
and thus, you can agree that I did not create a new parser.

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Holler Aug. 16, 2013, 8:54 a.m. UTC | #10
Am 15.08.2013 19:36, schrieb Benjamin Tissoires:
> On Wed, Aug 14, 2013 at 10:03 PM, Alexander Holler <holler@ahsoftware.de> wrote:
>>>>> -       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...
>>>
>>>
>>> Yep, my first attempt was to remove it, then I re-added it with a small
>>> tear...
>>
>>
>> So you actually create a new parser and the subject (that existing) of this
>> patch is misleading.
> 
> Hi Alexander,
> 
> I think you misread what Henrik and I were discussing about:
> Henrik complained about using the heap for something which is just
> used in this function, and using the stack would have been better.
> However, the size of the parser is too big that the compiler complains
> when we declare it on the stack.
> 
> So this line is just a copy/paste from the function hid_open_report(),
> and thus, you can agree that I did not create a new parser.

Hmm, not really, you do instantiate a new parser, wherever that one lives.

Of course, the code for the parser already exists, but at first I've
read the subject such, that something will be used which already was in
use. That "existing" did mislead me.

I think "Use hid_parser for ..." would describe the change more verbose
as the patch changes runtime behaviour quite a bit (by dispatching
everything through the parser).

Regards,

Alexander Holler
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
+}
+
+static void hid_scan_open_collection(struct hid_parser *parser, unsigned type)
+{
+	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));
+	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");
+			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);
+			goto out;
+		}
+
+		if (start == end) {
+			if (parser->local.delimiter_depth) {
+				hid_err(hid, "unbalanced delimiter at end of report description\n");
+				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;
+			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;
+	}
+
+	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 {