From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Aleksey Baulin <Aleksey.Baulin@gmail.com>
Cc: Thomas Monjalon <thomas@monjalon.net>, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] config: sort PMD config options
Date: Tue, 21 Nov 2017 16:27:15 -0800 [thread overview]
Message-ID: <28ed8f78-382c-9c46-484c-e4cc08ecc5cf@intel.com> (raw)
In-Reply-To: <CAJ+OjZ=zJ-3Nw1Yy1WTh2McqB+o6bi9ABW7u=5g4N=9Z1MLUEQ@mail.gmail.com>
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 <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> 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 <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>>
> ---
> 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
next prev parent reply other threads:[~2017-11-22 0:27 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-21 22:39 Ferruh Yigit
2017-11-21 23:17 ` Aleksey Baulin
2017-11-22 0:27 ` Ferruh Yigit [this message]
2018-01-09 15:34 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
2018-01-17 21:02 ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
2018-01-20 16:50 ` [dpdk-dev] [PATCH v4] " Ferruh Yigit
2018-01-22 0:54 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=28ed8f78-382c-9c46-484c-e4cc08ecc5cf@intel.com \
--to=ferruh.yigit@intel.com \
--cc=Aleksey.Baulin@gmail.com \
--cc=dev@dpdk.org \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).