From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: hidmt: separate contact inputs correctly
To: Ulf Brosziewski <ulf.brosziewski@t-online.de>
Cc: tech@openbsd.org
Date: Wed, 02 Jul 2025 11:38:06 +0200

> Date: Thu, 26 Jun 2025 22:22:03 +0200
> From: Ulf Brosziewski <ulf.brosziewski@t-online.de>
> 
> Thomas Dettbarn has found, analyzed und reported two bugs in imt(4) (see
>     https://marc.info/?l=openbsd-tech&m=174841257723328&w=2
> and the related messages).
> 
> This patch is a fix for the first one.
> 
> When processing input reports, hidmt_input identifies groups of contact-
> level items by checking for the repetition of an item type - the first
> type it has encountered in the report.  This fails for reports starting
> with top-level items, which only occur once.  (From the top-level
> collection, hidmt includes the contact count and the button states in
> its list of input items.)
> 
> The 'firstu' value must not be set to a top-level type.  The fix is
> trivial, and I don't see any risk of regressions.
> 
> OK?

HID report descriptors always confuse me.  I think the patch is
certainly an improvement, although I have some doubts how robust the
whole "firstu" thing is.  While it makes a lot of sense for a
descriptor to have its contact level usages in the same order for each
contact, I don't think the HID specification requires this to be the
case.  But let's don't worry about this for now.

So I think the diff is ok, but the use of the term "top-level"
confuses me a bit as the spec talks about top-level *collections* but
not top-level usages.  Elsewhere in the code the terms "contact level
usages" and "report level usages".  And the latter is what you mean
with "top-level".  So I think it is better if you use
IS_REPORT_LEVEL_USAGE() as the name for the macro.

> 
> (The second bug is in the *_find_winptp_reports functions in imt(4)
> and umt(4), which match wrong report IDs under certain conditions.
> Another patch will follow soon; it might be the short workaround from
> Thomas - see the imt.c part of his patch - or, perhaps, a more
> systematic solution.  I haven't made up my mind yet.)
> 
> 
> Index: hidmt.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/hid/hidmt.c,v
> diff -u -p -r1.13 hidmt.c
> --- hidmt.c	16 Oct 2022 20:17:08 -0000	1.13
> +++ hidmt.c	26 Jun 2025 12:40:56 -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) {
> 
>