On Mon, Jan 04, 2021 at 09:13:12AM +0100, Marcus Glocker wrote:
> 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.
Improved diff without the need for HC specific checks.
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 6 Jan 2021 07:19:32 -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_clear_endpoint_toggle(struct ugen_softc *, struct usbd_interface *);
#define UGENUNIT(n) ((minor(n) >> 4) & 0xf)
#define UGENENDPOINT(n) (minor(n) & 0xf)
@@ -302,6 +306,8 @@ 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_endpoint_toggle(sc, sce->iface);
switch (UE_GET_XFERTYPE(edesc->bmAttributes)) {
case UE_INTERRUPT:
if (dir == OUT) {
@@ -329,6 +335,8 @@ 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:
@@ -336,6 +344,8 @@ 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)
@@ -940,6 +950,50 @@ ugen_get_alt_index(struct ugen_softc *sc
if (err)
return (-1);
return (usbd_get_interface_altindex(iface));
+}
+
+int
+ugen_clear_endpoint_toggle(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 6 Jan 2021 07:19:32 -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 6 Jan 2021 07:19:32 -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 *,
