[PATCH v2] usb: uas: wait for pending commands before unbinding

[PATCH v2] usb: uas: wait for pending commands before unbinding

[Jiayi Li]

USB core normally disables interface endpoints before calling
uas_disconnect(). This cancels pending URBs while the device may still
be processing the associated commands. Some storage bridges then stop
responding after a driver rebind and need a disconnect and reconnect
to recover.

Enable soft unbinding and block new SCSI requests before waiting for
pending commands to finish. This keeps the endpoints available until
the wait completes. Stop waiting when USB core marks the device as
not attached, and continue with the existing cleanup on timeout.

Keep requests blocked when leaving the preparation step, including on
timeout. Set resetting before unblocking the host so that queued
requests cannot submit new URBs during teardown.

Signed-off-by: Jiayi Li <lijiayi@kylinos.cn>

---
Changes in v2:
- Pair SCSI request blocking and unblocking in uas_disconnect().
- Use the address of the embedded host_lock in the current tree.
- Add the missing Signed-off-by tag.

v1: https://lore.kernel.org/linux-usb/20261022123401.319479-1-lijiayi@kylinos.cn/

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 8655edb..300ab63 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/blkdev.h>
+#include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/module.h>
@@ -1211,16 +1212,54 @@ static int uas_reset_resume(struct usb_interface *intf)
 	return 0;
 }
 
+static void uas_pre_disconnect(struct usb_interface *intf)
+{
+	struct Scsi_Host *shost = usb_get_intfdata(intf);
+	struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
+	unsigned long start_time;
+
+	start_time = jiffies;
+	for (;;) {
+		if (READ_ONCE(devinfo->udev->state) == USB_STATE_NOTATTACHED)
+			return;
+
+		flush_work(&devinfo->work);
+
+		if (!usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000))
+			break;
+
+		if (!usb_wait_anchor_empty_timeout(&devinfo->data_urbs, 500))
+			break;
+
+		if (time_after(jiffies, start_time + 5 * HZ))
+			break;
+		if (uas_cmnd_list_empty(devinfo))
+			return;
+
+		msleep(20);
+	}
+
+	shost_printk(KERN_ERR, shost, "%s: timed out\n", __func__);
+}
+
 static void uas_disconnect(struct usb_interface *intf)
 {
 	struct Scsi_Host *shost = usb_get_intfdata(intf);
 	struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
 	unsigned long flags;
 
+	spin_lock_irqsave(&shost->host_lock, flags);
+	scsi_block_requests(shost);
+	spin_unlock_irqrestore(&shost->host_lock, flags);
+
+	uas_pre_disconnect(intf);
+
 	spin_lock_irqsave(&devinfo->lock, flags);
 	devinfo->resetting = 1;
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 
+	scsi_unblock_requests(shost);
+
 	cancel_work_sync(&devinfo->work);
 	usb_kill_anchored_urbs(&devinfo->cmd_urbs);
 	usb_kill_anchored_urbs(&devinfo->sense_urbs);
@@ -1260,6 +1299,7 @@ static void uas_shutdown(struct usb_interface *intf)
 
 static struct usb_driver uas_driver = {
 	.name = "uas",
+	.soft_unbind = 1,
 	.probe = uas_probe,
 	.disconnect = uas_disconnect,
 	.pre_reset = uas_pre_reset,

Re: [PATCH v2] usb: uas: wait for pending commands before unbinding

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: uas: stalled wait and false timeout error in uas_pre_disconnect()
--

commit 4c54d6873e1e380dddee1542eeac75bb8256858a
Author: Jiayi Li <lijiayi@kylinos.cn>

usb: uas: wait for pending commands before unbinding

This commit enables soft_unbind on the uas driver and adds
uas_pre_disconnect() to block SCSI requests and wait for pending URBs and
commands to finish before unbinding the interface.

> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 8655edbd66b16..300ab637be70b 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c

[ ... ]

> @@ -1211,16 +1212,54 @@ static int uas_reset_resume(struct usb_interface *intf)
>  	return 0;
>  }
>  
> +static void uas_pre_disconnect(struct usb_interface *intf)
> +{
> +	struct Scsi_Host *shost = usb_get_intfdata(intf);
> +	struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
> +	unsigned long start_time;
> +
> +	start_time = jiffies;
> +	for (;;) {
> +		if (READ_ONCE(devinfo->udev->state) == USB_STATE_NOTATTACHED)
> +			return;
> +
> +		flush_work(&devinfo->work);
> +
> +		if (!usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000))
> +			break;
> +
> +		if (!usb_wait_anchor_empty_timeout(&devinfo->data_urbs, 500))
> +			break;
> +
> +		if (time_after(jiffies, start_time + 5 * HZ))
> +			break;
> +		if (uas_cmnd_list_empty(devinfo))
> +			return;
> +
> +		msleep(20);
> +	}
> +
> +	shost_printk(KERN_ERR, shost, "%s: timed out\n", __func__);
> +}

