[PATCH net-next v5 0/9] net: lan966x: Add switchdev and vlan support

[PATCH net-next v5 0/9] net: lan966x: Add switchdev and vlan support

[Horatiu Vultur]

This patch series extends lan966x with switchdev and vlan support.
The first patches just adds new registers and extend the MAC table to
handle the interrupts when a new address is learn/forget.

v4->v5:
- make the notifier_block from lan966x to be singletones
- use switchdev_handle_port_obj_add and switchdev_handle_fdb_event_to_device
  when getting callbacks in the lan966x
- merge the two vlan patches in a single one

v3->v4:
- split the last patch in multiple patches
- replace spin_lock_irqsave/restore with spin_lock/spin_unlock
- remove lan966x_port_change_rx_flags because it was copying all the frames to
  the CPU instead of removing all RX filters.
- implement SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS
- remove calls to __dev_mc_unsync/sync as they are not needed
- replace 0/1 with false/true
- make sure that the lan966x ports are not added to bridges that have other
  interfaces except lan966x
- and allow the lan966x ports to be part of only the same bridge.

v2->v3:
- separate the PVID used when the port is in host mode or vlan unaware
- fix issue when the port was leaving the bridge

v1->v2:
- when allocating entries for the mac table use kzalloc instead of
  devm_kzalloc
- also use GFP_KERNEL instead of GFP_ATOMIC, because is never called
  in atomic context
- when deleting an mac table entry, the order of operations was wrong
- if ana irq is enabled make sure it gets disabled when the driver is
  removed

Horatiu Vultur (9):
  net: lan966x: Add registers that are used for switch and vlan
    functionality
  dt-bindings: net: lan966x: Extend with the analyzer interrupt
  net: lan966x: add support for interrupts from analyzer
  net: lan966x: More MAC table functionality
  net: lan966x: Remove .ndo_change_rx_flags
  net: lan966x: Add support to offload the forwarding.
  net: lan966x: Add vlan support.
  net: lan966x: Extend switchdev bridge flags
  net: lan966x: Extend switchdev with fdb support

 .../net/microchip,lan966x-switch.yaml         |   2 +
 .../net/ethernet/microchip/lan966x/Kconfig    |   1 +
 .../net/ethernet/microchip/lan966x/Makefile   |   3 +-
 .../ethernet/microchip/lan966x/lan966x_fdb.c  | 246 ++++++++
 .../ethernet/microchip/lan966x/lan966x_mac.c  | 342 +++++++++++
 .../ethernet/microchip/lan966x/lan966x_main.c | 109 +++-
 .../ethernet/microchip/lan966x/lan966x_main.h |  80 ++-
 .../ethernet/microchip/lan966x/lan966x_regs.h | 129 ++++
 .../microchip/lan966x/lan966x_switchdev.c     | 557 ++++++++++++++++++
 .../ethernet/microchip/lan966x/lan966x_vlan.c | 448 ++++++++++++++
 10 files changed, 1886 insertions(+), 31 deletions(-)
 create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c
 create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
 create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c

-- 
2.33.0

[PATCH net-next v5 1/9] net: lan966x: Add registers that are used for switch and vlan functionality

[Horatiu Vultur]

This patch adds the registers that will be used to enable switchdev and
vlan functionality in the HW.

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

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
index 879dcd807dec..2f2b26b9f8c6 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
@@ -61,6 +61,9 @@ enum lan966x_target {
 #define ANA_ADVLEARN_VLAN_CHK_GET(x)\
 	FIELD_GET(ANA_ADVLEARN_VLAN_CHK, x)
 
+/*      ANA:ANA:VLANMASK */
+#define ANA_VLANMASK              __REG(TARGET_ANA, 0, 1, 29824, 0, 1, 244, 8, 0, 1, 4)
+
 /*      ANA:ANA:ANAINTR */
 #define ANA_ANAINTR               __REG(TARGET_ANA, 0, 1, 29824, 0, 1, 244, 16, 0, 1, 4)
 
@@ -184,6 +187,102 @@ enum lan966x_target {
 #define ANA_MACACCESS_MAC_TABLE_CMD_GET(x)\
 	FIELD_GET(ANA_MACACCESS_MAC_TABLE_CMD, x)
 
+/*      ANA:ANA_TABLES:MACTINDX */
+#define ANA_MACTINDX              __REG(TARGET_ANA, 0, 1, 27520, 0, 1, 128, 52, 0, 1, 4)
+
+#define ANA_MACTINDX_BUCKET                      GENMASK(12, 11)
+#define ANA_MACTINDX_BUCKET_SET(x)\
+	FIELD_PREP(ANA_MACTINDX_BUCKET, x)
+#define ANA_MACTINDX_BUCKET_GET(x)\
+	FIELD_GET(ANA_MACTINDX_BUCKET, x)
+
+#define ANA_MACTINDX_M_INDEX                     GENMASK(10, 0)
+#define ANA_MACTINDX_M_INDEX_SET(x)\
+	FIELD_PREP(ANA_MACTINDX_M_INDEX, x)
+#define ANA_MACTINDX_M_INDEX_GET(x)\
+	FIELD_GET(ANA_MACTINDX_M_INDEX, x)
+
+/*      ANA:ANA_TABLES:VLAN_PORT_MASK */
+#define ANA_VLAN_PORT_MASK        __REG(TARGET_ANA, 0, 1, 27520, 0, 1, 128, 56, 0, 1, 4)
+
+#define ANA_VLAN_PORT_MASK_VLAN_PORT_MASK        GENMASK(8, 0)
+#define ANA_VLAN_PORT_MASK_VLAN_PORT_MASK_SET(x)\
+	FIELD_PREP(ANA_VLAN_PORT_MASK_VLAN_PORT_MASK, x)
+#define ANA_VLAN_PORT_MASK_VLAN_PORT_MASK_GET(x)\
+	FIELD_GET(ANA_VLAN_PORT_MASK_VLAN_PORT_MASK, x)
+
+/*      ANA:ANA_TABLES:VLANACCESS */
+#define ANA_VLANACCESS            __REG(TARGET_ANA, 0, 1, 27520, 0, 1, 128, 60, 0, 1, 4)
+
+#define ANA_VLANACCESS_VLAN_TBL_CMD              GENMASK(1, 0)
+#define ANA_VLANACCESS_VLAN_TBL_CMD_SET(x)\
+	FIELD_PREP(ANA_VLANACCESS_VLAN_TBL_CMD, x)
+#define ANA_VLANACCESS_VLAN_TBL_CMD_GET(x)\
+	FIELD_GET(ANA_VLANACCESS_VLAN_TBL_CMD, x)
+
+/*      ANA:ANA_TABLES:VLANTIDX */
+#define ANA_VLANTIDX              __REG(TARGET_ANA, 0, 1, 27520, 0, 1, 128, 64, 0, 1, 4)
+
+#define ANA_VLANTIDX_VLAN_PGID_CPU_DIS           BIT(18)
+#define ANA_VLANTIDX_VLAN_PGID_CPU_DIS_SET(x)\
+	FIELD_PREP(ANA_VLANTIDX_VLAN_PGID_CPU_DIS, x)
+#define ANA_VLANTIDX_VLAN_PGID_CPU_DIS_GET(x)\
+	FIELD_GET(ANA_VLANTIDX_VLAN_PGID_CPU_DIS, x)
+
+#define ANA_VLANTIDX_V_INDEX                     GENMASK(11, 0)
+#define ANA_VLANTIDX_V_INDEX_SET(x)\
+	FIELD_PREP(ANA_VLANTIDX_V_INDEX, x)
+#define ANA_VLANTIDX_V_INDEX_GET(x)\
+	FIELD_GET(ANA_VLANTIDX_V_INDEX, x)
+
+/*      ANA:PORT:VLAN_CFG */
+#define ANA_VLAN_CFG(g)           __REG(TARGET_ANA, 0, 1, 28672, g, 9, 128, 0, 0, 1, 4)
+
+#define ANA_VLAN_CFG_VLAN_AWARE_ENA              BIT(20)
+#define ANA_VLAN_CFG_VLAN_AWARE_ENA_SET(x)\
+	FIELD_PREP(ANA_VLAN_CFG_VLAN_AWARE_ENA, x)
+#define ANA_VLAN_CFG_VLAN_AWARE_ENA_GET(x)\
+	FIELD_GET(ANA_VLAN_CFG_VLAN_AWARE_ENA, x)
+
+#define ANA_VLAN_CFG_VLAN_POP_CNT                GENMASK(19, 18)
+#define ANA_VLAN_CFG_VLAN_POP_CNT_SET(x)\
+	FIELD_PREP(ANA_VLAN_CFG_VLAN_POP_CNT, x)
+#define ANA_VLAN_CFG_VLAN_POP_CNT_GET(x)\
+	FIELD_GET(ANA_VLAN_CFG_VLAN_POP_CNT, x)
+
+#define ANA_VLAN_CFG_VLAN_VID                    GENMASK(11, 0)
+#define ANA_VLAN_CFG_VLAN_VID_SET(x)\
+	FIELD_PREP(ANA_VLAN_CFG_VLAN_VID, x)
+#define ANA_VLAN_CFG_VLAN_VID_GET(x)\
+	FIELD_GET(ANA_VLAN_CFG_VLAN_VID, x)
+
+/*      ANA:PORT:DROP_CFG */
+#define ANA_DROP_CFG(g)           __REG(TARGET_ANA, 0, 1, 28672, g, 9, 128, 4, 0, 1, 4)
+
+#define ANA_DROP_CFG_DROP_UNTAGGED_ENA           BIT(6)
+#define ANA_DROP_CFG_DROP_UNTAGGED_ENA_SET(x)\
+	FIELD_PREP(ANA_DROP_CFG_DROP_UNTAGGED_ENA, x)
+#define ANA_DROP_CFG_DROP_UNTAGGED_ENA_GET(x)\
+	FIELD_GET(ANA_DROP_CFG_DROP_UNTAGGED_ENA, x)
+
+#define ANA_DROP_CFG_DROP_PRIO_S_TAGGED_ENA      BIT(3)
+#define ANA_DROP_CFG_DROP_PRIO_S_TAGGED_ENA_SET(x)\
+	FIELD_PREP(ANA_DROP_CFG_DROP_PRIO_S_TAGGED_ENA, x)
+#define ANA_DROP_CFG_DROP_PRIO_S_TAGGED_ENA_GET(x)\
+	FIELD_GET(ANA_DROP_CFG_DROP_PRIO_S_TAGGED_ENA, x)
+
+#define ANA_DROP_CFG_DROP_PRIO_C_TAGGED_ENA      BIT(2)
+#define ANA_DROP_CFG_DROP_PRIO_C_TAGGED_ENA_SET(x)\
+	FIELD_PREP(ANA_DROP_CFG_DROP_PRIO_C_TAGGED_ENA, x)
+#define ANA_DROP_CFG_DROP_PRIO_C_TAGGED_ENA_GET(x)\
+	FIELD_GET(ANA_DROP_CFG_DROP_PRIO_C_TAGGED_ENA, x)
+
+#define ANA_DROP_CFG_DROP_MC_SMAC_ENA            BIT(0)
+#define ANA_DROP_CFG_DROP_MC_SMAC_ENA_SET(x)\
+	FIELD_PREP(ANA_DROP_CFG_DROP_MC_SMAC_ENA, x)
+#define ANA_DROP_CFG_DROP_MC_SMAC_ENA_GET(x)\
+	FIELD_GET(ANA_DROP_CFG_DROP_MC_SMAC_ENA, x)
+
 /*      ANA:PORT:CPU_FWD_CFG */
 #define ANA_CPU_FWD_CFG(g)        __REG(TARGET_ANA, 0, 1, 28672, g, 9, 128, 96, 0, 1, 4)
 
@@ -589,6 +688,36 @@ enum lan966x_target {
 /*      QSYS:RES_CTRL:RES_CFG */
 #define QSYS_RES_CFG(g)           __REG(TARGET_QSYS, 0, 1, 32768, g, 1024, 8, 0, 0, 1, 4)
 
+/*      REW:PORT:PORT_VLAN_CFG */
+#define REW_PORT_VLAN_CFG(g)      __REG(TARGET_REW, 0, 1, 0, g, 10, 128, 0, 0, 1, 4)
+
+#define REW_PORT_VLAN_CFG_PORT_TPID              GENMASK(31, 16)
+#define REW_PORT_VLAN_CFG_PORT_TPID_SET(x)\
+	FIELD_PREP(REW_PORT_VLAN_CFG_PORT_TPID, x)
+#define REW_PORT_VLAN_CFG_PORT_TPID_GET(x)\
+	FIELD_GET(REW_PORT_VLAN_CFG_PORT_TPID, x)
+
+#define REW_PORT_VLAN_CFG_PORT_VID               GENMASK(11, 0)
+#define REW_PORT_VLAN_CFG_PORT_VID_SET(x)\
+	FIELD_PREP(REW_PORT_VLAN_CFG_PORT_VID, x)
+#define REW_PORT_VLAN_CFG_PORT_VID_GET(x)\
+	FIELD_GET(REW_PORT_VLAN_CFG_PORT_VID, x)
+
+/*      REW:PORT:TAG_CFG */
+#define REW_TAG_CFG(g)            __REG(TARGET_REW, 0, 1, 0, g, 10, 128, 4, 0, 1, 4)
+
+#define REW_TAG_CFG_TAG_CFG                      GENMASK(8, 7)
+#define REW_TAG_CFG_TAG_CFG_SET(x)\
+	FIELD_PREP(REW_TAG_CFG_TAG_CFG, x)
+#define REW_TAG_CFG_TAG_CFG_GET(x)\
+	FIELD_GET(REW_TAG_CFG_TAG_CFG, x)
+
+#define REW_TAG_CFG_TAG_TPID_CFG                 GENMASK(6, 5)
+#define REW_TAG_CFG_TAG_TPID_CFG_SET(x)\
+	FIELD_PREP(REW_TAG_CFG_TAG_TPID_CFG, x)
+#define REW_TAG_CFG_TAG_TPID_CFG_GET(x)\
+	FIELD_GET(REW_TAG_CFG_TAG_TPID_CFG, x)
+
 /*      REW:PORT:PORT_CFG */
 #define REW_PORT_CFG(g)           __REG(TARGET_REW, 0, 1, 0, g, 10, 128, 8, 0, 1, 4)
 
-- 
2.33.0

[PATCH net-next v5 2/9] dt-bindings: net: lan966x: Extend with the analyzer interrupt

[Horatiu Vultur]

Extend dt-bindings for lan966x with analyzer interrupt.
This interrupt can be generated for example when the HW learn/forgets
an entry in the MAC table.

Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../devicetree/bindings/net/microchip,lan966x-switch.yaml       | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml b/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml
index 5bee665d5fcf..e79e4e166ad8 100644
--- a/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml
+++ b/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml
@@ -37,12 +37,14 @@ properties:
     items:
       - description: register based extraction
       - description: frame dma based extraction
+      - description: analyzer interrupt
 
   interrupt-names:
     minItems: 1
     items:
       - const: xtr
       - const: fdma
+      - const: ana
 
   resets:
     items:
-- 
2.33.0

[PATCH net-next v5 3/9] net: lan966x: add support for interrupts from analyzer

[Horatiu Vultur]

This patch adds support for handling the interrupts generated by the
analyzer. Currently, only the MAC table generates these interrupts.
The MAC table will generate an interrupt whenever it learns or forgets
an entry in the table. It is the SW responsibility figure out which
entries were added/removed.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ethernet/microchip/lan966x/lan966x_mac.c  | 237 ++++++++++++++++++
 .../ethernet/microchip/lan966x/lan966x_main.c |  23 ++
 .../ethernet/microchip/lan966x/lan966x_main.h |   6 +
 3 files changed, 266 insertions(+)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
index f6878b9f57ef..855ea514f438 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0+
 
+#include <net/switchdev.h>
 #include "lan966x_main.h"
 
 #define LAN966X_MAC_COLUMNS		4
@@ -13,6 +14,23 @@
 #define MACACCESS_CMD_WRITE		7
 #define MACACCESS_CMD_SYNC_GET_NEXT	8
 
+#define LAN966X_MAC_INVALID_ROW		-1
+
+struct lan966x_mac_entry {
+	struct list_head list;
+	unsigned char mac[ETH_ALEN] __aligned(2);
+	u16 vid;
+	u16 port_index;
+	int row;
+};
+
+struct lan966x_mac_raw_entry {
+	u32 mach;
+	u32 macl;
+	u32 maca;
+	bool processed;
+};
+
 static int lan966x_mac_get_status(struct lan966x *lan966x)
 {
 	return lan_rd(lan966x, ANA_MACACCESS);
@@ -98,4 +116,223 @@ void lan966x_mac_init(struct lan966x *lan966x)
 	/* Clear the MAC table */
 	lan_wr(MACACCESS_CMD_INIT, lan966x, ANA_MACACCESS);
 	lan966x_mac_wait_for_completion(lan966x);
+
+	spin_lock_init(&lan966x->mac_lock);
+	INIT_LIST_HEAD(&lan966x->mac_entries);
+}
+
+static struct lan966x_mac_entry *lan966x_mac_alloc_entry(const unsigned char *mac,
+							 u16 vid, u16 port_index)
+{
+	struct lan966x_mac_entry *mac_entry;
+
+	mac_entry = kzalloc(sizeof(*mac_entry), GFP_KERNEL);
+	if (!mac_entry)
+		return NULL;
+
+	memcpy(mac_entry->mac, mac, ETH_ALEN);
+	mac_entry->vid = vid;
+	mac_entry->port_index = port_index;
+	mac_entry->row = LAN966X_MAC_INVALID_ROW;
+	return mac_entry;
+}
+
+static void lan966x_fdb_call_notifiers(enum switchdev_notifier_type type,
+				       const char *mac, u16 vid,
+				       struct net_device *dev)
+{
+	struct switchdev_notifier_fdb_info info = { 0 };
+
+	info.addr = mac;
+	info.vid = vid;
+	info.offloaded = true;
+	call_switchdev_notifiers(type, dev, &info.info, NULL);
+}
+
+void lan966x_mac_purge_entries(struct lan966x *lan966x)
+{
+	struct lan966x_mac_entry *mac_entry, *tmp;
+
+	spin_lock(&lan966x->mac_lock);
+	list_for_each_entry_safe(mac_entry, tmp, &lan966x->mac_entries,
+				 list) {
+		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);
+}
+
+static void lan966x_mac_notifiers(enum switchdev_notifier_type type,
+				  unsigned char *mac, u32 vid,
+				  struct net_device *dev)
+{
+	rtnl_lock();
+	lan966x_fdb_call_notifiers(type, mac, vid, dev);
+	rtnl_unlock();
+}
+
+static void lan966x_mac_process_raw_entry(struct lan966x_mac_raw_entry *raw_entry,
+					  u8 *mac, u16 *vid, u32 *dest_idx)
+{
+	mac[0] = (raw_entry->mach >> 8)  & 0xff;
+	mac[1] = (raw_entry->mach >> 0)  & 0xff;
+	mac[2] = (raw_entry->macl >> 24) & 0xff;
+	mac[3] = (raw_entry->macl >> 16) & 0xff;
+	mac[4] = (raw_entry->macl >> 8)  & 0xff;
+	mac[5] = (raw_entry->macl >> 0)  & 0xff;
+
+	*vid = (raw_entry->mach >> 16) & 0xfff;
+	*dest_idx = ANA_MACACCESS_DEST_IDX_GET(raw_entry->maca);
+}
+
+static void lan966x_mac_irq_process(struct lan966x *lan966x, u32 row,
+				    struct lan966x_mac_raw_entry *raw_entries)
+{
+	struct lan966x_mac_entry *mac_entry, *tmp;
+	unsigned char mac[ETH_ALEN] __aligned(2);
+	u32 dest_idx;
+	u32 column;
+	u16 vid;
+
+	spin_lock(&lan966x->mac_lock);
+	list_for_each_entry_safe(mac_entry, tmp, &lan966x->mac_entries, list) {
+		bool found = false;
+
+		if (mac_entry->row != row)
+			continue;
+
+		for (column = 0; column < LAN966X_MAC_COLUMNS; ++column) {
+			/* All the valid entries are at the start of the row,
+			 * so when get one invalid entry it can just skip the
+			 * rest of the columns
+			 */
+			if (!ANA_MACACCESS_VALID_GET(raw_entries[column].maca))
+				break;
+
+			lan966x_mac_process_raw_entry(&raw_entries[column],
+						      mac, &vid, &dest_idx);
+			WARN_ON(dest_idx > lan966x->num_phys_ports);
+
+			/* If the entry in SW is found, then there is nothing
+			 * to do
+			 */
+			if (mac_entry->vid == vid &&
+			    ether_addr_equal(mac_entry->mac, mac) &&
+			    mac_entry->port_index == dest_idx) {
+				raw_entries[column].processed = true;
+				found = true;
+				break;
+			}
+		}
+
+		if (!found) {
+			/* Notify the bridge that the entry doesn't exist
+			 * anymore in the HW and remove the entry from the SW
+			 * list
+			 */
+			lan966x_mac_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE,
+					      mac_entry->mac, mac_entry->vid,
+					      lan966x->ports[mac_entry->port_index]->dev);
+
+			list_del(&mac_entry->list);
+			kfree(mac_entry);
+		}
+	}
+	spin_unlock(&lan966x->mac_lock);
+
+	/* Now go to the list of columns and see if any entry was not in the SW
+	 * list, then that means that the entry is new so it needs to notify the
+	 * bridge.
+	 */
+	for (column = 0; column < LAN966X_MAC_COLUMNS; ++column) {
+		/* All the valid entries are at the start of the row, so when
+		 * get one invalid entry it can just skip the rest of the columns
+		 */
+		if (!ANA_MACACCESS_VALID_GET(raw_entries[column].maca))
+			break;
+
+		/* If the entry already exists then don't do anything */
+		if (raw_entries[column].processed)
+			continue;
+
+		lan966x_mac_process_raw_entry(&raw_entries[column],
+					      mac, &vid, &dest_idx);
+		WARN_ON(dest_idx > lan966x->num_phys_ports);
+
+		mac_entry = lan966x_mac_alloc_entry(mac, vid, dest_idx);
+		if (!mac_entry)
+			return;
+
+		mac_entry->row = row;
+
+		spin_lock(&lan966x->mac_lock);
+		list_add_tail(&mac_entry->list, &lan966x->mac_entries);
+		spin_unlock(&lan966x->mac_lock);
+
+		lan966x_mac_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE,
+				      mac, vid, lan966x->ports[dest_idx]->dev);
+	}
+}
+
+irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x)
+{
+	struct lan966x_mac_raw_entry entry[LAN966X_MAC_COLUMNS] = { 0 };
+	u32 index, column;
+	bool stop = true;
+	u32 val;
+
+	/* Start the scan from 0, 0 */
+	lan_wr(ANA_MACTINDX_M_INDEX_SET(0) |
+	       ANA_MACTINDX_BUCKET_SET(0),
+	       lan966x, ANA_MACTINDX);
+
+	while (1) {
+		lan_rmw(ANA_MACACCESS_MAC_TABLE_CMD_SET(MACACCESS_CMD_SYNC_GET_NEXT),
+			ANA_MACACCESS_MAC_TABLE_CMD,
+			lan966x, ANA_MACACCESS);
+		lan966x_mac_wait_for_completion(lan966x);
+
+		val = lan_rd(lan966x, ANA_MACTINDX);
+		index = ANA_MACTINDX_M_INDEX_GET(val);
+		column = ANA_MACTINDX_BUCKET_GET(val);
+
+		/* The SYNC-GET-NEXT returns all the entries(4) in a row in
+		 * which is suffered a change. By change it means that new entry
+		 * was added or an entry was removed because of ageing.
+		 * It would return all the columns for that row. And after that
+		 * it would return the next row The stop conditions of the
+		 * SYNC-GET-NEXT is when it reaches 'directly' to row 0
+		 * column 3. So if SYNC-GET-NEXT returns row 0 and column 0
+		 * then it is required to continue to read more even if it
+		 * reaches row 0 and column 3.
+		 */
+		if (index == 0 && column == 0)
+			stop = false;
+
+		if (column == LAN966X_MAC_COLUMNS - 1 &&
+		    index == 0 && stop)
+			break;
+
+		entry[column].mach = lan_rd(lan966x, ANA_MACHDATA);
+		entry[column].macl = lan_rd(lan966x, ANA_MACLDATA);
+		entry[column].maca = lan_rd(lan966x, ANA_MACACCESS);
+
+		/* Once all the columns are read process them */
+		if (column == LAN966X_MAC_COLUMNS - 1) {
+			lan966x_mac_irq_process(lan966x, index, entry);
+			/* A row was processed so it is safe to assume that the
+			 * next row/column can be the stop condition
+			 */
+			stop = true;
+		}
+	}
+
+	lan_rmw(ANA_ANAINTR_INTR_SET(0),
+		ANA_ANAINTR_INTR,
+		lan966x, ANA_ANAINTR);
+
+	return IRQ_HANDLED;
 }
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 101c1f005baf..7c6d6293611a 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -527,6 +527,13 @@ static irqreturn_t lan966x_xtr_irq_handler(int irq, void *args)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t lan966x_ana_irq_handler(int irq, void *args)
+{
+	struct lan966x *lan966x = args;
+
+	return lan966x_mac_irq_handler(lan966x);
+}
+
 static void lan966x_cleanup_ports(struct lan966x *lan966x)
 {
 	struct lan966x_port *port;
@@ -554,6 +561,11 @@ static void lan966x_cleanup_ports(struct lan966x *lan966x)
 
 	disable_irq(lan966x->xtr_irq);
 	lan966x->xtr_irq = -ENXIO;
+
+	if (lan966x->ana_irq) {
+		disable_irq(lan966x->ana_irq);
+		lan966x->ana_irq = -ENXIO;
+	}
 }
 
 static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
@@ -870,6 +882,15 @@ static int lan966x_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	lan966x->ana_irq = platform_get_irq_byname(pdev, "ana");
+	if (lan966x->ana_irq) {
+		err = devm_request_threaded_irq(&pdev->dev, lan966x->ana_irq, NULL,
+						lan966x_ana_irq_handler, IRQF_ONESHOT,
+						"ana irq", lan966x);
+		if (err)
+			return dev_err_probe(&pdev->dev, err, "Unable to use ana irq");
+	}
+
 	/* init switch */
 	lan966x_init(lan966x);
 	lan966x_stats_init(lan966x);
@@ -923,6 +944,8 @@ static int lan966x_remove(struct platform_device *pdev)
 	destroy_workqueue(lan966x->stats_queue);
 	mutex_destroy(&lan966x->stats_lock);
 
