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

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

[Jonas Johansson]

From: Jonas Johansson <jonas.johansson@westermo.se>

This patchset will implement hardware bonding support for the DSA driver and
the Marvell 88E6095 device.

Jonas Johansson (2):
  dsa: bonding: implement HW bonding
  mv88e6131: bonding: implement single device trunking

 drivers/net/dsa/mv88e6131.c | 254 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h |  14 +++
 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 ++++
 8 files changed, 377 insertions(+)

-- 
2.1.0

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

[Jonas Johansson]

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);
 }
 
 #define TEAM_VLAN_FEATURES (NETIF_F_ALL_CSUM | NETIF_F_SG | \
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5897b4e..8ebaefa 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -939,6 +939,12 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
  * int (*ndo_del_slave)(struct net_device *dev, struct net_device *slave_dev);
  *	Called to release previously enslaved netdev.
  *
+ * int (*ndo_bond_attach)(struct net_device *dev);
+ *	Called to attach the device to a bond group.
+ *
+ * int (*ndo_bond_detach)(struct net_device *dev);
+ *	Called to detach the device from a bond group.
+ *
  *      Feature/offload setting functions.
  * netdev_features_t (*ndo_fix_features)(struct net_device *dev,
  *		netdev_features_t features);
@@ -1131,6 +1137,8 @@ struct net_device_ops {
 						 struct net_device *slave_dev);
 	int			(*ndo_del_slave)(struct net_device *dev,
 						 struct net_device *slave_dev);
