[PATCH] fix usb skeleton driver

[PATCH] fix usb skeleton driver

[stefani]

From: Stefani Seibold <stefani@seibold.net>

This is a fix for the USB skeleton driver to bring it in shape.

- Code cleanup
- Eliminated dead code
- Handle a non blocking read without blocking
- Fix a small race condition
- Introduced fsync
- Single user mode

The patch is against linux 3.5.0 commit eea5b5510fc5545d15b69da8e485a7424ae388cf

Hope this helps

Signed-off-by: Stefani Seibold <stefani@seibold.net>

[PATCH 01/13] fix wrong label in skel_open

[stefani]

From: Stefani Seibold <stefani@seibold.net>

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 0616f23..efda3a5 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -114,10 +114,11 @@ static int skel_open(struct inode *inode, struct file *file)
 
 	retval = usb_autopm_get_interface(interface);
 	if (retval)
-		goto out_err;
+		goto exit2;
 
 	/* save our object in the file's private structure */
 	file->private_data = dev;
+exit2:
 	mutex_unlock(&dev->io_mutex);
 
 exit:
-- 
1.7.8.6

[PATCH 02/13] code cleanup

[stefani]

From: Stefani Seibold <stefani@seibold.net>

- consistent nameing
- more compact style
- remove unneeded code

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |  119 +++++++++++++++++---------------------------
 1 files changed, 46 insertions(+), 73 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index efda3a5..7385aa8 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -22,7 +22,6 @@
 #include <linux/usb.h>
 #include <linux/mutex.h>
 
-
 /* Define these values to match your devices */
 #define USB_SKEL_VENDOR_ID	0xfff0
 #define USB_SKEL_PRODUCT_ID	0xfff0
@@ -34,7 +33,6 @@ static const struct usb_device_id skel_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, skel_table);
 
-
 /* Get a minor range for your devices from the usb maintainer */
 #define USB_SKEL_MINOR_BASE	192
 
@@ -95,15 +93,12 @@ static int skel_open(struct inode *inode, struct file *file)
 	if (!interface) {
 		pr_err("%s - error, can't find device for minor %d\n",
 			__func__, subminor);
-		retval = -ENODEV;
-		goto exit;
+		return -ENODEV;
 	}
 
 	dev = usb_get_intfdata(interface);
-	if (!dev) {
-		retval = -ENODEV;
-		goto exit;
-	}
+	if (!dev)
+		return -ENODEV;
 
 	/* increment our usage count for the device */
 	kref_get(&dev->kref);
@@ -111,27 +106,19 @@ static int skel_open(struct inode *inode, struct file *file)
 	/* lock the device to allow correctly handling errors
 	 * in resumption */
 	mutex_lock(&dev->io_mutex);
-
 	retval = usb_autopm_get_interface(interface);
-	if (retval)
-		goto exit2;
+	mutex_unlock(&dev->io_mutex);
 
 	/* save our object in the file's private structure */
-	file->private_data = dev;
-exit2:
-	mutex_unlock(&dev->io_mutex);
+	if (!retval)
+		file->private_data = dev;
 
-exit:
 	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;
 
 	/* allow the device to be autosuspended */
 	mutex_lock(&dev->io_mutex);
@@ -146,13 +133,9 @@ static int skel_release(struct inode *inode, struct file *file)
 
 static int skel_flush(struct file *file, fl_owner_t id)
 {
-	struct usb_skel *dev;
+	struct usb_skel *dev = file->private_data;
 	int res;
 
-	dev = file->private_data;
-	if (dev == NULL)
-		return -ENODEV;
-
 	/* wait for io to stop */
 	mutex_lock(&dev->io_mutex);
 	skel_draw_down(dev);
@@ -188,7 +171,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);
@@ -196,7 +179,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,
@@ -209,45 +192,43 @@ static int skel_do_read_io(struct usb_skel *dev, size_t count)
 			dev);
 	/* tell everybody to leave the URB alone */
 	spin_lock_irq(&dev->err_lock);
-	dev->ongoing_read = 1;
+	dev->ongoing_read = true;
 	spin_unlock_irq(&dev->err_lock);
 
 	/* do it */
-	rv = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL);
-	if (rv < 0) {
+	retval = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL);
+	if (retval < 0) {
 		dev_err(&dev->interface->dev,
 			"%s - failed submitting read urb, error %d\n",
-			__func__, rv);
+			__func__, retval);
 		dev->bulk_in_filled = 0;
-		rv = (rv == -ENOMEM) ? rv : -EIO;
+		retval = (retval == -ENOMEM) ? retval : -EIO;
 		spin_lock_irq(&dev->err_lock);
-		dev->ongoing_read = 0;
+		dev->ongoing_read = false;
 		spin_unlock_irq(&dev->err_lock);
 	}
 
-	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;
+	struct usb_skel *dev = file->private_data;
+	int retval;
 	bool ongoing_io;
 
-	dev = file->private_data;
-
 	/* 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;
+	retval = mutex_lock_interruptible(&dev->io_mutex);
+	if (retval < 0)
+		return retval;
 
 	if (!dev->interface) {		/* disconnect() was called */
-		rv = -ENODEV;
+		retval = -ENODEV;
 		goto exit;
 	}
 
@@ -260,22 +241,22 @@ retry:
 	if (ongoing_io) {
 		/* 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
 		 * we must finish now
 		 */
 		dev->bulk_in_copied = 0;
-		dev->processed_urb = 1;
+		dev->processed_urb = true;
 	}
 
 	if (!dev->processed_urb) {
@@ -285,16 +266,16 @@ retry:
 		 */
 		wait_for_completion(&dev->bulk_in_completion);
 		dev->bulk_in_copied = 0;
-		dev->processed_urb = 1;
+		dev->processed_urb = true;
 	}
 
 	/* 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;
+		retval = (retval == -EPIPE) ? retval : -EIO;
 		/* no data to deliver */
 		dev->bulk_in_filled = 0;
 		/* report it */
@@ -316,8 +297,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;
@@ -330,9 +311,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;
 
@@ -344,16 +325,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)
@@ -385,32 +366,26 @@ 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;
+	if (!count)
+		return 0;
 
 	/*
 	 * limit the number of URBs in flight to stop a user from using up all
 	 * RAM
 	 */
 	if (!(file->f_flags & O_NONBLOCK)) {
-		if (down_interruptible(&dev->limit_sem)) {
-			retval = -ERESTARTSYS;
-			goto exit;
-		}
+		if (down_interruptible(&dev->limit_sem))
+			return -ERESTARTSYS;
 	} else {
-		if (down_trylock(&dev->limit_sem)) {
-			retval = -EAGAIN;
-			goto exit;
-		}
+		if (down_trylock(&dev->limit_sem))
+			return -EAGAIN;
 	}
 
 	spin_lock_irq(&dev->err_lock);
