Page MenuHomeFreeBSD

hid: fix typo in hid_is_collection
Needs RevisionPublic

Authored by ankohuu_outlook.com on Jan 14 2021, 5:10 AM.

Details

Summary

I found that in function hid_is_collection the third parameter
of hid_start_parse call is enum item 'hid_input'. I think
it should be a zero value bitmap here, although the value of hid_input currently
is 0. It makes more sense.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 36192
Build 33081: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Jan 14 2021, 8:27 AM
delphij requested changes to this revision.Jan 14 2021, 8:59 AM
delphij added a subscriber: delphij.
delphij added inline comments.
sys/dev/hid/hid.c
806

If I am understanding the code correctly (disclaimer: I am not familiar with this particular module so I could well be wrong), it appears like this should really be 1 << hid_input here. Passing 0 worked because when kind is collection, the kindset is blindly ignored, but for consistency with all other usages in the tree, I still think it's better to pass 1 << hid_input here.

BTW. despite documented as allowing a set of bits being passed in kindset, the code would fail when multiple bits are passed to hid_start_parse. It wasn't clear if that's a deliberate choice, or simply that was not being implemented, but the inconsistency of how kindset was kinda confusing.

This revision now requires changes to proceed.Jan 14 2021, 8:59 AM
wulf added inline comments.
sys/dev/hid/hid.c
806

Passing 0 is correct for our simple hid_is_collection() implementation.

1 << hid_input is required for more complex NetBSD version which tracks for input usage presence within a collection. See
https://github.com/NetBSD/src/blob/trunk/sys/dev/hid/hid.c#L516

So we should replace hid_input with 0 and leave while loop as is or replace hid_input with 1 << hid_input and import NetBSD code.

sys/dev/hid/hid.c
806

If I am understanding the code correctly (disclaimer: I am not familiar with this particular module so I could well be wrong), it appears like this should really be 1 << hid_input here. Passing 0 worked because when kind is collection, the kindset is blindly ignored, but for consistency with all other usages in the tree, I still think it's better to pass 1 << hid_input here.

I think hid_start_parse is for calling hid_get_item in a subsequent loop later. What kind of item we want to get in hid_get_item must be reflected in the third parameter kindset of hid_start_parse. In addition, items of type hid_collection/hid_endcollection will be returned regardless of the kindset.
Hid_is_collection tries to find out whether there is a hid_collection item of the specified usage in the descriptor given by the parameter, so it seems that the kindset is assigned to 1 << hid_input in the implementation and traverse the hid_input kind items is not necessary.

BTW. despite documented as allowing a set of bits being passed in kindset, the code would fail when multiple bits are passed to hid_start_parse. It wasn't clear if that's a deliberate choice, or simply that was not being implemented, but the inconsistency of how kindset was kinda confusing.

Maybe simply that was not being implemented in sys/dev/hid/hid.c, when multiple bits are passed to hid_start_parse, hid_get_item can't get item position correctly, because pos in struct hid_data can't handle multiple kinds, functions in lib/libusbhid/parse.c work the same as described in the USBHID(3).