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

["lihuisong (C)" <lihuisong@huawei.com>]

在 2025/1/11 1:54, Stephen Hemminger 写道:
> 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
> .

ok，many thanks for your reply.

I will resubmit this patch and send out separately.

And this series that testpmd add attach and detach port for multiple 
process will be updated later.