@@ -475,7 +450,6 @@ static ssize_t skel_write(struct file *file, const char *user_buffer,
 	 */
 	usb_free_urb(urb);
 
-
 	return writesize;
 
 error_unanchor:
@@ -487,7 +461,6 @@ error:
 	}
 	up(&dev->limit_sem);
 
-exit:
 	return retval;
 }
 
-- 
1.7.8.6

[PATCH 03/13] remove dead code

[stefani]

From: Stefani Seibold <stefani@seibold.net>

All code which use processed_urb can be eliminated.

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 7385aa8..92cdfca 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -59,7 +59,6 @@ struct usb_skel {
 	__u8			bulk_out_endpointAddr;	/* the address of the bulk out endpoint */
 	int			errors;			/* the last request tanked */
 	bool			ongoing_read;		/* a read is going on */
-	bool			processed_urb;		/* indicates we haven't processed the urb */
 	spinlock_t		err_lock;		/* lock for errors */
 	struct kref		kref;
 	struct mutex		io_mutex;		/* synchronize I/O with disconnect */
@@ -256,17 +255,6 @@ retry:
 		 * we must finish now
 		 */
 		dev->bulk_in_copied = 0;
-		dev->processed_urb = true;
-	}
-
-	if (!dev->processed_urb) {
-		/*
-		 * the URB hasn't been processed
-		 * do it now
-		 */
-		wait_for_completion(&dev->bulk_in_completion);
-		dev->bulk_in_copied = 0;
-		dev->processed_urb = true;
 	}
 
 	/* errors must be reported */
-- 
1.7.8.6

[PATCH 04/13] remove unneeded forward declaration

[stefani]

From: Stefani Seibold <stefani@seibold.net>

Place the skel_draw_down() function at the rigth place to remove
the forward declaration.

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 92cdfca..7fc9621 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -67,7 +67,6 @@ struct usb_skel {
 #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);
 
 static void skel_delete(struct kref *kref)
 {
@@ -130,6 +129,16 @@ static int skel_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static void skel_draw_down(struct usb_skel *dev)
+{
+	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);
+}
+
 static int skel_flush(struct file *file, fl_owner_t id)
 {
 	struct usb_skel *dev = file->private_data;
@@ -586,16 +595,6 @@ static void skel_disconnect(struct usb_interface *interface)
 	dev_info(&interface->dev, "USB Skeleton #%d now disconnected", minor);
 }
 
-static void skel_draw_down(struct usb_skel *dev)
-{
-	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);
-}
-
 static int skel_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct usb_skel *dev = usb_get_intfdata(intf);
-- 
1.7.8.6

[PATCH 05/13] remove pr_err() noise in skel_open

[stefani]

From: Stefani Seibold <stefani@seibold.net>

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 7fc9621..2ca2530 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -82,17 +82,11 @@ static int skel_open(struct inode *inode, struct file *file)
 {
 	struct usb_skel *dev;
 	struct usb_interface *interface;
-	int subminor;
-	int retval = 0;
-
-	subminor = iminor(inode);
+	int retval;
 
-	interface = usb_find_interface(&skel_driver, subminor);
-	if (!interface) {
-		pr_err("%s - error, can't find device for minor %d\n",
-			__func__, subminor);
+	interface = usb_find_interface(&skel_driver, iminor(inode));
+	if (!interface)
 		return -ENODEV;
-	}
 
 	dev = usb_get_intfdata(interface);
 	if (!dev)
-- 
1.7.8.6

[PATCH 06/13] Handle a non blocking read without blocking

[stefani]

From: Stefani Seibold <stefani@seibold.net>

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 2ca2530..e4cb88a 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -225,9 +225,14 @@ static ssize_t skel_read(struct file *file, char *buffer, size_t count,
 		return 0;
 
 	/* no concurrent readers */
-	retval = mutex_lock_interruptible(&dev->io_mutex);
-	if (retval < 0)
-		return retval;
+	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 */
 		retval = -ENODEV;
-- 
1.7.8.6

[PATCH 07/13] fix flush function

[stefani]

From: Stefani Seibold <stefani@seibold.net>

- Check for disconnect
- Fix nameing of the return value

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index e4cb88a..dc95129 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -136,21 +136,25 @@ static void skel_draw_down(struct usb_skel *dev)
 static int skel_flush(struct file *file, fl_owner_t id)
 {
 	struct usb_skel *dev = file->private_data;
-	int res;
+	int retval;
 
 	/* wait for io to stop */
 	mutex_lock(&dev->io_mutex);
+	if (!dev->interface) {		/* disconnect() was called */
+		retval = -ENODEV;
+		goto exit;
+	}
 	skel_draw_down(dev);
 
 	/* read out errors, leave subsequent opens a clean slate */
 	spin_lock_irq(&dev->err_lock);
-	res = dev->errors ? (dev->errors == -EPIPE ? -EPIPE : -EIO) : 0;
+	retval = dev->errors ? (dev->errors == -EPIPE ? -EPIPE : -EIO) : 0;
 	dev->errors = 0;
 	spin_unlock_irq(&dev->err_lock);
-
+exit:
 	mutex_unlock(&dev->io_mutex);
 
-	return res;
+	return retval;
 }
 
 static void skel_read_bulk_callback(struct urb *urb)
-- 
1.7.8.6

[PATCH 08/13] add fsync function

[stefani]

From: Stefani Seibold <stefani@seibold.net>

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index dc95129..61f4ac8 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -133,7 +133,7 @@ static void skel_draw_down(struct usb_skel *dev)
 	usb_kill_urb(dev->bulk_in_urb);
 }
 
-static int skel_flush(struct file *file, fl_owner_t id)
+static int skel_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 {
 	struct usb_skel *dev = file->private_data;
 	int retval;
@@ -157,6 +157,11 @@ exit:
 	return retval;
 }
 
+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;
@@ -470,6 +475,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,
 };
-- 
1.7.8.6

[PATCH 09/13] remove unneeded lock in skel_open

[stefani]

From: Stefani Seibold <stefani@seibold.net>

The io_mutex must not acquired since a disconnect waits in usb_deregister_dev()
due the already locked minor_rwsem in the usb_open() function

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 61f4ac8..6482acb 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -95,11 +95,11 @@ static int skel_open(struct inode *inode, struct file *file)
 	/* 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);
+	/*
+	 * must be not locked since disconnect waits in usb_deregister_dev()
+	 * due the already locked minor_rwsem in the usb_open() function
+	 */
 	retval = usb_autopm_get_interface(interface);
