[PATCH] nvme-fc: correct csn initialization and increments on error

[PATCH] nvme-fc: correct csn initialization and increments on error

[James Smart]

This patch fixes a long-standing bug that initialized the FC-NVME
cmnd iu CSN value to 1. Early FC-NVME specs had the connection starting
with CSN=1. By the time the spec reached approval, the language had
changed to state a connection should start with CSN=0.  This patch
corrects the initialization value for FC-NVME connections.

Additionally, in reviewing the transport, the CSN value is assigned to
the new IU early in the start routine. It's possible that a later dma
map request may fail, causing the command to never be sent to the
controller.  Change the location of the assignment so that it is
immediately prior to calling the lldd. Add a comment block to explain
the impacts if the lldd were to additionally fail sending the command.

Signed-off-by: Dick Kennedy <dick.kennedy at broadcom.com>
Signed-off-by: James Smart <jsmart2021 at gmail.com>
---
 drivers/nvme/host/fc.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index f3b9d91ba0df..1f30984b2d31 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1845,7 +1845,7 @@ nvme_fc_init_queue(struct nvme_fc_ctrl *ctrl, int idx)
 	memset(queue, 0, sizeof(*queue));
 	queue->ctrl = ctrl;
 	queue->qnum = idx;
-	atomic_set(&queue->csn, 1);
+	atomic_set(&queue->csn, 0);
 	queue->dev = ctrl->dev;
 
 	if (idx > 0)
@@ -1887,7 +1887,7 @@ nvme_fc_free_queue(struct nvme_fc_queue *queue)
 	 */
 
 	queue->connection_id = 0;
-	atomic_set(&queue->csn, 1);
+	atomic_set(&queue->csn, 0);
 }
 
 static void
@@ -2198,8 +2198,6 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
 
 	/* format the FC-NVME CMD IU and fcp_req */
 	cmdiu->connection_id = cpu_to_be64(queue->connection_id);
-	csn = atomic_inc_return(&queue->csn);
-	cmdiu->csn = cpu_to_be32(csn);
 	cmdiu->data_len = cpu_to_be32(data_len);
 	switch (io_dir) {
 	case NVMEFC_FCP_WRITE:
@@ -2257,11 +2255,28 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
 	if (!(op->flags & FCOP_FLAGS_AEN))
 		blk_mq_start_request(op->rq);
 
+	csn = atomic_inc_return(&queue->csn);
+	cmdiu->csn = cpu_to_be32(csn);
+
 	ret = ctrl->lport->ops->fcp_io(&ctrl->lport->localport,
 					&ctrl->rport->remoteport,
 					queue->lldd_handle, &op->fcp_req);
 
 	if (ret) {
+		/*
+		 * If the lld fails to send the command is there an issue
+		 * with the csn value ?  If the command that fails is the
+		 * Connect, no - as the connection won't be live. If it's
+		 * a command post-connect, it's possible a gap in csn may
+		 * be created. Does this matter? As linux initiators don't
+		 * send fused commands, no. The gap would exist, but as
+		 * there's nothing that depends on csn order to be delivered
+		 * on the target side, it shouldn't hurt. It would be
+		 * difficult for a target to even detect the csn gap as
+		 * it has no idea when the cmd with the csn was supposed
+		 * to arrive.
+		 */
+
 		opstate = atomic_xchg(&op->state, FCPOP_STATE_COMPLETE);
 		__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
 
-- 
2.13.7

[PATCH] nvme-fc: correct csn initialization and increments on error

[Ewan D. Milne]

On Mon, 2019-04-08@11:15 -0700, James Smart wrote:
> This patch fixes a long-standing bug that initialized the FC-NVME
> cmnd iu CSN value to 1. Early FC-NVME specs had the connection starting
> with CSN=1. By the time the spec reached approval, the language had
> changed to state a connection should start with CSN=0.  This patch
> corrects the initialization value for FC-NVME connections.
> 
> Additionally, in reviewing the transport, the CSN value is assigned to
> the new IU early in the start routine. It's possible that a later dma
> map request may fail, causing the command to never be sent to the
> controller.  Change the location of the assignment so that it is
> immediately prior to calling the lldd. Add a comment block to explain
> the impacts if the lldd were to additionally fail sending the command.
> 
> Signed-off-by: Dick Kennedy <dick.kennedy at broadcom.com>
> Signed-off-by: James Smart <jsmart2021 at gmail.com>
> ---
>  drivers/nvme/host/fc.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index f3b9d91ba0df..1f30984b2d31 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -1845,7 +1845,7 @@ nvme_fc_init_queue(struct nvme_fc_ctrl *ctrl, int idx)
>  	memset(queue, 0, sizeof(*queue));
>  	queue->ctrl = ctrl;
>  	queue->qnum = idx;
> -	atomic_set(&queue->csn, 1);
> +	atomic_set(&queue->csn, 0);
>  	queue->dev = ctrl->dev;
>  
>  	if (idx > 0)
> @@ -1887,7 +1887,7 @@ nvme_fc_free_queue(struct nvme_fc_queue *queue)
>  	 */
>  
>  	queue->connection_id = 0;
> -	atomic_set(&queue->csn, 1);
> +	atomic_set(&queue->csn, 0);
>  }
>  
>  static void
> @@ -2198,8 +2198,6 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
>  
>  	/* format the FC-NVME CMD IU and fcp_req */
>  	cmdiu->connection_id = cpu_to_be64(queue->connection_id);
> -	csn = atomic_inc_return(&queue->csn);
> -	cmdiu->csn = cpu_to_be32(csn);
>  	cmdiu->data_len = cpu_to_be32(data_len);
>  	switch (io_dir) {
>  	case NVMEFC_FCP_WRITE:
> @@ -2257,11 +2255,28 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
>  	if (!(op->flags & FCOP_FLAGS_AEN))
>  		blk_mq_start_request(op->rq);
>  
> +	csn = atomic_inc_return(&queue->csn);
> +	cmdiu->csn = cpu_to_be32(csn);
> +
>  	ret = ctrl->lport->ops->fcp_io(&ctrl->lport->localport,
>  					&ctrl->rport->remoteport,
>  					queue->lldd_handle, &op->fcp_req);
>  
>  	if (ret) {
> +		/*
> +		 * If the lld fails to send the command is there an issue
> +		 * with the csn value ?  If the command that fails is the
> +		 * Connect, no - as the connection won't be live. If it's
> +		 * a command post-connect, it's possible a gap in csn may
> +		 * be created. Does this matter? As linux initiators don't
> +		 * send fused commands, no. The gap would exist, but as
> +		 * there's nothing that depends on csn order to be delivered
> +		 * on the target side, it shouldn't hurt. It would be
> +		 * difficult for a target to even detect the csn gap as
> +		 * it has no idea when the cmd with the csn was supposed
> +		 * to arrive.
> +		 */
> +
>  		opstate = atomic_xchg(&op->state, FCPOP_STATE_COMPLETE);
>  		__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
>  

Reviewed-by: Ewan D. Milne <emilne at redhat.com>

[PATCH] nvme-fc: correct csn initialization and increments on error

[Christoph Hellwig]

Thanks, applied to nvme-5.1 with some trivial cleanups.