+	int			(*ndo_bond_attach)(struct net_device *dev);
+	int			(*ndo_bond_detach)(struct net_device *dev);
 	netdev_features_t	(*ndo_fix_features)(struct net_device *dev,
 						    netdev_features_t features);
 	int			(*ndo_set_features)(struct net_device *dev,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index ed3c34b..64b5963 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -254,6 +254,14 @@ struct dsa_switch_driver {
 	int	(*get_eee)(struct dsa_switch *ds, int port,
 			   struct ethtool_eee *e);
 
+	/*
+	 * Bonding
+	 */
+	int	(*bond_add_group)(struct dsa_switch *ds, int port, int gid);
+	int	(*bond_del_group)(struct dsa_switch *ds, int port, int gid);
+	int	(*bond_attach)(struct dsa_switch *ds, int port);
+	int	(*bond_detach)(struct dsa_switch *ds, int port);
+
 #ifdef CONFIG_NET_DSA_HWMON
 	/* Hardware monitoring */
 	int	(*get_temp)(struct dsa_switch *ds, int *temp);
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 2173402..3d2c599 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <net/dsa.h>
+#include <net/rtnetlink.h>
 #include <linux/of.h>
 #include <linux/of_mdio.h>
 #include <linux/of_platform.h>
@@ -442,6 +443,62 @@ static void dsa_link_poll_timer(unsigned long _dst)
 	schedule_work(&dst->link_poll_work);
 }
 
+/* net device notifier event handler ****************************************/
+static int dsa_master_changed(struct net_device *dev)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+	struct net_device *master = netdev_master_upper_dev_get(dev);
+	int err = 0;
+
+	/* Add port to bond group */
+	if (master && master->rtnl_link_ops &&
+	    !strcmp(master->rtnl_link_ops->kind, "team")) {
+
+		p->bond_gid = master->ifindex;
+
+		if (!ds->drv->bond_add_group)
+			return -EOPNOTSUPP;
+		return ds->drv->bond_add_group(ds, p->port, p->bond_gid);
+	}
+
+	/* Remove port from bond group */
+	if (!master && p->bond_gid) {
+		if (!ds->drv->bond_del_group)
+			return -EOPNOTSUPP;
+		err = ds->drv->bond_del_group(ds, p->port, p->bond_gid);
+		p->bond_gid = 0;
+		return err;
+	}
+
+	return 0;
+}
+
+static int dsa_event(struct notifier_block *nb, unsigned long event, void *data)
+
+{
+	struct net_device *dev;
+	int err = 0;
+
+	switch (event) {
+	case NETDEV_CHANGEUPPER:
+		dev = netdev_notifier_info_to_dev(data);
+		if (!dsa_slave_check(dev))
+			return NOTIFY_DONE;
+		err = dsa_master_changed(dev);
+		if (err)
+			netdev_warn(dev,
+				    "failed to reflect master change (err %d)\n",
+				    err);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block dsa_nb __read_mostly = {
+	.notifier_call = dsa_event,
+};
 
 /* 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);
+
 	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);
+}
+
+static int dsa_slave_bond_detach(struct net_device *dev)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	return ds->drv->bond_detach(ds, p->port);
+}
+
 static const struct ethtool_ops dsa_slave_ethtool_ops = {
 	.get_settings		= dsa_slave_get_settings,
 	.set_settings		= dsa_slave_set_settings,
@@ -470,6 +486,8 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
 	.ndo_set_rx_mode	= dsa_slave_set_rx_mode,
 	.ndo_set_mac_address	= dsa_slave_set_mac_address,
 	.ndo_do_ioctl		= dsa_slave_ioctl,
+	.ndo_bond_attach	= dsa_slave_bond_attach,
+	.ndo_bond_detach	= dsa_slave_bond_detach,
 };
 
 static void dsa_slave_adjust_link(struct net_device *dev)
@@ -574,6 +592,11 @@ static int dsa_slave_phy_setup(struct dsa_slave_priv *p,
 	return 0;
 }
 
+bool dsa_slave_check(struct net_device *ndev)
+{
+	return ndev->netdev_ops == &dsa_slave_netdev_ops;
+}
+
 int dsa_slave_suspend(struct net_device *slave_dev)
 {
 	struct dsa_slave_priv *p = netdev_priv(slave_dev);
-- 
2.1.0

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

[Andrew Lunn]

On Fri, Feb 20, 2015 at 11:51:12AM +0100, 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);
>  }
>  
>  #define TEAM_VLAN_FEATURES (NETIF_F_ALL_CSUM | NETIF_F_SG | \
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 5897b4e..8ebaefa 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -939,6 +939,12 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
>   * int (*ndo_del_slave)(struct net_device *dev, struct net_device *slave_dev);
>   *	Called to release previously enslaved netdev.
>   *
> + * int (*ndo_bond_attach)(struct net_device *dev);
> + *	Called to attach the device to a bond group.
> + *
> + * int (*ndo_bond_detach)(struct net_device *dev);
> + *	Called to detach the device from a bond group.
> + *
>   *      Feature/offload setting functions.
>   * netdev_features_t (*ndo_fix_features)(struct net_device *dev,
>   *		netdev_features_t features);
> @@ -1131,6 +1137,8 @@ struct net_device_ops {
>  						 struct net_device *slave_dev);
>  	int			(*ndo_del_slave)(struct net_device *dev,
>  						 struct net_device *slave_dev);
> +	int			(*ndo_bond_attach)(struct net_device *dev);
> +	int			(*ndo_bond_detach)(struct net_device *dev);
>  	netdev_features_t	(*ndo_fix_features)(struct net_device *dev,
>  						    netdev_features_t features);
>  	int			(*ndo_set_features)(struct net_device *dev,
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index ed3c34b..64b5963 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -254,6 +254,14 @@ struct dsa_switch_driver {
>  	int	(*get_eee)(struct dsa_switch *ds, int port,
>  			   struct ethtool_eee *e);
>  
> +	/*
> +	 * Bonding
> +	 */
> +	int	(*bond_add_group)(struct dsa_switch *ds, int port, int gid);
> +	int	(*bond_del_group)(struct dsa_switch *ds, int port, int gid);
> +	int	(*bond_attach)(struct dsa_switch *ds, int port);
> +	int	(*bond_detach)(struct dsa_switch *ds, int port);
> +
>  #ifdef CONFIG_NET_DSA_HWMON
>  	/* Hardware monitoring */
>  	int	(*get_temp)(struct dsa_switch *ds, int *temp);
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 2173402..3d2c599 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -17,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <net/dsa.h>
> +#include <net/rtnetlink.h>
>  #include <linux/of.h>
>  #include <linux/of_mdio.h>
>  #include <linux/of_platform.h>
> @@ -442,6 +443,62 @@ static void dsa_link_poll_timer(unsigned long _dst)
>  	schedule_work(&dst->link_poll_work);
>  }
>  
> +/* net device notifier event handler ****************************************/
> +static int dsa_master_changed(struct net_device *dev)
> +{
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	struct dsa_switch *ds = p->parent;
> +	struct net_device *master = netdev_master_upper_dev_get(dev);
> +	int err = 0;
> +
> +	/* Add port to bond group */
> +	if (master && master->rtnl_link_ops &&
> +	    !strcmp(master->rtnl_link_ops->kind, "team")) {
> +
> +		p->bond_gid = master->ifindex;
> +
> +		if (!ds->drv->bond_add_group)
> +			return -EOPNOTSUPP;
> +		return ds->drv->bond_add_group(ds, p->port, p->bond_gid);
> +	}

Hi Jonas

Maybe it is here and i cannot see it, but where do you check the slave
devices in the group are in the same physical switch. Remember that
DSA allows nearly arbitrary trees of switches.

       Andrew

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

[Jonas Johansson]

On Fri, 20 Feb 2015, Andrew Lunn wrote:

> On Fri, Feb 20, 2015 at 11:51:12AM +0100, 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);
>>  }
>>
>>  #define TEAM_VLAN_FEATURES (NETIF_F_ALL_CSUM | NETIF_F_SG | \
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 5897b4e..8ebaefa 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -939,6 +939,12 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
>>   * int (*ndo_del_slave)(struct net_device *dev, struct net_device *slave_dev);
>>   *	Called to release previously enslaved netdev.
>>   *
>> + * int (*ndo_bond_attach)(struct net_device *dev);
>> + *	Called to attach the device to a bond group.
>> + *
>> + * int (*ndo_bond_detach)(struct net_device *dev);
>> + *	Called to detach the device from a bond group.
>> + *
>>   *      Feature/offload setting functions.
>>   * netdev_features_t (*ndo_fix_features)(struct net_device *dev,
>>   *		netdev_features_t features);
>> @@ -1131,6 +1137,8 @@ struct net_device_ops {
>>  						 struct net_device *slave_dev);
>>  	int			(*ndo_del_slave)(struct net_device *dev,
>>  						 struct net_device *slave_dev);
>> +	int			(*ndo_bond_attach)(struct net_device *dev);
>> +	int			(*ndo_bond_detach)(struct net_device *dev);
>>  	netdev_features_t	(*ndo_fix_features)(struct net_device *dev,
>>  						    netdev_features_t features);
>>  	int			(*ndo_set_features)(struct net_device *dev,
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index ed3c34b..64b5963 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -254,6 +254,14 @@ struct dsa_switch_driver {
>>  	int	(*get_eee)(struct dsa_switch *ds, int port,
>>  			   struct ethtool_eee *e);
>>
>> +	/*
>> +	 * Bonding
>> +	 */
>> +	int	(*bond_add_group)(struct dsa_switch *ds, int port, int gid);
>> +	int	(*bond_del_group)(struct dsa_switch *ds, int port, int gid);
>> +	int	(*bond_attach)(struct dsa_switch *ds, int port);
>> +	int	(*bond_detach)(struct dsa_switch *ds, int port);
>> +
>>  #ifdef CONFIG_NET_DSA_HWMON
>>  	/* Hardware monitoring */
>>  	int	(*get_temp)(struct dsa_switch *ds, int *temp);
>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
>> index 2173402..3d2c599 100644
>> --- a/net/dsa/dsa.c
>> +++ b/net/dsa/dsa.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/module.h>
>>  #include <net/dsa.h>
>> +#include <net/rtnetlink.h>
>>  #include <linux/of.h>
>>  #include <linux/of_mdio.h>
>>  #include <linux/of_platform.h>
>> @@ -442,6 +443,62 @@ static void dsa_link_poll_timer(unsigned long _dst)
>>  	schedule_work(&dst->link_poll_work);
>>  }
>>
>> +/* net device notifier event handler ****************************************/
>> +static int dsa_master_changed(struct net_device *dev)
>> +{
>> +	struct dsa_slave_priv *p = netdev_priv(dev);
>> +	struct dsa_switch *ds = p->parent;
>> +	struct net_device *master = netdev_master_upper_dev_get(dev);
>> +	int err = 0;
>> +
>> +	/* Add port to bond group */
>> +	if (master && master->rtnl_link_ops &&
>> +	    !strcmp(master->rtnl_link_ops->kind, "team")) {
>> +
>> +		p->bond_gid = master->ifindex;
>> +
>> +		if (!ds->drv->bond_add_group)
>> +			return -EOPNOTSUPP;
>> +		return ds->drv->bond_add_group(ds, p->port, p->bond_gid);
>> +	}
>
> Hi Jonas
>
> Maybe it is here and i cannot see it, but where do you check the slave
> devices in the group are in the same physical switch. Remember that
> DSA allows nearly arbitrary trees of switches.
>
>       Andrew
>
Thanks for the review.
You are correct, there is no check if the slave devices are in the same 
physical switch. I know that some devices (e.g. 88E6095) can handle 
cross-chip trunking, but as this implementation was targeting single-chip 
trunking you are correct that a check needs to be implemented to prevent 
the slaves to spread over several devices. I'll look into it.

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

[Florian Fainelli]

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

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

[roopa]

On 2/20/15, 8:41 AM, Florian Fainelli wrote:
> 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?

+1, I was just typing a similar response. We need this for switchdevices 
too and notifiers can be used here.

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

[Jonas Johansson]

On Fri, 20 Feb 2015, Florian Fainelli wrote:

> 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?
>
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.

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

[Jiri Pirko]

Fri, Feb 20, 2015 at 08:28:48PM CET, jonasj76@gmail.com wrote:
>
>
>On Fri, 20 Feb 2015, Florian Fainelli wrote:
>
>>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?
>>
>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.

Hmm, I'm not sure I understand correctly what the marvel hw is capable of.
Would you care to explain it a bit ? (or maybe I should read the
datasheet)

If we want to offload bond functionality (tx, rx), we need to be careful
about the features, mainly mode/runner tx function which should not be
different in hw to what we have in kernel. That might be pretty hard for
example in case of loadbalance team mode, where bpf is used to get tx
port.

Also, switchdev ndos and notifier propagation needs to be implemented in
team/bond.

But maybe I'm getting your usecase wrong. Please help me to understand
it.

Thanks.


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

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

[Andrew Lunn]

> Hmm, I'm not sure I understand correctly what the marvel hw is capable of.
> Would you care to explain it a bit ? (or maybe I should read the
> datasheet)

The mv88e6060 data sheet is publicly available. It is a rather old
chip, but i guess trunking has not changed too much since then. All
the newer chips require an NDA to get the datahseet.

What i've done before is get the switch to combine two ports into a
trunk. It will then load balance packets over the two ports. If one of
the ports fails, i.e. link down, it will then concentrate all the
traffic over the one remaining port. i.e some basic form of
redundancy/failover.

Not covered by this patchset, but what would be interesting, is
getting trunking working towards the host. Many of the recent wireless
APs have two ethernet interfaces connected to the switch. The current
DSA architecture only allows one of them to be used for traffic
to/from the host from/to the switch.

	Andrew

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

[Scott Feldman]

On Fri, Feb 20, 2015 at 2:51 AM, Jonas Johansson <jonasj76@gmail.com> 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.

Hi Jonas,

Cc:'ing Andy as he mentioned at netdev01 he was looking into something
similar for switchdev.

My comments are along the lines of the others:

Can we up-level this not for just DSA but for switchdev in general?  I
think the requirements are:

1) It should work for team or bonding.

2) Port driver needs to know a) bond membership, and b) port (active,
inactive) status, at least for 802.3ad.