-	mutex_unlock(&dev->io_mutex);
 
 	/* save our object in the file's private structure */
 	if (!retval)
-- 
1.7.8.6

[PATCH 10/13] fix race in skel_write

[stefani]

From: Stefani Seibold <stefani@seibold.net>

Access to members of dev->interface without holding the io_mutex lock
could result in a NULL pointer access, since disconnect will do this
as a marker for disconnect.

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 6482acb..0a1ab0b 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -96,7 +96,7 @@ static int skel_open(struct inode *inode, struct file *file)
 	kref_get(&dev->kref);
 
 	/*
-	 * must be not locked since disconnect waits in usb_deregister_dev()
+	 * must be not locked since a disconnect waits in usb_deregister_dev()
 	 * due the already locked minor_rwsem in the usb_open() function
 	 */
 	retval = usb_autopm_get_interface(interface);
@@ -441,13 +441,14 @@ 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,
 			"%s - failed submitting write urb, error %d\n",
 			__func__, retval);
+		mutex_unlock(&dev->io_mutex);
 		goto error_unanchor;
 	}
+	mutex_unlock(&dev->io_mutex);
 
 	/*
 	 * release our reference to this urb, the USB core will eventually free
-- 
1.7.8.6

[PATCH 11/13] fix kref usage in skel_open

[stefani]

From: Stefani Seibold <stefani@seibold.net>

Increment the kref at last, so we need no extra error path for decrement
them in a case of error..

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 0a1ab0b..551fb51 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -92,18 +92,19 @@ static int skel_open(struct inode *inode, struct file *file)
 	if (!dev)
 		return -ENODEV;
 
-	/* increment our usage count for the device */
-	kref_get(&dev->kref);
-
 	/*
 	 * must be not locked since a disconnect waits in usb_deregister_dev()
 	 * due the already locked minor_rwsem in the usb_open() function
 	 */
 	retval = usb_autopm_get_interface(interface);
+	if (!retval)
+		return retval;
+
+	/* increment our usage count for the device */
+	kref_get(&dev->kref);
 
 	/* save our object in the file's private structure */
-	if (!retval)
-		file->private_data = dev;
+	file->private_data = dev;
 
 	return retval;
 }
-- 
1.7.8.6

[PATCH 12/13] Introduce single user mode

[stefani]

From: Stefani Seibold <stefani@seibold.net>

