[PATCH v2 0/3] usb: uas / scsi: fail inflight and queued commands on dead links

[PATCH v2 0/3] usb: uas / scsi: fail inflight and queued commands on dead links

[Sarthak Kukreti]

Unplugging a UAS device during system suspend after hub_wq is frozen
defers usb_disconnect() until hub_wq thaws. When sd_suspend_system()
issues SYNCHRONIZE_CACHE to the disconnected target:

1. If the HCD accepts the URBs, the status URB fails in uas_stat_cmplt()
   without completing the command, stranding SYNCHRONIZE_CACHE for the
   60s SCSI timeout and triggering the 25s DPM watchdog. Patch 1 tracks
   the active sense_urb per command and completes the owning command in
   uas_stat_cmplt() on status URB failure while unlinking any inflight
   data URBs.
2. If usb_submit_urb() fails with -ESHUTDOWN, -ENOENT, or -EHOSTUNREACH,
   uas_queuecommand_lck() requeues the command instead of failing it,
   and leaks unsubmitted URBs or orphans already-submitted URBs on
   partial submission failure. Patch 2 fails the command with
   DID_NO_CONNECT, frees unsubmitted URBs, and unlinks any submitted
   status or data URBs.
3. When the command completes with DID_NO_CONNECT,
   scsi_check_passthrough() matches SCMD_FAILURE_RESULT_ANY and retries
   it against the disconnected target. Patch 3 skips retries on
   DID_NO_CONNECT, DID_BAD_TARGET, and DID_TRANSPORT_FAILFAST.

Tested using a USB/IP (vhci_hcd) UAS target emulator that freezes
hub_wq, injects -EPROTO on the status URB during inflight
SYNCHRONIZE_CACHE and READ(10) commands, and verifies immediate
completion without DPM watchdog timeouts, along with the scsi_lib KUnit
test suite.

Changes in v2:
- Track sense_urb in uas_cmd_info and complete only the command owning
  the failed status URB in uas_stat_cmplt() instead of looping across
  all devinfo->cmnd[] slots and orphaning other commands' sense URBs.
- Set DID_NO_CONNECT only on -ENODEV and -ESHUTDOWN in uas_stat_cmplt()
  and uas_data_cmplt(), and DID_ERROR on other status/data URB errors.
- Clear pending submit and work flags and free unsubmitted URBs when
  completing commands on status URB or partial submission failure.
- Unlink already-submitted status and data URBs in
  uas_queuecommand_lck() when a later URB submission fails, and remove
  the unused out label.
- Add Fixes tags to all three patches.

v1:
  https://lore.kernel.org/r/20261113003536.3622755-1-sarthakkukreti@google.com

Sarthak Kukreti (3):
  usb: uas: complete command on status URB error
  usb: uas: fail command submission on fatal USB errors
  scsi: core: do not retry passthrough requests on disconnected targets

 drivers/scsi/scsi_lib.c      |   5 ++
 drivers/scsi/scsi_lib_test.c |   9 ++-
 drivers/usb/storage/uas.c    | 128 +++++++++++++++++++++++++++++------
 3 files changed, 122 insertions(+), 20 deletions(-)

-- 
2.56.0.611.g1030f0b940-goog

[PATCH v2 1/3] usb: uas: complete command on status URB error

[Sarthak Kukreti]

When a UAS device is unplugged while hub_wq is frozen during system
suspend, usb_disconnect() is deferred and sd_sync_cache() issues
SYNCHRONIZE_CACHE to the disconnected target. If the status URB fails in
uas_stat_cmplt(), uas_stat_cmplt() logs the error and frees the URB
without completing the command, leaving SYNCHRONIZE_CACHE pending until
the 60s SCSI timeout and triggering the 25s DPM watchdog.

Track each command's active sense_urb in uas_cmd_info. When a status URB
fails in uas_stat_cmplt() while not resetting, complete the command that
owns the failed status URB (with DID_NO_CONNECT on -ENODEV/-ESHUTDOWN
and DID_ERROR otherwise), free unsubmitted URBs, and unlink any inflight
data URBs outside devinfo->lock. Set DID_NO_CONNECT on -ENODEV/-ESHUTDOWN
and preserve non-DID_OK host bytes in uas_data_cmplt().

Fixes: 115bb1ffa54c ("USB: Add UAS driver")
Signed-off-by: Sarthak Kukreti <sarthakkukreti@google.com>
---
 drivers/usb/storage/uas.c | 76 +++++++++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 265162981269..904b4ce04fc6 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -71,6 +71,7 @@ struct uas_cmd_info {
 	struct urb *cmd_urb;
 	struct urb *data_in_urb;
 	struct urb *data_out_urb;
+	struct urb *sense_urb;
 };
 
 /* I hate forward declarations, but I actually have a loop */
@@ -232,14 +233,20 @@ static void uas_free_unsubmitted_urbs(struct scsi_cmnd *cmnd)
 
 	cmdinfo = scsi_cmd_priv(cmnd);
 
-	if (cmdinfo->state & SUBMIT_CMD_URB)
+	if (cmdinfo->state & SUBMIT_CMD_URB) {
 		usb_free_urb(cmdinfo->cmd_urb);
+		cmdinfo->cmd_urb = NULL;
+	}
 
 	/* data urbs may have never gotten their submit flag set */
-	if (!(cmdinfo->state & DATA_IN_URB_INFLIGHT))
+	if (!(cmdinfo->state & DATA_IN_URB_INFLIGHT)) {
 		usb_free_urb(cmdinfo->data_in_urb);
-	if (!(cmdinfo->state & DATA_OUT_URB_INFLIGHT))
+		cmdinfo->data_in_urb = NULL;
+	}
+	if (!(cmdinfo->state & DATA_OUT_URB_INFLIGHT)) {
 		usb_free_urb(cmdinfo->data_out_urb);
+		cmdinfo->data_out_urb = NULL;
+	}
 }
 
 static int uas_try_complete(struct scsi_cmnd *cmnd, const char *caller)
@@ -304,23 +311,58 @@ static void uas_stat_cmplt(struct urb *urb)
 	struct urb *data_out_urb = NULL;
 	struct scsi_cmnd *cmnd;
 	struct uas_cmd_info *cmdinfo;
+	struct uas_cmd_info *s_cmdinfo = NULL;
 	unsigned long flags;
 	unsigned int idx;
+	int s_idx;
 	int status = urb->status;
 	bool success;
 
+	spin_lock_irqsave(&devinfo->lock, flags);
+
+	for (s_idx = 0; s_idx < devinfo->qdepth; s_idx++) {
+		if (!devinfo->cmnd[s_idx])
+			continue;
+		cmdinfo = scsi_cmd_priv(devinfo->cmnd[s_idx]);
+		if (cmdinfo->sense_urb == urb) {
+			s_cmdinfo = cmdinfo;
+			s_cmdinfo->sense_urb = NULL;
+			break;
+		}
+	}
+
+	if (devinfo->resetting)
+		goto out;
+
 	if (status) {
 		if (status != -ENOENT && status != -ECONNRESET && status != -ESHUTDOWN)
 			dev_err(&urb->dev->dev, "stat urb: status %d\n", status);
-		goto bail;
+		if (!s_cmdinfo)
+			goto out;
+		cmnd = devinfo->cmnd[s_idx];
+		if (s_cmdinfo->state & DATA_IN_URB_INFLIGHT)
+			data_in_urb = usb_get_urb(s_cmdinfo->data_in_urb);
+		if (s_cmdinfo->state & DATA_OUT_URB_INFLIGHT)
+			data_out_urb = usb_get_urb(s_cmdinfo->data_out_urb);
+		uas_free_unsubmitted_urbs(cmnd);
+		s_cmdinfo->state &= ~(COMMAND_INFLIGHT |
+				      SUBMIT_STATUS_URB |
+				      ALLOC_DATA_IN_URB |
+				      SUBMIT_DATA_IN_URB |
+				      ALLOC_DATA_OUT_URB |
+				      SUBMIT_DATA_OUT_URB |
+				      ALLOC_CMD_URB |
+				      SUBMIT_CMD_URB |
+				      IS_IN_WORK_LIST);
+		if (status == -ENODEV || status == -ESHUTDOWN)
+			set_host_byte(cmnd, DID_NO_CONNECT);
+		else if (host_byte(cmnd->result) == DID_OK)
+			set_host_byte(cmnd, DID_ERROR);
+		uas_try_complete(cmnd, __func__);
+		goto out_unlink;
 	}
 
 	idx = be16_to_cpup(&iu->tag) - 1;
-
-	spin_lock_irqsave(&devinfo->lock, flags);
-
-	if (devinfo->resetting)
-		goto out;
 	if (idx >= MAX_CMNDS || !devinfo->cmnd[idx]) {
 		dev_err(&urb->dev->dev,
 			"stat urb: no pending cmd for uas-tag %d\n", idx + 1);
@@ -335,6 +377,10 @@ static void uas_stat_cmplt(struct urb *urb)
 		goto out;
 	}
 
+	if (s_cmdinfo)
+		s_cmdinfo->sense_urb = cmdinfo->sense_urb;
+	cmdinfo->sense_urb = NULL;
+
 	switch (iu->iu_id) {
 	case IU_ID_STATUS:
 		uas_sense(urb, cmnd);
@@ -375,6 +421,7 @@ static void uas_stat_cmplt(struct urb *urb)
 	default:
 		uas_log_cmd_state(cmnd, "bogus IU", iu->iu_id);
 	}
+out_unlink:
 	spin_unlock_irqrestore(&devinfo->lock, flags);
 	usb_free_urb(urb);
 
@@ -391,7 +438,6 @@ static void uas_stat_cmplt(struct urb *urb)
 
 out:
 	spin_unlock_irqrestore(&devinfo->lock, flags);
-bail:
 	usb_free_urb(urb);
 }
 
@@ -428,7 +474,10 @@ static void uas_data_cmplt(struct urb *urb)
 			uas_log_cmd_state(cmnd, "data cmplt err", status);
 		/* error: no data transfered */
 		scsi_set_resid(cmnd, sdb->length);
-		set_host_byte(cmnd, DID_ERROR);
+		if (status == -ENODEV || status == -ESHUTDOWN)
+			set_host_byte(cmnd, DID_NO_CONNECT);
+		else if (host_byte(cmnd->result) == DID_OK)
+			set_host_byte(cmnd, DID_ERROR);
 	} else {
 		scsi_set_resid(cmnd, sdb->length - urb->actual_length);
 	}
@@ -543,6 +592,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp,
 static int uas_submit_sense_urb(struct scsi_cmnd *cmnd, gfp_t gfp)
 {
 	struct uas_dev_info *devinfo = cmnd->device->hostdata;
+	struct uas_cmd_info *cmdinfo = scsi_cmd_priv(cmnd);
 	struct urb *urb;
 	int err;
 
@@ -555,8 +605,10 @@ static int uas_submit_sense_urb(struct scsi_cmnd *cmnd, gfp_t gfp)
 		usb_unanchor_urb(urb);
 		uas_log_cmd_state(cmnd, "sense submit err", err);
 		usb_free_urb(urb);
+		return err;
 	}
-	return err;
+	cmdinfo->sense_urb = urb;
+	return 0;
 }
 
 static int uas_submit_urbs(struct scsi_cmnd *cmnd,
-- 
2.56.0.611.g1030f0b940-goog

[PATCH v2 2/3] usb: uas: fail command submission on fatal USB errors

[Sarthak Kukreti]

uas_queuecommand_lck() only treats -ENODEV from uas_submit_urbs() as a
fatal submission error. When a link drops or an endpoint is disabled
while hub_wq is frozen, usb_submit_urb() can also fail with -ESHUTDOWN,
-ENOENT, or -EHOSTUNREACH, causing uas_queuecommand_lck() to return
SCSI_MLQUEUE_DEVICE_BUSY or queue retry work. In addition, if status or
data URB submission succeeds before a later URB submission fails,
uas_queuecommand_lck() either leaks unsubmitted URBs and orphans the
status URB, or leaves SUBMIT_CMD_URB set and COMMAND_INFLIGHT clear so
the command never completes.

Fail commands with DID_NO_CONNECT on -ESHUTDOWN, -ENOENT, and
-EHOSTUNREACH in uas_queuecommand_lck(), free unsubmitted URBs, and
unlink any already-submitted status or data URBs outside devinfo->lock.

Fixes: 26d56a9fcb20 ("usb: uas: fix urb unmapping issue when the uas device is remove during ongoing data transfer")
Signed-off-by: Sarthak Kukreti <sarthakkukreti@google.com>
---
 drivers/usb/storage/uas.c | 48 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 904b4ce04fc6..951d85b2e7ac 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -756,12 +756,49 @@ static enum scsi_qc_status uas_queuecommand_lck(struct scsi_cmnd *cmnd)
 	 * a command that has finished is a success for the purpose
 	 * of queueing, no matter how fatal the error
 	 */
-	if (err == -ENODEV) {
-		if (cmdinfo->state & (COMMAND_INFLIGHT | DATA_IN_URB_INFLIGHT |
-				DATA_OUT_URB_INFLIGHT))
-			goto out;
-
+	if (err == -ENODEV || err == -ESHUTDOWN || err == -ENOENT ||
+	    err == -EHOSTUNREACH) {
 		set_host_byte(cmnd, DID_NO_CONNECT);
+		uas_free_unsubmitted_urbs(cmnd);
+		if (cmdinfo->sense_urb ||
+		    (cmdinfo->state & (COMMAND_INFLIGHT |
+				       DATA_IN_URB_INFLIGHT |
+				       DATA_OUT_URB_INFLIGHT))) {
+			struct urb *sense_urb = usb_get_urb(cmdinfo->sense_urb);
+			struct urb *data_in_urb = NULL;
+			struct urb *data_out_urb = NULL;
+
+			if (cmdinfo->sense_urb)
+				cmdinfo->state |= COMMAND_INFLIGHT;
+			if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
+				data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
+			if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
+				data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
+			cmdinfo->state &= ~(SUBMIT_STATUS_URB |
+					    ALLOC_DATA_IN_URB |
+					    SUBMIT_DATA_IN_URB |
+					    ALLOC_DATA_OUT_URB |
+					    SUBMIT_DATA_OUT_URB |
+					    ALLOC_CMD_URB |
+					    SUBMIT_CMD_URB);
+			devinfo->cmnd[idx] = cmnd;
+			spin_unlock_irqrestore(&devinfo->lock, flags);
+
+			if (sense_urb) {
+				usb_unlink_urb(sense_urb);
+				usb_put_urb(sense_urb);
+			}
+			if (data_in_urb) {
+				usb_unlink_urb(data_in_urb);
+				usb_put_urb(data_in_urb);
+			}
+			if (data_out_urb) {
+				usb_unlink_urb(data_out_urb);
+				usb_put_urb(data_out_urb);
+			}
+			return 0;
+		}
+
 		scsi_done(cmnd);
 		goto zombie;
 	}
@@ -774,7 +811,6 @@ static enum scsi_qc_status uas_queuecommand_lck(struct scsi_cmnd *cmnd)
 		uas_add_work(cmnd);
 	}
 
-out:
 	devinfo->cmnd[idx] = cmnd;
 zombie:
 	spin_unlock_irqrestore(&devinfo->lock, flags);
-- 
2.56.0.611.g1030f0b940-goog

[PATCH v2 3/3] scsi: core: do not retry passthrough requests on disconnected targets

[Sarthak Kukreti]

Unlike scsi_decide_disposition(), scsi_check_passthrough() matches
SCMD_FAILURE_RESULT_ANY before checking the host byte and returns
-EAGAIN on DID_NO_CONNECT, DID_BAD_TARGET, and DID_TRANSPORT_FAILFAST.
Callers using SCMD_FAILURE_RESULT_ANY (such as sd_sync_cache())
therefore retry passthrough commands against disconnected targets.

Return 0 from scsi_check_passthrough() when the host byte is
DID_NO_CONNECT, DID_BAD_TARGET, or DID_TRANSPORT_FAILFAST, and update
scsi_lib_test_any_failure().

Fixes: 994724e6b3f0 ("scsi: core: Allow passthrough to request midlayer retries")
Signed-off-by: Sarthak Kukreti <sarthakkukreti@google.com>
---
 drivers/scsi/scsi_lib.c      | 5 +++++
 drivers/scsi/scsi_lib_test.c | 9 ++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index af27fd3df8d4..fa16a75e810c 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -219,6 +219,11 @@ static int scsi_check_passthrough(struct scsi_cmnd *scmd,
 	if (!scmd->result)
 		return 0;
 
+	if (host_byte(scmd->result) == DID_NO_CONNECT ||
+	    host_byte(scmd->result) == DID_BAD_TARGET ||
+	    host_byte(scmd->result) == DID_TRANSPORT_FAILFAST)
+		return 0;
+
 	if (!failures)
 		return 0;
 
diff --git a/drivers/scsi/scsi_lib_test.c b/drivers/scsi/scsi_lib_test.c
index ae8af0e0047a..aafafe676098 100644
--- a/drivers/scsi/scsi_lib_test.c
+++ b/drivers/scsi/scsi_lib_test.c
@@ -184,8 +184,15 @@ static void scsi_lib_test_any_failure(struct kunit *test)
 
 	/* Match SCMD_FAILURE_RESULT_ANY */
 	failures.failure_definitions = any_failure_defs;
-	sc.result = DID_TRANSPORT_FAILFAST << 16;
+	sc.result = DID_ERROR << 16;
 	KUNIT_EXPECT_EQ(test, -EAGAIN, scsi_check_passthrough(&sc, &failures));
+
+	sc.result = DID_NO_CONNECT << 16;
+	KUNIT_EXPECT_EQ(test, 0, scsi_check_passthrough(&sc, &failures));
+	sc.result = DID_BAD_TARGET << 16;
+	KUNIT_EXPECT_EQ(test, 0, scsi_check_passthrough(&sc, &failures));
+	sc.result = DID_TRANSPORT_FAILFAST << 16;
+	KUNIT_EXPECT_EQ(test, 0, scsi_check_passthrough(&sc, &failures));
 }
 
 static void scsi_lib_test_any_status(struct kunit *test)
-- 
2.56.0.611.g1030f0b940-goog