[dpdk-dev] [PATCH v5 6/6] ethdev: complete closing of port

[Thomas Monjalon <thomas@monjalon.net>]

After closing a port, it cannot be restarted.
So there is no reason to not free all associated resources.

The last step was done with rte_eth_dev_detach() which is deprecated.
Instead of blindly removing the associated rte_device, the driver should
check if no more port (ethdev, cryptodev, etc) is open for the device.

The last ethdev freeing which were done by rte_eth_dev_detach(),
are now done at the end of rte_eth_dev_close().

Some drivers does not allocate MAC addresses dynamically or separately.
In those cases, the pointer is set to NULL, in order to avoid wrongly
freeing them in rte_eth_dev_release_port().

A closed port will have the state RTE_ETH_DEV_UNUSED which is
considered as invalid by rte_eth_dev_is_valid_port().
So validity is not checked anymore for closed ports in testpmd.

If the driver is trying to free the port again, the function
rte_eth_dev_release_port() will abort with -ENODEV error.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/testpmd.c                    | 16 +++-------------
 doc/guides/rel_notes/release_18_11.rst    |  4 ++++
 drivers/net/af_packet/rte_eth_af_packet.c |  4 +++-
 drivers/net/bnxt/bnxt_ethdev.c            |  4 ----
 drivers/net/failsafe/failsafe_ops.c       |  2 ++
 drivers/net/mlx4/mlx4.c                   |  2 ++
 drivers/net/mlx5/mlx5.c                   |  2 ++
 drivers/net/netvsc/hn_ethdev.c            |  5 ++++-
 drivers/net/pcap/rte_eth_pcap.c           |  5 +++++
 drivers/net/softnic/rte_eth_softnic.c     |  3 ++-
 drivers/net/tap/rte_eth_tap.c             |  3 +++
 drivers/net/vhost/rte_eth_vhost.c         |  4 ----
 lib/librte_ethdev/rte_ethdev.c            |  9 +++------
 lib/librte_ethdev/rte_ethdev.h            |  3 +--
 14 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 31aadec63..2bd3fd986 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1938,18 +1938,6 @@ port_is_started(portid_t port_id)
 	return 1;
 }
 
