Index | Thread | Search

From:
Thomas Dettbarn <dettus@dettus.net>
Subject:
Re: imt(4)/umt(4): finding HID report IDs
To:
Ulf Brosziewski <ulf.brosziewski@t-online.de>, Mark Kettenis <mark.kettenis@xs4all.nl>
Cc:
tech@openbsd.org
Date:
Wed, 16 Jul 2025 16:56:17 +0200

Download raw body.

Thread
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 <ulf.brosziewski@t-online.de> 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 <ulf.brosziewski@t-online.de>
> >>
> >> 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));
> >>>
> >>
> >>