[14/18] dsa: ocelot: fix mixed module-builtin object

  With CONFIG_NET_DSA_MSCC_FELIX=m and CONFIG_NET_DSA_MSCC_SEVILLE=y
(or vice versa), felix.o are linked to a module and also to vmlinux
even though the expected CFLAGS are different between builtins and
modules.
This is the same situation as fixed by
commit 637a642f5ca5 ("zstd: Fixing mixed module-builtin objects").
There's also no need to duplicate relatively big piece of object
code into two modules.

Introduce the new module, mscc_core, to provide the common functions
to both mscc_felix and mscc_seville.

Fixes: d60bc62de4ae ("net: dsa: seville: build as separate module")
Suggested-by: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Alexander Lobakin <alobakin@pm.me>
---
 drivers/net/dsa/ocelot/Kconfig           | 18 ++++++++++--------
 drivers/net/dsa/ocelot/Makefile          | 12 +++++-------
 drivers/net/dsa/ocelot/felix.c           |  6 ++++++
 drivers/net/dsa/ocelot/felix_vsc9959.c   |  2 ++
 drivers/net/dsa/ocelot/seville_vsc9953.c |  2 ++
 5 files changed, 25 insertions(+), 15 deletions(-)

--
2.38.1
  

On Sat, Nov 19, 2022 at 11:09:28PM +0000, Alexander Lobakin wrote:
> With CONFIG_NET_DSA_MSCC_FELIX=m and CONFIG_NET_DSA_MSCC_SEVILLE=y
> (or vice versa), felix.o are linked to a module and also to vmlinux
> even though the expected CFLAGS are different between builtins and
> modules.
> This is the same situation as fixed by
> commit 637a642f5ca5 ("zstd: Fixing mixed module-builtin objects").
> There's also no need to duplicate relatively big piece of object
> code into two modules.
> 
> Introduce the new module, mscc_core, to provide the common functions
> to both mscc_felix and mscc_seville.
> 
> Fixes: d60bc62de4ae ("net: dsa: seville: build as separate module")
> Suggested-by: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> ---

I don't disagree with the patch, but I dislike the name chosen.
How about NET_DSA_OCELOT_LIB and mscc_ocelot_dsa_lib.o? The "core" of
the hardware support is arguably mscc_ocelot_switch_lib.o, I don't think
it would be good to use that word here, since the code you're moving is
no more than a thin glue layer with some DSA specific code.

Adding Colin for a second opinion on the naming. I'm sure things could
have been done better in the first place, just not sure how.

Also, could you please make the commit prefix "net: dsa: ocelot" when
you resend this and the other networking patches to the "net" tree?

>  drivers/net/dsa/ocelot/Kconfig           | 18 ++++++++++--------
>  drivers/net/dsa/ocelot/Makefile          | 12 +++++-------
>  drivers/net/dsa/ocelot/felix.c           |  6 ++++++
>  drivers/net/dsa/ocelot/felix_vsc9959.c   |  2 ++
>  drivers/net/dsa/ocelot/seville_vsc9953.c |  2 ++
>  5 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
> index 08db9cf76818..59845274e374 100644
> --- a/drivers/net/dsa/ocelot/Kconfig
> +++ b/drivers/net/dsa/ocelot/Kconfig
> @@ -1,4 +1,12 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +
> +config NET_DSA_MSCC_CORE
> +	tristate
> +	select MSCC_OCELOT_SWITCH_LIB
> +	select NET_DSA_TAG_OCELOT_8021Q
> +	select NET_DSA_TAG_OCELOT
> +	select PCS_LYNX

Please keep PCS_LYNX selected by MSCC_FELIX and MSCC_SEVILLE respectively.

