I noticed that usb-skeleton.c still has a TODO entry referring to a race involving urb->status, so I took a closer look. Fixing that race will be easy, and I will be happy to submit a patch for it. But there are two
FWIW, here's a patch I started to put together (unfinished). For that race (but I think the flag needs to be set earlier), magic number elimination, adding usb_buffer_alloc(), and a buffer length confusion I thought I saw.
other problems, partially related. Is it worthwhile to try to address them all? Is there no point, seeing as how usb-skeleton.c is just intended as a programming guide? But on the other hand, shouldn't a guide be _especially_ careful to do everything right?
I'd go for that second option ... :)
Here are the problems.
(1) Device writes use a normal, asynchronous usb_submit_urb() call to do their work. They return without waiting to see if the bulk
data transfer succeeded. Thus any problems during data transmission will
not be reflected back to the user.
Not necessarily a problem, but it's worth documenting IMO -- as well as the alternative design choice you mention (the synchronous call, used on the read/IN path).
(2) The release() routine unlinks an ongoing write in progress. This means that the last buffer of data written to the device before it is
closed will quite likely never be delivered. Owing to problem (1), the
user will not be aware of this.
As above, worth documenting, but if that message is only as significant as an ethernet packet (dropping them is allowed/expected) there may be no issue.
- Dave
--- ./drivers/usb-dist/usb-skeleton.c Tue Feb 11 08:37:49 2003
+++ ./drivers/usb/usb-skeleton.c Wed Feb 12 09:37:48 2003
@@ -15,8 +15,6 @@
* Thanks to Oliver Neukum and David Brownell for their help in debugging
* this driver.
*
- * TODO:
- * - fix urb->status race condition in write sequence
*
* History:
*
@@ -108,6 +106,7 @@
int bulk_out_size; /* the size of the send buffer
*/
struct urb * write_urb; /* the urb used to send data */
__u8 bulk_out_endpointAddr; /* the address of the bulk out
endpoint */
+ atomic_t write_busy; /* true iff write urb is busy
*/
struct work_struct work; /* work queue entry for line
discipline waking up */
int open; /* if the port is open or not
*/
@@ -206,7 +205,9 @@
if (dev->bulk_in_buffer != NULL)
kfree (dev->bulk_in_buffer);
if (dev->bulk_out_buffer != NULL)
- kfree (dev->bulk_out_buffer);
+ usb_buffer_free (dev->udev, dev->bulk_out_size,
+ dev->bulk_out_buffer,
+ dev->write_urb->transfer_dma);
if (dev->write_urb != NULL)
usb_free_urb (dev->write_urb);
kfree (dev);
@@ -288,7 +289,8 @@
}
/* shutdown any bulk writes that might be going on */
- usb_unlink_urb (dev->write_urb);
+ if (atomic_read (&dev->write_busy))
+ usb_unlink_urb (dev->write_urb);
dev->open = 0;
@@ -320,11 +322,12 @@
return -ENODEV;
}
- /* do an immediate bulk read to get data from the device */
+ /* do a blocking bulk read to get data from the device */
retval = usb_bulk_msg (dev->udev,
usb_rcvbulkpipe (dev->udev,
dev->bulk_in_endpointAddr),
- dev->bulk_in_buffer, dev->bulk_in_size,
+ dev->bulk_in_buffer,
+ min (dev->bulk_in_size, count),
&count, HZ*10);
/* if the read was successful, copy the data to userspace */
@@ -370,16 +373,18 @@
}
/* see if we are already in the middle of a write */
- if (dev->write_urb->status == -EINPROGRESS) {
+ if (atomic_read (&dev->write_busy)) {
dbg ("%s - already writing", __FUNCTION__);
goto exit;
}
- /* we can only write as much as 1 urb will hold */
+ /* we can only write as much as our bufffer will hold */
bytes_written = (count > dev->bulk_out_size) ?
dev->bulk_out_size : count;
- /* copy the data from userspace into our urb */
+ /* copy the data from userspace into our transfer buffer;
+ * this is the only copy required.
+ */
if (copy_from_user(dev->write_urb->transfer_buffer, buffer,
bytes_written)) {
retval = -EFAULT;
@@ -389,11 +394,8 @@
usb_skel_debug_data (__FUNCTION__, bytes_written,
dev->write_urb->transfer_buffer);
- /* set up our urb */
- usb_fill_bulk_urb(dev->write_urb, dev->udev,
- usb_sndbulkpipe(dev->udev, dev->bulk_out_endpointAddr),
- dev->write_urb->transfer_buffer, bytes_written,
- skel_write_bulk_callback, dev);
+ /* this urb was already set up, except for this write size */
+ dev->write_urb->transfer_buffer_length = bytes_written;
/* send the data out the bulk port */
/* a character device write uses GFP_KERNEL,
@@ -403,6 +405,7 @@
err("%s - failed submitting write urb, error %d",
__FUNCTION__, retval);
} else {
+ atomic_set (&dev->write_busy, 1);
retval = bytes_written;
}
@@ -455,13 +458,14 @@
dbg("%s - minor %d", __FUNCTION__, dev->minor);
+ /* sync/async unlink faults aren't errors */
if ((urb->status != -ENOENT) &&
(urb->status != -ECONNRESET)) {
dbg("%s - nonzero write bulk status received: %d",
__FUNCTION__, urb->status);
- return;
}
+ atomic_set (&dev->write_busy, 0);
return;
}
@@ -517,8 +521,9 @@
for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
endpoint = &iface_desc->endpoint[i].desc;
- if ((endpoint->bEndpointAddress & 0x80) &&
- ((endpoint->bmAttributes & 3) == 0x02)) {
+ if ((endpoint->bEndpointAddress & USB_DIR_IN) &&
+ ((endpoint->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
+ == USB_ENDPOINT_XFER_BULK)) {
/* we found a bulk in endpoint */
buffer_size = endpoint->wMaxPacketSize;
dev->bulk_in_size = buffer_size;
@@ -530,8 +535,9 @@
}
}
- if (((endpoint->bEndpointAddress & 0x80) == 0x00) &&
- ((endpoint->bmAttributes & 3) == 0x02)) {
+ if (((endpoint->bEndpointAddress & USB_DIR_IN) == 0x00) &&
+ ((endpoint->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
+ == USB_ENDPOINT_XFER_BULK)) {
/* we found a bulk out endpoint */
/* a probe() may sleep and has no restrictions on memory
allocations */
dev->write_urb = usb_alloc_urb(0, GFP_KERNEL);
@@ -539,10 +545,22 @@
err("No free urbs available");
goto error;
}
+ atomic_set (&dev->write_busy, 0);
+ dev->bulk_out_endpointAddr = endpoint->bEndpointAddress;
+
+ /* on some platforms using this kind of buffer alloc
+ * call eliminates a dma "bounce buffer".
+ *
+ * NOTE: you'd normally want i/o buffers that hold
+ * more than one packet, so that i/o delays between
+ * packets don't hurt throughput.
+ */
buffer_size = endpoint->wMaxPacketSize;
dev->bulk_out_size = buffer_size;
- dev->bulk_out_endpointAddr = endpoint->bEndpointAddress;
- dev->bulk_out_buffer = kmalloc (buffer_size, GFP_KERNEL);
+ dev->write_urb->transfer_flags = URB_NO_DMA_MAP;
+ dev->bulk_out_buffer = usb_buffer_alloc (udev,
+ buffer_size, GFP_KERNEL,
+ &dev->write_urb->transfer_dma);
if (!dev->bulk_out_buffer) {
err("Couldn't allocate bulk_out_buffer");
goto error;
