| Message ID | 20221020094804.13527-2-alexandru.tachici@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: adi: adin1110: Fix notifiers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 75 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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?
> > + > > + 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
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 <alexandru.tachici@analog.com>");
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 <alexandru.tachici@analog.com> --- drivers/net/ethernet/adi/adin1110.c | 38 ++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 9 deletions(-)