Date: Tue, 6 May 2003 10:36:11 -0700 (PDT) From: "Bruce R. Montague" <brucem@cruzio.com> To: freebsd-hackers@freebsd.org Cc: n_hibma@freebsd.org Subject: Patch to fix ohci usb isoc close/int race Message-ID: <200305061736.h46HaBke000587@cruzio.com>
next in thread | raw e-mail | index | archive | help
Hi, I found a race condition between the /dev/usb
ohci.c isochronous case interrupt handler and its
close routine (really an abort). Both an incremental
patch to my previous ohci isoc patch and a complete
patch with respect to the FreeBSD 5.0-current sources
of 10-april are included below.
I have limited testing strategies other than letting
the web-camera run in a loop with input into either
a file or /dev/null, and generating random aborts
of the image capture program. With the latest patch,
I have run the camera constantly into /dev/null for
up to around 16 hours. No other error messages or
unusual activity observed.
One other individual has used this code without
problems, but it would probably be of interest to
test this on other ohci isochronous usb devices. I'm
not sure I even know what any such devices would be.
If anyone is interested in trying such a test, I'd
certainly be interested in the result. The ohci usb
isochronous code currently only handles "reads", the
driver as coded has no "output" support (not sure
what that would be... audio?).
This has still only been tested under FreeBSD 5.0
-Current, I will try to do a test under some version
of 4-stable within the next few days, it might not
be under 4.8...
Incremental patch to fix the interrupt/close race:
--- ../ohci_5_4may/ohci.c Sun May 4 12:33:18 2003
+++ ohci.c Tue May 6 08:57:29 2003
@@ -255,6 +255,7 @@
struct ohci_pipe {
struct usbd_pipe pipe;
ohci_soft_ed_t *sed;
+ u_int32_t aborting;
union {
ohci_soft_td_t *td;
ohci_soft_itd_t *itd;
@@ -1472,11 +1473,14 @@
printf("ohci_softintr: sitd=%p is done\n", sitd);
sitd->isdone = 1;
#endif
+ struct ohci_pipe *opipe =
+ (struct ohci_pipe *)xfer->pipe;
+ if (opipe->aborting )
+ continue;
+
cc = OHCI_ITD_GET_CC(le32toh(sitd->itd.itd_flags));
if (cc == OHCI_CC_NO_ERROR) {
/* XXX compute length for input */
- struct ohci_pipe *opipe =
- (struct ohci_pipe *)xfer->pipe;
if (sitd->flags & OHCI_CALL_DONE) {
opipe->u.iso.inuse -= xfer->nframes;
/* XXX update frlengths with actual length */
@@ -2105,6 +2109,7 @@
if (sitd == NULL)
goto bad1;
opipe->tail.itd = sitd;
+ opipe->aborting = 0;
tdphys = sitd->physaddr;
fmt = OHCI_ED_FORMAT_ISO;
if (UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_IN)
@@ -3420,6 +3425,7 @@
int s;
s = splusb();
+ opipe->aborting = 1;
DPRINTFN(1,("ohci_device_isoc_abort: xfer=%p\n", xfer));
Complete patch with respect to the 10-April-2003
FreeBSD-current. This also contains the fix to
the int/close race:
--- ../ohci_5_10apr/ohci.c Sat May 3 15:37:04 2003
+++ ohci.c Tue May 6 08:57:29 2003
@@ -255,6 +255,7 @@
struct ohci_pipe {
struct usbd_pipe pipe;
ohci_soft_ed_t *sed;
+ u_int32_t aborting;
union {
ohci_soft_td_t *td;
ohci_soft_itd_t *itd;
@@ -635,6 +636,7 @@
OHCI_ITD_ALIGN, &dma);
if (err)
return (NULL);
+ s = splusb();
for(i = 0; i < OHCI_SITD_CHUNK; i++) {
offs = i * OHCI_SITD_SIZE;
sitd = KERNADDR(&dma, offs);
@@ -642,6 +644,7 @@
sitd->nextitd = sc->sc_freeitds;
sc->sc_freeitds = sitd;
}
+ splx(s);
}
s = splusb();
@@ -974,6 +977,18 @@
ohci_freex(struct usbd_bus *bus, usbd_xfer_handle xfer)
{
struct ohci_softc *sc = (struct ohci_softc *)bus;
+ struct ohci_xfer *oxfer = (struct ohci_xfer *)xfer;
+ ohci_soft_itd_t *sitd;
+
+ if (oxfer->ohci_xfer_flags & OHCI_ISOC_DIRTY) {
+ sitd = xfer->hcpriv;
+ if (sitd) {
+ for (; sitd->xfer == xfer; sitd = sitd->nextitd) {
+ if ( sitd == NULL ) break;
+ ohci_free_sitd(sc, sitd);
+ }
+ }
+ }
#ifdef DIAGNOSTIC
if (xfer->busy_free != XFER_BUSY) {
@@ -1380,7 +1395,9 @@
xfer->actlen += len;
if (std->flags & OHCI_CALL_DONE) {
xfer->status = USBD_NORMAL_COMPLETION;
+ s = splusb();
usb_transfer_complete(xfer);
+ splx(s);
}
ohci_free_std(sc, std);
} else {
@@ -1420,7 +1437,10 @@
xfer->status = USBD_STALLED;
else
xfer->status = USBD_IOERROR;
+
+ s = splusb();
usb_transfer_complete(xfer);
+ splx(s);
}
}
@@ -1445,27 +1465,37 @@
/* Handled by abort routine. */
continue;
}
+ if (xfer->pipe) {
+ if (xfer->pipe->aborting) continue; /*Ignore.*/
+ }
#ifdef DIAGNOSTIC
if (sitd->isdone)
printf("ohci_softintr: sitd=%p is done\n", sitd);
sitd->isdone = 1;
#endif
+ struct ohci_pipe *opipe =
+ (struct ohci_pipe *)xfer->pipe;
+ if (opipe->aborting )
+ continue;
+
cc = OHCI_ITD_GET_CC(le32toh(sitd->itd.itd_flags));
if (cc == OHCI_CC_NO_ERROR) {
/* XXX compute length for input */
- struct ohci_pipe *opipe =
- (struct ohci_pipe *)xfer->pipe;
if (sitd->flags & OHCI_CALL_DONE) {
opipe->u.iso.inuse -= xfer->nframes;
/* XXX update frlengths with actual length */
/* XXX xfer->actlen = actlen; */
xfer->status = USBD_NORMAL_COMPLETION;
+ s = splusb();
usb_transfer_complete(xfer);
+ splx(s);
}
} else {
/* XXX Do more */
xfer->status = USBD_IOERROR;
+ s = splusb();
usb_transfer_complete(xfer);
+ splx(s);
}
}
@@ -2079,6 +2109,7 @@
if (sitd == NULL)
goto bad1;
opipe->tail.itd = sitd;
+ opipe->aborting = 0;
tdphys = sitd->physaddr;
fmt = OHCI_ED_FORMAT_ISO;
if (UE_GET_DIR(ed->bEndpointAddress) == UE_DIR_IN)
@@ -3223,8 +3254,9 @@
ohci_softc_t *sc = (ohci_softc_t *)dev->bus;
ohci_soft_ed_t *sed = opipe->sed;
struct iso *iso = &opipe->u.iso;
+ struct ohci_xfer *oxfer = (struct ohci_xfer *)xfer;
ohci_soft_itd_t *sitd, *nsitd;
- ohci_physaddr_t buf, offs, noffs, bp0;
+ ohci_physaddr_t buf, offs, noffs, bp0, tdphys;
int i, ncur, nframes;
int s;
@@ -3242,6 +3274,24 @@
iso->next));
}
+ if (xfer->hcpriv) {
+ sitd = xfer->hcpriv;
+ for (; sitd->xfer == xfer; sitd = sitd->nextitd) {
+ if (sitd == NULL) break;
+ ohci_free_sitd(sc, sitd); /* Free ITDs in prev xfer*/
+ }
+ if (sitd == NULL) {
+ sitd = ohci_alloc_sitd(sc);
+ if (sitd == NULL) panic( "cant alloc isoc" );
+ opipe->tail.itd = sitd;
+ tdphys = sitd->physaddr;
+ sed->ed.ed_flags |= htole32(OHCI_ED_SKIP); /* Stop*/
+ sed->ed.ed_headp =
+ sed->ed.ed_tailp = htole32(tdphys);
+ sed->ed.ed_flags &= htole32(~OHCI_ED_SKIP); /* Start.*/
+ }
+ }
+
sitd = opipe->tail.itd;
buf = DMAADDR(&xfer->dmabuf, 0);
bp0 = OHCI_PAGE(buf);
@@ -3310,6 +3360,8 @@
xfer->status = USBD_IN_PROGRESS;
+ oxfer->ohci_xfer_flags |= OHCI_ISOC_DIRTY;
+
#ifdef USB_DEBUG
if (ohcidebug > 5) {
DPRINTF(("ohci_device_isoc_enter: frame=%d\n",
@@ -3340,6 +3392,8 @@
{
struct ohci_pipe *opipe = (struct ohci_pipe *)xfer->pipe;
ohci_softc_t *sc = (ohci_softc_t *)opipe->pipe.device->bus;
+ ohci_soft_ed_t *sed;
+ int s;
DPRINTFN(5,("ohci_device_isoc_start: xfer=%p\n", xfer));
@@ -3353,6 +3407,11 @@
/* XXX anything to do? */
+ s = splusb();
+ sed = opipe->sed; /* Turn off ED skip-bit to start processing */
+ sed->ed.ed_flags &= htole32(~OHCI_ED_SKIP); /* ED's ITD list.*/
+ splx(s);
+
return (USBD_IN_PROGRESS);
}
@@ -3366,6 +3425,7 @@
int s;
s = splusb();
+ opipe->aborting = 1;
DPRINTFN(1,("ohci_device_isoc_abort: xfer=%p\n", xfer));
@@ -3391,11 +3451,14 @@
return;
}
#endif
- for (; sitd->xfer == xfer; sitd = sitd->nextitd) {
+ if ( sitd ) { /* Only if xfer has ITDs. */
+ for (; sitd->xfer == xfer; sitd = sitd->nextitd) {
+ if ( sitd == NULL ) break;
#ifdef DIAGNOSTIC
DPRINTFN(1,("abort sets done sitd=%p\n", sitd));
sitd->isdone = 1;
#endif
+ }
}
splx(s);
@@ -3407,7 +3470,9 @@
/* Run callback. */
usb_transfer_complete(xfer);
- sed->ed.ed_headp = htole32(sitd->physaddr); /* unlink TDs */
+ if ( sitd ) { /* Only if there is an ITD... */
+ sed->ed.ed_headp = htole32(sitd->physaddr); /* unlink TDs */
+ }
sed->ed.ed_flags &= htole32(~OHCI_ED_SKIP); /* remove hardware skip */
splx(s);
@@ -3416,21 +3481,23 @@
void
ohci_device_isoc_done(usbd_xfer_handle xfer)
{
- struct ohci_pipe *opipe = (struct ohci_pipe *)xfer->pipe;
- ohci_softc_t *sc = (ohci_softc_t *)opipe->pipe.device->bus;
- ohci_soft_itd_t *sitd, *nsitd;
-
- DPRINTFN(1,("ohci_device_isoc_done: xfer=%p\n", xfer));
+/* This null routine corresponds to non-isoc "done()" routines
+ * that free the stds associated with an xfer after a completed
+ * xfer interrupt. However, in the case of isoc transfers, the
+ * sitds associated with the transfer have already been processed
+ * and reallocated for the next iteration by "ohci_device_isoc_transfer()".
+ *
+ * Routine "usb_transfer_complete()" is called at the end of every
+ * relevant usb interrupt. "usb_transfer_complete()" indirectly
+ * calls 1) "ohci_device_isoc_transfer()" (which keeps pumping the
+ * pipeline by setting up the next transfer iteration) and 2) then
+ * calls "ohci_device_isoc_done()". Isoc transfers have not been
+ * working for the ohci usb because this routine was trashing the
+ * xfer set up for the next iteration (thus, only the first
+ * UGEN_NISOREQS xfers outstanding on an open would work). Perhaps
+ * this could all be re-factored, but that's another pass...
+ */
- for (sitd = xfer->hcpriv;
- !(sitd->flags & OHCI_CALL_DONE);
- sitd = nsitd) {
- nsitd = sitd->nextitd;
- DPRINTFN(1,("ohci_device_isoc_done: free sitd=%p\n", sitd));
- ohci_free_sitd(sc, sitd);
- }
- ohci_free_sitd(sc, sitd);
- xfer->hcpriv = NULL;
}
usbd_status
@@ -3456,11 +3523,20 @@
{
struct ohci_pipe *opipe = (struct ohci_pipe *)pipe;
ohci_softc_t *sc = (ohci_softc_t *)pipe->device->bus;
+ ohci_soft_ed_t *sed;
DPRINTF(("ohci_device_isoc_close: pipe=%p\n", pipe));
- ohci_close_pipe(pipe, sc->sc_isoc_head);
+
+ sed = opipe->sed;
+ sed->ed.ed_flags |= htole32(OHCI_ED_SKIP); /* Stop device. */
+
+ ohci_close_pipe(pipe, sc->sc_isoc_head); /* Stop isoc list, free ED.*/
+
+ /* up to NISOREQs xfers still outstanding. */
+
+
#ifdef DIAGNOSTIC
opipe->tail.itd->isdone = 1;
#endif
- ohci_free_sitd(sc, opipe->tail.itd);
+ ohci_free_sitd(sc, opipe->tail.itd); /* Next `avail free' sitd.*/
}
--- ../ohci_5_10apr/ohcivar.h Sat May 3 15:37:11 2003
+++ ohcivar.h Tue May 6 08:05:00 2003
@@ -149,7 +149,9 @@
struct ohci_xfer {
struct usbd_xfer xfer;
struct usb_task abort_task;
+ u_int32_t ohci_xfer_flags;
};
+#define OHCI_ISOC_DIRTY 0x01
#define OXFER(xfer) ((struct ehci_xfer *)(xfer))
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200305061736.h46HaBke000587>