Re: Possible bug in isp116x-hcd

On Mon, Jun 18, 2007 at 11:00:10AM -0400, Alan Stern wrote:
> Olav,
> 
> What would be involved in refactoring isp116x-hcd so that the driver
> called usb_hcd_giveback_urb() as soon as the URB was completed?  If I
> understand the current code correctly, postproc_atl_queue() goes
> through all the active endpoints and stores the status for the
> completed URBs, and then later finish_atl_transfers() gives them back.
> 
> It looks like this shouldn't present any difficulty except for one 
> thing: When you get a short Control-IN transfer with URB_SHORT_NOT_OK 
> set, the error status gets stored while the URB's status stage 
> executes.  That would have to be changed so that the status stage was 
> skipped entirely.  Doing this shouldn't cause any problems in practice; 
> the other HCDs skip the status stage when this sort of error occurs.
> 
> The point of making this change is that it would allow the driver to 
> avoid saving values in urb->status, leading eventually to the 
> possibility of removing the status field from struct urb.

Hi Alan,

Do you mean something like that below? I haven't tested it.

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-20 22:49:27.000000000 +0300
@@ -210,6 +210,9 @@
 
 /*---------------------------------------------------------------*/
 
+static void finish_request(struct isp116x *isp116x, struct isp116x_ep *ep,
+			   struct urb *urb);
+
 /*
   Set up PTD's.
 */
@@ -296,11 +299,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 +306,16 @@
 				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];
+				finish_request(isp116x, ep, urb);
 				spin_unlock(&urb->lock);
 				continue;
 			}
 		}
-		/* 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,6 +324,7 @@
 				urb->status = cc_to_error[cc];
 			if (ep->nextpid == USB_PID_ACK)
 				ep->nextpid = 0;
+			finish_request(isp116x, ep, urb);
 			spin_unlock(&urb->lock);
 			continue;
 		}
@@ -341,6 +334,7 @@
 		if (usb_pipeint(urb->pipe) && !PTD_GET_LEN(ptd)) {
 			if (urb->status == -EINPROGRESS)
 				urb->status = 0;
+			finish_request(isp116x, ep, urb);
 			spin_unlock(&urb->lock);
 			continue;
 		}
@@ -409,6 +403,8 @@
 		default:
 			BUG();
 		}
+		if (urb->status != -EINPROGRESS)
+			finish_request(isp116x, ep, urb);
 		spin_unlock(&urb->lock);
 	}
 }
@@ -570,9 +566,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 +575,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