[dpdk-stable] [PATCH 01/11] ethdev: fix debug log of owner id

[dpdk-stable] [PATCH 01/11] ethdev: fix debug log of owner id

[Thomas Monjalon]

The owner id is 64-bit.
On 32-bit environment, it must be printed with PRIX64.

Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
Cc: stable@dpdk.org

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index a357ee09f..72f84d2f3 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -376,7 +376,7 @@ rte_eth_is_valid_owner_id(uint64_t owner_id)
 {
 	if (owner_id == RTE_ETH_DEV_NO_OWNER ||
 	    rte_eth_dev_shared_data->next_owner_id <= owner_id) {
-		RTE_PMD_DEBUG_TRACE("Invalid owner_id=%016lX.\n", owner_id);
+		RTE_PMD_DEBUG_TRACE("Invalid owner_id=%016"PRIX64".\n", owner_id);
 		return 0;
 	}
 	return 1;
@@ -426,7 +426,7 @@ _rte_eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
 	port_owner = &rte_eth_devices[port_id].data->owner;
 	if (port_owner->id != old_owner_id) {
 		RTE_PMD_DEBUG_TRACE("Cannot set owner to port %d already owned"
-				    " by %s_%016lX.\n", port_id,
+				    " by %s_%016"PRIX64".\n", port_id,
 				    port_owner->name, port_owner->id);
 		return -EPERM;
 	}
@@ -439,7 +439,7 @@ _rte_eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
 
 	port_owner->id = new_owner->id;
 
-	RTE_PMD_DEBUG_TRACE("Port %d owner is %s_%016lX.\n", port_id,
+	RTE_PMD_DEBUG_TRACE("Port %d owner is %s_%016"PRIX64".\n", port_id,
 			    new_owner->name, new_owner->id);
 
 	return 0;
@@ -491,8 +491,8 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 		RTE_ETH_FOREACH_DEV_OWNED_BY(port_id, owner_id)
 			memset(&rte_eth_devices[port_id].data->owner, 0,
 			       sizeof(struct rte_eth_dev_owner));
-		RTE_PMD_DEBUG_TRACE("All port owners owned by %016X identifier"
-				    " have removed.\n", owner_id);
+		RTE_PMD_DEBUG_TRACE("All port owners owned by %016"PRIX64
+				" identifier have removed.\n", owner_id);
 	}
 
 	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
-- 
2.16.2

Re: [dpdk-stable] [PATCH 01/11] ethdev: fix debug log of owner id

[Ferruh Yigit]

On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> The owner id is 64-bit.
> On 32-bit environment, it must be printed with PRIX64.
> 
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

[dpdk-stable] [PATCH 02/11] net/failsafe: fix sub-device visibility

[Thomas Monjalon]

The iterator function rte_eth_find_next_owned_by(), used by the
iterator macro RTE_ETH_FOREACH_DEV_OWNED_BY, are ignoring the devices
which are neither ATTACHED nor REMOVED. Thus sub-devices, having
the state DEFERRED, cannot be seen with the ethdev iterator.
The state RTE_ETH_DEV_DEFERRED can be replaced by
RTE_ETH_DEV_ATTACHED + owner.

Fixes: dcd0c9c32b8d ("net/failsafe: use ownership mechanism for slaves")
Cc: stable@dpdk.org

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/failsafe/failsafe_eal.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
index ee89236f1..ce767703f 100644
--- a/drivers/net/failsafe/failsafe_eal.c
+++ b/drivers/net/failsafe/failsafe_eal.c
@@ -98,7 +98,6 @@ fs_bus_init(struct rte_eth_dev *dev)
 		SUB_ID(sdev) = i;
 		sdev->fs_dev = dev;
 		sdev->dev = ETH(sdev)->device;
-		ETH(sdev)->state = RTE_ETH_DEV_DEFERRED;
 		sdev->state = DEV_PROBED;
 	}
 	return 0;
-- 
2.16.2

Re: [dpdk-stable] [dpdk-dev] [PATCH 02/11] net/failsafe: fix sub-device visibility

[Gaëtan Rivet]

I think this patch should be swapped with the next commit
"ethdev: add doxygen comments for each state",
While the comment about "DEFERRED" would be edited in this one to become:
/** The deferred state is deprecated and replaced by ownership. */
                          ^^^^^^^^^^
Otherwise I agree that the device state is not to be used anymore.

On Wed, May 09, 2018 at 11:43:28AM +0200, Thomas Monjalon wrote:
> The iterator function rte_eth_find_next_owned_by(), used by the
> iterator macro RTE_ETH_FOREACH_DEV_OWNED_BY, are ignoring the devices
> which are neither ATTACHED nor REMOVED. Thus sub-devices, having
> the state DEFERRED, cannot be seen with the ethdev iterator.
> The state RTE_ETH_DEV_DEFERRED can be replaced by
> RTE_ETH_DEV_ATTACHED + owner.
> 
> Fixes: dcd0c9c32b8d ("net/failsafe: use ownership mechanism for slaves")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Acked-by: Matan Azrad <matan@mellanox.com>

Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

> ---
>  drivers/net/failsafe/failsafe_eal.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
> index ee89236f1..ce767703f 100644
> --- a/drivers/net/failsafe/failsafe_eal.c
> +++ b/drivers/net/failsafe/failsafe_eal.c
> @@ -98,7 +98,6 @@ fs_bus_init(struct rte_eth_dev *dev)
>  		SUB_ID(sdev) = i;
>  		sdev->fs_dev = dev;
>  		sdev->dev = ETH(sdev)->device;
> -		ETH(sdev)->state = RTE_ETH_DEV_DEFERRED;
>  		sdev->state = DEV_PROBED;
>  	}
>  	return 0;
> -- 
> 2.16.2
> 

-- 
Gaëtan Rivet
6WIND

[dpdk-stable] [PATCH 04/11] drivers/net: use higher level of probing helper for PCI

[Thomas Monjalon]

The drivers avp, bnx2x and liquidio were using the helper function
rte_eth_dev_pci_allocate() and can be replaced by
rte_eth_dev_pci_generic_probe() which calls the former.

Fixes: dcd5c8112bc3 ("ethdev: add PCI driver helpers")
Cc: stable@dpdk.org

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/net/avp/avp_ethdev.c      | 15 ++-------------
 drivers/net/bnx2x/bnx2x_ethdev.c  | 20 +++++---------------
 drivers/net/liquidio/lio_ethdev.c | 15 ++-------------
 3 files changed, 9 insertions(+), 41 deletions(-)

diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c
index 5b3c4cebf..dc97e60e6 100644
--- a/drivers/net/avp/avp_ethdev.c
+++ b/drivers/net/avp/avp_ethdev.c
@@ -1048,19 +1048,8 @@ static int
 eth_avp_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		  struct rte_pci_device *pci_dev)
 {
-	struct rte_eth_dev *eth_dev;
-	int ret;
-
-	eth_dev = rte_eth_dev_pci_allocate(pci_dev,
-					   sizeof(struct avp_adapter));
-	if (eth_dev == NULL)
-		return -ENOMEM;
-
-	ret = eth_avp_dev_init(eth_dev);
-	if (ret)
-		rte_eth_dev_pci_release(eth_dev);
-
-	return ret;
+	return rte_eth_dev_pci_generic_probe(pci_dev, sizeof(struct avp_adapter),
+			eth_avp_dev_init);
 }
 
 static int
diff --git a/drivers/net/bnx2x/bnx2x_ethdev.c b/drivers/net/bnx2x/bnx2x_ethdev.c
index 6ccbb99c9..7a1e9f583 100644
--- a/drivers/net/bnx2x/bnx2x_ethdev.c
+++ b/drivers/net/bnx2x/bnx2x_ethdev.c
@@ -644,24 +644,14 @@ static struct rte_pci_driver rte_bnx2xvf_pmd;
 static int eth_bnx2x_pci_probe(struct rte_pci_driver *pci_drv,
 	struct rte_pci_device *pci_dev)
 {
-	struct rte_eth_dev *eth_dev;
-	int ret;
-
-	eth_dev = rte_eth_dev_pci_allocate(pci_dev, sizeof(struct bnx2x_softc));
-	if (!eth_dev)
-		return -ENOMEM;
-
 	if (pci_drv == &rte_bnx2x_pmd)
-		ret = eth_bnx2x_dev_init(eth_dev);
+		return rte_eth_dev_pci_generic_probe(pci_dev,
+				sizeof(struct bnx2x_softc), eth_bnx2x_dev_init);
 	else if (pci_drv == &rte_bnx2xvf_pmd)
-		ret = eth_bnx2xvf_dev_init(eth_dev);
+		return rte_eth_dev_pci_generic_probe(pci_dev,
+				sizeof(struct bnx2x_softc), eth_bnx2xvf_dev_init);
 	else
-		ret = -EINVAL;
-
-	if (ret)
-		rte_eth_dev_pci_release(eth_dev);
-
-	return ret;
+		return -EINVAL;
 }
 
 static int eth_bnx2x_pci_remove(struct rte_pci_device *pci_dev)
diff --git a/drivers/net/liquidio/lio_ethdev.c b/drivers/net/liquidio/lio_ethdev.c
index a13a566f9..0e0b5d84d 100644
--- a/drivers/net/liquidio/lio_ethdev.c
+++ b/drivers/net/liquidio/lio_ethdev.c
@@ -2110,19 +2110,8 @@ static int
 lio_eth_dev_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		      struct rte_pci_device *pci_dev)
 {
-	struct rte_eth_dev *eth_dev;
-	int ret;
-
-	eth_dev = rte_eth_dev_pci_allocate(pci_dev,
-					   sizeof(struct lio_device));
-	if (eth_dev == NULL)
-		return -ENOMEM;
-
-	ret = lio_eth_dev_init(eth_dev);
-	if (ret)
-		rte_eth_dev_pci_release(eth_dev);
-
-	return ret;
+	return rte_eth_dev_pci_generic_probe(pci_dev, sizeof(struct lio_device),
+			lio_eth_dev_init);
 }
 
 static int
-- 
2.16.2

Re: [dpdk-stable] [dpdk-dev] [PATCH 04/11] drivers/net: use higher level of probing helper for PCI

[Ferruh Yigit]

On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> The drivers avp, bnx2x and liquidio were using the helper function
> rte_eth_dev_pci_allocate() and can be replaced by
> rte_eth_dev_pci_generic_probe() which calls the former.
> 
> Fixes: dcd5c8112bc3 ("ethdev: add PCI driver helpers")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

[dpdk-stable] [PATCH 05/11] ethdev: add probing finish function

[Thomas Monjalon]

A new hook function is added and called inside the PMDs at the end
of the device probing:
	- in primary process, after allocating, init and config
	- in secondary process, after attaching and local init

This new function is almost empty for now.
It will be used later to add some post-initialization processing.

For the PMDs calling the helpers rte_eth_dev_create() or
rte_eth_dev_pci_generic_probe(), the hook rte_eth_dev_probing_finish()
is called from here, and not in the PMD itself.

Note that the helper rte_eth_dev_create() could be used more,
especially for vdevs, avoiding some code duplication in PMDs.

