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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <nouxv5jcrwuy3dyxgzjjs2fsnyhongh6rs5asrvnl2xkmf4vbb@tocsp6uf2wl5>
Date: Mon, 11 Dec 2023 09:25:37 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
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

On Mon, Dec 11, 2023 at 02:13:56AM -0300, Luiz Angelo Daros de Luca wrote:
> > > 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.

OK

> 
> > 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.

Agree, I don't know a good way to prevent it. I don't think it's a big
deal.

> 
> > > +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
> > >