nvmet_set_error ignores error_loc NVMET_NO_ERROR_LOC

nvmet_set_error ignores error_loc NVMET_NO_ERROR_LOC

[Engel, Amit]

Hello,
A question regarding Get Log Page - Error Information lid 01h:
According to nvme spec 1.4,  bytes 15:14 refer to 'Parameter Error Location'
"If the error is not specific to a particular command, then this field shall be set to FFFFh"

In the current implementation, nvmet_req_init initializes req->error_loc to NVMET_NO_ERROR_LOC (((u16)-1))
Then nvmet_set_error has this condition:

if (!ctrl || req->error_loc == NVMET_NO_ERROR_LOC)
	return;

means that the error log will not contain errors that don't specify error_loc
is that expected ? 
don't we expect that param_error_location log page field will be set to FFFFh in case there is no other valid req->error_loc ?

Thanks,
Amit Engel
 




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

Re: nvmet_set_error ignores error_loc NVMET_NO_ERROR_LOC

[Chaitanya Kulkarni]

On 3/16/21 02:09, Engel, Amit wrote:
> Hello,
> A question regarding Get Log Page - Error Information lid 01h:
> According to nvme spec 1.4,  bytes 15:14 refer to 'Parameter Error Location'
> "If the error is not specific to a particular command, then this field shall be set to FFFFh"
>
> In the current implementation, nvmet_req_init initializes req->error_loc to NVMET_NO_ERROR_LOC (((u16)-1))
> Then nvmet_set_error has this condition:
>
> if (!ctrl || req->error_loc == NVMET_NO_ERROR_LOC)
> 	return;
>
> means that the error log will not contain errors that don't specify error_loc
> is that expected ? 
> don't we expect that param_error_location log page field will be set to FFFFh in case there is no other valid req->error_loc ?
>

Based on your email description I think this is what is needed,
please confirm I'll send out a patch:-

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index b15ed7ac34eb..29a6a1446cd8 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -914,6 +914,7 @@ bool nvmet_req_init(struct nvmet_req *req, struct
nvmet_cq *cq,
        req->cqe->sq_head = 0;
        req->ns = NULL;
        req->error_loc = NVMET_NO_ERROR_LOC;
+       req->param_error_location = 0xffff;
        req->error_slba = 0;
 
        /* no support for fused commands yet */


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

Re: nvmet_set_error ignores error_loc NVMET_NO_ERROR_LOC

[Hou Pu]

On Date: Tue, 16 Mar 2021 08:57:08 +0000, Amit wrote:
> Hello,
> A question regarding Get Log Page - Error Information lid 01h:
> According to nvme spec 1.4,  bytes 15:14 refer to 'Parameter Error Location'
> "If the error is not specific to a particular command, then this field shall be set to FFFFh"
>
> In the current implementation, nvmet_req_init initializes req->error_loc to NVMET_NO_ERROR_LOC (((u16)-1))
> Then nvmet_set_error has this condition:

> if (!ctrl || req->error_loc == NVMET_NO_ERROR_LOC)
> 	return;
>
> means that the error log will not contain errors that don't specify error_loc
> is that expected ? 
> don't we expect that param_error_location log page field will be set to FFFFh in case there is no other valid req->error_loc ?

Hi Amit,
I think the specification does not tell us which error should go into the
error log page (or does it?). It is decided by implementation and current
implementation only records errors with an error_loc other than
NVMET_NO_ERROR_LOC.

So the problem here is that:
If some errors which do not set error_loc should go into the error log page,
we need some change here.

Thanks,
Hou

 

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

Re: nvmet_set_error ignores error_loc NVMET_NO_ERROR_LOC

[Chaitanya Kulkarni]

On 4/2/21 00:36, Hou Pu wrote:
> Hi Amit,
> I think the specification does not tell us which error should go into the
> error log page (or does it?). It is decided by implementation and current
> implementation only records errors with an error_loc other than
> NVMET_NO_ERROR_LOC.
>
> So the problem here is that:
> If some errors which do not set error_loc should go into the error log page,
> we need some change here.
>
> Thanks,
> Hou
>
>  

We set the error based on the request so I think for all the errors we
do have
a error location. Have you encountered a place in current code that sets the
error which is not related to any request ?

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

RE: nvmet_set_error ignores error_loc NVMET_NO_ERROR_LOC

[Engel, Amit]

nvme spec 1.4
Figure 195: Get Log Page - Error Information Log Entry (Log Identifier 01h)
Parameter Error Location:
"If the error is not specific to a particular command, then this field shall be set to FFFFh"

So as I understand, the error log may contain errors with error location value 0xffff

-----Original Message-----
From: Hou Pu <houpu.main@gmail.com> 
Sent: Friday, April 2, 2021 10:36 AM
To: Engel, Amit
Cc: Anner, Ran; linux-nvme@lists.infradead.org; sagi@grimberg.me; chaitanya.kulkarni@wdc.com
Subject: Re: nvmet_set_error ignores error_loc NVMET_NO_ERROR_LOC


[EXTERNAL EMAIL] 

On Date: Tue, 16 Mar 2021 08:57:08 +0000, Amit wrote:
> Hello,
> A question regarding Get Log Page - Error Information lid 01h:
> According to nvme spec 1.4,  bytes 15:14 refer to 'Parameter Error Location'
> "If the error is not specific to a particular command, then this field shall be set to FFFFh"
>
> In the current implementation, nvmet_req_init initializes 
> req->error_loc to NVMET_NO_ERROR_LOC (((u16)-1)) Then nvmet_set_error has this condition:

> if (!ctrl || req->error_loc == NVMET_NO_ERROR_LOC)
> 	return;
>
> means that the error log will not contain errors that don't specify 
> error_loc is that expected ?
> don't we expect that param_error_location log page field will be set to FFFFh in case there is no other valid req->error_loc ?

Hi Amit,
I think the specification does not tell us which error should go into the error log page (or does it?). It is decided by implementation and current implementation only records errors with an error_loc other than NVMET_NO_ERROR_LOC.

So the problem here is that:
If some errors which do not set error_loc should go into the error log page, we need some change here.

Thanks,
Hou

 

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