Most USB devices can only used in a single usage mode. This patch prevents a
reopening on an already opened device.

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 551fb51..456c9a8 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -59,6 +59,7 @@ struct usb_skel {
 	__u8			bulk_out_endpointAddr;	/* the address of the bulk out endpoint */
 	int			errors;			/* the last request tanked */
 	bool			ongoing_read;		/* a read is going on */
+	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 */
@@ -92,17 +93,28 @@ static int skel_open(struct inode *inode, struct file *file)
 	if (!dev)
 		return -ENODEV;
 
+	mutex_lock(&dev->io_mutex);
+	if (dev->in_use) {
+		mutex_unlock(&dev->io_mutex);
+		return -EBUSY;
+	}
+
 	/*
 	 * must be not locked since a disconnect waits in usb_deregister_dev()
 	 * due the already locked minor_rwsem in the usb_open() function
 	 */
 	retval = usb_autopm_get_interface(interface);
-	if (!retval)
+	if (!retval) {
+		mutex_unlock(&dev->io_mutex);
 		return retval;
+	}
 
 	/* increment our usage count for the device */
 	kref_get(&dev->kref);
 
+	dev->in_use = true;
+	mutex_unlock(&dev->io_mutex);
+
 	/* save our object in the file's private structure */
 	file->private_data = dev;
 
@@ -117,6 +129,7 @@ static int skel_release(struct inode *inode, struct file *file)
 	mutex_lock(&dev->io_mutex);
 	if (dev->interface)
 		usb_autopm_put_interface(dev->interface);
+	dev->in_use = false;
 	mutex_unlock(&dev->io_mutex);
 
 	/* decrement the count on our device */
@@ -517,6 +530,7 @@ static int skel_probe(struct usb_interface *interface,
 
 	dev->udev = usb_get_dev(interface_to_usbdev(interface));
 	dev->interface = interface;
+	dev->in_use = false;
 
 	/* set up the endpoint information */
 	/* use only the first bulk-in and bulk-out endpoints */
-- 
1.7.8.6

[PATCH 13/13] Bump version number and add aditional author

[stefani]

From: Stefani Seibold <stefani@seibold.net>

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 456c9a8..1e36782 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@kroah.com)
+ * Fixes and cleanup by Stefani Seibold (stefani@seibold.net)
  *
  *	This program is free software; you can redistribute it and/or
  *	modify it under the terms of the GNU General Public License as
-- 
1.7.8.6

Re: [PATCH 02/13] code cleanup

[Bjørn Mork]

stefani@seibold.net writes:

> @@ -95,15 +93,12 @@ static int skel_open(struct inode *inode, struct file *file)
>  	if (!interface) {
>  		pr_err("%s - error, can't find device for minor %d\n",
>  			__func__, subminor);
> -		retval = -ENODEV;
> -		goto exit;
> +		return -ENODEV;
>  	}


This may save you a line, but that line was there for a reason...

Using a common exit path for errors makes it easier to keep unlocking,
deallocation and other cleanups correct.  Although you *can* do that
change now, you introduce future bugs here.  Someone adding a lock
before this will now have to go through all the error paths to ensure
that they unlock before exiting.

See "Chapter 7: Centralized exiting of functions" in
Documentation/CodingStyle.

Most of this patch consists of this kind of bogus changes.  I won't
comment on the rest of them.

Focus on creating a *good* example.  Compacting the code is not
necessarily improving the code...



>  	/* verify that we actually have some data to write */
> -	if (count == 0)
> -		goto exit;
> +	if (!count)
> +		return 0;

zero-testing is discussed over and over again, and is a matter of
taste. But I fail to see how changing it can be part of a cleanup.  It
just changes the flavour to suit another taste.  What's the reason for
doing that?



Bjørn

Re: [PATCH 13/13] Bump version number and add aditional author

[Bjørn Mork]

Another bad example you do not want to teach anyone.  Remove the version
number instead.


Bjørn

Re: [PATCH 02/13] code cleanup

[Stefani Seibold]

Am Donnerstag, den 07.06.2012, 11:06 +0200 schrieb Bjørn Mork:
> stefani@seibold.net writes:
> 
> > @@ -95,15 +93,12 @@ static int skel_open(struct inode *inode, struct file *file)
> >  	if (!interface) {
> >  		pr_err("%s - error, can't find device for minor %d\n",
> >  			__func__, subminor);
> > -		retval = -ENODEV;
> > -		goto exit;
> > +		return -ENODEV;
> >  	}
> 
> 
> This may save you a line, but that line was there for a reason...
> 
> Using a common exit path for errors makes it easier to keep unlocking,
> deallocation and other cleanups correct.  Although you *can* do that
> change now, you introduce future bugs here.  Someone adding a lock
> before this will now have to go through all the error paths to ensure
> that they unlock before exiting.
> 
> See "Chapter 7: Centralized exiting of functions" in
> Documentation/CodingStyle.
> 

If it is necessary... I get alway ten different complains from six
developers. Developer A says do it in this way, developer B do it in the
other way.

> Focus on creating a *good* example.  Compacting the code is not
> necessarily improving the code...
> 

Compacting improves since it will make the code more readable.

> 
> 
> >  	/* verify that we actually have some data to write */
> > -	if (count == 0)
> > -		goto exit;
> > +	if (!count)
> > +		return 0;
> 
> zero-testing is discussed over and over again, and is a matter of
> taste. But I fail to see how changing it can be part of a cleanup.  It
> just changes the flavour to suit another taste.  What's the reason for
> doing that?
> 

Consistency - there are a lot places in the driver skeleton handling
this in the same way.

Re: [PATCH 02/13] code cleanup

[Bjørn Mork]

Stefani Seibold <stefani@seibold.net> writes:

> If it is necessary...

So, why is it necessary for you to change this code *from* the style
recommended by CodingStyle and LDD3?

Quoting from LDD3:

 "Error recovery is sometimes best handled with the goto statement. We
  normally hate to use goto, but in our opinion, this is one situation
  where it is useful. Careful use of goto in error situations can
  eliminate a great deal of complicated, highly-indented, "structured"
  logic. Thus, in the kernel, goto is often used as shown here to deal
  with errors."

> Compacting improves since it will make the code more readable.

No, it does not.  As pointed out, instead of having to follow a single
exit path from each function, your changes makes it necessary to follow
n exit paths.  That does not make the code more readable, and it
contradicts both CodingStyle and LDD3.

Note that I am not stating in any way that those documents contain
absolute truths and that you cannot write your own driver the way you
like.  I do however find it extremely strange that you insist on
changing a coding example to be inconsistent with those documents.
Regardless of whether you agree with them or not, you must see that such
inconsistent guidelines will be a problem for anyone trying to use this
code for learning?


Bjørn

Re: [PATCH 03/13] remove dead code

[Oliver Neukum]

Am Donnerstag, 7. Juni 2012, 10:20:33 schrieb stefani@seibold.net:
> From: Stefani Seibold <stefani@seibold.net>
> 
> All code which use processed_urb can be eliminated.

If you eliminate it, how do you balance the completion?

	Regards
		Oliver
> 
> Signed-off-by: Stefani Seibold <stefani@seibold.net>
> ---
>  drivers/usb/usb-skeleton.c |   12 ------------
>  1 files changed, 0 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
> index 7385aa8..92cdfca 100644
> --- a/drivers/usb/usb-skeleton.c
> +++ b/drivers/usb/usb-skeleton.c
> @@ -59,7 +59,6 @@ struct usb_skel {
>  	__u8			bulk_out_endpointAddr;	/* the address of the bulk out endpoint */
>  	int			errors;			/* the last request tanked */
>  	bool			ongoing_read;		/* a read is going on */
> -	bool			processed_urb;		/* indicates we haven't processed the urb */
>  	spinlock_t		err_lock;		/* lock for errors */
>  	struct kref		kref;
>  	struct mutex		io_mutex;		/* synchronize I/O with disconnect */
> @@ -256,17 +255,6 @@ retry:
>  		 * we must finish now
>  		 */
>  		dev->bulk_in_copied = 0;
> -		dev->processed_urb = true;
> -	}
> -
> -	if (!dev->processed_urb) {
> -		/*
> -		 * the URB hasn't been processed
> -		 * do it now
> -		 */
> -		wait_for_completion(&dev->bulk_in_completion);
> -		dev->bulk_in_copied = 0;
> -		dev->processed_urb = true;
>  	}
>  
>  	/* errors must be reported */
> -- 
> 1.7.8.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 03/13] remove dead code

[Stefani Seibold]

Am Donnerstag, den 07.06.2012, 17:04 +0200 schrieb Oliver Neukum:
> > 
> > All code which use processed_urb can be eliminated.
> 
> If you eliminate it, how do you balance the completion?
> 
> 	Regards
> 		Oliver

This was dead code, the process_urb was never used, there was no code
which set the value to false.

The completion is handled in the skel_read() function by the "if
(ongoing_io)" path.

Greetings,
Steffi

Re: [PATCH 13/13] Bump version number and add aditional author

[Greg KH]

On Thu, Jun 07, 2012 at 11:09:52AM +0200, Bjørn Mork wrote:
> Another bad example you do not want to teach anyone.  Remove the version
> number instead.

I agree, the version number should just be deleted.

thanks,

greg k-h

Re: [PATCH 02/13] code cleanup

[Greg KH]

On Thu, Jun 07, 2012 at 10:20:32AM +0200, stefani@seibold.net wrote:
> From: Stefani Seibold <stefani@seibold.net>
> 
> - consistent nameing
> - more compact style
> - remove unneeded code
> 
> Signed-off-by: Stefani Seibold <stefani@seibold.net>
> ---
>  drivers/usb/usb-skeleton.c |  119 +++++++++++++++++---------------------------
>  1 files changed, 46 insertions(+), 73 deletions(-)

Your subject: needs to be fixed for all of these, as it does not
describe what part of the kernel is being modified.

A better example would be to use a prefix of:
	Subject: [PATCH XX/13] USB: skeleton: 
for all of these patches, so the shortlog entry is correct.

Care to redo this series, based on the feedback you've gotten, and
resend?

thanks,

greg k-h

Re: [PATCH 02/13] code cleanup

[Greg KH]

