[PATCH v2] usb: core: Don't block while sleeping in do_proc_control()

[PATCH v2] usb: core: Don't block while sleeping in do_proc_control()

[Tasos Sahanidis]

Since commit ae8709b296d8 ("USB: core: Make do_proc_control() and
do_proc_bulk() killable") if a device has the USB_QUIRK_DELAY_CTRL_MSG
quirk set, it will temporarily block all other URBs (e.g. interrupts)
while sleeping due to a control.

This results in noticeable delays when, for example, a userspace usbfs
application is sending URB interrupts at a high rate to a keyboard and
simultaneously updates the lock indicators using controls. Interrupts
with direction set to IN are also affected by this, meaning that
delivery of HID reports (containing scancodes) to the usbfs application
is delayed as well.

This patch fixes the regression by calling msleep() while the device
mutex is unlocked, as was the case originally with usb_control_msg().

Fixes: ae8709b296d8 ("USB: core: Make do_proc_control() and do_proc_bulk() killable")
Signed-off-by: Tasos Sahanidis <tasos@tasossah.com>
---
 drivers/usb/core/devio.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 6abb7294e919..b5b85bf80329 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1209,12 +1209,16 @@ static int do_proc_control(struct usb_dev_state *ps,
 
 		usb_unlock_device(dev);
 		i = usbfs_start_wait_urb(urb, tmo, &actlen);
+
+		/* Linger a bit, prior to the next control message. */
+		if (dev->quirks & USB_QUIRK_DELAY_CTRL_MSG)
+			msleep(200);
 		usb_lock_device(dev);
 		snoop_urb(dev, NULL, pipe, actlen, i, COMPLETE, tbuf, actlen);
 		if (!i && actlen) {
 			if (copy_to_user(ctrl->data, tbuf, actlen)) {
 				ret = -EFAULT;
-				goto recv_fault;
+				goto done;
 			}
 		}
 	} else {
@@ -1231,6 +1235,10 @@ static int do_proc_control(struct usb_dev_state *ps,
 
 		usb_unlock_device(dev);
 		i = usbfs_start_wait_urb(urb, tmo, &actlen);
+
+		/* Linger a bit, prior to the next control message. */
+		if (dev->quirks & USB_QUIRK_DELAY_CTRL_MSG)
+			msleep(200);
 		usb_lock_device(dev);
 		snoop_urb(dev, NULL, pipe, actlen, i, COMPLETE, NULL, 0);
 	}
@@ -1242,10 +1250,6 @@ static int do_proc_control(struct usb_dev_state *ps,
 	}
 	ret = (i < 0 ? i : actlen);
 
- recv_fault:
-	/* Linger a bit, prior to the next control message. */
-	if (dev->quirks & USB_QUIRK_DELAY_CTRL_MSG)
-		msleep(200);
  done:
 	kfree(dr);
 	usb_free_urb(urb);
-- 
2.25.1

Re: [PATCH v2] usb: core: Don't block while sleeping in do_proc_control()

[Alan Stern]

On Sun, Mar 27, 2022 at 02:25:01AM +0200, Tasos Sahanidis wrote:
> Since commit ae8709b296d8 ("USB: core: Make do_proc_control() and
> do_proc_bulk() killable") if a device has the USB_QUIRK_DELAY_CTRL_MSG
> quirk set, it will temporarily block all other URBs (e.g. interrupts)
> while sleeping due to a control.
> 
> This results in noticeable delays when, for example, a userspace usbfs
> application is sending URB interrupts at a high rate to a keyboard and
> simultaneously updates the lock indicators using controls. Interrupts
> with direction set to IN are also affected by this, meaning that
> delivery of HID reports (containing scancodes) to the usbfs application
> is delayed as well.
> 
> This patch fixes the regression by calling msleep() while the device
> mutex is unlocked, as was the case originally with usb_control_msg().
> 
> Fixes: ae8709b296d8 ("USB: core: Make do_proc_control() and do_proc_bulk() killable")
> Signed-off-by: Tasos Sahanidis <tasos@tasossah.com>
> ---

Thanks for identifying and fixing this.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

You might want to change the patch name, however.  "Block" usually means 
pretty much the same thing as "sleep"; i.e., a process is blocked if it 
can't move forward.  The name should be something like "Don't hold the 
device lock while sleeping in do_proc_control()".

Alan Stern

Re: [PATCH v2] usb: core: Don't block while sleeping in do_proc_control()

[Tasos Sahanidis]

[-- Attachment #1.1: Type: text/plain, Size: 488 bytes --]

On 2022-03-27 18:08, Alan Stern wrote:
> You might want to change the patch name, however.  "Block" usually means 
> pretty much the same thing as "sleep"; i.e., a process is blocked if it 
> can't move forward.  The name should be something like "Don't hold the 
> device lock while sleeping in do_proc_control()".

The name does indeed need improvement, thank you for the explanation. I
was unable to come up with a better one, so I will submit v3 with the
name you suggested.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]