ohci usb isochronous xfers (abort panic, start)

[Bruce R. Montague]

The following patch fixes two problems with the usb
ohci driver and isochrous transfers: 1) Isochronous
transfers are never started; 2) if an application
conducting isochronous transfers is aborted (ctrl-C),
a kernel panic occurs.

The routine "ohci.c/ohci_device_isoc_start()" never
sets the bit in the EndPoint descriptor that allows
isochronous TDs to be processed by the controller
(that is, it never `starts the device'). There is
a comment in the code suggesting this was an unclear
issue.

The abort code in "ohci_device_isoc_abort()" and the
code in "ohci_device_isoc_done()" do not check for the
case (apparently common) in which there are no ITDs
attached to the relevant xfer structure (and this
panic).

If there is anyone that could check this, that
would be great.  Even better, if anyone has advice
on working with ohci isochronous transfers
(especially without a usb analyzer)...

I don't see how the current code could ever do
isochronous transfers on an OHCI usb controller.
My `test device' is an "OmniVision OV511+ Camera"
(actually a D-Link DSB-C100), known to work with
uhci usb controllers. With the patched code, the
application ("vid") reads the input stream, identifies
the image header in the data stream, and reads about
20 frames before things just quit. No errors or
anything, just no more data. Any advice most
appreciated!



The patch:
====================================
--- /usr/src/sys/dev/usb/ohci.c.old	Sat Apr 19 06:45:25 2003
+++ /usr/src/sys/dev/usb/ohci.c	Sat Apr 19 09:22:14 2003
@@ -3340,6 +3340,7 @@
 {
 	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;
 
 	DPRINTFN(5,("ohci_device_isoc_start: xfer=%p\n", xfer));
 
@@ -3353,6 +3354,9 @@
 
 	/* XXX anything to do? */
 
+        sed = opipe->sed;  /*  Turn off ED skip-bit to start processing */
+        sed->ed.ed_flags &= htole32(~OHCI_ED_SKIP);    /* ED's ITD list.*/
+
 	return (USBD_IN_PROGRESS);
 }
 
@@ -3391,11 +3395,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 +3414,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);
@@ -3421,6 +3430,8 @@
 	ohci_soft_itd_t *sitd, *nsitd;
 
 	DPRINTFN(1,("ohci_device_isoc_done: xfer=%p\n", xfer));
+
+	if( 0 == xfer->hcpriv ) return; /* Only if xfer has ITDs.*/
 
 	for (sitd = xfer->hcpriv;
 	     !(sitd->flags & OHCI_CALL_DONE);