> +
>  config NET_DSA_MSCC_FELIX
>  	tristate "Ocelot / Felix Ethernet switch support"
>  	depends on NET_DSA && PCI
> @@ -7,11 +15,8 @@ config NET_DSA_MSCC_FELIX
>  	depends on HAS_IOMEM
>  	depends on PTP_1588_CLOCK_OPTIONAL
>  	depends on NET_SCH_TAPRIO || NET_SCH_TAPRIO=n
> -	select MSCC_OCELOT_SWITCH_LIB
> -	select NET_DSA_TAG_OCELOT_8021Q
> -	select NET_DSA_TAG_OCELOT
> +	select NET_DSA_MSCC_CORE
>  	select FSL_ENETC_MDIO
> -	select PCS_LYNX
>  	help
>  	  This driver supports the VSC9959 (Felix) switch, which is embedded as
>  	  a PCIe function of the NXP LS1028A ENETC RCiEP.
> @@ -22,11 +27,8 @@ config NET_DSA_MSCC_SEVILLE
>  	depends on NET_VENDOR_MICROSEMI
>  	depends on HAS_IOMEM
>  	depends on PTP_1588_CLOCK_OPTIONAL
> +	select NET_DSA_MSCC_CORE
>  	select MDIO_MSCC_MIIM
> -	select MSCC_OCELOT_SWITCH_LIB
> -	select NET_DSA_TAG_OCELOT_8021Q
> -	select NET_DSA_TAG_OCELOT
> -	select PCS_LYNX
>  	help
>  	  This driver supports the VSC9953 (Seville) switch, which is embedded
>  	  as a platform device on the NXP T1040 SoC.
> diff --git a/drivers/net/dsa/ocelot/Makefile b/drivers/net/dsa/ocelot/Makefile
> index f6dd131e7491..f8c74b59b996 100644
> --- a/drivers/net/dsa/ocelot/Makefile
> +++ b/drivers/net/dsa/ocelot/Makefile
> @@ -1,11 +1,9 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_NET_DSA_MSCC_CORE) += mscc_core.o
>  obj-$(CONFIG_NET_DSA_MSCC_FELIX) += mscc_felix.o
>  obj-$(CONFIG_NET_DSA_MSCC_SEVILLE) += mscc_seville.o
> 
> -mscc_felix-objs := \
> -	felix.o \
> -	felix_vsc9959.o
> -
> -mscc_seville-objs := \
> -	felix.o \
> -	seville_vsc9953.o
> +mscc_core-objs := felix.o
> +mscc_felix-objs := felix_vsc9959.o
> +mscc_seville-objs := seville_vsc9953.o
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index dd3a18cc89dd..f9d0a24ebc3a 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -2112,6 +2112,7 @@ const struct dsa_switch_ops felix_switch_ops = {
>  	.port_set_host_flood		= felix_port_set_host_flood,
>  	.port_change_master		= felix_port_change_master,
>  };
> +EXPORT_SYMBOL_NS_GPL(felix_switch_ops, NET_DSA_MSCC_CORE);

What do we gain practically with the symbol namespacing?

> 
>  struct net_device *felix_port_to_netdev(struct ocelot *ocelot, int port)
>  {
> @@ -2123,6 +2124,7 @@ struct net_device *felix_port_to_netdev(struct ocelot *ocelot, int port)
> 
>  	return dsa_to_port(ds, port)->slave;
>  }
> +EXPORT_SYMBOL_NS_GPL(felix_port_to_netdev, NET_DSA_MSCC_CORE);
> 
>  int felix_netdev_to_port(struct net_device *dev)
>  {
> @@ -2134,3 +2136,7 @@ int felix_netdev_to_port(struct net_device *dev)
> 
>  	return dp->index;
>  }
> +EXPORT_SYMBOL_NS_GPL(felix_netdev_to_port, NET_DSA_MSCC_CORE);
> +
> +MODULE_DESCRIPTION("MSCC Switch driver core");

Ocelot switch DSA library

> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index 26a35ae322d1..52c8bff79fa3 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -2736,5 +2736,7 @@ static struct pci_driver felix_vsc9959_pci_driver = {
>  };
>  module_pci_driver(felix_vsc9959_pci_driver);
> 
> +MODULE_IMPORT_NS(NET_DSA_MSCC_CORE);
> +
>  MODULE_DESCRIPTION("Felix Switch driver");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
> index 7af33b2c685d..e9c63c642489 100644
> --- a/drivers/net/dsa/ocelot/seville_vsc9953.c
> +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
> @@ -1115,5 +1115,7 @@ static struct platform_driver seville_vsc9953_driver = {
>  };
>  module_platform_driver(seville_vsc9953_driver);
> 
> +MODULE_IMPORT_NS(NET_DSA_MSCC_CORE);
> +
>  MODULE_DESCRIPTION("Seville Switch driver");
>  MODULE_LICENSE("GPL v2");
> --
> 2.38.1
> 
>
  

