[PATCH net-next v3 0/7] net: lan966x: Add lag support

[PATCH net-next v3 0/7] net: lan966x: Add lag support

[Horatiu Vultur]

Add lag support for lan966x.
First 4 patches don't do any changes to the current behaviour, they
just prepare for lag support. While the rest is to add the lag support.

v2->v3:
- return error code from 'switchdev_bridge_port_offload()'
- fix lan966x_foreign_dev_check(), it was missing lag support
- remove lan966x_lag_mac_add_entry and lan966x_mac_del_entry as
  they are not needed
- fix race conditions when accessing port->bond
- move FDB entries when a new port joins the lag if it has a lower

v1->v2:
- fix the LAG PGIDs when ports go down, in this way is not
  needed anymore the last patch of the series.

Horatiu Vultur (7):
  net: lan966x: Add reqisters used to configure lag interfaces
  net: lan966x: Split lan966x_fdb_event_work
  net: lan966x: Expose lan966x_switchdev_nb and
    lan966x_switchdev_blocking_nb
  net: lan966x: Extend lan966x_foreign_bridging_check
  net: lan966x: Add lag support for lan966x.
  net: lan966x: Extend FDB to support also lag
  net: lan966x: Extend MAC to support also lag interfaces.

 .../net/ethernet/microchip/lan966x/Makefile   |   2 +-
 .../ethernet/microchip/lan966x/lan966x_fdb.c  | 155 +++++---
 .../ethernet/microchip/lan966x/lan966x_lag.c  | 335 ++++++++++++++++++
 .../ethernet/microchip/lan966x/lan966x_mac.c  |  66 +++-
 .../ethernet/microchip/lan966x/lan966x_main.h |  38 ++
 .../ethernet/microchip/lan966x/lan966x_regs.h |  45 +++
 .../microchip/lan966x/lan966x_switchdev.c     | 137 +++++--
 7 files changed, 680 insertions(+), 98 deletions(-)
 create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_lag.c

-- 
2.33.0

[PATCH net-next v3 1/7] net: lan966x: Add reqisters used to configure lag interfaces

[Horatiu Vultur]

Add the registers used by lan966x to configure the lag interface.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ethernet/microchip/lan966x/lan966x_regs.h | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
index 8265ad89f0bc..69eedff28ce3 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
@@ -363,6 +363,51 @@ enum lan966x_target {
 #define ANA_PFC_CFG_FC_LINK_SPEED_GET(x)\
 	FIELD_GET(ANA_PFC_CFG_FC_LINK_SPEED, x)
 
+/*      ANA:COMMON:AGGR_CFG */
+#define ANA_AGGR_CFG              __REG(TARGET_ANA, 0, 1, 31232, 0, 1, 552, 0, 0, 1, 4)
+
+#define ANA_AGGR_CFG_AC_RND_ENA                  BIT(6)
+#define ANA_AGGR_CFG_AC_RND_ENA_SET(x)\
+	FIELD_PREP(ANA_AGGR_CFG_AC_RND_ENA, x)
+#define ANA_AGGR_CFG_AC_RND_ENA_GET(x)\
+	FIELD_GET(ANA_AGGR_CFG_AC_RND_ENA, x)
+
+#define ANA_AGGR_CFG_AC_DMAC_ENA                 BIT(5)
+#define ANA_AGGR_CFG_AC_DMAC_ENA_SET(x)\
+	FIELD_PREP(ANA_AGGR_CFG_AC_DMAC_ENA, x)
+#define ANA_AGGR_CFG_AC_DMAC_ENA_GET(x)\
+	FIELD_GET(ANA_AGGR_CFG_AC_DMAC_ENA, x)
+
+#define ANA_AGGR_CFG_AC_SMAC_ENA                 BIT(4)
+#define ANA_AGGR_CFG_AC_SMAC_ENA_SET(x)\
+	FIELD_PREP(ANA_AGGR_CFG_AC_SMAC_ENA, x)
+#define ANA_AGGR_CFG_AC_SMAC_ENA_GET(x)\
+	FIELD_GET(ANA_AGGR_CFG_AC_SMAC_ENA, x)
+
+#define ANA_AGGR_CFG_AC_IP6_FLOW_LBL_ENA         BIT(3)
+#define ANA_AGGR_CFG_AC_IP6_FLOW_LBL_ENA_SET(x)\
+	FIELD_PREP(ANA_AGGR_CFG_AC_IP6_FLOW_LBL_ENA, x)
+#define ANA_AGGR_CFG_AC_IP6_FLOW_LBL_ENA_GET(x)\
+	FIELD_GET(ANA_AGGR_CFG_AC_IP6_FLOW_LBL_ENA, x)
+
+#define ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA           BIT(2)
+#define ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA_SET(x)\
+	FIELD_PREP(ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA, x)
+#define ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA_GET(x)\
+	FIELD_GET(ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA, x)
+
+#define ANA_AGGR_CFG_AC_IP4_SIPDIP_ENA           BIT(1)
+#define ANA_AGGR_CFG_AC_IP4_SIPDIP_ENA_SET(x)\
+	FIELD_PREP(ANA_AGGR_CFG_AC_IP4_SIPDIP_ENA, x)
+#define ANA_AGGR_CFG_AC_IP4_SIPDIP_ENA_GET(x)\
+	FIELD_GET(ANA_AGGR_CFG_AC_IP4_SIPDIP_ENA, x)
+
+#define ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA           BIT(0)
+#define ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA_SET(x)\
+	FIELD_PREP(ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA, x)
+#define ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA_GET(x)\
+	FIELD_GET(ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA, x)
+
 /*      CHIP_TOP:CUPHY_CFG:CUPHY_PORT_CFG */
 #define CHIP_TOP_CUPHY_PORT_CFG(r) __REG(TARGET_CHIP_TOP, 0, 1, 16, 0, 1, 20, 8, r, 2, 4)
 
-- 
2.33.0

[PATCH net-next v3 2/7] net: lan966x: Split lan966x_fdb_event_work

[Horatiu Vultur]

Split the function lan966x_fdb_event_work. One case for when the
orig_dev is a bridge and one case when orig_dev is lan966x port.
This is preparation for lag support. There is no functional change.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ethernet/microchip/lan966x/lan966x_fdb.c  | 127 ++++++++++--------
 .../ethernet/microchip/lan966x/lan966x_main.h |   1 +
 .../microchip/lan966x/lan966x_switchdev.c     |   7 +-
 3 files changed, 76 insertions(+), 59 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c
index da5ca7188679..5142e7c0de31 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c
@@ -8,6 +8,7 @@ struct lan966x_fdb_event_work {
 	struct work_struct work;
 	struct switchdev_notifier_fdb_info fdb_info;
 	struct net_device *dev;
+	struct net_device *orig_dev;
 	struct lan966x *lan966x;
 	unsigned long event;
 };
@@ -127,75 +128,89 @@ void lan966x_fdb_deinit(struct lan966x *lan966x)
 	lan966x_fdb_purge_entries(lan966x);
 }
 
