On Sat, 2 Jan 2021 11:11:18 +0000
Natasha Kerensikova <[email protected]> wrote:
> Hello,
>
> on Saturday 02 January 2021 at 09:34, Marcus Glocker wrote:
> > I was not following up the initial conversation on this thread, so I
> > probably missed a lot of what was already discussed.
>
> A lot of the conversation was outside of mailing lists, and I'm not
> sure exactly why. I summed up my understanding, but my knowledge very
> limit I might have misunderstood or misexplained some things.
>
> > I'm also not very familiar with any xhci specific packet loss
> > feature mechanism. But if there was already an proposal from
> > patrick@ to come over this by doing an EP clearing on device open,
> > I've just tested the following diff, and it makes my scanner
> > reliably fail now with xhci :-)
>
> Here are the explanations I got from patrick@, on 2019-11-05 (in case
> things changed since then), I hope I'm not committing a vile act by
> cut-and-pasting a private mail into a public ML:
>
> Interrupt transfers (IN and OUT) use a specific method to find out if
> packets have been lost. This can be done by toggling the PID between
> DATA0 <-> DATA1.
>
> When we open/close uhid(4) we open/close the pipe. On non-xHCI we
> can save the endpoint toggle state and keep it. With xHCI it looks
> like we can only reset the toggle state... to 0.
>
> This is because there are only add/drop and reset endpoint commands.
> "Add" and "drop" reset the toggle state, while with "reset" endpoint
> we have the possibility to keep it. open/close does add/drop, so we
> always lose it, but that's life.
>
> If there is a way to save the toggle state between add/drop, then it
> must have gotten lost somewhere, because it all reads like it does
> not exist.
>
> Are there other ways to "set" the toggle? Yes, there is. We can
> reset it on _both_ ends by clearing an endpoint stall. Yes, even
> though we are not actually stalled, a CLEAR_FEATURE on the endpoint
> always resets the toggle state, and clearing endpoint stall is perfect
> for this.
>
> Note that we have this issue on both the IN and OUT pipe. On the OUT
> pipe we did always send it with DATA0, even though the device expected
> DATA1. On the IN pipe (after drop/add) we expected DATA0, but the
> device sent DATA1.
Thanks for sharing this. It explains the issue quite well. In a
very quick summary it means:
ugen(4) being one of the rare device drivers were we have an open/close
cycle for interrupt and bulk endpoints without attach/detach cycle, the
issue manifests with xhci(4), that it will reset the data toggle state on
the HC, while on the device the data toggle state remains. This finally
leads to the common case where on a freshly initialized device the data
toggles are in sync, and therefore the first run succeeds, while the
second run fails due to the data toggles got out of sync.
So what we should do IMO is to reset the data toggle state on our
device endpoints _before_ we open the pipes. If we do it as already
tested with usbd_clear_endpoint_stall() after the pipe has been opened,
it will lead to the problems we saw.
NetBSD has introduced a function in their USB stack called
usbd_clear_endpoint_feature() with which one can issue the UR_CLEAR_FEATURE
request on endpoints without the need to have the pipe open. Also this
function won't try to call the cleartoggle USB pipe method, which is
anyway not implemented for xhci(4). And it also makes clear that
we're not executing it because of a stall situation. This function we
can perfectly use in our case to reset the data toggles for all the
endpoints on the active interface in one go, _before_ we open the pipes.
On the other hand this approach introduces an issue on ehci(4) HCs.
While we reset the data toggle on the device now, ehci(4) will keep the
last data toggle state since the usbd_clear_endpoint_feature() function
can't call the ehci(4) cleartoggle USB pipe method. That leads to the
situation that we run out of sync again when we use ugen(4) on a
ehci(4) HC. While I'm not a big fan of running code in device drivers
which is HC specific, I have no better idea currently than only execute
the new function on xhci(4) HCs, and leave everything else as is, since
this runs perfectly fine on non-xhci HCs.
Just as another side note; I have also tested the
usbd_clear_endpoint_stall() on pipe close approach with ehci(4), but
it will lead to a panic currently when a queue is still active because
the ehci(4) cleartoggle USB pipe method will check if the queue is
still in an active state, and panic if so.
I've tested the following diff on a xhci(4), and a ehci(4) HC with the
Canon LiDE 400 scanner.
Index: dev/usb/ugen.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/ugen.c,v
retrieving revision 1.109
diff -u -p -u -p -r1.109 ugen.c
--- dev/usb/ugen.c 25 Dec 2020 12:59:52 -0000 1.109
+++ dev/usb/ugen.c 4 Jan 2021 07:08:14 -0000
@@ -46,9 +46,12 @@
#include <sys/vnode.h>
#include <sys/poll.h>
+#include <machine/bus.h>
+
#include <dev/usb/usb.h>
#include <dev/usb/usbdi.h>
#include <dev/usb/usbdi_util.h>
+#include <dev/usb/usbdivar.h>
#ifdef UGEN_DEBUG
#define DPRINTF(x) do { if (ugendebug) printf x; } while (0)
@@ -114,6 +117,7 @@ int ugen_do_close(struct ugen_softc *, i
int ugen_set_config(struct ugen_softc *sc, int configno);
int ugen_set_interface(struct ugen_softc *, int, int);
int ugen_get_alt_index(struct ugen_softc *sc, int ifaceidx);
+int ugen_endpoint_halt(struct ugen_softc *, struct usbd_interface *);
#define UGENUNIT(n) ((minor(n) >> 4) & 0xf)
#define UGENENDPOINT(n) (minor(n) & 0xf)
@@ -302,6 +306,15 @@ 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;
+ /*
+ * XHCI resets the data toggle state, used for interrupt and
+ * bulk transfers, on a pipe open/close (add/drop) cycle.
+ * Therefore we also need to reset the device endpoints data
+ * toggles for the current interface to make sure we stay in
+ * sync.
+ */
+ if (sc->sc_udev->bus->usbrev == USBREV_3_0)
+ ugen_endpoint_halt(sc, sce->iface);
switch (UE_GET_XFERTYPE(edesc->bmAttributes)) {
case UE_INTERRUPT:
if (dir == OUT) {
@@ -940,6 +953,50 @@ ugen_get_alt_index(struct ugen_softc *sc
if (err)
return (-1);
return (usbd_get_interface_altindex(iface));
+}
+
+int
+ugen_endpoint_halt(struct ugen_softc *sc, struct usbd_interface *iface)
+{
+ usb_interface_descriptor_t *id;
+ usb_endpoint_descriptor_t *ed;
+ usbd_status err;
+ int i;
+
+ /* Only halt 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 (0);
+ }
+ DPRINTFN(1,("%s: halt interface endpoints\n", __func__));
+
+ id = usbd_get_interface_descriptor(iface);
+ if (id == NULL)
+ return (-1);
+
+ for (i = 0; i < id->bNumEndpoints; i++) {
+ ed = usbd_interface2endpoint_descriptor(iface, i);
+ if (ed == NULL)
+ return (-1);
+
+ switch (UE_GET_XFERTYPE(ed->bmAttributes)) {
+ case UE_BULK:
+ case UE_INTERRUPT:
+ err = usbd_clear_endpoint_feature(sc->sc_udev,
+ ed->bEndpointAddress, UF_ENDPOINT_HALT);
+ if (err) {
+ printf("%s: halt endpoint failed!\n", __func__);
+ return (-1);
+ }
+ break;
+ default:
+ break;
+ }
+ }
+
+ return (0);
}
int
Index: dev/usb/usbdi_util.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdi_util.c,v
retrieving revision 1.44
diff -u -p -u -p -r1.44 usbdi_util.c
--- dev/usb/usbdi_util.c 6 Oct 2019 17:11:51 -0000 1.44
+++ dev/usb/usbdi_util.c 4 Jan 2021 07:08:14 -0000
@@ -192,6 +192,19 @@ 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: dev/usb/usbdi_util.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdi_util.h,v
retrieving revision 1.29
diff -u -p -u -p -r1.29 usbdi_util.h
--- dev/usb/usbdi_util.h 8 Dec 2014 22:00:11 -0000 1.29
+++ dev/usb/usbdi_util.h 4 Jan 2021 07:08:14 -0000
@@ -41,6 +41,7 @@ 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 *,
