Re: USB polling + xhci(4) fix
> Date: Thu, 9 Mar 2017 16:04:44 +0100 > From: Martin Pieuchot > > On 09/03/17(Thu) 15:04, Mark Kettenis wrote: > > > Date: Thu, 9 Mar 2017 14:32:29 +0100 > > > From: Martin Pieuchot > > > > > > Diff below move per HC driver polling code to the stack. I know this > > > code contains a use-after-free and this will be addressed in a later > > > diff. For the moment merge 4 copies into 1 such that I don't have to > > > fix all of them. > > > > > > As a side effect, this fix xhci(4) polling mode. That means you can > > > now use ddb(4) with an xhci(4)-attached keyboard. > > > > > > ok? > > > > Looks like a good idea to me. Hwoever... > > > [...] > > > + if (polling) { > > > + int timo; > > > + > > > + for (timo = xfer->timeout; timo >= 0; timo--) { > > > + usb_delay_ms(bus, 1); > > > > The xxx_waitintr() implementations have a sc->sc_bus.dying check here. > > I'd expect that that check would still be necessary... > > That check is needed when coming back from a sleep. In polling mode > usb_delay_ms() calls delay(9) so we know that all the resources are > still there. > > But it might be better to leave that for another diff. Revised diff > below. Thanks for the explanation. Looks good to me. ok kettenis@OB > Index: usbdi.c > === > RCS file: /cvs/src/sys/dev/usb/usbdi.c,v > retrieving revision 1.87 > diff -u -p -r1.87 usbdi.c > --- usbdi.c 6 Mar 2017 12:13:58 - 1.87 > +++ usbdi.c 9 Mar 2017 14:58:44 - > @@ -279,6 +279,8 @@ usbd_status > usbd_transfer(struct usbd_xfer *xfer) > { > struct usbd_pipe *pipe = xfer->pipe; > + struct usbd_bus *bus = pipe->device->bus; > + int polling = bus->use_polling; > usbd_status err; > int flags, s; > > @@ -298,8 +300,6 @@ usbd_transfer(struct usbd_xfer *xfer) > > /* If there is no buffer, allocate one. */ > if ((xfer->rqflags & URQ_DEV_DMABUF) == 0) { > - struct usbd_bus *bus = pipe->device->bus; > - > #ifdef DIAGNOSTIC > if (xfer->rqflags & URQ_AUTO_DMABUF) > printf("usbd_transfer: has old buffer!\n"); > @@ -325,8 +325,6 @@ usbd_transfer(struct usbd_xfer *xfer) > if (err != USBD_IN_PROGRESS && err) { > /* The transfer has not been queued, so free buffer. */ > if (xfer->rqflags & URQ_AUTO_DMABUF) { > - struct usbd_bus *bus = pipe->device->bus; > - > usb_freemem(bus, &xfer->dmabuf); > xfer->rqflags &= ~URQ_AUTO_DMABUF; > } > @@ -338,19 +336,40 @@ usbd_transfer(struct usbd_xfer *xfer) > /* Sync transfer, wait for completion. */ > if (err != USBD_IN_PROGRESS) > return (err); > + > s = splusb(); > - while (!xfer->done) { > - if (pipe->device->bus->use_polling) > - panic("usbd_transfer: not done"); > - flags = PRIBIO | (xfer->flags & USBD_CATCH ? PCATCH : 0); > - > - err = tsleep(xfer, flags, "usbsyn", 0); > - if (err && !xfer->done) { > - usbd_abort_pipe(pipe); > - if (err == EINTR) > - xfer->status = USBD_INTERRUPTED; > - else > - xfer->status = USBD_TIMEOUT; > + if (polling) { > + int timo; > + > + for (timo = xfer->timeout; timo >= 0; timo--) { > + usb_delay_ms(bus, 1); > + if (bus->dying) { > + xfer->status = USBD_IOERROR; > + usb_transfer_complete(xfer); > + break; > + } > + > + usbd_dopoll(pipe->device); > + if (xfer->done) > + break; > + } > + > + if (timo < 0) { > + xfer->status = USBD_TIMEOUT; > + usb_transfer_complete(xfer); > + } > + } else { > + while (!xfer->done) { > + flags = PRIBIO|(xfer->flags & USBD_CATCH ? PCATCH : 0); > + > + err = tsleep(xfer, flags, "usbsyn", 0); > + if (err && !xfer->done) { > + usbd_abort_pipe(pipe); > + if (err == EINTR) > + xfer->status = USBD_INTERRUPTED; > + else > + xfer->status = USBD_TIMEOUT; > + } > } > } > splx(s); > Index: ehci.c > === > RCS file: /cvs/src/sys/dev/usb/ehci.c,v > retrieving revision 1.195 > diff -u -p -r1.195 ehci.c > --- ehci.c8 Nov 2016 10:31:30 - 1.195 > +++ ehci.c9 Mar 2017 13:19:42 - > @@ -117,7
Re: USB polling + xhci(4) fix
On 09/03/17(Thu) 15:04, Mark Kettenis wrote: > > Date: Thu, 9 Mar 2017 14:32:29 +0100 > > From: Martin Pieuchot > > > > Diff below move per HC driver polling code to the stack. I know this > > code contains a use-after-free and this will be addressed in a later > > diff. For the moment merge 4 copies into 1 such that I don't have to > > fix all of them. > > > > As a side effect, this fix xhci(4) polling mode. That means you can > > now use ddb(4) with an xhci(4)-attached keyboard. > > > > ok? > > Looks like a good idea to me. Hwoever... > [...] > > + if (polling) { > > + int timo; > > + > > + for (timo = xfer->timeout; timo >= 0; timo--) { > > + usb_delay_ms(bus, 1); > > The xxx_waitintr() implementations have a sc->sc_bus.dying check here. > I'd expect that that check would still be necessary... That check is needed when coming back from a sleep. In polling mode usb_delay_ms() calls delay(9) so we know that all the resources are still there. But it might be better to leave that for another diff. Revised diff below. Index: usbdi.c === RCS file: /cvs/src/sys/dev/usb/usbdi.c,v retrieving revision 1.87 diff -u -p -r1.87 usbdi.c --- usbdi.c 6 Mar 2017 12:13:58 - 1.87 +++ usbdi.c 9 Mar 2017 14:58:44 - @@ -279,6 +279,8 @@ usbd_status usbd_transfer(struct usbd_xfer *xfer) { struct usbd_pipe *pipe = xfer->pipe; + struct usbd_bus *bus = pipe->device->bus; + int polling = bus->use_polling; usbd_status err; int flags, s; @@ -298,8 +300,6 @@ usbd_transfer(struct usbd_xfer *xfer) /* If there is no buffer, allocate one. */ if ((xfer->rqflags & URQ_DEV_DMABUF) == 0) { - struct usbd_bus *bus = pipe->device->bus; - #ifdef DIAGNOSTIC if (xfer->rqflags & URQ_AUTO_DMABUF) printf("usbd_transfer: has old buffer!\n"); @@ -325,8 +325,6 @@ usbd_transfer(struct usbd_xfer *xfer) if (err != USBD_IN_PROGRESS && err) { /* The transfer has not been queued, so free buffer. */ if (xfer->rqflags & URQ_AUTO_DMABUF) { - struct usbd_bus *bus = pipe->device->bus; - usb_freemem(bus, &xfer->dmabuf); xfer->rqflags &= ~URQ_AUTO_DMABUF; } @@ -338,19 +336,40 @@ usbd_transfer(struct usbd_xfer *xfer) /* Sync transfer, wait for completion. */ if (err != USBD_IN_PROGRESS) return (err); + s = splusb(); - while (!xfer->done) { - if (pipe->device->bus->use_polling) - panic("usbd_transfer: not done"); - flags = PRIBIO | (xfer->flags & USBD_CATCH ? PCATCH : 0); - - err = tsleep(xfer, flags, "usbsyn", 0); - if (err && !xfer->done) { - usbd_abort_pipe(pipe); - if (err == EINTR) - xfer->status = USBD_INTERRUPTED; - else - xfer->status = USBD_TIMEOUT; + if (polling) { + int timo; + + for (timo = xfer->timeout; timo >= 0; timo--) { + usb_delay_ms(bus, 1); + if (bus->dying) { + xfer->status = USBD_IOERROR; + usb_transfer_complete(xfer); + break; + } + + usbd_dopoll(pipe->device); + if (xfer->done) + break; + } + + if (timo < 0) { + xfer->status = USBD_TIMEOUT; + usb_transfer_complete(xfer); + } + } else { + while (!xfer->done) { + flags = PRIBIO|(xfer->flags & USBD_CATCH ? PCATCH : 0); + + err = tsleep(xfer, flags, "usbsyn", 0); + if (err && !xfer->done) { + usbd_abort_pipe(pipe); + if (err == EINTR) + xfer->status = USBD_INTERRUPTED; + else + xfer->status = USBD_TIMEOUT; + } } } splx(s); Index: ehci.c === RCS file: /cvs/src/sys/dev/usb/ehci.c,v retrieving revision 1.195 diff -u -p -r1.195 ehci.c --- ehci.c 8 Nov 2016 10:31:30 - 1.195 +++ ehci.c 9 Mar 2017 13:19:42 - @@ -117,7 +117,6 @@ int ehci_setaddr(struct usbd_device *, void ehci_poll(struct usbd_bus *); void ehci_softintr(void *); intehci_intr1(struct ehci_softc *); -void ehci_waitintr(struct ehci_softc *,
Re: USB polling + xhci(4) fix
> Date: Thu, 9 Mar 2017 14:32:29 +0100 > From: Martin Pieuchot > > Diff below move per HC driver polling code to the stack. I know this > code contains a use-after-free and this will be addressed in a later > diff. For the moment merge 4 copies into 1 such that I don't have to > fix all of them. > > As a side effect, this fix xhci(4) polling mode. That means you can > now use ddb(4) with an xhci(4)-attached keyboard. > > ok? Looks like a good idea to me. Hwoever... > > Index: usbdi.c > === > RCS file: /cvs/src/sys/dev/usb/usbdi.c,v > retrieving revision 1.87 > diff -u -p -r1.87 usbdi.c > --- usbdi.c 6 Mar 2017 12:13:58 - 1.87 > +++ usbdi.c 9 Mar 2017 13:27:06 - > @@ -279,6 +279,8 @@ usbd_status > usbd_transfer(struct usbd_xfer *xfer) > { > struct usbd_pipe *pipe = xfer->pipe; > + struct usbd_bus *bus = pipe->device->bus; > + int polling = bus->use_polling; > usbd_status err; > int flags, s; > > @@ -298,8 +300,6 @@ usbd_transfer(struct usbd_xfer *xfer) > > /* If there is no buffer, allocate one. */ > if ((xfer->rqflags & URQ_DEV_DMABUF) == 0) { > - struct usbd_bus *bus = pipe->device->bus; > - > #ifdef DIAGNOSTIC > if (xfer->rqflags & URQ_AUTO_DMABUF) > printf("usbd_transfer: has old buffer!\n"); > @@ -325,8 +325,6 @@ usbd_transfer(struct usbd_xfer *xfer) > if (err != USBD_IN_PROGRESS && err) { > /* The transfer has not been queued, so free buffer. */ > if (xfer->rqflags & URQ_AUTO_DMABUF) { > - struct usbd_bus *bus = pipe->device->bus; > - > usb_freemem(bus, &xfer->dmabuf); > xfer->rqflags &= ~URQ_AUTO_DMABUF; > } > @@ -338,19 +336,34 @@ usbd_transfer(struct usbd_xfer *xfer) > /* Sync transfer, wait for completion. */ > if (err != USBD_IN_PROGRESS) > return (err); > + > s = splusb(); > - while (!xfer->done) { > - if (pipe->device->bus->use_polling) > - panic("usbd_transfer: not done"); > - flags = PRIBIO | (xfer->flags & USBD_CATCH ? PCATCH : 0); > - > - err = tsleep(xfer, flags, "usbsyn", 0); > - if (err && !xfer->done) { > - usbd_abort_pipe(pipe); > - if (err == EINTR) > - xfer->status = USBD_INTERRUPTED; > - else > - xfer->status = USBD_TIMEOUT; > + if (polling) { > + int timo; > + > + for (timo = xfer->timeout; timo >= 0; timo--) { > + usb_delay_ms(bus, 1); The xxx_waitintr() implementations have a sc->sc_bus.dying check here. I'd expect that that check would still be necessary... > + usbd_dopoll(pipe->device); > + if (xfer->done) > + break; > + } > + > + if (timo < 0) { > + xfer->status = USBD_TIMEOUT; > + usb_transfer_complete(xfer); > + } > + } else { > + while (!xfer->done) { > + flags = PRIBIO|(xfer->flags & USBD_CATCH ? PCATCH : 0); > + > + err = tsleep(xfer, flags, "usbsyn", 0); > + if (err && !xfer->done) { > + usbd_abort_pipe(pipe); > + if (err == EINTR) > + xfer->status = USBD_INTERRUPTED; > + else > + xfer->status = USBD_TIMEOUT; > + } > } > } > splx(s); > Index: ehci.c > === > RCS file: /cvs/src/sys/dev/usb/ehci.c,v > retrieving revision 1.195 > diff -u -p -r1.195 ehci.c > --- ehci.c8 Nov 2016 10:31:30 - 1.195 > +++ ehci.c9 Mar 2017 13:19:42 - > @@ -117,7 +117,6 @@ int ehci_setaddr(struct usbd_device *, > void ehci_poll(struct usbd_bus *); > void ehci_softintr(void *); > int ehci_intr1(struct ehci_softc *); > -void ehci_waitintr(struct ehci_softc *, struct usbd_xfer *); > void ehci_check_intr(struct ehci_softc *, struct usbd_xfer *); > void ehci_check_qh_intr(struct ehci_softc *, struct usbd_xfer *); > void ehci_check_itd_intr(struct ehci_softc *, struct usbd_xfer *); > @@ -918,37 +917,6 @@ ehci_idone(struct usbd_xfer *xfer) > DPRINTFN(/*12*/2, ("ehci_idone: ex=%p done\n", ex)); > } > > -/* > - * Wait here until controller claims to have an interrupt. > - * Then call ehci_intr and return. Use timeout to avoid waiting > - * too long. > - */ > -void > -ehci_waitintr(struct ehci_softc *sc, struct usbd_xfer *xfer) > -{ > - int timo; > - u_int32_