[PATCH] nvmet: return Invalid Log Page status code for unsupported lid

[PATCH] nvmet: return Invalid Log Page status code for unsupported lid

[amit.engel]

From: Amit Engel <amit.engel@dell.com>

In case of an invalid or not supported log page
return NVME_SC_INVALID_LOG_PAGE

Signed-off-by: Amit Engel <amit.engel@dell.com>
---
 drivers/nvme/target/admin-cmd.c | 2 +-
 drivers/nvme/target/discovery.c | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index aa6d84d8848e..5dcda2c87fdd 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -339,7 +339,7 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 	pr_debug("unhandled lid %d on qid %d\n",
 	       req->cmd->get_log_page.lid, req->sq->qid);
 	req->error_loc = offsetof(struct nvme_get_log_page_command, lid);
-	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
+	nvmet_req_complete(req, NVME_SC_INVALID_LOG_PAGE | NVME_SC_DNR);
 }
 
 static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 7aa62bc6ae84..fdd52dbdd00f 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -176,9 +176,11 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
 		return;
 
 	if (req->cmd->get_log_page.lid != NVME_LOG_DISC) {
+		pr_err("unhandled get log page cmd with lid %#x from ctrlid %d\n"
+			req->cmd->get_log_page.lid, ctrl->cntlid);
 		req->error_loc =
 			offsetof(struct nvme_get_log_page_command, lid);
-		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+		status = NVME_SC_INVALID_LOG_PAGE | NVME_SC_DNR;
 		goto out;
 	}
 
-- 
2.18.2


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

Re: [PATCH] nvmet: return Invalid Log Page status code for unsupported lid

[Klaus Jensen]

[-- Attachment #1.1: Type: text/plain, Size: 2229 bytes --]

On Sep  2 11:53, amit.engel@dell.com wrote:
> From: Amit Engel <amit.engel@dell.com>
> 
> In case of an invalid or not supported log page
> return NVME_SC_INVALID_LOG_PAGE
> 
> Signed-off-by: Amit Engel <amit.engel@dell.com>
> ---
>  drivers/nvme/target/admin-cmd.c | 2 +-
>  drivers/nvme/target/discovery.c | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index aa6d84d8848e..5dcda2c87fdd 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -339,7 +339,7 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
>  	pr_debug("unhandled lid %d on qid %d\n",
>  	       req->cmd->get_log_page.lid, req->sq->qid);
>  	req->error_loc = offsetof(struct nvme_get_log_page_command, lid);
> -	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
> +	nvmet_req_complete(req, NVME_SC_INVALID_LOG_PAGE | NVME_SC_DNR);
>  }
>  
>  static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
> diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
> index 7aa62bc6ae84..fdd52dbdd00f 100644
> --- a/drivers/nvme/target/discovery.c
> +++ b/drivers/nvme/target/discovery.c
> @@ -176,9 +176,11 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
>  		return;
>  
>  	if (req->cmd->get_log_page.lid != NVME_LOG_DISC) {
> +		pr_err("unhandled get log page cmd with lid %#x from ctrlid %d\n"
> +			req->cmd->get_log_page.lid, ctrl->cntlid);
>  		req->error_loc =
>  			offsetof(struct nvme_get_log_page_command, lid);
> -		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
> +		status = NVME_SC_INVALID_LOG_PAGE | NVME_SC_DNR;
>  		goto out;
>  	}
>  

This should only be done if the target is reporting v1.4 or above.

But honestly I'm not sure if there is any benefit to potentially
breaking hosts here. It is perfectly ok for a v1.4 compliant controller
to continue using Invalid Field in Command in this case.

The spec actually says that the controller *should* abort the command
with a status code of Invalid Field in Command. But I acknowledge that
v1.4+ specs are a bit contradictory here.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 158 bytes --]

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

RE: [PATCH] nvmet: return Invalid Log Page status code for unsupported lid

[Engel, Amit]

For v1.4 or above, it’s more specific to reply INVALID_LOG_PAGE when an unknown log id is being sent by the host (I think that's the reason to introduce INVALID_LOG_PAGE status code..) But I agree that it's not critical

Thanks
Amit


Internal Use - Confidential

-----Original Message-----
From: Klaus Jensen <its@irrelevant.dk>
Sent: Thursday, September 2, 2021 1:01 PM
To: Engel, Amit
Cc: sagi@grimberg.me; hch@infradead.org; linux-nvme@lists.infradead.org
Subject: Re: [PATCH] nvmet: return Invalid Log Page status code for unsupported lid

On Sep  2 11:53, amit.engel@dell.com wrote:
> From: Amit Engel <amit.engel@dell.com>
> 
> In case of an invalid or not supported log page return 
> NVME_SC_INVALID_LOG_PAGE
> 
> Signed-off-by: Amit Engel <amit.engel@dell.com>
> ---
>  drivers/nvme/target/admin-cmd.c | 2 +- 
> drivers/nvme/target/discovery.c | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c 
> b/drivers/nvme/target/admin-cmd.c index aa6d84d8848e..5dcda2c87fdd
> 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -339,7 +339,7 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
>  	pr_debug("unhandled lid %d on qid %d\n",
>  	       req->cmd->get_log_page.lid, req->sq->qid);
>  	req->error_loc = offsetof(struct nvme_get_log_page_command, lid);
> -	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
> +	nvmet_req_complete(req, NVME_SC_INVALID_LOG_PAGE | NVME_SC_DNR);
>  }
>  
>  static void nvmet_execute_identify_ctrl(struct nvmet_req *req) diff 
> --git a/drivers/nvme/target/discovery.c 
> b/drivers/nvme/target/discovery.c index 7aa62bc6ae84..fdd52dbdd00f
> 100644
> --- a/drivers/nvme/target/discovery.c
> +++ b/drivers/nvme/target/discovery.c
> @@ -176,9 +176,11 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
>  		return;
>  
>  	if (req->cmd->get_log_page.lid != NVME_LOG_DISC) {
> +		pr_err("unhandled get log page cmd with lid %#x from ctrlid %d\n"
> +			req->cmd->get_log_page.lid, ctrl->cntlid);
>  		req->error_loc =
>  			offsetof(struct nvme_get_log_page_command, lid);
> -		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
> +		status = NVME_SC_INVALID_LOG_PAGE | NVME_SC_DNR;
>  		goto out;
>  	}
>  

This should only be done if the target is reporting v1.4 or above.

But honestly I'm not sure if there is any benefit to potentially breaking hosts here. It is perfectly ok for a v1.4 compliant controller to continue using Invalid Field in Command in this case.

The spec actually says that the controller *should* abort the command with a status code of Invalid Field in Command. But I acknowledge that v1.4+ specs are a bit contradictory here.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme