[PATCH 0/4] clarify eth_dev state management

[PATCH 0/4] clarify eth_dev state management

[Gaetan Rivet]

1. The hotplug API currently available in DPDK introduces the notion of
   device states (DEV_DETACHED, DEV_ATTACHED). These states are implicit
   and internal to the ethdev layer.

2. Device enumeration and access is done directly, without regard to the
   underlying state of a device. Applications are currently expected to cycle
   through the static rte_eth_devices array. Those using the hotplug API
   (rte_eth_dev_attach(), rte_eth_dev_detach()), are thus expected to deal
   themselves with possible discrepancies internal to the ethdev layer,
   i.e. avoid detached devices if necessary, without the state of the devices
   having been explicitly defined or exposed.

3. The hotplug API itself is not complete and cannot be used without
   introducing some bugs. Detaching a device will introduce inconsistencies in
   the device count, as explained in the related API.
   No function is exposed in the ethdev layer to permit applications to deal
   with it.

This series addresses these issues so that:

1. Applications are not expected to manage the states of their devices, those
   should be kept internal to the ethdev layer.

2. The hotplug API is cleaner, with specific operations from the ethdev layer
   to be used if necessary.

3. Applications that are not interested in hotplug functionality are not concerned
   and do not require any change.

Gaetan Rivet (4):
  ethdev: expose device states
  ethdev: add device iterator
  ethdev: count devices consistently
  app/testpmd: use ethdev iterator to list devices

 app/test-pmd/cmdline.c        | 31 ++++++++++++++---------------
 app/test-pmd/cmdline_flow.c   |  2 +-
 app/test-pmd/config.c         | 12 +++++------
 app/test-pmd/parameters.c     |  4 ++--
 app/test-pmd/testpmd.c        | 44 +++++++++++------------------------------
 app/test-pmd/testpmd.h        |  9 ---------
 lib/librte_ether/rte_ethdev.c | 46 +++++++++++++++++++++++++------------------
 lib/librte_ether/rte_ethdev.h | 37 ++++++++++++++++++++++++++++++----
 8 files changed, 95 insertions(+), 90 deletions(-)

-- 
2.1.4

[PATCH 1/4] ethdev: expose device states

[Gaetan Rivet]

The hotplug API introduced multiple states for a device with possible
values defined internally, while the related field in struct rte_eth_dev
was made public.

Exposing those states improves consistency because applications have to
deal with the device list directly.

"DEV_DETACHED" is renamed "RTE_ETH_DEV_UNUSED" to better reflect that
the emptiness of a slot is not necessarily the result of detaching a
device.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_ether/rte_ethdev.c | 15 +++++----------
 lib/librte_ether/rte_ethdev.h | 10 +++++++++-
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index f3c32c4..d5d1b52 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -138,11 +138,6 @@ enum {
 	STAT_QMAP_RX
 };
 
-enum {
-	DEV_DETACHED = 0,
-	DEV_ATTACHED
-};
-
 static void
 rte_eth_dev_data_alloc(void)
 {
@@ -170,7 +165,7 @@ rte_eth_dev_allocated(const char *name)
 	unsigned i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-		if ((rte_eth_devices[i].attached == DEV_ATTACHED) &&
+		if ((rte_eth_devices[i].state == RTE_ETH_DEV_ATTACHED) &&
 		    strcmp(rte_eth_devices[i].data->name, name) == 0)
 			return &rte_eth_devices[i];
 	}
@@ -183,7 +178,7 @@ rte_eth_dev_find_free_port(void)
 	unsigned i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-		if (rte_eth_devices[i].attached == DEV_DETACHED)
+		if (rte_eth_devices[i].state == RTE_ETH_DEV_UNUSED)
 			return i;
 	}
 	return RTE_MAX_ETHPORTS;
@@ -195,7 +190,7 @@ eth_dev_get(uint8_t port_id)
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
 
 	eth_dev->data = &rte_eth_dev_data[port_id];
-	eth_dev->attached = DEV_ATTACHED;
+	eth_dev->state = RTE_ETH_DEV_ATTACHED;
 	TAILQ_INIT(&(eth_dev->link_intr_cbs));
 
 	eth_dev_last_created_port = port_id;
@@ -271,7 +266,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 	if (eth_dev == NULL)
 		return -EINVAL;
 
-	eth_dev->attached = DEV_DETACHED;
+	eth_dev->state = RTE_ETH_DEV_UNUSED;
 	nb_ports--;
 	return 0;
 }
@@ -377,7 +372,7 @@ int
 rte_eth_dev_is_valid_port(uint8_t port_id)
 {
 	if (port_id >= RTE_MAX_ETHPORTS ||
-	    rte_eth_devices[port_id].attached != DEV_ATTACHED)
+	    rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED)
 		return 0;
 	else
 		return 1;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 97f3e2d..d51b8e9 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1614,6 +1614,14 @@ struct rte_eth_rxtx_callback {
 };
 
 /**
+ * A set of values to describe the possible states of an eth device.
+ */
+enum rte_eth_dev_state {
+	RTE_ETH_DEV_UNUSED = 0,
+	RTE_ETH_DEV_ATTACHED,
+};
+
+/**
  * @internal
  * The generic data structure associated with each ethernet device.
  *
@@ -1644,7 +1652,7 @@ struct rte_eth_dev {
 	 * received packets before passing them to the driver for transmission.
 	 */
 	struct rte_eth_rxtx_callback *pre_tx_burst_cbs[RTE_MAX_QUEUES_PER_PORT];
-	uint8_t attached; /**< Flag indicating the port is attached */
+	enum rte_eth_dev_state state:8; /**< Flag indicating the port state */
 } __rte_cache_aligned;
 
 struct rte_eth_dev_sriov {
-- 
2.1.4

[PATCH 2/4] ethdev: add device iterator

[Gaetan Rivet]

This iterator helps applications iterate over the device list and skip
holes caused by invalid or detached devices.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_ether/rte_ethdev.c | 13 +++++++++++++
 lib/librte_ether/rte_ethdev.h | 19 +++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index d5d1b52..fcb9933 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -138,6 +138,19 @@ enum {
 	STAT_QMAP_RX
 };
 
+uint8_t
+rte_eth_find_next(uint8_t port_id)
+{
+	while (port_id < RTE_MAX_ETHPORTS &&
+	       rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED)
+		port_id++;
+
+	if (port_id >= RTE_MAX_ETHPORTS)
+		return RTE_MAX_ETHPORTS;
+
+	return port_id;
+}
+
 static void
 rte_eth_dev_data_alloc(void)
 {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index d51b8e9..59c4123 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1731,6 +1731,25 @@ struct rte_eth_dev_data {
 extern struct rte_eth_dev rte_eth_devices[];
 
 /**
+ * Iterates over valid ethdev ports.
+ *
+ * @param: port_id
+ *   The id of the next possible valid port.
+ * @return
+ *   Next valid port id, RTE_MAX_ETHPORTS is there is none.
+ */
+uint8_t rte_eth_find_next(uint8_t port_id);
+
+/**
+ * Macro to iterate over all enabled ethdev ports.
+ */
+#define RTE_ETH_FOREACH_DEV(p)			\
+	for (p = rte_eth_find_next(0);		\
+	     p < RTE_MAX_ETHPORTS;		\
+	     p = rte_eth_find_next(p + 1))
+
+
+/**
  * Get the total number of Ethernet devices that have been successfully
  * initialized by the [matching] Ethernet driver during the PCI probing phase.
  * All devices whose port identifier is in the range
-- 
2.1.4

[PATCH 3/4] ethdev: count devices consistently

[Gaetan Rivet]

Make the rte_eth_dev_count() return the correct number of devices even
after some are detached by the hotplug API.

This change does not affect existing applications that do not use
hotplug API calls. Those that do are already aware that port IDs are not
necessarily contiguous.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_ether/rte_ethdev.c | 20 ++++++++++----------
 lib/librte_ether/rte_ethdev.h | 14 ++++++++------
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index fcb9933..3a52d0a 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -72,7 +72,6 @@ static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
 struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
 static struct rte_eth_dev_data *rte_eth_dev_data;
 static uint8_t eth_dev_last_created_port;
-static uint8_t nb_ports;
 
 /* spinlock for eth device callbacks */
 static rte_spinlock_t rte_eth_dev_cb_lock = RTE_SPINLOCK_INITIALIZER;
@@ -207,7 +206,6 @@ eth_dev_get(uint8_t port_id)
 	TAILQ_INIT(&(eth_dev->link_intr_cbs));
 
 	eth_dev_last_created_port = port_id;
-	nb_ports++;
 
 	return eth_dev;
 }
@@ -280,7 +278,6 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 		return -EINVAL;
 
 	eth_dev->state = RTE_ETH_DEV_UNUSED;
-	nb_ports--;
 	return 0;
 }
 
@@ -401,7 +398,15 @@ rte_eth_dev_socket_id(uint8_t port_id)
 uint8_t
 rte_eth_dev_count(void)
 {
-	return nb_ports;
+	uint8_t p;
+	uint8_t count;
+
+	count = 0;
+
+	RTE_ETH_FOREACH_DEV(p)
+		count++;
+
+	return count;
 }
 
 int
@@ -433,13 +438,8 @@ rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id)
 		return -EINVAL;
 	}
 
-	if (!nb_ports)
-		return -ENODEV;
-
 	*port_id = RTE_MAX_ETHPORTS;
-
-	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-
+	RTE_ETH_FOREACH_DEV(i) {
 		if (!strncmp(name,
 			rte_eth_dev_data[i].name, strlen(name))) {
 
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 59c4123..bdad81b 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1743,9 +1743,9 @@ uint8_t rte_eth_find_next(uint8_t port_id);
 /**
  * Macro to iterate over all enabled ethdev ports.
  */
-#define RTE_ETH_FOREACH_DEV(p)			\
-	for (p = rte_eth_find_next(0);		\
-	     p < RTE_MAX_ETHPORTS;		\
+#define RTE_ETH_FOREACH_DEV(p)					\
+	for (p = rte_eth_find_next(0);				\
+	     (unsigned int)p < (unsigned int)RTE_MAX_ETHPORTS;	\
 	     p = rte_eth_find_next(p + 1))
 
 
@@ -1755,9 +1755,11 @@ uint8_t rte_eth_find_next(uint8_t port_id);
  * All devices whose port identifier is in the range
  * [0,  rte_eth_dev_count() - 1] can be operated on by network applications
  * immediately after invoking rte_eal_init().
- * If the application unplugs a port using hotplug function, The enabled port
- * numbers may be noncontiguous. In the case, the applications need to manage
- * enabled port by themselves.
+ * If the application unplugs a port using a hotplug function, the range of
+ * enabled ports may be non-contiguous. In this case, this function returns
+ * the actual number of enabled ports and the application must keep track
+ * of possible gaps in the enabled range, or use the ``RTE_ETH_FOREACH_DEV()``
+ * macro.
  *
  * @return
  *   - The total number of usable Ethernet devices.
-- 
2.1.4

[PATCH 4/4] app/testpmd: use ethdev iterator to list devices

[Gaetan Rivet]

This commit replaces redundant code with public ethdev layer calls.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 app/test-pmd/cmdline.c      | 31 +++++++++++++++----------------
 app/test-pmd/cmdline_flow.c |  2 +-
 app/test-pmd/config.c       | 12 ++++++------
 app/test-pmd/parameters.c   |  4 ++--
 app/test-pmd/testpmd.c      | 44 +++++++++++---------------------------------
 app/test-pmd/testpmd.h      |  9 ---------
 6 files changed, 35 insertions(+), 67 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 43fc636..871ab19 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1139,7 +1139,7 @@ cmd_config_speed_all_parsed(void *parsed_result,
 			&link_speed) < 0)
 		return;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		ports[pid].dev_conf.link_speeds = link_speed;
 	}
 
@@ -4641,7 +4641,6 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,
 		nb_ports = rte_eth_dev_count();
 		reconfig(port_id, res->socket);
 		rte_eth_promiscuous_enable(port_id);
-		ports[port_id].enabled = 1;
 	}
 
 }
@@ -5000,7 +4999,7 @@ static void cmd_set_promisc_mode_parsed(void *parsed_result,
 
 	/* all ports */
 	if (allports) {
-		FOREACH_PORT(i, ports) {
+		RTE_ETH_FOREACH_DEV(i) {
 			if (enable)
 				rte_eth_promiscuous_enable(i);
 			else
@@ -5080,7 +5079,7 @@ static void cmd_set_allmulti_mode_parsed(void *parsed_result,
 
 	/* all ports */
 	if (allports) {
-		FOREACH_PORT(i, ports) {
+		RTE_ETH_FOREACH_DEV(i) {
 			if (enable)
 				rte_eth_allmulticast_enable(i);
 			else
@@ -5814,31 +5813,31 @@ static void cmd_showportall_parsed(void *parsed_result,
 	struct cmd_showportall_result *res = parsed_result;
 	if (!strcmp(res->show, "clear")) {
 		if (!strcmp(res->what, "stats"))
-			FOREACH_PORT(i, ports)
+			RTE_ETH_FOREACH_DEV(i)
 				nic_stats_clear(i);
 		else if (!strcmp(res->what, "xstats"))
-			FOREACH_PORT(i, ports)
+			RTE_ETH_FOREACH_DEV(i)
 				nic_xstats_clear(i);
 	} else if (!strcmp(res->what, "info"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			port_infos_display(i);
 	else if (!strcmp(res->what, "stats"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			nic_stats_display(i);
 	else if (!strcmp(res->what, "xstats"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			nic_xstats_display(i);
 	else if (!strcmp(res->what, "fdir"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			fdir_get_infos(i);
 	else if (!strcmp(res->what, "stat_qmap"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			nic_stats_mapping_display(i);
 	else if (!strcmp(res->what, "dcb_tc"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			port_dcb_info_display(i);
 	else if (!strcmp(res->what, "cap"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			port_offload_cap_display(i);
 }
 
@@ -10315,7 +10314,7 @@ cmd_config_l2_tunnel_eth_type_all_parsed
 	entry.l2_tunnel_type = str2fdir_l2_tunnel_type(res->l2_tunnel_type);
 	entry.ether_type = res->eth_type_val;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		rte_eth_dev_l2_tunnel_eth_type_conf(pid, &entry);
 	}
 }
@@ -10431,7 +10430,7 @@ cmd_config_l2_tunnel_en_dis_all_parsed(
 	else
 		en = 0;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		rte_eth_dev_l2_tunnel_offload_set(pid,
 						  &entry,
 						  ETH_L2_TUNNEL_ENABLE_MASK,
@@ -12601,7 +12600,7 @@ cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue)
 	if (id == (portid_t)RTE_PORT_ALL) {
 		portid_t pid;
 
-		FOREACH_PORT(pid, ports) {
+		RTE_ETH_FOREACH_DEV(pid) {
 			/* check if need_reconfig has been set to 1 */
 			if (ports[pid].need_reconfig == 0)
 				ports[pid].need_reconfig = dev;
diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index ff98690..b7d2c41 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -2369,7 +2369,7 @@ comp_port(struct context *ctx, const struct token *token,
 
 	(void)ctx;
 	(void)token;
-	FOREACH_PORT(p, ports) {
+	RTE_ETH_FOREACH_DEV(p) {
 		if (buf && i == ent)
 			return snprintf(buf, size, "%u", p);
 		++i;
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 80491fc..54e38c4 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -174,7 +174,7 @@ nic_stats_display(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;
@@ -252,7 +252,7 @@ nic_stats_clear(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;
@@ -334,7 +334,7 @@ nic_stats_mapping_display(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;
@@ -452,7 +452,7 @@ port_infos_display(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;
@@ -725,7 +725,7 @@ port_id_is_invalid(portid_t port_id, enum print_warning warning)
 	if (port_id == (portid_t)RTE_PORT_ALL)
 		return 0;
 
-	if (port_id < RTE_MAX_ETHPORTS && ports[port_id].enabled)
+	if (rte_eth_dev_is_valid_port(port_id))
 		return 0;
 
 	if (warning == ENABLED_WARN)
@@ -2279,7 +2279,7 @@ set_fwd_ports_mask(uint64_t portmask)
 		return;
 	}
 	nb_pt = 0;
-	for (i = 0; i < (unsigned)RTE_MIN(64, RTE_MAX_ETHPORTS); i++) {
+	RTE_ETH_FOREACH_DEV(i) {
 		if (! ((uint64_t)(1ULL << i) & portmask))
 			continue;
 		portlist[nb_pt++] = i;
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 28db8cd..67d8bf2 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -394,7 +394,7 @@ parse_portnuma_config(const char *q_arg)
 		port_id = (uint8_t)int_fld[FLD_PORT];
 		if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 			printf("Valid port range is [0");
-			FOREACH_PORT(pid, ports)
+			RTE_ETH_FOREACH_DEV(pid)
 				printf(", %d", pid);
 			printf("]\n");
 			return -1;
@@ -454,7 +454,7 @@ parse_ringnuma_config(const char *q_arg)
 		port_id = (uint8_t)int_fld[FLD_PORT];
 		if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 			printf("Valid port range is [0");
-			FOREACH_PORT(pid, ports)
+			RTE_ETH_FOREACH_DEV(pid)
 				printf(", %d", pid);
 			printf("]\n");
 			return -1;
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index bfb2f8e..b8ec1fe 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -334,20 +334,6 @@ static void check_all_ports_link_status(uint32_t port_mask);
 static int all_ports_started(void);
 
 /*
- * Find next enabled port
- */
-portid_t
-find_next_port(portid_t p, struct rte_port *ports, int size)
-{
-	if (ports == NULL)
-		rte_exit(-EINVAL, "failed to find a next port id\n");
-
-	while ((p < size) && (ports[p].enabled == 0))
-		p++;
-	return p;
-}
-
-/*
  * Setup default configuration.
  */
 static void
@@ -547,7 +533,7 @@ init_config(void)
 						 socket_num);
 	}
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		port = &ports[pid];
 		rte_eth_dev_info_get(pid, &port->dev_info);
 
@@ -632,7 +618,7 @@ init_fwd_streams(void)
 	queueid_t q;
 
 	/* set socket id according to numa or not */
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		port = &ports[pid];
 		if (nb_rxq > port->dev_info.max_rx_queues) {
 			printf("Fail: nb_rxq(%d) is greater than "
@@ -1253,7 +1239,7 @@ all_ports_started(void)
 	portid_t pi;
 	struct rte_port *port;
 
-	FOREACH_PORT(pi, ports) {
+	RTE_ETH_FOREACH_DEV(pi) {
 		port = &ports[pi];
 		/* Check if there is a port which is not started */
 		if ((port->port_status != RTE_PORT_STARTED) &&
@@ -1271,7 +1257,7 @@ all_ports_stopped(void)
 	portid_t pi;
 	struct rte_port *port;
 
-	FOREACH_PORT(pi, ports) {
+	RTE_ETH_FOREACH_DEV(pi) {
 		port = &ports[pi];
 		if ((port->port_status != RTE_PORT_STOPPED) &&
 			(port->slave_flag == 0))
@@ -1319,7 +1305,7 @@ start_port(portid_t pid)
 
 	if(dcb_config)
 		dcb_test = 1;
-	FOREACH_PORT(pi, ports) {
+	RTE_ETH_FOREACH_DEV(pi) {
 		if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
 			continue;
 
@@ -1476,7 +1462,7 @@ stop_port(portid_t pid)
 
 	printf("Stopping ports...\n");
 
-	FOREACH_PORT(pi, ports) {
+	RTE_ETH_FOREACH_DEV(pi) {
 		if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
 			continue;
 
@@ -1519,7 +1505,7 @@ close_port(portid_t pid)
 
 	printf("Closing ports...\n");
 
-	FOREACH_PORT(pi, ports) {
+	RTE_ETH_FOREACH_DEV(pi) {
 		if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
 			continue;
 
@@ -1574,7 +1560,6 @@ attach_port(char *identifier)
 	if (rte_eth_dev_attach(identifier, &pi))
 		return;
 
-	ports[pi].enabled = 1;
 	socket_id = (unsigned)rte_eth_dev_socket_id(pi);
 	/* if socket_id is invalid, set to 0 */
 	if (check_socket_id(socket_id) < 0)
@@ -1608,7 +1593,6 @@ detach_port(uint8_t port_id)
 	if (rte_eth_dev_detach(port_id, name))
 		return;
 
-	ports[port_id].enabled = 0;
 	nb_ports = rte_eth_dev_count();
 
 	printf("Port '%s' is detached. Now total ports is %d\n",
@@ -1627,7 +1611,7 @@ pmd_test_exit(void)
 
 	if (ports != NULL) {
 		no_link_check = 1;
-		FOREACH_PORT(pt_id, ports) {
+		RTE_ETH_FOREACH_DEV(pt_id) {
 			printf("\nShutting down port %d...\n", pt_id);
 			fflush(stdout);
 			stop_port(pt_id);
@@ -1658,7 +1642,7 @@ check_all_ports_link_status(uint32_t port_mask)
 	fflush(stdout);
 	for (count = 0; count <= MAX_CHECK_TIME; count++) {
 		all_ports_up = 1;
-		FOREACH_PORT(portid, ports) {
+		RTE_ETH_FOREACH_DEV(portid) {
 			if ((port_mask & (1 << portid)) == 0)
 				continue;
 			memset(&link, 0, sizeof(link));
@@ -1823,7 +1807,7 @@ init_port_config(void)
 	portid_t pid;
 	struct rte_port *port;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		port = &ports[pid];
 		port->dev_conf.rxmode = rx_mode;
 		port->dev_conf.fdir_conf = fdir_conf;
@@ -2047,8 +2031,6 @@ init_port_dcb_config(portid_t pid,
 static void
 init_port(void)
 {
-	portid_t pid;
-
 	/* Configuration of Ethernet ports. */
 	ports = rte_zmalloc("testpmd: ports",
 			    sizeof(struct rte_port) * RTE_MAX_ETHPORTS,
@@ -2058,10 +2040,6 @@ init_port(void)
 				"rte_zmalloc(%d struct rte_port) failed\n",
 				RTE_MAX_ETHPORTS);
 	}
-
-	/* enabled allocated ports */
-	for (pid = 0; pid < nb_ports; pid++)
-		ports[pid].enabled = 1;
 }
 
 static void
@@ -2136,7 +2114,7 @@ main(int argc, char** argv)
 		rte_exit(EXIT_FAILURE, "Start ports failed\n");
 
 	/* set all ports to promiscuous mode by default */
-	FOREACH_PORT(port_id, ports)
+	RTE_ETH_FOREACH_DEV(port_id)
 		rte_eth_promiscuous_enable(port_id);
 
 #ifdef RTE_LIBRTE_CMDLINE
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 8cf2860..e1e4939 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -163,7 +163,6 @@ struct port_flow {
  * The data structure associated with each port.
  */
 struct rte_port {
-	uint8_t                 enabled;    /**< Port enabled or not */
 	struct rte_eth_dev_info dev_info;   /**< PCI info + driver name */
 	struct rte_eth_conf     dev_conf;   /**< Port configuration. */
 	struct ether_addr       eth_addr;   /**< Port ethernet address */
@@ -195,14 +194,6 @@ struct rte_port {
 	struct port_flow        *flow_list; /**< Associated flows. */
 };
 
-extern portid_t __rte_unused
-find_next_port(portid_t p, struct rte_port *ports, int size);
-
-#define FOREACH_PORT(p, ports) \
-	for (p = find_next_port(0, ports, RTE_MAX_ETHPORTS); \
-	    p < RTE_MAX_ETHPORTS; \
-	    p = find_next_port(p + 1, ports, RTE_MAX_ETHPORTS))
-
 /**
  * The data structure associated with each forwarding logical core.
  * The logical cores are internally numbered by a core index from 0 to
-- 
2.1.4

Re: [PATCH 3/4] ethdev: count devices consistently

[Thomas Monjalon]

2017-03-03 16:40, Gaetan Rivet:
> Make the rte_eth_dev_count() return the correct number of devices even
> after some are detached by the hotplug API.

Please explain what is the correct number,
or that the wrong number was a max id.

> This change does not affect existing applications that do not use
> hotplug API calls. Those that do are already aware that port IDs are not
> necessarily contiguous.
[...]
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> -#define RTE_ETH_FOREACH_DEV(p)			\
> -	for (p = rte_eth_find_next(0);		\
> -	     p < RTE_MAX_ETHPORTS;		\
> +#define RTE_ETH_FOREACH_DEV(p)					\
> +	for (p = rte_eth_find_next(0);				\
> +	     (unsigned int)p < (unsigned int)RTE_MAX_ETHPORTS;	\
>  	     p = rte_eth_find_next(p + 1))

This macro was introduced in previous patch.
Why adding the cast here?

Re: [PATCH 3/4] ethdev: count devices consistently

[Gaëtan Rivet]

On Thu, Mar 30, 2017 at 09:26:12PM +0200, Thomas Monjalon wrote:
>2017-03-03 16:40, Gaetan Rivet:
>> Make the rte_eth_dev_count() return the correct number of devices even
>> after some are detached by the hotplug API.
>
>Please explain what is the correct number,
>or that the wrong number was a max id.
>

will do.

>> This change does not affect existing applications that do not use
>> hotplug API calls. Those that do are already aware that port IDs are not
>> necessarily contiguous.
>[...]
>> --- a/lib/librte_ether/rte_ethdev.h
>> +++ b/lib/librte_ether/rte_ethdev.h
>> -#define RTE_ETH_FOREACH_DEV(p)			\
>> -	for (p = rte_eth_find_next(0);		\
>> -	     p < RTE_MAX_ETHPORTS;		\
>> +#define RTE_ETH_FOREACH_DEV(p)					\
>> +	for (p = rte_eth_find_next(0);				\
>> +	     (unsigned int)p < (unsigned int)RTE_MAX_ETHPORTS;	\
>>  	     p = rte_eth_find_next(p + 1))
>
>This macro was introduced in previous patch.
>Why adding the cast here?

In the function rte_eth_dev_get_port_by_name(), the iterator is an int.
When I introduced the use of the iterator there, I then realized that it 
would be better to allow users to use signed ints as well, to avoid 
unnecessary edits on their part.

In retrospective, I agree that it would have been better in the previous 
patch.

-- 
Gaëtan Rivet
6WIND

[PATCH v2 0/3] clarify eth_dev state management

[Gaetan Rivet]

1. The hotplug API currently available in DPDK introduces the notion of
   device states (DEV_DETACHED, DEV_ATTACHED). These states are implicit
   and internal to the ethdev layer.

2. Device enumeration and access is done directly, without regard to the
   underlying state of a device. Applications are currently expected to cycle
   through the static rte_eth_devices array. Those using the hotplug API
   (rte_eth_dev_attach(), rte_eth_dev_detach()), are thus expected to deal
   themselves with possible discrepancies internal to the ethdev layer,
   i.e. avoid detached devices if necessary, without the state of the devices
   having been explicitly defined or exposed.

3. The hotplug API itself is not complete and cannot be used without
   introducing some bugs. Detaching a device will introduce inconsistencies in
   the device count, as explained in the related API.
   No function is exposed in the ethdev layer to permit applications to deal
   with it.

This series addresses these issues so that:

1. Applications are not expected to manage the states of their devices, those
   should be kept internal to the ethdev layer.

2. The hotplug API is cleaner, with specific operations from the ethdev layer
   to be used if necessary.

3. Applications that are not interested in hotplug functionality are not concerned
   and do not require any change.

v1 --> v2:

 * Improved patches consistency, iterator definition in one patch only

 * Removed device count changes, not needed without additional device states.

Gaetan Rivet (3):
  ethdev: expose device states
  ethdev: add device iterator
  app/testpmd: use ethdev iterator to list devices

 app/test-pmd/cmdline.c        | 31 +++++++++++++++---------------
 app/test-pmd/cmdline_flow.c   |  2 +-
 app/test-pmd/config.c         | 12 ++++++------
 app/test-pmd/parameters.c     |  4 ++--
 app/test-pmd/testpmd.c        | 44 +++++++++++--------------------------------
 app/test-pmd/testpmd.h        |  9 ---------
 lib/librte_ether/rte_ethdev.c | 30 +++++++++++++++++------------
 lib/librte_ether/rte_ethdev.h | 31 ++++++++++++++++++++++++++++--
 8 files changed, 82 insertions(+), 81 deletions(-)

-- 
2.1.4

[PATCH v2 1/3] ethdev: expose device states

[Gaetan Rivet]

The hotplug API introduced multiple states for a device with possible
values defined internally, while the related field in struct rte_eth_dev
was made public.

Exposing those states improves consistency because applications have to
deal with the device list directly.

"DEV_DETACHED" is renamed "RTE_ETH_DEV_UNUSED" to better reflect that
the emptiness of a slot is not necessarily the result of detaching a
device.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_ether/rte_ethdev.c | 15 +++++----------
 lib/librte_ether/rte_ethdev.h | 10 +++++++++-
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index b796e7d..90d0713 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -138,11 +138,6 @@ enum {
 	STAT_QMAP_RX
 };
 
-enum {
-	DEV_DETACHED = 0,
-	DEV_ATTACHED
-};
-
 static void
 rte_eth_dev_data_alloc(void)
 {
@@ -170,7 +165,7 @@ rte_eth_dev_allocated(const char *name)
 	unsigned i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-		if ((rte_eth_devices[i].attached == DEV_ATTACHED) &&
+		if ((rte_eth_devices[i].state == RTE_ETH_DEV_ATTACHED) &&
 		    strcmp(rte_eth_devices[i].data->name, name) == 0)
 			return &rte_eth_devices[i];
 	}
@@ -183,7 +178,7 @@ rte_eth_dev_find_free_port(void)
 	unsigned i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-		if (rte_eth_devices[i].attached == DEV_DETACHED)
+		if (rte_eth_devices[i].state == RTE_ETH_DEV_UNUSED)
 			return i;
 	}
 	return RTE_MAX_ETHPORTS;
@@ -195,7 +190,7 @@ eth_dev_get(uint8_t port_id)
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
 
 	eth_dev->data = &rte_eth_dev_data[port_id];
-	eth_dev->attached = DEV_ATTACHED;
+	eth_dev->state = RTE_ETH_DEV_ATTACHED;
 	TAILQ_INIT(&(eth_dev->link_intr_cbs));
 
 	eth_dev_last_created_port = port_id;
@@ -271,7 +266,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 	if (eth_dev == NULL)
 		return -EINVAL;
 
-	eth_dev->attached = DEV_DETACHED;
+	eth_dev->state = RTE_ETH_DEV_UNUSED;
 	nb_ports--;
 	return 0;
 }
@@ -377,7 +372,7 @@ int
 rte_eth_dev_is_valid_port(uint8_t port_id)
 {
 	if (port_id >= RTE_MAX_ETHPORTS ||
-	    rte_eth_devices[port_id].attached != DEV_ATTACHED)
+	    rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED)
 		return 0;
 	else
 		return 1;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index b3ee872..b9531f3 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1619,6 +1619,14 @@ struct rte_eth_rxtx_callback {
 };
 
 /**
+ * A set of values to describe the possible states of an eth device.
+ */
+enum rte_eth_dev_state {
+	RTE_ETH_DEV_UNUSED = 0,
+	RTE_ETH_DEV_ATTACHED,
+};
+
+/**
  * @internal
  * The generic data structure associated with each ethernet device.
  *
@@ -1649,7 +1657,7 @@ struct rte_eth_dev {
 	 * received packets before passing them to the driver for transmission.
 	 */
 	struct rte_eth_rxtx_callback *pre_tx_burst_cbs[RTE_MAX_QUEUES_PER_PORT];
-	uint8_t attached; /**< Flag indicating the port is attached */
+	enum rte_eth_dev_state state:8; /**< Flag indicating the port state */
 } __rte_cache_aligned;
 
 struct rte_eth_dev_sriov {
-- 
2.1.4

[PATCH v2 2/3] ethdev: add device iterator

[Gaetan Rivet]

This iterator helps applications iterate over the device list and skip
holes caused by invalid or detached devices.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_ether/rte_ethdev.c | 17 ++++++++++++++---
 lib/librte_ether/rte_ethdev.h | 21 ++++++++++++++++++++-
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 90d0713..ca7af35 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -138,6 +138,19 @@ enum {
 	STAT_QMAP_RX
 };
 
+uint8_t
+rte_eth_find_next(uint8_t port_id)
+{
+	while (port_id < RTE_MAX_ETHPORTS &&
+	       rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED)
+		port_id++;
+
+	if (port_id >= RTE_MAX_ETHPORTS)
+		return RTE_MAX_ETHPORTS;
+
+	return port_id;
+}
+
 static void
 rte_eth_dev_data_alloc(void)
 {
@@ -424,9 +437,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id)
 		return -ENODEV;
 
 	*port_id = RTE_MAX_ETHPORTS;
-
-	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-
+	RTE_ETH_FOREACH_DEV(i) {
 		if (!strncmp(name,
 			rte_eth_dev_data[i].name, strlen(name))) {
 
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index b9531f3..bed7dff 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1736,6 +1736,25 @@ struct rte_eth_dev_data {
 extern struct rte_eth_dev rte_eth_devices[];
 
 /**
+ * Iterates over valid ethdev ports.
+ *
+ * @param: port_id
+ *   The id of the next possible valid port.
+ * @return
+ *   Next valid port id, RTE_MAX_ETHPORTS if there is none.
+ */
+uint8_t rte_eth_find_next(uint8_t port_id);
+
+/**
+ * Macro to iterate over all enabled ethdev ports.
+ */
+#define RTE_ETH_FOREACH_DEV(p)					\
+	for (p = rte_eth_find_next(0);				\
+	     (unsigned int)p < (unsigned int)RTE_MAX_ETHPORTS;	\
+	     p = rte_eth_find_next(p + 1))
+
+
+/**
  * Get the total number of Ethernet devices that have been successfully
  * initialized by the [matching] Ethernet driver during the PCI probing phase.
  * All devices whose port identifier is in the range
@@ -1743,7 +1762,7 @@ extern struct rte_eth_dev rte_eth_devices[];
  * immediately after invoking rte_eal_init().
  * If the application unplugs a port using hotplug function, The enabled port
  * numbers may be noncontiguous. In the case, the applications need to manage
- * enabled port by themselves.
+ * enabled port by using the ``RTE_ETH_FOREACH_DEV()`` macro.
  *
  * @return
  *   - The total number of usable Ethernet devices.
-- 
2.1.4

[PATCH v2 3/3] app/testpmd: use ethdev iterator to list devices

[Gaetan Rivet]

This commit replaces redundant code with public ethdev layer calls.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 app/test-pmd/cmdline.c      | 31 +++++++++++++++----------------
 app/test-pmd/cmdline_flow.c |  2 +-
 app/test-pmd/config.c       | 12 ++++++------
 app/test-pmd/parameters.c   |  4 ++--
 app/test-pmd/testpmd.c      | 44 +++++++++++---------------------------------
 app/test-pmd/testpmd.h      |  9 ---------
 6 files changed, 35 insertions(+), 67 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index aac4efb..cf5da4b 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1160,7 +1160,7 @@ cmd_config_speed_all_parsed(void *parsed_result,
 			&link_speed) < 0)
 		return;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		ports[pid].dev_conf.link_speeds = link_speed;
 	}
 
@@ -4662,7 +4662,6 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,
 		nb_ports = rte_eth_dev_count();
 		reconfig(port_id, res->socket);
 		rte_eth_promiscuous_enable(port_id);
-		ports[port_id].enabled = 1;
 	}
 
 }
@@ -5021,7 +5020,7 @@ static void cmd_set_promisc_mode_parsed(void *parsed_result,
 
 	/* all ports */
 	if (allports) {
-		FOREACH_PORT(i, ports) {
+		RTE_ETH_FOREACH_DEV(i) {
 			if (enable)
 				rte_eth_promiscuous_enable(i);
 			else
@@ -5101,7 +5100,7 @@ static void cmd_set_allmulti_mode_parsed(void *parsed_result,
 
 	/* all ports */
 	if (allports) {
-		FOREACH_PORT(i, ports) {
+		RTE_ETH_FOREACH_DEV(i) {
 			if (enable)
 				rte_eth_allmulticast_enable(i);
 			else
@@ -5835,31 +5834,31 @@ static void cmd_showportall_parsed(void *parsed_result,
 	struct cmd_showportall_result *res = parsed_result;
 	if (!strcmp(res->show, "clear")) {
 		if (!strcmp(res->what, "stats"))
-			FOREACH_PORT(i, ports)
+			RTE_ETH_FOREACH_DEV(i)
 				nic_stats_clear(i);
 		else if (!strcmp(res->what, "xstats"))
-			FOREACH_PORT(i, ports)
+			RTE_ETH_FOREACH_DEV(i)
 				nic_xstats_clear(i);
 	} else if (!strcmp(res->what, "info"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			port_infos_display(i);
 	else if (!strcmp(res->what, "stats"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			nic_stats_display(i);
 	else if (!strcmp(res->what, "xstats"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			nic_xstats_display(i);
 	else if (!strcmp(res->what, "fdir"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			fdir_get_infos(i);
 	else if (!strcmp(res->what, "stat_qmap"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			nic_stats_mapping_display(i);
 	else if (!strcmp(res->what, "dcb_tc"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			port_dcb_info_display(i);
 	else if (!strcmp(res->what, "cap"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			port_offload_cap_display(i);
 }
 
@@ -10339,7 +10338,7 @@ cmd_config_l2_tunnel_eth_type_all_parsed
 	entry.l2_tunnel_type = str2fdir_l2_tunnel_type(res->l2_tunnel_type);
 	entry.ether_type = res->eth_type_val;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		rte_eth_dev_l2_tunnel_eth_type_conf(pid, &entry);
 	}
 }
@@ -10455,7 +10454,7 @@ cmd_config_l2_tunnel_en_dis_all_parsed(
 	else
 		en = 0;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		rte_eth_dev_l2_tunnel_offload_set(pid,
 						  &entry,
 						  ETH_L2_TUNNEL_ENABLE_MASK,
@@ -13114,7 +13113,7 @@ cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue)
 	if (id == (portid_t)RTE_PORT_ALL) {
 		portid_t pid;
 
-		FOREACH_PORT(pid, ports) {
+		RTE_ETH_FOREACH_DEV(pid) {
 			/* check if need_reconfig has been set to 1 */
 			if (ports[pid].need_reconfig == 0)
 				ports[pid].need_reconfig = dev;
diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 2149df4..4e99f0f 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -2416,7 +2416,7 @@ comp_port(struct context *ctx, const struct token *token,
 
 	(void)ctx;
 	(void)token;
-	FOREACH_PORT(p, ports) {
+	RTE_ETH_FOREACH_DEV(p) {
 		if (buf && i == ent)
 			return snprintf(buf, size, "%u", p);
 		++i;
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 780ce6b..9d05e01 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -174,7 +174,7 @@ nic_stats_display(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;
@@ -252,7 +252,7 @@ nic_stats_clear(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;
@@ -334,7 +334,7 @@ nic_stats_mapping_display(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;
@@ -452,7 +452,7 @@ port_infos_display(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;
@@ -725,7 +725,7 @@ port_id_is_invalid(portid_t port_id, enum print_warning warning)
 	if (port_id == (portid_t)RTE_PORT_ALL)
 		return 0;
 
-	if (port_id < RTE_MAX_ETHPORTS && ports[port_id].enabled)
+	if (rte_eth_dev_is_valid_port(port_id))
 		return 0;
 
 	if (warning == ENABLED_WARN)
@@ -2281,7 +2281,7 @@ set_fwd_ports_mask(uint64_t portmask)
 		return;
 	}
 	nb_pt = 0;
-	for (i = 0; i < (unsigned)RTE_MIN(64, RTE_MAX_ETHPORTS); i++) {
+	RTE_ETH_FOREACH_DEV(i) {
 		if (! ((uint64_t)(1ULL << i) & portmask))
 			continue;
 		portlist[nb_pt++] = i;
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 28db8cd..67d8bf2 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -394,7 +394,7 @@ parse_portnuma_config(const char *q_arg)
 		port_id = (uint8_t)int_fld[FLD_PORT];
 		if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 			printf("Valid port range is [0");
-			FOREACH_PORT(pid, ports)
+			RTE_ETH_FOREACH_DEV(pid)
 				printf(", %d", pid);
 			printf("]\n");
 			return -1;
@@ -454,7 +454,7 @@ parse_ringnuma_config(const char *q_arg)
 		port_id = (uint8_t)int_fld[FLD_PORT];
 		if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 			printf("Valid port range is [0");
-			FOREACH_PORT(pid, ports)
+			RTE_ETH_FOREACH_DEV(pid)
 				printf(", %d", pid);
 			printf("]\n");
 			return -1;
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 484c19b..b27822b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -334,20 +334,6 @@ static void check_all_ports_link_status(uint32_t port_mask);
 static int all_ports_started(void);
 
 /*
- * Find next enabled port
- */
-portid_t
-find_next_port(portid_t p, struct rte_port *ports, int size)
-{
-	if (ports == NULL)
-		rte_exit(-EINVAL, "failed to find a next port id\n");
-
-	while ((p < size) && (ports[p].enabled == 0))
-		p++;
-	return p;
-}
-
-/*
  * Setup default configuration.
  */
 static void
@@ -547,7 +533,7 @@ init_config(void)
 						 socket_num);
 	}
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		port = &ports[pid];
 		rte_eth_dev_info_get(pid, &port->dev_info);
 
@@ -632,7 +618,7 @@ init_fwd_streams(void)
 	queueid_t q;
 
 	/* set socket id according to numa or not */
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		port = &ports[pid];
 		if (nb_rxq > port->dev_info.max_rx_queues) {
 			printf("Fail: nb_rxq(%d) is greater than "
@@ -1253,7 +1239,7 @@ all_ports_started(void)
 	portid_t pi;
 	struct rte_port *port;
 
-	FOREACH_PORT(pi, ports) {
+	RTE_ETH_FOREACH_DEV(pi) {
 		port = &ports[pi];
 		/* Check if there is a port which is not started */
 		if ((port->port_status != RTE_PORT_STARTED) &&
@@ -1271,7 +1257,7 @@ all_ports_stopped(void)
 	portid_t pi;
 	struct rte_port *port;
 
-	FOREACH_PORT(pi, ports) {
+	RTE_ETH_FOREACH_DEV(pi) {
 		port = &ports[pi];
 		if ((port->port_status != RTE_PORT_STOPPED) &&
 			(port->slave_flag == 0))
@@ -1319,7 +1305,7 @@ start_port(portid_t pid)
 
 	if(dcb_config)
 		dcb_test = 1;
-	FOREACH_PORT(pi, ports) {
+	RTE_ETH_FOREACH_DEV(pi) {
 		if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
 			continue;
 
@@ -1476,7 +1462,7 @@ stop_port(portid_t pid)
 
 	printf("Stopping ports...\n");
 
-	FOREACH_PORT(pi, ports) {
+	RTE_ETH_FOREACH_DEV(pi) {
 		if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
 			continue;
 
@@ -1519,7 +1505,7 @@ close_port(portid_t pid)
 
 	printf("Closing ports...\n");
 
-	FOREACH_PORT(pi, ports) {
+	RTE_ETH_FOREACH_DEV(pi) {
 		if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
 			continue;
 
@@ -1574,7 +1560,6 @@ attach_port(char *identifier)
 	if (rte_eth_dev_attach(identifier, &pi))
 		return;
 
-	ports[pi].enabled = 1;
 	socket_id = (unsigned)rte_eth_dev_socket_id(pi);
 	/* if socket_id is invalid, set to 0 */
 	if (check_socket_id(socket_id) < 0)
@@ -1608,7 +1593,6 @@ detach_port(uint8_t port_id)
 	if (rte_eth_dev_detach(port_id, name))
 		return;
 
-	ports[port_id].enabled = 0;
 	nb_ports = rte_eth_dev_count();
 
 	printf("Port '%s' is detached. Now total ports is %d\n",
@@ -1627,7 +1611,7 @@ pmd_test_exit(void)
 
 	if (ports != NULL) {
 		no_link_check = 1;
-		FOREACH_PORT(pt_id, ports) {
+		RTE_ETH_FOREACH_DEV(pt_id) {
 			printf("\nShutting down port %d...\n", pt_id);
 			fflush(stdout);
 			stop_port(pt_id);
@@ -1658,7 +1642,7 @@ check_all_ports_link_status(uint32_t port_mask)
 	fflush(stdout);
 	for (count = 0; count <= MAX_CHECK_TIME; count++) {
 		all_ports_up = 1;
-		FOREACH_PORT(portid, ports) {
+		RTE_ETH_FOREACH_DEV(portid) {
 			if ((port_mask & (1 << portid)) == 0)
 				continue;
 			memset(&link, 0, sizeof(link));
@@ -1823,7 +1807,7 @@ init_port_config(void)
 	portid_t pid;
 	struct rte_port *port;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		port = &ports[pid];
 		port->dev_conf.rxmode = rx_mode;
 		port->dev_conf.fdir_conf = fdir_conf;
@@ -2036,8 +2020,6 @@ init_port_dcb_config(portid_t pid,
 static void
 init_port(void)
 {
-	portid_t pid;
-
 	/* Configuration of Ethernet ports. */
 	ports = rte_zmalloc("testpmd: ports",
 			    sizeof(struct rte_port) * RTE_MAX_ETHPORTS,
@@ -2047,10 +2029,6 @@ init_port(void)
 				"rte_zmalloc(%d struct rte_port) failed\n",
 				RTE_MAX_ETHPORTS);
 	}
-
-	/* enabled allocated ports */
-	for (pid = 0; pid < nb_ports; pid++)
-		ports[pid].enabled = 1;
 }
 
 static void
@@ -2125,7 +2103,7 @@ main(int argc, char** argv)
 		rte_exit(EXIT_FAILURE, "Start ports failed\n");
 
 	/* set all ports to promiscuous mode by default */
-	FOREACH_PORT(port_id, ports)
+	RTE_ETH_FOREACH_DEV(port_id)
 		rte_eth_promiscuous_enable(port_id);
 
 #ifdef RTE_LIBRTE_CMDLINE
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 5c18151..011f4b5 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -163,7 +163,6 @@ struct port_flow {
  * The data structure associated with each port.
  */
 struct rte_port {
-	uint8_t                 enabled;    /**< Port enabled or not */
 	struct rte_eth_dev_info dev_info;   /**< PCI info + driver name */
 	struct rte_eth_conf     dev_conf;   /**< Port configuration. */
 	struct ether_addr       eth_addr;   /**< Port ethernet address */
@@ -195,14 +194,6 @@ struct rte_port {
 	struct port_flow        *flow_list; /**< Associated flows. */
 };
 
-extern portid_t __rte_unused
-find_next_port(portid_t p, struct rte_port *ports, int size);
-
-#define FOREACH_PORT(p, ports) \
-	for (p = find_next_port(0, ports, RTE_MAX_ETHPORTS); \
-	    p < RTE_MAX_ETHPORTS; \
-	    p = find_next_port(p + 1, ports, RTE_MAX_ETHPORTS))
-
 /**
  * The data structure associated with each forwarding logical core.
  * The logical cores are internally numbered by a core index from 0 to
-- 
2.1.4

Re: [PATCH v2 2/3] ethdev: add device iterator

[Thomas Monjalon]

2017-03-31 14:04, Gaetan Rivet:
>  /**
> + * Iterates over valid ethdev ports.
> + *
> + * @param: port_id

warning: expected whitespace after : command

Will fix it when applying

> + *   The id of the next possible valid port.
> + * @return
> + *   Next valid port id, RTE_MAX_ETHPORTS if there is none.
> + */
> +uint8_t rte_eth_find_next(uint8_t port_id);
> 

Re: [PATCH v2 0/3] clarify eth_dev state management

[Thomas Monjalon]

2017-03-31 14:04, Gaetan Rivet:
> 1. The hotplug API currently available in DPDK introduces the notion of
>    device states (DEV_DETACHED, DEV_ATTACHED). These states are implicit
>    and internal to the ethdev layer.
> 
> 2. Device enumeration and access is done directly, without regard to the
>    underlying state of a device. Applications are currently expected to cycle
>    through the static rte_eth_devices array. Those using the hotplug API
>    (rte_eth_dev_attach(), rte_eth_dev_detach()), are thus expected to deal
>    themselves with possible discrepancies internal to the ethdev layer,
>    i.e. avoid detached devices if necessary, without the state of the devices
>    having been explicitly defined or exposed.
> 
> 3. The hotplug API itself is not complete and cannot be used without
>    introducing some bugs. Detaching a device will introduce inconsistencies in
>    the device count, as explained in the related API.
>    No function is exposed in the ethdev layer to permit applications to deal
>    with it.
> 
> This series addresses these issues so that:
> 
> 1. Applications are not expected to manage the states of their devices, those
>    should be kept internal to the ethdev layer.
> 
> 2. The hotplug API is cleaner, with specific operations from the ethdev layer
>    to be used if necessary.
> 
> 3. Applications that are not interested in hotplug functionality are not concerned
>    and do not require any change.

Applied, thanks