| 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
| ||
|
Date: Mon, 6 Nov 2023 19:37:00 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Linus Walleij <linus.walleij@...aro.org>, netdev@...r.kernel.org, alsi@...g-olufsen.dk,
andrew@...n.ch, vivien.didelot@...il.com, f.fainelli@...il.com,
davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, robh+dt@...nel.org,
krzk+dt@...nel.org, arinc.unal@...nc9.com
Subject: Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller
> On Thu, Nov 02, 2023 at 03:59:48PM +0100, Linus Walleij wrote:
> > I don't know if this is an answer to your question, but look at what I did in
> >
> > drivers/usb/fotg210/Makefile:
> >
> > # This setup links the different object files into one single
> > # module so we don't have to EXPORT() a lot of internal symbols
> > # or create unnecessary submodules.
> > fotg210-objs-y += fotg210-core.o
> > fotg210-objs-$(CONFIG_USB_FOTG210_HCD) += fotg210-hcd.o
> > fotg210-objs-$(CONFIG_USB_FOTG210_UDC) += fotg210-udc.o
> > fotg210-objs := $(fotg210-objs-y)
> > obj-$(CONFIG_USB_FOTG210) += fotg210.o
> >
> > Everything starting with CONFIG_* is a Kconfig option obviously.
> >
> > The final module is just one file named fotg210.ko no matter whether
> > HCD (host controller), UDC (device controller) or both parts were
> > compiled into it. Often you just need one of them, sometimes you may
> > need both.
> >
> > It's a pretty clean example of how you do this "one module from
> > several optional parts" using Kbuild.
>
> To be clear, something like this is what you mean, right?
>
> diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
> index 060165a85fb7..857a039fb0f1 100644
> --- a/drivers/net/dsa/realtek/Kconfig
> +++ b/drivers/net/dsa/realtek/Kconfig
> @@ -15,39 +15,37 @@ menuconfig NET_DSA_REALTEK
>
> if NET_DSA_REALTEK
>
> +config NET_DSA_REALTEK_INTERFACE
> + tristate
> + help
> + Common interface driver for accessing Realtek switches, either
> + through MDIO or SMI.
> +
> config NET_DSA_REALTEK_MDIO
> - tristate "Realtek MDIO interface driver"
> - depends on OF
> - depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB
> - depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB
> - depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB
> + tristate "Realtek MDIO interface support"
> help
> Select to enable support for registering switches configured
> through MDIO.
>
> config NET_DSA_REALTEK_SMI
> - tristate "Realtek SMI interface driver"
> - depends on OF
> - depends on NET_DSA_REALTEK_RTL8365MB || NET_DSA_REALTEK_RTL8366RB
> - depends on NET_DSA_REALTEK_RTL8365MB || !NET_DSA_REALTEK_RTL8365MB
> - depends on NET_DSA_REALTEK_RTL8366RB || !NET_DSA_REALTEK_RTL8366RB
> + bool "Realtek SMI interface support"
> help
> Select to enable support for registering switches connected
> through SMI.
>
> config NET_DSA_REALTEK_RTL8365MB
> tristate "Realtek RTL8365MB switch subdriver"
> - imply NET_DSA_REALTEK_SMI
> - imply NET_DSA_REALTEK_MDIO
> + select NET_DSA_REALTEK_INTERFACE
> select NET_DSA_TAG_RTL8_4
> + depends on OF
> help
> Select to enable support for Realtek RTL8365MB-VC and RTL8367S.
>
> config NET_DSA_REALTEK_RTL8366RB
> tristate "Realtek RTL8366RB switch subdriver"
> - imply NET_DSA_REALTEK_SMI
> - imply NET_DSA_REALTEK_MDIO
> + select NET_DSA_REALTEK_INTERFACE
> select NET_DSA_TAG_RTL4_A
> + depends on OF
> help
> Select to enable support for Realtek RTL8366RB.
>
> diff --git a/drivers/net/dsa/realtek/Makefile b/drivers/net/dsa/realtek/Makefile
> index 0aab57252a7c..35b7734c0ad0 100644
> --- a/drivers/net/dsa/realtek/Makefile
> +++ b/drivers/net/dsa/realtek/Makefile
> @@ -1,6 +1,15 @@
> # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_NET_DSA_REALTEK_MDIO) += realtek-mdio.o
> -obj-$(CONFIG_NET_DSA_REALTEK_SMI) += realtek-smi.o
> +
> +obj-$(CONFIG_NET_DSA_REALTEK_INTERFACE) := realtek-interface.o
> +
> +realtek-interface-objs := realtek-interface-common.o
> +ifdef CONFIG_NET_DSA_REALTEK_MDIO
> +realtek-interface-objs += realtek-mdio.o
> +endif
> +ifdef CONFIG_NET_DSA_REALTEK_SMI
> +realtek-interface-objs += realtek-smi.o
> +endif
> +
> 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-interface-common.c b/drivers/net/dsa/realtek/realtek-interface-common.c
> new file mode 100644
> index 000000000000..bb7c77cdb9e2
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/realtek-interface-common.c
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/module.h>
> +
> +#include "realtek-mdio.h"
> +#include "realtek-smi.h"
> +
> +static int __init realtek_interface_init(void)
> +{
> + int err;
> +
> + err = realtek_mdio_init();
> + if (err)
> + return err;
> +
> + err = realtek_smi_init();
> + if (err) {
> + realtek_smi_exit();
> + return err;
> + }
> +
> + return 0;
> +}
> +module_init(realtek_interface_init);
> +
> +static void __exit realtek_interface_exit(void)
> +{
> + realtek_smi_exit();
> + realtek_mdio_exit();
> +}
> +module_exit(realtek_interface_exit);
> +
> +MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@...il.com>");
> +MODULE_AUTHOR("Linus Walleij <linus.walleij@...aro.org>");
> +MODULE_DESCRIPTION("Driver for interfacing with Realtek switches via MDIO or SMI");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 292e6d087e8b..6997dec14de2 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -1,5 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0+
> -/* Realtek MDIO interface driver
> +/* Realtek MDIO interface support
> *
> * ASICs we intend to support with this driver:
> *
> @@ -19,12 +19,12 @@
> * Copyright (C) 2009-2010 Gabor Juhos <juhosg@...nwrt.org>
> */
>
> -#include <linux/module.h>
> #include <linux/of.h>
> #include <linux/overflow.h>
> #include <linux/regmap.h>
>
> #include "realtek.h"
> +#include "realtek-mdio.h"
>
> /* Read/write via mdiobus */
> #define REALTEK_MDIO_CTRL0_REG 31
> @@ -283,8 +283,12 @@ static struct mdio_driver realtek_mdio_driver = {
> .shutdown = realtek_mdio_shutdown,
> };
>
> -mdio_module_driver(realtek_mdio_driver);
> +int realtek_mdio_init(void)
> +{
> + return mdio_driver_register(&realtek_mdio_driver);
> +}
>
> -MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@...il.com>");
> -MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via MDIO interface");
> -MODULE_LICENSE("GPL");
> +void realtek_mdio_exit(void)
> +{
> + mdio_driver_unregister(&realtek_mdio_driver);
> +}
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.h b/drivers/net/dsa/realtek/realtek-mdio.h
> new file mode 100644
> index 000000000000..941b4ef9d531
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/realtek-mdio.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef _REALTEK_MDIO_H
> +#define _REALTEK_MDIO_H
> +
> +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_MDIO)
> +
> +int realtek_mdio_init(void);
> +void realtek_mdio_exit(void);
> +
> +#else
> +
> +static inline int realtek_mdio_init(void)
> +{
> + return 0;
> +}
> +
> +static inline void realtek_mdio_exit(void)
> +{
> +}
> +
> +#endif
> +
> +#endif
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index 755546ed8db6..4c282bfc884d 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -1,5 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0+
> -/* Realtek Simple Management Interface (SMI) driver
> +/* Realtek Simple Management Interface (SMI) interface
> * It can be discussed how "simple" this interface is.
> *
> * The SMI protocol piggy-backs the MDIO MDC and MDIO signals levels
> @@ -26,7 +26,6 @@
> */
>
> #include <linux/kernel.h>
> -#include <linux/module.h>
> #include <linux/device.h>
> #include <linux/spinlock.h>
> #include <linux/skbuff.h>
> @@ -40,6 +39,7 @@
> #include <linux/if_bridge.h>
>
> #include "realtek.h"
> +#include "realtek-smi.h"
>
> #define REALTEK_SMI_ACK_RETRY_COUNT 5
>
> @@ -560,8 +560,13 @@ static struct platform_driver realtek_smi_driver = {
> .remove_new = realtek_smi_remove,
> .shutdown = realtek_smi_shutdown,
> };
> -module_platform_driver(realtek_smi_driver);
>
> -MODULE_AUTHOR("Linus Walleij <linus.walleij@...aro.org>");
> -MODULE_DESCRIPTION("Driver for Realtek ethernet switch connected via SMI interface");
> -MODULE_LICENSE("GPL");
> +int realtek_smi_init(void)
> +{
> + return platform_driver_register(&realtek_smi_driver);
> +}
> +
> +void realtek_smi_exit(void)
> +{
> + platform_driver_unregister(&realtek_smi_driver);
> +}
> diff --git a/drivers/net/dsa/realtek/realtek-smi.h b/drivers/net/dsa/realtek/realtek-smi.h
> new file mode 100644
> index 000000000000..9a4838321f94
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/realtek-smi.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef _REALTEK_SMI_H
> +#define _REALTEK_SMI_H
> +
> +#if IS_ENABLED(CONFIG_NET_DSA_REALTEK_SMI)
> +
> +int realtek_smi_init(void);
> +void realtek_smi_exit(void);
> +
> +#else
> +
> +static inline int realtek_smi_init(void)
> +{
> + return 0;
> +}
> +
> +static inline void realtek_smi_exit(void)
> +{
> +}
> +
> +#endif
> +
> +#endif
> --
> 2.34.1
>
>
> It looks pretty reasonable to me. More stuff could go into
> realtek-interface-common.c, that could be called directly from
> realtek-smi.c and realtek-mdio.c without exporting anything.
>
> I've eliminated the possibility for the SMI and MDIO options to be
> anything other than y or n, because only a single interface module
> (the common one) exists, and the y/n/m quality of that is
> implied/selected by the drivers which depend on it. I hope I wasn't too
> trigger-happy with this.
Vladimir, you did all the work ;-) Thanks!
I implemented this code (with the fixes), and this is the result:
30272 drivers/net/dsa/realtek/realtek-mdio.ko
42176 drivers/net/dsa/realtek/realtek-smi.ko
114544 drivers/net/dsa/realtek/rtl8365mb.ko
115080 drivers/net/dsa/realtek/rtl8366.ko
87264 drivers/net/dsa/realtek/realtek-interface.ko
114544 drivers/net/dsa/realtek/rtl8365mb.ko
115080 drivers/net/dsa/realtek/rtl8366.ko
It is still strange why merging both modules into a single
realtek-interface.ko results in a slightly larger file. Anyway, the
difference is not that significant. I still think that some systems
will miss that extra 30kb or 40kb for the interface code they don't
need.
Your proposed Kconfig does not attempt to avoid a realtek-interface
without both interfaces or without support for both switch families.
Is it possible in Kconfig to force it to, at least, select one of the
interfaces and one of the switches? Is it okay to leave it
unconstrained?
If merging the modules is the accepted solution, it makes me wonder if
rtl8365mb.ko and rtl8366.ko should get merged as well into a single
realtek-switch.ko. They are a hard dependency for realtek-interface.ko
(previously on each interface module). If the kernel is custom-built,
it would still be possible to exclude one switch family at build time.
I'll use these modules in OpenWrt, which builds a single kernel for a
bunch of devices. Is there a way to weakly depend on a module,
allowing the system to load only a single subdriver? Is it worth it?
Regards,
Luiz
Powered by blists - more mailing lists