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 29FA9A0A0C; Fri, 2 Jul 2021 13:57:14 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A21B241353; Fri, 2 Jul 2021 13:57:13 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mails.dpdk.org (Postfix) with ESMTP id 6763540686 for ; Fri, 2 Jul 2021 13:57:12 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10032"; a="205713879" X-IronPort-AV: E=Sophos;i="5.83,317,1616482800"; d="scan'208";a="205713879" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Jul 2021 04:57:11 -0700 X-IronPort-AV: E=Sophos;i="5.83,317,1616482800"; d="scan'208";a="490229250" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.205.74]) ([10.213.205.74]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Jul 2021 04:57:10 -0700 To: Andrew Rybchenko , Huisong Li , 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> From: Ferruh Yigit X-User: ferruhy Message-ID: <15094894-8483-1aef-cdae-700384130fa3@intel.com> Date: Fri, 2 Jul 2021 13:57:06 +0200 MIME-Version: 1.0 In-Reply-To: <256aa58e-7455-e861-ee22-eb87ddd2db67@oktetlabs.ru> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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" 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. > 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]; >>>> >>> >>> . >