[PATCH 0/5] NVMe cleanups for 5.8

[PATCH 0/5] NVMe cleanups for 5.8

[Max Gurtovoy]

This series introduce some cleanups and code/logic duplication for host
and target side. It introduces a new header file nvme-types.h that will
be used by both host and target drivers. For now it will include mainly
mapping structures from types to names, but in the future it can include
more common logic between host/target drivers (as done for specific TCP
and RDMA transports). The series continues with adding a flags member to
nvmet_fabrics_ops structure and removes the need for adding dedicated
member for each feature in this struct.

Max Gurtovoy (5):
  nvme: introduce nvme-types header file
  nvme: use nvme_ana_type_to_name to get state string
  nvme: replace transport name with trtype enum for ops
  nvmet-tcp: remove has_keyed_sgls initialization
  nvmet: introduce flags member in nvmet_fabrics_ops

 drivers/nvme/host/fabrics.c     |  3 +-
 drivers/nvme/host/fabrics.h     |  4 +-
 drivers/nvme/host/fc.c          |  2 +-
 drivers/nvme/host/multipath.c   | 16 ++-----
 drivers/nvme/host/nvme.h        |  1 +
 drivers/nvme/host/rdma.c        |  2 +-
 drivers/nvme/host/tcp.c         |  2 +-
 drivers/nvme/target/admin-cmd.c |  2 +-
 drivers/nvme/target/configfs.c  | 92 ++++++++-------------------------------
 drivers/nvme/target/core.c      |  2 +-
 drivers/nvme/target/discovery.c |  2 +-
 drivers/nvme/target/loop.c      |  2 +-
 drivers/nvme/target/nvmet.h     |  6 ++-
 drivers/nvme/target/rdma.c      |  3 +-
 drivers/nvme/target/tcp.c       |  1 -
 include/linux/nvme-types.h      | 95 +++++++++++++++++++++++++++++++++++++++++
 16 files changed, 132 insertions(+), 103 deletions(-)
 create mode 100644 include/linux/nvme-types.h

-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 1/5] nvme: introduce nvme-types header file

[Max Gurtovoy]

Centralize the mapping between name and type to a common header file
instead of duplicating logic in both NVMe host and target drivers.

Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
Reviewed-by: Israel Rukshin <israelr@mellanox.com>
---
 drivers/nvme/target/configfs.c | 92 ++++++++--------------------------------
 drivers/nvme/target/nvmet.h    |  1 +
 include/linux/nvme-types.h     | 95 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 113 insertions(+), 75 deletions(-)
 create mode 100644 include/linux/nvme-types.h

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 419e0d4..daeb17e 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -20,27 +20,6 @@
 static LIST_HEAD(nvmet_ports_list);
 struct list_head *nvmet_ports = &nvmet_ports_list;
 
