> Date: Thu, 9 Mar 2017 16:04:44 +0100
> From: Martin Pieuchot <m...@openbsd.org>
>
> On 09/03/17(Thu) 15:04, Mark Kettenis wrote:
> > > Date: Thu, 9 Mar 2017 14:32:29 +0100
> > > From: Martin Pieuchot <m...@openbsd.org>
> > >
> > > 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 -0000 1.87
> +++ usbdi.c 9 Mar 2017 14:58:44 -0000
> @@ -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 -0000 1.195
> +++ ehci.c 9 Mar 2017 13:19:42 -0000
> @@ -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_t intrs;
> -
> - xfer->status = USBD_IN_PROGRESS;
> - for (timo = xfer->timeout; timo >= 0; timo--) {
> - usb_delay_ms(&sc->sc_bus, 1);
> - if (sc->sc_bus.dying)
> - break;
> - intrs = EHCI_STS_INTRS(EOREAD4(sc, EHCI_USBSTS)) &
> - sc->sc_eintrs;
> - if (intrs) {
> - ehci_intr1(sc);
> - if (xfer->status != USBD_IN_PROGRESS)
> - return;
> - }
> - }
> -
> - /* Timeout */
> - xfer->status = USBD_TIMEOUT;
> - usb_transfer_complete(xfer);
> - /* XXX should free TD */
> -}
> -
> void
> ehci_poll(struct usbd_bus *bus)
> {
> @@ -2973,9 +2941,6 @@ ehci_device_ctrl_start(struct usbd_xfer
> xfer->status = USBD_IN_PROGRESS;
> splx(s);
>
> - if (sc->sc_bus.use_polling)
> - ehci_waitintr(sc, xfer);
> -
> return (USBD_IN_PROGRESS);
>
> bad3:
> @@ -3073,9 +3038,6 @@ ehci_device_bulk_start(struct usbd_xfer
> xfer->status = USBD_IN_PROGRESS;
> splx(s);
>
> - if (sc->sc_bus.use_polling)
> - ehci_waitintr(sc, xfer);
> -
> return (USBD_IN_PROGRESS);
> }
>
> @@ -3188,9 +3150,6 @@ ehci_device_intr_start(struct usbd_xfer
> TAILQ_INSERT_TAIL(&sc->sc_intrhead, ex, inext);
> xfer->status = USBD_IN_PROGRESS;
> splx(s);
> -
> - if (sc->sc_bus.use_polling)
> - ehci_waitintr(sc, xfer);
>
> return (USBD_IN_PROGRESS);
> }
> Index: ohci.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/ohci.c,v
> retrieving revision 1.147
> diff -u -p -r1.147 ohci.c
> --- ohci.c 15 Sep 2016 02:00:17 -0000 1.147
> +++ ohci.c 9 Mar 2017 13:22:37 -0000
> @@ -90,7 +90,6 @@ usbd_status ohci_open(struct usbd_pipe *
> int ohci_setaddr(struct usbd_device *, int);
> void ohci_poll(struct usbd_bus *);
> void ohci_softintr(void *);
> -void ohci_waitintr(struct ohci_softc *, struct usbd_xfer *);
> void ohci_add_done(struct ohci_softc *, ohci_physaddr_t);
> void ohci_rhsc(struct ohci_softc *, struct usbd_xfer *);
>
> @@ -1506,42 +1505,6 @@ ohci_root_ctrl_done(struct usbd_xfer *xf
> {
> }
>
> -/*
> - * Wait here until controller claims to have an interrupt.
> - * Then call ohci_intr and return. Use timeout to avoid waiting
> - * too long.
> - */
> -void
> -ohci_waitintr(struct ohci_softc *sc, struct usbd_xfer *xfer)
> -{
> - int timo;
> - u_int32_t intrs;
> -
> - xfer->status = USBD_IN_PROGRESS;
> - for (timo = xfer->timeout; timo >= 0; timo--) {
> - usb_delay_ms(&sc->sc_bus, 1);
> - if (sc->sc_bus.dying)
> - break;
> - intrs = OREAD4(sc, OHCI_INTERRUPT_STATUS) & sc->sc_eintrs;
> - DPRINTFN(15,("ohci_waitintr: 0x%04x\n", intrs));
> -#ifdef OHCI_DEBUG
> - if (ohcidebug > 15)
> - ohci_dumpregs(sc);
> -#endif
> - if (intrs) {
> - ohci_intr1(sc);
> - if (xfer->status != USBD_IN_PROGRESS)
> - return;
> - }
> - }
> -
> - /* Timeout */
> - DPRINTF(("ohci_waitintr: timeout\n"));
> - xfer->status = USBD_TIMEOUT;
> - usb_transfer_complete(xfer);
> - /* XXX should free TD */
> -}
> -
> void
> ohci_poll(struct usbd_bus *bus)
> {
> @@ -2686,9 +2649,6 @@ ohci_device_ctrl_start(struct usbd_xfer
> if (err)
> return (err);
>
> - if (sc->sc_bus.use_polling)
> - ohci_waitintr(sc, xfer);
> -
> return (USBD_IN_PROGRESS);
> }
>
> @@ -2826,9 +2786,6 @@ ohci_device_bulk_start(struct usbd_xfer
>
> splx(s);
>
> - if (sc->sc_bus.use_polling)
> - ohci_waitintr(sc, xfer);
> -
> return (USBD_IN_PROGRESS);
> }
>
> @@ -2944,9 +2901,6 @@ ohci_device_intr_start(struct usbd_xfer
> #endif
> splx(s);
>
> - if (sc->sc_bus.use_polling)
> - ohci_waitintr(sc, xfer);
> -
> return (USBD_IN_PROGRESS);
> }
>
> @@ -3219,13 +3173,6 @@ ohci_device_isoc_start(struct usbd_xfer
> if (xfer->status != USBD_IN_PROGRESS)
> printf("ohci_device_isoc_start: not in progress %p\n", xfer);
> #endif
> -
> - /* XXX anything to do? */
> -
> - if (sc->sc_bus.use_polling) {
> - DPRINTF(("Starting ohci isoc xfer with polling. Bad idea?\n"));
> - ohci_waitintr(sc, xfer);
> - }
>
> return (USBD_IN_PROGRESS);
> }
> Index: uhci.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uhci.c,v
> retrieving revision 1.140
> diff -u -p -r1.140 uhci.c
> --- uhci.c 2 Feb 2017 22:31:05 -0000 1.140
> +++ uhci.c 9 Mar 2017 13:22:56 -0000
> @@ -119,7 +119,6 @@ usbd_status uhci_alloc_std_chain(struct
> struct usbd_xfer *, struct uhci_soft_td **,
> struct uhci_soft_td **);
> void uhci_poll_hub(void *);
> -void uhci_waitintr(struct uhci_softc *, struct usbd_xfer *);
> void uhci_check_intr(struct uhci_softc *, struct usbd_xfer *);
> void uhci_idone(struct usbd_xfer *);
>
> @@ -1337,38 +1336,6 @@ uhci_timeout_task(void *addr)
> splx(s);
> }
>
> -/*
> - * Wait here until controller claims to have an interrupt.
> - * Then call uhci_intr and return. Use timeout to avoid waiting
> - * too long.
> - * Only used during boot when interrupts are not enabled yet.
> - */
> -void
> -uhci_waitintr(struct uhci_softc *sc, struct usbd_xfer *xfer)
> -{
> - int timo;
> - u_int32_t intrs;
> -
> - xfer->status = USBD_IN_PROGRESS;
> - for (timo = xfer->timeout; timo >= 0; timo--) {
> - usb_delay_ms(&sc->sc_bus, 1);
> - if (sc->sc_bus.dying)
> - break;
> - intrs = UREAD2(sc, UHCI_STS) & UHCI_STS_ALLINTRS;
> - DPRINTFN(15,("uhci_waitintr: 0x%04x\n", intrs));
> - if (intrs) {
> - uhci_intr1(sc);
> - if (xfer->status != USBD_IN_PROGRESS)
> - return;
> - }
> - }
> -
> - /* Timeout */
> - DPRINTF(("uhci_waitintr: timeout\n"));
> - xfer->status = USBD_TIMEOUT;
> - usb_transfer_complete(xfer);
> -}
> -
> void
> uhci_poll(struct usbd_bus *bus)
> {
> @@ -1707,9 +1674,6 @@ uhci_device_bulk_start(struct usbd_xfer
> }
> #endif
>
> - if (sc->sc_bus.use_polling)
> - uhci_waitintr(sc, xfer);
> -
> return (USBD_IN_PROGRESS);
> }
>
> @@ -1841,9 +1805,6 @@ uhci_device_ctrl_start(struct usbd_xfer
> if (err)
> return (err);
>
> - if (sc->sc_bus.use_polling)
> - uhci_waitintr(sc, xfer);
> -
> return (USBD_IN_PROGRESS);
> }
>
> @@ -1932,9 +1893,6 @@ uhci_device_intr_start(struct usbd_xfer
> }
> #endif
>
> - if (sc->sc_bus.use_polling)
> - uhci_waitintr(sc, xfer);
> -
> return (USBD_IN_PROGRESS);
> }
>
> @@ -2253,11 +2211,6 @@ uhci_device_isoc_start(struct usbd_xfer
> uhci_add_intr_list(sc, ux);
>
> splx(s);
> -
> - if (sc->sc_bus.use_polling) {
> - DPRINTF(("Starting uhci isoc xfer with polling. Bad idea?\n"));
> - uhci_waitintr(sc, xfer);
> - }
>
> return (USBD_IN_PROGRESS);
> }
> Index: xhci.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/xhci.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 xhci.c
> --- xhci.c 8 Nov 2016 10:31:30 -0000 1.70
> +++ xhci.c 9 Mar 2017 13:20:11 -0000
> @@ -75,7 +75,6 @@ struct xhci_pipe {
>
> int xhci_reset(struct xhci_softc *);
> int xhci_intr1(struct xhci_softc *);
> -void xhci_waitintr(struct xhci_softc *, struct usbd_xfer *);
> void xhci_event_dequeue(struct xhci_softc *);
> void xhci_event_xfer(struct xhci_softc *, uint64_t, uint32_t, uint32_t);
> void xhci_event_command(struct xhci_softc *, uint64_t);
> @@ -634,26 +633,6 @@ xhci_poll(struct usbd_bus *bus)
> }
>
> void
> -xhci_waitintr(struct xhci_softc *sc, struct usbd_xfer *xfer)
> -{
> - int timo;
> -
> - for (timo = xfer->timeout; timo >= 0; timo--) {
> - usb_delay_ms(&sc->sc_bus, 1);
> - if (sc->sc_bus.dying)
> - break;
> -
> - if (xfer->status != USBD_IN_PROGRESS)
> - return;
> -
> - xhci_intr1(sc);
> - }
> -
> - xfer->status = USBD_TIMEOUT;
> - usb_transfer_complete(xfer);
> -}
> -
> -void
> xhci_softintr(void *v)
> {
> struct xhci_softc *sc = v;
> @@ -2525,10 +2504,7 @@ xhci_device_ctrl_start(struct usbd_xfer
> XDWRITE4(sc, XHCI_DOORBELL(xp->slot), xp->dci);
>
> xfer->status = USBD_IN_PROGRESS;
> -
> - if (sc->sc_bus.use_polling)
> - xhci_waitintr(sc, xfer);
> - else if (xfer->timeout) {
> + if (xfer->timeout && !sc->sc_bus.use_polling) {
> timeout_del(&xfer->timeout_handle);
> timeout_set(&xfer->timeout_handle, xhci_timeout, xfer);
> timeout_add_msec(&xfer->timeout_handle, xfer->timeout);
> @@ -2645,10 +2621,7 @@ xhci_device_generic_start(struct usbd_xf
> XDWRITE4(sc, XHCI_DOORBELL(xp->slot), xp->dci);
>
> xfer->status = USBD_IN_PROGRESS;
> -
> - if (sc->sc_bus.use_polling)
> - xhci_waitintr(sc, xfer);
> - else if (xfer->timeout) {
> + if (xfer->timeout && !sc->sc_bus.use_polling) {
> timeout_del(&xfer->timeout_handle);
> timeout_set(&xfer->timeout_handle, xhci_timeout, xfer);
> timeout_add_msec(&xfer->timeout_handle, xfer->timeout);
> Index: dwc2/dwc2.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/dwc2/dwc2.c,v
> retrieving revision 1.41
> diff -u -p -r1.41 dwc2.c
> --- dwc2/dwc2.c 16 Feb 2017 14:09:00 -0000 1.41
> +++ dwc2/dwc2.c 9 Mar 2017 13:26:13 -0000
> @@ -94,7 +94,6 @@ STATIC usbd_status dwc2_open(struct usbd
> STATIC int dwc2_setaddr(struct usbd_device *, int);
> STATIC void dwc2_poll(struct usbd_bus *);
> STATIC void dwc2_softintr(void *);
> -STATIC void dwc2_waitintr(struct dwc2_softc *, struct usbd_xfer *);
>
> #if 0
> STATIC usbd_status dwc2_allocm(struct usbd_bus *, struct usb_dma *,
> uint32_t);
> @@ -406,38 +405,6 @@ dwc2_softintr(void *v)
> }
>
> STATIC void
> -dwc2_waitintr(struct dwc2_softc *sc, struct usbd_xfer *xfer)
> -{
> - struct dwc2_hsotg *hsotg = sc->sc_hsotg;
> - uint32_t intrs;
> - int timo;
> -
> - xfer->status = USBD_IN_PROGRESS;
> - for (timo = xfer->timeout; timo >= 0; timo--) {
> - usb_delay_ms(&sc->sc_bus, 1);
> - if (sc->sc_dying)
> - break;
> - intrs = dwc2_read_core_intr(hsotg);
> -
> - DPRINTFN(15, "0x%08x\n", intrs);
> -
> - if (intrs) {
> - mtx_enter(&hsotg->lock);
> - dwc2_interrupt(sc);
> - mtx_leave(&hsotg->lock);
> - if (xfer->status != USBD_IN_PROGRESS)
> - return;
> - }
> - }
> -
> - /* Timeout */
> - DPRINTF("timeout\n");
> -
> - xfer->status = USBD_TIMEOUT;
> - usb_transfer_complete(xfer);
> -}
> -
> -STATIC void
> dwc2_timeout(void *addr)
> {
> struct usbd_xfer *xfer = addr;
> @@ -987,7 +954,6 @@ dwc2_device_ctrl_transfer(struct usbd_xf
> STATIC usbd_status
> dwc2_device_ctrl_start(struct usbd_xfer *xfer)
> {
> - struct dwc2_softc *sc = DWC2_XFER2SC(xfer);
> usbd_status err;
>
> DPRINTF("\n");
> @@ -995,9 +961,6 @@ dwc2_device_ctrl_start(struct usbd_xfer
> xfer->status = USBD_IN_PROGRESS;
> err = dwc2_device_start(xfer);
>
> - if (sc->sc_bus.use_polling)
> - dwc2_waitintr(sc, xfer);
> -
> return err;
> }
>
> @@ -1044,7 +1007,6 @@ dwc2_device_bulk_transfer(struct usbd_xf
> STATIC usbd_status
> dwc2_device_bulk_start(struct usbd_xfer *xfer)
> {
> - struct dwc2_softc *sc = DWC2_XFER2SC(xfer);
> usbd_status err;
>
> DPRINTF("xfer=%p\n", xfer);
> @@ -1052,9 +1014,6 @@ dwc2_device_bulk_start(struct usbd_xfer
> xfer->status = USBD_IN_PROGRESS;
> err = dwc2_device_start(xfer);
>
> - if (sc->sc_bus.use_polling)
> - dwc2_waitintr(sc, xfer);
> -
> return err;
> }
>
> @@ -1103,15 +1062,10 @@ dwc2_device_intr_transfer(struct usbd_xf
> STATIC usbd_status
> dwc2_device_intr_start(struct usbd_xfer *xfer)
> {
> - struct dwc2_pipe *dpipe = DWC2_XFER2DPIPE(xfer)
> - struct dwc2_softc *sc = DWC2_DPIPE2SC(dpipe);
> usbd_status err;
>
> xfer->status = USBD_IN_PROGRESS;
> err = dwc2_device_start(xfer);
> -
> - if (sc->sc_bus.use_polling)
> - dwc2_waitintr(sc, xfer);
>
> return err;
> }
>
>
