From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A283346045; Fri, 10 Jan 2025 04:21:34 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 364C940648; Fri, 10 Jan 2025 04:21:34 +0100 (CET) Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by mails.dpdk.org (Postfix) with ESMTP id BC7E640612 for ; Fri, 10 Jan 2025 04:21:31 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.88.234]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4YTn1q4VKhz22kpR; Fri, 10 Jan 2025 11:19:11 +0800 (CST) Received: from dggemv704-chm.china.huawei.com (unknown [10.3.19.47]) by mail.maildlp.com (Postfix) with ESMTPS id 7F2D11400CA; Fri, 10 Jan 2025 11:21:28 +0800 (CST) Received: from kwepemn100009.china.huawei.com (7.202.194.112) by dggemv704-chm.china.huawei.com (10.3.19.47) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Fri, 10 Jan 2025 11:21:28 +0800 Received: from [10.67.121.59] (10.67.121.59) by kwepemn100009.china.huawei.com (7.202.194.112) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.11; Fri, 10 Jan 2025 11:21:27 +0800 Message-ID: <9177e30c-bb30-5a1e-cdbd-18050c689376@huawei.com> Date: Fri, 10 Jan 2025 11:21:26 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH RESEND v7 2/5] ethdev: fix skip valid port in probing callback From: "lihuisong (C)" To: Stephen Hemminger CC: , , , , Somnath Kotur , Ajit Khaparde , Dariusz Sosnowski , Suanming Mou , Matan Azrad , Ori Kam , Viacheslav Ovsiienko , , References: <20220825024425.10534-1-lihuisong@huawei.com> <20240929055241.29268-1-lihuisong@huawei.com> <20240929055241.29268-3-lihuisong@huawei.com> <7319082e-10ff-000e-abdf-e6d0d3a6549e@huawei.com> In-Reply-To: <7319082e-10ff-000e-abdf-e6d0d3a6549e@huawei.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To kwepemn100009.china.huawei.com (7.202.194.112) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 >> Acked-by: Chengwen Feng >> --- >>   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; > .