Re: [PATCH 1/3] nvme-fabrics: factor out auth code into helper

[Sagi Grimberg <sagi@grimberg.me>]

On 08/02/2024 8:24, Chaitanya Kulkarni wrote:
> Post connect command authentication handling code is repeated into in
> nvmf_connect_admin_queue() and nvmf_connect_io_queue().
>
> Add a helper to handle post connect command authentication helper. Use
> the same helper in nvmf_connect_admin_queue(). This also removes
> authentication specific code from a build where authentication feature
> is not configured.
>
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>   drivers/nvme/host/auth.c    | 32 ++++++++++++++++++++++++++++++++
>   drivers/nvme/host/fabrics.c | 25 +------------------------
>   drivers/nvme/host/nvme.h    |  8 ++++++++
>   3 files changed, 41 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index 3dce480d932e..159071462738 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -988,6 +988,38 @@ void nvme_auth_stop(struct nvme_ctrl *ctrl)
>   }
>   EXPORT_SYMBOL_GPL(nvme_auth_stop);
>   
> +u16 nvme_auth_post_connect(struct nvme_ctrl *ctrl, u16 qid, u32 result)
> +{
> +	int ret;
> +
> +	if (!(result & (NVME_CONNECT_AUTHREQ_ATR | NVME_CONNECT_AUTHREQ_ASCR)))
> +		return NVME_SC_SUCCESS;

I really dislike functions that may or may not do anything. I vote that 
we avoid
as much as possible.

How about calling the function nvme_authenticate_queue() and more the above
condition to the call-site?

> +
> +	/* Secure concatenation is not implemented */
> +	if (result & NVME_CONNECT_AUTHREQ_ASCR) {
> +		dev_warn(ctrl->device,
> +			  "qid %u: secure concatenation is not supported\n",
> +			  qid);
> +		return NVME_SC_AUTH_REQUIRED;
> +	}
> +	/* Authentication required */
> +	ret = nvme_auth_negotiate(ctrl, qid);
> +	if (ret) {
> +		dev_warn(ctrl->device,
> +			 "qid %u: authentication setup failed\n", qid);
> +		return NVME_SC_AUTH_REQUIRED;
> +	}
> +	ret = nvme_auth_wait(ctrl, qid);
> +	if (ret) {
> +		dev_warn(ctrl->device, "qid %u: authentication failed\n", qid);
> +		return ret;
> +	}
> +	if (!qid)
> +		dev_info(ctrl->device, "qid 0: authenticated\n");
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(nvme_auth_post_connect);
> +
>   void nvme_auth_free(struct nvme_ctrl *ctrl)
>   {
>   	int i;
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 373ed08e6b92..24f0d298825b 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -460,30 +460,7 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
>   
>   	result = le32_to_cpu(res.u32);
>   	ctrl->cntlid = result & 0xFFFF;
> -	if (result & (NVME_CONNECT_AUTHREQ_ATR | NVME_CONNECT_AUTHREQ_ASCR)) {
> -		/* Secure concatenation is not implemented */
> -		if (result & NVME_CONNECT_AUTHREQ_ASCR) {
> -			dev_warn(ctrl->device,
> -				 "qid 0: secure concatenation is not supported\n");
> -			ret = NVME_SC_AUTH_REQUIRED;
> -			goto out_free_data;
> -		}
> -		/* Authentication required */
> -		ret = nvme_auth_negotiate(ctrl, 0);
> -		if (ret) {
> -			dev_warn(ctrl->device,
> -				 "qid 0: authentication setup failed\n");
> -			ret = NVME_SC_AUTH_REQUIRED;
> -			goto out_free_data;
> -		}
> -		ret = nvme_auth_wait(ctrl, 0);
> -		if (ret)
> -			dev_warn(ctrl->device,
> -				 "qid 0: authentication failed\n");
> -		else
> -			dev_info(ctrl->device,
> -				 "qid 0: authenticated\n");
> -	}
> +	ret = nvme_auth_post_connect(ctrl, 0, result);
>   out_free_data:
>   	kfree(data);
>   	return ret;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 1700063bc24d..bb1c9b74aa55 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -1085,6 +1085,7 @@ void nvme_auth_stop(struct nvme_ctrl *ctrl);
>   int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid);
>   int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid);
>   void nvme_auth_free(struct nvme_ctrl *ctrl);
> +u16 nvme_auth_post_connect(struct nvme_ctrl *ctrl, u16 qid, u32 result);
>   #else
>   static inline int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
>   {
> @@ -1107,6 +1108,13 @@ static inline int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid)
>   	return NVME_SC_AUTH_REQUIRED;
>   }
>   static inline void nvme_auth_free(struct nvme_ctrl *ctrl) {};
> +static inline u16 nvme_auth_post_connect(struct nvme_ctrl *ctrl, u16 qid,
> +		u32 result)
> +{
> +	if (result & (NVME_CONNECT_AUTHREQ_ATR | NVME_CONNECT_AUTHREQ_ASCR))
> +		return NVME_SC_AUTH_REQUIRED;
> +	return NVME_SC_SUCCESS;
> +}
>   #endif
>   
>   u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,