Re: [PATCH] USB: usb-skeleton.c: fix blocked forever in skel_read

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Friday, March 15, 2013 12:53 AM, Oliver Neukum wrote:

> The problem is that I needed to work around the counting nature of completions.
> If you go to a waitqueue the need is removed. Your original patch together with
> the change to use a wait queue would be correct.


Got that. Below is original patch together with the change to use a waitqueue.
If it's OK to be merged, do i need to re-send it in another email.
---
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index ce31017..291e0ca 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 */
@@ -201,6 +200,12 @@ static int skel_do_read_io(struct usb_skel *dev, size_t count)
 			min(dev->bulk_in_size, count),
 			skel_read_bulk_callback,
 			dev);
+
+	/* submit bulk in urb, which means no data to deliver */
+	INIT_COMPLETION(dev->bulk_in_completion);
+	dev->bulk_in_copied = 0;
+	dev->bulk_in_filled = 0;
+
 	/* tell everybody to leave the URB alone */
 	spin_lock_irq(&dev->err_lock);
 	dev->ongoing_read = 1;
@@ -212,7 +217,6 @@ static int skel_do_read_io(struct usb_skel *dev, size_t count)
 		dev_err(&dev->interface->dev,
 			"%s - failed submitting read urb, error %d\n",
 			__func__, rv);
-		dev->bulk_in_filled = 0;
 		rv = (rv == -ENOMEM) ? rv : -EIO;
 		spin_lock_irq(&dev->err_lock);
 		dev->ongoing_read = 0;
@@ -264,22 +268,6 @@ retry:
 		rv = wait_for_completion_interruptible(&dev->bulk_in_completion);
 		if (rv < 0)
 			goto exit;
-		/*
-		 * by waiting we also semiprocessed the urb
-		 * we must finish now
-		 */
-		dev->bulk_in_copied = 0;
-		dev->processed_urb = 1;
-	}
-
-	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 = 1;
 	}
 
 	/* errors must be reported */
@@ -289,8 +277,6 @@ retry:
 		dev->errors = 0;
 		/* to preserve notifications about reset */
 		rv = (rv == -EPIPE) ? rv : -EIO;
-		/* no data to deliver */
-		dev->bulk_in_filled = 0;
 		/* report it */
 		goto exit;
 	}
---


> Not good. You are sleeping with a spinlock held. This is absolutely bad.
> If you wish to retain the completion you need to change locking a bit more.


Just for learning, I want to know whether below patch which retain the completion work or not.
---
diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
index ce31017..7ed3b03 100644
--- a/drivers/usb/usb-skeleton.c
+++ b/drivers/usb/usb-skeleton.c
@@ -61,11 +61,10 @@ 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 */
-	struct completion	bulk_in_completion;	/* to wait for an ongoing read */
+	wait_queue_head_t	bulk_in_wait;		/* to wait for an ongoing read */
 };
 #define to_skel_dev(d) container_of(d, struct usb_skel, kref)
 
@@ -185,7 +184,7 @@ static void skel_read_bulk_callback(struct urb *urb)
 	dev->ongoing_read = 0;
 	spin_unlock(&dev->err_lock);
 
-	complete(&dev->bulk_in_completion);
+	wake_up_interruptible(&dev->bulk_in_wait);
 }
 
 static int skel_do_read_io(struct usb_skel *dev, size_t count)
@@ -206,13 +205,16 @@ static int skel_do_read_io(struct usb_skel *dev, size_t count)
 	dev->ongoing_read = 1;
 	spin_unlock_irq(&dev->err_lock);
 
+	/* submit bulk in urb, which means no data to deliver */
+	dev->bulk_in_filled = 0;
+	dev->bulk_in_copied = 0;
+
 	/* do it */
 	rv = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL);
 	if (rv < 0) {
 		dev_err(&dev->interface->dev,
 			"%s - failed submitting read urb, error %d\n",
 			__func__, rv);
-		dev->bulk_in_filled = 0;
 		rv = (rv == -ENOMEM) ? rv : -EIO;
 		spin_lock_irq(&dev->err_lock);
 		dev->ongoing_read = 0;
@@ -261,25 +263,9 @@ retry:
 		 * IO may take forever
 		 * hence wait in an interruptible state
 		 */
-		rv = wait_for_completion_interruptible(&dev->bulk_in_completion);
+		rv = wait_event_interruptible(dev->bulk_in_wait, (!dev->ongoing_read));
 		if (rv < 0)
 			goto exit;
-		/*
-		 * by waiting we also semiprocessed the urb
-		 * we must finish now
-		 */
-		dev->bulk_in_copied = 0;
-		dev->processed_urb = 1;
-	}
-
-	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 = 1;
 	}
 
 	/* errors must be reported */
@@ -289,8 +275,6 @@ retry:
 		dev->errors = 0;
 		/* to preserve notifications about reset */
 		rv = (rv == -EPIPE) ? rv : -EIO;
-		/* no data to deliver */
-		dev->bulk_in_filled = 0;
 		/* report it */
 		goto exit;
 	}
@@ -526,7 +510,7 @@ static int skel_probe(struct usb_interface *interface,
 	mutex_init(&dev->io_mutex);
 	spin_lock_init(&dev->err_lock);
 	init_usb_anchor(&dev->submitted);
-	init_completion(&dev->bulk_in_completion);
+	init_waitqueue_head(&dev->bulk_in_wait);
 
 	dev->udev = usb_get_dev(interface_to_usbdev(interface));
 	dev->interface = interface;
---
Du Xing
	Thanks

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux