[PATCH net-next] net: build all switchdev drivers as modules when the bridge is a module

[PATCH net-next] net: build all switchdev drivers as modules when the bridge is a module

[Vladimir Oltean]

Currently, all drivers depend on the bool CONFIG_NET_SWITCHDEV, but only
the drivers that call some sort of function exported by the bridge, like
br_vlan_enabled() or whatever, have an extra dependency on CONFIG_BRIDGE.

Since the blamed commit, all switchdev drivers have a functional
dependency upon switchdev_bridge_port_{,un}offload(), which is a pair of
functions exported by the bridge module and not by the bridge-independent
part of CONFIG_NET_SWITCHDEV.

Problems appear when we have:

CONFIG_BRIDGE=m
CONFIG_NET_SWITCHDEV=y
CONFIG_TI_CPSW_SWITCHDEV=y

because cpsw, am65_cpsw and sparx5 will then be built-in but they will
call a symbol exported by a loadable module. This is not possible and
will result in the following build error:

drivers/net/ethernet/ti/cpsw_new.o: in function `cpsw_netdevice_event':
drivers/net/ethernet/ti/cpsw_new.c:1520: undefined reference to
					`switchdev_bridge_port_offload'
drivers/net/ethernet/ti/cpsw_new.c:1537: undefined reference to
					`switchdev_bridge_port_unoffload'

As mentioned, the other switchdev drivers don't suffer from this because
switchdev_bridge_port_offload() is not the first symbol exported by the
bridge that they are calling, so they already needed to deal with this
in the same way.

Fixes: 2f5dc00f7a3e ("net: bridge: switchdev: let drivers inform which bridge ports are offloaded")
Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/microchip/sparx5/Kconfig | 1 +
 drivers/net/ethernet/ti/Kconfig               | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/microchip/sparx5/Kconfig b/drivers/net/ethernet/microchip/sparx5/Kconfig
index 7bdbb2d09a14..d39ae2a6fb49 100644
--- a/drivers/net/ethernet/microchip/sparx5/Kconfig
+++ b/drivers/net/ethernet/microchip/sparx5/Kconfig
@@ -1,5 +1,6 @@
 config SPARX5_SWITCH
 	tristate "Sparx5 switch driver"
+	depends on BRIDGE || BRIDGE=n
 	depends on NET_SWITCHDEV
 	depends on HAS_IOMEM
 	depends on OF
diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
index affcf92cd3aa..7ac8e5ecbe97 100644
--- a/drivers/net/ethernet/ti/Kconfig
+++ b/drivers/net/ethernet/ti/Kconfig
@@ -64,6 +64,7 @@ config TI_CPSW
 config TI_CPSW_SWITCHDEV
 	tristate "TI CPSW Switch Support with switchdev"
 	depends on ARCH_DAVINCI || ARCH_OMAP2PLUS || COMPILE_TEST
+	depends on BRIDGE || BRIDGE=n
 	depends on NET_SWITCHDEV
 	depends on TI_CPTS || !TI_CPTS
 	select PAGE_POOL
@@ -109,6 +110,7 @@ config TI_K3_AM65_CPSW_NUSS
 config TI_K3_AM65_CPSW_SWITCHDEV
 	bool "TI K3 AM654x/J721E CPSW Switch mode support"
 	depends on TI_K3_AM65_CPSW_NUSS
+	depends on BRIDGE || BRIDGE=n
 	depends on NET_SWITCHDEV
 	help
 	 This enables switchdev support for TI K3 CPSWxG Ethernet
-- 
2.25.1

Re: [PATCH net-next] net: build all switchdev drivers as modules when the bridge is a module

[Anders Roxell]

On Mon, 26 Jul 2021 at 16:26, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> Currently, all drivers depend on the bool CONFIG_NET_SWITCHDEV, but only
> the drivers that call some sort of function exported by the bridge, like
> br_vlan_enabled() or whatever, have an extra dependency on CONFIG_BRIDGE.
>
> Since the blamed commit, all switchdev drivers have a functional
> dependency upon switchdev_bridge_port_{,un}offload(), which is a pair of
> functions exported by the bridge module and not by the bridge-independent
> part of CONFIG_NET_SWITCHDEV.
>
> Problems appear when we have:
>
> CONFIG_BRIDGE=m
> CONFIG_NET_SWITCHDEV=y
> CONFIG_TI_CPSW_SWITCHDEV=y
>
> because cpsw, am65_cpsw and sparx5 will then be built-in but they will
> call a symbol exported by a loadable module. This is not possible and
> will result in the following build error:
>
> drivers/net/ethernet/ti/cpsw_new.o: in function `cpsw_netdevice_event':
> drivers/net/ethernet/ti/cpsw_new.c:1520: undefined reference to
>                                         `switchdev_bridge_port_offload'
> drivers/net/ethernet/ti/cpsw_new.c:1537: undefined reference to
>                                         `switchdev_bridge_port_unoffload'
>
> As mentioned, the other switchdev drivers don't suffer from this because
> switchdev_bridge_port_offload() is not the first symbol exported by the
> bridge that they are calling, so they already needed to deal with this
> in the same way.
>
> Fixes: 2f5dc00f7a3e ("net: bridge: switchdev: let drivers inform which bridge ports are offloaded")
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Thank you for providing this fix.