-static void lan966x_fdb_event_work(struct work_struct *work)
+void lan966x_fdb_flush_workqueue(struct lan966x *lan966x)
+{
+	flush_workqueue(lan966x->fdb_work);
+}
+
+static void lan966x_fdb_port_event_work(struct lan966x_fdb_event_work *fdb_work)
 {
-	struct lan966x_fdb_event_work *fdb_work =
-		container_of(work, struct lan966x_fdb_event_work, work);
 	struct switchdev_notifier_fdb_info *fdb_info;
-	struct net_device *dev = fdb_work->dev;
 	struct lan966x_port *port;
 	struct lan966x *lan966x;
-	int ret;
 
-	fdb_info = &fdb_work->fdb_info;
 	lan966x = fdb_work->lan966x;
+	port = netdev_priv(fdb_work->orig_dev);
+	fdb_info = &fdb_work->fdb_info;
 
-	if (lan966x_netdevice_check(dev)) {
-		port = netdev_priv(dev);
-
-		switch (fdb_work->event) {
-		case SWITCHDEV_FDB_ADD_TO_DEVICE:
-			if (!fdb_info->added_by_user)
-				break;
-			lan966x_mac_add_entry(lan966x, port, fdb_info->addr,
-					      fdb_info->vid);
+	switch (fdb_work->event) {
+	case SWITCHDEV_FDB_ADD_TO_DEVICE:
+		if (!fdb_info->added_by_user)
 			break;
-		case SWITCHDEV_FDB_DEL_TO_DEVICE:
-			if (!fdb_info->added_by_user)
-				break;
-			lan966x_mac_del_entry(lan966x, fdb_info->addr,
-					      fdb_info->vid);
+		lan966x_mac_add_entry(lan966x, port, fdb_info->addr,
+				      fdb_info->vid);
+		break;
+	case SWITCHDEV_FDB_DEL_TO_DEVICE:
+		if (!fdb_info->added_by_user)
 			break;
-		}
-	} else {
-		if (!netif_is_bridge_master(dev))
-			goto out;
-
-		/* In case the bridge is called */
-		switch (fdb_work->event) {
-		case SWITCHDEV_FDB_ADD_TO_DEVICE:
-			/* If there is no front port in this vlan, there is no
-			 * point to copy the frame to CPU because it would be
-			 * just dropped at later point. So add it only if
-			 * there is a port but it is required to store the fdb
-			 * entry for later point when a port actually gets in
-			 * the vlan.
-			 */
-			lan966x_fdb_add_entry(lan966x, fdb_info);
-			if (!lan966x_vlan_cpu_member_cpu_vlan_mask(lan966x,
-								   fdb_info->vid))
-				break;
-
-			lan966x_mac_cpu_learn(lan966x, fdb_info->addr,
-					      fdb_info->vid);
+		lan966x_mac_del_entry(lan966x, fdb_info->addr,
+				      fdb_info->vid);
+		break;
+	}
+}
+
+static void lan966x_fdb_bridge_event_work(struct lan966x_fdb_event_work *fdb_work)
+{
+	struct switchdev_notifier_fdb_info *fdb_info;
+	struct lan966x *lan966x;
+	int ret;
+
+	lan966x = fdb_work->lan966x;
+	fdb_info = &fdb_work->fdb_info;
+
+	/* In case the bridge is called */
+	switch (fdb_work->event) {
+	case SWITCHDEV_FDB_ADD_TO_DEVICE:
+		/* If there is no front port in this vlan, there is no
+		 * point to copy the frame to CPU because it would be
+		 * just dropped at later point. So add it only if
+		 * there is a port but it is required to store the fdb
+		 * entry for later point when a port actually gets in
+		 * the vlan.
+		 */
+		lan966x_fdb_add_entry(lan966x, fdb_info);
+		if (!lan966x_vlan_cpu_member_cpu_vlan_mask(lan966x,
+							   fdb_info->vid))
 			break;
-		case SWITCHDEV_FDB_DEL_TO_DEVICE:
-			ret = lan966x_fdb_del_entry(lan966x, fdb_info);
-			if (!lan966x_vlan_cpu_member_cpu_vlan_mask(lan966x,
-								   fdb_info->vid))
-				break;
-
-			if (ret)
-				lan966x_mac_cpu_forget(lan966x, fdb_info->addr,
-						       fdb_info->vid);
+
+		lan966x_mac_cpu_learn(lan966x, fdb_info->addr,
+				      fdb_info->vid);
+		break;
+	case SWITCHDEV_FDB_DEL_TO_DEVICE:
+		ret = lan966x_fdb_del_entry(lan966x, fdb_info);
+		if (!lan966x_vlan_cpu_member_cpu_vlan_mask(lan966x,
+							   fdb_info->vid))
 			break;
-		}
+
+		if (ret)
+			lan966x_mac_cpu_forget(lan966x, fdb_info->addr,
+					       fdb_info->vid);
+		break;
 	}
+}
+
+static void lan966x_fdb_event_work(struct work_struct *work)
+{
+	struct lan966x_fdb_event_work *fdb_work =
+		container_of(work, struct lan966x_fdb_event_work, work);
+
+	if (lan966x_netdevice_check(fdb_work->orig_dev))
+		lan966x_fdb_port_event_work(fdb_work);
+	else if (netif_is_bridge_master(fdb_work->orig_dev))
+		lan966x_fdb_bridge_event_work(fdb_work);
 
-out:
 	kfree(fdb_work->fdb_info.addr);
 	kfree(fdb_work);
-	dev_put(dev);
 }
 
 int lan966x_handle_fdb(struct net_device *dev,
@@ -221,7 +236,8 @@ int lan966x_handle_fdb(struct net_device *dev,
 		if (!fdb_work)
 			return -ENOMEM;
 
-		fdb_work->dev = orig_dev;
+		fdb_work->dev = dev;
+		fdb_work->orig_dev = orig_dev;
 		fdb_work->lan966x = lan966x;
 		fdb_work->event = event;
 		INIT_WORK(&fdb_work->work, lan966x_fdb_event_work);
@@ -231,7 +247,6 @@ int lan966x_handle_fdb(struct net_device *dev,
 			goto err_addr_alloc;
 
 		ether_addr_copy((u8 *)fdb_work->fdb_info.addr, fdb_info->addr);
-		dev_hold(orig_dev);
 
 		queue_work(lan966x->fdb_work, &fdb_work->work);
 		break;
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 3b86ddddc756..0a4f4d27eaa7 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -368,6 +368,7 @@ void lan966x_fdb_write_entries(struct lan966x *lan966x, u16 vid);
 void lan966x_fdb_erase_entries(struct lan966x *lan966x, u16 vid);
 int lan966x_fdb_init(struct lan966x *lan966x);
 void lan966x_fdb_deinit(struct lan966x *lan966x);
+void lan966x_fdb_flush_workqueue(struct lan966x *lan966x);
 int lan966x_handle_fdb(struct net_device *dev,
 		       struct net_device *orig_dev,
 		       unsigned long event, const void *ctx,
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
index df2bee678559..d9fc6a9a3da1 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
@@ -320,9 +320,10 @@ static int lan966x_port_prechangeupper(struct net_device *dev,
 {
 	struct lan966x_port *port = netdev_priv(dev);
 
-	if (netif_is_bridge_master(info->upper_dev) && !info->linking)
-		switchdev_bridge_port_unoffload(port->dev, port,
-						NULL, NULL);
+	if (netif_is_bridge_master(info->upper_dev) && !info->linking) {
+		switchdev_bridge_port_unoffload(port->dev, port, NULL, NULL);
+		lan966x_fdb_flush_workqueue(port->lan966x);
+	}
 
 	return NOTIFY_DONE;
 }
-- 
2.33.0

[PATCH net-next v3 3/7] net: lan966x: Expose lan966x_switchdev_nb and lan966x_switchdev_blocking_nb

[Horatiu Vultur]

Expose lan966x_switchdev_nb and lan966x_switchdev_blocking_nb to the
lan966x_main.h file because they will be needed by the lag driver.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_main.h      | 2 ++
 drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c | 6 ++----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 0a4f4d27eaa7..f820b1c71c47 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -296,6 +296,8 @@ struct lan966x_port {
 extern const struct phylink_mac_ops lan966x_phylink_mac_ops;
 extern const struct phylink_pcs_ops lan966x_phylink_pcs_ops;
 extern const struct ethtool_ops lan966x_ethtool_ops;
+extern struct notifier_block lan966x_switchdev_nb __read_mostly;
+extern struct notifier_block lan966x_switchdev_blocking_nb __read_mostly;
 
 bool lan966x_netdevice_check(const struct net_device *dev);
 
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
index d9fc6a9a3da1..d9b3ca5f6214 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
@@ -6,8 +6,6 @@
 #include "lan966x_main.h"
 
 static struct notifier_block lan966x_netdevice_nb __read_mostly;
-static struct notifier_block lan966x_switchdev_nb __read_mostly;
-static struct notifier_block lan966x_switchdev_blocking_nb __read_mostly;
 
 static void lan966x_port_set_mcast_ip_flood(struct lan966x_port *port,
 					    u32 pgid_ip)
@@ -572,11 +570,11 @@ static struct notifier_block lan966x_netdevice_nb __read_mostly = {
 	.notifier_call = lan966x_netdevice_event,
 };
 
-static struct notifier_block lan966x_switchdev_nb __read_mostly = {
+struct notifier_block lan966x_switchdev_nb __read_mostly = {
 	.notifier_call = lan966x_switchdev_event,
 };
 
-static struct notifier_block lan966x_switchdev_blocking_nb __read_mostly = {
+struct notifier_block lan966x_switchdev_blocking_nb __read_mostly = {
 	.notifier_call = lan966x_switchdev_blocking_event,
 };
 
-- 
2.33.0

[PATCH net-next v3 4/7] net: lan966x: Extend lan966x_foreign_bridging_check

[Horatiu Vultur]

Extend lan966x_foreign_bridging_check to check also if the upper
interface is a lag device. Don't allow a lan966x port to be part of a
lag if it has foreign interfaces.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../microchip/lan966x/lan966x_switchdev.c     | 32 ++++++++++++++-----
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
index d9b3ca5f6214..fe872edfcdca 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
@@ -326,23 +326,25 @@ static int lan966x_port_prechangeupper(struct net_device *dev,
 	return NOTIFY_DONE;
 }
 
-static int lan966x_foreign_bridging_check(struct net_device *bridge,
+static int lan966x_foreign_bridging_check(struct net_device *upper,
+					  bool *has_foreign,
+					  bool *seen_lan966x,
 					  struct netlink_ext_ack *extack)
 {
 	struct lan966x *lan966x = NULL;
-	bool has_foreign = false;
 	struct net_device *dev;
 	struct list_head *iter;
 
-	if (!netif_is_bridge_master(bridge))
+	if (!netif_is_bridge_master(upper) &&
+	    !netif_is_lag_master(upper))
 		return 0;
 
-	netdev_for_each_lower_dev(bridge, dev, iter) {
+	netdev_for_each_lower_dev(upper, dev, iter) {
 		if (lan966x_netdevice_check(dev)) {
 			struct lan966x_port *port = netdev_priv(dev);
 
 			if (lan966x) {
-				/* Bridge already has at least one port of a
+				/* Upper already has at least one port of a
 				 * lan966x switch inside it, check that it's
 				 * the same instance of the driver.
 				 */
@@ -353,15 +355,24 @@ static int lan966x_foreign_bridging_check(struct net_device *bridge,
 				}
 			} else {
 				/* This is the first lan966x port inside this
-				 * bridge
+				 * upper device
 				 */
 				lan966x = port->lan966x;
+				*seen_lan966x = true;
 			}
+		} else if (netif_is_lag_master(dev)) {
+			/* Allow to have bond interfaces that have only lan966x
+			 * devices
+			 */
+			if (lan966x_foreign_bridging_check(dev, has_foreign,
+							   seen_lan966x,
+							   extack))
+				*has_foreign = true;
 		} else {
-			has_foreign = true;
+			*has_foreign = true;
 		}
 
-		if (lan966x && has_foreign) {
+		if (*seen_lan966x && *has_foreign) {
 			NL_SET_ERR_MSG_MOD(extack,
 					   "Bridging lan966x ports with foreign interfaces disallowed");
 			return -EINVAL;
@@ -374,7 +385,12 @@ static int lan966x_foreign_bridging_check(struct net_device *bridge,
 static int lan966x_bridge_check(struct net_device *dev,
 				struct netdev_notifier_changeupper_info *info)
 {
+	bool has_foreign = false;
+	bool seen_lan966x = false;
+
 	return lan966x_foreign_bridging_check(info->upper_dev,
+					      &has_foreign,
+					      &seen_lan966x,
 					      info->info.extack);
 }
 
-- 
2.33.0

[PATCH net-next v3 5/7] net: lan966x: Add lag support for lan966x.

[Horatiu Vultur]

Add link aggregation hardware offload support for lan966x

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../net/ethernet/microchip/lan966x/Makefile   |   2 +-
 .../ethernet/microchip/lan966x/lan966x_lag.c  | 309 ++++++++++++++++++
 .../ethernet/microchip/lan966x/lan966x_main.h |  30 ++
 .../microchip/lan966x/lan966x_switchdev.c     |  90 +++--
 4 files changed, 411 insertions(+), 20 deletions(-)
 create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_lag.c

diff --git a/drivers/net/ethernet/microchip/lan966x/Makefile b/drivers/net/ethernet/microchip/lan966x/Makefile
index fd2e0ebb2427..0c22c86bdaa9 100644
--- a/drivers/net/ethernet/microchip/lan966x/Makefile
+++ b/drivers/net/ethernet/microchip/lan966x/Makefile
@@ -8,4 +8,4 @@ obj-$(CONFIG_LAN966X_SWITCH) += lan966x-switch.o
 lan966x-switch-objs  := lan966x_main.o lan966x_phylink.o lan966x_port.o \
 			lan966x_mac.o lan966x_ethtool.o lan966x_switchdev.o \
 			lan966x_vlan.o lan966x_fdb.o lan966x_mdb.o \
-			lan966x_ptp.o lan966x_fdma.o
+			lan966x_ptp.o lan966x_fdma.o lan966x_lag.o
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_lag.c b/drivers/net/ethernet/microchip/lan966x/lan966x_lag.c
new file mode 100644
index 000000000000..8fd62611cfa4
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_lag.c
@@ -0,0 +1,309 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/if_bridge.h>
+
+#include "lan966x_main.h"
+
+static void lan966x_lag_set_aggr_pgids(struct lan966x *lan966x)
+{
+	u32 visited = GENMASK(lan966x->num_phys_ports - 1, 0);
+	int p, lag, i;
+
+	/* Reset destination and aggregation PGIDS */
+	for (p = 0; p < lan966x->num_phys_ports; ++p)
+		lan_wr(ANA_PGID_PGID_SET(BIT(p)),
+		       lan966x, ANA_PGID(p));
+
+	for (p = PGID_AGGR; p < PGID_SRC; ++p)
+		lan_wr(ANA_PGID_PGID_SET(visited),
+		       lan966x, ANA_PGID(p));
+
+	/* The visited ports bitmask holds the list of ports offloading any
+	 * bonding interface. Initially we mark all these ports as unvisited,
+	 * then every time we visit a port in this bitmask, we know that it is
+	 * the lowest numbered port, i.e. the one whose logical ID == physical
+	 * port ID == LAG ID. So we mark as visited all further ports in the
+	 * bitmask that are offloading the same bonding interface. This way,
+	 * we set up the aggregation PGIDs only once per bonding interface.
+	 */
+	for (p = 0; p < lan966x->num_phys_ports; ++p) {
+		struct lan966x_port *port = lan966x->ports[p];
+
+		if (!port || !port->bond)
+			continue;
+
+		visited &= ~BIT(p);
+	}
+
+	/* Now, set PGIDs for each active LAG */
+	for (lag = 0; lag < lan966x->num_phys_ports; ++lag) {
+		struct net_device *bond = lan966x->ports[lag]->bond;
+		int num_active_ports = 0;
+		unsigned long bond_mask;
+		u8 aggr_idx[16];
+
+		if (!bond || (visited & BIT(lag)))
+			continue;
+
+		bond_mask = lan966x_lag_get_mask(lan966x, bond);
+
+		for_each_set_bit(p, &bond_mask, lan966x->num_phys_ports) {
+			struct lan966x_port *port = lan966x->ports[p];
+
+			lan_wr(ANA_PGID_PGID_SET(bond_mask),
+			       lan966x, ANA_PGID(p));
+			if (port->lag_tx_active)
+				aggr_idx[num_active_ports++] = p;
+		}
+
+		for (i = PGID_AGGR; i < PGID_SRC; ++i) {
+			u32 ac;
+
+			ac = lan_rd(lan966x, ANA_PGID(i));
+			ac &= ~bond_mask;
+			/* 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]);
+			lan_wr(ANA_PGID_PGID_SET(ac),
+			       lan966x, ANA_PGID(i));
+		}
+
+		/* Mark all ports in the same LAG as visited to avoid applying
+		 * the same config again.
+		 */
+		for (p = lag; p < lan966x->num_phys_ports; p++) {
+			struct lan966x_port *port = lan966x->ports[p];
+
+			if (!port)
+				continue;
+
+			if (port->bond == bond)
+				visited |= BIT(p);
+		}
+	}
+}
+
+static void lan966x_lag_set_port_ids(struct lan966x *lan966x)
+{
+	struct lan966x_port *port;
+	u32 bond_mask;
+	u32 lag_id;
+	int p;
+
+	for (p = 0; p < lan966x->num_phys_ports; ++p) {
+		port = lan966x->ports[p];
+		if (!port)
+			continue;
+
+		lag_id = port->chip_port;
+
+		bond_mask = lan966x_lag_get_mask(lan966x, port->bond);
+		if (bond_mask)
+			lag_id = __ffs(bond_mask);
+
+		lan_rmw(ANA_PORT_CFG_PORTID_VAL_SET(lag_id),
+			ANA_PORT_CFG_PORTID_VAL,
+			lan966x, ANA_PORT_CFG(port->chip_port));
+	}
+}
+
+static void lan966x_lag_update_ids(struct lan966x *lan966x)
+{
+	lan966x_lag_set_port_ids(lan966x);
+	lan966x_update_fwd_mask(lan966x);
+	lan966x_lag_set_aggr_pgids(lan966x);
+}
+
+int lan966x_lag_port_join(struct lan966x_port *port,
+			  struct net_device *brport_dev,
+			  struct net_device *bond,
+			  struct netlink_ext_ack *extack)
+{
+	struct lan966x *lan966x = port->lan966x;
+	struct net_device *dev = port->dev;
+	int err;
+
+	port->bond = bond;
+	lan966x_lag_update_ids(lan966x);
+
+	err = switchdev_bridge_port_offload(brport_dev, dev, port,
+					    &lan966x_switchdev_nb,
+					    &lan966x_switchdev_blocking_nb,
+					    false, extack);
+	if (err)
+		goto out;
+
+	lan966x_port_stp_state_set(port, br_port_get_stp_state(brport_dev));
+
+	return 0;
+
+out:
+	port->bond = NULL;
+	lan966x_lag_update_ids(lan966x);
+
+	return err;
+}
+
+void lan966x_lag_port_leave(struct lan966x_port *port, struct net_device *bond)
+{
+	struct lan966x *lan966x = port->lan966x;
+
+	port->bond = NULL;
+	lan966x_lag_update_ids(lan966x);
+	lan966x_port_stp_state_set(port, BR_STATE_FORWARDING);
+}
+
+int lan966x_lag_port_prechangeupper(struct net_device *dev,
+				    struct netdev_notifier_changeupper_info *info)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+	struct lan966x *lan966x = port->lan966x;
+	struct netdev_lag_upper_info *lui;
+	struct netlink_ext_ack *extack;
+
+	extack = netdev_notifier_info_to_extack(&info->info);
+	lui = info->upper_info;
+	if (!lui)
+		return NOTIFY_DONE;
+
+	if (lui->tx_type != NETDEV_LAG_TX_TYPE_HASH) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "LAG device using unsupported Tx type");
+		return notifier_from_errno(-EINVAL);
+	}
+
+	switch (lui->hash_type) {
+	case NETDEV_LAG_HASH_L2:
+		lan_wr(ANA_AGGR_CFG_AC_DMAC_ENA_SET(1) |
+		       ANA_AGGR_CFG_AC_SMAC_ENA_SET(1),
+		       lan966x, ANA_AGGR_CFG);
+		break;
+	case NETDEV_LAG_HASH_L34:
+		lan_wr(ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA_SET(1) |
+		       ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA_SET(1) |
+		       ANA_AGGR_CFG_AC_IP4_SIPDIP_ENA_SET(1),
+		       lan966x, ANA_AGGR_CFG);
+		break;
+	case NETDEV_LAG_HASH_L23:
+		lan_wr(ANA_AGGR_CFG_AC_DMAC_ENA_SET(1) |
+		       ANA_AGGR_CFG_AC_SMAC_ENA_SET(1) |
+		       ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA_SET(1) |
+		       ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA_SET(1),
+		       lan966x, ANA_AGGR_CFG);
+		break;
+	default:
+		NL_SET_ERR_MSG_MOD(extack,
+				   "LAG device using unsupported hash type");
+		return notifier_from_errno(-EINVAL);
+	}
+
+	return NOTIFY_OK;
+}
+
+int lan966x_lag_port_changelowerstate(struct net_device *dev,
+				      struct netdev_notifier_changelowerstate_info *info)
+{
+	struct netdev_lag_lower_state_info *lag = info->lower_state_info;
+	struct lan966x_port *port = netdev_priv(dev);
+	struct lan966x *lan966x = port->lan966x;
+	bool is_active;
+
+	if (!port->bond)
+		return NOTIFY_DONE;
+
+	is_active = lag->link_up && lag->tx_enabled;
+	if (port->lag_tx_active == is_active)
+		return NOTIFY_DONE;
+
+	port->lag_tx_active = is_active;
+	lan966x_lag_set_aggr_pgids(lan966x);
+
+	return NOTIFY_OK;
+}
+
+int lan966x_lag_netdev_prechangeupper(struct net_device *dev,
+				      struct netdev_notifier_changeupper_info *info)
+{
+	struct lan966x_port *port;
+	struct net_device *lower;
+	struct list_head *iter;
+	int err;
+
+	netdev_for_each_lower_dev(dev, lower, iter) {
+		if (!lan966x_netdevice_check(lower))
+			continue;
+
+		port = netdev_priv(lower);
+		if (port->bond != dev)
+			continue;
+
+		err = lan966x_port_prechangeupper(lower, dev, info);
+		if (err)
+			return err;
+	}
+
+	return NOTIFY_DONE;
+}
+
+int lan966x_lag_netdev_changeupper(struct net_device *dev,
+				   struct netdev_notifier_changeupper_info *info)
+{
+	struct lan966x_port *port;
+	struct net_device *lower;
+	struct list_head *iter;
+	int err;
+
+	netdev_for_each_lower_dev(dev, lower, iter) {
+		if (!lan966x_netdevice_check(lower))
+			continue;
+
+		port = netdev_priv(lower);
+		if (port->bond != dev)
+			continue;
+
+		err = lan966x_port_changeupper(lower, dev, info);
+		if (err)
+			return err;
+	}
+
+	return NOTIFY_DONE;
+}
+
+bool lan966x_lag_first_port(struct net_device *lag, struct net_device *dev)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+	struct lan966x *lan966x = port->lan966x;
+	unsigned long bond_mask;
+
+	if (port->bond != lag)
+		return false;
+
+	bond_mask = lan966x_lag_get_mask(lan966x, lag);
+	if (bond_mask && port->chip_port == __ffs(bond_mask))
+		return true;
+
+	return false;
+}
+
+u32 lan966x_lag_get_mask(struct lan966x *lan966x, struct net_device *bond)
+{
+	struct lan966x_port *port;
+	u32 mask = 0;
+	int p;
+
+	if (!bond)
+		return mask;
+
+	for (p = 0; p < lan966x->num_phys_ports; p++) {
+		port = lan966x->ports[p];
+		if (!port)
+			continue;
+
+		if (port->bond == bond)
+			mask |= BIT(p);
+	}
+
+	return mask;
+}
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index f820b1c71c47..35c713c87661 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -291,6 +291,9 @@ struct lan966x_port {
 	u8 ptp_cmd;
 	u16 ts_id;
 	struct sk_buff_head tx_skbs;
+
+	struct net_device *bond;
+	bool lag_tx_active;
 };
 
 extern const struct phylink_mac_ops lan966x_phylink_mac_ops;
@@ -408,6 +411,33 @@ int lan966x_fdma_init(struct lan966x *lan966x);
 void lan966x_fdma_deinit(struct lan966x *lan966x);
 irqreturn_t lan966x_fdma_irq_handler(int irq, void *args);
 
+int lan966x_lag_port_join(struct lan966x_port *port,
+			  struct net_device *brport_dev,
+			  struct net_device *bond,
+			  struct netlink_ext_ack *extack);
+void lan966x_lag_port_leave(struct lan966x_port *port, struct net_device *bond);
+int lan966x_lag_port_prechangeupper(struct net_device *dev,
+				    struct netdev_notifier_changeupper_info *info);
+int lan966x_lag_port_changelowerstate(struct net_device *dev,
+				      struct netdev_notifier_changelowerstate_info *info);
+int lan966x_lag_netdev_prechangeupper(struct net_device *dev,
+				      struct netdev_notifier_changeupper_info *info);
+int lan966x_lag_netdev_changeupper(struct net_device *dev,
+				   struct netdev_notifier_changeupper_info *info);
+bool lan966x_lag_first_port(struct net_device *lag, struct net_device *dev);
+u32 lan966x_lag_get_mask(struct lan966x *lan966x, struct net_device *bond);
+
+int lan966x_port_changeupper(struct net_device *dev,
+			     struct net_device *brport_dev,
+			     struct netdev_notifier_changeupper_info *info);
+int lan966x_port_prechangeupper(struct net_device *dev,
+				struct net_device *brport_dev,
+				struct netdev_notifier_changeupper_info *info);
+void lan966x_port_stp_state_set(struct lan966x_port *port, u8 state);
+void lan966x_port_ageing_set(struct lan966x_port *port,
+			     unsigned long ageing_clock_t);
+void lan966x_update_fwd_mask(struct lan966x *lan966x);
+
 static inline void __iomem *lan_addr(void __iomem *base[],
 				     int id, int tinst, int tcnt,
 				     int gbase, int ginst,
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
index fe872edfcdca..0cd53dca87ce 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
@@ -130,7 +130,7 @@ static int lan966x_port_pre_bridge_flags(struct lan966x_port *port,
 	return 0;
 }
 
-static void lan966x_update_fwd_mask(struct lan966x *lan966x)
+void lan966x_update_fwd_mask(struct lan966x *lan966x)
 {
 	int i;
 
@@ -138,9 +138,14 @@ static void lan966x_update_fwd_mask(struct lan966x *lan966x)
 		struct lan966x_port *port = lan966x->ports[i];
 		unsigned long mask = 0;
 
-		if (port && lan966x->bridge_fwd_mask & BIT(i))
+		if (port && lan966x->bridge_fwd_mask & BIT(i)) {
 			mask = lan966x->bridge_fwd_mask & ~BIT(i);
 
+			if (port->bond)
+				mask &= ~lan966x_lag_get_mask(lan966x,
+							      port->bond);
+		}
+
 		mask |= BIT(CPU_PORT);
 
 		lan_wr(ANA_PGID_PGID_SET(mask),
@@ -148,7 +153,7 @@ static void lan966x_update_fwd_mask(struct lan966x *lan966x)
 	}
 }
 
-static void lan966x_port_stp_state_set(struct lan966x_port *port, u8 state)
+void lan966x_port_stp_state_set(struct lan966x_port *port, u8 state)
 {
 	struct lan966x *lan966x = port->lan966x;
 	bool learn_ena = false;
@@ -169,8 +174,8 @@ static void lan966x_port_stp_state_set(struct lan966x_port *port, u8 state)
 	lan966x_update_fwd_mask(lan966x);
 }
 
-static void lan966x_port_ageing_set(struct lan966x_port *port,
-				    unsigned long ageing_clock_t)
+void lan966x_port_ageing_set(struct lan966x_port *port,
+			     unsigned long ageing_clock_t)
 {
 	unsigned long ageing_jiffies = clock_t_to_jiffies(ageing_clock_t);
 	u32 ageing_time = jiffies_to_msecs(ageing_jiffies) / 1000;
@@ -239,6 +244,7 @@ static int lan966x_port_attr_set(struct net_device *dev, const void *ctx,
 }
 
 static int lan966x_port_bridge_join(struct lan966x_port *port,
+				    struct net_device *brport_dev,
 				    struct net_device *bridge,
 				    struct netlink_ext_ack *extack)
 {
@@ -256,7 +262,7 @@ static int lan966x_port_bridge_join(struct lan966x_port *port,
 		}
 	}
 
-	err = switchdev_bridge_port_offload(dev, dev, port,
+	err = switchdev_bridge_port_offload(brport_dev, dev, port,
 					    &lan966x_switchdev_nb,
 					    &lan966x_switchdev_blocking_nb,
 					    false, extack);
@@ -293,8 +299,9 @@ static void lan966x_port_bridge_leave(struct lan966x_port *port,
 	lan966x_vlan_port_apply(port);
 }
 
-static int lan966x_port_changeupper(struct net_device *dev,
-				    struct netdev_notifier_changeupper_info *info)
+int lan966x_port_changeupper(struct net_device *dev,
+			     struct net_device *brport_dev,
+			     struct netdev_notifier_changeupper_info *info)
 {
 	struct lan966x_port *port = netdev_priv(dev);
 	struct netlink_ext_ack *extack;
@@ -304,26 +311,44 @@ static int lan966x_port_changeupper(struct net_device *dev,
 
 	if (netif_is_bridge_master(info->upper_dev)) {
 		if (info->linking)
-			err = lan966x_port_bridge_join(port, info->upper_dev,
+			err = lan966x_port_bridge_join(port, brport_dev,
+						       info->upper_dev,
 						       extack);
 		else
 			lan966x_port_bridge_leave(port, info->upper_dev);
 	}
 
+	if (netif_is_lag_master(info->upper_dev)) {
+		if (info->linking)
+			err = lan966x_lag_port_join(port, info->upper_dev,
+						    info->upper_dev,
+						    extack);
+		else
+			lan966x_lag_port_leave(port, info->upper_dev);
+	}
+
 	return err;
 }
 
-static int lan966x_port_prechangeupper(struct net_device *dev,
-				       struct netdev_notifier_changeupper_info *info)
+int lan966x_port_prechangeupper(struct net_device *dev,
+				struct net_device *brport_dev,
+				struct netdev_notifier_changeupper_info *info)
 {
 	struct lan966x_port *port = netdev_priv(dev);
+	int err = NOTIFY_DONE;
 
 	if (netif_is_bridge_master(info->upper_dev) && !info->linking) {
 		switchdev_bridge_port_unoffload(port->dev, port, NULL, NULL);
 		lan966x_fdb_flush_workqueue(port->lan966x);
 	}
 
-	return NOTIFY_DONE;
+	if (netif_is_lag_master(info->upper_dev) && info->linking)
+		err = lan966x_lag_port_prechangeupper(dev, info);
+
+	if (netif_is_lag_master(info->upper_dev) && !info->linking)
+		switchdev_bridge_port_unoffload(brport_dev, port, NULL, NULL);
+
+	return err;
 }
 
 static int lan966x_foreign_bridging_check(struct net_device *upper,
@@ -401,21 +426,44 @@ static int lan966x_netdevice_port_event(struct net_device *dev,
 	int err = 0;
 
 	if (!lan966x_netdevice_check(dev)) {
-		if (event == NETDEV_CHANGEUPPER)
-			return lan966x_bridge_check(dev, ptr);
+		switch (event) {
+		case NETDEV_CHANGEUPPER:
+		case NETDEV_PRECHANGEUPPER:
+			err = lan966x_bridge_check(dev, ptr);
+			if (err)
+				return err;
+
+			if (netif_is_lag_master(dev)) {
+				if (event == NETDEV_CHANGEUPPER)
+					err = lan966x_lag_netdev_changeupper(dev,
+									     ptr);
+				else
+					err = lan966x_lag_netdev_prechangeupper(dev,
+										ptr);
+
+				return err;
+			}
+			break;
+		default:
+			return 0;
+		}
+
 		return 0;
 	}
 
 	switch (event) {
 	case NETDEV_PRECHANGEUPPER:
-		err = lan966x_port_prechangeupper(dev, ptr);
+		err = lan966x_port_prechangeupper(dev, dev, ptr);
 		break;
 	case NETDEV_CHANGEUPPER:
 		err = lan966x_bridge_check(dev, ptr);
 		if (err)
 			return err;
 
-		err = lan966x_port_changeupper(dev, ptr);
+		err = lan966x_port_changeupper(dev, dev, ptr);
+		break;
+	case NETDEV_CHANGELOWERSTATE:
+		err = lan966x_lag_port_changelowerstate(dev, ptr);
 		break;
 	}
 
@@ -433,19 +481,23 @@ static int lan966x_netdevice_event(struct notifier_block *nb,
 	return notifier_from_errno(ret);
 }
 
-/* We don't offload uppers such as LAG as bridge ports, so every device except
- * the bridge itself is foreign.
- */
 static bool lan966x_foreign_dev_check(const struct net_device *dev,
 				      const struct net_device *foreign_dev)
 {
 	struct lan966x_port *port = netdev_priv(dev);
 	struct lan966x *lan966x = port->lan966x;
+	int i;
 
 	if (netif_is_bridge_master(foreign_dev))
 		if (lan966x->bridge == foreign_dev)
 			return false;
 
+	if (netif_is_lag_master(foreign_dev))
+		for (i = 0; i < lan966x->num_phys_ports; ++i)
+			if (lan966x->ports[i] &&
+			    lan966x->ports[i]->bond == foreign_dev)
+				return false;
+
 	return true;
 }
 
-- 
2.33.0

[PATCH net-next v3 6/7] net: lan966x: Extend FDB to support also lag

[Horatiu Vultur]

Offload FDB entries when the original device is a lag interface. Because
all the ports under the lag have the same chip id, which is the chip id
of first port, then add the entries only for the first port.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ethernet/microchip/lan966x/lan966x_fdb.c  | 30 +++++++++++++++++++
 .../microchip/lan966x/lan966x_switchdev.c     |  4 ++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c
index 5142e7c0de31..2ea263e893ee 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c
@@ -199,6 +199,34 @@ static void lan966x_fdb_bridge_event_work(struct lan966x_fdb_event_work *fdb_wor
 	}
 }
 
+static void lan966x_fdb_lag_event_work(struct lan966x_fdb_event_work *fdb_work)
+{
+	struct switchdev_notifier_fdb_info *fdb_info;
+	struct lan966x_port *port;
+	struct lan966x *lan966x;
+
+	if (!lan966x_lag_first_port(fdb_work->orig_dev, fdb_work->dev))
+		return;
+
+	lan966x = fdb_work->lan966x;
+	port = netdev_priv(fdb_work->dev);
+	fdb_info = &fdb_work->fdb_info;
+
+	switch (fdb_work->event) {
+	case SWITCHDEV_FDB_ADD_TO_DEVICE:
+		if (!fdb_info->added_by_user)
+			break;
+		lan966x_mac_add_entry(lan966x, port, fdb_info->addr,
+				      fdb_info->vid);
+		break;
+	case SWITCHDEV_FDB_DEL_TO_DEVICE:
+		if (!fdb_info->added_by_user)
+			break;
+		lan966x_mac_del_entry(lan966x, fdb_info->addr, fdb_info->vid);
+		break;
+	}
+}
+
 static void lan966x_fdb_event_work(struct work_struct *work)
 {
 	struct lan966x_fdb_event_work *fdb_work =
@@ -208,6 +236,8 @@ static void lan966x_fdb_event_work(struct work_struct *work)
 		lan966x_fdb_port_event_work(fdb_work);
 	else if (netif_is_bridge_master(fdb_work->orig_dev))
 		lan966x_fdb_bridge_event_work(fdb_work);
+	else if (netif_is_lag_master(fdb_work->orig_dev))
+		lan966x_fdb_lag_event_work(fdb_work);
 
 	kfree(fdb_work->fdb_info.addr);
 	kfree(fdb_work);
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
index 0cd53dca87ce..ced0194f6246 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
@@ -345,8 +345,10 @@ int lan966x_port_prechangeupper(struct net_device *dev,
 	if (netif_is_lag_master(info->upper_dev) && info->linking)
 		err = lan966x_lag_port_prechangeupper(dev, info);
 
-	if (netif_is_lag_master(info->upper_dev) && !info->linking)
+	if (netif_is_lag_master(info->upper_dev) && !info->linking) {
 		switchdev_bridge_port_unoffload(brport_dev, port, NULL, NULL);
+		lan966x_fdb_flush_workqueue(port->lan966x);
+	}
 
 	return err;
 }
-- 
2.33.0

[PATCH net-next v3 7/7] net: lan966x: Extend MAC to support also lag interfaces.

[Horatiu Vultur]

Extend MAC support to support also lag interfaces:
1. In case an entry is learned on a port that is part of lag interface,
   then notify the upper layers that the entry is learned on the bond
   interface
2. If a port leaves the bond and the port is the first port in the lag
   group, then it is required to update all MAC entries to change the
   destination port.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ethernet/microchip/lan966x/lan966x_lag.c  | 26 ++++++++
 .../ethernet/microchip/lan966x/lan966x_mac.c  | 66 ++++++++++++++++---
 .../ethernet/microchip/lan966x/lan966x_main.h |  5 ++
 3 files changed, 89 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_lag.c b/drivers/net/ethernet/microchip/lan966x/lan966x_lag.c
index 8fd62611cfa4..35c11a4a3dbe 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_lag.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_lag.c
@@ -123,8 +123,14 @@ int lan966x_lag_port_join(struct lan966x_port *port,
 {
 	struct lan966x *lan966x = port->lan966x;
 	struct net_device *dev = port->dev;
+	u32 lag_id = -1;
+	u32 bond_mask;
 	int err;
 
+	bond_mask = lan966x_lag_get_mask(lan966x, bond);
+	if (bond_mask)
+		lag_id = __ffs(bond_mask);
+
 	port->bond = bond;
 	lan966x_lag_update_ids(lan966x);
 
@@ -137,6 +143,12 @@ int lan966x_lag_port_join(struct lan966x_port *port,
 
 	lan966x_port_stp_state_set(port, br_port_get_stp_state(brport_dev));
 
+	if (lan966x_lag_first_port(port->bond, port->dev) &&
+	    lag_id != -1)
+		lan966x_mac_lag_replace_port_entry(lan966x,
+						   lan966x->ports[lag_id],
+						   port);
+
 	return 0;
 
 out:
@@ -149,6 +161,20 @@ int lan966x_lag_port_join(struct lan966x_port *port,
 void lan966x_lag_port_leave(struct lan966x_port *port, struct net_device *bond)
 {
 	struct lan966x *lan966x = port->lan966x;
+	u32 bond_mask;
+	u32 lag_id;
+
+	if (lan966x_lag_first_port(port->bond, port->dev)) {
+		bond_mask = lan966x_lag_get_mask(lan966x, port->bond);
+		bond_mask &= ~BIT(port->chip_port);
+		if (bond_mask) {
+			lag_id = __ffs(bond_mask);
+			lan966x_mac_lag_replace_port_entry(lan966x, port,
+							   lan966x->ports[lag_id]);
+		} else {
+			lan966x_mac_lag_remove_port_entry(lan966x, port);
+		}
+	}
 
 	port->bond = NULL;
 	lan966x_lag_update_ids(lan966x);
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
index 005e56ea5da1..3348453d89df 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
@@ -22,6 +22,7 @@ struct lan966x_mac_entry {
 	u16 vid;
 	u16 port_index;
 	int row;
+	bool lag;
 };
 
 struct lan966x_mac_raw_entry {
@@ -156,8 +157,9 @@ void lan966x_mac_init(struct lan966x *lan966x)
 	INIT_LIST_HEAD(&lan966x->mac_entries);
 }
 
-static struct lan966x_mac_entry *lan966x_mac_alloc_entry(const unsigned char *mac,
-							 u16 vid, u16 port_index)
+static struct lan966x_mac_entry *lan966x_mac_alloc_entry(struct lan966x_port *port,
+							 const unsigned char *mac,
+							 u16 vid)
 {
 	struct lan966x_mac_entry *mac_entry;
 
@@ -167,8 +169,9 @@ static struct lan966x_mac_entry *lan966x_mac_alloc_entry(const unsigned char *ma
 
 	memcpy(mac_entry->mac, mac, ETH_ALEN);
 	mac_entry->vid = vid;
-	mac_entry->port_index = port_index;
+	mac_entry->port_index = port->chip_port;
 	mac_entry->row = LAN966X_MAC_INVALID_ROW;
+	mac_entry->lag = port->bond ? true : false;
 	return mac_entry;
 }
 
@@ -245,7 +248,7 @@ int lan966x_mac_add_entry(struct lan966x *lan966x, struct lan966x_port *port,
 		return lan966x_mac_learn(lan966x, port->chip_port,
 					 addr, vid, ENTRYTYPE_LOCKED);
 
-	mac_entry = lan966x_mac_alloc_entry(addr, vid, port->chip_port);
+	mac_entry = lan966x_mac_alloc_entry(port, addr, vid);
 	if (!mac_entry)
 		return -ENOMEM;
 
@@ -254,7 +257,8 @@ int lan966x_mac_add_entry(struct lan966x *lan966x, struct lan966x_port *port,
 	spin_unlock(&lan966x->mac_lock);
 
 	lan966x_mac_learn(lan966x, port->chip_port, addr, vid, ENTRYTYPE_LOCKED);
-	lan966x_fdb_call_notifiers(SWITCHDEV_FDB_OFFLOADED, addr, vid, port->dev);
+	lan966x_fdb_call_notifiers(SWITCHDEV_FDB_OFFLOADED, addr, vid,
+				   port->bond ?: port->dev);
 
 	return 0;
 }
@@ -281,6 +285,48 @@ int lan966x_mac_del_entry(struct lan966x *lan966x, const unsigned char *addr,
 	return 0;
 }
 
+void lan966x_mac_lag_replace_port_entry(struct lan966x *lan966x,
+					struct lan966x_port *src,
+					struct lan966x_port *dst)
+{
+	struct lan966x_mac_entry *mac_entry;
+
+	spin_lock(&lan966x->mac_lock);
+	list_for_each_entry(mac_entry, &lan966x->mac_entries, list) {
+		if (mac_entry->port_index == src->chip_port &&
+		    mac_entry->lag) {
+			lan966x_mac_forget(lan966x, mac_entry->mac, mac_entry->vid,
+					   ENTRYTYPE_LOCKED);
+
+			lan966x_mac_learn(lan966x, dst->chip_port,
+					  mac_entry->mac, mac_entry->vid,
+					  ENTRYTYPE_LOCKED);
+			mac_entry->port_index = dst->chip_port;
+		}
+	}
+	spin_unlock(&lan966x->mac_lock);
+}
+
+void lan966x_mac_lag_remove_port_entry(struct lan966x *lan966x,
+				       struct lan966x_port *src)
+{
+	struct lan966x_mac_entry *mac_entry, *tmp;
+
+	spin_lock(&lan966x->mac_lock);
+	list_for_each_entry_safe(mac_entry, tmp, &lan966x->mac_entries,
+				 list) {
+		if (mac_entry->port_index == src->chip_port &&
+		    mac_entry->lag) {
+			lan966x_mac_forget(lan966x, mac_entry->mac, mac_entry->vid,
+					   ENTRYTYPE_LOCKED);
+
+			list_del(&mac_entry->list);
+			kfree(mac_entry);
+		}
+	}
+	spin_unlock(&lan966x->mac_lock);
+}
+
 void lan966x_mac_purge_entries(struct lan966x *lan966x)
 {
 	struct lan966x_mac_entry *mac_entry, *tmp;
@@ -325,6 +371,7 @@ static void lan966x_mac_irq_process(struct lan966x *lan966x, u32 row,
 {
 	struct lan966x_mac_entry *mac_entry, *tmp;
 	unsigned char mac[ETH_ALEN] __aligned(2);
+	struct lan966x_port *port;
 	u32 dest_idx;
 	u32 column;
 	u16 vid;
@@ -366,9 +413,10 @@ static void lan966x_mac_irq_process(struct lan966x *lan966x, u32 row,
 			 * anymore in the HW and remove the entry from the SW
 			 * list
 			 */
+			port = lan966x->ports[mac_entry->port_index];
 			lan966x_mac_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE,
 					      mac_entry->mac, mac_entry->vid,
-					      lan966x->ports[mac_entry->port_index]->dev);
+					      port->bond ?: port->dev);
 
 			list_del(&mac_entry->list);
 			kfree(mac_entry);
@@ -396,7 +444,8 @@ static void lan966x_mac_irq_process(struct lan966x *lan966x, u32 row,
 		if (WARN_ON(dest_idx >= lan966x->num_phys_ports))
 			continue;
 
-		mac_entry = lan966x_mac_alloc_entry(mac, vid, dest_idx);
+		port = lan966x->ports[dest_idx];
+		mac_entry = lan966x_mac_alloc_entry(port, mac, vid);
 		if (!mac_entry)
 			return;
 
@@ -407,7 +456,8 @@ static void lan966x_mac_irq_process(struct lan966x *lan966x, u32 row,
 		spin_unlock(&lan966x->mac_lock);
 
 		lan966x_mac_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE,
-				      mac, vid, lan966x->ports[dest_idx]->dev);
+				      mac, vid,
+				      port->bond ?: port->dev);
 	}
 }
 
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 35c713c87661..ca35cc95c661 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -349,6 +349,11 @@ int lan966x_mac_add_entry(struct lan966x *lan966x,
 			  struct lan966x_port *port,
 			  const unsigned char *addr,
 			  u16 vid);
+void lan966x_mac_lag_replace_port_entry(struct lan966x *lan966x,
+					struct lan966x_port *src,
+					struct lan966x_port *dst);
+void lan966x_mac_lag_remove_port_entry(struct lan966x *lan966x,
+				       struct lan966x_port *src);
 void lan966x_mac_purge_entries(struct lan966x *lan966x);
 irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x);
 
-- 
2.33.0

Re: [PATCH net-next v3 2/7] net: lan966x: Split lan966x_fdb_event_work

[Vladimir Oltean]

On Fri, Jul 01, 2022 at 10:52:22PM +0200, Horatiu Vultur wrote:
> Split the function lan966x_fdb_event_work. One case for when the
> orig_dev is a bridge and one case when orig_dev is lan966x port.
> This is preparation for lag support. There is no functional change.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---

> -static void lan966x_fdb_event_work(struct work_struct *work)
> +void lan966x_fdb_flush_workqueue(struct lan966x *lan966x)
> +{
> +	flush_workqueue(lan966x->fdb_work);
> +}
> +

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> index df2bee678559..d9fc6a9a3da1 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> @@ -320,9 +320,10 @@ static int lan966x_port_prechangeupper(struct net_device *dev,
>  {
>  	struct lan966x_port *port = netdev_priv(dev);
>  
> -	if (netif_is_bridge_master(info->upper_dev) && !info->linking)
> -		switchdev_bridge_port_unoffload(port->dev, port,
> -						NULL, NULL);
> +	if (netif_is_bridge_master(info->upper_dev) && !info->linking) {
> +		switchdev_bridge_port_unoffload(port->dev, port, NULL, NULL);
> +		lan966x_fdb_flush_workqueue(port->lan966x);
> +	}

Very curious as to why you decided to stuff this change in here.
There was no functional change in v2, now there is. And it's a change
you might need to come back to later (probably sooner than you'd like),
since the flushing of the workqueue is susceptible to causing deadlocks
if done improperly - let's see how you blame a commit that was only
supposed to move code, in that case ;)

The deadlock that I'm talking about comes from the fact that
lan966x_port_prechangeupper() runs with rtnl_lock() held. So the code of
the flushed workqueue item must not hold rtnl_lock(), or any other lock
that is blocked by the rtnl_lock(). Otherwise, the flushing will wait
for a workqueue item to complete, that in turn waits to acquire the
rtnl_lock, which is held by the thread waiting the workqueue to complete.

Analyzing your code, lan966x_mac_notifiers() takes rtnl_lock().
That is taken from threaded interrupt context - lan966x_mac_irq_process(),
but is a sub-lock of spin_lock(&lan966x->mac_lock).

There are 2 problems with that already: rtnl_lock() is a mutex => can
sleep, but &lan966x->mac_lock is a spin lock => is atomic. You can't
take rtnl_lock() from atomic context. Lockdep and/or CONFIG_DEBUG_ATOMIC_SLEEP
will tell you so much.

The second problem is the lock ordering inversion that this causes.
There exists a threaded IRQ which takes the locks in the order mac_lock
-> rtnl_lock, and there exists this new fdb_flush_workqueue which takes
the locks in the order rtnl_lock -> mac_lock. If they run at the same
time, kaboom. Again, lockdep will tell you as much.

I'm sorry, but you need to solve the existing locking problems with the
code first.

>  
>  	return NOTIFY_DONE;
>  }
> -- 
> 2.33.0
>

Re: [PATCH net-next v3 5/7] net: lan966x: Add lag support for lan966x.

[Vladimir Oltean]

On Fri, Jul 01, 2022 at 10:52:25PM +0200, Horatiu Vultur wrote:
> Add link aggregation hardware offload support for lan966x
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---

Superficially the code looks OK (maybe if you could only remove the
trailing "." from the commit message). A comment below.

> +int lan966x_lag_port_prechangeupper(struct net_device *dev,
> +				    struct netdev_notifier_changeupper_info *info)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +	struct lan966x *lan966x = port->lan966x;
> +	struct netdev_lag_upper_info *lui;
> +	struct netlink_ext_ack *extack;
> +
> +	extack = netdev_notifier_info_to_extack(&info->info);
> +	lui = info->upper_info;
> +	if (!lui)
> +		return NOTIFY_DONE;
> +
> +	if (lui->tx_type != NETDEV_LAG_TX_TYPE_HASH) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "LAG device using unsupported Tx type");
> +		return notifier_from_errno(-EINVAL);
> +	}
> +
> +	switch (lui->hash_type) {
> +	case NETDEV_LAG_HASH_L2:
> +		lan_wr(ANA_AGGR_CFG_AC_DMAC_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_SMAC_ENA_SET(1),
> +		       lan966x, ANA_AGGR_CFG);
> +		break;
> +	case NETDEV_LAG_HASH_L34:
> +		lan_wr(ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP4_SIPDIP_ENA_SET(1),
> +		       lan966x, ANA_AGGR_CFG);
> +		break;
> +	case NETDEV_LAG_HASH_L23:
> +		lan_wr(ANA_AGGR_CFG_AC_DMAC_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_SMAC_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA_SET(1),
> +		       lan966x, ANA_AGGR_CFG);

ANA_AGGR_CFG is global to the switch, yet it constantly changes with the
hash_type of each new offloaded LAG. So a LAG that hashes on L3/L4 will
break the hash mode of an existing L2 LAG. I think you should implement
a system that disallows changes to ANA_AGGR_CFG as long as there are
users of a certain mode.

> +		break;
> +	default:
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "LAG device using unsupported hash type");
> +		return notifier_from_errno(-EINVAL);
> +	}
> +
> +	return NOTIFY_OK;
> +}

Re: [PATCH net-next v3 4/7] net: lan966x: Extend lan966x_foreign_bridging_check

[Vladimir Oltean]

On Fri, Jul 01, 2022 at 10:52:24PM +0200, Horatiu Vultur wrote:
> Extend lan966x_foreign_bridging_check to check also if the upper
> interface is a lag device. Don't allow a lan966x port to be part of a
> lag if it has foreign interfaces.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../microchip/lan966x/lan966x_switchdev.c     | 32 ++++++++++++++-----
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> index d9b3ca5f6214..fe872edfcdca 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> @@ -326,23 +326,25 @@ static int lan966x_port_prechangeupper(struct net_device *dev,
>  	return NOTIFY_DONE;
>  }
>  
> -static int lan966x_foreign_bridging_check(struct net_device *bridge,
> +static int lan966x_foreign_bridging_check(struct net_device *upper,
> +					  bool *has_foreign,
> +					  bool *seen_lan966x,
>  					  struct netlink_ext_ack *extack)
>  {
>  	struct lan966x *lan966x = NULL;
> -	bool has_foreign = false;
>  	struct net_device *dev;
>  	struct list_head *iter;
>  
> -	if (!netif_is_bridge_master(bridge))
> +	if (!netif_is_bridge_master(upper) &&
> +	    !netif_is_lag_master(upper))
>  		return 0;
>  
> -	netdev_for_each_lower_dev(bridge, dev, iter) {
> +	netdev_for_each_lower_dev(upper, dev, iter) {
>  		if (lan966x_netdevice_check(dev)) {
>  			struct lan966x_port *port = netdev_priv(dev);
>  
>  			if (lan966x) {
> -				/* Bridge already has at least one port of a
> +				/* Upper already has at least one port of a
>  				 * lan966x switch inside it, check that it's
>  				 * the same instance of the driver.
>  				 */
> @@ -353,15 +355,24 @@ static int lan966x_foreign_bridging_check(struct net_device *bridge,
>  				}
>  			} else {
>  				/* This is the first lan966x port inside this
> -				 * bridge
> +				 * upper device
>  				 */
>  				lan966x = port->lan966x;
> +				*seen_lan966x = true;
>  			}
> +		} else if (netif_is_lag_master(dev)) {
> +			/* Allow to have bond interfaces that have only lan966x
> +			 * devices
> +			 */
> +			if (lan966x_foreign_bridging_check(dev, has_foreign,
> +							   seen_lan966x,
> +							   extack))
> +				*has_foreign = true;

Not clear why you set *has_foreign here and not just stop and return.
The extack has presumably already been populated by the called function,
there is absolutely no need to continue if an error has already been found.

>  		} else {
> -			has_foreign = true;
> +			*has_foreign = true;
>  		}
>  
> -		if (lan966x && has_foreign) {
> +		if (*seen_lan966x && *has_foreign) {
>  			NL_SET_ERR_MSG_MOD(extack,
>  					   "Bridging lan966x ports with foreign interfaces disallowed");
>  			return -EINVAL;
> @@ -374,7 +385,12 @@ static int lan966x_foreign_bridging_check(struct net_device *bridge,
>  static int lan966x_bridge_check(struct net_device *dev,
>  				struct netdev_notifier_changeupper_info *info)
>  {
> +	bool has_foreign = false;
> +	bool seen_lan966x = false;
> +
>  	return lan966x_foreign_bridging_check(info->upper_dev,
> +					      &has_foreign,
> +					      &seen_lan966x,
>  					      info->info.extack);
>  }
>  
> -- 
> 2.33.0
>

Re: [PATCH net-next v3 1/7] net: lan966x: Add reqisters used to configure lag interfaces

[Vladimir Oltean]

Comment regarding patch title: s/reqisters/registers/

Re: [PATCH net-next v3 5/7] net: lan966x: Add lag support for lan966x.

[kernel test robot]

Hi Horatiu,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Horatiu-Vultur/net-lan966x-Add-lag-support/20220702-045154
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git dbdd9a28e1406ab8218a69e60f10a168b968c81d
config: powerpc-randconfig-r012-20220703 (https://download.01.org/0day-ci/archive/20220706/202207060247.0TIpleTV-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/a531636c4ccc3d4528016f83627b2e4677e83e59
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Horatiu-Vultur/net-lan966x-Add-lag-support/20220702-045154
        git checkout a531636c4ccc3d4528016f83627b2e4677e83e59
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   powerpc-linux-ld: drivers/net/ethernet/microchip/lan966x/lan966x_lag.o: in function `lan966x_lag_port_join':
>> drivers/net/ethernet/microchip/lan966x/lan966x_lag.c:138: undefined reference to `br_port_get_stp_state'


vim +138 drivers/net/ethernet/microchip/lan966x/lan966x_lag.c

   118	
   119	int lan966x_lag_port_join(struct lan966x_port *port,
   120				  struct net_device *brport_dev,
   121				  struct net_device *bond,
   122				  struct netlink_ext_ack *extack)
   123	{
   124		struct lan966x *lan966x = port->lan966x;
   125		struct net_device *dev = port->dev;
   126		int err;
   127	
   128		port->bond = bond;
   129		lan966x_lag_update_ids(lan966x);
   130	
   131		err = switchdev_bridge_port_offload(brport_dev, dev, port,
   132						    &lan966x_switchdev_nb,
   133						    &lan966x_switchdev_blocking_nb,
   134						    false, extack);
   135		if (err)
   136			goto out;
   137	
 > 138		lan966x_port_stp_state_set(port, br_port_get_stp_state(brport_dev));
   139	
   140		return 0;
   141	
   142	out:
   143		port->bond = NULL;
   144		lan966x_lag_update_ids(lan966x);
   145	
   146		return err;
   147	}
   148	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH net-next v3 2/7] net: lan966x: Split lan966x_fdb_event_work

[Horatiu Vultur]

The 07/02/2022 14:08, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Jul 01, 2022 at 10:52:22PM +0200, Horatiu Vultur wrote:
> > Split the function lan966x_fdb_event_work. One case for when the
> > orig_dev is a bridge and one case when orig_dev is lan966x port.
> > This is preparation for lag support. There is no functional change.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> 
> > -static void lan966x_fdb_event_work(struct work_struct *work)
> > +void lan966x_fdb_flush_workqueue(struct lan966x *lan966x)
> > +{
> > +     flush_workqueue(lan966x->fdb_work);
> > +}
> > +
> 
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > index df2bee678559..d9fc6a9a3da1 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > @@ -320,9 +320,10 @@ static int lan966x_port_prechangeupper(struct net_device *dev,
> >  {
> >       struct lan966x_port *port = netdev_priv(dev);
> >
> > -     if (netif_is_bridge_master(info->upper_dev) && !info->linking)
> > -             switchdev_bridge_port_unoffload(port->dev, port,
> > -                                             NULL, NULL);
> > +     if (netif_is_bridge_master(info->upper_dev) && !info->linking) {
> > +             switchdev_bridge_port_unoffload(port->dev, port, NULL, NULL);
> > +             lan966x_fdb_flush_workqueue(port->lan966x);
> > +     }
> 
> Very curious as to why you decided to stuff this change in here.
> There was no functional change in v2, now there is. And it's a change
> you might need to come back to later (probably sooner than you'd like),
> since the flushing of the workqueue is susceptible to causing deadlocks
> if done improperly - let's see how you blame a commit that was only
> supposed to move code, in that case ;)

There is a functional change here and I forgot to change the commit
message for this.
> 
> The deadlock that I'm talking about comes from the fact that
> lan966x_port_prechangeupper() runs with rtnl_lock() held. So the code of
> the flushed workqueue item must not hold rtnl_lock(), or any other lock
> that is blocked by the rtnl_lock(). Otherwise, the flushing will wait
> for a workqueue item to complete, that in turn waits to acquire the
> rtnl_lock, which is held by the thread waiting the workqueue to complete.
> 
> Analyzing your code, lan966x_mac_notifiers() takes rtnl_lock().
> That is taken from threaded interrupt context - lan966x_mac_irq_process(),
> but is a sub-lock of spin_lock(&lan966x->mac_lock).
> 
> There are 2 problems with that already: rtnl_lock() is a mutex => can
> sleep, but &lan966x->mac_lock is a spin lock => is atomic. You can't
> take rtnl_lock() from atomic context. Lockdep and/or CONFIG_DEBUG_ATOMIC_SLEEP
> will tell you so much.
> 
> The second problem is the lock ordering inversion that this causes.
> There exists a threaded IRQ which takes the locks in the order mac_lock
> -> rtnl_lock, and there exists this new fdb_flush_workqueue which takes
> the locks in the order rtnl_lock -> mac_lock. If they run at the same
> time, kaboom. Again, lockdep will tell you as much.
> 
> I'm sorry, but you need to solve the existing locking problems with the
> code first.

As I see it, there 2 'different problems' which both have the same root
cause, the usage of the lan966x->mac_lock:
1. One is with lan966x_mac_notifiers and lan966x_mac_irq_process, which
is an issue on net. And this needs a separate patch.
2. Second is introduced by flushing the workqueue.

I am pretty sure I have run with CONFIG_DEBUG_ATOMIC_SLEEP but I
couldn't see any errors/warnings.

So let me start by fixing first issue on net.

> 
> >
> >       return NOTIFY_DONE;
> >  }
> > --
> > 2.33.0
> >

-- 
/Horatiu

Re: [PATCH net-next v3 5/7] net: lan966x: Add lag support for lan966x.

[Vladimir Oltean]

On Wed, Jul 06, 2022 at 02:38:37AM +0800, kernel test robot wrote:
>  > 138		lan966x_port_stp_state_set(port, br_port_get_stp_state(brport_dev));

When you call a symbol exported by the bridge driver (in this case
br_port_get_stp_state) you need to enforce the fact that you should be
built as module when the bridge is a module.

	depends on BRIDGE || BRIDGE=n