nvme: use an integer value to Linux errno values

PATCH New Christoph Hellwig 4 participants 6 messages Oct 12, 2015 – Oct 15, 2015 NVMe

Share & tools

Christoph Hellwig

author maintainer

2015-10-12 19:23 UTC #0 lore raw

PATCH 1/1 nvme: use an integer value to Linux errno values

Use a separate integer variable to hold the signed Linux errno values we pass back to the block layer. Note that for pass through commands those might still be NVMe values, but those fit into the int as well.

Fixes f4829a9b7a61: ("blk-mq: fix racy updates of rq->errors")

Reported-by Dan Carpenter <dan.carpenter at oracle.com>

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/nvme/host/pci.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/nvme/host/pci.c +7 -5

index a526696..9049888 100644
@@ -606,8 +606,8 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	struct nvme_iod *iod = ctx;
 	struct request *req = iod_get_private(iod);
 	struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
-
 	u16 status = le16_to_cpup(&cqe->status) >> 1;
+	int error;
 
 	if (unlikely(status)) {
 		if (!(status & NVME_SC_DNR || blk_noretry_request(req))

@@ -624,9 +624,11 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 
 		if (req->cmd_type == REQ_TYPE_DRV_PRIV) {
 			if (cmd_rq->ctx == CMD_CTX_CANCELLED)
-				status = -EINTR;
+				error = -EINTR;
+			else
+				error = status;
 		} else {
-			status = nvme_error_status(status);
+			error = nvme_error_status(status);
 		}
 	}

@@ -638,7 +640,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	if (cmd_rq->aborted)
 		dev_warn(nvmeq->dev->dev,
 			"completing aborted command with status:%04x\n",
-			status);
+			error);
 
 	if (iod->nents) {
 		dma_unmap_sg(nvmeq->dev->dev, iod->sg, iod->nents,

@@ -652,7 +654,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	}
 	nvme_free_iod(nvmeq->dev, iod);
 
-	blk_mq_complete_request(req, status);
+	blk_mq_complete_request(req, error);
 }
 
 /* length is in bytes.  gfp flags indicates whether we may sleep. */

Signature · 1 line

1.9.1

Christoph Hellwig

2015-10-15 14:57 UTC #1 lore raw

PATCH 1/1 nvme: use an integer value to Linux errno values

Can I get a review for this one? This is something that should probably got to Linus for 4.3.

Quoted 66 lines · Christoph Hellwig

Fixes f4829a9b7a61: ("blk-mq: fix racy updates of rq->errors")

Reported-by Dan Carpenter <dan.carpenter at oracle.com>

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/nvme/host/pci.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/nvme/host/pci.c +7 -5

index a526696..9049888 100644
@@ -606,8 +606,8 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	struct nvme_iod *iod = ctx;
 	struct request *req = iod_get_private(iod);
 	struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
-
 	u16 status = le16_to_cpup(&cqe->status) >> 1;
+	int error;
 
 	if (unlikely(status)) {
 		if (!(status & NVME_SC_DNR || blk_noretry_request(req))

@@ -624,9 +624,11 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 
 		if (req->cmd_type == REQ_TYPE_DRV_PRIV) {
 			if (cmd_rq->ctx == CMD_CTX_CANCELLED)
-				status = -EINTR;
+				error = -EINTR;
+			else
+				error = status;
 		} else {
-			status = nvme_error_status(status);
+			error = nvme_error_status(status);
 		}
 	}

@@ -638,7 +640,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	if (cmd_rq->aborted)
 		dev_warn(nvmeq->dev->dev,
 			"completing aborted command with status:%04x\n",
-			status);
+			error);
 
 	if (iod->nents) {
 		dma_unmap_sg(nvmeq->dev->dev, iod->sg, iod->nents,

@@ -652,7 +654,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	}
 	nvme_free_iod(nvmeq->dev, iod);
 
-	blk_mq_complete_request(req, status);
+	blk_mq_complete_request(req, error);
 }
 
 /* length is in bytes.  gfp flags indicates whether we may sleep. */

Signature · 7 lines

1.9.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

---end quoted text---

Keith Busch

2015-10-15 15:13 UTC #2 lore raw

PATCH 1/1 nvme: use an integer value to Linux errno values

Quoted 4 lines · Christoph Hellwig

Quoted 3 lines · Christoph Hellwig

 drivers/nvme/host/pci.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Quoted 11 lines

