[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 at irrelevant.dk>
Sent: Thursday, September 2, 2021 1:01 PM
To: Engel, Amit
Cc: sagi at grimberg.me; hch at infradead.org; linux-nvme at lists.infradead.org
Subject: Re: [PATCH] nvmet: return Invalid Log Page status code for unsupported lid

On Sep  2 11:53, amit.engel at dell.com wrote:
> From: Amit Engel <amit.engel at 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 at 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.