| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20191213161021.6c96d8fe@cakuba.netronome.com>
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