[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.

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

[PATCH 01/11] 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 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 0616f23..a57f1d2 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -114,7 +114,7 @@ static int skel_open(struct inode *inode, struct file *file)
 
 	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;
-- 
1.7.8.6

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

[Oliver Neukum]

Am Mittwoch, 6. Juni 2012, 18:27:02 schrieb stefani@seibold.net:
> From: Stefani Seibold <stefani@seibold.net>

Thank you for putting the effort to even split this up.
It makes it much easier to review.
 
> Signed-off-by: Stefani Seibold <stefani@seibold.net>
> ---
>  drivers/usb/usb-skeleton.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
> index 0616f23..a57f1d2 100644
> --- a/drivers/usb/usb-skeleton.c
> +++ b/drivers/usb/usb-skeleton.c
> @@ -114,7 +114,7 @@ static int skel_open(struct inode *inode, struct file *file)
>  
>  	retval = usb_autopm_get_interface(interface);
>  	if (retval)
> -		goto out_err;
> +		goto exit;

Are you removing io_mutex? If not, the correct fix is a second label
to drop the lock, not changing the label.  

	Regards
		Oliver

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

[Stefani Seibold]

Am Mittwoch, den 06.06.2012, 20:52 +0200 schrieb Oliver Neukum:
> Am Mittwoch, 6. Juni 2012, 18:27:02 schrieb stefani@seibold.net:
> > From: Stefani Seibold <stefani@seibold.net>
> 
> Thank you for putting the effort to even split this up.
> It makes it much easier to review.
>  
> > Signed-off-by: Stefani Seibold <stefani@seibold.net>
> > ---
> >  drivers/usb/usb-skeleton.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
> > index 0616f23..a57f1d2 100644
> > --- a/drivers/usb/usb-skeleton.c
> > +++ b/drivers/usb/usb-skeleton.c
> > @@ -114,7 +114,7 @@ static int skel_open(struct inode *inode, struct file *file)
> >  
> >  	retval = usb_autopm_get_interface(interface);
> >  	if (retval)
> > -		goto out_err;
> > +		goto exit;
> 
> Are you removing io_mutex? If not, the correct fix is a second label
> to drop the lock, not changing the label.  
> 

You are right. The skeleton driver was not compile clean and i already
fixted this. But due the split into smaller patches this was lost.

Anyway it will be fixed with the Patch 09/11] "Synchronize disconnect()
handler with open() and release(), to fix races".

[PATCH 02/11] 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 |   80 ++++++++++++++++++-------------------------
 1 files changed, 34 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index a57f1d2..2298ef0 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -126,11 +126,7 @@ 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);
@@ -145,13 +141,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);
@@ -187,7 +179,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 +187,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,
@@ -208,45 +200,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;
 	}
 
@@ -259,22 +249,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) {
@@ -284,16 +274,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 */
@@ -315,8 +305,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 +319,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 +333,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)
@@ -384,14 +374,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;
-- 
1.7.8.6

Re: [PATCH 02/11] code cleanup

[Oliver Neukum]

Am Mittwoch, 6. Juni 2012, 18:27:03 schrieb stefani@seibold.net:
> From: Stefani Seibold <stefani@seibold.net>
> 
> - consistent nameing
> - more compact style
> - remove unneeded code
> 

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

flush() is intended to report errors. A check for disconnect is needed.

	Regards
		Oliver

[PATCH 03/11] 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 2298ef0..2f17991 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -61,7 +61,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 */
@@ -264,17 +263,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/11] remove usb_interface pointer

[stefani]

From: Stefani Seibold <stefani@seibold.net>

this saves a litte bit of memory space in the driver devices structure

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

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 2f17991..f7fe32f 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -49,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 */
 	struct urb		*bulk_in_urb;		/* the urb to read data with */
@@ -61,6 +60,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			connected;		/* connected flag */
 	spinlock_t		err_lock;		/* lock for errors */
 	struct kref		kref;
 	struct mutex		io_mutex;		/* synchronize I/O with disconnect */