+	lan966x_mac_purge_entries(lan966x);
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 7e5a3b6f168d..ba548d65b58a 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -75,6 +75,9 @@ struct lan966x {
 
 	u8 base_mac[ETH_ALEN];
 
+	struct list_head mac_entries;
+	spinlock_t mac_lock; /* lock for mac_entries list */
+
 	/* stats */
 	const struct lan966x_stat_layout *stats_layout;
 	u32 num_stats;
@@ -87,6 +90,7 @@ struct lan966x {
 
 	/* interrupts */
 	int xtr_irq;
+	int ana_irq;
 };
 
 struct lan966x_port_config {
@@ -141,6 +145,8 @@ int lan966x_mac_forget(struct lan966x *lan966x,
 int lan966x_mac_cpu_learn(struct lan966x *lan966x, const char *addr, u16 vid);
 int lan966x_mac_cpu_forget(struct lan966x *lan966x, const char *addr, u16 vid);
 void lan966x_mac_init(struct lan966x *lan966x);
+void lan966x_mac_purge_entries(struct lan966x *lan966x);
+irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x);
 
 static inline void __iomem *lan_addr(void __iomem *base[],
 				     int id, int tinst, int tcnt,
-- 
2.33.0

[PATCH net-next v5 4/9] net: lan966x: More MAC table functionality

[Horatiu Vultur]

This patch adds support for adding/removing mac entries in the SW list
of entries and in the HW table. This is used by the bridge
functionality.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ethernet/microchip/lan966x/lan966x_mac.c  | 105 ++++++++++++++++++
 .../ethernet/microchip/lan966x/lan966x_main.h |   9 ++
 2 files changed, 114 insertions(+)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
index 855ea514f438..efadb8d326cc 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
@@ -111,6 +111,14 @@ int lan966x_mac_cpu_forget(struct lan966x *lan966x, const char *addr, u16 vid)
 	return lan966x_mac_forget(lan966x, addr, vid, ENTRYTYPE_LOCKED);
 }
 
+void lan966x_mac_set_ageing(struct lan966x *lan966x,
+			    u32 ageing)
+{
+	lan_rmw(ANA_AUTOAGE_AGE_PERIOD_SET(ageing / 2),
+		ANA_AUTOAGE_AGE_PERIOD,
+		lan966x, ANA_AUTOAGE);
+}
+
 void lan966x_mac_init(struct lan966x *lan966x)
 {
 	/* Clear the MAC table */
@@ -137,6 +145,48 @@ static struct lan966x_mac_entry *lan966x_mac_alloc_entry(const unsigned char *ma
 	return mac_entry;
 }
 
+static struct lan966x_mac_entry *lan966x_mac_find_entry(struct lan966x *lan966x,
+							const unsigned char *mac,
+							u16 vid, u16 port_index)
+{
+	struct lan966x_mac_entry *res = NULL;
+	struct lan966x_mac_entry *mac_entry;
+
+	spin_lock(&lan966x->mac_lock);
+	list_for_each_entry(mac_entry, &lan966x->mac_entries, list) {
+		if (mac_entry->vid == vid &&
+		    ether_addr_equal(mac, mac_entry->mac) &&
+		    mac_entry->port_index == port_index) {
+			res = mac_entry;
+			break;
+		}
+	}
+	spin_unlock(&lan966x->mac_lock);
+
+	return res;
+}
+
+static int lan966x_mac_lookup(struct lan966x *lan966x,
+			      const unsigned char mac[ETH_ALEN],
+			      unsigned int vid, enum macaccess_entry_type type)
+{
+	int ret;
+
+	lan966x_mac_select(lan966x, mac, vid);
+
+	/* Issue a read command */
+	lan_wr(ANA_MACACCESS_ENTRYTYPE_SET(type) |
+	       ANA_MACACCESS_VALID_SET(1) |
+	       ANA_MACACCESS_MAC_TABLE_CMD_SET(MACACCESS_CMD_READ),
+	       lan966x, ANA_MACACCESS);
+
+	ret = lan966x_mac_wait_for_completion(lan966x);
+	if (ret)
+		return ret;
+
+	return ANA_MACACCESS_VALID_GET(lan_rd(lan966x, ANA_MACACCESS));
+}
+
 static void lan966x_fdb_call_notifiers(enum switchdev_notifier_type type,
 				       const char *mac, u16 vid,
 				       struct net_device *dev)
@@ -149,6 +199,61 @@ static void lan966x_fdb_call_notifiers(enum switchdev_notifier_type type,
 	call_switchdev_notifiers(type, dev, &info.info, NULL);
 }
 
+int lan966x_mac_add_entry(struct lan966x *lan966x, struct lan966x_port *port,
+			  const unsigned char *addr, u16 vid)
+{
+	struct lan966x_mac_entry *mac_entry;
+
+	if (lan966x_mac_lookup(lan966x, addr, vid, ENTRYTYPE_NORMAL))
+		return 0;
+
+	/* In case the entry already exists, don't add it again to SW,
+	 * just update HW, but we need to look in the actual HW because
+	 * it is possible for an entry to be learn by HW and before we
+	 * get the interrupt the frame will reach CPU and the CPU will
+	 * add the entry but without the extern_learn flag.
+	 */
+	mac_entry = lan966x_mac_find_entry(lan966x, addr, vid, port->chip_port);
+	if (mac_entry)
+		return lan966x_mac_learn(lan966x, port->chip_port,
+					 addr, vid, ENTRYTYPE_LOCKED);
+
+	mac_entry = lan966x_mac_alloc_entry(addr, vid, port->chip_port);
+	if (!mac_entry)
+		return -ENOMEM;
+
+	spin_lock(&lan966x->mac_lock);
+	list_add_tail(&mac_entry->list, &lan966x->mac_entries);
+	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);
+
+	return 0;
+}
+
+int lan966x_mac_del_entry(struct lan966x *lan966x, const unsigned char *addr,
+			  u16 vid)
+{
+	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->vid == vid &&
+		    ether_addr_equal(addr, mac_entry->mac)) {
+			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);
+
+	return 0;
+}
+
 void lan966x_mac_purge_entries(struct lan966x *lan966x)
 {
 	struct lan966x_mac_entry *mac_entry, *tmp;
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index ba548d65b58a..fcd5d09a070c 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -145,6 +145,15 @@ int lan966x_mac_forget(struct lan966x *lan966x,
 int lan966x_mac_cpu_learn(struct lan966x *lan966x, const char *addr, u16 vid);
 int lan966x_mac_cpu_forget(struct lan966x *lan966x, const char *addr, u16 vid);
 void lan966x_mac_init(struct lan966x *lan966x);
+void lan966x_mac_set_ageing(struct lan966x *lan966x,
+			    u32 ageing);
+int lan966x_mac_del_entry(struct lan966x *lan966x,
+			  const unsigned char *addr,
+			  u16 vid);
+int lan966x_mac_add_entry(struct lan966x *lan966x,
+			  struct lan966x_port *port,
+			  const unsigned char *addr,
+			  u16 vid);
 void lan966x_mac_purge_entries(struct lan966x *lan966x);
 irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x);
 
-- 
2.33.0

[PATCH net-next v5 5/9] net: lan966x: Remove .ndo_change_rx_flags

[Horatiu Vultur]

The function lan966x_port_change_rx_flags() was used only when
IFF_PROMISC flag was set. In that case it was setting to copy all the
frames to the CPU instead of removing any RX filters. Therefore remove
it.

Fixes: d28d6d2e37d10d ("net: lan966x: add port module support")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ethernet/microchip/lan966x/lan966x_main.c | 23 -------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 7c6d6293611a..dc40ac2eb246 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -298,28 +298,6 @@ static int lan966x_port_xmit(struct sk_buff *skb, struct net_device *dev)
 	return lan966x_port_ifh_xmit(skb, ifh, dev);
 }
 
-static void lan966x_set_promisc(struct lan966x_port *port, bool enable)
-{
-	struct lan966x *lan966x = port->lan966x;
-
-	lan_rmw(ANA_CPU_FWD_CFG_SRC_COPY_ENA_SET(enable),
-		ANA_CPU_FWD_CFG_SRC_COPY_ENA,
-		lan966x, ANA_CPU_FWD_CFG(port->chip_port));
-}
-
-static void lan966x_port_change_rx_flags(struct net_device *dev, int flags)
-{
-	struct lan966x_port *port = netdev_priv(dev);
-
-	if (!(flags & IFF_PROMISC))
-		return;
-
-	if (dev->flags & IFF_PROMISC)
-		lan966x_set_promisc(port, true);
-	else
-		lan966x_set_promisc(port, false);
-}
-
 static int lan966x_port_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct lan966x_port *port = netdev_priv(dev);
@@ -369,7 +347,6 @@ static const struct net_device_ops lan966x_port_netdev_ops = {
 	.ndo_open			= lan966x_port_open,
 	.ndo_stop			= lan966x_port_stop,
 	.ndo_start_xmit			= lan966x_port_xmit,
-	.ndo_change_rx_flags		= lan966x_port_change_rx_flags,
 	.ndo_change_mtu			= lan966x_port_change_mtu,
 	.ndo_set_rx_mode		= lan966x_port_set_rx_mode,
 	.ndo_get_phys_port_name		= lan966x_port_get_phys_port_name,
-- 
2.33.0

[PATCH net-next v5 6/9] net: lan966x: Add support to offload the forwarding.

[Horatiu Vultur]

This patch adds basic support to offload in the HW the forwarding of the
frames. The driver registers to the switchdev callbacks and implements
the callbacks for attributes SWITCHDEV_ATTR_ID_PORT_STP_STATE and
SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME.
It is not allowed to add a lan966x port to a bridge that contains a
different interface than lan966x.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../net/ethernet/microchip/lan966x/Kconfig    |   1 +
 .../net/ethernet/microchip/lan966x/Makefile   |   2 +-
 .../ethernet/microchip/lan966x/lan966x_main.c |  16 +-
 .../ethernet/microchip/lan966x/lan966x_main.h |  11 +
 .../microchip/lan966x/lan966x_switchdev.c     | 393 ++++++++++++++++++
 5 files changed, 419 insertions(+), 4 deletions(-)
 create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c

diff --git a/drivers/net/ethernet/microchip/lan966x/Kconfig b/drivers/net/ethernet/microchip/lan966x/Kconfig
index 2860a8c9923d..ac273f84b69e 100644
--- a/drivers/net/ethernet/microchip/lan966x/Kconfig
+++ b/drivers/net/ethernet/microchip/lan966x/Kconfig
@@ -2,6 +2,7 @@ config LAN966X_SWITCH
 	tristate "Lan966x switch driver"
 	depends on HAS_IOMEM
 	depends on OF
+	depends on NET_SWITCHDEV
 	select PHYLINK
 	select PACKING
 	help
diff --git a/drivers/net/ethernet/microchip/lan966x/Makefile b/drivers/net/ethernet/microchip/lan966x/Makefile
index 2989ba528236..974229c51f55 100644
--- a/drivers/net/ethernet/microchip/lan966x/Makefile
+++ b/drivers/net/ethernet/microchip/lan966x/Makefile
@@ -6,4 +6,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_mac.o lan966x_ethtool.o lan966x_switchdev.o
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index dc40ac2eb246..ee453967da71 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -355,6 +355,11 @@ static const struct net_device_ops lan966x_port_netdev_ops = {
 	.ndo_get_port_parent_id		= lan966x_port_get_parent_id,
 };
 
+bool lan966x_netdevice_check(const struct net_device *dev)
+{
+	return dev->netdev_ops == &lan966x_port_netdev_ops;
+}
+
 static int lan966x_port_xtr_status(struct lan966x *lan966x, u8 grp)
 {
 	return lan_rd(lan966x, QS_XTR_RD(grp));
@@ -491,6 +496,9 @@ static irqreturn_t lan966x_xtr_irq_handler(int irq, void *args)
 
 		skb->protocol = eth_type_trans(skb, dev);
 
+		if (lan966x->bridge_mask & BIT(src_port))
+			skb->offload_fwd_mark = 1;
+
 		netif_rx_ni(skb);
 		dev->stats.rx_bytes += len;
 		dev->stats.rx_packets++;
@@ -578,9 +586,6 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
 
 	eth_hw_addr_gen(dev, lan966x->base_mac, p + 1);
 
-	lan966x_mac_learn(lan966x, PGID_CPU, dev->dev_addr, port->pvid,
-			  ENTRYTYPE_LOCKED);
-
 	port->phylink_config.dev = &port->dev->dev;
 	port->phylink_config.type = PHYLINK_NETDEV;
 	port->phylink_pcs.poll = true;
@@ -897,6 +902,8 @@ static int lan966x_probe(struct platform_device *pdev)
 		lan966x_port_init(lan966x->ports[p]);
 	}
 
+	lan966x_register_notifier_blocks(lan966x);
+
 	return 0;
 
 cleanup_ports:
@@ -915,6 +922,8 @@ static int lan966x_remove(struct platform_device *pdev)
 {
 	struct lan966x *lan966x = platform_get_drvdata(pdev);
 
+	lan966x_unregister_notifier_blocks(lan966x);
+
 	lan966x_cleanup_ports(lan966x);
 
 	cancel_delayed_work_sync(&lan966x->stats_work);
@@ -922,6 +931,7 @@ static int lan966x_remove(struct platform_device *pdev)
 	mutex_destroy(&lan966x->stats_lock);
 
 	lan966x_mac_purge_entries(lan966x);
+	lan966x_ext_purge_entries();
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index fcd5d09a070c..3d228c9c0521 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -75,6 +75,10 @@ struct lan966x {
 
 	u8 base_mac[ETH_ALEN];
 
+	struct net_device *bridge;
+	u16 bridge_mask;
+	u16 bridge_fwd_mask;
+
 	struct list_head mac_entries;
 	spinlock_t mac_lock; /* lock for mac_entries list */
 
@@ -122,6 +126,11 @@ 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;
 
+bool lan966x_netdevice_check(const struct net_device *dev);
+
+void lan966x_register_notifier_blocks(struct lan966x *lan966x);
+void lan966x_unregister_notifier_blocks(struct lan966x *lan966x);
+
 void lan966x_stats_get(struct net_device *dev,
 		       struct rtnl_link_stats64 *stats);
 int lan966x_stats_init(struct lan966x *lan966x);
@@ -157,6 +166,8 @@ int lan966x_mac_add_entry(struct lan966x *lan966x,
 void lan966x_mac_purge_entries(struct lan966x *lan966x);
 irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x);
 
+void lan966x_ext_purge_entries(void);
+
 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
new file mode 100644
index 000000000000..722ce7cb61b3
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
@@ -0,0 +1,393 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/if_bridge.h>
+#include <net/switchdev.h>
+
+#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 LIST_HEAD(ext_entries);
+
+struct lan966x_ext_entry {
+	struct list_head list;
+	struct net_device *dev;
+	u32 ports;
+	struct lan966x *lan966x;
+};
+
+static void lan966x_update_fwd_mask(struct lan966x *lan966x)
+{
+	int i;
+
+	for (i = 0; i < lan966x->num_phys_ports; i++) {
+		struct lan966x_port *port = lan966x->ports[i];
+		unsigned long mask = 0;
+
+		if (port && lan966x->bridge_fwd_mask & BIT(i))
+			mask = lan966x->bridge_fwd_mask & ~BIT(i);
+
+		mask |= BIT(CPU_PORT);
+
+		lan_wr(ANA_PGID_PGID_SET(mask),
+		       lan966x, ANA_PGID(PGID_SRC + i));
+	}
+}
+
+static void lan966x_port_stp_state_set(struct lan966x_port *port, u8 state)
+{
+	struct lan966x *lan966x = port->lan966x;
+	bool learn_ena = false;
+
+	if (state == BR_STATE_FORWARDING || state == BR_STATE_LEARNING)
+		learn_ena = true;
+
+	if (state == BR_STATE_FORWARDING)
+		lan966x->bridge_fwd_mask |= BIT(port->chip_port);
+	else
+		lan966x->bridge_fwd_mask &= ~BIT(port->chip_port);
+
+	lan_rmw(ANA_PORT_CFG_LEARN_ENA_SET(learn_ena),
+		ANA_PORT_CFG_LEARN_ENA,
+		lan966x, ANA_PORT_CFG(port->chip_port));
+
+	lan966x_update_fwd_mask(lan966x);
+}
+
+static 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;
+
+	lan966x_mac_set_ageing(port->lan966x, ageing_time);
+}
+
+static int lan966x_port_attr_set(struct net_device *dev, const void *ctx,
+				 const struct switchdev_attr *attr,
+				 struct netlink_ext_ack *extack)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+	int err = 0;
+
+	if (ctx && ctx != port)
+		return 0;
+
+	switch (attr->id) {
+	case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
+		lan966x_port_stp_state_set(port, attr->u.stp_state);
+		break;
+	case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
+		lan966x_port_ageing_set(port, attr->u.ageing_time);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	return err;
+}
+
+static int lan966x_port_bridge_join(struct lan966x_port *port,
+				    struct net_device *bridge,
+				    struct netlink_ext_ack *extack)
+{
+	struct lan966x *lan966x = port->lan966x;
+	struct net_device *dev = port->dev;
+	int err;
+
+	if (!lan966x->bridge_mask) {
+		lan966x->bridge = bridge;
+	} else {
+		if (lan966x->bridge != bridge)
+			return -ENODEV;
+	}
+
+	err = switchdev_bridge_port_offload(dev, dev, port,
+					    &lan966x_switchdev_nb,
+					    &lan966x_switchdev_blocking_nb,
+					    false, extack);
+	if (err)
+		return err;
+
+	lan966x->bridge_mask |= BIT(port->chip_port);
+
+	return 0;
+}
+
+static void lan966x_port_bridge_leave(struct lan966x_port *port,
+				      struct net_device *bridge)
+{
+	struct lan966x *lan966x = port->lan966x;
+
+	lan966x->bridge_mask &= ~BIT(port->chip_port);
+
+	if (!lan966x->bridge_mask)
+		lan966x->bridge = NULL;
+
+	lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, PORT_PVID);
+}
+
+static int lan966x_port_changeupper(struct net_device *dev,
+				    struct netdev_notifier_changeupper_info *info)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+	struct netlink_ext_ack *extack;
+	int err = 0;
+
+	extack = netdev_notifier_info_to_extack(&info->info);
+
+	if (netif_is_bridge_master(info->upper_dev)) {
+		if (info->linking)
+			err = lan966x_port_bridge_join(port, info->upper_dev,
+						       extack);
+		else
+			lan966x_port_bridge_leave(port, info->upper_dev);
+	}
+
+	return err;
+}
+
+static int lan966x_port_prechangeupper(struct net_device *dev,
+				       struct netdev_notifier_changeupper_info *info)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+
+	if (netif_is_bridge_master(info->upper_dev) && !info->linking)
+		switchdev_bridge_port_unoffload(port->dev, port,
+						&lan966x_switchdev_nb,
+						&lan966x_switchdev_blocking_nb);
+
+	return NOTIFY_DONE;
+}
+
+static int lan966x_port_add_addr(struct net_device *dev, bool up)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+	struct lan966x *lan966x = port->lan966x;
+	u16 vid;
+
+	vid = port->pvid;
+
+	if (up)
+		lan966x_mac_cpu_learn(lan966x, dev->dev_addr, vid);
+	else
+		lan966x_mac_cpu_forget(lan966x, dev->dev_addr, vid);
+
+	return 0;
+}
+
+static struct lan966x_ext_entry *lan966x_ext_find_entry(struct net_device *dev)
+{
+	struct lan966x_ext_entry *ext_entry;
+
+	list_for_each_entry(ext_entry, &ext_entries, list) {
+		if (ext_entry->dev == dev)
+			return ext_entry;
+	}
+
+	return NULL;
+}
+
+static bool lan966x_ext_add_entry(struct net_device *dev, void *lan966x)
+{
+	struct lan966x_ext_entry *ext_entry;
+
+	ext_entry = lan966x_ext_find_entry(dev);
+	if (ext_entry) {
+		if (ext_entry->lan966x)
+			return false;
+
+		ext_entry->ports++;
+		return true;
+	}
+
+	ext_entry = kzalloc(sizeof(*ext_entry), GFP_KERNEL);
+	if (!ext_entry)
+		return false;
+
+	ext_entry->dev = dev;
+	ext_entry->ports = 1;
+	ext_entry->lan966x = lan966x;
+	list_add_tail(&ext_entry->list, &ext_entries);
+	return true;
+}
+
+static void lan966x_ext_remove_entry(struct net_device *dev)
+{
+	struct lan966x_ext_entry *ext_entry;
+
+	ext_entry = lan966x_ext_find_entry(dev);
+	if (!ext_entry)
+		return;
+
+	ext_entry->ports--;
+	if (!ext_entry->ports) {
+		list_del(&ext_entry->list);
+		kfree(ext_entry);
+	}
+}
+
+void lan966x_ext_purge_entries(void)
+{
+	struct lan966x_ext_entry *ext_entry, *tmp;
+
+	list_for_each_entry_safe(ext_entry, tmp, &ext_entries, list) {
+		list_del(&ext_entry->list);
+		kfree(ext_entry);
+	}
+}
+
+static int lan966x_ext_check_entry(struct net_device *dev,
+				   unsigned long event,
+				   void *ptr)
+{
+	struct netdev_notifier_changeupper_info *info;
+
+	if (event != NETDEV_PRECHANGEUPPER)
+		return 0;
+
+	info = ptr;
+	if (!netif_is_bridge_master(info->upper_dev))
+		return 0;
+
+	if (info->linking) {
+		if (!lan966x_ext_add_entry(info->upper_dev, NULL))
+			return -EOPNOTSUPP;
+	} else {
+		lan966x_ext_remove_entry(info->upper_dev);
+	}
+
+	return NOTIFY_DONE;
+}
+
+static bool lan966x_port_ext_check_entry(struct net_device *dev,
+					 struct netdev_notifier_changeupper_info *info)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+	struct lan966x *lan966x = port->lan966x;
+	struct lan966x_ext_entry *entry;
+
+	if (!netif_is_bridge_master(info->upper_dev))
+		return true;
+
+	entry = lan966x_ext_find_entry(info->upper_dev);
+	if (info->linking) {
+		if (!entry)
+			return lan966x_ext_add_entry(info->upper_dev, lan966x);
+
+		if (entry->lan966x == lan966x) {
+			entry->ports++;
+			return true;
+		}
+	} else {
+		lan966x_ext_remove_entry(info->upper_dev);
+		return true;
+	}
+
+	return false;
+}
+
+static int lan966x_netdevice_port_event(struct net_device *dev,
+					struct notifier_block *nb,
+					unsigned long event, void *ptr)
+{
+	int err = 0;
+
+	if (!lan966x_netdevice_check(dev))
+		return lan966x_ext_check_entry(dev, event, ptr);
+
+	switch (event) {
+	case NETDEV_PRECHANGEUPPER:
+		if (!lan966x_port_ext_check_entry(dev, ptr))
+			return -EOPNOTSUPP;
+
+		err = lan966x_port_prechangeupper(dev, ptr);
+		break;
+	case NETDEV_CHANGEUPPER:
+		err = lan966x_port_changeupper(dev, ptr);
+		break;
+	case NETDEV_PRE_UP:
+		err = lan966x_port_add_addr(dev, true);
+		break;
+	case NETDEV_DOWN:
+		err = lan966x_port_add_addr(dev, false);
+		break;
+	}
+
+	return err;
+}
+
+static int lan966x_netdevice_event(struct notifier_block *nb,
+				   unsigned long event, void *ptr)
+{
+	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+	int ret;
+
+	ret = lan966x_netdevice_port_event(dev, nb, event, ptr);
+
+	return notifier_from_errno(ret);
+}
+
+static int lan966x_switchdev_event(struct notifier_block *nb,
+				   unsigned long event, void *ptr)
+{
+	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
+	int err;
+
+	switch (event) {
+	case SWITCHDEV_PORT_ATTR_SET:
+		err = switchdev_handle_port_attr_set(dev, ptr,
+						     lan966x_netdevice_check,
+						     lan966x_port_attr_set);
+		return notifier_from_errno(err);
+	}
+
+	return NOTIFY_DONE;
+}
+
+static int lan966x_switchdev_blocking_event(struct notifier_block *nb,
+					    unsigned long event,
+					    void *ptr)
+{
+	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
+	int err;
+
+	switch (event) {
+	case SWITCHDEV_PORT_ATTR_SET:
+		err = switchdev_handle_port_attr_set(dev, ptr,
+						     lan966x_netdevice_check,
+						     lan966x_port_attr_set);
+		return notifier_from_errno(err);
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block lan966x_netdevice_nb __read_mostly = {
+	.notifier_call = lan966x_netdevice_event,
+};
+
+static struct notifier_block lan966x_switchdev_nb __read_mostly = {
+	.notifier_call = lan966x_switchdev_event,
+};
+
+static struct notifier_block lan966x_switchdev_blocking_nb __read_mostly = {
+	.notifier_call = lan966x_switchdev_blocking_event,
+};
+
+void lan966x_register_notifier_blocks(struct lan966x *lan966x)
+{
+	register_netdevice_notifier(&lan966x_netdevice_nb);
+	register_switchdev_notifier(&lan966x_switchdev_nb);
+	register_switchdev_blocking_notifier(&lan966x_switchdev_blocking_nb);
+}
+
+void lan966x_unregister_notifier_blocks(struct lan966x *lan966x)
+{
+	unregister_switchdev_blocking_notifier(&lan966x_switchdev_blocking_nb);
+	unregister_switchdev_notifier(&lan966x_switchdev_nb);
+	unregister_netdevice_notifier(&lan966x_netdevice_nb);
+}
-- 
2.33.0

[PATCH net-next v5 7/9] net: lan966x: Add vlan support.

[Horatiu Vultur]

Extend the driver to support vlan filtering  by implementing the
switchdev calls SWITCHDEV_OBJ_ID_PORT_VLAN and
SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../net/ethernet/microchip/lan966x/Makefile   |   3 +-
 .../ethernet/microchip/lan966x/lan966x_main.c |  39 +-
 .../ethernet/microchip/lan966x/lan966x_main.h |  40 +-
 .../microchip/lan966x/lan966x_switchdev.c     | 113 ++++-
 .../ethernet/microchip/lan966x/lan966x_vlan.c | 444 ++++++++++++++++++
 5 files changed, 632 insertions(+), 7 deletions(-)
 create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c

diff --git a/drivers/net/ethernet/microchip/lan966x/Makefile b/drivers/net/ethernet/microchip/lan966x/Makefile
index 974229c51f55..d82e896c2e53 100644
--- a/drivers/net/ethernet/microchip/lan966x/Makefile
+++ b/drivers/net/ethernet/microchip/lan966x/Makefile
@@ -6,4 +6,5 @@
 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_mac.o lan966x_ethtool.o lan966x_switchdev.o \
+			lan966x_vlan.o
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index ee453967da71..881c1678f3e9 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -103,17 +103,18 @@ static int lan966x_create_targets(struct platform_device *pdev,
 static int lan966x_port_set_mac_address(struct net_device *dev, void *p)
 {
 	struct lan966x_port *port = netdev_priv(dev);
+	u16 pvid = lan966x_vlan_port_get_pvid(port);
 	struct lan966x *lan966x = port->lan966x;
 	const struct sockaddr *addr = p;
 	int ret;
 
 	/* Learn the new net device MAC address in the mac table. */
-	ret = lan966x_mac_cpu_learn(lan966x, addr->sa_data, port->pvid);
+	ret = lan966x_mac_cpu_learn(lan966x, addr->sa_data, pvid);
 	if (ret)
 		return ret;
 
 	/* Then forget the previous one. */
-	ret = lan966x_mac_cpu_forget(lan966x, dev->dev_addr, port->pvid);
+	ret = lan966x_mac_cpu_forget(lan966x, dev->dev_addr, pvid);
 	if (ret)
 		return ret;
 
@@ -283,6 +284,12 @@ static void lan966x_ifh_set_ipv(void *ifh, u64 bypass)
 		IFH_POS_IPV, IFH_LEN * 4, PACK, 0);
 }
 
+static void lan966x_ifh_set_vid(void *ifh, u64 vid)
+{
+	packing(ifh, &vid, IFH_POS_TCI + IFH_WID_TCI - 1,
+		IFH_POS_TCI, IFH_LEN * 4, PACK, 0);
+}
+
 static int lan966x_port_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct lan966x_port *port = netdev_priv(dev);
@@ -294,6 +301,7 @@ static int lan966x_port_xmit(struct sk_buff *skb, struct net_device *dev)
 	lan966x_ifh_set_port(ifh, BIT_ULL(port->chip_port));
 	lan966x_ifh_set_qos_class(ifh, skb->priority >= 7 ? 0x7 : skb->priority);
 	lan966x_ifh_set_ipv(ifh, skb->priority >= 7 ? 0x7 : skb->priority);
+	lan966x_ifh_set_vid(ifh, skb_vlan_tag_get(skb));
 
 	return lan966x_port_ifh_xmit(skb, ifh, dev);
 }
@@ -343,6 +351,18 @@ static int lan966x_port_get_parent_id(struct net_device *dev,
 	return 0;
 }
 
+static int lan966x_port_set_features(struct net_device *dev,
+				     netdev_features_t features)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+	netdev_features_t changed = dev->features ^ features;
+
+	if (changed & NETIF_F_HW_VLAN_CTAG_FILTER)
+		lan966x_vlan_mode(port, features);
+
+	return 0;
+}
+
 static const struct net_device_ops lan966x_port_netdev_ops = {
 	.ndo_open			= lan966x_port_open,
 	.ndo_stop			= lan966x_port_stop,
@@ -353,6 +373,9 @@ static const struct net_device_ops lan966x_port_netdev_ops = {
 	.ndo_get_stats64		= lan966x_stats_get,
 	.ndo_set_mac_address		= lan966x_port_set_mac_address,
 	.ndo_get_port_parent_id		= lan966x_port_get_parent_id,
+	.ndo_set_features		= lan966x_port_set_features,
+	.ndo_vlan_rx_add_vid		= lan966x_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid		= lan966x_vlan_rx_kill_vid,
 };
 
 bool lan966x_netdevice_check(const struct net_device *dev)
@@ -575,13 +598,16 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
 	port->dev = dev;
 	port->lan966x = lan966x;
 	port->chip_port = p;
-	port->pvid = PORT_PVID;
 	lan966x->ports[p] = port;
 
 	dev->max_mtu = ETH_MAX_MTU;
 
 	dev->netdev_ops = &lan966x_port_netdev_ops;
 	dev->ethtool_ops = &lan966x_ethtool_ops;
+	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
+	dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER |
+			 NETIF_F_HW_VLAN_CTAG_TX |
+			 NETIF_F_HW_VLAN_STAG_TX;
 	dev->needed_headroom = IFH_LEN * sizeof(u32);
 
 	eth_hw_addr_gen(dev, lan966x->base_mac, p + 1);
@@ -625,6 +651,10 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
 		return err;
 	}
 
