LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Pete Zaitcev <zaitcev@redhat.com>
To: vitalivanov@gmail.com
Cc: Oliver Neukum <oliver@neukum.org>,
	linux-usb-devel@lists.sourceforge.net, greg@kroah.com,
	linux-kernel@vger.kernel.org, netwiz@crc.id.au,
	zaitcev@redhat.com
Subject: Re: USB: FIx locks and urb->status in adutux
Date: Tue, 30 Oct 2007 14:54:21 -0700	[thread overview]
Message-ID: <20071030145421.f1f0aa53.zaitcev@redhat.com> (raw)
In-Reply-To: <1193749794.27085.15.camel@dell1.softservecom.com>

On Tue, 30 Oct 2007 15:09:54 +0200, Vitaliy Ivanov <vitalivanov@gmail.com> wrote:

> As about read_urb_finished probably it's OK. But we shouldn't decrease
> open_count in the case of error as then we return normal exit value.

Oh, thanks. I fixed that.

One other small thing. I see you dropped the dev->mtx bracket that
I added around the initialization and submission. Notice that the
dev->udev is zeroed under dev->mtx, not the static lock. This is because
it has to be seen in read and write paths. If you don't like taking
across the submission, how about testing for it ahead of time?

Here's what I've got now:

diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..5a2c44e 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
 
 #define COMMAND_TIMEOUT	(2*HZ)	/* 60 second timeout for a command */
 
