On Sat, 9 Jan 2021 21:51:03 +0100 (CET) Mark Kettenis <[email protected]> wrote:
> > Date: Sat, 9 Jan 2021 10:38:20 +0100 > > From: Marcus Glocker <[email protected]> > > > > I have not much clue about HID, but when we did some testing for the > > new ujoy(4) driver it turned out that the PS4 controller doesn't get > > handled correctly by hid.c:hid_is_collection(). This made me peek > > in to the NetBSD code where I could find an update in this function. > > > > Syncing it up makes the PS4 controller attachment work correctly, > > while not breaking my other HID devices. > > > > The change was introduced in NetBSD with hid.c revision 1.25 on the > > 19.06.2006, obviously as part of a bigger change, not saying > > something about the hid_is_collection() update itself: > > > > -- > > > > Initial import of bluetooth stack on behalf of Iain Hibbert. > > (plunky@, NetBSD Foundation Membership still pending.) This stack > > was written by Iain under sponsorship from Itronix Inc. > > > > The stack includes support for rfcomm networking (networking via > > your bluetooth enabled cell phone), hid devices (keyboards/mice), > > and headsets. > > > > Drivers for both PCMCIA and USB bluetooth controllers are included. > > > > -- > > > > http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/usb/Attic/hid.c.diff?r1=1.24&r2=1.25&only_with_tag=MAIN > > > > More testers? Comments? OKs? > > Probably needs some testing with touchpad and touchscreen devices. Hmm, yes. I just quickly tested this on my X1 with ums(4) touchscreen. Beside that it still works, the diff seems to fix something in the ums attach. Before the diff: uhidev0 at uhub0 port 10 configuration 1 interface 0 "Raydium Corporation Raydium Touch System" rev 2.01/1.08 addr 4 uhidev0: iclass 3/0, 68 report ids uhid0 at uhidev0 reportid 1: input=0, output=63, feature=0 uhid1 at uhidev0 reportid 2: input=63, output=0, feature=0 uhid2 at uhidev0 reportid 3: input=0, output=63, feature=0 uhid3 at uhidev0 reportid 4: input=0, output=63, feature=0 uhid4 at uhidev0 reportid 5: input=0, output=63, feature=0 uhid5 at uhidev0 reportid 6: input=63, output=0, feature=0 uhid6 at uhidev0 reportid 7: input=0, output=63, feature=0 uhid7 at uhidev0 reportid 8: input=0, output=63, feature=0 uhid8 at uhidev0 reportid 9: input=63, output=0, feature=0 ums0 at uhidev0 reportid 10: 1 button, tip wsmouse2 at ums0 mux 0 uhid9 at uhidev0 reportid 11: input=0, output=0, feature=1 uhid10 at uhidev0 reportid 12: input=0, output=0, feature=255 uhid11 at uhidev0 reportid 13: input=0, output=0, feature=255 ums1 at uhidev0 reportid 68 ums1: mouse has no X report I didn't notice before, but ums1 seems not to make sense, since there is no second ums device attached here. After the diff the reportid 68 just turns in another uhid of uhidev0 which makes more sense I guess: uhidev0 at uhub0 port 10 configuration 1 interface 0 "Raydium Corporation Raydium Touch System" rev 2.01/1.08 addr 4 uhidev0: iclass 3/0, 68 report ids uhid0 at uhidev0 reportid 1: input=0, output=63, feature=0 uhid1 at uhidev0 reportid 2: input=63, output=0, feature=0 uhid2 at uhidev0 reportid 3: input=0, output=63, feature=0 uhid3 at uhidev0 reportid 4: input=0, output=63, feature=0 uhid4 at uhidev0 reportid 5: input=0, output=63, feature=0 uhid5 at uhidev0 reportid 6: input=63, output=0, feature=0 uhid6 at uhidev0 reportid 7: input=0, output=63, feature=0 uhid7 at uhidev0 reportid 8: input=0, output=63, feature=0 uhid8 at uhidev0 reportid 9: input=63, output=0, feature=0 ums0 at uhidev0 reportid 10: 1 button, tip wsmouse2 at ums0 mux 0 uhid9 at uhidev0 reportid 11: input=0, output=0, feature=1 uhid10 at uhidev0 reportid 12: input=0, output=0, feature=255 uhid11 at uhidev0 reportid 13: input=0, output=0, feature=255 uhid12 at uhidev0 reportid 68: input=0, output=0, feature=255 > > Index: dev/hid/hid.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/hid/hid.c,v > > retrieving revision 1.3 > > diff -u -p -u -p -r1.3 hid.c > > --- dev/hid/hid.c 4 Jun 2020 23:03:43 -0000 1.3 > > +++ dev/hid/hid.c 9 Jan 2021 08:43:30 -0000 > > @@ -630,6 +630,25 @@ hid_get_udata(const uint8_t *buf, int le > > return (hid_get_data_sub(buf, len, loc, 0)); > > } > > > > +/* > > + * hid_is_collection(desc, size, id, usage) > > + * > > + * This function is broken in the following way. > > + * > > + * It is used to discover if the given 'id' is part of 'usage' > > collection > > + * in the descriptor in order to match report id against device > > type. > > + * > > + * The semantics of hid_start_parse() means though, that only a > > single > > + * kind of report is considered. The current HID code that uses > > this for > > + * matching is actually only looking for input reports, so this > > works > > + * for now. > > + * > > + * This function could try all report kinds (input, output and > > feature) > > + * consecutively if necessary, but it may be better to integrate > > the > > + * libusbhid code which can consider multiple report kinds > > simultaneously > > + * > > + * Needs some thought. > > + */ > > int > > hid_is_collection(const void *desc, int size, uint8_t id, int32_t > > usage) { > > @@ -637,17 +656,25 @@ hid_is_collection(const void *desc, int > > struct hid_item hi; > > uint32_t coll_usage = ~0; > > > > - hd = hid_start_parse(desc, size, hid_none); > > + hd = hid_start_parse(desc, size, hid_input); > > + if (hd == NULL) > > + return (0); > > > > DPRINTF("%s: id=%d usage=0x%x\n", __func__, id, usage); > > while (hid_get_item(hd, &hi)) { > > DPRINTF("%s: kind=%d id=%d usage=0x%x(0x%x)\n", > > __func__, hi.kind, hi.report_ID, hi.usage, coll_usage); > > + > > if (hi.kind == hid_collection && > > hi.collection == HCOLL_APPLICATION) > > coll_usage = hi.usage; > > - if (hi.kind == hid_endcollection && > > - coll_usage == usage && hi.report_ID == id) { > > + > > + if (hi.kind == hid_endcollection) > > + coll_usage = ~0; > > + > > + if (hi.kind == hid_input && > > + coll_usage == usage && > > + hi.report_ID == id) { > > DPRINTF("%s: found\n", __func__); > > hid_end_parse(hd); > > return (1); > > > >
