| Message ID | 20210726142536.1223744-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b0e81817629a496854ff1799f6cbd89597db65fd |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: build all switchdev drivers as modules when the bridge is a module | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: ioana.ciornei@nxp.com; 4 maintainers not CCed: vigneshr@ti.com rdunlap@infradead.org linux-arm-kernel@lists.infradead.org ioana.ciornei@nxp.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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 >
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
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
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.
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.
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
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.
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
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(+)