From: Mark Kettenis Subject: Re: imt(4)/umt(4): finding HID report IDs To: Ulf Brosziewski Cc: tech@openbsd.org Date: Fri, 18 Jul 2025 11:46:22 +0200 > Date: Wed, 16 Jul 2025 12:49:33 +0200 > From: Ulf Brosziewski > > On 7/16/25 00:07, Mark Kettenis wrote: > >> Date: Tue, 15 Jul 2025 23:08:29 +0200 > >> From: Ulf Brosziewski > >> > >> Anybody? > > > > The same comment about "top-level usage" not being a thing in the spec > > and being inconsistent with the existing comments applies here as well. > > > > Otherwise this makes (some) sense to me. > > In hid_find_report(), "top-level usage" is the more accurate term because > it refers to collections without a parent collection in the descriptor. > However, this should be an application collection ("Each top level > collection must be an application collection", hid1_11, p. 67). I could > rename "top_usage" to "app_usage" and add the check for the collection > type (h.collection == HCOLL_APPLICATION). Well, we need consistency here. If we use different terminology for the same concepts in related drivers, they become harder to maintain. If we use terminology that doesn't match the public standards, code becomes harder to maintain. > Is something else missing? More tests would be nice, of course, but it's > summertime. I had checked a userland version of hid_find_report() with a > set of report descriptors grabbed from the web. > > Skipping unknown collections is a recommendation from the specification: > "If the Usage attached to a collection is unknown to an application, > then the application should ignore the collection and all Usages > contained in the collection. A collection can be used to modify the > meaning of the Usages that it contains [...]" (hud1_6, p. 20) > > > > > >> On 7/2/25 01:37, Ulf Brosziewski wrote: > >>> The way imt(4) and umt(4) search for report IDs is broken. Depending > >>> on the structure and distribution of IDs in a report descriptor, it might > >>> succeed, or fail with wrong ID values (as has been reported by Thomas > >>> Dettbarn, see https://marc.info/?l=openbsd-tech&m=174841257723328&w=2 ). > >>> > >>> The hid_*_collection functions only provide partial information, which, > >>> in isolation, may be useless if you aren't lucky and deal with simple, > >>> uniform descriptors. > >>> > >>> The patch adds another helper function to hid.c, which permits searching > >>> for several report items at once, and filters out "unknown" collections > >>> (see the comment in the diff). It doesn't put further constraints on the > >>> search and the structure of the descriptor, but the heuristic might be > >>> safe enough for most practical purposes. However, my experience with the > >>> real HID world is limited, comments and hints would be appreciated. > >>> > >>> The patch moves the search for "PTP" reports to hidmt, and adapts imt > >>> and umt accordingly. Please note that it also contains a fix for another > >>> bug in hidmt (see https://marc.info/?l=openbsd-tech&m=175101436027656&w=2 ). > >>> It may be hidden by the faulty report search; that's why for tests, it > >>> makes sense to combine the patches. > >>> > >>> Tests, comments, and OKs would be welcome. > >>> > >>> > >>> Index: dev/hid/hid.c > >>> =================================================================== > >>> RCS file: /cvs/src/sys/dev/hid/hid.c,v > >>> retrieving revision 1.7 > >>> diff -u -p -r1.7 hid.c > >>> --- dev/hid/hid.c 10 May 2024 10:49:10 -0000 1.7 > >>> +++ dev/hid/hid.c 1 Jul 2025 20:11:57 -0000 > >>> @@ -706,3 +706,70 @@ hid_get_id_of_collection(const void *des > >>> hid_end_parse(hd); > >>> return -1; > >>> } > >>> + > >>> +/* > >>> + * Find the first report that contains each of the given "usages" > >>> + * and belongs to a top-level collection with the 'top_usage' type. > >>> + * The size of the 'usages' array must be in the range [1..32]. > >>> + * > >>> + * If 'coll_usages' is NULL, the search will skip collections with > >>> + * usages from vendor pages (0xFF00 - 0xFFFF). > >>> + * > >>> + * If 'coll_usages' is non-NULL, it must point to a 0-terminated > >>> + * sequence of collection usages, and the search will skip collections > >>> + * with usages not present in this set. (It isn't necessary to include > >>> + * the top-level usage here). > >>> + * > >>> + * Return Values: > >>> + * -1: No match > >>> + * 0: Success (single report without an ID) > >>> + * [1..255]: Report ID > >>> + */ > >>> +int > >>> +hid_find_report(const void *desc, int len, enum hid_kind kind, > >>> + int32_t top_usage, int n_usages, int32_t *usages, int32_t *coll_usages) > >>> +{ > >>> + struct hid_data *hd; > >>> + struct hid_item h; > >>> + uint32_t matches; > >>> + int i, cur_id, skip; > >>> + > >>> + hd = hid_start_parse(desc, len, hid_all); > >>> + for (cur_id = -1, skip = 0; hid_get_item(hd, &h); ) { > >>> + if (cur_id != h.report_ID) { > >>> + matches = 0; > >>> + cur_id = h.report_ID; > >>> + } > >>> + if (h.kind == hid_collection) { > >>> + if (skip) > >>> + continue; > >>> + if (h.collevel == 1) { > >>> + if (h.usage != top_usage) > >>> + skip = 1; > >>> + } else if (coll_usages != NULL) { > >>> + for (i = 0; coll_usages[i] != h.usage; i++) > >>> + if (coll_usages[i] == 0) { > >>> + skip = h.collevel; > >>> + break; > >>> + } > >>> + } else if (((h.usage >> 16) & 0xffff) >= 0xff00) { > >>> + skip = h.collevel; > >>> + } > >>> + } else if (h.kind == hid_endcollection) { > >>> + if (h.collevel < skip) > >>> + skip = 0; > >>> + } > >>> + if (h.kind != kind || skip) > >>> + continue; > >>> + for (i = 0; i < n_usages; i++) > >>> + if (h.usage == usages[i] && !(matches & (1 << i))) { > >>> + matches |= (1 << i); > >>> + if (matches != (1 << n_usages) - 1) > >>> + break; > >>> + hid_end_parse(hd); > >>> + return (h.report_ID); > >>> + } > >>> + } > >>> + hid_end_parse(hd); > >>> + return (-1); > >>> +} > >>> Index: dev/hid/hid.h > >>> =================================================================== > >>> RCS file: /cvs/src/sys/dev/hid/hid.h,v > >>> retrieving revision 1.11 > >>> diff -u -p -r1.11 hid.h > >>> --- dev/hid/hid.h 12 Aug 2023 20:47:06 -0000 1.11 > >>> +++ dev/hid/hid.h 1 Jul 2025 20:11:57 -0000 > >>> @@ -95,6 +95,8 @@ uint32_t hid_get_udata(const uint8_t *bu > >>> int hid_is_collection(const void *, int, uint8_t, int32_t); > >>> struct hid_data *hid_get_collection_data(const void *, int, int32_t, uint32_t); > >>> int hid_get_id_of_collection(const void *, int, int32_t, uint32_t); > >>> +int hid_find_report(const void *, int len, enum hid_kind, int32_t, > >>> + int, int32_t *, int32_t *); > >>> > >>> #endif /* _KERNEL */ > >>> > >>> Index: dev/hid/hidmt.c > >>> =================================================================== > >>> RCS file: /cvs/src/sys/dev/hid/hidmt.c,v > >>> retrieving revision 1.13 > >>> diff -u -p -r1.13 hidmt.c > >>> --- dev/hid/hidmt.c 16 Oct 2022 20:17:08 -0000 1.13 > >>> +++ dev/hid/hidmt.c 1 Jul 2025 20:11:57 -0000 > >>> @@ -42,6 +42,9 @@ > >>> #define DPRINTF(x) > >>> #endif > >>> > >>> +#define IS_TOP_LEVEL_USAGE(u) ((((u) >> 16) & 0xffff) == HUP_BUTTON || \ > >>> + (u) == HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTCOUNT)) > >>> + > >>> #define HID_UNIT_CM 0x11 > >>> #define HID_UNIT_INCH 0x13 > >>> > >>> @@ -373,7 +376,7 @@ hidmt_input(struct hidmt *mt, uint8_t *d > >>> > >>> bzero(&hc, sizeof(struct hidmt_contact)); > >>> } > >>> - else if (!firstu) > >>> + else if (!firstu && !IS_TOP_LEVEL_USAGE(hi->usage)) > >>> firstu = hi->usage; > >>> > >>> switch (hi->usage) { > >>> @@ -530,4 +533,41 @@ void > >>> hidmt_disable(struct hidmt *mt) > >>> { > >>> mt->sc_enabled = 0; > >>> +} > >>> + > >>> +int > >>> +hidmt_find_winptp_reports(const void *desc, int len, int *input_rid, > >>> + int *config_rid, int *cap_rid) > >>> +{ > >>> + static int32_t ptp_collections[] = { > >>> + HID_USAGE2(HUP_DIGITIZERS, HUD_FINGER), 0 > >>> + }; > >>> + static int32_t input_usages[] = { > >>> + /* top-level */ > >>> + HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTCOUNT), > >>> + /* contact-level */ > >>> + HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_X), > >>> + HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_Y), > >>> + HID_USAGE2(HUP_DIGITIZERS, HUD_CONFIDENCE), > >>> + HID_USAGE2(HUP_DIGITIZERS, HUD_TIP_SWITCH), > >>> + HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTID), > >>> + }; > >>> + static int32_t cfg_usages[] = { > >>> + HID_USAGE2(HUP_DIGITIZERS, HUD_INPUT_MODE), > >>> + }; > >>> + static int32_t cap_usages[] = { > >>> + HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACT_MAX), > >>> + }; > >>> + > >>> + *input_rid = hid_find_report(desc, len, hid_input, > >>> + HID_USAGE2(HUP_DIGITIZERS, HUD_TOUCHPAD), > >>> + nitems(input_usages), input_usages, ptp_collections); > >>> + *config_rid = hid_find_report(desc, len, hid_feature, > >>> + HID_USAGE2(HUP_DIGITIZERS, HUD_CONFIG), > >>> + nitems(cfg_usages), cfg_usages, ptp_collections); > >>> + *cap_rid = hid_find_report(desc, len, hid_feature, > >>> + HID_USAGE2(HUP_DIGITIZERS, HUD_TOUCHPAD), > >>> + nitems(cap_usages), cap_usages, ptp_collections); > >>> + > >>> + return (*input_rid > 0 && *config_rid > 0 && *cap_rid > 0); > >>> } > >>> Index: dev/hid/hidmtvar.h > >>> =================================================================== > >>> RCS file: /cvs/src/sys/dev/hid/hidmtvar.h,v > >>> retrieving revision 1.8 > >>> diff -u -p -r1.8 hidmtvar.h > >>> --- dev/hid/hidmtvar.h 8 Nov 2019 01:20:22 -0000 1.8 > >>> +++ dev/hid/hidmtvar.h 1 Jul 2025 20:11:57 -0000 > >>> @@ -77,3 +77,4 @@ int hidmt_enable(struct hidmt *); > >>> void hidmt_input(struct hidmt *, uint8_t *, u_int); > >>> int hidmt_ioctl(struct hidmt *, u_long, caddr_t, int, struct proc *); > >>> int hidmt_setup(struct device *, struct hidmt *, void *, int); > >>> +int hidmt_find_winptp_reports(const void *, int, int *, int *, int *); > >>> Index: dev/i2c/imt.c > >>> =================================================================== > >>> RCS file: /cvs/src/sys/dev/i2c/imt.c,v > >>> retrieving revision 1.6 > >>> diff -u -p -r1.6 imt.c > >>> --- dev/i2c/imt.c 13 May 2024 01:15:50 -0000 1.6 > >>> +++ dev/i2c/imt.c 1 Jul 2025 20:11:57 -0000 > >>> @@ -56,8 +56,6 @@ const struct wsmouse_accessops imt_acces > >>> }; > >>> > >>> int imt_match(struct device *, void *, void *); > >>> -int imt_find_winptp_reports(struct ihidev_softc *, void *, int, > >>> - struct imt_softc *); > >>> void imt_attach(struct device *, struct device *, void *); > >>> int imt_hidev_get_report(struct device *, int, int, void *, int); > >>> int imt_hidev_set_report(struct device *, int, int, void *, int); > >>> @@ -78,16 +76,17 @@ int > >>> imt_match(struct device *parent, void *match, void *aux) > >>> { > >>> struct ihidev_attach_arg *iha = (struct ihidev_attach_arg *)aux; > >>> - struct imt_softc sc; > >>> + int input_rid, conf_rid, cap_rid; > >>> int size; > >>> void *desc; > >>> > >>> if (iha->reportid == IHIDEV_CLAIM_MULTIPLEID) { > >>> ihidev_get_report_desc(iha->parent, &desc, &size); > >>> - if (imt_find_winptp_reports(iha->parent, desc, size, &sc)) { > >>> - iha->claims[0] = sc.sc_rep_input; > >>> - iha->claims[1] = sc.sc_rep_config; > >>> - iha->claims[2] = sc.sc_rep_cap; > >>> + if (hidmt_find_winptp_reports(desc, size, > >>> + &input_rid, &conf_rid, &cap_rid)) { > >>> + iha->claims[0] = input_rid; > >>> + iha->claims[1] = conf_rid; > >>> + iha->claims[2] = cap_rid; > >>> iha->nclaims = 3; > >>> return (IMATCH_DEVCLASS_DEVSUBCLASS); > >>> } > >>> @@ -96,49 +95,6 @@ imt_match(struct device *parent, void *m > >>> return (IMATCH_NONE); > >>> } > >>> > >>> -int > >>> -imt_find_winptp_reports(struct ihidev_softc *parent, void *desc, int size, > >>> - struct imt_softc *sc) > >>> -{ > >>> - int repid; > >>> - int input = 0, conf = 0, cap = 0; > >>> - > >>> - if (sc != NULL) { > >>> - sc->sc_rep_input = -1; > >>> - sc->sc_rep_config = -1; > >>> - sc->sc_rep_cap = -1; > >>> - } > >>> - > >>> - for (repid = 0; repid < parent->sc_nrepid; repid++) { > >>> - if (hid_report_size(desc, size, hid_input, repid) == 0 && > >>> - hid_report_size(desc, size, hid_output, repid) == 0 && > >>> - hid_report_size(desc, size, hid_feature, repid) == 0) > >>> - continue; > >>> - > >>> - if (hid_is_collection(desc, size, repid, > >>> - HID_USAGE2(HUP_DIGITIZERS, HUD_TOUCHPAD))) { > >>> - input = 1; > >>> - if (sc != NULL && sc->sc_rep_input == -1) > >>> - sc->sc_rep_input = repid; > >>> - } else if (hid_is_collection(desc, size, repid, > >>> - HID_USAGE2(HUP_DIGITIZERS, HUD_CONFIG))) { > >>> - conf = 1; > >>> - if (sc != NULL && sc->sc_rep_config == -1) > >>> - sc->sc_rep_config = repid; > >>> - } > >>> - > >>> - /* capabilities report could be anywhere */ > >>> - if (hid_locate(desc, size, HID_USAGE2(HUP_DIGITIZERS, > >>> - HUD_CONTACT_MAX), repid, hid_feature, NULL, NULL)) { > >>> - cap = 1; > >>> - if (sc != NULL && sc->sc_rep_cap == -1) > >>> - sc->sc_rep_cap = repid; > >>> - } > >>> - } > >>> - > >>> - return (conf && input && cap); > >>> -} > >>> - > >>> void > >>> imt_attach(struct device *parent, struct device *self, void *aux) > >>> { > >>> @@ -152,7 +108,8 @@ imt_attach(struct device *parent, struct > >>> sc->sc_hdev.sc_parent = iha->parent; > >>> > >>> ihidev_get_report_desc(iha->parent, &desc, &size); > >>> - imt_find_winptp_reports(iha->parent, desc, size, sc); > >>> + hidmt_find_winptp_reports(desc, size, &sc->sc_rep_input, > >>> + &sc->sc_rep_config, &sc->sc_rep_cap); > >>> > >>> memset(mt, 0, sizeof(sc->sc_mt)); > >>> > >>> Index: dev/usb/umt.c > >>> =================================================================== > >>> RCS file: /cvs/src/sys/dev/usb/umt.c,v > >>> retrieving revision 1.8 > >>> diff -u -p -r1.8 umt.c > >>> --- dev/usb/umt.c 26 May 2024 20:06:27 -0000 1.8 > >>> +++ dev/usb/umt.c 1 Jul 2025 20:11:57 -0000 > >>> @@ -60,8 +60,6 @@ const struct wsmouse_accessops umt_acces > >>> }; > >>> > >>> int umt_match(struct device *, void *, void *); > >>> -int umt_find_winptp_reports(struct uhidev_softc *, void *, int, int *, > >>> - int *, int *); > >>> void umt_attach(struct device *, struct device *, void *); > >>> int umt_hidev_get_report(struct device *, int, int, void *, int); > >>> int umt_hidev_set_report(struct device *, int, int, void *, int); > >>> @@ -88,7 +86,7 @@ umt_match(struct device *parent, void *m > >>> > >>> if (UHIDEV_CLAIM_MULTIPLE_REPORTID(uha)) { > >>> uhidev_get_report_desc(uha->parent, &desc, &size); > >>> - if (umt_find_winptp_reports(uha->parent, desc, size, &input, > >>> + if (hidmt_find_winptp_reports(desc, size, &input, > >>> &conf, &cap)) { > >>> uha->claimed[input] = 1; > >>> uha->claimed[conf] = 1; > >>> @@ -100,50 +98,6 @@ umt_match(struct device *parent, void *m > >>> return (UMATCH_NONE); > >>> } > >>> > >>> -int > >>> -umt_find_winptp_reports(struct uhidev_softc *parent, void *desc, int size, > >>> - int *input, int *config, int *cap) > >>> -{ > >>> - int repid; > >>> - int finput = 0, fconf = 0, fcap = 0; > >>> - > >>> - if (input != NULL) > >>> - *input = -1; > >>> - if (config != NULL) > >>> - *config = -1; > >>> - if (cap != NULL) > >>> - *cap = -1; > >>> - > >>> - for (repid = 0; repid < parent->sc_nrepid; repid++) { > >>> - if (hid_report_size(desc, size, hid_input, repid) == 0 && > >>> - hid_report_size(desc, size, hid_output, repid) == 0 && > >>> - hid_report_size(desc, size, hid_feature, repid) == 0) > >>> - continue; > >>> - > >>> - if (hid_is_collection(desc, size, repid, > >>> - HID_USAGE2(HUP_DIGITIZERS, HUD_TOUCHPAD))) { > >>> - finput = 1; > >>> - if (input != NULL && *input == -1) > >>> - *input = repid; > >>> - } else if (hid_is_collection(desc, size, repid, > >>> - HID_USAGE2(HUP_DIGITIZERS, HUD_CONFIG))) { > >>> - fconf = 1; > >>> - if (config != NULL && *config == -1) > >>> - *config = repid; > >>> - } > >>> - > >>> - /* capabilities report could be anywhere */ > >>> - if (hid_locate(desc, size, HID_USAGE2(HUP_DIGITIZERS, > >>> - HUD_CONTACT_MAX), repid, hid_feature, NULL, NULL)) { > >>> - fcap = 1; > >>> - if (cap != NULL && *cap == -1) > >>> - *cap = repid; > >>> - } > >>> - } > >>> - > >>> - return (fconf && finput && fcap); > >>> -} > >>> - > >>> void > >>> umt_attach(struct device *parent, struct device *self, void *aux) > >>> { > >>> @@ -163,7 +117,7 @@ umt_attach(struct device *parent, struct > >>> sc->sc_quirks = usbd_get_quirks(sc->sc_hdev.sc_udev)->uq_flags; > >>> > >>> uhidev_get_report_desc(uha->parent, &desc, &size); > >>> - umt_find_winptp_reports(uha->parent, desc, size, &sc->sc_rep_input, > >>> + hidmt_find_winptp_reports(desc, size, &sc->sc_rep_input, > >>> &sc->sc_rep_config, &sc->sc_rep_cap); > >>> > >>> memset(mt, 0, sizeof(sc->sc_mt)); > >>> > >> > >> > >