* [PATCH] nvme: use an integer value to Linux errno values
@ 2015-10-12 19:23 Christoph Hellwig
2015-10-15 14:57 ` Christoph Hellwig
2015-10-15 15:04 ` Jens Axboe
0 siblings, 2 replies; 6+ messages in thread
From: Christoph Hellwig @ 2015-10-12 19:23 UTC (permalink / raw)
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(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index a526696..9049888 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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. */
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] nvme: use an integer value to Linux errno values
2015-10-12 19:23 [PATCH] nvme: use an integer value to Linux errno values Christoph Hellwig
@ 2015-10-15 14:57 ` Christoph Hellwig
2015-10-15 15:13 ` Keith Busch
2015-10-15 15:04 ` Jens Axboe
1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2015-10-15 14:57 UTC (permalink / raw)
Can I get a review for this one? This is something that should probably
got to Linus for 4.3.
On Mon, Oct 12, 2015@09:23:39PM +0200, Christoph Hellwig wrote:
> 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(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index a526696..9049888 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -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. */
> --
> 1.9.1
>
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
---end quoted text---
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] nvme: use an integer value to Linux errno values
2015-10-15 14:57 ` Christoph Hellwig
@ 2015-10-15 15:13 ` Keith Busch
2015-10-15 15:30 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Keith Busch @ 2015-10-15 15:13 UTC (permalink / raw)
On Thu, 15 Oct 2015, Christoph Hellwig wrote:
> On Mon, Oct 12, 2015@09:23:39PM +0200, Christoph Hellwig wrote:
>> drivers/nvme/host/pci.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>> index a526696..9049888 100644
>> --- a/drivers/nvme/host/pci.c
>> +++ b/drivers/nvme/host/pci.c
>> @@ -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.
>> 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);
>> }
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] nvme: use an integer value to Linux errno values
2015-10-15 15:13 ` Keith Busch
@ 2015-10-15 15:30 ` Christoph Hellwig
2015-10-15 15:51 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2015-10-15 15:30 UTC (permalink / raw)
On Thu, Oct 15, 2015@03:13:23PM +0000, Keith Busch wrote:
> On Thu, 15 Oct 2015, Christoph Hellwig wrote:
> >On Mon, Oct 12, 2015@09:23:39PM +0200, Christoph Hellwig wrote:
> >> drivers/nvme/host/pci.c | 12 +++++++-----
> >> 1 file changed, 7 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> >>index a526696..9049888 100644
> >>--- a/drivers/nvme/host/pci.c
> >>+++ b/drivers/nvme/host/pci.c
> >>@@ -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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] nvme: use an integer value to Linux errno values
2015-10-15 15:30 ` Christoph Hellwig
@ 2015-10-15 15:51 ` Jens Axboe
0 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2015-10-15 15:51 UTC (permalink / raw)
On 10/15/2015 09:30 AM, Christoph Hellwig wrote:
> On Thu, Oct 15, 2015@03:13:23PM +0000, Keith Busch wrote:
>> On Thu, 15 Oct 2015, Christoph Hellwig wrote:
>>> On Mon, Oct 12, 2015@09:23:39PM +0200, Christoph Hellwig wrote:
>>>> drivers/nvme/host/pci.c | 12 +++++++-----
>>>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>>>> index a526696..9049888 100644
>>>> --- a/drivers/nvme/host/pci.c
>>>> +++ b/drivers/nvme/host/pci.c
>>>> @@ -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.
Good catch Keith, I've fixed it up.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] nvme: use an integer value to Linux errno values
2015-10-12 19:23 [PATCH] nvme: use an integer value to Linux errno values Christoph Hellwig
2015-10-15 14:57 ` Christoph Hellwig
@ 2015-10-15 15:04 ` Jens Axboe
1 sibling, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2015-10-15 15:04 UTC (permalink / raw)
On 10/12/2015 01:23 PM, Christoph Hellwig wrote:
> 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.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-10-15 15:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-12 19:23 [PATCH] nvme: use an integer value to Linux errno values Christoph Hellwig
2015-10-15 14:57 ` Christoph Hellwig
2015-10-15 15:13 ` Keith Busch
2015-10-15 15:30 ` Christoph Hellwig
2015-10-15 15:51 ` Jens Axboe
2015-10-15 15:04 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).