Re: [PATCH v3] usb: core: Don't hold the device lock while sleeping in do_proc_control()

[Alan Stern <stern@rowland.harvard.edu>]

On Fri, Apr 01, 2022 at 12:47:00AM +0300, 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>
> ---

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

> v2: Resent as v1 got mangled
> v3: Renamed from: "usb: core: Don't block while sleeping in do_proc_control()"
> 
>  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
>