Date:   Fri, 13 Dec 2019 16:10:21 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     davem@...emloft.net, arnd@...db.de, maowenan@...wei.com,
        andrew@...n.ch, f.fainelli@...il.com, vivien.didelot@...il.com,
        claudiu.manoil@....com, alexandru.marginean@....com,
        xiaoliang.yang_1@....com, yangbo.lu@....com,
        netdev@...r.kernel.org, alexandre.belloni@...tlin.com,
        UNGLinuxDriver@...rochip.com,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH] net: mscc: ocelot: hide MSCC_OCELOT_SWITCH and move
 outside NET_VENDOR_MICROSEMI

On Thu, 12 Dec 2019 19:11:25 +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
> 
> NET_DSA_MSCC_FELIX and MSCC_OCELOT_SWITCH_OCELOT are 2 different drivers
> that use the same core operations, compiled under MSCC_OCELOT_SWITCH.
> 
> The DSA driver depends on HAVE_NET_DSA, and the switchdev driver depends
> on NET_VENDOR_MICROSEMI. This dependency is purely cosmetic (to hide
> options per driver class, or per networking vendor) from menuconfig
> choices.
> 
> However, there is an issue with the fact that the common core depends on
> NET_VENDOR_MICROSEMI, as can be seen below, when building the DSA
> driver:
> 
> WARNING: unmet direct dependencies detected for MSCC_OCELOT_SWITCH
> Depends on [n]: NETDEVICES [=y] && ETHERNET [=y] &&
> NET_VENDOR_MICROSEMI [=n] && NET_SWITCHDEV [=y] && HAS_IOMEM [=y]
> Selected by [y]:
> NET_DSA_MSCC_FELIX [=y] && NETDEVICES [=y] && HAVE_NET_DSA [=y]
> && NET_DSA [=y] && PCI [=y]
> 
> We don't want to make NET_DSA_MSCC_FELIX hidden under
> NET_VENDOR_MICROSEMI, since it is physically located under
> drivers/net/dsa and already findable in the configurator through DSA.
> 
> So we move the common Ocelot core outside the NET_VENDOR_MICROSEMI
> selector, and we make the switchdev and DSA drivers select it by
> default. In that process, we hide it from menuconfig, since the user
> shouldn't need to know anything about it, and we change it from tristate
> to bool, since it doesn't make a lot of sense to have it as yet another
> loadable kernel module.

Mmm. Is that really the only choice? Wouldn't it be better to do
something like:

diff --git a/drivers/net/ethernet/mscc/Kconfig b/drivers/net/ethernet/mscc/Kconfig
index 13fa11c30f68..991db8b94a9c 100644
--- a/drivers/net/ethernet/mscc/Kconfig
+++ b/drivers/net/ethernet/mscc/Kconfig
@@ -10,7 +10,8 @@ config NET_VENDOR_MICROSEMI
          the questions about Microsemi devices.
 
 config MSCC_OCELOT_SWITCH
-       bool
+       tristate
+       default (MSCC_OCELOT_SWITCH_OCELOT || NET_DSA_MSCC_FELIX)
        depends on NET_SWITCHDEV
        depends on HAS_IOMEM
        select REGMAP_MMIO

Presumably if user wants the end driver to be loadable module the
library should default to a module?

> With this, the DSA driver also needs to explicitly depend on ETHERNET,
> to avoid an unmet dependency situation caused by selecting
> MSCC_OCELOT_SWITCH when ETHERNET is disabled.
> 
> A few more dependencies were shuffled around. HAS_IOMEM is now "depends"
> to avoid a circular dependency:
> 
> symbol HAS_IOMEM is selected by MSCC_OCELOT_SWITCH
> symbol MSCC_OCELOT_SWITCH depends on NETDEVICES
> symbol NETDEVICES is selected by SCSI_CXGB3_ISCSI
> symbol SCSI_CXGB3_ISCSI depends on SCSI_LOWLEVEL
> symbol SCSI_LOWLEVEL depends on SCSI
> symbol SCSI is selected by ATA
> symbol ATA depends on HAS_IOMEM
> 
> Fixes: 56051948773e ("net: dsa: ocelot: add driver for Felix switch family")
> Reported-by: Arnd Bergmann <arnd@...db.de>
> Reported-by: Mao Wenan <maowenan@...wei.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>  drivers/net/dsa/ocelot/Kconfig    |  2 +-
>  drivers/net/ethernet/mscc/Kconfig | 27 ++++++++++++++++-----------
>  2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
> index 0031ca814346..c41d50ca98b7 100644
> --- a/drivers/net/dsa/ocelot/Kconfig
> +++ b/drivers/net/dsa/ocelot/Kconfig
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  config NET_DSA_MSCC_FELIX
>  	tristate "Ocelot / Felix Ethernet switch support"
> -	depends on NET_DSA && PCI
> +	depends on NET_DSA && PCI && ETHERNET
>  	select MSCC_OCELOT_SWITCH
>  	select NET_DSA_TAG_OCELOT
>  	help
> diff --git a/drivers/net/ethernet/mscc/Kconfig b/drivers/net/ethernet/mscc/Kconfig
> index bcec0587cf61..13fa11c30f68 100644
> --- a/drivers/net/ethernet/mscc/Kconfig
> +++ b/drivers/net/ethernet/mscc/Kconfig
> @@ -9,24 +9,29 @@ config NET_VENDOR_MICROSEMI
>  	  kernel: saying N will just cause the configurator to skip all
>  	  the questions about Microsemi devices.
>  
> -if NET_VENDOR_MICROSEMI
> -
>  config MSCC_OCELOT_SWITCH
> -	tristate "Ocelot switch driver"
> +	bool
>  	depends on NET_SWITCHDEV
>  	depends on HAS_IOMEM
> -	select PHYLIB
>  	select REGMAP_MMIO
> +	select PHYLIB

The move of select PHYLIB seems unnecessary, is there a reason?
Otherwise since this is net material perhaps better to refrain from it.

>  	help
> -	  This driver supports the Ocelot network switch device.
> +	  This is the core library for the Vitesse / Microsemi / Microchip
> +	  Ocelot network switch family. It offers a set of DSA-compatible
> +	  switchdev operations for managing switches of this class, like:
> +	  - VSC7514
> +	  - VSC9959
> 
> +if NET_VENDOR_MICROSEMI
>  
>  config MSCC_OCELOT_SWITCH_OCELOT
> -	tristate "Ocelot switch driver on Ocelot"
> -	depends on MSCC_OCELOT_SWITCH
> -	depends on GENERIC_PHY
> -	depends on OF_NET
> +	tristate "Ocelot switch driver for local management CPUs"
> +	select MSCC_OCELOT_SWITCH
> +	select GENERIC_PHY
> +	select OF_NET
>  	help
> -	  This driver supports the Ocelot network switch device as present on
> -	  the Ocelot SoCs.
> +	  This supports the network switch present on the Ocelot SoCs
> +	  (VSC7514). The driver is intended for use on the local MIPS
> +	  management CPU. Frame transfer is through polled I/O or DMA.
>  
>  endif # NET_VENDOR_MICROSEMI

Powered by blists - more mailing lists