On Thu, Jun 07, 2012 at 12:49:49PM +0200, Bjørn Mork wrote:
> Stefani Seibold <stefani@seibold.net> writes:
> 
> > If it is necessary...
> 
> So, why is it necessary for you to change this code *from* the style
> recommended by CodingStyle and LDD3?
> 
> Quoting from LDD3:
> 
>  "Error recovery is sometimes best handled with the goto statement. We
>   normally hate to use goto, but in our opinion, this is one situation
>   where it is useful. Careful use of goto in error situations can
>   eliminate a great deal of complicated, highly-indented, "structured"
>   logic. Thus, in the kernel, goto is often used as shown here to deal
>   with errors."
> 
> > Compacting improves since it will make the code more readable.
> 
> No, it does not.  As pointed out, instead of having to follow a single
> exit path from each function, your changes makes it necessary to follow
> n exit paths.  That does not make the code more readable, and it
> contradicts both CodingStyle and LDD3.
> 
> Note that I am not stating in any way that those documents contain
> absolute truths and that you cannot write your own driver the way you
> like.  I do however find it extremely strange that you insist on
> changing a coding example to be inconsistent with those documents.
> Regardless of whether you agree with them or not, you must see that such
> inconsistent guidelines will be a problem for anyone trying to use this
> code for learning?

I totally agree, the original style should be preserved.

thanks,

greg k-h

Re: [PATCH 01/13] fix wrong label in skel_open

[Greg KH]

On Thu, Jun 07, 2012 at 10:20:31AM +0200, stefani@seibold.net wrote:
> From: Stefani Seibold <stefani@seibold.net>
> 
> Signed-off-by: Stefani Seibold <stefani@seibold.net>
> ---
>  drivers/usb/usb-skeleton.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
> index 0616f23..efda3a5 100644
> --- a/drivers/usb/usb-skeleton.c
> +++ b/drivers/usb/usb-skeleton.c
> @@ -114,10 +114,11 @@ static int skel_open(struct inode *inode, struct file *file)
>  
>  	retval = usb_autopm_get_interface(interface);
>  	if (retval)
> -		goto out_err;
> +		goto exit2;
>  
>  	/* save our object in the file's private structure */
>  	file->private_data = dev;
> +exit2:
>  	mutex_unlock(&dev->io_mutex);

That's not so much as a "wrong label" as, "fix unlock bug on error
path", right?

thanks,

greg k-h

Re: [PATCH 12/13] Introduce single user mode

[Greg KH]

On Thu, Jun 07, 2012 at 10:20:42AM +0200, stefani@seibold.net wrote:
> From: Stefani Seibold <stefani@seibold.net>
> 
> Most USB devices can only used in a single usage mode. This patch prevents a
> reopening on an already opened device.

That's not true at all, most devices work just fine with multiple opens,
it just depends on the type of device you have.

thanks,

greg k-h

Re: [PATCH 02/13] code cleanup

[Stefani Seibold]

Am Dienstag, den 12.06.2012, 18:02 -0700 schrieb Greg KH:
> On Thu, Jun 07, 2012 at 10:20:32AM +0200, stefani@seibold.net wrote:
> > From: Stefani Seibold <stefani@seibold.net>
> > 
> > - consistent nameing
> > - more compact style
> > - remove unneeded code
> > 
> > Signed-off-by: Stefani Seibold <stefani@seibold.net>
> > ---
> >  drivers/usb/usb-skeleton.c |  119 +++++++++++++++++---------------------------
> >  1 files changed, 46 insertions(+), 73 deletions(-)
> 
> Your subject: needs to be fixed for all of these, as it does not
> describe what part of the kernel is being modified.
> 
> A better example would be to use a prefix of:
> 	Subject: [PATCH XX/13] USB: skeleton: 
> for all of these patches, so the shortlog entry is correct.
> 
> Care to redo this series, based on the feedback you've gotten, and
> resend?
> 

Yes, but i till take some time due my move to a new house.

Greetings,
Stefani

Re: [PATCH] fix usb skeleton driver

[Stefani Seibold]

Am Mittwoch, den 06.06.2012, 16:22 -0400 schrieb Alan Stern:
> On Wed, 6 Jun 2012, Stefani Seibold wrote:
> 
> > The reason to fix the skeleton driver was about the complains for my
> > NRPZ driver, which was based on the design of the usb skeleton driver.
> > 
> > > Going even farther, I'm not so sure it's a good idea for usb-skeleton
> > > to try supporting both synchronous and asynchronous accesses.  This
> > > adds a layer of complexity that people just don't need.  IMO it would 
> > > be better to have two separate example drivers, an easy one that is 
> > > purely synchronous and a more advanced one that is purely async.
> > > 
> > 
> > Agree, i think this would be a good idea to have to separate drivers.
> > Both should be also working drivers, for really simple hardware.
> > 
> > The best way for me to do this is to shrink later this to a simplified
> > driver. 
> 
> That makes sense.  Will you do it?

Yes, if nobody other will do this.

> 
> > I think it is important to have a clean and working example. It would
> > save a lot of time for everybody and shrinks the number of round trips.
> 
> How can you tell that it works?  By testing your NRPZ driver?
> 

That is an option. But i will look for a more generic hardware. Maybe a
smartphone or a usb pen drive.

Re: [PATCH] fix usb skeleton driver

[Bjørn Mork]

Stefani Seibold <stefani@seibold.net> writes:
> Am Mittwoch, den 06.06.2012, 14:16 -0400 schrieb Alan Stern:
>
>> But that's wrong -- the accesses should go through the interface 
>> pointer.  After all, the driver is bound to the interface, not to the 
>> device.
>> 
>
> Not really true, in whole driver only the open() and close() use the
> interface pointer.
>
> In the open path the interface is already determinate by
> usb_find_interface(), so it was reasonable to do this also in the close
> path.

You are ignoring a few printk's you had to change:

@@ -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;


IMHO this is really bad in an example driver.  This is an interface
driver, and any messages from it should either reference the interface
or some related device the driver has registered.

"Simplifying" like this is not the way to go when writing a HOWTO.


Bjørn

Re: [PATCH] fix usb skeleton driver

[Ming Lei]

On Wed, Jun 6, 2012 at 11:05 PM, Stefani Seibold <stefani@seibold.net> 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

Re: [PATCH] fix usb skeleton driver

[Alan Stern]

On Wed, 6 Jun 2012, Stefani Seibold wrote:

> The reason to fix the skeleton driver was about the complains for my
> NRPZ driver, which was based on the design of the usb skeleton driver.
> 
> > Going even farther, I'm not so sure it's a good idea for usb-skeleton
> > to try supporting both synchronous and asynchronous accesses.  This
> > adds a layer of complexity that people just don't need.  IMO it would 
> > be better to have two separate example drivers, an easy one that is 
> > purely synchronous and a more advanced one that is purely async.
> > 
> 
> Agree, i think this would be a good idea to have to separate drivers.
> Both should be also working drivers, for really simple hardware.
> 
> The best way for me to do this is to shrink later this to a simplified
> driver. 

