[RFC PATCH net-next 00/16] LAG offload for Ocelot DSA switches

[RFC PATCH net-next 00/16] LAG offload for Ocelot DSA switches

[Vladimir Oltean]

This patch series comes as a continuation of the discussion started with
Tobias Waldekranz in his patch series to offload bonding/team from DSA:
https://patchwork.kernel.org/project/netdevbpf/patch/20201202091356.24075-3-tobias@waldekranz.com/

On one hand, it shows the rework that needs to be done to ocelot such
that a pure switchdev and a DSA driver could share the same implementation.

On the other hand, it tries to identify what data structures does DSA
really need to keep and pass along to drivers, and which structures are
best left for the drivers to deal privately with them.

Testing has been done in the following topology:

         +----------------------------------+
         | Board 1         br0              |
         |             +---------+          |
         |            /           \         |
         |            |           |         |
         |            |         bond0       |
         |            |        +-----+      |
         |            |       /       \     |
         |  eno0     swp0    swp1    swp2   |
         +---|--------|-------|-------|-----+
             |        |       |       |
             +--------+       |       |
               Cable          |       |
                         Cable|       |Cable
               Cable          |       |
             +--------+       |       |
             |        |       |       |
         +---|--------|-------|-------|-----+
         |  eno0     swp0    swp1    swp2   |
         |            |       \       /     |
         |            |        +-----+      |
         |            |         bond0       |
         |            |           |         |
         |            \           /         |
         |             +---------+          |
         | Board 2         br0              |
         +----------------------------------+

The same script can be run on both Board 1 and Board 2 to set this up:

#!/bin/bash

ip link del bond0
ip link add bond0 type bond mode 802.3ad
ip link set swp1 down && ip link set swp1 master bond0 && ip link set swp1 up
ip link set swp2 down && ip link set swp2 master bond0 && ip link set swp2 up
ip link del br0
ip link add br0 type bridge
ip link set bond0 master br0
ip link set swp0 master br0

Then traffic can be tested between eno0 of Board 1 and eno0 of Board 2.

Vladimir Oltean (16):
  net: mscc: ocelot: offload bridge port flags to device
  net: mscc: ocelot: allow offloading of bridge on top of LAG
  net: mscc: ocelot: rename ocelot_netdevice_port_event to
    ocelot_netdevice_changeupper
  net: mscc: ocelot: use a switch-case statement in
    ocelot_netdevice_event
  net: mscc: ocelot: don't refuse bonding interfaces we can't offload
  net: mscc: ocelot: use ipv6 in the aggregation code
  net: mscc: ocelot: set up the bonding mask in a way that avoids a
    net_device
  net: mscc: ocelot: avoid unneeded "lp" variable in LAG join
  net: mscc: ocelot: use "lag" variable name in
    ocelot_bridge_stp_state_set
  net: mscc: ocelot: reapply bridge forwarding mask on bonding
    join/leave
  net: mscc: ocelot: set up logical port IDs centrally
  net: mscc: ocelot: drop the use of the "lags" array
  net: mscc: ocelot: rename aggr_count to num_ports_in_lag
  net: mscc: ocelot: rebalance LAGs on link up/down events
  net: dsa: felix: propagate the LAG offload ops towards the ocelot lib
  net: dsa: ocelot: tell DSA that we can offload link aggregation

 drivers/net/dsa/ocelot/felix.c         |  28 +++
 drivers/net/ethernet/mscc/ocelot.c     | 276 +++++++++++++++----------
 drivers/net/ethernet/mscc/ocelot.h     |   7 +-
 drivers/net/ethernet/mscc/ocelot_net.c | 139 ++++++++-----
 include/soc/mscc/ocelot.h              |  13 +-
 5 files changed, 298 insertions(+), 165 deletions(-)

-- 
2.25.1

[RFC PATCH net-next 01/16] net: mscc: ocelot: offload bridge port flags to device

[Vladimir Oltean]

We should not be unconditionally enabling address learning, since doing
that is actively detrimential when a port is standalone and not offloading
a bridge. Namely, if a port in the switch is standalone and others are
offloading the bridge, then we could enter a situation where we learn an
address towards the standalone port, but the bridged ports could not
forward the packet there, because the CPU is the only path between the
standalone and the bridged ports. The solution of course is to not
enable address learning unless the bridge asks for it. Currently this is
the only bridge port flag we are looking at. The others (flooding etc)
are TBD.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c     | 21 ++++++++++++++++++++-
 drivers/net/ethernet/mscc/ocelot.h     |  3 +++
 drivers/net/ethernet/mscc/ocelot_net.c |  4 ++++
 include/soc/mscc/ocelot.h              |  2 ++
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index b9626eec8db6..7a5c534099d3 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -883,6 +883,7 @@ EXPORT_SYMBOL(ocelot_get_ts_info);
 
 void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
 {
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	u32 port_cfg;
 	int p, i;
 
@@ -896,7 +897,8 @@ void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
 		ocelot->bridge_fwd_mask |= BIT(port);
 		fallthrough;
 	case BR_STATE_LEARNING:
-		port_cfg |= ANA_PORT_PORT_CFG_LEARN_ENA;
+		if (ocelot_port->brport_flags & BR_LEARNING)
+			port_cfg |= ANA_PORT_PORT_CFG_LEARN_ENA;
 		break;
 
 	default:
@@ -1178,6 +1180,7 @@ EXPORT_SYMBOL(ocelot_port_bridge_join);
 int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
 			     struct net_device *bridge)
 {
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	struct ocelot_vlan pvid = {0}, native_vlan = {0};
 	struct switchdev_trans trans;
 	int ret;
@@ -1200,6 +1203,10 @@ int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
 	ocelot_port_set_pvid(ocelot, port, pvid);
 	ocelot_port_set_native_vlan(ocelot, port, native_vlan);
 
+	ocelot_port->brport_flags = 0;
+	ocelot_rmw_gix(ocelot, 0, ANA_PORT_PORT_CFG_LEARN_ENA,
+		       ANA_PORT_PORT_CFG, port);
+
 	return 0;
 }
 EXPORT_SYMBOL(ocelot_port_bridge_leave);
@@ -1391,6 +1398,18 @@ int ocelot_get_max_mtu(struct ocelot *ocelot, int port)
 }
 EXPORT_SYMBOL(ocelot_get_max_mtu);
 
+void ocelot_port_bridge_flags(struct ocelot *ocelot, int port,
+			      unsigned long flags,
+			      struct switchdev_trans *trans)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+
+	if (switchdev_trans_ph_prepare(trans))
+		return;
+
+	ocelot_port->brport_flags = flags;
+}
+
 void ocelot_init_port(struct ocelot *ocelot, int port)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
index 291d39d49c4e..739bd201d951 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -102,6 +102,9 @@ struct ocelot_multicast {
 	struct ocelot_pgid *pgid;
 };
 