3) Try to avoid adding new ndo ops if possible, and rather use what's there.

It took me a bit to figure out your new ndo ops in this patch.  Going
by the name ndo_bond_attach/dettach, I thought these were for port
membership, but you're using them for port status change.  So the name
was confusing.  Since you already have port membership covered with
netdevice event NETDEV_CHANGEUPPER, can we also adapt port status
change into netdevice event NETDEV_BONDING_INFO?  (See
netdev_bonding_info_change()).

-scott

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

[Jonas Johansson]

On Sat, 21 Feb 2015, Scott Feldman wrote:

> On Fri, Feb 20, 2015 at 2:51 AM, Jonas Johansson <jonasj76@gmail.com> 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.
>
> Hi Jonas,
>
> Cc:'ing Andy as he mentioned at netdev01 he was looking into something
> similar for switchdev.
>
> My comments are along the lines of the others:
>
> Can we up-level this not for just DSA but for switchdev in general?  I
> think the requirements are:
>
> 1) It should work for team or bonding.
>
> 2) Port driver needs to know a) bond membership, and b) port (active,
> inactive) status, at least for 802.3ad.
>
> 3) Try to avoid adding new ndo ops if possible, and rather use what's there.
>
> It took me a bit to figure out your new ndo ops in this patch.  Going
> by the name ndo_bond_attach/dettach, I thought these were for port
> membership, but you're using them for port status change.  So the name
> was confusing.  Since you already have port membership covered with
> netdevice event NETDEV_CHANGEUPPER, can we also adapt port status
> change into netdevice event NETDEV_BONDING_INFO?  (See
> netdev_bonding_info_change()).
>
> -scott
>
I think aiming for switchdev is a good ides. I will look into this and if 
NETDEV_BONDING_INFO can be used instead if adding the new ndo fops. I'm on 
vacation atm, so I will start look into it the next week. Thanks for all 
the good input.
- Jonas

[PATCH net-next 2/2] mv88e6131: bonding: implement single device trunking

[Jonas Johansson]

From: Jonas Johansson <jonas.johansson@westermo.se>

This patch will use the DSA hardware bonding support hooks to setup trunking
for the Marvell 88E6095 device. The implementation only handles trunking in
a single device.

Hooks:
 .bond_add_group: Add port to a bond group
 .bond_del_group: Remove port from a bond group
 .bond_attach: Attach/activate port in bond group
 .bond_detach: Detach/inactivate port in bond group

Procedure to add/remome port from bond group:
 Setup trunk learning (Port Association Vector)
 Setup loop prevention (VLAN Table)
 Setup load balancing (Trunk Mask Load Balance Table)

Procedure to attach/detach port:
 Change load balancing (Trunk Mask Load Balance Table)

Signed-off-by: Jonas Johansson <jonas.johansson@westermo.se>
---
 drivers/net/dsa/mv88e6131.c | 254 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h |  14 +++
 2 files changed, 268 insertions(+)

diff --git a/drivers/net/dsa/mv88e6131.c b/drivers/net/dsa/mv88e6131.c
index 2540ef0..3ba7a0c 100644
--- a/drivers/net/dsa/mv88e6131.c
+++ b/drivers/net/dsa/mv88e6131.c
@@ -382,6 +382,256 @@ mv88e6131_get_ethtool_stats(struct dsa_switch *ds,
 				    mv88e6131_hw_stats, port, data);
 }
 
