Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH net-next] dsa: sja1105: fix reverse dependency
@ 2021-08-05 11:00 Arnd Bergmann
  2021-08-05 11:25 ` Vladimir Oltean
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2021-08-05 11:00 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
	David S. Miller, Jakub Kicinski
  Cc: Arnd Bergmann, Oleksij Rempel, Yangbo Lu, netdev, linux-kernel

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")
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
 	select PACKING
 	help
 	  Say Y or M if you want to enable support for tagging frames with the
-- 
2.29.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] dsa: sja1105: fix reverse dependency
  2021-08-05 11:00 [PATCH net-next] dsa: sja1105: fix reverse dependency Arnd Bergmann
@ 2021-08-05 11:25 ` Vladimir Oltean
  2021-08-05 11:39   ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Vladimir Oltean @ 2021-08-05 11:25 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
	Jakub Kicinski, Arnd Bergmann, Oleksij Rempel, Yangbo Lu, netdev,
	linux-kernel

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
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] dsa: sja1105: fix reverse dependency
  2021-08-05 11:25 ` Vladimir Oltean
@ 2021-08-05 11:39   ` Arnd Bergmann
  2021-08-05 11:49     ` Vladimir Oltean
  2021-08-05 12:17     ` Vladimir Oltean
  0 siblings, 2 replies; 7+ messages in thread
From: Arnd Bergmann @ 2021-08-05 11:39 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
	Jakub Kicinski, Arnd Bergmann, Oleksij Rempel, Yangbo Lu,
	Networking, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] dsa: sja1105: fix reverse dependency
  2021-08-05 11:39   ` Arnd Bergmann
@ 2021-08-05 11:49     ` Vladimir Oltean
  2021-08-05 12:05       ` Arnd Bergmann
  2021-08-05 12:17     ` Vladimir Oltean
  1 sibling, 1 reply; 7+ messages in thread
From: Vladimir Oltean @ 2021-08-05 11:49 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
	Jakub Kicinski, Arnd Bergmann, Oleksij Rempel, Yangbo Lu,
	Networking, Linux Kernel Mailing List

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] dsa: sja1105: fix reverse dependency
  2021-08-05 11:49     ` Vladimir Oltean
@ 2021-08-05 12:05       ` Arnd Bergmann
  0 siblings, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2021-08-05 12:05 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
	Jakub Kicinski, Arnd Bergmann, Oleksij Rempel, Yangbo Lu,
	Networking, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] dsa: sja1105: fix reverse dependency
  2021-08-05 11:39   ` Arnd Bergmann
  2021-08-05 11:49     ` Vladimir Oltean
@ 2021-08-05 12:17     ` Vladimir Oltean
  2021-08-05 12:22       ` Arnd Bergmann
  1 sibling, 1 reply; 7+ messages in thread
From: Vladimir Oltean @ 2021-08-05 12:17 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
	Jakub Kicinski, Arnd Bergmann, Oleksij Rempel, Yangbo Lu,
	Networking, Linux Kernel Mailing List

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/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] dsa: sja1105: fix reverse dependency
  2021-08-05 12:17     ` Vladimir Oltean
@ 2021-08-05 12:22       ` Arnd Bergmann
  0 siblings, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2021-08-05 12:22 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
	Jakub Kicinski, Arnd Bergmann, Oleksij Rempel, Yangbo Lu,
	Networking, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-08-05 12:22 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-05 11:00 [PATCH net-next] dsa: sja1105: fix reverse dependency Arnd Bergmann
2021-08-05 11:25 ` Vladimir Oltean
2021-08-05 11:39   ` Arnd Bergmann
2021-08-05 11:49     ` Vladimir Oltean
2021-08-05 12:05       ` Arnd Bergmann
2021-08-05 12:17     ` Vladimir Oltean
2021-08-05 12:22       ` Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).