[RFC net-next 0/5] net: dsa: LAG support

[RFC net-next 0/5] net: dsa: LAG support

[Florian Fainelli]

Hi all,

This patch series is sent as RFC since I have only been able to test LAG
with dsa-loop and not with real HW yet (that should be tomorrow). I also
looked at how the Marvell DSDT API is defined for adding ports to "trunk"
groups and the API proposed here should work there too. Can't speak about
QCA, Mediatek or KSZ switches though.

Few open questions that may need solving now or later:

- on Broadcom switches, we should allow enslaving a port as a LAG group
  member if its speed does not match that of the other members of the group

- not sure what to do with a switch fabric, naively, if adding two ports
  of two distinct switches as a LAG group, we may have to propagate that
  to "dsa" cross-chip interfaces as well

Thanks!

Florian Fainelli (5):
  net: dsa: Add infrastructure to support LAG
  net: dsa: b53: Define MAC trunking/bonding registers
  net: dsa: b53: Add support for LAG
  net: dsa: bcm_sf2: Add support for LAG
  net: dsa: loop: Add support for LAG

 drivers/net/dsa/b53/b53_common.c |  94 ++++++++++++++++++++++-
 drivers/net/dsa/b53/b53_priv.h   |   6 ++
 drivers/net/dsa/b53/b53_regs.h   |  18 +++++
 drivers/net/dsa/bcm_sf2.c        |   3 +
 drivers/net/dsa/dsa_loop.c       |  54 +++++++++++++-
 include/net/dsa.h                |  25 +++++++
 net/dsa/dsa2.c                   |  12 +++
 net/dsa/dsa_priv.h               |   7 ++
 net/dsa/port.c                   |  92 +++++++++++++++++++++++
 net/dsa/slave.c                  | 157 ++++++++++++++++++++++++++++++++++++---
 net/dsa/switch.c                 |  30 ++++++++
 11 files changed, 484 insertions(+), 14 deletions(-)

-- 
2.11.0

[RFC net-next 1/5] net: dsa: Add infrastructure to support LAG

[Florian Fainelli]

Add the necessary logic to support network device events targetting LAG events,
this is loosely inspired from mlxsw/spectrum.c.

In the process we change dsa_slave_changeupper() to be more generic and be called
from both LAG events as well as normal bridge enslaving events paths.

The DSA layer takes care of managing the LAG group identifiers, how many LAGs
may be supported by a switch, and how many members per LAG are supported by a
switch device. When a LAG group is identified, the port is then configured to
be a part of that group. When a LAG group no longer has any users, we remove it
and we tell the drivers whether it is safe to disable trunking altogether.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/net/dsa.h  |  25 +++++++++
 net/dsa/dsa2.c     |  12 ++++
 net/dsa/dsa_priv.h |   7 +++
 net/dsa/port.c     |  92 +++++++++++++++++++++++++++++++
 net/dsa/slave.c    | 157 +++++++++++++++++++++++++++++++++++++++++++++++++----
 net/dsa/switch.c   |  30 ++++++++++
 6 files changed, 312 insertions(+), 11 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 10dceccd9ce8..247ea58add68 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -182,12 +182,20 @@ struct dsa_port {
 	u8			stp_state;
 	struct net_device	*bridge_dev;
 	struct devlink_port	devlink_port;
+	u8			lag_id;
+	bool			lagged;
 	/*
 	 * Original copy of the master netdev ethtool_ops
 	 */
 	const struct ethtool_ops *orig_ethtool_ops;
 };
 
+struct dsa_lag_group {
+	/* Used to know when we can disable lag on the switch */
+	unsigned int		ref_count;
+	struct net_device	*lag_dev;
+};
+
 struct dsa_switch {
 	struct device *dev;
 
@@ -242,6 +250,12 @@ struct dsa_switch {
 	/* Number of switch port queues */
 	unsigned int		num_tx_queues;
 
+	/* Number of lag groups */
+	unsigned int		max_lags;
+	struct dsa_lag_group	*lags;
+	/* Number of members per lag group */
+	unsigned int		max_lag_members;
+
 	/* Dynamically allocated ports, keep last */
 	size_t num_ports;
 	struct dsa_port ports[];
@@ -431,6 +445,16 @@ struct dsa_switch_ops {
 					 int port, struct net_device *br);
 	void	(*crosschip_bridge_leave)(struct dsa_switch *ds, int sw_index,
 					  int port, struct net_device *br);
+
+	/*
+	 * Link aggregation
+	 */
+	bool	(*port_lag_member)(struct dsa_switch *ds, int port, u8 lag_id);
+	int	(*port_lag_join)(struct dsa_switch *ds, int port, u8 lag_id);
+	void	(*port_lag_leave)(struct dsa_switch *ds, int port, u8 lag_id,
+				  bool lag_disable);
+	int	(*port_lag_change)(struct dsa_switch *ds, int port,
+				   struct netdev_lag_lower_state_info *info);
 };
 
 struct dsa_switch_driver {
@@ -455,6 +479,7 @@ static inline bool netdev_uses_dsa(struct net_device *dev)
 }
 
 struct dsa_switch *dsa_switch_alloc(struct device *dev, size_t n);
+int dsa_switch_alloc_lags(struct dsa_switch *ds, size_t n);
 void dsa_unregister_switch(struct dsa_switch *ds);
 int dsa_register_switch(struct dsa_switch *ds);
 #ifdef CONFIG_PM_SLEEP
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 54ed054777bd..dddf8128ba04 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -813,6 +813,18 @@ struct dsa_switch *dsa_switch_alloc(struct device *dev, size_t n)
 }
 EXPORT_SYMBOL_GPL(dsa_switch_alloc);
 
