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;
> > }
> >
