Re: [linux-usb-devel] Discussion of problems in usb-skeleton.c
On Tue, 25 Feb 2003, David Brownell wrote:
Alan Stern wrote:
Heck, I've barely had a chance to write it, let alone test it. In fact,
it seemed clear that nobody had tried any testing recently. One of the
minor bugs I fixed was a subroutine call that passed a structure rather
than a pointer to the structure; that would never have compiled.
So it does compile again, eh? Good.
I'm embarrassed to say that when I tried to compile it, I found two
errors. Here is a fixed-up version of the patch.
As it turns out, this still generates a compiler warning. There's a
mistake in the min() and max() macros in include/linux/kernel.h. I will
post something about that on the linux kernel mailing list.
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 Wed Feb 26 10:26:33 2003
@@ -1,5 +1,5 @@
/*
- * USB Skeleton driver - 0.9
+ * USB Skeleton driver - 1.0
*
* Copyright (c) 2001-2002 Greg Kroah-Hartman ([EMAIL PROTECTED])
*
@@ -12,14 +12,14 @@
* USB driver quickly. The design of it is based on the usb-serial and
* dc2xx drivers.
*
- * Thanks to Oliver Neukum and David Brownell for their help in debugging
- * this driver.
+ * Thanks to Oliver Neukum, David Brownell, and Alan Stern 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 */
__u8bulk_out_endpointAddr; /* the address of the bulk out
endpoint */
+ atomic_twrite_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 semaphoresem;/* 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
Re: [linux-usb-devel] Discussion of problems in usb-skeleton.c
On Wed, 26 Feb 2003, Alan Stern wrote: As it turns out, this still generates a compiler warning. There's a mistake in the min() and max() macros in include/linux/kernel.h. I will post something about that on the linux kernel mailing list. I spoke too soon (don't you just hate it when that happens?). The macros are fine; it's a problem with type incompatibility in the driver. Here is another patch (that goes on top of the first one) to fix the warning. Alan Stern --- usb-2.4/drivers/usb/usb-skeleton.c.2Wed Feb 26 10:40:42 2003 +++ edited/drivers/usb/usb-skeleton.c Wed Feb 26 10:43:42 2003 @@ -101,11 +101,11 @@ charnum_bulk_out; /* number of bulk out endpoints we have */ unsigned char * bulk_in_buffer; /* the buffer to receive data */ - int bulk_in_size; /* the size of the receive buffer */ + size_t bulk_in_size; /* the size of the receive buffer */ __u8bulk_in_endpointAddr; /* the address of the bulk in endpoint */ unsigned char * bulk_out_buffer;/* the buffer to send data */ - int bulk_out_size; /* the size of the send buffer */ + size_t bulk_out_size; /* the size of the send buffer */ struct urb *write_urb; /* the urb used to send data */ __u8bulk_out_endpointAddr; /* the address of the bulk out endpoint */ atomic_twrite_busy; /* true iff write urb is busy */ @@ -403,8 +403,7 @@ wait_for_completion (dev-write_finished); /* we can only write as much as our buffer will hold */ - bytes_written = (count dev-bulk_out_size) ? - dev-bulk_out_size : count; + bytes_written = min (dev-bulk_out_size, count); /* copy the data from userspace into our transfer buffer; * this is the only copy required. @@ -509,7 +508,7 @@ struct usb_host_interface *iface_desc; struct usb_endpoint_descriptor *endpoint; int minor; - int buffer_size; + size_t buffer_size; int i; int retval; char name[10];
Re: [linux-usb-devel] Discussion of problems in usb-skeleton.c
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 */
__u8bulk_out_endpointAddr; /* the address of the bulk out
endpoint */
+ atomic_twrite_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 semaphoresem;/* 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
Re: [linux-usb-devel] Discussion of problems in usb-skeleton.c
On Fri, Feb 21, 2003 at 01:12:47PM -0500, Alan Stern wrote: 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 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? Yes, it should be correct. 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. That's pretty typical for a number of drivers, unfortunatly. (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. Ooh, good catch. Changing the write() routine to use usb_bulk_msg() would fix all these problems, but this would defeat the purpose of showing people how to use usb_submit_urb(), usb_unlink_urb(), and completion handlers. What do you think is the right thing to do here? Don't know, I'd like to show how to do the async urb usage properly, the sync calls are pretty easy for people to work out how to use. How about two different example drivers? (Incidentally, usb-skeleton.c is a good example of a driver where the disconnect() routine can exit with urbs still in flight.) Ah, another good reason for the urb reference count to be added :) thanks, greg k-h --- This SF.net email is sponsored by: SlickEdit Inc. Develop an edge. The most comprehensive and flexible code editor you can use. Code faster. C/C++, C#, Java, HTML, XML, many more. FREE 30-Day Trial. www.slickedit.com/sourceforge ___ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
[linux-usb-devel] Discussion of problems in usb-skeleton.c
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 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? 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. (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. Changing the write() routine to use usb_bulk_msg() would fix all these problems, but this would defeat the purpose of showing people how to use usb_submit_urb(), usb_unlink_urb(), and completion handlers. What do you think is the right thing to do here? (Incidentally, usb-skeleton.c is a good example of a driver where the disconnect() routine can exit with urbs still in flight.) Alan Stern --- This SF.net email is sponsored by: SlickEdit Inc. Develop an edge. The most comprehensive and flexible code editor you can use. Code faster. C/C++, C#, Java, HTML, XML, many more. FREE 30-Day Trial. www.slickedit.com/sourceforge ___ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