+	lan966x_vlan_port_set_vlan_aware(port, 0);
+	lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
+	lan966x_vlan_port_apply(port);
+
 	return 0;
 }
 
@@ -635,6 +665,9 @@ static void lan966x_init(struct lan966x *lan966x)
 	/* MAC table initialization */
 	lan966x_mac_init(lan966x);
 
+	/* Vlan initialization */
+	lan966x_vlan_init(lan966x);
+
 	/* Flush queues */
 	lan_wr(lan_rd(lan966x, QS_XTR_FLUSH) |
 	       GENMASK(1, 0),
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 3d228c9c0521..6d0d922617ae 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -4,6 +4,7 @@
 #define __LAN966X_MAIN_H__
 
 #include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
 #include <linux/jiffies.h>
 #include <linux/phy.h>
 #include <linux/phylink.h>
@@ -22,7 +23,8 @@
 #define PGID_SRC			80
 #define PGID_ENTRIES			89
 
-#define PORT_PVID			0
+#define UNAWARE_PVID			0
+#define HOST_PVID			4095
 
 /* Reserved amount for (SRC, PRIO) at index 8*SRC + PRIO */
 #define QSYS_Q_RSRV			95
@@ -82,6 +84,9 @@ struct lan966x {
 	struct list_head mac_entries;
 	spinlock_t mac_lock; /* lock for mac_entries list */
 
+	u16 vlan_mask[VLAN_N_VID];
+	DECLARE_BITMAP(cpu_vlan_mask, VLAN_N_VID);
+
 	/* stats */
 	const struct lan966x_stat_layout *stats_layout;
 	u32 num_stats;
@@ -113,6 +118,8 @@ struct lan966x_port {
 
 	u8 chip_port;
 	u16 pvid;
+	u16 vid;
+	u8 vlan_aware;
 
 	struct phylink_config phylink_config;
 	struct phylink_pcs phylink_pcs;
@@ -168,6 +175,37 @@ irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x);
 
 void lan966x_ext_purge_entries(void);
 
+void lan966x_vlan_init(struct lan966x *lan966x);
+void lan966x_vlan_port_apply(struct lan966x_port *port);
+
+int lan966x_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid);
+int lan966x_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid);
+
+void lan966x_vlan_mode(struct lan966x_port *port, netdev_features_t features);
+u16 lan966x_vlan_port_get_pvid(struct lan966x_port *port);
+
+bool lan966x_vlan_cpu_member_cpu_vlan_mask(struct lan966x *lan966x, u16 vid);
+void lan966x_vlan_cpu_add_cpu_vlan_mask(struct lan966x *lan966x, u16 vid);
+bool lan966x_vlan_port_any_vlan_mask(struct lan966x *lan966x, u16 vid);
+
+void lan966x_vlan_cpu_set_vlan_aware(struct lan966x_port *port);
+void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
+				      bool vlan_aware);
+int lan966x_vlan_port_set_vid(struct lan966x_port *port, u16 vid,
+			      bool pvid, bool untagged);
+int lan966x_vlan_port_add_vlan(struct lan966x_port *port,
+			       u16 vid,
+			       bool pvid,
+			       bool untagged);
+int lan966x_vlan_port_del_vlan(struct lan966x_port *port,
+			       u16 vid);
+int lan966x_vlan_cpu_add_vlan(struct lan966x *lan966x,
+			      struct net_device *dev,
+			      u16 vid);
+int lan966x_vlan_cpu_del_vlan(struct lan966x *lan966x,
+			      struct net_device *dev,
+			      u16 vid);
+
 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 722ce7cb61b3..61f9e906cf80 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
@@ -82,6 +82,11 @@ static int lan966x_port_attr_set(struct net_device *dev, const void *ctx,
 	case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
 		lan966x_port_ageing_set(port, attr->u.ageing_time);
 		break;
+	case SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING:
+		lan966x_vlan_port_set_vlan_aware(port, attr->u.vlan_filtering);
+		lan966x_vlan_port_apply(port);
+		lan966x_vlan_cpu_set_vlan_aware(port);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
@@ -127,7 +132,12 @@ static void lan966x_port_bridge_leave(struct lan966x_port *port,
 	if (!lan966x->bridge_mask)
 		lan966x->bridge = NULL;
 
-	lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, PORT_PVID);
+	/* Set the port back to host mode */
+	lan966x_vlan_port_set_vlan_aware(port, false);
+	lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
+	lan966x_vlan_port_apply(port);
+
+	lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, HOST_PVID);
 }
 
 static int lan966x_port_changeupper(struct net_device *dev,
@@ -169,7 +179,7 @@ static int lan966x_port_add_addr(struct net_device *dev, bool up)
 	struct lan966x *lan966x = port->lan966x;
 	u16 vid;
 
-	vid = port->pvid;
+	vid = lan966x_vlan_port_get_pvid(port);
 
 	if (up)
 		lan966x_mac_cpu_learn(lan966x, dev->dev_addr, vid);
@@ -348,6 +358,95 @@ static int lan966x_switchdev_event(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+static int lan966x_handle_port_vlan_add(struct lan966x_port *port,
+					const struct switchdev_obj *obj)
+{
+	const struct switchdev_obj_port_vlan *v = SWITCHDEV_OBJ_PORT_VLAN(obj);
+	struct lan966x *lan966x = port->lan966x;
+
+	/* When adding a port to a vlan, we get a callback for the port but
+	 * also for the bridge. When get the callback for the bridge just bail
+	 * out. Then when the bridge is added to the vlan, then we get a
+	 * callback here but in this case the flags has set:
+	 * BRIDGE_VLAN_INFO_BRENTRY. In this case it means that the CPU
+	 * port is added to the vlan, so the broadcast frames and unicast frames
+	 * with dmac of the bridge should be foward to CPU.
+	 */
+	if (netif_is_bridge_master(obj->orig_dev) &&
+	    !(v->flags & BRIDGE_VLAN_INFO_BRENTRY))
+		return 0;
+
+	if (!netif_is_bridge_master(obj->orig_dev))
+		return lan966x_vlan_port_add_vlan(port, v->vid,
+						  v->flags & BRIDGE_VLAN_INFO_PVID,
+						  v->flags & BRIDGE_VLAN_INFO_UNTAGGED);
+
+	if (netif_is_bridge_master(obj->orig_dev))
+		return lan966x_vlan_cpu_add_vlan(lan966x, obj->orig_dev, v->vid);
+
+	return 0;
+}
+
+static int lan966x_handle_port_obj_add(struct net_device *dev, const void *ctx,
+				       const struct switchdev_obj *obj,
+				       struct netlink_ext_ack *extack)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+	int err;
+
+	if (ctx && ctx != port)
+		return 0;
+
+	switch (obj->id) {
+	case SWITCHDEV_OBJ_ID_PORT_VLAN:
+		err = lan966x_handle_port_vlan_add(port, obj);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	return err;
+}
+
+static int lan966x_handle_port_vlan_del(struct lan966x_port *port,
+					const struct switchdev_obj *obj)
+{
+	const struct switchdev_obj_port_vlan *v = SWITCHDEV_OBJ_PORT_VLAN(obj);
+	struct lan966x *lan966x = port->lan966x;
+
+	/* In case the physical port gets called */
+	if (!netif_is_bridge_master(obj->orig_dev))
+		return lan966x_vlan_port_del_vlan(port, v->vid);
+
+	/* In case the bridge gets called */
+	if (netif_is_bridge_master(obj->orig_dev))
+		return lan966x_vlan_cpu_del_vlan(lan966x, obj->orig_dev, v->vid);
+
+	return 0;
+}
+
+static int lan966x_handle_port_obj_del(struct net_device *dev, const void *ctx,
+				       const struct switchdev_obj *obj)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+	int err;
+
+	if (ctx && ctx != port)
+		return 0;
+
+	switch (obj->id) {
+	case SWITCHDEV_OBJ_ID_PORT_VLAN:
+		err = lan966x_handle_port_vlan_del(port, obj);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	return err;
+}
+
 static int lan966x_switchdev_blocking_event(struct notifier_block *nb,
 					    unsigned long event,
 					    void *ptr)
@@ -356,6 +455,16 @@ static int lan966x_switchdev_blocking_event(struct notifier_block *nb,
 	int err;
 
 	switch (event) {
+	case SWITCHDEV_PORT_OBJ_ADD:
+		err = switchdev_handle_port_obj_add(dev, ptr,
+						    lan966x_netdevice_check,
+						    lan966x_handle_port_obj_add);
+		return notifier_from_errno(err);
+	case SWITCHDEV_PORT_OBJ_DEL:
+		err = switchdev_handle_port_obj_del(dev, ptr,
+						    lan966x_netdevice_check,
+						    lan966x_handle_port_obj_del);
+		return notifier_from_errno(err);
 	case SWITCHDEV_PORT_ATTR_SET:
 		err = switchdev_handle_port_attr_set(dev, ptr,
 						     lan966x_netdevice_check,
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
new file mode 100644
index 000000000000..e8ff95bb65fa
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
@@ -0,0 +1,444 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include "lan966x_main.h"
+
+#define VLANACCESS_CMD_IDLE		0
+#define VLANACCESS_CMD_READ		1
+#define VLANACCESS_CMD_WRITE		2
+#define VLANACCESS_CMD_INIT		3
+
+static int lan966x_vlan_get_status(struct lan966x *lan966x)
+{
+	return lan_rd(lan966x, ANA_VLANACCESS);
+}
+
+static int lan966x_vlan_wait_for_completion(struct lan966x *lan966x)
+{
+	u32 val;
+
+	return readx_poll_timeout(lan966x_vlan_get_status,
+		lan966x, val,
+		(val & ANA_VLANACCESS_VLAN_TBL_CMD) ==
+		VLANACCESS_CMD_IDLE,
+		TABLE_UPDATE_SLEEP_US, TABLE_UPDATE_TIMEOUT_US);
+}
+
+static int lan966x_vlan_set_mask(struct lan966x *lan966x, u16 vid)
+{
+	u16 mask = lan966x->vlan_mask[vid];
+	bool cpu_dis;
+
+	cpu_dis = !(mask & BIT(CPU_PORT));
+
+	/* Set flags and the VID to configure */
+	lan_rmw(ANA_VLANTIDX_VLAN_PGID_CPU_DIS_SET(cpu_dis) |
+		ANA_VLANTIDX_V_INDEX_SET(vid),
+		ANA_VLANTIDX_VLAN_PGID_CPU_DIS |
+		ANA_VLANTIDX_V_INDEX,
+		lan966x, ANA_VLANTIDX);
+
+	/* Set the vlan port members mask */
+	lan_rmw(ANA_VLAN_PORT_MASK_VLAN_PORT_MASK_SET(mask),
+		ANA_VLAN_PORT_MASK_VLAN_PORT_MASK,
+		lan966x, ANA_VLAN_PORT_MASK);
+
+	/* Issue a write command */
+	lan_rmw(ANA_VLANACCESS_VLAN_TBL_CMD_SET(VLANACCESS_CMD_WRITE),
+		ANA_VLANACCESS_VLAN_TBL_CMD,
+		lan966x, ANA_VLANACCESS);
+
+	return lan966x_vlan_wait_for_completion(lan966x);
+}
+
+void lan966x_vlan_init(struct lan966x *lan966x)
+{
+	u16 port, vid;
+
+	/* Clear VLAN table, by default all ports are members of all VLANS */
+	lan_rmw(ANA_VLANACCESS_VLAN_TBL_CMD_SET(VLANACCESS_CMD_INIT),
+		ANA_VLANACCESS_VLAN_TBL_CMD,
+		lan966x, ANA_VLANACCESS);
+	lan966x_vlan_wait_for_completion(lan966x);
+
+	for (vid = 1; vid < VLAN_N_VID; vid++) {
+		lan966x->vlan_mask[vid] = 0;
+		lan966x_vlan_set_mask(lan966x, vid);
+	}
+
+	/* Set all the ports + cpu to be part of HOST_PVID and UNAWARE_PVID */
+	lan966x->vlan_mask[HOST_PVID] =
+		GENMASK(lan966x->num_phys_ports - 1, 0) | BIT(CPU_PORT);
+	lan966x_vlan_set_mask(lan966x, HOST_PVID);
+
+	lan966x->vlan_mask[UNAWARE_PVID] =
+		GENMASK(lan966x->num_phys_ports - 1, 0) | BIT(CPU_PORT);
+	lan966x_vlan_set_mask(lan966x, UNAWARE_PVID);
+
+	lan966x_vlan_cpu_add_cpu_vlan_mask(lan966x, UNAWARE_PVID);
+
+	/* Configure the CPU port to be vlan aware */
+	lan_wr(ANA_VLAN_CFG_VLAN_VID_SET(0) |
+	       ANA_VLAN_CFG_VLAN_AWARE_ENA_SET(1) |
+	       ANA_VLAN_CFG_VLAN_POP_CNT_SET(1),
+	       lan966x, ANA_VLAN_CFG(CPU_PORT));
+
+	/* Set vlan ingress filter mask to all ports */
+	lan_wr(GENMASK(lan966x->num_phys_ports, 0),
+	       lan966x, ANA_VLANMASK);
+
+	for (port = 0; port < lan966x->num_phys_ports; port++) {
+		lan_wr(0, lan966x, REW_PORT_VLAN_CFG(port));
+		lan_wr(0, lan966x, REW_TAG_CFG(port));
+	}
+}
+
+static int lan966x_vlan_port_add_vlan_mask(struct lan966x_port *port, u16 vid)
+{
+	struct lan966x *lan966x = port->lan966x;
+	u8 p = port->chip_port;
+
+	lan966x->vlan_mask[vid] |= BIT(p);
+	return lan966x_vlan_set_mask(lan966x, vid);
+}
+
+static int lan966x_vlan_port_del_vlan_mask(struct lan966x_port *port, u16 vid)
+{
+	struct lan966x *lan966x = port->lan966x;
+	u8 p = port->chip_port;
+
+	lan966x->vlan_mask[vid] &= ~BIT(p);
+	return lan966x_vlan_set_mask(lan966x, vid);
+}
+
+static bool lan966x_vlan_port_member_vlan_mask(struct lan966x_port *port, u16 vid)
+{
+	struct lan966x *lan966x = port->lan966x;
+	u8 p = port->chip_port;
+
+	return lan966x->vlan_mask[vid] & BIT(p);
+}
+
+bool lan966x_vlan_port_any_vlan_mask(struct lan966x *lan966x, u16 vid)
+{
+	return !!(lan966x->vlan_mask[vid] & ~BIT(CPU_PORT));
+}
+
+static int lan966x_vlan_cpu_add_vlan_mask(struct lan966x *lan966x, u16 vid)
+{
+	lan966x->vlan_mask[vid] |= BIT(CPU_PORT);
+	return lan966x_vlan_set_mask(lan966x, vid);
+}
+
+static int lan966x_vlan_cpu_del_vlan_mask(struct lan966x *lan966x, u16 vid)
+{
+	lan966x->vlan_mask[vid] &= ~BIT(CPU_PORT);
+	return lan966x_vlan_set_mask(lan966x, vid);
+}
+
+void lan966x_vlan_cpu_add_cpu_vlan_mask(struct lan966x *lan966x, u16 vid)
+{
+	set_bit(vid, lan966x->cpu_vlan_mask);
+}
+
+static void lan966x_vlan_cpu_del_cpu_vlan_mask(struct lan966x *lan966x, u16 vid)
+{
+	clear_bit(vid, lan966x->cpu_vlan_mask);
+}
+
+bool lan966x_vlan_cpu_member_cpu_vlan_mask(struct lan966x *lan966x, u16 vid)
+{
+	return test_bit(vid, lan966x->cpu_vlan_mask);
+}
+
+u16 lan966x_vlan_port_get_pvid(struct lan966x_port *port)
+{
+	struct lan966x *lan966x = port->lan966x;
+
+	if (!(lan966x->bridge_mask & BIT(port->chip_port)))
+		return HOST_PVID;
+
+	return port->vlan_aware ? port->pvid : UNAWARE_PVID;
+}
+
+int lan966x_vlan_port_set_vid(struct lan966x_port *port, u16 vid,
+			      bool pvid, bool untagged)
+{
+	struct lan966x *lan966x = port->lan966x;
+
+	/* Egress vlan classification */
+	if (untagged && port->vid != vid) {
+		if (port->vid) {
+			dev_err(lan966x->dev,
+				"Port already has a native VLAN: %d\n",
+				port->vid);
+			return -EBUSY;
+		}
+		port->vid = vid;
+	}
+
+	/* Default ingress vlan classification */
+	if (pvid)
+		port->pvid = vid;
+
+	return 0;
+}
+
+static int lan966x_vlan_port_remove_vid(struct lan966x_port *port, u16 vid)
+{
+	if (port->pvid == vid)
+		port->pvid = 0;
+
+	if (port->vid == vid)
+		port->vid = 0;
+
+	return 0;
+}
+
+void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
+				      bool vlan_aware)
+{
+	port->vlan_aware = vlan_aware;
+}
+
+void lan966x_vlan_cpu_set_vlan_aware(struct lan966x_port *port)
+{
+	struct lan966x *lan966x = port->lan966x;
+
+	if (!port->vlan_aware) {
+		/* In case of vlan unaware, all the ports will be set in
+		 * UNAWARE_PVID and have their PVID set to this PVID
+		 * The CPU doesn't need to be added because it is always part of
+		 * that vlan, it is required just to add entries in the MAC
+		 * table for the front port and the CPU
+		 */
+		lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, UNAWARE_PVID);
+		lan966x_mac_cpu_learn(lan966x, lan966x->bridge->dev_addr,
+				      UNAWARE_PVID);
+
+		lan966x_vlan_port_add_vlan_mask(port, UNAWARE_PVID);
+		lan966x_vlan_port_apply(port);
+	} else {
+		/* In case of vlan aware, just clear what happened when changed
+		 * to vlan unaware
+		 */
+		lan966x_mac_cpu_forget(lan966x, port->dev->dev_addr, UNAWARE_PVID);
+		lan966x_mac_cpu_forget(lan966x, lan966x->bridge->dev_addr,
+				       UNAWARE_PVID);
+
+		lan966x_vlan_port_del_vlan_mask(port, UNAWARE_PVID);
+		lan966x_vlan_port_apply(port);
+	}
+}
+
+void lan966x_vlan_port_apply(struct lan966x_port *port)
+{
+	struct lan966x *lan966x = port->lan966x;
+	u16 pvid;
+	u32 val;
+
+	pvid = lan966x_vlan_port_get_pvid(port);
+
+	/* Ingress clasification (ANA_PORT_VLAN_CFG) */
+	/* Default vlan to casify for untagged frames (may be zero) */
+	val = ANA_VLAN_CFG_VLAN_VID_SET(pvid);
+	if (port->vlan_aware)
+		val |= ANA_VLAN_CFG_VLAN_AWARE_ENA_SET(1) |
+		       ANA_VLAN_CFG_VLAN_POP_CNT_SET(1);
+
+	lan_rmw(val,
+		ANA_VLAN_CFG_VLAN_VID | ANA_VLAN_CFG_VLAN_AWARE_ENA |
+		ANA_VLAN_CFG_VLAN_POP_CNT,
+		lan966x, ANA_VLAN_CFG(port->chip_port));
+
+	/* Drop frames with multicast source address */
+	val = ANA_DROP_CFG_DROP_MC_SMAC_ENA_SET(1);
+	if (port->vlan_aware && !pvid)
+		/* If port is vlan-aware and tagged, drop untagged and priority
+		 * tagged frames.
+		 */
+		val |= ANA_DROP_CFG_DROP_UNTAGGED_ENA_SET(1) |
+		       ANA_DROP_CFG_DROP_PRIO_S_TAGGED_ENA_SET(1) |
+		       ANA_DROP_CFG_DROP_PRIO_C_TAGGED_ENA_SET(1);
+
+	lan_wr(val, lan966x, ANA_DROP_CFG(port->chip_port));
+
+	/* Egress configuration (REW_TAG_CFG): VLAN tag type to 8021Q */
+	val = REW_TAG_CFG_TAG_TPID_CFG_SET(0);
+	if (port->vlan_aware) {
+		if (port->vid)
+			/* Tag all frames except when VID == DEFAULT_VLAN */
+			val |= REW_TAG_CFG_TAG_CFG_SET(1);
+		else
+			val |= REW_TAG_CFG_TAG_CFG_SET(3);
+	}
+
+	/* Update only some bits in the register */
+	lan_rmw(val,
+		REW_TAG_CFG_TAG_TPID_CFG | REW_TAG_CFG_TAG_CFG,
+		lan966x, REW_TAG_CFG(port->chip_port));
+
+	/* Set default VLAN and tag type to 8021Q */
+	lan_rmw(REW_PORT_VLAN_CFG_PORT_TPID_SET(ETH_P_8021Q) |
+		REW_PORT_VLAN_CFG_PORT_VID_SET(port->vid),
+		REW_PORT_VLAN_CFG_PORT_TPID |
+		REW_PORT_VLAN_CFG_PORT_VID,
+		lan966x, REW_PORT_VLAN_CFG(port->chip_port));
+}
+
+int lan966x_vlan_port_add_vlan(struct lan966x_port *port,
+			       u16 vid,
+			       bool pvid,
+			       bool untagged)
+{
+	struct lan966x *lan966x = port->lan966x;
+
+	/* If the CPU(br) is already part of the vlan then add the MAC
+	 * address of the device in MAC table to copy the frames to the
+	 * CPU(br). If the CPU(br) is not part of the vlan then it would
+	 * just drop the frames.
+	 */
+	if (lan966x_vlan_cpu_member_cpu_vlan_mask(lan966x, vid)) {
+		lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, vid);
+		lan966x_mac_cpu_learn(lan966x, lan966x->bridge->dev_addr, vid);
+		lan966x_vlan_cpu_add_vlan_mask(lan966x, vid);
+	}
+
+	lan966x_vlan_port_set_vid(port, vid, pvid, untagged);
+	lan966x_vlan_port_add_vlan_mask(port, vid);
+	lan966x_vlan_port_apply(port);
+
+	return 0;
+}
+
+int lan966x_vlan_port_del_vlan(struct lan966x_port *port,
+			       u16 vid)
+{
+	struct lan966x *lan966x = port->lan966x;
+
+	/* In case the CPU(br) is part of the vlan then remove the MAC entry
+	 * because frame doesn't need to reach to CPU
+	 */
+	if (lan966x_vlan_cpu_member_cpu_vlan_mask(lan966x, vid))
+		lan966x_mac_cpu_forget(lan966x, port->dev->dev_addr, vid);
+
+	lan966x_vlan_port_remove_vid(port, vid);
+	lan966x_vlan_port_del_vlan_mask(port, vid);
+	lan966x_vlan_port_apply(port);
+
+	/* In case there are no other ports in vlan then remove the CPU from
+	 * that vlan but still keep it in the mask because it may be needed
+	 * again then another port gets added in tha vlan
+	 */
+	if (!lan966x_vlan_port_any_vlan_mask(lan966x, vid)) {
+		lan966x_mac_cpu_forget(lan966x, lan966x->bridge->dev_addr, vid);
+		lan966x_vlan_cpu_del_vlan_mask(lan966x, vid);
+	}
+
+	return 0;
+}
+
+int lan966x_vlan_cpu_add_vlan(struct lan966x *lan966x,
+			      struct net_device *dev,
+			      u16 vid)
+{
+	int p;
+
+	/* Iterate over the ports and see which ones are part of the
+	 * vlan and for those ports add entry in the MAC table to
+	 * copy the frames to the CPU
+	 */
+	for (p = 0; p < lan966x->num_phys_ports; p++) {
+		struct lan966x_port *port = lan966x->ports[p];
+
+		if (!port ||
+		    !lan966x_vlan_port_member_vlan_mask(port, vid))
+			continue;
+
+		lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, vid);
+	}
+
+	/* Add an entry in the MAC table for the CPU
+	 * Add the CPU part of the vlan only if there is another port in that
+	 * vlan otherwise all the broadcast frames in that vlan will go to CPU
+	 * even if none of the ports are in the vlan and then the CPU will just
+	 * need to discard these frames. It is required to store this
+	 * information so when a front port is added then it would add also the
+	 * CPU port.
+	 */
+	if (lan966x_vlan_port_any_vlan_mask(lan966x, vid)) {
+		lan966x_mac_cpu_learn(lan966x, dev->dev_addr, vid);
+		lan966x_vlan_cpu_add_vlan_mask(lan966x, vid);
+	}
+
+	lan966x_vlan_cpu_add_cpu_vlan_mask(lan966x, vid);
+
+	return 0;
+}
+
+int lan966x_vlan_cpu_del_vlan(struct lan966x *lan966x,
+			      struct net_device *dev,
+			      u16 vid)
+{
+	int p;
+
+	/* Iterate over the ports and see which ones are part of the
+	 * vlan and for those ports remove entry in the MAC table to
+	 * copy the frames to the CPU
+	 */
+	for (p = 0; p < lan966x->num_phys_ports; p++) {
+		struct lan966x_port *port = lan966x->ports[p];
+
+		if (!port ||
+		    !lan966x_vlan_port_member_vlan_mask(port, vid))
+			continue;
+
+		lan966x_mac_cpu_forget(lan966x, port->dev->dev_addr, vid);
+	}
+
+	/* Remove an entry in the MAC table for the CPU */
+	lan966x_mac_cpu_forget(lan966x, dev->dev_addr, vid);
+
+	/* Remove the CPU part of the vlan */
+	lan966x_vlan_cpu_del_cpu_vlan_mask(lan966x, vid);
+	lan966x_vlan_cpu_del_vlan_mask(lan966x, vid);
+
+	return 0;
+}
+
+int lan966x_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+
+	lan966x_vlan_port_set_vid(port, vid, false, false);
+	lan966x_vlan_port_add_vlan_mask(port, vid);
+	lan966x_vlan_port_apply(port);
+
+	return 0;
+}
+
+int lan966x_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
+			     u16 vid)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+
+	lan966x_vlan_port_remove_vid(port, vid);
+	lan966x_vlan_port_del_vlan_mask(port, vid);
+	lan966x_vlan_port_apply(port);
+
+	return 0;
+}
+
+void lan966x_vlan_mode(struct lan966x_port *port,
+		       netdev_features_t features)
+{
+	struct lan966x *lan966x = port->lan966x;
+	u32 val;
+
+	/* Filtering */
+	val = lan_rd(lan966x, ANA_VLANMASK);
+	if (features & NETIF_F_HW_VLAN_CTAG_FILTER)
+		val |= BIT(port->chip_port);
+	else
+		val &= ~BIT(port->chip_port);
+	lan_wr(val, lan966x, ANA_VLANMASK);
+}
-- 
2.33.0