+/*
+ * The locking scheme is a vanilla 3-lock:
+ *   adu_device.buflock: A spinlock, covers what IRQs touch.
+ *   adutux_mutex:       A Static lock to cover open_count. It would also cover
+ *                       any globals, but we don't have them in 2.6.
+ *   adu_device.mtx:     A mutex to hold across sleepers like copy_from_user.
+ *                       It covers all of adu_device, except the open_count
+ *                       and what .buflock covers.
+ */
+
 /* Structure to hold all of our device specific stuff */
 struct adu_device {
-	struct mutex		mtx; /* locks this structure */
+	struct mutex		mtx;
 	struct usb_device*	udev; /* save off the usb device pointer */
 	struct usb_interface*	interface;
-	unsigned char		minor; /* the starting minor number for this device */
+	unsigned int		minor; /* the starting minor number for this device */
 	char			serial_number[8];
 
 	int			open_count; /* number of times this port has been opened */
@@ -107,8 +117,11 @@ struct adu_device {
 	char*			interrupt_out_buffer;
 	struct usb_endpoint_descriptor* interrupt_out_endpoint;
 	struct urb*		interrupt_out_urb;
+	int			out_urb_finished;
 };
 
+static DEFINE_MUTEX(adutux_mutex);
+
 static struct usb_driver adu_driver;
 
 static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, const char *function, int size,
  */
 static void adu_abort_transfers(struct adu_device *dev)
 {
-	dbg(2," %s : enter", __FUNCTION__);
+	unsigned long flags;
 
-	if (dev == NULL) {
-		dbg(1," %s : dev is null", __FUNCTION__);
-		goto exit;
-	}
+	dbg(2," %s : enter", __FUNCTION__);
 
 	if (dev->udev == NULL) {
 		dbg(1," %s : udev is null", __FUNCTION__);
 		goto exit;
 	}
 
-	dbg(2," %s : udev state %d", __FUNCTION__, dev->udev->state);
-	if (dev->udev->state == USB_STATE_NOTATTACHED) {
-		dbg(1," %s : udev is not attached", __FUNCTION__);
-		goto exit;
-	}
-
 	/* shutdown transfer */
-	usb_unlink_urb(dev->interrupt_in_urb);
-	usb_unlink_urb(dev->interrupt_out_urb);
+
+	/* XXX Anchor these instead */
+	spin_lock_irqsave(&dev->buflock, flags);
+	if (!dev->read_urb_finished) {
+		spin_unlock_irqrestore(&dev->buflock, flags);
+		usb_kill_urb(dev->interrupt_in_urb);
+	} else
+		spin_unlock_irqrestore(&dev->buflock, flags);
+
+	spin_lock_irqsave(&dev->buflock, flags);
+	if (!dev->out_urb_finished) {
+		spin_unlock_irqrestore(&dev->buflock, flags);
+		usb_kill_urb(dev->interrupt_out_urb);
+	} else
+		spin_unlock_irqrestore(&dev->buflock, flags);
 
 exit:
 	dbg(2," %s : leave", __FUNCTION__);
@@ -162,8 +179,6 @@ static void adu_delete(struct adu_device *dev)
 {
 	dbg(2, "%s enter", __FUNCTION__);
 
-	adu_abort_transfers(dev);
-
 	/* free data structures */
 	usb_free_urb(dev->interrupt_in_urb);
 	usb_free_urb(dev->interrupt_out_urb);
@@ -239,7 +254,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
 		goto exit;
 	}
 
-	wake_up_interruptible(&dev->write_wait);
+	spin_lock(&dev->buflock);
+	dev->out_urb_finished = 1;
+	wake_up(&dev->write_wait);
+	spin_unlock(&dev->buflock);
 exit:
 
 	adu_debug_data(5, __FUNCTION__, urb->actual_length,
@@ -252,12 +270,17 @@ static int adu_open(struct inode *inode, struct file *file)
 	struct adu_device *dev = NULL;
 	struct usb_interface *interface;
 	int subminor;
-	int retval = 0;
+	int retval;
 
 	dbg(2,"%s : enter", __FUNCTION__);
 
 	subminor = iminor(inode);
 
+	if ((retval = mutex_lock_interruptible(&adutux_mutex))) {
+		dbg(2, "%s : mutex lock failed", __FUNCTION__);
+		goto exit_no_lock;
+	}
+
 	interface = usb_find_interface(&adu_driver, subminor);
 	if (!interface) {
 		err("%s - error, can't find device for minor %d",
@@ -267,54 +290,54 @@ static int adu_open(struct inode *inode, struct file *file)
 	}
 
 	dev = usb_get_intfdata(interface);
-	if (!dev) {
+	if (!dev || !dev->udev) {
 		retval = -ENODEV;
 		goto exit_no_device;
 	}
 
-	/* lock this device */
-	if ((retval = mutex_lock_interruptible(&dev->mtx))) {
-		dbg(2, "%s : mutex lock failed", __FUNCTION__);
+	/* check that nobody else is using the device */
+	if (dev->open_count) {
+		retval = -EBUSY;
 		goto exit_no_device;
 	}
 
-	/* increment our usage count for the device */
 	++dev->open_count;
 	dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count);
 
 	/* save device in the file's private structure */
 	file->private_data = dev;
 
-	if (dev->open_count == 1) {
-		/* initialize in direction */
-		dev->read_buffer_length = 0;
+	/* initialize in direction */
+	dev->read_buffer_length = 0;
 
-		/* fixup first read by having urb waiting for it */
-		usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
-				 usb_rcvintpipe(dev->udev,
-				 		dev->interrupt_in_endpoint->bEndpointAddress),
-				 dev->interrupt_in_buffer,
-				 le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
-				 adu_interrupt_in_callback, dev,
-				 dev->interrupt_in_endpoint->bInterval);
-		/* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
-		dev->read_urb_finished = 0;
-		retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
-		if (retval)
-			--dev->open_count;
-	}
-	mutex_unlock(&dev->mtx);
+	/* fixup first read by having urb waiting for it */
+	usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
+			 usb_rcvintpipe(dev->udev,
+					dev->interrupt_in_endpoint->bEndpointAddress),
+			 dev->interrupt_in_buffer,
+			 le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+			 adu_interrupt_in_callback, dev,
+			 dev->interrupt_in_endpoint->bInterval);
+	dev->read_urb_finished = 0;
+	if (usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL))
+		dev->read_urb_finished = 1;
+	/* we ignore failure */
+	/* end of fixup for first read */
+
+	/* initialize out direction */
+	dev->out_urb_finished = 1;
+
+	retval = 0;
 
 exit_no_device:
+	mutex_unlock(&adutux_mutex);
+exit_no_lock:
 	dbg(2,"%s : leave, return value %d ", __FUNCTION__, retval);
-
 	return retval;
 }
 
-static int adu_release_internal(struct adu_device *dev)
+static void adu_release_internal(struct adu_device *dev)
 {
-	int retval = 0;
-
 	dbg(2," %s : enter", __FUNCTION__);
 
 	/* decrement our usage count for the device */
@@ -326,12 +349,11 @@ static int adu_release_internal(struct adu_device *dev)
 	}
 
 	dbg(2," %s : leave", __FUNCTION__);
-	return retval;
 }
 
 static int adu_release(struct inode *inode, struct file *file)
 {
-	struct adu_device *dev = NULL;
+	struct adu_device *dev;
 	int retval = 0;
 
 	dbg(2," %s : enter", __FUNCTION__);
@@ -343,15 +365,13 @@ static int adu_release(struct inode *inode, struct file *file)
 	}
 
 	dev = file->private_data;
-
 	if (dev == NULL) {
  		dbg(1," %s : object is NULL", __FUNCTION__);
 		retval = -ENODEV;
 		goto exit;
 	}
 
-	/* lock our device */
-	mutex_lock(&dev->mtx); /* not interruptible */
+	mutex_lock(&adutux_mutex); /* not interruptible */
 
 	if (dev->open_count <= 0) {
 		dbg(1," %s : device not opened", __FUNCTION__);
@@ -359,19 +379,15 @@ static int adu_release(struct inode *inode, struct file *file)
 		goto exit;
 	}
 
+	adu_release_internal(dev);
 	if (dev->udev == NULL) {
 		/* the device was unplugged before the file was released */
-		mutex_unlock(&dev->mtx);
-		adu_delete(dev);
-		dev = NULL;
-	} else {
-		/* do the work */
-		retval = adu_release_internal(dev);
+		if (!dev->open_count)	/* ... and we're the last user */
+			adu_delete(dev);
 	}
 
 exit:
-	if (dev)
-		mutex_unlock(&dev->mtx);
+	mutex_unlock(&adutux_mutex);
 	dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
 	return retval;
 }
