From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 7A392235 for ; Wed, 22 Nov 2017 01:27:18 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Nov 2017 16:27:16 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,432,1505804400"; d="scan'208";a="5003527" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.224.243]) ([10.241.224.243]) by orsmga003.jf.intel.com with ESMTP; 21 Nov 2017 16:27:16 -0800 To: Aleksey Baulin Cc: Thomas Monjalon , dev@dpdk.org References: <20171121223917.21560-1-ferruh.yigit@intel.com> From: Ferruh Yigit Message-ID: <28ed8f78-382c-9c46-484c-e4cc08ecc5cf@intel.com> Date: Tue, 21 Nov 2017 16:27:15 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] config: sort PMD config options X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Nov 2017 00:27:19 -0000 On 11/21/2017 3:17 PM, Aleksey Baulin wrote: > Hi Ferruh, > > Not that it really mattered much, but I couldn't help but notice several > irregularities in the order of options that could have been avoided. We are on same boat, couldn't help on not noticing :) > In particular, I believe it would make sense to put all xxx_DEBUG_xxx > options after the real control options, thus making it two classes of > options within each group. Agreed and there are a few more things can be updated, but I tried to avoid more changes, as mentioned I did this for new PMDs more than existing ones, so that new PMDs can easily figure out where to put their config options without causing more mess. And personally I am believer of the quote "A place for everything, everything in its place." > > In the end, I think the regular order of options will be broken sooner > or later anyway, as it's something that's highly difficult to enforce. > Not everyone has the same feel of the "correct" order. I hope can be preserved at some level. > > Please see the comments below in the middle of the patch. > > Thanks, > Aleksey. > > > On Wed, Nov 22, 2017 at 1:39 AM, Ferruh Yigit > wrote: > > No config option changed, added or removed. > Only reshuffle PMD config options mostly to help new PMDs where to put > their new config option. > > Ordered as physical, paravirtual and virtual groups. Alphabetical order > within a group. > > Also tried to group vendor devices together which breaks alphabetical > order in some places. > > Signed-off-by: Ferruh Yigit > > --- >  config/common_base | 214 ++++++++++++++++++++++++++--------------------------- >  1 file changed, 107 insertions(+), 107 deletions(-) > <...> > +# > +# Compile burst-oriented Broadcom PMD driver > +# > +CONFIG_RTE_LIBRTE_BNX2X_PMD=n > +CONFIG_RTE_LIBRTE_BNX2X_DEBUG=n > +CONFIG_RTE_LIBRTE_BNX2X_DEBUG_INIT=n > +CONFIG_RTE_LIBRTE_BNX2X_DEBUG_RX=n > +CONFIG_RTE_LIBRTE_BNX2X_DEBUG_TX=n > +CONFIG_RTE_LIBRTE_BNX2X_MF_SUPPORT=n > > > ​I would move​ this option up after the first one (PMD). Agreed to be careful for oncoming PMDs, but not really willing to change the existing one, not sure if worth updating. > +# > +# Compile burst-oriented Chelsio Terminator (CXGBE) PMD > +# > +CONFIG_RTE_LIBRTE_CXGBE_PMD=y > +CONFIG_RTE_LIBRTE_CXGBE_DEBUG=n > +CONFIG_RTE_LIBRTE_CXGBE_DEBUG_REG=n > +CONFIG_RTE_LIBRTE_CXGBE_DEBUG_MBOX=n > +CONFIG_RTE_LIBRTE_CXGBE_DEBUG_TX=n > +CONFIG_RTE_LIBRTE_CXGBE_DEBUG_RX=n > +CONFIG_RTE_LIBRTE_CXGBE_TPUT=y > > > ​​I would move​ this option up after the first one (PMD). Same as above. > ​ > > + > +# NXP DPAA Bus > +CONFIG_RTE_LIBRTE_DPAA_BUS=n > +CONFIG_RTE_LIBRTE_DPAA_MEMPOOL=n > +CONFIG_RTE_LIBRTE_DPAA_PMD=n > > > ​​I would place this option first in this group, for the sake > of uniformity. Everywhere else a similar option comes first. This is a little different, PMD depends on above ones. That is also why kept grouping them, specific to dpaa(2) case, I am for keeping this same order. > -# > -# Compile burst-oriented Broadcom BNXT PMD driver > -# > -CONFIG_RTE_LIBRTE_BNXT_PMD=y > +CONFIG_RTE_LIBRTE_QEDE_PMD=y > +CONFIG_RTE_LIBRTE_QEDE_DEBUG_INIT=n > +CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO=n > +CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER=n > +CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX=n > +CONFIG_RTE_LIBRTE_QEDE_DEBUG_RX=n > +CONFIG_RTE_LIBRTE_QEDE_VF_TX_SWITCH=y > > > ​​​I would move​ this option up after the first one (PMD). > Not sure about the option below. Given a lengthy comment > to it, perhaps it's best if it stays there. But then, perhaps not. Again, I am for keeping these at it is for now. > ​ > > +#Provides abs path/name of the firmware file. > +#Empty string denotes driver will use default firmware > +CONFIG_RTE_LIBRTE_QEDE_FW="" > <...> > -- > Aleksey Baulin