Re: Patch: xhci controller driver improvements

[Takahiro HAYASHI <t.hash425%gmail.com@localhost>]

Hi,

On 08/10/14 17:06, Nick Hudson wrote:
> Some comments...
>
> > --- src/sys/dev/usb/xhci.c.orig    2014-08-05 22:24:27.000000000 +0900
> > +++ src/sys/dev/usb/xhci.c    2014-08-07 17:35:16.000000000 +0900
> > @@ -55,6 +55,8 @@ __KERNEL_RCSID(0, "$NetBSD: xhci.c,v 1.2
> >   #include <dev/usb/xhcivar.h>
> >   #include <dev/usb/usbroothub_subr.h>
> > +#include <uvm/uvm.h> /* for vtophys on arm */
> > +
>
> Shouldn't be needed at all - the hexdump use is questionable.

The code of hexdump is wrapperd with #if 0, but I added
this just for someone who wants enable this and dump
TRBs on arm e.g. mirabox.

> > @@ -1127,6 +1165,81 @@ xhci_open(usbd_pipe_handle pipe)
> >   }
> >   static void
> > +xhci_abort_xfer(usbd_xfer_handle xfer, usbd_status status)
>
> This function needs usbmp-ifiction, i.e. updating for -current by removing spl, 
> tsleep, wakeup, etc.

Hmm, I need to learn about them.
I guess this patch is based on netbsd-5 so it uses spl family.

> > @@ -2889,6 +3028,7 @@ xhci_timeout(void *addr)
> >       struct xhci_softc * const sc = xfer->pipe->device->bus->hci_private;
> >       if (sc->sc_dying) {
> > +        xhci_abort_xfer(xfer, USBD_TIMEOUT);
> >           return;
> >       }
>
> This looks very strange.

Does it need sc_lock and unlock?
or it's strange that xhci_abort_xfer is called
from callout(softclock) interrupt context?

> The driver makes far too much use of device_printf and all USB should move to 
> KERNHIST.

I didn't know about KERNHIST, thanks for notifying.
I've replaced device_printf with DPRINTF or DPRINTFN in
my local tree.


--
t-hash