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

[Christoph Hellwig <hch@lst.de>]

> 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.


> +#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.

> +#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.

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

> +#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.

>  			    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?

> +		INIT_WORK(&chap->auth_work, __nvme_auth_work);

Btw, this really should lose the __ prefix in another prep patch.

> +	}
> +
> +out:
>  	return ret;

We can just drop the out label and return directly.