Skip to contentSkip to conversation

nvme-fabrics: Add host FC transport support

PATCH Superseded James Smart 3 participants 5 messages Oct 7, 2016 – Oct 13, 2016

Subsystems (2)

v2 of 6 Superseded Latest is v6 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 5/7 nvme-fabrics: Add host FC transport support

Add nvme-fabrics host FC transport support

Implements the FC-NVME T11 definition of how nvme fabric capsules are performed on an FC fabric. Utilizes a lower-layer API to FC host adapters to send/receive FC-4 LS operations and FCP operations that comprise NVME over FC operation.

The T11 definitions for FC-4 Link Services are implemented which create NVMeOF connections. Implements the hooks with blk-mq to then submit admin and io requests to the different connections.

The changes made since the prior patch:
- Revise Kconfig for changes in select/depends
- Remove todo on Association LS: pull values from opts struct
- Slight restructure of nvme_fc_create_io_queues() and nvme_fc_init_io_queues() for their return values
- Checked and corrected BLK_MQ_xx return values in nvme_fc_start_fcp_op()
- Remove the todo's around dma_map_page(). No need to use dma_map_page()
- Reworked teardown paths with ref counting
- Teardown now invokes callbackes on completion of unregister functions
- set sqid in fcp request structs
- Add blk poll support.
- Remove map_queue from templates
- Removal of fabric name for nport qualifier. Not necessary in T11.
- LS's moved to controller linked list rather than remoteport
- Modifications from prior reviews:

-  Create inline xx_t_yy() wrappers for container_of's
-  fix nvme_fc_timeout() to use reinit_completion
-  fix nvme_fc_map_data() to declare dma dir to avoid line splits
-  fix nvme_fc_shutdown_ctlr() to remove previously done

nvme_stop_keep_alive()
- minor parens changes

Signed-off-by James Smart <james.smart at broadcom.com>
---
 drivers/nvme/host/Kconfig  |   16 +
 drivers/nvme/host/Makefile |    3 +
 drivers/nvme/host/fc.c     | 2304 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 2323 insertions(+)
 create mode 100644 drivers/nvme/host/fc.c
drivers/nvme/host/Kconfig +16 -0
index f7d37a6..aacac94 100644
@@ -43,3 +43,19 @@ config NVME_RDMA
 	  from https://github.com/linux-nvme/nvme-cli.
 
 	  If unsure, say N.
+
+config NVME_FC
+	tristate "NVM Express over Fabrics FC host driver"
+	depends on BLOCK
+	select NVME_CORE
+	select NVME_FABRICS
+	select SG_POOL
+	help
+	  This provides support for the NVMe over Fabrics protocol using
+	  the FC transport.  This allows you to use remote block devices
+	  exported using the NVMe protocol set.
+
+	  To configure a NVMe over Fabrics controller use the nvme-cli tool
+	  from https://github.com/linux-nvme/nvme-cli.
+
+	  If unsure, say N.
drivers/nvme/host/Makefile +3 -0
index 47abcec..f1a7d94 100644
@@ -2,6 +2,7 @@ obj-$(CONFIG_NVME_CORE)			+= nvme-core.o
 obj-$(CONFIG_BLK_DEV_NVME)		+= nvme.o
 obj-$(CONFIG_NVME_FABRICS)		+= nvme-fabrics.o
 obj-$(CONFIG_NVME_RDMA)			+= nvme-rdma.o
+obj-$(CONFIG_NVME_FC)			+= nvme-fc.o
 
 nvme-core-y				:= core.o
 nvme-core-$(CONFIG_BLK_DEV_NVME_SCSI)	+= scsi.o
@@ -12,3 +13,5 @@ nvme-y					+= pci.o
 nvme-fabrics-y				+= fabrics.o
 
 nvme-rdma-y				+= rdma.o
+
+nvme-fc-y				+= fc.o
drivers/nvme/host/fc.c new file +2304 -0

2,304 lines not on this page · raw message

Signature · 1 line
2.5.0
#1 lore raw

PATCH v2 5/7 nvme-fabrics: Add host FC transport support

drivers/nvme/host/Kconfig config NVME_RDMA · line 59

Quoted 2 lines
drivers/nvme/host/Kconfig +2 -0
+	  To configure a NVMe over Fabrics controller use the nvme-cli tool
+	  from https://github.com/linux-nvme/nvme-cli.

