On 7/2/25 11:38, Mark Kettenis wrote:
>> 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.

I haven't seen such a requirement in the HID specifications, but the
Implementation Guide for Precision Touchpads describes the "Parallel
Mode" of packet reporting as follows:
  "In Parallel mode, devices report all contact information in a single
  packet. Each physical contact is represented by a logical collection
  that is embedded in the top-level collection. This logical collection
  contains all the usages that the device supports for each contact.
  When you use Parallel mode, each of the logical collections must be
  identical."
  ( https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-buttons-report-level-usages )

(The spec doesn't state explicitly that complete and "identical"
collections are also required in "Hybrid Mode", but who would think
that this means freestyle?  Perhaps we should think about more elaborate
checks when we know that they are necessary).

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

I agree.  I will change that.

I will commit this when the second patch is through (or not).

> 
>>
>> (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) {
>>
>>