-static int
-port_is_closed(portid_t port_id)
-{
-	if (port_id_is_invalid(port_id, ENABLED_WARN))
-		return 0;
-
-	if (ports[port_id].port_status != RTE_PORT_CLOSED)
-		return 0;
-
-	return 1;
-}
-
 int
 start_port(portid_t pid)
 {
@@ -2252,6 +2240,8 @@ close_port(portid_t pid)
 			port_flow_flush(pi);
 		rte_eth_dev_close(pi);
 
+		remove_unused_fwd_ports();
+
 		if (rte_atomic16_cmpset(&(port->port_status),
 			RTE_PORT_HANDLING, RTE_PORT_CLOSED) == 0)
 			printf("Port %d cannot be set to closed\n", pi);
@@ -2342,7 +2332,7 @@ detach_port(portid_t port_id)
 
 	printf("Detaching a port...\n");
 
-	if (!port_is_closed(port_id)) {
+	if (ports[port_id].port_status != RTE_PORT_CLOSED) {
 		if (ports[port_id].port_status != RTE_PORT_STOPPED) {
 			printf("Port not stopped\n");
 			return;
diff --git a/doc/guides/rel_notes/release_18_11.rst b/doc/guides/rel_notes/release_18_11.rst
index 2467d04e2..b982ca80e 100644
--- a/doc/guides/rel_notes/release_18_11.rst
+++ b/doc/guides/rel_notes/release_18_11.rst
@@ -215,6 +215,10 @@ API Changes
   functions were deprecated since 17.05 and are replaced by
   ``rte_mbuf_raw_free()`` and ``rte_pktmbuf_prefree_seg()``.
 
+* ethdev: A call to ``rte_eth_dev_release_port()`` has been added in
+  ``rte_eth_dev_close()``. As a consequence, a closed port is freed
+  and seen as invalid because of its state ``RTE_ETH_DEV_UNUSED``.
+
 * A new device flag, RTE_ETH_DEV_NOLIVE_MAC_ADDR, changes the order of
   actions inside rte_eth_dev_start regarding MAC set. Some NICs do not
   support MAC changes once the port has started and with this new device
diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 95a98c6b8..2678335f6 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -362,8 +362,10 @@ eth_stats_reset(struct rte_eth_dev *dev)
 }
 
 static void
-eth_dev_close(struct rte_eth_dev *dev __rte_unused)
+eth_dev_close(struct rte_eth_dev *dev)
 {
+	/* mac_addrs must not be freed alone because part of dev_private */
+	dev->data->mac_addrs = NULL;
 }
 
 static void
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 801c6ffad..141bd6376 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -712,10 +712,6 @@ static void bnxt_dev_close_op(struct rte_eth_dev *eth_dev)
 	if (bp->dev_stopped == 0)
 		bnxt_dev_stop_op(eth_dev);
 
-	if (eth_dev->data->mac_addrs != NULL) {
-		rte_free(eth_dev->data->mac_addrs);
-		eth_dev->data->mac_addrs = NULL;
-	}
 	if (bp->grp_info != NULL) {
 		rte_free(bp->grp_info);
 		bp->grp_info = NULL;
diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index 7f8bcd4c6..f10433b30 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -331,6 +331,8 @@ fs_dev_close(struct rte_eth_dev *dev)
 		sdev->state = DEV_ACTIVE - 1;
 	}
 	fs_dev_free_queues(dev);
+	/* mac_addrs must not be freed alone because part of dev_private */
+	dev->data->mac_addrs = NULL;
 	fs_unlock(dev, 0);
 }
 
diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 7f07b8dc0..4a7b5e220 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -209,6 +209,8 @@ mlx4_dev_close(struct rte_eth_dev *dev)
 		mlx4_rx_queue_release(dev->data->rx_queues[i]);
 	for (i = 0; i != dev->data->nb_tx_queues; ++i)
 		mlx4_tx_queue_release(dev->data->tx_queues[i]);
+	/* mac_addrs must not be freed because part of dev_private */
+	dev->data->mac_addrs = NULL;
 	mlx4_mr_release(dev);
 	if (priv->pd != NULL) {
 		assert(priv->ctx != NULL);
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index b2be74b51..82dd7ad41 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -337,6 +337,8 @@ mlx5_dev_close(struct rte_eth_dev *dev)
 	}
 	memset(priv, 0, sizeof(*priv));
 	priv->domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID;
+	/* mac_addrs must not be freed alone because part of dev_private */
+	dev->data->mac_addrs = NULL;
 }
 
 const struct eth_dev_ops mlx5_dev_ops = {
diff --git a/drivers/net/netvsc/hn_ethdev.c b/drivers/net/netvsc/hn_ethdev.c
index aa38ee7a3..5a7463628 100644
--- a/drivers/net/netvsc/hn_ethdev.c
+++ b/drivers/net/netvsc/hn_ethdev.c
@@ -630,11 +630,14 @@ hn_dev_stop(struct rte_eth_dev *dev)
 }
 
 static void
-hn_dev_close(struct rte_eth_dev *dev __rte_unused)
+hn_dev_close(struct rte_eth_dev *dev)
 {
 	PMD_INIT_LOG(DEBUG, "close");
 
 	hn_vf_close(dev);
+
+	/* mac_addrs must not be freed alone because part of dev_private */
+	dev->data->mac_addrs = NULL;
 }
 
 static const struct eth_dev_ops hn_eth_dev_ops = {
diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 7bbe72e25..bfc003663 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -623,6 +623,11 @@ eth_stats_reset(struct rte_eth_dev *dev)
 static void
 eth_dev_close(struct rte_eth_dev *dev __rte_unused)
 {
+	struct pmd_internals *internals = dev->data->dev_private;
+
+	if (internals != NULL && internals->phy_mac == 0)
+		/* not dynamically allocated, must not be freed */
+		dev->data->mac_addrs = NULL;
 }
 
 static void
diff --git a/drivers/net/softnic/rte_eth_softnic.c b/drivers/net/softnic/rte_eth_softnic.c
index 9a2418438..083519b38 100644
--- a/drivers/net/softnic/rte_eth_softnic.c
+++ b/drivers/net/softnic/rte_eth_softnic.c
@@ -194,8 +194,9 @@ pmd_dev_stop(struct rte_eth_dev *dev)
 }
 
 static void
-pmd_dev_close(struct rte_eth_dev *dev __rte_unused)
+pmd_dev_close(struct rte_eth_dev *dev)
 {
+	dev->data->mac_addrs = NULL; /* statically allocated */
 	return;
 }
 
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 53d37b3cb..32f2f888d 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -1000,6 +1000,9 @@ tap_dev_close(struct rte_eth_dev *dev)
 	 * Since TUN device has no more opened file descriptors
 	 * it will be removed from kernel
 	 */
+
+	/* mac_addrs must not be freed alone because part of dev_private */
+	dev->data->mac_addrs = NULL;
 }
 
 static void
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 0cd1a4642..d3e78503b 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -998,12 +998,8 @@ eth_dev_close(struct rte_eth_dev *dev)
 		for (i = 0; i < dev->data->nb_tx_queues; i++)
 			rte_free(dev->data->tx_queues[i]);
 
-	rte_free(dev->data->mac_addrs);
 	free(internal->dev_name);
 	free(internal->iface_name);
-	rte_free(internal);
-
-	dev->data->dev_private = NULL;
 }
 
 static int
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 178800a5b..987ba5ab1 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -367,6 +367,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 {
 	if (eth_dev == NULL)
 		return -EINVAL;
+	if (eth_dev->state == RTE_ETH_DEV_UNUSED)
+		return -ENODEV;
 
 	rte_eth_dev_shared_data_prepare();
 
@@ -1384,12 +1386,7 @@ rte_eth_dev_close(uint16_t port_id)
 	dev->data->dev_started = 0;
 	(*dev->dev_ops->dev_close)(dev);
 
-	dev->data->nb_rx_queues = 0;
-	rte_free(dev->data->rx_queues);
-	dev->data->rx_queues = NULL;
-	dev->data->nb_tx_queues = 0;
-	rte_free(dev->data->tx_queues);
-	dev->data->tx_queues = NULL;
+	rte_eth_dev_release_port(dev);
 }
 
 int
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index fb40c89e0..dcdeb184b 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1802,8 +1802,7 @@ int rte_eth_dev_set_link_down(uint16_t port_id);
 
 /**
  * Close a stopped Ethernet device. The device cannot be restarted!
- * The function frees all resources except for needed by the
- * closed state. To free these resources, call rte_eth_dev_detach().
+ * The function frees all port resources.
  *
  * @param port_id
  *   The port identifier of the Ethernet device.
-- 
2.19.0