| Message ID | 20210805110048.1696362-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] dsa: sja1105: fix reverse dependency | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net-next |
| netdev/tree_selection | success | Clearly marked for net-next |
Hi Arnd, On Thu, Aug 05, 2021 at 01:00:28PM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The DSA driver and the tag driver for sja1105 are closely linked, > and recently the dependency started becoming visible in the form > of the sja1110_process_meta_tstamp() that gets exported by one > and used by the other. > > This causes a rare build failure with CONFIG_NET_DSA_TAG_SJA1105=y > and CONFIG_NET_DSA_SJA1105=m, as the 'select' statement only > prevents the opposite configuration: > > aarch64-linux-ld: net/dsa/tag_sja1105.o: in function `sja1110_rcv': > tag_sja1105.c:(.text.sja1110_rcv+0x164): undefined reference to `sja1110_process_meta_tstamp' > > Add a stricter dependency for the CONFIG_NET_DSA_TAG_SJA110y to > prevent it from being built-in when the other one is not. > > Fixes: 566b18c8b752 ("net: dsa: sja1105: implement TX timestamping for SJA1110") > Fixes: 227d07a07ef1 ("net: dsa: sja1105: Add support for traffic through standalone ports") The second Fixes: tag makes no sense. > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Not sure if there is a more logical way to deal with this, > but the added dependency does help avoid the build failure. > > I found this one while verifying the PTP dependency patch, but > it's really a separate issue. > --- > net/dsa/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig > index bca1b5d66df2..548285539752 100644 > --- a/net/dsa/Kconfig > +++ b/net/dsa/Kconfig > @@ -138,6 +138,7 @@ config NET_DSA_TAG_LAN9303 > > config NET_DSA_TAG_SJA1105 > tristate "Tag driver for NXP SJA1105 switches" > + depends on NET_DSA_SJA1105 || !NET_DSA_SJA1105 I think I would prefer an optional "build as module if NET_DSA_SJA1105 is a module" dependency only if NET_DSA_SJA1105_PTP is enabled. I think this is how that is expressed: depends on (NET_DSA_SJA1105 && NET_DSA_SJA1105_PTP) || !NET_DSA_SJA1105 || !NET_DSA_SJA1105_PTP > select PACKING > help > Say Y or M if you want to enable support for tagging frames with the > -- > 2.29.2 >
On Thu, Aug 5, 2021 at 1:25 PM Vladimir Oltean <olteanv@gmail.com> wrote: > On Thu, Aug 05, 2021 at 01:00:28PM +0200, Arnd Bergmann wrote: > > > > Fixes: 566b18c8b752 ("net: dsa: sja1105: implement TX timestamping for SJA1110") > > Fixes: 227d07a07ef1 ("net: dsa: sja1105: Add support for traffic through standalone ports") > > The second Fixes: tag makes no sense. Fair enough. I added this because that was when the original 'select' got added, but of course it was not wrong at the time. > > diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig > > index bca1b5d66df2..548285539752 100644 > > --- a/net/dsa/Kconfig > > +++ b/net/dsa/Kconfig > > @@ -138,6 +138,7 @@ config NET_DSA_TAG_LAN9303 > > > > config NET_DSA_TAG_SJA1105 > > tristate "Tag driver for NXP SJA1105 switches" > > + depends on NET_DSA_SJA1105 || !NET_DSA_SJA1105 > > I think I would prefer an optional "build as module if NET_DSA_SJA1105 is a module" > dependency only if NET_DSA_SJA1105_PTP is enabled. I think this is how that is > expressed: > > depends on (NET_DSA_SJA1105 && NET_DSA_SJA1105_PTP) || !NET_DSA_SJA1105 || !NET_DSA_SJA1105_PTP Ah, I had not realized this dependency is only there when NET_DSA_SJA1105_PTP is also enabled. I will give this a little more testing and resend later with that change. Do you have any opinion on whether that 'select' going the other way is still relevant? Arnd
On Thu, Aug 05, 2021 at 01:39:34PM +0200, Arnd Bergmann wrote: > Do you have any opinion on whether that 'select' going the other way is still > relevant? Yes, of course it is. It also has nothing to do with build dependencies. With the original DSA design from 2008, an Ethernet switch has separate drivers for (a) accessing its registers (b) manipulating the packets that the switch sends towards a host Ethernet controller ("DSA master") The register access drivers are in drivers/net/dsa/*, the packet manipulation ("tagging protocol") drivers are in net/dsa/tag_*.c. [ This is because it was originally thought that a "tagging protocol" is completely stateless and you should never need to access a hardware register when manipulating a packet. ] When you enable a driver for a switch, you absolutely want to ping through it too, so all register access drivers enable the tagging protocol driver specific to their hardware as well, using 'select'. This works just fine because tagging protocol drivers generally have no dependencies, or if they do, the register access driver inherits them too. So a user does not need to manually enable the tagging protocol driver.
On Thu, Aug 5, 2021 at 1:49 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Thu, Aug 05, 2021 at 01:39:34PM +0200, Arnd Bergmann wrote: > > Do you have any opinion on whether that 'select' going the other way is still > > relevant? > > Yes, of course it is. It also has nothing to do with build dependencies. > With the original DSA design from 2008, an Ethernet switch has separate > drivers for > (a) accessing its registers > (b) manipulating the packets that the switch sends towards a host > Ethernet controller ("DSA master") > > The register access drivers are in drivers/net/dsa/*, the packet > manipulation ("tagging protocol") drivers are in net/dsa/tag_*.c. > > [ This is because it was originally thought that a "tagging protocol" is > completely stateless and you should never need to access a hardware > register when manipulating a packet. ] > > When you enable a driver for a switch, you absolutely want to ping > through it too, so all register access drivers enable the tagging > protocol driver specific to their hardware as well, using 'select'. > This works just fine because tagging protocol drivers generally have no > dependencies, or if they do, the register access driver inherits them too. > So a user does not need to manually enable the tagging protocol driver. Got it, thanks for the explanation. Arnd
On Thu, Aug 05, 2021 at 01:39:34PM +0200, Arnd Bergmann wrote: > I will give this a little more testing and resend > later with that change. Btw, not sure if you noticed but I did send that out already: https://patchwork.kernel.org/project/netdevbpf/patch/20210805113612.2174148-1-vladimir.oltean@nxp.com/
On Thu, Aug 5, 2021 at 2:17 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Thu, Aug 05, 2021 at 01:39:34PM +0200, Arnd Bergmann wrote: > > I will give this a little more testing and resend > > later with that change. > > Btw, not sure if you noticed but I did send that out already: > https://patchwork.kernel.org/project/netdevbpf/patch/20210805113612.2174148-1-vladimir.oltean@nxp.com/ Yes, saw it, but only after my reply. I don't expect any problems with it, but it's in my test tree now and I'll let you know if something comes up. Arnd
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig index bca1b5d66df2..548285539752 100644 --- a/net/dsa/Kconfig +++ b/net/dsa/Kconfig @@ -138,6 +138,7 @@ config NET_DSA_TAG_LAN9303 config NET_DSA_TAG_SJA1105 tristate "Tag driver for NXP SJA1105 switches" + depends on NET_DSA_SJA1105 || !NET_DSA_SJA1105 select PACKING help Say Y or M if you want to enable support for tagging frames with the