Thanks for testing this Greg. On Sat, 16 Jan 2021 16:10:26 -0800 Greg Steuck <gne...@openbsd.org> wrote:
> Hi Marcus, > > Marcus Glocker <mar...@nazgul.ch> writes: > > > There are a few threads going on related to problems with ugen(4) > > and uhidev(4) devices on xhci(4). This is related to the issue > > patrick@ already explained; while ehci(4) can save the last data > > toggle state, xhci(4) resets it on every open/close cycle, getting > > out of sync with the device. > > Is this related to the terrible contortions we go through in > src/lib/libfido2/src/hid_openbsd.c:terrible_ping_kludge? Same code > is also included into firefox and our chromium to get fido(4) to work. Oh, I wasn't even aware of this code. But yes, this is exactly what we want to solve with this diff. > Security keys work no better or worse with your patch. If you believe > there's a chance to remove terrible_ping_kludge, I'll try. That would be very interesting to test. Only what worries me at the moment are ... > > This diff addresses a possible solution for ugen(4) and uhidev(4). > > For ugen(4) we already did some positive testing related to scanner > > devices, see e.g.: > > > > https://marc.info/?l=openbsd-bugs&m=161056827312265&w=2 > > > > I would appreciate some regression testing and feedback. On xhci(4) > > it might resolve issues related to that, and on ehci(4) it shouldn't > > break your existing setup. > > I tested with xhci. Other than this message I see no changes in > behavior: > > uhidev_clear_iface_eps: clear endpoints failed 3! > uhidev_clear_iface_eps: clear endpoints failed 3! > uhidev_clear_iface_eps: clear endpoints failed 3! ... those messages you're getting. It basically means that the clear endpoint feature command is failing on some of your endpoints. So depending on which endpoints those are, I'm not sure if the diff works as expected. If you could also print the number of the failing endpoints and compare them with an lsusb of that device, it would help to understand what kind of endpoints those are, and how to proceed with that. > To make this a little more useful, I applied a bit more diagnostics on > top of your patch: > > diff --git a/sys/dev/usb/uhidev.c b/sys/dev/usb/uhidev.c > index 4ddf9425170..6296684fa8d 100644 > --- a/sys/dev/usb/uhidev.c > +++ b/sys/dev/usb/uhidev.c > @@ -966,6 +966,7 @@ uhidev_clear_iface_eps(struct uhidev_softc *sc, > struct usbd_interface *iface) usb_endpoint_descriptor_t *ed; > uint8_t xfertype; > int i; > + int reason = 0; > > /* Only clear interface endpoints when none are in use. */ > if (sc->sc_ipipe || sc->sc_opipe) > @@ -973,22 +974,28 @@ uhidev_clear_iface_eps(struct uhidev_softc *sc, > struct usbd_interface *iface) DPRINTFN(1,("%s: clear interface > eps\n", __func__)); > id = usbd_get_interface_descriptor(iface); > - if (id == NULL) > + if (id == NULL) { > + reason = 1; > goto bad; > + } > > for (i = 0; i < id->bNumEndpoints; i++) { > ed = usbd_interface2endpoint_descriptor(iface, i); > - if (ed == NULL) > + if (ed == NULL) { > + reason = 2; > goto bad; > + } > > xfertype = UE_GET_XFERTYPE(ed->bmAttributes); > if (xfertype == UE_BULK || xfertype == UE_INTERRUPT) > { if (usbd_clear_endpoint_feature(sc->sc_udev, > - ed->bEndpointAddress, UF_ENDPOINT_HALT)) > + ed->bEndpointAddress, UF_ENDPOINT_HALT)) > { > + reason = 3; > goto bad; > + } > } > } > return; > bad: > - printf("%s: clear endpoints failed!\n", __func__); > + printf("%s: clear endpoints failed %d!\n", __func__, reason); > } > > dmesg with your original patch: > > xhci0 at pci0 dev 20 function 0 "Intel 8 Series xHCI" rev 0x04: msi, > xHCI 1.0 usb0 at xhci0: USB revision 3.0 > uhub0 at usb0 configuration 1 interface 0 "Intel xHCI root hub" rev > 3.00/1.00 addr 1 usb1 at ehci0: USB revision 2.0 > uhub1 at usb1 configuration 1 interface 0 "Intel EHCI root hub" rev > 2.00/1.00 addr 1 uhidev0 at uhub3 port 3 configuration 1 interface 0 > "Microsoft Microsoft 3-Button Mouse with IntelliEye(TM)" rev > 1.10/3.00 addr 5 uhidev0: iclass 3/1 ums0 at uhidev0: 3 buttons, Z dir > uhidev1 at uhub3 port 1 configuration 1 interface 0 "Google Inc. > hg_0004" rev 2.00/1.00 addr 9 uhidev1: iclass 3/0 > fido0 at uhidev1: input=64, output=64, feature=0 > uhidev2 at uhub3 port 4 configuration 1 interface 0 "Kinesis > Advantage2 Keyboard" rev 2.00/1.00 addr 10 uhidev2: iclass 3/1 > ums1 at uhidev2: 3 buttons, Z dir > uhidev3 at uhub3 port 4 configuration 1 interface 1 "Kinesis > Advantage2 Keyboard" rev 2.00/1.00 addr 10 uhidev3: iclass 3/1 > ukbd0 at uhidev3: 8 variable keys, 6 key codes > uhidev4 at uhub3 port 4 configuration 1 interface 2 "Kinesis > Advantage2 Keyboard" rev 2.00/1.00 addr 10 uhidev4: iclass 3/0, 2 > report ids uhid0 at uhidev4 reportid 1: input=1, output=0, feature=0 > uhid1 at uhidev4 reportid 2: input=1, output=0, feature=0 > uhidev_clear_iface_eps: clear endpoints failed! > uhidev_clear_iface_eps: clear endpoints failed! > uhidev_clear_iface_eps: clear endpoints failed! > uhidev5 at uhub0 port 2 configuration 1 interface 0 "Yubico Security > Key by Yubico" rev 2.00/3.33 addr 11 uhidev5: iclass 3/0 > fido1 at uhidev5: input=64, output=64, feature=0 > fido1 detached > uhidev5 detached > uhidev_clear_iface_eps: clear endpoints failed! > uhidev_clear_iface_eps: clear endpoints failed! > uhidev_clear_iface_eps: clear endpoints failed! > uhidev5 at uhub0 port 2 configuration 1 interface 0 "Yubico Security > Key by Yubico" rev 2.00/3.33 addr 11 uhidev5: iclass 3/0 > fido1 at uhidev5: input=64, output=64, feature=0 > fido1 detached > uhidev5 detached > uhidev5 at uhub0 port 2 configuration 1 interface 0 "Yubico Security > Key by Yubico" rev 2.00/3.33 addr 11 uhidev5: iclass 3/0 > fido1 at uhidev5: input=64, output=64, feature=0 > uhidev_clear_iface_eps: clear endpoints failed! > fido1 detached > uhidev5 detached > uhidev5 at uhub0 port 2 configuration 1 interface 0 "Yubico Security > Key by Yubico" rev 2.00/3.33 addr 11 uhidev5: iclass 3/0 > fido1 at uhidev5: input=64, output=64, feature=0 > fido1 detached > uhidev5 detached > uhidev5 at uhub0 port 2 configuration 1 interface 0 "Yubico Security > Key by Yubico" rev 2.00/3.33 addr 11 uhidev5: iclass 3/0 > fido1 at uhidev5: input=64, output=64, feature=0 > uhidev_clear_iface_eps: clear endpoints failed! > uhidev_clear_iface_eps: clear endpoints failed! > fido1 detached > uhidev5 detached > uhidev5 at uhub0 port 2 configuration 1 interface 0 "Yubico Security > Key by Yubico" rev 2.00/3.33 addr 11 uhidev5: iclass 3/0 > fido1 at uhidev5: input=64, output=64, feature=0 > uhidev_clear_iface_eps: clear endpoints failed! > uhidev_clear_iface_eps: clear endpoints failed! > uhidev_clear_iface_eps: clear endpoints failed! > uhidev_clear_iface_eps: clear endpoints failed! > uhidev_clear_iface_eps: clear endpoints failed! > uhidev_clear_iface_eps: clear endpoints failed! > fido1 detached > uhidev5 detached > uhidev_clear_iface_eps: clear endpoints failed! > uhidev5 at uhub0 port 2 configuration 1 interface 0 "Yubico Security > Key by Yubico" rev 2.00/3.33 addr 11 uhidev5: iclass 3/0 > fido1 at uhidev5: input=64, output=64, feature=0 >