That makes sense.  Will you do it?

> I think it is important to have a clean and working example. It would
> save a lot of time for everybody and shrinks the number of round trips.

How can you tell that it works?  By testing your NRPZ driver?

Alan Stern

Re: [PATCH] fix usb skeleton driver

[Stefani Seibold]

Am Mittwoch, den 06.06.2012, 14:16 -0400 schrieb Alan Stern:
> On Wed, 6 Jun 2012, Stefani Seibold wrote:
> 
> > Am Mittwoch, den 06.06.2012, 12:55 -0400 schrieb Alan Stern:
> > > On Wed, 6 Jun 2012 stefani@seibold.net wrote:
> > > 
> > > > From: Stefani Seibold <stefani@seibold.net>
> > > > 
> > > > 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
> > > 
> > 
> > Sorry, missed to fix. Should be:
> > 
> > Every access to the USB will be handled through the usb_device pointer
> 
> But that's wrong -- the accesses should go through the interface 
> pointer.  After all, the driver is bound to the interface, not to the 
> device.
> 

Not really true, in whole driver only the open() and close() use the
interface pointer.

In the open path the interface is already determinate by
usb_find_interface(), so it was reasonable to do this also in the close
path.

> > > > - Introduced fsync
> > > > - Single user mode
> > > > - Eliminated dead code
> > > > - Save some bytes in the dev structure
> > > 
> > > How about simplifying the code so that it can be read by somebody who's 
> > > not already an expert?
> > > 
> > > Alan Stern
> > > 
> > 
> > Hey, i thought i get a little thank you for the voluntary work, what a
> > nice job. Not a demand for more work to do.
> 
> Have you submitted many kernel patches in the past?  :-)  This is the 
> way it usually works out...
> 

Yes i did (kfifo, superio, procfs, udpcp, nrpz, mtd nand, framebuffer
and so on). And i know the way it works ;-) But i do this mostly in my
spear time.

> In this case, I really think it's worthwhile to look for ways to
> simplify usb-skeleton.c.  For example, does supporting fsync really
> help somebody who's trying to learn how to write a USB device driver?  
> I suspect it doesn't.
> 

The reason to fix the skeleton driver was about the complains for my
NRPZ driver, which was based on the design of the usb skeleton driver.

> Going even farther, I'm not so sure it's a good idea for usb-skeleton
> to try supporting both synchronous and asynchronous accesses.  This
> adds a layer of complexity that people just don't need.  IMO it would 
> be better to have two separate example drivers, an easy one that is 
> purely synchronous and a more advanced one that is purely async.
> 

Agree, i think this would be a good idea to have to separate drivers.
Both should be also working drivers, for really simple hardware.

The best way for me to do this is to shrink later this to a simplified
driver. 

> Now, things like the race between disconnect and open are good for
> teaching, because they crop up in every driver and have to be handled.  
> Other things aren't so clear (such as the autosuspend support).
> 

I think it is important to have a clean and working example. It would
save a lot of time for everybody and shrinks the number of round trips.

Greetings,
Stefani

Re: [PATCH] fix usb skeleton driver

[Oliver Neukum]

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

Re: [PATCH] fix usb skeleton driver

[Alan Stern]

On Wed, 6 Jun 2012, Stefani Seibold wrote:

> Am Mittwoch, den 06.06.2012, 12:55 -0400 schrieb Alan Stern:
> > On Wed, 6 Jun 2012 stefani@seibold.net wrote:
> > 
> > > From: Stefani Seibold <stefani@seibold.net>
> > > 
> > > 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
> > 
> 
> Sorry, missed to fix. Should be:
> 
> Every access to the USB will be handled through the usb_device pointer

But that's wrong -- the accesses should go through the interface 
pointer.  After all, the driver is bound to the interface, not to the 
device.

> > Those two changes sound contradictory.
> > 
> > > - 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
> > > - Eliminated dead code
> > > - Save some bytes in the dev structure
> > 
> > How about simplifying the code so that it can be read by somebody who's 
> > not already an expert?
> > 
> > Alan Stern
> > 
> 
> Hey, i thought i get a little thank you for the voluntary work, what a
> nice job. Not a demand for more work to do.

Have you submitted many kernel patches in the past?  :-)  This is the 
way it usually works out...

Seriously, what seems like an improvement to one person might not seem 
so great to somebody else.  Often even the most trivial changes can't 
get accepted without a lot of back-and-forth arguing^Wdiscussion.

In this case, I really think it's worthwhile to look for ways to
simplify usb-skeleton.c.  For example, does supporting fsync really
help somebody who's trying to learn how to write a USB device driver?  
I suspect it doesn't.

Going even farther, I'm not so sure it's a good idea for usb-skeleton
to try supporting both synchronous and asynchronous accesses.  This
adds a layer of complexity that people just don't need.  IMO it would 
be better to have two separate example drivers, an easy one that is 
purely synchronous and a more advanced one that is purely async.

Now, things like the race between disconnect and open are good for
teaching, because they crop up in every driver and have to be handled.  
Other things aren't so clear (such as the autosuspend support).

Alan Stern

Re: [PATCH] fix usb skeleton driver

[Stefani Seibold]

Am Mittwoch, den 06.06.2012, 12:55 -0400 schrieb Alan Stern:
> On Wed, 6 Jun 2012 stefani@seibold.net wrote:
> 
> > From: Stefani Seibold <stefani@seibold.net>
> > 
> > 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
> 

Sorry, missed to fix. Should be:

Every access to the USB will be handled through the usb_device pointer

> Those two changes sound contradictory.
> 
> > - 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
> > - Eliminated dead code
> > - Save some bytes in the dev structure
> 
> How about simplifying the code so that it can be read by somebody who's 
> not already an expert?
> 
> Alan Stern
> 

Hey, i thought i get a little thank you for the voluntary work, what a
nice job. Not a demand for more work to do.

Re: [PATCH] fix usb skeleton driver

[Alan Stern]

On Wed, 6 Jun 2012 stefani@seibold.net wrote:

> From: Stefani Seibold <stefani@seibold.net>
> 
> 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

Those two changes sound contradictory.

> - 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
> - Eliminated dead code
> - Save some bytes in the dev structure

How about simplifying the code so that it can be read by somebody who's 
not already an expert?

Alan Stern

[PATCH] fix usb skeleton driver

[stefani]

From: Stefani Seibold <stefani@seibold.net>

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
- Eliminated dead code
- 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

Hope this helps

Signed-off-by: Stefani Seibold <stefani@seibold.net>

Re: [PATCH] fix usb skeleton driver

[Alan Stern]

On Wed, 6 Jun 2012, Ming Lei wrote:

> On Wed, Jun 6, 2012 at 9:23 PM,  <stefani@seibold.net> wrote:
> > From: Stefani Seibold <stefani@seibold.net>
> >
> > 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

Re: [PATCH] fix usb skeleton driver

[Stefani Seibold]

Am Mittwoch, den 06.06.2012, 22:28 +0800 schrieb Ming Lei:
> On Wed, Jun 6, 2012 at 9:23 PM,  <stefani@seibold.net> wrote:
> > From: Stefani Seibold <stefani@seibold.net>
> >
> > 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

Re: [PATCH] fix usb skeleton driver

[Ming Lei]

On Wed, Jun 6, 2012 at 9:23 PM,  <stefani@seibold.net> wrote:
> From: Stefani Seibold <stefani@seibold.net>
>
> 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 <stefani@seibold.net>
> ---
>  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@kroah.com)
> + * fixes by Stefani Seibold (stefani@seibold.net)
>  *
>  *     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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


Thanks,
-- 
Ming Lei

Re: [PATCH] fix usb skeleton driver

[Oliver Neukum]

Am Mittwoch, 6. Juni 2012, 15:23:18 schrieb stefani@seibold.net:

> 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

[PATCH] fix usb skeleton driver

[stefani]

From: Stefani Seibold <stefani@seibold.net>

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@seibold.net>
---
 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@kroah.com)
+ * fixes by Stefani Seibold (stefani@seibold.net)
  *
  *	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

Re: [PATCH] fix usb skeleton driver

[Stefani Seibold]

Am Mittwoch, den 06.06.2012, 09:32 +0200 schrieb Oliver Neukum:
> Am Mittwoch, 6. Juni 2012, 09:00:36 schrieb stefani@seibold.net:
> > @@ -126,32 +122,21 @@ exit:
> >  
> >  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;
> >  
> >         /* 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);
> > +       usb_autopm_put_interface(dev->interface);
> 
> That is a bug. You must check for disconnect here, because
> after a disconnect the interface may be bound already to another
> driver.
> 
> 	Regards
> 		Oliver

Yes, you are right.

But as i now figured out, the whole usb_skeleton.c drivers is full of
races in the skel_open and skel_release path.

I will send a clean and tested patch set in the next few days.

Greetings,
Stefani

Re: [PATCH] fix usb skeleton driver

[Greg KH]

On Wed, Jun 06, 2012 at 09:00:36AM +0200, stefani@seibold.net wrote:
> From: Stefani Seibold <stefani@seibold.net>
> 
> This is a fix for the USB skeleton driver to bring it in shape.
> 
> - The usb_device structure pointer will no longer stored 
> - Every access to the USB will be handled trought the usb_interface pointer
> - No longer assign a NULL to usb_interface pointer in the disconnect() handler
> - Add a new bool 'connected' for signaling a disconnect (== false)
> - Handle a non blocking read without blocking
> - Code clean up

Please break this up into individual patches for all of these things,
and also always cc: the linux-usb@vger.kernel.org mailing list when you
submit USB related patches.

thanks,

greg k-h

Re: [PATCH] fix usb skeleton driver

[Stefani Seibold]

Am Mittwoch, den 06.06.2012, 09:29 +0200 schrieb Oliver Neukum:
> Am Mittwoch, 6. Juni 2012, 09:00:36 schrieb stefani@seibold.net:
> > From: Stefani Seibold <stefani@seibold.net>
> > 
> > This is a fix for the USB skeleton driver to bring it in shape.
> > 
> > - The usb_device structure pointer will no longer stored 
> > - Every access to the USB will be handled trought the usb_interface pointer
> > - No longer assign a NULL to usb_interface pointer in the disconnect() handler
> 
> Why? What is gained?
> 

All of this topics was suggested by Greg.

If a NULL is assigned to the usb_interface pointer, the skel_delete
cannot do an usb_put_dev() since the usb_device pointer is no longer
available.

Greetings

Re: [PATCH] fix usb skeleton driver

[Oliver Neukum]

Am Mittwoch, 6. Juni 2012, 09:46:06 schrieb Stefani Seibold:
> Am Mittwoch, den 06.06.2012, 09:29 +0200 schrieb Oliver Neukum:
> > Am Mittwoch, 6. Juni 2012, 09:00:36 schrieb stefani@seibold.net:
> > > From: Stefani Seibold <stefani@seibold.net>
> > > 
> > > This is a fix for the USB skeleton driver to bring it in shape.
> > > 
> > > - The usb_device structure pointer will no longer stored 
> > > - Every access to the USB will be handled trought the usb_interface pointer
> > > - No longer assign a NULL to usb_interface pointer in the disconnect() handler
> > 
> > Why? What is gained?
> > 
> 
> All of this topics was suggested by Greg.
> 
> If a NULL is assigned to the usb_interface pointer, the skel_delete
> cannot do an usb_put_dev() since the usb_device pointer is no longer
> available.

Yes, that's true. Sorry for the confusion.

	Regards
		Oliver

Re: [PATCH] fix usb skeleton driver

[Oliver Neukum]

Am Mittwoch, 6. Juni 2012, 09:00:36 schrieb stefani@seibold.net:
> @@ -126,32 +122,21 @@ exit:
>  
>  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;
>  
>         /* 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);
> +       usb_autopm_put_interface(dev->interface);

That is a bug. You must check for disconnect here, because
after a disconnect the interface may be bound already to another
driver.

	Regards
		Oliver

Re: [PATCH] fix usb skeleton driver

[Oliver Neukum]

Am Mittwoch, 6. Juni 2012, 09:00:36 schrieb stefani@seibold.net:
> From: Stefani Seibold <stefani@seibold.net>
> 
> This is a fix for the USB skeleton driver to bring it in shape.
> 
> - The usb_device structure pointer will no longer stored 
> - Every access to the USB will be handled trought the usb_interface pointer
> - No longer assign a NULL to usb_interface pointer in the disconnect() handler

Why? What is gained?

	Regards
		Oliver

[PATCH] fix usb skeleton driver

[stefani]

From: Stefani Seibold <stefani@seibold.net>

This is a fix for the USB skeleton driver to bring it in shape.

- The usb_device structure pointer will no longer stored 
- Every access to the USB will be handled trought the usb_interface pointer
- No longer assign a NULL to usb_interface pointer in the disconnect() handler
- Add a new bool 'connected' for signaling a disconnect (== false)
- Handle a non blocking read without blocking
- Code clean up

The patch is against linux 3.5.0 commit eea5b5510fc5545d15b69da8e485a7424ae388cf

Hope this helps

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/usb/usb-skeleton.c |  107 ++++++++++++++++++--------------------------
 1 files changed, 44 insertions(+), 63 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 0616f23..01f7ca5 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@kroah.com)
+ * fixes by Stefan Seibold <stefani@seibold.net>
  *
  *	This program is free software; you can redistribute it and/or
  *	modify it under the terms of the GNU General Public License as
@@ -48,7 +49,6 @@ 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 semaphore	limit_sem;		/* limiting the number of writes in progress */
 	struct usb_anchor	submitted;		/* in case we need to retract our submissions */
@@ -62,9 +62,10 @@ 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 */
 	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)
@@ -77,7 +78,7 @@ static void skel_delete(struct kref *kref)
 	struct usb_skel *dev = to_skel_dev(kref);
 
 	usb_free_urb(dev->bulk_in_urb);
-	usb_put_dev(dev->udev);
+	usb_put_dev(interface_to_usbdev(dev->interface));
 	kfree(dev->bulk_in_buffer);
 	kfree(dev);
 }
@@ -100,7 +101,7 @@ static int skel_open(struct inode *inode, struct file *file)
 	}
 
 	dev = usb_get_intfdata(interface);
-	if (!dev) {
+	if (!dev || !dev->connected) {
 		retval = -ENODEV;
 		goto exit;
 	}
@@ -108,17 +109,12 @@ static int skel_open(struct inode *inode, struct file *file)
 	/* 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);
-
 	retval = usb_autopm_get_interface(interface);
 	if (retval)
-		goto out_err;
+		goto exit;
 
 	/* save our object in the file's private structure */
 	file->private_data = dev;
-	mutex_unlock(&dev->io_mutex);
 
 exit:
 	return retval;
@@ -126,32 +122,21 @@ exit:
 
 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;
 
 	/* 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);
+	usb_autopm_put_interface(dev->interface);
 
 	/* decrement the count on our device */
 	kref_put(&dev->kref, skel_delete);
 	return 0;
 }
 
-static int skel_flush(struct file *file, fl_owner_t id)
+static int skel_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 {
-	struct usb_skel *dev;
+	struct usb_skel *dev = file->private_data;
 	int res;
 
-	dev = file->private_data;
-	if (dev == NULL)
-		return -ENODEV;
-
 	/* wait for io to stop */
 	mutex_lock(&dev->io_mutex);
 	skel_draw_down(dev);
@@ -167,6 +152,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;
@@ -187,7 +177,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);
@@ -196,20 +186,19 @@ static void skel_read_bulk_callback(struct urb *urb)
 static int skel_do_read_io(struct usb_skel *dev, size_t count)
 {
 	int rv;
+	struct usb_device *udev = interface_to_usbdev(dev->interface);
 
 	/* prepare a read */
 	usb_fill_bulk_urb(dev->bulk_in_urb,
-			dev->udev,
-			usb_rcvbulkpipe(dev->udev,
+			udev,
+			usb_rcvbulkpipe(udev,
 				dev->bulk_in_endpointAddr),
 			dev->bulk_in_buffer,
 			min(dev->bulk_in_size, 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);
@@ -219,9 +208,7 @@ static int skel_do_read_io(struct usb_skel *dev, size_t count)
 			__func__, rv);
 		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);
+		dev->ongoing_read = false;
 	}
 
 	return rv;
@@ -230,33 +217,31 @@ static int skel_do_read_io(struct usb_skel *dev, size_t count)
 static ssize_t skel_read(struct file *file, char *buffer, size_t count,
 			 loff_t *ppos)
 {
-	struct usb_skel *dev;
+	struct usb_skel *dev = file->private_data;
 	int rv;
-	bool ongoing_io;
-
-	dev = file->private_data;
 
 	/* 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 {
+		rv = mutex_lock_interruptible(&dev->io_mutex);
+		if (rv < 0)
+			return rv;
+	}
 
-	if (!dev->interface) {		/* disconnect() was called */
+	if (!dev->connected) {		/* disconnect() was called */
 		rv = -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;
@@ -384,13 +369,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;
+	struct usb_device *udev = interface_to_usbdev(dev->interface);
 
 	/* verify that we actually have some data to write */
 	if (count == 0)
@@ -431,7 +415,7 @@ static ssize_t skel_write(struct file *file, const char *user_buffer,
 		goto error;
 	}
 
-	buf = usb_alloc_coherent(dev->udev, writesize, GFP_KERNEL,
+	buf = usb_alloc_coherent(udev, writesize, GFP_KERNEL,
 				 &urb->transfer_dma);
 	if (!buf) {
 		retval = -ENOMEM;
@@ -444,23 +428,20 @@ 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;
 	}
 
 	/* initialize the urb properly */
-	usb_fill_bulk_urb(urb, dev->udev,
-			  usb_sndbulkpipe(dev->udev, dev->bulk_out_endpointAddr),
+	usb_fill_bulk_urb(urb, udev,
+			  usb_sndbulkpipe(udev, dev->bulk_out_endpointAddr),
 			  buf, writesize, skel_write_bulk_callback, dev);
 	urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
 	usb_anchor_urb(urb, &dev->submitted);
 
 	/* 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,
 			"%s - failed submitting write urb, error %d\n",
@@ -481,7 +462,7 @@ error_unanchor:
 	usb_unanchor_urb(urb);
 error:
 	if (urb) {
-		usb_free_coherent(dev->udev, writesize, buf, urb->transfer_dma);
+		usb_free_coherent(udev, writesize, buf, urb->transfer_dma);
 		usb_free_urb(urb);
 	}
 	up(&dev->limit_sem);
@@ -496,6 +477,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,
 };
@@ -533,8 +515,9 @@ static int skel_probe(struct usb_interface *interface,
 	init_usb_anchor(&dev->submitted);
 	init_completion(&dev->bulk_in_completion);
 
-	dev->udev = usb_get_dev(interface_to_usbdev(interface));
+	usb_get_dev(interface_to_usbdev(interface));
 	dev->interface = interface;
+	dev->connected = true;
 
 	/* set up the endpoint information */
 	/* use only the first bulk-in and bulk-out endpoints */
@@ -612,9 +595,7 @@ static void skel_disconnect(struct usb_interface *interface)
 	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->connected = false;
 
 	usb_kill_anchored_urbs(&dev->submitted);
 
-- 
1.7.8.6