Tested building omap2plus_defconfig.

Tested-by: Anders Roxell <anders.roxell@linaro.org>

Cheers,
Anders

> ---
>  drivers/net/ethernet/microchip/sparx5/Kconfig | 1 +
>  drivers/net/ethernet/ti/Kconfig               | 2 ++
>  2 files changed, 3 insertions(+)
>
> diff --git a/drivers/net/ethernet/microchip/sparx5/Kconfig b/drivers/net/ethernet/microchip/sparx5/Kconfig
> index 7bdbb2d09a14..d39ae2a6fb49 100644
> --- a/drivers/net/ethernet/microchip/sparx5/Kconfig
> +++ b/drivers/net/ethernet/microchip/sparx5/Kconfig
> @@ -1,5 +1,6 @@
>  config SPARX5_SWITCH
>         tristate "Sparx5 switch driver"
> +       depends on BRIDGE || BRIDGE=n
>         depends on NET_SWITCHDEV
>         depends on HAS_IOMEM
>         depends on OF
> diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
> index affcf92cd3aa..7ac8e5ecbe97 100644
> --- a/drivers/net/ethernet/ti/Kconfig
> +++ b/drivers/net/ethernet/ti/Kconfig
> @@ -64,6 +64,7 @@ config TI_CPSW
>  config TI_CPSW_SWITCHDEV
>         tristate "TI CPSW Switch Support with switchdev"
>         depends on ARCH_DAVINCI || ARCH_OMAP2PLUS || COMPILE_TEST
> +       depends on BRIDGE || BRIDGE=n
>         depends on NET_SWITCHDEV
>         depends on TI_CPTS || !TI_CPTS
>         select PAGE_POOL
> @@ -109,6 +110,7 @@ config TI_K3_AM65_CPSW_NUSS
>  config TI_K3_AM65_CPSW_SWITCHDEV
>         bool "TI K3 AM654x/J721E CPSW Switch mode support"
>         depends on TI_K3_AM65_CPSW_NUSS
> +       depends on BRIDGE || BRIDGE=n
>         depends on NET_SWITCHDEV
>         help
>          This enables switchdev support for TI K3 CPSWxG Ethernet
> --
> 2.25.1
>

Re: [PATCH net-next] net: build all switchdev drivers as modules when the bridge is a module

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Mon, 26 Jul 2021 17:25:36 +0300 you wrote:
> Currently, all drivers depend on the bool CONFIG_NET_SWITCHDEV, but only
> the drivers that call some sort of function exported by the bridge, like
> br_vlan_enabled() or whatever, have an extra dependency on CONFIG_BRIDGE.
> 
> Since the blamed commit, all switchdev drivers have a functional
> dependency upon switchdev_bridge_port_{,un}offload(), which is a pair of
> functions exported by the bridge module and not by the bridge-independent
> part of CONFIG_NET_SWITCHDEV.
> 
> [...]

Here is the summary with links:
  - [net-next] net: build all switchdev drivers as modules when the bridge is a module
    https://git.kernel.org/netdev/net-next/c/b0e81817629a

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] net: build all switchdev drivers as modules when the bridge is a module

[Arnd Bergmann]