+void ocelot_port_bridge_flags(struct ocelot *ocelot, int port,
+			      unsigned long flags,
+			      struct switchdev_trans *trans);
 int ocelot_port_fdb_do_dump(const unsigned char *addr, u16 vid,
 			    bool is_static, void *data);
 int ocelot_mact_learn(struct ocelot *ocelot, int port,
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 9ba7e2b166e9..93ecd5274156 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -882,6 +882,10 @@ static int ocelot_port_attr_set(struct net_device *dev,
 	case SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED:
 		ocelot_port_attr_mc_set(ocelot, port, !attr->u.mc_disabled);
 		break;
+	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
+		ocelot_port_bridge_flags(ocelot, port, attr->u.brport_flags,
+					 trans);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 2f4cd3288bcc..50514c087231 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -581,6 +581,8 @@ struct ocelot_port {
 
 	struct regmap			*target;
 
+	unsigned long			brport_flags;
+
 	bool				vlan_aware;
 	/* VLAN that untagged frames are classified to, on ingress */
 	struct ocelot_vlan		pvid_vlan;
-- 
2.25.1

[RFC PATCH net-next 02/16] net: mscc: ocelot: allow offloading of bridge on top of LAG

[Vladimir Oltean]

Commit 7afb3e575e5a ("net: mscc: ocelot: don't handle netdev events for
other netdevs") was too aggressive, and it made ocelot_netdevice_event
react only to network interface events emitted for the ocelot switch
ports.

In fact, only the PRECHANGEUPPER should have had that check.

When we ignore all events that are not for us, we miss the fact that the
upper of the LAG changes, and the bonding interface gets enslaved to a
bridge. This is an operation we could offload under certain conditions.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_net.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 93ecd5274156..6fb2a813e694 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -1047,10 +1047,8 @@ static int ocelot_netdevice_event(struct notifier_block *unused,
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	int ret = 0;
 
-	if (!ocelot_netdevice_dev_check(dev))
-		return 0;
-
 	if (event == NETDEV_PRECHANGEUPPER &&
+	    ocelot_netdevice_dev_check(dev) &&
 	    netif_is_lag_master(info->upper_dev)) {
 		struct netdev_lag_upper_info *lag_upper_info = info->upper_info;
 		struct netlink_ext_ack *extack;
-- 
2.25.1

[RFC PATCH net-next 03/16] net: mscc: ocelot: rename ocelot_netdevice_port_event to ocelot_netdevice_changeupper

[Vladimir Oltean]

ocelot_netdevice_port_event treats a single event, NETDEV_CHANGEUPPER.
So we can remove the check for the type of event, and rename the
function to be more suggestive, since there already is a function with a
very similar name of ocelot_netdevice_event.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_net.c | 59 ++++++++++++--------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 6fb2a813e694..50765a3b1c44 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -1003,9 +1003,8 @@ static int ocelot_port_obj_del(struct net_device *dev,
 	return ret;
 }
 
-static int ocelot_netdevice_port_event(struct net_device *dev,
-				       unsigned long event,
-				       struct netdev_notifier_changeupper_info *info)
+static int ocelot_netdevice_changeupper(struct net_device *dev,
+					struct netdev_notifier_changeupper_info *info)
 {
 	struct ocelot_port_private *priv = netdev_priv(dev);
 	struct ocelot_port *ocelot_port = &priv->port;
@@ -1013,28 +1012,22 @@ static int ocelot_netdevice_port_event(struct net_device *dev,
 	int port = priv->chip_port;
 	int err = 0;
 
-	switch (event) {
-	case NETDEV_CHANGEUPPER:
-		if (netif_is_bridge_master(info->upper_dev)) {
-			if (info->linking) {
-				err = ocelot_port_bridge_join(ocelot, port,
-							      info->upper_dev);
-			} else {
-				err = ocelot_port_bridge_leave(ocelot, port,
-							       info->upper_dev);
-			}
-		}
-		if (netif_is_lag_master(info->upper_dev)) {
-			if (info->linking)
-				err = ocelot_port_lag_join(ocelot, port,
-							   info->upper_dev);
-			else
-				ocelot_port_lag_leave(ocelot, port,
+	if (netif_is_bridge_master(info->upper_dev)) {
+		if (info->linking) {
+			err = ocelot_port_bridge_join(ocelot, port,
 						      info->upper_dev);
+		} else {
+			err = ocelot_port_bridge_leave(ocelot, port,
+						       info->upper_dev);
 		}
-		break;
-	default:
-		break;
+	}
+	if (netif_is_lag_master(info->upper_dev)) {
+		if (info->linking)
+			err = ocelot_port_lag_join(ocelot, port,
+						   info->upper_dev);
+		else
+			ocelot_port_lag_leave(ocelot, port,
+					      info->upper_dev);
 	}
 
 	return err;
@@ -1063,17 +1056,19 @@ static int ocelot_netdevice_event(struct notifier_block *unused,
 		}
 	}
 
-	if (netif_is_lag_master(dev)) {
-		struct net_device *slave;
-		struct list_head *iter;
+	if (event == NETDEV_CHANGEUPPER) {
+		if (netif_is_lag_master(dev)) {
+			struct net_device *slave;
+			struct list_head *iter;
 
-		netdev_for_each_lower_dev(dev, slave, iter) {
-			ret = ocelot_netdevice_port_event(slave, event, info);
-			if (ret)
-				goto notify;
+			netdev_for_each_lower_dev(dev, slave, iter) {
+				ret = ocelot_netdevice_changeupper(slave, event, info);
+				if (ret)
+					goto notify;
+			}
+		} else {
+			ret = ocelot_netdevice_changeupper(dev, event, info);
 		}
-	} else {
-		ret = ocelot_netdevice_port_event(dev, event, info);
 	}
 
 notify:
-- 
2.25.1

[RFC PATCH net-next 04/16] net: mscc: ocelot: use a switch-case statement in ocelot_netdevice_event

[Vladimir Oltean]

Make ocelot's net device event handler more streamlined by structuring
it in a similar way with others. The inspiration here was
dsa_slave_netdevice_event.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_net.c | 68 +++++++++++++++++---------
 1 file changed, 45 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 50765a3b1c44..47b620967156 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -1030,49 +1030,71 @@ static int ocelot_netdevice_changeupper(struct net_device *dev,
 					      info->upper_dev);
 	}
 
-	return err;
+	return notifier_from_errno(err);
+}
+
+static int
+ocelot_netdevice_lag_changeupper(struct net_device *dev,
+				 struct netdev_notifier_changeupper_info *info)
+{
+	struct net_device *lower;
+	struct list_head *iter;
+	int err = NOTIFY_DONE;
+
+	netdev_for_each_lower_dev(dev, lower, iter) {
+		err = ocelot_netdevice_changeupper(lower, info);
+		if (err)
+			return notifier_from_errno(err);
+	}
+
+	return NOTIFY_DONE;
 }
 
 static int ocelot_netdevice_event(struct notifier_block *unused,
 				  unsigned long event, void *ptr)
 {
-	struct netdev_notifier_changeupper_info *info = ptr;
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
-	int ret = 0;
 
-	if (event == NETDEV_PRECHANGEUPPER &&
-	    ocelot_netdevice_dev_check(dev) &&
-	    netif_is_lag_master(info->upper_dev)) {
-		struct netdev_lag_upper_info *lag_upper_info = info->upper_info;
+	switch (event) {
+	case NETDEV_PRECHANGEUPPER: {
+		struct netdev_notifier_changeupper_info *info = ptr;
+		struct netdev_lag_upper_info *lag_upper_info;
 		struct netlink_ext_ack *extack;
 
+		if (!ocelot_netdevice_dev_check(dev))
+			break;
+
+		if (!netif_is_lag_master(info->upper_dev))
+			break;
+
+		lag_upper_info = info->upper_info;
+
 		if (lag_upper_info &&
 		    lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH) {
 			extack = netdev_notifier_info_to_extack(&info->info);
 			NL_SET_ERR_MSG_MOD(extack, "LAG device using unsupported Tx type");
 
-			ret = -EINVAL;
-			goto notify;
+			return NOTIFY_BAD;
 		}
+
+		break;
 	}
+	case NETDEV_CHANGEUPPER: {
+		struct netdev_notifier_changeupper_info *info = ptr;
 
-	if (event == NETDEV_CHANGEUPPER) {
-		if (netif_is_lag_master(dev)) {
-			struct net_device *slave;
-			struct list_head *iter;
+		if (ocelot_netdevice_dev_check(dev))
+			return ocelot_netdevice_changeupper(dev, info);
 
-			netdev_for_each_lower_dev(dev, slave, iter) {
-				ret = ocelot_netdevice_changeupper(slave, event, info);
-				if (ret)
-					goto notify;
-			}
-		} else {
-			ret = ocelot_netdevice_changeupper(dev, event, info);
-		}
+		if (netif_is_lag_master(dev))
+			return ocelot_netdevice_lag_changeupper(dev, info);
+
+		break;
+	}
+	default:
+		break;
 	}
 
-notify:
-	return notifier_from_errno(ret);
+	return NOTIFY_DONE;
 }
 
 struct notifier_block ocelot_netdevice_nb __read_mostly = {
-- 
2.25.1

[RFC PATCH net-next 05/16] net: mscc: ocelot: don't refuse bonding interfaces we can't offload

[Vladimir Oltean]

Since switchdev/DSA exposes network interfaces that fulfill many of the
same user space expectations that dedicated NICs do, it makes sense to
not deny bonding interfaces with a bonding policy that we cannot offload,
but instead allow the bonding driver to select the egress interface in
software.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_net.c | 38 ++++++++++----------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 47b620967156..77957328722a 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -1022,6 +1022,15 @@ static int ocelot_netdevice_changeupper(struct net_device *dev,
 		}
 	}
 	if (netif_is_lag_master(info->upper_dev)) {
+		struct netdev_lag_upper_info *lag_upper_info;
+
+		lag_upper_info = info->upper_info;
+
+		/* Only offload what we can */
+		if (lag_upper_info &&
+		    lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH)
+			return NOTIFY_DONE;
+
 		if (info->linking)
 			err = ocelot_port_lag_join(ocelot, port,
 						   info->upper_dev);
@@ -1037,10 +1046,16 @@ static int
 ocelot_netdevice_lag_changeupper(struct net_device *dev,
 				 struct netdev_notifier_changeupper_info *info)
 {
+	struct netdev_lag_upper_info *lag_upper_info = info->upper_info;
 	struct net_device *lower;
 	struct list_head *iter;
 	int err = NOTIFY_DONE;
 
+	/* Can't offload LAG => also do bridging in software */
+	if (lag_upper_info &&
+	    lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH)
+		return NOTIFY_DONE;
+
 	netdev_for_each_lower_dev(dev, lower, iter) {
 		err = ocelot_netdevice_changeupper(lower, info);
 		if (err)
@@ -1056,29 +1071,6 @@ static int ocelot_netdevice_event(struct notifier_block *unused,
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
 	switch (event) {
-	case NETDEV_PRECHANGEUPPER: {
-		struct netdev_notifier_changeupper_info *info = ptr;
-		struct netdev_lag_upper_info *lag_upper_info;
-		struct netlink_ext_ack *extack;
-
-		if (!ocelot_netdevice_dev_check(dev))
-			break;
-
-		if (!netif_is_lag_master(info->upper_dev))
-			break;
-
-		lag_upper_info = info->upper_info;
-
-		if (lag_upper_info &&
-		    lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH) {
-			extack = netdev_notifier_info_to_extack(&info->info);
-			NL_SET_ERR_MSG_MOD(extack, "LAG device using unsupported Tx type");
-
-			return NOTIFY_BAD;
-		}
-
-		break;
-	}
 	case NETDEV_CHANGEUPPER: {
 		struct netdev_notifier_changeupper_info *info = ptr;
 
-- 
2.25.1

[RFC PATCH net-next 06/16] net: mscc: ocelot: use ipv6 in the aggregation code

[Vladimir Oltean]

IPv6 header information is not currently part of the entropy source for
the 4-bit aggregation code used for LAG offload, even though it could be.
The hardware reference manual says about these fields:

ANA::AGGR_CFG.AC_IP6_TCPUDP_PORT_ENA
Use IPv6 TCP/UDP port when calculating aggregation code. Configure
identically for all ports. Recommended value is 1.

ANA::AGGR_CFG.AC_IP6_FLOW_LBL_ENA
Use IPv6 flow label when calculating AC. Configure identically for all
ports. Recommended value is 1.

Integration with the xmit_hash_policy of the bonding interface is TBD.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 7a5c534099d3..13e86dd71e5a 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1557,7 +1557,10 @@ int ocelot_init(struct ocelot *ocelot)
 	ocelot_write(ocelot, ANA_AGGR_CFG_AC_SMAC_ENA |
 			     ANA_AGGR_CFG_AC_DMAC_ENA |
 			     ANA_AGGR_CFG_AC_IP4_SIPDIP_ENA |
-			     ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA, ANA_AGGR_CFG);
+			     ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA |
+			     ANA_AGGR_CFG_AC_IP6_FLOW_LBL_ENA |
+			     ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA,
+			     ANA_AGGR_CFG);
 
 	/* Set MAC age time to default value. The entry is aged after
 	 * 2*AGE_PERIOD
-- 
2.25.1

[RFC PATCH net-next 07/16] net: mscc: ocelot: set up the bonding mask in a way that avoids a net_device

[Vladimir Oltean]

Since this code should be called from pure switchdev as well as from
DSA, we must find a way to determine the bonding mask not by looking
directly at the net_device lowers of the bonding interface, since those
could have different private structures.

We keep a pointer to the bonding upper interface, if present, in struct
ocelot_port. Then the bonding mask becomes the bitwise OR of all ports
that have the same bonding upper interface. This adds a duplication of
functionality with the current "lags" array, but the duplication will be
short-lived, since further patches will remove the latter completely.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 29 ++++++++++++++++++++++-------
 include/soc/mscc/ocelot.h          |  2 ++
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 13e86dd71e5a..30dee1f957d1 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -881,6 +881,24 @@ int ocelot_get_ts_info(struct ocelot *ocelot, int port,
 }
 EXPORT_SYMBOL(ocelot_get_ts_info);
 
+static u32 ocelot_get_bond_mask(struct ocelot *ocelot, struct net_device *bond)
+{
+	u32 bond_mask = 0;
+	int port;
+
+	for (port = 0; port < ocelot->num_phys_ports; port++) {
+		struct ocelot_port *ocelot_port = ocelot->ports[port];
+
+		if (!ocelot_port)
+			continue;
+
+		if (ocelot_port->bond == bond)
+			bond_mask |= BIT(port);
+	}
+
+	return bond_mask;
+}
+
 void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
@@ -1272,17 +1290,12 @@ static void ocelot_setup_lag(struct ocelot *ocelot, int lag)
 int ocelot_port_lag_join(struct ocelot *ocelot, int port,
 			 struct net_device *bond)
 {
-	struct net_device *ndev;
 	u32 bond_mask = 0;
 	int lag, lp;
 
-	rcu_read_lock();
-	for_each_netdev_in_bond_rcu(bond, ndev) {
-		struct ocelot_port_private *priv = netdev_priv(ndev);
+	ocelot->ports[port]->bond = bond;
 
-		bond_mask |= BIT(priv->chip_port);
-	}
-	rcu_read_unlock();
+	bond_mask = ocelot_get_bond_mask(ocelot, bond);
 
 	lp = __ffs(bond_mask);
 
@@ -1315,6 +1328,8 @@ void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
 	u32 port_cfg;
 	int i;
 
+	ocelot->ports[port]->bond = NULL;
+
 	/* Remove port from any lag */
 	for (i = 0; i < ocelot->num_phys_ports; i++)
 		ocelot->lags[i] &= ~BIT(port);
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 50514c087231..b812bdff1da1 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -597,6 +597,8 @@ struct ocelot_port {
 	phy_interface_t			phy_mode;
 
 	u8				*xmit_template;
+
+	struct net_device		*bond;
 };
 
 struct ocelot {
-- 
2.25.1

[RFC PATCH net-next 08/16] net: mscc: ocelot: avoid unneeded "lp" variable in LAG join

[Vladimir Oltean]

The index of the LAG is equal to the logical port ID that all the
physical port members have, which is further equal to the index of the
first physical port that is a member of the LAG.

The code gets a bit carried away with logic like this:

	if (a == b)
		c = a;
	else
		c = b;

which can be simplified, of course, into:

	c = b;

(with a being port, b being lp, c being lag)

This further makes the "lp" variable redundant, since we can use "lag"
everywhere where "lp" (logical port) was used. So instead of a "c = b"
assignment, we can do a complete deletion of b. Only one comment here:

		if (bond_mask) {
			lp = __ffs(bond_mask);
			ocelot->lags[lp] = 0;
		}

lp was clobbered before, because it was used as a temporary variable to
hold the new smallest port ID from the bond. Now that we don't have "lp"
any longer, we'll just avoid the temporary variable and zeroize the
bonding mask directly.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 30dee1f957d1..080fd4ce37ea 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1291,28 +1291,24 @@ int ocelot_port_lag_join(struct ocelot *ocelot, int port,
 			 struct net_device *bond)
 {
 	u32 bond_mask = 0;
-	int lag, lp;
+	int lag;
 
 	ocelot->ports[port]->bond = bond;
 
 	bond_mask = ocelot_get_bond_mask(ocelot, bond);
 
-	lp = __ffs(bond_mask);
+	lag = __ffs(bond_mask);
 
 	/* If the new port is the lowest one, use it as the logical port from
 	 * now on
 	 */
-	if (port == lp) {
-		lag = port;
+	if (port == lag) {
 		ocelot->lags[port] = bond_mask;
 		bond_mask &= ~BIT(port);
-		if (bond_mask) {
-			lp = __ffs(bond_mask);
-			ocelot->lags[lp] = 0;
-		}
+		if (bond_mask)
+			ocelot->lags[__ffs(bond_mask)] = 0;
 	} else {
-		lag = lp;
-		ocelot->lags[lp] |= BIT(port);
+		ocelot->lags[lag] |= BIT(port);
 	}
 
 	ocelot_setup_lag(ocelot, lag);
-- 
2.25.1

[RFC PATCH net-next 09/16] net: mscc: ocelot: use "lag" variable name in ocelot_bridge_stp_state_set

[Vladimir Oltean]

In anticipation of further simplification, make it more clear what we're
iterating over.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 080fd4ce37ea..c3c6682e6e79 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -903,7 +903,7 @@ void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	u32 port_cfg;
-	int p, i;
+	int p;
 
 	if (!(BIT(port) & ocelot->bridge_mask))
 		return;
@@ -928,14 +928,17 @@ void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
 	ocelot_write_gix(ocelot, port_cfg, ANA_PORT_PORT_CFG, port);
 
 	/* Apply FWD mask. The loop is needed to add/remove the current port as
-	 * a source for the other ports.
+	 * a source for the other ports. If the source port is in a bond, then
+	 * all the other ports from that bond need to be removed from this
+	 * source port's forwarding mask.
 	 */
 	for (p = 0; p < ocelot->num_phys_ports; p++) {
 		if (ocelot->bridge_fwd_mask & BIT(p)) {
 			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(p);
+			int lag;
 
-			for (i = 0; i < ocelot->num_phys_ports; i++) {
-				unsigned long bond_mask = ocelot->lags[i];
+			for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
+				unsigned long bond_mask = ocelot->lags[lag];
 
 				if (!bond_mask)
 					continue;
-- 
2.25.1

[RFC PATCH net-next 10/16] net: mscc: ocelot: reapply bridge forwarding mask on bonding join/leave

[Vladimir Oltean]

Applying the bridge forwarding mask currently is done only on the STP
state changes for any port. But it depends on both STP state changes,
and bonding interface state changes. Export the bit that recalculates
the forwarding mask so that it could be reused, and call it when a port
starts and stops offloading a bonding interface.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 68 +++++++++++++++++-------------
 1 file changed, 38 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index c3c6682e6e79..ee0fcee8e09a 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -899,11 +899,45 @@ static u32 ocelot_get_bond_mask(struct ocelot *ocelot, struct net_device *bond)
 	return bond_mask;
 }
 
+static void ocelot_apply_bridge_fwd_mask(struct ocelot *ocelot)
+{
+	int port;
+
+	/* Apply FWD mask. The loop is needed to add/remove the current port as
+	 * a source for the other ports. If the source port is in a bond, then
+	 * all the other ports from that bond need to be removed from this
+	 * source port's forwarding mask.
+	 */
+	for (port = 0; port < ocelot->num_phys_ports; port++) {
+		if (ocelot->bridge_fwd_mask & BIT(port)) {
+			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(port);
+			int lag;
+
+			for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
+				unsigned long bond_mask = ocelot->lags[lag];
+
+				if (!bond_mask)
+					continue;
+
+				if (bond_mask & BIT(port)) {
+					mask &= ~bond_mask;
+					break;
+				}
+			}
+
+			ocelot_write_rix(ocelot, mask,
+					 ANA_PGID_PGID, PGID_SRC + port);
+		} else {
+			ocelot_write_rix(ocelot, 0,
+					 ANA_PGID_PGID, PGID_SRC + port);
+		}
+	}
+}
+
 void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	u32 port_cfg;
-	int p;
 
 	if (!(BIT(port) & ocelot->bridge_mask))
 		return;
@@ -927,35 +961,7 @@ void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
 
 	ocelot_write_gix(ocelot, port_cfg, ANA_PORT_PORT_CFG, port);
 
-	/* Apply FWD mask. The loop is needed to add/remove the current port as
-	 * a source for the other ports. If the source port is in a bond, then
-	 * all the other ports from that bond need to be removed from this
-	 * source port's forwarding mask.
-	 */
-	for (p = 0; p < ocelot->num_phys_ports; p++) {
-		if (ocelot->bridge_fwd_mask & BIT(p)) {
-			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(p);
-			int lag;
-
-			for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
-				unsigned long bond_mask = ocelot->lags[lag];
-
-				if (!bond_mask)
-					continue;
-
-				if (bond_mask & BIT(p)) {
-					mask &= ~bond_mask;
-					break;
-				}
-			}
-
-			ocelot_write_rix(ocelot, mask,
-					 ANA_PGID_PGID, PGID_SRC + p);
-		} else {
-			ocelot_write_rix(ocelot, 0,
-					 ANA_PGID_PGID, PGID_SRC + p);
-		}
-	}
+	ocelot_apply_bridge_fwd_mask(ocelot);
 }
 EXPORT_SYMBOL(ocelot_bridge_stp_state_set);
 
@@ -1315,6 +1321,7 @@ int ocelot_port_lag_join(struct ocelot *ocelot, int port,
 	}
 
 	ocelot_setup_lag(ocelot, lag);
+	ocelot_apply_bridge_fwd_mask(ocelot);
 	ocelot_set_aggr_pgids(ocelot);
 
 	return 0;
@@ -1350,6 +1357,7 @@ void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
 	ocelot_write_gix(ocelot, port_cfg | ANA_PORT_PORT_CFG_PORTID_VAL(port),
 			 ANA_PORT_PORT_CFG, port);
 
+	ocelot_apply_bridge_fwd_mask(ocelot);
 	ocelot_set_aggr_pgids(ocelot);
 }
 EXPORT_SYMBOL(ocelot_port_lag_leave);
-- 
2.25.1

[RFC PATCH net-next 11/16] net: mscc: ocelot: set up logical port IDs centrally

[Vladimir Oltean]

The setup of logical port IDs is done in two places: from the inconclusively
named ocelot_setup_lag and from ocelot_port_lag_leave, a function that
also calls ocelot_setup_lag (which apparently does an incomplete setup
of the LAG).

To improve this situation, we can rename ocelot_setup_lag into
ocelot_setup_logical_port_ids, and drop the "lag" argument. It will now
set up the logical port IDs of all switch ports, which may be just
slightly more inefficient but more maintainable.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 47 ++++++++++++++++++------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index ee0fcee8e09a..1a98c24af056 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1279,20 +1279,36 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
 	}
 }
 
-static void ocelot_setup_lag(struct ocelot *ocelot, int lag)
+/* When offloading a bonding interface, the switch ports configured under the
+ * same bond must have the same logical port ID, equal to the physical port ID
+ * of the lowest numbered physical port in that bond. Otherwise, in standalone/
+ * bridged mode, each port has a logical port ID equal to its physical port ID.
+ */
+static void ocelot_setup_logical_port_ids(struct ocelot *ocelot)
 {
-	unsigned long bond_mask = ocelot->lags[lag];
-	unsigned int p;
+	int port;
 
-	for_each_set_bit(p, &bond_mask, ocelot->num_phys_ports) {
-		u32 port_cfg = ocelot_read_gix(ocelot, ANA_PORT_PORT_CFG, p);
+	for (port = 0; port < ocelot->num_phys_ports; port++) {
+		struct ocelot_port *ocelot_port = ocelot->ports[port];
+		struct net_device *bond;
+
+		if (!ocelot_port)
+			continue;
 
-		port_cfg &= ~ANA_PORT_PORT_CFG_PORTID_VAL_M;
+		bond = ocelot_port->bond;
+		if (bond) {
+			int lag = __ffs(ocelot_get_bond_mask(ocelot, bond));
 
-		/* Use lag port as logical port for port i */
-		ocelot_write_gix(ocelot, port_cfg |
-				 ANA_PORT_PORT_CFG_PORTID_VAL(lag),
-				 ANA_PORT_PORT_CFG, p);
+			ocelot_rmw_gix(ocelot,
+				       ANA_PORT_PORT_CFG_PORTID_VAL(lag),
+				       ANA_PORT_PORT_CFG_PORTID_VAL_M,
+				       ANA_PORT_PORT_CFG, port);
+		} else {
+			ocelot_rmw_gix(ocelot,
+				       ANA_PORT_PORT_CFG_PORTID_VAL(port),
+				       ANA_PORT_PORT_CFG_PORTID_VAL_M,
+				       ANA_PORT_PORT_CFG, port);
+		}
 	}
 }
 
@@ -1320,7 +1336,7 @@ int ocelot_port_lag_join(struct ocelot *ocelot, int port,
 		ocelot->lags[lag] |= BIT(port);
 	}
 
-	ocelot_setup_lag(ocelot, lag);
+	ocelot_setup_logical_port_ids(ocelot);
 	ocelot_apply_bridge_fwd_mask(ocelot);
 	ocelot_set_aggr_pgids(ocelot);
 
@@ -1331,7 +1347,6 @@ EXPORT_SYMBOL(ocelot_port_lag_join);
 void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
 			   struct net_device *bond)
 {
-	u32 port_cfg;
 	int i;
 
 	ocelot->ports[port]->bond = NULL;
@@ -1348,15 +1363,9 @@ void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
 
 		ocelot->lags[n] = ocelot->lags[port];
 		ocelot->lags[port] = 0;
-
-		ocelot_setup_lag(ocelot, n);
 	}
 
-	port_cfg = ocelot_read_gix(ocelot, ANA_PORT_PORT_CFG, port);
-	port_cfg &= ~ANA_PORT_PORT_CFG_PORTID_VAL_M;
-	ocelot_write_gix(ocelot, port_cfg | ANA_PORT_PORT_CFG_PORTID_VAL(port),
-			 ANA_PORT_PORT_CFG, port);
-
+	ocelot_setup_logical_port_ids(ocelot);
 	ocelot_apply_bridge_fwd_mask(ocelot);
 	ocelot_set_aggr_pgids(ocelot);
 }
-- 
2.25.1

[RFC PATCH net-next 12/16] net: mscc: ocelot: drop the use of the "lags" array

[Vladimir Oltean]

We can now simplify the implementation by always using ocelot_get_bond_mask
to look up the other ports that are offloading the same bonding interface
as us.

In ocelot_set_aggr_pgids, the code had a way to uniquely iterate through
LAGs. We need to achieve the same behavior by marking each LAG as visited,
which we do now by temporarily allocating an array of pointers to bonding
uppers of each port, and marking each bonding upper as NULL once it has
been treated by the first port that is a member. And because we now do
some dynamic allocation, we need to propagate errors from
ocelot_set_aggr_pgid all the way to ocelot_port_lag_leave.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c     | 104 ++++++++++---------------
 drivers/net/ethernet/mscc/ocelot.h     |   4 +-
 drivers/net/ethernet/mscc/ocelot_net.c |   4 +-
 include/soc/mscc/ocelot.h              |   2 -
 4 files changed, 47 insertions(+), 67 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 1a98c24af056..d4dbba66aa65 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -909,21 +909,17 @@ static void ocelot_apply_bridge_fwd_mask(struct ocelot *ocelot)
 	 * source port's forwarding mask.
 	 */
 	for (port = 0; port < ocelot->num_phys_ports; port++) {
-		if (ocelot->bridge_fwd_mask & BIT(port)) {
-			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(port);
-			int lag;
+		struct ocelot_port *ocelot_port = ocelot->ports[port];
 
-			for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
-				unsigned long bond_mask = ocelot->lags[lag];
+		if (!ocelot_port)
+			continue;
 
-				if (!bond_mask)
-					continue;
+		if (ocelot->bridge_fwd_mask & BIT(port)) {
+			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(port);
+			struct net_device *bond = ocelot_port->bond;
 
-				if (bond_mask & BIT(port)) {
-					mask &= ~bond_mask;
-					break;
-				}
-			}
+			if (bond)
+				mask &= ~ocelot_get_bond_mask(ocelot, bond);
 
 			ocelot_write_rix(ocelot, mask,
 					 ANA_PGID_PGID, PGID_SRC + port);
@@ -1238,10 +1234,16 @@ int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
 }
 EXPORT_SYMBOL(ocelot_port_bridge_leave);
 
-static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
+static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
 {
+	struct net_device **bonds;
 	int i, port, lag;
 
+	bonds = kcalloc(ocelot->num_phys_ports, sizeof(struct net_device *),
+			GFP_KERNEL);
+	if (!bonds)
+		return -ENOMEM;
+
 	/* Reset destination and aggregation PGIDS */
 	for_each_unicast_dest_pgid(ocelot, port)
 		ocelot_write_rix(ocelot, BIT(port), ANA_PGID_PGID, port);
@@ -1250,16 +1252,26 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
 		ocelot_write_rix(ocelot, GENMASK(ocelot->num_phys_ports - 1, 0),
 				 ANA_PGID_PGID, i);
 
+	for (port = 0; port < ocelot->num_phys_ports; port++) {
+		struct ocelot_port *ocelot_port = ocelot->ports[port];
+
+		if (!ocelot_port)
+			continue;
+
+		bonds[port] = ocelot_port->bond;
+	}
+
 	/* Now, set PGIDs for each LAG */
 	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
 		unsigned long bond_mask;
 		int aggr_count = 0;
 		u8 aggr_idx[16];
 
-		bond_mask = ocelot->lags[lag];
-		if (!bond_mask)
+		if (!bonds[lag])
 			continue;
 
+		bond_mask = ocelot_get_bond_mask(ocelot, bonds[lag]);
+
 		for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {
 			// Destination mask
 			ocelot_write_rix(ocelot, bond_mask,
@@ -1276,7 +1288,19 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
 			ac |= BIT(aggr_idx[i % aggr_count]);
 			ocelot_write_rix(ocelot, ac, ANA_PGID_PGID, i);
 		}
+
+		/* Mark the bonding interface as visited to avoid applying
+		 * the same config again
+		 */
+		for (i = lag + 1; i < ocelot->num_phys_ports; i++)
+			if (bonds[i] == bonds[lag])
+				bonds[i] = NULL;
+
+		bonds[lag] = NULL;
 	}
+
+	kfree(bonds);
+	return 0;
 }
 
 /* When offloading a bonding interface, the switch ports configured under the
@@ -1315,59 +1339,22 @@ static void ocelot_setup_logical_port_ids(struct ocelot *ocelot)
 int ocelot_port_lag_join(struct ocelot *ocelot, int port,
 			 struct net_device *bond)
 {
-	u32 bond_mask = 0;
-	int lag;
-
 	ocelot->ports[port]->bond = bond;
 
-	bond_mask = ocelot_get_bond_mask(ocelot, bond);
-
-	lag = __ffs(bond_mask);
-
-	/* If the new port is the lowest one, use it as the logical port from
-	 * now on
-	 */
-	if (port == lag) {
-		ocelot->lags[port] = bond_mask;
-		bond_mask &= ~BIT(port);
-		if (bond_mask)
-			ocelot->lags[__ffs(bond_mask)] = 0;
-	} else {
-		ocelot->lags[lag] |= BIT(port);
-	}
-
 	ocelot_setup_logical_port_ids(ocelot);
 	ocelot_apply_bridge_fwd_mask(ocelot);
-	ocelot_set_aggr_pgids(ocelot);
-
-	return 0;
+	return ocelot_set_aggr_pgids(ocelot);
 }
 EXPORT_SYMBOL(ocelot_port_lag_join);
 
-void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
-			   struct net_device *bond)
+int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
+			  struct net_device *bond)
 {
-	int i;
-
 	ocelot->ports[port]->bond = NULL;
 
-	/* Remove port from any lag */
-	for (i = 0; i < ocelot->num_phys_ports; i++)
-		ocelot->lags[i] &= ~BIT(port);
-
-	/* if it was the logical port of the lag, move the lag config to the
-	 * next port
-	 */
-	if (ocelot->lags[port]) {
-		int n = __ffs(ocelot->lags[port]);
-
-		ocelot->lags[n] = ocelot->lags[port];
-		ocelot->lags[port] = 0;
-	}
-
 	ocelot_setup_logical_port_ids(ocelot);
 	ocelot_apply_bridge_fwd_mask(ocelot);
-	ocelot_set_aggr_pgids(ocelot);
+	return ocelot_set_aggr_pgids(ocelot);
 }
 EXPORT_SYMBOL(ocelot_port_lag_leave);
 
@@ -1543,11 +1530,6 @@ int ocelot_init(struct ocelot *ocelot)
 		}
 	}
 
-	ocelot->lags = devm_kcalloc(ocelot->dev, ocelot->num_phys_ports,
-				    sizeof(u32), GFP_KERNEL);
-	if (!ocelot->lags)
-		return -ENOMEM;
-
 	ocelot->stats = devm_kcalloc(ocelot->dev,
 				     ocelot->num_phys_ports * ocelot->num_stats,
 				     sizeof(u64), GFP_KERNEL);
diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
index 739bd201d951..bef8d5f8e6e5 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -114,8 +114,8 @@ int ocelot_mact_forget(struct ocelot *ocelot,
 		       const unsigned char mac[ETH_ALEN], unsigned int vid);
 int ocelot_port_lag_join(struct ocelot *ocelot, int port,
 			 struct net_device *bond);
-void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
-			   struct net_device *bond);
+int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
+			  struct net_device *bond);
 struct net_device *ocelot_port_to_netdev(struct ocelot *ocelot, int port);
 int ocelot_netdev_to_port(struct net_device *dev);
 
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 77957328722a..93aaa631e347 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -1035,8 +1035,8 @@ static int ocelot_netdevice_changeupper(struct net_device *dev,
 			err = ocelot_port_lag_join(ocelot, port,
 						   info->upper_dev);
 		else
-			ocelot_port_lag_leave(ocelot, port,
-					      info->upper_dev);
+			err = ocelot_port_lag_leave(ocelot, port,
+						    info->upper_dev);
 	}
 
 	return notifier_from_errno(err);
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index b812bdff1da1..0cd45659430f 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -639,8 +639,6 @@ struct ocelot {
 	enum ocelot_tag_prefix		inj_prefix;
 	enum ocelot_tag_prefix		xtr_prefix;
 
-	u32				*lags;
-
 	struct list_head		multicast;
 	struct list_head		pgids;
 
-- 
2.25.1

[RFC PATCH net-next 13/16] net: mscc: ocelot: rename aggr_count to num_ports_in_lag

[Vladimir Oltean]

It makes it a bit easier to read and understand the code that deals with
balancing the 16 aggregation codes among the ports in a certain LAG.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index d4dbba66aa65..d87e80a15ca5 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1263,8 +1263,8 @@ static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
 
 	/* Now, set PGIDs for each LAG */
 	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
+		int num_ports_in_lag = 0;
 		unsigned long bond_mask;
-		int aggr_count = 0;
 		u8 aggr_idx[16];
 
 		if (!bonds[lag])
@@ -1276,8 +1276,7 @@ static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
 			// Destination mask
 			ocelot_write_rix(ocelot, bond_mask,
 					 ANA_PGID_PGID, port);
-			aggr_idx[aggr_count] = port;
-			aggr_count++;
+			aggr_idx[num_ports_in_lag++] = port;
 		}
 
 		for_each_aggr_pgid(ocelot, i) {
@@ -1285,7 +1284,7 @@ static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
 
 			ac = ocelot_read_rix(ocelot, ANA_PGID_PGID, i);
 			ac &= ~bond_mask;
-			ac |= BIT(aggr_idx[i % aggr_count]);
+			ac |= BIT(aggr_idx[i % num_ports_in_lag]);
 			ocelot_write_rix(ocelot, ac, ANA_PGID_PGID, i);
 		}
 
-- 
2.25.1

[RFC PATCH net-next 14/16] net: mscc: ocelot: rebalance LAGs on link up/down events

[Vladimir Oltean]

At present there is an issue when ocelot is offloading a bonding
interface, but one of the links of the physical ports goes down. Traffic
keeps being hashed towards that destination, and of course gets dropped
on egress.

Monitor the netdev notifier events emitted by the bonding driver for
changes in the physical state of lower interfaces, to determine which
ports are active and which ones are no longer.

Then extend ocelot_get_bond_mask to return either the configured bonding
interfaces, or the active ones, depending on a boolean argument. The
code that does rebalancing only needs to do so among the active ports,
whereas the bridge forwarding mask and the logical port IDs still need
to look at the permanently bonded ports.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c     | 43 ++++++++++++++++++++------
 drivers/net/ethernet/mscc/ocelot.h     |  2 ++
 drivers/net/ethernet/mscc/ocelot_net.c | 26 ++++++++++++++++
 include/soc/mscc/ocelot.h              |  1 +
 4 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index d87e80a15ca5..5c71d121048d 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -881,7 +881,8 @@ int ocelot_get_ts_info(struct ocelot *ocelot, int port,
 }
 EXPORT_SYMBOL(ocelot_get_ts_info);
 
-static u32 ocelot_get_bond_mask(struct ocelot *ocelot, struct net_device *bond)
+static u32 ocelot_get_bond_mask(struct ocelot *ocelot, struct net_device *bond,
+				bool just_active_ports)
 {
 	u32 bond_mask = 0;
 	int port;
@@ -892,8 +893,12 @@ static u32 ocelot_get_bond_mask(struct ocelot *ocelot, struct net_device *bond)
 		if (!ocelot_port)
 			continue;
 
-		if (ocelot_port->bond == bond)
+		if (ocelot_port->bond == bond) {
+			if (just_active_ports && !ocelot_port->lag_tx_active)
+				continue;
+
 			bond_mask |= BIT(port);
+		}
 	}
 
 	return bond_mask;
@@ -919,7 +924,7 @@ static void ocelot_apply_bridge_fwd_mask(struct ocelot *ocelot)
 			struct net_device *bond = ocelot_port->bond;
 
 			if (bond)
-				mask &= ~ocelot_get_bond_mask(ocelot, bond);
+				mask &= ~ocelot_get_bond_mask(ocelot, bond, false);
 
 			ocelot_write_rix(ocelot, mask,
 					 ANA_PGID_PGID, PGID_SRC + port);
@@ -1261,22 +1266,22 @@ static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
 		bonds[port] = ocelot_port->bond;
 	}
 
-	/* Now, set PGIDs for each LAG */
+	/* Now, set PGIDs for each active LAG */
 	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
-		int num_ports_in_lag = 0;
+		int num_active_ports = 0;
 		unsigned long bond_mask;
 		u8 aggr_idx[16];
 
 		if (!bonds[lag])
 			continue;
 
-		bond_mask = ocelot_get_bond_mask(ocelot, bonds[lag]);
+		bond_mask = ocelot_get_bond_mask(ocelot, bonds[lag], true);
 
 		for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {
 			// Destination mask
 			ocelot_write_rix(ocelot, bond_mask,
 					 ANA_PGID_PGID, port);
-			aggr_idx[num_ports_in_lag++] = port;
+			aggr_idx[num_active_ports++] = port;
 		}
 
 		for_each_aggr_pgid(ocelot, i) {
@@ -1284,7 +1289,11 @@ static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
 
 			ac = ocelot_read_rix(ocelot, ANA_PGID_PGID, i);
 			ac &= ~bond_mask;
-			ac |= BIT(aggr_idx[i % num_ports_in_lag]);
+			/* Don't do division by zero if there was no active
+			 * port. Just make all aggregation codes zero.
+			 */
+			if (num_active_ports)
+				ac |= BIT(aggr_idx[i % num_active_ports]);
 			ocelot_write_rix(ocelot, ac, ANA_PGID_PGID, i);
 		}
 
@@ -1320,7 +1329,8 @@ static void ocelot_setup_logical_port_ids(struct ocelot *ocelot)
 
 		bond = ocelot_port->bond;
 		if (bond) {
-			int lag = __ffs(ocelot_get_bond_mask(ocelot, bond));
+			int lag = __ffs(ocelot_get_bond_mask(ocelot, bond,
+							     false));
 
 			ocelot_rmw_gix(ocelot,
 				       ANA_PORT_PORT_CFG_PORTID_VAL(lag),
@@ -1357,6 +1367,21 @@ int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
 }
 EXPORT_SYMBOL(ocelot_port_lag_leave);
 
+int ocelot_port_lag_change(struct ocelot *ocelot, int port,
+			   struct netdev_lag_lower_state_info *info)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	bool is_active = info->link_up && info->tx_enabled;
+
+	if (ocelot_port->lag_tx_active == is_active)
+		return 0;
+
+	ocelot_port->lag_tx_active = is_active;
+
+	/* Rebalance the LAGs */
+	return ocelot_set_aggr_pgids(ocelot);
+}
+
 /* Configure the maximum SDU (L2 payload) on RX to the value specified in @sdu.
  * The length of VLAN tags is accounted for automatically via DEV_MAC_TAGS_CFG.
  * In the special case that it's the NPI port that we're configuring, the
diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
index bef8d5f8e6e5..0860125b623c 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -116,6 +116,8 @@ int ocelot_port_lag_join(struct ocelot *ocelot, int port,
 			 struct net_device *bond);
 int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
 			  struct net_device *bond);
+int ocelot_port_lag_change(struct ocelot *ocelot, int port,
+			   struct netdev_lag_lower_state_info *info);
 struct net_device *ocelot_port_to_netdev(struct ocelot *ocelot, int port);
 int ocelot_netdev_to_port(struct net_device *dev);
 
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 93aaa631e347..714fc99f8339 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -1065,6 +1065,23 @@ ocelot_netdevice_lag_changeupper(struct net_device *dev,
 	return NOTIFY_DONE;
 }
 
+static int
+ocelot_netdevice_changelowerstate(struct net_device *dev,
+				  struct netdev_lag_lower_state_info *linfo)
+{
+	struct ocelot_port_private *priv = netdev_priv(dev);
+	struct ocelot_port *ocelot_port = &priv->port;
+	struct ocelot *ocelot = ocelot_port->ocelot;
+	int port = priv->chip_port;
+	int err;
+
+	if (!ocelot_port->bond)
+		return NOTIFY_DONE;
+
+	err = ocelot_port_lag_change(ocelot, port, linfo);
+	return notifier_from_errno(err);
+}
+
 static int ocelot_netdevice_event(struct notifier_block *unused,
 				  unsigned long event, void *ptr)
 {
@@ -1082,6 +1099,15 @@ static int ocelot_netdevice_event(struct notifier_block *unused,
 
 		break;
 	}
+	case NETDEV_CHANGELOWERSTATE: {
+		struct netdev_notifier_changelowerstate_info *info = ptr;
+
+		if (!ocelot_netdevice_dev_check(dev))
+			break;
+
+		return ocelot_netdevice_changelowerstate(dev,
+							 info->lower_state_info);
+	}
 	default:
 		break;
 	}
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 0cd45659430f..8a44b9064789 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -599,6 +599,7 @@ struct ocelot_port {
 	u8				*xmit_template;
 
 	struct net_device		*bond;
+	bool				lag_tx_active;
 };
 
 struct ocelot {
-- 
2.25.1

[RFC PATCH net-next 15/16] net: dsa: felix: propagate the LAG offload ops towards the ocelot lib

[Vladimir Oltean]

The ocelot switch has been supporting LAG offload since its initial
commit, however felix could not make use of that, due to lack of a LAG
abstraction in DSA. Now that we have that, let's forward DSA's calls
towards the ocelot library, who will deal with setting up the bonding.

Note that ocelot_port_lag_leave can return an error due to memory
allocation but we are currently ignoring that, because the DSA method
returns void.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c     | 27 +++++++++++++++++++++++++++
 drivers/net/ethernet/mscc/ocelot.c |  1 +
 drivers/net/ethernet/mscc/ocelot.h |  6 ------
 include/soc/mscc/ocelot.h          |  6 ++++++
 4 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 7dc230677b78..53ed182fac12 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -112,6 +112,30 @@ static void felix_bridge_leave(struct dsa_switch *ds, int port,
 	ocelot_port_bridge_leave(ocelot, port, br);
 }
 
+static int felix_lag_join(struct dsa_switch *ds, int port,
+			  struct net_device *lag_dev)
+{
+	struct ocelot *ocelot = ds->priv;
+
+	return ocelot_port_lag_join(ocelot, port, lag_dev);
+}
+
+static void felix_lag_leave(struct dsa_switch *ds, int port,
+			    struct net_device *lag_dev)
+{
+	struct ocelot *ocelot = ds->priv;
+
+	ocelot_port_lag_leave(ocelot, port, lag_dev);
+}
+
+static int felix_lag_change(struct dsa_switch *ds, int port,
+			    struct netdev_lag_lower_state_info *linfo)
+{
+	struct ocelot *ocelot = ds->priv;
+
+	return ocelot_port_lag_change(ocelot, port, linfo);
+}
+
 static int felix_vlan_prepare(struct dsa_switch *ds, int port,
 			      const struct switchdev_obj_port_vlan *vlan)
 {
@@ -803,6 +827,9 @@ const struct dsa_switch_ops felix_switch_ops = {
 	.port_mdb_del		= felix_mdb_del,
 	.port_bridge_join	= felix_bridge_join,
 	.port_bridge_leave	= felix_bridge_leave,
+	.port_lag_join		= felix_lag_join,
+	.port_lag_leave		= felix_lag_leave,
+	.port_lag_change	= felix_lag_change,
 	.port_stp_state_set	= felix_bridge_stp_state_set,
 	.port_vlan_prepare	= felix_vlan_prepare,
 	.port_vlan_filtering	= felix_vlan_filtering,
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 5c71d121048d..cd7a2e558301 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1381,6 +1381,7 @@ int ocelot_port_lag_change(struct ocelot *ocelot, int port,
 	/* Rebalance the LAGs */
 	return ocelot_set_aggr_pgids(ocelot);
 }
+EXPORT_SYMBOL(ocelot_port_lag_change);
 
 /* Configure the maximum SDU (L2 payload) on RX to the value specified in @sdu.
  * The length of VLAN tags is accounted for automatically via DEV_MAC_TAGS_CFG.
diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
index 0860125b623c..3141ccde6a66 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -112,12 +112,6 @@ int ocelot_mact_learn(struct ocelot *ocelot, int port,
 		      unsigned int vid, enum macaccess_entry_type type);
 int ocelot_mact_forget(struct ocelot *ocelot,
 		       const unsigned char mac[ETH_ALEN], unsigned int vid);
-int ocelot_port_lag_join(struct ocelot *ocelot, int port,
-			 struct net_device *bond);
-int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
-			  struct net_device *bond);
-int ocelot_port_lag_change(struct ocelot *ocelot, int port,
-			   struct netdev_lag_lower_state_info *info);
 struct net_device *ocelot_port_to_netdev(struct ocelot *ocelot, int port);
 int ocelot_netdev_to_port(struct net_device *dev);
 
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 8a44b9064789..7c104f08796d 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -780,5 +780,11 @@ int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
 			const struct switchdev_obj_port_mdb *mdb);
 int ocelot_port_mdb_del(struct ocelot *ocelot, int port,
 			const struct switchdev_obj_port_mdb *mdb);
+int ocelot_port_lag_join(struct ocelot *ocelot, int port,
+			 struct net_device *bond);
+int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
+			  struct net_device *bond);
+int ocelot_port_lag_change(struct ocelot *ocelot, int port,
+			   struct netdev_lag_lower_state_info *info);
 
 #endif
-- 
2.25.1

[RFC PATCH net-next 16/16] net: dsa: ocelot: tell DSA that we can offload link aggregation

[Vladimir Oltean]

For preallocation purposes, we need to specify the maximum number of
individual bonding/team devices that we can offload, which in our case
is equal to the number of physical interfaces.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 53ed182fac12..ad73aaa4457c 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -653,6 +653,7 @@ static int felix_setup(struct dsa_switch *ds)
 
 	ds->mtu_enforcement_ingress = true;
 	ds->configure_vlan_while_not_filtering = true;
+	ds->num_lags = ds->num_ports;
 
 	return 0;
 }
-- 
2.25.1

Re: [RFC PATCH net-next 01/16] net: mscc: ocelot: offload bridge port flags to device

[Alexandre Belloni]

On 08/12/2020 14:07:47+0200, Vladimir Oltean wrote:
> We should not be unconditionally enabling address learning, since doing
> that is actively detrimential when a port is standalone and not offloading
> a bridge. Namely, if a port in the switch is standalone and others are
> offloading the bridge, then we could enter a situation where we learn an
> address towards the standalone port, but the bridged ports could not
> forward the packet there, because the CPU is the only path between the
> standalone and the bridged ports. The solution of course is to not
> enable address learning unless the bridge asks for it. Currently this is
> the only bridge port flag we are looking at. The others (flooding etc)
> are TBD.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot.c     | 21 ++++++++++++++++++++-
>  drivers/net/ethernet/mscc/ocelot.h     |  3 +++
>  drivers/net/ethernet/mscc/ocelot_net.c |  4 ++++
>  include/soc/mscc/ocelot.h              |  2 ++
>  4 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index b9626eec8db6..7a5c534099d3 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -883,6 +883,7 @@ EXPORT_SYMBOL(ocelot_get_ts_info);
>  
>  void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
>  {
> +	struct ocelot_port *ocelot_port = ocelot->ports[port];
>  	u32 port_cfg;
>  	int p, i;
>  
> @@ -896,7 +897,8 @@ void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
>  		ocelot->bridge_fwd_mask |= BIT(port);
>  		fallthrough;
>  	case BR_STATE_LEARNING:
> -		port_cfg |= ANA_PORT_PORT_CFG_LEARN_ENA;
> +		if (ocelot_port->brport_flags & BR_LEARNING)
> +			port_cfg |= ANA_PORT_PORT_CFG_LEARN_ENA;
>  		break;
>  
>  	default:
> @@ -1178,6 +1180,7 @@ EXPORT_SYMBOL(ocelot_port_bridge_join);
>  int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
>  			     struct net_device *bridge)
>  {
> +	struct ocelot_port *ocelot_port = ocelot->ports[port];
>  	struct ocelot_vlan pvid = {0}, native_vlan = {0};
>  	struct switchdev_trans trans;
>  	int ret;
> @@ -1200,6 +1203,10 @@ int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
>  	ocelot_port_set_pvid(ocelot, port, pvid);
>  	ocelot_port_set_native_vlan(ocelot, port, native_vlan);
>  
> +	ocelot_port->brport_flags = 0;
> +	ocelot_rmw_gix(ocelot, 0, ANA_PORT_PORT_CFG_LEARN_ENA,
> +		       ANA_PORT_PORT_CFG, port);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(ocelot_port_bridge_leave);
> @@ -1391,6 +1398,18 @@ int ocelot_get_max_mtu(struct ocelot *ocelot, int port)
>  }
>  EXPORT_SYMBOL(ocelot_get_max_mtu);
>  
> +void ocelot_port_bridge_flags(struct ocelot *ocelot, int port,
> +			      unsigned long flags,
> +			      struct switchdev_trans *trans)
> +{
> +	struct ocelot_port *ocelot_port = ocelot->ports[port];
> +
> +	if (switchdev_trans_ph_prepare(trans))
> +		return;
> +
> +	ocelot_port->brport_flags = flags;
> +}
> +
>  void ocelot_init_port(struct ocelot *ocelot, int port)
>  {
>  	struct ocelot_port *ocelot_port = ocelot->ports[port];
> diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
> index 291d39d49c4e..739bd201d951 100644
> --- a/drivers/net/ethernet/mscc/ocelot.h
> +++ b/drivers/net/ethernet/mscc/ocelot.h
> @@ -102,6 +102,9 @@ struct ocelot_multicast {
>  	struct ocelot_pgid *pgid;
>  };
>  
> +void ocelot_port_bridge_flags(struct ocelot *ocelot, int port,
> +			      unsigned long flags,
> +			      struct switchdev_trans *trans);
>  int ocelot_port_fdb_do_dump(const unsigned char *addr, u16 vid,
>  			    bool is_static, void *data);
>  int ocelot_mact_learn(struct ocelot *ocelot, int port,
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 9ba7e2b166e9..93ecd5274156 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -882,6 +882,10 @@ static int ocelot_port_attr_set(struct net_device *dev,
>  	case SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED:
>  		ocelot_port_attr_mc_set(ocelot, port, !attr->u.mc_disabled);
>  		break;
> +	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
> +		ocelot_port_bridge_flags(ocelot, port, attr->u.brport_flags,
> +					 trans);
> +		break;
>  	default:
>  		err = -EOPNOTSUPP;
>  		break;
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 2f4cd3288bcc..50514c087231 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -581,6 +581,8 @@ struct ocelot_port {
>  
>  	struct regmap			*target;
>  
> +	unsigned long			brport_flags;
> +
>  	bool				vlan_aware;
>  	/* VLAN that untagged frames are classified to, on ingress */
>  	struct ocelot_vlan		pvid_vlan;
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 02/16] net: mscc: ocelot: allow offloading of bridge on top of LAG

[Alexandre Belloni]

On 08/12/2020 14:07:48+0200, Vladimir Oltean wrote:
> Commit 7afb3e575e5a ("net: mscc: ocelot: don't handle netdev events for
> other netdevs") was too aggressive, and it made ocelot_netdevice_event
> react only to network interface events emitted for the ocelot switch
> ports.
> 
> In fact, only the PRECHANGEUPPER should have had that check.
> 
> When we ignore all events that are not for us, we miss the fact that the
> upper of the LAG changes, and the bonding interface gets enslaved to a
> bridge. This is an operation we could offload under certain conditions.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot_net.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 93ecd5274156..6fb2a813e694 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -1047,10 +1047,8 @@ static int ocelot_netdevice_event(struct notifier_block *unused,
>  	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>  	int ret = 0;
>  
> -	if (!ocelot_netdevice_dev_check(dev))
> -		return 0;
> -
>  	if (event == NETDEV_PRECHANGEUPPER &&
> +	    ocelot_netdevice_dev_check(dev) &&
>  	    netif_is_lag_master(info->upper_dev)) {
>  		struct netdev_lag_upper_info *lag_upper_info = info->upper_info;
>  		struct netlink_ext_ack *extack;
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 03/16] net: mscc: ocelot: rename ocelot_netdevice_port_event to ocelot_netdevice_changeupper

[Alexandre Belloni]

Hi,

On 08/12/2020 14:07:49+0200, Vladimir Oltean wrote:
> -static int ocelot_netdevice_port_event(struct net_device *dev,
> -				       unsigned long event,
> -				       struct netdev_notifier_changeupper_info *info)
> +static int ocelot_netdevice_changeupper(struct net_device *dev,
> +					struct netdev_notifier_changeupper_info *info)

[...]

> -		netdev_for_each_lower_dev(dev, slave, iter) {
> -			ret = ocelot_netdevice_port_event(slave, event, info);
> -			if (ret)
> -				goto notify;
> +			netdev_for_each_lower_dev(dev, slave, iter) {
> +				ret = ocelot_netdevice_changeupper(slave, event, info);
> +				if (ret)
> +					goto notify;
> +			}
> +		} else {
> +			ret = ocelot_netdevice_changeupper(dev, event, info);

Does that compile? Shouldn't event be dropped?


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 03/16] net: mscc: ocelot: rename ocelot_netdevice_port_event to ocelot_netdevice_changeupper

[Vladimir Oltean]

On Tue, Dec 15, 2020 at 04:01:32PM +0100, Alexandre Belloni wrote:
> Hi,
> 
> On 08/12/2020 14:07:49+0200, Vladimir Oltean wrote:
> > -static int ocelot_netdevice_port_event(struct net_device *dev,
> > -				       unsigned long event,
> > -				       struct netdev_notifier_changeupper_info *info)
> > +static int ocelot_netdevice_changeupper(struct net_device *dev,
> > +					struct netdev_notifier_changeupper_info *info)
> 
> [...]
> 
> > -		netdev_for_each_lower_dev(dev, slave, iter) {
> > -			ret = ocelot_netdevice_port_event(slave, event, info);
> > -			if (ret)
> > -				goto notify;
> > +			netdev_for_each_lower_dev(dev, slave, iter) {
> > +				ret = ocelot_netdevice_changeupper(slave, event, info);
> > +				if (ret)
> > +					goto notify;
> > +			}
> > +		} else {
> > +			ret = ocelot_netdevice_changeupper(dev, event, info);
> 
> Does that compile?

No it doesn't.

> Shouldn't event be dropped?

It is, but in the next patch. I'll fix it, thanks.

Re: [RFC PATCH net-next 04/16] net: mscc: ocelot: use a switch-case statement in ocelot_netdevice_event

[Alexandre Belloni]

On 08/12/2020 14:07:50+0200, Vladimir Oltean wrote:
> Make ocelot's net device event handler more streamlined by structuring
> it in a similar way with others. The inspiration here was
> dsa_slave_netdevice_event.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/mscc/ocelot_net.c | 68 +++++++++++++++++---------
>  1 file changed, 45 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 50765a3b1c44..47b620967156 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -1030,49 +1030,71 @@ static int ocelot_netdevice_changeupper(struct net_device *dev,
>  					      info->upper_dev);
>  	}
>  
> -	return err;
> +	return notifier_from_errno(err);
> +}
> +
> +static int
> +ocelot_netdevice_lag_changeupper(struct net_device *dev,
> +				 struct netdev_notifier_changeupper_info *info)
> +{
> +	struct net_device *lower;
> +	struct list_head *iter;
> +	int err = NOTIFY_DONE;
> +
> +	netdev_for_each_lower_dev(dev, lower, iter) {
> +		err = ocelot_netdevice_changeupper(lower, info);
> +		if (err)
> +			return notifier_from_errno(err);
> +	}
> +
> +	return NOTIFY_DONE;
>  }
>  
>  static int ocelot_netdevice_event(struct notifier_block *unused,
>  				  unsigned long event, void *ptr)
>  {
> -	struct netdev_notifier_changeupper_info *info = ptr;
>  	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> -	int ret = 0;
>  
> -	if (event == NETDEV_PRECHANGEUPPER &&
> -	    ocelot_netdevice_dev_check(dev) &&
> -	    netif_is_lag_master(info->upper_dev)) {
> -		struct netdev_lag_upper_info *lag_upper_info = info->upper_info;
> +	switch (event) {
> +	case NETDEV_PRECHANGEUPPER: {
> +		struct netdev_notifier_changeupper_info *info = ptr;
> +		struct netdev_lag_upper_info *lag_upper_info;
>  		struct netlink_ext_ack *extack;
>  
> +		if (!ocelot_netdevice_dev_check(dev))
> +			break;
> +
> +		if (!netif_is_lag_master(info->upper_dev))
> +			break;
> +
> +		lag_upper_info = info->upper_info;
> +
>  		if (lag_upper_info &&
>  		    lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH) {
>  			extack = netdev_notifier_info_to_extack(&info->info);
>  			NL_SET_ERR_MSG_MOD(extack, "LAG device using unsupported Tx type");
>  
> -			ret = -EINVAL;
> -			goto notify;
> +			return NOTIFY_BAD;

This changes the return value in case of error, I'm not sure how
important this is.

>  		}
> +
> +		break;
>  	}
> +	case NETDEV_CHANGEUPPER: {
> +		struct netdev_notifier_changeupper_info *info = ptr;
>  
> -	if (event == NETDEV_CHANGEUPPER) {
> -		if (netif_is_lag_master(dev)) {
> -			struct net_device *slave;
> -			struct list_head *iter;
> +		if (ocelot_netdevice_dev_check(dev))
> +			return ocelot_netdevice_changeupper(dev, info);
>  
> -			netdev_for_each_lower_dev(dev, slave, iter) {
> -				ret = ocelot_netdevice_changeupper(slave, event, info);
> -				if (ret)
> -					goto notify;
> -			}
> -		} else {
> -			ret = ocelot_netdevice_changeupper(dev, event, info);
> -		}
> +		if (netif_is_lag_master(dev))
> +			return ocelot_netdevice_lag_changeupper(dev, info);
> +
> +		break;
> +	}
> +	default:
> +		break;
>  	}
>  
> -notify:
> -	return notifier_from_errno(ret);
> +	return NOTIFY_DONE;

This changes the return value from NOTIFY_OK to NOTIFY_DONE but this is
probably what we want.

>  }
>  
>  struct notifier_block ocelot_netdevice_nb __read_mostly = {
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 04/16] net: mscc: ocelot: use a switch-case statement in ocelot_netdevice_event

[Alexandre Belloni]

On 15/12/2020 16:52:26+0100, Alexandre Belloni wrote:
> On 08/12/2020 14:07:50+0200, Vladimir Oltean wrote:
> > Make ocelot's net device event handler more streamlined by structuring
> > it in a similar way with others. The inspiration here was
> > dsa_slave_netdevice_event.
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  drivers/net/ethernet/mscc/ocelot_net.c | 68 +++++++++++++++++---------
> >  1 file changed, 45 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> > index 50765a3b1c44..47b620967156 100644
> > --- a/drivers/net/ethernet/mscc/ocelot_net.c
> > +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> > @@ -1030,49 +1030,71 @@ static int ocelot_netdevice_changeupper(struct net_device *dev,
> >  					      info->upper_dev);
> >  	}
> >  
> > -	return err;
> > +	return notifier_from_errno(err);
> > +}
> > +
> > +static int
> > +ocelot_netdevice_lag_changeupper(struct net_device *dev,
> > +				 struct netdev_notifier_changeupper_info *info)
> > +{
> > +	struct net_device *lower;
> > +	struct list_head *iter;
> > +	int err = NOTIFY_DONE;
> > +
> > +	netdev_for_each_lower_dev(dev, lower, iter) {
> > +		err = ocelot_netdevice_changeupper(lower, info);
> > +		if (err)
> > +			return notifier_from_errno(err);
> > +	}
> > +
> > +	return NOTIFY_DONE;
> >  }
> >  
> >  static int ocelot_netdevice_event(struct notifier_block *unused,
> >  				  unsigned long event, void *ptr)
> >  {
> > -	struct netdev_notifier_changeupper_info *info = ptr;
> >  	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> > -	int ret = 0;
> >  
> > -	if (event == NETDEV_PRECHANGEUPPER &&
> > -	    ocelot_netdevice_dev_check(dev) &&
> > -	    netif_is_lag_master(info->upper_dev)) {
> > -		struct netdev_lag_upper_info *lag_upper_info = info->upper_info;
> > +	switch (event) {
> > +	case NETDEV_PRECHANGEUPPER: {
> > +		struct netdev_notifier_changeupper_info *info = ptr;
> > +		struct netdev_lag_upper_info *lag_upper_info;
> >  		struct netlink_ext_ack *extack;
> >  
> > +		if (!ocelot_netdevice_dev_check(dev))
> > +			break;
> > +
> > +		if (!netif_is_lag_master(info->upper_dev))
> > +			break;
> > +
> > +		lag_upper_info = info->upper_info;
> > +
> >  		if (lag_upper_info &&
> >  		    lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH) {
> >  			extack = netdev_notifier_info_to_extack(&info->info);
> >  			NL_SET_ERR_MSG_MOD(extack, "LAG device using unsupported Tx type");
> >  
> > -			ret = -EINVAL;
> > -			goto notify;
> > +			return NOTIFY_BAD;
> 
> This changes the return value in case of error, I'm not sure how
> important this is.
> 

Ok, this is removed anyway, so

Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>


> >  		}
> > +
> > +		break;
> >  	}
> > +	case NETDEV_CHANGEUPPER: {
> > +		struct netdev_notifier_changeupper_info *info = ptr;
> >  
> > -	if (event == NETDEV_CHANGEUPPER) {
> > -		if (netif_is_lag_master(dev)) {
> > -			struct net_device *slave;
> > -			struct list_head *iter;
> > +		if (ocelot_netdevice_dev_check(dev))
> > +			return ocelot_netdevice_changeupper(dev, info);
> >  
> > -			netdev_for_each_lower_dev(dev, slave, iter) {
> > -				ret = ocelot_netdevice_changeupper(slave, event, info);
> > -				if (ret)
> > -					goto notify;
> > -			}
> > -		} else {
> > -			ret = ocelot_netdevice_changeupper(dev, event, info);
> > -		}
> > +		if (netif_is_lag_master(dev))
> > +			return ocelot_netdevice_lag_changeupper(dev, info);
> > +
> > +		break;
> > +	}
> > +	default:
> > +		break;
> >  	}
> >  
> > -notify:
> > -	return notifier_from_errno(ret);
> > +	return NOTIFY_DONE;
> 
> This changes the return value from NOTIFY_OK to NOTIFY_DONE but this is
> probably what we want.
> 
> >  }
> >  
> >  struct notifier_block ocelot_netdevice_nb __read_mostly = {
> > -- 
> > 2.25.1
> > 
> 
> -- 
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 05/16] net: mscc: ocelot: don't refuse bonding interfaces we can't offload

[Alexandre Belloni]

On 08/12/2020 14:07:51+0200, Vladimir Oltean wrote:
> Since switchdev/DSA exposes network interfaces that fulfill many of the
> same user space expectations that dedicated NICs do, it makes sense to
> not deny bonding interfaces with a bonding policy that we cannot offload,
> but instead allow the bonding driver to select the egress interface in
> software.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot_net.c | 38 ++++++++++----------------
>  1 file changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 47b620967156..77957328722a 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -1022,6 +1022,15 @@ static int ocelot_netdevice_changeupper(struct net_device *dev,
>  		}
>  	}
>  	if (netif_is_lag_master(info->upper_dev)) {
> +		struct netdev_lag_upper_info *lag_upper_info;
> +
> +		lag_upper_info = info->upper_info;
> +
> +		/* Only offload what we can */
> +		if (lag_upper_info &&
> +		    lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH)
> +			return NOTIFY_DONE;
> +
>  		if (info->linking)
>  			err = ocelot_port_lag_join(ocelot, port,
>  						   info->upper_dev);
> @@ -1037,10 +1046,16 @@ static int
>  ocelot_netdevice_lag_changeupper(struct net_device *dev,
>  				 struct netdev_notifier_changeupper_info *info)
>  {
> +	struct netdev_lag_upper_info *lag_upper_info = info->upper_info;
>  	struct net_device *lower;
>  	struct list_head *iter;
>  	int err = NOTIFY_DONE;
>  
> +	/* Can't offload LAG => also do bridging in software */
> +	if (lag_upper_info &&
> +	    lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH)
> +		return NOTIFY_DONE;
> +
>  	netdev_for_each_lower_dev(dev, lower, iter) {
>  		err = ocelot_netdevice_changeupper(lower, info);
>  		if (err)
> @@ -1056,29 +1071,6 @@ static int ocelot_netdevice_event(struct notifier_block *unused,
>  	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>  
>  	switch (event) {
> -	case NETDEV_PRECHANGEUPPER: {
> -		struct netdev_notifier_changeupper_info *info = ptr;
> -		struct netdev_lag_upper_info *lag_upper_info;
> -		struct netlink_ext_ack *extack;
> -
> -		if (!ocelot_netdevice_dev_check(dev))
> -			break;
> -
> -		if (!netif_is_lag_master(info->upper_dev))
> -			break;
> -
> -		lag_upper_info = info->upper_info;
> -
> -		if (lag_upper_info &&
> -		    lag_upper_info->tx_type != NETDEV_LAG_TX_TYPE_HASH) {
> -			extack = netdev_notifier_info_to_extack(&info->info);
> -			NL_SET_ERR_MSG_MOD(extack, "LAG device using unsupported Tx type");
> -
> -			return NOTIFY_BAD;
> -		}
> -
> -		break;
> -	}
>  	case NETDEV_CHANGEUPPER: {
>  		struct netdev_notifier_changeupper_info *info = ptr;
>  
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 06/16] net: mscc: ocelot: use ipv6 in the aggregation code

[Alexandre Belloni]

On 08/12/2020 14:07:52+0200, Vladimir Oltean wrote:
> IPv6 header information is not currently part of the entropy source for
> the 4-bit aggregation code used for LAG offload, even though it could be.
> The hardware reference manual says about these fields:
> 
> ANA::AGGR_CFG.AC_IP6_TCPUDP_PORT_ENA
> Use IPv6 TCP/UDP port when calculating aggregation code. Configure
> identically for all ports. Recommended value is 1.
> 
> ANA::AGGR_CFG.AC_IP6_FLOW_LBL_ENA
> Use IPv6 flow label when calculating AC. Configure identically for all
> ports. Recommended value is 1.
> 
> Integration with the xmit_hash_policy of the bonding interface is TBD.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 7a5c534099d3..13e86dd71e5a 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -1557,7 +1557,10 @@ int ocelot_init(struct ocelot *ocelot)
>  	ocelot_write(ocelot, ANA_AGGR_CFG_AC_SMAC_ENA |
>  			     ANA_AGGR_CFG_AC_DMAC_ENA |
>  			     ANA_AGGR_CFG_AC_IP4_SIPDIP_ENA |
> -			     ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA, ANA_AGGR_CFG);
> +			     ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA |
> +			     ANA_AGGR_CFG_AC_IP6_FLOW_LBL_ENA |
> +			     ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA,
> +			     ANA_AGGR_CFG);
>  
>  	/* Set MAC age time to default value. The entry is aged after
>  	 * 2*AGE_PERIOD
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 07/16] net: mscc: ocelot: set up the bonding mask in a way that avoids a net_device

[Alexandre Belloni]

On 08/12/2020 14:07:53+0200, Vladimir Oltean wrote:
> Since this code should be called from pure switchdev as well as from
> DSA, we must find a way to determine the bonding mask not by looking
> directly at the net_device lowers of the bonding interface, since those
> could have different private structures.
> 
> We keep a pointer to the bonding upper interface, if present, in struct
> ocelot_port. Then the bonding mask becomes the bitwise OR of all ports
> that have the same bonding upper interface. This adds a duplication of
> functionality with the current "lags" array, but the duplication will be
> short-lived, since further patches will remove the latter completely.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot.c | 29 ++++++++++++++++++++++-------
>  include/soc/mscc/ocelot.h          |  2 ++
>  2 files changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 13e86dd71e5a..30dee1f957d1 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -881,6 +881,24 @@ int ocelot_get_ts_info(struct ocelot *ocelot, int port,
>  }
>  EXPORT_SYMBOL(ocelot_get_ts_info);
>  
> +static u32 ocelot_get_bond_mask(struct ocelot *ocelot, struct net_device *bond)
> +{
> +	u32 bond_mask = 0;
> +	int port;
> +
> +	for (port = 0; port < ocelot->num_phys_ports; port++) {
> +		struct ocelot_port *ocelot_port = ocelot->ports[port];
> +
> +		if (!ocelot_port)
> +			continue;
> +
> +		if (ocelot_port->bond == bond)
> +			bond_mask |= BIT(port);
> +	}
> +
> +	return bond_mask;
> +}
> +
>  void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
>  {
>  	struct ocelot_port *ocelot_port = ocelot->ports[port];
> @@ -1272,17 +1290,12 @@ static void ocelot_setup_lag(struct ocelot *ocelot, int lag)
>  int ocelot_port_lag_join(struct ocelot *ocelot, int port,
>  			 struct net_device *bond)
>  {
> -	struct net_device *ndev;
>  	u32 bond_mask = 0;
>  	int lag, lp;
>  
> -	rcu_read_lock();
> -	for_each_netdev_in_bond_rcu(bond, ndev) {
> -		struct ocelot_port_private *priv = netdev_priv(ndev);
> +	ocelot->ports[port]->bond = bond;
>  
> -		bond_mask |= BIT(priv->chip_port);
> -	}
> -	rcu_read_unlock();
> +	bond_mask = ocelot_get_bond_mask(ocelot, bond);
>  
>  	lp = __ffs(bond_mask);
>  
> @@ -1315,6 +1328,8 @@ void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
>  	u32 port_cfg;
>  	int i;
>  
> +	ocelot->ports[port]->bond = NULL;
> +
>  	/* Remove port from any lag */
>  	for (i = 0; i < ocelot->num_phys_ports; i++)
>  		ocelot->lags[i] &= ~BIT(port);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 50514c087231..b812bdff1da1 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -597,6 +597,8 @@ struct ocelot_port {
>  	phy_interface_t			phy_mode;
>  
>  	u8				*xmit_template;
> +
> +	struct net_device		*bond;
>  };
>  
>  struct ocelot {
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 08/16] net: mscc: ocelot: avoid unneeded "lp" variable in LAG join

[Alexandre Belloni]

On 08/12/2020 14:07:54+0200, Vladimir Oltean wrote:
> The index of the LAG is equal to the logical port ID that all the
> physical port members have, which is further equal to the index of the
> first physical port that is a member of the LAG.
> 
> The code gets a bit carried away with logic like this:
> 
> 	if (a == b)
> 		c = a;
> 	else
> 		c = b;
> 
> which can be simplified, of course, into:
> 
> 	c = b;
> 
> (with a being port, b being lp, c being lag)
> 
> This further makes the "lp" variable redundant, since we can use "lag"
> everywhere where "lp" (logical port) was used. So instead of a "c = b"
> assignment, we can do a complete deletion of b. Only one comment here:
> 
> 		if (bond_mask) {
> 			lp = __ffs(bond_mask);
> 			ocelot->lags[lp] = 0;
> 		}
> 
> lp was clobbered before, because it was used as a temporary variable to
> hold the new smallest port ID from the bond. Now that we don't have "lp"
> any longer, we'll just avoid the temporary variable and zeroize the
> bonding mask directly.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 30dee1f957d1..080fd4ce37ea 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -1291,28 +1291,24 @@ int ocelot_port_lag_join(struct ocelot *ocelot, int port,
>  			 struct net_device *bond)
>  {
>  	u32 bond_mask = 0;
> -	int lag, lp;
> +	int lag;
>  
>  	ocelot->ports[port]->bond = bond;
>  
>  	bond_mask = ocelot_get_bond_mask(ocelot, bond);
>  
> -	lp = __ffs(bond_mask);
> +	lag = __ffs(bond_mask);
>  
>  	/* If the new port is the lowest one, use it as the logical port from
>  	 * now on
>  	 */
> -	if (port == lp) {
> -		lag = port;
> +	if (port == lag) {
>  		ocelot->lags[port] = bond_mask;
>  		bond_mask &= ~BIT(port);
> -		if (bond_mask) {
> -			lp = __ffs(bond_mask);
> -			ocelot->lags[lp] = 0;
> -		}
> +		if (bond_mask)
> +			ocelot->lags[__ffs(bond_mask)] = 0;
>  	} else {
> -		lag = lp;
> -		ocelot->lags[lp] |= BIT(port);
> +		ocelot->lags[lag] |= BIT(port);
>  	}
>  
>  	ocelot_setup_lag(ocelot, lag);
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 09/16] net: mscc: ocelot: use "lag" variable name in ocelot_bridge_stp_state_set

[Alexandre Belloni]

On 08/12/2020 14:07:55+0200, Vladimir Oltean wrote:
> In anticipation of further simplification, make it more clear what we're
> iterating over.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 080fd4ce37ea..c3c6682e6e79 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -903,7 +903,7 @@ void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
>  {
>  	struct ocelot_port *ocelot_port = ocelot->ports[port];
>  	u32 port_cfg;
> -	int p, i;
> +	int p;
>  
>  	if (!(BIT(port) & ocelot->bridge_mask))
>  		return;
> @@ -928,14 +928,17 @@ void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
>  	ocelot_write_gix(ocelot, port_cfg, ANA_PORT_PORT_CFG, port);
>  
>  	/* Apply FWD mask. The loop is needed to add/remove the current port as
> -	 * a source for the other ports.
> +	 * a source for the other ports. If the source port is in a bond, then
> +	 * all the other ports from that bond need to be removed from this
> +	 * source port's forwarding mask.
>  	 */
>  	for (p = 0; p < ocelot->num_phys_ports; p++) {
>  		if (ocelot->bridge_fwd_mask & BIT(p)) {
>  			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(p);
> +			int lag;
>  
> -			for (i = 0; i < ocelot->num_phys_ports; i++) {
> -				unsigned long bond_mask = ocelot->lags[i];
> +			for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
> +				unsigned long bond_mask = ocelot->lags[lag];
>  
>  				if (!bond_mask)
>  					continue;
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 10/16] net: mscc: ocelot: reapply bridge forwarding mask on bonding join/leave

[Alexandre Belloni]

On 08/12/2020 14:07:56+0200, Vladimir Oltean wrote:
> Applying the bridge forwarding mask currently is done only on the STP
> state changes for any port. But it depends on both STP state changes,
> and bonding interface state changes. Export the bit that recalculates
> the forwarding mask so that it could be reused, and call it when a port
> starts and stops offloading a bonding interface.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot.c | 68 +++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index c3c6682e6e79..ee0fcee8e09a 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -899,11 +899,45 @@ static u32 ocelot_get_bond_mask(struct ocelot *ocelot, struct net_device *bond)
>  	return bond_mask;
>  }
>  
> +static void ocelot_apply_bridge_fwd_mask(struct ocelot *ocelot)
> +{
> +	int port;
> +
> +	/* Apply FWD mask. The loop is needed to add/remove the current port as
> +	 * a source for the other ports. If the source port is in a bond, then
> +	 * all the other ports from that bond need to be removed from this
> +	 * source port's forwarding mask.
> +	 */
> +	for (port = 0; port < ocelot->num_phys_ports; port++) {
> +		if (ocelot->bridge_fwd_mask & BIT(port)) {
> +			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(port);
> +			int lag;
> +
> +			for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
> +				unsigned long bond_mask = ocelot->lags[lag];
> +
> +				if (!bond_mask)
> +					continue;
> +
> +				if (bond_mask & BIT(port)) {
> +					mask &= ~bond_mask;
> +					break;
> +				}
> +			}
> +
> +			ocelot_write_rix(ocelot, mask,
> +					 ANA_PGID_PGID, PGID_SRC + port);
> +		} else {
> +			ocelot_write_rix(ocelot, 0,
> +					 ANA_PGID_PGID, PGID_SRC + port);
> +		}
> +	}
> +}
> +
>  void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
>  {
>  	struct ocelot_port *ocelot_port = ocelot->ports[port];
>  	u32 port_cfg;
> -	int p;
>  
>  	if (!(BIT(port) & ocelot->bridge_mask))
>  		return;
> @@ -927,35 +961,7 @@ void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port, u8 state)
>  
>  	ocelot_write_gix(ocelot, port_cfg, ANA_PORT_PORT_CFG, port);
>  
> -	/* Apply FWD mask. The loop is needed to add/remove the current port as
> -	 * a source for the other ports. If the source port is in a bond, then
> -	 * all the other ports from that bond need to be removed from this
> -	 * source port's forwarding mask.
> -	 */
> -	for (p = 0; p < ocelot->num_phys_ports; p++) {
> -		if (ocelot->bridge_fwd_mask & BIT(p)) {
> -			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(p);
> -			int lag;
> -
> -			for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
> -				unsigned long bond_mask = ocelot->lags[lag];
> -
> -				if (!bond_mask)
> -					continue;
> -
> -				if (bond_mask & BIT(p)) {
> -					mask &= ~bond_mask;
> -					break;
> -				}
> -			}
> -
> -			ocelot_write_rix(ocelot, mask,
> -					 ANA_PGID_PGID, PGID_SRC + p);
> -		} else {
> -			ocelot_write_rix(ocelot, 0,
> -					 ANA_PGID_PGID, PGID_SRC + p);
> -		}
> -	}
> +	ocelot_apply_bridge_fwd_mask(ocelot);
>  }
>  EXPORT_SYMBOL(ocelot_bridge_stp_state_set);
>  
> @@ -1315,6 +1321,7 @@ int ocelot_port_lag_join(struct ocelot *ocelot, int port,
>  	}
>  
>  	ocelot_setup_lag(ocelot, lag);
> +	ocelot_apply_bridge_fwd_mask(ocelot);
>  	ocelot_set_aggr_pgids(ocelot);
>  
>  	return 0;
> @@ -1350,6 +1357,7 @@ void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
>  	ocelot_write_gix(ocelot, port_cfg | ANA_PORT_PORT_CFG_PORTID_VAL(port),
>  			 ANA_PORT_PORT_CFG, port);
>  
> +	ocelot_apply_bridge_fwd_mask(ocelot);
>  	ocelot_set_aggr_pgids(ocelot);
>  }
>  EXPORT_SYMBOL(ocelot_port_lag_leave);
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 11/16] net: mscc: ocelot: set up logical port IDs centrally

[Alexandre Belloni]

On 08/12/2020 14:07:57+0200, Vladimir Oltean wrote:
> The setup of logical port IDs is done in two places: from the inconclusively
> named ocelot_setup_lag and from ocelot_port_lag_leave, a function that
> also calls ocelot_setup_lag (which apparently does an incomplete setup
> of the LAG).
> 
> To improve this situation, we can rename ocelot_setup_lag into
> ocelot_setup_logical_port_ids, and drop the "lag" argument. It will now
> set up the logical port IDs of all switch ports, which may be just
> slightly more inefficient but more maintainable.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot.c | 47 ++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index ee0fcee8e09a..1a98c24af056 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -1279,20 +1279,36 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  	}
>  }
>  
> -static void ocelot_setup_lag(struct ocelot *ocelot, int lag)
> +/* When offloading a bonding interface, the switch ports configured under the
> + * same bond must have the same logical port ID, equal to the physical port ID
> + * of the lowest numbered physical port in that bond. Otherwise, in standalone/
> + * bridged mode, each port has a logical port ID equal to its physical port ID.
> + */
> +static void ocelot_setup_logical_port_ids(struct ocelot *ocelot)
>  {
> -	unsigned long bond_mask = ocelot->lags[lag];
> -	unsigned int p;
> +	int port;
>  
> -	for_each_set_bit(p, &bond_mask, ocelot->num_phys_ports) {
> -		u32 port_cfg = ocelot_read_gix(ocelot, ANA_PORT_PORT_CFG, p);
> +	for (port = 0; port < ocelot->num_phys_ports; port++) {
> +		struct ocelot_port *ocelot_port = ocelot->ports[port];
> +		struct net_device *bond;
> +
> +		if (!ocelot_port)
> +			continue;
>  
> -		port_cfg &= ~ANA_PORT_PORT_CFG_PORTID_VAL_M;
> +		bond = ocelot_port->bond;
> +		if (bond) {
> +			int lag = __ffs(ocelot_get_bond_mask(ocelot, bond));
>  
> -		/* Use lag port as logical port for port i */
> -		ocelot_write_gix(ocelot, port_cfg |
> -				 ANA_PORT_PORT_CFG_PORTID_VAL(lag),
> -				 ANA_PORT_PORT_CFG, p);
> +			ocelot_rmw_gix(ocelot,
> +				       ANA_PORT_PORT_CFG_PORTID_VAL(lag),
> +				       ANA_PORT_PORT_CFG_PORTID_VAL_M,
> +				       ANA_PORT_PORT_CFG, port);
> +		} else {
> +			ocelot_rmw_gix(ocelot,
> +				       ANA_PORT_PORT_CFG_PORTID_VAL(port),
> +				       ANA_PORT_PORT_CFG_PORTID_VAL_M,
> +				       ANA_PORT_PORT_CFG, port);
> +		}
>  	}
>  }
>  
> @@ -1320,7 +1336,7 @@ int ocelot_port_lag_join(struct ocelot *ocelot, int port,
>  		ocelot->lags[lag] |= BIT(port);
>  	}
>  
> -	ocelot_setup_lag(ocelot, lag);
> +	ocelot_setup_logical_port_ids(ocelot);
>  	ocelot_apply_bridge_fwd_mask(ocelot);
>  	ocelot_set_aggr_pgids(ocelot);
>  
> @@ -1331,7 +1347,6 @@ EXPORT_SYMBOL(ocelot_port_lag_join);
>  void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
>  			   struct net_device *bond)
>  {
> -	u32 port_cfg;
>  	int i;
>  
>  	ocelot->ports[port]->bond = NULL;
> @@ -1348,15 +1363,9 @@ void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
>  
>  		ocelot->lags[n] = ocelot->lags[port];
>  		ocelot->lags[port] = 0;
> -
> -		ocelot_setup_lag(ocelot, n);
>  	}
>  
> -	port_cfg = ocelot_read_gix(ocelot, ANA_PORT_PORT_CFG, port);
> -	port_cfg &= ~ANA_PORT_PORT_CFG_PORTID_VAL_M;
> -	ocelot_write_gix(ocelot, port_cfg | ANA_PORT_PORT_CFG_PORTID_VAL(port),
> -			 ANA_PORT_PORT_CFG, port);
> -
> +	ocelot_setup_logical_port_ids(ocelot);
>  	ocelot_apply_bridge_fwd_mask(ocelot);
>  	ocelot_set_aggr_pgids(ocelot);
>  }
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 12/16] net: mscc: ocelot: drop the use of the "lags" array

[Alexandre Belloni]

On 08/12/2020 14:07:58+0200, Vladimir Oltean wrote:
> We can now simplify the implementation by always using ocelot_get_bond_mask
> to look up the other ports that are offloading the same bonding interface
> as us.
> 
> In ocelot_set_aggr_pgids, the code had a way to uniquely iterate through
> LAGs. We need to achieve the same behavior by marking each LAG as visited,
> which we do now by temporarily allocating an array of pointers to bonding
> uppers of each port, and marking each bonding upper as NULL once it has
> been treated by the first port that is a member. And because we now do
> some dynamic allocation, we need to propagate errors from
> ocelot_set_aggr_pgid all the way to ocelot_port_lag_leave.

I would say that this allocation never fails but wouldn't it be better
to simply put that on the stack? It is just a bunch of pointers, that
would be 10 on ocelot, maximum would be 64 on jaguar if we ever use the
same driver. This seems reasonable to me, the main issue is that you
have to avoid VLAs.

> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/mscc/ocelot.c     | 104 ++++++++++---------------
>  drivers/net/ethernet/mscc/ocelot.h     |   4 +-
>  drivers/net/ethernet/mscc/ocelot_net.c |   4 +-
>  include/soc/mscc/ocelot.h              |   2 -
>  4 files changed, 47 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 1a98c24af056..d4dbba66aa65 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -909,21 +909,17 @@ static void ocelot_apply_bridge_fwd_mask(struct ocelot *ocelot)
>  	 * source port's forwarding mask.
>  	 */
>  	for (port = 0; port < ocelot->num_phys_ports; port++) {
> -		if (ocelot->bridge_fwd_mask & BIT(port)) {
> -			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(port);
> -			int lag;
> +		struct ocelot_port *ocelot_port = ocelot->ports[port];
>  
> -			for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
> -				unsigned long bond_mask = ocelot->lags[lag];
> +		if (!ocelot_port)
> +			continue;
>  
> -				if (!bond_mask)
> -					continue;
> +		if (ocelot->bridge_fwd_mask & BIT(port)) {
> +			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(port);
> +			struct net_device *bond = ocelot_port->bond;
>  
> -				if (bond_mask & BIT(port)) {
> -					mask &= ~bond_mask;
> -					break;
> -				}
> -			}
> +			if (bond)
> +				mask &= ~ocelot_get_bond_mask(ocelot, bond);
>  
>  			ocelot_write_rix(ocelot, mask,
>  					 ANA_PGID_PGID, PGID_SRC + port);
> @@ -1238,10 +1234,16 @@ int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
>  }
>  EXPORT_SYMBOL(ocelot_port_bridge_leave);
>  
> -static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
> +static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  {
> +	struct net_device **bonds;
>  	int i, port, lag;
>  
> +	bonds = kcalloc(ocelot->num_phys_ports, sizeof(struct net_device *),
> +			GFP_KERNEL);
> +	if (!bonds)
> +		return -ENOMEM;
> +
>  	/* Reset destination and aggregation PGIDS */
>  	for_each_unicast_dest_pgid(ocelot, port)
>  		ocelot_write_rix(ocelot, BIT(port), ANA_PGID_PGID, port);
> @@ -1250,16 +1252,26 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  		ocelot_write_rix(ocelot, GENMASK(ocelot->num_phys_ports - 1, 0),
>  				 ANA_PGID_PGID, i);
>  
> +	for (port = 0; port < ocelot->num_phys_ports; port++) {
> +		struct ocelot_port *ocelot_port = ocelot->ports[port];
> +
> +		if (!ocelot_port)
> +			continue;
> +
> +		bonds[port] = ocelot_port->bond;
> +	}
> +
>  	/* Now, set PGIDs for each LAG */
>  	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
>  		unsigned long bond_mask;
>  		int aggr_count = 0;
>  		u8 aggr_idx[16];
>  
> -		bond_mask = ocelot->lags[lag];
> -		if (!bond_mask)
> +		if (!bonds[lag])
>  			continue;
>  
> +		bond_mask = ocelot_get_bond_mask(ocelot, bonds[lag]);
> +
>  		for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {
>  			// Destination mask
>  			ocelot_write_rix(ocelot, bond_mask,
> @@ -1276,7 +1288,19 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  			ac |= BIT(aggr_idx[i % aggr_count]);
>  			ocelot_write_rix(ocelot, ac, ANA_PGID_PGID, i);
>  		}
> +
> +		/* Mark the bonding interface as visited to avoid applying
> +		 * the same config again
> +		 */
> +		for (i = lag + 1; i < ocelot->num_phys_ports; i++)
> +			if (bonds[i] == bonds[lag])
> +				bonds[i] = NULL;
> +
> +		bonds[lag] = NULL;
>  	}
> +
> +	kfree(bonds);
> +	return 0;
>  }
>  
>  /* When offloading a bonding interface, the switch ports configured under the
> @@ -1315,59 +1339,22 @@ static void ocelot_setup_logical_port_ids(struct ocelot *ocelot)
>  int ocelot_port_lag_join(struct ocelot *ocelot, int port,
>  			 struct net_device *bond)
>  {
> -	u32 bond_mask = 0;
> -	int lag;
> -
>  	ocelot->ports[port]->bond = bond;
>  
> -	bond_mask = ocelot_get_bond_mask(ocelot, bond);
> -
> -	lag = __ffs(bond_mask);
> -
> -	/* If the new port is the lowest one, use it as the logical port from
> -	 * now on
> -	 */
> -	if (port == lag) {
> -		ocelot->lags[port] = bond_mask;
> -		bond_mask &= ~BIT(port);
> -		if (bond_mask)
> -			ocelot->lags[__ffs(bond_mask)] = 0;
> -	} else {
> -		ocelot->lags[lag] |= BIT(port);
> -	}
> -
>  	ocelot_setup_logical_port_ids(ocelot);
>  	ocelot_apply_bridge_fwd_mask(ocelot);
> -	ocelot_set_aggr_pgids(ocelot);
> -
> -	return 0;
> +	return ocelot_set_aggr_pgids(ocelot);
>  }
>  EXPORT_SYMBOL(ocelot_port_lag_join);
>  
> -void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
> -			   struct net_device *bond)
> +int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
> +			  struct net_device *bond)
>  {
> -	int i;
> -
>  	ocelot->ports[port]->bond = NULL;
>  
> -	/* Remove port from any lag */
> -	for (i = 0; i < ocelot->num_phys_ports; i++)
> -		ocelot->lags[i] &= ~BIT(port);
> -
> -	/* if it was the logical port of the lag, move the lag config to the
> -	 * next port
> -	 */
> -	if (ocelot->lags[port]) {
> -		int n = __ffs(ocelot->lags[port]);
> -
> -		ocelot->lags[n] = ocelot->lags[port];
> -		ocelot->lags[port] = 0;
> -	}
> -
>  	ocelot_setup_logical_port_ids(ocelot);
>  	ocelot_apply_bridge_fwd_mask(ocelot);
> -	ocelot_set_aggr_pgids(ocelot);
> +	return ocelot_set_aggr_pgids(ocelot);
>  }
>  EXPORT_SYMBOL(ocelot_port_lag_leave);
>  
> @@ -1543,11 +1530,6 @@ int ocelot_init(struct ocelot *ocelot)
>  		}
>  	}
>  
> -	ocelot->lags = devm_kcalloc(ocelot->dev, ocelot->num_phys_ports,
> -				    sizeof(u32), GFP_KERNEL);
> -	if (!ocelot->lags)
> -		return -ENOMEM;
> -
>  	ocelot->stats = devm_kcalloc(ocelot->dev,
>  				     ocelot->num_phys_ports * ocelot->num_stats,
>  				     sizeof(u64), GFP_KERNEL);
> diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
> index 739bd201d951..bef8d5f8e6e5 100644
> --- a/drivers/net/ethernet/mscc/ocelot.h
> +++ b/drivers/net/ethernet/mscc/ocelot.h
> @@ -114,8 +114,8 @@ int ocelot_mact_forget(struct ocelot *ocelot,
>  		       const unsigned char mac[ETH_ALEN], unsigned int vid);
>  int ocelot_port_lag_join(struct ocelot *ocelot, int port,
>  			 struct net_device *bond);
> -void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
> -			   struct net_device *bond);
> +int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
> +			  struct net_device *bond);
>  struct net_device *ocelot_port_to_netdev(struct ocelot *ocelot, int port);
>  int ocelot_netdev_to_port(struct net_device *dev);
>  
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 77957328722a..93aaa631e347 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -1035,8 +1035,8 @@ static int ocelot_netdevice_changeupper(struct net_device *dev,
>  			err = ocelot_port_lag_join(ocelot, port,
>  						   info->upper_dev);
>  		else
> -			ocelot_port_lag_leave(ocelot, port,
> -					      info->upper_dev);
> +			err = ocelot_port_lag_leave(ocelot, port,
> +						    info->upper_dev);
>  	}
>  
>  	return notifier_from_errno(err);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index b812bdff1da1..0cd45659430f 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -639,8 +639,6 @@ struct ocelot {
>  	enum ocelot_tag_prefix		inj_prefix;
>  	enum ocelot_tag_prefix		xtr_prefix;
>  
> -	u32				*lags;
> -
>  	struct list_head		multicast;
>  	struct list_head		pgids;
>  
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 13/16] net: mscc: ocelot: rename aggr_count to num_ports_in_lag

