[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

[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