On Mon, Jul 26, 2021 at 4:28 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> Currently, all drivers depend on the bool CONFIG_NET_SWITCHDEV, but only
> the drivers that call some sort of function exported by the bridge, like
> br_vlan_enabled() or whatever, have an extra dependency on CONFIG_BRIDGE.
>
> Since the blamed commit, all switchdev drivers have a functional
> dependency upon switchdev_bridge_port_{,un}offload(), which is a pair of
> functions exported by the bridge module and not by the bridge-independent
> part of CONFIG_NET_SWITCHDEV.
>
> Problems appear when we have:
>
> CONFIG_BRIDGE=m
> CONFIG_NET_SWITCHDEV=y
> CONFIG_TI_CPSW_SWITCHDEV=y
>
> because cpsw, am65_cpsw and sparx5 will then be built-in but they will
> call a symbol exported by a loadable module. This is not possible and
> will result in the following build error:
>
> drivers/net/ethernet/ti/cpsw_new.o: in function `cpsw_netdevice_event':
> drivers/net/ethernet/ti/cpsw_new.c:1520: undefined reference to
>                                         `switchdev_bridge_port_offload'
> drivers/net/ethernet/ti/cpsw_new.c:1537: undefined reference to
>                                         `switchdev_bridge_port_unoffload'
>
> As mentioned, the other switchdev drivers don't suffer from this because
> switchdev_bridge_port_offload() is not the first symbol exported by the
> bridge that they are calling, so they already needed to deal with this
> in the same way.
>
> Fixes: 2f5dc00f7a3e ("net: bridge: switchdev: let drivers inform which bridge ports are offloaded")
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

I'm still seeing build failures after this patch was applied. I have a fixup
patch that seems to work, but I'm still not sure if that version is complete.

      Arnd

Re: [PATCH net-next] net: build all switchdev drivers as modules when the bridge is a module

[Grygorii Strashko]

Hi All,

On 02/08/2021 17:47, Arnd Bergmann wrote:
> On Mon, Jul 26, 2021 at 4:28 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>>
>> Currently, all drivers depend on the bool CONFIG_NET_SWITCHDEV, but only
>> the drivers that call some sort of function exported by the bridge, like
>> br_vlan_enabled() or whatever, have an extra dependency on CONFIG_BRIDGE.
>>
>> Since the blamed commit, all switchdev drivers have a functional
>> dependency upon switchdev_bridge_port_{,un}offload(), which is a pair of
>> functions exported by the bridge module and not by the bridge-independent
>> part of CONFIG_NET_SWITCHDEV.
>>
>> Problems appear when we have:
>>
>> CONFIG_BRIDGE=m
>> CONFIG_NET_SWITCHDEV=y
>> CONFIG_TI_CPSW_SWITCHDEV=y
>>
>> because cpsw, am65_cpsw and sparx5 will then be built-in but they will
>> call a symbol exported by a loadable module. This is not possible and
>> will result in the following build error:
>>
>> drivers/net/ethernet/ti/cpsw_new.o: in function `cpsw_netdevice_event':
>> drivers/net/ethernet/ti/cpsw_new.c:1520: undefined reference to
>>                                          `switchdev_bridge_port_offload'
>> drivers/net/ethernet/ti/cpsw_new.c:1537: undefined reference to
>>                                          `switchdev_bridge_port_unoffload'
>>
>> As mentioned, the other switchdev drivers don't suffer from this because
>> switchdev_bridge_port_offload() is not the first symbol exported by the
>> bridge that they are calling, so they already needed to deal with this
>> in the same way.
>>
>> Fixes: 2f5dc00f7a3e ("net: bridge: switchdev: let drivers inform which bridge ports are offloaded")
>> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
>> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> I'm still seeing build failures after this patch was applied. I have a fixup
> patch that seems to work, but I'm still not sure if that version is complete.