[PATCH net-next v5 8/9] net: lan966x: Extend switchdev bridge flags

[Horatiu Vultur]

Currently allow a port to be part or not of the multicast flooding mask.
By implementing the switchdev calls SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS
and SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS.

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

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
index 61f9e906cf80..560486267695 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
@@ -18,6 +18,34 @@ struct lan966x_ext_entry {
 	struct lan966x *lan966x;
 };
 
+static void lan966x_port_bridge_flags(struct lan966x_port *port,
+				      struct switchdev_brport_flags flags)
+{
+	u32 val = lan_rd(port->lan966x, ANA_PGID(PGID_MC));
+
+	val = ANA_PGID_PGID_GET(val);
+
+	if (flags.mask & BR_MCAST_FLOOD) {
+		if (flags.val & BR_MCAST_FLOOD)
+			val |= BIT(port->chip_port);
+		else
+			val &= ~BIT(port->chip_port);
+	}
+
+	lan_rmw(ANA_PGID_PGID_SET(val),
+		ANA_PGID_PGID,
+		port->lan966x, ANA_PGID(PGID_MC));
+}
+
+static int lan966x_port_pre_bridge_flags(struct lan966x_port *port,
+					 struct switchdev_brport_flags flags)
+{
+	if (flags.mask & ~BR_MCAST_FLOOD)
+		return -EINVAL;
+
+	return 0;
+}
+
 static void lan966x_update_fwd_mask(struct lan966x *lan966x)
 {
 	int i;
@@ -76,6 +104,12 @@ static int lan966x_port_attr_set(struct net_device *dev, const void *ctx,
 		return 0;
 
 	switch (attr->id) {
+	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
+		lan966x_port_bridge_flags(port, attr->u.brport_flags);
+		break;
+	case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS:
+		err = lan966x_port_pre_bridge_flags(port, attr->u.brport_flags);
+		break;
 	case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
 		lan966x_port_stp_state_set(port, attr->u.stp_state);
 		break;
-- 
2.33.0

[PATCH net-next v5 9/9] net: lan966x: Extend switchdev with fdb support

[Horatiu Vultur]

Extend lan966x driver with fdb support by implementing the switchdev
calls SWITCHDEV_FDB_ADD_TO_DEVICE and SWITCHDEV_FDB_DEL_TO_DEVICE.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../net/ethernet/microchip/lan966x/Makefile   |   2 +-
 .../ethernet/microchip/lan966x/lan966x_fdb.c  | 246 ++++++++++++++++++
 .../ethernet/microchip/lan966x/lan966x_main.c |   8 +
 .../ethernet/microchip/lan966x/lan966x_main.h |  14 +
 .../microchip/lan966x/lan966x_switchdev.c     |  21 ++
 .../ethernet/microchip/lan966x/lan966x_vlan.c |   4 +
 6 files changed, 294 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c

diff --git a/drivers/net/ethernet/microchip/lan966x/Makefile b/drivers/net/ethernet/microchip/lan966x/Makefile
index d82e896c2e53..ec1a1fa8b0d5 100644
--- a/drivers/net/ethernet/microchip/lan966x/Makefile
+++ b/drivers/net/ethernet/microchip/lan966x/Makefile
@@ -7,4 +7,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_vlan.o lan966x_fdb.o
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c
new file mode 100644
index 000000000000..660dae324e32
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c
@@ -0,0 +1,246 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <net/switchdev.h>
+
+#include "lan966x_main.h"
+
+struct lan966x_fdb_event_work {
+	struct work_struct work;
+	struct switchdev_notifier_fdb_info fdb_info;
+	struct net_device *dev;
+	struct lan966x *lan966x;
+	unsigned long event;
+};
+
+struct lan966x_fdb_entry {
+	struct list_head list;
+	unsigned char mac[ETH_ALEN] __aligned(2);
+	u16 vid;
+	u32 references;
+};
+
+static struct lan966x_fdb_entry *
+lan966x_fdb_find_entry(struct lan966x *lan966x,
+		       struct switchdev_notifier_fdb_info *fdb_info)
+{
+	struct lan966x_fdb_entry *fdb_entry;
+
+	list_for_each_entry(fdb_entry, &lan966x->fdb_entries, list) {
+		if (fdb_entry->vid == fdb_info->vid &&
+		    ether_addr_equal(fdb_entry->mac, fdb_info->addr))
+			return fdb_entry;
+	}
+
+	return NULL;
+}
+
+static void lan966x_fdb_add_entry(struct lan966x *lan966x,
+				  struct switchdev_notifier_fdb_info *fdb_info)
+{
+	struct lan966x_fdb_entry *fdb_entry;
+
+	fdb_entry = lan966x_fdb_find_entry(lan966x, fdb_info);
+	if (fdb_entry) {
+		fdb_entry->references++;
+		return;
+	}
+
+	fdb_entry = kzalloc(sizeof(*fdb_entry), GFP_KERNEL);
+	if (!fdb_entry)
+		return;
+
+	memcpy(fdb_entry->mac, fdb_info->addr, ETH_ALEN);
+	fdb_entry->vid = fdb_info->vid;
+	fdb_entry->references = 1;
+	list_add_tail(&fdb_entry->list, &lan966x->fdb_entries);
+}
+
+static bool lan966x_fdb_del_entry(struct lan966x *lan966x,
+				  struct switchdev_notifier_fdb_info *fdb_info)
+{
+	struct lan966x_fdb_entry *fdb_entry, *tmp;
+
+	list_for_each_entry_safe(fdb_entry, tmp, &lan966x->fdb_entries,
+				 list) {
+		if (fdb_entry->vid == fdb_info->vid &&
+		    ether_addr_equal(fdb_entry->mac, fdb_info->addr)) {
+			fdb_entry->references--;
+			if (!fdb_entry->references) {
+				list_del(&fdb_entry->list);
+				kfree(fdb_entry);
+				return true;
+			}
+			break;
+		}
+	}
+
+	return false;
+}
+
+void lan966x_fdb_write_entries(struct lan966x *lan966x, u16 vid)
+{
+	struct lan966x_fdb_entry *fdb_entry;
+
+	list_for_each_entry(fdb_entry, &lan966x->fdb_entries, list) {
+		if (fdb_entry->vid != vid)
+			continue;
+
+		lan966x_mac_cpu_learn(lan966x, fdb_entry->mac, fdb_entry->vid);
+	}
+}
+
+void lan966x_fdb_erase_entries(struct lan966x *lan966x, u16 vid)
+{
+	struct lan966x_fdb_entry *fdb_entry;
+
+	list_for_each_entry(fdb_entry, &lan966x->fdb_entries, list) {
+		if (fdb_entry->vid != vid)
+			continue;
+
+		lan966x_mac_cpu_forget(lan966x, fdb_entry->mac, fdb_entry->vid);
+	}
+}
+
+static void lan966x_fdb_purge_entries(struct lan966x *lan966x)
+{
+	struct lan966x_fdb_entry *fdb_entry, *tmp;
+
+	list_for_each_entry_safe(fdb_entry, tmp, &lan966x->fdb_entries, list) {
+		list_del(&fdb_entry->list);
+		kfree(fdb_entry);
+	}
+}
+
+int lan966x_fdb_init(struct lan966x *lan966x)
+{
+	INIT_LIST_HEAD(&lan966x->fdb_entries);
+	lan966x->fdb_work = alloc_ordered_workqueue("lan966x_order", 0);
+	if (!lan966x->fdb_work)
+		return -ENOMEM;
+
+	return 0;
+}
+
+void lan966x_fdb_deinit(struct lan966x *lan966x)
+{
+	destroy_workqueue(lan966x->fdb_work);
+	lan966x_fdb_purge_entries(lan966x);
+}
+
+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);
+	struct switchdev_notifier_fdb_info *fdb_info;
+	struct net_device *dev = fdb_work->dev;
+	struct lan966x_port *port;
+	struct lan966x *lan966x;
+
+	fdb_info = &fdb_work->fdb_info;
+	lan966x = fdb_work->lan966x;
+
+	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);
+			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;
+		}
+	} else {
+		if (!netif_is_bridge_master(dev))
+			goto out;
+
+		/* If the CPU is not part of the vlan then there is no point
+		 * to copy the frames to the CPU because they will be dropped
+		 */
+		if (!lan966x_vlan_cpu_member_cpu_vlan_mask(lan966x,
+							   fdb_info->vid))
+			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_port_any_vlan_mask(lan966x,
+							     fdb_info->vid))
+				break;
+
+			lan966x_mac_cpu_learn(lan966x, fdb_info->addr,
+					      fdb_info->vid);
+			break;
+		case SWITCHDEV_FDB_DEL_TO_DEVICE:
+			/* It is OK to always forget the entry */
+			if (lan966x_fdb_del_entry(lan966x, fdb_info))
+				lan966x_mac_cpu_forget(lan966x, fdb_info->addr,
+						       fdb_info->vid);
+			break;
+		}
+	}
+
+out:
+	kfree(fdb_work->fdb_info.addr);
+	kfree(fdb_work);
+	dev_put(dev);
+}
+
+int lan966x_handle_fdb(struct net_device *dev,
+		       struct net_device *orig_dev,
+		       unsigned long event, const void *ctx,
+		       const struct switchdev_notifier_fdb_info *fdb_info)
+{
+	struct lan966x_port *port = netdev_priv(dev);
+	struct lan966x *lan966x = port->lan966x;
+	struct lan966x_fdb_event_work *fdb_work;
+
+	if (ctx && ctx != port)
+		return 0;
+
+	switch (event) {
+	case SWITCHDEV_FDB_ADD_TO_DEVICE:
+	case SWITCHDEV_FDB_DEL_TO_DEVICE:
+		if (lan966x_netdevice_check(orig_dev) &&
+		    !fdb_info->added_by_user)
+			break;
+
+		fdb_work = kzalloc(sizeof(*fdb_work), GFP_ATOMIC);
+		if (!fdb_work)
+			return -ENOMEM;
+
+		fdb_work->dev = orig_dev;
+		fdb_work->lan966x = lan966x;
+		fdb_work->event = event;
+		INIT_WORK(&fdb_work->work, lan966x_fdb_event_work);
+		memcpy(&fdb_work->fdb_info, fdb_info, sizeof(fdb_work->fdb_info));
+		fdb_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
+		if (!fdb_work->fdb_info.addr)
+			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;
+	}
+
+	return 0;
+err_addr_alloc:
+	kfree(fdb_work);
+	return -ENOMEM;
+}
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 881c1678f3e9..4f2d3e99e702 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -937,8 +937,15 @@ static int lan966x_probe(struct platform_device *pdev)
 
 	lan966x_register_notifier_blocks(lan966x);
 
+	err = lan966x_fdb_init(lan966x);
+	if (err)
+		goto unregister_notifier_blocks;
+
 	return 0;
 
+unregister_notifier_blocks:
+	lan966x_unregister_notifier_blocks(lan966x);
+
 cleanup_ports:
 	fwnode_handle_put(portnp);
 
@@ -965,6 +972,7 @@ static int lan966x_remove(struct platform_device *pdev)
 
 	lan966x_mac_purge_entries(lan966x);
 	lan966x_ext_purge_entries();
+	lan966x_fdb_deinit(lan966x);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 6d0d922617ae..91b2cf069b4b 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -8,6 +8,7 @@
 #include <linux/jiffies.h>
 #include <linux/phy.h>
 #include <linux/phylink.h>
+#include <net/switchdev.h>
 
 #include "lan966x_regs.h"
 #include "lan966x_ifh.h"
@@ -100,6 +101,10 @@ struct lan966x {
 	/* interrupts */
 	int xtr_irq;
 	int ana_irq;
+
+	/* worqueue for fdb */
+	struct workqueue_struct *fdb_work;
+	struct list_head fdb_entries;
 };
 
 struct lan966x_port_config {
@@ -206,6 +211,15 @@ int lan966x_vlan_cpu_del_vlan(struct lan966x *lan966x,
 			      struct net_device *dev,
 			      u16 vid);
 
+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);
+int lan966x_handle_fdb(struct net_device *dev,
+		       struct net_device *orig_dev,
+		       unsigned long event, const void *ctx,
+		       const struct switchdev_notifier_fdb_info *fdb_info);
+
 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 560486267695..2afcf454ede5 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
@@ -375,6 +375,19 @@ static int lan966x_netdevice_event(struct notifier_block *nb,
 	return notifier_from_errno(ret);
 }
 
+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;
+
+	if (netif_is_bridge_master(foreign_dev))
+		if (lan966x->bridge != foreign_dev)
+			return true;
+
+	return false;
+}
+
 static int lan966x_switchdev_event(struct notifier_block *nb,
 				   unsigned long event, void *ptr)
 {
@@ -387,6 +400,14 @@ static int lan966x_switchdev_event(struct notifier_block *nb,
 						     lan966x_netdevice_check,
 						     lan966x_port_attr_set);
 		return notifier_from_errno(err);
+	case SWITCHDEV_FDB_ADD_TO_DEVICE:
+	case SWITCHDEV_FDB_DEL_TO_DEVICE:
+		err = switchdev_handle_fdb_event_to_device(dev, event, ptr,
+							   lan966x_netdevice_check,
+							   lan966x_foreign_dev_check,
+							   lan966x_handle_fdb,
+							   NULL);
+		return notifier_from_errno(err);
 	}
 
 	return NOTIFY_DONE;
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
index e8ff95bb65fa..7c4ceb8c9386 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
@@ -301,6 +301,7 @@ int lan966x_vlan_port_add_vlan(struct lan966x_port *port,
 		lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, vid);
 		lan966x_mac_cpu_learn(lan966x, lan966x->bridge->dev_addr, vid);
 		lan966x_vlan_cpu_add_vlan_mask(lan966x, vid);
+		lan966x_fdb_write_entries(lan966x, vid);
 	}
 
 	lan966x_vlan_port_set_vid(port, vid, pvid, untagged);
@@ -332,6 +333,7 @@ int lan966x_vlan_port_del_vlan(struct lan966x_port *port,
 	if (!lan966x_vlan_port_any_vlan_mask(lan966x, vid)) {
 		lan966x_mac_cpu_forget(lan966x, lan966x->bridge->dev_addr, vid);
 		lan966x_vlan_cpu_del_vlan_mask(lan966x, vid);
+		lan966x_fdb_erase_entries(lan966x, vid);
 	}
 
 	return 0;
@@ -371,6 +373,7 @@ int lan966x_vlan_cpu_add_vlan(struct lan966x *lan966x,
 	}
 
 	lan966x_vlan_cpu_add_cpu_vlan_mask(lan966x, vid);
+	lan966x_fdb_write_entries(lan966x, vid);
 
 	return 0;
 }
@@ -401,6 +404,7 @@ int lan966x_vlan_cpu_del_vlan(struct lan966x *lan966x,
 	/* Remove the CPU part of the vlan */
 	lan966x_vlan_cpu_del_cpu_vlan_mask(lan966x, vid);
 	lan966x_vlan_cpu_del_vlan_mask(lan966x, vid);
+	lan966x_fdb_erase_entries(lan966x, vid);
 
 	return 0;
 }
-- 
2.33.0

Re: [PATCH net-next v5 6/9] net: lan966x: Add support to offload the forwarding.

[Vladimir Oltean]

On Wed, Dec 15, 2021 at 01:13:06PM +0100, Horatiu Vultur wrote:
> This patch adds basic support to offload in the HW the forwarding of the
> frames. The driver registers to the switchdev callbacks and implements
> the callbacks for attributes SWITCHDEV_ATTR_ID_PORT_STP_STATE and
> SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME.
> It is not allowed to add a lan966x port to a bridge that contains a
> different interface than lan966x.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../net/ethernet/microchip/lan966x/Kconfig    |   1 +
>  .../net/ethernet/microchip/lan966x/Makefile   |   2 +-
>  .../ethernet/microchip/lan966x/lan966x_main.c |  16 +-
>  .../ethernet/microchip/lan966x/lan966x_main.h |  11 +
>  .../microchip/lan966x/lan966x_switchdev.c     | 393 ++++++++++++++++++
>  5 files changed, 419 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/Kconfig b/drivers/net/ethernet/microchip/lan966x/Kconfig
> index 2860a8c9923d..ac273f84b69e 100644
> --- a/drivers/net/ethernet/microchip/lan966x/Kconfig
> +++ b/drivers/net/ethernet/microchip/lan966x/Kconfig
> @@ -2,6 +2,7 @@ config LAN966X_SWITCH
>  	tristate "Lan966x switch driver"
>  	depends on HAS_IOMEM
>  	depends on OF
> +	depends on NET_SWITCHDEV
>  	select PHYLINK
>  	select PACKING
>  	help
> diff --git a/drivers/net/ethernet/microchip/lan966x/Makefile b/drivers/net/ethernet/microchip/lan966x/Makefile
> index 2989ba528236..974229c51f55 100644
> --- a/drivers/net/ethernet/microchip/lan966x/Makefile
> +++ b/drivers/net/ethernet/microchip/lan966x/Makefile
> @@ -6,4 +6,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_mac.o lan966x_ethtool.o lan966x_switchdev.o
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index dc40ac2eb246..ee453967da71 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -355,6 +355,11 @@ static const struct net_device_ops lan966x_port_netdev_ops = {
>  	.ndo_get_port_parent_id		= lan966x_port_get_parent_id,
>  };
>  
> +bool lan966x_netdevice_check(const struct net_device *dev)
> +{
> +	return dev->netdev_ops == &lan966x_port_netdev_ops;
> +}
> +
>  static int lan966x_port_xtr_status(struct lan966x *lan966x, u8 grp)
>  {
>  	return lan_rd(lan966x, QS_XTR_RD(grp));
> @@ -491,6 +496,9 @@ static irqreturn_t lan966x_xtr_irq_handler(int irq, void *args)
>  
>  		skb->protocol = eth_type_trans(skb, dev);
>  
> +		if (lan966x->bridge_mask & BIT(src_port))
> +			skb->offload_fwd_mark = 1;
> +
>  		netif_rx_ni(skb);
>  		dev->stats.rx_bytes += len;
>  		dev->stats.rx_packets++;
> @@ -578,9 +586,6 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
>  
>  	eth_hw_addr_gen(dev, lan966x->base_mac, p + 1);
>  
> -	lan966x_mac_learn(lan966x, PGID_CPU, dev->dev_addr, port->pvid,
> -			  ENTRYTYPE_LOCKED);
> -
>  	port->phylink_config.dev = &port->dev->dev;
>  	port->phylink_config.type = PHYLINK_NETDEV;
>  	port->phylink_pcs.poll = true;
> @@ -897,6 +902,8 @@ static int lan966x_probe(struct platform_device *pdev)
>  		lan966x_port_init(lan966x->ports[p]);
>  	}
>  
> +	lan966x_register_notifier_blocks(lan966x);

To be clear, "singleton" would mean that irrespective of the number of
driver instances, this function would be called once. So calling it from
lan966x_probe() isn't exactly a good choice, since every instance of the
driver "probes".

int dsa_slave_register_notifier(void)
{
	struct notifier_block *nb;
	int err;

	err = register_netdevice_notifier(&dsa_slave_nb);
	if (err)
		return err;

	err = register_switchdev_notifier(&dsa_slave_switchdev_notifier);
	if (err)
		goto err_switchdev_nb;

	nb = &dsa_slave_switchdev_blocking_notifier;
	err = register_switchdev_blocking_notifier(nb);
	if (err)
		goto err_switchdev_blocking_nb;
}

static int __init dsa_init_module(void)
{
	rc = dsa_slave_register_notifier();
}
module_init(dsa_init_module);

