Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1334965 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: check for signals in chaoskey read function Keith Packard <keithp@keithp.com> - 2016-02-16 04:00 +0100
    Re: [PATCH] usb: check for signals in chaoskey read function Oliver Neukum <oneukum@suse.com> - 2016-02-16 09:30 +0100
      Re: [PATCH] usb: check for signals in chaoskey read function Keith Packard <keithp@keithp.com> - 2016-02-16 20:20 +0100
        Re: [PATCH] usb: check for signals in chaoskey read function Oliver Neukum <oneukum@suse.com> - 2016-02-17 16:10 +0100
          Re: [PATCH] usb: check for signals in chaoskey read function Keith Packard <keithp@keithp.com> - 2016-02-17 19:10 +0100
            Re: [PATCH] usb: check for signals in chaoskey read function Keith Packard <keithp@keithp.com> - 2016-02-17 19:30 +0100

#1334965 — [PATCH] usb: check for signals in chaoskey read function

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

[toc] | [next] | [standalone]

#1335140

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

[toc] | [prev] | [next] | [standalone]

#1335742

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1336510

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1336620

[Multipart message — attachments visible in raw view] — view raw

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:

[toc] | [prev] | [next] | [standalone]

#1336632

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web