+int dsa_switch_alloc_lags(struct dsa_switch *ds, size_t n)
+{
+	ds->max_lags = n;
+	ds->lags = devm_kcalloc(ds->dev, ds->max_lags,
+			        sizeof(*ds->lags), GFP_KERNEL);
+	if (!ds->lags)
+		return -ENOMEM;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dsa_switch_alloc_lags);
+
 int dsa_register_switch(struct dsa_switch *ds)
 {
 	int err;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 2850077cc9cc..0bd964bd9642 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -159,6 +159,11 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 		      struct switchdev_trans *trans);
 int dsa_port_vlan_del(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan);
+int dsa_port_lag_join(struct dsa_port *dp, struct net_device *lag_dev);
+int dsa_port_lag_leave(struct dsa_port *dp, struct net_device *lag_dev);
+int dsa_port_lag_change(struct dsa_port *dp,
+			struct netdev_lag_lower_state_info *info);
+
 /* slave.c */
 extern const struct dsa_device_ops notag_netdev_ops;
 void dsa_slave_mii_bus_init(struct dsa_switch *ds);
@@ -170,6 +175,8 @@ int dsa_slave_register_notifier(void);
 void dsa_slave_unregister_notifier(void);
 
 /* switch.c */
+int dsa_switch_lag_get_index(struct dsa_switch *ds, struct net_device *lag_dev,
+			     u8 *lag_id);
 int dsa_switch_register_notifier(struct dsa_switch *ds);
 void dsa_switch_unregister_notifier(struct dsa_switch *ds);
 
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 72c8dbd3d3f2..d62fa7bfab4b 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -264,3 +264,95 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 
 	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 }
+
+static int dsa_port_lag_member(struct dsa_port *dp, u8 lag_id)
+{
+	struct dsa_switch *ds = dp->ds;
+	int err = -EOPNOTSUPP;
+	unsigned int i;
+
+	if (!ds->ops->port_lag_member && !ds->max_lag_members)
+		return err;
+
+	for (i = 0; i < ds->max_lag_members; i++) {
+		if (!ds->ops->port_lag_member(ds, i, lag_id)) {
+			return 0;
+		}
+	}
+
+	return -EBUSY;
+}
+
+int dsa_port_lag_join(struct dsa_port *dp, struct net_device *lag_dev)
+{
+	struct dsa_switch *ds = dp->ds;
+	struct dsa_lag_group *lag;
+	int err = -EOPNOTSUPP;
+	u8 lag_id;
+
+	if (!ds->ops->port_lag_join)
+		return err;
+
+	/* Obtain a new lag identifier */
+	err = dsa_switch_lag_get_index(ds, lag_dev, &lag_id);
+	if (err)
+		return err;
+
+	/* Create a lag group if non-existent */
+	lag = &ds->lags[lag_id];
+	if (!lag->ref_count)
+		lag->lag_dev = lag_dev;
+
+	err = dsa_port_lag_member(dp, lag_id);
+	if (err)
+		return err;
+
+	err = ds->ops->port_lag_join(ds, dp->index, lag_id);
+	if (err)
+		return err;
+
+	dp->lag_id = lag_id;
+	dp->lagged = true;
+	lag->ref_count++;
+
+	return err;
+}
+
+int dsa_port_lag_leave(struct dsa_port *dp, struct net_device *lag_dev)
+{
+	struct dsa_switch *ds = dp->ds;
+	struct dsa_lag_group *lag;
+	bool lag_disable = false;
+	int err = -EOPNOTSUPP;
+	u8 lag_id;
+
+	if (!ds->ops->port_lag_join)
+		return err;
+
+	if (!dp->lagged)
+		return 0;
+
+	lag_id = dp->lag_id;
+	lag = &ds->lags[lag_id];
+	WARN_ON(lag->ref_count == 0);
+
+	if (lag->ref_count == 1)
+		lag_disable = true;
+
+	ds->ops->port_lag_leave(ds, dp->index, lag_id, lag_disable);
+	dp->lagged = false;
+	lag->ref_count--;
+
+	return err;
+}
+
+int dsa_port_lag_change(struct dsa_port *dp,
+			struct netdev_lag_lower_state_info *info)
+{
+	struct dsa_switch *ds = dp->ds;
+
+	if (!ds->ops->port_lag_change)
+		return -EOPNOTSUPP;
+
+	return ds->ops->port_lag_change(ds, dp->index, info);
+}
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 4b634db05cee..b64320aa20f1 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1216,36 +1216,171 @@ static bool dsa_slave_dev_check(struct net_device *dev)
 	return dev->netdev_ops == &dsa_slave_netdev_ops;
 }
 
-static int dsa_slave_changeupper(struct net_device *dev,
-				 struct netdev_notifier_changeupper_info *info)
+static bool dsa_slave_lag_check(struct net_device *dev, struct net_device *lag_dev,
+				struct netdev_lag_upper_info *lag_upper_info)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	u8 lag_id;
+
+	/* No more lag identifiers available or already in use */
+	if (dsa_switch_lag_get_index(p->dp->ds, lag_dev, &lag_id) != 0)
+		return false;
+
+	if (lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH)
+		return false;
+
+	return true;
+}
+
+static int dsa_slave_changeupper_bridge(struct net_device *dev,
+					struct netdev_notifier_changeupper_info *info)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_port *dp = p->dp;
 	int err = NOTIFY_DONE;
 