> +
>  	return 0;
>  
>  cleanup_ports:
> @@ -915,6 +922,8 @@ static int lan966x_remove(struct platform_device *pdev)
>  {
>  	struct lan966x *lan966x = platform_get_drvdata(pdev);
>  
> +	lan966x_unregister_notifier_blocks(lan966x);
> +
>  	lan966x_cleanup_ports(lan966x);
>  
>  	cancel_delayed_work_sync(&lan966x->stats_work);
> @@ -922,6 +931,7 @@ static int lan966x_remove(struct platform_device *pdev)
>  	mutex_destroy(&lan966x->stats_lock);
>  
>  	lan966x_mac_purge_entries(lan966x);
> +	lan966x_ext_purge_entries();

Broken with multiple lan966x driver instances - you'd erase all other
drivers' tabs keps on bridges in the system as soon as one single switch
is unbound from its driver.

>  
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> index fcd5d09a070c..3d228c9c0521 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> @@ -75,6 +75,10 @@ struct lan966x {
>  
>  	u8 base_mac[ETH_ALEN];
>  
> +	struct net_device *bridge;
> +	u16 bridge_mask;
> +	u16 bridge_fwd_mask;
> +
>  	struct list_head mac_entries;
>  	spinlock_t mac_lock; /* lock for mac_entries list */
>  
> @@ -122,6 +126,11 @@ 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;
>  
> +bool lan966x_netdevice_check(const struct net_device *dev);
> +
> +void lan966x_register_notifier_blocks(struct lan966x *lan966x);
> +void lan966x_unregister_notifier_blocks(struct lan966x *lan966x);
> +
>  void lan966x_stats_get(struct net_device *dev,
>  		       struct rtnl_link_stats64 *stats);
>  int lan966x_stats_init(struct lan966x *lan966x);
> @@ -157,6 +166,8 @@ int lan966x_mac_add_entry(struct lan966x *lan966x,
>  void lan966x_mac_purge_entries(struct lan966x *lan966x);
>  irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x);
>  
> +void lan966x_ext_purge_entries(void);
> +
>  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
> new file mode 100644
> index 000000000000..722ce7cb61b3
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> @@ -0,0 +1,393 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/if_bridge.h>
> +#include <net/switchdev.h>
> +
> +#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 LIST_HEAD(ext_entries);
> +
> +struct lan966x_ext_entry {
> +	struct list_head list;
> +	struct net_device *dev;
> +	u32 ports;
> +	struct lan966x *lan966x;
> +};
> +
> +static void lan966x_update_fwd_mask(struct lan966x *lan966x)
> +{
> +	int i;
> +
> +	for (i = 0; i < lan966x->num_phys_ports; i++) {
> +		struct lan966x_port *port = lan966x->ports[i];
> +		unsigned long mask = 0;
> +
> +		if (port && lan966x->bridge_fwd_mask & BIT(i))
> +			mask = lan966x->bridge_fwd_mask & ~BIT(i);
> +
> +		mask |= BIT(CPU_PORT);
> +
> +		lan_wr(ANA_PGID_PGID_SET(mask),
> +		       lan966x, ANA_PGID(PGID_SRC + i));
> +	}

I vaguely remember this was implemented better in previous versions of
the patch set, and the restriction to not allow multiple bridges
spanning the same switch wasn't there. Why do you keep disallowing
multiple bridges for all the Microchip hardware? There are very real use
cases that need them.

> +}
> +
> +static void lan966x_port_stp_state_set(struct lan966x_port *port, u8 state)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +	bool learn_ena = false;
> +
> +	if (state == BR_STATE_FORWARDING || state == BR_STATE_LEARNING)
> +		learn_ena = true;
> +
> +	if (state == BR_STATE_FORWARDING)
> +		lan966x->bridge_fwd_mask |= BIT(port->chip_port);
> +	else
> +		lan966x->bridge_fwd_mask &= ~BIT(port->chip_port);
> +
> +	lan_rmw(ANA_PORT_CFG_LEARN_ENA_SET(learn_ena),
> +		ANA_PORT_CFG_LEARN_ENA,
> +		lan966x, ANA_PORT_CFG(port->chip_port));
> +
> +	lan966x_update_fwd_mask(lan966x);
> +}
> +
> +static 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;
> +
> +	lan966x_mac_set_ageing(port->lan966x, ageing_time);
> +}
> +
> +static int lan966x_port_attr_set(struct net_device *dev, const void *ctx,
> +				 const struct switchdev_attr *attr,
> +				 struct netlink_ext_ack *extack)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +	int err = 0;
> +
> +	if (ctx && ctx != port)
> +		return 0;
> +
> +	switch (attr->id) {
> +	case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
> +		lan966x_port_stp_state_set(port, attr->u.stp_state);
> +		break;
> +	case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
> +		lan966x_port_ageing_set(port, attr->u.ageing_time);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	return err;
> +}
> +
> +static int lan966x_port_bridge_join(struct lan966x_port *port,
> +				    struct net_device *bridge,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +	struct net_device *dev = port->dev;
> +	int err;
> +
> +	if (!lan966x->bridge_mask) {
> +		lan966x->bridge = bridge;
> +	} else {
> +		if (lan966x->bridge != bridge)

NL_SET_ERR_MSG_MOD(extack, "<excuse>");

> +			return -ENODEV;
> +	}
> +
> +	err = switchdev_bridge_port_offload(dev, dev, port,
> +					    &lan966x_switchdev_nb,
> +					    &lan966x_switchdev_blocking_nb,
> +					    false, extack);
> +	if (err)
> +		return err;
> +
> +	lan966x->bridge_mask |= BIT(port->chip_port);
> +
> +	return 0;
> +}
> +
> +static void lan966x_port_bridge_leave(struct lan966x_port *port,
> +				      struct net_device *bridge)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +
> +	lan966x->bridge_mask &= ~BIT(port->chip_port);
> +
> +	if (!lan966x->bridge_mask)
> +		lan966x->bridge = NULL;
> +
> +	lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, PORT_PVID);
> +}
> +
> +static int lan966x_port_changeupper(struct net_device *dev,
> +				    struct netdev_notifier_changeupper_info *info)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +	struct netlink_ext_ack *extack;
> +	int err = 0;
> +
> +	extack = netdev_notifier_info_to_extack(&info->info);
> +
> +	if (netif_is_bridge_master(info->upper_dev)) {
> +		if (info->linking)
> +			err = lan966x_port_bridge_join(port, info->upper_dev,
> +						       extack);
> +		else
> +			lan966x_port_bridge_leave(port, info->upper_dev);
> +	}
> +
> +	return err;
> +}
> +
> +static int lan966x_port_prechangeupper(struct net_device *dev,
> +				       struct netdev_notifier_changeupper_info *info)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +
> +	if (netif_is_bridge_master(info->upper_dev) && !info->linking)
> +		switchdev_bridge_port_unoffload(port->dev, port,
> +						&lan966x_switchdev_nb,
> +						&lan966x_switchdev_blocking_nb);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int lan966x_port_add_addr(struct net_device *dev, bool up)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +	struct lan966x *lan966x = port->lan966x;
> +	u16 vid;
> +
> +	vid = port->pvid;
> +
> +	if (up)
> +		lan966x_mac_cpu_learn(lan966x, dev->dev_addr, vid);
> +	else
> +		lan966x_mac_cpu_forget(lan966x, dev->dev_addr, vid);
> +
> +	return 0;
> +}
> +
> +static struct lan966x_ext_entry *lan966x_ext_find_entry(struct net_device *dev)
> +{
> +	struct lan966x_ext_entry *ext_entry;
> +
> +	list_for_each_entry(ext_entry, &ext_entries, list) {
> +		if (ext_entry->dev == dev)
> +			return ext_entry;
> +	}
> +
> +	return NULL;
> +}
> +
> +static bool lan966x_ext_add_entry(struct net_device *dev, void *lan966x)
> +{
> +	struct lan966x_ext_entry *ext_entry;
> +
> +	ext_entry = lan966x_ext_find_entry(dev);
> +	if (ext_entry) {
> +		if (ext_entry->lan966x)
> +			return false;
> +
> +		ext_entry->ports++;
> +		return true;
> +	}
> +
> +	ext_entry = kzalloc(sizeof(*ext_entry), GFP_KERNEL);
> +	if (!ext_entry)
> +		return false;
> +
> +	ext_entry->dev = dev;
> +	ext_entry->ports = 1;
> +	ext_entry->lan966x = lan966x;
> +	list_add_tail(&ext_entry->list, &ext_entries);
> +	return true;
> +}
> +
> +static void lan966x_ext_remove_entry(struct net_device *dev)
> +{
> +	struct lan966x_ext_entry *ext_entry;
> +
> +	ext_entry = lan966x_ext_find_entry(dev);
> +	if (!ext_entry)
> +		return;
> +
> +	ext_entry->ports--;
> +	if (!ext_entry->ports) {
> +		list_del(&ext_entry->list);
> +		kfree(ext_entry);
> +	}
> +}
> +
> +void lan966x_ext_purge_entries(void)
> +{
> +	struct lan966x_ext_entry *ext_entry, *tmp;
> +
> +	list_for_each_entry_safe(ext_entry, tmp, &ext_entries, list) {
> +		list_del(&ext_entry->list);
> +		kfree(ext_entry);
> +	}
> +}
> +
> +static int lan966x_ext_check_entry(struct net_device *dev,
> +				   unsigned long event,
> +				   void *ptr)
> +{
> +	struct netdev_notifier_changeupper_info *info;
> +
> +	if (event != NETDEV_PRECHANGEUPPER)
> +		return 0;
> +
> +	info = ptr;
> +	if (!netif_is_bridge_master(info->upper_dev))
> +		return 0;
> +
> +	if (info->linking) {
> +		if (!lan966x_ext_add_entry(info->upper_dev, NULL))
> +			return -EOPNOTSUPP;
> +	} else {
> +		lan966x_ext_remove_entry(info->upper_dev);
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static bool lan966x_port_ext_check_entry(struct net_device *dev,
> +					 struct netdev_notifier_changeupper_info *info)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +	struct lan966x *lan966x = port->lan966x;
> +	struct lan966x_ext_entry *entry;
> +
> +	if (!netif_is_bridge_master(info->upper_dev))
> +		return true;
> +
> +	entry = lan966x_ext_find_entry(info->upper_dev);

"entry" is unused in the "else" block below, so logically speaking it
could be moved inside the "if" block.

Anyway, this piece of code is objectively speaking very obscure: convoluted
(lan966x_port_ext_check_entry calls lan966x_ext_find_entry _twice_, once
here and once in lan966x_ext_add_entry ?!), no comments and poorly named
(a lan966x_ext_entry represents a _bridge_ ?! what does "ext_entry"
stand for?). Plus, with your design where the "ext_entries" list is
global, and there are two instances of the driver, each driver would do
this work twice and allocate memory twice. Although, I didn't really
understand why you need to allocate memory to keep a tab on every bridge
in the system in the first place.

If you move your check from NETDEV_PRECHANGEUPPER to NETDEV_CHANGEUPPER,
you allow the upper/lower adjacency list relationship to have formed
(allowing the use of netdev_for_each_lower_dev, and the newly joining
interface will be a lower of the bridge). But you can still reject the
bridge join.

So you can do something like this, and it should produce an equivalent
effect (not compiled, not tested, written straight in the email body):

static int lan966x_foreign_bridging_check(struct net_device *bridge,
					  struct netlink_ext_ack *extack)
{
	struct lan966x *lan966x = NULL;
	bool has_foreign = false;
	struct net_device *dev;
	struct list_head *iter;

	netdev_for_each_lower_dev(bridge, dev, iter) {
		if (lan966x_netdevice_check(dev)) {
			struct lan966x_port *port = netdev_priv(dev);

			if (lan996x) {
				/* Bridge already has at least one port
				 * of a lan966x switch inside it, check
				 * that it's the same instance of the
				 * driver.
				 */
				if (port->lan966x != lan996x) {
					NL_SET_ERR_MSG_MOD(extack, "Bridging between multiple lan966x switches disallowed");
					return -EINVAL;
				}
			} else {
				/* This is the first lan966x port inside
				 * this bridge
				 */
				lan966x = port->lan966x;
			}
		} else {
			has_foreign = true;
		}

		if (lan966x && has_foreign) {
			NL_SET_ERR_MSG_MOD(extack, "Bridging lan966x ports with foreign interfaces disallowed");
			return -EINVAL;
		}
	}

	return 0;
}

and call this from two distinct call paths: from the NETDEV_CHANGEUPPER
of foreign interfaces, and from the NETDEV_CHANGEUPPER of lan966x interfaces.

Is it just me, or does this look more obvious and straightforward?

> +	if (info->linking) {
> +		if (!entry)
> +			return lan966x_ext_add_entry(info->upper_dev, lan966x);
> +
> +		if (entry->lan966x == lan966x) {
> +			entry->ports++;
> +			return true;
> +		}
> +	} else {
> +		lan966x_ext_remove_entry(info->upper_dev);
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int lan966x_netdevice_port_event(struct net_device *dev,
> +					struct notifier_block *nb,
> +					unsigned long event, void *ptr)
> +{
> +	int err = 0;
> +
> +	if (!lan966x_netdevice_check(dev))
> +		return lan966x_ext_check_entry(dev, event, ptr);
> +
> +	switch (event) {
> +	case NETDEV_PRECHANGEUPPER:
> +		if (!lan966x_port_ext_check_entry(dev, ptr))
> +			return -EOPNOTSUPP;
> +
> +		err = lan966x_port_prechangeupper(dev, ptr);
> +		break;
> +	case NETDEV_CHANGEUPPER:
> +		err = lan966x_port_changeupper(dev, ptr);
> +		break;
> +	case NETDEV_PRE_UP:
> +		err = lan966x_port_add_addr(dev, true);
> +		break;
> +	case NETDEV_DOWN:
> +		err = lan966x_port_add_addr(dev, false);

Any reason why you track your own NETDEV_PRE_UP/NETDEV_DOWN and don't do
this directly in ->ndo_open/->ndo_close? Also, I don't think that the
"lan966x_port_add_addr" brings much value over "lan966x_mac_cpu_learn"
and "lan966x_mac_cpu_forget" called directly (especially if moved to
lan966x_port_open and lan966x_port_stop). And I don't see the relevance
of this change with respect to the commit title "add support to offload
the forwarding". CPU learned entries are for termination.

> +		break;
> +	}
> +
> +	return err;
> +}
> +
> +static int lan966x_netdevice_event(struct notifier_block *nb,
> +				   unsigned long event, void *ptr)
> +{
> +	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> +	int ret;
> +
> +	ret = lan966x_netdevice_port_event(dev, nb, event, ptr);
> +
> +	return notifier_from_errno(ret);
> +}
> +
> +static int lan966x_switchdev_event(struct notifier_block *nb,
> +				   unsigned long event, void *ptr)
> +{
> +	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
> +	int err;
> +
> +	switch (event) {
> +	case SWITCHDEV_PORT_ATTR_SET:
> +		err = switchdev_handle_port_attr_set(dev, ptr,
> +						     lan966x_netdevice_check,
> +						     lan966x_port_attr_set);
> +		return notifier_from_errno(err);
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int lan966x_switchdev_blocking_event(struct notifier_block *nb,
> +					    unsigned long event,
> +					    void *ptr)
> +{
> +	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
> +	int err;
> +
> +	switch (event) {
> +	case SWITCHDEV_PORT_ATTR_SET:
> +		err = switchdev_handle_port_attr_set(dev, ptr,
> +						     lan966x_netdevice_check,
> +						     lan966x_port_attr_set);
> +		return notifier_from_errno(err);
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block lan966x_netdevice_nb __read_mostly = {
> +	.notifier_call = lan966x_netdevice_event,
> +};
> +
> +static struct notifier_block lan966x_switchdev_nb __read_mostly = {
> +	.notifier_call = lan966x_switchdev_event,
> +};
> +
> +static struct notifier_block lan966x_switchdev_blocking_nb __read_mostly = {
> +	.notifier_call = lan966x_switchdev_blocking_event,
> +};
> +
> +void lan966x_register_notifier_blocks(struct lan966x *lan966x)
> +{
> +	register_netdevice_notifier(&lan966x_netdevice_nb);
> +	register_switchdev_notifier(&lan966x_switchdev_nb);
> +	register_switchdev_blocking_notifier(&lan966x_switchdev_blocking_nb);
> +}
> +
> +void lan966x_unregister_notifier_blocks(struct lan966x *lan966x)
> +{
> +	unregister_switchdev_blocking_notifier(&lan966x_switchdev_blocking_nb);
> +	unregister_switchdev_notifier(&lan966x_switchdev_nb);
> +	unregister_netdevice_notifier(&lan966x_netdevice_nb);
> +}
> -- 
> 2.33.0
>

Re: [PATCH net-next v5 7/9] net: lan966x: Add vlan support.

[Vladimir Oltean]

On Wed, Dec 15, 2021 at 01:13:07PM +0100, Horatiu Vultur wrote:
> Extend the driver to support vlan filtering  by implementing the
> switchdev calls SWITCHDEV_OBJ_ID_PORT_VLAN and
> SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING.

And the VLAN RX filtering net device ops.

> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../net/ethernet/microchip/lan966x/Makefile   |   3 +-
>  .../ethernet/microchip/lan966x/lan966x_main.c |  39 +-
>  .../ethernet/microchip/lan966x/lan966x_main.h |  40 +-
>  .../microchip/lan966x/lan966x_switchdev.c     | 113 ++++-
>  .../ethernet/microchip/lan966x/lan966x_vlan.c | 444 ++++++++++++++++++
>  5 files changed, 632 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/Makefile b/drivers/net/ethernet/microchip/lan966x/Makefile
> index 974229c51f55..d82e896c2e53 100644
> --- a/drivers/net/ethernet/microchip/lan966x/Makefile
> +++ b/drivers/net/ethernet/microchip/lan966x/Makefile
> @@ -6,4 +6,5 @@
>  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_mac.o lan966x_ethtool.o lan966x_switchdev.o \
> +			lan966x_vlan.o
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index ee453967da71..881c1678f3e9 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -103,17 +103,18 @@ static int lan966x_create_targets(struct platform_device *pdev,
>  static int lan966x_port_set_mac_address(struct net_device *dev, void *p)
>  {
>  	struct lan966x_port *port = netdev_priv(dev);
> +	u16 pvid = lan966x_vlan_port_get_pvid(port);
>  	struct lan966x *lan966x = port->lan966x;
>  	const struct sockaddr *addr = p;
>  	int ret;
>  
>  	/* Learn the new net device MAC address in the mac table. */
> -	ret = lan966x_mac_cpu_learn(lan966x, addr->sa_data, port->pvid);
> +	ret = lan966x_mac_cpu_learn(lan966x, addr->sa_data, pvid);

Logically speaking, there is a divide of responsibility. The bridge
emits switchdev FDB events for local MAC addresses, with a VID of 0
(corresponding to VLAN-unaware bridging) as well as for each installed
VLAN. Bridge VLAN 0 is equivalent to your UNAWARE_PVID macro. And the
driver is solely responsible for the MAC address in the HOST_PVID VLAN.
When the ndo_set_mac_address is called, you should just update the entry
learned in the HOST_PVID. The bridge will get an NETDEV_CHANGEADDR event
and update its local MAC addresses too, in the VLANs it handles.
Otherwise, if you just learn in the pvid that the port is currently in,
then RX filtering will be broken if you change your MAC address while
you're under a bridge, then you leave that bridge and become standalone.
So you need to re-learn the dev_addr in lan966x_port_bridge_leave, which
makes the implementation a bit more complicated than it needs to be
(unless I'm missing something about CPU-learned MAC addresses in VLANs
that aren't currently active, you seem to be avoiding that even though
it makes the driver keep a lot more state).

>  	if (ret)
>  		return ret;
>  
>  	/* Then forget the previous one. */
> -	ret = lan966x_mac_cpu_forget(lan966x, dev->dev_addr, port->pvid);
> +	ret = lan966x_mac_cpu_forget(lan966x, dev->dev_addr, pvid);
>  	if (ret)
>  		return ret;
>  
> @@ -283,6 +284,12 @@ static void lan966x_ifh_set_ipv(void *ifh, u64 bypass)
>  		IFH_POS_IPV, IFH_LEN * 4, PACK, 0);
>  }
>  
> +static void lan966x_ifh_set_vid(void *ifh, u64 vid)
> +{
> +	packing(ifh, &vid, IFH_POS_TCI + IFH_WID_TCI - 1,
> +		IFH_POS_TCI, IFH_LEN * 4, PACK, 0);
> +}
> +
>  static int lan966x_port_xmit(struct sk_buff *skb, struct net_device *dev)
>  {
>  	struct lan966x_port *port = netdev_priv(dev);
> @@ -294,6 +301,7 @@ static int lan966x_port_xmit(struct sk_buff *skb, struct net_device *dev)
>  	lan966x_ifh_set_port(ifh, BIT_ULL(port->chip_port));
>  	lan966x_ifh_set_qos_class(ifh, skb->priority >= 7 ? 0x7 : skb->priority);
>  	lan966x_ifh_set_ipv(ifh, skb->priority >= 7 ? 0x7 : skb->priority);
> +	lan966x_ifh_set_vid(ifh, skb_vlan_tag_get(skb));
>  
>  	return lan966x_port_ifh_xmit(skb, ifh, dev);
>  }
> @@ -343,6 +351,18 @@ static int lan966x_port_get_parent_id(struct net_device *dev,
>  	return 0;
>  }
>  
> +static int lan966x_port_set_features(struct net_device *dev,
> +				     netdev_features_t features)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +	netdev_features_t changed = dev->features ^ features;
> +
> +	if (changed & NETIF_F_HW_VLAN_CTAG_FILTER)
> +		lan966x_vlan_mode(port, features);
> +
> +	return 0;
> +}
> +
>  static const struct net_device_ops lan966x_port_netdev_ops = {
>  	.ndo_open			= lan966x_port_open,
>  	.ndo_stop			= lan966x_port_stop,
> @@ -353,6 +373,9 @@ static const struct net_device_ops lan966x_port_netdev_ops = {
>  	.ndo_get_stats64		= lan966x_stats_get,
>  	.ndo_set_mac_address		= lan966x_port_set_mac_address,
>  	.ndo_get_port_parent_id		= lan966x_port_get_parent_id,
> +	.ndo_set_features		= lan966x_port_set_features,
> +	.ndo_vlan_rx_add_vid		= lan966x_vlan_rx_add_vid,
> +	.ndo_vlan_rx_kill_vid		= lan966x_vlan_rx_kill_vid,

Do you have any particular use case for NETIF_F_HW_VLAN_CTAG_FILTER on
non-bridged ports? I find the fact that you implement these very strange
and likely bogus: you set port->vlan_aware = false when a port leaves a
bridge, yet you install VLANs to its RX filter as if those VLANs were to
actually match on any VLAN-tagged packet... which they won't because
lan966x_vlan_port_apply() clears ANA_VLAN_CFG_VLAN_AWARE_ENA_SET(1) when
port->vlan_aware isn't set. So you end up being "filtering" but not "aware"
- all packets get classified to the same VLAN, which isn't dropped.

>  };
>  
>  bool lan966x_netdevice_check(const struct net_device *dev)
> @@ -575,13 +598,16 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
>  	port->dev = dev;
>  	port->lan966x = lan966x;
>  	port->chip_port = p;
> -	port->pvid = PORT_PVID;
>  	lan966x->ports[p] = port;
>  
>  	dev->max_mtu = ETH_MAX_MTU;
>  
>  	dev->netdev_ops = &lan966x_port_netdev_ops;
>  	dev->ethtool_ops = &lan966x_ethtool_ops;
> +	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> +	dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER |
> +			 NETIF_F_HW_VLAN_CTAG_TX |
> +			 NETIF_F_HW_VLAN_STAG_TX;
>  	dev->needed_headroom = IFH_LEN * sizeof(u32);
>  
>  	eth_hw_addr_gen(dev, lan966x->base_mac, p + 1);
> @@ -625,6 +651,10 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
>  		return err;
>  	}
>  
> +	lan966x_vlan_port_set_vlan_aware(port, 0);
> +	lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
> +	lan966x_vlan_port_apply(port);
> +
>  	return 0;
>  }
>  
> @@ -635,6 +665,9 @@ static void lan966x_init(struct lan966x *lan966x)
>  	/* MAC table initialization */
>  	lan966x_mac_init(lan966x);
>  
> +	/* Vlan initialization */
> +	lan966x_vlan_init(lan966x);

Curious how the lan966x_ext_entry stuff doesn't have any comment and
lan966x_vlan_init has such a trivial one?!

> +
>  	/* Flush queues */
>  	lan_wr(lan_rd(lan966x, QS_XTR_FLUSH) |
>  	       GENMASK(1, 0),
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> index 3d228c9c0521..6d0d922617ae 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> @@ -4,6 +4,7 @@
>  #define __LAN966X_MAIN_H__
>  
>  #include <linux/etherdevice.h>
> +#include <linux/if_vlan.h>
>  #include <linux/jiffies.h>
>  #include <linux/phy.h>
>  #include <linux/phylink.h>
> @@ -22,7 +23,8 @@
>  #define PGID_SRC			80
>  #define PGID_ENTRIES			89
>  
> -#define PORT_PVID			0
> +#define UNAWARE_PVID			0
> +#define HOST_PVID			4095
>  
>  /* Reserved amount for (SRC, PRIO) at index 8*SRC + PRIO */
>  #define QSYS_Q_RSRV			95
> @@ -82,6 +84,9 @@ struct lan966x {
>  	struct list_head mac_entries;
>  	spinlock_t mac_lock; /* lock for mac_entries list */
>  
> +	u16 vlan_mask[VLAN_N_VID];
> +	DECLARE_BITMAP(cpu_vlan_mask, VLAN_N_VID);
> +
>  	/* stats */
>  	const struct lan966x_stat_layout *stats_layout;
>  	u32 num_stats;
> @@ -113,6 +118,8 @@ struct lan966x_port {
>  
>  	u8 chip_port;
>  	u16 pvid;
> +	u16 vid;
> +	u8 vlan_aware;

bool

>  
>  	struct phylink_config phylink_config;
>  	struct phylink_pcs phylink_pcs;
> @@ -168,6 +175,37 @@ irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x);
>  
>  void lan966x_ext_purge_entries(void);
>  
> +void lan966x_vlan_init(struct lan966x *lan966x);
> +void lan966x_vlan_port_apply(struct lan966x_port *port);
> +
> +int lan966x_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid);
> +int lan966x_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid);
> +
> +void lan966x_vlan_mode(struct lan966x_port *port, netdev_features_t features);
> +u16 lan966x_vlan_port_get_pvid(struct lan966x_port *port);
> +
> +bool lan966x_vlan_cpu_member_cpu_vlan_mask(struct lan966x *lan966x, u16 vid);
> +void lan966x_vlan_cpu_add_cpu_vlan_mask(struct lan966x *lan966x, u16 vid);
> +bool lan966x_vlan_port_any_vlan_mask(struct lan966x *lan966x, u16 vid);
> +
> +void lan966x_vlan_cpu_set_vlan_aware(struct lan966x_port *port);
> +void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
> +				      bool vlan_aware);
> +int lan966x_vlan_port_set_vid(struct lan966x_port *port, u16 vid,
> +			      bool pvid, bool untagged);
> +int lan966x_vlan_port_add_vlan(struct lan966x_port *port,
> +			       u16 vid,
> +			       bool pvid,
> +			       bool untagged);
> +int lan966x_vlan_port_del_vlan(struct lan966x_port *port,
> +			       u16 vid);
> +int lan966x_vlan_cpu_add_vlan(struct lan966x *lan966x,
> +			      struct net_device *dev,
> +			      u16 vid);
> +int lan966x_vlan_cpu_del_vlan(struct lan966x *lan966x,
> +			      struct net_device *dev,
> +			      u16 vid);
> +
>  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 722ce7cb61b3..61f9e906cf80 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> @@ -82,6 +82,11 @@ static int lan966x_port_attr_set(struct net_device *dev, const void *ctx,
>  	case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
>  		lan966x_port_ageing_set(port, attr->u.ageing_time);
>  		break;
> +	case SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING:
> +		lan966x_vlan_port_set_vlan_aware(port, attr->u.vlan_filtering);
> +		lan966x_vlan_port_apply(port);
> +		lan966x_vlan_cpu_set_vlan_aware(port);
> +		break;
>  	default:
>  		err = -EOPNOTSUPP;
>  		break;
> @@ -127,7 +132,12 @@ static void lan966x_port_bridge_leave(struct lan966x_port *port,
>  	if (!lan966x->bridge_mask)
>  		lan966x->bridge = NULL;
>  
> -	lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, PORT_PVID);
> +	/* Set the port back to host mode */
> +	lan966x_vlan_port_set_vlan_aware(port, false);
> +	lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
> +	lan966x_vlan_port_apply(port);
> +
> +	lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, HOST_PVID);
>  }
>  
>  static int lan966x_port_changeupper(struct net_device *dev,
> @@ -169,7 +179,7 @@ static int lan966x_port_add_addr(struct net_device *dev, bool up)
>  	struct lan966x *lan966x = port->lan966x;
>  	u16 vid;
>  
> -	vid = port->pvid;
> +	vid = lan966x_vlan_port_get_pvid(port);
>  
>  	if (up)
>  		lan966x_mac_cpu_learn(lan966x, dev->dev_addr, vid);
> @@ -348,6 +358,95 @@ static int lan966x_switchdev_event(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> +static int lan966x_handle_port_vlan_add(struct lan966x_port *port,
> +					const struct switchdev_obj *obj)
> +{
> +	const struct switchdev_obj_port_vlan *v = SWITCHDEV_OBJ_PORT_VLAN(obj);
> +	struct lan966x *lan966x = port->lan966x;
> +
> +	/* When adding a port to a vlan, we get a callback for the port but
> +	 * also for the bridge. When get the callback for the bridge just bail
> +	 * out. Then when the bridge is added to the vlan, then we get a
> +	 * callback here but in this case the flags has set:
> +	 * BRIDGE_VLAN_INFO_BRENTRY. In this case it means that the CPU
> +	 * port is added to the vlan, so the broadcast frames and unicast frames
> +	 * with dmac of the bridge should be foward to CPU.
> +	 */
> +	if (netif_is_bridge_master(obj->orig_dev) &&
> +	    !(v->flags & BRIDGE_VLAN_INFO_BRENTRY))
> +		return 0;
> +
> +	if (!netif_is_bridge_master(obj->orig_dev))
> +		return lan966x_vlan_port_add_vlan(port, v->vid,
> +						  v->flags & BRIDGE_VLAN_INFO_PVID,
> +						  v->flags & BRIDGE_VLAN_INFO_UNTAGGED);
> +
> +	if (netif_is_bridge_master(obj->orig_dev))

"else" will suffice.

> +		return lan966x_vlan_cpu_add_vlan(lan966x, obj->orig_dev, v->vid);
> +
> +	return 0;
> +}
> +
> +static int lan966x_handle_port_obj_add(struct net_device *dev, const void *ctx,
> +				       const struct switchdev_obj *obj,
> +				       struct netlink_ext_ack *extack)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +	int err;
> +
> +	if (ctx && ctx != port)
> +		return 0;
> +
> +	switch (obj->id) {
> +	case SWITCHDEV_OBJ_ID_PORT_VLAN:
> +		err = lan966x_handle_port_vlan_add(port, obj);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	return err;
> +}
> +
> +static int lan966x_handle_port_vlan_del(struct lan966x_port *port,
> +					const struct switchdev_obj *obj)
> +{
> +	const struct switchdev_obj_port_vlan *v = SWITCHDEV_OBJ_PORT_VLAN(obj);
> +	struct lan966x *lan966x = port->lan966x;
> +
> +	/* In case the physical port gets called */
> +	if (!netif_is_bridge_master(obj->orig_dev))
> +		return lan966x_vlan_port_del_vlan(port, v->vid);
> +
> +	/* In case the bridge gets called */
> +	if (netif_is_bridge_master(obj->orig_dev))

likewise.

> +		return lan966x_vlan_cpu_del_vlan(lan966x, obj->orig_dev, v->vid);
> +
> +	return 0;
> +}
> +
> +static int lan966x_handle_port_obj_del(struct net_device *dev, const void *ctx,
> +				       const struct switchdev_obj *obj)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +	int err;
> +
> +	if (ctx && ctx != port)
> +		return 0;
> +
> +	switch (obj->id) {
> +	case SWITCHDEV_OBJ_ID_PORT_VLAN:
> +		err = lan966x_handle_port_vlan_del(port, obj);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	return err;
> +}
> +
>  static int lan966x_switchdev_blocking_event(struct notifier_block *nb,
>  					    unsigned long event,
>  					    void *ptr)
> @@ -356,6 +455,16 @@ static int lan966x_switchdev_blocking_event(struct notifier_block *nb,
>  	int err;
>  
>  	switch (event) {
> +	case SWITCHDEV_PORT_OBJ_ADD:
> +		err = switchdev_handle_port_obj_add(dev, ptr,
> +						    lan966x_netdevice_check,
> +						    lan966x_handle_port_obj_add);
> +		return notifier_from_errno(err);
> +	case SWITCHDEV_PORT_OBJ_DEL:
> +		err = switchdev_handle_port_obj_del(dev, ptr,
> +						    lan966x_netdevice_check,
> +						    lan966x_handle_port_obj_del);
> +		return notifier_from_errno(err);
>  	case SWITCHDEV_PORT_ATTR_SET:
>  		err = switchdev_handle_port_attr_set(dev, ptr,
>  						     lan966x_netdevice_check,
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> new file mode 100644
> index 000000000000..e8ff95bb65fa
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> @@ -0,0 +1,444 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include "lan966x_main.h"
> +
> +#define VLANACCESS_CMD_IDLE		0
> +#define VLANACCESS_CMD_READ		1
> +#define VLANACCESS_CMD_WRITE		2
> +#define VLANACCESS_CMD_INIT		3
> +
> +static int lan966x_vlan_get_status(struct lan966x *lan966x)
> +{
> +	return lan_rd(lan966x, ANA_VLANACCESS);
> +}
> +
> +static int lan966x_vlan_wait_for_completion(struct lan966x *lan966x)
> +{
> +	u32 val;
> +
> +	return readx_poll_timeout(lan966x_vlan_get_status,
> +		lan966x, val,
> +		(val & ANA_VLANACCESS_VLAN_TBL_CMD) ==
> +		VLANACCESS_CMD_IDLE,
> +		TABLE_UPDATE_SLEEP_US, TABLE_UPDATE_TIMEOUT_US);
> +}
> +
> +static int lan966x_vlan_set_mask(struct lan966x *lan966x, u16 vid)
> +{
> +	u16 mask = lan966x->vlan_mask[vid];
> +	bool cpu_dis;
> +
> +	cpu_dis = !(mask & BIT(CPU_PORT));
> +
> +	/* Set flags and the VID to configure */
> +	lan_rmw(ANA_VLANTIDX_VLAN_PGID_CPU_DIS_SET(cpu_dis) |
> +		ANA_VLANTIDX_V_INDEX_SET(vid),
> +		ANA_VLANTIDX_VLAN_PGID_CPU_DIS |
> +		ANA_VLANTIDX_V_INDEX,
> +		lan966x, ANA_VLANTIDX);
> +
> +	/* Set the vlan port members mask */
> +	lan_rmw(ANA_VLAN_PORT_MASK_VLAN_PORT_MASK_SET(mask),
> +		ANA_VLAN_PORT_MASK_VLAN_PORT_MASK,
> +		lan966x, ANA_VLAN_PORT_MASK);
> +
> +	/* Issue a write command */
> +	lan_rmw(ANA_VLANACCESS_VLAN_TBL_CMD_SET(VLANACCESS_CMD_WRITE),
> +		ANA_VLANACCESS_VLAN_TBL_CMD,
> +		lan966x, ANA_VLANACCESS);
> +
> +	return lan966x_vlan_wait_for_completion(lan966x);

If you're not going to propagate the return code anywhere, at least
return void and print an error here. Otherwise it's totally silent.

> +}
> +
> +void lan966x_vlan_init(struct lan966x *lan966x)
> +{
> +	u16 port, vid;
> +
> +	/* Clear VLAN table, by default all ports are members of all VLANS */
> +	lan_rmw(ANA_VLANACCESS_VLAN_TBL_CMD_SET(VLANACCESS_CMD_INIT),
> +		ANA_VLANACCESS_VLAN_TBL_CMD,
> +		lan966x, ANA_VLANACCESS);
> +	lan966x_vlan_wait_for_completion(lan966x);

Again no error checking.

> +
> +	for (vid = 1; vid < VLAN_N_VID; vid++) {
> +		lan966x->vlan_mask[vid] = 0;
> +		lan966x_vlan_set_mask(lan966x, vid);
> +	}
> +
> +	/* Set all the ports + cpu to be part of HOST_PVID and UNAWARE_PVID */
> +	lan966x->vlan_mask[HOST_PVID] =
> +		GENMASK(lan966x->num_phys_ports - 1, 0) | BIT(CPU_PORT);
> +	lan966x_vlan_set_mask(lan966x, HOST_PVID);
> +
> +	lan966x->vlan_mask[UNAWARE_PVID] =
> +		GENMASK(lan966x->num_phys_ports - 1, 0) | BIT(CPU_PORT);
> +	lan966x_vlan_set_mask(lan966x, UNAWARE_PVID);
> +
> +	lan966x_vlan_cpu_add_cpu_vlan_mask(lan966x, UNAWARE_PVID);
> +
> +	/* Configure the CPU port to be vlan aware */
> +	lan_wr(ANA_VLAN_CFG_VLAN_VID_SET(0) |
> +	       ANA_VLAN_CFG_VLAN_AWARE_ENA_SET(1) |
> +	       ANA_VLAN_CFG_VLAN_POP_CNT_SET(1),
> +	       lan966x, ANA_VLAN_CFG(CPU_PORT));
> +
> +	/* Set vlan ingress filter mask to all ports */
> +	lan_wr(GENMASK(lan966x->num_phys_ports, 0),
> +	       lan966x, ANA_VLANMASK);
> +
> +	for (port = 0; port < lan966x->num_phys_ports; port++) {
> +		lan_wr(0, lan966x, REW_PORT_VLAN_CFG(port));
> +		lan_wr(0, lan966x, REW_TAG_CFG(port));
> +	}
> +}
> +
> +static int lan966x_vlan_port_add_vlan_mask(struct lan966x_port *port, u16 vid)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +	u8 p = port->chip_port;
> +
> +	lan966x->vlan_mask[vid] |= BIT(p);
> +	return lan966x_vlan_set_mask(lan966x, vid);
> +}
> +
> +static int lan966x_vlan_port_del_vlan_mask(struct lan966x_port *port, u16 vid)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +	u8 p = port->chip_port;
> +
> +	lan966x->vlan_mask[vid] &= ~BIT(p);
> +	return lan966x_vlan_set_mask(lan966x, vid);
> +}
> +
> +static bool lan966x_vlan_port_member_vlan_mask(struct lan966x_port *port, u16 vid)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +	u8 p = port->chip_port;
> +
> +	return lan966x->vlan_mask[vid] & BIT(p);
> +}
> +
> +bool lan966x_vlan_port_any_vlan_mask(struct lan966x *lan966x, u16 vid)
> +{
> +	return !!(lan966x->vlan_mask[vid] & ~BIT(CPU_PORT));
> +}
> +
> +static int lan966x_vlan_cpu_add_vlan_mask(struct lan966x *lan966x, u16 vid)
> +{
> +	lan966x->vlan_mask[vid] |= BIT(CPU_PORT);
> +	return lan966x_vlan_set_mask(lan966x, vid);
> +}
> +
> +static int lan966x_vlan_cpu_del_vlan_mask(struct lan966x *lan966x, u16 vid)
> +{
> +	lan966x->vlan_mask[vid] &= ~BIT(CPU_PORT);
> +	return lan966x_vlan_set_mask(lan966x, vid);
> +}
> +
> +void lan966x_vlan_cpu_add_cpu_vlan_mask(struct lan966x *lan966x, u16 vid)
> +{
> +	set_bit(vid, lan966x->cpu_vlan_mask);

Since these are all serialized by the rtnl_mutex, I think it's safe to
replace with __set_bit which is non-atomic and thus cheaper.

> +}
> +
> +static void lan966x_vlan_cpu_del_cpu_vlan_mask(struct lan966x *lan966x, u16 vid)
> +{
> +	clear_bit(vid, lan966x->cpu_vlan_mask);
> +}
> +
> +bool lan966x_vlan_cpu_member_cpu_vlan_mask(struct lan966x *lan966x, u16 vid)
> +{
> +	return test_bit(vid, lan966x->cpu_vlan_mask);
> +}
> +
> +u16 lan966x_vlan_port_get_pvid(struct lan966x_port *port)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +
> +	if (!(lan966x->bridge_mask & BIT(port->chip_port)))
> +		return HOST_PVID;
> +
> +	return port->vlan_aware ? port->pvid : UNAWARE_PVID;
> +}
> +
> +int lan966x_vlan_port_set_vid(struct lan966x_port *port, u16 vid,
> +			      bool pvid, bool untagged)

If you were to summarize what this function does, what would that be?

> +{
> +	struct lan966x *lan966x = port->lan966x;
> +
> +	/* Egress vlan classification */
> +	if (untagged && port->vid != vid) {
> +		if (port->vid) {
> +			dev_err(lan966x->dev,
> +				"Port already has a native VLAN: %d\n",
> +				port->vid);
> +			return -EBUSY;
> +		}
> +		port->vid = vid;
> +	}
> +
> +	/* Default ingress vlan classification */
> +	if (pvid)
> +		port->pvid = vid;
> +
> +	return 0;
> +}
> +
> +static int lan966x_vlan_port_remove_vid(struct lan966x_port *port, u16 vid)
> +{
> +	if (port->pvid == vid)
> +		port->pvid = 0;
> +
> +	if (port->vid == vid)
> +		port->vid = 0;
> +
> +	return 0;
> +}
> +
> +void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
> +				      bool vlan_aware)
> +{
> +	port->vlan_aware = vlan_aware;
> +}
> +
> +void lan966x_vlan_cpu_set_vlan_aware(struct lan966x_port *port)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +
> +	if (!port->vlan_aware) {
> +		/* In case of vlan unaware, all the ports will be set in
> +		 * UNAWARE_PVID and have their PVID set to this PVID
> +		 * The CPU doesn't need to be added because it is always part of
> +		 * that vlan, it is required just to add entries in the MAC
> +		 * table for the front port and the CPU
> +		 */
> +		lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, UNAWARE_PVID);
> +		lan966x_mac_cpu_learn(lan966x, lan966x->bridge->dev_addr,
> +				      UNAWARE_PVID);
> +
> +		lan966x_vlan_port_add_vlan_mask(port, UNAWARE_PVID);
> +		lan966x_vlan_port_apply(port);
> +	} else {
> +		/* In case of vlan aware, just clear what happened when changed
> +		 * to vlan unaware
> +		 */
> +		lan966x_mac_cpu_forget(lan966x, port->dev->dev_addr, UNAWARE_PVID);
> +		lan966x_mac_cpu_forget(lan966x, lan966x->bridge->dev_addr,
> +				       UNAWARE_PVID);
> +
> +		lan966x_vlan_port_del_vlan_mask(port, UNAWARE_PVID);
> +		lan966x_vlan_port_apply(port);
> +	}
> +}
> +
> +void lan966x_vlan_port_apply(struct lan966x_port *port)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +	u16 pvid;
> +	u32 val;
> +
> +	pvid = lan966x_vlan_port_get_pvid(port);
> +
> +	/* Ingress clasification (ANA_PORT_VLAN_CFG) */
> +	/* Default vlan to casify for untagged frames (may be zero) */

classify

> +	val = ANA_VLAN_CFG_VLAN_VID_SET(pvid);
> +	if (port->vlan_aware)
> +		val |= ANA_VLAN_CFG_VLAN_AWARE_ENA_SET(1) |
> +		       ANA_VLAN_CFG_VLAN_POP_CNT_SET(1);
> +
> +	lan_rmw(val,
> +		ANA_VLAN_CFG_VLAN_VID | ANA_VLAN_CFG_VLAN_AWARE_ENA |
> +		ANA_VLAN_CFG_VLAN_POP_CNT,
> +		lan966x, ANA_VLAN_CFG(port->chip_port));
> +
> +	/* Drop frames with multicast source address */
> +	val = ANA_DROP_CFG_DROP_MC_SMAC_ENA_SET(1);
> +	if (port->vlan_aware && !pvid)
> +		/* If port is vlan-aware and tagged, drop untagged and priority
> +		 * tagged frames.
> +		 */
> +		val |= ANA_DROP_CFG_DROP_UNTAGGED_ENA_SET(1) |
> +		       ANA_DROP_CFG_DROP_PRIO_S_TAGGED_ENA_SET(1) |
> +		       ANA_DROP_CFG_DROP_PRIO_C_TAGGED_ENA_SET(1);
> +
> +	lan_wr(val, lan966x, ANA_DROP_CFG(port->chip_port));
> +
> +	/* Egress configuration (REW_TAG_CFG): VLAN tag type to 8021Q */
> +	val = REW_TAG_CFG_TAG_TPID_CFG_SET(0);
> +	if (port->vlan_aware) {
> +		if (port->vid)
> +			/* Tag all frames except when VID == DEFAULT_VLAN */
> +			val |= REW_TAG_CFG_TAG_CFG_SET(1);
> +		else
> +			val |= REW_TAG_CFG_TAG_CFG_SET(3);
> +	}
> +
> +	/* Update only some bits in the register */
> +	lan_rmw(val,
> +		REW_TAG_CFG_TAG_TPID_CFG | REW_TAG_CFG_TAG_CFG,
> +		lan966x, REW_TAG_CFG(port->chip_port));
> +
> +	/* Set default VLAN and tag type to 8021Q */
> +	lan_rmw(REW_PORT_VLAN_CFG_PORT_TPID_SET(ETH_P_8021Q) |
> +		REW_PORT_VLAN_CFG_PORT_VID_SET(port->vid),
> +		REW_PORT_VLAN_CFG_PORT_TPID |
> +		REW_PORT_VLAN_CFG_PORT_VID,
> +		lan966x, REW_PORT_VLAN_CFG(port->chip_port));
> +}
> +
> +int lan966x_vlan_port_add_vlan(struct lan966x_port *port,
> +			       u16 vid,
> +			       bool pvid,
> +			       bool untagged)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +
> +	/* If the CPU(br) is already part of the vlan then add the MAC
> +	 * address of the device in MAC table to copy the frames to the
> +	 * CPU(br). If the CPU(br) is not part of the vlan then it would
> +	 * just drop the frames.
> +	 */
> +	if (lan966x_vlan_cpu_member_cpu_vlan_mask(lan966x, vid)) {
> +		lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, vid);
> +		lan966x_mac_cpu_learn(lan966x, lan966x->bridge->dev_addr, vid);

