| Message ID | 20231213121411.3091597-7-ioana.ciornei@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dpaa2-switch: small improvements | expand |
On Wed, Dec 13, 2023 at 02:14:09PM +0200, Ioana Ciornei wrote: > Create separate functions, dpaa2_switch_port_prechangeupper and > dpaa2_switch_port_changeupper, to be called directly when a DPSW port > changes its upper device. > > This way we are not open-coding everything in the main event callback > and we can easily extent when necessary. > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > --- > Changes in v2: > - none > > .../ethernet/freescale/dpaa2/dpaa2-switch.c | 76 +++++++++++++------ > 1 file changed, 52 insertions(+), 24 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > index d9906573f71f..58c0baee2d61 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > @@ -2180,51 +2180,79 @@ dpaa2_switch_prechangeupper_sanity_checks(struct net_device *netdev, > return 0; > } > > -static int dpaa2_switch_port_netdevice_event(struct notifier_block *nb, > - unsigned long event, void *ptr) > +static int dpaa2_switch_port_prechangeupper(struct net_device *netdev, > + struct netdev_notifier_changeupper_info *info) > { > - struct net_device *netdev = netdev_notifier_info_to_dev(ptr); > - struct netdev_notifier_changeupper_info *info = ptr; > struct netlink_ext_ack *extack; > struct net_device *upper_dev; > int err = 0; nit: I don't think that err needs to be initialised here. > > if (!dpaa2_switch_port_dev_check(netdev)) > - return NOTIFY_DONE; > + return 0; > > extack = netdev_notifier_info_to_extack(&info->info); > - > - switch (event) { > - case NETDEV_PRECHANGEUPPER: > - upper_dev = info->upper_dev; > - if (!netif_is_bridge_master(upper_dev)) > - break; > - > + upper_dev = info->upper_dev; > + if (netif_is_bridge_master(upper_dev)) { > err = dpaa2_switch_prechangeupper_sanity_checks(netdev, > upper_dev, > extack); > if (err) > - goto out; > + return err; > > if (!info->linking) > dpaa2_switch_port_pre_bridge_leave(netdev); > + } FWIIW, I think that a more idomatic flow would be to return if netif_is_bridge_master() is false. Something like this (completely untested!): if (!netif_is_bridge_master(upper_dev)) return 0; err = dpaa2_switch_prechangeupper_sanity_checks(netdev, upper_dev, extack); if (err) return err; if (!info->linking) dpaa2_switch_port_pre_bridge_leave(netdev); > + > + return 0; > +} > + > +static int dpaa2_switch_port_changeupper(struct net_device *netdev, > + struct netdev_notifier_changeupper_info *info) > +{ > + struct netlink_ext_ack *extack; > + struct net_device *upper_dev; > + int err = 0; nit: I don't think err is needed in this function it's value never changes. > + > + if (!dpaa2_switch_port_dev_check(netdev)) > + return 0; > + > + extack = netdev_notifier_info_to_extack(&info->info); > + > + upper_dev = info->upper_dev; > + if (netif_is_bridge_master(upper_dev)) { > + if (info->linking) > + return dpaa2_switch_port_bridge_join(netdev, > + upper_dev, > + extack); > + else > + return dpaa2_switch_port_bridge_leave(netdev); > + } > + > + return err; > +} In a similar vein to my comment above, FWIIW, I would have gone for something more like this (completely untested!). if (!netif_is_bridge_master(upper_dev)) return 0; if (info->linking) return dpaa2_switch_port_bridge_join(netdev, upper_dev, extack); return dpaa2_switch_port_bridge_leave(netdev); > + > +static int dpaa2_switch_port_netdevice_event(struct notifier_block *nb, > + unsigned long event, void *ptr) > +{ > + struct net_device *netdev = netdev_notifier_info_to_dev(ptr); > + int err = 0; > + > + switch (event) { > + case NETDEV_PRECHANGEUPPER: > + err = dpaa2_switch_port_prechangeupper(netdev, ptr); > + if (err) > + return notifier_from_errno(err); > > break; > case NETDEV_CHANGEUPPER: > - upper_dev = info->upper_dev; > - if (netif_is_bridge_master(upper_dev)) { > - if (info->linking) > - err = dpaa2_switch_port_bridge_join(netdev, > - upper_dev, > - extack); > - else > - err = dpaa2_switch_port_bridge_leave(netdev); > - } > + err = dpaa2_switch_port_changeupper(netdev, ptr); > + if (err) > + return notifier_from_errno(err); > + > break; > } > > -out: > - return notifier_from_errno(err); > + return NOTIFY_DONE; > } > > struct ethsw_switchdev_event_work { > -- > 2.34.1 >
On Fri, Dec 15, 2023 at 11:49:39AM +0000, Simon Horman wrote: > On Wed, Dec 13, 2023 at 02:14:09PM +0200, Ioana Ciornei wrote: > > Create separate functions, dpaa2_switch_port_prechangeupper and > > dpaa2_switch_port_changeupper, to be called directly when a DPSW port > > changes its upper device. > > > > This way we are not open-coding everything in the main event callback > > and we can easily extent when necessary. > > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > --- > > Changes in v2: > > - none > > > > .../ethernet/freescale/dpaa2/dpaa2-switch.c | 76 +++++++++++++------ > > 1 file changed, 52 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > > index d9906573f71f..58c0baee2d61 100644 > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > > @@ -2180,51 +2180,79 @@ dpaa2_switch_prechangeupper_sanity_checks(struct net_device *netdev, > > return 0; > > } > > > > -static int dpaa2_switch_port_netdevice_event(struct notifier_block *nb, > > - unsigned long event, void *ptr) > > +static int dpaa2_switch_port_prechangeupper(struct net_device *netdev, > > + struct netdev_notifier_changeupper_info *info) > > { > > - struct net_device *netdev = netdev_notifier_info_to_dev(ptr); > > - struct netdev_notifier_changeupper_info *info = ptr; > > struct netlink_ext_ack *extack; > > struct net_device *upper_dev; > > int err = 0; > > nit: I don't think that err needs to be initialised here. > Ok. > > > > if (!dpaa2_switch_port_dev_check(netdev)) > > - return NOTIFY_DONE; > > + return 0; > > > > extack = netdev_notifier_info_to_extack(&info->info); > > - > > - switch (event) { > > - case NETDEV_PRECHANGEUPPER: > > - upper_dev = info->upper_dev; > > - if (!netif_is_bridge_master(upper_dev)) > > - break; > > - > > + upper_dev = info->upper_dev; > > + if (netif_is_bridge_master(upper_dev)) { > > err = dpaa2_switch_prechangeupper_sanity_checks(netdev, > > upper_dev, > > extack); > > if (err) > > - goto out; > > + return err; > > > > if (!info->linking) > > dpaa2_switch_port_pre_bridge_leave(netdev); > > + } > > FWIIW, I think that a more idomatic flow would be to return if > netif_is_bridge_master() is false. Something like this (completely untested!): > > if (!netif_is_bridge_master(upper_dev)) > return 0; > > err = dpaa2_switch_prechangeupper_sanity_checks(netdev, upper_dev, > extack); > if (err) > return err; > > if (!info->linking) > dpaa2_switch_port_pre_bridge_leave(netdev); > It looks better but I don't think this it's easily extensible. I am planning to add support for LAG offloading which would mean that I would have to revert to the initial flow and extend it to something like: if (netif_is_bridge_master(upper_dev)) { ... } else if (netif_is_lag_master(upper_dev)) { ... } The same thing applies to the dpaa2_switch_port_changeupper() function below. > > + > > + return 0; > > +} > > + > > +static int dpaa2_switch_port_changeupper(struct net_device *netdev, > > + struct netdev_notifier_changeupper_info *info) > > +{ > > + struct netlink_ext_ack *extack; > > + struct net_device *upper_dev; > > + int err = 0; > > nit: I don't think err is needed in this function it's value never changes. > Yes, indeed. I'll remove it.
On Fri, Dec 15, 2023 at 02:08:51PM +0200, Ioana Ciornei wrote: > On Fri, Dec 15, 2023 at 11:49:39AM +0000, Simon Horman wrote: > > On Wed, Dec 13, 2023 at 02:14:09PM +0200, Ioana Ciornei wrote: ... > > > if (!dpaa2_switch_port_dev_check(netdev)) > > > - return NOTIFY_DONE; > > > + return 0; > > > > > > extack = netdev_notifier_info_to_extack(&info->info); > > > - > > > - switch (event) { > > > - case NETDEV_PRECHANGEUPPER: > > > - upper_dev = info->upper_dev; > > > - if (!netif_is_bridge_master(upper_dev)) > > > - break; > > > - > > > + upper_dev = info->upper_dev; > > > + if (netif_is_bridge_master(upper_dev)) { > > > err = dpaa2_switch_prechangeupper_sanity_checks(netdev, > > > upper_dev, > > > extack); > > > if (err) > > > - goto out; > > > + return err; > > > > > > if (!info->linking) > > > dpaa2_switch_port_pre_bridge_leave(netdev); > > > + } > > > > FWIIW, I think that a more idomatic flow would be to return if > > netif_is_bridge_master() is false. Something like this (completely untested!): > > > > if (!netif_is_bridge_master(upper_dev)) > > return 0; > > > > err = dpaa2_switch_prechangeupper_sanity_checks(netdev, upper_dev, > > extack); > > if (err) > > return err; > > > > if (!info->linking) > > dpaa2_switch_port_pre_bridge_leave(netdev); > > > > It looks better but I don't think this it's easily extensible. > > I am planning to add support for LAG offloading which would mean that I > would have to revert to the initial flow and extend it to something > like: > > if (netif_is_bridge_master(upper_dev)) { > ... > } else if (netif_is_lag_master(upper_dev)) { > ... > } > > The same thing applies to the dpaa2_switch_port_changeupper() function > below. Understood. If this is going somewhere then don't let me derail it. ,,,
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c index d9906573f71f..58c0baee2d61 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c @@ -2180,51 +2180,79 @@ dpaa2_switch_prechangeupper_sanity_checks(struct net_device *netdev, return 0; } -static int dpaa2_switch_port_netdevice_event(struct notifier_block *nb, - unsigned long event, void *ptr) +static int dpaa2_switch_port_prechangeupper(struct net_device *netdev, + struct netdev_notifier_changeupper_info *info) { - struct net_device *netdev = netdev_notifier_info_to_dev(ptr); - struct netdev_notifier_changeupper_info *info = ptr; struct netlink_ext_ack *extack; struct net_device *upper_dev; int err = 0; if (!dpaa2_switch_port_dev_check(netdev)) - return NOTIFY_DONE; + return 0; extack = netdev_notifier_info_to_extack(&info->info); - - switch (event) { - case NETDEV_PRECHANGEUPPER: - upper_dev = info->upper_dev; - if (!netif_is_bridge_master(upper_dev)) - break; - + upper_dev = info->upper_dev; + if (netif_is_bridge_master(upper_dev)) { err = dpaa2_switch_prechangeupper_sanity_checks(netdev, upper_dev, extack); if (err) - goto out; + return err; if (!info->linking) dpaa2_switch_port_pre_bridge_leave(netdev); + } + + return 0; +} + +static int dpaa2_switch_port_changeupper(struct net_device *netdev, + struct netdev_notifier_changeupper_info *info) +{ + struct netlink_ext_ack *extack; + struct net_device *upper_dev; + int err = 0; + + if (!dpaa2_switch_port_dev_check(netdev)) + return 0; + + extack = netdev_notifier_info_to_extack(&info->info); + + upper_dev = info->upper_dev; + if (netif_is_bridge_master(upper_dev)) { + if (info->linking) + return dpaa2_switch_port_bridge_join(netdev, + upper_dev, + extack); + else + return dpaa2_switch_port_bridge_leave(netdev); + } + + return err; +} + +static int dpaa2_switch_port_netdevice_event(struct notifier_block *nb, + unsigned long event, void *ptr) +{ + struct net_device *netdev = netdev_notifier_info_to_dev(ptr); + int err = 0; + + switch (event) { + case NETDEV_PRECHANGEUPPER: + err = dpaa2_switch_port_prechangeupper(netdev, ptr); + if (err) + return notifier_from_errno(err); break; case NETDEV_CHANGEUPPER: - upper_dev = info->upper_dev; - if (netif_is_bridge_master(upper_dev)) { - if (info->linking) - err = dpaa2_switch_port_bridge_join(netdev, - upper_dev, - extack); - else - err = dpaa2_switch_port_bridge_leave(netdev); - } + err = dpaa2_switch_port_changeupper(netdev, ptr); + if (err) + return notifier_from_errno(err); + break; } -out: - return notifier_from_errno(err); + return NOTIFY_DONE; } struct ethsw_switchdev_event_work {
Create separate functions, dpaa2_switch_port_prechangeupper and dpaa2_switch_port_changeupper, to be called directly when a DPSW port changes its upper device. This way we are not open-coding everything in the main event callback and we can easily extent when necessary. Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> --- Changes in v2: - none .../ethernet/freescale/dpaa2/dpaa2-switch.c | 76 +++++++++++++------ 1 file changed, 52 insertions(+), 24 deletions(-)