Date:	Wed, 14 Aug 2013 17:38:12 +0200
From:	Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:	rydberg@...omail.se
CC:	Benjamin Tissoires <benjamin.tissoires@...il.com>,
	Jiri Kosina <jkosina@...e.cz>,
	Stephane Chatty <chatty@...c.fr>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Alexander Holler <holler@...oftware.de>,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] HID: Use existing parser for pre-scanning the report
 descriptors


Hi Henrik,

On 13/08/13 21:17, rydberg@...omail.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@...hat.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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists