Re: Possible bug in isp116x-hcd

On Thu, Jun 21, 2007 at 05:17:42PM -0400, Alan Stern wrote:
> On Thu, 21 Jun 2007, ok wrote:
> > Do you mean something like that below? I haven't tested it.
> 
> Yes, something very much like this.  It could be simplified a little;  
> in a couple of places where you have
> 
> 	finish_request();
> 	spin_unlock();
> 	continue;
> 
> you ought to be able to use a "goto" or "break" instead.  Also there
> are a few minor problems, like calling finish_request before it is
> declared and calling it before releasing urb->lock, but those can 
> easily be fixed.

Yes, I made a mistake with urb->lock. OK, moved finish_request 
unmodified up in the file, got fatter patch. The one below.

> If you like, I'll try revising your patch.  You'll have to test it 

Yes, thanks.

> though; I don't have the hardware.

Sure.

Olav


--- linux-2.6.22-rc5-or/drivers/usb/host/isp116x-hcd.c	2007-06-17 05:09:12.000000000 +0300
+++ linux-2.6.22-rc5-isp/drivers/usb/host/isp116x-hcd.c	2007-06-23 00:22:01.000000000 +0300
@@ -275,6 +275,61 @@
 }
 
 /*
+  Take done or failed requests out of schedule. Give back
+  processed urbs.
+*/
+static void finish_request(struct isp116x *isp116x, struct isp116x_ep *ep,
+			   struct urb *urb)
+__releases(isp116x->lock) __acquires(isp116x->lock)
+{
+	unsigned i;
+
+	urb->hcpriv = NULL;
+	ep->error_count = 0;
+
+	if (usb_pipecontrol(urb->pipe))
+		ep->nextpid = USB_PID_SETUP;
+
+	urb_dbg(urb, "Finish");
+
+	spin_unlock(&isp116x->lock);
+	usb_hcd_giveback_urb(isp116x_to_hcd(isp116x), urb);
+	spin_lock(&isp116x->lock);
+
+	/* take idle endpoints out of the schedule */
+	if (!list_empty(&ep->hep->urb_list))
+		return;
+
+	/* async deschedule */
+	if (!list_empty(&ep->schedule)) {
+		list_del_init(&ep->schedule);
+		return;
+	}
+
+	/* periodic deschedule */
+	DBG("deschedule qh%d/%p branch %d\n", ep->period, ep, ep->branch);
+	for (i = ep->branch; i < PERIODIC_SIZE; i += ep->period) {
+		struct isp116x_ep *temp;
+		struct isp116x_ep **prev = &isp116x->periodic[i];
+
+		while (*prev && ((temp = *prev) != ep))
+			prev = &temp->next;
+		if (*prev)
+			*prev = ep->next;
+		isp116x->load[i] -= ep->load;
+	}
+	ep->branch = PERIODIC_SIZE;
+	isp116x_to_hcd(isp116x)->self.bandwidth_allocated -=
+	    ep->load / ep->period;
+
+	/* switch irq type? */
+	if (!--isp116x->periodic_count) {
+		isp116x->irqenb &= ~HCuPINT_SOF;
+		isp116x->irqenb |= HCuPINT_ATL;
+	}
+}
+
+/*
   Analyze transfer results, handle partial transfers and errors
 */
 static void postproc_atl_queue(struct isp116x *isp116x)
@@ -296,11 +351,6 @@
 		short_not_ok = 1;
 		spin_lock(&urb->lock);
 
-		/* Data underrun is special. For allowed underrun
-		   we clear the error and continue as normal. For
-		   forbidden underrun we finish the DATA stage
-		   immediately while for control transfer,
-		   we do a STATUS stage. */
 		if (cc == TD_DATAUNDERRUN) {
 			if (!(urb->transfer_flags & URB_SHORT_NOT_OK)) {
 				DBG("Allowed data underrun\n");
@@ -308,22 +358,14 @@
 				short_not_ok = 0;
 			} else {
 				ep->error_count = 1;
-				if (usb_pipecontrol(urb->pipe))
-					ep->nextpid = USB_PID_ACK;
-				else
-					usb_settoggle(udev, ep->epnum,
-						      ep->nextpid ==
-						      USB_PID_OUT,
-						      PTD_GET_TOGGLE(ptd));
+				usb_settoggle(udev, ep->epnum,
+					      ep->nextpid == USB_PID_OUT,
+					      PTD_GET_TOGGLE(ptd));
 				urb->actual_length += PTD_GET_COUNT(ptd);
 				urb->status = cc_to_error[TD_DATAUNDERRUN];
-				spin_unlock(&urb->lock);
-				continue;
+				goto done;
 			}
 		}