Talking about nvme-cli, can you post the FC support patches for that as well?

drivers/nvme/host/fc.c line 41

Quoted 6 lines
drivers/nvme/host/fc.c new file +6 -0
+#define NVME_FC_NR_AEN_COMMANDS	1
+#define NVME_FC_AQ_BLKMQ_DEPTH	\
+	(NVMF_AQ_DEPTH - NVME_FC_NR_AEN_COMMANDS)
+#define AEN_CMDID_BASE		(NVME_FC_AQ_BLKMQ_DEPTH + 1)
+#define IS_AEN_COMMAND(command_id) \
+	((command_id) >= AEN_CMDID_BASE)

This should be an inline. But given that it's only used once I'd actually prefer to just kill the helper and opencode it like in the other NVMe drivers.

drivers/nvme/host/fc.c line 127

Quoted 6 lines
drivers/nvme/host/fc.c new file +6 -0
+enum nvme_fcctrl_state {
+        FCCTRL_INIT             = 0,
+	FCCTRL_ACTIVE           = 1,
+	FCCTRL_DELETING         = 2,
+	FCCTRL_DELETED          = 3,
+};

Please integrate the FC controller state with the generic controller state in core.c

drivers/nvme/host/fc.c line 198

Quoted 3 lines
drivers/nvme/host/fc.c new file +3 -0
+static LIST_HEAD(nvme_fc_lport_list);
+static u32 nvme_fc_local_port_cnt;
+static u32 nvme_fc_ctrl_cnt;

Any reason not to use the ida allocator for the port number? a 32-bit integer can actually overflow fairly easily, so having a proper allocator and reusing ids instead of wrapping around would seem a lot safer.

drivers/nvme/host/fc.c line 341

