[PATCH] nvme: simplify request ready check for pci

[PATCH] nvme: simplify request ready check for pci

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The criteria for pci transports' request ready check is simpler than
fabrics. Move the pci check to a different function. This also makes the
existing ready check simpler since it doesn't need to repeatedly test if
the controller is a fabrics type.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/apple.c |  2 +-
 drivers/nvme/host/core.c  | 35 ++++++++++++++++-------------------
 drivers/nvme/host/nvme.h  | 10 +++++++++-
 drivers/nvme/host/pci.c   |  2 +-
 4 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
index a060f69558e76..8009eba9dd205 100644
--- a/drivers/nvme/host/apple.c
+++ b/drivers/nvme/host/apple.c
@@ -750,7 +750,7 @@ static blk_status_t apple_nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	if (unlikely(!READ_ONCE(q->enabled)))
 		return BLK_STS_IOERR;
 
-	if (!nvme_check_ready(&anv->ctrl, req, true))
+	if (!nvme_pci_check_ready(&anv->ctrl, req))
 		return nvme_fail_nonready_command(&anv->ctrl, req);
 
 	ret = nvme_setup_cmd(ns, req);
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f028913e2e622..6007cd7fdc3f4 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -780,25 +780,22 @@ bool __nvme_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
 	if (rq->q == ctrl->admin_q && (req->flags & NVME_REQ_USERCMD))
 		return false;
 
-	if (ctrl->ops->flags & NVME_F_FABRICS) {
-		/*
-		 * Only allow commands on a live queue, except for the connect
-		 * command, which is require to set the queue live in the
-		 * appropinquate states.
-		 */
-		switch (state) {
-		case NVME_CTRL_CONNECTING:
-			if (blk_rq_is_passthrough(rq) && nvme_is_fabrics(req->cmd) &&
-			    (req->cmd->fabrics.fctype == nvme_fabrics_type_connect ||
-			     req->cmd->fabrics.fctype == nvme_fabrics_type_auth_send ||
-			     req->cmd->fabrics.fctype == nvme_fabrics_type_auth_receive))
-				return true;
-			break;
-		default:
-			break;
-		case NVME_CTRL_DEAD:
-			return false;
-		}
+	/*
+	 * Only allow commands on a live queue, except for the connect command,
+	 * which is require to set the queue live in the appropinquate states.
+	 */
+	switch (state) {
+	case NVME_CTRL_CONNECTING:
+		if (blk_rq_is_passthrough(rq) && nvme_is_fabrics(req->cmd) &&
+		    (req->cmd->fabrics.fctype == nvme_fabrics_type_connect ||
+		     req->cmd->fabrics.fctype == nvme_fabrics_type_auth_send ||
+		     req->cmd->fabrics.fctype == nvme_fabrics_type_auth_receive))
+			return true;
+		break;
+	default:
+		break;
+	case NVME_CTRL_DEAD:
+		return false;
 	}
 
 	return queue_live;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 7be92d07430e9..9f9e742c714da 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -849,11 +849,19 @@ static inline bool nvme_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
 
 	if (likely(state == NVME_CTRL_LIVE))
 		return true;
-	if (ctrl->ops->flags & NVME_F_FABRICS && state == NVME_CTRL_DELETING)
+	if (state == NVME_CTRL_DELETING)
 		return queue_live;
 	return __nvme_check_ready(ctrl, rq, queue_live, state);
 }
 
+static inline bool nvme_pci_check_ready(struct nvme_ctrl *ctrl,
+					struct request *rq)
+{
+	return likely(nvme_ctrl_state(ctrl) == NVME_CTRL_LIVE) ||
+		!(nvme_req(rq)->flags & NVME_REQ_USERCMD) ||
+		rq->q->disk;
+}
+
 /*
  * NSID shall be unique for all shared namespaces, or if at least one of the
  * following conditions is met:
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 710f3dfef3663..f60e04ec8a246 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -972,7 +972,7 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
 		return BLK_STS_IOERR;
 
-	if (unlikely(!nvme_check_ready(&dev->ctrl, req, true)))
+	if (unlikely(!nvme_pci_check_ready(&dev->ctrl, req)))
 		return nvme_fail_nonready_command(&dev->ctrl, req);
 
 	ret = nvme_prep_rq(dev, req);
-- 
2.47.1

Re: [PATCH] nvme: simplify request ready check for pci

[Christoph Hellwig]

On Fri, Mar 07, 2025 at 03:26:55PM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The criteria for pci transports' request ready check is simpler than
> fabrics. Move the pci check to a different function. This also makes the
> existing ready check simpler since it doesn't need to repeatedly test if
> the controller is a fabrics type.

The change looks good, but now that this is split, the PCI version
should move to pci.c, and the fabrics version should move to fabrics.c
and into the nvmf_* namespace.

Re: [PATCH] nvme: simplify request ready check for pci

[Keith Busch]

On Mon, Mar 10, 2025 at 02:24:48PM +0100, Christoph Hellwig wrote:
> On Fri, Mar 07, 2025 at 03:26:55PM -0800, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > The criteria for pci transports' request ready check is simpler than
> > fabrics. Move the pci check to a different function. This also makes the
> > existing ready check simpler since it doesn't need to repeatedly test if
> > the controller is a fabrics type.
> 
> The change looks good, but now that this is split, the PCI version
> should move to pci.c, and the fabrics version should move to fabrics.c
> and into the nvmf_* namespace.

I don't think we can move it to pci.c. The apple device isn't fabrics
either, but is built without the pci.c module, so needs to be in a
header or common core.

Re: [PATCH] nvme: simplify request ready check for pci

[Christoph Hellwig]

On Mon, Mar 10, 2025 at 09:01:55AM -0600, Keith Busch wrote:
> On Mon, Mar 10, 2025 at 02:24:48PM +0100, Christoph Hellwig wrote:
> > On Fri, Mar 07, 2025 at 03:26:55PM -0800, Keith Busch wrote:
> > > From: Keith Busch <kbusch@kernel.org>
> > > 
> > > The criteria for pci transports' request ready check is simpler than
> > > fabrics. Move the pci check to a different function. This also makes the
> > > existing ready check simpler since it doesn't need to repeatedly test if
> > > the controller is a fabrics type.
> > 
> > The change looks good, but now that this is split, the PCI version
> > should move to pci.c, and the fabrics version should move to fabrics.c
> > and into the nvmf_* namespace.
> 
> I don't think we can move it to pci.c. The apple device isn't fabrics
> either, but is built without the pci.c module, so needs to be in a
> header or common core.

Indeed, Apple messes this up a bit.  But that also means the _pci namespace
is wrong.  It's about all memory transport including the (unspecified)
apple one.  So either rename it to something reflecting that, or just
duplicate it in apple.c as we'd done with a few similar things.