From: Stefani Seibold <stefani@xxxxxxxxxxx>
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 <stefani@xxxxxxxxxxx>
---
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 (greg@xxxxxxxxx)
+ * fixes by Stefani Seibold (stefani@xxxxxxxxxxx)
*
* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
