Re: [PATCH] usb: check for signals in chaoskey read function
From: Oliver Neukum
Date: Wed Feb 17 2016 - 10:02:17 EST
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
From 73fa56840b4158b203479eabf9be1e60da5ca4d2 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@xxxxxxxx>
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@xxxxxxxx>
---
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
From 075d334923bee2644bcb2cb59a1f4aee1369c800 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@xxxxxxxx>
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@xxxxxxxx>
---
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