This patch for usb-skeleton.c, based on earlier work of David Brownell,
fixes a number of errors:
Checking urb->status to see whether the urb had completed was racy
and unsafe.
The read transfer count was calculated wrong.
An unused struct work_struct has been removed.
There was a race between open() and disconnect(); open() might end
up trying to use a deallocated usb_skel structure.
Upon device release, a final write was cancelled instead of being
allowed to complete.
An error would be returned when a previous write had not
completed; now the driver simply waits.
An active write urb would not be cancelled during disconnect.
In addition, several improvements were added:
The device write pathway illustrates the use of driver-allocated
DMA buffers.
Magic numbers have been replaced with symbolic constants.
New comments discuss the merits of different I/O styles and the
requirements for a driver's disconnect() routine.
Alan Stern
===== usb-skeleton.c 1.36 vs edited =====
--- 1.36/drivers/usb/usb-skeleton.c Tue Jan 14 12:00:34 2003
+++ edited/drivers/usb/usb-skeleton.c Tue Feb 25 13:41:25 2003
@@ -1,5 +1,5 @@
/*
- * USB Skeleton driver - 0.9
+ * USB Skeleton driver - 1.0
*
* Copyright (c) 2001-2002 Greg Kroah-Hartman ([EMAIL PROTECTED])
*
@@ -15,11 +15,11 @@
* Thanks to Oliver Neukum and David Brownell for their help in debugging
* this driver.
*
- * TODO:
- * - fix urb->status race condition in write sequence
*
* History:
*
+ * 2003-02-25 - 1.0 - fix races involving urb->status, unlink_urb(), and
+ * disconnect.
* 2002_12_12 - 0.9 - compile fixes and got rid of fixed minor array.
* 2002_09_26 - 0.8 - changes due to USB core conversion to struct device
* driver.
@@ -42,8 +42,8 @@
#include <linux/init.h>
#include <linux/slab.h>
#include <linux/module.h>
-#include <linux/spinlock.h>
#include <linux/smp_lock.h>
+#include <linux/completion.h>
#include <linux/devfs_fs_kernel.h>
#include <asm/uaccess.h>
#include <linux/usb.h>
@@ -60,7 +60,7 @@
/* Version Information */
-#define DRIVER_VERSION "v0.9"
+#define DRIVER_VERSION "v1.0"
#define DRIVER_AUTHOR "Greg Kroah-Hartman, [EMAIL PROTECTED]"
#define DRIVER_DESC "USB Skeleton Driver"
@@ -108,8 +108,9 @@
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 completion write_finished; /* wait for the write to
finish */
- struct work_struct work; /* work queue entry for line
discipline waking up */
int open; /* if the port is open or not
*/
struct semaphore sem; /* locks this structure */
};
@@ -118,6 +119,8 @@
/* the global usb devfs handle */
extern devfs_handle_t usb_devfs_handle;
+/* prevent races between open() and disconnect() */
+static DECLARE_MUTEX (disconnect_sem);
/* local function prototypes */
static ssize_t skel_read (struct file *file, char *buffer, size_t count, loff_t
*ppos);
@@ -206,7 +209,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);
@@ -227,17 +232,23 @@
subminor = minor (inode->i_rdev);
+ /* prevent disconnects */
+ down (&disconnect_sem);
+
interface = usb_find_interface (&skel_driver,
mk_kdev(USB_MAJOR, subminor));
if (!interface) {
err ("%s - error, can't find device for minor %d",
__FUNCTION__, subminor);
- return -ENODEV;
+ retval = -ENODEV;
+ goto exit_no_device;
}
dev = usb_get_intfdata(interface);
- if (!dev)
- return -ENODEV;
+ if (!dev) {
+ retval = -ENODEV;
+ goto exit_no_device;
+ }
/* lock this device */
down (&dev->sem);
@@ -251,6 +262,8 @@
/* unlock this device */
up (&dev->sem);
+exit_no_device:
+ up (&disconnect_sem);
return retval;
}
@@ -280,6 +293,12 @@
goto exit_not_opened;
}
+ /* wait for any bulk writes that might be going on to finish up */
+ if (atomic_read (&dev->write_busy))
+ wait_for_completion (&dev->write_finished);
+
+ dev->open = 0;
+
if (dev->udev == NULL) {
/* the device was unplugged before the file was released */
up (&dev->sem);
@@ -287,11 +306,6 @@
return 0;
}
- /* shutdown any bulk writes that might be going on */
- usb_unlink_urb (dev->write_urb);
-
- dev->open = 0;
-
exit_not_opened:
up (&dev->sem);
@@ -320,11 +334,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 */
@@ -343,6 +358,18 @@
/**
* skel_write
+ *
+ * A device driver has to decide how to report I/O errors back to the
+ * user. The safest course is to wait for the transfer to finish before
+ * returning so that any errors will be reported reliably. Skel_read()
+ * works like this. But waiting for I/O is slow, so many drivers only
+ * check for errors during I/O initiation and do not report problems
+ * that occur during the actual transfer. That's what we will do here.
+ *
+ * A driver concerned with maximum I/O throughput would use double-
+ * buffering: Two urbs would be devoted to write transfers, so that
+ * one urb could always be active while the other was waiting for the
+ * user to send more data.
*/
static ssize_t skel_write (struct file *file, const char *buffer, size_t count,
loff_t *ppos)
{
@@ -369,17 +396,19 @@
goto exit;
}
- /* see if we are already in the middle of a write */
- if (dev->write_urb->status == -EINPROGRESS) {
- dbg ("%s - already writing", __FUNCTION__);
- goto exit;
- }
+ /* wait for a previous write to finish up; we don't use a timeout
+ * and so a nonresponsive device can delay us indefinitely.
+ */
+ if (atomic_read (&dev->write_busy))
+ wait_for_completion (&dev->write_finished);
- /* we can only write as much as 1 urb will hold */
+ /* we can only write as much as our buffer 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,17 +418,17 @@
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,
unless a spinlock is held */
+ init_completion (&dev->write_finished);
+ atomic_set (&dev->write_busy, 1);
retval = usb_submit_urb(dev->write_urb, GFP_KERNEL);
if (retval) {
+ atomic_set (&dev->write_busy, 0);
err("%s - failed submitting write urb, error %d",
__FUNCTION__, retval);
} else {
@@ -455,14 +484,15 @@
dbg("%s - minor %d", __FUNCTION__, dev->minor);
- if ((urb->status != -ENOENT) &&
- (urb->status != -ECONNRESET)) {
+ /* 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;
}
- return;
+ /* notify anyone waiting that the write has finished */
+ atomic_set (&dev->write_busy, 0);
+ complete (&dev->write_finished);
}
@@ -517,8 +547,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 +561,9 @@
}
}
- if (((endpoint->bEndpointAddress & 0x80) == 0x00) &&
- ((endpoint->bmAttributes & 3) == 0x02)) {
+ if ( !(endpoint->bEndpointAddress & USB_DIR_IN) &&
+ ((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 +571,22 @@
err("No free urbs available");
goto error;
}
+ 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 |
+ URB_ASYNC_UNLINK);
+ 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;
@@ -593,12 +637,21 @@
* skel_disconnect
*
* Called by the usb core when the device is removed from the system.
+ *
+ * This routine guarantees that the driver will not submit any more urbs
+ * by clearing dev->udev. It is also supposed to terminate any currently
+ * active urbs. Unfortunately, usb_bulk_msg(), used in skel_read(), does
+ * not provide any way to do this. But at least we can cancel an active
+ * write.
*/
static void skel_disconnect(struct usb_interface *interface)
{
struct usb_skel *dev;
int minor;
+ /* prevent races with open() */
+ down (&disconnect_sem);
+
dev = usb_get_intfdata (interface);
usb_set_intfdata (interface, NULL);
@@ -617,16 +670,22 @@
/* give back our dynamic minor */
usb_deregister_dev (1, minor);
-
- /* if the device is not opened, then we clean up right now */
- if (!dev->open) {
- up (&dev->sem);
- skel_delete (dev);
- } else {
- dev->udev = NULL;
- up (&dev->sem);
+
+ /* terminate an ongoing write */
+ if (atomic_read (&dev->write_busy)) {
+ usb_unlink_urb (&dev->write_urb);
+ wait_for_completion (&dev->write_finished);
}
+ dev->udev = NULL;
+ up (&dev->sem);
+
+ /* if the device is not opened, then we clean up right now */
+ if (!dev->open)
+ skel_delete (&dev);
+
+ up (&disconnect_sem);
+
info("USB Skeleton #%d now disconnected", minor);
}
@@ -668,4 +727,3 @@
MODULE_AUTHOR(DRIVER_AUTHOR);
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL");
-
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