[Alexandre Belloni]

On 08/12/2020 14:07:59+0200, Vladimir Oltean wrote:
> It makes it a bit easier to read and understand the code that deals with
> balancing the 16 aggregation codes among the ports in a certain LAG.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/net/ethernet/mscc/ocelot.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index d4dbba66aa65..d87e80a15ca5 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -1263,8 +1263,8 @@ static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  
>  	/* Now, set PGIDs for each LAG */
>  	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
> +		int num_ports_in_lag = 0;
>  		unsigned long bond_mask;
> -		int aggr_count = 0;
>  		u8 aggr_idx[16];
>  
>  		if (!bonds[lag])
> @@ -1276,8 +1276,7 @@ static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  			// Destination mask
>  			ocelot_write_rix(ocelot, bond_mask,
>  					 ANA_PGID_PGID, port);
> -			aggr_idx[aggr_count] = port;
> -			aggr_count++;
> +			aggr_idx[num_ports_in_lag++] = port;
>  		}
>  
>  		for_each_aggr_pgid(ocelot, i) {
> @@ -1285,7 +1284,7 @@ static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  
>  			ac = ocelot_read_rix(ocelot, ANA_PGID_PGID, i);
>  			ac &= ~bond_mask;
> -			ac |= BIT(aggr_idx[i % aggr_count]);
> +			ac |= BIT(aggr_idx[i % num_ports_in_lag]);
>  			ocelot_write_rix(ocelot, ac, ANA_PGID_PGID, i);
>  		}
>  
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 14/16] net: mscc: ocelot: rebalance LAGs on link up/down events

