xhci(4): fix for usbd_start_next: error=13

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

xhci(4): fix for usbd_start_next: error=13

Marcus Glocker
When playing around with uvideo(4) devices I'm quite regular hitting
the error message in the subject when closing the device.  The problem
seems to be some false return code ordering in xhci_device_isoc_start():

if (sc->sc_bus.dying || xp->halted)
        return (USBD_IOERROR);

/* Why would you do that anyway? */
if (sc->sc_bus.use_polling)
        return (USBD_INVAL);

/*
 * To allow continuous transfers, above we start all transfers
 * immediately. However, we're still going to get usbd_start_next call
 * this when another xfer completes. So, check if this is already
 * in progress or not
 */
if (xx->ntrb > 0)
        return (USBD_IN_PROGRESS);

When we close the device, xhci_abort_xfer() will set xp->halted.
When usbd_start_next() tries to dequeue an transfer afterwards which is
already in progress, xhci_device_isoc_start() will first check for
xp->halted and return USBD_IOERROR to usbd_start_next() which finally
results in the error message been print.

By re-ordering the USBD_IN_PROGRESS check to be first, the same
way as ehci(4) does, this gets fixed.

Feedback?  OK?


Index: xhci.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/xhci.c,v
retrieving revision 1.118
diff -u -p -u -p -r1.118 xhci.c
--- xhci.c 29 Jul 2020 16:37:12 -0000 1.118
+++ xhci.c 31 Jul 2020 16:45:52 -0000
@@ -3111,13 +3111,6 @@ xhci_device_isoc_start(struct usbd_xfer
 
  KASSERT(!(xfer->rqflags & URQ_REQUEST));
 
- if (sc->sc_bus.dying || xp->halted)
- return (USBD_IOERROR);
-
- /* Why would you do that anyway? */
- if (sc->sc_bus.use_polling)
- return (USBD_INVAL);
-
  /*
  * To allow continuous transfers, above we start all transfers
  * immediately. However, we're still going to get
usbd_start_next call @@ -3126,6 +3119,13 @@
xhci_device_isoc_start(struct usbd_xfer */
  if (xx->ntrb > 0)
  return (USBD_IN_PROGRESS);
+
+ if (sc->sc_bus.dying || xp->halted)
+ return (USBD_IOERROR);
+
+ /* Why would you do that anyway? */
+ if (sc->sc_bus.use_polling)
+ return (USBD_INVAL);
 
  paddr = DMAADDR(&xfer->dmabuf, 0);