-	if (netif_is_bridge_master(info->upper_dev)) {
-		if (info->linking) {
-			err = dsa_port_bridge_join(dp, info->upper_dev);
-			err = notifier_from_errno(err);
-		} else {
-			dsa_port_bridge_leave(dp, info->upper_dev);
-			err = NOTIFY_OK;
-		}
+	if (info->linking) {
+		err = dsa_port_bridge_join(dp, info->upper_dev);
+		err = notifier_from_errno(err);
+	} else {
+		dsa_port_bridge_leave(dp, info->upper_dev);
+		err = NOTIFY_OK;
+	}
+
+	return err;
+}
+
+static int dsa_slave_changeupper_lag(struct net_device *dev,
+				     struct netdev_notifier_changeupper_info *info)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_port *dp = p->dp;
+	int err = NOTIFY_DONE;
+
+	if (info->linking) {
+		err = dsa_port_lag_join(dp, info->upper_dev);
+		err = notifier_from_errno(err);
+	} else {
+		err = dsa_port_lag_leave(dp, info->upper_dev);
+		err = NOTIFY_OK;
+	}
+
+	return err;
+}
+
+static int dsa_slave_upper_event(struct net_device *lower_dev,
+				 struct net_device *slave_dev, unsigned long event,
+				 void *ptr)
+{
+	struct netdev_notifier_changeupper_info *info;
+	struct net_device *upper_dev;
+	struct dsa_slave_priv *p;
+	int err = 0;
+
+	info = ptr;
+	p = netdev_priv(slave_dev);
+
+	switch (event) {
+	case NETDEV_PRECHANGEUPPER:
+		upper_dev = info->upper_dev;
+		if (!is_vlan_dev(upper_dev) &&
+		    !netif_is_lag_master(upper_dev) &&
+		    !netif_is_bridge_master(upper_dev))
+			return -EINVAL;
+
+		if (!info->linking)
+			break;
+
+		if (netdev_has_any_upper_dev(upper_dev))
+			return -EINVAL;
+
+		if (netif_is_lag_master(upper_dev) &&
+		    !dsa_slave_lag_check(lower_dev, upper_dev, info->upper_info))
+			return -EINVAL;
+
+		break;
+	case NETDEV_CHANGEUPPER:
+		upper_dev = info->upper_dev;
+		if (netif_is_bridge_master(upper_dev))
+			err = dsa_slave_changeupper_bridge(lower_dev, info);
+		else if (netif_is_lag_master(upper_dev))
+			err = dsa_slave_changeupper_lag(lower_dev, info);
+		break;
 	}
 
 	return err;
 }
 
+static int dsa_slave_lower_event(struct net_device *dev,
+				 unsigned long event, void *ptr)
+{
+	struct netdev_notifier_changelowerstate_info *info;
+	struct dsa_slave_priv *p;
+	int err;
+
+	p = netdev_priv(dev);
+	info = ptr;
+
+	switch (event) {
+	case NETDEV_CHANGELOWERSTATE:
+		if (netif_is_lag_port(dev) && p->dp->lagged) {
+			err = dsa_port_lag_change(p->dp, info->lower_state_info);
+			if (err)
+				netdev_err(dev, "Failed to reflect LAG\n");
+		}
+		break;
+	}
+
+	return 0;
+}
+
+static int dsa_slave_upper_lower_event(struct net_device *lower_dev,
+				       struct net_device *slave_dev,
+				       unsigned long event, void *ptr)
+{
+	switch (event) {
+	case NETDEV_PRECHANGEUPPER:
+	case NETDEV_CHANGEUPPER:
+		return dsa_slave_upper_event(lower_dev, slave_dev, event, ptr);
+	case NETDEV_CHANGELOWERSTATE:
+		return dsa_slave_lower_event(slave_dev, event, ptr);
+	}
+
+	return NOTIFY_OK;
+}
+
+static int dsa_slave_lag_event(struct net_device *lag_dev,
+				unsigned long event, void *ptr)
+{
+	struct net_device *dev;
+	struct list_head *iter;
+	int err;
+
+	netdev_for_each_lower_dev(lag_dev, dev, iter) {
+		if (dsa_slave_dev_check(dev)) {
+			err = dsa_slave_upper_lower_event(lag_dev, dev,
+							  event, ptr);
+			if (err)
+				return err;
+		}
+	}
+
+	return NOTIFY_OK;
+}
+
 static int dsa_slave_netdevice_event(struct notifier_block *nb,
 				     unsigned long event, void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
+	if (netif_is_lag_master(dev))
+		return dsa_slave_lag_event(dev, event, ptr);
+
 	if (!dsa_slave_dev_check(dev))
 		return NOTIFY_DONE;
 
 	if (event == NETDEV_CHANGEUPPER)
-		return dsa_slave_changeupper(dev, ptr);
+		return dsa_slave_upper_event(dev, dev, event, ptr);
 
 	return NOTIFY_DONE;
 }
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index e6c06aa349a6..9ce1b25bf197 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -252,6 +252,36 @@ static int dsa_switch_event(struct notifier_block *nb,
 	return notifier_from_errno(err);
 }
 
