From: Hannes Reinecke <[email protected]>
Any authentication errors which are generated internally are always
non-retryable, so set the DNR bit to ensure they are not retried.
Signed-off-by: Hannes Reinecke <[email protected]>
Signed-off-by: Daniel Wagner <[email protected]>
---
drivers/nvme/host/core.c | 6 +++---
drivers/nvme/host/fabrics.c | 29 +++++++++++++++++------------
drivers/nvme/host/nvme.h | 2 +-
3 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 504dc352c458..66387bcca8ae 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -383,14 +383,14 @@ static inline enum nvme_disposition nvme_decide_disposition(struct request *req)
if (likely(nvme_req(req)->status == 0))
return COMPLETE;
- if ((nvme_req(req)->status & 0x7ff) == NVME_SC_AUTH_REQUIRED)
- return AUTHENTICATE;
-
if (blk_noretry_request(req) ||
(nvme_req(req)->status & NVME_SC_DNR) ||
nvme_req(req)->retries >= nvme_max_retries)
return COMPLETE;
+ if ((nvme_req(req)->status & 0x7ff) == NVME_SC_AUTH_REQUIRED)
+ return AUTHENTICATE;
+
if (req->cmd_flags & REQ_NVME_MPATH) {
if (nvme_is_path_error(nvme_req(req)->status) ||
blk_queue_dying(req->q))
diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 1f0ea1f32d22..309a69c24995 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -467,7 +467,7 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
if (result & NVME_CONNECT_AUTHREQ_ASCR) {
dev_warn(ctrl->device,
"qid 0: secure concatenation is not supported\n");
- ret = NVME_SC_AUTH_REQUIRED;
+ ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
goto out_free_data;
}
/* Authentication required */
@@ -475,14 +475,16 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
if (ret) {
dev_warn(ctrl->device,
"qid 0: authentication setup failed\n");
- ret = NVME_SC_AUTH_REQUIRED;
+ ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
goto out_free_data;
}
ret = nvme_auth_wait(ctrl, 0);
- if (ret)
+ if (ret) {
dev_warn(ctrl->device,
- "qid 0: authentication failed\n");
- else
+ "qid 0: authentication failed, error %d\n",
+ ret);
+ ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
+ } else
dev_info(ctrl->device,
"qid 0: authenticated\n");
}
@@ -542,7 +544,7 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid)
if (result & NVME_CONNECT_AUTHREQ_ASCR) {
dev_warn(ctrl->device,
"qid 0: secure concatenation is not supported\n");
- ret = NVME_SC_AUTH_REQUIRED;
+ ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
goto out_free_data;
}
/* Authentication required */
@@ -550,12 +552,15 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid)
if (ret) {
dev_warn(ctrl->device,
"qid %d: authentication setup failed\n", qid);
- ret = NVME_SC_AUTH_REQUIRED;
- } else {
- ret = nvme_auth_wait(ctrl, qid);
- if (ret)
- dev_warn(ctrl->device,
- "qid %u: authentication failed\n", qid);
+ ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
+ goto out_free_data;
+ }
+ ret = nvme_auth_wait(ctrl, qid);
+ if (ret) {
+ dev_warn(ctrl->device,
+ "qid %u: authentication failed, error %d\n",
+ qid, ret);
+ ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
}
}
out_free_data:
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index d0ed64dc7380..882967365108 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -1122,7 +1122,7 @@ static inline int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
}
static inline int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid)
{
- return NVME_SC_AUTH_REQUIRED;
+ return NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
}
static inline void nvme_auth_free(struct nvme_ctrl *ctrl) {};
#endif
--
2.44.0
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -467,7 +467,7 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
> if (result & NVME_CONNECT_AUTHREQ_ASCR) {
> dev_warn(ctrl->device,
> "qid 0: secure concatenation is not supported\n");
> - ret = NVME_SC_AUTH_REQUIRED;
> + ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
This is still abusing on the wire status code for in-kernel return
codes. Can we please sort this out properly?
On Fri, Apr 05, 2024 at 08:16:24AM +0200, Christoph Hellwig wrote:
> > --- a/drivers/nvme/host/fabrics.c
> > +++ b/drivers/nvme/host/fabrics.c
> > @@ -467,7 +467,7 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
> > if (result & NVME_CONNECT_AUTHREQ_ASCR) {
> > dev_warn(ctrl->device,
> > "qid 0: secure concatenation is not supported\n");
> > - ret = NVME_SC_AUTH_REQUIRED;
> > + ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
>
> This is still abusing on the wire status code for in-kernel return
> codes. Can we please sort this out properly?
Okay, though I am not really sure how to do it correctly.
So the current mapping is
ret < 0: kernel errors
ret = 0: all good
ret > 0: wire status incl DNR
In order to split the internal DNR away, we could attach it to the cmd.
Is this what you had in mind? Or do you mean we should not return
NVME_SC_AUTH_REQUIRED at all. Instead just a negative value and update
the error handling on the callers?
On Fri, Apr 05, 2024 at 08:45:11AM +0200, Daniel Wagner wrote:
> > This is still abusing on the wire status code for in-kernel return
> > codes. Can we please sort this out properly?
>
> Okay, though I am not really sure how to do it correctly.
>
> So the current mapping is
>
> ret < 0: kernel errors
> ret = 0: all good
> ret > 0: wire status incl DNR
Yes.
> In order to split the internal DNR away, we could attach it to the cmd.
> Is this what you had in mind? Or do you mean we should not return
> NVME_SC_AUTH_REQUIRED at all. Instead just a negative value and update
> the error handling on the callers?
The latter.