@@ -129,8 +129,9 @@ static int skel_release(struct inode *inode, struct file *file)
 
 	/* allow the device to be autosuspended */
 	mutex_lock(&dev->io_mutex);
-	if (dev->interface)
-		usb_autopm_put_interface(dev->interface);
+	if (dev->connected)
+		usb_autopm_put_interface(
+			usb_find_interface(&skel_driver, iminor(inode)));
 	mutex_unlock(&dev->io_mutex);
 
 	/* decrement the count on our device */
@@ -170,7 +171,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);
 
@@ -205,7 +206,7 @@ static int skel_do_read_io(struct usb_skel *dev, size_t count)
 	/* do it */
 	retval = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL);
 	if (retval < 0) {
-		dev_err(&dev->interface->dev,
+		dev_err(&dev->udev->dev,
 			"%s - failed submitting read urb, error %d\n",
 			__func__, retval);
 		dev->bulk_in_filled = 0;
@@ -234,7 +235,7 @@ static ssize_t skel_read(struct file *file, char *buffer, size_t count,
 	if (retval < 0)
 		return retval;
 
-	if (!dev->interface) {		/* disconnect() was called */
+	if (!dev->connected) {		/* disconnect() was called */
 		retval = -ENODEV;
 		goto exit;
 	}
@@ -344,7 +345,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);
 
@@ -421,7 +422,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 */
+	if (!dev->connected) {		/* disconnect() was called */
 		mutex_unlock(&dev->io_mutex);
 		retval = -ENODEV;
 		goto error;
@@ -438,7 +439,7 @@ static ssize_t skel_write(struct file *file, const char *user_buffer,
 	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;
@@ -510,7 +511,7 @@ 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;
 
 	/* set up the endpoint information */
 	/* use only the first bulk-in and bulk-out endpoints */
@@ -589,7 +590,7 @@ static void skel_disconnect(struct usb_interface *interface)
 
 	/* prevent more I/O from starting */
 	mutex_lock(&dev->io_mutex);
-	dev->interface = NULL;
+	dev->connected = false;
 	mutex_unlock(&dev->io_mutex);
 
 	usb_kill_anchored_urbs(&dev->submitted);
-- 
1.7.8.6

Re: [PATCH 04/11] remove usb_interface pointer

[Oliver Neukum]

Am Mittwoch, 6. Juni 2012, 18:27:05 schrieb stefani@seibold.net:
> 
> From: Stefani Seibold <stefani@seibold.net>
> 
> this saves a litte bit of memory space in the driver devices structure
> 

This really is the wrong way round.

	Regards
		Oliver

Re: [PATCH 04/11] remove usb_interface pointer

[Stefani Seibold]

Am Mittwoch, den 06.06.2012, 20:59 +0200 schrieb Oliver Neukum:
> Am Mittwoch, 6. Juni 2012, 18:27:05 schrieb stefani@seibold.net:
> > 
> > From: Stefani Seibold <stefani@seibold.net>
> > 
> > this saves a litte bit of memory space in the driver devices structure
> > 
> 
> This really is the wrong way round.
> 

This was what Grek asked for. And interface is the whole driver only in
the open and close path needed.

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.

Re: [PATCH 04/11] remove usb_interface pointer

[Alan Stern]

On Wed, 6 Jun 2012, Stefani Seibold wrote:

> Am Mittwoch, den 06.06.2012, 20:59 +0200 schrieb Oliver Neukum:
> > Am Mittwoch, 6. Juni 2012, 18:27:05 schrieb stefani@seibold.net:
> > > 
> > > From: Stefani Seibold <stefani@seibold.net>
> > > 
> > > this saves a litte bit of memory space in the driver devices structure
> > > 
> > 
> > This really is the wrong way round.
> > 
> 
> This was what Grek asked for. And interface is the whole driver only in
> the open and close path needed.

What about the error messages in the various callback routines?  What 
about in skel_release?

Alan Stern

Re: [PATCH 04/11] remove usb_interface pointer

[Greg KH]

On Wed, Jun 06, 2012 at 06:27:05PM +0200, stefani@seibold.net wrote:
> From: Stefani Seibold <stefani@seibold.net>
> 
> this saves a litte bit of memory space in the driver devices structure
> 
> Signed-off-by: Stefani Seibold <stefani@seibold.net>
> ---
>  drivers/usb/usb-skeleton.c |   23 ++++++++++++-----------
>  1 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
> index 2f17991..f7fe32f 100644
> --- a/drivers/usb/usb-skeleton.c
> +++ b/drivers/usb/usb-skeleton.c
> @@ -49,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 */

No, that's the exact opposite of what I asked you to do.

Re: [PATCH 04/11] remove usb_interface pointer

[Stefani Seibold]

Am Donnerstag, den 07.06.2012, 07:54 +0900 schrieb Greg KH:
> On Wed, Jun 06, 2012 at 06:27:05PM +0200, stefani@seibold.net wrote:
> > From: Stefani Seibold <stefani@seibold.net>
> > 
> > this saves a litte bit of memory space in the driver devices structure
> > 
> > Signed-off-by: Stefani Seibold <stefani@seibold.net>
> > ---
> >  drivers/usb/usb-skeleton.c |   23 ++++++++++++-----------
> >  1 files changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
> > index 2f17991..f7fe32f 100644
> > --- a/drivers/usb/usb-skeleton.c
> > +++ b/drivers/usb/usb-skeleton.c
> > @@ -49,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 */
> 
> No, that's the exact opposite of what I asked you to do.
> 

You asked me "Do you really need both?". But anyway i will find no way
to kick the usb_device pointer away without wasting the code.

So i will revert it for the first time.
 

Re: [PATCH 04/11] remove usb_interface pointer

[Stefani Seibold]

Hi Greg,

Am Donnerstag, den 07.06.2012, 07:54 +0900 schrieb Greg KH:
> On Wed, Jun 06, 2012 at 06:27:05PM +0200, stefani@seibold.net wrote:
> > From: Stefani Seibold <stefani@seibold.net>
> > 
> > this saves a litte bit of memory space in the driver devices structure
> > 
> > Signed-off-by: Stefani Seibold <stefani@seibold.net>
> > ---
> >  drivers/usb/usb-skeleton.c |   23 ++++++++++++-----------
> >  1 files changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
> > index 2f17991..f7fe32f 100644
> > --- a/drivers/usb/usb-skeleton.c
> > +++ b/drivers/usb/usb-skeleton.c
> > @@ -49,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 */
> 
> No, that's the exact opposite of what I asked you to do.
> 

I tried to implement your idea to kick away the usb_device pointer, but
i think it is impossible. The skel_delete() function needs the
usb_device pointer for calling usb_put, which is called from
skel_disconnect() or skel_release().

A call of interface_to_usbdev(dev->intf) results in a udev pointer, but
this pointer is only valid if it was called trough kref_put() from
skel_disconnect().

For an already opened devices, the call will come from skel_release()
and in this case the interface pointer could be already owned by an
other driver and no more longer handle by the skeleton driver.

So i think we need both.

HTH
Stefani

Re: [PATCH 04/11] remove usb_interface pointer

[Alan Stern]

On Thu, 7 Jun 2012, Stefani Seibold wrote:

> I tried to implement your idea to kick away the usb_device pointer, but
> i think it is impossible. The skel_delete() function needs the
> usb_device pointer for calling usb_put, which is called from
> skel_disconnect() or skel_release().
  
Does skel_delete() really need to call usb_put_dev()?  I suspect that  
the driver doesn't need to take a reference to either the device or the
interface.
  
> A call of interface_to_usbdev(dev->intf) results in a udev pointer, but
> this pointer is only valid if it was called trough kref_put() from
> skel_disconnect().
> 
> For an already opened devices, the call will come from skel_release()
> and in this case the interface pointer could be already owned by an
> other driver and no more longer handle by the skeleton driver.
> 
> So i think we need both.

If skel_delete is changed then we don't need the usb_device pointer.

Alan Stern

Re: [PATCH 04/11] remove usb_interface pointer

[Stefani Seibold]

Am Donnerstag, den 07.06.2012, 10:28 -0400 schrieb Alan Stern:
> On Thu, 7 Jun 2012, Stefani Seibold wrote:
> 
> > I tried to implement your idea to kick away the usb_device pointer, but
> > i think it is impossible. The skel_delete() function needs the
> > usb_device pointer for calling usb_put, which is called from
> > skel_disconnect() or skel_release().
>   
> Does skel_delete() really need to call usb_put_dev()?  I suspect that  
> the driver doesn't need to take a reference to either the device or the
> interface.
>  

I think it will be needed, since usb core will decrement the reference
when the device go away. Lock every access to the usb core if a tedious
thing and will waste a lot of code.

The problem with hotpluging is not the plug, it is the unplug, which can
happen any time.

BTW: An interface have no reference count.

> > A call of interface_to_usbdev(dev->intf) results in a udev pointer, but
> > this pointer is only valid if it was called trough kref_put() from
> > skel_disconnect().
> > 
> > For an already opened devices, the call will come from skel_release()
> > and in this case the interface pointer could be already owned by an
> > other driver and no more longer handle by the skeleton driver.
> > 
> > So i think we need both.
> 
> If skel_delete is changed then we don't need the usb_device pointer.
> 

I have no idea how to do this. Every USB driver i know stores the
usb_device pointer.

Maybe you have an idea?

Steffi

Re: [PATCH 04/11] remove usb_interface pointer

[Alan Stern]

On Thu, 7 Jun 2012, Stefani Seibold wrote:

> Am Donnerstag, den 07.06.2012, 10:28 -0400 schrieb Alan Stern:
> > On Thu, 7 Jun 2012, Stefani Seibold wrote:
> > 
> > > I tried to implement your idea to kick away the usb_device pointer, but
> > > i think it is impossible. The skel_delete() function needs the
> > > usb_device pointer for calling usb_put, which is called from
> > > skel_disconnect() or skel_release().
> >   
> > Does skel_delete() really need to call usb_put_dev()?  I suspect that  
> > the driver doesn't need to take a reference to either the device or the
> > interface.
> >  
> 
> I think it will be needed, since usb core will decrement the reference
> when the device go away.

But before it decrements the reference, it will unbind the driver.  As 
long as usb-skeleton is careful not to access the device or the 
interface after the disconnect routine returns, it doesn't need to keep 
a reference.

>  Lock every access to the usb core if a tedious
> thing and will waste a lot of code.

With proper design, it's not necessary to lock every access.

> The problem with hotpluging is not the plug, it is the unplug, which can
> happen any time.
> 
> BTW: An interface have no reference count.

Sure it does.  Look at the definitions of usb_get_intf() and 
usb_put_intf() in drivers/usb/core/usb.c.

> > If skel_delete is changed then we don't need the usb_device pointer.
> > 
> 
> I have no idea how to do this. Every USB driver i know stores the
> usb_device pointer.
> 
> Maybe you have an idea?

No, I was wrong.  It turns out we really do need to keep the udev 
pointer, because it gets used in calls to usb_fill_bulk_urb().  However 
there's nothing wrong with keeping pointers to both the device and the 
interface, if there's a good reason for doing so.

(It also gets used in calls to usb_alloc_coherent(), but I would prefer
to get rid of the coherent buffers.  They are rarely useful and don't
belong here.)

Alan Stern

Re: [PATCH 04/11] remove usb_interface pointer

[Stefani Seibold]

Am Donnerstag, den 07.06.2012, 11:27 -0400 schrieb Alan Stern:
> On Thu, 7 Jun 2012, Stefani Seibold wrote:
> > I think it will be needed, since usb core will decrement the reference
> > when the device go away.
> 
> But before it decrements the reference, it will unbind the driver.  As 
> long as usb-skeleton is careful not to access the device or the 
> interface after the disconnect routine returns, it doesn't need to keep 
> a reference.
> 
> >  Lock every access to the usb core if a tedious
> > thing and will waste a lot of code.
> 
> With proper design, it's not necessary to lock every access.
> > 
> > BTW: An interface have no reference count.
> 
> Sure it does.  Look at the definitions of usb_get_intf() and 
> usb_put_intf() in drivers/usb/core/usb.c.
> 

That what i am looking for. If we do an usb_get_intf() in the
skel_probe() function and an usb_put_intf() in skel_delete() function at
the end, then it should be save to access the usb_device pointer by
interface_to_usbdev() in the skel_delete() function. Right?

Greetings,
Stefani

Re: [PATCH 04/11] remove usb_interface pointer

[Alan Stern]

On Thu, 7 Jun 2012, Stefani Seibold wrote:

> That what i am looking for. If we do an usb_get_intf() in the
> skel_probe() function and an usb_put_intf() in skel_delete() function at
> the end, then it should be save to access the usb_device pointer by
> interface_to_usbdev() in the skel_delete() function. Right?

Safe, yes, but unnecessary.  Just get rid of the usb_put_dev() in 
skel_delete and the usb_get_dev in skel_probe.

Alan Stern

RFC: remove usb_device pointer from usb_skeleton.c

[Stefani Seibold]

Hi,

what do you think about this solution for removing the usb_device
pointer from the struct usb_skel in the usb_skeleton driver?

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 1e36782..af698b3 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -47,7 +47,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 */
@@ -61,6 +60,7 @@ struct usb_skel {
 	int			errors;			/* the last request tanked */
 	bool			ongoing_read;		/* a read is going on */
 	bool			in_use;			/* in use flag */
+	bool			connected;		/* connected flag */
 	spinlock_t		err_lock;		/* lock for errors */
 	struct kref		kref;
 	struct mutex		io_mutex;		/* synchronize I/O with disconnect */
@@ -73,9 +73,12 @@ static struct usb_driver skel_driver;
 static void skel_delete(struct kref *kref)
 {
 	struct usb_skel *dev = to_skel_dev(kref);
+	struct usb_interface *interface = dev->interface;
+	struct usb_device *udev = usb_get_dev(interface_to_usbdev(interface));
 
 	usb_free_urb(dev->bulk_in_urb);
-	usb_put_dev(dev->udev);
+	usb_put_dev(udev);
+	usb_put_intf(interface);
 	kfree(dev->bulk_in_buffer);
 	kfree(dev);
 }
@@ -128,7 +131,7 @@ static int skel_release(struct inode *inode, struct file *file)
 
 	/* allow the device to be autosuspended */
 	mutex_lock(&dev->io_mutex);
-	if (dev->interface)
+	if (dev->connected)
 		usb_autopm_put_interface(dev->interface);
 	dev->in_use = false;
 	mutex_unlock(&dev->io_mutex);
@@ -155,7 +158,7 @@ static int skel_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 
 	/* wait for io to stop */
 	mutex_lock(&dev->io_mutex);
-	if (!dev->interface) {		/* disconnect() was called */
+	if (!dev->connected) {		/* disconnect() was called */
 		retval = -ENODEV;
 		goto exit;
 	}
@@ -206,11 +209,12 @@ static void skel_read_bulk_callback(struct urb *urb)
 static int skel_do_read_io(struct usb_skel *dev, size_t count)
 {
 	int retval;
+	struct usb_device *udev = usb_get_dev(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),
@@ -258,7 +262,7 @@ static ssize_t skel_read(struct file *file, char *buffer, size_t count,
 			return retval;
 	}
 
-	if (!dev->interface) {		/* disconnect() was called */
+	if (!dev->connected) {		/* disconnect() was called */
 		retval = -ENODEV;
 		goto exit;
 	}
@@ -391,6 +395,7 @@ static ssize_t skel_write(struct file *file, const char *user_buffer,
 	struct urb *urb = NULL;
 	char *buf = NULL;
 	size_t writesize = min(count, (size_t)MAX_TRANSFER);
+	struct usb_device *udev = usb_get_dev(interface_to_usbdev(dev->interface));
 
 	/* verify that we actually have some data to write */
 	if (!count)
@@ -427,7 +432,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;
@@ -441,15 +446,15 @@ 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 */
+	if (!dev->connected) {		/* disconnect() was called */
 		mutex_unlock(&dev->io_mutex);
 		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);
@@ -477,7 +482,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);
@@ -529,9 +534,10 @@ 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));
-	dev->interface = interface;
+	usb_get_dev(interface_to_usbdev(interface));
+	dev->interface = usb_get_intf(interface);
 	dev->in_use = false;
+	dev->connected = true;
 
 	/* set up the endpoint information */
 	/* use only the first bulk-in and bulk-out endpoints */
@@ -610,7 +616,7 @@ static void skel_disconnect(struct usb_interface *interface)
 
 	/* prevent more I/O from starting */
 	mutex_lock(&dev->io_mutex);
-	dev->interface = NULL;
+	dev->connected = false;
 	mutex_unlock(&dev->io_mutex);
 
 	usb_kill_anchored_urbs(&dev->submitted);


The connected flag could be also replaced by a "intf->condition ==
USB_INTERFACE_BOUND", which can be hided by a macro.

Regards,
Stefani

Re: RFC: remove usb_device pointer from usb_skeleton.c

[Oliver Neukum]

Am Freitag, 8. Juni 2012, 09:18:55 schrieb Stefani Seibold:
> The connected flag could be also replaced by a "intf->condition ==
> USB_INTERFACE_BOUND", which can be hided by a macro.

No, I am sorry, it cannot. You'd know the interface is bound, but not necessarily
to your driver or even the same "incarnation" of your driver. The fundamental
problem is that we have no data structure describing the binding between a device
and a driver so we cannot refcount that.

As a consequence all refcounting just ensures that the device and interface
data structures stay in memory, but we can make only very limited (read-only)
use of them after a disconnection.

	Regards
		Oliver

Re: RFC: remove usb_device pointer from usb_skeleton.c

[Alan Stern]

On Fri, 8 Jun 2012, Stefani Seibold wrote:

> Hi,
> 
> what do you think about this solution for removing the usb_device
> pointer from the struct usb_skel in the usb_skeleton driver?

There's nothing wrong with keeping the pointer there.  Why do you want 
to remove it?

> @@ -73,9 +73,12 @@ static struct usb_driver skel_driver;
>  static void skel_delete(struct kref *kref)
>  {
>  	struct usb_skel *dev = to_skel_dev(kref);
> +	struct usb_interface *interface = dev->interface;
> +	struct usb_device *udev = usb_get_dev(interface_to_usbdev(interface));
>  
>  	usb_free_urb(dev->bulk_in_urb);
> -	usb_put_dev(dev->udev);
> +	usb_put_dev(udev);

On the other hand, this usb_put_dev call is useless.  Why do you want 
to keep it?

> +	usb_put_intf(interface);

If you get rid of the usb_put_dev then this call is also useless.

>  	kfree(dev->bulk_in_buffer);
>  	kfree(dev);
>  }
> @@ -128,7 +131,7 @@ static int skel_release(struct inode *inode, struct file *file)
>  
>  	/* allow the device to be autosuspended */
>  	mutex_lock(&dev->io_mutex);
> -	if (dev->interface)
> +	if (dev->connected)

There's nothing really wrong with adding a "disconnected" flag.  But 
setting dev->interface to NULL works just as well, in my opinion.  It's 
a matter of taste.

Alan Stern

[PATCH 05/11] 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 f7fe32f..e8d337a 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -69,7 +69,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)
 {
@@ -139,6 +138,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;
@@ -601,16 +610,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 06/11] introduce 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 e8d337a..e766696 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -148,7 +148,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 res;
@@ -168,6 +168,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;
@@ -482,6 +487,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 07/11] 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 |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index e766696..cab16e2 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -84,15 +84,11 @@ 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);
 
-	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;
 	}
-- 
1.7.8.6

[PATCH 08/11] 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 cab16e2..46c1bbb 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -241,9 +241,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->connected) {		/* disconnect() was called */
 		retval = -ENODEV;
-- 
1.7.8.6

[PATCH 09/11] Synchronize disconnect() handler with open() and release(), to fix races

[stefani]

From: Stefani Seibold <stefani@seibold.net>

open()/release() races with disconnect() of an USB device

- 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.
- The dev pointer return by usb_get_intfdata() could point to an
  already release memory.

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

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 46c1bbb..11cc97b 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -63,13 +63,16 @@ struct usb_skel {
 	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 with disconnect */
 	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;
 
+/* synchronize open/release with disconnect */
+static DEFINE_MUTEX(sync_mutex);
+
 static void skel_delete(struct kref *kref)
 {
 	struct usb_skel *dev = to_skel_dev(kref);
@@ -86,6 +89,8 @@ static int skel_open(struct inode *inode, struct file *file)
 	struct usb_interface *interface;
 	int retval;
 
+	/* lock against skel_disconnect() */
+	mutex_lock(&sync_mutex);
 
 	interface = usb_find_interface(&skel_driver, iminor(inode));
 	if (!interface) {
@@ -99,22 +104,20 @@ 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);
-
 	retval = usb_autopm_get_interface(interface);
 	if (retval)
 		goto exit;
 
+	/* increment our usage count for the device */
+	kref_get(&dev->kref);
+
+	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;
 }
 
@@ -122,15 +125,16 @@ static int skel_release(struct inode *inode, struct file *file)
 {
 	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->connected)
 		usb_autopm_put_interface(
 			usb_find_interface(&skel_driver, iminor(inode)));
-	mutex_unlock(&dev->io_mutex);
 
 	/* decrement the count on our device */
 	kref_put(&dev->kref, skel_delete);
+	mutex_unlock(&sync_mutex);
 	return 0;
 }
 
@@ -436,9 +440,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->connected) {		/* disconnect() was called */
-		mutex_unlock(&dev->io_mutex);
 		retval = -ENODEV;
 		goto error;
 	}
@@ -452,7 +454,6 @@ 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->udev->dev,
 			"%s - failed submitting write urb, error %d\n",
@@ -596,25 +597,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);
 
+	dev = usb_get_intfdata(interface);
+	usb_kill_anchored_urbs(&dev->submitted);
+
+	/* lock against skel_open() and skel_release() */
+	mutex_lock(&sync_mutex);
+	usb_set_intfdata(interface, NULL);
+
 	/* prevent more I/O from starting */
-	mutex_lock(&dev->io_mutex);
 	dev->connected = false;
-	mutex_unlock(&dev->io_mutex);
-
-	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);
+	mutex_unlock(&sync_mutex);
 }
 
 static int skel_suspend(struct usb_interface *intf, pm_message_t message)
