[PATCH 12/16] nvme-auth: convert dhchap_auth_list to an array

[Sagi Grimberg]

>> Make dhchap_auth_mutex protect only the ctrl host_key and ctrl_key
>> in a fine-graned lock such that there is no long lasting acuisition
>> of the lock.
> 
> Can you split that part into a separate patch?  Right now the patch
> is a bit confusing because it does multiple things.

I couldn't find a way to split it in a way that won't have holes in
it. I'll try to look into this again.

>> +#define nvme_auth_chap_from_qid(ctrl, qid) \
>> +	&((struct nvme_dhchap_queue_context*)(ctrl)->dhchap_ctxs)[qid]
> 
> Please properly type dhchap_ctxs instead of the ugly cast here.  That
> also removed the need for the macro.
> functions instead of macros.

If I type it, nvme_dhchap_queue_context definition needs to move to
nvme.h, is that fine with everyone?

>> +#define ctrl_max_dhchaps(ctrl) \
>> +	(ctrl)->opts->nr_io_queues + (ctrl)->opts->nr_write_queues + \
>> +			(ctrl)->opts->nr_poll_queues + 1
> 
> This overallocates in case the controller supports less queues than
> requested, but I guess that's not a big problem in practice.

It isn't, we do the same with queues.

> Also please make this an inline function to add type safety and to make
> it more readable.

OK.

> 
>> +#define nvme_foreach_dhchap(i, chap, ctrl) \
>> +	for (i = 0, chap = (ctrl)->dhchap_ctxs; \
>> +	     i < ctrl_max_dhchaps(ctrl) && chap != NULL; i++, chap++)
> 
> I find this macro a bit obsfucating.  There's only three callers anyway,
> and they only use chap 1, 2 or three times respectively.
> 
> So I'd just loop:
> 
> 	for (i = 0, i < ctrl_max_dhchaps(ctrl); i++)
> 
> and then do the dereference of >dhchap_ctxs in the loop.

I take it that you don't like the foreach iterator...

>>   			    void *data, size_t data_len, bool auth_send)
>> @@ -330,7 +338,7 @@ static int nvme_auth_process_dhchap_success1(struct nvme_ctrl *ctrl,
>>   	struct nvmf_auth_dhchap_success1_data *data = chap->buf;
>>   	size_t size = sizeof(*data);
>>   
>> -	if (ctrl->ctrl_key)
>> +	if (chap->ctrl_key)
> 
> How is this related to the rest of the patch?

Here the code is dereferencing ctrl->ctrl_key which needs to synchronize
with sysfs driven modifications, but it has a queue chap context as 
well, which does not need a lock. Hence I changed the condition.

>> +		INIT_WORK(&chap->auth_work, __nvme_auth_work);
> 
> Btw, this really should lose the __ prefix in another prep patch.

Yes.