Page MenuHomeFreeBSD

hid: fix typo in hid_is_collection
ClosedPublic

Authored by ankohuu_outlook.com on Jan 14 2021, 5:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 17, 10:07 PM
Unknown Object (File)
Tue, Apr 16, 8:14 PM
Unknown Object (File)
Fri, Apr 5, 1:52 PM
Unknown Object (File)
Dec 20 2023, 8:04 AM
Unknown Object (File)
Nov 29 2023, 3:00 PM
Unknown Object (File)
Oct 28 2023, 8:10 AM
Unknown Object (File)
Oct 27 2023, 2:55 AM
Unknown Object (File)
Oct 15 2023, 1:34 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
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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
802

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
802

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
802

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

This revision was not accepted when it landed; it landed in state Needs Revision.Apr 24 2023, 11:20 AM
This revision was automatically updated to reflect the committed changes.