[dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor

[dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor

[Viacheslav Ovsiienko]

On BlueField platform we have the new entity - PF representor.
This one represents the PCI PF attached to external host on the
side of ARM. The traffic sent by the external host to the NIC
via PF will be seem by ARM on this PF representor.

This patch extends port recognizing capability on the base of
physical port name. The following naming formats are supported:

  - missing physical port name (no sysfs/netlink key) at all,
    this is old style (before kernel 5.0) format, master assumed
  - 1 (decimal digits) - old style (before kernel 5.0) format,
    exists only for representors, master does not have physical
    port name at all (see above)
  - p0 ("p" followed by decimal digits), new style (kernel version
    is 5.0 or higher, Mellanox OFED 4.6 or higher) name format
    for uplink representor, plays the role of master
  - pf0vf0 ("pf" followed by PF index concatenated with "vf"
    followed by VF index),  new style (kernel version  is 5.0
    or higher, Mellanox OFED 4.6 or higher) name format for
    VF representor. If index of VF is "-1" it is a special case
    of host PF representor, this representor must be indexed in
    devargs as 65535, for example representor=[0-3,65535] will
    allow representors for VF0, VF1, VF2, VF3 and host PF.
    Note: do not specify representor=[0-65535] it causes devargs
    processing error, because number of ports (rte_eth_dev) is
    limited.

Applications should distinguish representors and master devices
exclusively by device flag RTE_ETH_DEV_REPRESENTOR and do not
rely on switch port_id (mlx5 PMD deduces ones from representor_id)
values returned by dev_infos_get() API.

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 drivers/net/mlx5/mlx5.h        | 11 ++++++-
 drivers/net/mlx5/mlx5_ethdev.c | 68 +++++++++++++++++++++++++++---------------
 drivers/net/mlx5/mlx5_nl.c     | 42 +++++++++++++++++---------
 3 files changed, 82 insertions(+), 39 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 8eb1019..81c02ce 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -80,11 +80,20 @@ struct mlx5_mp_param {
 /** Key string for IPC. */
 #define MLX5_MP_NAME "net_mlx5_mp"
 
+/* Recognized Infiniband device physical port name types. */
+enum mlx5_phys_port_name_type {
+	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN = 0, /* Unrecognized. */
+	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0 */
+	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
+	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0 */
+};
+
 /** Switch information returned by mlx5_nl_switch_info(). */
 struct mlx5_switch_info {
 	uint32_t master:1; /**< Master device. */
 	uint32_t representor:1; /**< Representor device. */
-	uint32_t port_name_new:1; /**< Rep. port name is in new format. */
+	enum mlx5_phys_port_name_type name_type; /** < Port name type. */
+	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
 	int32_t port_name; /**< Representor port name. */
 	uint64_t switch_id; /**< Switch identifier. */
 };
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 3992918..371989f 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 	struct mlx5_switch_info data = {
 		.master = 0,
 		.representor = 0,
-		.port_name_new = 0,
+		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
 		.port_name = 0,
 		.switch_id = 0,
 	};
 	DIR *dir;
-	bool port_name_set = false;
 	bool port_switch_id_set = false;
 	bool device_dir = false;
 	char c;
@@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 		ret = fscanf(file, "%s", port_name);
 		fclose(file);
 		if (ret == 1)
-			port_name_set = mlx5_translate_port_name(port_name,
-								 &data);
+			mlx5_translate_port_name(port_name, &data);
 	}
 	file = fopen(phys_switch_id, "rb");
 	if (file == NULL) {
@@ -1440,8 +1438,15 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 		closedir(dir);
 		device_dir = true;
 	}
-	data.master = port_switch_id_set && (!port_name_set || device_dir);
-	data.representor = port_switch_id_set && port_name_set && !device_dir;
+	if (port_switch_id_set) {
+		data.master =
+			device_dir ||
+			data.name_type == MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN ||
+			data.name_type == MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
+		data.representor = !device_dir &&
+			(data.name_type == MLX5_PHYS_PORT_NAME_TYPE_LEGACY ||
+			 data.name_type == MLX5_PHYS_PORT_NAME_TYPE_PFVF);
+	}
 	*info = data;
 	assert(!(data.master && data.representor));
 	if (data.master && data.representor) {
@@ -1459,10 +1464,11 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
  * @param[in] port_name_in
  *   String representing the port name.
  * @param[out] port_info_out
- *   Port information, including port name as a number.
+ *   Port information, including port name as a number and port name
+ *   type if recognized
  *
  * @return
- *   true on success, false otherwise.
+ *   true on success (if name format recognized), false otherwise.
  */
 bool
 mlx5_translate_port_name(const char *port_name_in,
@@ -1470,25 +1476,39 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 {
 	char pf_c1, pf_c2, vf_c1, vf_c2;
 	char *end;
-	int32_t pf_num;
-	bool port_name_set = false;
+	int sc_items;
 
 	/*
 	 * Check for port-name as a string of the form pf0vf0
-	 * (support kernel ver >= 5.0)
+	 * (support kernel ver >= 5.0 or OFED ver >= 4.6).
 	 */
-	port_name_set =	(sscanf(port_name_in, "%c%c%d%c%c%d", &pf_c1, &pf_c2,
-				&pf_num, &vf_c1, &vf_c2,
-				&port_info_out->port_name) == 6);
-	if (port_name_set) {
-		port_info_out->port_name_new = 1;
-	} else {
-		/* Check for port-name as a number (support kernel ver < 5.0 */
-		errno = 0;
-		port_info_out->port_name = strtol(port_name_in, &end, 0);
-		if (!errno &&
-		    (size_t)(end - port_name_in) == strlen(port_name_in))
-			port_name_set = true;
+	sc_items = sscanf(port_name_in, "%c%c%d%c%c%d",
+			  &pf_c1, &pf_c2, &port_info_out->pf_num,
+			  &vf_c1, &vf_c2, &port_info_out->port_name);
+	if (sc_items == 6 &&
+	    pf_c1 == 'p' && pf_c2 == 'f' &&
+	    vf_c1 == 'v' && vf_c2 == 'f') {
+		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_PFVF;
+		return true;
+	}
+	/*
+	 * Check for port-name as a string of the form p0
+	 * (support kernel ver >= 5.0, or OFED ver >= 4.6).
+	 */
+	sc_items = sscanf(port_name_in, "%c%d",
+			  &pf_c1, &port_info_out->port_name);
+	if (sc_items == 2 && pf_c1 == 'p') {
+		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
+		return true;
+	}
+	/* Check for port-name as a number (support kernel ver < 5.0 */
+	errno = 0;
+	port_info_out->port_name = strtol(port_name_in, &end, 0);
+	if (!errno &&
+	    (size_t)(end - port_name_in) == strlen(port_name_in)) {
+		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
+		return  true;
 	}
-	return port_name_set;
+	port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
+	return false;
 }
diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c
index fd9226b..669de76 100644
--- a/drivers/net/mlx5/mlx5_nl.c
+++ b/drivers/net/mlx5/mlx5_nl.c
@@ -887,12 +887,11 @@ struct mlx5_nl_ifindex_data {
 	struct mlx5_switch_info info = {
 		.master = 0,
 		.representor = 0,
-		.port_name_new = 0,
+		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
 		.port_name = 0,
 		.switch_id = 0,
 	};
 	size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
-	bool port_name_set = false;
 	bool switch_id_set = false;
 	bool num_vf_set = false;
 
@@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
 			num_vf_set = true;
 			break;
 		case IFLA_PHYS_PORT_NAME:
-			port_name_set =
-				mlx5_translate_port_name((char *)payload,
-							 &info);
+			mlx5_translate_port_name((char *)payload, &info);
 			break;
 		case IFLA_PHYS_SWITCH_ID:
 			info.switch_id = 0;
@@ -926,16 +923,33 @@ struct mlx5_nl_ifindex_data {
 		off += RTA_ALIGN(ra->rta_len);
 	}
 	if (switch_id_set) {
-		if (info.port_name_new) {
-			/* New representors naming schema. */
-			if (port_name_set) {
-				info.master = (info.port_name == -1);
-				info.representor = (info.port_name != -1);
-			}
-		} else {
+		/* We have some E-Switch configuration. */
+		switch (info.name_type) {
+		case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
+			/*
+			 * Name is not recognized or not set,
+			 * it can not be representor, check
+			 * VF number to see if it is a master.
+			 */
+			info.master = num_vf_set;
+			break;
+		case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
 			/* Legacy representors naming schema. */
-			info.master = (!port_name_set || num_vf_set);
-			info.representor = port_name_set && !num_vf_set;
+			info.representor = !num_vf_set;
+			break;
+		case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
+			/* New uplink naming schema. */
+			info.master = 1;
+			break;
+		case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
+			/* New representors naming schema. */
+			info.representor = 1;
+			break;
+		}
+		if (!info.master && !info.representor) {
+			DRV_LOG(INFO,
+				"unable to recognize master/representors"
+				" on the device in switch domain");
 		}
 	}
 	assert(!(info.master && info.representor));
-- 
1.8.3.1

[dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor

[Viacheslav Ovsiienko]

On BlueField platform we have the new entity - PF representor.
This one represents the PCI PF attached to external host on the
side of ARM. The traffic sent by the external host to the NIC
via PF will be seem by ARM on this PF representor.

This patch extends port recognizing capability on the base of
physical port name. The following naming formats are supported:

  - missing physical port name (no sysfs/netlink key) at all,
    this is old style (before kernel 5.0) format, master assumed
  - 1 (decimal digits) - old style (before kernel 5.0) format,
    exists only for representors, master does not have physical
    port name at all (see above)
  - p0 ("p" followed by decimal digits), new style (kernel version
    is 5.0 or higher, Mellanox OFED 4.6 or higher) name format
    for uplink representor, plays the role of master
  - pf0vf0 ("pf" followed by PF index concatenated with "vf"
    followed by VF index),  new style (kernel version  is 5.0
    or higher, Mellanox OFED 4.6 or higher) name format for
    VF representor. If index of VF is "-1" it is a special case
    of host PF representor, this representor must be indexed in
    devargs as 65535, for example representor=[0-3,65535] will
    allow representors for VF0, VF1, VF2, VF3 and host PF.
    Note: do not specify representor=[0-65535] it causes devargs
    processing error, because number of ports (rte_eth_dev) is
    limited.

Applications should distinguish representors and master devices
exclusively by device flag RTE_ETH_DEV_REPRESENTOR and do not
rely on switch port_id (mlx5 PMD deduces ones from representor_id)
values returned by dev_infos_get() API.

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 drivers/net/mlx5/mlx5.h        | 11 ++++++-
 drivers/net/mlx5/mlx5_ethdev.c | 68 +++++++++++++++++++++++++++---------------
 drivers/net/mlx5/mlx5_nl.c     | 42 +++++++++++++++++---------
 3 files changed, 82 insertions(+), 39 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 8eb1019..81c02ce 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -80,11 +80,20 @@ struct mlx5_mp_param {
 /** Key string for IPC. */
 #define MLX5_MP_NAME "net_mlx5_mp"
 
+/* Recognized Infiniband device physical port name types. */
+enum mlx5_phys_port_name_type {
+	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN = 0, /* Unrecognized. */
+	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0 */
+	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
+	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0 */
+};
+
 /** Switch information returned by mlx5_nl_switch_info(). */
 struct mlx5_switch_info {
 	uint32_t master:1; /**< Master device. */
 	uint32_t representor:1; /**< Representor device. */
-	uint32_t port_name_new:1; /**< Rep. port name is in new format. */
+	enum mlx5_phys_port_name_type name_type; /** < Port name type. */
+	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
 	int32_t port_name; /**< Representor port name. */
 	uint64_t switch_id; /**< Switch identifier. */
 };
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 3992918..371989f 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 	struct mlx5_switch_info data = {
 		.master = 0,
 		.representor = 0,
-		.port_name_new = 0,
+		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
 		.port_name = 0,
 		.switch_id = 0,
 	};
 	DIR *dir;
-	bool port_name_set = false;
 	bool port_switch_id_set = false;
 	bool device_dir = false;
 	char c;
@@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 		ret = fscanf(file, "%s", port_name);
 		fclose(file);
 		if (ret == 1)
-			port_name_set = mlx5_translate_port_name(port_name,
-								 &data);
+			mlx5_translate_port_name(port_name, &data);
 	}
 	file = fopen(phys_switch_id, "rb");
 	if (file == NULL) {
@@ -1440,8 +1438,15 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 		closedir(dir);
 		device_dir = true;
 	}
-	data.master = port_switch_id_set && (!port_name_set || device_dir);
-	data.representor = port_switch_id_set && port_name_set && !device_dir;
+	if (port_switch_id_set) {
+		data.master =
+			device_dir ||
+			data.name_type == MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN ||
+			data.name_type == MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
+		data.representor = !device_dir &&
+			(data.name_type == MLX5_PHYS_PORT_NAME_TYPE_LEGACY ||
+			 data.name_type == MLX5_PHYS_PORT_NAME_TYPE_PFVF);
+	}
 	*info = data;
 	assert(!(data.master && data.representor));
 	if (data.master && data.representor) {
@@ -1459,10 +1464,11 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
  * @param[in] port_name_in
  *   String representing the port name.
  * @param[out] port_info_out
- *   Port information, including port name as a number.
+ *   Port information, including port name as a number and port name
+ *   type if recognized
  *
  * @return
- *   true on success, false otherwise.
+ *   true on success (if name format recognized), false otherwise.
  */
 bool
 mlx5_translate_port_name(const char *port_name_in,
@@ -1470,25 +1476,39 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 {
 	char pf_c1, pf_c2, vf_c1, vf_c2;
 	char *end;
-	int32_t pf_num;
-	bool port_name_set = false;
+	int sc_items;
 
 	/*
 	 * Check for port-name as a string of the form pf0vf0
-	 * (support kernel ver >= 5.0)
+	 * (support kernel ver >= 5.0 or OFED ver >= 4.6).
 	 */
-	port_name_set =	(sscanf(port_name_in, "%c%c%d%c%c%d", &pf_c1, &pf_c2,
-				&pf_num, &vf_c1, &vf_c2,
-				&port_info_out->port_name) == 6);
-	if (port_name_set) {
-		port_info_out->port_name_new = 1;
-	} else {
-		/* Check for port-name as a number (support kernel ver < 5.0 */
-		errno = 0;
-		port_info_out->port_name = strtol(port_name_in, &end, 0);
-		if (!errno &&
-		    (size_t)(end - port_name_in) == strlen(port_name_in))
-			port_name_set = true;
+	sc_items = sscanf(port_name_in, "%c%c%d%c%c%d",
+			  &pf_c1, &pf_c2, &port_info_out->pf_num,
+			  &vf_c1, &vf_c2, &port_info_out->port_name);
+	if (sc_items == 6 &&
+	    pf_c1 == 'p' && pf_c2 == 'f' &&
+	    vf_c1 == 'v' && vf_c2 == 'f') {
+		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_PFVF;
+		return true;
+	}
+	/*
+	 * Check for port-name as a string of the form p0
+	 * (support kernel ver >= 5.0, or OFED ver >= 4.6).
+	 */
+	sc_items = sscanf(port_name_in, "%c%d",
+			  &pf_c1, &port_info_out->port_name);
+	if (sc_items == 2 && pf_c1 == 'p') {
+		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
+		return true;
+	}
+	/* Check for port-name as a number (support kernel ver < 5.0 */
+	errno = 0;
+	port_info_out->port_name = strtol(port_name_in, &end, 0);
+	if (!errno &&
+	    (size_t)(end - port_name_in) == strlen(port_name_in)) {
+		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
+		return  true;
 	}
-	return port_name_set;
+	port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
+	return false;
 }
diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c
index fd9226b..669de76 100644
--- a/drivers/net/mlx5/mlx5_nl.c
+++ b/drivers/net/mlx5/mlx5_nl.c
@@ -887,12 +887,11 @@ struct mlx5_nl_ifindex_data {
 	struct mlx5_switch_info info = {
 		.master = 0,
 		.representor = 0,
-		.port_name_new = 0,
+		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
 		.port_name = 0,
 		.switch_id = 0,
 	};
 	size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
-	bool port_name_set = false;
 	bool switch_id_set = false;
 	bool num_vf_set = false;
 
@@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
 			num_vf_set = true;
 			break;
 		case IFLA_PHYS_PORT_NAME:
-			port_name_set =
-				mlx5_translate_port_name((char *)payload,
-							 &info);
+			mlx5_translate_port_name((char *)payload, &info);
 			break;
 		case IFLA_PHYS_SWITCH_ID:
 			info.switch_id = 0;
@@ -926,16 +923,33 @@ struct mlx5_nl_ifindex_data {
 		off += RTA_ALIGN(ra->rta_len);
 	}
 	if (switch_id_set) {
-		if (info.port_name_new) {
-			/* New representors naming schema. */
-			if (port_name_set) {
-				info.master = (info.port_name == -1);
-				info.representor = (info.port_name != -1);
-			}
-		} else {
+		/* We have some E-Switch configuration. */
+		switch (info.name_type) {
+		case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
+			/*
+			 * Name is not recognized or not set,
+			 * it can not be representor, check
+			 * VF number to see if it is a master.
+			 */
+			info.master = num_vf_set;
+			break;
+		case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
 			/* Legacy representors naming schema. */
-			info.master = (!port_name_set || num_vf_set);
-			info.representor = port_name_set && !num_vf_set;
+			info.representor = !num_vf_set;
+			break;
+		case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
+			/* New uplink naming schema. */
+			info.master = 1;
+			break;
+		case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
+			/* New representors naming schema. */
+			info.representor = 1;
+			break;
+		}
+		if (!info.master && !info.representor) {
+			DRV_LOG(INFO,
+				"unable to recognize master/representors"
+				" on the device in switch domain");
 		}
 	}
 	assert(!(info.master && info.representor));
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor

[Shahaf Shuler]

Hi Slava,

Friday, April 12, 2019 6:48 PM, Viacheslav Ovsiienko:
> Subject: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor
> 
> On BlueField platform we have the new entity - PF representor.
> This one represents the PCI PF attached to external host on the side of ARM.
> The traffic sent by the external host to the NIC via PF will be seem by ARM on
> this PF representor.
> 
> This patch extends port recognizing capability on the base of physical port
> name. The following naming formats are supported:
> 
>   - missing physical port name (no sysfs/netlink key) at all,
>     this is old style (before kernel 5.0) format, master assumed
>   - 1 (decimal digits) - old style (before kernel 5.0) format,
>     exists only for representors, master does not have physical
>     port name at all (see above)
>   - p0 ("p" followed by decimal digits), new style (kernel version
>     is 5.0 or higher, Mellanox OFED 4.6 or higher) name format
>     for uplink representor, plays the role of master
>   - pf0vf0 ("pf" followed by PF index concatenated with "vf"
>     followed by VF index),  new style (kernel version  is 5.0
>     or higher, Mellanox OFED 4.6 or higher) name format for
>     VF representor. If index of VF is "-1" it is a special case
>     of host PF representor, this representor must be indexed in
>     devargs as 65535, for example representor=[0-3,65535] will
>     allow representors for VF0, VF1, VF2, VF3 and host PF.
>     Note: do not specify representor=[0-65535] it causes devargs
>     processing error, because number of ports (rte_eth_dev) is
>     limited.
> 

The above is a bit complex to understand and in fact we have 2 modes:
1. legacy - phys_port_name are numbers. Master doesn't have phys_port_name
2. modern - phys_port_name are strings. 
uplink representor is p%d
representors are (including PF representor) pf%dvf%d. the vf index for the PF representor is 65535. 

> Applications should distinguish representors and master devices exclusively
> by device flag RTE_ETH_DEV_REPRESENTOR and do not rely on switch
> port_id (mlx5 PMD deduces ones from representor_id) values returned by
> dev_infos_get() API.
> 

Please also reference the kernel commit which introduce the name change. 

> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5.h        | 11 ++++++-
>  drivers/net/mlx5/mlx5_ethdev.c | 68 +++++++++++++++++++++++++++----
> -----------
>  drivers/net/mlx5/mlx5_nl.c     | 42 +++++++++++++++++---------
>  3 files changed, 82 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> 8eb1019..81c02ce 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -80,11 +80,20 @@ struct mlx5_mp_param {
>  /** Key string for IPC. */
>  #define MLX5_MP_NAME "net_mlx5_mp"
> 
> +/* Recognized Infiniband device physical port name types. */ enum
> +mlx5_phys_port_name_type {
> +	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN = 0, /* Unrecognized.
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
> +	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0
> */ };
> +
>  /** Switch information returned by mlx5_nl_switch_info(). */  struct
> mlx5_switch_info {
>  	uint32_t master:1; /**< Master device. */
>  	uint32_t representor:1; /**< Representor device. */
> -	uint32_t port_name_new:1; /**< Rep. port name is in new format.
> */
> +	enum mlx5_phys_port_name_type name_type; /** < Port name
> type. */
> +	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
>  	int32_t port_name; /**< Representor port name. */
>  	uint64_t switch_id; /**< Switch identifier. */  }; diff --git
> a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c index
> 3992918..371989f 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
>  	struct mlx5_switch_info data = {
>  		.master = 0,
>  		.representor = 0,
> -		.port_name_new = 0,
> +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
>  		.port_name = 0,
>  		.switch_id = 0,
>  	};
>  	DIR *dir;
> -	bool port_name_set = false;
>  	bool port_switch_id_set = false;
>  	bool device_dir = false;
>  	char c;
> @@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev,
> char *fw_ver, size_t fw_size)
>  		ret = fscanf(file, "%s", port_name);
>  		fclose(file);
>  		if (ret == 1)
> -			port_name_set =
> mlx5_translate_port_name(port_name,
> -								 &data);
> +			mlx5_translate_port_name(port_name, &data);
>  	}
>  	file = fopen(phys_switch_id, "rb");
>  	if (file == NULL) {
> @@ -1440,8 +1438,15 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
>  		closedir(dir);
>  		device_dir = true;
>  	}
> -	data.master = port_switch_id_set && (!port_name_set ||
> device_dir);
> -	data.representor = port_switch_id_set && port_name_set &&
> !device_dir;
> +	if (port_switch_id_set) {
> +		data.master =
> +			device_dir ||
> +			data.name_type ==
> MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN ||
> +			data.name_type ==
> MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> +		data.representor = !device_dir &&
> +			(data.name_type ==
> MLX5_PHYS_PORT_NAME_TYPE_LEGACY ||
> +			 data.name_type ==
> MLX5_PHYS_PORT_NAME_TYPE_PFVF);


Why we need to split the logic of the master/representor detection between the mlx5_translate_port_name and the caller function?

The way I envision it is mlx5_tranlate_port_name receives the phys_port_name and maybe more metadata and return the port classification (master/representor) and the representor/pf number. 
No need for data.master = some_logic(translate_port_name_info). 

Inside the translate function I would expect to have 2 smaller function:
1. to handle the modern format (strings)
2. to handle the legacy format (integers) 

> +	}
>  	*info = data;
>  	assert(!(data.master && data.representor));
>  	if (data.master && data.representor) { @@ -1459,10 +1464,11 @@ int
> mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t
> fw_size)
>   * @param[in] port_name_in
>   *   String representing the port name.
>   * @param[out] port_info_out
> - *   Port information, including port name as a number.
> + *   Port information, including port name as a number and port name
> + *   type if recognized
>   *
>   * @return
> - *   true on success, false otherwise.
> + *   true on success (if name format recognized), false otherwise.
>   */
>  bool
>  mlx5_translate_port_name(const char *port_name_in, @@ -1470,25
> +1476,39 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char
> *fw_ver, size_t fw_size)  {
>  	char pf_c1, pf_c2, vf_c1, vf_c2;
>  	char *end;
> -	int32_t pf_num;
> -	bool port_name_set = false;
> +	int sc_items;
> 
>  	/*
>  	 * Check for port-name as a string of the form pf0vf0
> -	 * (support kernel ver >= 5.0)
> +	 * (support kernel ver >= 5.0 or OFED ver >= 4.6).
>  	 */
> -	port_name_set =	(sscanf(port_name_in, "%c%c%d%c%c%d",
> &pf_c1, &pf_c2,
> -				&pf_num, &vf_c1, &vf_c2,
> -				&port_info_out->port_name) == 6);
> -	if (port_name_set) {
> -		port_info_out->port_name_new = 1;
> -	} else {
> -		/* Check for port-name as a number (support kernel ver <
> 5.0 */
> -		errno = 0;
> -		port_info_out->port_name = strtol(port_name_in, &end, 0);
> -		if (!errno &&
> -		    (size_t)(end - port_name_in) == strlen(port_name_in))
> -			port_name_set = true;
> +	sc_items = sscanf(port_name_in, "%c%c%d%c%c%d",
> +			  &pf_c1, &pf_c2, &port_info_out->pf_num,
> +			  &vf_c1, &vf_c2, &port_info_out->port_name);
> +	if (sc_items == 6 &&
> +	    pf_c1 == 'p' && pf_c2 == 'f' &&
> +	    vf_c1 == 'v' && vf_c2 == 'f') {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_PFVF;
> +		return true;
> +	}
> +	/*
> +	 * Check for port-name as a string of the form p0
> +	 * (support kernel ver >= 5.0, or OFED ver >= 4.6).
> +	 */
> +	sc_items = sscanf(port_name_in, "%c%d",
> +			  &pf_c1, &port_info_out->port_name);
> +	if (sc_items == 2 && pf_c1 == 'p') {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> +		return true;
> +	}
> +	/* Check for port-name as a number (support kernel ver < 5.0 */
> +	errno = 0;
> +	port_info_out->port_name = strtol(port_name_in, &end, 0);
> +	if (!errno &&
> +	    (size_t)(end - port_name_in) == strlen(port_name_in)) {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
> +		return  true;
>  	}
> -	return port_name_set;
> +	port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
> +	return false;
>  }
> diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c index
> fd9226b..669de76 100644
> --- a/drivers/net/mlx5/mlx5_nl.c
> +++ b/drivers/net/mlx5/mlx5_nl.c
> @@ -887,12 +887,11 @@ struct mlx5_nl_ifindex_data {
>  	struct mlx5_switch_info info = {
>  		.master = 0,
>  		.representor = 0,
> -		.port_name_new = 0,
> +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
>  		.port_name = 0,
>  		.switch_id = 0,
>  	};
>  	size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> -	bool port_name_set = false;
>  	bool switch_id_set = false;
>  	bool num_vf_set = false;
> 
> @@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
>  			num_vf_set = true;
>  			break;
>  		case IFLA_PHYS_PORT_NAME:
> -			port_name_set =
> -				mlx5_translate_port_name((char *)payload,
> -							 &info);
> +			mlx5_translate_port_name((char *)payload, &info);
>  			break;
>  		case IFLA_PHYS_SWITCH_ID:
>  			info.switch_id = 0;
> @@ -926,16 +923,33 @@ struct mlx5_nl_ifindex_data {
>  		off += RTA_ALIGN(ra->rta_len);
>  	}
>  	if (switch_id_set) {
> -		if (info.port_name_new) {
> -			/* New representors naming schema. */
> -			if (port_name_set) {
> -				info.master = (info.port_name == -1);
> -				info.representor = (info.port_name != -1);
> -			}
> -		} else {
> +		/* We have some E-Switch configuration. */
> +		switch (info.name_type) {
> +		case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
> +			/*
> +			 * Name is not recognized or not set,
> +			 * it can not be representor, check
> +			 * VF number to see if it is a master.
> +			 */
> +			info.master = num_vf_set;
> +			break;
> +		case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
>  			/* Legacy representors naming schema. */
> -			info.master = (!port_name_set || num_vf_set);
> -			info.representor = port_name_set && !num_vf_set;
> +			info.representor = !num_vf_set;
> +			break;
> +		case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
> +			/* New uplink naming schema. */
> +			info.master = 1;
> +			break;
> +		case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
> +			/* New representors naming schema. */
> +			info.representor = 1;
> +			break;
> +		}
> +		if (!info.master && !info.representor) {
> +			DRV_LOG(INFO,
> +				"unable to recognize master/representors"
> +				" on the device in switch domain");

Same comment as above. Would like to avoid this switch case outside of the translate function .

>  		}
>  	}
>  	assert(!(info.master && info.representor));
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor

[Shahaf Shuler]

Hi Slava,

Friday, April 12, 2019 6:48 PM, Viacheslav Ovsiienko:
> Subject: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor
> 
> On BlueField platform we have the new entity - PF representor.
> This one represents the PCI PF attached to external host on the side of ARM.
> The traffic sent by the external host to the NIC via PF will be seem by ARM on
> this PF representor.
> 
> This patch extends port recognizing capability on the base of physical port
> name. The following naming formats are supported:
> 
>   - missing physical port name (no sysfs/netlink key) at all,
>     this is old style (before kernel 5.0) format, master assumed
>   - 1 (decimal digits) - old style (before kernel 5.0) format,
>     exists only for representors, master does not have physical
>     port name at all (see above)
>   - p0 ("p" followed by decimal digits), new style (kernel version
>     is 5.0 or higher, Mellanox OFED 4.6 or higher) name format
>     for uplink representor, plays the role of master
>   - pf0vf0 ("pf" followed by PF index concatenated with "vf"
>     followed by VF index),  new style (kernel version  is 5.0
>     or higher, Mellanox OFED 4.6 or higher) name format for
>     VF representor. If index of VF is "-1" it is a special case
>     of host PF representor, this representor must be indexed in
>     devargs as 65535, for example representor=[0-3,65535] will
>     allow representors for VF0, VF1, VF2, VF3 and host PF.
>     Note: do not specify representor=[0-65535] it causes devargs
>     processing error, because number of ports (rte_eth_dev) is
>     limited.
> 

The above is a bit complex to understand and in fact we have 2 modes:
1. legacy - phys_port_name are numbers. Master doesn't have phys_port_name
2. modern - phys_port_name are strings. 
uplink representor is p%d
representors are (including PF representor) pf%dvf%d. the vf index for the PF representor is 65535. 

> Applications should distinguish representors and master devices exclusively
> by device flag RTE_ETH_DEV_REPRESENTOR and do not rely on switch
> port_id (mlx5 PMD deduces ones from representor_id) values returned by
> dev_infos_get() API.
> 

Please also reference the kernel commit which introduce the name change. 

> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5.h        | 11 ++++++-
>  drivers/net/mlx5/mlx5_ethdev.c | 68 +++++++++++++++++++++++++++----
> -----------
>  drivers/net/mlx5/mlx5_nl.c     | 42 +++++++++++++++++---------
>  3 files changed, 82 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> 8eb1019..81c02ce 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -80,11 +80,20 @@ struct mlx5_mp_param {
>  /** Key string for IPC. */
>  #define MLX5_MP_NAME "net_mlx5_mp"
> 
> +/* Recognized Infiniband device physical port name types. */ enum
> +mlx5_phys_port_name_type {
> +	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN = 0, /* Unrecognized.
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
> +	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0
> */ };
> +
>  /** Switch information returned by mlx5_nl_switch_info(). */  struct
> mlx5_switch_info {
>  	uint32_t master:1; /**< Master device. */
>  	uint32_t representor:1; /**< Representor device. */
> -	uint32_t port_name_new:1; /**< Rep. port name is in new format.
> */
> +	enum mlx5_phys_port_name_type name_type; /** < Port name
> type. */
> +	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
>  	int32_t port_name; /**< Representor port name. */
>  	uint64_t switch_id; /**< Switch identifier. */  }; diff --git
> a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c index
> 3992918..371989f 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
>  	struct mlx5_switch_info data = {
>  		.master = 0,
>  		.representor = 0,
> -		.port_name_new = 0,
> +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
>  		.port_name = 0,
>  		.switch_id = 0,
>  	};
>  	DIR *dir;
> -	bool port_name_set = false;
>  	bool port_switch_id_set = false;
>  	bool device_dir = false;
>  	char c;
> @@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev,
> char *fw_ver, size_t fw_size)
>  		ret = fscanf(file, "%s", port_name);
>  		fclose(file);
>  		if (ret == 1)
> -			port_name_set =
> mlx5_translate_port_name(port_name,
> -								 &data);
> +			mlx5_translate_port_name(port_name, &data);
>  	}
>  	file = fopen(phys_switch_id, "rb");
>  	if (file == NULL) {
> @@ -1440,8 +1438,15 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
>  		closedir(dir);
>  		device_dir = true;
>  	}
> -	data.master = port_switch_id_set && (!port_name_set ||
> device_dir);
> -	data.representor = port_switch_id_set && port_name_set &&
> !device_dir;
> +	if (port_switch_id_set) {
> +		data.master =
> +			device_dir ||
> +			data.name_type ==
> MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN ||
> +			data.name_type ==
> MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> +		data.representor = !device_dir &&
> +			(data.name_type ==
> MLX5_PHYS_PORT_NAME_TYPE_LEGACY ||
> +			 data.name_type ==
> MLX5_PHYS_PORT_NAME_TYPE_PFVF);


Why we need to split the logic of the master/representor detection between the mlx5_translate_port_name and the caller function?

The way I envision it is mlx5_tranlate_port_name receives the phys_port_name and maybe more metadata and return the port classification (master/representor) and the representor/pf number. 
No need for data.master = some_logic(translate_port_name_info). 

Inside the translate function I would expect to have 2 smaller function:
1. to handle the modern format (strings)
2. to handle the legacy format (integers) 

> +	}
>  	*info = data;
>  	assert(!(data.master && data.representor));
>  	if (data.master && data.representor) { @@ -1459,10 +1464,11 @@ int
> mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t
> fw_size)
>   * @param[in] port_name_in
>   *   String representing the port name.
>   * @param[out] port_info_out
> - *   Port information, including port name as a number.
> + *   Port information, including port name as a number and port name
> + *   type if recognized
>   *
>   * @return
> - *   true on success, false otherwise.
> + *   true on success (if name format recognized), false otherwise.
>   */
>  bool
>  mlx5_translate_port_name(const char *port_name_in, @@ -1470,25
> +1476,39 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char
> *fw_ver, size_t fw_size)  {
>  	char pf_c1, pf_c2, vf_c1, vf_c2;
>  	char *end;
> -	int32_t pf_num;
> -	bool port_name_set = false;
> +	int sc_items;
> 
>  	/*
>  	 * Check for port-name as a string of the form pf0vf0
> -	 * (support kernel ver >= 5.0)
> +	 * (support kernel ver >= 5.0 or OFED ver >= 4.6).
>  	 */
> -	port_name_set =	(sscanf(port_name_in, "%c%c%d%c%c%d",
> &pf_c1, &pf_c2,
> -				&pf_num, &vf_c1, &vf_c2,
> -				&port_info_out->port_name) == 6);
> -	if (port_name_set) {
> -		port_info_out->port_name_new = 1;
> -	} else {
> -		/* Check for port-name as a number (support kernel ver <
> 5.0 */
> -		errno = 0;
> -		port_info_out->port_name = strtol(port_name_in, &end, 0);
> -		if (!errno &&
> -		    (size_t)(end - port_name_in) == strlen(port_name_in))
> -			port_name_set = true;
> +	sc_items = sscanf(port_name_in, "%c%c%d%c%c%d",
> +			  &pf_c1, &pf_c2, &port_info_out->pf_num,
> +			  &vf_c1, &vf_c2, &port_info_out->port_name);
> +	if (sc_items == 6 &&
> +	    pf_c1 == 'p' && pf_c2 == 'f' &&
> +	    vf_c1 == 'v' && vf_c2 == 'f') {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_PFVF;
> +		return true;
> +	}
> +	/*
> +	 * Check for port-name as a string of the form p0
> +	 * (support kernel ver >= 5.0, or OFED ver >= 4.6).
> +	 */
> +	sc_items = sscanf(port_name_in, "%c%d",
> +			  &pf_c1, &port_info_out->port_name);
> +	if (sc_items == 2 && pf_c1 == 'p') {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> +		return true;
> +	}
> +	/* Check for port-name as a number (support kernel ver < 5.0 */
> +	errno = 0;
> +	port_info_out->port_name = strtol(port_name_in, &end, 0);
> +	if (!errno &&
> +	    (size_t)(end - port_name_in) == strlen(port_name_in)) {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
> +		return  true;
>  	}
> -	return port_name_set;
> +	port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
> +	return false;
>  }
> diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c index
> fd9226b..669de76 100644
> --- a/drivers/net/mlx5/mlx5_nl.c
> +++ b/drivers/net/mlx5/mlx5_nl.c
> @@ -887,12 +887,11 @@ struct mlx5_nl_ifindex_data {
>  	struct mlx5_switch_info info = {
>  		.master = 0,
>  		.representor = 0,
> -		.port_name_new = 0,
> +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
>  		.port_name = 0,
>  		.switch_id = 0,
>  	};
>  	size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> -	bool port_name_set = false;
>  	bool switch_id_set = false;
>  	bool num_vf_set = false;
> 
> @@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
>  			num_vf_set = true;
>  			break;
>  		case IFLA_PHYS_PORT_NAME:
> -			port_name_set =
> -				mlx5_translate_port_name((char *)payload,
> -							 &info);
> +			mlx5_translate_port_name((char *)payload, &info);
>  			break;
>  		case IFLA_PHYS_SWITCH_ID:
>  			info.switch_id = 0;
> @@ -926,16 +923,33 @@ struct mlx5_nl_ifindex_data {
>  		off += RTA_ALIGN(ra->rta_len);
>  	}
>  	if (switch_id_set) {
> -		if (info.port_name_new) {
> -			/* New representors naming schema. */
> -			if (port_name_set) {
> -				info.master = (info.port_name == -1);
> -				info.representor = (info.port_name != -1);
> -			}
> -		} else {
> +		/* We have some E-Switch configuration. */
> +		switch (info.name_type) {
> +		case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
> +			/*
> +			 * Name is not recognized or not set,
> +			 * it can not be representor, check
> +			 * VF number to see if it is a master.
> +			 */
> +			info.master = num_vf_set;
> +			break;
> +		case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
>  			/* Legacy representors naming schema. */
> -			info.master = (!port_name_set || num_vf_set);
> -			info.representor = port_name_set && !num_vf_set;
> +			info.representor = !num_vf_set;
> +			break;
> +		case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
> +			/* New uplink naming schema. */
> +			info.master = 1;
> +			break;
> +		case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
> +			/* New representors naming schema. */
> +			info.representor = 1;
> +			break;
> +		}
> +		if (!info.master && !info.representor) {
> +			DRV_LOG(INFO,
> +				"unable to recognize master/representors"
> +				" on the device in switch domain");

Same comment as above. Would like to avoid this switch case outside of the translate function .

>  		}
>  	}
>  	assert(!(info.master && info.representor));
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor

[Slava Ovsiienko]

Hi, Shahaf

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Sunday, April 14, 2019 10:43
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> representor
> 
> Hi Slava,
> 
> Friday, April 12, 2019 6:48 PM, Viacheslav Ovsiienko:
> > Subject: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> > representor
> >
> > On BlueField platform we have the new entity - PF representor.
> > This one represents the PCI PF attached to external host on the side of
> ARM.
> > The traffic sent by the external host to the NIC via PF will be seem
> > by ARM on this PF representor.
> >
> > This patch extends port recognizing capability on the base of physical
> > port name. The following naming formats are supported:
> >
> >   - missing physical port name (no sysfs/netlink key) at all,
> >     this is old style (before kernel 5.0) format, master assumed
> >   - 1 (decimal digits) - old style (before kernel 5.0) format,
> >     exists only for representors, master does not have physical
> >     port name at all (see above)
> >   - p0 ("p" followed by decimal digits), new style (kernel version
> >     is 5.0 or higher, Mellanox OFED 4.6 or higher) name format
> >     for uplink representor, plays the role of master
> >   - pf0vf0 ("pf" followed by PF index concatenated with "vf"
> >     followed by VF index),  new style (kernel version  is 5.0
> >     or higher, Mellanox OFED 4.6 or higher) name format for
> >     VF representor. If index of VF is "-1" it is a special case
> >     of host PF representor, this representor must be indexed in
> >     devargs as 65535, for example representor=[0-3,65535] will
> >     allow representors for VF0, VF1, VF2, VF3 and host PF.
> >     Note: do not specify representor=[0-65535] it causes devargs
> >     processing error, because number of ports (rte_eth_dev) is
> >     limited.
> >
> 
> The above is a bit complex to understand and in fact we have 2 modes:
> 1. legacy - phys_port_name are numbers. Master doesn't have
> phys_port_name 2. modern - phys_port_name are strings.
> uplink representor is p%d
> representors are (including PF representor) pf%dvf%d. the vf index for the PF
> representor is 65535.

OK, I'll try to update the commit message to make it more clear.
> 
> > Applications should distinguish representors and master devices
> > exclusively by device flag RTE_ETH_DEV_REPRESENTOR and do not rely on
> > switch port_id (mlx5 PMD deduces ones from representor_id) values
> > returned by
> > dev_infos_get() API.
> >
> 
> Please also reference the kernel commit which introduce the name change.
OK.

> 
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > ---
> >  drivers/net/mlx5/mlx5.h        | 11 ++++++-
> >  drivers/net/mlx5/mlx5_ethdev.c | 68 +++++++++++++++++++++++++++----
> > -----------
> >  drivers/net/mlx5/mlx5_nl.c     | 42 +++++++++++++++++---------
> >  3 files changed, 82 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > 8eb1019..81c02ce 100644
> > --- a/drivers/net/mlx5/mlx5.h
> > +++ b/drivers/net/mlx5/mlx5.h
> > @@ -80,11 +80,20 @@ struct mlx5_mp_param {
> >  /** Key string for IPC. */
> >  #define MLX5_MP_NAME "net_mlx5_mp"
> >
> > +/* Recognized Infiniband device physical port name types. */ enum
> > +mlx5_phys_port_name_type {
> > +	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN = 0, /* Unrecognized.
> > */
> > +	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0
> > */
> > +	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
> > +	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0
> > */ };
> > +
> >  /** Switch information returned by mlx5_nl_switch_info(). */  struct
> > mlx5_switch_info {
> >  	uint32_t master:1; /**< Master device. */
> >  	uint32_t representor:1; /**< Representor device. */
> > -	uint32_t port_name_new:1; /**< Rep. port name is in new format.
> > */
> > +	enum mlx5_phys_port_name_type name_type; /** < Port name
> > type. */
> > +	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
> >  	int32_t port_name; /**< Representor port name. */
> >  	uint64_t switch_id; /**< Switch identifier. */  }; diff --git
> > a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> > index 3992918..371989f 100644
> > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > @@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > *dev, char *fw_ver, size_t fw_size)
> >  	struct mlx5_switch_info data = {
> >  		.master = 0,
> >  		.representor = 0,
> > -		.port_name_new = 0,
> > +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
> >  		.port_name = 0,
> >  		.switch_id = 0,
> >  	};
> >  	DIR *dir;
> > -	bool port_name_set = false;
> >  	bool port_switch_id_set = false;
> >  	bool device_dir = false;
> >  	char c;
> > @@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev,
> > char *fw_ver, size_t fw_size)
> >  		ret = fscanf(file, "%s", port_name);
> >  		fclose(file);
> >  		if (ret == 1)
> > -			port_name_set =
> > mlx5_translate_port_name(port_name,
> > -								 &data);
> > +			mlx5_translate_port_name(port_name, &data);
> >  	}
> >  	file = fopen(phys_switch_id, "rb");
> >  	if (file == NULL) {
> > @@ -1440,8 +1438,15 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > *dev, char *fw_ver, size_t fw_size)
> >  		closedir(dir);
> >  		device_dir = true;
> >  	}
> > -	data.master = port_switch_id_set && (!port_name_set ||
> > device_dir);
> > -	data.representor = port_switch_id_set && port_name_set &&
> > !device_dir;
> > +	if (port_switch_id_set) {
> > +		data.master =
> > +			device_dir ||
> > +			data.name_type ==
> > MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN ||
> > +			data.name_type ==
> > MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> > +		data.representor = !device_dir &&
> > +			(data.name_type ==
> > MLX5_PHYS_PORT_NAME_TYPE_LEGACY ||
> > +			 data.name_type ==
> > MLX5_PHYS_PORT_NAME_TYPE_PFVF);
> 
> 
> Why we need to split the logic of the master/representor detection between
> the mlx5_translate_port_name and the caller function?

We have two stages:
- gathering the port attributes (name, vf_num, switchdev_id, presence of device directory, etc.)  in callbacks
- analyzing the parameters on gathering completion. We need all the parameters to make a reliable master/representor
 recognition.

Translate routine is called on gathering stage and just recognizes the name format, extracts useful values (pf/vf num) and stores the results in compact form. It is easier than storing the entire port name. 

> 
> The way I envision it is mlx5_tranlate_port_name receives the
> phys_port_name and maybe more metadata and return the port
This "metadata" may be not gathered yet at the moment of port name processing.

> classification (master/representor) and the representor/pf number.
> No need for data.master = some_logic(translate_port_name_info).
> 
> Inside the translate function I would expect to have 2 smaller function:
> 1. to handle the modern format (strings) 2. to handle the legacy format
> (integers)

Actually this patch is also some kind of preparation for coming subfuctions.
If we have set of mutual exclusive formats the switch/case seems to be the most
native way to treat these ones. I'd prefer to keep switch/case. Comments are clear
and it is easy to understand where legacy/new format is treated. And it is
easy to extend for coming subfunctions.

I think we should isolate master/representor recognition logic into separate
routine, "translate" routine is for parameter gathering stage, "recognize" for
analyzing.


> 
> > +	}
> >  	*info = data;
> >  	assert(!(data.master && data.representor));
> >  	if (data.master && data.representor) { @@ -1459,10 +1464,11 @@
> int
> > mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t
> > fw_size)
> >   * @param[in] port_name_in
> >   *   String representing the port name.
> >   * @param[out] port_info_out
> > - *   Port information, including port name as a number.
> > + *   Port information, including port name as a number and port name
> > + *   type if recognized
> >   *
> >   * @return
> > - *   true on success, false otherwise.
> > + *   true on success (if name format recognized), false otherwise.
> >   */
> >  bool
> >  mlx5_translate_port_name(const char *port_name_in, @@ -1470,25
> > +1476,39 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char
> > *fw_ver, size_t fw_size)  {
> >  	char pf_c1, pf_c2, vf_c1, vf_c2;
> >  	char *end;
> > -	int32_t pf_num;
> > -	bool port_name_set = false;
> > +	int sc_items;
> >
> >  	/*
> >  	 * Check for port-name as a string of the form pf0vf0
> > -	 * (support kernel ver >= 5.0)
> > +	 * (support kernel ver >= 5.0 or OFED ver >= 4.6).
> >  	 */
> > -	port_name_set =	(sscanf(port_name_in, "%c%c%d%c%c%d",
> > &pf_c1, &pf_c2,
> > -				&pf_num, &vf_c1, &vf_c2,
> > -				&port_info_out->port_name) == 6);
> > -	if (port_name_set) {
> > -		port_info_out->port_name_new = 1;
> > -	} else {
> > -		/* Check for port-name as a number (support kernel ver <
> > 5.0 */
> > -		errno = 0;
> > -		port_info_out->port_name = strtol(port_name_in, &end, 0);
> > -		if (!errno &&
> > -		    (size_t)(end - port_name_in) == strlen(port_name_in))
> > -			port_name_set = true;
> > +	sc_items = sscanf(port_name_in, "%c%c%d%c%c%d",
> > +			  &pf_c1, &pf_c2, &port_info_out->pf_num,
> > +			  &vf_c1, &vf_c2, &port_info_out->port_name);
> > +	if (sc_items == 6 &&
> > +	    pf_c1 == 'p' && pf_c2 == 'f' &&
> > +	    vf_c1 == 'v' && vf_c2 == 'f') {
> > +		port_info_out->name_type =
> > MLX5_PHYS_PORT_NAME_TYPE_PFVF;
> > +		return true;
> > +	}
> > +	/*
> > +	 * Check for port-name as a string of the form p0
> > +	 * (support kernel ver >= 5.0, or OFED ver >= 4.6).
> > +	 */
> > +	sc_items = sscanf(port_name_in, "%c%d",
> > +			  &pf_c1, &port_info_out->port_name);
> > +	if (sc_items == 2 && pf_c1 == 'p') {
> > +		port_info_out->name_type =
> > MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> > +		return true;
> > +	}
> > +	/* Check for port-name as a number (support kernel ver < 5.0 */
> > +	errno = 0;
> > +	port_info_out->port_name = strtol(port_name_in, &end, 0);
> > +	if (!errno &&
> > +	    (size_t)(end - port_name_in) == strlen(port_name_in)) {
> > +		port_info_out->name_type =
> > MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
> > +		return  true;
> >  	}
> > -	return port_name_set;
> > +	port_info_out->name_type =
> > MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
> > +	return false;
> >  }
> > diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c
> > index
> > fd9226b..669de76 100644
> > --- a/drivers/net/mlx5/mlx5_nl.c
> > +++ b/drivers/net/mlx5/mlx5_nl.c
> > @@ -887,12 +887,11 @@ struct mlx5_nl_ifindex_data {
> >  	struct mlx5_switch_info info = {
> >  		.master = 0,
> >  		.representor = 0,
> > -		.port_name_new = 0,
> > +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
> >  		.port_name = 0,
> >  		.switch_id = 0,
> >  	};
> >  	size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> > -	bool port_name_set = false;
> >  	bool switch_id_set = false;
> >  	bool num_vf_set = false;
> >
> > @@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
> >  			num_vf_set = true;
> >  			break;
> >  		case IFLA_PHYS_PORT_NAME:
> > -			port_name_set =
> > -				mlx5_translate_port_name((char *)payload,
> > -							 &info);
> > +			mlx5_translate_port_name((char *)payload, &info);
> >  			break;
> >  		case IFLA_PHYS_SWITCH_ID:
> >  			info.switch_id = 0;
> > @@ -926,16 +923,33 @@ struct mlx5_nl_ifindex_data {
> >  		off += RTA_ALIGN(ra->rta_len);
> >  	}
> >  	if (switch_id_set) {
> > -		if (info.port_name_new) {
> > -			/* New representors naming schema. */
> > -			if (port_name_set) {
> > -				info.master = (info.port_name == -1);
> > -				info.representor = (info.port_name != -1);
> > -			}
> > -		} else {
> > +		/* We have some E-Switch configuration. */
> > +		switch (info.name_type) {
> > +		case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
> > +			/*
> > +			 * Name is not recognized or not set,
> > +			 * it can not be representor, check
> > +			 * VF number to see if it is a master.
> > +			 */
> > +			info.master = num_vf_set;
> > +			break;
> > +		case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
> >  			/* Legacy representors naming schema. */
> > -			info.master = (!port_name_set || num_vf_set);
> > -			info.representor = port_name_set && !num_vf_set;
> > +			info.representor = !num_vf_set;
> > +			break;
> > +		case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
> > +			/* New uplink naming schema. */
> > +			info.master = 1;
> > +			break;
> > +		case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
> > +			/* New representors naming schema. */
> > +			info.representor = 1;
> > +			break;
> > +		}
> > +		if (!info.master && !info.representor) {
> > +			DRV_LOG(INFO,
> > +				"unable to recognize master/representors"
> > +				" on the device in switch domain");
> 
> Same comment as above. Would like to avoid this switch case outside of the
> translate function .
Don't you mind if we provide separated analyzing routine ?

> 
> >  		}
> >  	}
> >  	assert(!(info.master && info.representor));
> > --
> > 1.8.3.1

With best regards,
Slava

Re: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor

[Slava Ovsiienko]

Hi, Shahaf

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Sunday, April 14, 2019 10:43
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> representor
> 
> Hi Slava,
> 
> Friday, April 12, 2019 6:48 PM, Viacheslav Ovsiienko:
> > Subject: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> > representor
> >
> > On BlueField platform we have the new entity - PF representor.
> > This one represents the PCI PF attached to external host on the side of
> ARM.
> > The traffic sent by the external host to the NIC via PF will be seem
> > by ARM on this PF representor.
> >
> > This patch extends port recognizing capability on the base of physical
> > port name. The following naming formats are supported:
> >
> >   - missing physical port name (no sysfs/netlink key) at all,
> >     this is old style (before kernel 5.0) format, master assumed
> >   - 1 (decimal digits) - old style (before kernel 5.0) format,
> >     exists only for representors, master does not have physical
> >     port name at all (see above)
> >   - p0 ("p" followed by decimal digits), new style (kernel version
> >     is 5.0 or higher, Mellanox OFED 4.6 or higher) name format
> >     for uplink representor, plays the role of master
> >   - pf0vf0 ("pf" followed by PF index concatenated with "vf"
> >     followed by VF index),  new style (kernel version  is 5.0
> >     or higher, Mellanox OFED 4.6 or higher) name format for
> >     VF representor. If index of VF is "-1" it is a special case
> >     of host PF representor, this representor must be indexed in
> >     devargs as 65535, for example representor=[0-3,65535] will
> >     allow representors for VF0, VF1, VF2, VF3 and host PF.
> >     Note: do not specify representor=[0-65535] it causes devargs
> >     processing error, because number of ports (rte_eth_dev) is
> >     limited.
> >
> 
> The above is a bit complex to understand and in fact we have 2 modes:
> 1. legacy - phys_port_name are numbers. Master doesn't have
> phys_port_name 2. modern - phys_port_name are strings.
> uplink representor is p%d
> representors are (including PF representor) pf%dvf%d. the vf index for the PF
> representor is 65535.

OK, I'll try to update the commit message to make it more clear.
> 
> > Applications should distinguish representors and master devices
> > exclusively by device flag RTE_ETH_DEV_REPRESENTOR and do not rely on
> > switch port_id (mlx5 PMD deduces ones from representor_id) values
> > returned by
> > dev_infos_get() API.
> >
> 
> Please also reference the kernel commit which introduce the name change.
OK.

> 
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > ---
> >  drivers/net/mlx5/mlx5.h        | 11 ++++++-
> >  drivers/net/mlx5/mlx5_ethdev.c | 68 +++++++++++++++++++++++++++----
> > -----------
> >  drivers/net/mlx5/mlx5_nl.c     | 42 +++++++++++++++++---------
> >  3 files changed, 82 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > 8eb1019..81c02ce 100644
> > --- a/drivers/net/mlx5/mlx5.h
> > +++ b/drivers/net/mlx5/mlx5.h
> > @@ -80,11 +80,20 @@ struct mlx5_mp_param {
> >  /** Key string for IPC. */
> >  #define MLX5_MP_NAME "net_mlx5_mp"
> >
> > +/* Recognized Infiniband device physical port name types. */ enum
> > +mlx5_phys_port_name_type {
> > +	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN = 0, /* Unrecognized.
> > */
> > +	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0
> > */
> > +	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
> > +	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0
> > */ };
> > +
> >  /** Switch information returned by mlx5_nl_switch_info(). */  struct
> > mlx5_switch_info {
> >  	uint32_t master:1; /**< Master device. */
> >  	uint32_t representor:1; /**< Representor device. */
> > -	uint32_t port_name_new:1; /**< Rep. port name is in new format.
> > */
> > +	enum mlx5_phys_port_name_type name_type; /** < Port name
> > type. */
> > +	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
> >  	int32_t port_name; /**< Representor port name. */
> >  	uint64_t switch_id; /**< Switch identifier. */  }; diff --git
> > a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> > index 3992918..371989f 100644
> > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > @@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > *dev, char *fw_ver, size_t fw_size)
> >  	struct mlx5_switch_info data = {
> >  		.master = 0,
> >  		.representor = 0,
> > -		.port_name_new = 0,
> > +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
> >  		.port_name = 0,
> >  		.switch_id = 0,
> >  	};
> >  	DIR *dir;
> > -	bool port_name_set = false;
> >  	bool port_switch_id_set = false;
> >  	bool device_dir = false;
> >  	char c;
> > @@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev,
> > char *fw_ver, size_t fw_size)
> >  		ret = fscanf(file, "%s", port_name);
> >  		fclose(file);
> >  		if (ret == 1)
> > -			port_name_set =
> > mlx5_translate_port_name(port_name,
> > -								 &data);
> > +			mlx5_translate_port_name(port_name, &data);
> >  	}
> >  	file = fopen(phys_switch_id, "rb");
> >  	if (file == NULL) {
> > @@ -1440,8 +1438,15 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > *dev, char *fw_ver, size_t fw_size)
> >  		closedir(dir);
> >  		device_dir = true;
> >  	}
> > -	data.master = port_switch_id_set && (!port_name_set ||
> > device_dir);
> > -	data.representor = port_switch_id_set && port_name_set &&
> > !device_dir;
> > +	if (port_switch_id_set) {
> > +		data.master =
> > +			device_dir ||
> > +			data.name_type ==
> > MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN ||
> > +			data.name_type ==
> > MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> > +		data.representor = !device_dir &&
> > +			(data.name_type ==
> > MLX5_PHYS_PORT_NAME_TYPE_LEGACY ||
> > +			 data.name_type ==
> > MLX5_PHYS_PORT_NAME_TYPE_PFVF);
> 
> 
> Why we need to split the logic of the master/representor detection between
> the mlx5_translate_port_name and the caller function?

We have two stages:
- gathering the port attributes (name, vf_num, switchdev_id, presence of device directory, etc.)  in callbacks
- analyzing the parameters on gathering completion. We need all the parameters to make a reliable master/representor
 recognition.

Translate routine is called on gathering stage and just recognizes the name format, extracts useful values (pf/vf num) and stores the results in compact form. It is easier than storing the entire port name. 

> 
> The way I envision it is mlx5_tranlate_port_name receives the
> phys_port_name and maybe more metadata and return the port
This "metadata" may be not gathered yet at the moment of port name processing.

> classification (master/representor) and the representor/pf number.
> No need for data.master = some_logic(translate_port_name_info).
> 
> Inside the translate function I would expect to have 2 smaller function:
> 1. to handle the modern format (strings) 2. to handle the legacy format
> (integers)

Actually this patch is also some kind of preparation for coming subfuctions.
If we have set of mutual exclusive formats the switch/case seems to be the most
native way to treat these ones. I'd prefer to keep switch/case. Comments are clear
and it is easy to understand where legacy/new format is treated. And it is
easy to extend for coming subfunctions.

I think we should isolate master/representor recognition logic into separate
routine, "translate" routine is for parameter gathering stage, "recognize" for
analyzing.


> 
> > +	}
> >  	*info = data;
> >  	assert(!(data.master && data.representor));
> >  	if (data.master && data.representor) { @@ -1459,10 +1464,11 @@
> int
> > mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t
> > fw_size)
> >   * @param[in] port_name_in
> >   *   String representing the port name.
> >   * @param[out] port_info_out
> > - *   Port information, including port name as a number.
> > + *   Port information, including port name as a number and port name
> > + *   type if recognized
> >   *
> >   * @return
> > - *   true on success, false otherwise.
> > + *   true on success (if name format recognized), false otherwise.
> >   */
> >  bool
> >  mlx5_translate_port_name(const char *port_name_in, @@ -1470,25
> > +1476,39 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char
> > *fw_ver, size_t fw_size)  {
> >  	char pf_c1, pf_c2, vf_c1, vf_c2;
> >  	char *end;
> > -	int32_t pf_num;
> > -	bool port_name_set = false;
> > +	int sc_items;
> >
> >  	/*
> >  	 * Check for port-name as a string of the form pf0vf0
> > -	 * (support kernel ver >= 5.0)
> > +	 * (support kernel ver >= 5.0 or OFED ver >= 4.6).
> >  	 */
> > -	port_name_set =	(sscanf(port_name_in, "%c%c%d%c%c%d",
> > &pf_c1, &pf_c2,
> > -				&pf_num, &vf_c1, &vf_c2,
> > -				&port_info_out->port_name) == 6);
> > -	if (port_name_set) {
> > -		port_info_out->port_name_new = 1;
> > -	} else {
> > -		/* Check for port-name as a number (support kernel ver <
> > 5.0 */
> > -		errno = 0;
> > -		port_info_out->port_name = strtol(port_name_in, &end, 0);
> > -		if (!errno &&
> > -		    (size_t)(end - port_name_in) == strlen(port_name_in))
> > -			port_name_set = true;
> > +	sc_items = sscanf(port_name_in, "%c%c%d%c%c%d",
> > +			  &pf_c1, &pf_c2, &port_info_out->pf_num,
> > +			  &vf_c1, &vf_c2, &port_info_out->port_name);
> > +	if (sc_items == 6 &&
> > +	    pf_c1 == 'p' && pf_c2 == 'f' &&
> > +	    vf_c1 == 'v' && vf_c2 == 'f') {
> > +		port_info_out->name_type =
> > MLX5_PHYS_PORT_NAME_TYPE_PFVF;
> > +		return true;
> > +	}
> > +	/*
> > +	 * Check for port-name as a string of the form p0
> > +	 * (support kernel ver >= 5.0, or OFED ver >= 4.6).
> > +	 */
> > +	sc_items = sscanf(port_name_in, "%c%d",
> > +			  &pf_c1, &port_info_out->port_name);
> > +	if (sc_items == 2 && pf_c1 == 'p') {
> > +		port_info_out->name_type =
> > MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> > +		return true;
> > +	}
> > +	/* Check for port-name as a number (support kernel ver < 5.0 */
> > +	errno = 0;
> > +	port_info_out->port_name = strtol(port_name_in, &end, 0);
> > +	if (!errno &&
> > +	    (size_t)(end - port_name_in) == strlen(port_name_in)) {
> > +		port_info_out->name_type =
> > MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
> > +		return  true;
> >  	}
> > -	return port_name_set;
> > +	port_info_out->name_type =
> > MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
> > +	return false;
> >  }
> > diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c
> > index
> > fd9226b..669de76 100644
> > --- a/drivers/net/mlx5/mlx5_nl.c
> > +++ b/drivers/net/mlx5/mlx5_nl.c
> > @@ -887,12 +887,11 @@ struct mlx5_nl_ifindex_data {
> >  	struct mlx5_switch_info info = {
> >  		.master = 0,
> >  		.representor = 0,
> > -		.port_name_new = 0,
> > +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
> >  		.port_name = 0,
> >  		.switch_id = 0,
> >  	};
> >  	size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> > -	bool port_name_set = false;
> >  	bool switch_id_set = false;
> >  	bool num_vf_set = false;
> >
> > @@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
> >  			num_vf_set = true;
> >  			break;
> >  		case IFLA_PHYS_PORT_NAME:
> > -			port_name_set =
> > -				mlx5_translate_port_name((char *)payload,
> > -							 &info);
> > +			mlx5_translate_port_name((char *)payload, &info);
> >  			break;
> >  		case IFLA_PHYS_SWITCH_ID:
> >  			info.switch_id = 0;
> > @@ -926,16 +923,33 @@ struct mlx5_nl_ifindex_data {
> >  		off += RTA_ALIGN(ra->rta_len);
> >  	}
> >  	if (switch_id_set) {
> > -		if (info.port_name_new) {
> > -			/* New representors naming schema. */
> > -			if (port_name_set) {
> > -				info.master = (info.port_name == -1);
> > -				info.representor = (info.port_name != -1);
> > -			}
> > -		} else {
> > +		/* We have some E-Switch configuration. */
> > +		switch (info.name_type) {
> > +		case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
> > +			/*
> > +			 * Name is not recognized or not set,
> > +			 * it can not be representor, check
> > +			 * VF number to see if it is a master.
> > +			 */
> > +			info.master = num_vf_set;
> > +			break;
> > +		case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
> >  			/* Legacy representors naming schema. */
> > -			info.master = (!port_name_set || num_vf_set);
> > -			info.representor = port_name_set && !num_vf_set;
> > +			info.representor = !num_vf_set;
> > +			break;
> > +		case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
> > +			/* New uplink naming schema. */
> > +			info.master = 1;
> > +			break;
> > +		case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
> > +			/* New representors naming schema. */
> > +			info.representor = 1;
> > +			break;
> > +		}
> > +		if (!info.master && !info.representor) {
> > +			DRV_LOG(INFO,
> > +				"unable to recognize master/representors"
> > +				" on the device in switch domain");
> 
> Same comment as above. Would like to avoid this switch case outside of the
> translate function .
Don't you mind if we provide separated analyzing routine ?

> 
> >  		}
> >  	}
> >  	assert(!(info.master && info.representor));
> > --
> > 1.8.3.1

With best regards,
Slava

Re: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor

[Shahaf Shuler]

Monday, April 15, 2019 12:12 PM, Slava Ovsiienko:
> Subject: RE: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> representor
> 
> Hi, Shahaf
> 
> > -----Original Message-----
> > From: Shahaf Shuler
> > Sent: Sunday, April 14, 2019 10:43
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> > representor
> >
> > Hi Slava,
> >
> > Friday, April 12, 2019 6:48 PM, Viacheslav Ovsiienko:
> > > Subject: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> > > representor
> > >
> > > On BlueField platform we have the new entity - PF representor.
> > > This one represents the PCI PF attached to external host on the side
> > > of
> > ARM.
> > > The traffic sent by the external host to the NIC via PF will be seem
> > > by ARM on this PF representor.
> > >
> > > This patch extends port recognizing capability on the base of
> > > physical port name. The following naming formats are supported:
> > >
> > >   - missing physical port name (no sysfs/netlink key) at all,
> > >     this is old style (before kernel 5.0) format, master assumed
> > >   - 1 (decimal digits) - old style (before kernel 5.0) format,
> > >     exists only for representors, master does not have physical
> > >     port name at all (see above)
> > >   - p0 ("p" followed by decimal digits), new style (kernel version
> > >     is 5.0 or higher, Mellanox OFED 4.6 or higher) name format
> > >     for uplink representor, plays the role of master
> > >   - pf0vf0 ("pf" followed by PF index concatenated with "vf"
> > >     followed by VF index),  new style (kernel version  is 5.0
> > >     or higher, Mellanox OFED 4.6 or higher) name format for
> > >     VF representor. If index of VF is "-1" it is a special case
> > >     of host PF representor, this representor must be indexed in
> > >     devargs as 65535, for example representor=[0-3,65535] will
> > >     allow representors for VF0, VF1, VF2, VF3 and host PF.
> > >     Note: do not specify representor=[0-65535] it causes devargs
> > >     processing error, because number of ports (rte_eth_dev) is
> > >     limited.
> > >
> >
> > The above is a bit complex to understand and in fact we have 2 modes:
> > 1. legacy - phys_port_name are numbers. Master doesn't have
> > phys_port_name 2. modern - phys_port_name are strings.
> > uplink representor is p%d
> > representors are (including PF representor) pf%dvf%d. the vf index for
> > the PF representor is 65535.
> 
> OK, I'll try to update the commit message to make it more clear.
> >
> > > Applications should distinguish representors and master devices
> > > exclusively by device flag RTE_ETH_DEV_REPRESENTOR and do not rely
> > > on switch port_id (mlx5 PMD deduces ones from representor_id) values
> > > returned by
> > > dev_infos_get() API.
> > >
> >
> > Please also reference the kernel commit which introduce the name
> change.
> OK.
> 
> >
> > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > > ---
> > >  drivers/net/mlx5/mlx5.h        | 11 ++++++-
> > >  drivers/net/mlx5/mlx5_ethdev.c | 68
> +++++++++++++++++++++++++++----
> > > -----------
> > >  drivers/net/mlx5/mlx5_nl.c     | 42 +++++++++++++++++---------
> > >  3 files changed, 82 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > > 8eb1019..81c02ce 100644
> > > --- a/drivers/net/mlx5/mlx5.h
> > > +++ b/drivers/net/mlx5/mlx5.h
> > > @@ -80,11 +80,20 @@ struct mlx5_mp_param {
> > >  /** Key string for IPC. */
> > >  #define MLX5_MP_NAME "net_mlx5_mp"
> > >
> > > +/* Recognized Infiniband device physical port name types. */ enum
> > > +mlx5_phys_port_name_type {
> > > +	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN = 0, /* Unrecognized.
> > > */
> > > +	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0
> > > */
> > > +	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
> > > +	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0
> > > */ };
> > > +
> > >  /** Switch information returned by mlx5_nl_switch_info(). */
> > > struct mlx5_switch_info {
> > >  	uint32_t master:1; /**< Master device. */
> > >  	uint32_t representor:1; /**< Representor device. */
> > > -	uint32_t port_name_new:1; /**< Rep. port name is in new format.
> > > */
> > > +	enum mlx5_phys_port_name_type name_type; /** < Port name
> > > type. */
> > > +	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
> > >  	int32_t port_name; /**< Representor port name. */
> > >  	uint64_t switch_id; /**< Switch identifier. */  }; diff --git
> > > a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> > > index 3992918..371989f 100644
> > > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > > @@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > > *dev, char *fw_ver, size_t fw_size)
> > >  	struct mlx5_switch_info data = {
> > >  		.master = 0,
> > >  		.representor = 0,
> > > -		.port_name_new = 0,
> > > +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
> > >  		.port_name = 0,
> > >  		.switch_id = 0,
> > >  	};
> > >  	DIR *dir;
> > > -	bool port_name_set = false;
> > >  	bool port_switch_id_set = false;
> > >  	bool device_dir = false;
> > >  	char c;
> > > @@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > *dev,
> > > char *fw_ver, size_t fw_size)
> > >  		ret = fscanf(file, "%s", port_name);
> > >  		fclose(file);
> > >  		if (ret == 1)
> > > -			port_name_set =
> > > mlx5_translate_port_name(port_name,
> > > -								 &data);
> > > +			mlx5_translate_port_name(port_name, &data);
> > >  	}
> > >  	file = fopen(phys_switch_id, "rb");
> > >  	if (file == NULL) {
> > > @@ -1440,8 +1438,15 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > > *dev, char *fw_ver, size_t fw_size)
> > >  		closedir(dir);
> > >  		device_dir = true;
> > >  	}
> > > -	data.master = port_switch_id_set && (!port_name_set ||
> > > device_dir);
> > > -	data.representor = port_switch_id_set && port_name_set &&
> > > !device_dir;
> > > +	if (port_switch_id_set) {
> > > +		data.master =
> > > +			device_dir ||
> > > +			data.name_type ==
> > > MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN ||
> > > +			data.name_type ==
> > > MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> > > +		data.representor = !device_dir &&
> > > +			(data.name_type ==
> > > MLX5_PHYS_PORT_NAME_TYPE_LEGACY ||
> > > +			 data.name_type ==
> > > MLX5_PHYS_PORT_NAME_TYPE_PFVF);
> >
> >
> > Why we need to split the logic of the master/representor detection
> > between the mlx5_translate_port_name and the caller function?
> 
> We have two stages:
> - gathering the port attributes (name, vf_num, switchdev_id, presence of
> device directory, etc.)  in callbacks
> - analyzing the parameters on gathering completion. We need all the
> parameters to make a reliable master/representor  recognition.
> 
> Translate routine is called on gathering stage and just recognizes the name
> format, extracts useful values (pf/vf num) and stores the results in compact
> form. It is easier than storing the entire port name.
> 
> >
> > The way I envision it is mlx5_tranlate_port_name receives the
> > phys_port_name and maybe more metadata and return the port
> This "metadata" may be not gathered yet at the moment of port name
> processing.
> 
> > classification (master/representor) and the representor/pf number.
> > No need for data.master = some_logic(translate_port_name_info).
> >
> > Inside the translate function I would expect to have 2 smaller function:
> > 1. to handle the modern format (strings) 2. to handle the legacy
> > format
> > (integers)
> 
> Actually this patch is also some kind of preparation for coming subfuctions.
> If we have set of mutual exclusive formats the switch/case seems to be the
> most native way to treat these ones. I'd prefer to keep switch/case.
> Comments are clear and it is easy to understand where legacy/new format is
> treated. And it is easy to extend for coming subfunctions.
> 
> I think we should isolate master/representor recognition logic into separate
> routine, "translate" routine is for parameter gathering stage, "recognize" for
> analyzing.

If you insist to split it then we should have 2 separate routines, unlike the current approach where switch/case are spread in different places.

Re: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF representor

[Shahaf Shuler]

Monday, April 15, 2019 12:12 PM, Slava Ovsiienko:
> Subject: RE: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> representor
> 
> Hi, Shahaf
> 
> > -----Original Message-----
> > From: Shahaf Shuler
> > Sent: Sunday, April 14, 2019 10:43
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> > representor
> >
> > Hi Slava,
> >
> > Friday, April 12, 2019 6:48 PM, Viacheslav Ovsiienko:
> > > Subject: [dpdk-dev] [PATCH 1/1] net/mlx5: add support for PF
> > > representor
> > >
> > > On BlueField platform we have the new entity - PF representor.
> > > This one represents the PCI PF attached to external host on the side
> > > of
> > ARM.
> > > The traffic sent by the external host to the NIC via PF will be seem
> > > by ARM on this PF representor.
> > >
> > > This patch extends port recognizing capability on the base of
> > > physical port name. The following naming formats are supported:
> > >
> > >   - missing physical port name (no sysfs/netlink key) at all,
> > >     this is old style (before kernel 5.0) format, master assumed
> > >   - 1 (decimal digits) - old style (before kernel 5.0) format,
> > >     exists only for representors, master does not have physical
> > >     port name at all (see above)
> > >   - p0 ("p" followed by decimal digits), new style (kernel version
> > >     is 5.0 or higher, Mellanox OFED 4.6 or higher) name format
> > >     for uplink representor, plays the role of master
> > >   - pf0vf0 ("pf" followed by PF index concatenated with "vf"
> > >     followed by VF index),  new style (kernel version  is 5.0
> > >     or higher, Mellanox OFED 4.6 or higher) name format for
> > >     VF representor. If index of VF is "-1" it is a special case
> > >     of host PF representor, this representor must be indexed in
> > >     devargs as 65535, for example representor=[0-3,65535] will
> > >     allow representors for VF0, VF1, VF2, VF3 and host PF.
> > >     Note: do not specify representor=[0-65535] it causes devargs
> > >     processing error, because number of ports (rte_eth_dev) is
> > >     limited.
> > >
> >
> > The above is a bit complex to understand and in fact we have 2 modes:
> > 1. legacy - phys_port_name are numbers. Master doesn't have
> > phys_port_name 2. modern - phys_port_name are strings.
> > uplink representor is p%d
> > representors are (including PF representor) pf%dvf%d. the vf index for
> > the PF representor is 65535.
> 
> OK, I'll try to update the commit message to make it more clear.
> >
> > > Applications should distinguish representors and master devices
> > > exclusively by device flag RTE_ETH_DEV_REPRESENTOR and do not rely
> > > on switch port_id (mlx5 PMD deduces ones from representor_id) values
> > > returned by
> > > dev_infos_get() API.
> > >
> >
> > Please also reference the kernel commit which introduce the name
> change.
> OK.
> 
> >
> > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > > ---
> > >  drivers/net/mlx5/mlx5.h        | 11 ++++++-
> > >  drivers/net/mlx5/mlx5_ethdev.c | 68
> +++++++++++++++++++++++++++----
> > > -----------
> > >  drivers/net/mlx5/mlx5_nl.c     | 42 +++++++++++++++++---------
> > >  3 files changed, 82 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > > 8eb1019..81c02ce 100644
> > > --- a/drivers/net/mlx5/mlx5.h
> > > +++ b/drivers/net/mlx5/mlx5.h
> > > @@ -80,11 +80,20 @@ struct mlx5_mp_param {
> > >  /** Key string for IPC. */
> > >  #define MLX5_MP_NAME "net_mlx5_mp"
> > >
> > > +/* Recognized Infiniband device physical port name types. */ enum
> > > +mlx5_phys_port_name_type {
> > > +	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN = 0, /* Unrecognized.
> > > */
> > > +	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0
> > > */
> > > +	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
> > > +	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0
> > > */ };
> > > +
> > >  /** Switch information returned by mlx5_nl_switch_info(). */
> > > struct mlx5_switch_info {
> > >  	uint32_t master:1; /**< Master device. */
> > >  	uint32_t representor:1; /**< Representor device. */
> > > -	uint32_t port_name_new:1; /**< Rep. port name is in new format.
> > > */
> > > +	enum mlx5_phys_port_name_type name_type; /** < Port name
> > > type. */
> > > +	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
> > >  	int32_t port_name; /**< Representor port name. */
> > >  	uint64_t switch_id; /**< Switch identifier. */  }; diff --git
> > > a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> > > index 3992918..371989f 100644
> > > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > > @@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > > *dev, char *fw_ver, size_t fw_size)
> > >  	struct mlx5_switch_info data = {
> > >  		.master = 0,
> > >  		.representor = 0,
> > > -		.port_name_new = 0,
> > > +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN,
> > >  		.port_name = 0,
> > >  		.switch_id = 0,
> > >  	};
> > >  	DIR *dir;
> > > -	bool port_name_set = false;
> > >  	bool port_switch_id_set = false;
> > >  	bool device_dir = false;
> > >  	char c;
> > > @@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > *dev,
> > > char *fw_ver, size_t fw_size)
> > >  		ret = fscanf(file, "%s", port_name);
> > >  		fclose(file);
> > >  		if (ret == 1)
> > > -			port_name_set =
> > > mlx5_translate_port_name(port_name,
> > > -								 &data);
> > > +			mlx5_translate_port_name(port_name, &data);
> > >  	}
> > >  	file = fopen(phys_switch_id, "rb");
> > >  	if (file == NULL) {
> > > @@ -1440,8 +1438,15 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > > *dev, char *fw_ver, size_t fw_size)
> > >  		closedir(dir);
> > >  		device_dir = true;
> > >  	}
> > > -	data.master = port_switch_id_set && (!port_name_set ||
> > > device_dir);
> > > -	data.representor = port_switch_id_set && port_name_set &&
> > > !device_dir;
> > > +	if (port_switch_id_set) {
> > > +		data.master =
> > > +			device_dir ||
> > > +			data.name_type ==
> > > MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN ||
> > > +			data.name_type ==
> > > MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> > > +		data.representor = !device_dir &&
> > > +			(data.name_type ==
> > > MLX5_PHYS_PORT_NAME_TYPE_LEGACY ||
> > > +			 data.name_type ==
> > > MLX5_PHYS_PORT_NAME_TYPE_PFVF);
> >
> >
> > Why we need to split the logic of the master/representor detection
> > between the mlx5_translate_port_name and the caller function?
> 
> We have two stages:
> - gathering the port attributes (name, vf_num, switchdev_id, presence of
> device directory, etc.)  in callbacks
> - analyzing the parameters on gathering completion. We need all the
> parameters to make a reliable master/representor  recognition.
> 
> Translate routine is called on gathering stage and just recognizes the name
> format, extracts useful values (pf/vf num) and stores the results in compact
> form. It is easier than storing the entire port name.
> 
> >
> > The way I envision it is mlx5_tranlate_port_name receives the
> > phys_port_name and maybe more metadata and return the port
> This "metadata" may be not gathered yet at the moment of port name
> processing.
> 
> > classification (master/representor) and the representor/pf number.
> > No need for data.master = some_logic(translate_port_name_info).
> >
> > Inside the translate function I would expect to have 2 smaller function:
> > 1. to handle the modern format (strings) 2. to handle the legacy
> > format
> > (integers)
> 
> Actually this patch is also some kind of preparation for coming subfuctions.
> If we have set of mutual exclusive formats the switch/case seems to be the
> most native way to treat these ones. I'd prefer to keep switch/case.
> Comments are clear and it is easy to understand where legacy/new format is
> treated. And it is easy to extend for coming subfunctions.
> 
> I think we should isolate master/representor recognition logic into separate
> routine, "translate" routine is for parameter gathering stage, "recognize" for
> analyzing.

If you insist to split it then we should have 2 separate routines, unlike the current approach where switch/case are spread in different places.

[dpdk-dev] [PATCH v2] net/mlx5: add support for PF representor

[Viacheslav Ovsiienko]

On BlueField platform we have the new entity - PF representor.
This one represents the PCI PF attached to external host on the
side of ARM. The traffic sent by the external host to the NIC
via PF will be seem by ARM on this PF representor.

This patch refactors port recognizing capability on the base of
physical port name. We have two groups of name formats. Legacy
name formats are supported by kernels before ver 5.0 (being
more precise - before the patch [1]) or before Mellanox OFED 4.6,
and new naming formats added by the patch [1].

Legacy naming formats are supported:

  - missing physical port name (no sysfs/netlink key) at all,
    master is assumed

  - decimal digits (for example "12"), representor is assumed,
    the value is the index of attached VF

New naming formats are supported:

  - "p" followed by decimal digits, for example "p2", master
    is assumed

  - "pf" followed by PF index concatenated with "vf" followed by
    VF index, for example "pf0vf1", representor is assumed.
    If index of VF is "-1" it is a special case  of host PF
    representor, this representor must be indexed in devargs
    as 65535, for example representor=[0-3,65535] will
    allow representors for VF0, VF1, VF2, VF3 and for host PF.

    Note: do not specify representor=[0-65535], it causes devargs
    processing error, because number of ports (rte_eth_dev) is
    limited.

Applications should distinguish representors and master devices
exclusively by device flag RTE_ETH_DEV_REPRESENTOR and do not
rely on switch port_id (mlx5 PMD deduces ones from representor_id)
values returned by dev_infos_get() API.

[1] https://www.spinics.net/lists/netdev/msg547007.html
    Linux-tree: c12ecc23 (Or Gerlitz 2018-04-25 17:32 +0300)
    "net/mlx5e: Move to use common phys port names for vport representors"

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

---
v2: - clarified commit log message
    - separated port attributes analyzing routines
    - port name format related routines are gathered together

v1: http://patches.dpdk.org/patch/52725/

 drivers/net/mlx5/mlx5.h        |  18 ++++-
 drivers/net/mlx5/mlx5_ethdev.c | 163 ++++++++++++++++++++++++++++++++++-------
 drivers/net/mlx5/mlx5_nl.c     |  20 +----
 3 files changed, 158 insertions(+), 43 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 14c7f3c..2069fad 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -80,11 +80,21 @@ struct mlx5_mp_param {
 /** Key string for IPC. */
 #define MLX5_MP_NAME "net_mlx5_mp"
 
+/* Recognized Infiniband device physical port name types. */
+enum mlx5_phys_port_name_type {
+	MLX5_PHYS_PORT_NAME_TYPE_NOTSET = 0, /* Not set. */
+	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0 */
+	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
+	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0 */
+	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN, /* Unrecognized. */
+};
+
 /** Switch information returned by mlx5_nl_switch_info(). */
 struct mlx5_switch_info {
 	uint32_t master:1; /**< Master device. */
 	uint32_t representor:1; /**< Representor device. */
-	uint32_t port_name_new:1; /**< Rep. port name is in new format. */
+	enum mlx5_phys_port_name_type name_type; /** < Port name type. */
+	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
 	int32_t port_name; /**< Representor port name. */
 	uint64_t switch_id; /**< Switch identifier. */
 };
@@ -404,7 +414,11 @@ unsigned int mlx5_dev_to_port_id(const struct rte_device *dev,
 				 unsigned int port_list_n);
 int mlx5_sysfs_switch_info(unsigned int ifindex,
 			   struct mlx5_switch_info *info);
-bool mlx5_translate_port_name(const char *port_name_in,
+void mlx5_sysfs_check_switch_info(bool device_dir,
+				  struct mlx5_switch_info *switch_info);
+void mlx5_nl_check_switch_info(bool nun_vf_set,
+			       struct mlx5_switch_info *switch_info);
+void mlx5_translate_port_name(const char *port_name_in,
 			      struct mlx5_switch_info *port_info_out);
 
 /* mlx5_mac.c */
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 3992918..9c24462 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 	struct mlx5_switch_info data = {
 		.master = 0,
 		.representor = 0,
-		.port_name_new = 0,
+		.name_type = MLX5_PHYS_PORT_NAME_TYPE_NOTSET,
 		.port_name = 0,
 		.switch_id = 0,
 	};
 	DIR *dir;
-	bool port_name_set = false;
 	bool port_switch_id_set = false;
 	bool device_dir = false;
 	char c;
@@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 		ret = fscanf(file, "%s", port_name);
 		fclose(file);
 		if (ret == 1)
-			port_name_set = mlx5_translate_port_name(port_name,
-								 &data);
+			mlx5_translate_port_name(port_name, &data);
 	}
 	file = fopen(phys_switch_id, "rb");
 	if (file == NULL) {
@@ -1440,8 +1438,10 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 		closedir(dir);
 		device_dir = true;
 	}
-	data.master = port_switch_id_set && (!port_name_set || device_dir);
-	data.representor = port_switch_id_set && port_name_set && !device_dir;
+	if (port_switch_id_set) {
+		/* We have some E-Switch configuration. */
+		mlx5_sysfs_check_switch_info(device_dir, &data);
+	}
 	*info = data;
 	assert(!(data.master && data.representor));
 	if (data.master && data.representor) {
@@ -1454,41 +1454,154 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 }
 
 /**
+ * Analyze gathered port parameters via Netlink to recognize master
+ * and representor devices for E-Switch configuration.
+ *
+ * @param[in] num_vf_set
+ *   flag of presence of number of VFs port attribute.
+ * @param[inout] switch_info
+ *   Port information, including port name as a number and port name
+ *   type if recognized
+ *
+ * @return
+ *   master and representor flags are set in switch_info according to
+ *   recognized parameters (if any).
+ */
+void
+mlx5_nl_check_switch_info(bool num_vf_set,
+			  struct mlx5_switch_info *switch_info)
+{
+	switch (switch_info->name_type) {
+	case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
+		/*
+		 * Name is not recognized, assume the master,
+		 * check the number of VFs key presence.
+		 */
+		switch_info->master = num_vf_set;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_NOTSET:
+		/*
+		 * Name is not set, this assumes the legacy naming
+		 * schema for master, just check if there is a
+		 * number of VFs key.
+		 */
+		switch_info->master = num_vf_set;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
+		/* New uplink naming schema recognized. */
+		switch_info->master = 1;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
+		/* Legacy representors naming schema. */
+		switch_info->representor = !num_vf_set;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
+		/* New representors naming schema. */
+		switch_info->representor = 1;
+		break;
+	}
+}
+
+/**
+ * Analyze gathered port parameters via sysfs to recognize master
+ * and representor devices for E-Switch configuration.
+ *
+ * @param[in] device_dir
+ *   flag of presence of "device" directory under port device key.
+ * @param[inout] switch_info
+ *   Port information, including port name as a number and port name
+ *   type if recognized
+ *
+ * @return
+ *   master and representor flags are set in switch_info according to
+ *   recognized parameters (if any).
+ */
+void
+mlx5_sysfs_check_switch_info(bool device_dir,
+			     struct mlx5_switch_info *switch_info)
+{
+	switch (switch_info->name_type) {
+	case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
+		/*
+		 * Name is not recognized, assume the master,
+		 * check the device directory presence.
+		 */
+		switch_info->master = device_dir;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_NOTSET:
+		/*
+		 * Name is not set, this assumes the legacy naming
+		 * schema for master, just check if there is
+		 * a device directory.
+		 */
+		switch_info->master = device_dir;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
+		/* New uplink naming schema recognized. */
+		switch_info->master = 1;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
+		/* Legacy representors naming schema. */
+		switch_info->representor = !device_dir;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
+		/* New representors naming schema. */
+		switch_info->representor = 1;
+		break;
+	}
+}
+
+/**
  * Extract port name, as a number, from sysfs or netlink information.
  *
  * @param[in] port_name_in
  *   String representing the port name.
  * @param[out] port_info_out
- *   Port information, including port name as a number.
+ *   Port information, including port name as a number and port name
+ *   type if recognized
  *
  * @return
- *   true on success, false otherwise.
+ *   port_name field set according to recognized name format.
  */
-bool
+void
 mlx5_translate_port_name(const char *port_name_in,
 			 struct mlx5_switch_info *port_info_out)
 {
 	char pf_c1, pf_c2, vf_c1, vf_c2;
 	char *end;
-	int32_t pf_num;
-	bool port_name_set = false;
+	int sc_items;
 
 	/*
 	 * Check for port-name as a string of the form pf0vf0
-	 * (support kernel ver >= 5.0)
+	 * (support kernel ver >= 5.0 or OFED ver >= 4.6).
 	 */
-	port_name_set =	(sscanf(port_name_in, "%c%c%d%c%c%d", &pf_c1, &pf_c2,
-				&pf_num, &vf_c1, &vf_c2,
-				&port_info_out->port_name) == 6);
-	if (port_name_set) {
-		port_info_out->port_name_new = 1;
-	} else {
-		/* Check for port-name as a number (support kernel ver < 5.0 */
-		errno = 0;
-		port_info_out->port_name = strtol(port_name_in, &end, 0);
-		if (!errno &&
-		    (size_t)(end - port_name_in) == strlen(port_name_in))
-			port_name_set = true;
+	sc_items = sscanf(port_name_in, "%c%c%d%c%c%d",
+			  &pf_c1, &pf_c2, &port_info_out->pf_num,
+			  &vf_c1, &vf_c2, &port_info_out->port_name);
+	if (sc_items == 6 &&
+	    pf_c1 == 'p' && pf_c2 == 'f' &&
+	    vf_c1 == 'v' && vf_c2 == 'f') {
+		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_PFVF;
+		return;
+	}
+	/*
+	 * Check for port-name as a string of the form p0
+	 * (support kernel ver >= 5.0, or OFED ver >= 4.6).
+	 */
+	sc_items = sscanf(port_name_in, "%c%d",
+			  &pf_c1, &port_info_out->port_name);
+	if (sc_items == 2 && pf_c1 == 'p') {
+		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
+		return;
+	}
+	/* Check for port-name as a number (support kernel ver < 5.0 */
+	errno = 0;
+	port_info_out->port_name = strtol(port_name_in, &end, 0);
+	if (!errno &&
+	    (size_t)(end - port_name_in) == strlen(port_name_in)) {
+		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
+		return;
 	}
-	return port_name_set;
+	port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
+	return;
 }
diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c
index fd9226b..0ff9667 100644
--- a/drivers/net/mlx5/mlx5_nl.c
+++ b/drivers/net/mlx5/mlx5_nl.c
@@ -887,12 +887,11 @@ struct mlx5_nl_ifindex_data {
 	struct mlx5_switch_info info = {
 		.master = 0,
 		.representor = 0,
-		.port_name_new = 0,
+		.name_type = MLX5_PHYS_PORT_NAME_TYPE_NOTSET,
 		.port_name = 0,
 		.switch_id = 0,
 	};
 	size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
-	bool port_name_set = false;
 	bool switch_id_set = false;
 	bool num_vf_set = false;
 
@@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
 			num_vf_set = true;
 			break;
 		case IFLA_PHYS_PORT_NAME:
-			port_name_set =
-				mlx5_translate_port_name((char *)payload,
-							 &info);
+			mlx5_translate_port_name((char *)payload, &info);
 			break;
 		case IFLA_PHYS_SWITCH_ID:
 			info.switch_id = 0;
@@ -926,17 +923,8 @@ struct mlx5_nl_ifindex_data {
 		off += RTA_ALIGN(ra->rta_len);
 	}
 	if (switch_id_set) {
-		if (info.port_name_new) {
-			/* New representors naming schema. */
-			if (port_name_set) {
-				info.master = (info.port_name == -1);
-				info.representor = (info.port_name != -1);
-			}
-		} else {
-			/* Legacy representors naming schema. */
-			info.master = (!port_name_set || num_vf_set);
-			info.representor = port_name_set && !num_vf_set;
-		}
+		/* We have some E-Switch configuration. */
+		mlx5_nl_check_switch_info(num_vf_set, &info);
 	}
 	assert(!(info.master && info.representor));
 	memcpy(arg, &info, sizeof(info));
-- 
1.8.3.1

[dpdk-dev] [PATCH v2] net/mlx5: add support for PF representor

[Viacheslav Ovsiienko]

On BlueField platform we have the new entity - PF representor.
This one represents the PCI PF attached to external host on the
side of ARM. The traffic sent by the external host to the NIC
via PF will be seem by ARM on this PF representor.

This patch refactors port recognizing capability on the base of
physical port name. We have two groups of name formats. Legacy
name formats are supported by kernels before ver 5.0 (being
more precise - before the patch [1]) or before Mellanox OFED 4.6,
and new naming formats added by the patch [1].

Legacy naming formats are supported:

  - missing physical port name (no sysfs/netlink key) at all,
    master is assumed

  - decimal digits (for example "12"), representor is assumed,
    the value is the index of attached VF

New naming formats are supported:

  - "p" followed by decimal digits, for example "p2", master
    is assumed

  - "pf" followed by PF index concatenated with "vf" followed by
    VF index, for example "pf0vf1", representor is assumed.
    If index of VF is "-1" it is a special case  of host PF
    representor, this representor must be indexed in devargs
    as 65535, for example representor=[0-3,65535] will
    allow representors for VF0, VF1, VF2, VF3 and for host PF.

    Note: do not specify representor=[0-65535], it causes devargs
    processing error, because number of ports (rte_eth_dev) is
    limited.

Applications should distinguish representors and master devices
exclusively by device flag RTE_ETH_DEV_REPRESENTOR and do not
rely on switch port_id (mlx5 PMD deduces ones from representor_id)
values returned by dev_infos_get() API.

[1] https://www.spinics.net/lists/netdev/msg547007.html
    Linux-tree: c12ecc23 (Or Gerlitz 2018-04-25 17:32 +0300)
    "net/mlx5e: Move to use common phys port names for vport representors"

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

---
v2: - clarified commit log message
    - separated port attributes analyzing routines
    - port name format related routines are gathered together

v1: http://patches.dpdk.org/patch/52725/

 drivers/net/mlx5/mlx5.h        |  18 ++++-
 drivers/net/mlx5/mlx5_ethdev.c | 163 ++++++++++++++++++++++++++++++++++-------
 drivers/net/mlx5/mlx5_nl.c     |  20 +----
 3 files changed, 158 insertions(+), 43 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 14c7f3c..2069fad 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -80,11 +80,21 @@ struct mlx5_mp_param {
 /** Key string for IPC. */
 #define MLX5_MP_NAME "net_mlx5_mp"
 
+/* Recognized Infiniband device physical port name types. */
+enum mlx5_phys_port_name_type {
+	MLX5_PHYS_PORT_NAME_TYPE_NOTSET = 0, /* Not set. */
+	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0 */
+	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
+	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0 */
+	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN, /* Unrecognized. */
+};
+
 /** Switch information returned by mlx5_nl_switch_info(). */
 struct mlx5_switch_info {
 	uint32_t master:1; /**< Master device. */
 	uint32_t representor:1; /**< Representor device. */
-	uint32_t port_name_new:1; /**< Rep. port name is in new format. */
+	enum mlx5_phys_port_name_type name_type; /** < Port name type. */
+	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
 	int32_t port_name; /**< Representor port name. */
 	uint64_t switch_id; /**< Switch identifier. */
 };
@@ -404,7 +414,11 @@ unsigned int mlx5_dev_to_port_id(const struct rte_device *dev,
 				 unsigned int port_list_n);
 int mlx5_sysfs_switch_info(unsigned int ifindex,
 			   struct mlx5_switch_info *info);
-bool mlx5_translate_port_name(const char *port_name_in,
+void mlx5_sysfs_check_switch_info(bool device_dir,
+				  struct mlx5_switch_info *switch_info);
+void mlx5_nl_check_switch_info(bool nun_vf_set,
+			       struct mlx5_switch_info *switch_info);
+void mlx5_translate_port_name(const char *port_name_in,
 			      struct mlx5_switch_info *port_info_out);
 
 /* mlx5_mac.c */
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 3992918..9c24462 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 	struct mlx5_switch_info data = {
 		.master = 0,
 		.representor = 0,
-		.port_name_new = 0,
+		.name_type = MLX5_PHYS_PORT_NAME_TYPE_NOTSET,
 		.port_name = 0,
 		.switch_id = 0,
 	};
 	DIR *dir;
-	bool port_name_set = false;
 	bool port_switch_id_set = false;
 	bool device_dir = false;
 	char c;
@@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 		ret = fscanf(file, "%s", port_name);
 		fclose(file);
 		if (ret == 1)
-			port_name_set = mlx5_translate_port_name(port_name,
-								 &data);
+			mlx5_translate_port_name(port_name, &data);
 	}
 	file = fopen(phys_switch_id, "rb");
 	if (file == NULL) {
@@ -1440,8 +1438,10 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 		closedir(dir);
 		device_dir = true;
 	}
-	data.master = port_switch_id_set && (!port_name_set || device_dir);
-	data.representor = port_switch_id_set && port_name_set && !device_dir;
+	if (port_switch_id_set) {
+		/* We have some E-Switch configuration. */
+		mlx5_sysfs_check_switch_info(device_dir, &data);
+	}
 	*info = data;
 	assert(!(data.master && data.representor));
 	if (data.master && data.representor) {
@@ -1454,41 +1454,154 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t fw_size)
 }
 
 /**
+ * Analyze gathered port parameters via Netlink to recognize master
+ * and representor devices for E-Switch configuration.
+ *
+ * @param[in] num_vf_set
+ *   flag of presence of number of VFs port attribute.
+ * @param[inout] switch_info
+ *   Port information, including port name as a number and port name
+ *   type if recognized
+ *
+ * @return
+ *   master and representor flags are set in switch_info according to
+ *   recognized parameters (if any).
+ */
+void
+mlx5_nl_check_switch_info(bool num_vf_set,
+			  struct mlx5_switch_info *switch_info)
+{
+	switch (switch_info->name_type) {
+	case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
+		/*
+		 * Name is not recognized, assume the master,
+		 * check the number of VFs key presence.
+		 */
+		switch_info->master = num_vf_set;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_NOTSET:
+		/*
+		 * Name is not set, this assumes the legacy naming
+		 * schema for master, just check if there is a
+		 * number of VFs key.
+		 */
+		switch_info->master = num_vf_set;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
+		/* New uplink naming schema recognized. */
+		switch_info->master = 1;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
+		/* Legacy representors naming schema. */
+		switch_info->representor = !num_vf_set;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
+		/* New representors naming schema. */
+		switch_info->representor = 1;
+		break;
+	}
+}
+
+/**
+ * Analyze gathered port parameters via sysfs to recognize master
+ * and representor devices for E-Switch configuration.
+ *
+ * @param[in] device_dir
+ *   flag of presence of "device" directory under port device key.
+ * @param[inout] switch_info
+ *   Port information, including port name as a number and port name
+ *   type if recognized
+ *
+ * @return
+ *   master and representor flags are set in switch_info according to
+ *   recognized parameters (if any).
+ */
+void
+mlx5_sysfs_check_switch_info(bool device_dir,
+			     struct mlx5_switch_info *switch_info)
+{
+	switch (switch_info->name_type) {
+	case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
+		/*
+		 * Name is not recognized, assume the master,
+		 * check the device directory presence.
+		 */
+		switch_info->master = device_dir;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_NOTSET:
+		/*
+		 * Name is not set, this assumes the legacy naming
+		 * schema for master, just check if there is
+		 * a device directory.
+		 */
+		switch_info->master = device_dir;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
+		/* New uplink naming schema recognized. */
+		switch_info->master = 1;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
+		/* Legacy representors naming schema. */
+		switch_info->representor = !device_dir;
+		break;
+	case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
+		/* New representors naming schema. */
+		switch_info->representor = 1;
+		break;
+	}
+}
+
+/**
  * Extract port name, as a number, from sysfs or netlink information.
  *
  * @param[in] port_name_in
  *   String representing the port name.
  * @param[out] port_info_out
- *   Port information, including port name as a number.
+ *   Port information, including port name as a number and port name
+ *   type if recognized
  *
  * @return
- *   true on success, false otherwise.
+ *   port_name field set according to recognized name format.
  */
-bool
+void
 mlx5_translate_port_name(const char *port_name_in,
 			 struct mlx5_switch_info *port_info_out)
 {
 	char pf_c1, pf_c2, vf_c1, vf_c2;
 	char *end;
-	int32_t pf_num;
-	bool port_name_set = false;
+	int sc_items;
 
 	/*
 	 * Check for port-name as a string of the form pf0vf0
-	 * (support kernel ver >= 5.0)
+	 * (support kernel ver >= 5.0 or OFED ver >= 4.6).
 	 */
-	port_name_set =	(sscanf(port_name_in, "%c%c%d%c%c%d", &pf_c1, &pf_c2,
-				&pf_num, &vf_c1, &vf_c2,
-				&port_info_out->port_name) == 6);
-	if (port_name_set) {
-		port_info_out->port_name_new = 1;
-	} else {
-		/* Check for port-name as a number (support kernel ver < 5.0 */
-		errno = 0;
-		port_info_out->port_name = strtol(port_name_in, &end, 0);
-		if (!errno &&
-		    (size_t)(end - port_name_in) == strlen(port_name_in))
-			port_name_set = true;
+	sc_items = sscanf(port_name_in, "%c%c%d%c%c%d",
+			  &pf_c1, &pf_c2, &port_info_out->pf_num,
+			  &vf_c1, &vf_c2, &port_info_out->port_name);
+	if (sc_items == 6 &&
+	    pf_c1 == 'p' && pf_c2 == 'f' &&
+	    vf_c1 == 'v' && vf_c2 == 'f') {
+		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_PFVF;
+		return;
+	}
+	/*
+	 * Check for port-name as a string of the form p0
+	 * (support kernel ver >= 5.0, or OFED ver >= 4.6).
+	 */
+	sc_items = sscanf(port_name_in, "%c%d",
+			  &pf_c1, &port_info_out->port_name);
+	if (sc_items == 2 && pf_c1 == 'p') {
+		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
+		return;
+	}
+	/* Check for port-name as a number (support kernel ver < 5.0 */
+	errno = 0;
+	port_info_out->port_name = strtol(port_name_in, &end, 0);
+	if (!errno &&
+	    (size_t)(end - port_name_in) == strlen(port_name_in)) {
+		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
+		return;
 	}
-	return port_name_set;
+	port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
+	return;
 }
diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c
index fd9226b..0ff9667 100644
--- a/drivers/net/mlx5/mlx5_nl.c
+++ b/drivers/net/mlx5/mlx5_nl.c
@@ -887,12 +887,11 @@ struct mlx5_nl_ifindex_data {
 	struct mlx5_switch_info info = {
 		.master = 0,
 		.representor = 0,
-		.port_name_new = 0,
+		.name_type = MLX5_PHYS_PORT_NAME_TYPE_NOTSET,
 		.port_name = 0,
 		.switch_id = 0,
 	};
 	size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
-	bool port_name_set = false;
 	bool switch_id_set = false;
 	bool num_vf_set = false;
 
@@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
 			num_vf_set = true;
 			break;
 		case IFLA_PHYS_PORT_NAME:
-			port_name_set =
-				mlx5_translate_port_name((char *)payload,
-							 &info);
+			mlx5_translate_port_name((char *)payload, &info);
 			break;
 		case IFLA_PHYS_SWITCH_ID:
 			info.switch_id = 0;
@@ -926,17 +923,8 @@ struct mlx5_nl_ifindex_data {
 		off += RTA_ALIGN(ra->rta_len);
 	}
 	if (switch_id_set) {
-		if (info.port_name_new) {
-			/* New representors naming schema. */
-			if (port_name_set) {
-				info.master = (info.port_name == -1);
-				info.representor = (info.port_name != -1);
-			}
-		} else {
-			/* Legacy representors naming schema. */
-			info.master = (!port_name_set || num_vf_set);
-			info.representor = port_name_set && !num_vf_set;
-		}
+		/* We have some E-Switch configuration. */
+		mlx5_nl_check_switch_info(num_vf_set, &info);
 	}
 	assert(!(info.master && info.representor));
 	memcpy(arg, &info, sizeof(info));
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/mlx5: add support for PF representor

[Dekel Peled]

PSB minor comments.

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Viacheslav Ovsiienko
> Sent: Tuesday, April 16, 2019 5:10 PM
> To: dev@dpdk.org
> Cc: Shahaf Shuler <shahafs@mellanox.com>
> Subject: [dpdk-dev] [PATCH v2] net/mlx5: add support for PF representor
> 
> On BlueField platform we have the new entity - PF representor.
> This one represents the PCI PF attached to external host on the side of ARM.
> The traffic sent by the external host to the NIC via PF will be seem by ARM on
> this PF representor.
> 
> This patch refactors port recognizing capability on the base of physical port
> name. We have two groups of name formats. Legacy name formats are
> supported by kernels before ver 5.0 (being more precise - before the patch
> [1]) or before Mellanox OFED 4.6, and new naming formats added by the
> patch [1].
> 
> Legacy naming formats are supported:
> 
>   - missing physical port name (no sysfs/netlink key) at all,
>     master is assumed
> 
>   - decimal digits (for example "12"), representor is assumed,
>     the value is the index of attached VF
> 
> New naming formats are supported:
> 
>   - "p" followed by decimal digits, for example "p2", master
>     is assumed
> 
>   - "pf" followed by PF index concatenated with "vf" followed by
>     VF index, for example "pf0vf1", representor is assumed.
>     If index of VF is "-1" it is a special case  of host PF
>     representor, this representor must be indexed in devargs
>     as 65535, for example representor=[0-3,65535] will
>     allow representors for VF0, VF1, VF2, VF3 and for host PF.
> 
>     Note: do not specify representor=[0-65535], it causes devargs
>     processing error, because number of ports (rte_eth_dev) is
>     limited.
> 
> Applications should distinguish representors and master devices exclusively
> by device flag RTE_ETH_DEV_REPRESENTOR and do not rely on switch
> port_id (mlx5 PMD deduces ones from representor_id) values returned by
> dev_infos_get() API.
> 
> [1]
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> .spinics.net%2Flists%2Fnetdev%2Fmsg547007.html&amp;data=02%7C01%7C
> dekelp%40mellanox.com%7C1bb595beee23451f97ec08d6c275566b%7Ca6529
> 71c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636910206560409734&amp;sd
> ata=hdO9GhbFdoVt4pQND5ROXhTeDBhVhrNA1HyQbchsgvg%3D&amp;rese
> rved=0
>     Linux-tree: c12ecc23 (Or Gerlitz 2018-04-25 17:32 +0300)
>     "net/mlx5e: Move to use common phys port names for vport
> representors"
> 

I suggest to use this link instead:
https://github.com/torvalds/linux/commit/c12ecc2

> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> 
> ---
> v2: - clarified commit log message
>     - separated port attributes analyzing routines
>     - port name format related routines are gathered together
> 
> v1:
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> es.dpdk.org%2Fpatch%2F52725%2F&amp;data=02%7C01%7Cdekelp%40mell
> anox.com%7C1bb595beee23451f97ec08d6c275566b%7Ca652971c7d2e4d9ba6
> a4d149256f461b%7C0%7C0%7C636910206560409734&amp;sdata=pB9vIdQ4s
> VzLcKNL0flg8SnDP%2F%2BtisSJVCbj1RkkPwg%3D&amp;reserved=0
> 
>  drivers/net/mlx5/mlx5.h        |  18 ++++-
>  drivers/net/mlx5/mlx5_ethdev.c | 163
> ++++++++++++++++++++++++++++++++++-------
>  drivers/net/mlx5/mlx5_nl.c     |  20 +----
>  3 files changed, 158 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> 14c7f3c..2069fad 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -80,11 +80,21 @@ struct mlx5_mp_param {
>  /** Key string for IPC. */
>  #define MLX5_MP_NAME "net_mlx5_mp"
> 
> +/* Recognized Infiniband device physical port name types. */ enum
> +mlx5_phys_port_name_type {
> +	MLX5_PHYS_PORT_NAME_TYPE_NOTSET = 0, /* Not set. */
> +	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
> +	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN, /* Unrecognized. */ };
> +
>  /** Switch information returned by mlx5_nl_switch_info(). */  struct
> mlx5_switch_info {
>  	uint32_t master:1; /**< Master device. */
>  	uint32_t representor:1; /**< Representor device. */
> -	uint32_t port_name_new:1; /**< Rep. port name is in new format.
> */
> +	enum mlx5_phys_port_name_type name_type; /** < Port name
> type. */
> +	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
>  	int32_t port_name; /**< Representor port name. */
>  	uint64_t switch_id; /**< Switch identifier. */  }; @@ -404,7 +414,11
> @@ unsigned int mlx5_dev_to_port_id(const struct rte_device *dev,
>  				 unsigned int port_list_n);
>  int mlx5_sysfs_switch_info(unsigned int ifindex,
>  			   struct mlx5_switch_info *info);
> -bool mlx5_translate_port_name(const char *port_name_in,
> +void mlx5_sysfs_check_switch_info(bool device_dir,
> +				  struct mlx5_switch_info *switch_info); void
> +mlx5_nl_check_switch_info(bool nun_vf_set,
> +			       struct mlx5_switch_info *switch_info); void
> +mlx5_translate_port_name(const char *port_name_in,
>  			      struct mlx5_switch_info *port_info_out);
> 
>  /* mlx5_mac.c */
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> b/drivers/net/mlx5/mlx5_ethdev.c index 3992918..9c24462 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
>  	struct mlx5_switch_info data = {
>  		.master = 0,
>  		.representor = 0,
> -		.port_name_new = 0,
> +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_NOTSET,
>  		.port_name = 0,
>  		.switch_id = 0,
>  	};
>  	DIR *dir;
> -	bool port_name_set = false;
>  	bool port_switch_id_set = false;
>  	bool device_dir = false;
>  	char c;
> @@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev,
> char *fw_ver, size_t fw_size)
>  		ret = fscanf(file, "%s", port_name);
>  		fclose(file);
>  		if (ret == 1)
> -			port_name_set =
> mlx5_translate_port_name(port_name,
> -								 &data);
> +			mlx5_translate_port_name(port_name, &data);
>  	}
>  	file = fopen(phys_switch_id, "rb");
>  	if (file == NULL) {
> @@ -1440,8 +1438,10 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
>  		closedir(dir);
>  		device_dir = true;
>  	}
> -	data.master = port_switch_id_set && (!port_name_set ||
> device_dir);
> -	data.representor = port_switch_id_set && port_name_set &&
> !device_dir;
> +	if (port_switch_id_set) {
> +		/* We have some E-Switch configuration. */
> +		mlx5_sysfs_check_switch_info(device_dir, &data);
> +	}
>  	*info = data;
>  	assert(!(data.master && data.representor));
>  	if (data.master && data.representor) { @@ -1454,41 +1454,154 @@
> int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t
> fw_size)  }
> 
>  /**
> + * Analyze gathered port parameters via Netlink to recognize master
> + * and representor devices for E-Switch configuration.
> + *
> + * @param[in] num_vf_set
> + *   flag of presence of number of VFs port attribute.
> + * @param[inout] switch_info
> + *   Port information, including port name as a number and port name
> + *   type if recognized
> + *
> + * @return
> + *   master and representor flags are set in switch_info according to
> + *   recognized parameters (if any).
> + */
> +void
> +mlx5_nl_check_switch_info(bool num_vf_set,
> +			  struct mlx5_switch_info *switch_info) {
> +	switch (switch_info->name_type) {
> +	case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
> +		/*
> +		 * Name is not recognized, assume the master,
> +		 * check the number of VFs key presence.
> +		 */
> +		switch_info->master = num_vf_set;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_NOTSET:
> +		/*
> +		 * Name is not set, this assumes the legacy naming
> +		 * schema for master, just check if there is a
> +		 * number of VFs key.
> +		 */
> +		switch_info->master = num_vf_set;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
> +		/* New uplink naming schema recognized. */
> +		switch_info->master = 1;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
> +		/* Legacy representors naming schema. */
> +		switch_info->representor = !num_vf_set;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
> +		/* New representors naming schema. */
> +		switch_info->representor = 1;
> +		break;

Suggest to add default for clarity.

> +	}
> +}
> +
> +/**
> + * Analyze gathered port parameters via sysfs to recognize master
> + * and representor devices for E-Switch configuration.
> + *
> + * @param[in] device_dir
> + *   flag of presence of "device" directory under port device key.
> + * @param[inout] switch_info
> + *   Port information, including port name as a number and port name
> + *   type if recognized
> + *
> + * @return
> + *   master and representor flags are set in switch_info according to
> + *   recognized parameters (if any).
> + */
> +void
> +mlx5_sysfs_check_switch_info(bool device_dir,
> +			     struct mlx5_switch_info *switch_info) {
> +	switch (switch_info->name_type) {
> +	case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
> +		/*
> +		 * Name is not recognized, assume the master,
> +		 * check the device directory presence.
> +		 */
> +		switch_info->master = device_dir;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_NOTSET:
> +		/*
> +		 * Name is not set, this assumes the legacy naming
> +		 * schema for master, just check if there is
> +		 * a device directory.
> +		 */
> +		switch_info->master = device_dir;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
> +		/* New uplink naming schema recognized. */
> +		switch_info->master = 1;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
> +		/* Legacy representors naming schema. */
> +		switch_info->representor = !device_dir;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
> +		/* New representors naming schema. */
> +		switch_info->representor = 1;
> +		break;

Suggest to add default for clarity.

> +	}
> +}
> +
> +/**
>   * Extract port name, as a number, from sysfs or netlink information.
>   *
>   * @param[in] port_name_in
>   *   String representing the port name.
>   * @param[out] port_info_out
> - *   Port information, including port name as a number.
> + *   Port information, including port name as a number and port name
> + *   type if recognized
>   *
>   * @return
> - *   true on success, false otherwise.
> + *   port_name field set according to recognized name format.
>   */
> -bool
> +void
>  mlx5_translate_port_name(const char *port_name_in,
>  			 struct mlx5_switch_info *port_info_out)  {
>  	char pf_c1, pf_c2, vf_c1, vf_c2;
>  	char *end;
> -	int32_t pf_num;
> -	bool port_name_set = false;
> +	int sc_items;
> 
>  	/*
>  	 * Check for port-name as a string of the form pf0vf0
> -	 * (support kernel ver >= 5.0)
> +	 * (support kernel ver >= 5.0 or OFED ver >= 4.6).
>  	 */
> -	port_name_set =	(sscanf(port_name_in, "%c%c%d%c%c%d",
> &pf_c1, &pf_c2,
> -				&pf_num, &vf_c1, &vf_c2,
> -				&port_info_out->port_name) == 6);
> -	if (port_name_set) {
> -		port_info_out->port_name_new = 1;
> -	} else {
> -		/* Check for port-name as a number (support kernel ver <
> 5.0 */
> -		errno = 0;
> -		port_info_out->port_name = strtol(port_name_in, &end, 0);
> -		if (!errno &&
> -		    (size_t)(end - port_name_in) == strlen(port_name_in))
> -			port_name_set = true;
> +	sc_items = sscanf(port_name_in, "%c%c%d%c%c%d",
> +			  &pf_c1, &pf_c2, &port_info_out->pf_num,
> +			  &vf_c1, &vf_c2, &port_info_out->port_name);
> +	if (sc_items == 6 &&
> +	    pf_c1 == 'p' && pf_c2 == 'f' &&
> +	    vf_c1 == 'v' && vf_c2 == 'f') {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_PFVF;
> +		return;
> +	}
> +	/*
> +	 * Check for port-name as a string of the form p0
> +	 * (support kernel ver >= 5.0, or OFED ver >= 4.6).
> +	 */
> +	sc_items = sscanf(port_name_in, "%c%d",
> +			  &pf_c1, &port_info_out->port_name);
> +	if (sc_items == 2 && pf_c1 == 'p') {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> +		return;
> +	}
> +	/* Check for port-name as a number (support kernel ver < 5.0 */
> +	errno = 0;
> +	port_info_out->port_name = strtol(port_name_in, &end, 0);
> +	if (!errno &&
> +	    (size_t)(end - port_name_in) == strlen(port_name_in)) {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
> +		return;
>  	}
> -	return port_name_set;
> +	port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
> +	return;
>  }
> diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c index
> fd9226b..0ff9667 100644
> --- a/drivers/net/mlx5/mlx5_nl.c
> +++ b/drivers/net/mlx5/mlx5_nl.c
> @@ -887,12 +887,11 @@ struct mlx5_nl_ifindex_data {
>  	struct mlx5_switch_info info = {
>  		.master = 0,
>  		.representor = 0,
> -		.port_name_new = 0,
> +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_NOTSET,
>  		.port_name = 0,
>  		.switch_id = 0,
>  	};
>  	size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> -	bool port_name_set = false;
>  	bool switch_id_set = false;
>  	bool num_vf_set = false;
> 
> @@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
>  			num_vf_set = true;
>  			break;
>  		case IFLA_PHYS_PORT_NAME:
> -			port_name_set =
> -				mlx5_translate_port_name((char *)payload,
> -							 &info);
> +			mlx5_translate_port_name((char *)payload, &info);
>  			break;
>  		case IFLA_PHYS_SWITCH_ID:
>  			info.switch_id = 0;
> @@ -926,17 +923,8 @@ struct mlx5_nl_ifindex_data {
>  		off += RTA_ALIGN(ra->rta_len);
>  	}
>  	if (switch_id_set) {
> -		if (info.port_name_new) {
> -			/* New representors naming schema. */
> -			if (port_name_set) {
> -				info.master = (info.port_name == -1);
> -				info.representor = (info.port_name != -1);
> -			}
> -		} else {
> -			/* Legacy representors naming schema. */
> -			info.master = (!port_name_set || num_vf_set);
> -			info.representor = port_name_set && !num_vf_set;
> -		}
> +		/* We have some E-Switch configuration. */
> +		mlx5_nl_check_switch_info(num_vf_set, &info);
>  	}
>  	assert(!(info.master && info.representor));
>  	memcpy(arg, &info, sizeof(info));
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/mlx5: add support for PF representor

[Dekel Peled]

PSB minor comments.

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Viacheslav Ovsiienko
> Sent: Tuesday, April 16, 2019 5:10 PM
> To: dev@dpdk.org
> Cc: Shahaf Shuler <shahafs@mellanox.com>
> Subject: [dpdk-dev] [PATCH v2] net/mlx5: add support for PF representor
> 
> On BlueField platform we have the new entity - PF representor.
> This one represents the PCI PF attached to external host on the side of ARM.
> The traffic sent by the external host to the NIC via PF will be seem by ARM on
> this PF representor.
> 
> This patch refactors port recognizing capability on the base of physical port
> name. We have two groups of name formats. Legacy name formats are
> supported by kernels before ver 5.0 (being more precise - before the patch
> [1]) or before Mellanox OFED 4.6, and new naming formats added by the
> patch [1].
> 
> Legacy naming formats are supported:
> 
>   - missing physical port name (no sysfs/netlink key) at all,
>     master is assumed
> 
>   - decimal digits (for example "12"), representor is assumed,
>     the value is the index of attached VF
> 
> New naming formats are supported:
> 
>   - "p" followed by decimal digits, for example "p2", master
>     is assumed
> 
>   - "pf" followed by PF index concatenated with "vf" followed by
>     VF index, for example "pf0vf1", representor is assumed.
>     If index of VF is "-1" it is a special case  of host PF
>     representor, this representor must be indexed in devargs
>     as 65535, for example representor=[0-3,65535] will
>     allow representors for VF0, VF1, VF2, VF3 and for host PF.
> 
>     Note: do not specify representor=[0-65535], it causes devargs
>     processing error, because number of ports (rte_eth_dev) is
>     limited.
> 
> Applications should distinguish representors and master devices exclusively
> by device flag RTE_ETH_DEV_REPRESENTOR and do not rely on switch
> port_id (mlx5 PMD deduces ones from representor_id) values returned by
> dev_infos_get() API.
> 
> [1]
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> .spinics.net%2Flists%2Fnetdev%2Fmsg547007.html&amp;data=02%7C01%7C
> dekelp%40mellanox.com%7C1bb595beee23451f97ec08d6c275566b%7Ca6529
> 71c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636910206560409734&amp;sd
> ata=hdO9GhbFdoVt4pQND5ROXhTeDBhVhrNA1HyQbchsgvg%3D&amp;rese
> rved=0
>     Linux-tree: c12ecc23 (Or Gerlitz 2018-04-25 17:32 +0300)
>     "net/mlx5e: Move to use common phys port names for vport
> representors"
> 

I suggest to use this link instead:
https://github.com/torvalds/linux/commit/c12ecc2

> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> 
> ---
> v2: - clarified commit log message
>     - separated port attributes analyzing routines
>     - port name format related routines are gathered together
> 
> v1:
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> es.dpdk.org%2Fpatch%2F52725%2F&amp;data=02%7C01%7Cdekelp%40mell
> anox.com%7C1bb595beee23451f97ec08d6c275566b%7Ca652971c7d2e4d9ba6
> a4d149256f461b%7C0%7C0%7C636910206560409734&amp;sdata=pB9vIdQ4s
> VzLcKNL0flg8SnDP%2F%2BtisSJVCbj1RkkPwg%3D&amp;reserved=0
> 
>  drivers/net/mlx5/mlx5.h        |  18 ++++-
>  drivers/net/mlx5/mlx5_ethdev.c | 163
> ++++++++++++++++++++++++++++++++++-------
>  drivers/net/mlx5/mlx5_nl.c     |  20 +----
>  3 files changed, 158 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> 14c7f3c..2069fad 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -80,11 +80,21 @@ struct mlx5_mp_param {
>  /** Key string for IPC. */
>  #define MLX5_MP_NAME "net_mlx5_mp"
> 
> +/* Recognized Infiniband device physical port name types. */ enum
> +mlx5_phys_port_name_type {
> +	MLX5_PHYS_PORT_NAME_TYPE_NOTSET = 0, /* Not set. */
> +	MLX5_PHYS_PORT_NAME_TYPE_LEGACY, /* before kernel ver < 5.0
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_UPLINK, /* p0, kernel ver >= 5.0 */
> +	MLX5_PHYS_PORT_NAME_TYPE_PFVF, /* pf0vf0, kernel ver >= 5.0
> */
> +	MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN, /* Unrecognized. */ };
> +
>  /** Switch information returned by mlx5_nl_switch_info(). */  struct
> mlx5_switch_info {
>  	uint32_t master:1; /**< Master device. */
>  	uint32_t representor:1; /**< Representor device. */
> -	uint32_t port_name_new:1; /**< Rep. port name is in new format.
> */
> +	enum mlx5_phys_port_name_type name_type; /** < Port name
> type. */
> +	int32_t pf_num; /**< PF number (valid for pfxvfx format only). */
>  	int32_t port_name; /**< Representor port name. */
>  	uint64_t switch_id; /**< Switch identifier. */  }; @@ -404,7 +414,11
> @@ unsigned int mlx5_dev_to_port_id(const struct rte_device *dev,
>  				 unsigned int port_list_n);
>  int mlx5_sysfs_switch_info(unsigned int ifindex,
>  			   struct mlx5_switch_info *info);
> -bool mlx5_translate_port_name(const char *port_name_in,
> +void mlx5_sysfs_check_switch_info(bool device_dir,
> +				  struct mlx5_switch_info *switch_info); void
> +mlx5_nl_check_switch_info(bool nun_vf_set,
> +			       struct mlx5_switch_info *switch_info); void
> +mlx5_translate_port_name(const char *port_name_in,
>  			      struct mlx5_switch_info *port_info_out);
> 
>  /* mlx5_mac.c */
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> b/drivers/net/mlx5/mlx5_ethdev.c index 3992918..9c24462 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -1395,12 +1395,11 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
>  	struct mlx5_switch_info data = {
>  		.master = 0,
>  		.representor = 0,
> -		.port_name_new = 0,
> +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_NOTSET,
>  		.port_name = 0,
>  		.switch_id = 0,
>  	};
>  	DIR *dir;
> -	bool port_name_set = false;
>  	bool port_switch_id_set = false;
>  	bool device_dir = false;
>  	char c;
> @@ -1423,8 +1422,7 @@ int mlx5_fw_version_get(struct rte_eth_dev *dev,
> char *fw_ver, size_t fw_size)
>  		ret = fscanf(file, "%s", port_name);
>  		fclose(file);
>  		if (ret == 1)
> -			port_name_set =
> mlx5_translate_port_name(port_name,
> -								 &data);
> +			mlx5_translate_port_name(port_name, &data);
>  	}
>  	file = fopen(phys_switch_id, "rb");
>  	if (file == NULL) {
> @@ -1440,8 +1438,10 @@ int mlx5_fw_version_get(struct rte_eth_dev
> *dev, char *fw_ver, size_t fw_size)
>  		closedir(dir);
>  		device_dir = true;
>  	}
> -	data.master = port_switch_id_set && (!port_name_set ||
> device_dir);
> -	data.representor = port_switch_id_set && port_name_set &&
> !device_dir;
> +	if (port_switch_id_set) {
> +		/* We have some E-Switch configuration. */
> +		mlx5_sysfs_check_switch_info(device_dir, &data);
> +	}
>  	*info = data;
>  	assert(!(data.master && data.representor));
>  	if (data.master && data.representor) { @@ -1454,41 +1454,154 @@
> int mlx5_fw_version_get(struct rte_eth_dev *dev, char *fw_ver, size_t
> fw_size)  }
> 
>  /**
> + * Analyze gathered port parameters via Netlink to recognize master
> + * and representor devices for E-Switch configuration.
> + *
> + * @param[in] num_vf_set
> + *   flag of presence of number of VFs port attribute.
> + * @param[inout] switch_info
> + *   Port information, including port name as a number and port name
> + *   type if recognized
> + *
> + * @return
> + *   master and representor flags are set in switch_info according to
> + *   recognized parameters (if any).
> + */
> +void
> +mlx5_nl_check_switch_info(bool num_vf_set,
> +			  struct mlx5_switch_info *switch_info) {
> +	switch (switch_info->name_type) {
> +	case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
> +		/*
> +		 * Name is not recognized, assume the master,
> +		 * check the number of VFs key presence.
> +		 */
> +		switch_info->master = num_vf_set;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_NOTSET:
> +		/*
> +		 * Name is not set, this assumes the legacy naming
> +		 * schema for master, just check if there is a
> +		 * number of VFs key.
> +		 */
> +		switch_info->master = num_vf_set;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
> +		/* New uplink naming schema recognized. */
> +		switch_info->master = 1;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
> +		/* Legacy representors naming schema. */
> +		switch_info->representor = !num_vf_set;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
> +		/* New representors naming schema. */
> +		switch_info->representor = 1;
> +		break;

Suggest to add default for clarity.

> +	}
> +}
> +
> +/**
> + * Analyze gathered port parameters via sysfs to recognize master
> + * and representor devices for E-Switch configuration.
> + *
> + * @param[in] device_dir
> + *   flag of presence of "device" directory under port device key.
> + * @param[inout] switch_info
> + *   Port information, including port name as a number and port name
> + *   type if recognized
> + *
> + * @return
> + *   master and representor flags are set in switch_info according to
> + *   recognized parameters (if any).
> + */
> +void
> +mlx5_sysfs_check_switch_info(bool device_dir,
> +			     struct mlx5_switch_info *switch_info) {
> +	switch (switch_info->name_type) {
> +	case MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN:
> +		/*
> +		 * Name is not recognized, assume the master,
> +		 * check the device directory presence.
> +		 */
> +		switch_info->master = device_dir;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_NOTSET:
> +		/*
> +		 * Name is not set, this assumes the legacy naming
> +		 * schema for master, just check if there is
> +		 * a device directory.
> +		 */
> +		switch_info->master = device_dir;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_UPLINK:
> +		/* New uplink naming schema recognized. */
> +		switch_info->master = 1;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_LEGACY:
> +		/* Legacy representors naming schema. */
> +		switch_info->representor = !device_dir;
> +		break;
> +	case MLX5_PHYS_PORT_NAME_TYPE_PFVF:
> +		/* New representors naming schema. */
> +		switch_info->representor = 1;
> +		break;

Suggest to add default for clarity.

> +	}
> +}
> +
> +/**
>   * Extract port name, as a number, from sysfs or netlink information.
>   *
>   * @param[in] port_name_in
>   *   String representing the port name.
>   * @param[out] port_info_out
> - *   Port information, including port name as a number.
> + *   Port information, including port name as a number and port name
> + *   type if recognized
>   *
>   * @return
> - *   true on success, false otherwise.
> + *   port_name field set according to recognized name format.
>   */
> -bool
> +void
>  mlx5_translate_port_name(const char *port_name_in,
>  			 struct mlx5_switch_info *port_info_out)  {
>  	char pf_c1, pf_c2, vf_c1, vf_c2;
>  	char *end;
> -	int32_t pf_num;
> -	bool port_name_set = false;
> +	int sc_items;
> 
>  	/*
>  	 * Check for port-name as a string of the form pf0vf0
> -	 * (support kernel ver >= 5.0)
> +	 * (support kernel ver >= 5.0 or OFED ver >= 4.6).
>  	 */
> -	port_name_set =	(sscanf(port_name_in, "%c%c%d%c%c%d",
> &pf_c1, &pf_c2,
> -				&pf_num, &vf_c1, &vf_c2,
> -				&port_info_out->port_name) == 6);
> -	if (port_name_set) {
> -		port_info_out->port_name_new = 1;
> -	} else {
> -		/* Check for port-name as a number (support kernel ver <
> 5.0 */
> -		errno = 0;
> -		port_info_out->port_name = strtol(port_name_in, &end, 0);
> -		if (!errno &&
> -		    (size_t)(end - port_name_in) == strlen(port_name_in))
> -			port_name_set = true;
> +	sc_items = sscanf(port_name_in, "%c%c%d%c%c%d",
> +			  &pf_c1, &pf_c2, &port_info_out->pf_num,
> +			  &vf_c1, &vf_c2, &port_info_out->port_name);
> +	if (sc_items == 6 &&
> +	    pf_c1 == 'p' && pf_c2 == 'f' &&
> +	    vf_c1 == 'v' && vf_c2 == 'f') {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_PFVF;
> +		return;
> +	}
> +	/*
> +	 * Check for port-name as a string of the form p0
> +	 * (support kernel ver >= 5.0, or OFED ver >= 4.6).
> +	 */
> +	sc_items = sscanf(port_name_in, "%c%d",
> +			  &pf_c1, &port_info_out->port_name);
> +	if (sc_items == 2 && pf_c1 == 'p') {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
> +		return;
> +	}
> +	/* Check for port-name as a number (support kernel ver < 5.0 */
> +	errno = 0;
> +	port_info_out->port_name = strtol(port_name_in, &end, 0);
> +	if (!errno &&
> +	    (size_t)(end - port_name_in) == strlen(port_name_in)) {
> +		port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_LEGACY;
> +		return;
>  	}
> -	return port_name_set;
> +	port_info_out->name_type =
> MLX5_PHYS_PORT_NAME_TYPE_UNKNOWN;
> +	return;
>  }
> diff --git a/drivers/net/mlx5/mlx5_nl.c b/drivers/net/mlx5/mlx5_nl.c index
> fd9226b..0ff9667 100644
> --- a/drivers/net/mlx5/mlx5_nl.c
> +++ b/drivers/net/mlx5/mlx5_nl.c
> @@ -887,12 +887,11 @@ struct mlx5_nl_ifindex_data {
>  	struct mlx5_switch_info info = {
>  		.master = 0,
>  		.representor = 0,
> -		.port_name_new = 0,
> +		.name_type = MLX5_PHYS_PORT_NAME_TYPE_NOTSET,
>  		.port_name = 0,
>  		.switch_id = 0,
>  	};
>  	size_t off = NLMSG_LENGTH(sizeof(struct ifinfomsg));
> -	bool port_name_set = false;
>  	bool switch_id_set = false;
>  	bool num_vf_set = false;
> 
> @@ -910,9 +909,7 @@ struct mlx5_nl_ifindex_data {
>  			num_vf_set = true;
>  			break;
>  		case IFLA_PHYS_PORT_NAME:
> -			port_name_set =
> -				mlx5_translate_port_name((char *)payload,
> -							 &info);
> +			mlx5_translate_port_name((char *)payload, &info);
>  			break;
>  		case IFLA_PHYS_SWITCH_ID:
>  			info.switch_id = 0;
> @@ -926,17 +923,8 @@ struct mlx5_nl_ifindex_data {
>  		off += RTA_ALIGN(ra->rta_len);
>  	}
>  	if (switch_id_set) {
> -		if (info.port_name_new) {
> -			/* New representors naming schema. */
> -			if (port_name_set) {
> -				info.master = (info.port_name == -1);
> -				info.representor = (info.port_name != -1);
> -			}
> -		} else {
> -			/* Legacy representors naming schema. */
> -			info.master = (!port_name_set || num_vf_set);
> -			info.representor = port_name_set && !num_vf_set;
> -		}
> +		/* We have some E-Switch configuration. */
> +		mlx5_nl_check_switch_info(num_vf_set, &info);
>  	}
>  	assert(!(info.master && info.representor));
>  	memcpy(arg, &info, sizeof(info));
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/mlx5: add support for PF representor

[Shahaf Shuler]

Tuesday, April 16, 2019 5:10 PM, Viacheslav Ovsiienko:
> Subject: [dpdk-dev] [PATCH v2] net/mlx5: add support for PF representor
> 
> On BlueField platform we have the new entity - PF representor.
> This one represents the PCI PF attached to external host on the side of ARM.
> The traffic sent by the external host to the NIC via PF will be seem by ARM on
> this PF representor.
> 
> This patch refactors port recognizing capability on the base of physical port
> name. We have two groups of name formats. Legacy name formats are
> supported by kernels before ver 5.0 (being more precise - before the patch
> [1]) or before Mellanox OFED 4.6, and new naming formats added by the
> patch [1].
> 
> Legacy naming formats are supported:
> 
>   - missing physical port name (no sysfs/netlink key) at all,
>     master is assumed
> 
>   - decimal digits (for example "12"), representor is assumed,
>     the value is the index of attached VF
> 
> New naming formats are supported:
> 
>   - "p" followed by decimal digits, for example "p2", master
>     is assumed
> 
>   - "pf" followed by PF index concatenated with "vf" followed by
>     VF index, for example "pf0vf1", representor is assumed.
>     If index of VF is "-1" it is a special case  of host PF
>     representor, this representor must be indexed in devargs
>     as 65535, for example representor=[0-3,65535] will
>     allow representors for VF0, VF1, VF2, VF3 and for host PF.
> 
>     Note: do not specify representor=[0-65535], it causes devargs
>     processing error, because number of ports (rte_eth_dev) is
>     limited.
> 
> Applications should distinguish representors and master devices exclusively
> by device flag RTE_ETH_DEV_REPRESENTOR and do not rely on switch
> port_id (mlx5 PMD deduces ones from representor_id) values returned by
> dev_infos_get() API.
> 
> [1]
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> .spinics.net%2Flists%2Fnetdev%2Fmsg547007.html&data=02%7C01%7C
> shahafs%40mellanox.com%7C95dc9265aee344ca1c1108d6c27553f3%7Ca6529
> 71c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636910206515011521&sd
> ata=xJFDvK63LaQB9B8eVNn8iKfH5YzsnLW%2FsN1wPh6iyK8%3D&reser
> ved=0
>     Linux-tree: c12ecc23 (Or Gerlitz 2018-04-25 17:32 +0300)
>     "net/mlx5e: Move to use common phys port names for vport
> representors"
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

Applied to next-net-mlx, thanks. 

Re: [dpdk-dev] [PATCH v2] net/mlx5: add support for PF representor

[Shahaf Shuler]

Tuesday, April 16, 2019 5:10 PM, Viacheslav Ovsiienko:
> Subject: [dpdk-dev] [PATCH v2] net/mlx5: add support for PF representor
> 
> On BlueField platform we have the new entity - PF representor.
> This one represents the PCI PF attached to external host on the side of ARM.
> The traffic sent by the external host to the NIC via PF will be seem by ARM on
> this PF representor.
> 
> This patch refactors port recognizing capability on the base of physical port
> name. We have two groups of name formats. Legacy name formats are
> supported by kernels before ver 5.0 (being more precise - before the patch
> [1]) or before Mellanox OFED 4.6, and new naming formats added by the
> patch [1].
> 
> Legacy naming formats are supported:
> 
>   - missing physical port name (no sysfs/netlink key) at all,
>     master is assumed
> 
>   - decimal digits (for example "12"), representor is assumed,
>     the value is the index of attached VF
> 
> New naming formats are supported:
> 
>   - "p" followed by decimal digits, for example "p2", master
>     is assumed
> 
>   - "pf" followed by PF index concatenated with "vf" followed by
>     VF index, for example "pf0vf1", representor is assumed.
>     If index of VF is "-1" it is a special case  of host PF
>     representor, this representor must be indexed in devargs
>     as 65535, for example representor=[0-3,65535] will
>     allow representors for VF0, VF1, VF2, VF3 and for host PF.
> 
>     Note: do not specify representor=[0-65535], it causes devargs
>     processing error, because number of ports (rte_eth_dev) is
>     limited.
> 
> Applications should distinguish representors and master devices exclusively
> by device flag RTE_ETH_DEV_REPRESENTOR and do not rely on switch
> port_id (mlx5 PMD deduces ones from representor_id) values returned by
> dev_infos_get() API.
> 
> [1]
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> .spinics.net%2Flists%2Fnetdev%2Fmsg547007.html&data=02%7C01%7C
> shahafs%40mellanox.com%7C95dc9265aee344ca1c1108d6c27553f3%7Ca6529
> 71c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636910206515011521&sd
> ata=xJFDvK63LaQB9B8eVNn8iKfH5YzsnLW%2FsN1wPh6iyK8%3D&reser
> ved=0
>     Linux-tree: c12ecc23 (Or Gerlitz 2018-04-25 17:32 +0300)
>     "net/mlx5e: Move to use common phys port names for vport
> representors"
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

Applied to next-net-mlx, thanks.