* [PATCH] usb: check for signals in chaoskey read function @ 2016-02-16 2:49 Keith Packard 2016-02-16 8:24 ` Oliver Neukum 0 siblings, 1 reply; 6+ messages in thread From: Keith Packard @ 2016-02-16 2:49 UTC (permalink / raw) To: linux-usb; +Cc: Greg Kroah-Hartman, linux-kernel, Keith Packard Call signal_pending before reading a chunk of data from the device so that long read operations can be interrupted with a signal. Signed-off-by: Keith Packard <keithp@keithp.com> --- drivers/usb/misc/chaoskey.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c index 23c7948..ab87db2 100644 --- a/drivers/usb/misc/chaoskey.c +++ b/drivers/usb/misc/chaoskey.c @@ -394,6 +394,13 @@ static ssize_t chaoskey_read(struct file *file, if (result) goto bail; if (dev->valid == dev->used) { + + if (signal_pending(current)) { + result = -ERESTARTSYS; + mutex_unlock(&dev->lock); + goto bail; + } + result = _chaoskey_fill(dev); if (result) { mutex_unlock(&dev->lock); -- 2.7.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: check for signals in chaoskey read function 2016-02-16 2:49 [PATCH] usb: check for signals in chaoskey read function Keith Packard @ 2016-02-16 8:24 ` Oliver Neukum 2016-02-16 19:09 ` Keith Packard 0 siblings, 1 reply; 6+ messages in thread From: Oliver Neukum @ 2016-02-16 8:24 UTC (permalink / raw) To: Keith Packard; +Cc: linux-usb, Greg Kroah-Hartman, linux-kernel On Mon, 2016-02-15 at 18:49 -0800, Keith Packard wrote: > Call signal_pending before reading a chunk of data from the device so > that long read operations can be interrupted with a signal. Hi, why is this needed? You are doing this right after a mutex_lock_interruptible(). Regards Oliver ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: check for signals in chaoskey read function 2016-02-16 8:24 ` Oliver Neukum @ 2016-02-16 19:09 ` Keith Packard 2016-02-17 14:59 ` Oliver Neukum 0 siblings, 1 reply; 6+ messages in thread From: Keith Packard @ 2016-02-16 19:09 UTC (permalink / raw) To: Oliver Neukum; +Cc: linux-usb, Greg Kroah-Hartman, linux-kernel [-- Attachment #1: Type: text/plain, Size: 798 bytes --] Oliver Neukum <oneukum@suse.com> writes: > why is this needed? You are doing this right after a > mutex_lock_interruptible(). When the device isn't contended, this mutex will never block and so mutex_lock_interruptible will never check for a signal. I had mistakenly assumed that usb_bulk_msg would abort when a signal was delivered, but it doesn't. 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. -- -keith [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 810 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: check for signals in chaoskey read function 2016-02-16 19:09 ` Keith Packard @ 2016-02-17 14:59 ` Oliver Neukum 2016-02-17 18:05 ` Keith Packard 0 siblings, 1 reply; 6+ messages in thread From: Oliver Neukum @ 2016-02-17 14:59 UTC (permalink / raw) To: Keith Packard; +Cc: Greg Kroah-Hartman, linux-kernel, linux-usb [-- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: check for signals in chaoskey read function 2016-02-17 14:59 ` Oliver Neukum @ 2016-02-17 18:05 ` Keith Packard 2016-02-17 18:21 ` Keith Packard 0 siblings, 1 reply; 6+ messages in thread From: Keith Packard @ 2016-02-17 18:05 UTC (permalink / raw) To: Oliver Neukum; +Cc: Greg Kroah-Hartman, linux-kernel, linux-usb [-- Attachment #1.1: Type: text/plain, Size: 1570 bytes --] Oliver Neukum <oneukum@suse.com> writes: > 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? Yup, with a few minor fixes to pass the right arguments: diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c index 67102b4..76350e4 100644 --- a/drivers/usb/misc/chaoskey.c +++ b/drivers/usb/misc/chaoskey.c @@ -156,14 +156,14 @@ static int chaoskey_probe(struct usb_interface *interface, if (dev->buf == NULL) goto out; - dev->urb = usb_alloc_urb(GFP_KERNEL, 0); + dev->urb = usb_alloc_urb(0, GFP_KERNEL); if (!dev->urb) goto out; usb_fill_bulk_urb(dev->urb, udev, - usb_rcvbulkpipe(udev, altsetting->endpoint[in_ep].desc.bEndpointAddress), + usb_rcvbulkpipe(udev, in_ep), dev->buf, size, chaos_read_callback, The first patch also has the URB allocation, which should be in the second patch. I removed the comment about 'more bandwidth' as the driver is still synchronous and runs at the same speed as before. Thanks very much for making this 'right', instead of just kludging it. Here's a fixed sequence: [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1.2: 0001-usb-misc-chaoskey-Cleanup-probe-failure-paths.patch --] [-- Type: text/x-diff, Size: 3037 bytes --] From 6676d98364b10db5e30816dd6f9305439ee8e658 Mon Sep 17 00:00:00 2001 From: Oliver Neukum <oneukum@suse.com> Date: Wed, 17 Feb 2016 09:58:11 -0800 Subject: [PATCH 1/2] usb/misc/chaoskey: Cleanup probe failure paths Shares the cleanup code between all probe failure paths, instead of having per-failure cleanup at each point in the function. Signed-off-by: Oliver Neukum <oneukum@suse.com> Signed-off-by: Keith Packard <keithp@keithp.com> --- drivers/usb/misc/chaoskey.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c index 23c7948..cb7b829 100644 --- a/drivers/usb/misc/chaoskey.c +++ b/drivers/usb/misc/chaoskey.c @@ -93,10 +93,13 @@ struct chaoskey { 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 +110,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 +145,12 @@ 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; /* Construct a name using the product and serial values. Each * device needs a unique name for the hwrng code @@ -158,11 +159,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 +184,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 +211,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.7.0 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1.3: 0002-usb-misc-chaoskey-introduce-an-URB-for-asynchronous-.patch --] [-- Type: text/x-diff, Size: 4711 bytes --] From 01d91276c9dd8183ef524d45da3e0a3438fc133a Mon Sep 17 00:00:00 2001 From: Oliver Neukum <oneukum@suse.com> Date: Wed, 17 Feb 2016 10:01:33 -0800 Subject: [PATCH 2/2] usb/misc/chaoskey: introduce an URB for asynchronous reads To allow for and clean handling of signals an URB is introduced. Signed-off-by: Oliver Neukum <oneukum@suse.com> Signed-off-by: Keith Packard <keithp@keithp.com> --- drivers/usb/misc/chaoskey.c | 85 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 64 insertions(+), 21 deletions(-) diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c index cb7b829..76350e4 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; @@ -80,7 +82,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,6 +91,7 @@ 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; }; @@ -152,6 +156,19 @@ static int chaoskey_probe(struct usb_interface *interface, if (dev->buf == NULL) goto out; + dev->urb = usb_alloc_urb(0, GFP_KERNEL); + + if (!dev->urb) + goto out; + + usb_fill_bulk_urb(dev->urb, + udev, + usb_rcvbulkpipe(udev, in_ep), + 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 */ @@ -238,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); @@ -312,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"); @@ -344,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; } @@ -396,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; } @@ -436,6 +477,8 @@ bail: return read_count; } usb_dbg(dev->interface, "empty read, result %d", result); + if (result == -ETIMEDOUT) + result = -EAGAIN; return result; } -- 2.7.0 [-- Attachment #1.4: Type: text/plain, Size: 15 bytes --] -- -keith [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 810 bytes --] ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: check for signals in chaoskey read function 2016-02-17 18:05 ` Keith Packard @ 2016-02-17 18:21 ` Keith Packard 0 siblings, 0 replies; 6+ messages in thread From: Keith Packard @ 2016-02-17 18:21 UTC (permalink / raw) To: Oliver Neukum; +Cc: Greg Kroah-Hartman, linux-kernel, linux-usb [-- Attachment #1.1: Type: text/plain, Size: 447 bytes --] Keith Packard <keithp@keithp.com> writes: > Yup, with a few minor fixes to pass the right arguments: Argh. Just after hitting 'send', I noticed that I'd not moved the URB deallocation out of the way when merging the first patch in this series. That means the driver won't build with only the first patch applied, which is always annoying for other people bisecting through this. Here's the patches again, this time build tested in the middle. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1.2: 0001-usb-misc-chaoskey-Cleanup-probe-failure-paths.patch --] [-- Type: text/x-diff, Size: 3008 bytes --] From d9ae8ca45c86e81a455bf19a4b4c580ec872c7df Mon Sep 17 00:00:00 2001 From: Oliver Neukum <oneukum@suse.com> Date: Wed, 17 Feb 2016 09:58:11 -0800 Subject: [PATCH 1/2] usb/misc/chaoskey: Cleanup probe failure paths Shares the cleanup code between all probe failure paths, instead of having per-failure cleanup at each point in the function. Signed-off-by: Oliver Neukum <oneukum@suse.com> Signed-off-by: Keith Packard <keithp@keithp.com> --- drivers/usb/misc/chaoskey.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c index 23c7948..cb1c239 100644 --- a/drivers/usb/misc/chaoskey.c +++ b/drivers/usb/misc/chaoskey.c @@ -93,10 +93,12 @@ struct chaoskey { 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"); + kfree(dev->name); + kfree(dev->buf); + kfree(dev); + } } static int chaoskey_probe(struct usb_interface *interface, @@ -107,7 +109,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 +144,12 @@ 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; /* Construct a name using the product and serial values. Each * device needs a unique name for the hwrng code @@ -158,11 +158,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 +183,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 +210,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.7.0 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1.3: 0002-usb-misc-chaoskey-introduce-an-URB-for-asynchronous-.patch --] [-- Type: text/x-diff, Size: 4920 bytes --] From 7271de7c68a2cd288d2735d617f4e15b17f23c2a Mon Sep 17 00:00:00 2001 From: Oliver Neukum <oneukum@suse.com> Date: Wed, 17 Feb 2016 10:01:33 -0800 Subject: [PATCH 2/2] usb/misc/chaoskey: introduce an URB for asynchronous reads To allow for and clean handling of signals an URB is introduced. Signed-off-by: Oliver Neukum <oneukum@suse.com> Signed-off-by: Keith Packard <keithp@keithp.com> --- drivers/usb/misc/chaoskey.c | 86 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 65 insertions(+), 21 deletions(-) diff --git a/drivers/usb/misc/chaoskey.c b/drivers/usb/misc/chaoskey.c index cb1c239..76350e4 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; @@ -80,7 +82,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,6 +91,7 @@ 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; }; @@ -95,6 +99,7 @@ static void chaoskey_free(struct chaoskey *dev) { if (dev) { usb_dbg(dev->interface, "free"); + usb_free_urb(dev->urb); kfree(dev->name); kfree(dev->buf); kfree(dev); @@ -151,6 +156,19 @@ static int chaoskey_probe(struct usb_interface *interface, if (dev->buf == NULL) goto out; + dev->urb = usb_alloc_urb(0, GFP_KERNEL); + + if (!dev->urb) + goto out; + + usb_fill_bulk_urb(dev->urb, + udev, + usb_rcvbulkpipe(udev, in_ep), + 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 */ @@ -237,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); @@ -311,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"); @@ -343,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; } @@ -395,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; } @@ -435,6 +477,8 @@ bail: return read_count; } usb_dbg(dev->interface, "empty read, result %d", result); + if (result == -ETIMEDOUT) + result = -EAGAIN; return result; } -- 2.7.0 [-- Attachment #1.4: Type: text/plain, Size: 15 bytes --] -- -keith [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 810 bytes --] ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-17 18:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-16 2:49 [PATCH] usb: check for signals in chaoskey read function Keith Packard 2016-02-16 8:24 ` Oliver Neukum 2016-02-16 19:09 ` Keith Packard 2016-02-17 14:59 ` Oliver Neukum 2016-02-17 18:05 ` Keith Packard 2016-02-17 18:21 ` Keith Packard
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.