+/* Trunking */
+static int mv88e6131_bond_set_trunk_learning(struct dsa_switch *ds,
+					     int *ports, size_t num)
+{
+	u16 port_vec = 0;
+	int ret;
+	int i;
+
+	num = num < MAX_PORTS ? num : MAX_PORTS;
+
+	for (i = 0; i < num; i++)
+		port_vec |= 1 << ports[i];
+
+	for (i = 0; i < num; i++) {
+		ret = mv88e6xxx_reg_read(ds, REG_PORT(ports[i]), REG_PORT_PAV);
+		if (ret < 0)
+			continue;
+		ret = (ret & 0xf800) | (port_vec & 0x7ff);
+		mv88e6xxx_reg_write(ds, REG_PORT(ports[i]), REG_PORT_PAV, ret);
+	}
+
+	return 0;
+}
+
+static int mv88e6131_bond_set_loop_prevention(struct dsa_switch *ds,
+					      int *ports, size_t num)
+{
+	u16 port_vec = 0;
+	int ret;
+	int i;
+
+	num = num < MAX_PORTS ? num : MAX_PORTS;
+
+	for (i = 0; i < num; i++)
+		port_vec |= 1 << ports[i];
+
+	for (i = 0; i < num; i++) {
+		ret = mv88e6xxx_reg_read(ds, REG_PORT(ports[i]), REG_PORT_VLAN_MAP);
+		if (ret < 0)
+			continue;
+		ret &= ~port_vec & 0x7ff;
+		mv88e6xxx_reg_write(ds, REG_PORT(ports[i]), REG_PORT_VLAN_MAP, ret);
+	}
+
+	return 0;
+}
+
+static int mv88e6131_wait_trunk_mask(struct dsa_switch *ds)
+{
+	const int max_retries = 10;
+	int retries = 0;
+	int ret;
+
+	/* Wait for update Trunk Mask data */
+	while (1) {
+		ret = REG_READ(REG_GLOBAL2, REG_TRUNK_MASK);
+		if (!(ret & 0x8000))
+			return ret;
+		if (retries > max_retries) {
+			pr_warn("mv88e6131: Timeout waiting for "
+				"Trunk Mask Table Register Update\n");
+			return -EBUSY;
+		}
+		retries++;
+		usleep_range(20, 50);
+	};
+
+	return -EPERM;
+}
+
+static int mv88e6131_get_trunk_mask(struct dsa_switch *ds, int trunk_nr, u16 *mask)
+{
+	int ret;
+
+	if (trunk_nr > 0x7)
+		return -EINVAL;
+
+	ret = mv88e6131_wait_trunk_mask(ds);
+	if (ret < 0)
+		return ret;
+
+	/* Set MaskNum */
+	ret = (ret & 0x0fff) | (trunk_nr << 12);
+	REG_WRITE(REG_GLOBAL2, REG_TRUNK_MASK, ret);
+
+	/* Get TrunkMask */
+	ret = REG_READ(REG_GLOBAL2, REG_TRUNK_MASK);
+	*mask = ret & 0x7FF;
+
+	return 0;
+}
+
+static int mv88e6131_set_trunk_mask(struct dsa_switch *ds, int trunk_nr, u16 mask)
+{
+	int ret;
+
+	if (trunk_nr > 0x7)
+		return -EINVAL;
+
+	ret = mv88e6131_wait_trunk_mask(ds);
+	if (ret < 0)
+		return ret;
+
+	/* Write TrunkMask */
+	ret = 0x8000 | (ret & 0x7800) | (trunk_nr << 12) | (mask & 0x7ff);
+	REG_WRITE(REG_GLOBAL2, REG_TRUNK_MASK, ret);
+
+	return 0;
+}
+
+static int mv88e6131_bond_set_load_balancing(struct dsa_switch *ds,
+					     int *ports, bool *attached, size_t num)
+{
+	u16 mask;
+	u16 member_mask = 0;
+	int att_ports[MAX_PORTS];
+	int att_num = 0;
+	int ret;
+	int i;
+
+	num = num < MAX_PORTS ? num : MAX_PORTS;
+
+	for (i = 0; i < num; i++) {
+		member_mask |= 1 << ports[i];
+		if (attached[i])
+			att_ports[att_num++] = ports[i];
+	}
+
+	for (i = 0; i < 8; i++) {
+		ret = mv88e6131_get_trunk_mask(ds, i, &mask);
+		if (ret < 0)
+			continue;
+		mask &= ~member_mask;
+		if (att_num)
+			mask |= 1 << att_ports[i % att_num];
+		mv88e6131_set_trunk_mask(ds, i, mask);
+	}
+
+	return 0;
+}
+
+static int mv88e6131_bond_get_ports(struct dsa_switch *ds, int gid, int *ports, bool *attached, size_t sz)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int num = 0;
+	int p;
+
+	for (p = 0; (p < MAX_PORTS) && (num < sz) ; p++) {
+		if (ps->bond_port[p].gid == gid) {
+			ports[num] = p;
+			attached[num] = ps->bond_port[p].attached;
+			num++;
+		}
+	}
+
+	return num;
+}
+
+static int mv88e6131_bond_setup(struct dsa_switch *ds, int gid)
+{
+	int ports[MAX_PORTS];
+	bool attached[MAX_PORTS];
+	int num;
+	int err = 0;
+
+	num = mv88e6131_bond_get_ports(ds, gid, ports, attached, MAX_PORTS);
+
+	err = mv88e6131_bond_set_trunk_learning(ds, ports, num);
+	if (err)
+		return err;
+	err = mv88e6131_bond_set_loop_prevention(ds, ports, num);
+	if (err)
+		return err;
+	err = mv88e6131_bond_set_load_balancing(ds, ports, attached, num);
+	if (err)
+		return err;
+
+	return 0;
+
+}
+
+static int mv88e6131_bond_add_group(struct dsa_switch *ds, int port, int gid)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+
+	ps->bond_port[port].gid = gid;
+	ps->bond_port[port].attached = false;
+
+	return mv88e6131_bond_setup(ds, gid);
+}
+
+static int mv88e6131_bond_del_group(struct dsa_switch *ds, int port, int gid)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	bool btmp;
+	int ret;
+
+	ps->bond_port[port].gid = 0;
+	ps->bond_port[port].attached = false;
+	mv88e6131_bond_setup(ds, gid);
+
+	/* Reset trunk learning */
+	ret = mv88e6xxx_reg_read(ds, REG_PORT(port), REG_PORT_PAV);
+	if (ret >= 0) {
+		ret = (ret & 0xf800) | ((1 << port) & 0x7ff);
+		mv88e6xxx_reg_write(ds, REG_PORT(port), REG_PORT_PAV, ret);
+	}
+	/* Reset loop prevention  */
+	ret = mv88e6xxx_reg_read(ds, REG_PORT(port), REG_PORT_VLAN_MAP);
+	if (ret >= 0) {
+		ret = (ret & 0xf800) | ((1 << dsa_upstream_port(ds)) & 0x7ff);
+		mv88e6xxx_reg_write(ds, REG_PORT(port), REG_PORT_VLAN_MAP, ret);
+	}
+	/* Reset load balancing */
+	btmp = true;
+	mv88e6131_bond_set_load_balancing(ds, &port, &btmp, 1);
+
+	return 0;
+}
+
+static int mv88e6131_bond_attach(struct dsa_switch *ds, int port)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int ports[MAX_PORTS];
+	bool attached[MAX_PORTS];
+	int gid = ps->bond_port[port].gid;
+	int num;
+
+	ps->bond_port[port].attached = true;
+	num = mv88e6131_bond_get_ports(ds, gid, ports, attached, MAX_PORTS);
+
+	return mv88e6131_bond_set_load_balancing(ds, ports, attached, num);
+}
+
+static int mv88e6131_bond_detach(struct dsa_switch *ds, int port)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	int ports[MAX_PORTS];
+	bool attached[MAX_PORTS];
+	int gid = ps->bond_port[port].gid;
+	int num;
+
+	ps->bond_port[port].attached = false;
+	num = mv88e6131_bond_get_ports(ds, gid, ports, attached, MAX_PORTS);
+
+	return mv88e6131_bond_set_load_balancing(ds, ports, attached, num);
+}
+
+
+
 static int mv88e6131_get_sset_count(struct dsa_switch *ds)
 {
 	return ARRAY_SIZE(mv88e6131_hw_stats);
@@ -399,6 +649,10 @@ struct dsa_switch_driver mv88e6131_switch_driver = {
 	.get_strings		= mv88e6131_get_strings,
 	.get_ethtool_stats	= mv88e6131_get_ethtool_stats,
 	.get_sset_count		= mv88e6131_get_sset_count,
+	.bond_add_group		= mv88e6131_bond_add_group,
+	.bond_del_group		= mv88e6131_bond_del_group,
+	.bond_attach		= mv88e6131_bond_attach,
+	.bond_detach		= mv88e6131_bond_detach,
 };
 
 MODULE_ALIAS("platform:mv88e6085");
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 7294227..383b224 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -11,9 +11,19 @@
 #ifndef __MV88E6XXX_H
 #define __MV88E6XXX_H
 
+#define MAX_PORTS		11
+
 #define REG_PORT(p)		(0x10 + (p))
+#define REG_PORT_VLAN_MAP	0x6
+#define REG_PORT_PAV		0xb
 #define REG_GLOBAL		0x1b
 #define REG_GLOBAL2		0x1c
+#define REG_TRUNK_MASK		0x7
+
+struct mv88e6xxx_bond_port {
+	int gid;
+	bool attached;
+};
 
 struct mv88e6xxx_priv_state {
 	/* When using multi-chip addressing, this mutex protects
@@ -49,6 +59,10 @@ struct mv88e6xxx_priv_state {
 	struct mutex eeprom_mutex;
 
 	int		id; /* switch product id */
+
+	/* Contains bonding info for each port
+	 */
+	struct mv88e6xxx_bond_port	bond_port[MAX_PORTS];
 };
 
 struct mv88e6xxx_hw_stat {
-- 
2.1.0

Re: [PATCH net-next 2/2] mv88e6131: bonding: implement single device trunking

[Andrew Lunn]

On Fri, Feb 20, 2015 at 11:51:13AM +0100, Jonas Johansson wrote:
> From: Jonas Johansson <jonas.johansson@westermo.se>
> 
> This patch will use the DSA hardware bonding support hooks to setup trunking
> for the Marvell 88E6095 device. The implementation only handles trunking in
> a single device.
> 
> Hooks:
>  .bond_add_group: Add port to a bond group
>  .bond_del_group: Remove port from a bond group
>  .bond_attach: Attach/activate port in bond group
>  .bond_detach: Detach/inactivate port in bond group
> 
> Procedure to add/remome port from bond group:
>  Setup trunk learning (Port Association Vector)
>  Setup loop prevention (VLAN Table)
>  Setup load balancing (Trunk Mask Load Balance Table)
> 
> Procedure to attach/detach port:
>  Change load balancing (Trunk Mask Load Balance Table)
> 
> Signed-off-by: Jonas Johansson <jonas.johansson@westermo.se>
> ---
>  drivers/net/dsa/mv88e6131.c | 254 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/mv88e6xxx.h |  14 +++
>  2 files changed, 268 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6131.c b/drivers/net/dsa/mv88e6131.c
> index 2540ef0..3ba7a0c 100644
> --- a/drivers/net/dsa/mv88e6131.c
> +++ b/drivers/net/dsa/mv88e6131.c
> @@ -382,6 +382,256 @@ mv88e6131_get_ethtool_stats(struct dsa_switch *ds,
>  				    mv88e6131_hw_stats, port, data);
>  }
>  
> +/* Trunking */
> +static int mv88e6131_bond_set_trunk_learning(struct dsa_switch *ds,
> +					     int *ports, size_t num)
> +{
> +	u16 port_vec = 0;
> +	int ret;
> +	int i;
> +
> +	num = num < MAX_PORTS ? num : MAX_PORTS;
> +
> +	for (i = 0; i < num; i++)
> +		port_vec |= 1 << ports[i];
> +
> +	for (i = 0; i < num; i++) {
> +		ret = mv88e6xxx_reg_read(ds, REG_PORT(ports[i]), REG_PORT_PAV);
> +		if (ret < 0)
> +			continue;
> +		ret = (ret & 0xf800) | (port_vec & 0x7ff);
> +		mv88e6xxx_reg_write(ds, REG_PORT(ports[i]), REG_PORT_PAV, ret);
> +	}
> +
> +	return 0;
> +}

The mv886060 seems to have the PAV register. So i guess most of the
Marvell switches support this. Is there anything specific to the 6131
here? Could this be moved into mv88x6xxx so other switch drivers can
use it?

> +
> +static int mv88e6131_bond_set_loop_prevention(struct dsa_switch *ds,
> +					      int *ports, size_t num)
> +{
> +	u16 port_vec = 0;
> +	int ret;
> +	int i;
> +
> +	num = num < MAX_PORTS ? num : MAX_PORTS;
> +
> +	for (i = 0; i < num; i++)
> +		port_vec |= 1 << ports[i];
> +
> +	for (i = 0; i < num; i++) {
> +		ret = mv88e6xxx_reg_read(ds, REG_PORT(ports[i]), REG_PORT_VLAN_MAP);
> +		if (ret < 0)
> +			continue;
> +		ret &= ~port_vec & 0x7ff;
> +		mv88e6xxx_reg_write(ds, REG_PORT(ports[i]), REG_PORT_VLAN_MAP, ret);
> +	}
> +
> +	return 0;
> +}

Same question again, anything specific to the 6131 here?

> +static int mv88e6131_wait_trunk_mask(struct dsa_switch *ds)
> +{
> +	const int max_retries = 10;
> +	int retries = 0;
> +	int ret;
> +
> +	/* Wait for update Trunk Mask data */
> +	while (1) {
> +		ret = REG_READ(REG_GLOBAL2, REG_TRUNK_MASK);
> +		if (!(ret & 0x8000))
> +			return ret;
> +		if (retries > max_retries) {
> +			pr_warn("mv88e6131: Timeout waiting for "
> +				"Trunk Mask Table Register Update\n");
> +			return -EBUSY;
> +		}
> +		retries++;
> +		usleep_range(20, 50);
> +	};

This looks a lot like the wait functions what Guenter Roeck added to
6352 and i just moved to mv88e6xxx.c. Please use the generic
infrastructure in the shared code.

Please could you look at all your functions and see what is specific
to the 6131 and what is generic. Place the generic code into mv88e6xxx
please so we can all use it.

       Thanks
	Andrew

Re: [PATCH net-next 2/2] mv88e6131: bonding: implement single device trunking

[Jonas Johansson]

On Fri, 20 Feb 2015, Andrew Lunn wrote:

> On Fri, Feb 20, 2015 at 11:51:13AM +0100, Jonas Johansson wrote:
>> From: Jonas Johansson <jonas.johansson@westermo.se>
>>
>> This patch will use the DSA hardware bonding support hooks to setup trunking
>> for the Marvell 88E6095 device. The implementation only handles trunking in
>> a single device.
>>
>> Hooks:
>>  .bond_add_group: Add port to a bond group
>>  .bond_del_group: Remove port from a bond group
>>  .bond_attach: Attach/activate port in bond group
>>  .bond_detach: Detach/inactivate port in bond group
>>
>> Procedure to add/remome port from bond group:
>>  Setup trunk learning (Port Association Vector)
>>  Setup loop prevention (VLAN Table)
>>  Setup load balancing (Trunk Mask Load Balance Table)
>>
>> Procedure to attach/detach port:
>>  Change load balancing (Trunk Mask Load Balance Table)
>>
>> Signed-off-by: Jonas Johansson <jonas.johansson@westermo.se>
>> ---
>>  drivers/net/dsa/mv88e6131.c | 254 ++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/net/dsa/mv88e6xxx.h |  14 +++
>>  2 files changed, 268 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mv88e6131.c b/drivers/net/dsa/mv88e6131.c
>> index 2540ef0..3ba7a0c 100644
>> --- a/drivers/net/dsa/mv88e6131.c
>> +++ b/drivers/net/dsa/mv88e6131.c
>> @@ -382,6 +382,256 @@ mv88e6131_get_ethtool_stats(struct dsa_switch *ds,
>>  				    mv88e6131_hw_stats, port, data);
>>  }
>>
>> +/* Trunking */
>> +static int mv88e6131_bond_set_trunk_learning(struct dsa_switch *ds,
>> +					     int *ports, size_t num)
>> +{
>> +	u16 port_vec = 0;
>> +	int ret;
>> +	int i;
>> +
>> +	num = num < MAX_PORTS ? num : MAX_PORTS;
>> +
>> +	for (i = 0; i < num; i++)
>> +		port_vec |= 1 << ports[i];
>> +
>> +	for (i = 0; i < num; i++) {
>> +		ret = mv88e6xxx_reg_read(ds, REG_PORT(ports[i]), REG_PORT_PAV);
>> +		if (ret < 0)
>> +			continue;
>> +		ret = (ret & 0xf800) | (port_vec & 0x7ff);
>> +		mv88e6xxx_reg_write(ds, REG_PORT(ports[i]), REG_PORT_PAV, ret);
>> +	}
>> +
>> +	return 0;
>> +}
>
> The mv886060 seems to have the PAV register. So i guess most of the
> Marvell switches support this. Is there anything specific to the 6131
> here? Could this be moved into mv88x6xxx so other switch drivers can
> use it?
>
I guess you are correct, i'll look into it.
>> +
>> +static int mv88e6131_bond_set_loop_prevention(struct dsa_switch *ds,
>> +					      int *ports, size_t num)
>> +{
>> +	u16 port_vec = 0;
>> +	int ret;
>> +	int i;
>> +
>> +	num = num < MAX_PORTS ? num : MAX_PORTS;
>> +
>> +	for (i = 0; i < num; i++)
>> +		port_vec |= 1 << ports[i];
>> +
>> +	for (i = 0; i < num; i++) {
>> +		ret = mv88e6xxx_reg_read(ds, REG_PORT(ports[i]), REG_PORT_VLAN_MAP);
>> +		if (ret < 0)
>> +			continue;
>> +		ret &= ~port_vec & 0x7ff;
>> +		mv88e6xxx_reg_write(ds, REG_PORT(ports[i]), REG_PORT_VLAN_MAP, ret);
>> +	}
>> +
>> +	return 0;
>> +}
>
> Same question again, anything specific to the 6131 here?
>
I'll check.
>> +static int mv88e6131_wait_trunk_mask(struct dsa_switch *ds)
>> +{
>> +	const int max_retries = 10;
>> +	int retries = 0;
>> +	int ret;
>> +
>> +	/* Wait for update Trunk Mask data */
>> +	while (1) {
>> +		ret = REG_READ(REG_GLOBAL2, REG_TRUNK_MASK);
>> +		if (!(ret & 0x8000))
>> +			return ret;
>> +		if (retries > max_retries) {
>> +			pr_warn("mv88e6131: Timeout waiting for "
>> +				"Trunk Mask Table Register Update\n");
>> +			return -EBUSY;
>> +		}
>> +		retries++;
>> +		usleep_range(20, 50);
>> +	};
>
> This looks a lot like the wait functions what Guenter Roeck added to
> 6352 and i just moved to mv88e6xxx.c. Please use the generic
> infrastructure in the shared code.
>
Seems like the function is doing exactly what I'm looking for.
> Please could you look at all your functions and see what is specific
> to the 6131 and what is generic. Place the generic code into mv88e6xxx
> please so we can all use it.
>
>       Thanks
> 	Andrew
>
Thanks for your review, I'll check if the functions is specific to the 
6131 or if it the code could be moved to mv88e6xxx.c.
On vacation a.t.m, back in a week.

Re: [PATCH net-next 2/2] mv88e6131: bonding: implement single device trunking

[Florian Fainelli]

On 20/02/15 07:26, Andrew Lunn wrote:
> On Fri, Feb 20, 2015 at 11:51:13AM +0100, Jonas Johansson wrote:
> 
> Please could you look at all your functions and see what is specific
> to the 6131 and what is generic. Place the generic code into mv88e6xxx
> please so we can all use it.

Out of curiosity, how many bonding/trunking groups are supported on the
switches models currently in tree?

Let's say there is a limitation: like no more than 2 different bonding
groups on a given physical switch, where would we put this limitation,
down to the switch driver?
-- 
Florian

Re: [PATCH net-next 2/2] mv88e6131: bonding: implement single device trunking

[Andrew Lunn]

On Fri, Mar 06, 2015 at 09:06:50AM -0800, Florian Fainelli wrote:
> On 20/02/15 07:26, Andrew Lunn wrote:
> > On Fri, Feb 20, 2015 at 11:51:13AM +0100, Jonas Johansson wrote:
> > 
> > Please could you look at all your functions and see what is specific
> > to the 6131 and what is generic. Place the generic code into mv88e6xxx
> > please so we can all use it.
> 
> Out of curiosity, how many bonding/trunking groups are supported on the
> switches models currently in tree?
> 
> Let's say there is a limitation: like no more than 2 different bonding
> groups on a given physical switch, where would we put this limitation,
> down to the switch driver?

Hi Florian

It is limited, but it seems to be quite a high limit. I don't have
exact numbers for the devices currently in tree, but i've used an 10
port switch which had a limit something like 8 trunk groups, and a
maximum of 8 ports per trunk.

I suspect we would put the resource tracking in the shared mv88e6xxx
code, and configure it with the limitations from the specific chip
driver.

However, does SF2 have similar trunking capabilities, and limits? Does
it make sense to have this at a higher level so it can be used by all
drivers?

	Andrew

Re: [PATCH net-next 2/2] mv88e6131: bonding: implement single device trunking

[Florian Fainelli]

On 06/03/15 11:23, Andrew Lunn wrote:
> On Fri, Mar 06, 2015 at 09:06:50AM -0800, Florian Fainelli wrote:
>> On 20/02/15 07:26, Andrew Lunn wrote:
>>> On Fri, Feb 20, 2015 at 11:51:13AM +0100, Jonas Johansson wrote:
>>>
>>> Please could you look at all your functions and see what is specific
>>> to the 6131 and what is generic. Place the generic code into mv88e6xxx
>>> please so we can all use it.
>>
>> Out of curiosity, how many bonding/trunking groups are supported on the
>> switches models currently in tree?
>>
>> Let's say there is a limitation: like no more than 2 different bonding
>> groups on a given physical switch, where would we put this limitation,
>> down to the switch driver?
> 
> Hi Florian
> 
> It is limited, but it seems to be quite a high limit. I don't have
> exact numbers for the devices currently in tree, but i've used an 10
> port switch which had a limit something like 8 trunk groups, and a
> maximum of 8 ports per trunk.
> 
> I suspect we would put the resource tracking in the shared mv88e6xxx
> code, and configure it with the limitations from the specific chip
> driver.
> 
> However, does SF2 have similar trunking capabilities, and limits? Does
> it make sense to have this at a higher level so it can be used by all
> drivers?

Most Broadcom switches, either SF2 or roboswitch (b53) have a limit of 2
trunking groups, without limitations on the number of ports included in
any of these two groups.

The larger question is once we start advertising capabilities, where
does that stop, right? It would probably be simpler for now to e.g:
allow 2 trunking groups to be configured, and when trying to configure a
3rd one, return -ENOSPC and act upon that to either take the software
slow path (which is probably not possible) or just return a hard error
condition.
-- 
Florian

Re: [PATCH net-next 2/2] mv88e6131: bonding: implement single device trunking

[Andrew Lunn]

Hi Florian

> Most Broadcom switches, either SF2 or roboswitch (b53) have a limit of 2
> trunking groups, without limitations on the number of ports included in
> any of these two groups.

O.K, so maybe we want the basic resource management in the DSA layer,
not the switch drivers.
 
> The larger question is once we start advertising capabilities, where
> does that stop, right? It would probably be simpler for now to e.g:
> allow 2 trunking groups to be configured, and when trying to configure a
> 3rd one, return -ENOSPC and act upon that to either take the software
> slow path (which is probably not possible) or just return a hard error
> condition.

This is more than a DSA question. It applies to all the hardware
acceleration being discussed at the moment. As you hinted to above, i
suppose we have two different situations:

1) We can fall back to a software slow path.

2) There is no software fallback, we have to error out, and it would
   be nice to have a well defined error code for out of hardware
   resources.

