| Message ID | 20190109035018.GA12679@jelly (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | ee46967fc6e74d412fe1ec15f77fdb8624bde2b0 |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: core: replace the collection tree pointers with indices | expand |
On 1/9/19 2:48 PM, Pandruvada, Srinivas wrote: > On Wed, 2019-01-09 at 13:50 +1000, Peter Hutterer wrote: >> Introduced in c53431eb696f3c64c12c00afb81048af54b61532 >> "HID: core: store the collections as a basic tree". >> >> Previously, the pointer to the parent collection was stored. If a >> device >> exceeds 16 collections (HID_DEFAULT_NUM_COLLECTIONS), the array to >> store >> the collections is reallocated, the pointer to the parent collection >> becomes >> invalid. >> >> Replace the pointers with an index-based lookup into the collections >> array. >> >> Reported-by: Pandruvada, Srinivas <srinivas.pandruvada@intel.com> >> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>Tested-by: Kyle Pelton <kyle.d.pelton@linux.intel.com> >> --- >> I have a test locally for hid-tools now but it relies on the kernel >> crashing, so it's going to generate false positives. I verified the >> test >> with printks though. >> >> drivers/hid/hid-core.c | 32 +++++++++++++++++++++----------- >> include/linux/hid.h | 4 ++-- >> 2 files changed, 23 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c >> index f41d5fe51abe..f9093dedf647 100644 >> --- a/drivers/hid/hid-core.c >> +++ b/drivers/hid/hid-core.c >> @@ -125,6 +125,7 @@ static int open_collection(struct hid_parser >> *parser, unsigned type) >> { >> struct hid_collection *collection; >> unsigned usage; >> + int collection_index; >> >> usage = parser->local.usage[0]; >> >> @@ -167,13 +168,13 @@ static int open_collection(struct hid_parser >> *parser, unsigned type) >> parser->collection_stack[parser->collection_stack_ptr++] = >> parser->device->maxcollection; >> >> - collection = parser->device->collection + >> - parser->device->maxcollection++; >> + collection_index = parser->device->maxcollection++; >> + collection = parser->device->collection + collection_index; >> collection->type = type; >> collection->usage = usage; >> collection->level = parser->collection_stack_ptr - 1; >> - collection->parent = parser->active_collection; >> - parser->active_collection = collection; >> + collection->parent_idx = parser->active_collection_idx; >> + parser->active_collection_idx = collection_index; >> >> if (type == HID_COLLECTION_APPLICATION) >> parser->device->maxapplication++; >> @@ -192,8 +193,13 @@ static int close_collection(struct hid_parser >> *parser) >> return -EINVAL; >> } >> parser->collection_stack_ptr--; >> - if (parser->active_collection) >> - parser->active_collection = parser->active_collection- >> >parent; >> + if (parser->active_collection_idx != -1) { >> + struct hid_device *device = parser->device; >> + struct hid_collection *c; >> + >> + c = &device->collection[parser->active_collection_idx]; >> + parser->active_collection_idx = c->parent_idx; >> + } >> return 0; >> } >> >> @@ -819,6 +825,7 @@ static int hid_scan_report(struct hid_device >> *hid) >> return -ENOMEM; >> >> parser->device = hid; >> + parser->active_collection_idx = -1; >> hid->group = HID_GROUP_GENERIC; >> >> /* >> @@ -1006,10 +1013,12 @@ static void >> hid_apply_multiplier_to_field(struct hid_device *hid, >> usage = &field->usage[i]; >> >> collection = &hid->collection[usage->collection_index]; >> - while (collection && collection != >> multiplier_collection) >> - collection = collection->parent; >> + while (collection->parent_idx != -1 && >> + collection != multiplier_collection) >> + collection = &hid->collection[collection- >> >parent_idx]; >> >> - if (collection || multiplier_collection == NULL) >> + if (collection->parent_idx != -1 || >> + multiplier_collection == NULL) >> usage->resolution_multiplier = >> effective_multiplier; >> >> } >> @@ -1044,9 +1053,9 @@ static void hid_apply_multiplier(struct >> hid_device *hid, >> * applicable fields later. >> */ >> multiplier_collection = &hid->collection[multiplier->usage- >> >collection_index]; >> - while (multiplier_collection && >> + while (multiplier_collection->parent_idx != -1 && >> multiplier_collection->type != HID_COLLECTION_LOGICAL) >> - multiplier_collection = multiplier_collection->parent; >> + multiplier_collection = &hid- >> >collection[multiplier_collection->parent_idx]; >> >> effective_multiplier = hid_calculate_multiplier(hid, >> multiplier); >> >> @@ -1170,6 +1179,7 @@ int hid_open_report(struct hid_device *device) >> } >> >> parser->device = device; >> + parser->active_collection_idx = -1; >> >> end = start + size; >> >> diff --git a/include/linux/hid.h b/include/linux/hid.h >> index 93db548f8761..554e82d812da 100644 >> --- a/include/linux/hid.h >> +++ b/include/linux/hid.h >> @@ -429,7 +429,7 @@ struct hid_local { >> */ >> >> struct hid_collection { >> - struct hid_collection *parent; >> + int parent_idx; /* device->collection */ >> unsigned type; >> unsigned usage; >> unsigned level; >> @@ -657,7 +657,7 @@ struct hid_parser { >> unsigned int *collection_stack; >> unsigned int collection_stack_ptr; >> unsigned int collection_stack_size; >> - struct hid_collection *active_collection; >> + int active_collection_idx; /* device- >> >collection */ >> struct hid_device *device; >> unsigned int scan_flags; >> }; >
On Wed, 2019-01-09 at 13:50 +1000, Peter Hutterer wrote: > Introduced in c53431eb696f3c64c12c00afb81048af54b61532 > "HID: core: store the collections as a basic tree". > > Previously, the pointer to the parent collection was stored. If a > device > exceeds 16 collections (HID_DEFAULT_NUM_COLLECTIONS), the array to > store > the collections is reallocated, the pointer to the parent collection > becomes > invalid. > > Replace the pointers with an index-based lookup into the collections > array. > > Reported-by: Pandruvada, Srinivas <srinivas.pandruvada@intel.com> > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> Added Kyle to this email, who is testing this change. Thanks, Srinivas > --- > I have a test locally for hid-tools now but it relies on the kernel > crashing, so it's going to generate false positives. I verified the > test > with printks though. > > drivers/hid/hid-core.c | 32 +++++++++++++++++++++----------- > include/linux/hid.h | 4 ++-- > 2 files changed, 23 insertions(+), 13 deletions(-) > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index f41d5fe51abe..f9093dedf647 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -125,6 +125,7 @@ static int open_collection(struct hid_parser > *parser, unsigned type) > { > struct hid_collection *collection; > unsigned usage; > + int collection_index; > > usage = parser->local.usage[0]; > > @@ -167,13 +168,13 @@ static int open_collection(struct hid_parser > *parser, unsigned type) > parser->collection_stack[parser->collection_stack_ptr++] = > parser->device->maxcollection; > > - collection = parser->device->collection + > - parser->device->maxcollection++; > + collection_index = parser->device->maxcollection++; > + collection = parser->device->collection + collection_index; > collection->type = type; > collection->usage = usage; > collection->level = parser->collection_stack_ptr - 1; > - collection->parent = parser->active_collection; > - parser->active_collection = collection; > + collection->parent_idx = parser->active_collection_idx; > + parser->active_collection_idx = collection_index; > > if (type == HID_COLLECTION_APPLICATION) > parser->device->maxapplication++; > @@ -192,8 +193,13 @@ static int close_collection(struct hid_parser > *parser) > return -EINVAL; > } > parser->collection_stack_ptr--; > - if (parser->active_collection) > - parser->active_collection = parser->active_collection- > >parent; > + if (parser->active_collection_idx != -1) { > + struct hid_device *device = parser->device; > + struct hid_collection *c; > + > + c = &device->collection[parser->active_collection_idx]; > + parser->active_collection_idx = c->parent_idx; > + } > return 0; > } > > @@ -819,6 +825,7 @@ static int hid_scan_report(struct hid_device > *hid) > return -ENOMEM; > > parser->device = hid; > + parser->active_collection_idx = -1; > hid->group = HID_GROUP_GENERIC; > > /* > @@ -1006,10 +1013,12 @@ static void > hid_apply_multiplier_to_field(struct hid_device *hid, > usage = &field->usage[i]; > > collection = &hid->collection[usage->collection_index]; > - while (collection && collection != > multiplier_collection) > - collection = collection->parent; > + while (collection->parent_idx != -1 && > + collection != multiplier_collection) > + collection = &hid->collection[collection- > >parent_idx]; > > - if (collection || multiplier_collection == NULL) > + if (collection->parent_idx != -1 || > + multiplier_collection == NULL) > usage->resolution_multiplier = > effective_multiplier; > > } > @@ -1044,9 +1053,9 @@ static void hid_apply_multiplier(struct > hid_device *hid, > * applicable fields later. > */ > multiplier_collection = &hid->collection[multiplier->usage- > >collection_index]; > - while (multiplier_collection && > + while (multiplier_collection->parent_idx != -1 && > multiplier_collection->type != HID_COLLECTION_LOGICAL) > - multiplier_collection = multiplier_collection->parent; > + multiplier_collection = &hid- > >collection[multiplier_collection->parent_idx]; > > effective_multiplier = hid_calculate_multiplier(hid, > multiplier); > > @@ -1170,6 +1179,7 @@ int hid_open_report(struct hid_device *device) > } > > parser->device = device; > + parser->active_collection_idx = -1; > > end = start + size; > > diff --git a/include/linux/hid.h b/include/linux/hid.h > index 93db548f8761..554e82d812da 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -429,7 +429,7 @@ struct hid_local { > */ > > struct hid_collection { > - struct hid_collection *parent; > + int parent_idx; /* device->collection */ > unsigned type; > unsigned usage; > unsigned level; > @@ -657,7 +657,7 @@ struct hid_parser { > unsigned int *collection_stack; > unsigned int collection_stack_ptr; > unsigned int collection_stack_size; > - struct hid_collection *active_collection; > + int active_collection_idx; /* device- > >collection */ > struct hid_device *device; > unsigned int scan_flags; > };
On Wed, 9 Jan 2019, Peter Hutterer wrote: > Introduced in c53431eb696f3c64c12c00afb81048af54b61532 > "HID: core: store the collections as a basic tree". I've removed this from the changelog and added appropriate Fixes: tag, and applied to for-5.0/upstream-fixes. Thanks,
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index f41d5fe51abe..f9093dedf647 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -125,6 +125,7 @@ static int open_collection(struct hid_parser *parser, unsigned type) { struct hid_collection *collection; unsigned usage; + int collection_index; usage = parser->local.usage[0]; @@ -167,13 +168,13 @@ static int open_collection(struct hid_parser *parser, unsigned type) parser->collection_stack[parser->collection_stack_ptr++] = parser->device->maxcollection; - collection = parser->device->collection + - parser->device->maxcollection++; + collection_index = parser->device->maxcollection++; + collection = parser->device->collection + collection_index; collection->type = type; collection->usage = usage; collection->level = parser->collection_stack_ptr - 1; - collection->parent = parser->active_collection; - parser->active_collection = collection; + collection->parent_idx = parser->active_collection_idx; + parser->active_collection_idx = collection_index; if (type == HID_COLLECTION_APPLICATION) parser->device->maxapplication++; @@ -192,8 +193,13 @@ static int close_collection(struct hid_parser *parser) return -EINVAL; } parser->collection_stack_ptr--; - if (parser->active_collection) - parser->active_collection = parser->active_collection->parent; + if (parser->active_collection_idx != -1) { + struct hid_device *device = parser->device; + struct hid_collection *c; + + c = &device->collection[parser->active_collection_idx]; + parser->active_collection_idx = c->parent_idx; + } return 0; } @@ -819,6 +825,7 @@ static int hid_scan_report(struct hid_device *hid) return -ENOMEM; parser->device = hid; + parser->active_collection_idx = -1; hid->group = HID_GROUP_GENERIC; /* @@ -1006,10 +1013,12 @@ static void hid_apply_multiplier_to_field(struct hid_device *hid, usage = &field->usage[i]; collection = &hid->collection[usage->collection_index]; - while (collection && collection != multiplier_collection) - collection = collection->parent; + while (collection->parent_idx != -1 && + collection != multiplier_collection) + collection = &hid->collection[collection->parent_idx]; - if (collection || multiplier_collection == NULL) + if (collection->parent_idx != -1 || + multiplier_collection == NULL) usage->resolution_multiplier = effective_multiplier; } @@ -1044,9 +1053,9 @@ static void hid_apply_multiplier(struct hid_device *hid, * applicable fields later. */ multiplier_collection = &hid->collection[multiplier->usage->collection_index]; - while (multiplier_collection && + while (multiplier_collection->parent_idx != -1 && multiplier_collection->type != HID_COLLECTION_LOGICAL) - multiplier_collection = multiplier_collection->parent; + multiplier_collection = &hid->collection[multiplier_collection->parent_idx]; effective_multiplier = hid_calculate_multiplier(hid, multiplier); @@ -1170,6 +1179,7 @@ int hid_open_report(struct hid_device *device) } parser->device = device; + parser->active_collection_idx = -1; end = start + size; diff --git a/include/linux/hid.h b/include/linux/hid.h index 93db548f8761..554e82d812da 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -429,7 +429,7 @@ struct hid_local { */ struct hid_collection { - struct hid_collection *parent; + int parent_idx; /* device->collection */ unsigned type; unsigned usage; unsigned level; @@ -657,7 +657,7 @@ struct hid_parser { unsigned int *collection_stack; unsigned int collection_stack_ptr; unsigned int collection_stack_size; - struct hid_collection *active_collection; + int active_collection_idx; /* device->collection */ struct hid_device *device; unsigned int scan_flags; };
Introduced in c53431eb696f3c64c12c00afb81048af54b61532 "HID: core: store the collections as a basic tree". Previously, the pointer to the parent collection was stored. If a device exceeds 16 collections (HID_DEFAULT_NUM_COLLECTIONS), the array to store the collections is reallocated, the pointer to the parent collection becomes invalid. Replace the pointers with an index-based lookup into the collections array. Reported-by: Pandruvada, Srinivas <srinivas.pandruvada@intel.com> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> --- I have a test locally for hid-tools now but it relies on the kernel crashing, so it's going to generate false positives. I verified the test with printks though. drivers/hid/hid-core.c | 32 +++++++++++++++++++++----------- include/linux/hid.h | 4 ++-- 2 files changed, 23 insertions(+), 13 deletions(-)