The 12/13/2021 13:43, Vladimir Oltean wrote: > > On Mon, Dec 13, 2021 at 11:25:29AM +0100, Horatiu Vultur wrote: > > The 12/09/2021 17:43, Horatiu Vultur wrote: > > > > > +int lan966x_register_notifier_blocks(struct lan966x *lan966x) > > > > > +{ > > > > > + int err; > > > > > + > > > > > + lan966x->netdevice_nb.notifier_call = lan966x_netdevice_event; > > > > > + err = register_netdevice_notifier(&lan966x->netdevice_nb); > > > > > + if (err) > > > > > + return err; > > > > > + > > > > > + lan966x->switchdev_nb.notifier_call = lan966x_switchdev_event; > > > > > + err = register_switchdev_notifier(&lan966x->switchdev_nb); > > > > > + if (err) > > > > > + goto err_switchdev_nb; > > > > > + > > > > > + lan966x->switchdev_blocking_nb.notifier_call = lan966x_switchdev_blocking_event; > > > > > + err = register_switchdev_blocking_notifier(&lan966x->switchdev_blocking_nb); > > > > > + if (err) > > > > > + goto err_switchdev_blocking_nb; > > > > > + > > > > > + lan966x_owq = alloc_ordered_workqueue("lan966x_order", 0); > > > > > + if (!lan966x_owq) { > > > > > + err = -ENOMEM; > > > > > + goto err_switchdev_blocking_nb; > > > > > + } > > > > > > > > These should be singleton objects, otherwise things get problematic if > > > > you have more than one switch device instantiated in the system. > > > > > > Yes, I will update this. > > > > Actually I think they need to be part of lan966x. > > Because we want each lan966x instance to be independent of each other. > > This is not seen in this version but is more clear in the next version > > (v4). > > They are independent of each other. You deduce the interface on which > the notifier was emitted using switchdev_notifier_info_to_dev() and act > upon it, if lan966x_netdevice_check() is true. The notifier handling > code itself is stateless, all the state is per port / per switch. > If you register one notifier handler per switch, lan966x_netdevice_check() > would return true for each notifier handler instance, and you would > handle each event twice, would you not? That is correct, I will get the event twice which is a problem in the lan966x. The function lan966x_netdevice_check should be per instance, in this way each instance can filter the events. The reason why I am putting the notifier_block inside lan966x is to be able to get to the instance of lan966x even if I get a event that is not for lan966x port. > notifier handlers should be registered as singletons, like other drivers > do. It looks like not all the other driver register them as singletone. For example: prestera, mlx5, sparx5. (I just have done a git grep for register_switchdev_notifier, I have not looked in details at the implementation). -- /Horatiu