netdev - Re: [PATCH net-next 5/7] net: dsa: realtek: merge interface modules into common

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 11 Dec 2023 02:13:56 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>, "andrew@...n.ch" <andrew@...n.ch>, 
	"f.fainelli@...il.com" <f.fainelli@...il.com>, "olteanv@...il.com" <olteanv@...il.com>, 
	"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>, 
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>, 
	"arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next 5/7] net: dsa: realtek: merge interface modules
 into common

> > As both realtek-common and realtek-{smi,mdio} must always be loaded
> > together, we can save some resources merging them into a single module.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> > ---
> >  drivers/net/dsa/realtek/Kconfig          | 4 ++--
> >  drivers/net/dsa/realtek/Makefile         | 8 +++++---
> >  drivers/net/dsa/realtek/realtek-common.c | 1 +
> >  drivers/net/dsa/realtek/realtek-mdio.c   | 4 ----
> >  drivers/net/dsa/realtek/realtek-smi.c    | 4 ----
> >  5 files changed, 8 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
> > index 9d182fde11b4..6989972eebc3 100644
> > --- a/drivers/net/dsa/realtek/Kconfig
> > +++ b/drivers/net/dsa/realtek/Kconfig
> > @@ -16,14 +16,14 @@ menuconfig NET_DSA_REALTEK
> >  if NET_DSA_REALTEK
> >
> >  config NET_DSA_REALTEK_MDIO
> > -     tristate "Realtek MDIO interface support"
> > +     bool "Realtek MDIO interface support"
> >       depends on OF
> >       help
> >         Select to enable support for registering switches configured
> >         through MDIO.
> >
> >  config NET_DSA_REALTEK_SMI
> > -     tristate "Realtek SMI interface support"
> > +     bool "Realtek SMI interface support"
> >       depends on OF
> >       help
> >         Select to enable support for registering switches connected
> > diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
> > index 5e0c1ef200a3..88f6652f9850 100644
> > --- a/drivers/net/dsa/realtek/Makefile
> > +++ b/drivers/net/dsa/realtek/Makefile
> > @@ -1,7 +1,9 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_NET_DSA_REALTEK)                += realtek-common.o
> > -obj-$(CONFIG_NET_DSA_REALTEK_MDIO)   += realtek-mdio.o
> > -obj-$(CONFIG_NET_DSA_REALTEK_SMI)    += realtek-smi.o
> > +obj-$(CONFIG_NET_DSA_REALTEK)                += realtek_common.o
> > +realtek_common-objs-y                        := realtek-common.o
>
> This is weird with the - and _. Also realtek-common is not a very
> descriptive module name. Maybe realtek-dsa?
>
> obj-$(CONFIG_NET_DSA_REALTEK) += realtek-dsa.o
> realtek-dsa-objs-y            += realtek-common.o
> realtek-dsa-objs-$(..._MDIO)  += realtek-mdio.o
> realtek-dsa-objs-$(..._SMI)   += realtek-smi.o

Yes, I'm not proud of it. The realtek_common/realtek-common trick is
just to bypass the fact that I cannot link multiple files into a
module that has the same name as one of these files. But realtek-dsa
is fine and it would avoid conflicts with other realtek stuff in the
kernel that might have a common module. However, I would introduce
that name already in the previous patch.

> Also what happens if I just enable CONFIG_NET_DSA_REALTEK and nothing
> else. Do I get a module that doesn't do anything? Not sure if it's a big
> deal.

The config language might not be good enough to handle that nicely.
There is the "imply" keyword but it does not force anything. I don't
know how to require "at least one of these two interfaces" without
creating a dependency cycle. I would just let it build a useless
common module. Building without variants does make a little bit of
sense if you want to build a new driver out-of-tree.

> > +realtek_common-objs-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o
> > +realtek_common-objs-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
> > +realtek_common-objs                  := $(realtek_common-objs-y)
> >  obj-$(CONFIG_NET_DSA_REALTEK_RTL8366RB) += rtl8366.o
> >  rtl8366-objs                                 := rtl8366-core.o rtl8366rb.o
> >  obj-$(CONFIG_NET_DSA_REALTEK_RTL8365MB) += rtl8365mb.o
> > diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c
> > index 75b6aa071990..73c25d114dd3 100644
> > --- a/drivers/net/dsa/realtek/realtek-common.c
> > +++ b/drivers/net/dsa/realtek/realtek-common.c
> > @@ -132,5 +132,6 @@ void realtek_common_remove(struct realtek_priv *priv)
> >  EXPORT_SYMBOL(realtek_common_remove);
> >
> >  MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@...il.com>");
> > +MODULE_AUTHOR("Linus Walleij <linus.walleij@...aro.org>");
> >  MODULE_DESCRIPTION("Realtek DSA switches common module");
> >  MODULE_LICENSE("GPL");
> > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> > index 4c9a744b72f8..bb5bff719ae9 100644
> > --- a/drivers/net/dsa/realtek/realtek-mdio.c
> > +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> > @@ -168,7 +168,3 @@ void realtek_mdio_shutdown(struct mdio_device *mdiodev)
> >       dev_set_drvdata(&mdiodev->dev, NULL);
> >  }
> >  EXPORT_SYMBOL_GPL(realtek_mdio_shutdown);
> > -
> > -MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@...il.com>");
> > -MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
> > -MODULE_LICENSE("GPL");
> > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> > index 246024eec3bd..1ca2aa784d24 100644
> > --- a/drivers/net/dsa/realtek/realtek-smi.c
> > +++ b/drivers/net/dsa/realtek/realtek-smi.c
> > @@ -443,7 +443,3 @@ void realtek_smi_shutdown(struct platform_device *pdev)
> >       platform_set_drvdata(pdev, NULL);
> >  }
> >  EXPORT_SYMBOL_GPL(realtek_smi_shutdown);
> > -
> > -MODULE_AUTHOR("Linus Walleij <linus.walleij@...aro.org>");
> > -MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface");
> > -MODULE_LICENSE("GPL");
> > --
> > 2.43.0
> >