From: Thomas Dettbarn Subject: Re: imt(4)/umt(4): finding HID report IDs To: Ulf Brosziewski , Mark Kettenis Cc: tech@openbsd.org Date: Wed, 16 Jul 2025 16:56:17 +0200 Hello! If you guys are still hung up on the name of this macro: #define IS_TOP_LEVEL_USAGE(u) ((((u) >> 16) & 0xffff) == HUP_BUTTON || \ (u) == HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTCOUNT)) let me offer my 2 cents... What you are actually trying to match with it is a usage ID for a button or something equivalent. So "TOP_LEVEL_USAGE" is perfect for the first half of the Macro, but the second half, after the ||, it is not. Sorry, Ulf! My propopsal: #define IS_USAGEID_FOR_BUTTON(u) ... Thomas > Ulf Brosziewski hat am 16.07.2025 12:49 CEST geschrieben: > > > 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). > > 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)); > >>> > >> > >>