In my opinion, the problem is a bit bigger here than just fixing the build :(

In case, of ^cpsw the switchdev mode is kinda optional and in many cases
(especially for testing purposes, NFS) the multi-mac mode is still preferable mode.

There were no such tight dependency between switchdev drivers and bridge core before and switchdev serviced as
independent, notification based layer between them, so ^cpsw still can be "Y" and bridge can be "M".
Now for mostly every kernel build configuration the CONFIG_BRIDGE will need to be set as "Y", or we will have
to update drivers to support build with BRIDGE=n and maintain separate builds for networking vs non-networking testing.
But is this enough? Wouldn't it cause 'chain reaction' required to add more and more "Y" options (like CONFIG_VLAN_8021Q)?

PS. Just to be sure we on the same page - ARM builds will be forced (with this patch) to have CONFIG_TI_CPSW_SWITCHDEV=m
and so all our automation testing will just fail with omap2plus_defconfig.
-- 
Best regards,
grygorii

Re: [PATCH net-next] net: build all switchdev drivers as modules when the bridge is a module

[Vladimir Oltean]

On Tue, Aug 03, 2021 at 02:18:38PM +0300, Grygorii Strashko wrote:
> In my opinion, the problem is a bit bigger here than just fixing the build :(
> 
> In case, of ^cpsw the switchdev mode is kinda optional and in many cases
> (especially for testing purposes, NFS) the multi-mac mode is still preferable mode.
> 
> There were no such tight dependency between switchdev drivers and bridge core before and switchdev serviced as
> independent, notification based layer between them, so ^cpsw still can be "Y" and bridge can be "M".
> Now for mostly every kernel build configuration the CONFIG_BRIDGE will need to be set as "Y", or we will have
> to update drivers to support build with BRIDGE=n and maintain separate builds for networking vs non-networking testing.
> But is this enough? Wouldn't it cause 'chain reaction' required to add more and more "Y" options (like CONFIG_VLAN_8021Q)?
> 
> PS. Just to be sure we on the same page - ARM builds will be forced (with this patch) to have CONFIG_TI_CPSW_SWITCHDEV=m
> and so all our automation testing will just fail with omap2plus_defconfig.

I didn't realize it is such a big use case to have the bridge built as
module and cpsw as built-in. I will send a patch that converts the
switchdev_bridge_port_{,un}offload functions to simply emit a blocking
switchdev notifier which the bridge processes (a la SWITCHDEV_FDB_ADD_TO_BRIDGE),
therefore making switchdev and the bridge loosely coupled in order to
keep your setup the way it was before.

Re: [PATCH net-next] net: build all switchdev drivers as modules when the bridge is a module

[Arnd Bergmann]

On Tue, Aug 3, 2021 at 1:58 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Tue, Aug 03, 2021 at 02:18:38PM +0300, Grygorii Strashko wrote:
> > In my opinion, the problem is a bit bigger here than just fixing the build :(
> >
> > In case, of ^cpsw the switchdev mode is kinda optional and in many cases
> > (especially for testing purposes, NFS) the multi-mac mode is still preferable mode.
> >
> > There were no such tight dependency between switchdev drivers and bridge core before and switchdev serviced as
> > independent, notification based layer between them, so ^cpsw still can be "Y" and bridge can be "M".
> > Now for mostly every kernel build configuration the CONFIG_BRIDGE will need to be set as "Y", or we will have
> > to update drivers to support build with BRIDGE=n and maintain separate builds for networking vs non-networking testing.
> > But is this enough? Wouldn't it cause 'chain reaction' required to add more and more "Y" options (like CONFIG_VLAN_8021Q)?
> >
> > PS. Just to be sure we on the same page - ARM builds will be forced (with this patch) to have CONFIG_TI_CPSW_SWITCHDEV=m
> > and so all our automation testing will just fail with omap2plus_defconfig.
>
> I didn't realize it is such a big use case to have the bridge built as
> module and cpsw as built-in.

I don't think anybody realistically cares about doing, I was only interested in
correctly expressing what the dependency is.

> I will send a patch that converts the
> switchdev_bridge_port_{,un}offload functions to simply emit a blocking
> switchdev notifier which the bridge processes (a la SWITCHDEV_FDB_ADD_TO_BRIDGE),
> therefore making switchdev and the bridge loosely coupled in order to
> keep your setup the way it was before.

That does sounds like it can avoid future build regressions, and simplify the
Kconfig dependencies, so that would probably be a good solution.

       arnd

Re: [PATCH net-next] net: build all switchdev drivers as modules when the bridge is a module

[Grygorii Strashko]

On 03/08/2021 15:33, Arnd Bergmann wrote:
> On Tue, Aug 3, 2021 at 1:58 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>>
>> On Tue, Aug 03, 2021 at 02:18:38PM +0300, Grygorii Strashko wrote:
>>> In my opinion, the problem is a bit bigger here than just fixing the build :(
>>>
>>> In case, of ^cpsw the switchdev mode is kinda optional and in many cases
>>> (especially for testing purposes, NFS) the multi-mac mode is still preferable mode.
>>>
>>> There were no such tight dependency between switchdev drivers and bridge core before and switchdev serviced as
>>> independent, notification based layer between them, so ^cpsw still can be "Y" and bridge can be "M".
>>> Now for mostly every kernel build configuration the CONFIG_BRIDGE will need to be set as "Y", or we will have
>>> to update drivers to support build with BRIDGE=n and maintain separate builds for networking vs non-networking testing.
>>> But is this enough? Wouldn't it cause 'chain reaction' required to add more and more "Y" options (like CONFIG_VLAN_8021Q)?
>>>
>>> PS. Just to be sure we on the same page - ARM builds will be forced (with this patch) to have CONFIG_TI_CPSW_SWITCHDEV=m
>>> and so all our automation testing will just fail with omap2plus_defconfig.
>>
>> I didn't realize it is such a big use case to have the bridge built as
>> module and cpsw as built-in.
> 
> I don't think anybody realistically cares about doing, I was only interested in
> correctly expressing what the dependency is.
> 
>> I will send a patch that converts the
>> switchdev_bridge_port_{,un}offload functions to simply emit a blocking
>> switchdev notifier which the bridge processes (a la SWITCHDEV_FDB_ADD_TO_BRIDGE),
>> therefore making switchdev and the bridge loosely coupled in order to
>> keep your setup the way it was before.
> 
> That does sounds like it can avoid future build regressions, and simplify the
> Kconfig dependencies, so that would probably be a good solution.

Yes. it sounds good, thank you.
Just a thought - might be good to follow switchdev_attr approach (extendable), but in the opposite direction as, I feel,
more notification dev->bridge might be added in the future.

-- 
Best regards,
grygorii