On Mon, Nov 21, 2022 at 07:55:04PM +0200, Vladimir Oltean wrote:
> On Sat, Nov 19, 2022 at 11:09:28PM +0000, Alexander Lobakin wrote:
> > With CONFIG_NET_DSA_MSCC_FELIX=m and CONFIG_NET_DSA_MSCC_SEVILLE=y
> > (or vice versa), felix.o are linked to a module and also to vmlinux
> > even though the expected CFLAGS are different between builtins and
> > modules.
> > This is the same situation as fixed by
> > commit 637a642f5ca5 ("zstd: Fixing mixed module-builtin objects").
> > There's also no need to duplicate relatively big piece of object
> > code into two modules.
> > 
> > Introduce the new module, mscc_core, to provide the common functions
> > to both mscc_felix and mscc_seville.
> > 
> > Fixes: d60bc62de4ae ("net: dsa: seville: build as separate module")
> > Suggested-by: Masahiro Yamada <masahiroy@kernel.org>
> > Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> > ---
> 
> I don't disagree with the patch, but I dislike the name chosen.
> How about NET_DSA_OCELOT_LIB and mscc_ocelot_dsa_lib.o? The "core" of
> the hardware support is arguably mscc_ocelot_switch_lib.o, I don't think
> it would be good to use that word here, since the code you're moving is
> no more than a thin glue layer with some DSA specific code.
> 
> Adding Colin for a second opinion on the naming. I'm sure things could
> have been done better in the first place, just not sure how.

Good catch on this patch. "mscc_ocelot_dsa_lib" makes sense. The only
other option that might be considered would be along the lines of
"felix_lib". While I know "Felix" is the chip, in the dsa directory it
seems to represent the DSA lib in general.

Either one seems fine for me. And thanks for the heads up, as I'll need
to make the same changes for ocelot_ext when it is ready.
  

On Mon, Nov 21, 2022 at 07:55:04PM +0200, Vladimir Oltean wrote:
> On Sat, Nov 19, 2022 at 11:09:28PM +0000, Alexander Lobakin wrote:

...

> > +EXPORT_SYMBOL_NS_GPL(felix_switch_ops, NET_DSA_MSCC_CORE);
> 
> What do we gain practically with the symbol namespacing?

I guess this wrap-up can possibly answer this:
https://lwn.net/Articles/760045/
  

On Mon, Nov 21, 2022 at 10:12:59AM -0800, Colin Foster wrote:
> Good catch on this patch. "mscc_ocelot_dsa_lib" makes sense. The only
> other option that might be considered would be along the lines of
> "felix_lib". While I know "Felix" is the chip, in the dsa directory it
> seems to represent the DSA lib in general.

"mscc_felix_lib" could work too. At least it's more consistent with
felix.c and the function names.
  

From: Alexander Lobakin <alobakin@maibox.org>

From: Colin Foster <colin.foster@in-advantage.com>
Date: Mon, 21 Nov 2022 10:12:59 -0800

> On Mon, Nov 21, 2022 at 07:55:04PM +0200, Vladimir Oltean wrote:
> > On Sat, Nov 19, 2022 at 11:09:28PM +0000, Alexander Lobakin wrote:
> > > With CONFIG_NET_DSA_MSCC_FELIX=m and CONFIG_NET_DSA_MSCC_SEVILLE=y
> > > (or vice versa), felix.o are linked to a module and also to vmlinux
> > > even though the expected CFLAGS are different between builtins and
> > > modules.
> > > This is the same situation as fixed by
> > > commit 637a642f5ca5 ("zstd: Fixing mixed module-builtin objects").
> > > There's also no need to duplicate relatively big piece of object
> > > code into two modules.
> > > 
> > > Introduce the new module, mscc_core, to provide the common functions
> > > to both mscc_felix and mscc_seville.
> > > 
> > > Fixes: d60bc62de4ae ("net: dsa: seville: build as separate module")
> > > Suggested-by: Masahiro Yamada <masahiroy@kernel.org>
> > > Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> > > ---
> > 
> > I don't disagree with the patch, but I dislike the name chosen.
> > How about NET_DSA_OCELOT_LIB and mscc_ocelot_dsa_lib.o? The "core" of
> > the hardware support is arguably mscc_ocelot_switch_lib.o, I don't think
> > it would be good to use that word here, since the code you're moving is
> > no more than a thin glue layer with some DSA specific code.