-		/* Keep underrun error through the STATUS stage */
-		if (urb->status == cc_to_error[TD_DATAUNDERRUN])
-			cc = TD_DATAUNDERRUN;
 
 		if (cc != TD_CC_NOERROR && cc != TD_NOTACCESSED
 		    && (++ep->error_count >= 3 || cc == TD_CC_STALL
@@ -332,8 +374,7 @@
 				urb->status = cc_to_error[cc];
 			if (ep->nextpid == USB_PID_ACK)
 				ep->nextpid = 0;
-			spin_unlock(&urb->lock);
-			continue;
+			goto done;
 		}
 		/* According to usb spec, zero-length Int transfer signals
 		   finishing of the urb. Hey, does this apply only
@@ -341,8 +382,7 @@
 		if (usb_pipeint(urb->pipe) && !PTD_GET_LEN(ptd)) {
 			if (urb->status == -EINPROGRESS)
 				urb->status = 0;
-			spin_unlock(&urb->lock);
-			continue;
+			goto done;
 		}
 
 		/* Relax after previously failed, but later succeeded
@@ -409,62 +449,10 @@
 		default:
 			BUG();
 		}
+	done:
 		spin_unlock(&urb->lock);
-	}
-}
-
-/*
-  Take done or failed requests out of schedule. Give back
-  processed urbs.
-*/
-static void finish_request(struct isp116x *isp116x, struct isp116x_ep *ep,
-			   struct urb *urb)
-__releases(isp116x->lock) __acquires(isp116x->lock)
-{
-	unsigned i;
-
-	urb->hcpriv = NULL;
-	ep->error_count = 0;
-
-	if (usb_pipecontrol(urb->pipe))
-		ep->nextpid = USB_PID_SETUP;
-
-	urb_dbg(urb, "Finish");
-
-	spin_unlock(&isp116x->lock);
-	usb_hcd_giveback_urb(isp116x_to_hcd(isp116x), urb);
-	spin_lock(&isp116x->lock);
-
-	/* take idle endpoints out of the schedule */
-	if (!list_empty(&ep->hep->urb_list))
-		return;
-
-	/* async deschedule */
-	if (!list_empty(&ep->schedule)) {
-		list_del_init(&ep->schedule);
-		return;
-	}
-
-	/* periodic deschedule */
-	DBG("deschedule qh%d/%p branch %d\n", ep->period, ep, ep->branch);
-	for (i = ep->branch; i < PERIODIC_SIZE; i += ep->period) {
-		struct isp116x_ep *temp;
-		struct isp116x_ep **prev = &isp116x->periodic[i];
-
-		while (*prev && ((temp = *prev) != ep))
-			prev = &temp->next;
-		if (*prev)
-			*prev = ep->next;
-		isp116x->load[i] -= ep->load;
-	}
-	ep->branch = PERIODIC_SIZE;
-	isp116x_to_hcd(isp116x)->self.bandwidth_allocated -=
-	    ep->load / ep->period;
-
-	/* switch irq type? */
-	if (!--isp116x->periodic_count) {
-		isp116x->irqenb &= ~HCuPINT_SOF;
-		isp116x->irqenb |= HCuPINT_ATL;
+		if (urb->status != -EINPROGRESS)
+			finish_request(isp116x, ep, urb);
 	}
 }
 
@@ -570,9 +558,6 @@
 */
 static void finish_atl_transfers(struct isp116x *isp116x)
 {
-	struct isp116x_ep *ep;
-	struct urb *urb;
-
 	if (!isp116x->atl_active)
 		return;
 	/* Fifo not ready? */
@@ -582,16 +567,6 @@
 	atomic_inc(&isp116x->atl_finishing);
 	unpack_fifo(isp116x);
 	postproc_atl_queue(isp116x);
-	for (ep = isp116x->atl_active; ep; ep = ep->active) {
-		urb =
-		    container_of(ep->hep->urb_list.next, struct urb, urb_list);
-		/* USB_PID_ACK check here avoids finishing of
-		   control transfers, for which TD_DATAUNDERRUN
-		   occured, while URB_SHORT_NOT_OK was set */
-		if (urb && urb->status != -EINPROGRESS
-		    && ep->nextpid != USB_PID_ACK)
-			finish_request(isp116x, ep, urb);
-	}
 	atomic_dec(&isp116x->atl_finishing);
 }
 

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel