git: 0a0490ed551d - main - nvme: do not complete a command when its Abort is not performed

The branch main has been updated by seuros:

URL: https://cgit.FreeBSD.org/src/commit/?id=0a0490ed551d8c89aca16b6b2f1a3b3681b60972

commit 0a0490ed551d8c89aca16b6b2f1a3b3681b60972
Author:     Abdelkader Boudih <seuros@FreeBSD.org>
AuthorDate: 2026-10-04 17:40:52 +0000
Commit:     Abdelkader Boudih <seuros@FreeBSD.org>
CommitDate: 2026-10-04 17:40:52 +0000

    nvme: do not complete a command when its Abort is not performed
    
    An Abort completion with cdw0 bit 0 set means the controller did not
    abort the command; the command can still complete later. The driver
    treated this as aborted anyway: it completed the command itself and
    freed the CID. When the controller completed the command later,
    the CID may already belong to a new command, so the new command
    finished with the old command's status, or the unknown-cid assertion
    fired on INVARIANTS kernels. The watchdog also kept sending a new
    Abort for the same command every half second while the first one was
    still pending.
    
    Reviewed by:    imp, adrian
    Differential Revision:  https://reviews.freebsd.org/D59634
---
 sys/dev/nvme/nvme_private.h |  6 ++++++
 sys/dev/nvme/nvme_qpair.c   | 38 ++++++++++++++------------------------
 2 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/sys/dev/nvme/nvme_private.h b/sys/dev/nvme/nvme_private.h
index d801f8285f90..c7095bc34334 100644
--- a/sys/dev/nvme/nvme_private.h
+++ b/sys/dev/nvme/nvme_private.h
@@ -132,6 +132,11 @@ struct nvme_async_event_request {
 	uint8_t				log_page_buffer[NVME_MAX_AER_LOG_SIZE];
 };
 
+/* Progress of a hardware Abort for a timed-out tracker */
+#define	NVME_ABORT_NONE		0	/* No abort submitted */
+#define	NVME_ABORT_SENT		1	/* Abort submitted, not yet completed */
+#define	NVME_ABORT_FAILED	2	/* Abort did not abort the command */
+
 struct nvme_tracker {
 	TAILQ_ENTRY(nvme_tracker)	tailq;
 	struct nvme_request		*req;
@@ -139,6 +144,7 @@ struct nvme_tracker {
 	sbintime_t			deadline;
 	bus_dmamap_t			payload_dma_map;
 	uint16_t			cid;
+	uint8_t				abort_state;
 
 	uint64_t			*prp;
 	bus_addr_t			prp_bus_addr;
diff --git a/sys/dev/nvme/nvme_qpair.c b/sys/dev/nvme/nvme_qpair.c
index 6411a3909917..c918539ce28c 100644
--- a/sys/dev/nvme/nvme_qpair.c
+++ b/sys/dev/nvme/nvme_qpair.c
@@ -814,34 +814,18 @@ nvme_abort_complete(void *arg, const struct nvme_completion *status)
 	struct nvme_tracker     *tr = arg;
 
 	/*
-	 * If cdw0 bit 0 == 1, the controller was not able to abort the command
-	 * we requested.  We still need to check the active tracker array, to
-	 * cover race where I/O timed out at same time controller was completing
-	 * the I/O. An abort command always is on the admin queue, but affects
-	 * either an admin or an I/O queue, so take the appropriate qpair lock
-	 * for the original command's queue, since we'll need it to avoid races
-	 * with the completion code and to complete the command manually.
+	 * On IANP or Abort failure the command may still complete through
+	 * a deferred abort: the tracker stays live to keep its CID
+	 * reserved, and the timeout handler escalates to a reset.
 	 */
 	mtx_lock(&tr->qpair->lock);
-	if ((status->cdw0 & 1) == 1 && tr->qpair->act_tr[tr->cid] != NULL) {
-		/*
-		 * An I/O has timed out, and the controller was unable to abort
-		 * it for some reason.  And we've not processed a completion for
-		 * it yet. Construct a fake completion status, and then complete
-		 * the I/O's tracker manually.
-		 */
+	if ((nvme_completion_is_error(status) || (status->cdw0 & 1) == 1) &&
+	    tr->qpair->act_tr[tr->cid] == tr &&
+	    tr->abort_state == NVME_ABORT_SENT) {
 		nvme_printf(tr->qpair->ctrlr,
-		    "abort command failed, aborting command manually\n");
-		nvme_qpair_manual_complete_tracker(tr,
-		    NVME_SCT_GENERIC, NVME_SC_ABORTED_BY_REQUEST, 0, ERROR_PRINT_ALL);
+		    "abort command failed, resetting on next timeout pass\n");
+		tr->abort_state = NVME_ABORT_FAILED;
 	}
-	/*
-	 * XXX We don't check status for the possible 'Could not abort because
-	 * excess aborts were submitted to the controller'. We don't prevent
-	 * that, either. Document for the future here, since the standard is
-	 * squishy and only says 'may generate' but implies anything is possible
-	 * including hangs if you exceed the ACL.
-	 */
 	mtx_unlock(&tr->qpair->lock);
 }
 
@@ -992,6 +976,7 @@ do_reset:
 			 * Timeout expired, abort it or reset controller.
 			 */
 			if (ctrlr->enable_aborts &&
+			    tr->abort_state == NVME_ABORT_NONE &&
 			    tr->req->cb_fn != nvme_abort_complete) {
 				/*
 				 * This isn't an abort command, ask for a
@@ -999,9 +984,13 @@ do_reset:
 				 * queue which will reset the card if it
 				 * times out.
 				 */
+				tr->abort_state = NVME_ABORT_SENT;
 				nvme_ctrlr_cmd_abort(ctrlr,
 				    qpair->cid_base + tr->cid, qpair->id,
 				    nvme_abort_complete, tr);
+			} else if (ctrlr->enable_aborts &&
+			    tr->abort_state == NVME_ABORT_SENT) {
+				continue;
 			} else {
 				/*
 				 * We have a live command in the card (either
@@ -1177,6 +1166,7 @@ _nvme_qpair_submit_request(struct nvme_qpair *qpair, struct nvme_request *req)
 	TAILQ_REMOVE(&qpair->free_tr, tr, tailq);
 	TAILQ_INSERT_TAIL(&qpair->outstanding_tr, tr, tailq);
 	tr->deadline = SBT_MAX;
+	tr->abort_state = NVME_ABORT_NONE;
 	tr->req = req;
 	req->cmd.cid = qpair->cid_base + tr->cid;