Doesn't the bridge notify you of all the addresses you need to learn on
the CPU port? What is the benefit of the added complexity of only
learning the addresses when the CPU joins the VLAN? Doesn't the CPU_DIS
bit work if an entry is present in the MAC table?

> +		lan966x_vlan_cpu_add_vlan_mask(lan966x, vid);
> +	}
> +
> +	lan966x_vlan_port_set_vid(port, vid, pvid, untagged);
> +	lan966x_vlan_port_add_vlan_mask(port, vid);
> +	lan966x_vlan_port_apply(port);
> +
> +	return 0;
> +}
> +
> +int lan966x_vlan_port_del_vlan(struct lan966x_port *port,
> +			       u16 vid)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +
> +	/* In case the CPU(br) is part of the vlan then remove the MAC entry
> +	 * because frame doesn't need to reach to CPU
> +	 */
> +	if (lan966x_vlan_cpu_member_cpu_vlan_mask(lan966x, vid))
> +		lan966x_mac_cpu_forget(lan966x, port->dev->dev_addr, vid);
> +
> +	lan966x_vlan_port_remove_vid(port, vid);
> +	lan966x_vlan_port_del_vlan_mask(port, vid);
> +	lan966x_vlan_port_apply(port);
> +
> +	/* In case there are no other ports in vlan then remove the CPU from
> +	 * that vlan but still keep it in the mask because it may be needed
> +	 * again then another port gets added in tha vlan

s/tha/that/

> +	 */
> +	if (!lan966x_vlan_port_any_vlan_mask(lan966x, vid)) {
> +		lan966x_mac_cpu_forget(lan966x, lan966x->bridge->dev_addr, vid);
> +		lan966x_vlan_cpu_del_vlan_mask(lan966x, vid);
> +	}
> +
> +	return 0;
> +}
> +
> +int lan966x_vlan_cpu_add_vlan(struct lan966x *lan966x,
> +			      struct net_device *dev,
> +			      u16 vid)
> +{
> +	int p;
> +
> +	/* Iterate over the ports and see which ones are part of the
> +	 * vlan and for those ports add entry in the MAC table to
> +	 * copy the frames to the CPU
> +	 */
> +	for (p = 0; p < lan966x->num_phys_ports; p++) {
> +		struct lan966x_port *port = lan966x->ports[p];
> +
> +		if (!port ||
> +		    !lan966x_vlan_port_member_vlan_mask(port, vid))
> +			continue;
> +
> +		lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, vid);
> +	}
> +
> +	/* Add an entry in the MAC table for the CPU
> +	 * Add the CPU part of the vlan only if there is another port in that
> +	 * vlan otherwise all the broadcast frames in that vlan will go to CPU
> +	 * even if none of the ports are in the vlan and then the CPU will just
> +	 * need to discard these frames. It is required to store this
> +	 * information so when a front port is added then it would add also the
> +	 * CPU port.
> +	 */
> +	if (lan966x_vlan_port_any_vlan_mask(lan966x, vid)) {
> +		lan966x_mac_cpu_learn(lan966x, dev->dev_addr, vid);
> +		lan966x_vlan_cpu_add_vlan_mask(lan966x, vid);
> +	}
> +
> +	lan966x_vlan_cpu_add_cpu_vlan_mask(lan966x, vid);
> +
> +	return 0;
> +}
> +
> +int lan966x_vlan_cpu_del_vlan(struct lan966x *lan966x,
> +			      struct net_device *dev,
> +			      u16 vid)
> +{
> +	int p;
> +
> +	/* Iterate over the ports and see which ones are part of the
> +	 * vlan and for those ports remove entry in the MAC table to
> +	 * copy the frames to the CPU
> +	 */
> +	for (p = 0; p < lan966x->num_phys_ports; p++) {
> +		struct lan966x_port *port = lan966x->ports[p];
> +
> +		if (!port ||
> +		    !lan966x_vlan_port_member_vlan_mask(port, vid))
> +			continue;
> +
> +		lan966x_mac_cpu_forget(lan966x, port->dev->dev_addr, vid);
> +	}
> +
> +	/* Remove an entry in the MAC table for the CPU */
> +	lan966x_mac_cpu_forget(lan966x, dev->dev_addr, vid);
> +
> +	/* Remove the CPU part of the vlan */
> +	lan966x_vlan_cpu_del_cpu_vlan_mask(lan966x, vid);
> +	lan966x_vlan_cpu_del_vlan_mask(lan966x, vid);
> +
> +	return 0;
> +}
> +
> +int lan966x_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +
> +	lan966x_vlan_port_set_vid(port, vid, false, false);
> +	lan966x_vlan_port_add_vlan_mask(port, vid);
> +	lan966x_vlan_port_apply(port);
> +
> +	return 0;
> +}
> +
> +int lan966x_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
> +			     u16 vid)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +
> +	lan966x_vlan_port_remove_vid(port, vid);
> +	lan966x_vlan_port_del_vlan_mask(port, vid);
> +	lan966x_vlan_port_apply(port);
> +
> +	return 0;
> +}
> +
> +void lan966x_vlan_mode(struct lan966x_port *port,
> +		       netdev_features_t features)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +	u32 val;
> +
> +	/* Filtering */
> +	val = lan_rd(lan966x, ANA_VLANMASK);
> +	if (features & NETIF_F_HW_VLAN_CTAG_FILTER)
> +		val |= BIT(port->chip_port);
> +	else
> +		val &= ~BIT(port->chip_port);
> +	lan_wr(val, lan966x, ANA_VLANMASK);
> +}
> -- 
> 2.33.0
>

Re: [PATCH net-next v5 6/9] net: lan966x: Add support to offload the forwarding.

[Horatiu Vultur]

The 12/15/2021 23:50, Vladimir Oltean wrote:
> 
> On Wed, Dec 15, 2021 at 01:13:06PM +0100, Horatiu Vultur wrote:
> > This patch adds basic support to offload in the HW the forwarding of the
> > frames. The driver registers to the switchdev callbacks and implements
> > the callbacks for attributes SWITCHDEV_ATTR_ID_PORT_STP_STATE and
> > SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME.
> > It is not allowed to add a lan966x port to a bridge that contains a
> > different interface than lan966x.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  .../net/ethernet/microchip/lan966x/Kconfig    |   1 +
> >  .../net/ethernet/microchip/lan966x/Makefile   |   2 +-
> >  .../ethernet/microchip/lan966x/lan966x_main.c |  16 +-
> >  .../ethernet/microchip/lan966x/lan966x_main.h |  11 +
> >  .../microchip/lan966x/lan966x_switchdev.c     | 393 ++++++++++++++++++
> >  5 files changed, 419 insertions(+), 4 deletions(-)
> >  create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> >
> > diff --git a/drivers/net/ethernet/microchip/lan966x/Kconfig b/drivers/net/ethernet/microchip/lan966x/Kconfig
> > index 2860a8c9923d..ac273f84b69e 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/Kconfig
> > +++ b/drivers/net/ethernet/microchip/lan966x/Kconfig
> > @@ -2,6 +2,7 @@ config LAN966X_SWITCH
> >       tristate "Lan966x switch driver"
> >       depends on HAS_IOMEM
> >       depends on OF
> > +     depends on NET_SWITCHDEV
> >       select PHYLINK
> >       select PACKING
> >       help
> > diff --git a/drivers/net/ethernet/microchip/lan966x/Makefile b/drivers/net/ethernet/microchip/lan966x/Makefile
> > index 2989ba528236..974229c51f55 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/Makefile
> > +++ b/drivers/net/ethernet/microchip/lan966x/Makefile
> > @@ -6,4 +6,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_mac.o lan966x_ethtool.o lan966x_switchdev.o
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > index dc40ac2eb246..ee453967da71 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > @@ -355,6 +355,11 @@ static const struct net_device_ops lan966x_port_netdev_ops = {
> >       .ndo_get_port_parent_id         = lan966x_port_get_parent_id,
> >  };
> >
> > +bool lan966x_netdevice_check(const struct net_device *dev)
> > +{
> > +     return dev->netdev_ops == &lan966x_port_netdev_ops;
> > +}
> > +
> >  static int lan966x_port_xtr_status(struct lan966x *lan966x, u8 grp)
> >  {
> >       return lan_rd(lan966x, QS_XTR_RD(grp));
> > @@ -491,6 +496,9 @@ static irqreturn_t lan966x_xtr_irq_handler(int irq, void *args)
> >
> >               skb->protocol = eth_type_trans(skb, dev);
> >
> > +             if (lan966x->bridge_mask & BIT(src_port))
> > +                     skb->offload_fwd_mark = 1;
> > +
> >               netif_rx_ni(skb);
> >               dev->stats.rx_bytes += len;
> >               dev->stats.rx_packets++;
> > @@ -578,9 +586,6 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
> >
> >       eth_hw_addr_gen(dev, lan966x->base_mac, p + 1);
> >
> > -     lan966x_mac_learn(lan966x, PGID_CPU, dev->dev_addr, port->pvid,
> > -                       ENTRYTYPE_LOCKED);
> > -
> >       port->phylink_config.dev = &port->dev->dev;
> >       port->phylink_config.type = PHYLINK_NETDEV;
> >       port->phylink_pcs.poll = true;
> > @@ -897,6 +902,8 @@ static int lan966x_probe(struct platform_device *pdev)
> >               lan966x_port_init(lan966x->ports[p]);
> >       }
> >
> > +     lan966x_register_notifier_blocks(lan966x);
> 
> To be clear, "singleton" would mean that irrespective of the number of
> driver instances, this function would be called once. So calling it from
> lan966x_probe() isn't exactly a good choice, since every instance of the
> driver "probes".

Ah.. yes. I will update it in the next version.

> 
> int dsa_slave_register_notifier(void)
> {
>         struct notifier_block *nb;
>         int err;
> 
>         err = register_netdevice_notifier(&dsa_slave_nb);
>         if (err)
>                 return err;
> 
>         err = register_switchdev_notifier(&dsa_slave_switchdev_notifier);
>         if (err)
>                 goto err_switchdev_nb;
> 
>         nb = &dsa_slave_switchdev_blocking_notifier;
>         err = register_switchdev_blocking_notifier(nb);
>         if (err)
>                 goto err_switchdev_blocking_nb;
> }
> 
> static int __init dsa_init_module(void)
> {
>         rc = dsa_slave_register_notifier();
> }
> module_init(dsa_init_module);
> 
> > +
> >       return 0;
> >
> >  cleanup_ports:
> > @@ -915,6 +922,8 @@ static int lan966x_remove(struct platform_device *pdev)
> >  {
> >       struct lan966x *lan966x = platform_get_drvdata(pdev);
> >
> > +     lan966x_unregister_notifier_blocks(lan966x);
> > +
> >       lan966x_cleanup_ports(lan966x);
> >
> >       cancel_delayed_work_sync(&lan966x->stats_work);
> > @@ -922,6 +931,7 @@ static int lan966x_remove(struct platform_device *pdev)
> >       mutex_destroy(&lan966x->stats_lock);
> >
> >       lan966x_mac_purge_entries(lan966x);
> > +     lan966x_ext_purge_entries();
> 
> Broken with multiple lan966x driver instances - you'd erase all other
> drivers' tabs keps on bridges in the system as soon as one single switch
> is unbound from its driver.

This will not be needed anymore in next version.

> 
> >
> >       return 0;
> >  }
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > index fcd5d09a070c..3d228c9c0521 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > @@ -75,6 +75,10 @@ struct lan966x {
> >
> >       u8 base_mac[ETH_ALEN];
> >
> > +     struct net_device *bridge;
> > +     u16 bridge_mask;
> > +     u16 bridge_fwd_mask;
> > +
> >       struct list_head mac_entries;
> >       spinlock_t mac_lock; /* lock for mac_entries list */
> >
> > @@ -122,6 +126,11 @@ 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;
> >
> > +bool lan966x_netdevice_check(const struct net_device *dev);
> > +
> > +void lan966x_register_notifier_blocks(struct lan966x *lan966x);
> > +void lan966x_unregister_notifier_blocks(struct lan966x *lan966x);
> > +
> >  void lan966x_stats_get(struct net_device *dev,
> >                      struct rtnl_link_stats64 *stats);
> >  int lan966x_stats_init(struct lan966x *lan966x);
> > @@ -157,6 +166,8 @@ int lan966x_mac_add_entry(struct lan966x *lan966x,
> >  void lan966x_mac_purge_entries(struct lan966x *lan966x);
> >  irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x);
> >
> > +void lan966x_ext_purge_entries(void);
> > +
> >  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
> > new file mode 100644
> > index 000000000000..722ce7cb61b3
> > --- /dev/null
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > @@ -0,0 +1,393 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include <linux/if_bridge.h>
> > +#include <net/switchdev.h>
> > +
> > +#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 LIST_HEAD(ext_entries);
> > +
> > +struct lan966x_ext_entry {
> > +     struct list_head list;
> > +     struct net_device *dev;
> > +     u32 ports;
> > +     struct lan966x *lan966x;
> > +};
> > +
> > +static void lan966x_update_fwd_mask(struct lan966x *lan966x)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < lan966x->num_phys_ports; i++) {
> > +             struct lan966x_port *port = lan966x->ports[i];
> > +             unsigned long mask = 0;
> > +
> > +             if (port && lan966x->bridge_fwd_mask & BIT(i))
> > +                     mask = lan966x->bridge_fwd_mask & ~BIT(i);
> > +
> > +             mask |= BIT(CPU_PORT);
> > +
> > +             lan_wr(ANA_PGID_PGID_SET(mask),
> > +                    lan966x, ANA_PGID(PGID_SRC + i));
> > +     }
> 
> I vaguely remember this was implemented better in previous versions of
> the patch set, and the restriction to not allow multiple bridges
> spanning the same switch wasn't there. Why do you keep disallowing
> multiple bridges for all the Microchip hardware? There are very real use
> cases that need them.

I think there are some cases where this is required. I am not 100% but just
to be in sync with the other Microchip devices, I have implemented the
same.