Quoted 11 lines
drivers/nvme/host/fc.c new file +11 -0
+{
+	struct nvme_fc_lport *lport = localport_to_lport(portptr);
+	unsigned long flags;
+	u32 lnum;
+
+	if (!portptr)
+		return -EINVAL;
+
+	lnum = portptr->port_num;
+
+	spin_lock_irqsave(&nvme_fc_lock, flags);

Just curious: where and why do you call into the NVMe code from IRQ conect?

drivers/nvme/host/fc.c line 997

Quoted 7 lines
drivers/nvme/host/fc.c new file +7 -0
+	/*
+	 * If successful and ERSP, use the returned CQE
+	 *
+	 * Otherwise, there isn't a CQE or it may not have valid content.
+	 * FC-NVME will need to fudge one up. We also need to fudge up
+	 * CQE's for LLDD/transport errors.
+	 */

So the big question here is why? So far the model we use in the core NVMe code is that as long as the lower layer returns a negative Linux errno the CQE isn't valid, and only if we return a valid NVMe status we need a CQE. I suspect if you followed that convention you could avoid a lot of these synthetic CQE games.

drivers/nvme/host/fc.c line 1026

Quoted 5 lines
drivers/nvme/host/fc.c new file +5 -0
+			/* passed validation, use the cqe */
+			/* TODO: fix sqhd - deal with out of order */
+			queue->sqhd = le16_to_cpu(cqe->sq_head);
+			queue->seqno = be32_to_cpu(op->rsp_iu.rsn);
+			goto validation_done;

We don't really care about the sq_head at all in Linux, so I don't think we need to bother with maintaining the SQ head.

Quoted 7 lines
drivers/nvme/host/fc.c new file +7 -0
+static int
+__nvme_fc_abort_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_fcp_op *op)
+{
+	int state;
+
+	state = atomic_xchg(&op->state, FCPOP_STATE_ABORTED);
+	if (state != FCPOP_STATE_ACTIVE) {

drivers/nvme/host/fc.c line 1420

Quoted 10 lines
drivers/nvme/host/fc.c new file +10 -0
+		atomic_set(&op->state, state);
+		return 1; /* fail */
+	}
+
+	ctrl->lport->ops->fcp_abort(&ctrl->lport->localport,
+					&ctrl->rport->remoteport,
+					op->queue->lldd_handle,
+					&op->fcp_req);
+
+	return 0;

Not complaining about the code, but we really need to figure out how to fit transport aborts into the NVMe architecture. We can take this offline or to the NVMe working group, though.

Quoted 9 lines
drivers/nvme/host/fc.c new file +9 -0
+enum blk_eh_timer_return
+nvme_fc_timeout(struct request *rq, bool reserved)
+{
+	struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(rq);
+	struct nvme_fc_ctrl *ctrl = op->ctrl;
+	int ret;
+
+	if (reserved)
+		return BLK_EH_RESET_TIMER;

drivers/nvme/host/fc.c line 1441

Quoted 10 lines
drivers/nvme/host/fc.c new file +10 -0
+
+	ret = __nvme_fc_abort_op(ctrl, op);
+	if (ret)
+		/* io wasn't active to abort consider it done */
+		return BLK_EH_HANDLED;
+
+	/* fail with DNR on cmd timeout */
+	rq->errors = NVME_SC_ABORT_REQ | NVME_SC_DNR;
+
+	wait_for_completion(&op->abort_done);

The blk timeout handle isn't really supposed to be blocking. The idea is to perform the EH action and then complete things in the completion handler of the abort command.

drivers/nvme/host/fc.c line 2251

Quoted 5 lines
drivers/nvme/host/fc.c new file +5 -0
+	/* find the host and remote ports to connect together */
+	spin_lock_irqsave(&nvme_fc_lock, flags);
+	list_for_each_entry(lport, &nvme_fc_lport_list, port_list) {
+		if ((lport->localport.node_name != laddr.nn) ||
+		    (lport->localport.port_name != laddr.pn))

Nitpick: no need for the inner braces.

drivers/nvme/host/fc.c line 2256

Quoted 2 lines
drivers/nvme/host/fc.c new file +2 -0
+			if ((rport->remoteport.node_name != raddr.nn) ||
+			    (rport->remoteport.port_name != raddr.pn))

Same here.

drivers/nvme/host/fc.c line 2259

Quoted 3 lines
drivers/nvme/host/fc.c new file +3 -0
+				continue;
+
+			nvme_fc_rport_get(rport);

nvme_fc_rport_get is a kref_get_unless_zero, so you need to check the return value.

drivers/nvme/host/fc.c line 2293

Quoted 5 lines
drivers/nvme/host/fc.c new file +5 -0
+static void __exit nvme_fc_exit_module(void)
+{
+	/* sanity check - all lports should be removed */
+	if (!list_empty(&nvme_fc_lport_list))
+		pr_warn("%s: localport list not empty\n", __func__);

Nitpick: use WARN_ON?

#2 lore raw

PATCH v2 5/7 nvme-fabrics: Add host FC transport support

Quoted 7 lines · Christoph Hellwig

Review

Quoted 4 lines
(quoted diff) +1 -0
+	??To configure a NVMe over Fabrics controller use the

nvme-cli tool

(quoted diff) +1 -0
+	??from https://github.com/linux-nvme/nvme-cli.

Talking about nvme-cli, can you post the FC support patches for that as well?

Yes, this would be good, esp since Christoph and I have been writing man pages and mention "fc" in one of the parameters.

(In fact, providing an fc example in the man pages would go a long way to enabling fc people using nvme-cli).

I'll try and give your series a look after this patchset is resubmitted with Christoph's feedback.

J

#3 lore raw

PATCH v2 5/7 nvme-fabrics: Add host FC transport support

In reply to Christoph Hellwig
Quoted 4 lines · Christoph Hellwig

drivers/nvme/host/Kconfig config NVME_RDMA · line 59

Quoted 2 lines
drivers/nvme/host/Kconfig +2 -0
+	  To configure a NVMe over Fabrics controller use the nvme-cli tool
+	  from https://github.com/linux-nvme/nvme-cli.

Talking about nvme-cli, can you post the FC support patches for that as well?

Yes - getting to it. I also need to ensure that we have the udev hooks to force auto-connections.

Quoted 10 lines

drivers/nvme/host/fc.c line 41

Quoted 7 lines
drivers/nvme/host/fc.c new file +6 -0
+#define NVME_FC_NR_AEN_COMMANDS	1
+#define NVME_FC_AQ_BLKMQ_DEPTH	\
+	(NVMF_AQ_DEPTH - NVME_FC_NR_AEN_COMMANDS)
+#define AEN_CMDID_BASE		(NVME_FC_AQ_BLKMQ_DEPTH + 1)
+#define IS_AEN_COMMAND(command_id) \
+	((command_id) >= AEN_CMDID_BASE)

This should be an inline. But given that it's only used once I'd actually prefer to just kill the helper and opencode it like in the other NVMe drivers.

ok

Quoted 9 lines

drivers/nvme/host/fc.c line 127

Quoted 7 lines
drivers/nvme/host/fc.c new file +6 -0
+enum nvme_fcctrl_state {
+        FCCTRL_INIT             = 0,
+	FCCTRL_ACTIVE           = 1,
+	FCCTRL_DELETING         = 2,
+	FCCTRL_DELETED          = 3,
+};

Please integrate the FC controller state with the generic controller state in core.c

I'm not sure about this. I believe there are things about the fc nvme ctlr struct, which has to wait on interactions from the FC LLDD before it cleans things up - that are not part of the nvme ctrl state. Thus the distinction between DELETING (e.g. stopping, waiting for nvme to call us back after it's last put) vs DELETED (we've received NVME's callback, so can now act on our own ref counts).

I will look at it again.

I've noticed RDMA and loop cut a lot of corners on teardown of commands, etc - as they are mostly memory references with very quick hw context teardown that isn't necessarily the case with FC where we have real hw contexts in the LLDD and perhaps line traffic. I would assume most of the references in the LLDD come back to the blk request thus nvme ctrl, so your assumption that they are close in state to each other should be true, but I need to review the paths again.

Quoted 9 lines

drivers/nvme/host/fc.c line 198

Quoted 5 lines
drivers/nvme/host/fc.c new file +3 -0
+static LIST_HEAD(nvme_fc_lport_list);
+static u32 nvme_fc_local_port_cnt;
+static u32 nvme_fc_ctrl_cnt;

Any reason not to use the ida allocator for the port number? a 32-bit integer can actually overflow fairly easily, so having a proper allocator and reusing ids instead of wrapping around would seem a lot safer.

No reason - just that it didn't seem necessary. I'll look at ida.

Quoted 14 lines

drivers/nvme/host/fc.c line 341

Quoted 12 lines
drivers/nvme/host/fc.c new file +11 -0
+{
+	struct nvme_fc_lport *lport = localport_to_lport(portptr);
+	unsigned long flags;
+	u32 lnum;
+
+	if (!portptr)
+		return -EINVAL;
+
+	lnum = portptr->port_num;
+
+	spin_lock_irqsave(&nvme_fc_lock, flags);

Just curious: where and why do you call into the NVMe code from IRQ conect?

Well, its not specified in the api. It's up to the context of the LLD when it calls the nvmefc_fcp_req->done routine, which is nvme_fc_fcpio_done() routine, which you're looking at here. The routine does make the blk_mq_complete_request() call without changing context. So whether it's true IRQ or not - depends on the driver and whether it calls from the real IRQ or from a DPC-like thread. Any recommendations ?

Quoted 13 lines

drivers/nvme/host/fc.c line 997

Quoted 8 lines
drivers/nvme/host/fc.c new file +7 -0
+	/*
+	 * If successful and ERSP, use the returned CQE
+	 *
+	 * Otherwise, there isn't a CQE or it may not have valid content.
+	 * FC-NVME will need to fudge one up. We also need to fudge up
+	 * CQE's for LLDD/transport errors.
+	 */

So the big question here is why? So far the model we use in the core NVMe code is that as long as the lower layer returns a negative Linux errno the CQE isn't valid, and only if we return a valid NVMe status we need a CQE. I suspect if you followed that convention you could avoid a lot of these synthetic CQE games.

FC-NVME uses existing hw definitions for FCP RSP's which allow a short all zeros's response frame to be passed on the wire for good completions (lots of specific rules in order to do so). As such we don't have CQE content, so the transport has to create it (SQ ID, SQHD, Command Id, and status=0). So we can't avoid this fudging. NVME fabrics group was well aware of this.

As for negative responses from LLDD/transport errors - true, although the fudging is minimal. At best there's 1 synthetic CQE game to be avoided.

Quoted 8 lines

drivers/nvme/host/fc.c line 1026

Quoted 6 lines
drivers/nvme/host/fc.c new file +5 -0
+			/* passed validation, use the cqe */
+			/* TODO: fix sqhd - deal with out of order */
+			queue->sqhd = le16_to_cpu(cqe->sq_head);
+			queue->seqno = be32_to_cpu(op->rsp_iu.rsn);
+			goto validation_done;

We don't really care about the sq_head at all in Linux, so I don't think we need to bother with maintaining the SQ head.

I know. It doesn't care as it always sizes the SQs to the maximum number of outstanding commands possible (io request q tags count). So it doesn't care about sqhd pacing vs posting cmds to any queue. But I had heard that there was potentially a desire to change that. I would hate to have the nvme layer change and not know the transport had to as well.

FC-NVME also has a requirement to deliver ERSP's in order so that SQHD goes base in "order". So it's goodness as that linux doesn't care as I can ignore this requirement. But, I will at least want a comment in the code to state why that functionality isn't present.

Quoted 13 lines
Quoted 13 lines
drivers/nvme/host/fc.c new file +12 -0
+static int
+__nvme_fc_abort_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_fcp_op *op)
+{
+	int state;
+
+	state = atomic_xchg(&op->state, FCPOP_STATE_ABORTED);
+	if (state != FCPOP_STATE_ACTIVE) {
+		atomic_set(&op->state, state);
+		return 1; /* fail */
+	}
+
+	ctrl->lport->ops->fcp_abort(&ctrl->lport->localport,
Quoted 8 lines

drivers/nvme/host/fc.c line 1420

Quoted 5 lines
drivers/nvme/host/fc.c new file +5 -0
+					&ctrl->rport->remoteport,
+					op->queue->lldd_handle,
+					&op->fcp_req);
+
+	return 0;

Not complaining about the code, but we really need to figure out how to fit transport aborts into the NVMe architecture. We can take this offline or to the NVMe working group, though.

Agree. Its a tricky subject as simply changing completion status isn't great as the target side may not be aware of that change.

Quoted 15 lines
Quoted 15 lines
drivers/nvme/host/fc.c new file +14 -0
+enum blk_eh_timer_return
+nvme_fc_timeout(struct request *rq, bool reserved)
+{
+	struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(rq);
+	struct nvme_fc_ctrl *ctrl = op->ctrl;
+	int ret;
+
+	if (reserved)
+		return BLK_EH_RESET_TIMER;
+
+	ret = __nvme_fc_abort_op(ctrl, op);
+	if (ret)
+		/* io wasn't active to abort consider it done */
+		return BLK_EH_HANDLED;
Quoted 8 lines

drivers/nvme/host/fc.c line 1441

Quoted 5 lines
drivers/nvme/host/fc.c new file +5 -0
+
+	/* fail with DNR on cmd timeout */
+	rq->errors = NVME_SC_ABORT_REQ | NVME_SC_DNR;
+
+	wait_for_completion(&op->abort_done);

The blk timeout handle isn't really supposed to be blocking. The idea is to perform the EH action and then complete things in the completion handler of the abort command.

Ok - will address.

Quoted 12 lines

drivers/nvme/host/fc.c line 2251

Quoted 6 lines
drivers/nvme/host/fc.c new file +5 -0
+	/* find the host and remote ports to connect together */
+	spin_lock_irqsave(&nvme_fc_lock, flags);
+	list_for_each_entry(lport, &nvme_fc_lport_list, port_list) {
+		if ((lport->localport.node_name != laddr.nn) ||
+		    (lport->localport.port_name != laddr.pn))

Nitpick: no need for the inner braces.

drivers/nvme/host/fc.c line 2256

Quoted 2 lines
drivers/nvme/host/fc.c new file +2 -0
+			if ((rport->remoteport.node_name != raddr.nn) ||
+			    (rport->remoteport.port_name != raddr.pn))

Same here.

Quoted 5 lines

drivers/nvme/host/fc.c line 2259

Quoted 3 lines
drivers/nvme/host/fc.c new file +3 -0
+				continue;
+
+			nvme_fc_rport_get(rport);

nvme_fc_rport_get is a kref_get_unless_zero, so you need to check the return value.

ok

Quoted 7 lines

drivers/nvme/host/fc.c line 2293

Quoted 6 lines
drivers/nvme/host/fc.c new file +5 -0
+static void __exit nvme_fc_exit_module(void)
+{
+	/* sanity check - all lports should be removed */
+	if (!list_empty(&nvme_fc_lport_list))
+		pr_warn("%s: localport list not empty\n", __func__);

Nitpick: use WARN_ON?

sure.

-- james

#4 lore raw

PATCH v2 5/7 nvme-fabrics: Add host FC transport support

Quoted 9 lines · James Smart
Quoted 8 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+enum nvme_fcctrl_state {
+        FCCTRL_INIT             = 0,
+	FCCTRL_ACTIVE           = 1,
+	FCCTRL_DELETING         = 2,
+	FCCTRL_DELETED          = 3,
+};

Please integrate the FC controller state with the generic controller state in core.c

Quoted 6 lines

I'm not sure about this. I believe there are things about the fc nvme ctlr struct, which has to wait on interactions from the FC LLDD before it cleans things up - that are not part of the nvme ctrl state. Thus the distinction between DELETING (e.g. stopping, waiting for nvme to call us back after it's last put) vs DELETED (we've received NVME's callback, so can now act on our own ref counts).

I would much prefer to just have an additional state in the common state machine, with comments on the specific, e.g. NVME_CTRL_RECONNECTING is not used by PCIe. That being said the state machine could use some more documentation, I'll look into it.

Quoted 7 lines

I've noticed RDMA and loop cut a lot of corners on teardown of commands, etc
- as they are mostly memory references with very quick hw context teardown that isn't necessarily the case with FC where we have real hw contexts in the LLDD and perhaps line traffic. I would assume most of the references in the LLDD come back to the blk request thus nvme ctrl, so your assumption that they are close in state to each other should be true, but I need to review the paths again.

I'd be happy to get a review of the RDMA code in that respect. Note that PCIe shouldn't be all that different from FC in this respect.

Quoted 6 lines

Well, its not specified in the api. It's up to the context of the LLD when it calls the nvmefc_fcp_req->done routine, which is nvme_fc_fcpio_done() routine, which you're looking at here. The routine does make the blk_mq_complete_request() call without changing context. So whether it's true IRQ or not - depends on the driver and whether it calls from the real IRQ or from a DPC-like thread. Any recommendations ?

But nvme_fc_fcpio_done doesn't actually use any locks, that's why I'm wondering. I think it's perfectly fine to call the fast path completion handler from irq context, but I'd try to avoid dragging irq safe locks into the rest of the code.

Quoted 5 lines

FC-NVME uses existing hw definitions for FCP RSP's which allow a short all zeros's response frame to be passed on the wire for good completions (lots of specific rules in order to do so). As such we don't have CQE content, so the transport has to create it (SQ ID, SQHD, Command Id, and status=0). So we can't avoid this fudging. NVME fabrics group was well aware of this.

I'm fine with that concept. I think you're doing way to much work in the implementation, though..

Unless it's a REQ_TYPE_DRV_PRIV request, and rq->special points to the CQE there is no point in faking up a CQE at all, as no one is going to look at it ever. This is especially true for the fast path case you mention above. See the untested incremental patch at the end of the mail on how I'd handle this.

Quoted 4 lines

FC-NVME also has a requirement to deliver ERSP's in order so that SQHD goes base in "order". So it's goodness as that linux doesn't care as I can ignore this requirement. But, I will at least want a comment in the code to state why that functionality isn't present.

Sure, comments are always good..

drivers/nvme/host/fc.c +50 -58
index 35ea164..67984572 100644
@@ -974,7 +974,7 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
 	struct nvme_fc_ctrl *ctrl = op->ctrl;
 	struct nvme_fc_queue *queue = op->queue;
 	struct nvme_completion *cqe = &op->rsp_iu.cqe;
-	int status, llddstatus = freq->status;
+	int status;
 
 	dma_sync_single_for_cpu(ctrl->lport->dev, op->fcp_req.rspdma,
 				sizeof(op->rsp_iu), DMA_FROM_DEVICE);
@@ -983,73 +983,65 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
 	 * if we aborted the op, the routines that invoked the abort
 	 * set the desired status on the request (namely DNR bits as
 	 * well as NVME_SC_ABORT_REQ). Thus, use those values for
-	 * the llddstatus which gets put into cQEs.
+	 * the status which gets put into CQEs.
 	 */
 	if (atomic_read(&op->state) == FCPOP_STATE_ABORTED)
-		llddstatus = rq->errors;
-
-	/*
-	 * If successful and ERSP, use the returned CQE
-	 *
-	 * Otherwise, there isn't a CQE or it may not have valid content.
-	 * FC-NVME will need to fudge one up. We also need to fudge up
-	 * CQE's for LLDD/transport errors.
-	 */
-
-	/*
-	 * if length of the received response is 0 or 12 and llddstatus is 0,
-	 * then a successful response is assumed.  But, need to create a CQE.
-	 */
-	if (!llddstatus && (!freq->rcv_rsplen ||
-			  (freq->rcv_rsplen == NVME_FC_SIZEOF_ZEROS_RSP))) {
-
-		memset(cqe, 0, sizeof(*cqe));
-		cqe->sq_head = cpu_to_le16(queue->sqhd);
-		cqe->command_id = cpu_to_le16(op->rqno);
-		goto validation_done;
-	}
-
-	/* successful ersp */
-	if ((!llddstatus) &&
-			(freq->rcv_rsplen == sizeof(struct nvme_fc_ersp_iu))) {
-		/* validate it */
+		status = rq->errors;
+	else
+		status = freq->status;
+
+	if (status)
+		goto done;
+
+	switch (freq->rcv_rsplen) {
+	case 0:
+	case NVME_FC_SIZEOF_ZEROS_RSP:
+		/* fasth path for successful operations without result */
+		if (unlikely(rq->cmd_type == REQ_TYPE_DRV_PRIV &&
+				rq->special)) {
+			struct nvme_completion *cqe = rq->special;
+
+			memset(cqe, 0, sizeof(*cqe));
+			cqe->sq_head = cpu_to_le16(queue->sqhd);
+			cqe->command_id = cpu_to_le16(op->rqno);
+			cqe->status = cpu_to_le16(status << 1);
+		}
+		break;
+	case sizeof(struct nvme_fc_ersp_iu):
+		/* successful ersp */
 		if (unlikely(be16_to_cpu(op->rsp_iu.iu_len) !=
-				(freq->rcv_rsplen/4)))
-			llddstatus = NVME_SC_FC_FORMAT;
-		else if (unlikely(op->rqno != le16_to_cpu(cqe->command_id)))
-			llddstatus = NVME_SC_FC_CMDID_MISMATCH;
-		else {
-			/* passed validation, use the cqe */
-			/* TODO: fix sqhd - deal with out of order */
-			queue->sqhd = le16_to_cpu(cqe->sq_head);
-			queue->seqno = be32_to_cpu(op->rsp_iu.rsn);
-			goto validation_done;
+				freq->rcv_rsplen / 4)) {
+			status = -EIO;
+			goto done;
 		}
-		/* if error - will fall thru below */
 
-	/* if a bad length */
-	} else if (!llddstatus)
-		llddstatus = NVME_SC_FC_FORMAT;
+		if (unlikely(op->rqno != le16_to_cpu(cqe->command_id))) {
+			status = -EIO;
+			break;
+		}
 
-	/* we have validation errors or a lldd/transport error */
-	memset(cqe, 0, sizeof(*cqe));
-	cqe->sq_head = cpu_to_le16(queue->sqhd);
-	cqe->command_id = cpu_to_le16(op->rqno);
-	cqe->status = cpu_to_le16(llddstatus << 1);
+		/* passed validation, use the cqe */
+		/* TODO: fix sqhd - deal with out of order */
+		queue->sqhd = le16_to_cpu(cqe->sq_head);
+		queue->seqno = be32_to_cpu(op->rsp_iu.rsn);
+	
+		if (!queue->qnum && IS_AEN_COMMAND(cqe->command_id)) {
+			nvme_complete_async_event(&queue->ctrl->ctrl, cqe);
+			return;
+		}
 
-validation_done:
+		if (rq->cmd_type == REQ_TYPE_DRV_PRIV && rq->special)
+			memcpy(rq->special, cqe, sizeof(*cqe));
 
-	if (!queue->qnum && IS_AEN_COMMAND(cqe->command_id)) {
-		nvme_complete_async_event(&queue->ctrl->ctrl, cqe);
-		return;
+		status = le16_to_cpu(cqe->status) >> 1;
+		break;
+	default:
+		/* invalid size */
+		status = -EIO;
+		break;
 	}
 
-	status = le16_to_cpu(cqe->status);
-	status >>= 1;
-
-	if (rq->cmd_type == REQ_TYPE_DRV_PRIV && rq->special)
-		memcpy(rq->special, cqe, sizeof(*cqe));
-
+done:
 	blk_mq_complete_request(rq, status);
 }