| Message ID | 1376405889-12378-2-git-send-email-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
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
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
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
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
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
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
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
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
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
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 --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 {
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(-)