From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jonas Johansson Subject: Re: [PATCH net-next 1/2] dsa: bonding: implement HW bonding Date: Fri, 20 Feb 2015 20:28:48 +0100 (CET) Message-ID: References: <1424429473-4601-1-git-send-email-jonasj76@gmail.com> <1424429473-4601-2-git-send-email-jonasj76@gmail.com> <54E763A6.4020505@gmail.com> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: Jonas Johansson , netdev@vger.kernel.org, Jonas Johansson , Jiri Pirko , Scott Feldman To: Florian Fainelli Return-path: Received: from mail-lb0-f170.google.com ([209.85.217.170]:40329 "EHLO mail-lb0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754512AbbBTTcq (ORCPT ); Fri, 20 Feb 2015 14:32:46 -0500 Received: by lbdu10 with SMTP id u10so8284723lbd.7 for ; Fri, 20 Feb 2015 11:32:44 -0800 (PST) In-Reply-To: <54E763A6.4020505@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, 20 Feb 2015, Florian Fainelli wrote: > On 20/02/15 02:51, Jonas Johansson wrote: >> From: Jonas Johansson >> >> This patch will implement hooks for hardware bonding support for the DSA >> driver. When the team driver adds a DSA slave port the port will be assigned >> a bond group id and the DSA slave driver can setup the hardware. When team >> changes the port state (enabled/disabled) the DSA slave driver is able to >> use the attach/detach callback which will allow the hardware to change the >> hardware settings to reflect the state. >> >> Added DSA hooks: >> bond_add_group: To add a port to a bond group >> bond_del_group: To remove a port from a bond group >> bond_attach: To mark the port in a bond group as attached/active >> bond_detach: To unmark the port in a bond group as detach/inactive >> >> Added new network device hooks: >> ndo_bond_attach: To attach a device to a bond group. >> ndo_bond_detach: To detach a device from a bond group. >> >> Team: >> Added callback to ndo_bond_attach when port is enabled. >> Added callback to ndo_bond_detach when port is disabled. >> >> Added DSA notifier: >> Listening on NETDEV_CHANGEUPPER to add or deleta a port to/from a bond group. >> >> Signed-off-by: Jonas Johansson >> --- >> drivers/net/team/team.c | 4 ++++ >> include/linux/netdevice.h | 8 +++++++ >> include/net/dsa.h | 8 +++++++ >> net/dsa/dsa.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++ >> net/dsa/dsa_priv.h | 6 +++++ >> net/dsa/slave.c | 23 ++++++++++++++++++ >> 6 files changed, 109 insertions(+) >> >> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c >> index 0e62274..f7b2afb 100644 >> --- a/drivers/net/team/team.c >> +++ b/drivers/net/team/team.c >> @@ -934,6 +934,8 @@ static void team_port_enable(struct team *team, >> team->ops.port_enabled(team, port); >> team_notify_peers(team); >> team_mcast_rejoin(team); >> + if (port->dev->netdev_ops->ndo_bond_attach) >> + port->dev->netdev_ops->ndo_bond_attach(port->dev); >> } >> >> static void __reconstruct_port_hlist(struct team *team, int rm_index) >> @@ -965,6 +967,8 @@ static void team_port_disable(struct team *team, >> team_adjust_ops(team); >> team_notify_peers(team); >> team_mcast_rejoin(team); >> + if (port->dev->netdev_ops->ndo_bond_detach) >> + port->dev->netdev_ops->ndo_bond_detach(port->dev); >> } > > Do we really need new ndos here? Cannot we learn this via > NETDEV_CHANGEUPPER? > If team e.g. uses LACP as runner, a number of ports can be configured as a bond group, but only the ports of the same physical type will be enabled and used for bonding. team_port_enable() and team_port_disable() are the functions to set the ports which should be included for bonding. > > [snip] > >> >> /* platform driver init and cleanup *****************************************/ >> static int dev_is_class(struct device *dev, void *class) >> @@ -778,6 +835,9 @@ static int dsa_probe(struct platform_device *pdev) >> add_timer(&dst->link_poll_timer); >> } >> >> + /* Setup notifier */ >> + register_netdevice_notifier(&dsa_nb); > > I do not think we need to register the netdevice_notifier for every DSA > platform_device we might instantiate, a single one, global, whenever the > DSA driver gets inserted should be enough. > > Also, I would prefer if we moved this to net/dsa/slave.c where the other > netdevice_ops are layered, very much like this patch: > > [1]: http://patchwork.ozlabs.org/patch/440696/ > I agree. >> + >> return 0; >> >> out: >> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h >> index dc9756d..6a1456a 100644 >> --- a/net/dsa/dsa_priv.h >> +++ b/net/dsa/dsa_priv.h >> @@ -45,6 +45,11 @@ struct dsa_slave_priv { >> int old_link; >> int old_pause; >> int old_duplex; >> + >> + /* >> + * Bond group id, or 0 if port is not bonded. >> + */ >> + int bond_gid; >> }; >> >> /* dsa.c */ >> @@ -58,6 +63,7 @@ struct net_device *dsa_slave_create(struct dsa_switch *ds, >> int port, char *name); >> int dsa_slave_suspend(struct net_device *slave_dev); >> int dsa_slave_resume(struct net_device *slave_dev); >> +bool dsa_slave_check(struct net_device *dev); >> >> /* tag_dsa.c */ >> extern const struct dsa_device_ops dsa_netdev_ops; >> diff --git a/net/dsa/slave.c b/net/dsa/slave.c >> index f23dead..88c84bf 100644 >> --- a/net/dsa/slave.c >> +++ b/net/dsa/slave.c >> @@ -441,6 +441,22 @@ static int dsa_slave_get_eee(struct net_device *dev, struct ethtool_eee *e) >> return ret; >> } >> >> +static int dsa_slave_bond_attach(struct net_device *dev) >> +{ >> + struct dsa_slave_priv *p = netdev_priv(dev); >> + struct dsa_switch *ds = p->parent; >> + >> + return ds->drv->bond_attach(ds, p->port); > > You need to test for the callback implementation in the driver, since it > is optional and likely not to be implemented immediately. > -- > Florian > Thanks, I missed that.