Yes, sure. I'm totally open for renaming stuff -- usually I barely
touch most of the code from the series, so the names can make no
sense at all. _dsa_lib sounds good, I like it.

> > 
> > Adding Colin for a second opinion on the naming. I'm sure things could
> > have been done better in the first place, just not sure how.
> 
> Good catch on this patch. "mscc_ocelot_dsa_lib" makes sense. The only
> other option that might be considered would be along the lines of
> "felix_lib". While I know "Felix" is the chip, in the dsa directory it
> seems to represent the DSA lib in general.

The thing confused me is that one of the platforms is named Felix
and the other is Seville, but the file they share is also felix,
although Seville has no references to the name "felix" AFAICS :D
I thought maybe Felix is a family and Seville is a chip from this
family, dunno.

> 
> Either one seems fine for me. And thanks for the heads up, as I'll need
> to make the same changes for ocelot_ext when it is ready.
> 

Ooh, something interesting is coming, nice <.<

Thanks,
Olek
  

From: Colin Foster <colin.foster@in-advantage.com>,
Date: Mon, 21 Nov 2022 10:12:59 -0800

> On Mon, Nov 21, 2022 at 07:55:04PM +0200, Vladimir Oltean wrote:
> > On Sat, Nov 19, 2022 at 11:09:28PM +0000, Alexander Lobakin wrote:
> > > With CONFIG_NET_DSA_MSCC_FELIX=m and CONFIG_NET_DSA_MSCC_SEVILLE=y
> > > (or vice versa), felix.o are linked to a module and also to vmlinux
> > > even though the expected CFLAGS are different between builtins and
> > > modules.
> > > This is the same situation as fixed by
> > > commit 637a642f5ca5 ("zstd: Fixing mixed module-builtin objects").
> > > There's also no need to duplicate relatively big piece of object
> > > code into two modules.
> > > 
> > > Introduce the new module, mscc_core, to provide the common functions
> > > to both mscc_felix and mscc_seville.
> > > 
> > > Fixes: d60bc62de4ae ("net: dsa: seville: build as separate module")
> > > Suggested-by: Masahiro Yamada <masahiroy@kernel.org>
> > > Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> > > ---
> > 
> > I don't disagree with the patch, but I dislike the name chosen.
> > How about NET_DSA_OCELOT_LIB and mscc_ocelot_dsa_lib.o? The "core" of
> > the hardware support is arguably mscc_ocelot_switch_lib.o, I don't think
> > it would be good to use that word here, since the code you're moving is
> > no more than a thin glue layer with some DSA specific code.

Sure. I usually barely work with the code touched by the series, so
often the names can make no sense -- I'm open for better namings
from the real developers.
_dsa_lib sounds good, I like it.

> > 
> > Adding Colin for a second opinion on the naming. I'm sure things could
> > have been done better in the first place, just not sure how.
> 
> Good catch on this patch. "mscc_ocelot_dsa_lib" makes sense. The only
> other option that might be considered would be along the lines of
> "felix_lib". While I know "Felix" is the chip, in the dsa directory it
> seems to represent the DSA lib in general.

The thing confused me is that one chip is named Felix and the other
one is Seville, but the shared code is named felix as well. So at
first I thought maybe Felix is a family of chips and Seville is a
chip from that family, dunno :D

> 
> Either one seems fine for me. And thanks for the heads up, as I'll need
> to make the same changes for ocelot_ext when it is ready.

Something interesting is coming, nice <.<

(re "pls prefix with "net: dsa: ..."" -- roger that)

Thanks,
Olek
  

