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 E7FCD46054; Mon, 13 Jan 2025 13:06:12 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D0F6B40B97; Mon, 13 Jan 2025 13:06:12 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id B0C92402A7 for ; Mon, 13 Jan 2025 13:06:10 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.163.48]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4YWrV91R2pzbk2s; Mon, 13 Jan 2025 20:02:25 +0800 (CST) Received: from dggemv704-chm.china.huawei.com (unknown [10.3.19.47]) by mail.maildlp.com (Postfix) with ESMTPS id A791E180064; Mon, 13 Jan 2025 20:05:32 +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; Mon, 13 Jan 2025 20:05:32 +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; Mon, 13 Jan 2025 20:05:31 +0800 Message-ID: Date: Mon, 13 Jan 2025 20:05:30 +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 v1 2/2] ethdev: fix skip valid port in probing callback From: "lihuisong (C)" To: Thomas Monjalon CC: , , , "Ajit Khaparde" , Somnath Kotur , Praveen Shetty , Andrew Boyer , Dariusz Sosnowski , Viacheslav Ovsiienko , "Bing Zhao" , Ori Kam , Suanming Mou , Matan Azrad , Chaoyong He , Andrew Rybchenko , References: <20250113025521.32703-1-lihuisong@huawei.com> <1988729.PYKUYFuaPT@thomas> <0293d4d1-0df7-63e1-3dba-244729a78bb0@huawei.com> <9389517.CDJkKcVGEf@thomas> <870781f8-951e-7575-0ee0-7b1ceb7833c0@huawei.com> In-Reply-To: <870781f8-951e-7575-0ee0-7b1ceb7833c0@huawei.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) 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 Thomas, 在 2025/1/13 19:23, lihuisong (C) 写道: > > 在 2025/1/13 18:57, Thomas Monjalon 写道: >> 13/01/2025 10:35, lihuisong (C): >>> 在 2025/1/13 16:16, Thomas Monjalon 写道: >>>> 13/01/2025 03:55, 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'. >>>> If you do that, changing the definition of eth_dev_find_free_port() >>>> you allow the application using a port before probing is finished. >>> Yes, it's not reasonable. >>> >>> Thinking your comment twice, I feel that the root cause of this >>> issue is >>> application want to check if the port id is valid. >>> However, application just receive the new event from the device and the >>> port id of this device must be valid when report new event. >>> So application can think the received new event is valid and don't need >>> to check, right? >> Yes >> Do you think it should be highlighted in the API doc? > Security detection is common and always good for application. > So I think it's better to highlight that in doc. > Now I remember why I have to put this patch into the patchset [1] that testpmd support multiple process attach and detach port. Becase patch 4/5 in this series depands on this patch. The setup_attached_port() have to move to eth_event_callback() in testpmd to update something. And the setup_attached_port() would indirectyly check if this port is valid by rte_eth_dev_is_valid_port(). Their caller stack is as follows: eth_event_callback     -->setup_attached_port         -->rte_eth_dev_socket_id             -->rte_eth_dev_is_valid_port From the testpmd's modification, that is to say, it is possible for appllication to call some APIs like rte_eth_dev_socket_id() and indirectyly check if this port id is valid in event new callback. So should we add this patch? I think there are many like these API in ethdev layer. I'm confused a bit now. [1] https://mails.dpdk.org/archives/dev/2024-January/286026.html /Huisong >> >> We currently have this: >>     RTE_ETH_EVENT_NEW,      /**< port is probed */ >> >> >>> If so I think this series can be dropped. >>>> It is the same as changing the state to RTE_ETH_DEV_ATTACHED >>>> before calling the event callback. >>>> >>>> So this is a NACK. >>>> >>>> Why do you need drivers to check the state of a notified device? >>>> If it is RTE_ETH_EVENT_NEW, you know that's a new device, >>>> there is nothing else to check. >>> It just modified the verification about RTE_ETH_DEV_UNUSED in the >>> device >>> driver. >>> Driver not need to know the event. >> Sorry I was not clear. >> I said "drivers", but it should be "apps & drivers" because they can >> both >> register to the event RTE_ETH_EVENT_NEW. >> In some situations, it is convenient for a driver to listen to new ports >> (it was done for failsafe driver). > Yes. but it doesn't matter now.😁 >> >> >> .