From: Stefani Seibold <[email protected]>
This is a fix for the USB skeleton driver to bring it in shape.
- The usb_interface structure pointer will be no longer stored
- Every access to the USB will be handled trought the usb_interface pointer
- Add a new bool 'connected' for signaling a disconnect (== false)
- Handle a non blocking read without blocking
- Code cleanup
- Synchronize disconnect() handler with open() and release(), to fix races
- Introduced fsync
- Single user mode
- More code cleanup :-)
- Save some bytes in the dev structure
Some word about the open()/release() races with disconnect() of an USB device
(which can happen any time):
- The return interface pointer from usb_find_interface() could be already owned
by an other driver and no more longer handle by the skeleton driver.
- Or the dev pointer return by usb_get_intfdata() could point to an already
release memory.
This races can not handled by a per device mutex. Only a driver global mutex
could do this job, since the kref_put() in the skel_disconnect() must be
protected, otherwise skel_open() could access an already released memory.
I know that this races are very small, but on heavy load or misdesigned or buggy
hardware this could lead in a OOPS or unpredictable behavior.
The patch is against linux 3.5.0 commit eea5b5510fc5545d15b69da8e485a7424ae388cf
Grek ask me to do this in more pieces, but i will post it for a first RFC.
Hope this helps
Signed-off-by: Stefani Seibold <[email protected]>
---
drivers/usb/usb-skeleton.c | 218 ++++++++++++++++++++++----------------------
1 files changed, 108 insertions(+), 110 deletions(-)
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 0616f23..fce5a54 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -1,7 +1,8 @@
/*
- * USB Skeleton driver - 2.2
+ * USB Skeleton driver - 2.3
*
* Copyright (C) 2001-2004 Greg Kroah-Hartman ([email protected])
+ * fixes by Stefani Seibold ([email protected])
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License as
@@ -48,8 +49,7 @@ MODULE_DEVICE_TABLE(usb, skel_table);
/* Structure to hold all of our device specific stuff */
struct usb_skel {
- struct usb_device *udev; /* the usb device for this device */
- struct usb_interface *interface; /* the interface for this device */
+ struct usb_device *udev; /* the usb device */
struct semaphore limit_sem; /* limiting the number of writes in progress */
struct usb_anchor submitted; /* in case we need to retract our submissions */
struct urb *bulk_in_urb; /* the urb to read data with */
@@ -62,15 +62,19 @@ struct usb_skel {
int errors; /* the last request tanked */
bool ongoing_read; /* a read is going on */
bool processed_urb; /* indicates we haven't processed the urb */
+ bool connected; /* connected flag */
+ bool in_use; /* in use flag */
spinlock_t err_lock; /* lock for errors */
struct kref kref;
- struct mutex io_mutex; /* synchronize I/O with disconnect */
+ struct mutex io_mutex; /* synchronize I/O */
struct completion bulk_in_completion; /* to wait for an ongoing read */
};
#define to_skel_dev(d) container_of(d, struct usb_skel, kref)
static struct usb_driver skel_driver;
-static void skel_draw_down(struct usb_skel *dev);
+
+/* synchronize open/release with disconnect */
+static DEFINE_MUTEX(sync_mutex);
static void skel_delete(struct kref *kref)
{
@@ -86,15 +90,13 @@ static int skel_open(struct inode *inode, struct file *file)
{
struct usb_skel *dev;
struct usb_interface *interface;
- int subminor;
- int retval = 0;
+ int retval;
- subminor = iminor(inode);
+ /* lock against skel_disconnect() */
+ mutex_lock(&sync_mutex);
- interface = usb_find_interface(&skel_driver, subminor);
+ interface = usb_find_interface(&skel_driver, iminor(inode));
if (!interface) {
- pr_err("%s - error, can't find device for minor %d\n",
- __func__, subminor);
retval = -ENODEV;
goto exit;
}
@@ -105,52 +107,61 @@ static int skel_open(struct inode *inode, struct file *file)
goto exit;
}
- /* increment our usage count for the device */
- kref_get(&dev->kref);
-
- /* lock the device to allow correctly handling errors
- * in resumption */
- mutex_lock(&dev->io_mutex);
+ if (dev->in_use) {
+ retval = -EBUSY;
+ goto exit;
+ }
retval = usb_autopm_get_interface(interface);
if (retval)
- goto out_err;
+ goto exit;
+
+ /* increment our usage count for the device */
+ kref_get(&dev->kref);
+
+ dev->in_use = true;
+ mutex_unlock(&sync_mutex);
/* save our object in the file's private structure */
file->private_data = dev;
- mutex_unlock(&dev->io_mutex);
-
+ return 0;
exit:
+ mutex_unlock(&sync_mutex);
return retval;
}
static int skel_release(struct inode *inode, struct file *file)
{
- struct usb_skel *dev;
-
- dev = file->private_data;
- if (dev == NULL)
- return -ENODEV;
+ struct usb_skel *dev = file->private_data;
+ /* lock against skel_disconnect() */
+ mutex_lock(&sync_mutex);
/* allow the device to be autosuspended */
- mutex_lock(&dev->io_mutex);
- if (dev->interface)
- usb_autopm_put_interface(dev->interface);
- mutex_unlock(&dev->io_mutex);
+ if (dev->connected)
+ usb_autopm_put_interface(
+ usb_find_interface(&skel_driver, iminor(inode)));
+ dev->in_use = false;
/* decrement the count on our device */
kref_put(&dev->kref, skel_delete);
+ mutex_unlock(&sync_mutex);
return 0;
}
-static int skel_flush(struct file *file, fl_owner_t id)
+static void skel_draw_down(struct usb_skel *dev)
{
- struct usb_skel *dev;
- int res;
+ int time;
+
+ time = usb_wait_anchor_empty_timeout(&dev->submitted, 1000);
+ if (!time)
+ usb_kill_anchored_urbs(&dev->submitted);
+ usb_kill_urb(dev->bulk_in_urb);
+}
- dev = file->private_data;
- if (dev == NULL)
- return -ENODEV;
+static int skel_fsync(struct file *file, loff_t start, loff_t end, int datasync)
+{
+ struct usb_skel *dev = file->private_data;
+ int res;
/* wait for io to stop */
mutex_lock(&dev->io_mutex);
@@ -167,6 +178,11 @@ static int skel_flush(struct file *file, fl_owner_t id)
return res;
}
+static int skel_flush(struct file *file, fl_owner_t id)
+{
+ return skel_fsync(file, 0, LLONG_MAX, 0);
+}
+
static void skel_read_bulk_callback(struct urb *urb)
{
struct usb_skel *dev;
@@ -179,7 +195,7 @@ static void skel_read_bulk_callback(struct urb *urb)
if (!(urb->status == -ENOENT ||
urb->status == -ECONNRESET ||
urb->status == -ESHUTDOWN))
- dev_err(&dev->interface->dev,
+ dev_err(&urb->dev->dev,
"%s - nonzero write bulk status received: %d\n",
__func__, urb->status);
@@ -187,7 +203,7 @@ static void skel_read_bulk_callback(struct urb *urb)
} else {
dev->bulk_in_filled = urb->actual_length;
}
- dev->ongoing_read = 0;
+ dev->ongoing_read = false;
spin_unlock(&dev->err_lock);
complete(&dev->bulk_in_completion);
@@ -195,7 +211,7 @@ static void skel_read_bulk_callback(struct urb *urb)
static int skel_do_read_io(struct usb_skel *dev, size_t count)
{
- int rv;
+ int retval;
/* prepare a read */
usb_fill_bulk_urb(dev->bulk_in_urb,
@@ -207,67 +223,61 @@ static int skel_do_read_io(struct usb_skel *dev, size_t count)
skel_read_bulk_callback,
dev);
/* tell everybody to leave the URB alone */
- spin_lock_irq(&dev->err_lock);
- dev->ongoing_read = 1;
- spin_unlock_irq(&dev->err_lock);
+ dev->ongoing_read = true;
/* do it */
- rv = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL);
- if (rv < 0) {
- dev_err(&dev->interface->dev,
+ retval = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL);
+ if (retval < 0) {
+ dev_err(&dev->udev->dev,
"%s - failed submitting read urb, error %d\n",
- __func__, rv);
+ __func__, retval);
dev->bulk_in_filled = 0;
- rv = (rv == -ENOMEM) ? rv : -EIO;
- spin_lock_irq(&dev->err_lock);
- dev->ongoing_read = 0;
- spin_unlock_irq(&dev->err_lock);
+ retval = (retval == -ENOMEM) ? retval : -EIO;
+ dev->ongoing_read = false;
}
- return rv;
+ return retval;
}
static ssize_t skel_read(struct file *file, char *buffer, size_t count,
loff_t *ppos)
{
- struct usb_skel *dev;
- int rv;
- bool ongoing_io;
-
- dev = file->private_data;
+ struct usb_skel *dev = file->private_data;
+ int retval;
/* if we cannot read at all, return EOF */
if (!dev->bulk_in_urb || !count)
return 0;
/* no concurrent readers */
- rv = mutex_lock_interruptible(&dev->io_mutex);
- if (rv < 0)
- return rv;
+ if (file->f_flags & O_NONBLOCK) {
+ if (!mutex_trylock(&dev->io_mutex))
+ return -EAGAIN;
+ } else {
+ retval = mutex_lock_interruptible(&dev->io_mutex);
+ if (retval < 0)
+ return retval;
+ }
- if (!dev->interface) { /* disconnect() was called */
- rv = -ENODEV;
+ if (!dev->connected) { /* disconnect() was called */
+ retval = -ENODEV;
goto exit;
}
/* if IO is under way, we must not touch things */
retry:
- spin_lock_irq(&dev->err_lock);
- ongoing_io = dev->ongoing_read;
- spin_unlock_irq(&dev->err_lock);
-
- if (ongoing_io) {
+ if (dev->ongoing_read) {
/* nonblocking IO shall not wait */
if (file->f_flags & O_NONBLOCK) {
- rv = -EAGAIN;
+ retval = -EAGAIN;
goto exit;
}
/*
* IO may take forever
* hence wait in an interruptible state
*/
- rv = wait_for_completion_interruptible(&dev->bulk_in_completion);
- if (rv < 0)
+ retval = wait_for_completion_interruptible(&dev->bulk_in_completion);
+ if (retval < 0)
goto exit;
/*
* by waiting we also semiprocessed the urb
@@ -288,12 +298,12 @@ retry:
}
/* errors must be reported */
- rv = dev->errors;
- if (rv < 0) {
+ retval = dev->errors;
+ if (retval < 0) {
/* any error is reported once */
dev->errors = 0;
- /* to preserve notifications about reset */
- rv = (rv == -EPIPE) ? rv : -EIO;
+ /* to preseretvale notifications about reset */
+ retval = (retval == -EPIPE) ? retval : -EIO;
/* no data to deliver */
dev->bulk_in_filled = 0;
/* report it */
@@ -315,8 +325,8 @@ retry:
* all data has been used
* actual IO needs to be done
*/
- rv = skel_do_read_io(dev, count);
- if (rv < 0)
+ retval = skel_do_read_io(dev, count);
+ if (retval < 0)
goto exit;
else
goto retry;
@@ -329,9 +339,9 @@ retry:
if (copy_to_user(buffer,
dev->bulk_in_buffer + dev->bulk_in_copied,
chunk))
- rv = -EFAULT;
+ retval = -EFAULT;
else
- rv = chunk;
+ retval = chunk;
dev->bulk_in_copied += chunk;
@@ -343,16 +353,16 @@ retry:
skel_do_read_io(dev, count - chunk);
} else {
/* no data in the buffer */
- rv = skel_do_read_io(dev, count);
- if (rv < 0)
+ retval = skel_do_read_io(dev, count);
+ if (retval < 0)
goto exit;
else if (!(file->f_flags & O_NONBLOCK))
goto retry;
- rv = -EAGAIN;
+ retval = -EAGAIN;
}
exit:
mutex_unlock(&dev->io_mutex);
- return rv;
+ return retval;
}
static void skel_write_bulk_callback(struct urb *urb)
@@ -366,7 +376,7 @@ static void skel_write_bulk_callback(struct urb *urb)
if (!(urb->status == -ENOENT ||
urb->status == -ECONNRESET ||
urb->status == -ESHUTDOWN))
- dev_err(&dev->interface->dev,
+ dev_err(&urb->dev->dev,
"%s - nonzero write bulk status received: %d\n",
__func__, urb->status);
@@ -384,14 +394,12 @@ static void skel_write_bulk_callback(struct urb *urb)
static ssize_t skel_write(struct file *file, const char *user_buffer,
size_t count, loff_t *ppos)
{
- struct usb_skel *dev;
+ struct usb_skel *dev = file->private_data;
int retval = 0;
struct urb *urb = NULL;
char *buf = NULL;
size_t writesize = min(count, (size_t)MAX_TRANSFER);
- dev = file->private_data;
-
/* verify that we actually have some data to write */
if (count == 0)
goto exit;
@@ -444,9 +452,7 @@ static ssize_t skel_write(struct file *file, const char *user_buffer,
}
/* this lock makes sure we don't submit URBs to gone devices */
- mutex_lock(&dev->io_mutex);
- if (!dev->interface) { /* disconnect() was called */
- mutex_unlock(&dev->io_mutex);
+ if (!dev->connected) { /* disconnect() was called */
retval = -ENODEV;
goto error;
}
@@ -460,9 +466,8 @@ static ssize_t skel_write(struct file *file, const char *user_buffer,
/* send the data out the bulk port */
retval = usb_submit_urb(urb, GFP_KERNEL);
- mutex_unlock(&dev->io_mutex);
if (retval) {
- dev_err(&dev->interface->dev,
+ dev_err(&dev->udev->dev,
"%s - failed submitting write urb, error %d\n",
__func__, retval);
goto error_unanchor;
@@ -496,6 +501,7 @@ static const struct file_operations skel_fops = {
.write = skel_write,
.open = skel_open,
.release = skel_release,
+ .fsync = skel_fsync,
.flush = skel_flush,
.llseek = noop_llseek,
};
@@ -534,7 +540,8 @@ static int skel_probe(struct usb_interface *interface,
init_completion(&dev->bulk_in_completion);
dev->udev = usb_get_dev(interface_to_usbdev(interface));
- dev->interface = interface;
+ dev->connected = true;
+ dev->in_use = false;
/* set up the endpoint information */
/* use only the first bulk-in and bulk-out endpoints */
@@ -603,35 +610,26 @@ error:
static void skel_disconnect(struct usb_interface *interface)
{
struct usb_skel *dev;
- int minor = interface->minor;
- dev = usb_get_intfdata(interface);
- usb_set_intfdata(interface, NULL);
+ dev_info(&interface->dev, "USB Skeleton disconnect #%d",
+ interface->minor);
/* give back our minor */
usb_deregister_dev(interface, &skel_class);
- /* prevent more I/O from starting */
- mutex_lock(&dev->io_mutex);
- dev->interface = NULL;
- mutex_unlock(&dev->io_mutex);
-
+ dev = usb_get_intfdata(interface);
usb_kill_anchored_urbs(&dev->submitted);
- /* decrement our usage count */
- kref_put(&dev->kref, skel_delete);
-
- dev_info(&interface->dev, "USB Skeleton #%d now disconnected", minor);
-}
+ /* lock against skel_open() and skel_release() */
+ mutex_lock(&sync_mutex);
+ usb_set_intfdata(interface, NULL);
-static void skel_draw_down(struct usb_skel *dev)
-{
- int time;
+ /* prevent more I/O from starting */
+ dev->connected = false;
- time = usb_wait_anchor_empty_timeout(&dev->submitted, 1000);
- if (!time)
- usb_kill_anchored_urbs(&dev->submitted);
- usb_kill_urb(dev->bulk_in_urb);
+ /* decrement our usage count */
+ kref_put(&dev->kref, skel_delete);
+ mutex_unlock(&sync_mutex);
}
static int skel_suspend(struct usb_interface *intf, pm_message_t message)
--
1.7.8.6
Am Mittwoch, 6. Juni 2012, 15:23:18 schrieb [email protected]:
> Grek ask me to do this in more pieces, but i will post it for a first RFC.
I've put comments into the code.
> @@ -48,8 +49,7 @@ MODULE_DEVICE_TABLE(usb, skel_table);
>
> /* Structure to hold all of our device specific stuff */
> struct usb_skel {
> - struct usb_device *udev; /* the usb device for this device */
> - struct usb_interface *interface; /* the interface for this device */
> + struct usb_device *udev; /* the usb device */
???
> struct semaphore limit_sem; /* limiting the number of writes in progress */
> struct usb_anchor submitted; /* in case we need to retract our submissions */
> struct urb *bulk_in_urb; /* the urb to read data with */
> @@ -62,15 +62,19 @@ struct usb_skel {
> int errors; /* the last request tanked */
> bool ongoing_read; /* a read is going on */
> bool processed_urb; /* indicates we haven't processed the urb */
> + bool connected; /* connected flag */
> + bool in_use; /* in use flag */
> spinlock_t err_lock; /* lock for errors */
> struct kref kref;
> - struct mutex io_mutex; /* synchronize I/O with disconnect */
> + struct mutex io_mutex; /* synchronize I/O */
> struct completion bulk_in_completion; /* to wait for an ongoing read */
> };
> #define to_skel_dev(d) container_of(d, struct usb_skel, kref)
>
> static struct usb_driver skel_driver;
> -static void skel_draw_down(struct usb_skel *dev);
> +
> +/* synchronize open/release with disconnect */
> +static DEFINE_MUTEX(sync_mutex);
>
> static void skel_delete(struct kref *kref)
> {
> @@ -86,15 +90,13 @@ static int skel_open(struct inode *inode, struct file *file)
> {
> struct usb_skel *dev;
> struct usb_interface *interface;
> - int subminor;
> - int retval = 0;
> + int retval;
>
> - subminor = iminor(inode);
> + /* lock against skel_disconnect() */
> + mutex_lock(&sync_mutex);
>
> - interface = usb_find_interface(&skel_driver, subminor);
> + interface = usb_find_interface(&skel_driver, iminor(inode));
> if (!interface) {
> - pr_err("%s - error, can't find device for minor %d\n",
> - __func__, subminor);
> retval = -ENODEV;
> goto exit;
> }
> @@ -105,52 +107,61 @@ static int skel_open(struct inode *inode, struct file *file)
> goto exit;
> }
>
> - /* increment our usage count for the device */
> - kref_get(&dev->kref);
> -
> - /* lock the device to allow correctly handling errors
> - * in resumption */
> - mutex_lock(&dev->io_mutex);
> + if (dev->in_use) {
> + retval = -EBUSY;
> + goto exit;
> + }
For an example driver we don't want exclusive open by default.
>
> retval = usb_autopm_get_interface(interface);
> if (retval)
> - goto out_err;
> + goto exit;
> +
> + /* increment our usage count for the device */
> + kref_get(&dev->kref);
> +
> + dev->in_use = true;
> + mutex_unlock(&sync_mutex);
>
> /* save our object in the file's private structure */
> file->private_data = dev;
> - mutex_unlock(&dev->io_mutex);
> -
> + return 0;
> exit:
> + mutex_unlock(&sync_mutex);
> return retval;
> }
> static void skel_read_bulk_callback(struct urb *urb)
> {
> struct usb_skel *dev;
> @@ -179,7 +195,7 @@ static void skel_read_bulk_callback(struct urb *urb)
> if (!(urb->status == -ENOENT ||
> urb->status == -ECONNRESET ||
> urb->status == -ESHUTDOWN))
> - dev_err(&dev->interface->dev,
> + dev_err(&urb->dev->dev,
> "%s - nonzero write bulk status received: %d\n",
> __func__, urb->status);
>
> @@ -187,7 +203,7 @@ static void skel_read_bulk_callback(struct urb *urb)
> } else {
> dev->bulk_in_filled = urb->actual_length;
> }
> - dev->ongoing_read = 0;
> + dev->ongoing_read = false;
And here we have a very subtle SMP race
which can be seen only in another place.
> spin_unlock(&dev->err_lock);
>
> complete(&dev->bulk_in_completion);
> @@ -195,7 +211,7 @@ static void skel_read_bulk_callback(struct urb *urb)
>
> static ssize_t skel_read(struct file *file, char *buffer, size_t count,
> loff_t *ppos)
> {
> - struct usb_skel *dev;
> - int rv;
> - bool ongoing_io;
> -
> - dev = file->private_data;
> + struct usb_skel *dev = file->private_data;
> + int retval;
>
> /* if we cannot read at all, return EOF */
> if (!dev->bulk_in_urb || !count)
> return 0;
>
> /* no concurrent readers */
> - rv = mutex_lock_interruptible(&dev->io_mutex);
> - if (rv < 0)
> - return rv;
> + if (file->f_flags & O_NONBLOCK) {
> + if (!mutex_trylock(&dev->io_mutex))
> + return -EAGAIN;
> + } else {
> + retval = mutex_lock_interruptible(&dev->io_mutex);
> + if (retval < 0)
> + return retval;
> + }
>
> - if (!dev->interface) { /* disconnect() was called */
> - rv = -ENODEV;
> + if (!dev->connected) { /* disconnect() was called */
> + retval = -ENODEV;
> goto exit;
> }
>
> /* if IO is under way, we must not touch things */
> retry:
> - spin_lock_irq(&dev->err_lock);
> - ongoing_io = dev->ongoing_read;
> - spin_unlock_irq(&dev->err_lock);
> -
> - if (ongoing_io) {
> + if (dev->ongoing_read) {
> /* nonblocking IO shall not wait */
> if (file->f_flags & O_NONBLOCK) {
> - rv = -EAGAIN;
> + retval = -EAGAIN;
> goto exit;
> }
> /*
> * IO may take forever
> * hence wait in an interruptible state
> */
> - rv = wait_for_completion_interruptible(&dev->bulk_in_completion);
> - if (rv < 0)
> + retval = wait_for_completion_interruptible(&dev->bulk_in_completion);
> + if (retval < 0)
> goto exit;
> /*
> * by waiting we also semiprocessed the urb
> @@ -288,12 +298,12 @@ retry:
> }
>
> /* errors must be reported */
> - rv = dev->errors;
> - if (rv < 0) {
> + retval = dev->errors;
And here we hit the race pointed out above. And this one is for the
gourmets of races. On some architectures we are hitting a memory
ordering race here.
You cannot be sure that dev->errors is valid if ongoing_read == false
because there is no locking involved. The CPU on which the interrupt handler
has run may have scheduled the write to ongoing_read before the write
to dev->error and you can run into the window.
You must use smp_wmb() between the writes and smp_rmb() between
the reads.
(And Greg will come after me for this suggestion and wield his canoo as a cudgel)
> + if (retval < 0) {
> /* any error is reported once */
> dev->errors = 0;
> - /* to preserve notifications about reset */
> - rv = (rv == -EPIPE) ? rv : -EIO;
> + /* to preseretvale notifications about reset */
> + retval = (retval == -EPIPE) ? retval : -EIO;
> /* no data to deliver */
> dev->bulk_in_filled = 0;
> /* report it */
> @@ -315,8 +325,8 @@ retry:
> * all data has been used
> * actual IO needs to be done
> */
> - rv = skel_do_read_io(dev, count);
> - if (rv < 0)
> + retval = skel_do_read_io(dev, count);
> + if (retval < 0)
> goto exit;
> else
> goto retry;
> @@ -329,9 +339,9 @@ retry:
> if (copy_to_user(buffer,
> dev->bulk_in_buffer + dev->bulk_in_copied,
> chunk))
> - rv = -EFAULT;
> + retval = -EFAULT;
> else
> - rv = chunk;
> + retval = chunk;
>
> dev->bulk_in_copied += chunk;
>
> @@ -343,16 +353,16 @@ retry:
> skel_do_read_io(dev, count - chunk);
> } else {
> /* no data in the buffer */
> - rv = skel_do_read_io(dev, count);
> - if (rv < 0)
> + retval = skel_do_read_io(dev, count);
> + if (retval < 0)
> goto exit;
> else if (!(file->f_flags & O_NONBLOCK))
> goto retry;
> - rv = -EAGAIN;
> + retval = -EAGAIN;
> }
> exit:
> mutex_unlock(&dev->io_mutex);
> - return rv;
> + return retval;
> }
>
Regards
Oliver
On Wed, Jun 6, 2012 at 9:23 PM, <[email protected]> wrote:
> From: Stefani Seibold <[email protected]>
>
> This is a fix for the USB skeleton driver to bring it in shape.
>
> - The usb_interface structure pointer will be no longer stored
> - Every access to the USB will be handled trought the usb_interface pointer
> - Add a new bool 'connected' for signaling a disconnect (== false)
> - Handle a non blocking read without blocking
> - Code cleanup
> - Synchronize disconnect() handler with open() and release(), to fix races
> - Introduced fsync
> - Single user mode
> - More code cleanup :-)
> - Save some bytes in the dev structure
So many purposes, you need to split your patches for review easily, :-)
>
> Some word about the open()/release() races with disconnect() of an USB device
> (which can happen any time):
> - The return interface pointer from usb_find_interface() could be already owned
> ?by an other driver and no more longer handle by the skeleton driver.
> - Or the dev pointer return by usb_get_intfdata() could point to an already
> ?release memory.
>
> This races can not handled by a per device mutex. Only a driver global mutex
> could do this job, since the kref_put() in the skel_disconnect() must be
> protected, otherwise skel_open() could access an already released memory.
>
> I know that this races are very small, but on heavy load or misdesigned or buggy
> hardware this could lead in a OOPS or unpredictable behavior.
>
> The patch is against linux 3.5.0 commit eea5b5510fc5545d15b69da8e485a7424ae388cf
>
> Grek ask me to do this in more pieces, but i will post it for a first RFC.
>
> Hope this helps
>
> Signed-off-by: Stefani Seibold <[email protected]>
> ---
> ?drivers/usb/usb-skeleton.c | ?218 ++++++++++++++++++++++----------------------
> ?1 files changed, 108 insertions(+), 110 deletions(-)
>
> diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
> index 0616f23..fce5a54 100644
> --- a/drivers/usb/usb-skeleton.c
> +++ b/drivers/usb/usb-skeleton.c
> @@ -1,7 +1,8 @@
> ?/*
> - * USB Skeleton driver - 2.2
> + * USB Skeleton driver - 2.3
> ?*
> ?* Copyright (C) 2001-2004 Greg Kroah-Hartman ([email protected])
> + * fixes by Stefani Seibold ([email protected])
> ?*
> ?* ? ? This program is free software; you can redistribute it and/or
> ?* ? ? modify it under the terms of the GNU General Public License as
> @@ -48,8 +49,7 @@ MODULE_DEVICE_TABLE(usb, skel_table);
>
> ?/* Structure to hold all of our device specific stuff */
> ?struct usb_skel {
> - ? ? ? struct usb_device ? ? ? *udev; ? ? ? ? ? ? ? ? ?/* the usb device for this device */
> - ? ? ? struct usb_interface ? ?*interface; ? ? ? ? ? ? /* the interface for this device */
> + ? ? ? struct usb_device ? ? ? *udev; ? ? ? ? ? ? ? ? ?/* the usb device */
> ? ? ? ?struct semaphore ? ? ? ?limit_sem; ? ? ? ? ? ? ?/* limiting the number of writes in progress */
> ? ? ? ?struct usb_anchor ? ? ? submitted; ? ? ? ? ? ? ?/* in case we need to retract our submissions */
> ? ? ? ?struct urb ? ? ? ? ? ? ?*bulk_in_urb; ? ? ? ? ? /* the urb to read data with */
> @@ -62,15 +62,19 @@ struct usb_skel {
> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? errors; ? ? ? ? ? ? ? ? /* the last request tanked */
> ? ? ? ?bool ? ? ? ? ? ? ? ? ? ?ongoing_read; ? ? ? ? ? /* a read is going on */
> ? ? ? ?bool ? ? ? ? ? ? ? ? ? ?processed_urb; ? ? ? ? ?/* indicates we haven't processed the urb */
> + ? ? ? bool ? ? ? ? ? ? ? ? ? ?connected; ? ? ? ? ? ? ?/* connected flag */
> + ? ? ? bool ? ? ? ? ? ? ? ? ? ?in_use; ? ? ? ? ? ? ? ? /* in use flag */
> ? ? ? ?spinlock_t ? ? ? ? ? ? ?err_lock; ? ? ? ? ? ? ? /* lock for errors */
> ? ? ? ?struct kref ? ? ? ? ? ? kref;
> - ? ? ? struct mutex ? ? ? ? ? ?io_mutex; ? ? ? ? ? ? ? /* synchronize I/O with disconnect */
> + ? ? ? struct mutex ? ? ? ? ? ?io_mutex; ? ? ? ? ? ? ? /* synchronize I/O */
> ? ? ? ?struct completion ? ? ? bulk_in_completion; ? ? /* to wait for an ongoing read */
> ?};
> ?#define to_skel_dev(d) container_of(d, struct usb_skel, kref)
>
> ?static struct usb_driver skel_driver;
> -static void skel_draw_down(struct usb_skel *dev);
> +
> +/* synchronize open/release with disconnect */
> +static DEFINE_MUTEX(sync_mutex);
>
> ?static void skel_delete(struct kref *kref)
> ?{
> @@ -86,15 +90,13 @@ static int skel_open(struct inode *inode, struct file *file)
> ?{
> ? ? ? ?struct usb_skel *dev;
> ? ? ? ?struct usb_interface *interface;
> - ? ? ? int subminor;
> - ? ? ? int retval = 0;
> + ? ? ? int retval;
>
> - ? ? ? subminor = iminor(inode);
> + ? ? ? /* lock against skel_disconnect() */
> + ? ? ? mutex_lock(&sync_mutex);
This one is not needed since we have minor_rwsem(drivers/usb/core/file.c)
to avoid the race.
>
> - ? ? ? interface = usb_find_interface(&skel_driver, subminor);
> + ? ? ? interface = usb_find_interface(&skel_driver, iminor(inode));
> ? ? ? ?if (!interface) {
> - ? ? ? ? ? ? ? pr_err("%s - error, can't find device for minor %d\n",
> - ? ? ? ? ? ? ? ? ? ? ? __func__, subminor);
> ? ? ? ? ? ? ? ?retval = -ENODEV;
> ? ? ? ? ? ? ? ?goto exit;
> ? ? ? ?}
> @@ -105,52 +107,61 @@ static int skel_open(struct inode *inode, struct file *file)
> ? ? ? ? ? ? ? ?goto exit;
> ? ? ? ?}
>
> - ? ? ? /* increment our usage count for the device */
> - ? ? ? kref_get(&dev->kref);
> -
> - ? ? ? /* lock the device to allow correctly handling errors
> - ? ? ? ?* in resumption */
> - ? ? ? mutex_lock(&dev->io_mutex);
> + ? ? ? if (dev->in_use) {
> + ? ? ? ? ? ? ? retval = -EBUSY;
> + ? ? ? ? ? ? ? goto exit;
> + ? ? ? }
>
> ? ? ? ?retval = usb_autopm_get_interface(interface);
> ? ? ? ?if (retval)
> - ? ? ? ? ? ? ? goto out_err;
> + ? ? ? ? ? ? ? goto exit;
> +
> + ? ? ? /* increment our usage count for the device */
> + ? ? ? kref_get(&dev->kref);
> +
> + ? ? ? dev->in_use = true;
> + ? ? ? mutex_unlock(&sync_mutex);
>
> ? ? ? ?/* save our object in the file's private structure */
> ? ? ? ?file->private_data = dev;
> - ? ? ? mutex_unlock(&dev->io_mutex);
> -
> + ? ? ? return 0;
> ?exit:
> + ? ? ? mutex_unlock(&sync_mutex);
> ? ? ? ?return retval;
> ?}
>
> ?static int skel_release(struct inode *inode, struct file *file)
> ?{
> - ? ? ? struct usb_skel *dev;
> -
> - ? ? ? dev = file->private_data;
> - ? ? ? if (dev == NULL)
> - ? ? ? ? ? ? ? return -ENODEV;
> + ? ? ? struct usb_skel *dev = file->private_data;
>
> + ? ? ? /* lock against skel_disconnect() */
> + ? ? ? mutex_lock(&sync_mutex);
Since the reference count is held now, so is there any race between
release and disconnect?
> ? ? ? ?/* allow the device to be autosuspended */
> - ? ? ? mutex_lock(&dev->io_mutex);
> - ? ? ? if (dev->interface)
> - ? ? ? ? ? ? ? usb_autopm_put_interface(dev->interface);
> - ? ? ? mutex_unlock(&dev->io_mutex);
> + ? ? ? if (dev->connected)
> + ? ? ? ? ? ? ? usb_autopm_put_interface(
> + ? ? ? ? ? ? ? ? ? ? ? usb_find_interface(&skel_driver, iminor(inode)));
> + ? ? ? dev->in_use = false;
>
> ? ? ? ?/* decrement the count on our device */
> ? ? ? ?kref_put(&dev->kref, skel_delete);
> + ? ? ? mutex_unlock(&sync_mutex);
> ? ? ? ?return 0;
> ?}
>
> -static int skel_flush(struct file *file, fl_owner_t id)
> +static void skel_draw_down(struct usb_skel *dev)
> ?{
> - ? ? ? struct usb_skel *dev;
> - ? ? ? int res;
> + ? ? ? int time;
> +
> + ? ? ? time = usb_wait_anchor_empty_timeout(&dev->submitted, 1000);
> + ? ? ? if (!time)
> + ? ? ? ? ? ? ? usb_kill_anchored_urbs(&dev->submitted);
> + ? ? ? usb_kill_urb(dev->bulk_in_urb);
> +}
>
> - ? ? ? dev = file->private_data;
> - ? ? ? if (dev == NULL)
> - ? ? ? ? ? ? ? return -ENODEV;
> +static int skel_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> +{
> + ? ? ? struct usb_skel *dev = file->private_data;
> + ? ? ? int res;
>
> ? ? ? ?/* wait for io to stop */
> ? ? ? ?mutex_lock(&dev->io_mutex);
> @@ -167,6 +178,11 @@ static int skel_flush(struct file *file, fl_owner_t id)
> ? ? ? ?return res;
> ?}
>
> +static int skel_flush(struct file *file, fl_owner_t id)
> +{
> + ? ? ? return skel_fsync(file, 0, LLONG_MAX, 0);
> +}
> +
> ?static void skel_read_bulk_callback(struct urb *urb)
> ?{
> ? ? ? ?struct usb_skel *dev;
> @@ -179,7 +195,7 @@ static void skel_read_bulk_callback(struct urb *urb)
> ? ? ? ? ? ? ? ?if (!(urb->status == -ENOENT ||
> ? ? ? ? ? ? ? ? ? ?urb->status == -ECONNRESET ||
> ? ? ? ? ? ? ? ? ? ?urb->status == -ESHUTDOWN))
> - ? ? ? ? ? ? ? ? ? ? ? dev_err(&dev->interface->dev,
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(&urb->dev->dev,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"%s - nonzero write bulk status received: %d\n",
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__func__, urb->status);
>
> @@ -187,7 +203,7 @@ static void skel_read_bulk_callback(struct urb *urb)
> ? ? ? ?} else {
> ? ? ? ? ? ? ? ?dev->bulk_in_filled = urb->actual_length;
> ? ? ? ?}
> - ? ? ? dev->ongoing_read = 0;
> + ? ? ? dev->ongoing_read = false;
> ? ? ? ?spin_unlock(&dev->err_lock);
>
> ? ? ? ?complete(&dev->bulk_in_completion);
> @@ -195,7 +211,7 @@ static void skel_read_bulk_callback(struct urb *urb)
>
> ?static int skel_do_read_io(struct usb_skel *dev, size_t count)
> ?{
> - ? ? ? int rv;
> + ? ? ? int retval;
>
> ? ? ? ?/* prepare a read */
> ? ? ? ?usb_fill_bulk_urb(dev->bulk_in_urb,
> @@ -207,67 +223,61 @@ static int skel_do_read_io(struct usb_skel *dev, size_t count)
> ? ? ? ? ? ? ? ? ? ? ? ?skel_read_bulk_callback,
> ? ? ? ? ? ? ? ? ? ? ? ?dev);
> ? ? ? ?/* tell everybody to leave the URB alone */
> - ? ? ? spin_lock_irq(&dev->err_lock);
> - ? ? ? dev->ongoing_read = 1;
> - ? ? ? spin_unlock_irq(&dev->err_lock);
> + ? ? ? dev->ongoing_read = true;
>
> ? ? ? ?/* do it */
> - ? ? ? rv = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL);
> - ? ? ? if (rv < 0) {
> - ? ? ? ? ? ? ? dev_err(&dev->interface->dev,
> + ? ? ? retval = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL);
> + ? ? ? if (retval < 0) {
> + ? ? ? ? ? ? ? dev_err(&dev->udev->dev,
> ? ? ? ? ? ? ? ? ? ? ? ?"%s - failed submitting read urb, error %d\n",
> - ? ? ? ? ? ? ? ? ? ? ? __func__, rv);
> + ? ? ? ? ? ? ? ? ? ? ? __func__, retval);
> ? ? ? ? ? ? ? ?dev->bulk_in_filled = 0;
> - ? ? ? ? ? ? ? rv = (rv == -ENOMEM) ? rv : -EIO;
> - ? ? ? ? ? ? ? spin_lock_irq(&dev->err_lock);
> - ? ? ? ? ? ? ? dev->ongoing_read = 0;
> - ? ? ? ? ? ? ? spin_unlock_irq(&dev->err_lock);
> + ? ? ? ? ? ? ? retval = (retval == -ENOMEM) ? retval : -EIO;
> + ? ? ? ? ? ? ? dev->ongoing_read = false;
> ? ? ? ?}
>
> - ? ? ? return rv;
> + ? ? ? return retval;
> ?}
>
> ?static ssize_t skel_read(struct file *file, char *buffer, size_t count,
> ? ? ? ? ? ? ? ? ? ? ? ? loff_t *ppos)
> ?{
> - ? ? ? struct usb_skel *dev;
> - ? ? ? int rv;
> - ? ? ? bool ongoing_io;
> -
> - ? ? ? dev = file->private_data;
> + ? ? ? struct usb_skel *dev = file->private_data;
> + ? ? ? int retval;
>
> ? ? ? ?/* if we cannot read at all, return EOF */
> ? ? ? ?if (!dev->bulk_in_urb || !count)
> ? ? ? ? ? ? ? ?return 0;
>
> ? ? ? ?/* no concurrent readers */
> - ? ? ? rv = mutex_lock_interruptible(&dev->io_mutex);
> - ? ? ? if (rv < 0)
> - ? ? ? ? ? ? ? return rv;
> + ? ? ? if (file->f_flags & O_NONBLOCK) {
> + ? ? ? ? ? ? ? if (!mutex_trylock(&dev->io_mutex))
> + ? ? ? ? ? ? ? ? ? ? ? return -EAGAIN;
> + ? ? ? } else {
> + ? ? ? ? ? ? ? retval = mutex_lock_interruptible(&dev->io_mutex);
> + ? ? ? ? ? ? ? if (retval < 0)
> + ? ? ? ? ? ? ? ? ? ? ? return retval;
> + ? ? ? }
>
> - ? ? ? if (!dev->interface) { ? ? ? ? ?/* disconnect() was called */
> - ? ? ? ? ? ? ? rv = -ENODEV;
> + ? ? ? if (!dev->connected) { ? ? ? ? ?/* disconnect() was called */
> + ? ? ? ? ? ? ? retval = -ENODEV;
> ? ? ? ? ? ? ? ?goto exit;
> ? ? ? ?}
>
> ? ? ? ?/* if IO is under way, we must not touch things */
> ?retry:
> - ? ? ? spin_lock_irq(&dev->err_lock);
> - ? ? ? ongoing_io = dev->ongoing_read;
> - ? ? ? spin_unlock_irq(&dev->err_lock);
> -
> - ? ? ? if (ongoing_io) {
> + ? ? ? if (dev->ongoing_read) {
> ? ? ? ? ? ? ? ?/* nonblocking IO shall not wait */
> ? ? ? ? ? ? ? ?if (file->f_flags & O_NONBLOCK) {
> - ? ? ? ? ? ? ? ? ? ? ? rv = -EAGAIN;
> + ? ? ? ? ? ? ? ? ? ? ? retval = -EAGAIN;
> ? ? ? ? ? ? ? ? ? ? ? ?goto exit;
> ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ?/*
> ? ? ? ? ? ? ? ? * IO may take forever
> ? ? ? ? ? ? ? ? * hence wait in an interruptible state
> ? ? ? ? ? ? ? ? */
> - ? ? ? ? ? ? ? rv = wait_for_completion_interruptible(&dev->bulk_in_completion);
> - ? ? ? ? ? ? ? if (rv < 0)
> + ? ? ? ? ? ? ? retval = wait_for_completion_interruptible(&dev->bulk_in_completion);
> + ? ? ? ? ? ? ? if (retval < 0)
> ? ? ? ? ? ? ? ? ? ? ? ?goto exit;
> ? ? ? ? ? ? ? ?/*
> ? ? ? ? ? ? ? ? * by waiting we also semiprocessed the urb
> @@ -288,12 +298,12 @@ retry:
> ? ? ? ?}
>
> ? ? ? ?/* errors must be reported */
> - ? ? ? rv = dev->errors;
> - ? ? ? if (rv < 0) {
> + ? ? ? retval = dev->errors;
> + ? ? ? if (retval < 0) {
> ? ? ? ? ? ? ? ?/* any error is reported once */
> ? ? ? ? ? ? ? ?dev->errors = 0;
> - ? ? ? ? ? ? ? /* to preserve notifications about reset */
> - ? ? ? ? ? ? ? rv = (rv == -EPIPE) ? rv : -EIO;
> + ? ? ? ? ? ? ? /* to preseretvale notifications about reset */
> + ? ? ? ? ? ? ? retval = (retval == -EPIPE) ? retval : -EIO;
> ? ? ? ? ? ? ? ?/* no data to deliver */
> ? ? ? ? ? ? ? ?dev->bulk_in_filled = 0;
> ? ? ? ? ? ? ? ?/* report it */
> @@ -315,8 +325,8 @@ retry:
> ? ? ? ? ? ? ? ? ? ? ? ? * all data has been used
> ? ? ? ? ? ? ? ? ? ? ? ? * actual IO needs to be done
> ? ? ? ? ? ? ? ? ? ? ? ? */
> - ? ? ? ? ? ? ? ? ? ? ? rv = skel_do_read_io(dev, count);
> - ? ? ? ? ? ? ? ? ? ? ? if (rv < 0)
> + ? ? ? ? ? ? ? ? ? ? ? retval = skel_do_read_io(dev, count);
> + ? ? ? ? ? ? ? ? ? ? ? if (retval < 0)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?goto exit;
> ? ? ? ? ? ? ? ? ? ? ? ?else
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?goto retry;
> @@ -329,9 +339,9 @@ retry:
> ? ? ? ? ? ? ? ?if (copy_to_user(buffer,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev->bulk_in_buffer + dev->bulk_in_copied,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? chunk))
> - ? ? ? ? ? ? ? ? ? ? ? rv = -EFAULT;
> + ? ? ? ? ? ? ? ? ? ? ? retval = -EFAULT;
> ? ? ? ? ? ? ? ?else
> - ? ? ? ? ? ? ? ? ? ? ? rv = chunk;
> + ? ? ? ? ? ? ? ? ? ? ? retval = chunk;
>
> ? ? ? ? ? ? ? ?dev->bulk_in_copied += chunk;
>
> @@ -343,16 +353,16 @@ retry:
> ? ? ? ? ? ? ? ? ? ? ? ?skel_do_read_io(dev, count - chunk);
> ? ? ? ?} else {
> ? ? ? ? ? ? ? ?/* no data in the buffer */
> - ? ? ? ? ? ? ? rv = skel_do_read_io(dev, count);
> - ? ? ? ? ? ? ? if (rv < 0)
> + ? ? ? ? ? ? ? retval = skel_do_read_io(dev, count);
> + ? ? ? ? ? ? ? if (retval < 0)
> ? ? ? ? ? ? ? ? ? ? ? ?goto exit;
> ? ? ? ? ? ? ? ?else if (!(file->f_flags & O_NONBLOCK))
> ? ? ? ? ? ? ? ? ? ? ? ?goto retry;
> - ? ? ? ? ? ? ? rv = -EAGAIN;
> + ? ? ? ? ? ? ? retval = -EAGAIN;
> ? ? ? ?}
> ?exit:
> ? ? ? ?mutex_unlock(&dev->io_mutex);
> - ? ? ? return rv;
> + ? ? ? return retval;
> ?}
>
> ?static void skel_write_bulk_callback(struct urb *urb)
> @@ -366,7 +376,7 @@ static void skel_write_bulk_callback(struct urb *urb)
> ? ? ? ? ? ? ? ?if (!(urb->status == -ENOENT ||
> ? ? ? ? ? ? ? ? ? ?urb->status == -ECONNRESET ||
> ? ? ? ? ? ? ? ? ? ?urb->status == -ESHUTDOWN))
> - ? ? ? ? ? ? ? ? ? ? ? dev_err(&dev->interface->dev,
> + ? ? ? ? ? ? ? ? ? ? ? dev_err(&urb->dev->dev,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"%s - nonzero write bulk status received: %d\n",
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__func__, urb->status);
>
> @@ -384,14 +394,12 @@ static void skel_write_bulk_callback(struct urb *urb)
> ?static ssize_t skel_write(struct file *file, const char *user_buffer,
> ? ? ? ? ? ? ? ? ? ? ? ? ?size_t count, loff_t *ppos)
> ?{
> - ? ? ? struct usb_skel *dev;
> + ? ? ? struct usb_skel *dev = file->private_data;
> ? ? ? ?int retval = 0;
> ? ? ? ?struct urb *urb = NULL;
> ? ? ? ?char *buf = NULL;
> ? ? ? ?size_t writesize = min(count, (size_t)MAX_TRANSFER);
>
> - ? ? ? dev = file->private_data;
> -
> ? ? ? ?/* verify that we actually have some data to write */
> ? ? ? ?if (count == 0)
> ? ? ? ? ? ? ? ?goto exit;
> @@ -444,9 +452,7 @@ static ssize_t skel_write(struct file *file, const char *user_buffer,
> ? ? ? ?}
>
> ? ? ? ?/* this lock makes sure we don't submit URBs to gone devices */
> - ? ? ? mutex_lock(&dev->io_mutex);
> - ? ? ? if (!dev->interface) { ? ? ? ? ?/* disconnect() was called */
> - ? ? ? ? ? ? ? mutex_unlock(&dev->io_mutex);
> + ? ? ? if (!dev->connected) { ? ? ? ? ?/* disconnect() was called */
> ? ? ? ? ? ? ? ?retval = -ENODEV;
> ? ? ? ? ? ? ? ?goto error;
> ? ? ? ?}
> @@ -460,9 +466,8 @@ static ssize_t skel_write(struct file *file, const char *user_buffer,
>
> ? ? ? ?/* send the data out the bulk port */
> ? ? ? ?retval = usb_submit_urb(urb, GFP_KERNEL);
> - ? ? ? mutex_unlock(&dev->io_mutex);
> ? ? ? ?if (retval) {
> - ? ? ? ? ? ? ? dev_err(&dev->interface->dev,
> + ? ? ? ? ? ? ? dev_err(&dev->udev->dev,
> ? ? ? ? ? ? ? ? ? ? ? ?"%s - failed submitting write urb, error %d\n",
> ? ? ? ? ? ? ? ? ? ? ? ?__func__, retval);
> ? ? ? ? ? ? ? ?goto error_unanchor;
> @@ -496,6 +501,7 @@ static const struct file_operations skel_fops = {
> ? ? ? ?.write = ? ? ? ?skel_write,
> ? ? ? ?.open = ? ? ? ? skel_open,
> ? ? ? ?.release = ? ? ?skel_release,
> + ? ? ? .fsync = ? ? ? ?skel_fsync,
> ? ? ? ?.flush = ? ? ? ?skel_flush,
> ? ? ? ?.llseek = ? ? ? noop_llseek,
> ?};
> @@ -534,7 +540,8 @@ static int skel_probe(struct usb_interface *interface,
> ? ? ? ?init_completion(&dev->bulk_in_completion);
>
> ? ? ? ?dev->udev = usb_get_dev(interface_to_usbdev(interface));
> - ? ? ? dev->interface = interface;
> + ? ? ? dev->connected = true;
> + ? ? ? dev->in_use = false;
>
> ? ? ? ?/* set up the endpoint information */
> ? ? ? ?/* use only the first bulk-in and bulk-out endpoints */
> @@ -603,35 +610,26 @@ error:
> ?static void skel_disconnect(struct usb_interface *interface)
> ?{
> ? ? ? ?struct usb_skel *dev;
> - ? ? ? int minor = interface->minor;
>
> - ? ? ? dev = usb_get_intfdata(interface);
> - ? ? ? usb_set_intfdata(interface, NULL);
> + ? ? ? dev_info(&interface->dev, "USB Skeleton disconnect #%d",
> + ? ? ? ? ? ? ? ? ? ? ? interface->minor);
>
> ? ? ? ?/* give back our minor */
> ? ? ? ?usb_deregister_dev(interface, &skel_class);
>
> - ? ? ? /* prevent more I/O from starting */
> - ? ? ? mutex_lock(&dev->io_mutex);
> - ? ? ? dev->interface = NULL;
> - ? ? ? mutex_unlock(&dev->io_mutex);
> -
> + ? ? ? dev = usb_get_intfdata(interface);
> ? ? ? ?usb_kill_anchored_urbs(&dev->submitted);
>
> - ? ? ? /* decrement our usage count */
> - ? ? ? kref_put(&dev->kref, skel_delete);
> -
> - ? ? ? dev_info(&interface->dev, "USB Skeleton #%d now disconnected", minor);
> -}
> + ? ? ? /* lock against skel_open() and skel_release() */
> + ? ? ? mutex_lock(&sync_mutex);
> + ? ? ? usb_set_intfdata(interface, NULL);
>
> -static void skel_draw_down(struct usb_skel *dev)
> -{
> - ? ? ? int time;
> + ? ? ? /* prevent more I/O from starting */
> + ? ? ? dev->connected = false;
>
> - ? ? ? time = usb_wait_anchor_empty_timeout(&dev->submitted, 1000);
> - ? ? ? if (!time)
> - ? ? ? ? ? ? ? usb_kill_anchored_urbs(&dev->submitted);
> - ? ? ? usb_kill_urb(dev->bulk_in_urb);
> + ? ? ? /* decrement our usage count */
> + ? ? ? kref_put(&dev->kref, skel_delete);
> + ? ? ? mutex_unlock(&sync_mutex);
> ?}
>
> ?static int skel_suspend(struct usb_interface *intf, pm_message_t message)
> --
> 1.7.8.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
Thanks,
--
Ming Lei
Am Mittwoch, den 06.06.2012, 22:28 +0800 schrieb Ming Lei:
> On Wed, Jun 6, 2012 at 9:23 PM, <[email protected]> wrote:
> > From: Stefani Seibold <[email protected]>
> >
> > This is a fix for the USB skeleton driver to bring it in shape.
> >
> > - The usb_interface structure pointer will be no longer stored
> > - Every access to the USB will be handled trought the usb_interface pointer
> > - Add a new bool 'connected' for signaling a disconnect (== false)
> > - Handle a non blocking read without blocking
> > - Code cleanup
> > - Synchronize disconnect() handler with open() and release(), to fix races
> > - Introduced fsync
> > - Single user mode
> > - More code cleanup :-)
> > - Save some bytes in the dev structure
>
> So many purposes, you need to split your patches for review easily, :-)
>
This is the next step :-)
> > static void skel_delete(struct kref *kref)
> > {
> > @@ -86,15 +90,13 @@ static int skel_open(struct inode *inode, struct file *file)
> > {
> > struct usb_skel *dev;
> > struct usb_interface *interface;
> > - int subminor;
> > - int retval = 0;
> > + int retval;
> >
> > - subminor = iminor(inode);
> > + /* lock against skel_disconnect() */
> > + mutex_lock(&sync_mutex);
>
> This one is not needed since we have minor_rwsem(drivers/usb/core/file.c)
> to avoid the race.
>
The mutex is not for the minor handling, it is for the disconnect(). As
mentioned in the previous posting, there is a race betwenn open() and
connect().
Oliver told me that a interface pointer can be already used by an other
driver when the disconnect() was called.
So the interface will be used to determinate the dev pointer, which can
at this time also owned by an other driver.
And at last the dev pointer could be point to an already released
memory.
So it is IMHO needed.
> >
> > static int skel_release(struct inode *inode, struct file *file)
> > {
> > - struct usb_skel *dev;
> > -
> > - dev = file->private_data;
> > - if (dev == NULL)
> > - return -ENODEV;
> > + struct usb_skel *dev = file->private_data;
> >
> > + /* lock against skel_disconnect() */
> > + mutex_lock(&sync_mutex);
>
> Since the reference count is held now, so is there any race between
> release and disconnect?
>
Same as above, the interface could be owned by an other driver.
Thanks,
Stefani
On Wed, 6 Jun 2012, Ming Lei wrote:
> On Wed, Jun 6, 2012 at 9:23 PM, <[email protected]> wrote:
> > From: Stefani Seibold <[email protected]>
> >
> > This is a fix for the USB skeleton driver to bring it in shape.
> >
> > - The usb_interface structure pointer will be no longer stored
> > - Every access to the USB will be handled trought the usb_interface pointer
> > - Add a new bool 'connected' for signaling a disconnect (== false)
> > - Handle a non blocking read without blocking
> > - Code cleanup
> > - Synchronize disconnect() handler with open() and release(), to fix races
> > - Introduced fsync
> > - Single user mode
> > - More code cleanup :-)
> > - Save some bytes in the dev structure
>
> So many purposes, you need to split your patches for review easily, :-)
Not just that... usb-skeleton is intended to be a good example, to
help people learn how to write USB drivers. It's already far too
complicated for this purpose, and adding more complication will just
make it worse.
Instead of adding things to usb-skeleton, we should take things away.
Alan Stern
Am Mittwoch, 6. Juni 2012, 17:06:59 schrieb Alan Stern:
> On Wed, 6 Jun 2012, Ming Lei wrote:
> > So many purposes, you need to split your patches for review easily, :-)
>
> Not just that... usb-skeleton is intended to be a good example, to
> help people learn how to write USB drivers. It's already far too
> complicated for this purpose, and adding more complication will just
> make it worse.
>
> Instead of adding things to usb-skeleton, we should take things away.
Not really. The features in the driver make sense. Probably we should
have two versions. One simple and basic and the other showing advanced
techniques. And a lot more comments.
Regards
Oliver
On Wed, Jun 6, 2012 at 11:05 PM, Stefani Seibold <[email protected]> wrote:
>>
>> This one is not needed since we have minor_rwsem(drivers/usb/core/file.c)
>> to avoid the race.
>>
>
> The mutex is not for the minor handling, it is for the disconnect(). As
usb_deregister_dev is called by disconnect, and usb_deregister_dev
will acquire minor_rwsem, so there is no race between open and disconnect.
When skel_open is being called, the minor_rwsem has been held already,
so disconnect() will staying on acquiring minor_rwsem.
So sync_mutex is not necessary at all.
> mentioned in the previous posting, there is a race betwenn open() and
> connect().
>
> Oliver told me that a interface pointer can be already used by an other
> driver when the disconnect() was called.
If you mean dev->interface, that is protected by io_mutex already.
Thanks,
--
Ming Lei