On Fri, Oct 25, 2019 at 03:59:25PM +0200, Patrick Wildt wrote:
> On Fri, Oct 25, 2019 at 03:10:48PM +1100, Damien Miller wrote:
> > Hi,
> >
> > Some HID devices do not list any report IDs for communication, e.g. my
> > Yubikey5:
> >
> > > uhidev0: iclass 3/0
> > > uhid0 at uhidev0: input=64, output=64, feature=0
> > > ugen0 at uhub0 port 2 configuration 1 "Yubico YubiKey FIDO+CCID" rev
> > > 2.00/5.12 addr 2
> >
> > on Linux, these can be talked to by sending to report ID 0. This AFAIK
> > causes the write(2)'d data to be sent to the interrupt pipe.
> >
> > Our uhid(4) devices already consider write(2) operations to be
> > equivalent to USB_SET_REPORT to the UHID_OUTPUT_REPORT, so we can't
> > use that. So this patch allows USB_SET_REPORT with ucr_report=0
> > to do something similar to what Linux does.
> >
> > Caveat: this is literally my first attempt at writing USB code, and
> > I ended up futzing with the kernel.
>
> So from what I understood the Yubikey expects the transfer to happen
> on the Interrupt OUT pipe instead of doing a control transfer. Read-
> ing some code and documentation, it looks like that we should by de-
> fault send our reports on the interrupt pipe, and only if it does not
> exist fall back to control transfers. Linux seems to do exactly that.
>
> I tried to come up with the following diff, which appeard to make a
> test program work for me. Though I'm not sure I got all the specifics
> right.
>
> Can you give this a go with your test progam?
>
> Patrick
Oops, obvious error. Though I still cannot write/read multiple times
in a row, so there is probably still something wrong (unless my test
is as well).
diff --git a/sys/dev/usb/uhidev.c b/sys/dev/usb/uhidev.c
index 7c02c82f7d5..73de7aeaf56 100644
--- a/sys/dev/usb/uhidev.c
+++ b/sys/dev/usb/uhidev.c
@@ -97,6 +97,7 @@ int uhidev_detach(struct device *, int);
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);
struct cfdriver uhidev_cd = {
NULL, "uhidev", DV_DULL
@@ -670,14 +671,23 @@ uhidev_set_report(struct uhidev_softc *sc, int type, int
id, void *data,
memcpy(buf + 1, data, len - 1);
}
- req.bmRequestType = UT_WRITE_CLASS_INTERFACE;
- req.bRequest = UR_SET_REPORT;
- USETW2(req.wValue, type, id);
- USETW(req.wIndex, sc->sc_ifaceno);
- USETW(req.wLength, len);
+ if (sc->sc_opipe != NULL) {
+ usbd_setup_xfer(sc->sc_owxfer, sc->sc_opipe, 0, buf, len,
+ USBD_SYNCHRONOUS | USBD_CATCH, 0, NULL);
+ if (usbd_transfer(sc->sc_owxfer)) {
+ usbd_clear_endpoint_stall(sc->sc_opipe);
+ actlen = -1;
+ }
+ } else {
+ req.bmRequestType = UT_WRITE_CLASS_INTERFACE;
+ req.bRequest = UR_SET_REPORT;
+ USETW2(req.wValue, type, id);
+ USETW(req.wIndex, sc->sc_ifaceno);
+ USETW(req.wLength, len);
- if (usbd_do_request(sc->sc_udev, &req, buf))
- actlen = -1;
+ if (usbd_do_request(sc->sc_udev, &req, buf))
+ actlen = -1;
+ }
if (id > 0)
free(buf, M_TEMP, len);
@@ -685,6 +695,16 @@ uhidev_set_report(struct uhidev_softc *sc, int type, int
id, void *data,
return (actlen);
}
+void
+uhidev_set_report_async_cb(struct usbd_xfer *xfer, void *priv, usbd_status err)
+{
+ struct uhidev_softc *sc = priv;
+
+ if (err == USBD_STALLED)
+ usbd_clear_endpoint_stall_async(sc->sc_opipe);
+ usbd_free_xfer(xfer);
+}
+
int
uhidev_set_report_async(struct uhidev_softc *sc, int type, int id, void *data,
int len)
@@ -715,14 +735,23 @@ uhidev_set_report_async(struct uhidev_softc *sc, int
type, int id, void *data,
memcpy(buf, data, len);
}
- req.bmRequestType = UT_WRITE_CLASS_INTERFACE;
- req.bRequest = UR_SET_REPORT;
- USETW2(req.wValue, type, id);
- USETW(req.wIndex, sc->sc_ifaceno);
- USETW(req.wLength, len);
-
- if (usbd_request_async(xfer, &req, NULL, NULL))
- actlen = -1;
+ if (sc->sc_opipe != NULL) {
+ usbd_setup_xfer(xfer, sc->sc_opipe, sc, buf, len,
+ USBD_NO_COPY, USBD_DEFAULT_TIMEOUT,
+ uhidev_set_report_async_cb);
+ if (usbd_transfer(xfer)) {
+ usbd_clear_endpoint_stall_async(sc->sc_opipe);
+ actlen = -1;
+ }
+ } else {
+ req.bmRequestType = UT_WRITE_CLASS_INTERFACE;
+ req.bRequest = UR_SET_REPORT;
+ USETW2(req.wValue, type, id);
+ USETW(req.wIndex, sc->sc_ifaceno);
+ USETW(req.wLength, len);
+ if (usbd_request_async(xfer, &req, NULL, NULL))
+ actlen = -1;
+ }
return (actlen);
}
