From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1423747AbcBQSVS (ORCPT ); Wed, 17 Feb 2016 13:21:18 -0500 Received: from home.keithp.com ([63.227.221.253]:49714 "EHLO elaine.keithp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1423663AbcBQSVP (ORCPT ); Wed, 17 Feb 2016 13:21:15 -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: <86k2m343gd.fsf@hiro.keithp.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> <86k2m343gd.fsf@hiro.keithp.com> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Wed, 17 Feb 2016 10:21:13 -0800 Message-ID: <868u2j42qu.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 Keith Packard 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. --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-usb-misc-chaoskey-Cleanup-probe-failure-paths.patch Content-Transfer-Encoding: quoted-printable From=20d9ae8ca45c86e81a455bf19a4b4c580ec872c7df 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 | 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 =2D-- a/drivers/usb/misc/chaoskey.c +++ b/drivers/usb/misc/chaoskey.c @@ -93,10 +93,12 @@ 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"); + kfree(dev->name); + kfree(dev->buf); + kfree(dev); + } } =20 static int chaoskey_probe(struct usb_interface *interface, @@ -107,7 +109,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 +144,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 +158,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 +183,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 +210,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=207271de7c68a2cd288d2735d617f4e15b17f23c2a 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 | 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 =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 @@ -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 *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 */ @@ -237,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); @@ -311,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 @@ -343,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; } @@ -395,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; } @@ -435,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 iQIVAwUBVsS6GdsiGmkAAAARAQhx3w/8DBUxtlYRu8MIK57NXDgcVqOSY8726rwT 0jq0jyJ9bSaWOCeB2jTm69LxJVX/ZEcOFSb2dZO1aQXjwS9QVBq/tWF+OD1RCRGg h65vpMHC6h/p3xSD7iCpHqV5jrc8Ic+SExw04nDbzulOL7VUDpmEv7Im2RmJLrE8 Lrt0qyoHfb5hEkMtI1qaPM71aySmmSy3pMEO4vhjXUfJ8DuijycAs+Do5w21P4A9 1DJUWR42xHFDKb3hTe6o3Taq2mbL0HnvLfT53XtLAxlfcsRHP1qZnZH0DOONSDsn eH1fjrbqYAoLVhEuxIhAmaJHbyhW2c/tq0YLyNjzO1OkDLAB0W/BSw9A7kZVRG2Q +oeOrcbSilvd36lQ0xkJmuIE/XOZBXD/7zfulqDy5OaTHVAyDzqNtrFgRGUynwSx 3ZXOt5A3YLZAQ8xEen3s9aMuH4l7jmSFs3ZmaOq5sL6MFRmXcI0BnQgWePJezfuQ BDRSezbPBI3pgvKUAsNii6IwJkhZg8aB8/vADrOyVtOMSk2JsDKPsg5Z/4jcwTni wp4AkkRLr3tfQz623lnPYgi1rrWfRLTpb585+Y1yKVYvrEEQNpW01R6UeSUTV53v 4Q/jXe28pDass8DQjhvQVuIdrfA/NXGGb8lMEnHuHByLcv7YoqY+6l0g1Jh4GRlT dVwPtQ4FV2U= =Pg3q -----END PGP SIGNATURE----- --==-=-=--