+int dsa_switch_lag_get_index(struct dsa_switch *ds, struct net_device *lag_dev,
+			     u8 *lag_id)
+{
+	struct dsa_lag_group *lag;
+	int free_lag_idx = -1;
+	unsigned int i;
+
+	if (!ds->max_lags)
+		return -EOPNOTSUPP;
+
+	for (i = 0; i < ds->max_lags; i++) {
+		lag = &ds->lags[i];
+		if (lag->ref_count) {
+			if (lag->lag_dev == lag_dev) {
+				*lag_id = i;
+				return 0;
+			}
+		} else if (free_lag_idx < 0) {
+			free_lag_idx = i;
+		}
+	}
+
+	if (free_lag_idx < 0)
+		return -EBUSY;
+
+	*lag_id = free_lag_idx;
+
+	return 0;
+}
+
 int dsa_switch_register_notifier(struct dsa_switch *ds)
 {
 	ds->nb.notifier_call = dsa_switch_event;
-- 
2.11.0

Re: [RFC net-next 1/5] net: dsa: Add infrastructure to support LAG

[Andrew Lunn]

On Sun, Oct 01, 2017 at 12:46:35PM -0700, Florian Fainelli wrote:
> Add the necessary logic to support network device events targetting LAG events,
> this is loosely inspired from mlxsw/spectrum.c.
> 
> In the process we change dsa_slave_changeupper() to be more generic and be called
> from both LAG events as well as normal bridge enslaving events paths.
> 
> The DSA layer takes care of managing the LAG group identifiers, how many LAGs
> may be supported by a switch, and how many members per LAG are supported by a
> switch device. When a LAG group is identified, the port is then configured to
> be a part of that group. When a LAG group no longer has any users, we remove it
> and we tell the drivers whether it is safe to disable trunking altogether.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  include/net/dsa.h  |  25 +++++++++
>  net/dsa/dsa2.c     |  12 ++++
>  net/dsa/dsa_priv.h |   7 +++
>  net/dsa/port.c     |  92 +++++++++++++++++++++++++++++++
>  net/dsa/slave.c    | 157 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  net/dsa/switch.c   |  30 ++++++++++
>  6 files changed, 312 insertions(+), 11 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 10dceccd9ce8..247ea58add68 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -182,12 +182,20 @@ struct dsa_port {
>  	u8			stp_state;
>  	struct net_device	*bridge_dev;
>  	struct devlink_port	devlink_port;
> +	u8			lag_id;
> +	bool			lagged;
>  	/*
>  	 * Original copy of the master netdev ethtool_ops
>  	 */
>  	const struct ethtool_ops *orig_ethtool_ops;
>  };
>  
> +struct dsa_lag_group {
> +	/* Used to know when we can disable lag on the switch */
> +	unsigned int		ref_count;

Hi Florian

In what contexts is ref_count manipulated. Normally you use would
refcounf_t and the operations in linux/refcount.h. But if you know
there is some other protection, e.g. rtnl, an unsigned int is O.K.
Maybe scatter some assert_RTNL() in the code?

> +static bool dsa_slave_lag_check(struct net_device *dev, struct net_device *lag_dev,
> +				struct netdev_lag_upper_info *lag_upper_info)
> +{
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	u8 lag_id;
> +
> +	/* No more lag identifiers available or already in use */
> +	if (dsa_switch_lag_get_index(p->dp->ds, lag_dev, &lag_id) != 0)
> +		return false;
> +
> +	if (lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH)
> +		return false;

I wounder if the driver needs to decide this? Can different hardware
support different tx_types?

	Andrew

Re: [RFC net-next 1/5] net: dsa: Add infrastructure to support LAG

[Ido Schimmel]

On Mon, Oct 02, 2017 at 04:03:27AM +0200, Andrew Lunn wrote:
> On Sun, Oct 01, 2017 at 12:46:35PM -0700, Florian Fainelli wrote:
> > +static bool dsa_slave_lag_check(struct net_device *dev, struct net_device *lag_dev,
> > +				struct netdev_lag_upper_info *lag_upper_info)
> > +{
> > +	struct dsa_slave_priv *p = netdev_priv(dev);
> > +	u8 lag_id;
> > +
> > +	/* No more lag identifiers available or already in use */
> > +	if (dsa_switch_lag_get_index(p->dp->ds, lag_dev, &lag_id) != 0)
> > +		return false;
> > +
> > +	if (lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH)
> > +		return false;
> 
> I wounder if the driver needs to decide this? Can different hardware
> support different tx_types?

FWIW, the same check exists in mlxsw, but maybe other devices support
more methods, so I think it makes sense to have the driver decide this.

Re: [RFC net-next 1/5] net: dsa: Add infrastructure to support LAG

[Florian Fainelli]

On 10/01/2017 07:03 PM, Andrew Lunn wrote:
> On Sun, Oct 01, 2017 at 12:46:35PM -0700, Florian Fainelli wrote:
>> Add the necessary logic to support network device events targetting LAG events,
>> this is loosely inspired from mlxsw/spectrum.c.
>>
>> In the process we change dsa_slave_changeupper() to be more generic and be called
>> from both LAG events as well as normal bridge enslaving events paths.
>>
>> The DSA layer takes care of managing the LAG group identifiers, how many LAGs
>> may be supported by a switch, and how many members per LAG are supported by a
>> switch device. When a LAG group is identified, the port is then configured to
>> be a part of that group. When a LAG group no longer has any users, we remove it
>> and we tell the drivers whether it is safe to disable trunking altogether.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  include/net/dsa.h  |  25 +++++++++
>>  net/dsa/dsa2.c     |  12 ++++
>>  net/dsa/dsa_priv.h |   7 +++
>>  net/dsa/port.c     |  92 +++++++++++++++++++++++++++++++
>>  net/dsa/slave.c    | 157 +++++++++++++++++++++++++++++++++++++++++++++++++----
>>  net/dsa/switch.c   |  30 ++++++++++
>>  6 files changed, 312 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index 10dceccd9ce8..247ea58add68 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -182,12 +182,20 @@ struct dsa_port {
>>  	u8			stp_state;
>>  	struct net_device	*bridge_dev;
>>  	struct devlink_port	devlink_port;
>> +	u8			lag_id;
>> +	bool			lagged;
>>  	/*
>>  	 * Original copy of the master netdev ethtool_ops
>>  	 */
>>  	const struct ethtool_ops *orig_ethtool_ops;
>>  };
>>  
>> +struct dsa_lag_group {
>> +	/* Used to know when we can disable lag on the switch */
>> +	unsigned int		ref_count;
> 
> Hi Florian
> 
> In what contexts is ref_count manipulated. Normally you use would
> refcounf_t and the operations in linux/refcount.h. But if you know
> there is some other protection, e.g. rtnl, an unsigned int is O.K.
> Maybe scatter some assert_RTNL() in the code?

Hi Andrew,

This is called with rtnl held, but this is a good point. In fact, I
don't think we need the reference count at all, what I am going to
propose now is that we just maintain a bitmask of port members per lag
group (along with the reference to the lag device) and when the hamming
weight of that bitmask is 1, that means we were removing the lat port of
the LAG group and we can stop using that LAG group. This also allow us
to remove the port_lag_member operation, since we would be maintaining
that at the DSA layer now.

> 
>> +static bool dsa_slave_lag_check(struct net_device *dev, struct net_device *lag_dev,
>> +				struct netdev_lag_upper_info *lag_upper_info)
>> +{
>> +	struct dsa_slave_priv *p = netdev_priv(dev);
>> +	u8 lag_id;
>> +
>> +	/* No more lag identifiers available or already in use */
>> +	if (dsa_switch_lag_get_index(p->dp->ds, lag_dev, &lag_id) != 0)
>> +		return false;
>> +
>> +	if (lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH)
>> +		return false;
> 
> I wounder if the driver needs to decide this? Can different hardware
> support different tx_types?

That is a valid point. For instance, the b53/bcm_sf2 switches can only
do MAC DA and SA, SA only, DA only hashing, but you can't do hashing at
a higher level than L2 addresses, this does appear to be something that
the driver should indeed decide.
-- 
Florian

[RFC net-next 2/5] net: dsa: b53: Define MAC trunking/bonding registers

[Florian Fainelli]

Define the MAC trunking page offset and its register layout to implement
bonding in the next patches.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_regs.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_regs.h b/drivers/net/dsa/b53/b53_regs.h
index 2a9f421680aa..d02e4f7dda10 100644
--- a/drivers/net/dsa/b53/b53_regs.h
+++ b/drivers/net/dsa/b53/b53_regs.h
@@ -44,6 +44,9 @@
 /* Port VLAN Page */
 #define B53_PVLAN_PAGE			0x31
 
+/* Trunking Page */
+#define B53_TRUNK_PAGE			0x32
+
 /* VLAN Registers */
 #define B53_VLAN_PAGE			0x34
 
@@ -360,6 +363,21 @@
 #define B53_JOIN_ALL_VLAN_EN		0x50
 
 /*************************************************************************
+ * Trunking Registers
+ *************************************************************************/
+
+/* MAC Trunking Control Register (8 bit) */
+#define B53_MAC_TRUNK_CTRL		0x00
+#define  TRK_HASH_IDX_DA_SA		0
+#define  TRK_HASH_IDX_DA		1
+#define  TRK_HASH_IDX_SA		2
+#define  TRK_HASH_IDX_MASK		0x3
+#define  MAC_BASE_TRNK_EN		BIT(3)
+
+/* MAC Trunking Group Register (16 bit) */
+#define B53_MAC_TRUNK_GROUP(x)		(0x10 + (x) * 2)
+
+/*************************************************************************
  * 802.1Q Page Registers
  *************************************************************************/
 
-- 
2.11.0

[RFC net-next 3/5] net: dsa: b53: Add support for LAG

[Florian Fainelli]

Add support for LAG in the b53 driver by implementing the port_lag_join,
port_lag_leave and port_lag_member operations. port_lag_change is not supported
since the HW does not let us change anyting regarding tx_enabled or not.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 94 +++++++++++++++++++++++++++++++++++++++-
 drivers/net/dsa/b53/b53_priv.h   |  6 +++
 2 files changed, 99 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index d4ce092def83..e9903947f050 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1603,6 +1603,58 @@ int b53_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e)
 }
 EXPORT_SYMBOL(b53_set_mac_eee);
 
+bool b53_lag_member(struct dsa_switch *ds, int port, u8 lag_id)
+{
+	struct b53_device *dev = ds->priv;
+	u16 reg;
+
+	b53_read16(dev, B53_TRUNK_PAGE, B53_MAC_TRUNK_GROUP(lag_id), &reg);
+
+	return !!(BIT(port) & reg);
+}
+EXPORT_SYMBOL(b53_lag_member);
+
+int b53_lag_join(struct dsa_switch *ds, int port, u8 lag_id)
+{
+	struct b53_device *dev = ds->priv;
+	u8 trunk_ctl;
+	u16 lag;
+
+	/* Program this port and the CPU port in this trunking group */
+	b53_read16(dev, B53_TRUNK_PAGE, B53_MAC_TRUNK_GROUP(lag_id), &lag);
+	lag |= BIT(port);
+	b53_write16(dev, B53_TRUNK_PAGE, B53_MAC_TRUNK_GROUP(lag_id), lag);
+
+	/* Enable MAC DA,SA hashing, enable trunking */
+	b53_read8(dev, B53_TRUNK_PAGE, B53_MAC_TRUNK_CTRL, &trunk_ctl);
+	trunk_ctl &= ~TRK_HASH_IDX_MASK;
+	trunk_ctl |= MAC_BASE_TRNK_EN;
+	b53_write8(dev, B53_TRUNK_PAGE, B53_MAC_TRUNK_CTRL, trunk_ctl);
+
+	return 0;
+}
+EXPORT_SYMBOL(b53_lag_join);
+
+void b53_lag_leave(struct dsa_switch *ds, int port, u8 lag_id, bool lag_disable)
+{
+	struct b53_device *dev = ds->priv;
+	u8 trunk_ctl;
+	u16 lag;
+
+	/* Remove this port from the trunking group */
+	b53_read16(dev, B53_TRUNK_PAGE, B53_MAC_TRUNK_GROUP(lag_id), &lag);
+	lag &= ~BIT(port);
+	b53_write16(dev, B53_TRUNK_PAGE, B53_MAC_TRUNK_GROUP(lag_id), lag);
+
+	/* Disable trunking if the lag group is being removed */
+	if (lag_disable) {
+		b53_read8(dev, B53_TRUNK_PAGE, B53_MAC_TRUNK_CTRL, &trunk_ctl);
+		trunk_ctl &= ~(TRK_HASH_IDX_MASK | MAC_BASE_TRNK_EN);
+		b53_write8(dev, B53_TRUNK_PAGE, B53_MAC_TRUNK_CTRL, trunk_ctl);
+	}
+}
+EXPORT_SYMBOL(b53_lag_leave);
+
 static const struct dsa_switch_ops b53_switch_ops = {
 	.get_tag_protocol	= b53_get_tag_protocol,
 	.setup			= b53_setup,
@@ -1629,6 +1681,9 @@ static const struct dsa_switch_ops b53_switch_ops = {
 	.port_fdb_del		= b53_fdb_del,
 	.port_mirror_add	= b53_mirror_add,
 	.port_mirror_del	= b53_mirror_del,
+	.port_lag_member	= b53_lag_member,
+	.port_lag_join		= b53_lag_join,
+	.port_lag_leave		= b53_lag_leave,
 };
 
 struct b53_chip_data {
@@ -1642,6 +1697,8 @@ struct b53_chip_data {
 	u8 duplex_reg;
 	u8 jumbo_pm_reg;
 	u8 jumbo_size_reg;
+	unsigned int num_lags;
+	unsigned int max_lag_members;
 };
 
 #define B53_VTA_REGS	\
@@ -1681,6 +1738,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM5397_DEVICE_ID,
@@ -1693,6 +1752,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM5398_DEVICE_ID,
@@ -1705,6 +1766,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM53115_DEVICE_ID,
@@ -1717,6 +1780,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM53125_DEVICE_ID,
@@ -1729,6 +1794,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM53128_DEVICE_ID,
@@ -1741,6 +1808,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM63XX_DEVICE_ID,
@@ -1753,6 +1822,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_63XX,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK_63XX,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE_63XX,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM53010_DEVICE_ID,
@@ -1765,6 +1836,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM53011_DEVICE_ID,
@@ -1777,6 +1850,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM53012_DEVICE_ID,
@@ -1789,6 +1864,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM53018_DEVICE_ID,
@@ -1801,6 +1878,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM53019_DEVICE_ID,
@@ -1813,6 +1892,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM58XX_DEVICE_ID,
@@ -1825,6 +1906,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM7445_DEVICE_ID,
@@ -1837,6 +1920,8 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 	{
 		.chip_id = BCM7278_DEVICE_ID,
@@ -1849,11 +1934,14 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+		.num_lags = 2,
+		.max_lag_members = 4,
 	},
 };
 
 static int b53_switch_init(struct b53_device *dev)
 {
+	struct dsa_switch *ds = dev->ds;
 	unsigned int i;
 	int ret;
 
@@ -1872,6 +1960,8 @@ static int b53_switch_init(struct b53_device *dev)
 			dev->cpu_port = chip->cpu_port;
 			dev->num_vlans = chip->vlans;
 			dev->num_arl_entries = chip->arl_entries;
+			dev->num_lags = chip->num_lags;
+			dev->max_lag_members = chip->max_lag_members;
 			break;
 		}
 	}
@@ -1933,7 +2023,9 @@ static int b53_switch_init(struct b53_device *dev)
 			return ret;
 	}
 
-	return 0;
+	ds->max_lag_members = dev->max_lag_members;
+
+	return dsa_switch_alloc_lags(ds, dev->num_lags);
 }
 
 struct b53_device *b53_switch_alloc(struct device *base,
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 603c66d240d8..0f48184c9b54 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -118,6 +118,9 @@ struct b53_device {
 	struct b53_vlan *vlans;
 	unsigned int num_ports;
 	struct b53_port *ports;
+
+	unsigned int num_lags;
+	unsigned int max_lag_members;
 };
 
 #define b53_for_each_port(dev, i) \
@@ -318,5 +321,8 @@ void b53_eee_enable_set(struct dsa_switch *ds, int port, bool enable);
 int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy);
 int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e);
 int b53_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_eee *e);
