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 A3582A0A0C; Mon, 5 Jul 2021 05:04:02 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 50F0D40686; Mon, 5 Jul 2021 05:04:02 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 117F140141 for ; Mon, 5 Jul 2021 05:03:59 +0200 (CEST) Received: from dggemv704-chm.china.huawei.com (unknown [172.30.72.57]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4GJ9Rg1CLCzZcGC; Mon, 5 Jul 2021 11:00:47 +0800 (CST) Received: from dggema767-chm.china.huawei.com (10.1.198.209) by dggemv704-chm.china.huawei.com (10.3.19.47) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.2176.2; Mon, 5 Jul 2021 11:03:57 +0800 Received: from [10.66.74.184] (10.66.74.184) by dggema767-chm.china.huawei.com (10.1.198.209) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2176.2; Mon, 5 Jul 2021 11:03:56 +0800 To: "Ananyev, Konstantin" , "Yigit, Ferruh" , Andrew Rybchenko , "dev@dpdk.org" CC: Thomas Monjalon References: <1620460836-38506-1-git-send-email-lihuisong@huawei.com> <644f214d-887a-02ae-5476-af8e6d100221@huawei.com> <256aa58e-7455-e861-ee22-eb87ddd2db67@oktetlabs.ru> <15094894-8483-1aef-cdae-700384130fa3@intel.com> <61f83abc-4567-7a42-220d-ccc808a2aaa4@huawei.com> From: Huisong Li Message-ID: Date: Mon, 5 Jul 2021 11:03:56 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.66.74.184] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To dggema767-chm.china.huawei.com (10.1.198.209) X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [RFC] lib/ethdev: add dev configured flag 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 Sender: "dev" 在 2021/7/3 19:04, Ananyev, Konstantin 写道: >> 在 2021/7/2 21:23, Ananyev, Konstantin 写道: >>>> On 7/2/2021 12:08 PM, Andrew Rybchenko wrote: >>>>> @Thomas, @Ferruh, I tend to accept it (with minor style fixes), >>>>> but I need your opinion on it before doing it. >>>>> >>>> I guess we were relying on the user/application to have correct order up until >>>> now, it can be good to add this into the API. OK to add it for me. >>> I don't know do we really need that flag in dev_data or not, >>> but if we do - probably better to reset it at dev_confgure() straight before >>> we start to make any changes in dev_data. >> Sorry, I don't get you. Some fields in rte_eth_dev_data are initialized >> firstly in the probe phase. >> >> Do you mean to add clear this flag at the beginning of dev_configure()? > Yes, just before we start to modify things. In this patch, this flag has been cleared for all scenarios where the rte_eth_dev_data modification fails in the dev_configure(). And it is set to 1 when dev_configure() is configured successfully. Please check the rollback. Thanks😁 > >>> That way SP can also figure out that device is not configured yet, etc. >>> >>>>> Thanks, >>>>> Andrew. >>>>> >>>>> On 6/29/21 5:27 AM, Huisong Li wrote: >>>>>> 在 2021/6/14 23:37, Andrew Rybchenko 写道: >>>>>>> Summary should start from "ethdev: " >>>>>>> >>>>>>> Don't forget to include all maintainers in Cc the next time. >>>>>>> Just use --cc-cmd or --to-cmd options. >>>>>> ok, thanks! >>>>>>> Adding Thomas. >>>>>>> >>>>>>> On 5/8/21 11:00 AM, Huisong Li wrote: >>>>>>>> Currently, if dev_configure is not invoked or fails to be invoked, users >>>>>>>> can still invoke dev_start successfully. This patch adds a >>>>>>>> "dev_configured" >>>>>>>> flag in "rte_eth_dev_data" to control whether dev_start can be invoked. >>>>>>> In theory there is an indirect condition. If number of configured Tx >>>>>>> *and* Rx queues is 0, device is not configured. >>>>>> That's true. If the framework doesn't have this check, each driver needs >>>>>> to do this. >>>>>> >>>>>> But it's a common thing, and it's probably more reasonable to put it in >>>>>> the ethdev layer. >>>>>> >>>>>>> I have no strong opinion on the topic. Extra flag requires >>>>>>> extra housekeeping. Indirect conditions are not always good >>>>>>> and could be a subject to change. >>>>>>> >>>>>>>> Signed-off-by: Huisong Li >>>>>>>> --- >>>>>>>>   lib/ethdev/rte_ethdev.c      | 11 +++++++++++ >>>>>>>>   lib/ethdev/rte_ethdev_core.h |  6 +++++- >>>>>>>>   2 files changed, 16 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >>>>>>>> index a187976..7d74b17 100644 >>>>>>>> --- a/lib/ethdev/rte_ethdev.c >>>>>>>> +++ b/lib/ethdev/rte_ethdev.c >>>>>>>> @@ -1604,6 +1604,8 @@ rte_eth_dev_configure(uint16_t port_id, >>>>>>>> uint16_t nb_rx_q, uint16_t nb_tx_q, >>>>>>>>       } >>>>>>>>         rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q, >>>>>>>> dev_conf, 0); >>>>>>>> +    dev->data->dev_configured = 1; >>>>>>>> + >>>>>>>>       return 0; >>>>>>>>   reset_queues: >>>>>>>>       eth_dev_rx_queue_config(dev, 0); >>>>>>>> @@ -1614,6 +1616,8 @@ rte_eth_dev_configure(uint16_t port_id, >>>>>>>> uint16_t nb_rx_q, uint16_t nb_tx_q, >>>>>>>>           dev->data->mtu = old_mtu; >>>>>>>>         rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q, >>>>>>>> dev_conf, ret); >>>>>>>> +    dev->data->dev_configured = 0; >>>>>>>> + >>>> I would move it before trace function. >>>> >>>>>>>>       return ret; >>>>>>>>   } >>>>>>>>   @@ -1749,6 +1753,13 @@ rte_eth_dev_start(uint16_t port_id) >>>>>>>>         RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_start, -ENOTSUP); >>>>>>>>   +    if (dev->data->dev_configured == 0) { >>>>>>>> +        RTE_ETHDEV_LOG(INFO, >>>>>>>> +            "Device with port_id=%"PRIu16" is not configured.\n", >>>>>>>> +            port_id); >>>> Should log type be warning/error? >>>> >>>>>>>> +        return -EINVAL; >>>>>>>> +    } >>>>>>>> + >>>>>>>>       if (dev->data->dev_started != 0) { >>>>>>>>           RTE_ETHDEV_LOG(INFO, >>>>>>>>               "Device with port_id=%"PRIu16" already started\n", >>>>>>>> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h >>>>>>>> index 4679d94..b508769 100644 >>>>>>>> --- a/lib/ethdev/rte_ethdev_core.h >>>>>>>> +++ b/lib/ethdev/rte_ethdev_core.h >>>>>>>> @@ -167,7 +167,11 @@ struct rte_eth_dev_data { >>>>>>>>           scattered_rx : 1,  /**< RX of scattered packets is ON(1) / >>>>>>>> OFF(0) */ >>>>>>>>           all_multicast : 1, /**< RX all multicast mode ON(1) / >>>>>>>> OFF(0). */ >>>>>>>>           dev_started : 1,   /**< Device state: STARTED(1) / >>>>>>>> STOPPED(0). */ >>>>>>>> -        lro         : 1;   /**< RX LRO is ON(1) / OFF(0) */ >>>>>>>> +        lro         : 1,  /**< RX LRO is ON(1) / OFF(0) */ >>>>>>>> +        dev_configured : 1; >>>>>>>> +        /**< Device configuration state: >>>>>>>> +         * CONFIGURED(1) / NOT CONFIGURED(0). >>>>>>>> +         */ >>>>>>>>       uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT]; >>>>>>>>           /**< Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0). */ >>>>>>>>       uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT]; >>>>>>>> >>>>>>> .