> 
> > +}
> > +
> > +static void lan966x_port_stp_state_set(struct lan966x_port *port, u8 state)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +     bool learn_ena = false;
> > +
> > +     if (state == BR_STATE_FORWARDING || state == BR_STATE_LEARNING)
> > +             learn_ena = true;
> > +
> > +     if (state == BR_STATE_FORWARDING)
> > +             lan966x->bridge_fwd_mask |= BIT(port->chip_port);
> > +     else
> > +             lan966x->bridge_fwd_mask &= ~BIT(port->chip_port);
> > +
> > +     lan_rmw(ANA_PORT_CFG_LEARN_ENA_SET(learn_ena),
> > +             ANA_PORT_CFG_LEARN_ENA,
> > +             lan966x, ANA_PORT_CFG(port->chip_port));
> > +
> > +     lan966x_update_fwd_mask(lan966x);
> > +}
> > +
> > +static 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;
> > +
> > +     lan966x_mac_set_ageing(port->lan966x, ageing_time);
> > +}
> > +
> > +static int lan966x_port_attr_set(struct net_device *dev, const void *ctx,
> > +                              const struct switchdev_attr *attr,
> > +                              struct netlink_ext_ack *extack)
> > +{
> > +     struct lan966x_port *port = netdev_priv(dev);
> > +     int err = 0;
> > +
> > +     if (ctx && ctx != port)
> > +             return 0;
> > +
> > +     switch (attr->id) {
> > +     case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
> > +             lan966x_port_stp_state_set(port, attr->u.stp_state);
> > +             break;
> > +     case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
> > +             lan966x_port_ageing_set(port, attr->u.ageing_time);
> > +             break;
> > +     default:
> > +             err = -EOPNOTSUPP;
> > +             break;
> > +     }
> > +
> > +     return err;
> > +}
> > +
> > +static int lan966x_port_bridge_join(struct lan966x_port *port,
> > +                                 struct net_device *bridge,
> > +                                 struct netlink_ext_ack *extack)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +     struct net_device *dev = port->dev;
> > +     int err;
> > +
> > +     if (!lan966x->bridge_mask) {
> > +             lan966x->bridge = bridge;
> > +     } else {
> > +             if (lan966x->bridge != bridge)
> 
> NL_SET_ERR_MSG_MOD(extack, "<excuse>");
> 
> > +                     return -ENODEV;
> > +     }
> > +
> > +     err = switchdev_bridge_port_offload(dev, dev, port,
> > +                                         &lan966x_switchdev_nb,
> > +                                         &lan966x_switchdev_blocking_nb,
> > +                                         false, extack);
> > +     if (err)
> > +             return err;
> > +
> > +     lan966x->bridge_mask |= BIT(port->chip_port);
> > +
> > +     return 0;
> > +}
> > +
> > +static void lan966x_port_bridge_leave(struct lan966x_port *port,
> > +                                   struct net_device *bridge)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +
> > +     lan966x->bridge_mask &= ~BIT(port->chip_port);
> > +
> > +     if (!lan966x->bridge_mask)
> > +             lan966x->bridge = NULL;
> > +
> > +     lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, PORT_PVID);
> > +}
> > +
> > +static int lan966x_port_changeupper(struct net_device *dev,
> > +                                 struct netdev_notifier_changeupper_info *info)
> > +{
> > +     struct lan966x_port *port = netdev_priv(dev);
> > +     struct netlink_ext_ack *extack;
> > +     int err = 0;
> > +
> > +     extack = netdev_notifier_info_to_extack(&info->info);
> > +
> > +     if (netif_is_bridge_master(info->upper_dev)) {
> > +             if (info->linking)
> > +                     err = lan966x_port_bridge_join(port, info->upper_dev,
> > +                                                    extack);
> > +             else
> > +                     lan966x_port_bridge_leave(port, info->upper_dev);
> > +     }
> > +
> > +     return err;
> > +}
> > +
> > +static int lan966x_port_prechangeupper(struct net_device *dev,
> > +                                    struct netdev_notifier_changeupper_info *info)
> > +{
> > +     struct lan966x_port *port = netdev_priv(dev);
> > +
> > +     if (netif_is_bridge_master(info->upper_dev) && !info->linking)
> > +             switchdev_bridge_port_unoffload(port->dev, port,
> > +                                             &lan966x_switchdev_nb,
> > +                                             &lan966x_switchdev_blocking_nb);
> > +
> > +     return NOTIFY_DONE;
> > +}
> > +
> > +static int lan966x_port_add_addr(struct net_device *dev, bool up)
> > +{
> > +     struct lan966x_port *port = netdev_priv(dev);
> > +     struct lan966x *lan966x = port->lan966x;
> > +     u16 vid;
> > +
> > +     vid = port->pvid;
> > +
> > +     if (up)
> > +             lan966x_mac_cpu_learn(lan966x, dev->dev_addr, vid);
> > +     else
> > +             lan966x_mac_cpu_forget(lan966x, dev->dev_addr, vid);
> > +
> > +     return 0;
> > +}
> > +
> > +static struct lan966x_ext_entry *lan966x_ext_find_entry(struct net_device *dev)
> > +{
> > +     struct lan966x_ext_entry *ext_entry;
> > +
> > +     list_for_each_entry(ext_entry, &ext_entries, list) {
> > +             if (ext_entry->dev == dev)
> > +                     return ext_entry;
> > +     }
> > +
> > +     return NULL;
> > +}
> > +
> > +static bool lan966x_ext_add_entry(struct net_device *dev, void *lan966x)
> > +{
> > +     struct lan966x_ext_entry *ext_entry;
> > +
> > +     ext_entry = lan966x_ext_find_entry(dev);
> > +     if (ext_entry) {
> > +             if (ext_entry->lan966x)
> > +                     return false;
> > +
> > +             ext_entry->ports++;
> > +             return true;
> > +     }
> > +
> > +     ext_entry = kzalloc(sizeof(*ext_entry), GFP_KERNEL);
> > +     if (!ext_entry)
> > +             return false;
> > +
> > +     ext_entry->dev = dev;
> > +     ext_entry->ports = 1;
> > +     ext_entry->lan966x = lan966x;
> > +     list_add_tail(&ext_entry->list, &ext_entries);
> > +     return true;
> > +}
> > +
> > +static void lan966x_ext_remove_entry(struct net_device *dev)
> > +{
> > +     struct lan966x_ext_entry *ext_entry;
> > +
> > +     ext_entry = lan966x_ext_find_entry(dev);
> > +     if (!ext_entry)
> > +             return;
> > +
> > +     ext_entry->ports--;
> > +     if (!ext_entry->ports) {
> > +             list_del(&ext_entry->list);
> > +             kfree(ext_entry);
> > +     }
> > +}
> > +
> > +void lan966x_ext_purge_entries(void)
> > +{
> > +     struct lan966x_ext_entry *ext_entry, *tmp;
> > +
> > +     list_for_each_entry_safe(ext_entry, tmp, &ext_entries, list) {
> > +             list_del(&ext_entry->list);
> > +             kfree(ext_entry);
> > +     }
> > +}
> > +
> > +static int lan966x_ext_check_entry(struct net_device *dev,
> > +                                unsigned long event,
> > +                                void *ptr)
> > +{
> > +     struct netdev_notifier_changeupper_info *info;
> > +
> > +     if (event != NETDEV_PRECHANGEUPPER)
> > +             return 0;
> > +
> > +     info = ptr;
> > +     if (!netif_is_bridge_master(info->upper_dev))
> > +             return 0;
> > +
> > +     if (info->linking) {
> > +             if (!lan966x_ext_add_entry(info->upper_dev, NULL))
> > +                     return -EOPNOTSUPP;
> > +     } else {
> > +             lan966x_ext_remove_entry(info->upper_dev);
> > +     }
> > +
> > +     return NOTIFY_DONE;
> > +}
> > +
> > +static bool lan966x_port_ext_check_entry(struct net_device *dev,
> > +                                      struct netdev_notifier_changeupper_info *info)
> > +{
> > +     struct lan966x_port *port = netdev_priv(dev);
> > +     struct lan966x *lan966x = port->lan966x;
> > +     struct lan966x_ext_entry *entry;
> > +
> > +     if (!netif_is_bridge_master(info->upper_dev))
> > +             return true;
> > +
> > +     entry = lan966x_ext_find_entry(info->upper_dev);
> 
> "entry" is unused in the "else" block below, so logically speaking it
> could be moved inside the "if" block.
> 
> Anyway, this piece of code is objectively speaking very obscure: convoluted
> (lan966x_port_ext_check_entry calls lan966x_ext_find_entry _twice_, once
> here and once in lan966x_ext_add_entry ?!), no comments and poorly named
> (a lan966x_ext_entry represents a _bridge_ ?! what does "ext_entry"
> stand for?). Plus, with your design where the "ext_entries" list is
> global, and there are two instances of the driver, each driver would do
> this work twice and allocate memory twice. Although, I didn't really
> understand why you need to allocate memory to keep a tab on every bridge
> in the system in the first place.
> 
> If you move your check from NETDEV_PRECHANGEUPPER to NETDEV_CHANGEUPPER,
> you allow the upper/lower adjacency list relationship to have formed
> (allowing the use of netdev_for_each_lower_dev, and the newly joining
> interface will be a lower of the bridge). But you can still reject the
> bridge join.
> 
> So you can do something like this, and it should produce an equivalent
> effect (not compiled, not tested, written straight in the email body):
> 
> static int lan966x_foreign_bridging_check(struct net_device *bridge,
>                                           struct netlink_ext_ack *extack)
> {
>         struct lan966x *lan966x = NULL;
>         bool has_foreign = false;
>         struct net_device *dev;
>         struct list_head *iter;
> 
>         netdev_for_each_lower_dev(bridge, dev, iter) {
>                 if (lan966x_netdevice_check(dev)) {
>                         struct lan966x_port *port = netdev_priv(dev);
> 
>                         if (lan996x) {
>                                 /* Bridge already has at least one port
>                                  * of a lan966x switch inside it, check
>                                  * that it's the same instance of the
>                                  * driver.
>                                  */
>                                 if (port->lan966x != lan996x) {
>                                         NL_SET_ERR_MSG_MOD(extack, "Bridging between multiple lan966x switches disallowed");
>                                         return -EINVAL;
>                                 }
>                         } else {
>                                 /* This is the first lan966x port inside
>                                  * this bridge
>                                  */
>                                 lan966x = port->lan966x;
>                         }
>                 } else {
>                         has_foreign = true;
>                 }
> 
>                 if (lan966x && has_foreign) {
>                         NL_SET_ERR_MSG_MOD(extack, "Bridging lan966x ports with foreign interfaces disallowed");
>                         return -EINVAL;
>                 }
>         }
> 
>         return 0;
> }
> 
> and call this from two distinct call paths: from the NETDEV_CHANGEUPPER
> of foreign interfaces, and from the NETDEV_CHANGEUPPER of lan966x interfaces.
> 
> Is it just me, or does this look more obvious and straightforward?

Thanks for the code, it looks much more better and it is not to required
to keep the list of other bridges.

> 
> > +     if (info->linking) {
> > +             if (!entry)
> > +                     return lan966x_ext_add_entry(info->upper_dev, lan966x);
> > +
> > +             if (entry->lan966x == lan966x) {
> > +                     entry->ports++;
> > +                     return true;
> > +             }
> > +     } else {
> > +             lan966x_ext_remove_entry(info->upper_dev);
> > +             return true;
> > +     }
> > +
> > +     return false;
> > +}
> > +
> > +static int lan966x_netdevice_port_event(struct net_device *dev,
> > +                                     struct notifier_block *nb,
> > +                                     unsigned long event, void *ptr)
> > +{
> > +     int err = 0;
> > +
> > +     if (!lan966x_netdevice_check(dev))
> > +             return lan966x_ext_check_entry(dev, event, ptr);
> > +
> > +     switch (event) {
> > +     case NETDEV_PRECHANGEUPPER:
> > +             if (!lan966x_port_ext_check_entry(dev, ptr))
> > +                     return -EOPNOTSUPP;
> > +
> > +             err = lan966x_port_prechangeupper(dev, ptr);
> > +             break;
> > +     case NETDEV_CHANGEUPPER:
> > +             err = lan966x_port_changeupper(dev, ptr);
> > +             break;
> > +     case NETDEV_PRE_UP:
> > +             err = lan966x_port_add_addr(dev, true);
> > +             break;
> > +     case NETDEV_DOWN:
> > +             err = lan966x_port_add_addr(dev, false);
> 
> Any reason why you track your own NETDEV_PRE_UP/NETDEV_DOWN and don't do
> this directly in ->ndo_open/->ndo_close? Also, I don't think that the
> "lan966x_port_add_addr" brings much value over "lan966x_mac_cpu_learn"
> and "lan966x_mac_cpu_forget" called directly (especially if moved to
> lan966x_port_open and lan966x_port_stop). And I don't see the relevance
> of this change with respect to the commit title "add support to offload
> the forwarding". CPU learned entries are for termination.

OK, I will drop this for now. I will add this or in a different patch of
this series or will be another patch by itself.

> 
> > +             break;
> > +     }
> > +
> > +     return err;
> > +}
> > +
> > +static int lan966x_netdevice_event(struct notifier_block *nb,
> > +                                unsigned long event, void *ptr)
> > +{
> > +     struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> > +     int ret;
> > +
> > +     ret = lan966x_netdevice_port_event(dev, nb, event, ptr);
> > +
> > +     return notifier_from_errno(ret);
> > +}
> > +
> > +static int lan966x_switchdev_event(struct notifier_block *nb,
> > +                                unsigned long event, void *ptr)
> > +{
> > +     struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
> > +     int err;
> > +
> > +     switch (event) {
> > +     case SWITCHDEV_PORT_ATTR_SET:
> > +             err = switchdev_handle_port_attr_set(dev, ptr,
> > +                                                  lan966x_netdevice_check,
> > +                                                  lan966x_port_attr_set);
> > +             return notifier_from_errno(err);
> > +     }
> > +
> > +     return NOTIFY_DONE;
> > +}
> > +
> > +static int lan966x_switchdev_blocking_event(struct notifier_block *nb,
> > +                                         unsigned long event,
> > +                                         void *ptr)
> > +{
> > +     struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
> > +     int err;
> > +
> > +     switch (event) {
> > +     case SWITCHDEV_PORT_ATTR_SET:
> > +             err = switchdev_handle_port_attr_set(dev, ptr,
> > +                                                  lan966x_netdevice_check,
> > +                                                  lan966x_port_attr_set);
> > +             return notifier_from_errno(err);
> > +     }
> > +
> > +     return NOTIFY_DONE;
> > +}
> > +
> > +static struct notifier_block lan966x_netdevice_nb __read_mostly = {
> > +     .notifier_call = lan966x_netdevice_event,
> > +};
> > +
> > +static struct notifier_block lan966x_switchdev_nb __read_mostly = {
> > +     .notifier_call = lan966x_switchdev_event,
> > +};
> > +
> > +static struct notifier_block lan966x_switchdev_blocking_nb __read_mostly = {
> > +     .notifier_call = lan966x_switchdev_blocking_event,
> > +};
> > +
> > +void lan966x_register_notifier_blocks(struct lan966x *lan966x)
> > +{
> > +     register_netdevice_notifier(&lan966x_netdevice_nb);
> > +     register_switchdev_notifier(&lan966x_switchdev_nb);
> > +     register_switchdev_blocking_notifier(&lan966x_switchdev_blocking_nb);
> > +}
> > +
> > +void lan966x_unregister_notifier_blocks(struct lan966x *lan966x)
> > +{
> > +     unregister_switchdev_blocking_notifier(&lan966x_switchdev_blocking_nb);
> > +     unregister_switchdev_notifier(&lan966x_switchdev_nb);
> > +     unregister_netdevice_notifier(&lan966x_netdevice_nb);
> > +}
> > --
> > 2.33.0
> >

-- 
/Horatiu

Re: [PATCH net-next v5 7/9] net: lan966x: Add vlan support.

[Horatiu Vultur]