On Wed, Nov 23, 2022 at 10:47:46PM +0100, Alexander Lobakin wrote:
> From: Colin Foster <colin.foster@in-advantage.com>,
> Date: Mon, 21 Nov 2022 10:12:59 -0800
> > On Mon, Nov 21, 2022 at 07:55:04PM +0200, Vladimir Oltean wrote:
> > > On Sat, Nov 19, 2022 at 11:09:28PM +0000, Alexander Lobakin wrote:
> > > 
> > > Adding Colin for a second opinion on the naming. I'm sure things could
> > > have been done better in the first place, just not sure how.
> > 
> > Good catch on this patch. "mscc_ocelot_dsa_lib" makes sense. The only
> > other option that might be considered would be along the lines of
> > "felix_lib". While I know "Felix" is the chip, in the dsa directory it
> > seems to represent the DSA lib in general.
> 
> The thing confused me is that one chip is named Felix and the other
> one is Seville, but the shared code is named felix as well. So at
> first I thought maybe Felix is a family of chips and Seville is a
> chip from that family, dunno :D
> 

Not important, but in case anyone is curious:

Ocelot is a family of switches. Linux support exists for the internal
MIPS on some of those devices. My understanding is the switching
hardware is licensed out to other chips that can be controlled
externally (e.g. PCIe). Felix was the first chip to do so with full
Linux support. When Seville came along, it utilized a lot of common
code from Felix. Thus, Felix is a "chip" as well as a "library" -
specifically the DSA implementation of Ocelot. At least in my mind.

(Note: I haven't verified this timeline back to the early days of
Felix... I'm mostly speculating)

> > 
> > Either one seems fine for me. And thanks for the heads up, as I'll need
> > to make the same changes for ocelot_ext when it is ready.
> 
> Something interesting is coming, nice <.<

Interesting to a very select group of people :-) The Ocelot chips can be
controlled externally. 6.1 has basic support for these chips -
essentially an expensive GPIO expander. Adding support for half of the
ports is phase 2, and I need to sit down for another day or two to finish
things up before that can happen. Hopefully very soon, as my calendar is
finally freeing up... Still going for 6.2!

> 
> (re "pls prefix with "net: dsa: ..."" -- roger that)
> 
> Thanks,
> Olek
  

On Wed, Nov 23, 2022 at 02:18:02PM -0800, Colin Foster wrote:
> > The thing confused me is that one chip is named Felix and the other
> > one is Seville, but the shared code is named felix as well. So at
> > first I thought maybe Felix is a family of chips and Seville is a
> > chip from that family, dunno :D
> 
> Not important, but in case anyone is curious:
> 
> Ocelot is a family of switches. Linux support exists for the internal
> MIPS on some of those devices. My understanding is the switching
> hardware is licensed out to other chips that can be controlled
> externally (e.g. PCIe). Felix was the first chip to do so with full
> Linux support. When Seville came along, it utilized a lot of common
> code from Felix. Thus, Felix is a "chip" as well as a "library" -
> specifically the DSA implementation of Ocelot. At least in my mind.
> 
> (Note: I haven't verified this timeline back to the early days of
> Felix... I'm mostly speculating)

I'm not sure marketing would agree that Ocelot, Felix, Seville are part
of the same "family". They're all Vitesse switch designs which share the
same core architecture, even if some are sold by other companies.

The Ocelot switchdev driver came first to Linux. The Felix switch was
very similar, except it was DSA and not switchdev. So when it got added,
Ocelot became the name of a library for sharing code between a switchdev
front-end and a DSA front-end, as well as the name of a driver proper.

The Seville hardware is actually much older than both Ocelot and Felix.
It comes from the same family as Serval. It's integrated into old
Freescale PowerPC SoCs. It only got Linux support late in its life,
when it became super easy to do it, basically after Felix paved the way.
When that happened, Felix also got split up into a library (for the DSA
aspects of interfacing with the ocelot library) and a driver proper.

Colin is now working on a switch which marketing really would say that
it's part of the Ocelot family. Except it's DSA, so it has to use the
Felix library.

Anyway, TL;DR: name of common code is given by the first supported
hardware, it's quite a common pattern really.

What's more interesting to me is the strange humour of somebody at
Vitesse (now Microchip) who gave the feline code names for these
switches (Ocelot, Serval, Jaguar). Felix is none other than Felix the Cat.