[Alexandre Belloni]

On 08/12/2020 14:08:00+0200, Vladimir Oltean wrote:
> At present there is an issue when ocelot is offloading a bonding
> interface, but one of the links of the physical ports goes down. Traffic
> keeps being hashed towards that destination, and of course gets dropped
> on egress.
> 
> Monitor the netdev notifier events emitted by the bonding driver for
> changes in the physical state of lower interfaces, to determine which
> ports are active and which ones are no longer.
> 
> Then extend ocelot_get_bond_mask to return either the configured bonding
> interfaces, or the active ones, depending on a boolean argument. The
> code that does rebalancing only needs to do so among the active ports,
> whereas the bridge forwarding mask and the logical port IDs still need
> to look at the permanently bonded ports.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/mscc/ocelot.c     | 43 ++++++++++++++++++++------
>  drivers/net/ethernet/mscc/ocelot.h     |  2 ++
>  drivers/net/ethernet/mscc/ocelot_net.c | 26 ++++++++++++++++
>  include/soc/mscc/ocelot.h              |  1 +
>  4 files changed, 63 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index d87e80a15ca5..5c71d121048d 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -881,7 +881,8 @@ int ocelot_get_ts_info(struct ocelot *ocelot, int port,
>  }
>  EXPORT_SYMBOL(ocelot_get_ts_info);
>  
> -static u32 ocelot_get_bond_mask(struct ocelot *ocelot, struct net_device *bond)
> +static u32 ocelot_get_bond_mask(struct ocelot *ocelot, struct net_device *bond,
> +				bool just_active_ports)