The 12/16/2021 00:44, Vladimir Oltean wrote:
> 
> On Wed, Dec 15, 2021 at 01:13:07PM +0100, Horatiu Vultur wrote:
> > Extend the driver to support vlan filtering  by implementing the
> > switchdev calls SWITCHDEV_OBJ_ID_PORT_VLAN and
> > SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING.
> 
> And the VLAN RX filtering net device ops.
> 
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  .../net/ethernet/microchip/lan966x/Makefile   |   3 +-
> >  .../ethernet/microchip/lan966x/lan966x_main.c |  39 +-
> >  .../ethernet/microchip/lan966x/lan966x_main.h |  40 +-
> >  .../microchip/lan966x/lan966x_switchdev.c     | 113 ++++-
> >  .../ethernet/microchip/lan966x/lan966x_vlan.c | 444 ++++++++++++++++++
> >  5 files changed, 632 insertions(+), 7 deletions(-)
> >  create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> >
> > diff --git a/drivers/net/ethernet/microchip/lan966x/Makefile b/drivers/net/ethernet/microchip/lan966x/Makefile
> > index 974229c51f55..d82e896c2e53 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/Makefile
> > +++ b/drivers/net/ethernet/microchip/lan966x/Makefile
> > @@ -6,4 +6,5 @@
> >  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_mac.o lan966x_ethtool.o lan966x_switchdev.o \
> > +                     lan966x_vlan.o
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > index ee453967da71..881c1678f3e9 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > @@ -103,17 +103,18 @@ static int lan966x_create_targets(struct platform_device *pdev,
> >  static int lan966x_port_set_mac_address(struct net_device *dev, void *p)
> >  {
> >       struct lan966x_port *port = netdev_priv(dev);
> > +     u16 pvid = lan966x_vlan_port_get_pvid(port);
> >       struct lan966x *lan966x = port->lan966x;
> >       const struct sockaddr *addr = p;
> >       int ret;
> >
> >       /* Learn the new net device MAC address in the mac table. */
> > -     ret = lan966x_mac_cpu_learn(lan966x, addr->sa_data, port->pvid);
> > +     ret = lan966x_mac_cpu_learn(lan966x, addr->sa_data, pvid);
> 
> Logically speaking, there is a divide of responsibility. The bridge
> emits switchdev FDB events for local MAC addresses, with a VID of 0
> (corresponding to VLAN-unaware bridging) as well as for each installed
> VLAN. Bridge VLAN 0 is equivalent to your UNAWARE_PVID macro. And the
> driver is solely responsible for the MAC address in the HOST_PVID VLAN.
> When the ndo_set_mac_address is called, you should just update the entry
> learned in the HOST_PVID. The bridge will get an NETDEV_CHANGEADDR event
> and update its local MAC addresses too, in the VLANs it handles.
> Otherwise, if you just learn in the pvid that the port is currently in,
> then RX filtering will be broken if you change your MAC address while
> you're under a bridge, then you leave that bridge and become standalone.
> So you need to re-learn the dev_addr in lan966x_port_bridge_leave, which
> makes the implementation a bit more complicated than it needs to be
> (unless I'm missing something about CPU-learned MAC addresses in VLANs
> that aren't currently active, you seem to be avoiding that even though
> it makes the driver keep a lot more state).

Yes, you are right. Thanks for the explanation.

> 
> >       if (ret)
> >               return ret;
> >
> >       /* Then forget the previous one. */
> > -     ret = lan966x_mac_cpu_forget(lan966x, dev->dev_addr, port->pvid);
> > +     ret = lan966x_mac_cpu_forget(lan966x, dev->dev_addr, pvid);
> >       if (ret)
> >               return ret;
> >
> > @@ -283,6 +284,12 @@ static void lan966x_ifh_set_ipv(void *ifh, u64 bypass)
> >               IFH_POS_IPV, IFH_LEN * 4, PACK, 0);
> >  }
> >
> > +static void lan966x_ifh_set_vid(void *ifh, u64 vid)
> > +{
> > +     packing(ifh, &vid, IFH_POS_TCI + IFH_WID_TCI - 1,
> > +             IFH_POS_TCI, IFH_LEN * 4, PACK, 0);
> > +}
> > +
> >  static int lan966x_port_xmit(struct sk_buff *skb, struct net_device *dev)
> >  {
> >       struct lan966x_port *port = netdev_priv(dev);
> > @@ -294,6 +301,7 @@ static int lan966x_port_xmit(struct sk_buff *skb, struct net_device *dev)
> >       lan966x_ifh_set_port(ifh, BIT_ULL(port->chip_port));
> >       lan966x_ifh_set_qos_class(ifh, skb->priority >= 7 ? 0x7 : skb->priority);
> >       lan966x_ifh_set_ipv(ifh, skb->priority >= 7 ? 0x7 : skb->priority);
> > +     lan966x_ifh_set_vid(ifh, skb_vlan_tag_get(skb));
> >
> >       return lan966x_port_ifh_xmit(skb, ifh, dev);
> >  }
> > @@ -343,6 +351,18 @@ static int lan966x_port_get_parent_id(struct net_device *dev,
> >       return 0;
> >  }
> >
> > +static int lan966x_port_set_features(struct net_device *dev,
> > +                                  netdev_features_t features)
> > +{
> > +     struct lan966x_port *port = netdev_priv(dev);
> > +     netdev_features_t changed = dev->features ^ features;
> > +
> > +     if (changed & NETIF_F_HW_VLAN_CTAG_FILTER)
> > +             lan966x_vlan_mode(port, features);
> > +
> > +     return 0;
> > +}
> > +
> >  static const struct net_device_ops lan966x_port_netdev_ops = {
> >       .ndo_open                       = lan966x_port_open,
> >       .ndo_stop                       = lan966x_port_stop,
> > @@ -353,6 +373,9 @@ static const struct net_device_ops lan966x_port_netdev_ops = {
> >       .ndo_get_stats64                = lan966x_stats_get,
> >       .ndo_set_mac_address            = lan966x_port_set_mac_address,
> >       .ndo_get_port_parent_id         = lan966x_port_get_parent_id,
> > +     .ndo_set_features               = lan966x_port_set_features,
> > +     .ndo_vlan_rx_add_vid            = lan966x_vlan_rx_add_vid,
> > +     .ndo_vlan_rx_kill_vid           = lan966x_vlan_rx_kill_vid,
> 
> Do you have any particular use case for NETIF_F_HW_VLAN_CTAG_FILTER on
> non-bridged ports?

Not from what I am aware of, so for now I can remove this.

> I find the fact that you implement these very strange
> and likely bogus: you set port->vlan_aware = false when a port leaves a
> bridge, yet you install VLANs to its RX filter as if those VLANs were to
> actually match on any VLAN-tagged packet... which they won't because
> lan966x_vlan_port_apply() clears ANA_VLAN_CFG_VLAN_AWARE_ENA_SET(1) when
> port->vlan_aware isn't set. So you end up being "filtering" but not "aware"
> - all packets get classified to the same VLAN, which isn't dropped.
> 
> >  };
> >
> >  bool lan966x_netdevice_check(const struct net_device *dev)
> > @@ -575,13 +598,16 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
> >       port->dev = dev;
> >       port->lan966x = lan966x;
> >       port->chip_port = p;
> > -     port->pvid = PORT_PVID;
> >       lan966x->ports[p] = port;
> >
> >       dev->max_mtu = ETH_MAX_MTU;
> >
> >       dev->netdev_ops = &lan966x_port_netdev_ops;
> >       dev->ethtool_ops = &lan966x_ethtool_ops;
> > +     dev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> > +     dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER |
> > +                      NETIF_F_HW_VLAN_CTAG_TX |
> > +                      NETIF_F_HW_VLAN_STAG_TX;
> >       dev->needed_headroom = IFH_LEN * sizeof(u32);
> >
> >       eth_hw_addr_gen(dev, lan966x->base_mac, p + 1);
> > @@ -625,6 +651,10 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
> >               return err;
> >       }
> >
> > +     lan966x_vlan_port_set_vlan_aware(port, 0);
> > +     lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
> > +     lan966x_vlan_port_apply(port);
> > +
> >       return 0;
> >  }
> >
> > @@ -635,6 +665,9 @@ static void lan966x_init(struct lan966x *lan966x)
> >       /* MAC table initialization */
> >       lan966x_mac_init(lan966x);
> >
> > +     /* Vlan initialization */
> > +     lan966x_vlan_init(lan966x);
> 
> Curious how the lan966x_ext_entry stuff doesn't have any comment and
> lan966x_vlan_init has such a trivial one?!
> 
> > +
> >       /* Flush queues */
> >       lan_wr(lan_rd(lan966x, QS_XTR_FLUSH) |
> >              GENMASK(1, 0),
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > index 3d228c9c0521..6d0d922617ae 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > @@ -4,6 +4,7 @@
> >  #define __LAN966X_MAIN_H__
> >
> >  #include <linux/etherdevice.h>
> > +#include <linux/if_vlan.h>
> >  #include <linux/jiffies.h>
> >  #include <linux/phy.h>
> >  #include <linux/phylink.h>
> > @@ -22,7 +23,8 @@
> >  #define PGID_SRC                     80
> >  #define PGID_ENTRIES                 89
> >
> > -#define PORT_PVID                    0
> > +#define UNAWARE_PVID                 0
> > +#define HOST_PVID                    4095
> >
> >  /* Reserved amount for (SRC, PRIO) at index 8*SRC + PRIO */
> >  #define QSYS_Q_RSRV                  95
> > @@ -82,6 +84,9 @@ struct lan966x {
> >       struct list_head mac_entries;
> >       spinlock_t mac_lock; /* lock for mac_entries list */
> >
> > +     u16 vlan_mask[VLAN_N_VID];
> > +     DECLARE_BITMAP(cpu_vlan_mask, VLAN_N_VID);
> > +
> >       /* stats */
> >       const struct lan966x_stat_layout *stats_layout;
> >       u32 num_stats;
> > @@ -113,6 +118,8 @@ struct lan966x_port {
> >
> >       u8 chip_port;
> >       u16 pvid;
> > +     u16 vid;
> > +     u8 vlan_aware;
> 
> bool
> 
> >
> >       struct phylink_config phylink_config;
> >       struct phylink_pcs phylink_pcs;
> > @@ -168,6 +175,37 @@ irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x);
> >
> >  void lan966x_ext_purge_entries(void);
> >
> > +void lan966x_vlan_init(struct lan966x *lan966x);
> > +void lan966x_vlan_port_apply(struct lan966x_port *port);
> > +
> > +int lan966x_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid);
> > +int lan966x_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid);
> > +
> > +void lan966x_vlan_mode(struct lan966x_port *port, netdev_features_t features);
> > +u16 lan966x_vlan_port_get_pvid(struct lan966x_port *port);
> > +
> > +bool lan966x_vlan_cpu_member_cpu_vlan_mask(struct lan966x *lan966x, u16 vid);
> > +void lan966x_vlan_cpu_add_cpu_vlan_mask(struct lan966x *lan966x, u16 vid);
> > +bool lan966x_vlan_port_any_vlan_mask(struct lan966x *lan966x, u16 vid);
> > +
> > +void lan966x_vlan_cpu_set_vlan_aware(struct lan966x_port *port);
> > +void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
> > +                                   bool vlan_aware);
> > +int lan966x_vlan_port_set_vid(struct lan966x_port *port, u16 vid,
> > +                           bool pvid, bool untagged);
> > +int lan966x_vlan_port_add_vlan(struct lan966x_port *port,
> > +                            u16 vid,
> > +                            bool pvid,
> > +                            bool untagged);
> > +int lan966x_vlan_port_del_vlan(struct lan966x_port *port,
> > +                            u16 vid);
> > +int lan966x_vlan_cpu_add_vlan(struct lan966x *lan966x,
> > +                           struct net_device *dev,
> > +                           u16 vid);
> > +int lan966x_vlan_cpu_del_vlan(struct lan966x *lan966x,
> > +                           struct net_device *dev,
> > +                           u16 vid);
> > +
> >  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 722ce7cb61b3..61f9e906cf80 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > @@ -82,6 +82,11 @@ static int lan966x_port_attr_set(struct net_device *dev, const void *ctx,
> >       case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
> >               lan966x_port_ageing_set(port, attr->u.ageing_time);
> >               break;
> > +     case SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING:
> > +             lan966x_vlan_port_set_vlan_aware(port, attr->u.vlan_filtering);
> > +             lan966x_vlan_port_apply(port);
> > +             lan966x_vlan_cpu_set_vlan_aware(port);
> > +             break;
> >       default:
> >               err = -EOPNOTSUPP;
> >               break;
> > @@ -127,7 +132,12 @@ static void lan966x_port_bridge_leave(struct lan966x_port *port,
> >       if (!lan966x->bridge_mask)
> >               lan966x->bridge = NULL;
> >
> > -     lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, PORT_PVID);
> > +     /* Set the port back to host mode */
> > +     lan966x_vlan_port_set_vlan_aware(port, false);
> > +     lan966x_vlan_port_set_vid(port, HOST_PVID, false, false);
> > +     lan966x_vlan_port_apply(port);
> > +
> > +     lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, HOST_PVID);
> >  }
> >
> >  static int lan966x_port_changeupper(struct net_device *dev,
> > @@ -169,7 +179,7 @@ static int lan966x_port_add_addr(struct net_device *dev, bool up)
> >       struct lan966x *lan966x = port->lan966x;
> >       u16 vid;
> >
> > -     vid = port->pvid;
> > +     vid = lan966x_vlan_port_get_pvid(port);
> >
> >       if (up)
> >               lan966x_mac_cpu_learn(lan966x, dev->dev_addr, vid);
> > @@ -348,6 +358,95 @@ static int lan966x_switchdev_event(struct notifier_block *nb,
> >       return NOTIFY_DONE;
> >  }
> >
> > +static int lan966x_handle_port_vlan_add(struct lan966x_port *port,
> > +                                     const struct switchdev_obj *obj)
> > +{
> > +     const struct switchdev_obj_port_vlan *v = SWITCHDEV_OBJ_PORT_VLAN(obj);
> > +     struct lan966x *lan966x = port->lan966x;
> > +
> > +     /* When adding a port to a vlan, we get a callback for the port but
> > +      * also for the bridge. When get the callback for the bridge just bail
> > +      * out. Then when the bridge is added to the vlan, then we get a
> > +      * callback here but in this case the flags has set:
> > +      * BRIDGE_VLAN_INFO_BRENTRY. In this case it means that the CPU
> > +      * port is added to the vlan, so the broadcast frames and unicast frames
> > +      * with dmac of the bridge should be foward to CPU.
> > +      */
> > +     if (netif_is_bridge_master(obj->orig_dev) &&
> > +         !(v->flags & BRIDGE_VLAN_INFO_BRENTRY))
> > +             return 0;
> > +
> > +     if (!netif_is_bridge_master(obj->orig_dev))
> > +             return lan966x_vlan_port_add_vlan(port, v->vid,
> > +                                               v->flags & BRIDGE_VLAN_INFO_PVID,
> > +                                               v->flags & BRIDGE_VLAN_INFO_UNTAGGED);
> > +
> > +     if (netif_is_bridge_master(obj->orig_dev))
> 
> "else" will suffice.
> 
> > +             return lan966x_vlan_cpu_add_vlan(lan966x, obj->orig_dev, v->vid);
> > +
> > +     return 0;
> > +}
> > +
> > +static int lan966x_handle_port_obj_add(struct net_device *dev, const void *ctx,
> > +                                    const struct switchdev_obj *obj,
> > +                                    struct netlink_ext_ack *extack)
> > +{
> > +     struct lan966x_port *port = netdev_priv(dev);
> > +     int err;
> > +
> > +     if (ctx && ctx != port)
> > +             return 0;
> > +
> > +     switch (obj->id) {
> > +     case SWITCHDEV_OBJ_ID_PORT_VLAN:
> > +             err = lan966x_handle_port_vlan_add(port, obj);
> > +             break;
> > +     default:
> > +             err = -EOPNOTSUPP;
> > +             break;
> > +     }
> > +
> > +     return err;
> > +}
> > +
> > +static int lan966x_handle_port_vlan_del(struct lan966x_port *port,
> > +                                     const struct switchdev_obj *obj)
> > +{
> > +     const struct switchdev_obj_port_vlan *v = SWITCHDEV_OBJ_PORT_VLAN(obj);
> > +     struct lan966x *lan966x = port->lan966x;
> > +
> > +     /* In case the physical port gets called */
> > +     if (!netif_is_bridge_master(obj->orig_dev))
> > +             return lan966x_vlan_port_del_vlan(port, v->vid);
> > +
> > +     /* In case the bridge gets called */
> > +     if (netif_is_bridge_master(obj->orig_dev))
> 
> likewise.
> 
> > +             return lan966x_vlan_cpu_del_vlan(lan966x, obj->orig_dev, v->vid);
> > +
> > +     return 0;
> > +}
> > +
> > +static int lan966x_handle_port_obj_del(struct net_device *dev, const void *ctx,
> > +                                    const struct switchdev_obj *obj)
> > +{
> > +     struct lan966x_port *port = netdev_priv(dev);
> > +     int err;
> > +
> > +     if (ctx && ctx != port)
> > +             return 0;
> > +
> > +     switch (obj->id) {
> > +     case SWITCHDEV_OBJ_ID_PORT_VLAN:
> > +             err = lan966x_handle_port_vlan_del(port, obj);
> > +             break;
> > +     default:
> > +             err = -EOPNOTSUPP;
> > +             break;
> > +     }
> > +
> > +     return err;
> > +}
> > +
> >  static int lan966x_switchdev_blocking_event(struct notifier_block *nb,
> >                                           unsigned long event,
> >                                           void *ptr)
> > @@ -356,6 +455,16 @@ static int lan966x_switchdev_blocking_event(struct notifier_block *nb,
> >       int err;
> >
> >       switch (event) {
> > +     case SWITCHDEV_PORT_OBJ_ADD:
> > +             err = switchdev_handle_port_obj_add(dev, ptr,
> > +                                                 lan966x_netdevice_check,
> > +                                                 lan966x_handle_port_obj_add);
> > +             return notifier_from_errno(err);
> > +     case SWITCHDEV_PORT_OBJ_DEL:
> > +             err = switchdev_handle_port_obj_del(dev, ptr,
> > +                                                 lan966x_netdevice_check,
> > +                                                 lan966x_handle_port_obj_del);
> > +             return notifier_from_errno(err);
> >       case SWITCHDEV_PORT_ATTR_SET:
> >               err = switchdev_handle_port_attr_set(dev, ptr,
> >                                                    lan966x_netdevice_check,
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> > new file mode 100644
> > index 000000000000..e8ff95bb65fa
> > --- /dev/null
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c
> > @@ -0,0 +1,444 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include "lan966x_main.h"
> > +
> > +#define VLANACCESS_CMD_IDLE          0
> > +#define VLANACCESS_CMD_READ          1
> > +#define VLANACCESS_CMD_WRITE         2
> > +#define VLANACCESS_CMD_INIT          3
> > +
> > +static int lan966x_vlan_get_status(struct lan966x *lan966x)
> > +{
> > +     return lan_rd(lan966x, ANA_VLANACCESS);
> > +}
> > +
> > +static int lan966x_vlan_wait_for_completion(struct lan966x *lan966x)
> > +{
> > +     u32 val;
> > +
> > +     return readx_poll_timeout(lan966x_vlan_get_status,
> > +             lan966x, val,
> > +             (val & ANA_VLANACCESS_VLAN_TBL_CMD) ==
> > +             VLANACCESS_CMD_IDLE,
> > +             TABLE_UPDATE_SLEEP_US, TABLE_UPDATE_TIMEOUT_US);
> > +}
> > +
> > +static int lan966x_vlan_set_mask(struct lan966x *lan966x, u16 vid)
> > +{
> > +     u16 mask = lan966x->vlan_mask[vid];
> > +     bool cpu_dis;
> > +
> > +     cpu_dis = !(mask & BIT(CPU_PORT));
> > +
> > +     /* Set flags and the VID to configure */
> > +     lan_rmw(ANA_VLANTIDX_VLAN_PGID_CPU_DIS_SET(cpu_dis) |
> > +             ANA_VLANTIDX_V_INDEX_SET(vid),
> > +             ANA_VLANTIDX_VLAN_PGID_CPU_DIS |
> > +             ANA_VLANTIDX_V_INDEX,
> > +             lan966x, ANA_VLANTIDX);
> > +
> > +     /* Set the vlan port members mask */
> > +     lan_rmw(ANA_VLAN_PORT_MASK_VLAN_PORT_MASK_SET(mask),
> > +             ANA_VLAN_PORT_MASK_VLAN_PORT_MASK,
> > +             lan966x, ANA_VLAN_PORT_MASK);
> > +
> > +     /* Issue a write command */
> > +     lan_rmw(ANA_VLANACCESS_VLAN_TBL_CMD_SET(VLANACCESS_CMD_WRITE),
> > +             ANA_VLANACCESS_VLAN_TBL_CMD,
> > +             lan966x, ANA_VLANACCESS);
> > +
> > +     return lan966x_vlan_wait_for_completion(lan966x);
> 
> If you're not going to propagate the return code anywhere, at least
> return void and print an error here. Otherwise it's totally silent.

Yes, I will return void and print an error.

> 
> > +}
> > +
> > +void lan966x_vlan_init(struct lan966x *lan966x)
> > +{
> > +     u16 port, vid;
> > +
> > +     /* Clear VLAN table, by default all ports are members of all VLANS */
> > +     lan_rmw(ANA_VLANACCESS_VLAN_TBL_CMD_SET(VLANACCESS_CMD_INIT),
> > +             ANA_VLANACCESS_VLAN_TBL_CMD,
> > +             lan966x, ANA_VLANACCESS);
> > +     lan966x_vlan_wait_for_completion(lan966x);
> 
> Again no error checking.
> 
> > +
> > +     for (vid = 1; vid < VLAN_N_VID; vid++) {
> > +             lan966x->vlan_mask[vid] = 0;
> > +             lan966x_vlan_set_mask(lan966x, vid);
> > +     }
> > +
> > +     /* Set all the ports + cpu to be part of HOST_PVID and UNAWARE_PVID */
> > +     lan966x->vlan_mask[HOST_PVID] =
> > +             GENMASK(lan966x->num_phys_ports - 1, 0) | BIT(CPU_PORT);
> > +     lan966x_vlan_set_mask(lan966x, HOST_PVID);
> > +
> > +     lan966x->vlan_mask[UNAWARE_PVID] =
> > +             GENMASK(lan966x->num_phys_ports - 1, 0) | BIT(CPU_PORT);
> > +     lan966x_vlan_set_mask(lan966x, UNAWARE_PVID);
> > +
> > +     lan966x_vlan_cpu_add_cpu_vlan_mask(lan966x, UNAWARE_PVID);
> > +
> > +     /* Configure the CPU port to be vlan aware */
> > +     lan_wr(ANA_VLAN_CFG_VLAN_VID_SET(0) |
> > +            ANA_VLAN_CFG_VLAN_AWARE_ENA_SET(1) |
> > +            ANA_VLAN_CFG_VLAN_POP_CNT_SET(1),
> > +            lan966x, ANA_VLAN_CFG(CPU_PORT));
> > +
> > +     /* Set vlan ingress filter mask to all ports */
> > +     lan_wr(GENMASK(lan966x->num_phys_ports, 0),
> > +            lan966x, ANA_VLANMASK);
> > +
> > +     for (port = 0; port < lan966x->num_phys_ports; port++) {
> > +             lan_wr(0, lan966x, REW_PORT_VLAN_CFG(port));
> > +             lan_wr(0, lan966x, REW_TAG_CFG(port));
> > +     }
> > +}
> > +
> > +static int lan966x_vlan_port_add_vlan_mask(struct lan966x_port *port, u16 vid)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +     u8 p = port->chip_port;
> > +
> > +     lan966x->vlan_mask[vid] |= BIT(p);
> > +     return lan966x_vlan_set_mask(lan966x, vid);
> > +}
> > +
> > +static int lan966x_vlan_port_del_vlan_mask(struct lan966x_port *port, u16 vid)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +     u8 p = port->chip_port;
> > +
> > +     lan966x->vlan_mask[vid] &= ~BIT(p);
> > +     return lan966x_vlan_set_mask(lan966x, vid);
> > +}
> > +
> > +static bool lan966x_vlan_port_member_vlan_mask(struct lan966x_port *port, u16 vid)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +     u8 p = port->chip_port;
> > +
> > +     return lan966x->vlan_mask[vid] & BIT(p);
> > +}
> > +
> > +bool lan966x_vlan_port_any_vlan_mask(struct lan966x *lan966x, u16 vid)
> > +{
> > +     return !!(lan966x->vlan_mask[vid] & ~BIT(CPU_PORT));
> > +}
> > +
> > +static int lan966x_vlan_cpu_add_vlan_mask(struct lan966x *lan966x, u16 vid)
> > +{
> > +     lan966x->vlan_mask[vid] |= BIT(CPU_PORT);
> > +     return lan966x_vlan_set_mask(lan966x, vid);
> > +}
> > +
> > +static int lan966x_vlan_cpu_del_vlan_mask(struct lan966x *lan966x, u16 vid)
> > +{
> > +     lan966x->vlan_mask[vid] &= ~BIT(CPU_PORT);
> > +     return lan966x_vlan_set_mask(lan966x, vid);
> > +}
> > +
> > +void lan966x_vlan_cpu_add_cpu_vlan_mask(struct lan966x *lan966x, u16 vid)
> > +{
> > +     set_bit(vid, lan966x->cpu_vlan_mask);
> 
> Since these are all serialized by the rtnl_mutex, I think it's safe to
> replace with __set_bit which is non-atomic and thus cheaper.
> 
> > +}
> > +
> > +static void lan966x_vlan_cpu_del_cpu_vlan_mask(struct lan966x *lan966x, u16 vid)
> > +{
> > +     clear_bit(vid, lan966x->cpu_vlan_mask);
> > +}
> > +
> > +bool lan966x_vlan_cpu_member_cpu_vlan_mask(struct lan966x *lan966x, u16 vid)
> > +{
> > +     return test_bit(vid, lan966x->cpu_vlan_mask);
> > +}
> > +
> > +u16 lan966x_vlan_port_get_pvid(struct lan966x_port *port)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +
> > +     if (!(lan966x->bridge_mask & BIT(port->chip_port)))
> > +             return HOST_PVID;
> > +
> > +     return port->vlan_aware ? port->pvid : UNAWARE_PVID;
> > +}
> > +
> > +int lan966x_vlan_port_set_vid(struct lan966x_port *port, u16 vid,
> > +                           bool pvid, bool untagged)
> 
> If you were to summarize what this function does, what would that be?

This will set the port pvid and vid based on the parameters pvid and
untagged.

> 
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +
> > +     /* Egress vlan classification */
> > +     if (untagged && port->vid != vid) {
> > +             if (port->vid) {
> > +                     dev_err(lan966x->dev,
> > +                             "Port already has a native VLAN: %d\n",
> > +                             port->vid);
> > +                     return -EBUSY;
> > +             }
> > +             port->vid = vid;
> > +     }
> > +
> > +     /* Default ingress vlan classification */
> > +     if (pvid)
> > +             port->pvid = vid;
> > +
> > +     return 0;
> > +}
> > +
> > +static int lan966x_vlan_port_remove_vid(struct lan966x_port *port, u16 vid)
> > +{
> > +     if (port->pvid == vid)
> > +             port->pvid = 0;
> > +
> > +     if (port->vid == vid)
> > +             port->vid = 0;
> > +
> > +     return 0;
> > +}
> > +
> > +void lan966x_vlan_port_set_vlan_aware(struct lan966x_port *port,
> > +                                   bool vlan_aware)
> > +{
> > +     port->vlan_aware = vlan_aware;
> > +}
> > +
> > +void lan966x_vlan_cpu_set_vlan_aware(struct lan966x_port *port)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +
> > +     if (!port->vlan_aware) {
> > +             /* In case of vlan unaware, all the ports will be set in
> > +              * UNAWARE_PVID and have their PVID set to this PVID
> > +              * The CPU doesn't need to be added because it is always part of
> > +              * that vlan, it is required just to add entries in the MAC
> > +              * table for the front port and the CPU
> > +              */
> > +             lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, UNAWARE_PVID);
> > +             lan966x_mac_cpu_learn(lan966x, lan966x->bridge->dev_addr,
> > +                                   UNAWARE_PVID);
> > +
> > +             lan966x_vlan_port_add_vlan_mask(port, UNAWARE_PVID);
> > +             lan966x_vlan_port_apply(port);
> > +     } else {
> > +             /* In case of vlan aware, just clear what happened when changed
> > +              * to vlan unaware
> > +              */
> > +             lan966x_mac_cpu_forget(lan966x, port->dev->dev_addr, UNAWARE_PVID);
> > +             lan966x_mac_cpu_forget(lan966x, lan966x->bridge->dev_addr,
> > +                                    UNAWARE_PVID);
> > +
> > +             lan966x_vlan_port_del_vlan_mask(port, UNAWARE_PVID);
> > +             lan966x_vlan_port_apply(port);
> > +     }
> > +}
> > +
> > +void lan966x_vlan_port_apply(struct lan966x_port *port)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +     u16 pvid;
> > +     u32 val;
> > +
> > +     pvid = lan966x_vlan_port_get_pvid(port);
> > +
> > +     /* Ingress clasification (ANA_PORT_VLAN_CFG) */
> > +     /* Default vlan to casify for untagged frames (may be zero) */
> 
> classify
> 
> > +     val = ANA_VLAN_CFG_VLAN_VID_SET(pvid);
> > +     if (port->vlan_aware)
> > +             val |= ANA_VLAN_CFG_VLAN_AWARE_ENA_SET(1) |
> > +                    ANA_VLAN_CFG_VLAN_POP_CNT_SET(1);
> > +
> > +     lan_rmw(val,
> > +             ANA_VLAN_CFG_VLAN_VID | ANA_VLAN_CFG_VLAN_AWARE_ENA |
> > +             ANA_VLAN_CFG_VLAN_POP_CNT,
> > +             lan966x, ANA_VLAN_CFG(port->chip_port));
> > +
> > +     /* Drop frames with multicast source address */
> > +     val = ANA_DROP_CFG_DROP_MC_SMAC_ENA_SET(1);
> > +     if (port->vlan_aware && !pvid)
> > +             /* If port is vlan-aware and tagged, drop untagged and priority
> > +              * tagged frames.
> > +              */
> > +             val |= ANA_DROP_CFG_DROP_UNTAGGED_ENA_SET(1) |
> > +                    ANA_DROP_CFG_DROP_PRIO_S_TAGGED_ENA_SET(1) |
> > +                    ANA_DROP_CFG_DROP_PRIO_C_TAGGED_ENA_SET(1);
> > +
> > +     lan_wr(val, lan966x, ANA_DROP_CFG(port->chip_port));
> > +
> > +     /* Egress configuration (REW_TAG_CFG): VLAN tag type to 8021Q */
> > +     val = REW_TAG_CFG_TAG_TPID_CFG_SET(0);
> > +     if (port->vlan_aware) {
> > +             if (port->vid)
> > +                     /* Tag all frames except when VID == DEFAULT_VLAN */
> > +                     val |= REW_TAG_CFG_TAG_CFG_SET(1);
> > +             else
> > +                     val |= REW_TAG_CFG_TAG_CFG_SET(3);
> > +     }
> > +
> > +     /* Update only some bits in the register */
> > +     lan_rmw(val,
> > +             REW_TAG_CFG_TAG_TPID_CFG | REW_TAG_CFG_TAG_CFG,
> > +             lan966x, REW_TAG_CFG(port->chip_port));
> > +
> > +     /* Set default VLAN and tag type to 8021Q */
> > +     lan_rmw(REW_PORT_VLAN_CFG_PORT_TPID_SET(ETH_P_8021Q) |
> > +             REW_PORT_VLAN_CFG_PORT_VID_SET(port->vid),
> > +             REW_PORT_VLAN_CFG_PORT_TPID |
> > +             REW_PORT_VLAN_CFG_PORT_VID,
> > +             lan966x, REW_PORT_VLAN_CFG(port->chip_port));
> > +}
> > +
> > +int lan966x_vlan_port_add_vlan(struct lan966x_port *port,
> > +                            u16 vid,
> > +                            bool pvid,
> > +                            bool untagged)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +
> > +     /* If the CPU(br) is already part of the vlan then add the MAC
> > +      * address of the device in MAC table to copy the frames to the
> > +      * CPU(br). If the CPU(br) is not part of the vlan then it would
> > +      * just drop the frames.
> > +      */
> > +     if (lan966x_vlan_cpu_member_cpu_vlan_mask(lan966x, vid)) {
> > +             lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, vid);
> > +             lan966x_mac_cpu_learn(lan966x, lan966x->bridge->dev_addr, vid);
> 
> Doesn't the bridge notify you of all the addresses you need to learn on
> the CPU port?

Yes it does so I don't need these lan966x_mac_cpu_learn/forget here and
in the other places in this file.

> What is the benefit of the added complexity of only
> learning the addresses when the CPU joins the VLAN? 

If we add an entry MAC table regardless if the CPU is in that vlan, then if
there are any trunk ports then, we need to add an entry in MAC table for each
vlan. That is the reason why to add the entries in MAC table only if the
CPU is in the vlan.

> Doesn't the CPU_DIS bit work if an entry is present in the MAC table?

Yes it works.

> 
> > +             lan966x_vlan_cpu_add_vlan_mask(lan966x, vid);
> > +     }
> > +
> > +     lan966x_vlan_port_set_vid(port, vid, pvid, untagged);
> > +     lan966x_vlan_port_add_vlan_mask(port, vid);
> > +     lan966x_vlan_port_apply(port);
> > +
> > +     return 0;
> > +}
> > +
> > +int lan966x_vlan_port_del_vlan(struct lan966x_port *port,
> > +                            u16 vid)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +
> > +     /* In case the CPU(br) is part of the vlan then remove the MAC entry
> > +      * because frame doesn't need to reach to CPU
> > +      */
> > +     if (lan966x_vlan_cpu_member_cpu_vlan_mask(lan966x, vid))
> > +             lan966x_mac_cpu_forget(lan966x, port->dev->dev_addr, vid);
> > +
> > +     lan966x_vlan_port_remove_vid(port, vid);
> > +     lan966x_vlan_port_del_vlan_mask(port, vid);
> > +     lan966x_vlan_port_apply(port);
> > +
> > +     /* In case there are no other ports in vlan then remove the CPU from
> > +      * that vlan but still keep it in the mask because it may be needed
> > +      * again then another port gets added in tha vlan
> 
> s/tha/that/
> 
> > +      */
> > +     if (!lan966x_vlan_port_any_vlan_mask(lan966x, vid)) {
> > +             lan966x_mac_cpu_forget(lan966x, lan966x->bridge->dev_addr, vid);
> > +             lan966x_vlan_cpu_del_vlan_mask(lan966x, vid);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +int lan966x_vlan_cpu_add_vlan(struct lan966x *lan966x,
> > +                           struct net_device *dev,
> > +                           u16 vid)
> > +{
> > +     int p;
> > +
> > +     /* Iterate over the ports and see which ones are part of the
> > +      * vlan and for those ports add entry in the MAC table to
> > +      * copy the frames to the CPU
> > +      */
> > +     for (p = 0; p < lan966x->num_phys_ports; p++) {
> > +             struct lan966x_port *port = lan966x->ports[p];
> > +
> > +             if (!port ||
> > +                 !lan966x_vlan_port_member_vlan_mask(port, vid))
> > +                     continue;
> > +
> > +             lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, vid);
> > +     }
> > +
> > +     /* Add an entry in the MAC table for the CPU
> > +      * Add the CPU part of the vlan only if there is another port in that
> > +      * vlan otherwise all the broadcast frames in that vlan will go to CPU
> > +      * even if none of the ports are in the vlan and then the CPU will just
> > +      * need to discard these frames. It is required to store this
> > +      * information so when a front port is added then it would add also the
> > +      * CPU port.
> > +      */
> > +     if (lan966x_vlan_port_any_vlan_mask(lan966x, vid)) {
> > +             lan966x_mac_cpu_learn(lan966x, dev->dev_addr, vid);
> > +             lan966x_vlan_cpu_add_vlan_mask(lan966x, vid);
> > +     }
> > +
> > +     lan966x_vlan_cpu_add_cpu_vlan_mask(lan966x, vid);
> > +
> > +     return 0;
> > +}
> > +
> > +int lan966x_vlan_cpu_del_vlan(struct lan966x *lan966x,
> > +                           struct net_device *dev,
> > +                           u16 vid)
> > +{
> > +     int p;
> > +
> > +     /* Iterate over the ports and see which ones are part of the
> > +      * vlan and for those ports remove entry in the MAC table to
> > +      * copy the frames to the CPU
> > +      */
> > +     for (p = 0; p < lan966x->num_phys_ports; p++) {
> > +             struct lan966x_port *port = lan966x->ports[p];
> > +
> > +             if (!port ||
> > +                 !lan966x_vlan_port_member_vlan_mask(port, vid))
> > +                     continue;
> > +
> > +             lan966x_mac_cpu_forget(lan966x, port->dev->dev_addr, vid);
> > +     }
> > +
> > +     /* Remove an entry in the MAC table for the CPU */
> > +     lan966x_mac_cpu_forget(lan966x, dev->dev_addr, vid);
> > +
> > +     /* Remove the CPU part of the vlan */
> > +     lan966x_vlan_cpu_del_cpu_vlan_mask(lan966x, vid);
> > +     lan966x_vlan_cpu_del_vlan_mask(lan966x, vid);
> > +
> > +     return 0;
> > +}
> > +
> > +int lan966x_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
> > +{
> > +     struct lan966x_port *port = netdev_priv(dev);
> > +
> > +     lan966x_vlan_port_set_vid(port, vid, false, false);
> > +     lan966x_vlan_port_add_vlan_mask(port, vid);
> > +     lan966x_vlan_port_apply(port);
> > +
> > +     return 0;
> > +}
> > +
> > +int lan966x_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
> > +                          u16 vid)
> > +{
> > +     struct lan966x_port *port = netdev_priv(dev);
> > +
> > +     lan966x_vlan_port_remove_vid(port, vid);
> > +     lan966x_vlan_port_del_vlan_mask(port, vid);
> > +     lan966x_vlan_port_apply(port);
> > +
> > +     return 0;
> > +}
> > +
> > +void lan966x_vlan_mode(struct lan966x_port *port,
> > +                    netdev_features_t features)
> > +{
> > +     struct lan966x *lan966x = port->lan966x;
> > +     u32 val;
> > +
> > +     /* Filtering */
> > +     val = lan_rd(lan966x, ANA_VLANMASK);
> > +     if (features & NETIF_F_HW_VLAN_CTAG_FILTER)
> > +             val |= BIT(port->chip_port);
> > +     else
> > +             val &= ~BIT(port->chip_port);
> > +     lan_wr(val, lan966x, ANA_VLANMASK);
> > +}
> > --
> > 2.33.0
> >

-- 
/Horatiu