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.
Signed-off-by: Pete Zaitcev <[email protected]>
---
If someone with a real device tested this, it would be great. Please
make sure to pull the plug while application is opening/closing.
A critical review is welcome too.
Vitaliy, this is the static lock example, which I promised on Friday.
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..a8afb66 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 mutex mtx; /* locks this structure */
+ struct mutex mtx;
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 int minor; /* the starting minor number for this device */
char serial_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,29 +145,36 @@ static void adu_debug_data(int level, const char *function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
+ unsigned long flags;
+
dbg(2," %s : enter", __FUNCTION__);
- if (dev == NULL) {
- dbg(1," %s : dev is null", __FUNCTION__);
- goto exit;
- }
+ mutex_lock(&dev->mtx);
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:
+ mutex_unlock(&dev->mtx);
dbg(2," %s : leave", __FUNCTION__);
}
@@ -162,8 +182,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 +257,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
+ spin_lock(&dev->buflock);
+ dev->out_urb_finished = 1;
wake_up_interruptible(&dev->write_wait);
+ spin_unlock(&dev->buflock);
exit:
adu_debug_data(5, __FUNCTION__, urb->actual_length,
@@ -272,13 +293,11 @@ static int adu_open(struct inode *inode, struct file *file)
goto exit_no_device;
}
- /* lock this device */
- if ((retval = mutex_lock_interruptible(&dev->mtx))) {
+ if ((retval = mutex_lock_interruptible(&adutux_mutex))) {
dbg(2, "%s : mutex lock failed", __FUNCTION__);
goto exit_no_device;
}
- /* increment our usage count for the device */
++dev->open_count;
dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count);
@@ -297,13 +316,14 @@ static int adu_open(struct inode *inode, struct file *file)
le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
adu_interrupt_in_callback, dev,
dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
dev->read_urb_finished = 0;
retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
- if (retval)
+ if (retval) {
+ dev->read_urb_finished = 1;
--dev->open_count;
+ }
}
- mutex_unlock(&dev->mtx);
+ mutex_unlock(&adutux_mutex);
exit_no_device:
dbg(2,"%s : leave, return value %d ", __FUNCTION__, retval);
@@ -311,10 +331,8 @@ exit_no_device:
return retval;
}
-static int adu_release_internal(struct adu_device *dev)
+static void adu_release_internal(struct adu_device *dev)
{
- int retval = 0;
-
dbg(2," %s : enter", __FUNCTION__);
/* decrement our usage count for the device */
@@ -326,12 +344,11 @@ static int adu_release_internal(struct adu_device *dev)
}
dbg(2," %s : leave", __FUNCTION__);
- return retval;
}
static int adu_release(struct inode *inode, struct file *file)
{
- struct adu_device *dev = NULL;
+ struct adu_device *dev;
int retval = 0;
dbg(2," %s : enter", __FUNCTION__);
@@ -343,15 +360,13 @@ static int adu_release(struct inode *inode, struct file *file)
}
dev = file->private_data;
-
if (dev == NULL) {
dbg(1," %s : object is NULL", __FUNCTION__);
retval = -ENODEV;
goto exit;
}
- /* lock our device */
- mutex_lock(&dev->mtx); /* not interruptible */
+ mutex_lock(&adutux_mutex); /* not interruptible */
if (dev->open_count <= 0) {
dbg(1," %s : device not opened", __FUNCTION__);
@@ -359,19 +374,15 @@ static int adu_release(struct inode *inode, struct file *file)
goto exit;
}
+ adu_release_internal(dev);
if (dev->udev == NULL) {
/* the device was unplugged before the file was released */
- mutex_unlock(&dev->mtx);
- adu_delete(dev);
- dev = NULL;
- } else {
- /* do the work */
- retval = adu_release_internal(dev);
+ if (!dev->open_count) /* ... and we're the last user */
+ adu_delete(dev);
}
exit:
- if (dev)
- mutex_unlock(&dev->mtx);
+ mutex_unlock(&adutux_mutex);
dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
return retval;
}
@@ -393,12 +404,12 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
dev = file->private_data;
dbg(2," %s : dev=%p", __FUNCTION__, dev);
- /* lock this object */
+
if (mutex_lock_interruptible(&dev->mtx))
return -ERESTARTSYS;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL || dev->minor == 0) {
+ if (dev->udev == NULL) {
retval = -ENODEV;
err("No device or device unplugged %d", retval);
goto exit;
@@ -452,7 +463,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
should_submit = 1;
} else {
/* even the primary was empty - we may need to do IO */
- if (dev->interrupt_in_urb->status == -EINPROGRESS) {
+ if (!dev->read_urb_finished) {
/* somebody is doing IO */
spin_unlock_irqrestore(&dev->buflock, flags);
dbg(2," %s : submitted already", __FUNCTION__);
@@ -460,6 +471,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
/* we must initiate input */
dbg(2," %s : initiate input", __FUNCTION__);
dev->read_urb_finished = 0;
+ spin_unlock_irqrestore(&dev->buflock, flags);
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
@@ -470,14 +482,11 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
dev,
dev->interrupt_in_endpoint->bInterval);
retval = usb_submit_urb(dev->interrupt_in_urb, GFP_ATOMIC);
- if (!retval) {
- spin_unlock_irqrestore(&dev->buflock, flags);
- dbg(2," %s : submitted OK", __FUNCTION__);
- } else {
+ if (retval) {
+ dev->read_urb_finished = 1;
if (retval == -ENOMEM) {
retval = bytes_read ? bytes_read : -ENOMEM;
}
- spin_unlock_irqrestore(&dev->buflock, flags);
dbg(2," %s : submit failed", __FUNCTION__);
goto exit;
}
@@ -486,10 +495,14 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
/* we wait for I/O to complete */
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(&dev->read_wait, &wait);
- if (!dev->read_urb_finished)
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(&dev->buflock, flags);
timeout = schedule_timeout(COMMAND_TIMEOUT);
- else
+ } else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
set_current_state(TASK_RUNNING);
+ }
remove_wait_queue(&dev->read_wait, &wait);
if (timeout <= 0) {
@@ -509,19 +522,23 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
retval = bytes_read;
/* if the primary buffer is empty then use it */
- if (should_submit && !dev->interrupt_in_urb->status==-EINPROGRESS) {
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (should_submit && dev->read_urb_finished) {
+ dev->read_urb_finished = 0;
+ spin_unlock_irqrestore(&dev->buflock, flags);
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
dev->interrupt_in_endpoint->bEndpointAddress),
- dev->interrupt_in_buffer,
- le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
- adu_interrupt_in_callback,
- dev,
- dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
- dev->read_urb_finished = 0;
- usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback,
+ dev,
+ dev->interrupt_in_endpoint->bInterval);
+ if (usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL) != 0)
+ dev->read_urb_finished = 1;
/* we ignore failure */
+ } else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
}
exit:
@@ -539,6 +556,7 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
size_t bytes_written = 0;
size_t bytes_to_write;
size_t buffer_size;
+ unsigned long flags;
int retval;
int timeout = 0;
@@ -546,13 +564,12 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
dev = file->private_data;
- /* lock this object */
retval = mutex_lock_interruptible(&dev->mtx);
if (retval)
goto exit_nolock;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL || dev->minor == 0) {
+ if (dev->udev == NULL) {
retval = -ENODEV;
err("No device or device unplugged %d", retval);
goto exit;
@@ -564,42 +581,43 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
goto exit;
}
-
while (count > 0) {
- if (dev->interrupt_out_urb->status == -EINPROGRESS) {
- timeout = COMMAND_TIMEOUT;
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(&dev->buflock, flags);
+ timeout = COMMAND_TIMEOUT;
while (timeout > 0) {
if (signal_pending(current)) {
- dbg(1," %s : interrupted", __FUNCTION__);
- retval = -EINTR;
- goto exit;
- }
- mutex_unlock(&dev->mtx);
- timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
- retval = mutex_lock_interruptible(&dev->mtx);
- if (retval) {
- retval = bytes_written ? bytes_written : retval;
- goto exit_nolock;
- }
- if (timeout > 0) {
- break;
+ dbg(1," %s : interrupted", __FUNCTION__);
+ retval = -EINTR;
+ goto exit;
+ }
+ mutex_unlock(&dev->mtx);
+ timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
+ retval = mutex_lock_interruptible(&dev->mtx);
+ if (retval) {
+ retval = bytes_written ? bytes_written : retval;
+ goto exit_nolock;
+ }
+ if (timeout > 0) {
+ break;
+ }
+ dbg(1," %s : interrupted timeout: %d", __FUNCTION__, timeout);
}
- dbg(1," %s : interrupted timeout: %d", __FUNCTION__, timeout);
- }
+ dbg(1," %s : final timeout: %d", __FUNCTION__, timeout);
- dbg(1," %s : final timeout: %d", __FUNCTION__, timeout);
-
- if (timeout == 0) {
- dbg(1, "%s - command timed out.", __FUNCTION__);
- retval = -ETIMEDOUT;
- goto exit;
- }
+ if (timeout == 0) {
+ dbg(1, "%s - command timed out.", __FUNCTION__);
+ retval = -ETIMEDOUT;
+ goto exit;
+ }
- dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
+ dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
} else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
dbg(4," %s : sending, count = %Zd", __FUNCTION__, count);
/* write the data into interrupt_out_buffer from userspace */
@@ -623,10 +641,11 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
adu_interrupt_out_callback,
dev,
dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
dev->interrupt_out_urb->actual_length = bytes_to_write;
+ dev->out_urb_finished = 0;
retval = usb_submit_urb(dev->interrupt_out_urb, GFP_KERNEL);
if (retval < 0) {
+ dev->out_urb_finished = 1;
err("Couldn't submit interrupt_out_urb %d", retval);
goto exit;
}
@@ -641,7 +660,6 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
retval = bytes_written;
exit:
- /* unlock the device */
mutex_unlock(&dev->mtx);
exit_nolock:
@@ -831,25 +849,27 @@ static void adu_disconnect(struct usb_interface *interface)
dbg(2," %s : enter", __FUNCTION__);
dev = usb_get_intfdata(interface);
+
+ mutex_lock(&dev->mtx); /* not interruptible */
+
+ dev->udev = NULL;
usb_set_intfdata(interface, NULL);
minor = dev->minor;
/* give back our minor */
usb_deregister_dev(interface, &adu_class);
- dev->minor = 0;
- mutex_lock(&dev->mtx); /* not interruptible */
+ mutex_unlock(&dev->mtx);
+
+ mutex_lock(&adutux_mutex);
/* if the device is not opened, then we clean up right now */
dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
- if (!dev->open_count) {
- mutex_unlock(&dev->mtx);
+ if (!dev->open_count)
adu_delete(dev);
- } else {
- dev->udev = NULL;
- mutex_unlock(&dev->mtx);
- }
+
+ mutex_unlock(&adutux_mutex);
dev_info(&interface->dev, "ADU device adutux%d now disconnected\n",
(minor - ADU_MINOR_BASE));
Am Dienstag 23 Oktober 2007 schrieb 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
> + /* 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);
Why bother? Simply call usb_kill_urb() unconditionally.
> exit:
> + mutex_unlock(&dev->mtx);
> dbg(2," %s : leave", __FUNCTION__);
> }
>
> @@ -162,8 +182,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 +257,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
> goto exit;
> }
>
> + spin_lock(&dev->buflock);
> + dev->out_urb_finished = 1;
> wake_up_interruptible(&dev->write_wait);
> + spin_unlock(&dev->buflock);
This leaves a race here:
while (count > 0) {
spin_lock_irqsave(&dev->buflock, flags);
if (!dev->out_urb_finished) {
spin_unlock_irqrestore(&dev->buflock, flags);
timeout = COMMAND_TIMEOUT;
while (timeout > 0) {
if (signal_pending(current)) {
dbg(1," %s : interrupted", __FUNCTION__);
retval = -EINTR;
goto exit;
}
mutex_unlock(&dev->mtx);
timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
You can detect that the urb has not finished but the notification happens before
you go to sleep.
> exit:
>
> adu_debug_data(5, __FUNCTION__, urb->actual_length,
> @@ -272,13 +293,11 @@ static int adu_open(struct inode *inode, struct file *file)
> goto exit_no_device;
> }
>
> - /* lock this device */
> - if ((retval = mutex_lock_interruptible(&dev->mtx))) {
> + if ((retval = mutex_lock_interruptible(&adutux_mutex))) {
> dbg(2, "%s : mutex lock failed", __FUNCTION__);
> goto exit_no_device;
> }
This is racy:
dev = usb_get_intfdata(interface);
if (!dev) {
retval = -ENODEV;
goto exit_no_device;
}
if ((retval = mutex_lock_interruptible(&adutux_mutex))) {
dbg(2, "%s : mutex lock failed", __FUNCTION__);
goto exit_no_device;
}
You need to manipulate intfdata under lock, or disconnect will
happily free the datastructures.
>
> - /* increment our usage count for the device */
> ++dev->open_count;
> dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count);
>
> @@ -297,13 +316,14 @@ static int adu_open(struct inode *inode, struct file *file)
> le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
> adu_interrupt_in_callback, dev,
> dev->interrupt_in_endpoint->bInterval);
> - /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
> dev->read_urb_finished = 0;
> retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
> - if (retval)
> + if (retval) {
> + dev->read_urb_finished = 1;
> --dev->open_count;
> + }
You should set file->private_data to NULL in the error case.
[..]
> @@ -486,10 +495,14 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
> /* we wait for I/O to complete */
> set_current_state(TASK_INTERRUPTIBLE);
> add_wait_queue(&dev->read_wait, &wait);
> - if (!dev->read_urb_finished)
> + spin_lock_irqsave(&dev->buflock, flags);
> + if (!dev->read_urb_finished) {
> + spin_unlock_irqrestore(&dev->buflock, flags);
> timeout = schedule_timeout(COMMAND_TIMEOUT);
I find it a bit silly to bother with _irqsave if you call schedule_timeout() in the
next line.
Regards
Oliver
On Tue, 23 Oct 2007 11:38:37 +0200, Oliver Neukum <[email protected]> wrote:
> > + /* 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);
> Why bother? Simply call usb_kill_urb() unconditionally.
Is it always safe to kill unfilled URBs? The filled but unsubmitted ones
are ok, but in this case it's possible that we only allocated something
but never submitted. Our current implementation happens to be safe by
virtue of ->dev being NULL in such case. I do not remember if we always
guaranteed that and since Vitaly is going to take this code for a
backport, I decided to play it safe.
I would rather leave this.
> > @@ -162,8 +182,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 +257,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
> > goto exit;
> > }
> >
> > + spin_lock(&dev->buflock);
> > + dev->out_urb_finished = 1;
> > wake_up_interruptible(&dev->write_wait);
> > + spin_unlock(&dev->buflock);
>
> This leaves a race here:
>
> while (count > 0) {
> spin_lock_irqsave(&dev->buflock, flags);
> if (!dev->out_urb_finished) {
> spin_unlock_irqrestore(&dev->buflock, flags);
>
> timeout = COMMAND_TIMEOUT;
> while (timeout > 0) {
> if (signal_pending(current)) {
> dbg(1," %s : interrupted", __FUNCTION__);
> retval = -EINTR;
> goto exit;
> }
> mutex_unlock(&dev->mtx);
> timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
>
> You can detect that the urb has not finished but the notification happens before
> you go to sleep.
That's a common issue with sleep_on and its derivatives really.
I would need to replace it with explicit queue adds to fix.
I suppose I can fix it too.
> > @@ -272,13 +293,11 @@ static int adu_open(struct inode *inode, struct file *file)
> > goto exit_no_device;
> > }
> >
> > - /* lock this device */
> > - if ((retval = mutex_lock_interruptible(&dev->mtx))) {
> > + if ((retval = mutex_lock_interruptible(&adutux_mutex))) {
> > dbg(2, "%s : mutex lock failed", __FUNCTION__);
> > goto exit_no_device;
> > }
> This is racy:
> dev = usb_get_intfdata(interface);
> if ((retval = mutex_lock_interruptible(&adutux_mutex))) {
> dbg(2, "%s : mutex lock failed", __FUNCTION__);
> goto exit_no_device;
> }
>
> You need to manipulate intfdata under lock, or disconnect will
> happily free the datastructures.
Ah yes. Sorry about that, will fix.
> > @@ -297,13 +316,14 @@ static int adu_open(struct inode *inode, struct file *file)
> > dev->read_urb_finished = 0;
> > retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
> > - if (retval)
> > + if (retval) {
> > + dev->read_urb_finished = 1;
> > --dev->open_count;
> > + }
>
> You should set file->private_data to NULL in the error case.
What for? Nobody ever tests it or dereferences it after ->open returned
an error. I can set 0xaaaabbbb to it just as well.
> > @@ -486,10 +495,14 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
> > /* we wait for I/O to complete */
> > set_current_state(TASK_INTERRUPTIBLE);
> > add_wait_queue(&dev->read_wait, &wait);
> > - if (!dev->read_urb_finished)
> > + spin_lock_irqsave(&dev->buflock, flags);
> > + if (!dev->read_urb_finished) {
> > + spin_unlock_irqrestore(&dev->buflock, flags);
> > timeout = schedule_timeout(COMMAND_TIMEOUT);
>
> I find it a bit silly to bother with _irqsave if you call schedule_timeout() in the
> next line.
True, this is not really necessary... And I set and clear such flags
without locking around them when I'm sure that URB is not in progress.
I just added it in case someone wants to expand this code by testing
two things together or something, because here the callback certainly
can strike at any time. The whole excercise started because Vitaliy
wanted to reuse the code.
-- Pete
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.
- Remove sleep_on.
Signed-off-by: Pete Zaitcev <[email protected]>
---
Oliver, thanks for the inftdata catch. I also fixed the sleep_on.
But I left the silly dances around usb_kill_urb in, at least for now.
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..d278161 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 mutex mtx; /* locks this structure */
+ struct mutex mtx;
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 int minor; /* the starting minor number for this device */
char serial_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,29 +145,36 @@ static void adu_debug_data(int level, const char *function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
+ unsigned long flags;
+
dbg(2," %s : enter", __FUNCTION__);
- if (dev == NULL) {
- dbg(1," %s : dev is null", __FUNCTION__);
- goto exit;
- }
+ mutex_lock(&dev->mtx);
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:
+ mutex_unlock(&dev->mtx);
dbg(2," %s : leave", __FUNCTION__);
}
@@ -162,8 +182,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 +257,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 +273,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))) {
+ dbg(2, "%s : mutex lock failed", __FUNCTION__);
+ goto exit_no_lock;
+ }
+
interface = usb_find_interface(&adu_driver, subminor);
if (!interface) {
err("%s - error, can't find device for minor %d",
@@ -272,13 +298,6 @@ static int adu_open(struct inode *inode, struct file *file)
goto exit_no_device;
}
- /* lock this device */
- if ((retval = mutex_lock_interruptible(&dev->mtx))) {
- dbg(2, "%s : mutex lock failed", __FUNCTION__);
- goto exit_no_device;
- }
-
- /* increment our usage count for the device */
++dev->open_count;
dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count);
@@ -297,24 +316,25 @@ static int adu_open(struct inode *inode, struct file *file)
le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
adu_interrupt_in_callback, dev,
dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
dev->read_urb_finished = 0;
retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
- if (retval)
+ if (retval) {
+ dev->read_urb_finished = 1;
--dev->open_count;
+ }
}
- mutex_unlock(&dev->mtx);
+
+ retval = 0;
exit_no_device:
+ mutex_unlock(&adutux_mutex);
+exit_no_lock:
dbg(2,"%s : leave, return value %d ", __FUNCTION__, retval);
-
return retval;
}
-static int adu_release_internal(struct adu_device *dev)
+static void adu_release_internal(struct adu_device *dev)
{
- int retval = 0;
-
dbg(2," %s : enter", __FUNCTION__);
/* decrement our usage count for the device */
@@ -326,12 +346,11 @@ static int adu_release_internal(struct adu_device *dev)
}
dbg(2," %s : leave", __FUNCTION__);
- return retval;
}
static int adu_release(struct inode *inode, struct file *file)
{
- struct adu_device *dev = NULL;
+ struct adu_device *dev;
int retval = 0;
dbg(2," %s : enter", __FUNCTION__);
@@ -343,15 +362,13 @@ static int adu_release(struct inode *inode, struct file *file)
}
dev = file->private_data;
-
if (dev == NULL) {
dbg(1," %s : object is NULL", __FUNCTION__);
retval = -ENODEV;
goto exit;
}
- /* lock our device */
- mutex_lock(&dev->mtx); /* not interruptible */
+ mutex_lock(&adutux_mutex); /* not interruptible */
if (dev->open_count <= 0) {
dbg(1," %s : device not opened", __FUNCTION__);
@@ -359,19 +376,15 @@ static int adu_release(struct inode *inode, struct file *file)
goto exit;
}
+ adu_release_internal(dev);
if (dev->udev == NULL) {
/* the device was unplugged before the file was released */
- mutex_unlock(&dev->mtx);
- adu_delete(dev);
- dev = NULL;
- } else {
- /* do the work */
- retval = adu_release_internal(dev);
+ if (!dev->open_count) /* ... and we're the last user */
+ adu_delete(dev);
}
exit:
- if (dev)
- mutex_unlock(&dev->mtx);
+ mutex_unlock(&adutux_mutex);
dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
return retval;
}
@@ -393,12 +406,12 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
dev = file->private_data;
dbg(2," %s : dev=%p", __FUNCTION__, dev);
- /* lock this object */
+
if (mutex_lock_interruptible(&dev->mtx))
return -ERESTARTSYS;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL || dev->minor == 0) {
+ if (dev->udev == NULL) {
retval = -ENODEV;
err("No device or device unplugged %d", retval);
goto exit;
@@ -452,7 +465,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
should_submit = 1;
} else {
/* even the primary was empty - we may need to do IO */
- if (dev->interrupt_in_urb->status == -EINPROGRESS) {
+ if (!dev->read_urb_finished) {
/* somebody is doing IO */
spin_unlock_irqrestore(&dev->buflock, flags);
dbg(2," %s : submitted already", __FUNCTION__);
@@ -460,6 +473,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
/* we must initiate input */
dbg(2," %s : initiate input", __FUNCTION__);
dev->read_urb_finished = 0;
+ spin_unlock_irqrestore(&dev->buflock, flags);
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
@@ -470,14 +484,11 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
dev,
dev->interrupt_in_endpoint->bInterval);
retval = usb_submit_urb(dev->interrupt_in_urb, GFP_ATOMIC);
- if (!retval) {
- spin_unlock_irqrestore(&dev->buflock, flags);
- dbg(2," %s : submitted OK", __FUNCTION__);
- } else {
+ if (retval) {
+ dev->read_urb_finished = 1;
if (retval == -ENOMEM) {
retval = bytes_read ? bytes_read : -ENOMEM;
}
- spin_unlock_irqrestore(&dev->buflock, flags);
dbg(2," %s : submit failed", __FUNCTION__);
goto exit;
}
@@ -486,10 +497,14 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
/* we wait for I/O to complete */
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(&dev->read_wait, &wait);
- if (!dev->read_urb_finished)
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(&dev->buflock, flags);
timeout = schedule_timeout(COMMAND_TIMEOUT);
- else
+ } else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
set_current_state(TASK_RUNNING);
+ }
remove_wait_queue(&dev->read_wait, &wait);
if (timeout <= 0) {
@@ -509,19 +524,23 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
retval = bytes_read;
/* if the primary buffer is empty then use it */
- if (should_submit && !dev->interrupt_in_urb->status==-EINPROGRESS) {
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (should_submit && dev->read_urb_finished) {
+ dev->read_urb_finished = 0;
+ spin_unlock_irqrestore(&dev->buflock, flags);
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
dev->interrupt_in_endpoint->bEndpointAddress),
- dev->interrupt_in_buffer,
- le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
- adu_interrupt_in_callback,
- dev,
- dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
- dev->read_urb_finished = 0;
- usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback,
+ dev,
+ dev->interrupt_in_endpoint->bInterval);
+ if (usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL) != 0)
+ dev->read_urb_finished = 1;
/* we ignore failure */
+ } else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
}
exit:
@@ -535,24 +554,24 @@ exit:
static ssize_t adu_write(struct file *file, const __user char *buffer,
size_t count, loff_t *ppos)
{
+ DECLARE_WAITQUEUE(waita, current);
struct adu_device *dev;
size_t bytes_written = 0;
size_t bytes_to_write;
size_t buffer_size;
+ unsigned long flags;
int retval;
- int timeout = 0;
dbg(2," %s : enter, count = %Zd", __FUNCTION__, count);
dev = file->private_data;
- /* lock this object */
retval = mutex_lock_interruptible(&dev->mtx);
if (retval)
goto exit_nolock;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL || dev->minor == 0) {
+ if (dev->udev == NULL) {
retval = -ENODEV;
err("No device or device unplugged %d", retval);
goto exit;
@@ -564,42 +583,35 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
goto exit;
}
-
+ add_wait_queue(&dev->write_wait, &waita);
while (count > 0) {
- if (dev->interrupt_out_urb->status == -EINPROGRESS) {
- timeout = COMMAND_TIMEOUT;
+ set_current_state(TASK_INTERRUPTIBLE);
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(&dev->buflock, flags);
- while (timeout > 0) {
- if (signal_pending(current)) {
+ mutex_unlock(&dev->mtx);
+ if (signal_pending(current)) {
dbg(1," %s : interrupted", __FUNCTION__);
+ set_current_state(TASK_RUNNING);
retval = -EINTR;
- goto exit;
+ goto exit_nolock;
+ }
+ if (schedule_timeout(COMMAND_TIMEOUT) == 0) {
+ dbg(1, "%s - command timed out.", __FUNCTION__);
+ retval = -ETIMEDOUT;
+ goto exit_nolock;
}
- mutex_unlock(&dev->mtx);
- timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
retval = mutex_lock_interruptible(&dev->mtx);
if (retval) {
retval = bytes_written ? bytes_written : retval;
goto exit_nolock;
}
- if (timeout > 0) {
- break;
- }
- dbg(1," %s : interrupted timeout: %d", __FUNCTION__, timeout);
- }
-
-
- dbg(1," %s : final timeout: %d", __FUNCTION__, timeout);
-
- if (timeout == 0) {
- dbg(1, "%s - command timed out.", __FUNCTION__);
- retval = -ETIMEDOUT;
- goto exit;
- }
-
- dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
+ dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
} else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
+ set_current_state(TASK_RUNNING);
dbg(4," %s : sending, count = %Zd", __FUNCTION__, count);
/* write the data into interrupt_out_buffer from userspace */
@@ -623,10 +635,11 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
adu_interrupt_out_callback,
dev,
dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
dev->interrupt_out_urb->actual_length = bytes_to_write;
+ dev->out_urb_finished = 0;
retval = usb_submit_urb(dev->interrupt_out_urb, GFP_KERNEL);
if (retval < 0) {
+ dev->out_urb_finished = 1;
err("Couldn't submit interrupt_out_urb %d", retval);
goto exit;
}
@@ -637,11 +650,11 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
bytes_written += bytes_to_write;
}
}
+ remove_wait_queue(&dev->write_wait, &waita);
retval = bytes_written;
exit:
- /* unlock the device */
mutex_unlock(&dev->mtx);
exit_nolock:
@@ -831,25 +844,22 @@ static void adu_disconnect(struct usb_interface *interface)
dbg(2," %s : enter", __FUNCTION__);
dev = usb_get_intfdata(interface);
- usb_set_intfdata(interface, NULL);
+ mutex_lock(&dev->mtx); /* not interruptible */
+ dev->udev = NULL; /* poison */
minor = dev->minor;
-
- /* give back our minor */
usb_deregister_dev(interface, &adu_class);
- dev->minor = 0;
+ mutex_unlock(&dev->mtx);
- mutex_lock(&dev->mtx); /* not interruptible */
+ mutex_lock(&adutux_mutex);
+ usb_set_intfdata(interface, NULL);
/* if the device is not opened, then we clean up right now */
dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
- if (!dev->open_count) {
- mutex_unlock(&dev->mtx);
+ if (!dev->open_count)
adu_delete(dev);
- } else {
- dev->udev = NULL;
- mutex_unlock(&dev->mtx);
- }
+
+ mutex_unlock(&adutux_mutex);
dev_info(&interface->dev, "ADU device adutux%d now disconnected\n",
(minor - ADU_MINOR_BASE));
Am Dienstag 23 Oktober 2007 schrieb Pete Zaitcev:
> On Tue, 23 Oct 2007 11:38:37 +0200, Oliver Neukum <[email protected]> wrote:
>
> > > +???/* 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);
>
> > Why bother? Simply call usb_kill_urb() unconditionally.
>
> Is it always safe to kill unfilled URBs? The filled but unsubmitted ones
> are ok, but in this case it's possible that we only allocated something
> but never submitted. Our current implementation happens to be safe by
> virtue of ->dev being NULL in such case. I do not remember if we always
> guaranteed that and since Vitaly is going to take this code for a
> backport, I decided to play it safe.
I am not sure as far as 2.4 is concerned. In fact I am not sure 2.4 has
usb_kill_urb() at all.
Regards
Oliver
Pete,
On Wed, 2007-10-24 at 04:53, Pete Zaitcev wrote:
1)
> +/*
> + * 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.
> + */
> static void adu_abort_transfers(struct adu_device *dev)
> {
...
> + mutex_lock(&dev->mtx);
...
> + mutex_unlock(&dev->mtx);
> }
Don't you think it's needed? You call adu_abort_transfers from adu_release only where it's locked by adutux_mutex.
2)
Also one issue. adu_write has:
/* send off the urb */
usb_fill_int_urb(
dev->interrupt_out_urb,
dev->udev,
usb_sndintpipe(dev->udev, dev->interrupt_out_endpoint->bEndpointAddress),
dev->interrupt_out_buffer,
bytes_to_write,
adu_interrupt_out_callback,
dev,
dev->interrupt_in_endpoint->bInterval);
Seems like there should be not:
dev->interrupt_in_endpoint->bInterval
but:
dev->interrupt_out_endpoint->bInterval
Typo? Seems like that.
I just tried your patch on the adutux from 2.6.24-rc1 but on my 2.6.20.7. It failed with the following message:
-----------------
[176188.466898] drivers/usb/misc/adutux.c : adu_open : enter
[176188.468000] drivers/usb/misc/adutux.c : adu_open : open count 1
[176188.468601] drivers/usb/misc/adutux.c : adu_open : leave, return value 0
[176188.486218] drivers/usb/misc/adutux.c : adu_interrupt_in_callback : enter, status 0
[176188.486269] drivers/usb/misc/adutux.c: adu_interrupt_in_callback - length = 8, data = 00 00 00 00 00 00 00 00
[176188.486430] drivers/usb/misc/adutux.c: adu_interrupt_in_callback - length = 8, data = 00 00 00 00 00 00 00 00
[176188.486482] drivers/usb/misc/adutux.c : adu_interrupt_in_callback : leave, status 0
[176191.303853] drivers/usb/misc/adutux.c : adu_write : enter, count = 8
[176193.301872] drivers/usb/misc/adutux.c : adu_write - command timed out.
[176193.301912] drivers/usb/misc/adutux.c : adu_write : leave, return value -110
[176200.425157] drivers/usb/misc/adutux.c : adu_write : enter, count = 8
[176200.425776] list_add corruption. next->prev should be prev (c15e6c30), but was 00000000. (next=c58bdf5c).
[176200.425776] list_add corruption. next->prev should be prev (c15e6c30), but was 00000000. (next=c58bdf5c).
[176200.426475] ------------[ cut here ]------------
[176200.426475] ------------[ cut here ]------------
[176200.426536] kernel BUG at lib/list_debug.c:27!
[176200.426536] kernel BUG at lib/list_debug.c:28!
[176200.426647] invalid opcode: 0000 [#1]
[176200.426647] invalid opcode: 0000 [#1]
[176200.426736] Modules linked in: adutux(F) ppdev autofs4 hidp rfcomm l2cap bluetooth sunrpc xt_tcpudp iptable_filter ip_tables x_tables dm_multipath video button battery asus_acpi backlight ac ipv6 lp parport_pc parport floppy nvram uhci_hcd sg snd_ens1371 gameport snd_rawmidi snd_ac97_codec ac97_bus snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss snd_mixer_oss snd_pcm pcnet32 snd_timer snd pcspkr mii soundcore i2c_piix4 i2c_core snd_page_alloc dm_snapshot dm_zero dm_mirror dm_mod ext3 jbd mptspi scsi_transport_spi mptscsih sd_mod scsi_mod mptbase
[176200.426736] Modules linked in: adutux(F) ppdev autofs4 hidp rfcomm l2cap bluetooth sunrpc xt_tcpudp iptable_filter ip_tables x_tables dm_multipath video button battery asus_acpi backlight ac ipv6 lp parport_pc parport floppy nvram uhci_hcd sg snd_ens1371 gameport snd_rawmidi snd_ac97_codec ac97_bus snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss snd_mixer_oss snd_pcm pcnet32 snd_timer snd pcspkr mii soundcore i2c_piix4 i2c_core snd_page_alloc dm_snapshot dm_zero dm_mirror dm_mod ext3 jbd mptspi scsi_transport_spi mptscsih sd_mod scsi_mod mptbase
[176200.427025] CPU: 0
[176200.427025] CPU: 0
[176200.427027] EIP: 0060:[<c01e31a2>] Tainted: GF VLI
[176200.427027] EIP: 0060:[<c01e31a2>] Tainted: GF VLI
[176200.427029] EFLAGS: 00010082 (2.6.20.7 #1)
[176200.427029] EFLAGS: 00010082 (2.6.20.7 #1)
[176200.427405] EIP is at __list_add+0x29/0x60
[176200.427405] EIP is at __list_add+0x29/0x60
[176200.427448] eax: 00000071 ebx: c58bdf5c ecx: c0117787 edx: d10e0ab0
[176200.427448] eax: 00000071 ebx: c58bdf5c ecx: c0117787 edx: d10e0ab0
[176200.427481] esi: c15e6c08 edi: 00000296 ebp: c58bded8 esp: c58bdec0
[176200.427481] esi: c15e6c08 edi: 00000296 ebp: c58bded8 esp: c58bdec0
[176200.427503] ds: 007b es: 007b ss: 0068
[176200.427503] ds: 007b es: 007b ss: 0068
[176200.427532] Process aducmd (pid: 14413, ti=c58bd000 task=d10e0ab0 task.ti=c58bd000)
[176200.427532] Process aducmd (pid: 14413, ti=c58bd000 task=d10e0ab0 task.ti=c58bd000)
[176200.427555] Stack: c03b2a10 c15e6c30 00000000 c58bdf5c c58bdf50 c15e6c08 c58bdee0 c01e31e3
[176200.427555] Stack: c03b2a10 c15e6c30 00000000 c58bdf5c c58bdf50 c15e6c08 c58bdee0 c01e31e3
[176200.427647] c58bdef4 c012bc4b 00000000 c58bdf50 c58bdf44 c58bdf70 e03437a0 e0344ebb
[176200.427647] c58bdef4 c012bc4b 00000000 c58bdf50 c58bdf44 c58bdf70 e03437a0 e0344ebb
[176200.427662] e0344950 00000008 c02280a9 c58bdf20 c58bdf40 00000008 bf8aceef c15e6b38
[176200.427662] e0344950 00000008 c02280a9 c58bdf20 c58bdf40 00000008 bf8aceef c15e6b38
[176200.427687] Call Trace:
[176200.427687] Call Trace:
[176200.427739] [<c0103be8>] show_trace_log_lvl+0x1a/0x2f
[176200.427739] [<c0103be8>] show_trace_log_lvl+0x1a/0x2f
[176200.427780] [<c0103c98>] show_stack_log_lvl+0x9b/0xa3
[176200.427780] [<c0103c98>] show_stack_log_lvl+0x9b/0xa3
[176200.427788] [<c0103e32>] show_registers+0x192/0x268
[176200.427788] [<c0103e32>] show_registers+0x192/0x268
[176200.427793] [<c0103ff7>] die+0xef/0x204
[176200.427793] [<c0103ff7>] die+0xef/0x204
[176200.427798] [<c03167cf>] do_trap+0x79/0x91
[176200.427798] [<c03167cf>] do_trap+0x79/0x91
[176200.427833] [<c010459c>] do_invalid_op+0x97/0xa1
[176200.427833] [<c010459c>] do_invalid_op+0x97/0xa1
[176200.427838] [<c03165ac>] error_code+0x74/0x7c
[176200.427838] [<c03165ac>] error_code+0x74/0x7c
[176200.427843] [<c01e31e3>] list_add+0xa/0xf
[176200.427843] [<c01e31e3>] list_add+0xa/0xf
[176200.427848] [<c012bc4b>] add_wait_queue+0x1f/0x2d
[176200.427848] [<c012bc4b>] add_wait_queue+0x1f/0x2d
[176200.427857] [<e03437a0>] adu_write+0xef/0x3f9 [adutux]
[176200.427857] [<e03437a0>] adu_write+0xef/0x3f9 [adutux]
[176200.428151] [<c01636f9>] vfs_write+0xaf/0x163
[176200.428151] [<c01636f9>] vfs_write+0xaf/0x163
[176200.428184] [<c0163c45>] sys_write+0x3d/0x61
[176200.428184] [<c0163c45>] sys_write+0x3d/0x61
[176200.428189] [<c0102ce4>] syscall_call+0x7/0xb
[176200.428189] [<c0102ce4>] syscall_call+0x7/0xb
[176200.428231] =======================
[176200.428231] =======================
[176200.428270] Code: 5d c3 55 89 e5 56 53 89 c3 83 ec 10 8b 41 04 39 d0 74 1c 89 4c 24 0c 89 54 24 04 89 44 24 08 c7 04 24 10 2a 3b c0 e8 40 8b f3 ff <0f> 0b eb fe 8b 32 39 ce 74 1c 89 54 24 0c 89 74 24 08 89 4c 24
[176200.428270] Code: 5d c3 55 89 e5 56 53 89 c3 83 ec 10 8b 41 04 39 d0 74 1c 89 4c 24 0c 89 54 24 04 89 44 24 08 c7 04 24 10 2a 3b c0 e8 40 8b f3 ff <0f> 0b eb fe 8b 32 39 ce 74 1c 89 54 24 0c 89 74 24 08 89 4c 24
[176200.428270] EIP: [<c01e31a2>] __list_add+0x29/0x60 SS:ESP 0068:c58bdec0
[176200.428270] EIP: [<c01e31a2>] __list_add+0x29/0x60 SS:ESP 0068:c58bdec0
[176200.428593] <3>BUG: sleeping function called from invalid context at kernel/rwsem.c:20
[176200.428593] <3>BUG: sleeping function called from invalid context at kernel/rwsem.c:20
[176200.428798] in_atomic():0, irqs_disabled():1
[176200.428798] in_atomic():0, irqs_disabled():1
[176200.428847] no locks held by aducmd/14413.
[176200.428847] no locks held by aducmd/14413.
[176200.428918] irq event stamp: 0
[176200.428918] irq event stamp: 0
[176200.428939] hardirqs last enabled at (0): [<00000000>] 0x0
[176200.428939] hardirqs last enabled at (0): [<00000000>] 0x0
[176200.429038] hardirqs last disabled at (0): [<c0119746>] copy_process+0x2e5/0x1194
[176200.429038] hardirqs last disabled at (0): [<c0119746>] copy_process+0x2e5/0x1194
[176200.429094] softirqs last enabled at (0): [<c0119746>] copy_process+0x2e5/0x1194
[176200.429094] softirqs last enabled at (0): [<c0119746>] copy_process+0x2e5/0x1194
[176200.429148] softirqs last disabled at (0): [<00000000>] 0x0
[176200.429148] softirqs last disabled at (0): [<00000000>] 0x0
[176200.429231] [<c0103be8>] show_trace_log_lvl+0x1a/0x2f
[176200.429231] [<c0103be8>] show_trace_log_lvl+0x1a/0x2f
[176200.429239] [<c010414d>] show_trace+0x12/0x14
[176200.429239] [<c010414d>] show_trace+0x12/0x14
[176200.429244] [<c01041d1>] dump_stack+0x16/0x18
[176200.429244] [<c01041d1>] dump_stack+0x16/0x18
[176200.429249] [<c0117bca>] __might_sleep+0xc9/0xcf
[176200.429249] [<c0117bca>] __might_sleep+0xc9/0xcf
[176200.429254] [<c012e495>] down_read+0x18/0x50
[176200.429254] [<c012e495>] down_read+0x18/0x50
[176200.429262] [<c013e297>] acct_collect+0x3b/0x144
[176200.429262] [<c013e297>] acct_collect+0x3b/0x144
[176200.429271] [<c011daa5>] do_exit+0x1b6/0x707
[176200.429271] [<c011daa5>] do_exit+0x1b6/0x707
[176200.429276] [<c01040e6>] die+0x1de/0x204
[176200.429276] [<c01040e6>] die+0x1de/0x204
[176200.429281] [<c03167cf>] do_trap+0x79/0x91
[176200.429281] [<c03167cf>] do_trap+0x79/0x91
[176200.429286] [<c010459c>] do_invalid_op+0x97/0xa1
[176200.429286] [<c010459c>] do_invalid_op+0x97/0xa1
[176200.429291] [<c03165ac>] error_code+0x74/0x7c
[176200.429291] [<c03165ac>] error_code+0x74/0x7c
[176200.429295] [<c01e31e3>] list_add+0xa/0xf
[176200.429295] [<c01e31e3>] list_add+0xa/0xf
[176200.429301] [<c012bc4b>] add_wait_queue+0x1f/0x2d
[176200.429301] [<c012bc4b>] add_wait_queue+0x1f/0x2d
[176200.429306] [<e03437a0>] adu_write+0xef/0x3f9 [adutux]
[176200.429306] [<e03437a0>] adu_write+0xef/0x3f9 [adutux]
[176200.429313] [<c01636f9>] vfs_write+0xaf/0x163
[176200.429313] [<c01636f9>] vfs_write+0xaf/0x163
[176200.429318] [<c0163c45>] sys_write+0x3d/0x61
[176200.429318] [<c0163c45>] sys_write+0x3d/0x61
[176200.429323] [<c0102ce4>] syscall_call+0x7/0xb
[176200.429323] [<c0102ce4>] syscall_call+0x7/0xb
[176200.429328] =======================
-----------------
I'm not sure whether it's because I'm trying it on 2.6.20.7 and not on 2.6.24-rc1.
Will check it on 2.6.24-rc1 asap and let you know.
Best,
Vitaliy
Am Mittwoch 24 Oktober 2007 schrieb Pete Zaitcev:
> Oliver, thanks for the inftdata catch. I also fixed the sleep_on.
But you are leaving a function while still on a waitqueue left on the stack.
Here's a patch on top of yours.
Regards
Oliver
----
--- work/drivers/usb/misc/adutux.c.alt 2007-10-24 16:36:02.000000000 +0200
+++ work/drivers/usb/misc/adutux.c 2007-10-24 16:36:06.000000000 +0200
@@ -567,19 +567,21 @@ static ssize_t adu_write(struct file *fi
retval = mutex_lock_interruptible(&dev->mtx);
if (retval)
- goto exit_nolock;
+ goto exit_nolock_intr;
/* verify that the device wasn't unplugged */
if (dev->udev == NULL) {
+ mutex_unlock(&dev->mtx);
retval = -ENODEV;
err("No device or device unplugged %d", retval);
- goto exit;
+ goto exit_nolock_intr;
}
/* verify that we actually have some data to write */
if (count == 0) {
+ mutex_unlock(&dev->mtx);
dbg(1," %s : write request of 0 bytes", __FUNCTION__);
- goto exit;
+ goto exit_nolock_intr;
}
add_wait_queue(&dev->write_wait, &waita);
@@ -649,13 +651,14 @@ static ssize_t adu_write(struct file *fi
bytes_written += bytes_to_write;
}
}
- remove_wait_queue(&dev->write_wait, &waita);
retval = bytes_written;
exit:
mutex_unlock(&dev->mtx);
exit_nolock:
+ remove_wait_queue(&dev->write_wait, &waita);
+exit_nolock_intr:
dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
On Wed, Oct 24, 2007 at 04:49:12PM +0200, Oliver Neukum wrote:
> Am Mittwoch 24 Oktober 2007 schrieb Pete Zaitcev:
> > Oliver, thanks for the inftdata catch. I also fixed the sleep_on.
>
> But you are leaving a function while still on a waitqueue left on the stack.
> Here's a patch on top of yours.
Hm, I think I'll wait for Pete and you to agree and send me a final
version before applying anything here :)
thanks,
greg k-h
On Wed, 24 Oct 2007 17:09:47 +0300, Vitaliy Ivanov <[email protected]> wrote:
> > static void adu_abort_transfers(struct adu_device *dev)
> > {
> ...
> > + mutex_lock(&dev->mtx);
> ...
> > + mutex_unlock(&dev->mtx);
>
> > }
>
> Don't you think it's needed? You call adu_abort_transfers from adu_release only where it's locked by adutux_mutex.
Yes, it's not needed anymore. It's a carry-over from the time
when the old code aborted ransfers from the outside of adutux_mutex.
> /* send off the urb */
> usb_fill_int_urb(
> dev->interrupt_out_urb,
....
> dev->interrupt_in_endpoint->bInterval);
> Typo? Seems like that.
Looks like one. It actually is a risky thing to fix... What if the device
as broken descriptors and the driver worked only thanks to the bug above?
But let's fix it and see.
> I just tried your patch on the adutux from 2.6.24-rc1 but on my 2.6.20.7. It failed with the following message:
> [176200.425776] list_add corruption. next->prev should be prev (c15e6c30), but was 00000000. (next=c58bdf5c).
> [176200.426536] kernel BUG at lib/list_debug.c:28!
> [176200.427843] [<c01e31e3>] list_add+0xa/0xf
> [176200.427848] [<c012bc4b>] add_wait_queue+0x1f/0x2d
This is probably what Oliver caught, I did not have all removals
from wait queue where necessary.
> [176200.428593] <3>BUG: sleeping function called from invalid context at kernel/rwsem.c:20
This looks like a debugging-caused oops. It should go away by itself
once the above is fixed.
> I'm not sure whether it's because I'm trying it on 2.6.20.7 and not on 2.6.24-rc1.
> Will check it on 2.6.24-rc1 asap and let you know.
Please try attached on any of those.
Thank you,
-- Pete
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..bea6262 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 mutex mtx; /* locks this structure */
+ struct mutex mtx;
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 int minor; /* the starting minor number for this device */
char serial_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))) {
+ dbg(2, "%s : mutex lock failed", __FUNCTION__);
+ goto exit_no_lock;
+ }
+
interface = usb_find_interface(&adu_driver, subminor);
if (!interface) {
err("%s - error, can't find device for minor %d",
@@ -272,13 +295,6 @@ static int adu_open(struct inode *inode, struct file *file)
goto exit_no_device;
}
- /* lock this device */
- if ((retval = mutex_lock_interruptible(&dev->mtx))) {
- dbg(2, "%s : mutex lock failed", __FUNCTION__);
- goto exit_no_device;
- }
-
- /* increment our usage count for the device */
++dev->open_count;
dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count);
@@ -290,6 +306,7 @@ static int adu_open(struct inode *inode, struct file *file)
dev->read_buffer_length = 0;
/* fixup first read by having urb waiting for it */
+ mutex_lock(&dev->mtx); // protect udev
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
dev->interrupt_in_endpoint->bEndpointAddress),
@@ -297,24 +314,26 @@ static int adu_open(struct inode *inode, struct file *file)
le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
adu_interrupt_in_callback, dev,
dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
dev->read_urb_finished = 0;
retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
- if (retval)
+ if (retval) {
+ dev->read_urb_finished = 1;
--dev->open_count;
+ }
+ mutex_unlock(&dev->mtx);
}
- mutex_unlock(&dev->mtx);
+
+ retval = 0;
exit_no_device:
+ mutex_unlock(&adutux_mutex);
+exit_no_lock:
dbg(2,"%s : leave, return value %d ", __FUNCTION__, retval);
-
return retval;
}
-static int adu_release_internal(struct adu_device *dev)
+static void adu_release_internal(struct adu_device *dev)
{
- int retval = 0;
-
dbg(2," %s : enter", __FUNCTION__);
/* decrement our usage count for the device */
@@ -326,12 +345,11 @@ static int adu_release_internal(struct adu_device *dev)
}
dbg(2," %s : leave", __FUNCTION__);
- return retval;
}
static int adu_release(struct inode *inode, struct file *file)
{
- struct adu_device *dev = NULL;
+ struct adu_device *dev;
int retval = 0;
dbg(2," %s : enter", __FUNCTION__);
@@ -343,15 +361,13 @@ static int adu_release(struct inode *inode, struct file *file)
}
dev = file->private_data;
-
if (dev == NULL) {
dbg(1," %s : object is NULL", __FUNCTION__);
retval = -ENODEV;
goto exit;
}
- /* lock our device */
- mutex_lock(&dev->mtx); /* not interruptible */
+ mutex_lock(&adutux_mutex); /* not interruptible */
if (dev->open_count <= 0) {
dbg(1," %s : device not opened", __FUNCTION__);
@@ -359,19 +375,15 @@ static int adu_release(struct inode *inode, struct file *file)
goto exit;
}
+ adu_release_internal(dev);
if (dev->udev == NULL) {
/* the device was unplugged before the file was released */
- mutex_unlock(&dev->mtx);
- adu_delete(dev);
- dev = NULL;
- } else {
- /* do the work */
- retval = adu_release_internal(dev);
+ if (!dev->open_count) /* ... and we're the last user */
+ adu_delete(dev);
}
exit:
- if (dev)
- mutex_unlock(&dev->mtx);
+ mutex_unlock(&adutux_mutex);
dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
return retval;
}
@@ -393,12 +405,12 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
dev = file->private_data;
dbg(2," %s : dev=%p", __FUNCTION__, dev);
- /* lock this object */
+
if (mutex_lock_interruptible(&dev->mtx))
return -ERESTARTSYS;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL || dev->minor == 0) {
+ if (dev->udev == NULL) {
retval = -ENODEV;
err("No device or device unplugged %d", retval);
goto exit;
@@ -452,7 +464,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
should_submit = 1;
} else {
/* even the primary was empty - we may need to do IO */
- if (dev->interrupt_in_urb->status == -EINPROGRESS) {
+ if (!dev->read_urb_finished) {
/* somebody is doing IO */
spin_unlock_irqrestore(&dev->buflock, flags);
dbg(2," %s : submitted already", __FUNCTION__);
@@ -460,6 +472,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
/* we must initiate input */
dbg(2," %s : initiate input", __FUNCTION__);
dev->read_urb_finished = 0;
+ spin_unlock_irqrestore(&dev->buflock, flags);
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
@@ -470,14 +483,11 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
dev,
dev->interrupt_in_endpoint->bInterval);
retval = usb_submit_urb(dev->interrupt_in_urb, GFP_ATOMIC);
- if (!retval) {
- spin_unlock_irqrestore(&dev->buflock, flags);
- dbg(2," %s : submitted OK", __FUNCTION__);
- } else {
+ if (retval) {
+ dev->read_urb_finished = 1;
if (retval == -ENOMEM) {
retval = bytes_read ? bytes_read : -ENOMEM;
}
- spin_unlock_irqrestore(&dev->buflock, flags);
dbg(2," %s : submit failed", __FUNCTION__);
goto exit;
}
@@ -486,10 +496,14 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
/* we wait for I/O to complete */
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(&dev->read_wait, &wait);
- if (!dev->read_urb_finished)
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(&dev->buflock, flags);
timeout = schedule_timeout(COMMAND_TIMEOUT);
- else
+ } else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
set_current_state(TASK_RUNNING);
+ }
remove_wait_queue(&dev->read_wait, &wait);
if (timeout <= 0) {
@@ -509,19 +523,23 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
retval = bytes_read;
/* if the primary buffer is empty then use it */
- if (should_submit && !dev->interrupt_in_urb->status==-EINPROGRESS) {
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (should_submit && dev->read_urb_finished) {
+ dev->read_urb_finished = 0;
+ spin_unlock_irqrestore(&dev->buflock, flags);
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
dev->interrupt_in_endpoint->bEndpointAddress),
- dev->interrupt_in_buffer,
- le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
- adu_interrupt_in_callback,
- dev,
- dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
- dev->read_urb_finished = 0;
- usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback,
+ dev,
+ dev->interrupt_in_endpoint->bInterval);
+ if (usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL) != 0)
+ dev->read_urb_finished = 1;
/* we ignore failure */
+ } else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
}
exit:
@@ -535,24 +553,24 @@ exit:
static ssize_t adu_write(struct file *file, const __user char *buffer,
size_t count, loff_t *ppos)
{
+ DECLARE_WAITQUEUE(waita, current);
struct adu_device *dev;
size_t bytes_written = 0;
size_t bytes_to_write;
size_t buffer_size;
+ unsigned long flags;
int retval;
- int timeout = 0;
dbg(2," %s : enter, count = %Zd", __FUNCTION__, count);
dev = file->private_data;
- /* lock this object */
retval = mutex_lock_interruptible(&dev->mtx);
if (retval)
goto exit_nolock;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL || dev->minor == 0) {
+ if (dev->udev == NULL) {
retval = -ENODEV;
err("No device or device unplugged %d", retval);
goto exit;
@@ -564,42 +582,37 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
goto exit;
}
-
while (count > 0) {
- if (dev->interrupt_out_urb->status == -EINPROGRESS) {
- timeout = COMMAND_TIMEOUT;
+ add_wait_queue(&dev->write_wait, &waita);
+ set_current_state(TASK_INTERRUPTIBLE);
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(&dev->buflock, flags);
- while (timeout > 0) {
- if (signal_pending(current)) {
+ mutex_unlock(&dev->mtx);
+ if (signal_pending(current)) {
dbg(1," %s : interrupted", __FUNCTION__);
+ set_current_state(TASK_RUNNING);
retval = -EINTR;
- goto exit;
+ goto exit_onqueue;
}
- mutex_unlock(&dev->mtx);
- timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
+ if (schedule_timeout(COMMAND_TIMEOUT) == 0) {
+ dbg(1, "%s - command timed out.", __FUNCTION__);
+ retval = -ETIMEDOUT;
+ goto exit_onqueue;
+ }
+ remove_wait_queue(&dev->write_wait, &waita);
retval = mutex_lock_interruptible(&dev->mtx);
if (retval) {
retval = bytes_written ? bytes_written : retval;
goto exit_nolock;
}
- if (timeout > 0) {
- break;
- }
- dbg(1," %s : interrupted timeout: %d", __FUNCTION__, timeout);
- }
-
-
- dbg(1," %s : final timeout: %d", __FUNCTION__, timeout);
-
- if (timeout == 0) {
- dbg(1, "%s - command timed out.", __FUNCTION__);
- retval = -ETIMEDOUT;
- goto exit;
- }
-
- dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
+ dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
} else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
+ set_current_state(TASK_RUNNING);
+ remove_wait_queue(&dev->write_wait, &waita);
dbg(4," %s : sending, count = %Zd", __FUNCTION__, count);
/* write the data into interrupt_out_buffer from userspace */
@@ -622,12 +635,14 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
bytes_to_write,
adu_interrupt_out_callback,
dev,
- dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->interrupt_out_endpoint->bInterval);
dev->interrupt_out_urb->actual_length = bytes_to_write;
+ dev->out_urb_finished = 0;
retval = usb_submit_urb(dev->interrupt_out_urb, GFP_KERNEL);
if (retval < 0) {
+ dev->out_urb_finished = 1;
err("Couldn't submit interrupt_out_urb %d", retval);
+ remove_wait_queue(&dev->write_wait, &waita);
goto exit;
}
@@ -637,16 +652,17 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
bytes_written += bytes_to_write;
}
}
-
- retval = bytes_written;
+ mutex_unlock(&dev->mtx);
+ return bytes_written;
exit:
- /* unlock the device */
mutex_unlock(&dev->mtx);
exit_nolock:
-
dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+ return retval;
+exit_onqueue:
+ remove_wait_queue(&dev->write_wait, &waita);
return retval;
}
@@ -831,25 +847,22 @@ static void adu_disconnect(struct usb_interface *interface)
dbg(2," %s : enter", __FUNCTION__);
dev = usb_get_intfdata(interface);
- usb_set_intfdata(interface, NULL);
+ mutex_lock(&dev->mtx); /* not interruptible */
+ dev->udev = NULL; /* poison */
minor = dev->minor;
-
- /* give back our minor */
usb_deregister_dev(interface, &adu_class);
- dev->minor = 0;
+ mutex_unlock(&dev->mtx);
- mutex_lock(&dev->mtx); /* not interruptible */
+ mutex_lock(&adutux_mutex);
+ usb_set_intfdata(interface, NULL);
/* if the device is not opened, then we clean up right now */
dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
- if (!dev->open_count) {
- mutex_unlock(&dev->mtx);
+ if (!dev->open_count)
adu_delete(dev);
- } else {
- dev->udev = NULL;
- mutex_unlock(&dev->mtx);
- }
+
+ mutex_unlock(&adutux_mutex);
dev_info(&interface->dev, "ADU device adutux%d now disconnected\n",
(minor - ADU_MINOR_BASE));
BTW, the patch in my previous message had one buglet - an extra
remove_wait_queue. It was in an error case though, so it should be
ok to test. But just in case, here's a fixed one.
-- Pete
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..6914c0b 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 mutex mtx; /* locks this structure */
+ struct mutex mtx;
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 int minor; /* the starting minor number for this device */
char serial_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))) {
+ dbg(2, "%s : mutex lock failed", __FUNCTION__);
+ goto exit_no_lock;
+ }
+
interface = usb_find_interface(&adu_driver, subminor);
if (!interface) {
err("%s - error, can't find device for minor %d",
@@ -272,13 +295,6 @@ static int adu_open(struct inode *inode, struct file *file)
goto exit_no_device;
}
- /* lock this device */
- if ((retval = mutex_lock_interruptible(&dev->mtx))) {
- dbg(2, "%s : mutex lock failed", __FUNCTION__);
- goto exit_no_device;
- }
-
- /* increment our usage count for the device */
++dev->open_count;
dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count);
@@ -290,6 +306,7 @@ static int adu_open(struct inode *inode, struct file *file)
dev->read_buffer_length = 0;
/* fixup first read by having urb waiting for it */
+ mutex_lock(&dev->mtx); // protect udev
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
dev->interrupt_in_endpoint->bEndpointAddress),
@@ -297,24 +314,26 @@ static int adu_open(struct inode *inode, struct file *file)
le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
adu_interrupt_in_callback, dev,
dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
dev->read_urb_finished = 0;
retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
- if (retval)
+ if (retval) {
+ dev->read_urb_finished = 1;
--dev->open_count;
+ }
+ mutex_unlock(&dev->mtx);
}
- mutex_unlock(&dev->mtx);
+
+ retval = 0;
exit_no_device:
+ mutex_unlock(&adutux_mutex);
+exit_no_lock:
dbg(2,"%s : leave, return value %d ", __FUNCTION__, retval);
-
return retval;
}
-static int adu_release_internal(struct adu_device *dev)
+static void adu_release_internal(struct adu_device *dev)
{
- int retval = 0;
-
dbg(2," %s : enter", __FUNCTION__);
/* decrement our usage count for the device */
@@ -326,12 +345,11 @@ static int adu_release_internal(struct adu_device *dev)
}
dbg(2," %s : leave", __FUNCTION__);
- return retval;
}
static int adu_release(struct inode *inode, struct file *file)
{
- struct adu_device *dev = NULL;
+ struct adu_device *dev;
int retval = 0;
dbg(2," %s : enter", __FUNCTION__);
@@ -343,15 +361,13 @@ static int adu_release(struct inode *inode, struct file *file)
}
dev = file->private_data;
-
if (dev == NULL) {
dbg(1," %s : object is NULL", __FUNCTION__);
retval = -ENODEV;
goto exit;
}
- /* lock our device */
- mutex_lock(&dev->mtx); /* not interruptible */
+ mutex_lock(&adutux_mutex); /* not interruptible */
if (dev->open_count <= 0) {
dbg(1," %s : device not opened", __FUNCTION__);
@@ -359,19 +375,15 @@ static int adu_release(struct inode *inode, struct file *file)
goto exit;
}
+ adu_release_internal(dev);
if (dev->udev == NULL) {
/* the device was unplugged before the file was released */
- mutex_unlock(&dev->mtx);
- adu_delete(dev);
- dev = NULL;
- } else {
- /* do the work */
- retval = adu_release_internal(dev);
+ if (!dev->open_count) /* ... and we're the last user */
+ adu_delete(dev);
}
exit:
- if (dev)
- mutex_unlock(&dev->mtx);
+ mutex_unlock(&adutux_mutex);
dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
return retval;
}
@@ -393,12 +405,12 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
dev = file->private_data;
dbg(2," %s : dev=%p", __FUNCTION__, dev);
- /* lock this object */
+
if (mutex_lock_interruptible(&dev->mtx))
return -ERESTARTSYS;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL || dev->minor == 0) {
+ if (dev->udev == NULL) {
retval = -ENODEV;
err("No device or device unplugged %d", retval);
goto exit;
@@ -452,7 +464,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
should_submit = 1;
} else {
/* even the primary was empty - we may need to do IO */
- if (dev->interrupt_in_urb->status == -EINPROGRESS) {
+ if (!dev->read_urb_finished) {
/* somebody is doing IO */
spin_unlock_irqrestore(&dev->buflock, flags);
dbg(2," %s : submitted already", __FUNCTION__);
@@ -460,6 +472,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
/* we must initiate input */
dbg(2," %s : initiate input", __FUNCTION__);
dev->read_urb_finished = 0;
+ spin_unlock_irqrestore(&dev->buflock, flags);
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
@@ -470,14 +483,11 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
dev,
dev->interrupt_in_endpoint->bInterval);
retval = usb_submit_urb(dev->interrupt_in_urb, GFP_ATOMIC);
- if (!retval) {
- spin_unlock_irqrestore(&dev->buflock, flags);
- dbg(2," %s : submitted OK", __FUNCTION__);
- } else {
+ if (retval) {
+ dev->read_urb_finished = 1;
if (retval == -ENOMEM) {
retval = bytes_read ? bytes_read : -ENOMEM;
}
- spin_unlock_irqrestore(&dev->buflock, flags);
dbg(2," %s : submit failed", __FUNCTION__);
goto exit;
}
@@ -486,10 +496,14 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
/* we wait for I/O to complete */
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(&dev->read_wait, &wait);
- if (!dev->read_urb_finished)
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(&dev->buflock, flags);
timeout = schedule_timeout(COMMAND_TIMEOUT);
- else
+ } else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
set_current_state(TASK_RUNNING);
+ }
remove_wait_queue(&dev->read_wait, &wait);
if (timeout <= 0) {
@@ -509,19 +523,23 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
retval = bytes_read;
/* if the primary buffer is empty then use it */
- if (should_submit && !dev->interrupt_in_urb->status==-EINPROGRESS) {
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (should_submit && dev->read_urb_finished) {
+ dev->read_urb_finished = 0;
+ spin_unlock_irqrestore(&dev->buflock, flags);
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
dev->interrupt_in_endpoint->bEndpointAddress),
- dev->interrupt_in_buffer,
- le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
- adu_interrupt_in_callback,
- dev,
- dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
- dev->read_urb_finished = 0;
- usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback,
+ dev,
+ dev->interrupt_in_endpoint->bInterval);
+ if (usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL) != 0)
+ dev->read_urb_finished = 1;
/* we ignore failure */
+ } else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
}
exit:
@@ -535,24 +553,24 @@ exit:
static ssize_t adu_write(struct file *file, const __user char *buffer,
size_t count, loff_t *ppos)
{
+ DECLARE_WAITQUEUE(waita, current);
struct adu_device *dev;
size_t bytes_written = 0;
size_t bytes_to_write;
size_t buffer_size;
+ unsigned long flags;
int retval;
- int timeout = 0;
dbg(2," %s : enter, count = %Zd", __FUNCTION__, count);
dev = file->private_data;
- /* lock this object */
retval = mutex_lock_interruptible(&dev->mtx);
if (retval)
goto exit_nolock;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL || dev->minor == 0) {
+ if (dev->udev == NULL) {
retval = -ENODEV;
err("No device or device unplugged %d", retval);
goto exit;
@@ -564,42 +582,37 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
goto exit;
}
-
while (count > 0) {
- if (dev->interrupt_out_urb->status == -EINPROGRESS) {
- timeout = COMMAND_TIMEOUT;
+ add_wait_queue(&dev->write_wait, &waita);
+ set_current_state(TASK_INTERRUPTIBLE);
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(&dev->buflock, flags);
- while (timeout > 0) {
- if (signal_pending(current)) {
+ mutex_unlock(&dev->mtx);
+ if (signal_pending(current)) {
dbg(1," %s : interrupted", __FUNCTION__);
+ set_current_state(TASK_RUNNING);
retval = -EINTR;
- goto exit;
+ goto exit_onqueue;
}
- mutex_unlock(&dev->mtx);
- timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
+ if (schedule_timeout(COMMAND_TIMEOUT) == 0) {
+ dbg(1, "%s - command timed out.", __FUNCTION__);
+ retval = -ETIMEDOUT;
+ goto exit_onqueue;
+ }
+ remove_wait_queue(&dev->write_wait, &waita);
retval = mutex_lock_interruptible(&dev->mtx);
if (retval) {
retval = bytes_written ? bytes_written : retval;
goto exit_nolock;
}
- if (timeout > 0) {
- break;
- }
- dbg(1," %s : interrupted timeout: %d", __FUNCTION__, timeout);
- }
-
-
- dbg(1," %s : final timeout: %d", __FUNCTION__, timeout);
-
- if (timeout == 0) {
- dbg(1, "%s - command timed out.", __FUNCTION__);
- retval = -ETIMEDOUT;
- goto exit;
- }
-
- dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
+ dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
} else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
+ set_current_state(TASK_RUNNING);
+ remove_wait_queue(&dev->write_wait, &waita);
dbg(4," %s : sending, count = %Zd", __FUNCTION__, count);
/* write the data into interrupt_out_buffer from userspace */
@@ -622,11 +635,12 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
bytes_to_write,
adu_interrupt_out_callback,
dev,
- dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->interrupt_out_endpoint->bInterval);
dev->interrupt_out_urb->actual_length = bytes_to_write;
+ dev->out_urb_finished = 0;
retval = usb_submit_urb(dev->interrupt_out_urb, GFP_KERNEL);
if (retval < 0) {
+ dev->out_urb_finished = 1;
err("Couldn't submit interrupt_out_urb %d", retval);
goto exit;
}
@@ -637,16 +651,17 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
bytes_written += bytes_to_write;
}
}
-
- retval = bytes_written;
+ mutex_unlock(&dev->mtx);
+ return bytes_written;
exit:
- /* unlock the device */
mutex_unlock(&dev->mtx);
exit_nolock:
-
dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+ return retval;
+exit_onqueue:
+ remove_wait_queue(&dev->write_wait, &waita);
return retval;
}
@@ -831,25 +846,22 @@ static void adu_disconnect(struct usb_interface *interface)
dbg(2," %s : enter", __FUNCTION__);
dev = usb_get_intfdata(interface);
- usb_set_intfdata(interface, NULL);
+ mutex_lock(&dev->mtx); /* not interruptible */
+ dev->udev = NULL; /* poison */
minor = dev->minor;
-
- /* give back our minor */
usb_deregister_dev(interface, &adu_class);
- dev->minor = 0;
+ mutex_unlock(&dev->mtx);
- mutex_lock(&dev->mtx); /* not interruptible */
+ mutex_lock(&adutux_mutex);
+ usb_set_intfdata(interface, NULL);
/* if the device is not opened, then we clean up right now */
dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
- if (!dev->open_count) {
- mutex_unlock(&dev->mtx);
+ if (!dev->open_count)
adu_delete(dev);
- } else {
- dev->udev = NULL;
- mutex_unlock(&dev->mtx);
- }
+
+ mutex_unlock(&adutux_mutex);
dev_info(&interface->dev, "ADU device adutux%d now disconnected\n",
(minor - ADU_MINOR_BASE));
On Thu, 25 Oct 2007 14:03:48 +0200, Oliver Neukum <[email protected]> wrote:
> Am Donnerstag 25 Oktober 2007 schrieb Pete Zaitcev:
> > + if (signal_pending(current)) {
> > dbg(1," %s : interrupted", __FUNCTION__);
> > + set_current_state(TASK_RUNNING);
> > retval = -EINTR;
> > - goto exit;
> > + goto exit_onqueue;
>
> Are you sure you cannot have written any data here?
I noticed that myself, but this is what the old driver did.
-- Pete
Greg,
On 10/25/07, Greg KH <[email protected]> wrote:
> Hm, I think I'll wait for Pete and you to agree and send me a final
> version before applying anything here :)
I'm testing the final patch Pete created with all the notes.
Will let you know if there are issues with it. If not it can be
applied to your tree.
Thanks,
Vitaliy
Finally had a time on my weekend to perform tests and fix Pete's patch a little. Now it seems to be correct.
Added a few changes:
1. One device per user space application. Old driver allowed many users application to work with same device which can lead to IO mess.
2. GFP_KERNEL is used instead of GFP_ATOMIC as we are now out of spin locks.
3. Added myself to maintainers(already discussed this in 2.4). Does anyone mind?
(Diff between this latest patch and the one from Pete is in attach).
Also part of Pete's notes:
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.
- Remove sleep_on.
Here is the final patch. Comments are welcomed.
--
Signed-off-by: Pete Zaitcev <[email protected]>
Signed-off-by: Vitaliy Ivanov <[email protected]>
Tested-by: Vitaliy Ivanov <[email protected]>
--
diff -uprN -X linux-2.6.24-rc1.org/Documentation/dontdiff linux-2.6.24-rc1.org/drivers/usb/misc/adutux.c linux-2.6.24-rc1.my/drivers/usb/misc/adutux.c
--- linux-2.6.24-rc1.org/drivers/usb/misc/adutux.c 2007-10-29 19:25:00.000000000 +0200
+++ linux-2.6.24-rc1.my/drivers/usb/misc/adutux.c 2007-10-29 19:01:10.000000000 +0200
@@ -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 mutex mtx; /* locks this structure */
+ struct mutex mtx;
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 int minor; /* the starting minor number for this device */
char serial_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, co
*/
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
{
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(s
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 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))) {
+ dbg(2, "%s : mutex lock failed", __FUNCTION__);
+ goto exit_no_lock;
+ }
+
interface = usb_find_interface(&adu_driver, subminor);
if (!interface) {
err("%s - error, can't find device for minor %d",
@@ -272,49 +295,48 @@ static int adu_open(struct inode *inode,
goto exit_no_device;
}
- /* lock this device */
- if ((retval = mutex_lock_interruptible(&dev->mtx))) {
- dbg(2, "%s : mutex lock failed", __FUNCTION__);
+ /* check that nobody else is using the device */
+ if (dev->open_count) {
+ retval = -EBUSY;
goto exit_no_device;
}
- /* increment our usage count for the device */
++dev->open_count;
dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count);
/* save device in the file's private structure */
file->private_data = dev;
- if (dev->open_count == 1) {
- /* initialize in direction */
- dev->read_buffer_length = 0;
+ /* initialize in direction */
+ dev->read_buffer_length = 0;
- /* fixup first read by having urb waiting for it */
- usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
- usb_rcvintpipe(dev->udev,
- dev->interrupt_in_endpoint->bEndpointAddress),
- dev->interrupt_in_buffer,
- le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
- adu_interrupt_in_callback, dev,
- dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
- dev->read_urb_finished = 0;
- retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
- if (retval)
- --dev->open_count;
- }
- mutex_unlock(&dev->mtx);
+ /* fixup first read by having urb waiting for it */
+ usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
+ usb_rcvintpipe(dev->udev,
+ dev->interrupt_in_endpoint->bEndpointAddress),
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback, dev,
+ dev->interrupt_in_endpoint->bInterval);
+ dev->read_urb_finished = 0;
+ retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
+ /* we ignore failure */
+ /* end of fixup for first read */
+
+ /* initialize out direction */
+ dev->out_urb_finished = 1;
+
+ retval = 0;
exit_no_device:
+ mutex_unlock(&adutux_mutex);
+exit_no_lock:
dbg(2,"%s : leave, return value %d ", __FUNCTION__, retval);
-
return retval;
}
-static int adu_release_internal(struct adu_device *dev)
+static void adu_release_internal(struct adu_device *dev)
{
- int retval = 0;
-
dbg(2," %s : enter", __FUNCTION__);
/* decrement our usage count for the device */
@@ -326,12 +348,11 @@ static int adu_release_internal(struct a
}
dbg(2," %s : leave", __FUNCTION__);
- return retval;
}
static int adu_release(struct inode *inode, struct file *file)
{
- struct adu_device *dev = NULL;
+ struct adu_device *dev;
int retval = 0;
dbg(2," %s : enter", __FUNCTION__);
@@ -343,15 +364,13 @@ static int adu_release(struct inode *ino
}
dev = file->private_data;
-
if (dev == NULL) {
dbg(1," %s : object is NULL", __FUNCTION__);
retval = -ENODEV;
goto exit;
}
- /* lock our device */
- mutex_lock(&dev->mtx); /* not interruptible */
+ mutex_lock(&adutux_mutex); /* not interruptible */
if (dev->open_count <= 0) {
dbg(1," %s : device not opened", __FUNCTION__);
@@ -359,19 +378,15 @@ static int adu_release(struct inode *ino
goto exit;
}
+ adu_release_internal(dev);
if (dev->udev == NULL) {
/* the device was unplugged before the file was released */
- mutex_unlock(&dev->mtx);
- adu_delete(dev);
- dev = NULL;
- } else {
- /* do the work */
- retval = adu_release_internal(dev);
+ if (!dev->open_count) /* ... and we're the last user */
+ adu_delete(dev);
}
exit:
- if (dev)
- mutex_unlock(&dev->mtx);
+ mutex_unlock(&adutux_mutex);
dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
return retval;
}
@@ -393,12 +408,12 @@ static ssize_t adu_read(struct file *fil
dev = file->private_data;
dbg(2," %s : dev=%p", __FUNCTION__, dev);
- /* lock this object */
+
if (mutex_lock_interruptible(&dev->mtx))
return -ERESTARTSYS;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL || dev->minor == 0) {
+ if (dev->udev == NULL) {
retval = -ENODEV;
err("No device or device unplugged %d", retval);
goto exit;
@@ -452,7 +467,7 @@ static ssize_t adu_read(struct file *fil
should_submit = 1;
} else {
/* even the primary was empty - we may need to do IO */
- if (dev->interrupt_in_urb->status == -EINPROGRESS) {
+ if (!dev->read_urb_finished) {
/* somebody is doing IO */
spin_unlock_irqrestore(&dev->buflock, flags);
dbg(2," %s : submitted already", __FUNCTION__);
@@ -460,6 +475,7 @@ static ssize_t adu_read(struct file *fil
/* we must initiate input */
dbg(2," %s : initiate input", __FUNCTION__);
dev->read_urb_finished = 0;
+ spin_unlock_irqrestore(&dev->buflock, flags);
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
@@ -469,15 +485,12 @@ static ssize_t adu_read(struct file *fil
adu_interrupt_in_callback,
dev,
dev->interrupt_in_endpoint->bInterval);
- retval = usb_submit_urb(dev->interrupt_in_urb, GFP_ATOMIC);
- if (!retval) {
- spin_unlock_irqrestore(&dev->buflock, flags);
- dbg(2," %s : submitted OK", __FUNCTION__);
- } else {
+ retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
+ if (retval) {
+ dev->read_urb_finished = 1;
if (retval == -ENOMEM) {
retval = bytes_read ? bytes_read : -ENOMEM;
}
- spin_unlock_irqrestore(&dev->buflock, flags);
dbg(2," %s : submit failed", __FUNCTION__);
goto exit;
}
@@ -486,10 +499,14 @@ static ssize_t adu_read(struct file *fil
/* we wait for I/O to complete */
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(&dev->read_wait, &wait);
- if (!dev->read_urb_finished)
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(&dev->buflock, flags);
timeout = schedule_timeout(COMMAND_TIMEOUT);
- else
+ } else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
set_current_state(TASK_RUNNING);
+ }
remove_wait_queue(&dev->read_wait, &wait);
if (timeout <= 0) {
@@ -509,19 +526,23 @@ static ssize_t adu_read(struct file *fil
retval = bytes_read;
/* if the primary buffer is empty then use it */
- if (should_submit && !dev->interrupt_in_urb->status==-EINPROGRESS) {
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (should_submit && dev->read_urb_finished) {
+ dev->read_urb_finished = 0;
+ spin_unlock_irqrestore(&dev->buflock, flags);
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
dev->interrupt_in_endpoint->bEndpointAddress),
- dev->interrupt_in_buffer,
- le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
- adu_interrupt_in_callback,
- dev,
- dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
- dev->read_urb_finished = 0;
- usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback,
+ dev,
+ dev->interrupt_in_endpoint->bInterval);
+ if (usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL) != 0)
+ dev->read_urb_finished = 1;
/* we ignore failure */
+ } else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
}
exit:
@@ -535,24 +556,24 @@ exit:
static ssize_t adu_write(struct file *file, const __user char *buffer,
size_t count, loff_t *ppos)
{
+ DECLARE_WAITQUEUE(waita, current);
struct adu_device *dev;
size_t bytes_written = 0;
size_t bytes_to_write;
size_t buffer_size;
+ unsigned long flags;
int retval;
- int timeout = 0;
dbg(2," %s : enter, count = %Zd", __FUNCTION__, count);
dev = file->private_data;
- /* lock this object */
retval = mutex_lock_interruptible(&dev->mtx);
if (retval)
goto exit_nolock;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL || dev->minor == 0) {
+ if (dev->udev == NULL) {
retval = -ENODEV;
err("No device or device unplugged %d", retval);
goto exit;
@@ -564,42 +585,37 @@ static ssize_t adu_write(struct file *fi
goto exit;
}
-
while (count > 0) {
- if (dev->interrupt_out_urb->status == -EINPROGRESS) {
- timeout = COMMAND_TIMEOUT;
+ add_wait_queue(&dev->write_wait, &waita);
+ set_current_state(TASK_INTERRUPTIBLE);
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(&dev->buflock, flags);
- while (timeout > 0) {
- if (signal_pending(current)) {
+ mutex_unlock(&dev->mtx);
+ if (signal_pending(current)) {
dbg(1," %s : interrupted", __FUNCTION__);
+ set_current_state(TASK_RUNNING);
retval = -EINTR;
- goto exit;
+ goto exit_onqueue;
}
- mutex_unlock(&dev->mtx);
- timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
+ if (schedule_timeout(COMMAND_TIMEOUT) == 0) {
+ dbg(1, "%s - command timed out.", __FUNCTION__);
+ retval = -ETIMEDOUT;
+ goto exit_onqueue;
+ }
+ remove_wait_queue(&dev->write_wait, &waita);
retval = mutex_lock_interruptible(&dev->mtx);
if (retval) {
retval = bytes_written ? bytes_written : retval;
goto exit_nolock;
}
- if (timeout > 0) {
- break;
- }
- dbg(1," %s : interrupted timeout: %d", __FUNCTION__, timeout);
- }
-
-
- dbg(1," %s : final timeout: %d", __FUNCTION__, timeout);
-
- if (timeout == 0) {
- dbg(1, "%s - command timed out.", __FUNCTION__);
- retval = -ETIMEDOUT;
- goto exit;
- }
-
- dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
+ dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
} else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
+ set_current_state(TASK_RUNNING);
+ remove_wait_queue(&dev->write_wait, &waita);
dbg(4," %s : sending, count = %Zd", __FUNCTION__, count);
/* write the data into interrupt_out_buffer from userspace */
@@ -622,11 +638,12 @@ static ssize_t adu_write(struct file *fi
bytes_to_write,
adu_interrupt_out_callback,
dev,
- dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->interrupt_out_endpoint->bInterval);
dev->interrupt_out_urb->actual_length = bytes_to_write;
+ dev->out_urb_finished = 0;
retval = usb_submit_urb(dev->interrupt_out_urb, GFP_KERNEL);
if (retval < 0) {
+ dev->out_urb_finished = 1;
err("Couldn't submit interrupt_out_urb %d", retval);
goto exit;
}
@@ -637,16 +654,17 @@ static ssize_t adu_write(struct file *fi
bytes_written += bytes_to_write;
}
}
-
- retval = bytes_written;
+ mutex_unlock(&dev->mtx);
+ return bytes_written;
exit:
- /* unlock the device */
mutex_unlock(&dev->mtx);
exit_nolock:
-
dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+ return retval;
+exit_onqueue:
+ remove_wait_queue(&dev->write_wait, &waita);
return retval;
}
@@ -831,25 +849,22 @@ static void adu_disconnect(struct usb_in
dbg(2," %s : enter", __FUNCTION__);
dev = usb_get_intfdata(interface);
- usb_set_intfdata(interface, NULL);
+ mutex_lock(&dev->mtx); /* not interruptible */
+ dev->udev = NULL; /* poison */
minor = dev->minor;
-
- /* give back our minor */
usb_deregister_dev(interface, &adu_class);
- dev->minor = 0;
+ mutex_unlock(&dev->mtx);
- mutex_lock(&dev->mtx); /* not interruptible */
+ mutex_lock(&adutux_mutex);
+ usb_set_intfdata(interface, NULL);
/* if the device is not opened, then we clean up right now */
dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
- if (!dev->open_count) {
- mutex_unlock(&dev->mtx);
+ if (!dev->open_count)
adu_delete(dev);
- } else {
- dev->udev = NULL;
- mutex_unlock(&dev->mtx);
- }
+
+ mutex_unlock(&adutux_mutex);
dev_info(&interface->dev, "ADU device adutux%d now disconnected\n",
(minor - ADU_MINOR_BASE));
diff -uprN -X linux-2.6.24-rc1.org/Documentation/dontdiff linux-2.6.24-rc1.org/MAINTAINERS linux-2.6.24-rc1.my/MAINTAINERS
--- linux-2.6.24-rc1.org/MAINTAINERS 2007-10-29 15:22:46.000000000 +0200
+++ linux-2.6.24-rc1.my/MAINTAINERS 2007-10-29 19:16:53.000000000 +0200
@@ -286,6 +286,11 @@ P: Colin Leroy
M: [email protected]
S: Maintained
+ADUTUX ONTRAK CONTROL SYSTEMS USB DRIVER
+P: Vitaliy Ivanov
+M: [email protected]
+S: Maintained
+
ADVANSYS SCSI DRIVER
P: Matthew Wilcox
M: [email protected]
On Mon, 29 Oct 2007 20:04:57 +0200, Vitaliy Ivanov <[email protected]> wrote:
> Finally had a time on my weekend to perform tests and fix Pete's patch a little. Now it seems to be correct.
Great!
> 1. One device per user space application. Old driver allowed many users application to work with same device which can lead to IO mess.
OK. This trick was popular in UNIX. Personally I think it's in a bad
taste, because good applications still need to verify if only one
instance is running, and threfore can use application level locking.
But if you are gunning for the maintenership I'm not going to argue
your style. The busy lock-out certainly works better than "/dev/cua" :-)
However, this looks wrong:
> + dev->interrupt_in_endpoint->bInterval);
> + dev->read_urb_finished = 0;
> + retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
> + /* we ignore failure */
> + /* end of fixup for first read */
> +
> + /* initialize out direction */
> + dev->out_urb_finished = 1;
The finished flag is only set when URB is not in use anymore. Did you
observe an anomaly with my code? Any hangs? If so, I assure you this
is not the fix. As it's written, even if we ignore the failure (e.g.
do not pass it to userland), we sill have to maintain the correct
flag state.
-- Pete
On Tue, 2007-10-30 at 06:24, Pete Zaitcev wrote:
> However, this looks wrong:
>
> > + dev->interrupt_in_endpoint->bInterval);
> > + dev->read_urb_finished = 0;
> > + retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
> > + /* we ignore failure */
> > + /* end of fixup for first read */
> > +
> > + /* initialize out direction */
> > + dev->out_urb_finished = 1;
>
> The finished flag is only set when URB is not in use anymore. Did you
> observe an anomaly with my code? Any hangs? If so, I assure you this
> is not the fix. As it's written, even if we ignore the failure (e.g.
> do not pass it to userland), we sill have to maintain the correct
> flag state.
As about read_urb_finished probably it's OK. But we shouldn't decrease open_count in the case of error as then we return normal exit value.
Here is what we had before:
dev->interrupt_in_endpoint->bInterval);
dev->read_urb_finished = 0;
retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
if (retval) {
dev->read_urb_finished = 1;
--dev->open_count;
}
So I can left it but w/o this line:
--dev->open_count;
What is more critical is that I added:
/* initialize out direction */
dev->out_urb_finished = 1;
Without this we'll always have write timeouts.
Vitaliy
On Tue, 30 Oct 2007 15:09:54 +0200, Vitaliy Ivanov <[email protected]> wrote:
> As about read_urb_finished probably it's OK. But we shouldn't decrease
> open_count in the case of error as then we return normal exit value.
Oh, thanks. I fixed that.
One other small thing. I see you dropped the dev->mtx bracket that
I added around the initialization and submission. Notice that the
dev->udev is zeroed under dev->mtx, not the static lock. This is because
it has to be seen in read and write paths. If you don't like taking
across the submission, how about testing for it ahead of time?
Here's what I've got now:
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 mutex mtx; /* locks this structure */
+ struct mutex mtx;
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 int minor; /* the starting minor number for this device */
char serial_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))) {
+ dbg(2, "%s : mutex lock failed", __FUNCTION__);
+ goto exit_no_lock;
+ }
+
interface = usb_find_interface(&adu_driver, subminor);
if (!interface) {
err("%s - error, can't find device for minor %d",
@@ -267,54 +290,54 @@ static int adu_open(struct inode *inode, struct file *file)
}
dev = usb_get_intfdata(interface);
- if (!dev) {
+ if (!dev || !dev->udev) {
retval = -ENODEV;
goto exit_no_device;
}
- /* lock this device */
- if ((retval = mutex_lock_interruptible(&dev->mtx))) {
- dbg(2, "%s : mutex lock failed", __FUNCTION__);
+ /* check that nobody else is using the device */
+ if (dev->open_count) {
+ retval = -EBUSY;
goto exit_no_device;
}
- /* increment our usage count for the device */
++dev->open_count;
dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count);
/* save device in the file's private structure */
file->private_data = dev;
- if (dev->open_count == 1) {
- /* initialize in direction */
- dev->read_buffer_length = 0;
+ /* initialize in direction */
+ dev->read_buffer_length = 0;
- /* fixup first read by having urb waiting for it */
- usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
- usb_rcvintpipe(dev->udev,
- dev->interrupt_in_endpoint->bEndpointAddress),
- dev->interrupt_in_buffer,
- le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
- adu_interrupt_in_callback, dev,
- dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
- dev->read_urb_finished = 0;
- retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
- if (retval)
- --dev->open_count;
- }
- mutex_unlock(&dev->mtx);
+ /* fixup first read by having urb waiting for it */
+ usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
+ usb_rcvintpipe(dev->udev,
+ dev->interrupt_in_endpoint->bEndpointAddress),
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback, dev,
+ dev->interrupt_in_endpoint->bInterval);
+ dev->read_urb_finished = 0;
+ if (usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL))
+ dev->read_urb_finished = 1;
+ /* we ignore failure */
+ /* end of fixup for first read */
+
+ /* initialize out direction */
+ dev->out_urb_finished = 1;
+
+ retval = 0;
exit_no_device:
+ mutex_unlock(&adutux_mutex);
+exit_no_lock:
dbg(2,"%s : leave, return value %d ", __FUNCTION__, retval);
-
return retval;
}
-static int adu_release_internal(struct adu_device *dev)
+static void adu_release_internal(struct adu_device *dev)
{
- int retval = 0;
-
dbg(2," %s : enter", __FUNCTION__);
/* decrement our usage count for the device */
@@ -326,12 +349,11 @@ static int adu_release_internal(struct adu_device *dev)
}
dbg(2," %s : leave", __FUNCTION__);
- return retval;
}
static int adu_release(struct inode *inode, struct file *file)
{
- struct adu_device *dev = NULL;
+ struct adu_device *dev;
int retval = 0;
dbg(2," %s : enter", __FUNCTION__);
@@ -343,15 +365,13 @@ static int adu_release(struct inode *inode, struct file *file)
}
dev = file->private_data;
-
if (dev == NULL) {
dbg(1," %s : object is NULL", __FUNCTION__);
retval = -ENODEV;
goto exit;
}
- /* lock our device */
- mutex_lock(&dev->mtx); /* not interruptible */
+ mutex_lock(&adutux_mutex); /* not interruptible */
if (dev->open_count <= 0) {
dbg(1," %s : device not opened", __FUNCTION__);
@@ -359,19 +379,15 @@ static int adu_release(struct inode *inode, struct file *file)
goto exit;
}
+ adu_release_internal(dev);
if (dev->udev == NULL) {
/* the device was unplugged before the file was released */
- mutex_unlock(&dev->mtx);
- adu_delete(dev);
- dev = NULL;
- } else {
- /* do the work */
- retval = adu_release_internal(dev);
+ if (!dev->open_count) /* ... and we're the last user */
+ adu_delete(dev);
}
exit:
- if (dev)
- mutex_unlock(&dev->mtx);
+ mutex_unlock(&adutux_mutex);
dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
return retval;
}
@@ -393,12 +409,12 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
dev = file->private_data;
dbg(2," %s : dev=%p", __FUNCTION__, dev);
- /* lock this object */
+
if (mutex_lock_interruptible(&dev->mtx))
return -ERESTARTSYS;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL || dev->minor == 0) {
+ if (dev->udev == NULL) {
retval = -ENODEV;
err("No device or device unplugged %d", retval);
goto exit;
@@ -452,7 +468,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
should_submit = 1;
} else {
/* even the primary was empty - we may need to do IO */
- if (dev->interrupt_in_urb->status == -EINPROGRESS) {
+ if (!dev->read_urb_finished) {
/* somebody is doing IO */
spin_unlock_irqrestore(&dev->buflock, flags);
dbg(2," %s : submitted already", __FUNCTION__);
@@ -460,6 +476,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
/* we must initiate input */
dbg(2," %s : initiate input", __FUNCTION__);
dev->read_urb_finished = 0;
+ spin_unlock_irqrestore(&dev->buflock, flags);
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
@@ -469,15 +486,12 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
adu_interrupt_in_callback,
dev,
dev->interrupt_in_endpoint->bInterval);
- retval = usb_submit_urb(dev->interrupt_in_urb, GFP_ATOMIC);
- if (!retval) {
- spin_unlock_irqrestore(&dev->buflock, flags);
- dbg(2," %s : submitted OK", __FUNCTION__);
- } else {
+ retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
+ if (retval) {
+ dev->read_urb_finished = 1;
if (retval == -ENOMEM) {
retval = bytes_read ? bytes_read : -ENOMEM;
}
- spin_unlock_irqrestore(&dev->buflock, flags);
dbg(2," %s : submit failed", __FUNCTION__);
goto exit;
}
@@ -486,10 +500,14 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
/* we wait for I/O to complete */
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(&dev->read_wait, &wait);
- if (!dev->read_urb_finished)
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(&dev->buflock, flags);
timeout = schedule_timeout(COMMAND_TIMEOUT);
- else
+ } else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
set_current_state(TASK_RUNNING);
+ }
remove_wait_queue(&dev->read_wait, &wait);
if (timeout <= 0) {
@@ -509,19 +527,23 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
retval = bytes_read;
/* if the primary buffer is empty then use it */
- if (should_submit && !dev->interrupt_in_urb->status==-EINPROGRESS) {
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (should_submit && dev->read_urb_finished) {
+ dev->read_urb_finished = 0;
+ spin_unlock_irqrestore(&dev->buflock, flags);
usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
usb_rcvintpipe(dev->udev,
dev->interrupt_in_endpoint->bEndpointAddress),
- dev->interrupt_in_buffer,
- le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
- adu_interrupt_in_callback,
- dev,
- dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
- dev->read_urb_finished = 0;
- usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
+ dev->interrupt_in_buffer,
+ le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+ adu_interrupt_in_callback,
+ dev,
+ dev->interrupt_in_endpoint->bInterval);
+ if (usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL) != 0)
+ dev->read_urb_finished = 1;
/* we ignore failure */
+ } else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
}
exit:
@@ -535,24 +557,24 @@ exit:
static ssize_t adu_write(struct file *file, const __user char *buffer,
size_t count, loff_t *ppos)
{
+ DECLARE_WAITQUEUE(waita, current);
struct adu_device *dev;
size_t bytes_written = 0;
size_t bytes_to_write;
size_t buffer_size;
+ unsigned long flags;
int retval;
- int timeout = 0;
dbg(2," %s : enter, count = %Zd", __FUNCTION__, count);
dev = file->private_data;
- /* lock this object */
retval = mutex_lock_interruptible(&dev->mtx);
if (retval)
goto exit_nolock;
/* verify that the device wasn't unplugged */
- if (dev->udev == NULL || dev->minor == 0) {
+ if (dev->udev == NULL) {
retval = -ENODEV;
err("No device or device unplugged %d", retval);
goto exit;
@@ -564,42 +586,37 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
goto exit;
}
-
while (count > 0) {
- if (dev->interrupt_out_urb->status == -EINPROGRESS) {
- timeout = COMMAND_TIMEOUT;
+ add_wait_queue(&dev->write_wait, &waita);
+ set_current_state(TASK_INTERRUPTIBLE);
+ spin_lock_irqsave(&dev->buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(&dev->buflock, flags);
- while (timeout > 0) {
- if (signal_pending(current)) {
+ mutex_unlock(&dev->mtx);
+ if (signal_pending(current)) {
dbg(1," %s : interrupted", __FUNCTION__);
+ set_current_state(TASK_RUNNING);
retval = -EINTR;
- goto exit;
+ goto exit_onqueue;
}
- mutex_unlock(&dev->mtx);
- timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
+ if (schedule_timeout(COMMAND_TIMEOUT) == 0) {
+ dbg(1, "%s - command timed out.", __FUNCTION__);
+ retval = -ETIMEDOUT;
+ goto exit_onqueue;
+ }
+ remove_wait_queue(&dev->write_wait, &waita);
retval = mutex_lock_interruptible(&dev->mtx);
if (retval) {
retval = bytes_written ? bytes_written : retval;
goto exit_nolock;
}
- if (timeout > 0) {
- break;
- }
- dbg(1," %s : interrupted timeout: %d", __FUNCTION__, timeout);
- }
-
-
- dbg(1," %s : final timeout: %d", __FUNCTION__, timeout);
-
- if (timeout == 0) {
- dbg(1, "%s - command timed out.", __FUNCTION__);
- retval = -ETIMEDOUT;
- goto exit;
- }
-
- dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
+ dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
} else {
+ spin_unlock_irqrestore(&dev->buflock, flags);
+ set_current_state(TASK_RUNNING);
+ remove_wait_queue(&dev->write_wait, &waita);
dbg(4," %s : sending, count = %Zd", __FUNCTION__, count);
/* write the data into interrupt_out_buffer from userspace */
@@ -622,11 +639,12 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
bytes_to_write,
adu_interrupt_out_callback,
dev,
- dev->interrupt_in_endpoint->bInterval);
- /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+ dev->interrupt_out_endpoint->bInterval);
dev->interrupt_out_urb->actual_length = bytes_to_write;
+ dev->out_urb_finished = 0;
retval = usb_submit_urb(dev->interrupt_out_urb, GFP_KERNEL);
if (retval < 0) {
+ dev->out_urb_finished = 1;
err("Couldn't submit interrupt_out_urb %d", retval);
goto exit;
}
@@ -637,16 +655,17 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
bytes_written += bytes_to_write;
}
}
-
- retval = bytes_written;
+ mutex_unlock(&dev->mtx);
+ return bytes_written;
exit:
- /* unlock the device */
mutex_unlock(&dev->mtx);
exit_nolock:
-
dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+ return retval;
+exit_onqueue:
+ remove_wait_queue(&dev->write_wait, &waita);
return retval;
}
@@ -831,25 +850,22 @@ static void adu_disconnect(struct usb_interface *interface)
dbg(2," %s : enter", __FUNCTION__);
dev = usb_get_intfdata(interface);
- usb_set_intfdata(interface, NULL);
+ mutex_lock(&dev->mtx); /* not interruptible */
+ dev->udev = NULL; /* poison */
minor = dev->minor;
-
- /* give back our minor */
usb_deregister_dev(interface, &adu_class);
- dev->minor = 0;
+ mutex_unlock(&dev->mtx);
- mutex_lock(&dev->mtx); /* not interruptible */
+ mutex_lock(&adutux_mutex);
+ usb_set_intfdata(interface, NULL);
/* if the device is not opened, then we clean up right now */
dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
- if (!dev->open_count) {
- mutex_unlock(&dev->mtx);
+ if (!dev->open_count)
adu_delete(dev);
- } else {
- dev->udev = NULL;
- mutex_unlock(&dev->mtx);
- }
+
+ mutex_unlock(&adutux_mutex);
dev_info(&interface->dev, "ADU device adutux%d now disconnected\n",
(minor - ADU_MINOR_BASE));
-- Pete
On Tue, 2007-10-30 at 23:54, Pete Zaitcev wrote:
> One other small thing. I see you dropped the dev->mtx bracket that
> I added around the initialization and submission. Notice that the
> dev->udev is zeroed under dev->mtx, not the static lock. This is because
> it has to be seen in read and write paths. If you don't like taking
> across the submission, how about testing for it ahead of time?
I thought it can be managed under static lock.
But if you sure we can leave device lock too, I'm not familiar with all this on such deep level... at least for now;).
I'm not sure what kind of testing do you mean by "ahead of time". I just tried the latest patch and all seems to be good.
BTW, slab corruption issue that I saw on the original driver we started fixing on is not an issue any more.
Vitaliy
On Wed, 31 Oct 2007 13:54:54 +0200, Vitaliy Ivanov <[email protected]> wrote:
> On Tue, 2007-10-30 at 23:54, Pete Zaitcev wrote:
>
> > One other small thing. I see you dropped the dev->mtx bracket that
> > I added around the initialization and submission. Notice that the
> > dev->udev is zeroed under dev->mtx, not the static lock. This is because
> > it has to be seen in read and write paths. If you don't like taking
> > across the submission, how about testing for it ahead of time?
>
> I thought it can be managed under static lock.
The paragraph you quoted above explains why dev->udev cannot be managed
under the static lock: because dev->udev is accessed by read/write methods,
which do not take the static lock.
> I'm not sure what kind of testing do you mean by "ahead of time".
No, I meant testing before the rest of the ->open method is executed,
sorry. This part is "ahead of" the rest:
@@ -267,54 +290,54 @@ static int adu_open(struct inode *inode, struct file *file)
}
dev = usb_get_intfdata(interface);
- if (!dev) {
+ if (!dev || !dev->udev) {
retval = -ENODEV;
goto exit_no_device;
}
Sorry about that. I'll try to be more explicit in the future.
> I just tried the latest patch and all seems to be good.
> BTW, slab corruption issue that I saw on the original driver we started fixing on is not an issue any more.
Very well, I'll post this for Greg anew today.
Do you still want to go ahead with a 2.4 backport?
-- Pete
On Thu, 2007-11-01 at 00:01, Pete Zaitcev wrote:
> Sorry about that. I'll try to be more explicit in the future.
OK, got it. Thanks.
> > I just tried the latest patch and all seems to be good.
> > BTW, slab corruption issue that I saw on the original driver we started fixing on is not an issue any more.
>
> Very well, I'll post this for Greg anew today.
Great, tnx.
> Do you still want to go ahead with a 2.4 backport?
Yep. Will do it asap. In latest 2.4 patch we had just a locks issue.
So do you think I should modify 2.4 patch with all modifications we done in 2.6 version included?
Vitaliy
On Thu, 01 Nov 2007 11:06:24 +0200, Vitaliy Ivanov <[email protected]> wrote:
> > Do you still want to go ahead with a 2.4 backport?
>
> Yep. Will do it asap. In latest 2.4 patch we had just a locks issue.
> So do you think I should modify 2.4 patch with all modifications we done in 2.6 version included?
Something like that, yes.
-- Pete