USB: FIx locks and urb->status in adutux (updated)

[Pete Zaitcev]

Two main issues fixed here are:
 - An improper use of in-struct lock to protect an open count
 - Use of urb status for -EINPROGRESS

Also, along the way:
 - Change usb_unlink_urb to usb_kill_urb. Apparently there's no need
   to use usb_unlink_urb whatsoever in this driver, and the old use of
   usb_kill_urb was outright racy (it unlinked and immediately freed).
 - Fix indentation in adu_write. Looks like it was damaged by a script.
 - Vitaly wants -EBUSY on multiply opens.
 - bInterval was taken from a wrong endpoint.

Signed-off-by: Pete Zaitcev <[EMAIL PROTECTED]>
Signed-off-by: Vitaliy Ivanov <[EMAIL PROTECTED]>
Tested-by: Vitaliy Ivanov <[EMAIL PROTECTED]>

diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..5a2c44e 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
 
 #define COMMAND_TIMEOUT(2*HZ)  /* 60 second timeout for a command */
 
+/*
+ * The locking scheme is a vanilla 3-lock:
+ *   adu_device.buflock: A spinlock, covers what IRQs touch.
+ *   adutux_mutex:   A Static lock to cover open_count. It would also cover
+ *   any globals, but we don't have them in 2.6.
+ *   adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ *   It covers all of adu_device, except the open_count
+ *   and what .buflock covers.
+ */
+
 /* Structure to hold all of our device specific stuff */
 struct adu_device {
-   struct mutexmtx; /* locks this structure */
+   struct mutexmtx;
struct usb_device*  udev; /* save off the usb device pointer */
struct usb_interface*   interface;
-   unsigned char   minor; /* the starting minor number for this 
device */
+   unsigned intminor; /* the starting minor number for this 
device */
charserial_number[8];
 
int open_count; /* number of times this port has 
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char*   interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+   int out_urb_finished;
 };
 
+static DEFINE_MUTEX(adutux_mutex);
+
 static struct usb_driver adu_driver;
 
 static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, const char 
*function, int size,
  */
 static void adu_abort_transfers(struct adu_device *dev)
 {
-   dbg(2," %s : enter", __FUNCTION__);
+   unsigned long flags;
 
-   if (dev == NULL) {
-   dbg(1," %s : dev is null", __FUNCTION__);
-   goto exit;
-   }
+   dbg(2," %s : enter", __FUNCTION__);
 
if (dev->udev == NULL) {
dbg(1," %s : udev is null", __FUNCTION__);
goto exit;
}
 
-   dbg(2," %s : udev state %d", __FUNCTION__, dev->udev->state);
-   if (dev->udev->state == USB_STATE_NOTATTACHED) {
-   dbg(1," %s : udev is not attached", __FUNCTION__);
-   goto exit;
-   }
-
/* shutdown transfer */
-   usb_unlink_urb(dev->interrupt_in_urb);
-   usb_unlink_urb(dev->interrupt_out_urb);
+
+   /* XXX Anchor these instead */
+   spin_lock_irqsave(>buflock, flags);
+   if (!dev->read_urb_finished) {
+   spin_unlock_irqrestore(>buflock, flags);
+   usb_kill_urb(dev->interrupt_in_urb);
+   } else
+   spin_unlock_irqrestore(>buflock, flags);
+
+   spin_lock_irqsave(>buflock, flags);
+   if (!dev->out_urb_finished) {
+   spin_unlock_irqrestore(>buflock, flags);
+   usb_kill_urb(dev->interrupt_out_urb);
+   } else
+   spin_unlock_irqrestore(>buflock, flags);
 
 exit:
dbg(2," %s : leave", __FUNCTION__);
@@ -162,8 +179,6 @@ static void adu_delete(struct adu_device *dev)
 {
dbg(2, "%s enter", __FUNCTION__);
 
-   adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev->interrupt_in_urb);
usb_free_urb(dev->interrupt_out_urb);
@@ -239,7 +254,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
 
-   wake_up_interruptible(>write_wait);
+   spin_lock(>buflock);
+   dev->out_urb_finished = 1;
+   wake_up(>write_wait);
+   spin_unlock(>buflock);
 exit:
 
adu_debug_data(5, __FUNCTION__, urb->actual_length,
@@ -252,12 +270,17 @@ static int adu_open(struct inode *inode, struct file 
*file)
struct adu_device *dev = NULL;
struct usb_interface *interface;
int subminor;
-   int retval = 0;
+   int retval;
 
dbg(2,"%s : enter", __FUNCTION__);
 
subminor = iminor(inode);
 
+   if ((retval = 

USB: FIx locks and urb-status in adutux (updated)

[Pete Zaitcev]

Two main issues fixed here are:
 - An improper use of in-struct lock to protect an open count
 - Use of urb status for -EINPROGRESS

Also, along the way:
 - Change usb_unlink_urb to usb_kill_urb. Apparently there's no need
   to use usb_unlink_urb whatsoever in this driver, and the old use of
   usb_kill_urb was outright racy (it unlinked and immediately freed).
 - Fix indentation in adu_write. Looks like it was damaged by a script.
 - Vitaly wants -EBUSY on multiply opens.
 - bInterval was taken from a wrong endpoint.

Signed-off-by: Pete Zaitcev [EMAIL PROTECTED]
Signed-off-by: Vitaliy Ivanov [EMAIL PROTECTED]
Tested-by: Vitaliy Ivanov [EMAIL PROTECTED]

diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..5a2c44e 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
 
 #define COMMAND_TIMEOUT(2*HZ)  /* 60 second timeout for a command */
 
+/*
+ * The locking scheme is a vanilla 3-lock:
+ *   adu_device.buflock: A spinlock, covers what IRQs touch.
+ *   adutux_mutex:   A Static lock to cover open_count. It would also cover
+ *   any globals, but we don't have them in 2.6.
+ *   adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ *   It covers all of adu_device, except the open_count
+ *   and what .buflock covers.
+ */
+
 /* Structure to hold all of our device specific stuff */
 struct adu_device {
-   struct mutexmtx; /* locks this structure */
+   struct mutexmtx;
struct usb_device*  udev; /* save off the usb device pointer */
struct usb_interface*   interface;
-   unsigned char   minor; /* the starting minor number for this 
device */
+   unsigned intminor; /* the starting minor number for this 
device */
charserial_number[8];
 
int open_count; /* number of times this port has 
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char*   interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+   int out_urb_finished;
 };
 
+static DEFINE_MUTEX(adutux_mutex);
+
 static struct usb_driver adu_driver;
 
 static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, const char 
*function, int size,
  */
 static void adu_abort_transfers(struct adu_device *dev)
 {
-   dbg(2, %s : enter, __FUNCTION__);
+   unsigned long flags;
 
-   if (dev == NULL) {
-   dbg(1, %s : dev is null, __FUNCTION__);
-   goto exit;
-   }
+   dbg(2, %s : enter, __FUNCTION__);
 
if (dev-udev == NULL) {
dbg(1, %s : udev is null, __FUNCTION__);
goto exit;
}
 
-   dbg(2, %s : udev state %d, __FUNCTION__, dev-udev-state);
-   if (dev-udev-state == USB_STATE_NOTATTACHED) {
-   dbg(1, %s : udev is not attached, __FUNCTION__);
-   goto exit;
-   }
-
/* shutdown transfer */
-   usb_unlink_urb(dev-interrupt_in_urb);
-   usb_unlink_urb(dev-interrupt_out_urb);
+
+   /* XXX Anchor these instead */
+   spin_lock_irqsave(dev-buflock, flags);
+   if (!dev-read_urb_finished) {
+   spin_unlock_irqrestore(dev-buflock, flags);
+   usb_kill_urb(dev-interrupt_in_urb);
+   } else
+   spin_unlock_irqrestore(dev-buflock, flags);
+
+   spin_lock_irqsave(dev-buflock, flags);
+   if (!dev-out_urb_finished) {
+   spin_unlock_irqrestore(dev-buflock, flags);
+   usb_kill_urb(dev-interrupt_out_urb);
+   } else
+   spin_unlock_irqrestore(dev-buflock, flags);
 
 exit:
dbg(2, %s : leave, __FUNCTION__);
@@ -162,8 +179,6 @@ static void adu_delete(struct adu_device *dev)
 {
dbg(2, %s enter, __FUNCTION__);
 
-   adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev-interrupt_in_urb);
usb_free_urb(dev-interrupt_out_urb);
@@ -239,7 +254,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
 
-   wake_up_interruptible(dev-write_wait);
+   spin_lock(dev-buflock);
+   dev-out_urb_finished = 1;
+   wake_up(dev-write_wait);
+   spin_unlock(dev-buflock);
 exit:
 
adu_debug_data(5, __FUNCTION__, urb-actual_length,
@@ -252,12 +270,17 @@ static int adu_open(struct inode *inode, struct file 
*file)
struct adu_device *dev = NULL;
struct usb_interface *interface;
int subminor;
-   int retval = 0;
+   int retval;
 
dbg(2,%s : enter, __FUNCTION__);
 
subminor = iminor(inode);
 
+   if ((retval = mutex_lock_interruptible(adutux_mutex)))