On 12/6/2022 9:26 AM, Huisong Li wrote:
> The event callback in application may use the macro RTE_ETH_FOREACH_DEV to
> iterate over all enabled ports to do something(like, verifying the port id
> validity) when receive a probing event. If the ethdev state of a port is
> RTE_ETH_DEV_UNUSED, this port will be skipped.
>
> Currently, this state is set to RTE_ETH_DEV_ATTACHED after pushing probing
> event. It means that probing callback will skip this port. But this
> assignment can not move to front of probing notification. See
> commit be8cd210379a ("ethdev: fix port probing notification")
>
OK to abstract ethdev state check to a function, 'rte_eth_dev_in_used'.
BTW, I guess intention is to say 'is' instead of 'in', like
'rte_eth_dev_is_used'.
But not sure if new state is necessary.
I can see from next patches that if testpmd event callback wants to
validate port_id, it is using 'port_id_is_invalid()' function which is
using 'RTE_ETH_FOREACH_DEV' macro.
And using 'RTE_ETH_FOREACH_DEV' macro in callback function has the
problem you described above.
1) Do we really need to validate port_id in the event callback, callback
provided 'port_id' from 'rte_eth_dev_callback_process()' API, from
'dev->data->port_id'. I am not sure if there is case that
'dev->data->port_id' can be invalid.
2) is there any other specific usecase to use 'RTE_ETH_FOREACH_DEV' in
the 'RTE_ETH_EVENT_NEW' event callback function?
> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set the ethdev
> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and to
> RTE_ETH_DEV_ATTACHED when definitely probed. And this port is valid if its
> device state is ALLOCATED or ATTACHED. Naturally, a lot of places had to be
> modified for this change.
>
> Fixes: be8cd210379a ("ethdev: fix port probing notification")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Huisong Li <lihuis...@huawei.com>
> ---
> drivers/net/bnxt/bnxt_ethdev.c | 3 ++-
> drivers/net/mlx5/mlx5.c | 2 +-
> lib/ethdev/ethdev_driver.c | 13 ++++++++++---
> lib/ethdev/ethdev_driver.h | 12 ++++++++++++
> lib/ethdev/ethdev_pci.h | 2 +-
> lib/ethdev/rte_class_eth.c | 2 +-
> lib/ethdev/rte_ethdev.c | 4 ++--
> lib/ethdev/rte_ethdev.h | 4 +++-
> lib/ethdev/version.map | 1 +
> 9 files changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index b3de490d36..7b08353ed5 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -6003,7 +6003,8 @@ bnxt_dev_uninit(struct rte_eth_dev *eth_dev)
>
> PMD_DRV_LOG(DEBUG, "Calling Device uninit\n");
>
> - if (eth_dev->state != RTE_ETH_DEV_UNUSED)
> +
> + if (rte_eth_dev_in_used(eth_dev->state))
> bnxt_dev_close_op(eth_dev);
>
> return 0;
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index e55be8720e..949d8c1367 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -3014,7 +3014,7 @@ mlx5_eth_find_next(uint16_t port_id, struct rte_device
> *odev)
> while (port_id < RTE_MAX_ETHPORTS) {
> struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>
> - if (dev->state != RTE_ETH_DEV_UNUSED &&
> + if (rte_eth_dev_in_used(dev->state) &&
> dev->device &&
> (dev->device == odev ||
> (dev->device->driver &&
> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
> index 0be1e8ca04..7868958dff 100644
> --- a/lib/ethdev/ethdev_driver.c
> +++ b/lib/ethdev/ethdev_driver.c
> @@ -50,8 +50,8 @@ eth_dev_find_free_port(void)
> for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> /* Using shared name field to find a free port. */
> if (eth_dev_shared_data->data[i].name[0] == '\0') {
> - RTE_ASSERT(rte_eth_devices[i].state ==
> - RTE_ETH_DEV_UNUSED);
> + RTE_ASSERT(!rte_eth_dev_in_used(
> + rte_eth_devices[i].state));
> return i;
> }
> }
> @@ -208,11 +208,18 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
> if (rte_eal_process_type() == RTE_PROC_SECONDARY)
> eth_dev_fp_ops_setup(rte_eth_fp_ops + dev->data->port_id, dev);
>
> + dev->state = RTE_ETH_DEV_ALLOCATED;
> rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
>
> dev->state = RTE_ETH_DEV_ATTACHED;
> }
>
> +bool rte_eth_dev_in_used(uint16_t dev_state)
> +{
> + return dev_state == RTE_ETH_DEV_ALLOCATED ||
> + dev_state == RTE_ETH_DEV_ATTACHED;
> +}
> +
> int
> rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
> {
> @@ -221,7 +228,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
>
> eth_dev_shared_data_prepare();
>
> - if (eth_dev->state != RTE_ETH_DEV_UNUSED)
> + if (rte_eth_dev_in_used(eth_dev->state))
> rte_eth_dev_callback_process(eth_dev,
> RTE_ETH_EVENT_DESTROY, NULL);
>
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 6a550cfc83..e04f88fe46 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1542,6 +1542,18 @@ int rte_eth_dev_callback_process(struct rte_eth_dev
> *dev,
> __rte_internal
> void rte_eth_dev_probing_finish(struct rte_eth_dev *dev);
>
> +/**
> + * Check if a Ethernet device state is in used or not
> + *
> + * @param dev_state
> + * The state of the Ethernet device
> + * @return
> + * - true if the state of the Ethernet device is allocated or attached
> + * - false if this state is neither allocated nor attached
> + */
> +__rte_internal
> +bool rte_eth_dev_in_used(uint16_t dev_state);
> +
> /**
> * Create memzone for HW rings.
> * malloc can't be used as the physical address is needed.
> diff --git a/lib/ethdev/ethdev_pci.h b/lib/ethdev/ethdev_pci.h
> index 94b8fba5d7..362e5c2dec 100644
> --- a/lib/ethdev/ethdev_pci.h
> +++ b/lib/ethdev/ethdev_pci.h
> @@ -164,7 +164,7 @@ rte_eth_dev_pci_generic_remove(struct rte_pci_device
> *pci_dev,
> * eth device has been released.
> */
> if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
> - eth_dev->state == RTE_ETH_DEV_UNUSED)
> + !rte_eth_dev_in_used(eth_dev->state))
> return 0;
>
> if (dev_uninit) {
> diff --git a/lib/ethdev/rte_class_eth.c b/lib/ethdev/rte_class_eth.c
> index 838b3a8f9f..a78d503e59 100644
> --- a/lib/ethdev/rte_class_eth.c
> +++ b/lib/ethdev/rte_class_eth.c
> @@ -118,7 +118,7 @@ eth_dev_match(const struct rte_eth_dev *edev,
> const struct rte_kvargs *kvlist = arg->kvlist;
> unsigned int pair;
>
> - if (edev->state == RTE_ETH_DEV_UNUSED)
> + if (!rte_eth_dev_in_used(edev->state))
> return -1;
> if (arg->device != NULL && arg->device != edev->device)
> return -1;
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 5d5e18db1e..a225141937 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -325,7 +325,7 @@ uint16_t
> rte_eth_find_next(uint16_t port_id)
> {
> while (port_id < RTE_MAX_ETHPORTS &&
> - rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)
> + !rte_eth_dev_in_used(rte_eth_devices[port_id].state))
> port_id++;
>
> if (port_id >= RTE_MAX_ETHPORTS)
> @@ -372,7 +372,7 @@ int
> rte_eth_dev_is_valid_port(uint16_t port_id)
> {
> if (port_id >= RTE_MAX_ETHPORTS ||
> - (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED))
> + !rte_eth_dev_in_used(rte_eth_devices[port_id].state))
> return 0;
> else
> return 1;
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index c129ca1eaf..459ad44021 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -2000,7 +2000,9 @@ typedef uint16_t (*rte_tx_callback_fn)(uint16_t
> port_id, uint16_t queue,
> enum rte_eth_dev_state {
> /** Device is unused before being probed. */
> RTE_ETH_DEV_UNUSED = 0,
> - /** Device is attached when allocated in probing. */
> + /** Device is allocated and is set before reporting new event. */
> + RTE_ETH_DEV_ALLOCATED,
> + /** Device is attached when definitely probed. */
> RTE_ETH_DEV_ATTACHED,
> /** Device is in removed state when plug-out is detected. */
> RTE_ETH_DEV_REMOVED,
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 17201fbe0f..66d70f1aac 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -327,4 +327,5 @@ INTERNAL {
> rte_eth_representor_id_get;
> rte_eth_switch_domain_alloc;
> rte_eth_switch_domain_free;
> + rte_eth_dev_in_used;
> };