-struct nvmet_type_name_map {
-	u8		type;
-	const char	*name;
-};
-
-static struct nvmet_type_name_map nvmet_transport[] = {
-	{ NVMF_TRTYPE_RDMA,	"rdma" },
-	{ NVMF_TRTYPE_FC,	"fc" },
-	{ NVMF_TRTYPE_TCP,	"tcp" },
-	{ NVMF_TRTYPE_LOOP,	"loop" },
-};
-
-static const struct nvmet_type_name_map nvmet_addr_family[] = {
-	{ NVMF_ADDR_FAMILY_PCI,		"pcie" },
-	{ NVMF_ADDR_FAMILY_IP4,		"ipv4" },
-	{ NVMF_ADDR_FAMILY_IP6,		"ipv6" },
-	{ NVMF_ADDR_FAMILY_IB,		"ib" },
-	{ NVMF_ADDR_FAMILY_FC,		"fc" },
-	{ NVMF_ADDR_FAMILY_LOOP,	"loop" },
-};
-
 static bool nvmet_is_port_enabled(struct nvmet_port *p, const char *caller)
 {
 	if (p->enabled)
@@ -56,14 +35,8 @@ static bool nvmet_is_port_enabled(struct nvmet_port *p, const char *caller)
 static ssize_t nvmet_addr_adrfam_show(struct config_item *item, char *page)
 {
 	u8 adrfam = to_nvmet_port(item)->disc_addr.adrfam;
-	int i;
-
-	for (i = 1; i < ARRAY_SIZE(nvmet_addr_family); i++) {
-		if (nvmet_addr_family[i].type == adrfam)
-			return sprintf(page, "%s\n", nvmet_addr_family[i].name);
-	}
 
-	return sprintf(page, "\n");
+	return sprintf(page, "%s\n", nvme_adrfam_type_to_name(adrfam));
 }
 
 static ssize_t nvmet_addr_adrfam_store(struct config_item *item,
@@ -75,8 +48,8 @@ static ssize_t nvmet_addr_adrfam_store(struct config_item *item,
 	if (nvmet_is_port_enabled(port, __func__))
 		return -EACCES;
 
-	for (i = 1; i < ARRAY_SIZE(nvmet_addr_family); i++) {
-		if (sysfs_streq(page, nvmet_addr_family[i].name))
+	for (i = 1; i < ARRAY_SIZE(nvme_addr_family); i++) {
+		if (sysfs_streq(page, nvme_addr_family[i].name))
 			goto found;
 	}
 
@@ -84,7 +57,7 @@ static ssize_t nvmet_addr_adrfam_store(struct config_item *item,
 	return -EINVAL;
 
 found:
-	port->disc_addr.adrfam = nvmet_addr_family[i].type;
+	port->disc_addr.adrfam = nvme_addr_family[i].type;
 	return count;
 }
 
@@ -148,24 +121,12 @@ static ssize_t nvmet_addr_traddr_store(struct config_item *item,
 
 CONFIGFS_ATTR(nvmet_, addr_traddr);
 
-static const struct nvmet_type_name_map nvmet_addr_treq[] = {
-	{ NVMF_TREQ_NOT_SPECIFIED,	"not specified" },
-	{ NVMF_TREQ_REQUIRED,		"required" },
-	{ NVMF_TREQ_NOT_REQUIRED,	"not required" },
-};
-
 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;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(nvmet_addr_treq); i++) {
-		if (treq == nvmet_addr_treq[i].type)
-			return sprintf(page, "%s\n", nvmet_addr_treq[i].name);
-	}
 
-	return sprintf(page, "\n");
+	return sprintf(page, "%s\n", nvme_treq_type_to_name(treq));
 }
 
 static ssize_t nvmet_addr_treq_store(struct config_item *item,
@@ -178,8 +139,8 @@ static ssize_t nvmet_addr_treq_store(struct config_item *item,
 	if (nvmet_is_port_enabled(port, __func__))
 		return -EACCES;
 
-	for (i = 0; i < ARRAY_SIZE(nvmet_addr_treq); i++) {
-		if (sysfs_streq(page, nvmet_addr_treq[i].name))
+	for (i = 0; i < ARRAY_SIZE(nvme_addr_treq); i++) {
+		if (sysfs_streq(page, nvme_addr_treq[i].name))
 			goto found;
 	}
 
@@ -187,7 +148,7 @@ static ssize_t nvmet_addr_treq_store(struct config_item *item,
 	return -EINVAL;
 
 found:
-	treq |= nvmet_addr_treq[i].type;
+	treq |= nvme_addr_treq[i].type;
 	port->disc_addr.treq = treq;
 	return count;
 }
@@ -282,14 +243,9 @@ static ssize_t nvmet_addr_trtype_show(struct config_item *item,
 		char *page)
 {
 	struct nvmet_port *port = to_nvmet_port(item);
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(nvmet_transport); i++) {
-		if (port->disc_addr.trtype == nvmet_transport[i].type)
-			return sprintf(page, "%s\n", nvmet_transport[i].name);
-	}
 
-	return sprintf(page, "\n");
+	return sprintf(page, "%s\n",
+		       nvme_trtype_to_name(port->disc_addr.trtype));
 }
 
 static void nvmet_port_init_tsas_rdma(struct nvmet_port *port)
@@ -308,8 +264,8 @@ static ssize_t nvmet_addr_trtype_store(struct config_item *item,
 	if (nvmet_is_port_enabled(port, __func__))
 		return -EACCES;
 
-	for (i = 0; i < ARRAY_SIZE(nvmet_transport); i++) {
-		if (sysfs_streq(page, nvmet_transport[i].name))
+	for (i = 0; i < ARRAY_SIZE(nvme_transport); i++) {
+		if (sysfs_streq(page, nvme_transport[i].name))
 			goto found;
 	}
 
@@ -318,7 +274,7 @@ static ssize_t nvmet_addr_trtype_store(struct config_item *item,
 
 found:
 	memset(&port->disc_addr.tsas, 0, NVMF_TSAS_SIZE);
-	port->disc_addr.trtype = nvmet_transport[i].type;
+	port->disc_addr.trtype = nvme_transport[i].type;
 	if (port->disc_addr.trtype == NVMF_TRTYPE_RDMA)
 		nvmet_port_init_tsas_rdma(port);
 	return count;
@@ -1227,27 +1183,13 @@ static struct config_group *nvmet_referral_make(
 	.ct_group_ops	= &nvmet_referral_group_ops,
 };
 
-static struct nvmet_type_name_map nvmet_ana_state[] = {
-	{ NVME_ANA_OPTIMIZED,		"optimized" },
-	{ NVME_ANA_NONOPTIMIZED,	"non-optimized" },
-	{ NVME_ANA_INACCESSIBLE,	"inaccessible" },
-	{ NVME_ANA_PERSISTENT_LOSS,	"persistent-loss" },
-	{ NVME_ANA_CHANGE,		"change" },
-};
-
 static ssize_t nvmet_ana_group_ana_state_show(struct config_item *item,
 		char *page)
 {
 	struct nvmet_ana_group *grp = to_ana_group(item);
 	enum nvme_ana_state state = grp->port->ana_state[grp->grpid];
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(nvmet_ana_state); i++) {
-		if (state == nvmet_ana_state[i].type)
-			return sprintf(page, "%s\n", nvmet_ana_state[i].name);
-	}
 
-	return sprintf(page, "\n");
+	return sprintf(page, "%s\n", nvme_ana_type_to_name(state));
 }
 
 static ssize_t nvmet_ana_group_ana_state_store(struct config_item *item,
@@ -1257,8 +1199,8 @@ static ssize_t nvmet_ana_group_ana_state_store(struct config_item *item,
 	enum nvme_ana_state *ana_state = grp->port->ana_state;
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(nvmet_ana_state); i++) {
-		if (sysfs_streq(page, nvmet_ana_state[i].name))
+	for (i = 0; i < ARRAY_SIZE(nvme_ana_states); i++) {
+		if (sysfs_streq(page, nvme_ana_states[i].name))
 			goto found;
 	}
 
@@ -1267,7 +1209,7 @@ static ssize_t nvmet_ana_group_ana_state_store(struct config_item *item,
 
 found:
 	down_write(&nvmet_ana_sem);
-	ana_state[grp->grpid] = (enum nvme_ana_state) nvmet_ana_state[i].type;
+	ana_state[grp->grpid] = (enum nvme_ana_state) nvme_ana_states[i].type;
 	nvmet_ana_chgcnt++;
 	up_write(&nvmet_ana_sem);
 	nvmet_port_send_ana_event(grp->port);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 8096912..ce57d5f 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -15,6 +15,7 @@
 #include <linux/mutex.h>
 #include <linux/uuid.h>
 #include <linux/nvme.h>
+#include <linux/nvme-types.h>
 #include <linux/configfs.h>
 #include <linux/rcupdate.h>
 #include <linux/blkdev.h>
diff --git a/include/linux/nvme-types.h b/include/linux/nvme-types.h
new file mode 100644
index 0000000..38ed980
--- /dev/null
+++ b/include/linux/nvme-types.h
@@ -0,0 +1,95 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2020 Mellanox Technologies. All rights reserved.
+ */
+
+#ifndef _LINUX_NVME_TYPES_H
+#define _LINUX_NVME_TYPES_H
+
+#include <linux/kernel.h>
+#include <linux/nvme.h>
+
+struct nvme_type_name_map {
+	u8		type;
+	const char	*name;
+};
+
+static const struct nvme_type_name_map nvme_transport[] = {
+	{ NVMF_TRTYPE_RDMA,	"rdma" },
+	{ NVMF_TRTYPE_FC,	"fc" },
+	{ NVMF_TRTYPE_TCP,	"tcp" },
+	{ NVMF_TRTYPE_LOOP,	"loop" },
+};
+
+static inline const char *nvme_trtype_to_name(u8 type)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(nvme_transport); i++) {
+		if (type == nvme_transport[i].type)
+			return nvme_transport[i].name;
+	}
+
+	return "invalid";
+}
+
+static const struct nvme_type_name_map nvme_addr_family[] = {
+	{ NVMF_ADDR_FAMILY_PCI,		"pcie" },
+	{ NVMF_ADDR_FAMILY_IP4,		"ipv4" },
+	{ NVMF_ADDR_FAMILY_IP6,		"ipv6" },
+	{ NVMF_ADDR_FAMILY_IB,		"ib" },
+	{ NVMF_ADDR_FAMILY_FC,		"fc" },
+	{ NVMF_ADDR_FAMILY_LOOP,	"loop" },
+};
+
+static inline const char *nvme_adrfam_type_to_name(u8 type)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(nvme_addr_family); i++) {
+		if (type == nvme_addr_family[i].type)
+			return nvme_addr_family[i].name;
+	}
+
+	return "invalid";
+}
+
+static const struct nvme_type_name_map nvme_addr_treq[] = {
+	{ NVMF_TREQ_NOT_SPECIFIED,	"not specified" },
+	{ NVMF_TREQ_REQUIRED,		"required" },
+	{ NVMF_TREQ_NOT_REQUIRED,	"not required" },
+};
+
+static inline const char *nvme_treq_type_to_name(u8 type)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(nvme_addr_treq); i++) {
+		if (type == nvme_addr_treq[i].type)
+			return nvme_addr_treq[i].name;
+	}
+
+	return "invalid";
+}
+
+static const struct nvme_type_name_map nvme_ana_states[] = {
+	{ NVME_ANA_OPTIMIZED,		"optimized" },
+	{ NVME_ANA_NONOPTIMIZED,	"non-optimized" },
+	{ NVME_ANA_INACCESSIBLE,	"inaccessible" },
+	{ NVME_ANA_PERSISTENT_LOSS,	"persistent-loss" },
+	{ NVME_ANA_CHANGE,		"change" },
+};
+
+static inline const char *nvme_ana_type_to_name(u8 type)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(nvme_ana_states); i++) {
+		if (type == nvme_ana_states[i].type)
+			return nvme_ana_states[i].name;
+	}
+
+	return "invalid";
+}
+
+#endif /* _LINUX_NVME_TYPES_H */
-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/5] nvme: introduce nvme-types header file

[Himanshu Madhani]

On 6/2/20 8:15 AM, Max Gurtovoy wrote:
> Centralize the mapping between name and type to a common header file
> instead of duplicating logic in both NVMe host and target drivers.
> 
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> Reviewed-by: Israel Rukshin <israelr@mellanox.com>
> ---
>   drivers/nvme/target/configfs.c | 92 ++++++++--------------------------------
>   drivers/nvme/target/nvmet.h    |  1 +
>   include/linux/nvme-types.h     | 95 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 113 insertions(+), 75 deletions(-)
>   create mode 100644 include/linux/nvme-types.h
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 419e0d4..daeb17e 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -20,27 +20,6 @@
>   static LIST_HEAD(nvmet_ports_list);
>   struct list_head *nvmet_ports = &nvmet_ports_list;
>   
> -struct nvmet_type_name_map {
> -	u8		type;
> -	const char	*name;
> -};
> -
> -static struct nvmet_type_name_map nvmet_transport[] = {
> -	{ NVMF_TRTYPE_RDMA,	"rdma" },
> -	{ NVMF_TRTYPE_FC,	"fc" },
> -	{ NVMF_TRTYPE_TCP,	"tcp" },
> -	{ NVMF_TRTYPE_LOOP,	"loop" },
> -};
> -
> -static const struct nvmet_type_name_map nvmet_addr_family[] = {
> -	{ NVMF_ADDR_FAMILY_PCI,		"pcie" },
> -	{ NVMF_ADDR_FAMILY_IP4,		"ipv4" },
> -	{ NVMF_ADDR_FAMILY_IP6,		"ipv6" },
> -	{ NVMF_ADDR_FAMILY_IB,		"ib" },
> -	{ NVMF_ADDR_FAMILY_FC,		"fc" },
> -	{ NVMF_ADDR_FAMILY_LOOP,	"loop" },
> -};
> -
>   static bool nvmet_is_port_enabled(struct nvmet_port *p, const char *caller)
>   {
>   	if (p->enabled)
> @@ -56,14 +35,8 @@ static bool nvmet_is_port_enabled(struct nvmet_port *p, const char *caller)
>   static ssize_t nvmet_addr_adrfam_show(struct config_item *item, char *page)
>   {
>   	u8 adrfam = to_nvmet_port(item)->disc_addr.adrfam;
> -	int i;
> -
> -	for (i = 1; i < ARRAY_SIZE(nvmet_addr_family); i++) {
> -		if (nvmet_addr_family[i].type == adrfam)
> -			return sprintf(page, "%s\n", nvmet_addr_family[i].name);
> -	}
>   
> -	return sprintf(page, "\n");
> +	return sprintf(page, "%s\n", nvme_adrfam_type_to_name(adrfam));
>   }
>   
>   static ssize_t nvmet_addr_adrfam_store(struct config_item *item,
> @@ -75,8 +48,8 @@ static ssize_t nvmet_addr_adrfam_store(struct config_item *item,
>   	if (nvmet_is_port_enabled(port, __func__))
>   		return -EACCES;
>   
> -	for (i = 1; i < ARRAY_SIZE(nvmet_addr_family); i++) {
> -		if (sysfs_streq(page, nvmet_addr_family[i].name))
> +	for (i = 1; i < ARRAY_SIZE(nvme_addr_family); i++) {
> +		if (sysfs_streq(page, nvme_addr_family[i].name))
>   			goto found;
>   	}
>   
> @@ -84,7 +57,7 @@ static ssize_t nvmet_addr_adrfam_store(struct config_item *item,
>   	return -EINVAL;
>   
>   found:
> -	port->disc_addr.adrfam = nvmet_addr_family[i].type;
> +	port->disc_addr.adrfam = nvme_addr_family[i].type;
>   	return count;
>   }
>   
> @@ -148,24 +121,12 @@ static ssize_t nvmet_addr_traddr_store(struct config_item *item,
>   
>   CONFIGFS_ATTR(nvmet_, addr_traddr);
>   
> -static const struct nvmet_type_name_map nvmet_addr_treq[] = {
> -	{ NVMF_TREQ_NOT_SPECIFIED,	"not specified" },
> -	{ NVMF_TREQ_REQUIRED,		"required" },
> -	{ NVMF_TREQ_NOT_REQUIRED,	"not required" },
> -};
> -
>   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;
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(nvmet_addr_treq); i++) {
> -		if (treq == nvmet_addr_treq[i].type)
> -			return sprintf(page, "%s\n", nvmet_addr_treq[i].name);
> -	}
>   
> -	return sprintf(page, "\n");
> +	return sprintf(page, "%s\n", nvme_treq_type_to_name(treq));
>   }
>   
>   static ssize_t nvmet_addr_treq_store(struct config_item *item,
> @@ -178,8 +139,8 @@ static ssize_t nvmet_addr_treq_store(struct config_item *item,
>   	if (nvmet_is_port_enabled(port, __func__))
>   		return -EACCES;
>   
> -	for (i = 0; i < ARRAY_SIZE(nvmet_addr_treq); i++) {
> -		if (sysfs_streq(page, nvmet_addr_treq[i].name))
> +	for (i = 0; i < ARRAY_SIZE(nvme_addr_treq); i++) {
> +		if (sysfs_streq(page, nvme_addr_treq[i].name))
>   			goto found;
>   	}
>   
> @@ -187,7 +148,7 @@ static ssize_t nvmet_addr_treq_store(struct config_item *item,
>   	return -EINVAL;
>   
>   found:
> -	treq |= nvmet_addr_treq[i].type;
> +	treq |= nvme_addr_treq[i].type;
>   	port->disc_addr.treq = treq;
>   	return count;
>   }
> @@ -282,14 +243,9 @@ static ssize_t nvmet_addr_trtype_show(struct config_item *item,
>   		char *page)
>   {
>   	struct nvmet_port *port = to_nvmet_port(item);
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(nvmet_transport); i++) {
> -		if (port->disc_addr.trtype == nvmet_transport[i].type)
> -			return sprintf(page, "%s\n", nvmet_transport[i].name);
> -	}
>   
> -	return sprintf(page, "\n");
> +	return sprintf(page, "%s\n",
> +		       nvme_trtype_to_name(port->disc_addr.trtype));
>   }
>   
>   static void nvmet_port_init_tsas_rdma(struct nvmet_port *port)
> @@ -308,8 +264,8 @@ static ssize_t nvmet_addr_trtype_store(struct config_item *item,
>   	if (nvmet_is_port_enabled(port, __func__))
>   		return -EACCES;
>   
> -	for (i = 0; i < ARRAY_SIZE(nvmet_transport); i++) {
> -		if (sysfs_streq(page, nvmet_transport[i].name))
> +	for (i = 0; i < ARRAY_SIZE(nvme_transport); i++) {
> +		if (sysfs_streq(page, nvme_transport[i].name))
>   			goto found;
>   	}
>   
> @@ -318,7 +274,7 @@ static ssize_t nvmet_addr_trtype_store(struct config_item *item,
>   
>   found:
>   	memset(&port->disc_addr.tsas, 0, NVMF_TSAS_SIZE);
> -	port->disc_addr.trtype = nvmet_transport[i].type;
> +	port->disc_addr.trtype = nvme_transport[i].type;
>   	if (port->disc_addr.trtype == NVMF_TRTYPE_RDMA)
>   		nvmet_port_init_tsas_rdma(port);
>   	return count;
> @@ -1227,27 +1183,13 @@ static struct config_group *nvmet_referral_make(
>   	.ct_group_ops	= &nvmet_referral_group_ops,
>   };
>   
> -static struct nvmet_type_name_map nvmet_ana_state[] = {
> -	{ NVME_ANA_OPTIMIZED,		"optimized" },
> -	{ NVME_ANA_NONOPTIMIZED,	"non-optimized" },
> -	{ NVME_ANA_INACCESSIBLE,	"inaccessible" },
> -	{ NVME_ANA_PERSISTENT_LOSS,	"persistent-loss" },
> -	{ NVME_ANA_CHANGE,		"change" },
> -};
> -
>   static ssize_t nvmet_ana_group_ana_state_show(struct config_item *item,
>   		char *page)
>   {
>   	struct nvmet_ana_group *grp = to_ana_group(item);
>   	enum nvme_ana_state state = grp->port->ana_state[grp->grpid];
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(nvmet_ana_state); i++) {
> -		if (state == nvmet_ana_state[i].type)
> -			return sprintf(page, "%s\n", nvmet_ana_state[i].name);
> -	}
>   
> -	return sprintf(page, "\n");
> +	return sprintf(page, "%s\n", nvme_ana_type_to_name(state));
>   }
>   
>   static ssize_t nvmet_ana_group_ana_state_store(struct config_item *item,
> @@ -1257,8 +1199,8 @@ static ssize_t nvmet_ana_group_ana_state_store(struct config_item *item,
>   	enum nvme_ana_state *ana_state = grp->port->ana_state;
>   	int i;
>   
> -	for (i = 0; i < ARRAY_SIZE(nvmet_ana_state); i++) {
> -		if (sysfs_streq(page, nvmet_ana_state[i].name))
> +	for (i = 0; i < ARRAY_SIZE(nvme_ana_states); i++) {
> +		if (sysfs_streq(page, nvme_ana_states[i].name))
>   			goto found;
>   	}
>   
> @@ -1267,7 +1209,7 @@ static ssize_t nvmet_ana_group_ana_state_store(struct config_item *item,
>   
>   found:
>   	down_write(&nvmet_ana_sem);
> -	ana_state[grp->grpid] = (enum nvme_ana_state) nvmet_ana_state[i].type;
> +	ana_state[grp->grpid] = (enum nvme_ana_state) nvme_ana_states[i].type;
>   	nvmet_ana_chgcnt++;
>   	up_write(&nvmet_ana_sem);
>   	nvmet_port_send_ana_event(grp->port);
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 8096912..ce57d5f 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -15,6 +15,7 @@
>   #include <linux/mutex.h>
>   #include <linux/uuid.h>
>   #include <linux/nvme.h>
> +#include <linux/nvme-types.h>
>   #include <linux/configfs.h>
>   #include <linux/rcupdate.h>
>   #include <linux/blkdev.h>
> diff --git a/include/linux/nvme-types.h b/include/linux/nvme-types.h
> new file mode 100644
> index 0000000..38ed980
> --- /dev/null
> +++ b/include/linux/nvme-types.h
> @@ -0,0 +1,95 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2020 Mellanox Technologies. All rights reserved.
> + */
> +
> +#ifndef _LINUX_NVME_TYPES_H
> +#define _LINUX_NVME_TYPES_H
> +
> +#include <linux/kernel.h>
> +#include <linux/nvme.h>
> +
> +struct nvme_type_name_map {
> +	u8		type;
> +	const char	*name;
> +};
> +
> +static const struct nvme_type_name_map nvme_transport[] = {
> +	{ NVMF_TRTYPE_RDMA,	"rdma" },
> +	{ NVMF_TRTYPE_FC,	"fc" },
> +	{ NVMF_TRTYPE_TCP,	"tcp" },
> +	{ NVMF_TRTYPE_LOOP,	"loop" },
> +};
> +
> +static inline const char *nvme_trtype_to_name(u8 type)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(nvme_transport); i++) {
> +		if (type == nvme_transport[i].type)
> +			return nvme_transport[i].name;
> +	}
> +
> +	return "invalid";
> +}
> +
> +static const struct nvme_type_name_map nvme_addr_family[] = {
> +	{ NVMF_ADDR_FAMILY_PCI,		"pcie" },
> +	{ NVMF_ADDR_FAMILY_IP4,		"ipv4" },
> +	{ NVMF_ADDR_FAMILY_IP6,		"ipv6" },
> +	{ NVMF_ADDR_FAMILY_IB,		"ib" },
> +	{ NVMF_ADDR_FAMILY_FC,		"fc" },
> +	{ NVMF_ADDR_FAMILY_LOOP,	"loop" },
> +};
> +
> +static inline const char *nvme_adrfam_type_to_name(u8 type)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(nvme_addr_family); i++) {
> +		if (type == nvme_addr_family[i].type)
> +			return nvme_addr_family[i].name;
> +	}
> +
> +	return "invalid";
> +}
> +
> +static const struct nvme_type_name_map nvme_addr_treq[] = {
> +	{ NVMF_TREQ_NOT_SPECIFIED,	"not specified" },
> +	{ NVMF_TREQ_REQUIRED,		"required" },
> +	{ NVMF_TREQ_NOT_REQUIRED,	"not required" },
> +};
> +
> +static inline const char *nvme_treq_type_to_name(u8 type)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(nvme_addr_treq); i++) {
> +		if (type == nvme_addr_treq[i].type)
> +			return nvme_addr_treq[i].name;
> +	}
> +
> +	return "invalid";
> +}
> +
> +static const struct nvme_type_name_map nvme_ana_states[] = {
> +	{ NVME_ANA_OPTIMIZED,		"optimized" },
> +	{ NVME_ANA_NONOPTIMIZED,	"non-optimized" },
> +	{ NVME_ANA_INACCESSIBLE,	"inaccessible" },
> +	{ NVME_ANA_PERSISTENT_LOSS,	"persistent-loss" },
> +	{ NVME_ANA_CHANGE,		"change" },
> +};
> +
> +static inline const char *nvme_ana_type_to_name(u8 type)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(nvme_ana_states); i++) {
> +		if (type == nvme_ana_states[i].type)
> +			return nvme_ana_states[i].name;
> +	}
> +
> +	return "invalid";
> +}
> +
> +#endif /* _LINUX_NVME_TYPES_H */
> 
Looks Good.

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

-- 
Himanshu Madhani                     Oracle Linux Engineering

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/5] nvme: introduce nvme-types header file

[Sagi Grimberg]

On 6/2/20 6:15 AM, Max Gurtovoy wrote:
> Centralize the mapping between name and type to a common header file
> instead of duplicating logic in both NVMe host and target drivers.
> 
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> Reviewed-by: Israel Rukshin <israelr@mellanox.com>
> ---
>   drivers/nvme/target/configfs.c | 92 ++++++++--------------------------------
>   drivers/nvme/target/nvmet.h    |  1 +
>   include/linux/nvme-types.h     | 95 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 113 insertions(+), 75 deletions(-)
>   create mode 100644 include/linux/nvme-types.h
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 419e0d4..daeb17e 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -20,27 +20,6 @@
>   static LIST_HEAD(nvmet_ports_list);
>   struct list_head *nvmet_ports = &nvmet_ports_list;
>   
> -struct nvmet_type_name_map {
> -	u8		type;
> -	const char	*name;
> -};
> -
> -static struct nvmet_type_name_map nvmet_transport[] = {
> -	{ NVMF_TRTYPE_RDMA,	"rdma" },
> -	{ NVMF_TRTYPE_FC,	"fc" },
> -	{ NVMF_TRTYPE_TCP,	"tcp" },
> -	{ NVMF_TRTYPE_LOOP,	"loop" },
> -};
> -
> -static const struct nvmet_type_name_map nvmet_addr_family[] = {
> -	{ NVMF_ADDR_FAMILY_PCI,		"pcie" },
> -	{ NVMF_ADDR_FAMILY_IP4,		"ipv4" },
> -	{ NVMF_ADDR_FAMILY_IP6,		"ipv6" },
> -	{ NVMF_ADDR_FAMILY_IB,		"ib" },
> -	{ NVMF_ADDR_FAMILY_FC,		"fc" },
> -	{ NVMF_ADDR_FAMILY_LOOP,	"loop" },
> -};
> -
>   static bool nvmet_is_port_enabled(struct nvmet_port *p, const char *caller)
>   {
>   	if (p->enabled)
> @@ -56,14 +35,8 @@ static bool nvmet_is_port_enabled(struct nvmet_port *p, const char *caller)
>   static ssize_t nvmet_addr_adrfam_show(struct config_item *item, char *page)
>   {
>   	u8 adrfam = to_nvmet_port(item)->disc_addr.adrfam;
> -	int i;
> -
> -	for (i = 1; i < ARRAY_SIZE(nvmet_addr_family); i++) {
> -		if (nvmet_addr_family[i].type == adrfam)
> -			return sprintf(page, "%s\n", nvmet_addr_family[i].name);
> -	}
>   
> -	return sprintf(page, "\n");
> +	return sprintf(page, "%s\n", nvme_adrfam_type_to_name(adrfam));
>   }
>   
>   static ssize_t nvmet_addr_adrfam_store(struct config_item *item,
> @@ -75,8 +48,8 @@ static ssize_t nvmet_addr_adrfam_store(struct config_item *item,
>   	if (nvmet_is_port_enabled(port, __func__))
>   		return -EACCES;
>   
> -	for (i = 1; i < ARRAY_SIZE(nvmet_addr_family); i++) {
> -		if (sysfs_streq(page, nvmet_addr_family[i].name))
> +	for (i = 1; i < ARRAY_SIZE(nvme_addr_family); i++) {
> +		if (sysfs_streq(page, nvme_addr_family[i].name))
>   			goto found;
>   	}
>   
> @@ -84,7 +57,7 @@ static ssize_t nvmet_addr_adrfam_store(struct config_item *item,
>   	return -EINVAL;
>   
>   found:
> -	port->disc_addr.adrfam = nvmet_addr_family[i].type;
> +	port->disc_addr.adrfam = nvme_addr_family[i].type;
>   	return count;
>   }
>   
> @@ -148,24 +121,12 @@ static ssize_t nvmet_addr_traddr_store(struct config_item *item,
>   
>   CONFIGFS_ATTR(nvmet_, addr_traddr);
>   
> -static const struct nvmet_type_name_map nvmet_addr_treq[] = {
> -	{ NVMF_TREQ_NOT_SPECIFIED,	"not specified" },
> -	{ NVMF_TREQ_REQUIRED,		"required" },
> -	{ NVMF_TREQ_NOT_REQUIRED,	"not required" },
> -};
> -
>   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;
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(nvmet_addr_treq); i++) {
> -		if (treq == nvmet_addr_treq[i].type)
> -			return sprintf(page, "%s\n", nvmet_addr_treq[i].name);
> -	}
>   
> -	return sprintf(page, "\n");
> +	return sprintf(page, "%s\n", nvme_treq_type_to_name(treq));
>   }
>   
>   static ssize_t nvmet_addr_treq_store(struct config_item *item,
> @@ -178,8 +139,8 @@ static ssize_t nvmet_addr_treq_store(struct config_item *item,
>   	if (nvmet_is_port_enabled(port, __func__))
>   		return -EACCES;
>   
> -	for (i = 0; i < ARRAY_SIZE(nvmet_addr_treq); i++) {
> -		if (sysfs_streq(page, nvmet_addr_treq[i].name))
> +	for (i = 0; i < ARRAY_SIZE(nvme_addr_treq); i++) {
> +		if (sysfs_streq(page, nvme_addr_treq[i].name))
>   			goto found;
>   	}
>   
> @@ -187,7 +148,7 @@ static ssize_t nvmet_addr_treq_store(struct config_item *item,
>   	return -EINVAL;
>   
>   found:
> -	treq |= nvmet_addr_treq[i].type;
> +	treq |= nvme_addr_treq[i].type;
>   	port->disc_addr.treq = treq;
>   	return count;
>   }
> @@ -282,14 +243,9 @@ static ssize_t nvmet_addr_trtype_show(struct config_item *item,
>   		char *page)
>   {
>   	struct nvmet_port *port = to_nvmet_port(item);
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(nvmet_transport); i++) {
> -		if (port->disc_addr.trtype == nvmet_transport[i].type)
> -			return sprintf(page, "%s\n", nvmet_transport[i].name);
> -	}
>   
> -	return sprintf(page, "\n");
> +	return sprintf(page, "%s\n",
> +		       nvme_trtype_to_name(port->disc_addr.trtype));
>   }
>   
>   static void nvmet_port_init_tsas_rdma(struct nvmet_port *port)
> @@ -308,8 +264,8 @@ static ssize_t nvmet_addr_trtype_store(struct config_item *item,
>   	if (nvmet_is_port_enabled(port, __func__))
>   		return -EACCES;
>   
> -	for (i = 0; i < ARRAY_SIZE(nvmet_transport); i++) {
> -		if (sysfs_streq(page, nvmet_transport[i].name))
> +	for (i = 0; i < ARRAY_SIZE(nvme_transport); i++) {
> +		if (sysfs_streq(page, nvme_transport[i].name))
>   			goto found;
>   	}
>   
> @@ -318,7 +274,7 @@ static ssize_t nvmet_addr_trtype_store(struct config_item *item,
>   
>   found:
>   	memset(&port->disc_addr.tsas, 0, NVMF_TSAS_SIZE);
> -	port->disc_addr.trtype = nvmet_transport[i].type;
> +	port->disc_addr.trtype = nvme_transport[i].type;
>   	if (port->disc_addr.trtype == NVMF_TRTYPE_RDMA)
>   		nvmet_port_init_tsas_rdma(port);
>   	return count;
> @@ -1227,27 +1183,13 @@ static struct config_group *nvmet_referral_make(
>   	.ct_group_ops	= &nvmet_referral_group_ops,
>   };
>   
> -static struct nvmet_type_name_map nvmet_ana_state[] = {
> -	{ NVME_ANA_OPTIMIZED,		"optimized" },
> -	{ NVME_ANA_NONOPTIMIZED,	"non-optimized" },
> -	{ NVME_ANA_INACCESSIBLE,	"inaccessible" },
> -	{ NVME_ANA_PERSISTENT_LOSS,	"persistent-loss" },
> -	{ NVME_ANA_CHANGE,		"change" },
> -};
> -
>   static ssize_t nvmet_ana_group_ana_state_show(struct config_item *item,
>   		char *page)
>   {
>   	struct nvmet_ana_group *grp = to_ana_group(item);
>   	enum nvme_ana_state state = grp->port->ana_state[grp->grpid];
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(nvmet_ana_state); i++) {
> -		if (state == nvmet_ana_state[i].type)
> -			return sprintf(page, "%s\n", nvmet_ana_state[i].name);
> -	}
>   
> -	return sprintf(page, "\n");
> +	return sprintf(page, "%s\n", nvme_ana_type_to_name(state));
>   }
>   
>   static ssize_t nvmet_ana_group_ana_state_store(struct config_item *item,
> @@ -1257,8 +1199,8 @@ static ssize_t nvmet_ana_group_ana_state_store(struct config_item *item,
>   	enum nvme_ana_state *ana_state = grp->port->ana_state;
>   	int i;
>   
> -	for (i = 0; i < ARRAY_SIZE(nvmet_ana_state); i++) {
> -		if (sysfs_streq(page, nvmet_ana_state[i].name))
> +	for (i = 0; i < ARRAY_SIZE(nvme_ana_states); i++) {
> +		if (sysfs_streq(page, nvme_ana_states[i].name))
>   			goto found;
>   	}
>   
> @@ -1267,7 +1209,7 @@ static ssize_t nvmet_ana_group_ana_state_store(struct config_item *item,
>   
>   found:
>   	down_write(&nvmet_ana_sem);
> -	ana_state[grp->grpid] = (enum nvme_ana_state) nvmet_ana_state[i].type;
> +	ana_state[grp->grpid] = (enum nvme_ana_state) nvme_ana_states[i].type;
>   	nvmet_ana_chgcnt++;
>   	up_write(&nvmet_ana_sem);
>   	nvmet_port_send_ana_event(grp->port);
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 8096912..ce57d5f 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -15,6 +15,7 @@
>   #include <linux/mutex.h>
>   #include <linux/uuid.h>
>   #include <linux/nvme.h>
> +#include <linux/nvme-types.h>
>   #include <linux/configfs.h>
>   #include <linux/rcupdate.h>
>   #include <linux/blkdev.h>
> diff --git a/include/linux/nvme-types.h b/include/linux/nvme-types.h
> new file mode 100644
> index 0000000..38ed980
> --- /dev/null
> +++ b/include/linux/nvme-types.h
> @@ -0,0 +1,95 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2020 Mellanox Technologies. All rights reserved.
> + */
> +
> +#ifndef _LINUX_NVME_TYPES_H
> +#define _LINUX_NVME_TYPES_H
> +
> +#include <linux/kernel.h>
> +#include <linux/nvme.h>
> +
> +struct nvme_type_name_map {
> +	u8		type;
> +	const char	*name;
> +};
> +
> +static const struct nvme_type_name_map nvme_transport[] = {
> +	{ NVMF_TRTYPE_RDMA,	"rdma" },
> +	{ NVMF_TRTYPE_FC,	"fc" },
> +	{ NVMF_TRTYPE_TCP,	"tcp" },
> +	{ NVMF_TRTYPE_LOOP,	"loop" },
> +};
> +
> +static inline const char *nvme_trtype_to_name(u8 type)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(nvme_transport); i++) {
> +		if (type == nvme_transport[i].type)
> +			return nvme_transport[i].name;
> +	}
> +
> +	return "invalid";

Why does this need a loop?

maybe just:
	return (type < ARRAY_SIZE(nvme_transport) && nvme_transport[type]) ? 
nvme_transport[i].name : "invalid";

Same for the rest...

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/5] nvme: introduce nvme-types header file

[Max Gurtovoy]

On 6/4/2020 12:18 AM, Sagi Grimberg wrote:
>> +struct nvme_type_name_map {
>> +    u8        type;
>> +    const char    *name;
>> +};
>> +
>> +static const struct nvme_type_name_map nvme_transport[] = {
>> +    { NVMF_TRTYPE_RDMA,    "rdma" },
>> +    { NVMF_TRTYPE_FC,    "fc" },
>> +    { NVMF_TRTYPE_TCP,    "tcp" },
>> +    { NVMF_TRTYPE_LOOP,    "loop" },
>> +};
>> +
>> +static inline const char *nvme_trtype_to_name(u8 type)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(nvme_transport); i++) {
>> +        if (type == nvme_transport[i].type)
>> +            return nvme_transport[i].name;
>> +    }
>> +
>> +    return "invalid";
>
> Why does this need a loop?
>
> maybe just:
>     return (type < ARRAY_SIZE(nvme_transport) && nvme_transport[type]) 
> ? nvme_transport[i].name : "invalid";
>
> Same for the rest... 

It won't work since i != type.

I didn't change logic here, and mainly moved the target logic for 
type_name mappings to common code.



_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/5] nvme: introduce nvme-types header file

[Sagi Grimberg]

>> Why does this need a loop?
>>
>> maybe just:
>>     return (type < ARRAY_SIZE(nvme_transport) && nvme_transport[type]) 
>> ? nvme_transport[i].name : "invalid";
>>
>> Same for the rest... 
> 
> It won't work since i != type.

Yea, would be nice to fix it so we don't need the loop.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/5] nvme: introduce nvme-types header file

[Max Gurtovoy]

On 6/5/2020 11:00 AM, Sagi Grimberg wrote:
>
>>> Why does this need a loop?
>>>
>>> maybe just:
>>>     return (type < ARRAY_SIZE(nvme_transport) && 
>>> nvme_transport[type]) ? nvme_transport[i].name : "invalid";
>>>
>>> Same for the rest... 
>>
>> It won't work since i != type.
>
> Yea, would be nice to fix it so we don't need the loop.

I'm not sure that wasting memory on arrays that are bigger than needed 
is better.

NVMF_TRTYPE_LOOP        = 254

NVMF_ADDR_FAMILY_LOOP   = 254

I can create inline functions that have switch/case that will return 
type_to_name/name_to_type mappings instead of arrays (like we do in 
nvme-rdma.h).

is this better ?


>
> _______________________________________________
> linux-nvme mailing list
> linux-nvme@lists.infradead.org
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-nvme&amp;data=02%7C01%7Cmaxg%40mellanox.com%7C849cad0b92424000a08808d8092692ad%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637269408560493242&amp;sdata=y4PYjHBCWEzm6QzVIP%2FyhatTDRAVYvxMfztosIN8pzw%3D&amp;reserved=0 
>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/5] nvme: introduce nvme-types header file

[Christoph Hellwig]

On Tue, Jun 02, 2020 at 04:15:42PM +0300, Max Gurtovoy wrote:
> Centralize the mapping between name and type to a common header file
> instead of duplicating logic in both NVMe host and target drivers.

The idea looks ok to me, but nvme_types suggested a header ala
<sys/types.h> or <linux/types.h> to me.  Not sure what a good name
would be, though.  I also suspect that keeping it under drivers/nvme/
in some fork might make more sense.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/5] nvme: introduce nvme-types header file

[Max Gurtovoy]

On 6/9/2020 4:57 PM, Christoph Hellwig wrote:
> On Tue, Jun 02, 2020 at 04:15:42PM +0300, Max Gurtovoy wrote:
>> Centralize the mapping between name and type to a common header file
>> instead of duplicating logic in both NVMe host and target drivers.
> The idea looks ok to me, but nvme_types suggested a header ala
> <sys/types.h> or <linux/types.h> to me.  Not sure what a good name
> would be, though.  I also suspect that keeping it under drivers/nvme/
> in some fork might make more sense.

how about <linux/nvme-common.h>  that will be used for common code 
between host/target subsystems ?



_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 2/5] nvme: use nvme_ana_type_to_name to get state string

[Max Gurtovoy]

Remove code duplication from nvme-core.ko and use common function to
translate enum to ANA state string.

Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
Reviewed-by: Israel Rukshin <israelr@mellanox.com>
---
 drivers/nvme/host/multipath.c | 16 +++-------------
 drivers/nvme/host/nvme.h      |  1 +
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index da78e49..37cb9df 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -124,15 +124,6 @@ void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
 	up_read(&ctrl->namespaces_rwsem);
 }
 
-static const char *nvme_ana_state_names[] = {
-	[0]				= "invalid state",
-	[NVME_ANA_OPTIMIZED]		= "optimized",
-	[NVME_ANA_NONOPTIMIZED]		= "non-optimized",
-	[NVME_ANA_INACCESSIBLE]		= "inaccessible",
-	[NVME_ANA_PERSISTENT_LOSS]	= "persistent-loss",
-	[NVME_ANA_CHANGE]		= "change",
-};
-
 bool nvme_mpath_clear_current_path(struct nvme_ns *ns)
 {
 	struct nvme_ns_head *head = ns->head;
@@ -500,9 +491,8 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 	unsigned *nr_change_groups = data;
 	struct nvme_ns *ns;
 
-	dev_dbg(ctrl->device, "ANA group %d: %s.\n",
-			le32_to_cpu(desc->grpid),
-			nvme_ana_state_names[desc->state]);
+	dev_dbg(ctrl->device, "ANA group %d: %s.\n", le32_to_cpu(desc->grpid),
+		nvme_ana_type_to_name(desc->state));
 
 	if (desc->state == NVME_ANA_CHANGE)
 		(*nr_change_groups)++;
@@ -636,7 +626,7 @@ static ssize_t ana_state_show(struct device *dev, struct device_attribute *attr,
 {
 	struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
 
-	return sprintf(buf, "%s\n", nvme_ana_state_names[ns->ana_state]);
+	return sprintf(buf, "%s\n", nvme_ana_type_to_name(ns->ana_state));
 }
 DEVICE_ATTR_RO(ana_state);
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index fa5c755..eef0e88 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -7,6 +7,7 @@
 #define _NVME_H
 
 #include <linux/nvme.h>
+#include <linux/nvme-types.h>
 #include <linux/cdev.h>
 #include <linux/pci.h>
 #include <linux/kref.h>
-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 2/5] nvme: use nvme_ana_type_to_name to get state string

[Himanshu Madhani]

On 6/2/20 8:15 AM, Max Gurtovoy wrote:
> Remove code duplication from nvme-core.ko and use common function to
> translate enum to ANA state string.
> 
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> Reviewed-by: Israel Rukshin <israelr@mellanox.com>
> ---
>   drivers/nvme/host/multipath.c | 16 +++-------------
>   drivers/nvme/host/nvme.h      |  1 +
>   2 files changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index da78e49..37cb9df 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -124,15 +124,6 @@ void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
>   	up_read(&ctrl->namespaces_rwsem);
>   }
>   
> -static const char *nvme_ana_state_names[] = {
> -	[0]				= "invalid state",
> -	[NVME_ANA_OPTIMIZED]		= "optimized",
> -	[NVME_ANA_NONOPTIMIZED]		= "non-optimized",
> -	[NVME_ANA_INACCESSIBLE]		= "inaccessible",
> -	[NVME_ANA_PERSISTENT_LOSS]	= "persistent-loss",
> -	[NVME_ANA_CHANGE]		= "change",
> -};
> -
>   bool nvme_mpath_clear_current_path(struct nvme_ns *ns)
>   {
>   	struct nvme_ns_head *head = ns->head;
> @@ -500,9 +491,8 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
>   	unsigned *nr_change_groups = data;
>   	struct nvme_ns *ns;
>   
> -	dev_dbg(ctrl->device, "ANA group %d: %s.\n",
> -			le32_to_cpu(desc->grpid),
> -			nvme_ana_state_names[desc->state]);
> +	dev_dbg(ctrl->device, "ANA group %d: %s.\n", le32_to_cpu(desc->grpid),
> +		nvme_ana_type_to_name(desc->state));
>   
>   	if (desc->state == NVME_ANA_CHANGE)
>   		(*nr_change_groups)++;
> @@ -636,7 +626,7 @@ static ssize_t ana_state_show(struct device *dev, struct device_attribute *attr,
>   {
>   	struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
>   
> -	return sprintf(buf, "%s\n", nvme_ana_state_names[ns->ana_state]);
> +	return sprintf(buf, "%s\n", nvme_ana_type_to_name(ns->ana_state));
>   }
>   DEVICE_ATTR_RO(ana_state);
>   
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index fa5c755..eef0e88 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -7,6 +7,7 @@
>   #define _NVME_H
>   
>   #include <linux/nvme.h>
> +#include <linux/nvme-types.h>
>   #include <linux/cdev.h>
>   #include <linux/pci.h>
>   #include <linux/kref.h>
> 

Looks Good.

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

-- 
Himanshu Madhani                     Oracle Linux Engineering

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 3/5] nvme: replace transport name with trtype enum for ops

[Max Gurtovoy]

Use common code to map transport type to transport name instead of
duplicating logic.

Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
Reviewed-by: Israel Rukshin <israelr@mellanox.com>
---
 drivers/nvme/host/fabrics.c | 3 ++-
 drivers/nvme/host/fabrics.h | 4 ++--
 drivers/nvme/host/fc.c      | 2 +-
 drivers/nvme/host/rdma.c    | 2 +-
 drivers/nvme/host/tcp.c     | 2 +-
 drivers/nvme/target/loop.c  | 2 +-
 6 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 2a6c819..bd5e44e 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -528,7 +528,8 @@ static struct nvmf_transport_ops *nvmf_lookup_transport(
 	lockdep_assert_held(&nvmf_transports_rwsem);
 
 	list_for_each_entry(ops, &nvmf_transports, entry) {
-		if (strcmp(ops->name, opts->transport) == 0)
+		if (strcmp(nvme_trtype_to_name(ops->trtype),
+			   opts->transport) == 0)
 			return ops;
 	}
 
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index a0ec40a..1fd52f0 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -119,7 +119,7 @@ struct nvmf_ctrl_options {
  * @entry:		Used by the fabrics library to add the new
  *			registration entry to its linked-list internal tree.
  * @module:             Transport module reference
- * @name:		Name of the NVMe fabric driver implementation.
+ * @trtype:		NVMe fabric transport type
  * @required_opts:	sysfs command-line options that must be specified
  *			when adding a new NVMe controller.
  * @allowed_opts:	sysfs command-line options that can be specified
@@ -141,7 +141,7 @@ struct nvmf_ctrl_options {
 struct nvmf_transport_ops {
 	struct list_head	entry;
 	struct module		*module;
-	const char		*name;
+	u8			trtype;
 	int			required_opts;
 	int			allowed_opts;
 	struct nvme_ctrl	*(*create_ctrl)(struct device *dev,
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index cb00075..67db0ae 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3694,7 +3694,7 @@ struct nvmet_fc_traddr {
 
 
 static struct nvmf_transport_ops nvme_fc_transport = {
-	.name		= "fc",
+	.trtype		= NVMF_TRTYPE_FC,
 	.module		= THIS_MODULE,
 	.required_opts	= NVMF_OPT_TRADDR | NVMF_OPT_HOST_TRADDR,
 	.allowed_opts	= NVMF_OPT_RECONNECT_DELAY | NVMF_OPT_CTRL_LOSS_TMO,
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index f8f856d..5e7dda3 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2329,7 +2329,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 }
 
 static struct nvmf_transport_ops nvme_rdma_transport = {
-	.name		= "rdma",
+	.trtype		= NVMF_TRTYPE_RDMA,
 	.module		= THIS_MODULE,
 	.required_opts	= NVMF_OPT_TRADDR,
 	.allowed_opts	= NVMF_OPT_TRSVCID | NVMF_OPT_RECONNECT_DELAY |
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 7c7c188..c678f97 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2487,7 +2487,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
 }
 
 static struct nvmf_transport_ops nvme_tcp_transport = {
-	.name		= "tcp",
+	.trtype		= NVMF_TRTYPE_TCP,
 	.module		= THIS_MODULE,
 	.required_opts	= NVMF_OPT_TRADDR,
 	.allowed_opts	= NVMF_OPT_TRSVCID | NVMF_OPT_RECONNECT_DELAY |
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 0d54e73..257ee60 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -674,7 +674,7 @@ static void nvme_loop_remove_port(struct nvmet_port *port)
 };
 
 static struct nvmf_transport_ops nvme_loop_transport = {
-	.name		= "loop",
+	.trtype		= NVMF_TRTYPE_LOOP,
 	.module		= THIS_MODULE,
 	.create_ctrl	= nvme_loop_create_ctrl,
 	.allowed_opts	= NVMF_OPT_TRADDR,
-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 3/5] nvme: replace transport name with trtype enum for ops

[Himanshu Madhani]

On 6/2/20 8:15 AM, Max Gurtovoy wrote:
> Use common code to map transport type to transport name instead of
> duplicating logic.
> 
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> Reviewed-by: Israel Rukshin <israelr@mellanox.com>
> ---
>   drivers/nvme/host/fabrics.c | 3 ++-
>   drivers/nvme/host/fabrics.h | 4 ++--
>   drivers/nvme/host/fc.c      | 2 +-
>   drivers/nvme/host/rdma.c    | 2 +-
>   drivers/nvme/host/tcp.c     | 2 +-
>   drivers/nvme/target/loop.c  | 2 +-
>   6 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 2a6c819..bd5e44e 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -528,7 +528,8 @@ static struct nvmf_transport_ops *nvmf_lookup_transport(
>   	lockdep_assert_held(&nvmf_transports_rwsem);
>   
>   	list_for_each_entry(ops, &nvmf_transports, entry) {
> -		if (strcmp(ops->name, opts->transport) == 0)
> +		if (strcmp(nvme_trtype_to_name(ops->trtype),
> +			   opts->transport) == 0)
>   			return ops;
>   	}
>   
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index a0ec40a..1fd52f0 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -119,7 +119,7 @@ struct nvmf_ctrl_options {
>    * @entry:		Used by the fabrics library to add the new
>    *			registration entry to its linked-list internal tree.
>    * @module:             Transport module reference
> - * @name:		Name of the NVMe fabric driver implementation.
> + * @trtype:		NVMe fabric transport type
>    * @required_opts:	sysfs command-line options that must be specified
>    *			when adding a new NVMe controller.
>    * @allowed_opts:	sysfs command-line options that can be specified
> @@ -141,7 +141,7 @@ struct nvmf_ctrl_options {
>   struct nvmf_transport_ops {
>   	struct list_head	entry;
>   	struct module		*module;
> -	const char		*name;
> +	u8			trtype;
>   	int			required_opts;
>   	int			allowed_opts;
>   	struct nvme_ctrl	*(*create_ctrl)(struct device *dev,
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index cb00075..67db0ae 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -3694,7 +3694,7 @@ struct nvmet_fc_traddr {
>   
>   
>   static struct nvmf_transport_ops nvme_fc_transport = {
> -	.name		= "fc",
> +	.trtype		= NVMF_TRTYPE_FC,
>   	.module		= THIS_MODULE,
>   	.required_opts	= NVMF_OPT_TRADDR | NVMF_OPT_HOST_TRADDR,
>   	.allowed_opts	= NVMF_OPT_RECONNECT_DELAY | NVMF_OPT_CTRL_LOSS_TMO,
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index f8f856d..5e7dda3 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -2329,7 +2329,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
>   }
>   
>   static struct nvmf_transport_ops nvme_rdma_transport = {
> -	.name		= "rdma",
> +	.trtype		= NVMF_TRTYPE_RDMA,
>   	.module		= THIS_MODULE,
>   	.required_opts	= NVMF_OPT_TRADDR,
>   	.allowed_opts	= NVMF_OPT_TRSVCID | NVMF_OPT_RECONNECT_DELAY |
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 7c7c188..c678f97 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2487,7 +2487,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
>   }
>   
>   static struct nvmf_transport_ops nvme_tcp_transport = {
> -	.name		= "tcp",
> +	.trtype		= NVMF_TRTYPE_TCP,
>   	.module		= THIS_MODULE,
>   	.required_opts	= NVMF_OPT_TRADDR,
>   	.allowed_opts	= NVMF_OPT_TRSVCID | NVMF_OPT_RECONNECT_DELAY |
> diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
> index 0d54e73..257ee60 100644
> --- a/drivers/nvme/target/loop.c
> +++ b/drivers/nvme/target/loop.c
> @@ -674,7 +674,7 @@ static void nvme_loop_remove_port(struct nvmet_port *port)
>   };
>   
>   static struct nvmf_transport_ops nvme_loop_transport = {
> -	.name		= "loop",
> +	.trtype		= NVMF_TRTYPE_LOOP,
>   	.module		= THIS_MODULE,
>   	.create_ctrl	= nvme_loop_create_ctrl,
>   	.allowed_opts	= NVMF_OPT_TRADDR,
> 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

-- 
Himanshu Madhani                     Oracle Linux Engineering

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 3/5] nvme: replace transport name with trtype enum for ops

[Sagi Grimberg]

> Use common code to map transport type to transport name instead of
> duplicating logic.

I'd prefer to keep the driver ops name and not rely on the trtype,
userspace passes a name so I'd rather keep the transport registered with
a name.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 3/5] nvme: replace transport name with trtype enum for ops

[Max Gurtovoy]

On 6/5/2020 7:49 PM, Sagi Grimberg wrote:
>
>> Use common code to map transport type to transport name instead of
>> duplicating logic.
>
> I'd prefer to keep the driver ops name and not rely on the trtype,
> userspace passes a name so I'd rather keep the transport registered with
> a name.

Userspace can continue passing names, but why not using common code ? as 
we do in the target side..


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 3/5] nvme: replace transport name with trtype enum for ops

[Sagi Grimberg]

>>> Use common code to map transport type to transport name instead of
>>> duplicating logic.
>>
>> I'd prefer to keep the driver ops name and not rely on the trtype,
>> userspace passes a name so I'd rather keep the transport registered with
>> a name.
> 
> Userspace can continue passing names, but why not using common code ? as 
> we do in the target side..

We can still use common code, but I'd like to keep the driver name in
the ops instead of trtype enumeration.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 4/5] nvmet-tcp: remove has_keyed_sgls initialization

[Max Gurtovoy]

Since the nvmet_tcp_ops is static, there is no need to initialize values
to zero.

Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
Reviewed-by: Israel Rukshin <israelr@mellanox.com>
---
 drivers/nvme/target/tcp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 9e4cb90..3099237 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1751,7 +1751,6 @@ static void nvmet_tcp_disc_port_addr(struct nvmet_req *req,
 	.owner			= THIS_MODULE,
 	.type			= NVMF_TRTYPE_TCP,
 	.msdbd			= 1,
-	.has_keyed_sgls		= 0,
 	.add_port		= nvmet_tcp_add_port,
 	.remove_port		= nvmet_tcp_remove_port,
 	.queue_response		= nvmet_tcp_queue_response,
-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 4/5] nvmet-tcp: remove has_keyed_sgls initialization

[Himanshu Madhani]

On 6/2/20 8:15 AM, Max Gurtovoy wrote:
> Since the nvmet_tcp_ops is static, there is no need to initialize values
> to zero.
> 
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> Reviewed-by: Israel Rukshin <israelr@mellanox.com>
> ---
>   drivers/nvme/target/tcp.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index 9e4cb90..3099237 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -1751,7 +1751,6 @@ static void nvmet_tcp_disc_port_addr(struct nvmet_req *req,
>   	.owner			= THIS_MODULE,
>   	.type			= NVMF_TRTYPE_TCP,
>   	.msdbd			= 1,
> -	.has_keyed_sgls		= 0,
>   	.add_port		= nvmet_tcp_add_port,
>   	.remove_port		= nvmet_tcp_remove_port,
>   	.queue_response		= nvmet_tcp_queue_response,
> 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

-- 
Himanshu Madhani                     Oracle Linux Engineering

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 5/5] nvmet: introduce flags member in nvmet_fabrics_ops

[Max Gurtovoy]

Replace has_keyed_sgls and metadata_support booleans with a flags member
that will be used for adding more features in the future.

Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
Reviewed-by: Israel Rukshin <israelr@mellanox.com>
---
 drivers/nvme/target/admin-cmd.c | 2 +-
 drivers/nvme/target/core.c      | 2 +-
 drivers/nvme/target/discovery.c | 2 +-
 drivers/nvme/target/nvmet.h     | 5 +++--
 drivers/nvme/target/rdma.c      | 3 +--
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 1db8c04..95bb3bc 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -427,7 +427,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	id->awupf = 0;
 
 	id->sgls = cpu_to_le32(1 << 0);	/* we always support SGLs */
-	if (ctrl->ops->has_keyed_sgls)
+	if (ctrl->ops->flags & NVMF_KEYED_SGLS)
 		id->sgls |= cpu_to_le32(1 << 2);
 	if (req->port->inline_data_size)
 		id->sgls |= cpu_to_le32(1 << 20);
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 6392bcd..86882d4 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -322,7 +322,7 @@ int nvmet_enable_port(struct nvmet_port *port)
 	 * If the user requested PI support and the transport isn't pi capable,
 	 * don't enable the port.
 	 */
-	if (port->pi_enable && !ops->metadata_support) {
+	if (port->pi_enable && !(ops->flags & NVMF_METADATA_SUPPORTED)) {
 		pr_err("T10-PI is not supported by transport type %d\n",
 		       port->disc_addr.trtype);
 		ret = -EINVAL;
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 40cf0b6..f40c05c 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -277,7 +277,7 @@ static void nvmet_execute_disc_identify(struct nvmet_req *req)
 	id->maxcmd = cpu_to_le16(NVMET_MAX_CMD);
 
 	id->sgls = cpu_to_le32(1 << 0);	/* we always support SGLs */
-	if (ctrl->ops->has_keyed_sgls)
+	if (ctrl->ops->flags & NVMF_KEYED_SGLS)
 		id->sgls |= cpu_to_le32(1 << 2);
 	if (req->port->inline_data_size)
 		id->sgls |= cpu_to_le32(1 << 20);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index ce57d5f..daf3838 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -287,8 +287,9 @@ struct nvmet_fabrics_ops {
 	struct module *owner;
 	unsigned int type;
 	unsigned int msdbd;
-	bool has_keyed_sgls : 1;
-	bool metadata_support : 1;
+	unsigned int flags;
+#define NVMF_KEYED_SGLS			(1 << 0)
+#define NVMF_METADATA_SUPPORTED		(1 << 1)
 	void (*queue_response)(struct nvmet_req *req);
 	int (*add_port)(struct nvmet_port *port);
 	void (*remove_port)(struct nvmet_port *port);
diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index d514178..4c89f58 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1968,8 +1968,7 @@ static u8 nvmet_rdma_get_mdts(const struct nvmet_ctrl *ctrl)
 	.owner			= THIS_MODULE,
 	.type			= NVMF_TRTYPE_RDMA,
 	.msdbd			= 1,
-	.has_keyed_sgls		= 1,
-	.metadata_support	= 1,
+	.flags			= NVMF_KEYED_SGLS | NVMF_METADATA_SUPPORTED,
 	.add_port		= nvmet_rdma_add_port,
 	.remove_port		= nvmet_rdma_remove_port,
 	.queue_response		= nvmet_rdma_queue_response,
-- 
1.8.3.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 5/5] nvmet: introduce flags member in nvmet_fabrics_ops

[Himanshu Madhani]

On 6/2/20 8:15 AM, Max Gurtovoy wrote:
> Replace has_keyed_sgls and metadata_support booleans with a flags member
> that will be used for adding more features in the future.
> 
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> Reviewed-by: Israel Rukshin <israelr@mellanox.com>
> ---
>   drivers/nvme/target/admin-cmd.c | 2 +-
>   drivers/nvme/target/core.c      | 2 +-
>   drivers/nvme/target/discovery.c | 2 +-
>   drivers/nvme/target/nvmet.h     | 5 +++--
>   drivers/nvme/target/rdma.c      | 3 +--
>   5 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 1db8c04..95bb3bc 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -427,7 +427,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
>   	id->awupf = 0;
>   
>   	id->sgls = cpu_to_le32(1 << 0);	/* we always support SGLs */
> -	if (ctrl->ops->has_keyed_sgls)
> +	if (ctrl->ops->flags & NVMF_KEYED_SGLS)
>   		id->sgls |= cpu_to_le32(1 << 2);
>   	if (req->port->inline_data_size)
>   		id->sgls |= cpu_to_le32(1 << 20);
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 6392bcd..86882d4 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -322,7 +322,7 @@ int nvmet_enable_port(struct nvmet_port *port)
>   	 * If the user requested PI support and the transport isn't pi capable,
>   	 * don't enable the port.
>   	 */
> -	if (port->pi_enable && !ops->metadata_support) {
> +	if (port->pi_enable && !(ops->flags & NVMF_METADATA_SUPPORTED)) {
>   		pr_err("T10-PI is not supported by transport type %d\n",
>   		       port->disc_addr.trtype);
>   		ret = -EINVAL;
> diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
> index 40cf0b6..f40c05c 100644
> --- a/drivers/nvme/target/discovery.c
> +++ b/drivers/nvme/target/discovery.c
> @@ -277,7 +277,7 @@ static void nvmet_execute_disc_identify(struct nvmet_req *req)
>   	id->maxcmd = cpu_to_le16(NVMET_MAX_CMD);
>   
>   	id->sgls = cpu_to_le32(1 << 0);	/* we always support SGLs */
> -	if (ctrl->ops->has_keyed_sgls)
> +	if (ctrl->ops->flags & NVMF_KEYED_SGLS)
>   		id->sgls |= cpu_to_le32(1 << 2);
>   	if (req->port->inline_data_size)
>   		id->sgls |= cpu_to_le32(1 << 20);
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index ce57d5f..daf3838 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -287,8 +287,9 @@ struct nvmet_fabrics_ops {
>   	struct module *owner;
>   	unsigned int type;
>   	unsigned int msdbd;
> -	bool has_keyed_sgls : 1;
> -	bool metadata_support : 1;
> +	unsigned int flags;
> +#define NVMF_KEYED_SGLS			(1 << 0)
> +#define NVMF_METADATA_SUPPORTED		(1 << 1)
>   	void (*queue_response)(struct nvmet_req *req);
>   	int (*add_port)(struct nvmet_port *port);
>   	void (*remove_port)(struct nvmet_port *port);
> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> index d514178..4c89f58 100644
> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c
> @@ -1968,8 +1968,7 @@ static u8 nvmet_rdma_get_mdts(const struct nvmet_ctrl *ctrl)
>   	.owner			= THIS_MODULE,
>   	.type			= NVMF_TRTYPE_RDMA,
>   	.msdbd			= 1,
> -	.has_keyed_sgls		= 1,
> -	.metadata_support	= 1,
> +	.flags			= NVMF_KEYED_SGLS | NVMF_METADATA_SUPPORTED,
>   	.add_port		= nvmet_rdma_add_port,
>   	.remove_port		= nvmet_rdma_remove_port,
>   	.queue_response		= nvmet_rdma_queue_response,
> 
Looks Good.

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

-- 
Himanshu Madhani                     Oracle Linux Engineering

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 0/5] NVMe cleanups for 5.8

[Christoph Hellwig]

On Tue, Jun 02, 2020 at 04:15:41PM +0300, Max Gurtovoy wrote:
> This series introduce some cleanups and code/logic duplication for host
> and target side. It introduces a new header file nvme-types.h that will
> be used by both host and target drivers. For now it will include mainly
> mapping structures from types to names, but in the future it can include
> more common logic between host/target drivers (as done for specific TCP
> and RDMA transports). The series continues with adding a flags member to
> nvmet_fabrics_ops structure and removes the need for adding dedicated
> member for each feature in this struct.

From a quick look this looks ok, but w're done with the 5.8 merge
window except for bug fixes.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 0/5] NVMe cleanups for 5.8

[Max Gurtovoy]

On 6/3/2020 4:08 PM, Christoph Hellwig wrote:
> On Tue, Jun 02, 2020 at 04:15:41PM +0300, Max Gurtovoy wrote:
>> This series introduce some cleanups and code/logic duplication for host
>> and target side. It introduces a new header file nvme-types.h that will
>> be used by both host and target drivers. For now it will include mainly
>> mapping structures from types to names, but in the future it can include
>> more common logic between host/target drivers (as done for specific TCP
>> and RDMA transports). The series continues with adding a flags member to
>> nvmet_fabrics_ops structure and removes the need for adding dedicated
>> member for each feature in this struct.
>  From a quick look this looks ok, but w're done with the 5.8 merge
> window except for bug fixes.

Ok so it can go to nvme-5.9..


> _______________________________________________
> linux-nvme mailing list
> linux-nvme@lists.infradead.org
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-nvme&data=02%7C01%7Cmaxg%40mellanox.com%7Ca5d736a6fdf14b4e6e8108d807bf3ab4%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637267865201210781&sdata=BHo4gR2xiT6WIgiE8BEReIiwVhfUfKcJl7zw651W0l4%3D&reserved=0

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 0/5] NVMe cleanups for 5.8

[Christoph Hellwig]

I've applied patches 4 and 5 to nvme-5.9 for now, while the rest is still
under discussion.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme