Re: [PATCH 3/3] nvme-fabrics: factor out common for auth

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 5/23/2023 12:00 AM, Christoph Hellwig wrote:
> On Mon, May 22, 2023 at 06:04:08AM -0700, Chaitanya Kulkarni wrote:
>> nvmf_connect_admin_queue and nvmf_connect_io_queue() shares common code
>> for post connect command authentication processing that includes,
>> returning appropriate NVMe authentication status based on the
>> command result, authentication negotiation per qid, waiting on
>> negotiation per qid.
>>
>> Add a common helper function to reduce the code duplication with
>> necessary aruments of qid and result of connect command.
>>
>> No functional changes in this patch.
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   drivers/nvme/host/fabrics.c | 81 ++++++++++++++++---------------------
>>   1 file changed, 35 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index 9d3df63eb49a..1d48c0b77cd0 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -433,6 +433,39 @@ static void nvmf_connect_cmd_prep(struct nvme_ctrl *ctrl, u16 qid,
>>   		cmd->connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
>>   }
>>   
>> +static u16 nvmf_auth_post_queue_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;
> 
> While it's a minor duplication, I'd prepfer to keep this check in the
> callers.  That makes it very explicit we are only calling into the helper
> when authentication is required.
> 

we are cleaning up so much code I guess addition of 2 lines in 
send/receive callers should be fine, I'll add it in the V2.

>> +	ret = nvme_auth_wait(ctrl, qid);
>> +	if (ret) {
>> +		dev_warn(ctrl->device, "qid %u: authentication failed\n", qid);
>> +		return ret;
>> +	}
>> +	if (!ret && qid == 0)
> 
> ret must be 0 at this point bsed on the check above.

will fix it in V2 .

-ck