We also should think about how we tell user space we have fallen back
to a slow path. I'm sure users want to know why it works, but works
much slower.

     Andrew

Re: [PATCH net-next 2/2] mv88e6131: bonding: implement single device trunking

[Scott Feldman]

On Fri, Mar 6, 2015 at 1:47 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> Hi Florian
>
>> Most Broadcom switches, either SF2 or roboswitch (b53) have a limit of 2
>> trunking groups, without limitations on the number of ports included in
>> any of these two groups.
>
> O.K, so maybe we want the basic resource management in the DSA layer,
> not the switch drivers.
>
>> The larger question is once we start advertising capabilities, where
>> does that stop, right? It would probably be simpler for now to e.g:
>> allow 2 trunking groups to be configured, and when trying to configure a
>> 3rd one, return -ENOSPC and act upon that to either take the software
>> slow path (which is probably not possible) or just return a hard error
>> condition.
>
> This is more than a DSA question. It applies to all the hardware
> acceleration being discussed at the moment. As you hinted to above, i
> suppose we have two different situations:
>
> 1) We can fall back to a software slow path.
>
> 2) There is no software fallback, we have to error out, and it would
>    be nice to have a well defined error code for out of hardware
>    resources.
>
> We also should think about how we tell user space we have fallen back
> to a slow path. I'm sure users want to know why it works, but works
> much slower.

