[dpdk-dev] [PATCH] config: sort PMD config options

[Ferruh Yigit]

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 at intel.com
> <mailto:ferruh.yigit at 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 at intel.com
>     <mailto:ferruh.yigit at 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