Cc: stable@dpdk.org

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/net/af_packet/rte_eth_af_packet.c |  2 ++
 drivers/net/ark/ark_ethdev.c              |  2 ++
 drivers/net/bonding/rte_eth_bond_pmd.c    |  2 ++
 drivers/net/cxgbe/cxgbe_ethdev.c          |  1 +
 drivers/net/cxgbe/cxgbe_main.c            |  5 +++++
 drivers/net/cxgbe/cxgbevf_ethdev.c        |  1 +
 drivers/net/cxgbe/cxgbevf_main.c          |  5 +++++
 drivers/net/dpaa/dpaa_ethdev.c            |  5 ++++-
 drivers/net/dpaa2/dpaa2_ethdev.c          |  4 +++-
 drivers/net/failsafe/failsafe.c           |  2 ++
 drivers/net/kni/rte_eth_kni.c             |  2 ++
 drivers/net/mlx4/mlx4.c                   |  1 +
 drivers/net/mlx5/mlx5.c                   |  2 ++
 drivers/net/mvpp2/mrvl_ethdev.c           |  1 +
 drivers/net/nfp/nfp_net.c                 |  2 ++
 drivers/net/null/rte_eth_null.c           |  2 ++
 drivers/net/octeontx/octeontx_ethdev.c    |  3 +++
 drivers/net/pcap/rte_eth_pcap.c           |  2 ++
 drivers/net/ring/rte_eth_ring.c           |  1 +
 drivers/net/softnic/rte_eth_softnic.c     |  3 +++
 drivers/net/szedata2/rte_eth_szedata2.c   |  2 ++
 drivers/net/tap/rte_eth_tap.c             |  2 ++
 drivers/net/vhost/rte_eth_vhost.c         |  2 ++
 drivers/net/virtio/virtio_user_ethdev.c   |  3 +++
 lib/librte_ethdev/rte_ethdev.c            |  9 +++++++++
 lib/librte_ethdev/rte_ethdev_driver.h     | 10 ++++++++++
 lib/librte_ethdev/rte_ethdev_pci.h        |  2 ++
 lib/librte_ethdev/rte_ethdev_version.map  |  1 +
 test/test/virtual_pmd.c                   |  2 ++
 29 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 12a08650c..ea47abbf8 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -911,6 +911,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
 	eth_dev->rx_pkt_burst = eth_af_packet_rx;
 	eth_dev->tx_pkt_burst = eth_af_packet_tx;
 
+	rte_eth_dev_probing_finish(eth_dev);
 	return 0;
 }
 
@@ -934,6 +935,7 @@ rte_pmd_af_packet_probe(struct rte_vdev_device *dev)
 		}
 		/* TODO: request info from primary to set up Rx and Tx */
 		eth_dev->dev_ops = &ops;
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}
 
diff --git a/drivers/net/ark/ark_ethdev.c b/drivers/net/ark/ark_ethdev.c
index d275ab7e8..62e4fd35a 100644
--- a/drivers/net/ark/ark_ethdev.c
+++ b/drivers/net/ark/ark_ethdev.c
@@ -422,6 +422,8 @@ eth_ark_dev_init(struct rte_eth_dev *dev)
 			ark->user_data[eth_dev->data->port_id] =
 				ark->user_ext.dev_init(dev, ark->a_bar, p);
 		}
+
+		rte_eth_dev_probing_finish(eth_dev);
 	}
 
 	return ret;
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 0c44a9249..d0941c870 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -3052,6 +3052,7 @@ bond_probe(struct rte_vdev_device *dev)
 		}
 		/* TODO: request info from primary to set up Rx and Tx */
 		eth_dev->dev_ops = &default_dev_ops;
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}
 
@@ -3124,6 +3125,7 @@ bond_probe(struct rte_vdev_device *dev)
 		rte_eth_bond_8023ad_agg_selection_set(port_id, AGG_STABLE);
 	}
 
+	rte_eth_dev_probing_finish(&rte_eth_devices[port_id]);
 	RTE_BOND_LOG(INFO, "Create bonded device %s on port %d in mode %u on "
 			"socket %u.",	name, port_id, bonding_mode, socket_id);
 	return 0;
diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index 3df51b5be..ec71dc426 100644
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -1147,6 +1147,7 @@ static int eth_cxgbe_dev_init(struct rte_eth_dev *eth_dev)
 					eth_dev->rx_pkt_burst;
 				rest_eth_dev->tx_pkt_burst =
 					eth_dev->tx_pkt_burst;
+				rte_eth_dev_probing_finish(rest_eth_dev);
 			}
 		}
 		return 0;
diff --git a/drivers/net/cxgbe/cxgbe_main.c b/drivers/net/cxgbe/cxgbe_main.c
index 74bccd514..9ad5e5493 100644
--- a/drivers/net/cxgbe/cxgbe_main.c
+++ b/drivers/net/cxgbe/cxgbe_main.c
@@ -1435,6 +1435,11 @@ int cxgbe_probe(struct adapter *adapter)
 			err = -1;
 			goto out_free;
 		}
+
+		if (i > 0) {
+			/* First port will be notified by upper layer */
+			rte_eth_dev_probing_finish(eth_dev);
+		}
 	}
 
 	if (adapter->flags & FW_OK) {
diff --git a/drivers/net/cxgbe/cxgbevf_ethdev.c b/drivers/net/cxgbe/cxgbevf_ethdev.c
index 4885b9748..a942ba6b6 100644
--- a/drivers/net/cxgbe/cxgbevf_ethdev.c
+++ b/drivers/net/cxgbe/cxgbevf_ethdev.c
@@ -138,6 +138,7 @@ static int eth_cxgbevf_dev_init(struct rte_eth_dev *eth_dev)
 					eth_dev->rx_pkt_burst;
 				rest_eth_dev->tx_pkt_burst =
 					eth_dev->tx_pkt_burst;
+				rte_eth_dev_probing_finish(rest_eth_dev);
 			}
 		}
 		return 0;
diff --git a/drivers/net/cxgbe/cxgbevf_main.c b/drivers/net/cxgbe/cxgbevf_main.c
index 6c81fd12e..5b3fb5399 100644
--- a/drivers/net/cxgbe/cxgbevf_main.c
+++ b/drivers/net/cxgbe/cxgbevf_main.c
@@ -267,6 +267,11 @@ int cxgbevf_probe(struct adapter *adapter)
 			err = -ENOMEM;
 			goto out_free;
 		}
+
+		if (i > 0) {
+			/* First port will be notified by upper layer */
+			rte_eth_dev_probing_finish(eth_dev);
+		}
 	}
 
 	if (adapter->flags & FW_OK) {
diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index 6bf8c1590..ffeed1159 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -1385,6 +1385,7 @@ rte_dpaa_probe(struct rte_dpaa_driver *dpaa_drv,
 		eth_dev = rte_eth_dev_attach_secondary(dpaa_dev->name);
 		if (!eth_dev)
 			return -ENOMEM;
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}
 
@@ -1434,8 +1435,10 @@ rte_dpaa_probe(struct rte_dpaa_driver *dpaa_drv,
 
 	/* Invoke PMD device initialization function */
 	diag = dpaa_dev_init(eth_dev);
-	if (diag == 0)
+	if (diag == 0) {
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
+	}
 
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
 		rte_free(eth_dev->data->dev_private);
diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index c304b82bd..b5dfd708b 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -2034,8 +2034,10 @@ rte_dpaa2_probe(struct rte_dpaa2_driver *dpaa2_drv,
 
 	/* Invoke PMD device initialization function */
 	diag = dpaa2_dev_init(eth_dev);
-	if (diag == 0)
+	if (diag == 0) {
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
+	}
 
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
 		rte_free(eth_dev->data->dev_private);
diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
index 5e7a8ba1b..fc989c4f5 100644
--- a/drivers/net/failsafe/failsafe.c
+++ b/drivers/net/failsafe/failsafe.c
@@ -259,6 +259,7 @@ fs_eth_dev_create(struct rte_vdev_device *vdev)
 		.fd = -1,
 		.type = RTE_INTR_HANDLE_EXT,
 	};
+	rte_eth_dev_probing_finish(dev);
 	return 0;
 free_args:
 	failsafe_args_free(dev);
@@ -311,6 +312,7 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
 		}
 		/* TODO: request info from primary to set up Rx and Tx */
 		eth_dev->dev_ops = &failsafe_ops;
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}
 
diff --git a/drivers/net/kni/rte_eth_kni.c b/drivers/net/kni/rte_eth_kni.c
index b468138bd..ab63ea427 100644
--- a/drivers/net/kni/rte_eth_kni.c
+++ b/drivers/net/kni/rte_eth_kni.c
@@ -419,6 +419,7 @@ eth_kni_probe(struct rte_vdev_device *vdev)
 		}
 		/* TODO: request info from primary to set up Rx and Tx */
 		eth_dev->dev_ops = &eth_kni_ops;
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}
 
@@ -437,6 +438,7 @@ eth_kni_probe(struct rte_vdev_device *vdev)
 	eth_dev->rx_pkt_burst = eth_kni_rx;
 	eth_dev->tx_pkt_burst = eth_kni_tx;
 
+	rte_eth_dev_probing_finish(eth_dev);
 	return 0;
 
 kni_uninit:
diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 47451b651..7ad61d5a2 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -737,6 +737,7 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
 		/* Update link status once if waiting for LSC. */
 		if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
 			mlx4_link_update(eth_dev, 0);
+		rte_eth_dev_probing_finish(eth_dev);
 		continue;
 port_error:
 		rte_free(priv);
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 5190b9fcd..8c1f6d63e 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -826,6 +826,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 				mlx5_select_rx_function(eth_dev);
 			eth_dev->tx_pkt_burst =
 				mlx5_select_tx_function(eth_dev);
+			rte_eth_dev_probing_finish(eth_dev);
 			continue;
 		}
 		DRV_LOG(DEBUG, "using port %u (%08" PRIx32 ")", port, test);
@@ -1055,6 +1056,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 			goto port_error;
 		}
 		priv->config.max_verbs_prio = verb_priorities;
+		rte_eth_dev_probing_finish(eth_dev);
 		continue;
 port_error:
 		if (priv)
diff --git a/drivers/net/mvpp2/mrvl_ethdev.c b/drivers/net/mvpp2/mrvl_ethdev.c
index 05998bf2d..799de8ad0 100644
--- a/drivers/net/mvpp2/mrvl_ethdev.c
+++ b/drivers/net/mvpp2/mrvl_ethdev.c
@@ -2606,6 +2606,7 @@ mrvl_eth_dev_create(struct rte_vdev_device *vdev, const char *name)
 	eth_dev->device = &vdev->device;
 	eth_dev->dev_ops = &mrvl_ops;
 
+	rte_eth_dev_probing_finish(eth_dev);
 	return 0;
 out_free_mac:
 	rte_free(eth_dev->data->mac_addrs);
diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 048324ec9..02a98b983 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -3130,6 +3130,8 @@ nfp_pf_create_dev(struct rte_pci_device *dev, int port, int ports,
 
 	if (ret)
 		rte_eth_dev_release_port(eth_dev);
+	else
+		rte_eth_dev_probing_finish(eth_dev);
 
 	rte_free(port_name);
 
diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index f04a7d7bf..1d2e6b9e9 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -559,6 +559,7 @@ eth_dev_null_create(struct rte_vdev_device *dev,
 		eth_dev->tx_pkt_burst = eth_null_tx;
 	}
 
+	rte_eth_dev_probing_finish(eth_dev);
 	return 0;
 }
 
@@ -622,6 +623,7 @@ rte_pmd_null_probe(struct rte_vdev_device *dev)
 		}
 		/* TODO: request info from primary to set up Rx and Tx */
 		eth_dev->dev_ops = &ops;
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}
 
diff --git a/drivers/net/octeontx/octeontx_ethdev.c b/drivers/net/octeontx/octeontx_ethdev.c
index 261b17f9b..1854711e4 100644
--- a/drivers/net/octeontx/octeontx_ethdev.c
+++ b/drivers/net/octeontx/octeontx_ethdev.c
@@ -1078,6 +1078,7 @@ octeontx_create(struct rte_vdev_device *dev, int port, uint8_t evdev,
 
 		eth_dev->tx_pkt_burst = octeontx_xmit_pkts;
 		eth_dev->rx_pkt_burst = octeontx_recv_pkts;
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}
 
@@ -1162,6 +1163,7 @@ octeontx_create(struct rte_vdev_device *dev, int port, uint8_t evdev,
 	rte_octeontx_pchan_map[(nic->base_ochan >> 8) & 0x7]
 		[(nic->base_ochan >> 4) & 0xF] = data->port_id;
 
+	rte_eth_dev_probing_finish(eth_dev);
 	return data->port_id;
 
 err:
@@ -1242,6 +1244,7 @@ octeontx_probe(struct rte_vdev_device *dev)
 		}
 		/* TODO: request info from primary to set up Rx and Tx */
 		eth_dev->dev_ops = &octeontx_dev_ops;
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}
 
diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 44c4d8ee0..6bd4a7d79 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -893,6 +893,7 @@ eth_from_pcaps(struct rte_vdev_device *vdev,
 	else
 		eth_dev->tx_pkt_burst = eth_pcap_tx;
 
+	rte_eth_dev_probing_finish(eth_dev);
 	return 0;
 }
 
@@ -924,6 +925,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
 		}
 		/* TODO: request info from primary to set up Rx and Tx */
 		eth_dev->dev_ops = &ops;
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}
 
diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index be934cffa..35b837c3f 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -329,6 +329,7 @@ do_eth_dev_ring_create(const char *name,
 	eth_dev->rx_pkt_burst = eth_ring_rx;
 	eth_dev->tx_pkt_burst = eth_ring_tx;
 
+	rte_eth_dev_probing_finish(eth_dev);
 	*eth_dev_p = eth_dev;
 
 	return data->port_id;
diff --git a/drivers/net/softnic/rte_eth_softnic.c b/drivers/net/softnic/rte_eth_softnic.c
index b1f2780c7..6b3c13e5c 100644
--- a/drivers/net/softnic/rte_eth_softnic.c
+++ b/drivers/net/softnic/rte_eth_softnic.c
@@ -535,6 +535,8 @@ pmd_ethdev_register(struct rte_vdev_device *vdev,
 	soft_dev->data->kdrv = RTE_KDRV_NONE;
 	soft_dev->data->numa_node = numa_node;
 
+	rte_eth_dev_probing_finish(soft_dev);
+
 	return 0;
 }
 
@@ -748,6 +750,7 @@ pmd_probe(struct rte_vdev_device *vdev)
 		}
 		/* TODO: request info from primary to set up Rx and Tx */
 		eth_dev->dev_ops = &pmd_ops;
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}
 
diff --git a/drivers/net/szedata2/rte_eth_szedata2.c b/drivers/net/szedata2/rte_eth_szedata2.c
index d105e50f3..910c64d04 100644
--- a/drivers/net/szedata2/rte_eth_szedata2.c
+++ b/drivers/net/szedata2/rte_eth_szedata2.c
@@ -1841,6 +1841,8 @@ static int szedata2_eth_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 			rte_free(list_entry);
 			return ret;
 		}
+
+		rte_eth_dev_probing_finish(eth_devs[i]);
 	}
 
 	/*
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 172a7ba4c..01232dcdc 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -1532,6 +1532,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		}
 	}
 
+	rte_eth_dev_probing_finish(dev);
 	return 0;
 
 disable_rte_flow:
@@ -1728,6 +1729,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 		}
 		/* TODO: request info from primary to set up Rx and Tx */
 		eth_dev->dev_ops = &ops;
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}
 
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index bd42eee6b..0d000c71c 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1284,6 +1284,7 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 		goto error;
 	}
 
+	rte_eth_dev_probing_finish(eth_dev);
 	return data->port_id;
 
 error:
@@ -1354,6 +1355,7 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
 		}
 		/* TODO: request info from primary to set up Rx and Tx */
 		eth_dev->dev_ops = &ops;
+		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
 	}
 
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 4e7b3c34f..8eab909da 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -563,6 +563,7 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
 			virtio_user_eth_dev_free(eth_dev);
 			goto end;
 		}
+
 	} else {
 		eth_dev = rte_eth_dev_attach_secondary(rte_vdev_device_name(dev));
 		if (!eth_dev)
@@ -575,6 +576,8 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
 		virtio_user_eth_dev_free(eth_dev);
 		goto end;
 	}
+
+	rte_eth_dev_probing_finish(eth_dev);
 	ret = 0;
 
 end:
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 72f84d2f3..6071e3a9b 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -3361,6 +3361,13 @@ _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
 	return rc;
 }
 
+void
+rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
+{
+	if (dev == NULL)
+		return;
+}
+
 int
 rte_eth_dev_rx_intr_ctl(uint16_t port_id, int epfd, int op, void *data)
 {
@@ -3475,6 +3482,8 @@ rte_eth_dev_create(struct rte_device *device, const char *name,
 		goto probe_failed;
 	}
 
+	rte_eth_dev_probing_finish(ethdev);
+
 	return retval;
 probe_failed:
 	/* free ports private data if primary process */
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index da52b7026..3821f0b1d 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -101,6 +101,16 @@ void _rte_eth_dev_reset(struct rte_eth_dev *dev);
 int _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
 		enum rte_eth_event_type event, void *ret_param);
 
+/**
+ * @internal
+ * This is the last step of device probing.
+ * It must be called after a port is allocated and initialized successfully.
+ *
+ * @param dev
+ *  New ethdev port.
+ */
+void rte_eth_dev_probing_finish(struct rte_eth_dev *dev);
+
 /**
  * Create memzone for HW rings.
  * malloc can't be used as the physical address is needed.
diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
index 603287c28..2cfd37274 100644
--- a/lib/librte_ethdev/rte_ethdev_pci.h
+++ b/lib/librte_ethdev/rte_ethdev_pci.h
@@ -175,6 +175,8 @@ rte_eth_dev_pci_generic_probe(struct rte_pci_device *pci_dev,
 	ret = dev_init(eth_dev);
 	if (ret)
 		rte_eth_dev_pci_release(eth_dev);
+	else
+		rte_eth_dev_probing_finish(eth_dev);
 
 	return ret;
 }
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 9c9394c58..2fe2f6ed2 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -199,6 +199,7 @@ DPDK_18.05 {
 	global:
 
 	rte_eth_dev_count_avail;
+	rte_eth_dev_probing_finish;
 	rte_eth_find_next_owned_by;
 	rte_flow_copy;
 	rte_flow_create;
diff --git a/test/test/virtual_pmd.c b/test/test/virtual_pmd.c
index 69b4ba034..f8ddc2db8 100644
--- a/test/test/virtual_pmd.c
+++ b/test/test/virtual_pmd.c
@@ -590,6 +590,8 @@ virtual_ethdev_create(const char *name, struct ether_addr *mac_addr,
 	eth_dev->rx_pkt_burst = virtual_ethdev_rx_burst_success;
 	eth_dev->tx_pkt_burst = virtual_ethdev_tx_burst_success;
 
+	rte_eth_dev_probing_finish(eth_dev);
+
 	return eth_dev->data->port_id;
 
 err:
-- 
2.16.2

Re: [dpdk-stable] [dpdk-dev] [PATCH 05/11] ethdev: add probing finish function

[Stephen Hemminger]

On Wed,  9 May 2018 11:43:31 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> A new hook function is added and called inside the PMDs at the end
> of the device probing:
> 	- in primary process, after allocating, init and config
> 	- in secondary process, after attaching and local init
> 
> This new function is almost empty for now.
> It will be used later to add some post-initialization processing.
> 
> For the PMDs calling the helpers rte_eth_dev_create() or
> rte_eth_dev_pci_generic_probe(), the hook rte_eth_dev_probing_finish()
> is called from here, and not in the PMD itself.
> 
> Note that the helper rte_eth_dev_create() could be used more,
> especially for vdevs, avoiding some code duplication in PMDs.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  drivers/net/af_packet/rte_eth_af_packet.c |  2 ++
>  drivers/net/ark/ark_ethdev.c              |  2 ++
>  drivers/net/bonding/rte_eth_bond_pmd.c    |  2 ++
>  drivers/net/cxgbe/cxgbe_ethdev.c          |  1 +
>  drivers/net/cxgbe/cxgbe_main.c            |  5 +++++
>  drivers/net/cxgbe/cxgbevf_ethdev.c        |  1 +
>  drivers/net/cxgbe/cxgbevf_main.c          |  5 +++++
>  drivers/net/dpaa/dpaa_ethdev.c            |  5 ++++-
>  drivers/net/dpaa2/dpaa2_ethdev.c          |  4 +++-
>  drivers/net/failsafe/failsafe.c           |  2 ++
>  drivers/net/kni/rte_eth_kni.c             |  2 ++
>  drivers/net/mlx4/mlx4.c                   |  1 +
>  drivers/net/mlx5/mlx5.c                   |  2 ++
>  drivers/net/mvpp2/mrvl_ethdev.c           |  1 +
>  drivers/net/nfp/nfp_net.c                 |  2 ++
>  drivers/net/null/rte_eth_null.c           |  2 ++
>  drivers/net/octeontx/octeontx_ethdev.c    |  3 +++
>  drivers/net/pcap/rte_eth_pcap.c           |  2 ++
>  drivers/net/ring/rte_eth_ring.c           |  1 +
>  drivers/net/softnic/rte_eth_softnic.c     |  3 +++
>  drivers/net/szedata2/rte_eth_szedata2.c   |  2 ++
>  drivers/net/tap/rte_eth_tap.c             |  2 ++
>  drivers/net/vhost/rte_eth_vhost.c         |  2 ++
>  drivers/net/virtio/virtio_user_ethdev.c   |  3 +++
>  lib/librte_ethdev/rte_ethdev.c            |  9 +++++++++
>  lib/librte_ethdev/rte_ethdev_driver.h     | 10 ++++++++++
>  lib/librte_ethdev/rte_ethdev_pci.h        |  2 ++
>  lib/librte_ethdev/rte_ethdev_version.map  |  1 +
>  test/test/virtual_pmd.c                   |  2 ++
>  29 files changed, 79 insertions(+), 2 deletions(-)

Rather than changing so many drivers would it be possible to put the finish in bus layer?
For virtual devices could be handle by vdev_probe_all_drivers.

[dpdk-stable] [PATCH 06/11] ethdev: allow ownership operations on unused port

[Thomas Monjalon]

From: Matan Azrad <matan@mellanox.com>

When the state will be updated later than in allocation,
we may need to update the ownership of a port which is
still in state unused.

It will be used to take ownership of a port before it is
declared as available for other entities.

Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 6071e3a9b..ae86d0ba7 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -414,10 +414,14 @@ static int
 _rte_eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
 		       const struct rte_eth_dev_owner *new_owner)
 {
+	struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
 	struct rte_eth_dev_owner *port_owner;
 	int sret;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	if (port_id >= RTE_MAX_ETHPORTS || ethdev->data->name[0] == '\0') {
+		RTE_PMD_DEBUG_TRACE("Port id %"PRIu16" is not allocated.\n", port_id);
+		return -ENODEV;
+	}
 
 	if (!rte_eth_is_valid_owner_id(new_owner->id) &&
 	    !rte_eth_is_valid_owner_id(old_owner_id))
@@ -481,16 +485,17 @@ rte_eth_dev_owner_unset(const uint16_t port_id, const uint64_t owner_id)
 void __rte_experimental
 rte_eth_dev_owner_delete(const uint64_t owner_id)
 {
-	uint16_t port_id;
+	uint32_t port_id;
 
 	rte_eth_dev_shared_data_prepare();
 
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
 
 	if (rte_eth_is_valid_owner_id(owner_id)) {
-		RTE_ETH_FOREACH_DEV_OWNED_BY(port_id, owner_id)
-			memset(&rte_eth_devices[port_id].data->owner, 0,
-			       sizeof(struct rte_eth_dev_owner));
+		for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++)
+			if (rte_eth_devices[port_id].data->owner.id == owner_id)
+				memset(&rte_eth_devices[port_id].data->owner, 0,
+				       sizeof(struct rte_eth_dev_owner));
 		RTE_PMD_DEBUG_TRACE("All port owners owned by %016"PRIX64
 				" identifier have removed.\n", owner_id);
 	}
@@ -502,17 +507,17 @@ int __rte_experimental
 rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
 {
 	int ret = 0;
+	struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
 
 	rte_eth_dev_shared_data_prepare();
 
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
 
-	if (!rte_eth_dev_is_valid_port(port_id)) {
-		RTE_PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+	if (port_id >= RTE_MAX_ETHPORTS || ethdev->data->name[0] == '\0') {
+		RTE_PMD_DEBUG_TRACE("Port id %"PRIu16" is not allocated.\n", port_id);
 		ret = -ENODEV;
 	} else {
-		rte_memcpy(owner, &rte_eth_devices[port_id].data->owner,
-			   sizeof(*owner));
+		rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
 	}
 
 	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
-- 
2.16.2

Re: [dpdk-stable] [dpdk-dev] [PATCH 06/11] ethdev: allow ownership operations on unused port

[Ferruh Yigit]

On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> From: Matan Azrad <matan@mellanox.com>
> 
> When the state will be updated later than in allocation,
> we may need to update the ownership of a port which is
> still in state unused.
> 
> It will be used to take ownership of a port before it is
> declared as available for other entities.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

Hi Thomas,

If I remember correctly port ownership merged last release in last minute,
without much review. Now we are having these updates on it on rc3, not sure if
many people aware of this feature.

Instead of getting these updates in rc3, any chance to postpone to next release
and do more reviews?

Thanks,
ferruh

Re: [dpdk-stable] [dpdk-dev] [PATCH 06/11] ethdev: allow ownership operations on unused port

[Thomas Monjalon]

09/05/2018 20:00, Ferruh Yigit:
> On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> > From: Matan Azrad <matan@mellanox.com>
> > 
> > When the state will be updated later than in allocation,
> > we may need to update the ownership of a port which is
> > still in state unused.
> > 
> > It will be used to take ownership of a port before it is
> > declared as available for other entities.
> > 
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> Hi Thomas,
> 
> If I remember correctly port ownership merged last release in last minute,
> without much review. Now we are having these updates on it on rc3, not sure if
> many people aware of this feature.

The main purpose is about fixing events NEW and DESTROY.

> Instead of getting these updates in rc3, any chance to postpone to next release
> and do more reviews?

They are not updates but fixes, I think we must consider them.

Re: [dpdk-stable] [dpdk-dev] [PATCH 06/11] ethdev: allow ownership operations on unused port

[Stephen Hemminger]

On Wed,  9 May 2018 11:43:32 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	if (port_id >= RTE_MAX_ETHPORTS || ethdev->data->name[0] == '\0') {

Since name being empty now has significance, why not introduce an macro or inline function
to make the test. Also, static checkers don't like pointers which maybe outside valid
range (sometimes).

static inline bool rte_ethdev_is_unused(const struct rte_ethdev *ethdev)
{
	return ethdev->data->name[0] == '\0';
}

#define RTE_ETH_UNUSED_OR_ERR_RET(ethdev, retval)  do { \
	if (!rte_ethdev_is_unused(ethdev)) { \
		RTE_PMD_DEBUG_TRACE("Port already in use=%d\n", ethdev->port_id); \
		return retval; \
	} } while(0)


	

[dpdk-stable] [PATCH 07/11] ethdev: add lock to port allocation check

[Thomas Monjalon]

From: Matan Azrad <matan@mellanox.com>

When comparing the port name, there can be a race condition with
a thread allocating a new port and writing the name at the same time.
It can lead to match with a partial name by error.

The check of the port is now considered as a critical section
protected with locks.

This fix will be even more required for multi-process when the
port availability will rely only on the name, in a following patch.

Fixes: 84934303a17c ("ethdev: synchronize port allocation")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index ae86d0ba7..357be2dca 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -227,8 +227,8 @@ rte_eth_dev_shared_data_prepare(void)
 	rte_spinlock_unlock(&rte_eth_shared_data_lock);
 }
 
-struct rte_eth_dev *
-rte_eth_dev_allocated(const char *name)
+static struct rte_eth_dev *
+rte_eth_dev_allocated_lock_free(const char *name)
 {
 	unsigned i;
 
@@ -240,6 +240,22 @@ rte_eth_dev_allocated(const char *name)
 	return NULL;
 }
 
+struct rte_eth_dev *
+rte_eth_dev_allocated(const char *name)
+{
+	struct rte_eth_dev *ethdev;
+
+	rte_eth_dev_shared_data_prepare();
+
+	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
+
+	ethdev = rte_eth_dev_allocated_lock_free(name);
+
+	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
+
+	return ethdev;
+}
+
 static uint16_t
 rte_eth_dev_find_free_port(void)
 {
@@ -286,7 +302,7 @@ rte_eth_dev_allocate(const char *name)
 		goto unlock;
 	}
 
-	if (rte_eth_dev_allocated(name) != NULL) {
+	if (rte_eth_dev_allocated_lock_free(name) != NULL) {
 		ethdev_log(ERR,
 			"Ethernet Device with name %s already allocated!",
 			name);
-- 
2.16.2

Re: [dpdk-stable] [dpdk-dev] [PATCH 07/11] ethdev: add lock to port allocation check

[Gaëtan Rivet]

Hi,

On Wed, May 09, 2018 at 11:43:33AM +0200, Thomas Monjalon wrote:
> From: Matan Azrad <matan@mellanox.com>
> 
> When comparing the port name, there can be a race condition with
> a thread allocating a new port and writing the name at the same time.
> It can lead to match with a partial name by error.
> 
> The check of the port is now considered as a critical section
> protected with locks.
> 
> This fix will be even more required for multi-process when the
> port availability will rely only on the name, in a following patch.
> 
> Fixes: 84934303a17c ("ethdev: synchronize port allocation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index ae86d0ba7..357be2dca 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -227,8 +227,8 @@ rte_eth_dev_shared_data_prepare(void)
>  	rte_spinlock_unlock(&rte_eth_shared_data_lock);
>  }
>  
> -struct rte_eth_dev *
> -rte_eth_dev_allocated(const char *name)
> +static struct rte_eth_dev *
> +rte_eth_dev_allocated_lock_free(const char *name)

A suggestion about the naming here.
Reading subsequent patches, we can see this function being used during
ethdev allocation routines. The _lock_free suffix is a little
misleading, as for an instant one can think that there is something
being freed about an allocated ethdev lock.

I would suggest

  * rte_eth_dev_allocated_nolock
    or
  * rte_eth_dev_allocated_lockless
    (or even rte_eth_lockless_dev_allocated)

instead.

>  {
>  	unsigned i;
>  
> @@ -240,6 +240,22 @@ rte_eth_dev_allocated(const char *name)
>  	return NULL;
>  }
>  
> +struct rte_eth_dev *
> +rte_eth_dev_allocated(const char *name)
> +{
> +	struct rte_eth_dev *ethdev;
> +
> +	rte_eth_dev_shared_data_prepare();
> +
> +	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
> +
> +	ethdev = rte_eth_dev_allocated_lock_free(name);
> +
> +	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
> +
> +	return ethdev;
> +}
> +
>  static uint16_t
>  rte_eth_dev_find_free_port(void)
>  {
> @@ -286,7 +302,7 @@ rte_eth_dev_allocate(const char *name)
>  		goto unlock;
>  	}
>  
> -	if (rte_eth_dev_allocated(name) != NULL) {
> +	if (rte_eth_dev_allocated_lock_free(name) != NULL) {
>  		ethdev_log(ERR,
>  			"Ethernet Device with name %s already allocated!",
>  			name);
> -- 
> 2.16.2
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-stable] [dpdk-dev] [PATCH 07/11] ethdev: add lock to port allocation check

[Stephen Hemminger]

On Wed, 9 May 2018 14:21:17 +0200
Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:

> A suggestion about the naming here.
> Reading subsequent patches, we can see this function being used during
> ethdev allocation routines. The _lock_free suffix is a little
> misleading, as for an instant one can think that there is something
> being freed about an allocated ethdev lock.
> 
> I would suggest
> 
>   * rte_eth_dev_allocated_nolock
>     or
>   * rte_eth_dev_allocated_lockless
>     (or even rte_eth_lockless_dev_allocated)
> 
> instead.

Personally, used to the convention of:
    rte_eth_dev_find(name)
and
    _rte_eth_dev_find(name)

The _ implies internal version without lock.

Also allocated to me implies a boolean test only.

Re: [dpdk-stable] [dpdk-dev] [PATCH 07/11] ethdev: add lock to port allocation check

[Stephen Hemminger]

On Wed,  9 May 2018 11:43:33 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

>  
> +struct rte_eth_dev *
> +rte_eth_dev_allocated(const char *name)
> +{
> +	struct rte_eth_dev *ethdev;
> +
> +	rte_eth_dev_shared_data_prepare();
> +
> +	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
> +
> +	ethdev = rte_eth_dev_allocated_lock_free(name);
> +
> +	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
> +
> +	return ethdev;
> +}
> +

Not sure about this. The code it self is correct, but it creates
a racy semantic.

If caller doesn't already hold a lock then there is no guarantee that
the device returned won't be destroyed by some other thread or that
the name was just allocated by some other process.

Re: [dpdk-stable] [dpdk-dev] [PATCH 07/11] ethdev: add lock to port allocation check

[Thomas Monjalon]

10/05/2018 22:33, Stephen Hemminger:
> On Wed,  9 May 2018 11:43:33 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> >  
> > +struct rte_eth_dev *
> > +rte_eth_dev_allocated(const char *name)
> > +{
> > +	struct rte_eth_dev *ethdev;
> > +
> > +	rte_eth_dev_shared_data_prepare();
> > +
> > +	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
> > +
> > +	ethdev = rte_eth_dev_allocated_lock_free(name);
> > +
> > +	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
> > +
> > +	return ethdev;
> > +}
> > +
> 
> Not sure about this. The code it self is correct, but it creates
> a racy semantic.
> 
> If caller doesn't already hold a lock then there is no guarantee that
> the device returned won't be destroyed by some other thread

It is an old high level design decision in DPDK:
We do not hold a lock during the whole life of a port.
So it is the application responsibility to not mess its own ports.
The consequence is that one port must be managed by only one thread.

We can discuss the original thread design but it is out of the
scope of this patchset.

> or that the name was just allocated by some other process.

It does not say which process allocated the port, yes.
But the name is unique among processes.
So the process knows for sure what to do with the port having this name.

Re: [dpdk-stable] [dpdk-dev] [PATCH 07/11] ethdev: add lock to port allocation check

[Stephen Hemminger]

On Fri, 11 May 2018 00:10:19 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 10/05/2018 22:33, Stephen Hemminger:
> > On Wed,  9 May 2018 11:43:33 +0200
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> >   
> > >  
> > > +struct rte_eth_dev *
> > > +rte_eth_dev_allocated(const char *name)
> > > +{
> > > +	struct rte_eth_dev *ethdev;
> > > +
> > > +	rte_eth_dev_shared_data_prepare();
> > > +
> > > +	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
> > > +
> > > +	ethdev = rte_eth_dev_allocated_lock_free(name);
> > > +
> > > +	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
> > > +
> > > +	return ethdev;
> > > +}
> > > +  
> > 
> > Not sure about this. The code it self is correct, but it creates
> > a racy semantic.
> > 
> > If caller doesn't already hold a lock then there is no guarantee that
> > the device returned won't be destroyed by some other thread  
> 
> It is an old high level design decision in DPDK:
> We do not hold a lock during the whole life of a port.
> So it is the application responsibility to not mess its own ports.
> The consequence is that one port must be managed by only one thread.
> 
> We can discuss the original thread design but it is out of the
> scope of this patchset.
> 
> > or that the name was just allocated by some other process.  
> 
> It does not say which process allocated the port, yes.
> But the name is unique among processes.
> So the process knows for sure what to do with the port having this name.

For future, I would like to change rte_eth_devices from an array of structures to
an array of pointers.  Reserving a port could be done with atomic exchange, and keep
a bitmap as hint for next free port to choose.  When supporting tunnels etc, it makes sense
to support lots of ports (like > 16 bit); and devices may come and go.

Also, change link state in eth device into full operational state value.
That should be enough to cover both tunnel and failsafe usage, and existing state
value can go away.

The ownership model should also be expressed more as functional operations in the
device model. It needs to be used consistently in multiple places, allow more layering
and also have more error checks builtin.

[dpdk-stable] [PATCH 08/11] ethdev: fix port visibility before initialization

[Thomas Monjalon]

The port was set to the state ATTACHED during allocation.
The consequence was to iterate over ports which are not initialized.

The state ATTACHED is now set as the last step of probing.

The uniqueness of port name is now checked before the availability
of a port id for allocation (order reversed).

As the state is not set on allocation anymore, it is also not checked
in the function telling whether a port is allocated or not.
The name of the port is set on allocation, so it is enough as a check.

Fixes: 5588909af21b ("ethdev: add device iterator")
Cc: stable@dpdk.org

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 lib/librte_ethdev/rte_ethdev.c        | 18 +++++++++---------
 lib/librte_ethdev/rte_ethdev_driver.h |  2 ++
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 357be2dca..91cd0db11 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -233,7 +233,7 @@ rte_eth_dev_allocated_lock_free(const char *name)
 	unsigned i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-		if ((rte_eth_devices[i].state == RTE_ETH_DEV_ATTACHED) &&
+		if (rte_eth_devices[i].data != NULL &&
 		    strcmp(rte_eth_devices[i].data->name, name) == 0)
 			return &rte_eth_devices[i];
 	}
@@ -278,7 +278,6 @@ eth_dev_get(uint16_t port_id)
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
 
 	eth_dev->data = &rte_eth_dev_shared_data->data[port_id];
-	eth_dev->state = RTE_ETH_DEV_ATTACHED;
 
 	eth_dev_last_created_port = port_id;
 
@@ -296,16 +295,15 @@ rte_eth_dev_allocate(const char *name)
 	/* Synchronize port creation between primary and secondary threads. */
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
 