@@ -393,12 +409,12 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
 
 	dev = file->private_data;
 	dbg(2," %s : dev=%p", __FUNCTION__, dev);
-	/* lock this object */
+
 	if (mutex_lock_interruptible(&dev->mtx))
 		return -ERESTARTSYS;
 
 	/* verify that the device wasn't unplugged */
-	if (dev->udev == NULL || dev->minor == 0) {
+	if (dev->udev == NULL) {
 		retval = -ENODEV;
 		err("No device or device unplugged %d", retval);
 		goto exit;
@@ -452,7 +468,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
 				should_submit = 1;
 			} else {
 				/* even the primary was empty - we may need to do IO */
-				if (dev->interrupt_in_urb->status == -EINPROGRESS) {
+				if (!dev->read_urb_finished) {
 					/* somebody is doing IO */
 					spin_unlock_irqrestore(&dev->buflock, flags);
 					dbg(2," %s : submitted already", __FUNCTION__);
@@ -460,6 +476,7 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
 					/* we must initiate input */
 					dbg(2," %s : initiate input", __FUNCTION__);
 					dev->read_urb_finished = 0;
+					spin_unlock_irqrestore(&dev->buflock, flags);
 
 					usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
 							 usb_rcvintpipe(dev->udev,
@@ -469,15 +486,12 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
 							 adu_interrupt_in_callback,
 							 dev,
 							 dev->interrupt_in_endpoint->bInterval);
-					retval = usb_submit_urb(dev->interrupt_in_urb, GFP_ATOMIC);
-					if (!retval) {
-						spin_unlock_irqrestore(&dev->buflock, flags);
-						dbg(2," %s : submitted OK", __FUNCTION__);
-					} else {
+					retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
+					if (retval) {
+						dev->read_urb_finished = 1;
 						if (retval == -ENOMEM) {
 							retval = bytes_read ? bytes_read : -ENOMEM;
 						}
-						spin_unlock_irqrestore(&dev->buflock, flags);
 						dbg(2," %s : submit failed", __FUNCTION__);
 						goto exit;
 					}
@@ -486,10 +500,14 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
 				/* we wait for I/O to complete */
 				set_current_state(TASK_INTERRUPTIBLE);
 				add_wait_queue(&dev->read_wait, &wait);
-				if (!dev->read_urb_finished)
+				spin_lock_irqsave(&dev->buflock, flags);
+				if (!dev->read_urb_finished) {
+					spin_unlock_irqrestore(&dev->buflock, flags);
 					timeout = schedule_timeout(COMMAND_TIMEOUT);
-				else
+				} else {
+					spin_unlock_irqrestore(&dev->buflock, flags);
 					set_current_state(TASK_RUNNING);
+				}
 				remove_wait_queue(&dev->read_wait, &wait);
 
 				if (timeout <= 0) {
@@ -509,19 +527,23 @@ static ssize_t adu_read(struct file *file, __user char *buffer, size_t count,
 
 	retval = bytes_read;
 	/* if the primary buffer is empty then use it */
-	if (should_submit && !dev->interrupt_in_urb->status==-EINPROGRESS) {
+	spin_lock_irqsave(&dev->buflock, flags);
+	if (should_submit && dev->read_urb_finished) {
+		dev->read_urb_finished = 0;
+		spin_unlock_irqrestore(&dev->buflock, flags);
 		usb_fill_int_urb(dev->interrupt_in_urb,dev->udev,
 				 usb_rcvintpipe(dev->udev,
 				 		dev->interrupt_in_endpoint->bEndpointAddress),
-						dev->interrupt_in_buffer,
-						le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
-						adu_interrupt_in_callback,
-						dev,
-						dev->interrupt_in_endpoint->bInterval);
-		/* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
-		dev->read_urb_finished = 0;
-		usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
+				dev->interrupt_in_buffer,
+				le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
+				adu_interrupt_in_callback,
+				dev,
+				dev->interrupt_in_endpoint->bInterval);
+		if (usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL) != 0)
+			dev->read_urb_finished = 1;
 		/* we ignore failure */
+	} else {
+		spin_unlock_irqrestore(&dev->buflock, flags);
 	}
 
 exit:
@@ -535,24 +557,24 @@ exit:
 static ssize_t adu_write(struct file *file, const __user char *buffer,
 			 size_t count, loff_t *ppos)
 {
+	DECLARE_WAITQUEUE(waita, current);
 	struct adu_device *dev;
 	size_t bytes_written = 0;
 	size_t bytes_to_write;
 	size_t buffer_size;
+	unsigned long flags;
 	int retval;
-	int timeout = 0;
 
 	dbg(2," %s : enter, count = %Zd", __FUNCTION__, count);
 
 	dev = file->private_data;
 
-	/* lock this object */
 	retval = mutex_lock_interruptible(&dev->mtx);
 	if (retval)
 		goto exit_nolock;
 
 	/* verify that the device wasn't unplugged */
-	if (dev->udev == NULL || dev->minor == 0) {
+	if (dev->udev == NULL) {
 		retval = -ENODEV;
 		err("No device or device unplugged %d", retval);
 		goto exit;
@@ -564,42 +586,37 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
 		goto exit;
 	}
 
-
 	while (count > 0) {
-		if (dev->interrupt_out_urb->status == -EINPROGRESS) {
-			timeout = COMMAND_TIMEOUT;
+		add_wait_queue(&dev->write_wait, &waita);
+		set_current_state(TASK_INTERRUPTIBLE);
+		spin_lock_irqsave(&dev->buflock, flags);
+		if (!dev->out_urb_finished) {
+			spin_unlock_irqrestore(&dev->buflock, flags);
 
-			while (timeout > 0) {
-				if (signal_pending(current)) {
+			mutex_unlock(&dev->mtx);
+			if (signal_pending(current)) {
 				dbg(1," %s : interrupted", __FUNCTION__);
+				set_current_state(TASK_RUNNING);
 				retval = -EINTR;
-				goto exit;
+				goto exit_onqueue;
 			}
-			mutex_unlock(&dev->mtx);
-			timeout = interruptible_sleep_on_timeout(&dev->write_wait, timeout);
+			if (schedule_timeout(COMMAND_TIMEOUT) == 0) {
+				dbg(1, "%s - command timed out.", __FUNCTION__);
+				retval = -ETIMEDOUT;
+				goto exit_onqueue;
+			}
+			remove_wait_queue(&dev->write_wait, &waita);
 			retval = mutex_lock_interruptible(&dev->mtx);
 			if (retval) {
 				retval = bytes_written ? bytes_written : retval;
 				goto exit_nolock;
 			}
-			if (timeout > 0) {
-				break;
-			}
-			dbg(1," %s : interrupted timeout: %d", __FUNCTION__, timeout);
-		}
-
-
-		dbg(1," %s : final timeout: %d", __FUNCTION__, timeout);
-
-		if (timeout == 0) {
-			dbg(1, "%s - command timed out.", __FUNCTION__);
-			retval = -ETIMEDOUT;
-			goto exit;
-		}
-
-		dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
 
+			dbg(4," %s : in progress, count = %Zd", __FUNCTION__, count);
 		} else {
+			spin_unlock_irqrestore(&dev->buflock, flags);
+			set_current_state(TASK_RUNNING);
+			remove_wait_queue(&dev->write_wait, &waita);
 			dbg(4," %s : sending, count = %Zd", __FUNCTION__, count);
 
 			/* write the data into interrupt_out_buffer from userspace */
@@ -622,11 +639,12 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
 				bytes_to_write,
 				adu_interrupt_out_callback,
 				dev,
-				dev->interrupt_in_endpoint->bInterval);
-			/* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
+				dev->interrupt_out_endpoint->bInterval);
 			dev->interrupt_out_urb->actual_length = bytes_to_write;
+			dev->out_urb_finished = 0;
 			retval = usb_submit_urb(dev->interrupt_out_urb, GFP_KERNEL);
 			if (retval < 0) {
+				dev->out_urb_finished = 1;
 				err("Couldn't submit interrupt_out_urb %d", retval);
 				goto exit;
 			}
@@ -637,16 +655,17 @@ static ssize_t adu_write(struct file *file, const __user char *buffer,
 			bytes_written += bytes_to_write;
 		}
 	}
-
-	retval = bytes_written;
+	mutex_unlock(&dev->mtx);
+	return bytes_written;
 
 exit:
-	/* unlock the device */
 	mutex_unlock(&dev->mtx);
 exit_nolock:
-
 	dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
+	return retval;
 
+exit_onqueue:
+	remove_wait_queue(&dev->write_wait, &waita);
 	return retval;
 }
 
@@ -831,25 +850,22 @@ static void adu_disconnect(struct usb_interface *interface)
 	dbg(2," %s : enter", __FUNCTION__);
 
 	dev = usb_get_intfdata(interface);
-	usb_set_intfdata(interface, NULL);
 
+	mutex_lock(&dev->mtx);	/* not interruptible */
+	dev->udev = NULL;	/* poison */
 	minor = dev->minor;
-
-	/* give back our minor */
 	usb_deregister_dev(interface, &adu_class);
-	dev->minor = 0;
+	mutex_unlock(&dev->mtx);
 
-	mutex_lock(&dev->mtx); /* not interruptible */
+	mutex_lock(&adutux_mutex);
+	usb_set_intfdata(interface, NULL);
 
 	/* if the device is not opened, then we clean up right now */
 	dbg(2," %s : open count %d", __FUNCTION__, dev->open_count);
-	if (!dev->open_count) {
-		mutex_unlock(&dev->mtx);
+	if (!dev->open_count)
 		adu_delete(dev);
-	} else {
-		dev->udev = NULL;
-		mutex_unlock(&dev->mtx);
-	}
+
+	mutex_unlock(&adutux_mutex);
 
 	dev_info(&interface->dev, "ADU device adutux%d now disconnected\n",
 		 (minor - ADU_MINOR_BASE));

-- Pete

  reply	other threads:[~2007-10-30 21:54 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-10-23  3:34 Pete Zaitcev
2007-10-23  9:38 ` [linux-usb-devel] " Oliver Neukum
2007-10-23 21:38   ` Pete Zaitcev
2007-10-24 14:04     ` Oliver Neukum
2007-10-24  1:53   ` Pete Zaitcev
2007-10-24 14:09     ` Vitaliy Ivanov
2007-10-25  3:20       ` Pete Zaitcev
2007-10-25  3:25       ` Pete Zaitcev
     [not found]         ` <200710251403.48688.oliver@neukum.org>
2007-10-25 16:38           ` Pete Zaitcev
2007-10-29 18:04         ` Vitaliy Ivanov
2007-10-30  4:24           ` Pete Zaitcev
2007-10-30 13:09             ` Vitaliy Ivanov
2007-10-30 21:54               ` Pete Zaitcev [this message]
2007-10-31 11:54                 ` Vitaliy Ivanov
2007-10-31 22:01                   ` Pete Zaitcev
2007-11-01  9:06                     ` Vitaliy Ivanov
2007-11-01 17:28                       ` Pete Zaitcev
2007-10-24 14:49     ` Oliver Neukum
2007-10-24 21:25       ` Greg KH
2007-10-26  9:57         ` Vitaliy Ivanov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20071030145421.f1f0aa53.zaitcev@redhat.com \
    --to=zaitcev@redhat.com \
    --cc=greg@kroah.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb-devel@lists.sourceforge.net \
    --cc=netwiz@crc.id.au \
    --cc=oliver@neukum.org \
    --cc=vitalivanov@gmail.com \
    --subject='Re: USB: FIx locks and urb->status in adutux' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).