[PATCH 14/16] nvmet: Allow to change 'TSAS' and 'TREQ'

[Sagi Grimberg]

On 8/8/23 19:53, Hannes Reinecke wrote:
> Allow to change 'TSAS' and 'TREQ' to values specified in the spec.

I think a little more description is needed here. About the
rules of how the two interact.

> 
> Signed-off-by: Hannes Reinecke <hare at suse.de>
> ---
>   drivers/nvme/target/configfs.c | 36 +++++++++++++++++++++++++++++++---
>   1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 7f826ac8b75c..3c2aeb763e22 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -159,10 +159,15 @@ static const struct nvmet_type_name_map nvmet_addr_treq[] = {
>   	{ NVMF_TREQ_NOT_REQUIRED,	"not required" },
>   };
>   
> +static inline u8 nvmet_port_treq(struct nvmet_port *port)
> +{
> +	return (port->disc_addr.treq & NVME_TREQ_SECURE_CHANNEL_MASK);
> +}

The function name does not match what it does.

> +
>   static ssize_t nvmet_addr_treq_show(struct config_item *item, char *page)
>   {
> -	u8 treq = to_nvmet_port(item)->disc_addr.treq &
> -		NVME_TREQ_SECURE_CHANNEL_MASK;
> +	struct nvmet_port *port = to_nvmet_port(item);
> +	u8 treq = nvmet_port_treq(port);
>   	int i;
>   
>   	for (i = 0; i < ARRAY_SIZE(nvmet_addr_treq); i++) {
> @@ -174,11 +179,16 @@ static ssize_t nvmet_addr_treq_show(struct config_item *item, char *page)
>   	return snprintf(page, PAGE_SIZE, "\n");
>   }
>   
> +static inline u8 nvmet_port_treq_mask(struct nvmet_port *port)
> +{
> +	return (port->disc_addr.treq & ~NVME_TREQ_SECURE_CHANNEL_MASK);
> +}

Same here, function name and what it returns seem diverged from each
other.

> +
>   static ssize_t nvmet_addr_treq_store(struct config_item *item,
>   		const char *page, size_t count)
>   {
>   	struct nvmet_port *port = to_nvmet_port(item);
> -	u8 treq = port->disc_addr.treq & ~NVME_TREQ_SECURE_CHANNEL_MASK;
> +	u8 treq = nvmet_port_treq_mask(port);
>   	int i;
>   
>   	if (nvmet_is_port_enabled(port, __func__))
> @@ -193,6 +203,15 @@ static ssize_t nvmet_addr_treq_store(struct config_item *item,
>   	return -EINVAL;
>   
>   found:
> +	if (port->disc_addr.trtype == NVMF_TRTYPE_TCP) {
> +		if (port->disc_addr.tsas.tcp.sectype != NVMF_TCP_SECTYPE_TLS13) {
> +			pr_warn("cannot change TREQ when TLS is not enabled\n");
> +			return -EINVAL;

So what are you supposed to set in treq without tls?

> +		} else if (nvmet_addr_treq[i].type == NVMF_TREQ_NOT_SPECIFIED) {
> +			pr_warn("cannot set TREQ to 'not specified' when TLS is enabled\n");
> +			return -EINVAL;
> +		}
> +	}
>   	treq |= nvmet_addr_treq[i].type;
>   	port->disc_addr.treq = treq;
>   	return count;
> @@ -371,6 +390,7 @@ static ssize_t nvmet_addr_tsas_store(struct config_item *item,
>   		const char *page, size_t count)
>   {
>   	struct nvmet_port *port = to_nvmet_port(item);
> +	u8 treq = nvmet_port_treq_mask(port);
>   	int i;
>   
>   	if (nvmet_is_port_enabled(port, __func__))
> @@ -389,6 +409,16 @@ static ssize_t nvmet_addr_tsas_store(struct config_item *item,
>   
>   found:
>   	nvmet_port_init_tsas_tcp(port, nvmet_addr_tsas_tcp[i].type);
> +	if (nvmet_addr_tsas_tcp[i].type == NVMF_TCP_SECTYPE_TLS13) {
> +		if (nvmet_port_treq(port) == NVMF_TREQ_NOT_SPECIFIED)
> +			treq |= NVMF_TREQ_REQUIRED;
> +		else
> +			treq |= nvmet_port_treq(port);
> +	} else {
> +		/* Set to 'not specified' if TLS is not enabled */
> +		treq |= NVMF_TREQ_NOT_SPECIFIED;
> +	}
> +	port->disc_addr.treq = treq;

Hmm, so setting tsas modifies treq under the hood.
Surprising...

It is not clear to me what is the user supposed to set in configfs.
Just set tsas to tls1.3 and that's it? any treq setting will not be
allowed after it? Its a bit confusing.