[Severity: Medium]
Can uas_pre_disconnect() stall for 5 seconds or emit a false timeout error in
the following cases?

First, in uas_pre_disconnect(), time_after(jiffies, start_time + 5 * HZ) is
checked before uas_cmnd_list_empty(devinfo). If the waits on sense_urbs and
data_urbs both succeed (for example, 4.7s waiting on sense_urbs followed by
0.4s waiting on data_urbs, or a scheduling delay), can total elapsed time
exceed 5 * HZ and cause the loop to break out and log a timeout error even
when all commands have completed and uas_cmnd_list_empty(devinfo) is true?

Second, because uas_driver sets soft_unbind = 1, usb_unbind_interface() holds
the interface lock (and callers such as usb_set_configuration() or
usb_reset_device() also hold the device lock) when calling
uas_disconnect() -> uas_pre_disconnect() without disabling interface
endpoints. If the device is physically unplugged while uas_pre_disconnect()
is sleeping in usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000),
usb_disconnect() sets udev->state = USB_STATE_NOTATTACHED and calls
usb_disable_device(), which blocks on the device or interface lock before
reaching usb_disable_device_endpoints():

drivers/usb/core/message.c:usb_disable_device() {
	...
			remove_intf_ep_devs(interface);
			device_del(&interface->dev);
		}
	...
	usb_disable_device_endpoints(dev, skip_ep0);
}

Since pending sense_urbs are not unlinked by USB core until uas_disconnect()
returns, won't usb_wait_anchor_empty_timeout() wait the full 5000 ms, break
out of the loop, and log a timeout error without rechecking whether
devinfo->udev->state became USB_STATE_NOTATTACHED?

Third, if a status URB completes with a non-zero status, uas_stat_cmplt()
frees the URB without clearing COMMAND_INFLIGHT or devinfo->cmnd[idx]:

drivers/usb/storage/uas.c:uas_stat_cmplt() {
	...
	if (status) {
		if (status != -ENOENT && status != -ECONNRESET && status != -ESHUTDOWN)
			dev_err(&urb->dev->dev, "stat urb: status %d\n", status);
		goto bail;
	}
	...
bail:
	usb_free_urb(urb);
}

Once sense_urbs and data_urbs are empty and devinfo->work is flushed, no
further status URB will complete for that command until uas_zap_pending() runs
after uas_pre_disconnect(). Wouldn't uas_cmnd_list_empty(devinfo) remain false
and cause uas_pre_disconnect() to spin in msleep(20) for the full 5 seconds
before logging a timeout error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023013931.388916-1-lijiayi@kylinos.cn?part=1

Re: [PATCH v2] usb: uas: wait for pending commands before unbinding

[Oliver Neukum]

Hi,

On 23.10.26 03:39, Jiayi Li wrote:
> USB core normally disables interface endpoints before calling
> uas_disconnect(). This cancels pending URBs while the device may still
> be processing the associated commands. Some storage bridges then stop
> responding after a driver rebind and need a disconnect and reconnect
> to recover.

I am afraid there are some issues left. Let me address them inline
to the code.

  
> +static void uas_pre_disconnect(struct usb_interface *intf)
> +{
> +	struct Scsi_Host *shost = usb_get_intfdata(intf);
> +	struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
> +	unsigned long start_time;
> +
> +	start_time = jiffies;
> +	for (;;) {
> +		if (READ_ONCE(devinfo->udev->state) == USB_STATE_NOTATTACHED)
> +			return;
> +
> +		flush_work(&devinfo->work);

This raises two questions:

1. If an error happens while disconnect is running, why do we want to schedule
work?
2. Why only this and not also scan_work?
> +		if (!usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000))
> +			break;

Some commands run longer
> +		if (!usb_wait_anchor_empty_timeout(&devinfo->data_urbs, 500))
> +			break;
> +
> +		if (time_after(jiffies, start_time + 5 * HZ))
> +			break;
> +		if (uas_cmnd_list_empty(devinfo))
> +			return;
> +
> +		msleep(20);
> +	}
> +
> +	shost_printk(KERN_ERR, shost, "%s: timed out\n", __func__);
> +}
> +
>   static void uas_disconnect(struct usb_interface *intf)
>   {
>   	struct Scsi_Host *shost = usb_get_intfdata(intf);
>   	struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
>   	unsigned long flags;
>   
> +	spin_lock_irqsave(&shost->host_lock, flags);
> +	scsi_block_requests(shost);
> +	spin_unlock_irqrestore(&shost->host_lock, flags);
> +
> +	uas_pre_disconnect(intf);
> +
>   	spin_lock_irqsave(&devinfo->lock, flags);
>   	devinfo->resetting = 1;
>   	spin_unlock_irqrestore(&devinfo->lock, flags);

Is the resetting flag even needed anymore?
If it is needed, why set it so late?

	Regards
		Oliver