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

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

[Jiayi Li]

USB core normally disables interface endpoints before calling
uas_disconnect(), cancelling URBs while the device may still be
processing the associated commands. Some storage bridges may then
fail to recover after a driver rebind and require a disconnect and
reconnect.

Enable soft unbinding and give already accepted commands a bounded
grace period to complete before teardown. The disconnect sequence is:

  -> cancel pending scan work and block new requests
  -> stop software retries for accepted commands
  -> wait for commands that can still complete normally
  -> set resetting and unblock requests
  -> cancel work and URBs, then clean up remaining commands
  -> synchronize scan work and remove the host

Commands needing a retry are left for the existing cleanup, while
other accepted commands can still complete normally. Stop waiting
when there are no commands left to wait for, USB core marks the device
as not attached, or the grace period expires.

This does not abort tasks already accepted by the device. If a command
cannot finish during the grace period, an affected bridge may still
require a disconnect and reconnect to recover.

Signed-off-by: Jiayi Li <lijiayi@kylinos.cn>
---
Changes in v3:
- Stop software retries during disconnect, including retries triggered
  by later READY IUs, and leave those commands for final cleanup.
- Replace flush_work() and anchor waits with command-table polling.
- Cancel pending scan work before blocking the host, while keeping
  synchronous cancellation after command cleanup.

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

 drivers/usb/storage/uas.c | 68 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 8655edbd66b168b34ff793509457e7f921028f17..63a0ac515737e4782bba50aa2bc2b54ee8282acd 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>
@@ -62,6 +63,7 @@ enum {
 	DATA_OUT_URB_INFLIGHT   = BIT(10),
 	COMMAND_ABORTED         = BIT(11),
 	IS_IN_WORK_LIST         = BIT(12),
+	COMMAND_NO_RETRY        = BIT(13),
 };
 
 /* Overrides scsi_pointer */
@@ -119,6 +121,9 @@ static void uas_do_work(struct work_struct *work)
 		if (!(cmdinfo->state & IS_IN_WORK_LIST))
 			continue;
 
+		if (cmdinfo->state & COMMAND_NO_RETRY)
+			continue;
+
 		err = uas_submit_urbs(cmnd, cmnd->device->hostdata);
 		if (!err)
 			cmdinfo->state &= ~IS_IN_WORK_LIST;
@@ -147,7 +152,8 @@ static void uas_add_work(struct scsi_cmnd *cmnd)
 
 	lockdep_assert_held(&devinfo->lock);
 	cmdinfo->state |= IS_IN_WORK_LIST;
-	queue_work(workqueue, &devinfo->work);
+	if (!(cmdinfo->state & COMMAND_NO_RETRY))
+		queue_work(workqueue, &devinfo->work);
 }
 
 static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
@@ -265,6 +271,10 @@ static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
 	struct uas_cmd_info *cmdinfo = scsi_cmd_priv(cmnd);
 	int err;
 
+	if ((cmdinfo->state & (COMMAND_NO_RETRY | IS_IN_WORK_LIST)) ==
+	    (COMMAND_NO_RETRY | IS_IN_WORK_LIST))
+		return;
+
 	cmdinfo->state |= direction | SUBMIT_STATUS_URB;
 	err = uas_submit_urbs(cmnd, cmnd->device->hostdata);
 	if (err) {
@@ -1211,16 +1221,71 @@ 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;
+	struct uas_cmd_info *cmdinfo;
+	unsigned long flags, start_time;
+	int i;
+
+	spin_lock_irqsave(&devinfo->lock, flags);
+	for (i = 0; i < devinfo->qdepth; i++) {
+		if (!devinfo->cmnd[i])
+			continue;
+		cmdinfo = scsi_cmd_priv(devinfo->cmnd[i]);
+		cmdinfo->state |= COMMAND_NO_RETRY;
+	}
+	spin_unlock_irqrestore(&devinfo->lock, flags);
+
+	start_time = jiffies;
+	for (;;) {
+		if (READ_ONCE(devinfo->udev->state) == USB_STATE_NOTATTACHED)
+			return;
+
+		spin_lock_irqsave(&devinfo->lock, flags);
+		for (i = 0; i < devinfo->qdepth; i++) {
+			if (!devinfo->cmnd[i])
+				continue;
+			cmdinfo = scsi_cmd_priv(devinfo->cmnd[i]);
+			if ((cmdinfo->state &
+			     (COMMAND_NO_RETRY | IS_IN_WORK_LIST)) !=
+			    (COMMAND_NO_RETRY | IS_IN_WORK_LIST))
+				break;
+		}
+		spin_unlock_irqrestore(&devinfo->lock, flags);
+
+		if (i >= devinfo->qdepth)
+			return;
+		if (time_after(jiffies, start_time + 5 * HZ))
+			break;
+
+		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;
 
+	cancel_work(&devinfo->scan_work);
+
+	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 +1325,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,

base-commit: f1ac44c32cba88ad65dcecd4a279661d14e93651
-- 
2.53.0.windows.1

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

[Oliver Neukum]

Hi,

On 27.10.26 14:14, Jiayi Li wrote:
> USB core normally disables interface endpoints before calling
> uas_disconnect(), cancelling URBs while the device may still be
> processing the associated commands. Some storage bridges may then
> fail to recover after a driver rebind and require a disconnect and
> reconnect.

[..]

I regret torturing you again, but there is a simple issue left.

>   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;
>   
> +	cancel_work(&devinfo->scan_work);

No _sync
> +	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);

With _sync

I'd say both ought to use cancel_work_sync()

	Regards
		Oliver