Re: [PATCH RESEND v7 2/5] ethdev: fix skip valid port in probing callback

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 10 Jan 2025 11:21:26 +0800
"lihuisong (C)" <lihuisong@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: stable@dpdk.org
> >>
> >> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >> Acked-by: Chengwen Feng <fengchengwen@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