For the general hardware acceleration of bonds, my thoughts for switchdev are:

(Assume 802.3ad LAG setup for discussion sake, other bonding modes are similar).

1. The driver has access to port membership notification via netevent,
so driver knows which ports are in which bonds.  This notification is
passive; there is no way to signal to user that when port was put in a
bond if it was programmed into a device LAG group or not.  It's
totally up to the driver and device resources to make that decision.

2. The driver can know port active status via netevent as well.  If
device has the port in a LAG group, then reflect the active status
down to device port.  Again, a passive notification.

3. CPU-originating egress traffic would be LAGed by bonding (or team)
driver.  (This is true regardless if the device LAG group was setup or
not).

4a. If device LAG group setup, forwarded traffic would be LAG egressed
by device (fast path).

4b. If no device LAG group, ingress traffic is sent to CPU (slow path)
for bonding (or team) to LAG egress.

So software fall-back (4b) is the default case if driver/device can't
setup LAG group for forwarding.

So the question is: how does user know which bonds are accelerated?
So far, we've used the label "external" to mark L2 bridge FDB which
are offloaded to the device and "external" to mark L3 routes offloaded
to the device.  Do we mark bonds as "external" if the LAG path is
offloaded to the device?

-scott

Re: [PATCH net-next 2/2] mv88e6131: bonding: implement single device trunking

