Re: USB polling + xhci(4) fix

[Mark Kettenis]

> 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, >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 

Re: USB polling + xhci(4) fix

[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.

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, >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 

Re: USB polling + xhci(4) fix

[Mark Kettenis]

> 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, >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;
> -