2022-10-20 09:59:33

by Alexandru Tachici

Subject: [net v2 1/1] net: ethernet: adi: adin1110: Fix notifiers

ADIN1110 was registering netdev_notifiers on each device probe.
This leads to warnings/probe failures because of double registration
of the same notifier when to adin1110/2111 devices are connected to
the same system.

Move the registration of netdev_notifiers in module init call,
in this way multiple driver instances can use the same notifiers.

Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
Signed-off-by: Alexandru Tachici <[email protected]>
---
drivers/net/ethernet/adi/adin1110.c | 38 ++++++++++++++++++++++-------
1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
index 086aa9c96b31..78ded19dd8c1 100644
--- a/drivers/net/ethernet/adi/adin1110.c
+++ b/drivers/net/ethernet/adi/adin1110.c
@@ -1507,16 +1507,15 @@ static struct notifier_block adin1110_switchdev_notifier = {
.notifier_call = adin1110_switchdev_event,
};

-static void adin1110_unregister_notifiers(void *data)
+static void adin1110_unregister_notifiers(void)
{
unregister_switchdev_blocking_notifier(&adin1110_switchdev_blocking_notifier);
unregister_switchdev_notifier(&adin1110_switchdev_notifier);
unregister_netdevice_notifier(&adin1110_netdevice_nb);
}

-static int adin1110_setup_notifiers(struct adin1110_priv *priv)
+static int adin1110_setup_notifiers(void)
{
- struct device *dev = &priv->spidev->dev;
int ret;

ret = register_netdevice_notifier(&adin1110_netdevice_nb);
@@ -1531,13 +1530,14 @@ static int adin1110_setup_notifiers(struct adin1110_priv *priv)
if (ret < 0)
goto err_sdev;

- return devm_add_action_or_reset(dev, adin1110_unregister_notifiers, NULL);
+ return 0;

err_sdev:
unregister_switchdev_notifier(&adin1110_switchdev_notifier);

err_netdev:
unregister_netdevice_notifier(&adin1110_netdevice_nb);
+
return ret;
}

@@ -1608,10 +1608,6 @@ static int adin1110_probe_netdevs(struct adin1110_priv *priv)
if (ret < 0)
return ret;

- ret = adin1110_setup_notifiers(priv);
- if (ret < 0)
- return ret;
-
for (i = 0; i < priv->cfg->ports_nr; i++) {
ret = devm_register_netdev(dev, priv->ports[i]->netdev);
if (ret < 0) {
@@ -1688,7 +1684,31 @@ static struct spi_driver adin1110_driver = {
.probe = adin1110_probe,
.id_table = adin1110_spi_id,
};
-module_spi_driver(adin1110_driver);
+
+static int __init adin1110_driver_init(void)
+{
+ int err;
+
+ err = spi_register_driver(&adin1110_driver);
+ if (err)
+ return err;
+
+ err = adin1110_setup_notifiers();
+ if (err) {
+ spi_unregister_driver(&adin1110_driver);
+ return err;
+ }
+
+ return 0;
+}
+
+static void __exit adin1110_exit(void)
+{
+ adin1110_unregister_notifiers();
+ spi_unregister_driver(&adin1110_driver);
+}
+module_init(adin1110_driver_init);
+module_exit(adin1110_exit);

MODULE_DESCRIPTION("ADIN1110 Network driver");
MODULE_AUTHOR("Alexandru Tachici <[email protected]>");
--
2.34.1

2022-10-20 12:57:42

by Russell King (Oracle)

Subject: Re: [net v2 1/1] net: ethernet: adi: adin1110: Fix notifiers

Hi,

On Thu, Oct 20, 2022 at 12:48:04PM +0300, Alexandru Tachici wrote:
> @@ -1688,7 +1684,31 @@ static struct spi_driver adin1110_driver = {
> .probe = adin1110_probe,
> .id_table = adin1110_spi_id,
> };
> -module_spi_driver(adin1110_driver);
> +
> +static int __init adin1110_driver_init(void)
> +{
> + int err;
> +
> + err = spi_register_driver(&adin1110_driver);
> + if (err)
> + return err;

This is the point that devices can be bound and thus published to
userspace.

> +
> + err = adin1110_setup_notifiers();
> + if (err) {
> + spi_unregister_driver(&adin1110_driver);
> + return err;
> + }

And you setup the notifier after, so there is a window when
notifications could be lost. Is this safe?

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2022-10-20 13:46:34

by Alexandru Tachici

Subject: Re: [net v2 1/1] net: ethernet: adi: adin1110: Fix notifiers

> > +
> > + err = adin1110_setup_notifiers();
> > + if (err) {
> > + spi_unregister_driver(&adin1110_driver);
> > + return err;
> > + }
>
> And you setup the notifier after, so there is a window when
> notifications could be lost. Is this safe?

At boot time this should be ok. If the module is inserted and then user starts
bridging/bonding etc. will lose some events. Will move notifiers registration
before registering device. Should be fine as the driver checks in all callbacks
if it is meant for him or not the event.

Thanks,
Alexandru