On Sun, Feb 14, 2021 at 03:34:11PM +0000, Mikolaj Kucharski wrote: > On Sun, Feb 14, 2021 at 03:22:28PM +0100, Marcus Glocker wrote: > > Unfortunately I'm seeing more and more USB device breakages reported > > the last few days related to the USB data toggle fix which we did > > commit 2-3 weeks ago. > > > > Since I can't reproduce the issue here with my USB gear, it's very > > difficult for me to pin point the issue. Therefore I think we should > > back it out for now. > > > > OK? > > Could ugen changes stay and just uhidev be reverted?
Good point. Since I haven't received any problem reports about devices attaching to ugen(4), I'll leave it in for now. > > Index: lib/libfido2/src/hid_openbsd.c > > =================================================================== > > RCS file: /cvs/src/lib/libfido2/src/hid_openbsd.c,v > > retrieving revision 1.6 > > diff -u -p -u -p -r1.6 hid_openbsd.c > > --- lib/libfido2/src/hid_openbsd.c 5 Feb 2021 14:19:21 -0000 1.6 > > +++ lib/libfido2/src/hid_openbsd.c 14 Feb 2021 13:58:22 -0000 > > @@ -88,6 +88,62 @@ fido_hid_manifest(fido_dev_info_t *devli > > return FIDO_OK; > > } > > > > +/* > > + * Workaround for OpenBSD <=6.6-current (as of 201910) bug that loses > > + * sync of DATA0/DATA1 sequence bit across uhid open/close. > > + * Send pings until we get a response - early pings with incorrect > > + * sequence bits will be ignored as duplicate packets by the device. > > + */ > > +static int > > +terrible_ping_kludge(struct hid_openbsd *ctx) > > +{ > > + u_char data[256]; > > + int i, n; > > + struct pollfd pfd; > > + > > + if (sizeof(data) < ctx->report_out_len + 1) > > + return -1; > > + for (i = 0; i < 4; i++) { > > + memset(data, 0, sizeof(data)); > > + /* broadcast channel ID */ > > + data[1] = 0xff; > > + data[2] = 0xff; > > + data[3] = 0xff; > > + data[4] = 0xff; > > + /* Ping command */ > > + data[5] = 0x81; > > + /* One byte ping only, Vasili */ > > + data[6] = 0; > > + data[7] = 1; > > + fido_log_debug("%s: send ping %d", __func__, i); > > + if (fido_hid_write(ctx, data, ctx->report_out_len + 1) == -1) > > + return -1; > > + fido_log_debug("%s: wait reply", __func__); > > + memset(&pfd, 0, sizeof(pfd)); > > + pfd.fd = ctx->fd; > > + pfd.events = POLLIN; > > + if ((n = poll(&pfd, 1, 100)) == -1) { > > + fido_log_debug("%s: poll: %s", __func__, > > strerror(errno)); > > + return -1; > > + } else if (n == 0) { > > + fido_log_debug("%s: timed out", __func__); > > + continue; > > + } > > + if (fido_hid_read(ctx, data, ctx->report_out_len, 250) == -1) > > + return -1; > > + /* > > + * Ping isn't always supported on the broadcast channel, > > + * so we might get an error, but we don't care - we're > > + * synched now. > > + */ > > + fido_log_debug("%s: got reply", __func__); > > + fido_log_xxd(data, ctx->report_out_len); > > + return 0; > > + } > > + fido_log_debug("%s: no response", __func__); > > + return -1; > > +} > > + > > void * > > fido_hid_open(const char *path) > > { > > @@ -101,6 +157,16 @@ fido_hid_open(const char *path) > > ret->report_in_len = ret->report_out_len = MAX_U2FHID_LEN; > > fido_log_debug("%s: inlen = %zu outlen = %zu", __func__, > > ret->report_in_len, ret->report_out_len); > > + > > + /* > > + * OpenBSD (as of 201910) has a bug that causes it to lose > > + * track of the DATA0/DATA1 sequence toggle across uhid device > > + * open and close. This is a terrible hack to work around it. > > + */ > > + if (terrible_ping_kludge(ret) != 0) { > > + fido_hid_close(ret); > > + return NULL; > > + } > > > > return (ret); > > } > > Index: sys/dev/usb/ugen.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/ugen.c,v > > retrieving revision 1.115 > > diff -u -p -u -p -r1.115 ugen.c > > --- sys/dev/usb/ugen.c 5 Feb 2021 08:17:22 -0000 1.115 > > +++ sys/dev/usb/ugen.c 14 Feb 2021 13:58:26 -0000 > > @@ -117,7 +117,6 @@ int ugen_do_close(struct ugen_softc *, i > > int ugen_set_config(struct ugen_softc *, int); > > int ugen_set_interface(struct ugen_softc *, int, int); > > int ugen_get_alt_index(struct ugen_softc *, int); > > -void ugen_clear_iface_eps(struct ugen_softc *, struct usbd_interface *); > > > > #define UGENUNIT(n) ((minor(n) >> 4) & 0xf) > > #define UGENENDPOINT(n) (minor(n) & 0xf) > > @@ -310,8 +309,6 @@ ugenopen(dev_t dev, int flag, int mode, > > DPRINTFN(5, ("ugenopen: sc=%p, endpt=%d, dir=%d, sce=%p\n", > > sc, endpt, dir, sce)); > > edesc = sce->edesc; > > - /* Clear device endpoint toggle. */ > > - ugen_clear_iface_eps(sc, sce->iface); > > switch (UE_GET_XFERTYPE(edesc->bmAttributes)) { > > case UE_INTERRUPT: > > if (dir == OUT) { > > @@ -339,8 +336,6 @@ ugenopen(dev_t dev, int flag, int mode, > > clfree(&sce->q); > > return (EIO); > > } > > - /* Clear HC endpoint toggle. */ > > - usbd_clear_endpoint_toggle(sce->pipeh); > > DPRINTFN(5, ("ugenopen: interrupt open done\n")); > > break; > > case UE_BULK: > > @@ -348,8 +343,6 @@ ugenopen(dev_t dev, int flag, int mode, > > edesc->bEndpointAddress, 0, &sce->pipeh); > > if (err) > > return (EIO); > > - /* Clear HC endpoint toggle. */ > > - usbd_clear_endpoint_toggle(sce->pipeh); > > break; > > case UE_ISOCHRONOUS: > > if (dir == OUT) > > @@ -1431,42 +1424,4 @@ ugenkqfilter(dev_t dev, struct knote *kn > > splx(s); > > > > return (0); > > -} > > - > > -void > > -ugen_clear_iface_eps(struct ugen_softc *sc, struct usbd_interface *iface) > > -{ > > - usb_interface_descriptor_t *id; > > - usb_endpoint_descriptor_t *ed; > > - uint8_t xfertype; > > - int i; > > - > > - /* Only clear interface endpoints when none are in use. */ > > - for (i = 0; i < USB_MAX_ENDPOINTS; i++) { > > - if (i == USB_CONTROL_ENDPOINT) > > - continue; > > - if (sc->sc_is_open[i] != 0) > > - return; > > - } > > - DPRINTFN(1,("%s: clear interface eps\n", __func__)); > > - > > - id = usbd_get_interface_descriptor(iface); > > - if (id == NULL) > > - goto bad; > > - > > - for (i = 0; i < id->bNumEndpoints; i++) { > > - ed = usbd_interface2endpoint_descriptor(iface, i); > > - if (ed == NULL) > > - 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)) > > - goto bad; > > - } > > - } > > - return; > > -bad: > > - printf("%s: clear endpoints failed!\n", __func__); > > } > > Index: sys/dev/usb/uhidev.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/uhidev.c,v > > retrieving revision 1.88 > > diff -u -p -u -p -r1.88 uhidev.c > > --- sys/dev/usb/uhidev.c 11 Feb 2021 06:55:10 -0000 1.88 > > +++ sys/dev/usb/uhidev.c 14 Feb 2021 13:58:26 -0000 > > @@ -98,7 +98,6 @@ int uhidev_activate(struct device *, int > > > > void uhidev_get_report_async_cb(struct usbd_xfer *, void *, usbd_status); > > void uhidev_set_report_async_cb(struct usbd_xfer *, void *, usbd_status); > > -void uhidev_clear_iface_eps(struct uhidev_softc *, struct usbd_interface > > *); > > > > struct cfdriver uhidev_cd = { > > NULL, "uhidev", DV_DULL > > @@ -518,9 +517,6 @@ uhidev_open(struct uhidev *scd) > > DPRINTF(("uhidev_open: isize=%d, ep=0x%02x\n", sc->sc_isize, > > sc->sc_iep_addr)); > > > > - /* Clear device endpoint toggle. */ > > - uhidev_clear_iface_eps(sc, sc->sc_iface); > > - > > err = usbd_open_pipe_intr(sc->sc_iface, sc->sc_iep_addr, > > USBD_SHORT_XFER_OK, &sc->sc_ipipe, sc, sc->sc_ibuf, > > sc->sc_isize, uhidev_intr, USBD_DEFAULT_INTERVAL); > > @@ -530,8 +526,6 @@ uhidev_open(struct uhidev *scd) > > error = EIO; > > goto out1; > > } > > - /* Clear HC endpoint toggle. */ > > - usbd_clear_endpoint_toggle(sc->sc_ipipe); > > > > DPRINTF(("uhidev_open: sc->sc_ipipe=%p\n", sc->sc_ipipe)); > > > > @@ -557,8 +551,6 @@ uhidev_open(struct uhidev *scd) > > error = EIO; > > goto out2; > > } > > - /* Clear HC endpoint toggle. */ > > - usbd_clear_endpoint_toggle(sc->sc_opipe); > > > > DPRINTF(("uhidev_open: sc->sc_opipe=%p\n", sc->sc_opipe)); > > > > @@ -966,40 +958,6 @@ uhidev_ioctl(struct uhidev *sc, u_long c > > return -1; > > } > > return 0; > > -} > > - > > -void > > -uhidev_clear_iface_eps(struct uhidev_softc *sc, struct usbd_interface > > *iface) > > -{ > > - usb_interface_descriptor_t *id; > > - usb_endpoint_descriptor_t *ed; > > - uint8_t xfertype; > > - int i; > > - > > - /* Only clear interface endpoints when none are in use. */ > > - if (sc->sc_ipipe || sc->sc_opipe) > > - return; > > - DPRINTFN(1,("%s: clear interface eps\n", __func__)); > > - > > - id = usbd_get_interface_descriptor(iface); > > - if (id == NULL) > > - goto bad; > > - > > - for (i = 0; i < id->bNumEndpoints; i++) { > > - ed = usbd_interface2endpoint_descriptor(iface, i); > > - if (ed == NULL) > > - 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)) > > - goto bad; > > - } > > - } > > - return; > > -bad: > > - printf("%s: clear endpoints failed!\n", __func__); > > } > > > > int > > Index: sys/dev/usb/usbdi_util.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/usbdi_util.c,v > > retrieving revision 1.45 > > diff -u -p -u -p -r1.45 usbdi_util.c > > --- sys/dev/usb/usbdi_util.c 25 Jan 2021 14:05:57 -0000 1.45 > > +++ sys/dev/usb/usbdi_util.c 14 Feb 2021 13:58:26 -0000 > > @@ -192,19 +192,6 @@ usbd_clear_port_feature(struct usbd_devi > > } > > > > usbd_status > > -usbd_clear_endpoint_feature(struct usbd_device *dev, int epaddr, int sel) > > -{ > > - usb_device_request_t req; > > - > > - req.bmRequestType = UT_WRITE_ENDPOINT; > > - req.bRequest = UR_CLEAR_FEATURE; > > - USETW(req.wValue, sel); > > - USETW(req.wIndex, epaddr); > > - USETW(req.wLength, 0); > > - return (usbd_do_request(dev, &req, 0)); > > -} > > - > > -usbd_status > > usbd_set_port_feature(struct usbd_device *dev, int port, int sel) > > { > > usb_device_request_t req; > > Index: sys/dev/usb/usbdi_util.h > > =================================================================== > > RCS file: /cvs/src/sys/dev/usb/usbdi_util.h,v > > retrieving revision 1.30 > > diff -u -p -u -p -r1.30 usbdi_util.h > > --- sys/dev/usb/usbdi_util.h 25 Jan 2021 14:05:57 -0000 1.30 > > +++ sys/dev/usb/usbdi_util.h 14 Feb 2021 13:58:26 -0000 > > @@ -41,7 +41,6 @@ usbd_status usbd_set_hub_feature(struct > > usbd_status usbd_clear_hub_feature(struct usbd_device *, int); > > usbd_status usbd_set_port_feature(struct usbd_device *dev, int, > > int); > > usbd_status usbd_clear_port_feature(struct usbd_device *, int, int); > > -usbd_status usbd_clear_endpoint_feature(struct usbd_device *, int, > > int); > > usbd_status usbd_get_device_status(struct usbd_device *, > > usb_status_t *); > > usbd_status usbd_get_hub_status(struct usbd_device *, > > usb_hub_status_t *); > > usbd_status usbd_get_hub_descriptor(struct usbd_device *, > > Index: www/chromium/files/hid_service_fido.cc > > =================================================================== > > RCS file: /cvs/ports/www/chromium/files/hid_service_fido.cc,v > > retrieving revision 1.4 > > diff -u -p -u -p -r1.4 hid_service_fido.cc > > --- www/chromium/files/hid_service_fido.cc 6 Feb 2021 05:05:04 -0000 > > 1.4 > > +++ www/chromium/files/hid_service_fido.cc 14 Feb 2021 14:00:25 -0000 > > @@ -11,7 +11,10 @@ > > #include <sys/un.h> > > #include <unistd.h> > > > > +// TODO: remove once the missing guard in fido.h is fixed upstream. > > +extern "C" { > > #include <fido.h> > > +} > > > > #include <set> > > #include <string> > > @@ -72,6 +75,55 @@ void FinishOpen(std::unique_ptr<ConnectP > > base::Bind(&CreateConnection, > > base::Passed(¶ms))); > > } > > > > +bool terrible_ping_kludge(int fd, const std::string &path) { > > + u_char data[256]; > > + int i, n; > > + struct pollfd pfd; > > + > > + for (i = 0; i < 4; i++) { > > + memset(data, 0, sizeof(data)); > > + /* broadcast channel ID */ > > + data[1] = 0xff; > > + data[2] = 0xff; > > + data[3] = 0xff; > > + data[4] = 0xff; > > + /* Ping command */ > > + data[5] = 0x81; > > + /* One byte ping only, Vasili */ > > + data[6] = 0; > > + data[7] = 1; > > + HID_LOG(EVENT) << "send ping " << i << " " << path; > > + if (write(fd, data, 64) == -1) { > > + HID_PLOG(ERROR) << "write " << path; > > + return false; > > + } > > + HID_LOG(EVENT) << "wait reply " << path; > > + memset(&pfd, 0, sizeof(pfd)); > > + pfd.fd = fd; > > + pfd.events = POLLIN; > > + if ((n = poll(&pfd, 1, 100)) == -1) { > > + HID_PLOG(EVENT) << "poll " << path; > > + return false; > > + } else if (n == 0) { > > + HID_LOG(EVENT) << "timed out " << path; > > + continue; > > + } > > + if (read(fd, data, 64) == -1) { > > + HID_PLOG(ERROR) << "read " << path; > > + return false; > > + } > > + /* > > + * Ping isn't always supported on the broadcast channel, > > + * so we might get an error, but we don't care - we're > > + * synched now. > > + */ > > + HID_LOG(EVENT) << "got reply " << path; > > + return true; > > + } > > + HID_LOG(ERROR) << "no response " << path; > > + return false; > > +} > > + > > void OpenOnBlockingThread(std::unique_ptr<ConnectParams> params) { > > base::ScopedBlockingCall scoped_blocking_call(FROM_HERE, > > > > base::BlockingType::MAY_BLOCK); > > @@ -85,6 +137,11 @@ void OpenOnBlockingThread(std::unique_pt > > if (!device_file.IsValid()) { > > HID_LOG(EVENT) << "Failed to open '" << device_node << "': " > > << > > base::File::ErrorToString(device_file.error_details()); > > + task_runner->PostTask(FROM_HERE, > > base::BindOnce(std::move(params->callback), nullptr)); > > + return; > > + } > > + if (!terrible_ping_kludge(device_file.GetPlatformFile(), device_node)) { > > + HID_LOG(EVENT) << "Failed to ping " << device_node; > > task_runner->PostTask(FROM_HERE, > > base::BindOnce(std::move(params->callback), nullptr)); > > return; > > } > >