[Jiri Pirko]

Fri, Mar 06, 2015 at 11:43:55PM CET, sfeldma@gmail.com wrote:
>On Fri, Mar 6, 2015 at 1:47 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> Hi Florian
>>
>>> Most Broadcom switches, either SF2 or roboswitch (b53) have a limit of 2
>>> trunking groups, without limitations on the number of ports included in
>>> any of these two groups.
>>
>> O.K, so maybe we want the basic resource management in the DSA layer,
>> not the switch drivers.
>>
>>> The larger question is once we start advertising capabilities, where
>>> does that stop, right? It would probably be simpler for now to e.g:
>>> allow 2 trunking groups to be configured, and when trying to configure a
>>> 3rd one, return -ENOSPC and act upon that to either take the software
>>> slow path (which is probably not possible) or just return a hard error
>>> condition.
>>
>> This is more than a DSA question. It applies to all the hardware
>> acceleration being discussed at the moment. As you hinted to above, i
>> suppose we have two different situations:
>>
>> 1) We can fall back to a software slow path.
>>
>> 2) There is no software fallback, we have to error out, and it would
>>    be nice to have a well defined error code for out of hardware
>>    resources.
>>
>> We also should think about how we tell user space we have fallen back
>> to a slow path. I'm sure users want to know why it works, but works
>> much slower.
>
>For the general hardware acceleration of bonds, my thoughts for switchdev are:
>
>(Assume 802.3ad LAG setup for discussion sake, other bonding modes are similar).
>
>1. The driver has access to port membership notification via netevent,
>so driver knows which ports are in which bonds.  This notification is
>passive; there is no way to signal to user that when port was put in a
>bond if it was programmed into a device LAG group or not.  It's
>totally up to the driver and device resources to make that decision.