drivers/nvme/host/pci.c static void req_completion(struct nvme_queue *nvmeq, void *ctx, · line 610

Quoted 11 lines

drivers/nvme/host/pci.c +1 -1

index a526696..9049888 100644
@@ -606,8 +606,8 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	struct nvme_iod *iod = ctx;
 	struct request *req = iod_get_private(iod);
 	struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
-
 	u16 status = le16_to_cpup(&cqe->status) >> 1;
+	int error;

My compiler didn't warn, so maybe I'm missing something, but this looks like this needs to initialize error to 0, right? It's setting error only in the non-zero status case, but used in both cases.

Quoted 32 lines

drivers/nvme/host/pci.c +6 -4

 	if (unlikely(status)) {
 		if (!(status & NVME_SC_DNR || blk_noretry_request(req))

@@ -624,9 +624,11 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 
 		if (req->cmd_type == REQ_TYPE_DRV_PRIV) {
 			if (cmd_rq->ctx == CMD_CTX_CANCELLED)
-				status = -EINTR;
+				error = -EINTR;
+			else
+				error = status;
 		} else {
-			status = nvme_error_status(status);
+			error = nvme_error_status(status);
 		}
 	}

@@ -638,7 +640,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	if (cmd_rq->aborted)
 		dev_warn(nvmeq->dev->dev,
 			"completing aborted command with status:%04x\n",
-			status);
+			error);
 
 	if (iod->nents) {
 		dma_unmap_sg(nvmeq->dev->dev, iod->sg, iod->nents,

@@ -652,7 +654,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	}
 	nvme_free_iod(nvmeq->dev, iod);
 
-	blk_mq_complete_request(req, status);
+	blk_mq_complete_request(req, error);
 }

Christoph Hellwig

2015-10-15 15:30 UTC #3 lore raw

PATCH 1/1 nvme: use an integer value to Linux errno values

Quoted 5 lines · Keith Busch

Quoted 4 lines · Christoph Hellwig
Quoted 3 lines · Christoph Hellwig
drivers/nvme/host/pci.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

Quoted 15 lines

Quoted 11 lines
drivers/nvme/host/pci.c static void req_completion(struct nvme_queue *nvmeq, void *ctx, · line 610
Quoted 11 lines
drivers/nvme/host/pci.c +1 -1
index a526696..9049888 100644
@@ -606,8 +606,8 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	struct nvme_iod *iod = ctx;
 	struct request *req = iod_get_private(iod);
 	struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
-
 	u16 status = le16_to_cpup(&cqe->status) >> 1;
+	int error;
My compiler didn't warn, so maybe I'm missing something, but this looks like this needs to initialize error to 0, right? It's setting error only in the non-zero status case, but used in both cases.

Yes, it should be set to zero. That's what I did in the tested version because this one will actually blow up.

Jens Axboe

2015-10-15 15:51 UTC #4 lore raw

PATCH 1/1 nvme: use an integer value to Linux errno values

Quoted 22 lines · Christoph Hellwig

Quoted 5 lines · Keith Busch
Quoted 4 lines · Christoph Hellwig
Quoted 3 lines · Christoph Hellwig
drivers/nvme/host/pci.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
Quoted 15 lines
Quoted 11 lines
drivers/nvme/host/pci.c static void req_completion(struct nvme_queue *nvmeq, void *ctx, · line 610
Quoted 11 lines
drivers/nvme/host/pci.c +1 -1
index a526696..9049888 100644
@@ -606,8 +606,8 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	struct nvme_iod *iod = ctx;
 	struct request *req = iod_get_private(iod);
 	struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
-
 	u16 status = le16_to_cpup(&cqe->status) >> 1;
+	int error;
My compiler didn't warn, so maybe I'm missing something, but this looks like this needs to initialize error to 0, right? It's setting error only in the non-zero status case, but used in both cases.

Quoted 2 lines

Yes, it should be set to zero. That's what I did in the tested version because this one will actually blow up.

Good catch Keith, I've fixed it up.

Signature · 1 line

Jens Axboe

Jens Axboe

2015-10-15 15:04 UTC #5 lore raw

PATCH 1/1 nvme: use an integer value to Linux errno values

In reply to Christoph Hellwig

Quoted 8 lines · Christoph Hellwig

Use a separate integer variable to hold the signed Linux errno values we pass back to the block layer. Note that for pass through commands those might still be NVMe values, but those fit into the int as well.
Fixes f4829a9b7a61: ("blk-mq: fix racy updates of rq->errors")
Reported-by Dan Carpenter <dan.carpenter at oracle.com>
Signed-off-by Christoph Hellwig <hch at lst.de>

Looks good to me. I'll manually fold this into the 4.3 base as well.

Signature · 1 line

Jens Axboe