Re: [PATCH] usb: check for signals in chaoskey read function

[Oliver Neukum <oneukum@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 526 bytes --]

On Tue, 2016-02-16 at 11:09 -0800, Keith Packard wrote:
> I could be convinced that the driver should be using a different path
> through the USB stack that would allow a signal to wake up while
> waiting
> for the URB to complete, but this patch at least avoids needing to
> wait
> for a huge read to finish. The other option would be to eliminate the
> loop reading multiple URBs from the device, but that would reduce the
> available bandwidth from the device pretty considerably.

Do these do the job?

	Regards
		Oliver


[-- Attachment #2: 0001-chaoskey-introduce-an-URB-for-asynchronous-reads.patch --]
[-- Type: text/x-patch, Size: 3549 bytes --]

From 73fa56840b4158b203479eabf9be1e60da5ca4d2 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Wed, 17 Feb 2016 10:51:56 +0100
Subject: [PATCH 1/2] chaoskey: introduce an URB for asynchronous reads

To get more bandwidth and clean handling of signals
an URB is introduced. Also a cleanup of allocation
and freeing resources.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/misc/chaoskey.c | 46 ++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c
index 23c7948..5a67a04 100644
--- a/drivers/usb/misc/chaoskey.c
+++ b/drivers/usb/misc/chaoskey.c
@@ -80,7 +80,8 @@ struct chaoskey {
 	struct mutex lock;
 	struct mutex rng_lock;
 	int open;			/* open count */
-	int present;			/* device not disconnected */
+	bool present;			/* device not disconnected */
+	bool reading;			/* ongoing IO */
 	int size;			/* size of buf */
 	int valid;			/* bytes of buf read */
 	int used;			/* bytes of buf consumed */
@@ -88,15 +89,19 @@ struct chaoskey {
 	struct hwrng hwrng;		/* Embedded struct for hwrng */
 	int hwrng_registered;		/* registered with hwrng API */
 	wait_queue_head_t wait_q;	/* for timeouts */
+	struct urb *urb;		/* for performing IO */
 	char *buf;
 };
 
 static void chaoskey_free(struct chaoskey *dev)
 {
-	usb_dbg(dev->interface, "free");
-	kfree(dev->name);
-	kfree(dev->buf);
-	kfree(dev);
+	if (dev) {
+		usb_dbg(dev->interface, "free");
+		usb_free_urb(dev->urb);
+		kfree(dev->name);
+		kfree(dev->buf);
+		kfree(dev);
+	}
 }
 
 static int chaoskey_probe(struct usb_interface *interface,
@@ -107,7 +112,7 @@ static int chaoskey_probe(struct usb_interface *interface,
 	int i;
 	int in_ep = -1;
 	struct chaoskey *dev;
-	int result;
+	int result = -ENOMEM;
 	int size;
 
 	usb_dbg(interface, "probe %s-%s", udev->product, udev->serial);
@@ -142,14 +147,17 @@ static int chaoskey_probe(struct usb_interface *interface,
 	dev = kzalloc(sizeof(struct chaoskey), GFP_KERNEL);
 
 	if (dev == NULL)
-		return -ENOMEM;
+		goto out;
 
 	dev->buf = kmalloc(size, GFP_KERNEL);
 
-	if (dev->buf == NULL) {
-		kfree(dev);
-		return -ENOMEM;
-	}
+	if (dev->buf == NULL)
+		goto out;
+
+	dev->urb = usb_alloc_urb(GFP_KERNEL, 0);
+
+	if (!dev->urb)
+		goto out;
 
 	/* Construct a name using the product and serial values. Each
 	 * device needs a unique name for the hwrng code
@@ -158,11 +166,8 @@ static int chaoskey_probe(struct usb_interface *interface,
 	if (udev->product && udev->serial) {
 		dev->name = kmalloc(strlen(udev->product) + 1 +
 				    strlen(udev->serial) + 1, GFP_KERNEL);
-		if (dev->name == NULL) {
-			kfree(dev->buf);
-			kfree(dev);
-			return -ENOMEM;
-		}
+		if (dev->name == NULL)
+			goto out;
 
 		strcpy(dev->name, udev->product);
 		strcat(dev->name, "-");
@@ -186,9 +191,7 @@ static int chaoskey_probe(struct usb_interface *interface,
 	result = usb_register_dev(interface, &chaoskey_class);
 	if (result) {
 		usb_err(interface, "Unable to allocate minor number.");
-		usb_set_intfdata(interface, NULL);
-		chaoskey_free(dev);
-		return result;
+		goto out;
 	}
 
 	dev->hwrng.name = dev->name ? dev->name : chaoskey_driver.name;
@@ -215,6 +218,11 @@ static int chaoskey_probe(struct usb_interface *interface,
 
 	usb_dbg(interface, "chaoskey probe success, size %d", dev->size);
 	return 0;
+
+out:
+	usb_set_intfdata(interface, NULL);
+	chaoskey_free(dev);
+	return result;
 }
 
 static void chaoskey_disconnect(struct usb_interface *interface)
-- 
2.1.4


[-- Attachment #3: 0002-chaoskey-use-an-URB-for-requesting-data.patch --]
[-- Type: text/x-patch, Size: 3775 bytes --]

From 075d334923bee2644bcb2cb59a1f4aee1369c800 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Wed, 17 Feb 2016 14:56:57 +0100
Subject: [PATCH 2/2] chaoskey: use an URB for requesting data

This allows handling signals.

Signed-off-by: Oliver Neukum <ONeukum@suse.com>
---
 drivers/usb/misc/chaoskey.c | 76 +++++++++++++++++++++++++++++++++------------
 1 file changed, 56 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c
index 5a67a04..67102b4 100644
--- a/drivers/usb/misc/chaoskey.c
+++ b/drivers/usb/misc/chaoskey.c
@@ -73,6 +73,8 @@ static const struct usb_device_id chaoskey_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, chaoskey_table);
 
+static void chaos_read_callback(struct urb *urb);
+
 /* Driver-local specific stuff */
 struct chaoskey {
 	struct usb_interface *interface;
@@ -159,6 +161,14 @@ static int chaoskey_probe(struct usb_interface *interface,
 	if (!dev->urb)
 		goto out;
 
+	usb_fill_bulk_urb(dev->urb,
+		udev,
+		usb_rcvbulkpipe(udev, altsetting->endpoint[in_ep].desc.bEndpointAddress),
+		dev->buf,
+		size,
+		chaos_read_callback,
+		dev);
+
 	/* Construct a name using the product and serial values. Each
 	 * device needs a unique name for the hwrng code
 	 */
@@ -245,6 +255,7 @@ static void chaoskey_disconnect(struct usb_interface *interface)
 	mutex_lock(&dev->lock);
 
 	dev->present = 0;
+	usb_poison_urb(dev->urb);
 
 	if (!dev->open) {
 		mutex_unlock(&dev->lock);
@@ -319,14 +330,33 @@ static int chaoskey_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static void chaos_read_callback(struct urb *urb)
+{
+	struct chaoskey *dev = urb->context;
+	int status = urb->status;
+
+	usb_dbg(dev->interface, "callback status (%d)", status);
+
+	if (status == 0)
+		dev->valid = urb->actual_length;
+	else
+		dev->valid = 0;
+
+	dev->used = 0;
+
+	/* must be seen first before validity is announced */
+	smp_wmb();
+
+	dev->reading = false;
+	wake_up(&dev->wait_q);
+}
+
 /* Fill the buffer. Called with dev->lock held
  */
 static int _chaoskey_fill(struct chaoskey *dev)
 {
 	DEFINE_WAIT(wait);
 	int result;
-	int this_read;
-	struct usb_device *udev = interface_to_usbdev(dev->interface);
 
 	usb_dbg(dev->interface, "fill");
 
@@ -351,21 +381,31 @@ static int _chaoskey_fill(struct chaoskey *dev)
 		return result;
 	}
 
-	result = usb_bulk_msg(udev,
-			      usb_rcvbulkpipe(udev, dev->in_ep),
-			      dev->buf, dev->size, &this_read,
-			      NAK_TIMEOUT);
+	dev->reading = true;
+	result = usb_submit_urb(dev->urb, GFP_KERNEL);
+	if (result < 0) {
+		result = usb_translate_errors(result);
+		dev->reading = false;
+		goto out;
+	}
+
+	result = wait_event_interruptible_timeout(
+		dev->wait_q,
+		!dev->reading,
+		NAK_TIMEOUT);
+
+	if (result < 0)
+		goto out;
 
+	if (result == 0)
+		result = -ETIMEDOUT;
+	else
+		result = dev->valid;
+out:
 	/* Let the device go back to sleep eventually */
 	usb_autopm_put_interface(dev->interface);
 
-	if (result == 0) {
-		dev->valid = this_read;
-		dev->used = 0;
-	}
-
-	usb_dbg(dev->interface, "bulk_msg result %d this_read %d",
-		result, this_read);
+	usb_dbg(dev->interface, "read %d bytes", dev->valid);
 
 	return result;
 }
@@ -403,13 +443,7 @@ static ssize_t chaoskey_read(struct file *file,
 			goto bail;
 		if (dev->valid == dev->used) {
 			result = _chaoskey_fill(dev);
-			if (result) {
-				mutex_unlock(&dev->lock);
-				goto bail;
-			}
-
-			/* Read returned zero bytes */
-			if (dev->used == dev->valid) {
+			if (result < 0) {
 				mutex_unlock(&dev->lock);
 				goto bail;
 			}
@@ -443,6 +477,8 @@ bail:
 		return read_count;
 	}
 	usb_dbg(dev->interface, "empty read, result %d", result);
+	if (result == -ETIMEDOUT)
+		result = -EAGAIN;
 	return result;
 }
 
-- 
2.1.4