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

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

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;
> .