only_active_ports maybe ?

>  {
>  	u32 bond_mask = 0;
>  	int port;
> @@ -892,8 +893,12 @@ static u32 ocelot_get_bond_mask(struct ocelot *ocelot, struct net_device *bond)
>  		if (!ocelot_port)
>  			continue;
>  
> -		if (ocelot_port->bond == bond)
> +		if (ocelot_port->bond == bond) {
> +			if (just_active_ports && !ocelot_port->lag_tx_active)
> +				continue;
> +
>  			bond_mask |= BIT(port);
> +		}
>  	}
>  
>  	return bond_mask;
> @@ -919,7 +924,7 @@ static void ocelot_apply_bridge_fwd_mask(struct ocelot *ocelot)
>  			struct net_device *bond = ocelot_port->bond;
>  
>  			if (bond)
> -				mask &= ~ocelot_get_bond_mask(ocelot, bond);
> +				mask &= ~ocelot_get_bond_mask(ocelot, bond, false);
>  
>  			ocelot_write_rix(ocelot, mask,
>  					 ANA_PGID_PGID, PGID_SRC + port);
> @@ -1261,22 +1266,22 @@ static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  		bonds[port] = ocelot_port->bond;
>  	}
>  
> -	/* Now, set PGIDs for each LAG */
> +	/* Now, set PGIDs for each active LAG */
>  	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
> -		int num_ports_in_lag = 0;
> +		int num_active_ports = 0;
>  		unsigned long bond_mask;
>  		u8 aggr_idx[16];
>  
>  		if (!bonds[lag])
>  			continue;
>  
> -		bond_mask = ocelot_get_bond_mask(ocelot, bonds[lag]);
> +		bond_mask = ocelot_get_bond_mask(ocelot, bonds[lag], true);
>  
>  		for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {
>  			// Destination mask
>  			ocelot_write_rix(ocelot, bond_mask,
>  					 ANA_PGID_PGID, port);
> -			aggr_idx[num_ports_in_lag++] = port;
> +			aggr_idx[num_active_ports++] = port;
>  		}
>  
>  		for_each_aggr_pgid(ocelot, i) {
> @@ -1284,7 +1289,11 @@ static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  
>  			ac = ocelot_read_rix(ocelot, ANA_PGID_PGID, i);
>  			ac &= ~bond_mask;
> -			ac |= BIT(aggr_idx[i % num_ports_in_lag]);
> +			/* Don't do division by zero if there was no active
> +			 * port. Just make all aggregation codes zero.
> +			 */
> +			if (num_active_ports)
> +				ac |= BIT(aggr_idx[i % num_active_ports]);
>  			ocelot_write_rix(ocelot, ac, ANA_PGID_PGID, i);
>  		}
>  
> @@ -1320,7 +1329,8 @@ static void ocelot_setup_logical_port_ids(struct ocelot *ocelot)
>  
>  		bond = ocelot_port->bond;
>  		if (bond) {
> -			int lag = __ffs(ocelot_get_bond_mask(ocelot, bond));
> +			int lag = __ffs(ocelot_get_bond_mask(ocelot, bond,
> +							     false));
>  
>  			ocelot_rmw_gix(ocelot,
>  				       ANA_PORT_PORT_CFG_PORTID_VAL(lag),
> @@ -1357,6 +1367,21 @@ int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
>  }
>  EXPORT_SYMBOL(ocelot_port_lag_leave);
>  
> +int ocelot_port_lag_change(struct ocelot *ocelot, int port,
> +			   struct netdev_lag_lower_state_info *info)
> +{
> +	struct ocelot_port *ocelot_port = ocelot->ports[port];
> +	bool is_active = info->link_up && info->tx_enabled;
> +
> +	if (ocelot_port->lag_tx_active == is_active)
> +		return 0;
> +
> +	ocelot_port->lag_tx_active = is_active;
> +
> +	/* Rebalance the LAGs */
> +	return ocelot_set_aggr_pgids(ocelot);
> +}
> +
>  /* Configure the maximum SDU (L2 payload) on RX to the value specified in @sdu.
>   * The length of VLAN tags is accounted for automatically via DEV_MAC_TAGS_CFG.
>   * In the special case that it's the NPI port that we're configuring, the
> diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
> index bef8d5f8e6e5..0860125b623c 100644
> --- a/drivers/net/ethernet/mscc/ocelot.h
> +++ b/drivers/net/ethernet/mscc/ocelot.h
> @@ -116,6 +116,8 @@ int ocelot_port_lag_join(struct ocelot *ocelot, int port,
>  			 struct net_device *bond);
>  int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
>  			  struct net_device *bond);
> +int ocelot_port_lag_change(struct ocelot *ocelot, int port,
> +			   struct netdev_lag_lower_state_info *info);
>  struct net_device *ocelot_port_to_netdev(struct ocelot *ocelot, int port);
>  int ocelot_netdev_to_port(struct net_device *dev);
>  
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 93aaa631e347..714fc99f8339 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -1065,6 +1065,23 @@ ocelot_netdevice_lag_changeupper(struct net_device *dev,
>  	return NOTIFY_DONE;
>  }
>  
> +static int
> +ocelot_netdevice_changelowerstate(struct net_device *dev,
> +				  struct netdev_lag_lower_state_info *linfo)
> +{
> +	struct ocelot_port_private *priv = netdev_priv(dev);
> +	struct ocelot_port *ocelot_port = &priv->port;
> +	struct ocelot *ocelot = ocelot_port->ocelot;
> +	int port = priv->chip_port;
> +	int err;
> +
> +	if (!ocelot_port->bond)
> +		return NOTIFY_DONE;
> +
> +	err = ocelot_port_lag_change(ocelot, port, linfo);
> +	return notifier_from_errno(err);
> +}
> +
>  static int ocelot_netdevice_event(struct notifier_block *unused,
>  				  unsigned long event, void *ptr)
>  {
> @@ -1082,6 +1099,15 @@ static int ocelot_netdevice_event(struct notifier_block *unused,
>  
>  		break;
>  	}
> +	case NETDEV_CHANGELOWERSTATE: {
> +		struct netdev_notifier_changelowerstate_info *info = ptr;
> +
> +		if (!ocelot_netdevice_dev_check(dev))
> +			break;
> +
> +		return ocelot_netdevice_changelowerstate(dev,
> +							 info->lower_state_info);
> +	}
>  	default:
>  		break;
>  	}
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 0cd45659430f..8a44b9064789 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -599,6 +599,7 @@ struct ocelot_port {
>  	u8				*xmit_template;
>  
>  	struct net_device		*bond;
> +	bool				lag_tx_active;
>  };
>  
>  struct ocelot {
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH net-next 12/16] net: mscc: ocelot: drop the use of the "lags" array

[Vladimir Oltean]

On Tue, Dec 08, 2020 at 02:07:58PM +0200, Vladimir Oltean wrote:
> We can now simplify the implementation by always using ocelot_get_bond_mask
> to look up the other ports that are offloading the same bonding interface
> as us.
> 
> In ocelot_set_aggr_pgids, the code had a way to uniquely iterate through
> LAGs. We need to achieve the same behavior by marking each LAG as visited,
> which we do now by temporarily allocating an array of pointers to bonding
> uppers of each port, and marking each bonding upper as NULL once it has
> been treated by the first port that is a member. And because we now do
> some dynamic allocation, we need to propagate errors from
> ocelot_set_aggr_pgid all the way to ocelot_port_lag_leave.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/mscc/ocelot.c     | 104 ++++++++++---------------
>  drivers/net/ethernet/mscc/ocelot.h     |   4 +-
>  drivers/net/ethernet/mscc/ocelot_net.c |   4 +-
>  include/soc/mscc/ocelot.h              |   2 -
>  4 files changed, 47 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 1a98c24af056..d4dbba66aa65 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -909,21 +909,17 @@ static void ocelot_apply_bridge_fwd_mask(struct ocelot *ocelot)
>  	 * source port's forwarding mask.
>  	 */
>  	for (port = 0; port < ocelot->num_phys_ports; port++) {
> -		if (ocelot->bridge_fwd_mask & BIT(port)) {
> -			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(port);
> -			int lag;
> +		struct ocelot_port *ocelot_port = ocelot->ports[port];
>  
> -			for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
> -				unsigned long bond_mask = ocelot->lags[lag];
> +		if (!ocelot_port)
> +			continue;
>  
> -				if (!bond_mask)
> -					continue;
> +		if (ocelot->bridge_fwd_mask & BIT(port)) {
> +			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(port);
> +			struct net_device *bond = ocelot_port->bond;
>  
> -				if (bond_mask & BIT(port)) {
> -					mask &= ~bond_mask;
> -					break;
> -				}
> -			}
> +			if (bond)
> +				mask &= ~ocelot_get_bond_mask(ocelot, bond);
>  
>  			ocelot_write_rix(ocelot, mask,
>  					 ANA_PGID_PGID, PGID_SRC + port);
> @@ -1238,10 +1234,16 @@ int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
>  }
>  EXPORT_SYMBOL(ocelot_port_bridge_leave);
>  
> -static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
> +static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  {
> +	struct net_device **bonds;
>  	int i, port, lag;
>  
> +	bonds = kcalloc(ocelot->num_phys_ports, sizeof(struct net_device *),
> +			GFP_KERNEL);
> +	if (!bonds)
> +		return -ENOMEM;
> +

I remember somebody complaining about the temporary memory allocation
done here, but I can't seem to find that email for some reason.

Is it ok if I still keep the dynamic allocation there, though? Felix has
up to 5 user ports, Seville has up to 9, Ocelot up to 11. I would like
to not hardcode anything, in case (who knows!) more switches get added.

>  	/* Reset destination and aggregation PGIDS */
>  	for_each_unicast_dest_pgid(ocelot, port)
>  		ocelot_write_rix(ocelot, BIT(port), ANA_PGID_PGID, port);
> @@ -1250,16 +1252,26 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  		ocelot_write_rix(ocelot, GENMASK(ocelot->num_phys_ports - 1, 0),
>  				 ANA_PGID_PGID, i);
>  
> +	for (port = 0; port < ocelot->num_phys_ports; port++) {
> +		struct ocelot_port *ocelot_port = ocelot->ports[port];
> +
> +		if (!ocelot_port)
> +			continue;
> +
> +		bonds[port] = ocelot_port->bond;
> +	}
> +
>  	/* Now, set PGIDs for each LAG */
>  	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
>  		unsigned long bond_mask;
>  		int aggr_count = 0;
>  		u8 aggr_idx[16];
>  
> -		bond_mask = ocelot->lags[lag];
> -		if (!bond_mask)
> +		if (!bonds[lag])
>  			continue;
>  
> +		bond_mask = ocelot_get_bond_mask(ocelot, bonds[lag]);
> +
>  		for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {
>  			// Destination mask
>  			ocelot_write_rix(ocelot, bond_mask,
> @@ -1276,7 +1288,19 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  			ac |= BIT(aggr_idx[i % aggr_count]);
>  			ocelot_write_rix(ocelot, ac, ANA_PGID_PGID, i);
>  		}
> +
> +		/* Mark the bonding interface as visited to avoid applying
> +		 * the same config again
> +		 */
> +		for (i = lag + 1; i < ocelot->num_phys_ports; i++)
> +			if (bonds[i] == bonds[lag])
> +				bonds[i] = NULL;
> +
> +		bonds[lag] = NULL;
>  	}
> +
> +	kfree(bonds);
> +	return 0;
>  }
>  
>  /* When offloading a bonding interface, the switch ports configured under the
> @@ -1315,59 +1339,22 @@ static void ocelot_setup_logical_port_ids(struct ocelot *ocelot)
>  int ocelot_port_lag_join(struct ocelot *ocelot, int port,
>  			 struct net_device *bond)
>  {
> -	u32 bond_mask = 0;
> -	int lag;
> -
>  	ocelot->ports[port]->bond = bond;
>  
> -	bond_mask = ocelot_get_bond_mask(ocelot, bond);
> -
> -	lag = __ffs(bond_mask);
> -
> -	/* If the new port is the lowest one, use it as the logical port from
> -	 * now on
> -	 */
> -	if (port == lag) {
> -		ocelot->lags[port] = bond_mask;
> -		bond_mask &= ~BIT(port);
> -		if (bond_mask)
> -			ocelot->lags[__ffs(bond_mask)] = 0;
> -	} else {
> -		ocelot->lags[lag] |= BIT(port);
> -	}
> -
>  	ocelot_setup_logical_port_ids(ocelot);
>  	ocelot_apply_bridge_fwd_mask(ocelot);
> -	ocelot_set_aggr_pgids(ocelot);
> -
> -	return 0;
> +	return ocelot_set_aggr_pgids(ocelot);
>  }
>  EXPORT_SYMBOL(ocelot_port_lag_join);
>  
> -void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
> -			   struct net_device *bond)
> +int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
> +			  struct net_device *bond)
>  {
> -	int i;
> -
>  	ocelot->ports[port]->bond = NULL;
>  
> -	/* Remove port from any lag */
> -	for (i = 0; i < ocelot->num_phys_ports; i++)
> -		ocelot->lags[i] &= ~BIT(port);
> -
> -	/* if it was the logical port of the lag, move the lag config to the
> -	 * next port
> -	 */
> -	if (ocelot->lags[port]) {
> -		int n = __ffs(ocelot->lags[port]);
> -
> -		ocelot->lags[n] = ocelot->lags[port];
> -		ocelot->lags[port] = 0;
> -	}
> -
>  	ocelot_setup_logical_port_ids(ocelot);
>  	ocelot_apply_bridge_fwd_mask(ocelot);
> -	ocelot_set_aggr_pgids(ocelot);
> +	return ocelot_set_aggr_pgids(ocelot);
>  }
>  EXPORT_SYMBOL(ocelot_port_lag_leave);
>  
> @@ -1543,11 +1530,6 @@ int ocelot_init(struct ocelot *ocelot)
>  		}
>  	}
>  
> -	ocelot->lags = devm_kcalloc(ocelot->dev, ocelot->num_phys_ports,
> -				    sizeof(u32), GFP_KERNEL);
> -	if (!ocelot->lags)
> -		return -ENOMEM;
> -
>  	ocelot->stats = devm_kcalloc(ocelot->dev,
>  				     ocelot->num_phys_ports * ocelot->num_stats,
>  				     sizeof(u64), GFP_KERNEL);
> diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
> index 739bd201d951..bef8d5f8e6e5 100644
> --- a/drivers/net/ethernet/mscc/ocelot.h
> +++ b/drivers/net/ethernet/mscc/ocelot.h
> @@ -114,8 +114,8 @@ int ocelot_mact_forget(struct ocelot *ocelot,
>  		       const unsigned char mac[ETH_ALEN], unsigned int vid);
>  int ocelot_port_lag_join(struct ocelot *ocelot, int port,
>  			 struct net_device *bond);
> -void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
> -			   struct net_device *bond);
> +int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
> +			  struct net_device *bond);
>  struct net_device *ocelot_port_to_netdev(struct ocelot *ocelot, int port);
>  int ocelot_netdev_to_port(struct net_device *dev);
>  
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 77957328722a..93aaa631e347 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -1035,8 +1035,8 @@ static int ocelot_netdevice_changeupper(struct net_device *dev,
>  			err = ocelot_port_lag_join(ocelot, port,
>  						   info->upper_dev);
>  		else
> -			ocelot_port_lag_leave(ocelot, port,
> -					      info->upper_dev);
> +			err = ocelot_port_lag_leave(ocelot, port,
> +						    info->upper_dev);
>  	}
>  
>  	return notifier_from_errno(err);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index b812bdff1da1..0cd45659430f 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -639,8 +639,6 @@ struct ocelot {
>  	enum ocelot_tag_prefix		inj_prefix;
>  	enum ocelot_tag_prefix		xtr_prefix;
>  
> -	u32				*lags;
> -
>  	struct list_head		multicast;
>  	struct list_head		pgids;
>  
> -- 
> 2.25.1
> 

Re: [RFC PATCH net-next 12/16] net: mscc: ocelot: drop the use of the "lags" array

[Alexandre Belloni]

On 15/01/2021 13:05:47+0200, Vladimir Oltean wrote:
> > -static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
> > +static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
> >  {
> > +	struct net_device **bonds;
> >  	int i, port, lag;
> >  
> > +	bonds = kcalloc(ocelot->num_phys_ports, sizeof(struct net_device *),
> > +			GFP_KERNEL);
> > +	if (!bonds)
> > +		return -ENOMEM;
> > +
> 
> I remember somebody complaining about the temporary memory allocation
> done here, but I can't seem to find that email for some reason.
> 
> Is it ok if I still keep the dynamic allocation there, though? Felix has
> up to 5 user ports, Seville has up to 9, Ocelot up to 11. I would like
> to not hardcode anything, in case (who knows!) more switches get added.
> 

I was probably the one, the main reason being that this make this
function able to fail. Removing the dynamic allocation would ensure it
never fails. However, I didn't suggest any other solution so I'm fine if
you keep it.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com