Re: [PATCH] fix usb skeleton driver

[Oliver Neukum <oneukum@suse.de>]

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