Exactly. The fact if another group can be added or not should be decided
and handled by driver. The driver then notifies higher layers using
switchdev notifier event.
 
>
>2. The driver can know port active status via netevent as well.  If
>device has the port in a LAG group, then reflect the active status
>down to device port.  Again, a passive notification.
>
>3. CPU-originating egress traffic would be LAGed by bonding (or team)
>driver.  (This is true regardless if the device LAG group was setup or
>not).
>
>4a. If device LAG group setup, forwarded traffic would be LAG egressed
>by device (fast path).
>
>4b. If no device LAG group, ingress traffic is sent to CPU (slow path)
>for bonding (or team) to LAG egress.
>
>So software fall-back (4b) is the default case if driver/device can't
>setup LAG group for forwarding.
>
>So the question is: how does user know which bonds are accelerated?
>So far, we've used the label "external" to mark L2 bridge FDB which
>are offloaded to the device and "external" to mark L3 routes offloaded
>to the device.  Do we mark bonds as "external" if the LAG path is
>offloaded to the device?

I believe that this is the way to go. Introduce this flag for bond and
team to signal user if LAG is offloaded or not.


>
>-scott

Re: [PATCH net-next 2/2] mv88e6131: bonding: implement single device trunking

[John Fastabend]

On 03/07/2015 06:38 AM, Jiri Pirko wrote:
> Fri, Mar 06, 2015 at 11:43:55PM CET, sfeldma@gmail.com wrote:
>> On Fri, Mar 6, 2015 at 1:47 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>> Hi Florian
>>>
>>>> Most Broadcom switches, either SF2 or roboswitch (b53) have a limit of 2
>>>> trunking groups, without limitations on the number of ports included in
>>>> any of these two groups.
>>>
>>> O.K, so maybe we want the basic resource management in the DSA layer,
>>> not the switch drivers.
>>>
>>>> The larger question is once we start advertising capabilities, where
>>>> does that stop, right? It would probably be simpler for now to e.g:
>>>> allow 2 trunking groups to be configured, and when trying to configure a
>>>> 3rd one, return -ENOSPC and act upon that to either take the software
>>>> slow path (which is probably not possible) or just return a hard error
>>>> condition.
>>>
>>> This is more than a DSA question. It applies to all the hardware
>>> acceleration being discussed at the moment. As you hinted to above, i
>>> suppose we have two different situations:
>>>
>>> 1) We can fall back to a software slow path.
>>>
>>> 2) There is no software fallback, we have to error out, and it would
>>>     be nice to have a well defined error code for out of hardware
>>>     resources.
>>>

I have a (3) case where we don't ever want to fall back to slow path
even if it is possible and (4) we don't ever want to offload the
operation even when it is possible. Having to program the device then
unwind it from user space seems a bit error prone and ugly. At some
point I think we will have to handle these cases its probably fine to
get the transparent offload working as a start though. Specifically,
talking about LAG I'm not sure of the use case for (4) but in
general it is useful.

>>> We also should think about how we tell user space we have fallen back
>>> to a slow path. I'm sure users want to know why it works, but works
>>> much slower.
>>
>> For the general hardware acceleration of bonds, my thoughts for switchdev are:
>>
>> (Assume 802.3ad LAG setup for discussion sake, other bonding modes are similar).
>>
>> 1. The driver has access to port membership notification via netevent,
>> so driver knows which ports are in which bonds.  This notification is
>> passive; there is no way to signal to user that when port was put in a
>> bond if it was programmed into a device LAG group or not.  It's
>> totally up to the driver and device resources to make that decision.
>
> Exactly. The fact if another group can be added or not should be decided
> and handled by driver. The driver then notifies higher layers using
> switchdev notifier event.

hmm same point I need to be able to know ahead of time if it is going to
succeed or fail. I think we can extend this in two ways one add an
explicit flag to say 'add this to hardware or fail' or 'never offload
this' although for LAG setup this might be challenging because the
notification is passive as noted. The other way to do this is to
sufficient exposure of the hardware model and resources so software
can predict a priori that the LAG setup will be accelerated. I think we
need both...

My only point here is eventually management of the system is challenging
if its entirely a driver decision at least in many cases where we have
intelligent agents managing the network. But sure get the simple case
working as a start.

>
>>
>> 2. The driver can know port active status via netevent as well.  If
>> device has the port in a LAG group, then reflect the active status
>> down to device port.  Again, a passive notification.
>>
>> 3. CPU-originating egress traffic would be LAGed by bonding (or team)
>> driver.  (This is true regardless if the device LAG group was setup or
>> not).
>>
>> 4a. If device LAG group setup, forwarded traffic would be LAG egressed
>> by device (fast path).
>>
>> 4b. If no device LAG group, ingress traffic is sent to CPU (slow path)
>> for bonding (or team) to LAG egress.
>>
>> So software fall-back (4b) is the default case if driver/device can't
>> setup LAG group for forwarding.
>>
>> So the question is: how does user know which bonds are accelerated?
>> So far, we've used the label "external" to mark L2 bridge FDB which
>> are offloaded to the device and "external" to mark L3 routes offloaded
>> to the device.  Do we mark bonds as "external" if the LAG path is
>> offloaded to the device?
>
> I believe that this is the way to go. Introduce this flag for bond and
> team to signal user if LAG is offloaded or not.
>
>
>>
>> -scott
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
John Fastabend         Intel Corporation

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

[Jiri Pirko]

Fri, Feb 20, 2015 at 11:51:11AM CET, jonasj76@gmail.com wrote:
>From: Jonas Johansson <jonas.johansson@westermo.se>
>
>This patchset will implement hardware bonding support for the DSA driver and
>the Marvell 88E6095 device.
>
>Jonas Johansson (2):
>  dsa: bonding: implement HW bonding
>  mv88e6131: bonding: implement single device trunking
>

You should use scripts/get_maintainer.pl script and cc all relevant
people...


> drivers/net/dsa/mv88e6131.c | 254 ++++++++++++++++++++++++++++++++++++++++++++
> drivers/net/dsa/mv88e6xxx.h |  14 +++
> 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 ++++
> 8 files changed, 377 insertions(+)
>
>-- 
>2.1.0
>
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html