Re: Multiple outstanding transfer vs xhci / ehci (Re: CVS commit: src/sys/dev/usb)

[Rin Okuyama <rokuyama.rk%gmail.com@localhost>]

On 2019/02/15 21:57, Jonathan A. Kollasch wrote:
> On Wed, Feb 13, 2019 at 06:35:14PM +0900, Rin Okuyama wrote:
> > Hi,
> >
> > On 2019/02/13 3:54, Nick Hudson wrote:
> > > On 12/02/2019 16:02, Rin Okuyama wrote:
> > > > Hi,
> > > >
> > > > The system freezes indefinitely with xhci(4) or ehci(4), when NIC with
> > > > multiple outstanding transfers [axen(4), mue(4), and ure(4)] is stopped
> > > > by "ifconfig down" or detached.
> > > >
> > > > As discussed in the previous message, this is due to infinite loop in
> > > > usbd_ar_pipe(); xfers with USBD_NOT_STARTED remain in a queue forever
> > > > because upm_abort [= xhci_device_bulk_abort() etc.] cannot remove them.
> > >
> > >
> > > Why not the attached patch instead?
> > >
> > > Nick
> >
> > Thank you so much for your prompt reply!
> >
> > It works if s/ux_state/ux_status/ here:
> >
> > +		if (xfer->ux_state == USBD_NOT_STARTED) {
> >
> > Can I commit the revised patch?
>
> The revised patch results in
> https://nxr.netbsd.org/xref/src/sys/dev/usb/ehci.c?r=1.265#1566
> firing upon `drvctl detach -d bwfm0` on Pinebook.
>
> 	Jonathan Kollasch

IMO, this is because NOT_STARTED queues are removed in a way that is
unexpected to ehci. For my old amd64 box, the attached patch fixes
assertion failures when devices are detached from ehci. I would like
to commit it together with the previous patch.

Thanks,
rin
----
Index: sys/dev/usb/ehci.c
===================================================================
RCS file: /home/netbsd/src/sys/dev/usb/ehci.c,v
retrieving revision 1.265
diff -p -u -r1.265 ehci.c
--- sys/dev/usb/ehci.c	18 Sep 2018 02:00:06 -0000	1.265
+++ sys/dev/usb/ehci.c	13 Feb 2019 19:13:34 -0000
@@ -1562,9 +1562,10 @@ ehci_freex(struct usbd_bus *bus, struct
 	struct ehci_softc *sc = EHCI_BUS2SC(bus);
 	struct ehci_xfer *ex __diagused = EHCI_XFER2EXFER(xfer);
 
-	KASSERTMSG(xfer->ux_state == XFER_BUSY, "xfer %p state %d\n", xfer,
-	    xfer->ux_state);
-	KASSERT(ex->ex_isdone);
+	KASSERTMSG(xfer->ux_status == USBD_NOT_STARTED ||
+	    xfer->ux_state == XFER_BUSY,
+	    "xfer %p state %d\n", xfer, xfer->ux_state);
+	KASSERT(xfer->ux_status == USBD_NOT_STARTED || ex->ex_isdone);
 
 #ifdef DIAGNOSTIC
 	xfer->ux_state = XFER_FREE;