Re: [Libusb-devel] Handling short transfers

[Alan S. <st...@ro...>]

Daniel and David:

This kernel patch implements the plan we discussed for handling short 
bulk transfers.  It adds a new URB flag: USBDEVFS_URB_XFER_START.  
Here's how it works:

When libusb breaks a bulk-IN transfer up into multiple URBs, it should 
set

	USBDEVFS_URB_SHORT_NOT_OK | USBDEVFS_URB_XFER_START

in the flags for the first URB, and

	USBDEVFS_URB_SHORT_NOT_OK

for all the following ones.  If any of these URBs completes with an 
error before libusb has finished submitting all of them, it should stop 
the submissions (they'll fail anyway with an EREMOTEIO error; perhaps 
a different code would be better).  It probably already does this.

The kernel changes are transparent for URBs that don't have
USBDEVFS_URB_SHORT_NOT_OK set; they'll just sail on through as before.  
Hence versions of libusb that aren't updated should continue to work 
with no ill effects.

But for bulk-IN URBs that do have this flag bit set, any fault other
than an unlink will cause the kernel to immediately cancel all the
following URBs for the same endpoint and refuse to accept any new ones.  
This state of affairs will persist until an URB that also has
USBDEVFS_URB_XFER_START is encountered, at which point the behavior
reverts to normal.  Such an URB would of course mark the beginning of a 
new transfer.

The patch behaves correctly in simple tests on my system.  We should be
possible to get it incorporated into version 2.6.32 of the kernel if
you want.  Do you have a good way to try it out?

Alan Stern



Index: 2.6.30/include/linux/usbdevice_fs.h
===================================================================
--- 2.6.30.orig/include/linux/usbdevice_fs.h
+++ 2.6.30/include/linux/usbdevice_fs.h
@@ -77,6 +77,7 @@ struct usbdevfs_connectinfo {
 
 #define USBDEVFS_URB_SHORT_NOT_OK	0x01
 #define USBDEVFS_URB_ISO_ASAP		0x02
+#define USBDEVFS_URB_XFER_START		0x04
 #define USBDEVFS_URB_NO_FSBR		0x20
 #define USBDEVFS_URB_ZERO_PACKET	0x40
 #define USBDEVFS_URB_NO_INTERRUPT	0x80
Index: 2.6.30/drivers/usb/core/devio.c
===================================================================
--- 2.6.30.orig/drivers/usb/core/devio.c
+++ 2.6.30/drivers/usb/core/devio.c
@@ -73,6 +73,7 @@ struct dev_state {
 	void __user *disccontext;
 	unsigned long ifclaimed;
 	u32 secid;
+	u16 disabled_bulk_eps;
 };
 
 struct async {
@@ -87,6 +88,8 @@ struct async {
 	struct urb *urb;
 	int status;
 	u32 secid;
+	u16 ep_bit;
+	u8 short_packet_status;
 };
 
 static int usbfs_snoop;
@@ -320,6 +323,45 @@ static void snoop_urb(struct urb *urb, v
 	printk("\n");
 }
 
+#define AS_START	1
+#define AS_UNLINK	2
+
+static void cancel_bulk_urbs(struct dev_state *ps, unsigned ep_bit)
+__releases(ps->lock)
+__acquires(ps->lock)
+{
+	struct async *as;
+
+	/* Mark all the pending URBs that match ep_bit, up to but not
+	 * including the first one with AS_START set.  If such an URB
+	 * is encountered then a new transfer has already started so the
+	 * endpoint doesn't need to be disabled; otherwise it does.
+	 */
+	list_for_each_entry(as, &ps->async_pending, asynclist) {
+		if (ep_bit == as->ep_bit) {
+			if (as->short_packet_status == AS_START) {
+				ep_bit = 0;
+				break;
+			}
+			as->short_packet_status = AS_UNLINK;
+			as->ep_bit = 0;
+		}
+	}
+	ps->disabled_bulk_eps |= ep_bit;
+
+	/* Now carefully unlink all the marked pending URBs */
+ rescan:
+	list_for_each_entry(as, &ps->async_pending, asynclist) {
+		if (as->short_packet_status == AS_UNLINK) {
+			as->short_packet_status = 0;	/* Only once */
+			spin_unlock(&ps->lock);		/* Allow completions */
+			usb_unlink_urb(as->urb);
+			spin_lock(&ps->lock);
+			goto rescan;
+		}
+	}
+}
+
 static void async_completed(struct urb *urb)
 {
 	struct async *as = urb->context;
@@ -328,6 +370,8 @@ static void async_completed(struct urb *
 
 	spin_lock(&ps->lock);
 	list_move_tail(&as->asynclist, &ps->async_completed);
+	if (urb->status < 0 && urb->status != -ECONNRESET && as->ep_bit)
+		cancel_bulk_urbs(ps, as->ep_bit);
 	spin_unlock(&ps->lock);
 	as->status = urb->status;
 	if (as->signr) {
@@ -978,6 +1022,7 @@ static int proc_do_submiturb(struct dev_
 
 	if (uurb->flags & ~(USBDEVFS_URB_ISO_ASAP |
 				USBDEVFS_URB_SHORT_NOT_OK |
+				USBDEVFS_URB_XFER_START |
 				USBDEVFS_URB_NO_FSBR |
 				USBDEVFS_URB_ZERO_PACKET |
 				USBDEVFS_URB_NO_INTERRUPT))
@@ -1149,6 +1194,8 @@ static int proc_do_submiturb(struct dev_
 	if (uurb->flags & USBDEVFS_URB_NO_INTERRUPT)
 		u |= URB_NO_INTERRUPT;
 	as->urb->transfer_flags = u;
+	if (uurb->flags & USBDEVFS_URB_XFER_START)
+		as->short_packet_status = AS_START;
 
 	as->urb->transfer_buffer_length = uurb->buffer_length;
 	as->urb->setup_packet = (unsigned char *)dr;
@@ -1186,9 +1233,30 @@ static int proc_do_submiturb(struct dev_
 			return -EFAULT;
 		}
 	}
+	if ((uurb->flags & USBDEVFS_URB_SHORT_NOT_OK) &&
+			usb_endpoint_is_bulk_in(&ep->desc))
+		as->ep_bit = 1 << usb_endpoint_num(&ep->desc);
 	snoop_urb(as->urb, as->userurb);
 	async_newpending(as);
-	if ((ret = usb_submit_urb(as->urb, GFP_KERNEL))) {
+
+	if (as->ep_bit) {
+		/* Don't submit bulk-IN URBs if the endpoint is disabled
+		 * because a short packet was received, unless this URB
+		 * is the start of a new transfer.
+		 */
+		spin_lock_irq(&ps->lock);
+		if (as->short_packet_status == AS_START)
+			ps->disabled_bulk_eps &= ~as->ep_bit;
+		if (ps->disabled_bulk_eps & as->ep_bit)
+			ret = -EREMOTEIO;
+		else
+			ret = usb_submit_urb(as->urb, GFP_ATOMIC);
+		spin_unlock_irq(&ps->lock);
+	} else {
+		ret = usb_submit_urb(as->urb, GFP_KERNEL);
+	}
+
+	if (ret) {
 		dev_printk(KERN_DEBUG, &ps->dev->dev,
 			   "usbfs: usb_submit_urb returned %d\n", ret);
 		async_removepending(as);