From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1423625AbcBQSGN (ORCPT ); Wed, 17 Feb 2016 13:06:13 -0500 Received: from home.keithp.com ([63.227.221.253]:49520 "EHLO elaine.keithp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1423479AbcBQSF5 (ORCPT ); Wed, 17 Feb 2016 13:05:57 -0500 From: Keith Packard To: Oliver Neukum Cc: Greg Kroah-Hartman , linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Subject: Re: [PATCH] usb: check for signals in chaoskey read function In-Reply-To: <1455721173.7626.23.camel@suse.com> References: <1455590972-2120-1-git-send-email-keithp@keithp.com> <1455611093.4532.2.camel@suse.com> <86h9h8ii9o.fsf@hiro.keithp.com> <1455721173.7626.23.camel@suse.com> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Wed, 17 Feb 2016 10:05:54 -0800 Message-ID: <86k2m343gd.fsf@hiro.keithp.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="==-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --==-=-= Content-Type: multipart/mixed; boundary="=-=-=" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Oliver Neukum 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 =2D-- a/drivers/usb/misc/chaoskey.c +++ b/drivers/usb/misc/chaoskey.c @@ -156,14 +156,14 @@ static int chaoskey_probe(struct usb_interface *inter= face, if (dev->buf =3D=3D NULL) goto out; =20 =2D dev->urb =3D usb_alloc_urb(GFP_KERNEL, 0); + dev->urb =3D usb_alloc_urb(0, GFP_KERNEL); =20 if (!dev->urb) goto out; =20 usb_fill_bulk_urb(dev->urb, udev, =2D usb_rcvbulkpipe(udev, altsetting->endpoint[in_ep].desc.bEndpointAddres= s), + 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: --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-usb-misc-chaoskey-Cleanup-probe-failure-paths.patch Content-Transfer-Encoding: quoted-printable From=206676d98364b10db5e30816dd6f9305439ee8e658 Mon Sep 17 00:00:00 2001 From: Oliver Neukum 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 Signed-off-by: Keith Packard =2D-- 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 =2D-- a/drivers/usb/misc/chaoskey.c +++ b/drivers/usb/misc/chaoskey.c @@ -93,10 +93,13 @@ struct chaoskey { =20 static void chaoskey_free(struct chaoskey *dev) { =2D usb_dbg(dev->interface, "free"); =2D kfree(dev->name); =2D kfree(dev->buf); =2D kfree(dev); + if (dev) { + usb_dbg(dev->interface, "free"); + usb_free_urb(dev->urb); + kfree(dev->name); + kfree(dev->buf); + kfree(dev); + } } =20 static int chaoskey_probe(struct usb_interface *interface, @@ -107,7 +110,7 @@ static int chaoskey_probe(struct usb_interface *interfa= ce, int i; int in_ep =3D -1; struct chaoskey *dev; =2D int result; + int result =3D -ENOMEM; int size; =20 usb_dbg(interface, "probe %s-%s", udev->product, udev->serial); @@ -142,14 +145,12 @@ static int chaoskey_probe(struct usb_interface *inter= face, dev =3D kzalloc(sizeof(struct chaoskey), GFP_KERNEL); =20 if (dev =3D=3D NULL) =2D return -ENOMEM; + goto out; =20 dev->buf =3D kmalloc(size, GFP_KERNEL); =20 =2D if (dev->buf =3D=3D NULL) { =2D kfree(dev); =2D return -ENOMEM; =2D } + if (dev->buf =3D=3D NULL) + goto out; =20 /* 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 *interf= ace, if (udev->product && udev->serial) { dev->name =3D kmalloc(strlen(udev->product) + 1 + strlen(udev->serial) + 1, GFP_KERNEL); =2D if (dev->name =3D=3D NULL) { =2D kfree(dev->buf); =2D kfree(dev); =2D return -ENOMEM; =2D } + if (dev->name =3D=3D NULL) + goto out; =20 strcpy(dev->name, udev->product); strcat(dev->name, "-"); @@ -186,9 +184,7 @@ static int chaoskey_probe(struct usb_interface *interfa= ce, result =3D usb_register_dev(interface, &chaoskey_class); if (result) { usb_err(interface, "Unable to allocate minor number."); =2D usb_set_intfdata(interface, NULL); =2D chaoskey_free(dev); =2D return result; + goto out; } =20 dev->hwrng.name =3D dev->name ? dev->name : chaoskey_driver.name; @@ -215,6 +211,11 @@ static int chaoskey_probe(struct usb_interface *interf= ace, =20 usb_dbg(interface, "chaoskey probe success, size %d", dev->size); return 0; + +out: + usb_set_intfdata(interface, NULL); + chaoskey_free(dev); + return result; } =20 static void chaoskey_disconnect(struct usb_interface *interface) =2D-=20 2.7.0 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0002-usb-misc-chaoskey-introduce-an-URB-for-asynchronous-.patch Content-Transfer-Encoding: quoted-printable From=2001d91276c9dd8183ef524d45da3e0a3438fc133a Mon Sep 17 00:00:00 2001 From: Oliver Neukum 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 Signed-off-by: Keith Packard =2D-- 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 =2D-- a/drivers/usb/misc/chaoskey.c +++ b/drivers/usb/misc/chaoskey.c @@ -73,6 +73,8 @@ static const struct usb_device_id chaoskey_table[] =3D { }; MODULE_DEVICE_TABLE(usb, chaoskey_table); =20 +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 */ =2D 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; }; =20 @@ -152,6 +156,19 @@ static int chaoskey_probe(struct usb_interface *interf= ace, if (dev->buf =3D=3D NULL) goto out; =20 + dev->urb =3D 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 *i= nterface) mutex_lock(&dev->lock); =20 dev->present =3D 0; + usb_poison_urb(dev->urb); =20 if (!dev->open) { mutex_unlock(&dev->lock); @@ -312,14 +330,33 @@ static int chaoskey_release(struct inode *inode, stru= ct file *file) return 0; } =20 +static void chaos_read_callback(struct urb *urb) +{ + struct chaoskey *dev =3D urb->context; + int status =3D urb->status; + + usb_dbg(dev->interface, "callback status (%d)", status); + + if (status =3D=3D 0) + dev->valid =3D urb->actual_length; + else + dev->valid =3D 0; + + dev->used =3D 0; + + /* must be seen first before validity is announced */ + smp_wmb(); + + dev->reading =3D 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; =2D int this_read; =2D struct usb_device *udev =3D interface_to_usbdev(dev->interface); =20 usb_dbg(dev->interface, "fill"); =20 @@ -344,21 +381,31 @@ static int _chaoskey_fill(struct chaoskey *dev) return result; } =20 =2D result =3D usb_bulk_msg(udev, =2D usb_rcvbulkpipe(udev, dev->in_ep), =2D dev->buf, dev->size, &this_read, =2D NAK_TIMEOUT); + dev->reading =3D true; + result =3D usb_submit_urb(dev->urb, GFP_KERNEL); + if (result < 0) { + result =3D usb_translate_errors(result); + dev->reading =3D false; + goto out; + } + + result =3D wait_event_interruptible_timeout( + dev->wait_q, + !dev->reading, + NAK_TIMEOUT); =20 + if (result < 0) + goto out; + + if (result =3D=3D 0) + result =3D -ETIMEDOUT; + else + result =3D dev->valid; +out: /* Let the device go back to sleep eventually */ usb_autopm_put_interface(dev->interface); =20 =2D if (result =3D=3D 0) { =2D dev->valid =3D this_read; =2D dev->used =3D 0; =2D } =2D =2D usb_dbg(dev->interface, "bulk_msg result %d this_read %d", =2D result, this_read); + usb_dbg(dev->interface, "read %d bytes", dev->valid); =20 return result; } @@ -396,13 +443,7 @@ static ssize_t chaoskey_read(struct file *file, goto bail; if (dev->valid =3D=3D dev->used) { result =3D _chaoskey_fill(dev); =2D if (result) { =2D mutex_unlock(&dev->lock); =2D goto bail; =2D } =2D =2D /* Read returned zero bytes */ =2D if (dev->used =3D=3D 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 =3D=3D -ETIMEDOUT) + result =3D -EAGAIN; return result; } =20 =2D-=20 2.7.0 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable =2D-=20 =2Dkeith --=-=-=-- --==-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIVAwUBVsS2gtsiGmkAAAARAQgUTg//ZnNmVeGxlGNJryz2ogE4O23UdO0H4iQJ EeQ2LPmEvWKdypC/5+iaIAd3AhrTnFDKKCR1r8KV8b2EwQ00klm0XWjua77XQNxG s5jU5FDmUnc3KqCCyWsEa1y4FMiRx5RFLit1jvfmgDOIusyDO2zQJ+5QUdy3tVCh vmZy4DwoxbWTH8hOLeUQBGhgwg8jGNowHiARMMIAMyewDksL1+K/6Dp6Z6DDokAO QCdtWDiVhZ7nnlugxEKgfandlRLWPWUcsQgu4rn8FcnQAzIkFfGOJR+DK6cRTEh4 ACgz6/pGfTEJSyih1s0vx8wgrGjaYOzqMpAdAQAaPXi4XRetI+ohTnIax3E6JfYM hvBt9Q9XOeIOQGBEI8qEM0/qUV8o9bE/NjvBt0vx6FquZ2E5Vfl8uDu9Dkkn3J1x dTvP2S0U7OzLx+9Nn07GNPDUhMDNEop00ZfraPCb+CQTP2HyPellx+/1BQKBWJzl hYLMRqUewRDdZb94/HJqYeDt7yk4XJajuGHkH7AGykfjriFKj9Z/9BO1nOfq/Zak KZ58hDLxy7gx5X/vqKuYvc9BJJF3n0bDu468rMCnLj7MF/ewD6qYTJmuEJ33jDtm BEdc5QX+eY9tAwZCk9hcR0EKJCduyxfUeS6S3mV3afTGWDGuzCwHhl7OJ0VpY7de gvnwfQMn/No= =9luN -----END PGP SIGNATURE----- --==-=-=--