+bool b53_lag_member(struct dsa_switch *ds, int port, u8 lag_id);
+int b53_lag_join(struct dsa_switch *ds, int port, u8 lag_id);
+void b53_lag_leave(struct dsa_switch *ds, int port, u8 lag_id, bool lag_disable);
 
 #endif
-- 
2.11.0

[RFC net-next 4/5] net: dsa: bcm_sf2: Add support for LAG

[Florian Fainelli]

Utilize the recently introduced b53 LAG operations and use those as-is.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 7aecc98d0a18..41a06fde7510 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -901,6 +901,9 @@ static const struct dsa_switch_ops bcm_sf2_ops = {
 	.set_rxnfc		= bcm_sf2_set_rxnfc,
 	.port_mirror_add	= b53_mirror_add,
 	.port_mirror_del	= b53_mirror_del,
+	.port_lag_member	= b53_lag_member,
+	.port_lag_join		= b53_lag_join,
+	.port_lag_leave		= b53_lag_leave,
 };
 
 struct bcm_sf2_of_data {
-- 
2.11.0

[RFC net-next 5/5] net: dsa: loop: Add support for LAG

[Florian Fainelli]

Implement LAG operations for the DSA loopback/mock-up driver in order to
exercise the DSA core code. This just maintains a software bitmask of ports
belonging to a LAG group, we allow up to 2 LAG groups and up to 2 members per
LAG group.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/dsa_loop.c | 54 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
index d55051abf4ed..776130c69c7f 100644
--- a/drivers/net/dsa/dsa_loop.c
+++ b/drivers/net/dsa/dsa_loop.c
@@ -1,7 +1,7 @@
 /*
  * Distributed Switch Architecture loopback driver
  *
- * Copyright (C) 2016, Florian Fainelli <f.fainelli@gmail.com>
+ * Copyright (C) 2016-2017, Florian Fainelli <f.fainelli@gmail.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -27,6 +27,10 @@ struct dsa_loop_vlan {
 	u16 untagged;
 };
 
+struct dsa_loop_lag {
+	u8 members;
+};
+
 struct dsa_loop_mib_entry {
 	char name[ETH_GSTRING_LEN];
 	unsigned long val;
@@ -52,6 +56,7 @@ struct dsa_loop_port {
 };
 
 #define DSA_LOOP_VLANS	5
+#define DSA_LOOP_LAGS	2
 
 struct dsa_loop_priv {
 	struct mii_bus	*bus;
@@ -60,6 +65,7 @@ struct dsa_loop_priv {
 	struct net_device *netdev;
 	struct dsa_loop_port ports[DSA_MAX_PORTS];
 	u16 pvid;
+	struct dsa_loop_lag lags[DSA_LOOP_LAGS];
 };
 
 static struct phy_device *phydevs[PHY_MAX_ADDR];
@@ -257,6 +263,40 @@ static int dsa_loop_port_vlan_del(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static bool dsa_loop_lag_member(struct dsa_switch *ds, int port, u8 lag_id)
+{
+	struct dsa_loop_priv *priv = ds->priv;
+	struct dsa_loop_lag *lag = &priv->lags[lag_id];
+
+	return !!(BIT(port) & lag->members);
+}
+
+static int dsa_loop_lag_join(struct dsa_switch *ds, int port, u8 lag_id)
+{
+	struct dsa_loop_priv *priv = ds->priv;
+	struct dsa_loop_lag *lag = &priv->lags[lag_id];
+
+	lag->members |= BIT(port);
+
+	dev_dbg(ds->dev, "%s, added port %d to lag: %d\n",
+		__func__, port, lag_id);
+
+	return 0;
+}
+
+static void dsa_loop_lag_leave(struct dsa_switch *ds, int port, u8 lag_id,
+				bool lag_disable)
+{
+	struct dsa_loop_priv *priv = ds->priv;
+	struct dsa_loop_lag *lag = &priv->lags[lag_id];
+
+
+	lag->members &= ~BIT(port);
+
+	dev_dbg(ds->dev, "%s, removed port %d from lag: %d\n",
+		__func__, port, lag_id);
+}
+
 static const struct dsa_switch_ops dsa_loop_driver = {
 	.get_tag_protocol	= dsa_loop_get_protocol,
 	.setup			= dsa_loop_setup,
@@ -273,6 +313,9 @@ static const struct dsa_switch_ops dsa_loop_driver = {
 	.port_vlan_prepare	= dsa_loop_port_vlan_prepare,
 	.port_vlan_add		= dsa_loop_port_vlan_add,
 	.port_vlan_del		= dsa_loop_port_vlan_del,
+	.port_lag_member	= dsa_loop_lag_member,
+	.port_lag_join		= dsa_loop_lag_join,
+	.port_lag_leave		= dsa_loop_lag_leave,
 };
 
 static int dsa_loop_drv_probe(struct mdio_device *mdiodev)
@@ -280,6 +323,7 @@ static int dsa_loop_drv_probe(struct mdio_device *mdiodev)
 	struct dsa_loop_pdata *pdata = mdiodev->dev.platform_data;
 	struct dsa_loop_priv *ps;
 	struct dsa_switch *ds;
+	int ret;
 
 	if (!pdata)
 		return -ENODEV;
@@ -291,6 +335,13 @@ static int dsa_loop_drv_probe(struct mdio_device *mdiodev)
 	if (!ds)
 		return -ENOMEM;
 
+	ds->dev = &mdiodev->dev;
+	ds->max_lag_members = 2;
+
+	ret = dsa_switch_alloc_lags(ds, DSA_LOOP_LAGS);
+	if (ret)
+		return ret;
+
 	ps = devm_kzalloc(&mdiodev->dev, sizeof(*ps), GFP_KERNEL);
 	if (!ps)
 		return -ENOMEM;
@@ -301,7 +352,6 @@ static int dsa_loop_drv_probe(struct mdio_device *mdiodev)
 
 	pdata->cd.netdev[DSA_LOOP_CPU_PORT] = &ps->netdev->dev;
 
-	ds->dev = &mdiodev->dev;
 	ds->ops = &dsa_loop_driver;
 	ds->priv = ps;
 	ps->bus = mdiodev->bus;
-- 
2.11.0

Re: [RFC net-next 0/5] net: dsa: LAG support

[Ido Schimmel]

Hi Florian,

On Sun, Oct 01, 2017 at 12:46:34PM -0700, Florian Fainelli wrote:
> Hi all,
> 
> This patch series is sent as RFC since I have only been able to test LAG
> with dsa-loop and not with real HW yet (that should be tomorrow). I also
> looked at how the Marvell DSDT API is defined for adding ports to "trunk"
> groups and the API proposed here should work there too. Can't speak about
> QCA, Mediatek or KSZ switches though.

Thanks for working on this. I've yet to look at the patches, but I
thought I'll mention a few issues we bumped into with LAG devices:

1) It is possible for users to stack devices on top of the LAG and only
then enslave your port. This means that the underlying driver might not
be aware of all the necessary configuration. It's quite a complicated
problem to solve properly, so we currently forbid enslavements to
devices that already have uppers.

There's also an issue with IP addresses and routes configured on top of
the LAG, but I hope to fix that soon. I don't think you support L3 in
DSA yet, so it shouldn't be a problem for you.

2) Similarly, you're no longer guaranteed to have the bridge do proper
clean up in case you pull a port out of a bridged LAG, so you'll need to
handle that. Any context you store for the bridge port needs to be
destroyed upon the removal of the last port from the LAG.

> Few open questions that may need solving now or later:
> 
> - on Broadcom switches, we should allow enslaving a port as a LAG group
>   member if its speed does not match that of the other members of the group
> 
> - not sure what to do with a switch fabric, naively, if adding two ports
>   of two distinct switches as a LAG group, we may have to propagate that
>   to "dsa" cross-chip interfaces as well

At least in mlxsw case, enslaving switch and non-switch ports to the
same LAG doesn't make sense. Any traffic routed by the switch will only
be load-balanced between the switch ports. One way to solve that is to
forbid such enslavements during NETDEV_PRECHANGEUPPER in case the lower
devices in the adjacency list of the LAG don't belong to the same
switch.

Note that such configurations are bound to fail anyway, as the
non-switch ports will not have `switchdev_ops` configured and thus fail
during __switchdev_port_obj_add() / __switchdev_port_attr_set().

Re: [RFC net-next 0/5] net: dsa: LAG support

[Andrew Lunn]

> > - not sure what to do with a switch fabric, naively, if adding two ports
> >   of two distinct switches as a LAG group, we may have to propagate that
> >   to "dsa" cross-chip interfaces as well
> 
> At least in mlxsw case, enslaving switch and non-switch ports to the
> same LAG doesn't make sense. Any traffic routed by the switch will only
> be load-balanced between the switch ports. One way to solve that is to
> forbid such enslavements during NETDEV_PRECHANGEUPPER in case the lower
> devices in the adjacency list of the LAG don't belong to the same
> switch.
> 
> Note that such configurations are bound to fail anyway, as the
> non-switch ports will not have `switchdev_ops` configured and thus fail
> during __switchdev_port_obj_add() / __switchdev_port_attr_set().

Hi Ido

Here Florian is thinking about the D in DSA. Marvell switches have the
capabilities of building a switch fabric out of multiple
interconnected switches. To switchdev, they appear as a single switch.
switchdev has no idea of the mapping of interfaces to switches, nor
the routing of frames between switches. This all happens in the layers
bellow. The hardware does support LAG members on different switches
within the same fabric. But it requires some additional setup for the
ports which link switches together. We have the same issues with MDB,
where additional setup is required for group members spread over the
switch fabric.

      Andrew

Re: [RFC net-next 0/5] net: dsa: LAG support

[Ido Schimmel]

Hi Andrew,

On Mon, Oct 02, 2017 at 02:59:32PM +0200, Andrew Lunn wrote:
> > > - not sure what to do with a switch fabric, naively, if adding two ports
> > >   of two distinct switches as a LAG group, we may have to propagate that
> > >   to "dsa" cross-chip interfaces as well
> > 
> > At least in mlxsw case, enslaving switch and non-switch ports to the
> > same LAG doesn't make sense. Any traffic routed by the switch will only
> > be load-balanced between the switch ports. One way to solve that is to
> > forbid such enslavements during NETDEV_PRECHANGEUPPER in case the lower
> > devices in the adjacency list of the LAG don't belong to the same
> > switch.
> > 
> > Note that such configurations are bound to fail anyway, as the
> > non-switch ports will not have `switchdev_ops` configured and thus fail
> > during __switchdev_port_obj_add() / __switchdev_port_attr_set().
> 
> Hi Ido
> 
> Here Florian is thinking about the D in DSA. Marvell switches have the
> capabilities of building a switch fabric out of multiple
> interconnected switches. To switchdev, they appear as a single switch.
> switchdev has no idea of the mapping of interfaces to switches, nor
> the routing of frames between switches. This all happens in the layers
> bellow. The hardware does support LAG members on different switches
> within the same fabric. But it requires some additional setup for the
> ports which link switches together. We have the same issues with MDB,
> where additional setup is required for group members spread over the
> switch fabric.

Yes, I understood that. I was simply referring to the more general
problem of any two net devices and how to solve it. Not currently
implemented in mlxsw, but should be necessary for DSA as well.

Agree with your previous mail about keeping it simple for the first
implementation.

Re: [RFC net-next 0/5] net: dsa: LAG support

[Andrew Lunn]

> - not sure what to do with a switch fabric, naively, if adding two ports
>   of two distinct switches as a LAG group, we may have to propagate that
>   to "dsa" cross-chip interfaces as well

Hi Florian

Marvell switches do support this. If i remember correctly, it requires
some setup for forwarding over the DSA ports.

But for a first implementation, i would be tempted to disallow such
setups. Force the LAG members to be on the same switch.

	Andrew