-	port_id = rte_eth_dev_find_free_port();
-	if (port_id == RTE_MAX_ETHPORTS) {
-		ethdev_log(ERR, "Reached maximum number of Ethernet ports");
+	if (rte_eth_dev_allocated_lock_free(name) != NULL) {
+		ethdev_log(ERR, "Ethernet device with name %s already allocated",
+				name);
 		goto unlock;
 	}
 
-	if (rte_eth_dev_allocated_lock_free(name) != NULL) {
-		ethdev_log(ERR,
-			"Ethernet Device with name %s already allocated!",
-			name);
+	port_id = rte_eth_dev_find_free_port();
+	if (port_id == RTE_MAX_ETHPORTS) {
+		ethdev_log(ERR, "Reached maximum number of Ethernet ports");
 		goto unlock;
 	}
 
@@ -3387,6 +3385,8 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
 {
 	if (dev == NULL)
 		return;
+
+	dev->state = RTE_ETH_DEV_ATTACHED;
 }
 
 int
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index 3821f0b1d..3640dff68 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -106,6 +106,8 @@ int _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
  * This is the last step of device probing.
  * It must be called after a port is allocated and initialized successfully.
  *
+ * The state is set as RTE_ETH_DEV_ATTACHED.
+ *
  * @param dev
  *  New ethdev port.
  */
-- 
2.16.2

Re: [dpdk-stable] [PATCH 08/11] ethdev: fix port visibility before initialization

[Ferruh Yigit]

On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> The port was set to the state ATTACHED during allocation.
> The consequence was to iterate over ports which are not initialized.
> 
> The state ATTACHED is now set as the last step of probing.
> 
> The uniqueness of port name is now checked before the availability
> of a port id for allocation (order reversed).
> 
> As the state is not set on allocation anymore, it is also not checked
> in the function telling whether a port is allocated or not.
> The name of the port is set on allocation, so it is enough as a check.
> 
> Fixes: 5588909af21b ("ethdev: add device iterator")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c        | 18 +++++++++---------
>  lib/librte_ethdev/rte_ethdev_driver.h |  2 ++
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 357be2dca..91cd0db11 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -233,7 +233,7 @@ rte_eth_dev_allocated_lock_free(const char *name)
>  	unsigned i;
>  
>  	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> -		if ((rte_eth_devices[i].state == RTE_ETH_DEV_ATTACHED) &&
> +		if (rte_eth_devices[i].data != NULL &&
>  		    strcmp(rte_eth_devices[i].data->name, name) == 0)
>  			return &rte_eth_devices[i];
>  	}
> @@ -278,7 +278,6 @@ eth_dev_get(uint16_t port_id)
>  	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
>  
>  	eth_dev->data = &rte_eth_dev_shared_data->data[port_id];
> -	eth_dev->state = RTE_ETH_DEV_ATTACHED;
>  
>  	eth_dev_last_created_port = port_id;
>  
> @@ -296,16 +295,15 @@ rte_eth_dev_allocate(const char *name)
>  	/* Synchronize port creation between primary and secondary threads. */
>  	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
>  
> -	port_id = rte_eth_dev_find_free_port();
> -	if (port_id == RTE_MAX_ETHPORTS) {
> -		ethdev_log(ERR, "Reached maximum number of Ethernet ports");
> +	if (rte_eth_dev_allocated_lock_free(name) != NULL) {
> +		ethdev_log(ERR, "Ethernet device with name %s already allocated",
> +				name);
>  		goto unlock;
>  	}
>  
> -	if (rte_eth_dev_allocated_lock_free(name) != NULL) {
> -		ethdev_log(ERR,
> -			"Ethernet Device with name %s already allocated!",
> -			name);
> +	port_id = rte_eth_dev_find_free_port();
> +	if (port_id == RTE_MAX_ETHPORTS) {
> +		ethdev_log(ERR, "Reached maximum number of Ethernet ports");
>  		goto unlock;
>  	}
>  
> @@ -3387,6 +3385,8 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
>  {
>  	if (dev == NULL)
>  		return;
> +
> +	dev->state = RTE_ETH_DEV_ATTACHED;

Aren't ethdev allocation and probe in different levels? Why not make ethdev
allocation self sufficient but tie it to the device probe() ?

>  }
>  
>  int
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index 3821f0b1d..3640dff68 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -106,6 +106,8 @@ int _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
>   * This is the last step of device probing.
>   * It must be called after a port is allocated and initialized successfully.
>   *
> + * The state is set as RTE_ETH_DEV_ATTACHED.
> + *
>   * @param dev
>   *  New ethdev port.
>   */
> 

Re: [dpdk-stable] [PATCH 08/11] ethdev: fix port visibility before initialization

[Thomas Monjalon]

09/05/2018 20:03, Ferruh Yigit:
> On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> > The port was set to the state ATTACHED during allocation.
> > The consequence was to iterate over ports which are not initialized.
> > 
> > The state ATTACHED is now set as the last step of probing.
> > 
> > The uniqueness of port name is now checked before the availability
> > of a port id for allocation (order reversed).
> > 
> > As the state is not set on allocation anymore, it is also not checked
> > in the function telling whether a port is allocated or not.
> > The name of the port is set on allocation, so it is enough as a check.
> > 
> > Fixes: 5588909af21b ("ethdev: add device iterator")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> > @@ -3387,6 +3385,8 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
> >  {
> >  	if (dev == NULL)
> >  		return;
> > +
> > +	dev->state = RTE_ETH_DEV_ATTACHED;
> 
> Aren't ethdev allocation and probe in different levels? Why not make ethdev
> allocation self sufficient but tie it to the device probe() ?

Ethdev allocation is self sufficient.
Probing is done by the PMD with these steps:
	1/ allocate ethdev port
	2/ initialize device
	3/ notify NEW event and set ethdev status

Re: [dpdk-stable] [dpdk-dev] [PATCH 08/11] ethdev: fix port visibility before initialization

[Stephen Hemminger]

On Wed,  9 May 2018 11:43:34 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> The port was set to the state ATTACHED during allocation.
> The consequence was to iterate over ports which are not initialized.
> 
> The state ATTACHED is now set as the last step of probing.
> 
> The uniqueness of port name is now checked before the availability
> of a port id for allocation (order reversed).
> 
> As the state is not set on allocation anymore, it is also not checked
> in the function telling whether a port is allocated or not.
> The name of the port is set on allocation, so it is enough as a check.
> 
> Fixes: 5588909af21b ("ethdev: add device iterator")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Signed-off-by: Matan Azrad <matan@mellanox.com>

Could these states be described somewhere in the documentation.
Maybe some doc/guides? Maybe even exposed in API under rte_eth_dev_info.

Ideally, the device states could be aligned to the SNMP MIB for
interfaces (RFC2863) which defines complex state model appropriate for devices
and tunnels.

Re: [dpdk-stable] [dpdk-dev] [PATCH 08/11] ethdev: fix port visibility before initialization

[Thomas Monjalon]

10/05/2018 22:40, Stephen Hemminger:
> On Wed,  9 May 2018 11:43:34 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > The port was set to the state ATTACHED during allocation.
> > The consequence was to iterate over ports which are not initialized.
> > 
> > The state ATTACHED is now set as the last step of probing.
> > 
> > The uniqueness of port name is now checked before the availability
> > of a port id for allocation (order reversed).
> > 
> > As the state is not set on allocation anymore, it is also not checked
> > in the function telling whether a port is allocated or not.
> > The name of the port is set on allocation, so it is enough as a check.
> > 
> > Fixes: 5588909af21b ("ethdev: add device iterator")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> 
> Could these states be described somewhere in the documentation.

It is in this patchset:
	ethdev: add doxygen comments for each state

> Maybe some doc/guides? Maybe even exposed in API under rte_eth_dev_info.

It is an internal state. It should not be used by the application.

> Ideally, the device states could be aligned to the SNMP MIB for
> interfaces (RFC2863) which defines complex state model appropriate for devices
> and tunnels.

Something to be discussed, MIB compliance would be a totally new feature
I guess. I think it is a huge work with not a lot of interest.

[dpdk-stable] [PATCH 09/11] ethdev: fix port probing notification

[Thomas Monjalon]

The new device was notified as soon as it was allocated.
It leads to use a device which is not yet initialized.

The notification must be published after the initialization is done
by the PMD, but before the state is changed, in order to let
notified entities taking ownership before general availability.

Fixes: 29aa41e36de7 ("ethdev: add notifications for probing and removal")
Cc: stable@dpdk.org

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c        | 5 ++---
 lib/librte_ethdev/rte_ethdev_driver.h | 2 ++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 91cd0db11..e1209bb2a 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -315,9 +315,6 @@ rte_eth_dev_allocate(const char *name)
 unlock:
 	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
 
-	if (eth_dev != NULL)
-		_rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_NEW, NULL);
-
 	return eth_dev;
 }
 
@@ -3386,6 +3383,8 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
 	if (dev == NULL)
 		return;
 
+	_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
+
 	dev->state = RTE_ETH_DEV_ATTACHED;
 }
 
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index 3640dff68..c9c825e3f 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -106,6 +106,8 @@ int _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
  * This is the last step of device probing.
  * It must be called after a port is allocated and initialized successfully.
  *
+ * The notification RTE_ETH_EVENT_NEW is sent to other entities
+ * (libraries and applications).
  * The state is set as RTE_ETH_DEV_ATTACHED.
  *
  * @param dev
-- 
2.16.2

Re: [dpdk-stable] [PATCH 09/11] ethdev: fix port probing notification

[Ferruh Yigit]

On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> The new device was notified as soon as it was allocated.
> It leads to use a device which is not yet initialized.
> 
> The notification must be published after the initialization is done
> by the PMD, but before the state is changed, in order to let
> notified entities taking ownership before general availability.
> 
> Fixes: 29aa41e36de7 ("ethdev: add notifications for probing and removal")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.c        | 5 ++---
>  lib/librte_ethdev/rte_ethdev_driver.h | 2 ++
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 91cd0db11..e1209bb2a 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -315,9 +315,6 @@ rte_eth_dev_allocate(const char *name)
>  unlock:
>  	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
>  
> -	if (eth_dev != NULL)
> -		_rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_NEW, NULL);
> -
>  	return eth_dev;
>  }
>  
> @@ -3386,6 +3383,8 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
>  	if (dev == NULL)
>  		return;
>  
> +	_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
> +

Technically we can have as many ethdev created as we want in probe() right?
Doesn't have to be a one to one mapping there, having user event in
rte_eth_dev_allocate() guaranties each ethdev created sends the event.

But when you moved this into probe() now one event sent for event, same comment
for previous one, I don't think it is good idea to tie ethdev allocation with
probe()

>  	dev->state = RTE_ETH_DEV_ATTACHED;
>  }
>  
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index 3640dff68..c9c825e3f 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -106,6 +106,8 @@ int _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
>   * This is the last step of device probing.
>   * It must be called after a port is allocated and initialized successfully.
>   *
> + * The notification RTE_ETH_EVENT_NEW is sent to other entities
> + * (libraries and applications).
>   * The state is set as RTE_ETH_DEV_ATTACHED.
>   *
>   * @param dev
> 

Re: [dpdk-stable] [PATCH 09/11] ethdev: fix port probing notification

[Thomas Monjalon]

09/05/2018 20:07, Ferruh Yigit:
> On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> > @@ -3386,6 +3383,8 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
> >  	if (dev == NULL)
> >  		return;
> >  
> > +	_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
> > +
> 
> Technically we can have as many ethdev created as we want in probe() right?

Yes probing can create several ports.

> Doesn't have to be a one to one mapping there, having user event in
> rte_eth_dev_allocate() guaranties each ethdev created sends the event.

Allocation is too early to notify a new port.
We need to wait it is initialized before using it.

> But when you moved this into probe() now one event sent for event, same comment
> for previous one, I don't think it is good idea to tie ethdev allocation with
> probe()

The PMD sends one event per port by calling the appropriate ethdev function.
Event and allocation are not tied. I don't see the issue.

Note the definition of this event has always been about probing,
not allocation:
	RTE_ETH_EVENT_NEW,      /**< port is probed */

[dpdk-stable] [PATCH 10/11] net/failsafe: fix sub-device ownership race

[Thomas Monjalon]

From: Matan Azrad <matan@mellanox.com>

There is time between the sub-device port probing by the sub-device PMD
to the sub-device port ownership taking by a fail-safe port.

In this time, the port is available for the application usage. For
example, the port will be exposed to the applications which use
RTE_ETH_FOREACH_DEV iterator.

Thus, ownership unaware applications may manage the port in this time
what may cause a lot of problematic behaviors in the fail-safe
sub-device initialization.

Register to the ethdev NEW event to take the sub-device port ownership
before it becomes exposed to the application.

Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/failsafe/failsafe.c         | 22 ++++++++++---
 drivers/net/failsafe/failsafe_eal.c     | 58 +++++++++++++++++++++------------
 drivers/net/failsafe/failsafe_ether.c   | 23 +++++++++++++
 drivers/net/failsafe/failsafe_private.h |  4 +++
 4 files changed, 83 insertions(+), 24 deletions(-)

diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
index fc989c4f5..c9d128de3 100644
--- a/drivers/net/failsafe/failsafe.c
+++ b/drivers/net/failsafe/failsafe.c
@@ -204,16 +204,25 @@ fs_eth_dev_create(struct rte_vdev_device *vdev)
 	}
 	snprintf(priv->my_owner.name, sizeof(priv->my_owner.name),
 		 FAILSAFE_OWNER_NAME);
+	DEBUG("Failsafe port %u owner info: %s_%016"PRIX64, dev->data->port_id,
+	      priv->my_owner.name, priv->my_owner.id);
+	ret = rte_eth_dev_callback_register(RTE_ETH_ALL, RTE_ETH_EVENT_NEW,
+					    failsafe_eth_new_event_callback,
+					    dev);
+	if (ret) {
+		ERROR("Failed to register NEW callback");
+		goto free_args;
+	}
 	ret = failsafe_eal_init(dev);
 	if (ret)
-		goto free_args;
+		goto unregister_new_callback;
 	ret = fs_mutex_init(priv);
 	if (ret)
-		goto free_args;
+		goto unregister_new_callback;
 	ret = failsafe_hotplug_alarm_install(dev);
 	if (ret) {
 		ERROR("Could not set up plug-in event detection");
-		goto free_args;
+		goto unregister_new_callback;
 	}
 	mac = &dev->data->mac_addrs[0];
 	if (mac_from_arg) {
@@ -226,7 +235,7 @@ fs_eth_dev_create(struct rte_vdev_device *vdev)
 							       mac);
 			if (ret) {
 				ERROR("Failed to set default MAC address");
-				goto free_args;
+				goto unregister_new_callback;
 			}
 		}
 	} else {
@@ -261,6 +270,9 @@ fs_eth_dev_create(struct rte_vdev_device *vdev)
 	};
 	rte_eth_dev_probing_finish(dev);
 	return 0;
+unregister_new_callback:
+	rte_eth_dev_callback_unregister(RTE_ETH_ALL, RTE_ETH_EVENT_NEW,
+					failsafe_eth_new_event_callback, dev);
 free_args:
 	failsafe_args_free(dev);
 free_subs:
@@ -280,6 +292,8 @@ fs_rte_eth_free(const char *name)
 	dev = rte_eth_dev_allocated(name);
 	if (dev == NULL)
 		return -ENODEV;
+	rte_eth_dev_callback_unregister(RTE_ETH_ALL, RTE_ETH_EVENT_NEW,
+					failsafe_eth_new_event_callback, dev);
 	ret = failsafe_eal_uninit(dev);
 	if (ret)
 		ERROR("Error while uninitializing sub-EAL");
diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
index ce767703f..8f1b9d845 100644
--- a/drivers/net/failsafe/failsafe_eal.c
+++ b/drivers/net/failsafe/failsafe_eal.c
@@ -10,7 +10,7 @@
 static int
 fs_ethdev_portid_get(const char *name, uint16_t *port_id)
 {
-	uint16_t pid;
+	uint32_t pid;
 	size_t len;
 
 	if (name == NULL) {
@@ -18,8 +18,9 @@ fs_ethdev_portid_get(const char *name, uint16_t *port_id)
 		return -EINVAL;
 	}
 	len = strlen(name);
-	RTE_ETH_FOREACH_DEV(pid) {
-		if (!strncmp(name, rte_eth_devices[pid].device->name, len)) {
+	for (pid = 0; pid < RTE_MAX_ETHPORTS; pid++) {
+		if (rte_eth_dev_is_valid_port(pid) &&
+		    !strncmp(name, rte_eth_devices[pid].device->name, len)) {
 			*port_id = pid;
 			return 0;
 		}
@@ -41,6 +42,8 @@ fs_bus_init(struct rte_eth_dev *dev)
 			continue;
 		da = &sdev->devargs;
 		if (fs_ethdev_portid_get(da->name, &pid) != 0) {
+			struct rte_eth_dev_owner pid_owner;
+
 			ret = rte_eal_hotplug_add(da->bus->name,
 						  da->name,
 						  da->args);
@@ -55,12 +58,26 @@ fs_bus_init(struct rte_eth_dev *dev)
 				ERROR("sub_device %d init went wrong", i);
 				return -ENODEV;
 			}
+			/*
+			 * The NEW callback tried to take ownership, check
+			 * whether it succeed or didn't.
+			 */
+			rte_eth_dev_owner_get(pid, &pid_owner);
+			if (pid_owner.id != PRIV(dev)->my_owner.id) {
+				INFO("sub_device %d owner(%s_%016"PRIX64") is not my,"
+				     " owner(%s_%016"PRIX64"), will try again later",
+				     i, pid_owner.name, pid_owner.id,
+				     PRIV(dev)->my_owner.name,
+				     PRIV(dev)->my_owner.id);
+				continue;
+			}
 		} else {
+			/* The sub-device port was found. */
 			char devstr[DEVARGS_MAXLEN] = "";
 			struct rte_devargs *probed_da =
 					rte_eth_devices[pid].device->devargs;
 
-			/* Take control of device probed by EAL options. */
+			/* Take control of probed device. */
 			free(da->args);
 			memset(da, 0, sizeof(*da));
 			if (probed_da != NULL)
@@ -77,22 +94,23 @@ fs_bus_init(struct rte_eth_dev *dev)
 			}
 			INFO("Taking control of a probed sub device"
 			      " %d named %s", i, da->name);
-		}
-		ret = rte_eth_dev_owner_set(pid, &PRIV(dev)->my_owner);
-		if (ret < 0) {
-			INFO("sub_device %d owner set failed (%s),"
-			     " will try again later", i, strerror(-ret));
-			continue;
-		} else if (strncmp(rte_eth_devices[pid].device->name, da->name,
-			   strlen(da->name)) != 0) {
-			/*
-			 * The device probably was removed and its port id was
-			 * reallocated before ownership set.
-			 */
-			rte_eth_dev_owner_unset(pid, PRIV(dev)->my_owner.id);
-			INFO("sub_device %d was probably removed before taking"
-			     " ownership, will try again later", i);
-			continue;
+			ret = rte_eth_dev_owner_set(pid, &PRIV(dev)->my_owner);
+			if (ret < 0) {
+				INFO("sub_device %d owner set failed (%s), "
+				     "will try again later", i, strerror(-ret));
+				continue;
+			} else if (strncmp(rte_eth_devices[pid].device->name,
+				   da->name, strlen(da->name)) != 0) {
+				/*
+				 * The device probably was removed and its port
+				 * id was reallocated before ownership set.
+				 */
+				rte_eth_dev_owner_unset(pid,
+							PRIV(dev)->my_owner.id);
+				INFO("sub_device %d was removed before taking"
+				     " ownership, will try again later", i);
+				continue;
+			}
 		}
 		ETH(sdev) = &rte_eth_devices[pid];
 		SUB_ID(sdev) = i;
diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index b414a7884..ebce87841 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -463,3 +463,26 @@ failsafe_eth_lsc_event_callback(uint16_t port_id __rte_unused,
 	else
 		return 0;
 }
+
+/* Take sub-device ownership before it becomes exposed to the application. */
+int
+failsafe_eth_new_event_callback(uint16_t port_id,
+				enum rte_eth_event_type event __rte_unused,
+				void *cb_arg, void *out __rte_unused)
+{
+	struct rte_eth_dev *fs_dev = cb_arg;
+	struct sub_device *sdev;
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+	size_t len = strlen(dev->device->name);
+	uint8_t i;
+
+	FOREACH_SUBDEV_STATE(sdev, i, fs_dev, DEV_PARSED) {
+		if (sdev->state >= DEV_PROBED)
+			continue;
+		if (strncmp(sdev->devargs.name, dev->device->name, len) != 0)
+			continue;
+		rte_eth_dev_owner_set(port_id, &PRIV(fs_dev)->my_owner);
+		break;
+	}
+	return 0;
+}
diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
index b54f8e336..cd8e0b8c9 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -220,6 +220,10 @@ int failsafe_eth_rmv_event_callback(uint16_t port_id,
 int failsafe_eth_lsc_event_callback(uint16_t port_id,
 				    enum rte_eth_event_type event,
 				    void *cb_arg, void *out);
+int
+failsafe_eth_new_event_callback(uint16_t port_id,
+				enum rte_eth_event_type event,
+				void *cb_arg, void *out);
 
 /* GLOBALS */
 
-- 
2.16.2

Re: [dpdk-stable] [dpdk-dev] [PATCH 10/11] net/failsafe: fix sub-device ownership race

[Gaëtan Rivet]

Hello Matan,

Two nitpicks below:

On Wed, May 09, 2018 at 11:43:36AM +0200, Thomas Monjalon wrote:
> From: Matan Azrad <matan@mellanox.com>
> 
> There is time between the sub-device port probing by the sub-device PMD
> to the sub-device port ownership taking by a fail-safe port.
> 
> In this time, the port is available for the application usage. For
> example, the port will be exposed to the applications which use
> RTE_ETH_FOREACH_DEV iterator.
> 
> Thus, ownership unaware applications may manage the port in this time
> what may cause a lot of problematic behaviors in the fail-safe
> sub-device initialization.
> 
> Register to the ethdev NEW event to take the sub-device port ownership
> before it becomes exposed to the application.
> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")

This fix is relying on the RTE_ETH_EVENT_NEW, an API that I think is not
meant to be backported in the stable release that would be targetted by
this commit id.

I think this fix is useless without the rest of this series, so I don't
know what is exactly planned about the rest (whether it is backported,
and where), but I would only CC stable if this is planned, and only as
soon as the relevant APIs are introduced.

> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/failsafe/failsafe.c         | 22 ++++++++++---
>  drivers/net/failsafe/failsafe_eal.c     | 58 +++++++++++++++++++++------------
>  drivers/net/failsafe/failsafe_ether.c   | 23 +++++++++++++
>  drivers/net/failsafe/failsafe_private.h |  4 +++
>  4 files changed, 83 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> index fc989c4f5..c9d128de3 100644
> --- a/drivers/net/failsafe/failsafe.c
> +++ b/drivers/net/failsafe/failsafe.c
> @@ -204,16 +204,25 @@ fs_eth_dev_create(struct rte_vdev_device *vdev)
>  	}
>  	snprintf(priv->my_owner.name, sizeof(priv->my_owner.name),
>  		 FAILSAFE_OWNER_NAME);
> +	DEBUG("Failsafe port %u owner info: %s_%016"PRIX64, dev->data->port_id,
> +	      priv->my_owner.name, priv->my_owner.id);
> +	ret = rte_eth_dev_callback_register(RTE_ETH_ALL, RTE_ETH_EVENT_NEW,
> +					    failsafe_eth_new_event_callback,
> +					    dev);
> +	if (ret) {
> +		ERROR("Failed to register NEW callback");
> +		goto free_args;
> +	}
>  	ret = failsafe_eal_init(dev);
>  	if (ret)
> -		goto free_args;
> +		goto unregister_new_callback;
>  	ret = fs_mutex_init(priv);
>  	if (ret)
> -		goto free_args;
> +		goto unregister_new_callback;
>  	ret = failsafe_hotplug_alarm_install(dev);
>  	if (ret) {
>  		ERROR("Could not set up plug-in event detection");
> -		goto free_args;
> +		goto unregister_new_callback;
>  	}
>  	mac = &dev->data->mac_addrs[0];
>  	if (mac_from_arg) {
> @@ -226,7 +235,7 @@ fs_eth_dev_create(struct rte_vdev_device *vdev)
>  							       mac);
>  			if (ret) {
>  				ERROR("Failed to set default MAC address");
> -				goto free_args;
> +				goto unregister_new_callback;
>  			}
>  		}
>  	} else {
> @@ -261,6 +270,9 @@ fs_eth_dev_create(struct rte_vdev_device *vdev)
>  	};
>  	rte_eth_dev_probing_finish(dev);
>  	return 0;
> +unregister_new_callback:
> +	rte_eth_dev_callback_unregister(RTE_ETH_ALL, RTE_ETH_EVENT_NEW,
> +					failsafe_eth_new_event_callback, dev);
>  free_args:
>  	failsafe_args_free(dev);
>  free_subs:
> @@ -280,6 +292,8 @@ fs_rte_eth_free(const char *name)
>  	dev = rte_eth_dev_allocated(name);
>  	if (dev == NULL)
>  		return -ENODEV;
> +	rte_eth_dev_callback_unregister(RTE_ETH_ALL, RTE_ETH_EVENT_NEW,
> +					failsafe_eth_new_event_callback, dev);
>  	ret = failsafe_eal_uninit(dev);
>  	if (ret)
>  		ERROR("Error while uninitializing sub-EAL");
> diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
> index ce767703f..8f1b9d845 100644
> --- a/drivers/net/failsafe/failsafe_eal.c
> +++ b/drivers/net/failsafe/failsafe_eal.c
> @@ -10,7 +10,7 @@
>  static int
>  fs_ethdev_portid_get(const char *name, uint16_t *port_id)
>  {
> -	uint16_t pid;
> +	uint32_t pid;

I do not see why the port_id is made uint32_t? Is there a reason?

Otherwise all seems fine. With the proper justification or with uin16_t
pid, and with a second pass on the backport tagging,

Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Thanks,
-- 
Gaëtan Rivet
6WIND

Re: [dpdk-stable] [dpdk-dev] [PATCH 10/11] net/failsafe: fix sub-device ownership race

[Matan Azrad]

Hi Gaetan

Regarding backporting.
This version should be bacported for 18.02.1.
There we have the new event.

Regarding uint32
The maximum port id number can be 0xffff.
In this case the loop will be infinite if we use uint16 to iterate over all the ports.

Hello Matan, Two nitpicks below: On Wed, May 09, 2018 at 11:43:36AM +0200, Thomas Monjalon wrote: > From: Matan Azrad > > There is time between the sub-device port probing by the sub-device PMD > to the sub-device port ownership taking by a fail-safe port. > > In this time, the port is available for the application usage. For > example, the port will be exposed to the applications which use > RTE_ETH_FOREACH_DEV iterator. > > Thus, ownership unaware applications may manage the port in this time > what may cause a lot of problematic behaviors in the fail-safe > sub-device initialization. > > Register to the ethdev NEW event to take the sub-device port ownership > before it becomes exposed to the application. > > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD") This fix is relying on the RTE_ETH_EVENT_NEW, an API that I think is not meant to be backported in the stable release that would be targetted by this commit id. I think this fix is useless without the rest of this series, so I don't know what is exactly planned about the rest (whether it is backported, and where), but I would only CC stable if this is planned, and only as soon as the relevant APIs are introduced. > Cc: stable@dpdk.org > > Signed-off-by: Matan Azrad > --- > drivers/net/failsafe/failsafe.c | 22 ++++++++++--- > drivers/net/failsafe/failsafe_eal.c | 58 +++++++++++++++++++++------------ > drivers/net/failsafe/failsafe_ether.c | 23 +++++++++++++ > drivers/net/failsafe/failsafe_private.h | 4 +++ > 4 files changed, 83 insertions(+), 24 deletions(-) > > diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c > index fc989c4f5..c9d128de3 100644 > --- a/drivers/net/failsafe/failsafe.c > +++ b/drivers/net/failsafe/failsafe.c > @@ -204,16 +204,25 @@ fs_eth_dev_create(struct rte_vdev_device *vdev) > } > snprintf(priv->my_owner.name, sizeof(priv->my_owner.name), > FAILSAFE_OWNER_NAME); > + DEBUG("Failsafe port %u owner info: %s_%016"PRIX64, dev->data->port_id, > + priv->my_owner.name, priv->my_owner.id); > + ret = rte_eth_dev_callback_register(RTE_ETH_ALL, RTE_ETH_EVENT_NEW, > + failsafe_eth_new_event_callback, > + dev); > + if (ret) { > + ERROR("Failed to register NEW callback"); > + goto free_args; > + } > ret = failsafe_eal_init(dev); > if (ret) > - goto free_args; > + goto unregister_new_callback; > ret = fs_mutex_init(priv); > if (ret) > - goto free_args; > + goto unregister_new_callback; > ret = failsafe_hotplug_alarm_install(dev); > if (ret) { > ERROR("Could not set up plug-in event detection"); > - goto free_args; > + goto unregister_new_callback; > } > mac = &dev->data->mac_addrs[0]; > if (mac_from_arg) { > @@ -226,7 +235,7 @@ fs_eth_dev_create(struct rte_vdev_device *vdev) > mac); > if (ret) { > ERROR("Failed to set default MAC address"); > - goto free_args; > + goto unregister_new_callback; > } > } > } else { > @@ -261,6 +270,9 @@ fs_eth_dev_create(struct rte_vdev_device *vdev) > }; > rte_eth_dev_probing_finish(dev); > return 0; > +unregister_new_callback: > + rte_eth_dev_callback_unregister(RTE_ETH_ALL, RTE_ETH_EVENT_NEW, > + failsafe_eth_new_event_callback, dev); > free_args: > failsafe_args_free(dev); > free_subs: > @@ -280,6 +292,8 @@ fs_rte_eth_free(const char *name) > dev = rte_eth_dev_allocated(name); > if (dev == NULL) > return -ENODEV; > + rte_eth_dev_callback_unregister(RTE_ETH_ALL, RTE_ETH_EVENT_NEW, > + failsafe_eth_new_event_callback, dev); > ret = failsafe_eal_uninit(dev); > if (ret) > ERROR("Error while uninitializing sub-EAL"); > diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c > index ce767703f..8f1b9d845 100644 > --- a/drivers/net/failsafe/failsafe_eal.c > +++ b/drivers/net/failsafe/failsafe_eal.c > @@ -10,7 +10,7 @@ > static int > fs_ethdev_portid_get(const char *name, uint16_t *port_id) > { > - uint16_t pid; > + uint32_t pid; I do not see why the port_id is made uint32_t? Is there a reason? Otherwise all seems fine. With the proper justification or with uin16_t pid, and with a second pass on the backport tagging, Acked-by: Gaetan Rivet Thanks, -- Gaëtan Rivet 6WIND

Re: [dpdk-stable] [dpdk-dev] [PATCH 10/11] net/failsafe: fix sub-device ownership race

[Gaëtan Rivet]

On Wed, May 09, 2018 at 01:01:58PM +0000, Matan Azrad wrote:
> Hi Gaetan
> 
> Regarding backporting.
> This version should be bacported for 18.02.1.
> There we have the new event.
> 

Then the fixline should probably reflect this instead.
Targetting the initial failsafe release won't work.

This patch also relies on probing_finish() being introduced, so I guess
the plan is to backport the whole series in 18.02.1?

If so, I think the whole series should target the same commit id within
this release, maybe the introduction of ownership or RTE_ETH_EVENT_NEW.

In any case, I think I recall being told to leave this to stable
maintainers to deal with. However, I do not see the benefit of having
a fixline if the information is meant to be discarded for someone to do
the work again.

> Regarding uint32
> The maximum port id number can be 0xffff.
> In this case the loop will be infinite if we use uint16 to iterate over all the ports.

If RTE_MAX_ETHPORTS is set to 0xffff, an array rte_eth_devices[0xffff]
would be defined statically, and I think other issues would arise
before our being stuck in an infinite loop?

In any case, if this had to be fixed, then there should be a
BUILD_BUG_ON RTE_MAX_ETHPORTS being 0xffff, in the relevant part of
librte_ethdev, instead of relying on librte_ethdev users skirting
shortfalls of the library. Anyone iterating on port IDs should expect the
port_id type to be sufficient to hold this information.

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-stable] [dpdk-dev] [PATCH 10/11] net/failsafe: fix sub-device ownership race

[Thomas Monjalon]

09/05/2018 15:30, Gaëtan Rivet:
> On Wed, May 09, 2018 at 01:01:58PM +0000, Matan Azrad wrote:
> > Hi Gaetan
> > 
> > Regarding backporting.
> > This version should be bacported for 18.02.1.
> > There we have the new event.
> > 
> 
> Then the fixline should probably reflect this instead.
> Targetting the initial failsafe release won't work.
> 
> This patch also relies on probing_finish() being introduced, so I guess
> the plan is to backport the whole series in 18.02.1?

Yes, I will provide a backported series for 18.02.

 
> If so, I think the whole series should target the same commit id within
> this release, maybe the introduction of ownership or RTE_ETH_EVENT_NEW.
> 
> In any case, I think I recall being told to leave this to stable
> maintainers to deal with. However, I do not see the benefit of having
> a fixline if the information is meant to be discarded for someone to do
> the work again.

The information in the Fixes line shows where the bug was introduced.
It is used to do our backports (to know if a fix is relevant)
but it can be used for other purposes like identifying known issues
in a given version.

So, in short, these bugs can be fixed easily in 18.02, but probably not
worth to backport in older releases (no 17.11 backport).

> > Regarding uint32
> > The maximum port id number can be 0xffff.
> > In this case the loop will be infinite if we use uint16 to iterate over all the ports.
> 
> If RTE_MAX_ETHPORTS is set to 0xffff, an array rte_eth_devices[0xffff]
> would be defined statically, and I think other issues would arise
> before our being stuck in an infinite loop?
> 
> In any case, if this had to be fixed, then there should be a
> BUILD_BUG_ON RTE_MAX_ETHPORTS being 0xffff, in the relevant part of
> librte_ethdev, instead of relying on librte_ethdev users skirting
> shortfalls of the library. Anyone iterating on port IDs should expect the
> port_id type to be sufficient to hold this information.

Interesting thought.
I vote for keeping Matan's option as it is correct,
and will accept a patch in 18.08 for your option (BUILD_BUG_ON).
Maybe we should send a deprecation notice before limiting the max
number of ports to 0xfffe? Or is it ridiculous for such unlikely constraint?

Re: [dpdk-stable] [dpdk-dev] [PATCH 10/11] net/failsafe: fix sub-device ownership race

[Gaëtan Rivet]

On Wed, May 09, 2018 at 03:43:31PM +0200, Thomas Monjalon wrote:
> 09/05/2018 15:30, Gaëtan Rivet:
> > On Wed, May 09, 2018 at 01:01:58PM +0000, Matan Azrad wrote:
> > > Regarding uint32
> > > The maximum port id number can be 0xffff.
> > > In this case the loop will be infinite if we use uint16 to iterate over all the ports.
> > 
> > If RTE_MAX_ETHPORTS is set to 0xffff, an array rte_eth_devices[0xffff]
> > would be defined statically, and I think other issues would arise
> > before our being stuck in an infinite loop?
> > 
> > In any case, if this had to be fixed, then there should be a
> > BUILD_BUG_ON RTE_MAX_ETHPORTS being 0xffff, in the relevant part of
> > librte_ethdev, instead of relying on librte_ethdev users skirting
> > shortfalls of the library. Anyone iterating on port IDs should expect the
> > port_id type to be sufficient to hold this information.
> 
> Interesting thought.
> I vote for keeping Matan's option as it is correct,
> and will accept a patch in 18.08 for your option (BUILD_BUG_ON).
> Maybe we should send a deprecation notice before limiting the max
> number of ports to 0xfffe? Or is it ridiculous for such unlikely constraint?
> 
> 

No actually the issue is when RTE_MAX_ETHPORTS is equal (or superior) to
0x10000.

If this is an issue that you think is worth having a workaround here,
you should also consider that rte_eth_find_next_owned_by (and
rte_eth_find_next, even if this one should be phased out), would also
result in an overflow and an infinite loop.

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-stable] [dpdk-dev] [PATCH 10/11] net/failsafe: fix sub-device ownership race

[Thomas Monjalon]

09/05/2018 14:41, Gaëtan Rivet:
> > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> 
> This fix is relying on the RTE_ETH_EVENT_NEW, an API that I think is not
> meant to be backported in the stable release that would be targetted by
> this commit id.

This event was added in 18.02. So yes it can be backported.

> I think this fix is useless without the rest of this series, so I don't
> know what is exactly planned about the rest (whether it is backported,
> and where), but I would only CC stable if this is planned, and only as
> soon as the relevant APIs are introduced.

All the series is candidate for 18.02 backport.


> >  static int
> >  fs_ethdev_portid_get(const char *name, uint16_t *port_id)
> >  {
> > -	uint16_t pid;
> > +	uint32_t pid;
> 
> I do not see why the port_id is made uint32_t? Is there a reason?

Copying Matan's answer:
"
The maximum port id number can be 0xffff.
In this case the loop will be infinite if we use uint16 to iterate over all the ports.
"

[dpdk-stable] [PATCH 11/11] ethdev: fix port removal notification timing

[Thomas Monjalon]

From: Matan Azrad <matan@mellanox.com>

When an ethdev port is released, a destroy event is triggered to notify
the users about the released port.

A bit before the destroy event is triggered, the port becomes invalid
by changing its state to UNUSED and cleaning its data. Therefore, the
port is invalid for the destroy event callback process and the users
may get a wrong information of the port.

Move the destroy event emitting to be called before the port
invalidation.

Fixes: 133b54779aa1 ("ethdev: fix port data reset timing")
Fixes: 29aa41e36de7 ("ethdev: add notifications for probing and removal")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index e1209bb2a..45ef13ab8 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -359,6 +359,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 
 	rte_eth_dev_shared_data_prepare();
 
+	_rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_DESTROY, NULL);
+
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
 
 	eth_dev->state = RTE_ETH_DEV_UNUSED;
@@ -367,8 +369,6 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 
 	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
 
-	_rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_DESTROY, NULL);
-
 	return 0;
 }
 
-- 
2.16.2

Re: [dpdk-stable] [PATCH 11/11] ethdev: fix port removal notification timing

[Ferruh Yigit]

On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> From: Matan Azrad <matan@mellanox.com>
> 
> When an ethdev port is released, a destroy event is triggered to notify
> the users about the released port.
> 
> A bit before the destroy event is triggered, the port becomes invalid
> by changing its state to UNUSED and cleaning its data. Therefore, the
> port is invalid for the destroy event callback process and the users
> may get a wrong information of the port.
> 
> Move the destroy event emitting to be called before the port
> invalidation.
> 
> Fixes: 133b54779aa1 ("ethdev: fix port data reset timing")
> Fixes: 29aa41e36de7 ("ethdev: add notifications for probing and removal")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>