-- 
1.7.8.6

[PATCH 10/11] 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 |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index 11cc97b..81d256f 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -61,6 +61,7 @@ struct usb_skel {
 	int			errors;			/* the last request tanked */
 	bool			ongoing_read;		/* a read is going on */
 	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 with disconnect */
@@ -104,6 +105,11 @@ static int skel_open(struct inode *inode, struct file *file)
 		goto exit;
 	}
 
+	if (dev->in_use) {
+		retval = -EBUSY;
+		goto exit;
+	}
+
 	retval = usb_autopm_get_interface(interface);
 	if (retval)
 		goto exit;
@@ -111,6 +117,7 @@ static int skel_open(struct inode *inode, struct file *file)
 	/* 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 */
@@ -131,6 +138,7 @@ static int skel_release(struct inode *inode, struct file *file)
 	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);
@@ -529,6 +537,7 @@ static int skel_probe(struct usb_interface *interface,
 
 	dev->udev = usb_get_dev(interface_to_usbdev(interface));
 	dev->connected = true;
+	dev->in_use = false;
 
 	/* set up the endpoint information */
 	/* use only the first bulk-in and bulk-out endpoints */
-- 
1.7.8.6

[PATCH 11/11] 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 81d256f..c93841c 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
-- 
1.7.8.6

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

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 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, 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

[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, 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