On Fri, 10 Jan 2025 11:21:26 +0800
"lihuisong (C)" <lihuis...@huawei.com> wrote:
> Hi Stephen,
>
> Can you take a look at my below reply and reconsider this patch?
>
> /Huisong
>
> 在 2024/12/10 9:50, lihuisong (C) 写道:
> > Hi Ferruh, Stephen and Thomas,
> >
> > Can you take a look at this patch? After all, it is an issue in ethdev
> > layer.
> > This also is the fruit we disscussed with Thomas and Ferruh before.
> > Please go back to this thread. If we don't need this patch, please let
> > me know. I will drop it from my upstreaming list.
> >
> > /Huisong
> >
> >
> > 在 2024/9/29 13:52, Huisong Li 写道:
> >> 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
> >> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
> >>
> >> However, 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")
> >>
> >> 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 set
> >> it to
> >> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is valid
> >> if its
> >> device state is 'ALLOCATED' or 'ATTACHED'.
> >>
> >> In addition, the new state has to be placed behind 'REMOVED' to avoid
> >> ABI
> >> break. Fortunately, this ethdev state is internal and applications
> >> can not
> >> access it directly. So this patch encapsulates an API,
> >> rte_eth_dev_is_used,
> >> for ethdev or PMD to call and eliminate concerns about using this state
> >> enum value comparison.
> >>
> >> Fixes: be8cd210379a ("ethdev: fix port probing notification")
> >> Cc: sta...@dpdk.org
> >>
> >> Signed-off-by: Huisong Li <lihuis...@huawei.com>
> >> Acked-by: Chengwen Feng <fengcheng...@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 c6ad764813..7401dcd8b5 100644
> >> --- a/drivers/net/bnxt/bnxt_ethdev.c
> >> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> >> @@ -6612,7 +6612,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_is_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 8d266b0e64..0df49e1f69 100644
> >> --- a/drivers/net/mlx5/mlx5.c
> >> +++ b/drivers/net/mlx5/mlx5.c
> >> @@ -3371,7 +3371,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_is_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 c335a25a82..a87dbb00ff 100644
> >> --- a/lib/ethdev/ethdev_driver.c
> >> +++ b/lib/ethdev/ethdev_driver.c
> >> @@ -55,8 +55,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_is_used(
> >> + rte_eth_devices[i].state));
> >> return i;
> >> }
> >> }
> >> @@ -221,11 +221,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_is_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)
> >> {
> >> @@ -243,7 +250,7 @@ rte_eth_dev_release_port(struct rte_eth_dev
> >> *eth_dev)
> >> if (ret != 0)
> >> return ret;
> >> - if (eth_dev->state != RTE_ETH_DEV_UNUSED)
> >> + if (rte_eth_dev_is_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 abed4784aa..aa35b65848 100644
> >> --- a/lib/ethdev/ethdev_driver.h
> >> +++ b/lib/ethdev/ethdev_driver.h
> >> @@ -1704,6 +1704,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 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_is_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 ec4f731270..05dec6716b 100644
> >> --- a/lib/ethdev/ethdev_pci.h
> >> +++ b/lib/ethdev/ethdev_pci.h
> >> @@ -179,7 +179,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_is_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 b52f1dd9f2..81e70670d9 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_is_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 a1f7efa913..4dc66abb7b 100644
> >> --- a/lib/ethdev/rte_ethdev.c
> >> +++ b/lib/ethdev/rte_ethdev.c
> >> @@ -349,7 +349,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_is_used(rte_eth_devices[port_id].state))
> >> port_id++;
> >> if (port_id >= RTE_MAX_ETHPORTS)
> >> @@ -408,7 +408,7 @@ rte_eth_dev_is_valid_port(uint16_t port_id)
> >> int is_valid;
> >> if (port_id >= RTE_MAX_ETHPORTS ||
> >> - (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED))
> >> + !rte_eth_dev_is_used(rte_eth_devices[port_id].state))
> >> is_valid = 0;
> >> else
> >> is_valid = 1;
> >> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> >> index a9f92006da..9cc37e8cde 100644
> >> --- a/lib/ethdev/rte_ethdev.h
> >> +++ b/lib/ethdev/rte_ethdev.h
> >> @@ -2083,10 +2083,12 @@ 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 attached when definitely probed. */
> >> RTE_ETH_DEV_ATTACHED,
> >> /** Device is in removed state when plug-out is detected. */
> >> RTE_ETH_DEV_REMOVED,
> >> + /** Device is allocated and is set before reporting new event. */
> >> + RTE_ETH_DEV_ALLOCATED,
> >> };
> >> struct rte_eth_dev_sriov {
> >> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> >> index f63dc32aa2..6ecf1ab89d 100644
> >> --- a/lib/ethdev/version.map
> >> +++ b/lib/ethdev/version.map
> >> @@ -349,6 +349,7 @@ INTERNAL {
> >> rte_eth_dev_get_by_name;
> >> rte_eth_dev_is_rx_hairpin_queue;
> >> rte_eth_dev_is_tx_hairpin_queue;
> >> + rte_eth_dev_is_used;
> >> rte_eth_dev_probing_finish;
> >> rte_eth_dev_release_port;
> >> rte_eth_dev_internal_reset;
> > .
Please resubmit for 25.03 release.
But it looks like an API/ABI change since rte_eth_dev_state is visible
to applications.
A more detailed bug report would also help
