Re: [PATCH net-next 1/2] dsa: bonding: implement HW bonding

[Florian Fainelli <f.fainelli@gmail.com>]

On 20/02/15 02:51, Jonas Johansson wrote:
> From: Jonas Johansson <jonas.johansson@westermo.se>
> 
> 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 <jonas.johansson@westermo.se>
> ---
>  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?

>  

[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/

> +
>  	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