[PATCH net-next 0/4] Add Multicast Filtering support for VLAN interface

[PATCH net-next 0/4] Add Multicast Filtering support for VLAN interface

[MD Danish Anwar]

This series adds Multicast filtering support for VLAN interfaces in dual
EMAC and HSR offload mode for ICSSG driver.

Patch 1/4 - Selects symbol HSR for TI_ICSSG_PRUETH. Changes NET_SWITCHDEV
from 'depends on' to 'select' as keeping it as 'depends on' results in
below error

  *** Default configuration is based on 'defconfig'
  error: recursive dependency detected!
    symbol NET_DSA depends on HSR
    symbol HSR is selected by TI_ICSSG_PRUETH
    symbol TI_ICSSG_PRUETH depends on NET_SWITCHDEV
    symbol NET_SWITCHDEV is selected by NET_DSA
  For a resolution refer to Documentation/kbuild/kconfig-language.rst
  subsection "Kconfig recursive dependency limitations"

Patch 2/4 - Adds support for VLAN in dual EMAC mode
Patch 3/4 - Adds MC filtering support for VLAN in dual EMAC mode
Patch 4/4 - Adds MC filtering support for VLAN in HSR mode

MD Danish Anwar (4):
  net: ti: Kconfig: Select HSR for ICSSG Driver
  net: ti: icssg-prueth: Add VLAN support in EMAC mode
  net: ti: icssg-prueth: Add Multicast Filtering support for VLAN in MAC
    mode
  net: ti: icssg-prueth: Add Support for Multicast filtering with VLAN
    in HSR mode

 drivers/net/ethernet/ti/Kconfig              |   3 +-
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 170 ++++++++++++++-----
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |   8 +
 include/linux/if_hsr.h                       |  16 ++
 include/linux/netdevice.h                    |   3 +
 net/core/dev_addr_lists.c                    |   7 +-
 net/hsr/hsr_device.c                         |  12 ++
 net/hsr/hsr_main.h                           |   9 -
 8 files changed, 168 insertions(+), 60 deletions(-)


base-commit: 92c932b9946c1e082406aa0515916adb3e662e24
-- 
2.34.1

[PATCH net-next 1/4] net: ti: Kconfig: Select HSR for ICSSG Driver

[MD Danish Anwar]

HSR offloading is supported by ICSSG driver. Select the symbol HSR for
TI_ICSSG_PRUETH. Also select NET_SWITCHDEV instead of depending on it to
remove recursive dependency.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
index 0d5a862cd78a..ad366abfa746 100644
--- a/drivers/net/ethernet/ti/Kconfig
+++ b/drivers/net/ethernet/ti/Kconfig
@@ -187,8 +187,9 @@ config TI_ICSSG_PRUETH
 	select PHYLIB
 	select TI_ICSS_IEP
 	select TI_K3_CPPI_DESC_POOL
+	select NET_SWITCHDEV
+	select HSR
 	depends on PRU_REMOTEPROC
-	depends on NET_SWITCHDEV
 	depends on ARCH_K3 && OF && TI_K3_UDMA_GLUE_LAYER
 	depends on PTP_1588_CLOCK_OPTIONAL
 	help
-- 
2.34.1

[PATCH net-next 2/4] net: ti: icssg-prueth: Add VLAN support in EMAC mode

[MD Danish Anwar]

Add support for vlan filtering in dual EMAC mode.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 29 +++++++++-----------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index c568c84a032b..e031bccf31dc 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -822,19 +822,18 @@ static int emac_ndo_vlan_rx_add_vid(struct net_device *ndev,
 {
 	struct prueth_emac *emac = netdev_priv(ndev);
 	struct prueth *prueth = emac->prueth;
+	int port_mask = BIT(emac->port_id);
 	int untag_mask = 0;
-	int port_mask;
 
-	if (prueth->is_hsr_offload_mode) {
-		port_mask = BIT(PRUETH_PORT_HOST) | BIT(emac->port_id);
-		untag_mask = 0;
+	if (prueth->is_hsr_offload_mode)
+		port_mask |= BIT(PRUETH_PORT_HOST);
 
-		netdev_dbg(emac->ndev, "VID add vid:%u port_mask:%X untag_mask %X\n",
-			   vid, port_mask, untag_mask);
+	netdev_err(emac->ndev, "VID add vid:%u port_mask:%X untag_mask %X\n",
+		   vid, port_mask, untag_mask);
+
+	icssg_vtbl_modify(emac, vid, port_mask, untag_mask, true);
+	icssg_set_pvid(emac->prueth, vid, emac->port_id);
 
-		icssg_vtbl_modify(emac, vid, port_mask, untag_mask, true);
-		icssg_set_pvid(emac->prueth, vid, emac->port_id);
-	}
 	return 0;
 }
 
@@ -843,18 +842,16 @@ static int emac_ndo_vlan_rx_del_vid(struct net_device *ndev,
 {
 	struct prueth_emac *emac = netdev_priv(ndev);
 	struct prueth *prueth = emac->prueth;
+	int port_mask = BIT(emac->port_id);
 	int untag_mask = 0;
-	int port_mask;
 
-	if (prueth->is_hsr_offload_mode) {
+	if (prueth->is_hsr_offload_mode)
 		port_mask = BIT(PRUETH_PORT_HOST);
-		untag_mask = 0;
 
-		netdev_dbg(emac->ndev, "VID del vid:%u port_mask:%X untag_mask  %X\n",
-			   vid, port_mask, untag_mask);
+	netdev_err(emac->ndev, "VID del vid:%u port_mask:%X untag_mask  %X\n",
+		   vid, port_mask, untag_mask);
+	icssg_vtbl_modify(emac, vid, port_mask, untag_mask, false);
 
-		icssg_vtbl_modify(emac, vid, port_mask, untag_mask, false);
-	}
 	return 0;
 }
 
-- 
2.34.1

[PATCH net-next 3/4] net: ti: icssg-prueth: Add Multicast Filtering support for VLAN in MAC mode

[MD Danish Anwar]

Add multicast filtering support for VLAN interfaces in dual EMAC mode
for ICSSG driver.

The driver uses vlan_for_each() API to get the list of available
vlans. The driver then sync mc addr of vlan interface with a locally
mainatined list emac->vlan_mcast_list[vid] using __hw_addr_sync_multiple()
API.

The driver then calls the sync / unsync callbacks and based on whether
the ndev is vlan or not, driver passes appropriate vid to FDB helper
functions.

This commit also exports __hw_addr_sync_multiple() in order to use it
from the ICSSG driver.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 69 ++++++++++++++++----
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  6 ++
 include/linux/netdevice.h                    |  3 +
 net/core/dev_addr_lists.c                    |  7 +-
 4 files changed, 68 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index e031bccf31dc..a18773ef6eab 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -472,30 +472,43 @@ const struct icss_iep_clockops prueth_iep_clockops = {
 
 static int icssg_prueth_add_mcast(struct net_device *ndev, const u8 *addr)
 {
-	struct prueth_emac *emac = netdev_priv(ndev);
-	int port_mask = BIT(emac->port_id);
+	struct net_device *real_dev;
+	struct prueth_emac *emac;
+	int port_mask;
+	u8 vlan_id;
 
-	port_mask |= icssg_fdb_lookup(emac, addr, 0);
-	icssg_fdb_add_del(emac, addr, 0, port_mask, true);
-	icssg_vtbl_modify(emac, 0, port_mask, port_mask, true);
+	vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) : PRUETH_DFLT_VLAN_MAC;
+	real_dev = is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
+	emac = netdev_priv(real_dev);
+
+	port_mask = BIT(emac->port_id) | icssg_fdb_lookup(emac, addr, vlan_id);
+	icssg_fdb_add_del(emac, addr, vlan_id, port_mask, true);
+	icssg_vtbl_modify(emac, vlan_id, port_mask, port_mask, true);
 
 	return 0;
 }
 
 static int icssg_prueth_del_mcast(struct net_device *ndev, const u8 *addr)
 {
-	struct prueth_emac *emac = netdev_priv(ndev);
-	int port_mask = BIT(emac->port_id);
+	struct net_device *real_dev;
+	struct prueth_emac *emac;
 	int other_port_mask;
+	int port_mask;
+	u8 vlan_id;
+
+	vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) : PRUETH_DFLT_VLAN_MAC;
+	real_dev = is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
+	emac = netdev_priv(real_dev);
 
-	other_port_mask = port_mask ^ icssg_fdb_lookup(emac, addr, 0);
+	port_mask = BIT(emac->port_id);
+	other_port_mask = port_mask ^ icssg_fdb_lookup(emac, addr, vlan_id);
 
-	icssg_fdb_add_del(emac, addr, 0, port_mask, false);
-	icssg_vtbl_modify(emac, 0, port_mask, port_mask, false);
+	icssg_fdb_add_del(emac, addr, vlan_id, port_mask, false);
+	icssg_vtbl_modify(emac, vlan_id, port_mask, port_mask, false);
 
 	if (other_port_mask) {
-		icssg_fdb_add_del(emac, addr, 0, other_port_mask, true);
-		icssg_vtbl_modify(emac, 0, other_port_mask, other_port_mask, true);
+		icssg_fdb_add_del(emac, addr, vlan_id, other_port_mask, true);
+		icssg_vtbl_modify(emac, vlan_id, other_port_mask, other_port_mask, true);
 	}
 
 	return 0;
@@ -531,6 +544,28 @@ static int icssg_prueth_hsr_del_mcast(struct net_device *ndev, const u8 *addr)
 	return 0;
 }
 
+static int icssg_update_vlan_mcast(struct net_device *vdev, int vid,
+				   void *args)
+{
+	struct net_device *vport_ndev;
+	struct prueth_emac *emac;
+
+	if (!vdev || !vid)
+		return 0;
+
+	vport_ndev = vlan_dev_real_dev(vdev);
+	emac = netdev_priv(vport_ndev);
+
+	netif_addr_lock_bh(vdev);
+	__hw_addr_sync_multiple(&emac->vlan_mcast_list[vid], &vdev->mc, vdev->addr_len);
+	netif_addr_unlock_bh(vdev);
+
+	__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
+			   icssg_prueth_add_mcast, icssg_prueth_del_mcast);
+
+	return 0;
+}
+
 /**
  * emac_ndo_open - EMAC device open
  * @ndev: network adapter device
@@ -772,12 +807,17 @@ static void emac_ndo_set_rx_mode_work(struct work_struct *work)
 		return;
 	}
 
-	if (emac->prueth->is_hsr_offload_mode)
+	if (emac->prueth->is_hsr_offload_mode) {
 		__dev_mc_sync(ndev, icssg_prueth_hsr_add_mcast,
 			      icssg_prueth_hsr_del_mcast);
-	else
+	} else {
 		__dev_mc_sync(ndev, icssg_prueth_add_mcast,
 			      icssg_prueth_del_mcast);
+		if (rtnl_trylock()) {
+			vlan_for_each(ndev, icssg_update_vlan_mcast, NULL);
+			rtnl_unlock();
+		}
+	}
 }
 
 /**
@@ -828,6 +868,7 @@ static int emac_ndo_vlan_rx_add_vid(struct net_device *ndev,
 	if (prueth->is_hsr_offload_mode)
 		port_mask |= BIT(PRUETH_PORT_HOST);
 
+	__hw_addr_init(&emac->vlan_mcast_list[vid]);
 	netdev_err(emac->ndev, "VID add vid:%u port_mask:%X untag_mask %X\n",
 		   vid, port_mask, untag_mask);
 
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index f5c1d473e9f9..4da8b87408b5 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -83,6 +83,10 @@
 #define ICSS_CMD_ADD_FILTER 0x7
 #define ICSS_CMD_ADD_MAC 0x8
 
+/* VLAN Filtering Related MACROs */
+#define PRUETH_DFLT_VLAN_MAC	0
+#define MAX_VLAN_ID		256
+
 /* In switch mode there are 3 real ports i.e. 3 mac addrs.
  * however Linux sees only the host side port. The other 2 ports
  * are the switch ports.
@@ -201,6 +205,8 @@ struct prueth_emac {
 	/* RX IRQ Coalescing Related */
 	struct hrtimer rx_hrtimer;
 	unsigned long rx_pace_timeout_ns;
+
+	struct netdev_hw_addr_list vlan_mcast_list[MAX_VLAN_ID];
 };
 
 /**
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d917949bba03..a5c169b19543 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4685,6 +4685,9 @@ int devm_register_netdev(struct device *dev, struct net_device *ndev);
 /* General hardware address lists handling functions */
 int __hw_addr_sync(struct netdev_hw_addr_list *to_list,
 		   struct netdev_hw_addr_list *from_list, int addr_len);
+int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
+			    struct netdev_hw_addr_list *from_list,
+			    int addr_len);
 void __hw_addr_unsync(struct netdev_hw_addr_list *to_list,
 		      struct netdev_hw_addr_list *from_list, int addr_len);
 int __hw_addr_sync_dev(struct netdev_hw_addr_list *list,
diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index 166e404f7c03..90716bd736f3 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -242,9 +242,9 @@ static void __hw_addr_unsync_one(struct netdev_hw_addr_list *to_list,
 	__hw_addr_del_entry(from_list, ha, false, false);
 }
 
-static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
-				   struct netdev_hw_addr_list *from_list,
-				   int addr_len)
+int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
+			    struct netdev_hw_addr_list *from_list,
+			    int addr_len)
 {
 	int err = 0;
 	struct netdev_hw_addr *ha, *tmp;
@@ -260,6 +260,7 @@ static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
 	}
 	return err;
 }
+EXPORT_SYMBOL(__hw_addr_sync_multiple);
 
 /* This function only works where there is a strict 1-1 relationship
  * between source and destination of they synch. If you ever need to
-- 
2.34.1

[PATCH net-next 4/4] net: ti: icssg-prueth: Add Support for Multicast filtering with VLAN in HSR mode

[MD Danish Anwar]

Add multicast filtering support for VLAN interfaces in HSR offload mode
for ICSSG driver.

The driver calls vlan_for_each() API on the hsr device's ndev to get the
list of available vlans for the hsr device. The driver then sync mc addr of
vlan interface with a locally mainatined list emac->vlan_mcast_list[vid]
using __hw_addr_sync_multiple() API.

The driver then calls the sync / unsync callbacks.

In the sync / unsync call back, driver checks if the vdev's real dev is
hsr device or not. If the real dev is hsr device, driver gets the per
port device using hsr_get_port_ndev() and then driver passes appropriate
vid to FDB helper functions.

This commit makes below changes in the hsr files.
- Move enum hsr_port_type from net/hsr/hsr_main.h to include/linux/if_hsr.h
  so that the enum can be accessed by drivers using hsr.
- Create hsr_get_port_ndev() API that can be used to get the ndev
  pointer to the slave port from ndev pointer to the hsr net device.
- Export hsr_get_port_ndev() API so that the API can be accessed by
  drivers using hsr.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 88 ++++++++++++++------
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  2 +
 include/linux/if_hsr.h                       | 16 ++++
 net/hsr/hsr_device.c                         | 12 +++
 net/hsr/hsr_main.h                           |  9 --
 5 files changed, 93 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index a18773ef6eab..4b13787a5fa8 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -514,32 +514,64 @@ static int icssg_prueth_del_mcast(struct net_device *ndev, const u8 *addr)
 	return 0;
 }
 
-static int icssg_prueth_hsr_add_mcast(struct net_device *ndev, const u8 *addr)
+static void icssg_prueth_hsr_fdb_add_del(struct prueth_emac *emac,
+					 const u8 *addr, u8 vid, bool add)
 {
-	struct prueth_emac *emac = netdev_priv(ndev);
-	struct prueth *prueth = emac->prueth;
-
-	icssg_fdb_add_del(emac, addr, prueth->default_vlan,
+	icssg_fdb_add_del(emac, addr, vid,
 			  ICSSG_FDB_ENTRY_P0_MEMBERSHIP |
 			  ICSSG_FDB_ENTRY_P1_MEMBERSHIP |
 			  ICSSG_FDB_ENTRY_P2_MEMBERSHIP |
-			  ICSSG_FDB_ENTRY_BLOCK, true);
+			  ICSSG_FDB_ENTRY_BLOCK, add);
+
+	if (add)
+		icssg_vtbl_modify(emac, vid, BIT(emac->port_id),
+				  BIT(emac->port_id), add);
+}
+
+static int icssg_prueth_hsr_add_mcast(struct net_device *ndev, const u8 *addr)
+{
+	struct net_device *real_dev;
+	struct prueth_emac *emac;
+	u8 vlan_id, i;
+
+	vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) : PRUETH_DFLT_VLAN_HSR;
+	real_dev = is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
+
+	if (is_hsr_master(real_dev)) {
+		for (i = HSR_PT_SLAVE_A; i < HSR_PT_INTERLINK; i++) {
+			emac = netdev_priv(hsr_get_port_ndev(real_dev, i));
+			if (!emac)
+				return -EINVAL;
+			icssg_prueth_hsr_fdb_add_del(emac, addr, vlan_id, true);
+		}
+	} else {
+		emac = netdev_priv(real_dev);
+		icssg_prueth_hsr_fdb_add_del(emac, addr, vlan_id, true);
+	}
 
-	icssg_vtbl_modify(emac, emac->port_vlan, BIT(emac->port_id),
-			  BIT(emac->port_id), true);
 	return 0;
 }
 
 static int icssg_prueth_hsr_del_mcast(struct net_device *ndev, const u8 *addr)
 {
-	struct prueth_emac *emac = netdev_priv(ndev);
-	struct prueth *prueth = emac->prueth;
+	struct net_device *real_dev;
+	struct prueth_emac *emac;
+	u8 vlan_id, i;
 
-	icssg_fdb_add_del(emac, addr, prueth->default_vlan,
-			  ICSSG_FDB_ENTRY_P0_MEMBERSHIP |
-			  ICSSG_FDB_ENTRY_P1_MEMBERSHIP |
-			  ICSSG_FDB_ENTRY_P2_MEMBERSHIP |
-			  ICSSG_FDB_ENTRY_BLOCK, false);
+	vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) : PRUETH_DFLT_VLAN_HSR;
+	real_dev = is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
+
+	if (is_hsr_master(real_dev)) {
+		for (i = HSR_PT_SLAVE_A; i < HSR_PT_INTERLINK; i++) {
+			emac = netdev_priv(hsr_get_port_ndev(real_dev, i));
+			if (!emac)
+				return -EINVAL;
+			icssg_prueth_hsr_fdb_add_del(emac, addr, vlan_id, false);
+		}
+	} else {
+		emac = netdev_priv(real_dev);
+		icssg_prueth_hsr_fdb_add_del(emac, addr, vlan_id, false);
+	}
 
 	return 0;
 }
@@ -547,21 +579,23 @@ static int icssg_prueth_hsr_del_mcast(struct net_device *ndev, const u8 *addr)
 static int icssg_update_vlan_mcast(struct net_device *vdev, int vid,
 				   void *args)
 {
-	struct net_device *vport_ndev;
-	struct prueth_emac *emac;
+	struct prueth_emac *emac = args;
 
 	if (!vdev || !vid)
 		return 0;
 
-	vport_ndev = vlan_dev_real_dev(vdev);
-	emac = netdev_priv(vport_ndev);
-
 	netif_addr_lock_bh(vdev);
 	__hw_addr_sync_multiple(&emac->vlan_mcast_list[vid], &vdev->mc, vdev->addr_len);
 	netif_addr_unlock_bh(vdev);
 
-	__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
-			   icssg_prueth_add_mcast, icssg_prueth_del_mcast);
+	if (emac->prueth->is_hsr_offload_mode)
+		__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
+				   icssg_prueth_hsr_add_mcast,
+				   icssg_prueth_hsr_del_mcast);
+	else
+		__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
+				   icssg_prueth_add_mcast,
+				   icssg_prueth_del_mcast);
 
 	return 0;
 }
@@ -810,11 +844,15 @@ static void emac_ndo_set_rx_mode_work(struct work_struct *work)
 	if (emac->prueth->is_hsr_offload_mode) {
 		__dev_mc_sync(ndev, icssg_prueth_hsr_add_mcast,
 			      icssg_prueth_hsr_del_mcast);
+		if (rtnl_trylock()) {
+			vlan_for_each(emac->prueth->hsr_dev, icssg_update_vlan_mcast, emac);
+			rtnl_unlock();
+		}
 	} else {
 		__dev_mc_sync(ndev, icssg_prueth_add_mcast,
 			      icssg_prueth_del_mcast);
 		if (rtnl_trylock()) {
-			vlan_for_each(ndev, icssg_update_vlan_mcast, NULL);
+			vlan_for_each(ndev, icssg_update_vlan_mcast, emac);
 			rtnl_unlock();
 		}
 	}
@@ -1196,7 +1234,7 @@ static int prueth_netdevice_port_link(struct net_device *ndev,
 		if (prueth->br_members & BIT(PRUETH_PORT_MII0) &&
 		    prueth->br_members & BIT(PRUETH_PORT_MII1)) {
 			prueth->is_switch_mode = true;
-			prueth->default_vlan = 1;
+			prueth->default_vlan = PRUETH_DFLT_VLAN_SW;
 			emac->port_vlan = prueth->default_vlan;
 			icssg_change_mode(prueth);
 		}
@@ -1249,7 +1287,7 @@ static int prueth_hsr_port_link(struct net_device *ndev)
 			      NETIF_PRUETH_HSR_OFFLOAD_FEATURES))
 				return -EOPNOTSUPP;
 			prueth->is_hsr_offload_mode = true;
-			prueth->default_vlan = 1;
+			prueth->default_vlan = PRUETH_DFLT_VLAN_HSR;
 			emac0->port_vlan = prueth->default_vlan;
 			emac1->port_vlan = prueth->default_vlan;
 			icssg_change_mode(prueth);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index 4da8b87408b5..956cb59d98b2 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -84,6 +84,8 @@
 #define ICSS_CMD_ADD_MAC 0x8
 
 /* VLAN Filtering Related MACROs */
+#define PRUETH_DFLT_VLAN_HSR	1
+#define PRUETH_DFLT_VLAN_SW	1
 #define PRUETH_DFLT_VLAN_MAC	0
 #define MAX_VLAN_ID		256
 
diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
index 0404f5bf4f30..d97e1d1599f0 100644
--- a/include/linux/if_hsr.h
+++ b/include/linux/if_hsr.h
@@ -13,6 +13,15 @@ enum hsr_version {
 	PRP_V1,
 };
 
+enum hsr_port_type {
+	HSR_PT_NONE = 0,	/* Must be 0, used by framereg */
+	HSR_PT_SLAVE_A,
+	HSR_PT_SLAVE_B,
+	HSR_PT_INTERLINK,
+	HSR_PT_MASTER,
+	HSR_PT_PORTS,	/* This must be the last item in the enum */
+};
+
 /* HSR Tag.
  * As defined in IEC-62439-3:2010, the HSR tag is really { ethertype = 0x88FB,
  * path, LSDU_size, sequence Nr }. But we let eth_header() create { h_dest,
@@ -32,6 +41,7 @@ struct hsr_tag {
 #if IS_ENABLED(CONFIG_HSR)
 extern bool is_hsr_master(struct net_device *dev);
 extern int hsr_get_version(struct net_device *dev, enum hsr_version *ver);
+struct net_device *hsr_get_port_ndev(struct net_device *ndev, enum hsr_port_type pt);
 #else
 static inline bool is_hsr_master(struct net_device *dev)
 {
@@ -42,6 +52,12 @@ static inline int hsr_get_version(struct net_device *dev,
 {
 	return -EINVAL;
 }
+
+static inline struct net_device *hsr_get_port_ndev(struct net_device *ndev, enum hsr_port_type pt)
+{
+	return ERR_PTR(-EINVAL);
+}
+
 #endif /* CONFIG_HSR */
 
 #endif /*_LINUX_IF_HSR_H_*/
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 03eadd6c51fd..1e1cbfd02778 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -663,6 +663,18 @@ bool is_hsr_master(struct net_device *dev)
 }
 EXPORT_SYMBOL(is_hsr_master);
 
+struct net_device *hsr_get_port_ndev(struct net_device *ndev, enum hsr_port_type pt)
+{
+	struct hsr_priv *hsr = netdev_priv(ndev);
+	struct hsr_port *port;
+
+	hsr_for_each_port(hsr, port)
+		if (port->type == pt)
+			return port->dev;
+	return NULL;
+}
+EXPORT_SYMBOL(hsr_get_port_ndev);
+
 /* Default multicast address for HSR Supervision frames */
 static const unsigned char def_multicast_addr[ETH_ALEN] __aligned(2) = {
 	0x01, 0x15, 0x4e, 0x00, 0x01, 0x00
diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
index fcfeb79bb040..db7d88c05b7f 100644
--- a/net/hsr/hsr_main.h
+++ b/net/hsr/hsr_main.h
@@ -121,15 +121,6 @@ struct hsrv1_ethhdr_sp {
 	struct hsr_sup_tag	hsr_sup;
 } __packed;
 
-enum hsr_port_type {
-	HSR_PT_NONE = 0,	/* Must be 0, used by framereg */
-	HSR_PT_SLAVE_A,
-	HSR_PT_SLAVE_B,
-	HSR_PT_INTERLINK,
-	HSR_PT_MASTER,
-	HSR_PT_PORTS,	/* This must be the last item in the enum */
-};
-
 /* PRP Redunancy Control Trailor (RCT).
  * As defined in IEC-62439-4:2012, the PRP RCT is really { sequence Nr,
  * Lan indentifier (LanId), LSDU_size and PRP_suffix = 0x88FB }.
-- 
2.34.1

Re: [PATCH net-next 1/4] net: ti: Kconfig: Select HSR for ICSSG Driver

[MD Danish Anwar]

On 18/12/24 10:29 pm, Larysa Zaremba wrote:
> On Mon, Dec 16, 2024 at 03:30:41PM +0530, MD Danish Anwar wrote:
>> HSR offloading is supported by ICSSG driver. Select the symbol HSR for
>> TI_ICSSG_PRUETH. Also select NET_SWITCHDEV instead of depending on it to
>> remove recursive dependency.
>>
> 
> 2 things:
> 1) The explanation from the cover should have been included in the commit 
>    message.

I wanted to keep the commit message brief so I provided the actual
errors in cover letter. I will add the logs here as well.

> 2) Why not `depends on HSR`?

Adding `depends on HSR` in `config TI_ICSSG_PRUETH` is not setting HSR.
I have tried below scenarios and only one of them work.

1) depends on NET_SWITCHDEV
   depends on HSR

	HSR doesn't get set in .config - `# CONFIG_HSR is not set`. Even the
CONFIG_TI_ICSSG_PRUETH also gets unset although this is set to =m in
defconfig. But keeping both as `depends on` makes CONFIG_TI_ICSSG_PRUETH
disabled.

2) select NET_SWITCHDEV
   depends on HSR

	HSR doesn't get set in .config - `# CONFIG_HSR is not set`. Even the
CONFIG_TI_ICSSG_PRUETH also gets unset although this is set to =m in
defconfig. But keeping both as `depends on` makes CONFIG_TI_ICSSG_PRUETH
disabled.

3) depends on NET_SWITCHDEV
   select HSR
	
	Results in recursive dependency

error: recursive dependency detected!
	symbol NET_DSA depends on HSR
	symbol HSR is selected by TI_ICSSG_PRUETH
	symbol TI_ICSSG_PRUETH depends on NET_SWITCHDEV
	symbol NET_SWITCHDEV is selected by NET_DSA
For a resolution refer to Documentation/kbuild/kconfig-language.rst
subsection "Kconfig recursive dependency limitations"

make[2]: *** [scripts/kconfig/Makefile:95: defconfig] Error 1
make[1]: *** [/home/danish/workspace/net-next/Makefile:733: defconfig]
Error 2
make: *** [Makefile:251: __sub-make] Error 2

4) select NET_SWITCHDEV
   select HSR

	HSR is set as `m` along with `CONFIG_TI_ICSSG_PRUETH`

CONFIG_HSR=m
CONFIG_NET_SWITCHDEV=y
CONFIG_TI_ICSSG_PRUETH=m

#4 is the only secnario where HSR gets built. That's why I sent the
patch with `select NET_SWITCHDEV` and `select HSR`

>  
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>  drivers/net/ethernet/ti/Kconfig | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
>> index 0d5a862cd78a..ad366abfa746 100644
>> --- a/drivers/net/ethernet/ti/Kconfig
>> +++ b/drivers/net/ethernet/ti/Kconfig
>> @@ -187,8 +187,9 @@ config TI_ICSSG_PRUETH
>>  	select PHYLIB
>>  	select TI_ICSS_IEP
>>  	select TI_K3_CPPI_DESC_POOL
>> +	select NET_SWITCHDEV
>> +	select HSR
>>  	depends on PRU_REMOTEPROC
>> -	depends on NET_SWITCHDEV
>>  	depends on ARCH_K3 && OF && TI_K3_UDMA_GLUE_LAYER
>>  	depends on PTP_1588_CLOCK_OPTIONAL
>>  	help
>> -- 
>> 2.34.1
>>
>>

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next 2/4] net: ti: icssg-prueth: Add VLAN support in EMAC mode

[Michal Swiatkowski]

On Mon, Dec 16, 2024 at 03:30:42PM +0530, MD Danish Anwar wrote:
> Add support for vlan filtering in dual EMAC mode.
> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 29 +++++++++-----------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index c568c84a032b..e031bccf31dc 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -822,19 +822,18 @@ static int emac_ndo_vlan_rx_add_vid(struct net_device *ndev,
>  {
>  	struct prueth_emac *emac = netdev_priv(ndev);
>  	struct prueth *prueth = emac->prueth;
> +	int port_mask = BIT(emac->port_id);
>  	int untag_mask = 0;
> -	int port_mask;
>  
> -	if (prueth->is_hsr_offload_mode) {
> -		port_mask = BIT(PRUETH_PORT_HOST) | BIT(emac->port_id);
> -		untag_mask = 0;
> +	if (prueth->is_hsr_offload_mode)
> +		port_mask |= BIT(PRUETH_PORT_HOST);
>  
> -		netdev_dbg(emac->ndev, "VID add vid:%u port_mask:%X untag_mask %X\n",
> -			   vid, port_mask, untag_mask);
> +	netdev_err(emac->ndev, "VID add vid:%u port_mask:%X untag_mask %X\n",
> +		   vid, port_mask, untag_mask);
> +
> +	icssg_vtbl_modify(emac, vid, port_mask, untag_mask, true);
> +	icssg_set_pvid(emac->prueth, vid, emac->port_id);
>  
> -		icssg_vtbl_modify(emac, vid, port_mask, untag_mask, true);
> -		icssg_set_pvid(emac->prueth, vid, emac->port_id);
> -	}
>  	return 0;
>  }
>  
> @@ -843,18 +842,16 @@ static int emac_ndo_vlan_rx_del_vid(struct net_device *ndev,
>  {
>  	struct prueth_emac *emac = netdev_priv(ndev);
>  	struct prueth *prueth = emac->prueth;
> +	int port_mask = BIT(emac->port_id);
>  	int untag_mask = 0;
> -	int port_mask;
>  
> -	if (prueth->is_hsr_offload_mode) {
> +	if (prueth->is_hsr_offload_mode)
>  		port_mask = BIT(PRUETH_PORT_HOST);
> -		untag_mask = 0;
>  
> -		netdev_dbg(emac->ndev, "VID del vid:%u port_mask:%X untag_mask  %X\n",
> -			   vid, port_mask, untag_mask);
> +	netdev_err(emac->ndev, "VID del vid:%u port_mask:%X untag_mask  %X\n",
> +		   vid, port_mask, untag_mask);
Why error? It doesn't look like error path, previously there was
netdev_dbg (made more sense in my opinion)

> +	icssg_vtbl_modify(emac, vid, port_mask, untag_mask, false);
>  
> -		icssg_vtbl_modify(emac, vid, port_mask, untag_mask, false);
> -	}
>  	return 0;
>  }
>  
> -- 
> 2.34.1

Re: [PATCH net-next 3/4] net: ti: icssg-prueth: Add Multicast Filtering support for VLAN in MAC mode

[Michal Swiatkowski]

On Mon, Dec 16, 2024 at 03:30:43PM +0530, MD Danish Anwar wrote:
> Add multicast filtering support for VLAN interfaces in dual EMAC mode
> for ICSSG driver.
> 
> The driver uses vlan_for_each() API to get the list of available
> vlans. The driver then sync mc addr of vlan interface with a locally
> mainatined list emac->vlan_mcast_list[vid] using __hw_addr_sync_multiple()
> API.
> 
> The driver then calls the sync / unsync callbacks and based on whether
> the ndev is vlan or not, driver passes appropriate vid to FDB helper
> functions.
> 
> This commit also exports __hw_addr_sync_multiple() in order to use it
> from the ICSSG driver.
> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 69 ++++++++++++++++----
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  6 ++
>  include/linux/netdevice.h                    |  3 +
>  net/core/dev_addr_lists.c                    |  7 +-
>  4 files changed, 68 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index e031bccf31dc..a18773ef6eab 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -472,30 +472,43 @@ const struct icss_iep_clockops prueth_iep_clockops = {
>  
>  static int icssg_prueth_add_mcast(struct net_device *ndev, const u8 *addr)
>  {
> -	struct prueth_emac *emac = netdev_priv(ndev);
> -	int port_mask = BIT(emac->port_id);
> +	struct net_device *real_dev;
> +	struct prueth_emac *emac;
> +	int port_mask;
> +	u8 vlan_id;
>  
> -	port_mask |= icssg_fdb_lookup(emac, addr, 0);
> -	icssg_fdb_add_del(emac, addr, 0, port_mask, true);
> -	icssg_vtbl_modify(emac, 0, port_mask, port_mask, true);
> +	vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) : PRUETH_DFLT_VLAN_MAC;
> +	real_dev = is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
Looks like a helper for that can be nice to have.

> +	emac = netdev_priv(real_dev);
> +
> +	port_mask = BIT(emac->port_id) | icssg_fdb_lookup(emac, addr, vlan_id);
> +	icssg_fdb_add_del(emac, addr, vlan_id, port_mask, true);
> +	icssg_vtbl_modify(emac, vlan_id, port_mask, port_mask, true);
>  
>  	return 0;
>  }
>  
>  static int icssg_prueth_del_mcast(struct net_device *ndev, const u8 *addr)
>  {
> -	struct prueth_emac *emac = netdev_priv(ndev);
> -	int port_mask = BIT(emac->port_id);
> +	struct net_device *real_dev;
> +	struct prueth_emac *emac;
>  	int other_port_mask;
> +	int port_mask;
> +	u8 vlan_id;
> +
> +	vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) : PRUETH_DFLT_VLAN_MAC;
> +	real_dev = is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
> +	emac = netdev_priv(real_dev);
>  
> -	other_port_mask = port_mask ^ icssg_fdb_lookup(emac, addr, 0);
> +	port_mask = BIT(emac->port_id);
> +	other_port_mask = port_mask ^ icssg_fdb_lookup(emac, addr, vlan_id);
>  
> -	icssg_fdb_add_del(emac, addr, 0, port_mask, false);
> -	icssg_vtbl_modify(emac, 0, port_mask, port_mask, false);
> +	icssg_fdb_add_del(emac, addr, vlan_id, port_mask, false);
> +	icssg_vtbl_modify(emac, vlan_id, port_mask, port_mask, false);
>  
>  	if (other_port_mask) {
> -		icssg_fdb_add_del(emac, addr, 0, other_port_mask, true);
> -		icssg_vtbl_modify(emac, 0, other_port_mask, other_port_mask, true);
> +		icssg_fdb_add_del(emac, addr, vlan_id, other_port_mask, true);
> +		icssg_vtbl_modify(emac, vlan_id, other_port_mask, other_port_mask, true);
>  	}
>  
>  	return 0;
> @@ -531,6 +544,28 @@ static int icssg_prueth_hsr_del_mcast(struct net_device *ndev, const u8 *addr)
>  	return 0;
>  }
>  
> +static int icssg_update_vlan_mcast(struct net_device *vdev, int vid,
> +				   void *args)
> +{
> +	struct net_device *vport_ndev;
> +	struct prueth_emac *emac;
> +
> +	if (!vdev || !vid)
> +		return 0;
> +
> +	vport_ndev = vlan_dev_real_dev(vdev);
> +	emac = netdev_priv(vport_ndev);
> +
> +	netif_addr_lock_bh(vdev);
> +	__hw_addr_sync_multiple(&emac->vlan_mcast_list[vid], &vdev->mc, vdev->addr_len);
Only question, why dev_mc_sync_multiple can't be used here?

> +	netif_addr_unlock_bh(vdev);
> +
> +	__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
> +			   icssg_prueth_add_mcast, icssg_prueth_del_mcast);
> +
> +	return 0;
> +}
> +
>  /**
>   * emac_ndo_open - EMAC device open
>   * @ndev: network adapter device
> @@ -772,12 +807,17 @@ static void emac_ndo_set_rx_mode_work(struct work_struct *work)
>  		return;
>  	}
>  
> -	if (emac->prueth->is_hsr_offload_mode)
> +	if (emac->prueth->is_hsr_offload_mode) {
>  		__dev_mc_sync(ndev, icssg_prueth_hsr_add_mcast,
>  			      icssg_prueth_hsr_del_mcast);
> -	else
> +	} else {
>  		__dev_mc_sync(ndev, icssg_prueth_add_mcast,
>  			      icssg_prueth_del_mcast);
> +		if (rtnl_trylock()) {
> +			vlan_for_each(ndev, icssg_update_vlan_mcast, NULL);
> +			rtnl_unlock();
> +		}
> +	}
>  }
>  
>  /**
> @@ -828,6 +868,7 @@ static int emac_ndo_vlan_rx_add_vid(struct net_device *ndev,
>  	if (prueth->is_hsr_offload_mode)
>  		port_mask |= BIT(PRUETH_PORT_HOST);
>  
> +	__hw_addr_init(&emac->vlan_mcast_list[vid]);
>  	netdev_err(emac->ndev, "VID add vid:%u port_mask:%X untag_mask %X\n",
>  		   vid, port_mask, untag_mask);
>  
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> index f5c1d473e9f9..4da8b87408b5 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -83,6 +83,10 @@
>  #define ICSS_CMD_ADD_FILTER 0x7
>  #define ICSS_CMD_ADD_MAC 0x8
>  
> +/* VLAN Filtering Related MACROs */
> +#define PRUETH_DFLT_VLAN_MAC	0
> +#define MAX_VLAN_ID		256
> +
>  /* In switch mode there are 3 real ports i.e. 3 mac addrs.
>   * however Linux sees only the host side port. The other 2 ports
>   * are the switch ports.
> @@ -201,6 +205,8 @@ struct prueth_emac {
>  	/* RX IRQ Coalescing Related */
>  	struct hrtimer rx_hrtimer;
>  	unsigned long rx_pace_timeout_ns;
> +
> +	struct netdev_hw_addr_list vlan_mcast_list[MAX_VLAN_ID];
>  };
>  
>  /**
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index d917949bba03..a5c169b19543 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4685,6 +4685,9 @@ int devm_register_netdev(struct device *dev, struct net_device *ndev);
>  /* General hardware address lists handling functions */
>  int __hw_addr_sync(struct netdev_hw_addr_list *to_list,
>  		   struct netdev_hw_addr_list *from_list, int addr_len);
> +int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
> +			    struct netdev_hw_addr_list *from_list,
> +			    int addr_len);
>  void __hw_addr_unsync(struct netdev_hw_addr_list *to_list,
>  		      struct netdev_hw_addr_list *from_list, int addr_len);
>  int __hw_addr_sync_dev(struct netdev_hw_addr_list *list,
> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index 166e404f7c03..90716bd736f3 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
> @@ -242,9 +242,9 @@ static void __hw_addr_unsync_one(struct netdev_hw_addr_list *to_list,
>  	__hw_addr_del_entry(from_list, ha, false, false);
>  }
>  
> -static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
> -				   struct netdev_hw_addr_list *from_list,
> -				   int addr_len)
> +int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
> +			    struct netdev_hw_addr_list *from_list,
> +			    int addr_len)
>  {
>  	int err = 0;
>  	struct netdev_hw_addr *ha, *tmp;
> @@ -260,6 +260,7 @@ static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
>  	}
>  	return err;
>  }
> +EXPORT_SYMBOL(__hw_addr_sync_multiple);
>  
>  /* This function only works where there is a strict 1-1 relationship
>   * between source and destination of they synch. If you ever need to
> -- 
> 2.34.1

Re: [PATCH net-next 2/4] net: ti: icssg-prueth: Add VLAN support in EMAC mode

[MD Danish Anwar]

On 19/12/24 12:32 pm, Michal Swiatkowski wrote:
> On Mon, Dec 16, 2024 at 03:30:42PM +0530, MD Danish Anwar wrote:
>> Add support for vlan filtering in dual EMAC mode.
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 29 +++++++++-----------
>>  1 file changed, 13 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index c568c84a032b..e031bccf31dc 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -822,19 +822,18 @@ static int emac_ndo_vlan_rx_add_vid(struct net_device *ndev,
>>  {
>>  	struct prueth_emac *emac = netdev_priv(ndev);
>>  	struct prueth *prueth = emac->prueth;
>> +	int port_mask = BIT(emac->port_id);
>>  	int untag_mask = 0;
>> -	int port_mask;
>>  
>> -	if (prueth->is_hsr_offload_mode) {
>> -		port_mask = BIT(PRUETH_PORT_HOST) | BIT(emac->port_id);
>> -		untag_mask = 0;
>> +	if (prueth->is_hsr_offload_mode)
>> +		port_mask |= BIT(PRUETH_PORT_HOST);
>>  
>> -		netdev_dbg(emac->ndev, "VID add vid:%u port_mask:%X untag_mask %X\n",
>> -			   vid, port_mask, untag_mask);
>> +	netdev_err(emac->ndev, "VID add vid:%u port_mask:%X untag_mask %X\n",
>> +		   vid, port_mask, untag_mask);
>> +
>> +	icssg_vtbl_modify(emac, vid, port_mask, untag_mask, true);
>> +	icssg_set_pvid(emac->prueth, vid, emac->port_id);
>>  
>> -		icssg_vtbl_modify(emac, vid, port_mask, untag_mask, true);
>> -		icssg_set_pvid(emac->prueth, vid, emac->port_id);
>> -	}
>>  	return 0;
>>  }
>>  
>> @@ -843,18 +842,16 @@ static int emac_ndo_vlan_rx_del_vid(struct net_device *ndev,
>>  {
>>  	struct prueth_emac *emac = netdev_priv(ndev);
>>  	struct prueth *prueth = emac->prueth;
>> +	int port_mask = BIT(emac->port_id);
>>  	int untag_mask = 0;
>> -	int port_mask;
>>  
>> -	if (prueth->is_hsr_offload_mode) {
>> +	if (prueth->is_hsr_offload_mode)
>>  		port_mask = BIT(PRUETH_PORT_HOST);
>> -		untag_mask = 0;
>>  
>> -		netdev_dbg(emac->ndev, "VID del vid:%u port_mask:%X untag_mask  %X\n",
>> -			   vid, port_mask, untag_mask);
>> +	netdev_err(emac->ndev, "VID del vid:%u port_mask:%X untag_mask  %X\n",
>> +		   vid, port_mask, untag_mask);
> Why error? It doesn't look like error path, previously there was
> netdev_dbg (made more sense in my opinion)
> 

My bad. It should be netdev_dbg(). I'll change it in v2. Thanks

>> +	icssg_vtbl_modify(emac, vid, port_mask, untag_mask, false);
>>  
>> -		icssg_vtbl_modify(emac, vid, port_mask, untag_mask, false);
>> -	}
>>  	return 0;
>>  }
>>  
>> -- 
>> 2.34.1

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next 3/4] net: ti: icssg-prueth: Add Multicast Filtering support for VLAN in MAC mode

[MD Danish Anwar]

On 19/12/24 12:58 pm, Michal Swiatkowski wrote:
> On Mon, Dec 16, 2024 at 03:30:43PM +0530, MD Danish Anwar wrote:
>> Add multicast filtering support for VLAN interfaces in dual EMAC mode
>> for ICSSG driver.
>>
>> The driver uses vlan_for_each() API to get the list of available
>> vlans. The driver then sync mc addr of vlan interface with a locally
>> mainatined list emac->vlan_mcast_list[vid] using __hw_addr_sync_multiple()
>> API.
>>
>> The driver then calls the sync / unsync callbacks and based on whether
>> the ndev is vlan or not, driver passes appropriate vid to FDB helper
>> functions.
>>
>> This commit also exports __hw_addr_sync_multiple() in order to use it
>> from the ICSSG driver.
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 69 ++++++++++++++++----
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  6 ++
>>  include/linux/netdevice.h                    |  3 +
>>  net/core/dev_addr_lists.c                    |  7 +-
>>  4 files changed, 68 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index e031bccf31dc..a18773ef6eab 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -472,30 +472,43 @@ const struct icss_iep_clockops prueth_iep_clockops = {
>>  
>>  static int icssg_prueth_add_mcast(struct net_device *ndev, const u8 *addr)
>>  {
>> -	struct prueth_emac *emac = netdev_priv(ndev);
>> -	int port_mask = BIT(emac->port_id);
>> +	struct net_device *real_dev;
>> +	struct prueth_emac *emac;
>> +	int port_mask;
>> +	u8 vlan_id;
>>  
>> -	port_mask |= icssg_fdb_lookup(emac, addr, 0);
>> -	icssg_fdb_add_del(emac, addr, 0, port_mask, true);
>> -	icssg_vtbl_modify(emac, 0, port_mask, port_mask, true);
>> +	vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) : PRUETH_DFLT_VLAN_MAC;
>> +	real_dev = is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
> Looks like a helper for that can be nice to have.
>

I don't think that's neccessary. vlan_dev_real_dev() itself is a helper
function to give the real dev, only down side is vlan_dev_real_dev()
assumes that the dev is vlan only.

In this function, we don't know if ndev is vlan or not, hence the check.
Most drivers are using vlan_dev_real_dev() the same way.


>> +	emac = netdev_priv(real_dev);
>> +
>> +	port_mask = BIT(emac->port_id) | icssg_fdb_lookup(emac, addr, vlan_id);
>> +	icssg_fdb_add_del(emac, addr, vlan_id, port_mask, true);
>> +	icssg_vtbl_modify(emac, vlan_id, port_mask, port_mask, true);
>>  
>>  	return 0;
>>  }
>>  
>>  static int icssg_prueth_del_mcast(struct net_device *ndev, const u8 *addr)
>>  {
>> -	struct prueth_emac *emac = netdev_priv(ndev);
>> -	int port_mask = BIT(emac->port_id);
>> +	struct net_device *real_dev;
>> +	struct prueth_emac *emac;
>>  	int other_port_mask;
>> +	int port_mask;
>> +	u8 vlan_id;
>> +
>> +	vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) : PRUETH_DFLT_VLAN_MAC;
>> +	real_dev = is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
>> +	emac = netdev_priv(real_dev);
>>  
>> -	other_port_mask = port_mask ^ icssg_fdb_lookup(emac, addr, 0);
>> +	port_mask = BIT(emac->port_id);
>> +	other_port_mask = port_mask ^ icssg_fdb_lookup(emac, addr, vlan_id);
>>  
>> -	icssg_fdb_add_del(emac, addr, 0, port_mask, false);
>> -	icssg_vtbl_modify(emac, 0, port_mask, port_mask, false);
>> +	icssg_fdb_add_del(emac, addr, vlan_id, port_mask, false);
>> +	icssg_vtbl_modify(emac, vlan_id, port_mask, port_mask, false);
>>  
>>  	if (other_port_mask) {
>> -		icssg_fdb_add_del(emac, addr, 0, other_port_mask, true);
>> -		icssg_vtbl_modify(emac, 0, other_port_mask, other_port_mask, true);
>> +		icssg_fdb_add_del(emac, addr, vlan_id, other_port_mask, true);
>> +		icssg_vtbl_modify(emac, vlan_id, other_port_mask, other_port_mask, true);
>>  	}
>>  
>>  	return 0;
>> @@ -531,6 +544,28 @@ static int icssg_prueth_hsr_del_mcast(struct net_device *ndev, const u8 *addr)
>>  	return 0;
>>  }
>>  
>> +static int icssg_update_vlan_mcast(struct net_device *vdev, int vid,
>> +				   void *args)
>> +{
>> +	struct net_device *vport_ndev;
>> +	struct prueth_emac *emac;
>> +
>> +	if (!vdev || !vid)
>> +		return 0;
>> +
>> +	vport_ndev = vlan_dev_real_dev(vdev);
>> +	emac = netdev_priv(vport_ndev);
>> +
>> +	netif_addr_lock_bh(vdev);
>> +	__hw_addr_sync_multiple(&emac->vlan_mcast_list[vid], &vdev->mc, vdev->addr_len);
> Only question, why dev_mc_sync_multiple can't be used here?
> 

dev_mc_sync_multiple() doesn't work here.

Let's say we call dev_mc_sync_multiple() with emac->ndev (the netdevice
for current port say eth1) and vdev (the netdevice for the vlan
interface say eth1.x with x being the vid).

Now dev_mc_sync_multiple() will sync the mc list from vdev to ndev. And
ndev will have the address added to vdev. After this set_rx_mode() gets
called for ndev. Which eventually calls sync / unsync APIs
icssg_prueth_add/del_mcast.

Now in this API, we will have the address to add but we won't have the vid.

The sync/unsync API will be called for the emac->ndev (the *to* device)
and emac->ndev will have no information about vid. Since it is not a
vlan dev and is_vlan_dev() will be false for it as a result we will
fallback to default vid.

	vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) :
PRUETH_DFLT_VLAN_HSR;

We don't want this. We want to capture the correct vid. Hence sync /
unsync needs to get called for the vlan device i.e. vdev.

Due to this dev_mc_sync_multiple() doesn't work and we need to call
__hw_addr_sync_multiple and __hw_addr_sync_dev on vdev so that our sync
/ unsync APIs are called for vdev.


>> +	netif_addr_unlock_bh(vdev);
>> +
>> +	__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
>> +			   icssg_prueth_add_mcast, icssg_prueth_del_mcast);
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * emac_ndo_open - EMAC device open
>>   * @ndev: network adapter device
>> @@ -772,12 +807,17 @@ static void emac_ndo_set_rx_mode_work(struct work_struct *work)
>>  		return;
>>  	}
>>  
>> -	if (emac->prueth->is_hsr_offload_mode)
>> +	if (emac->prueth->is_hsr_offload_mode) {
>>  		__dev_mc_sync(ndev, icssg_prueth_hsr_add_mcast,
>>  			      icssg_prueth_hsr_del_mcast);
>> -	else
>> +	} else {
>>  		__dev_mc_sync(ndev, icssg_prueth_add_mcast,
>>  			      icssg_prueth_del_mcast);
>> +		if (rtnl_trylock()) {
>> +			vlan_for_each(ndev, icssg_update_vlan_mcast, NULL);
>> +			rtnl_unlock();
>> +		}
>> +	}
>>  }
>>  
>>  /**
>> @@ -828,6 +868,7 @@ static int emac_ndo_vlan_rx_add_vid(struct net_device *ndev,
>>  	if (prueth->is_hsr_offload_mode)
>>  		port_mask |= BIT(PRUETH_PORT_HOST);
>>  
>> +	__hw_addr_init(&emac->vlan_mcast_list[vid]);
>>  	netdev_err(emac->ndev, "VID add vid:%u port_mask:%X untag_mask %X\n",
>>  		   vid, port_mask, untag_mask);
>>  
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> index f5c1d473e9f9..4da8b87408b5 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> @@ -83,6 +83,10 @@
>>  #define ICSS_CMD_ADD_FILTER 0x7
>>  #define ICSS_CMD_ADD_MAC 0x8
>>  
>> +/* VLAN Filtering Related MACROs */
>> +#define PRUETH_DFLT_VLAN_MAC	0
>> +#define MAX_VLAN_ID		256
>> +
>>  /* In switch mode there are 3 real ports i.e. 3 mac addrs.
>>   * however Linux sees only the host side port. The other 2 ports
>>   * are the switch ports.
>> @@ -201,6 +205,8 @@ struct prueth_emac {
>>  	/* RX IRQ Coalescing Related */
>>  	struct hrtimer rx_hrtimer;
>>  	unsigned long rx_pace_timeout_ns;
>> +
>> +	struct netdev_hw_addr_list vlan_mcast_list[MAX_VLAN_ID];
>>  };
>>  
>>  /**
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index d917949bba03..a5c169b19543 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -4685,6 +4685,9 @@ int devm_register_netdev(struct device *dev, struct net_device *ndev);
>>  /* General hardware address lists handling functions */
>>  int __hw_addr_sync(struct netdev_hw_addr_list *to_list,
>>  		   struct netdev_hw_addr_list *from_list, int addr_len);
>> +int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
>> +			    struct netdev_hw_addr_list *from_list,
>> +			    int addr_len);
>>  void __hw_addr_unsync(struct netdev_hw_addr_list *to_list,
>>  		      struct netdev_hw_addr_list *from_list, int addr_len);
>>  int __hw_addr_sync_dev(struct netdev_hw_addr_list *list,
>> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
>> index 166e404f7c03..90716bd736f3 100644
>> --- a/net/core/dev_addr_lists.c
>> +++ b/net/core/dev_addr_lists.c
>> @@ -242,9 +242,9 @@ static void __hw_addr_unsync_one(struct netdev_hw_addr_list *to_list,
>>  	__hw_addr_del_entry(from_list, ha, false, false);
>>  }
>>  
>> -static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
>> -				   struct netdev_hw_addr_list *from_list,
>> -				   int addr_len)
>> +int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
>> +			    struct netdev_hw_addr_list *from_list,
>> +			    int addr_len)
>>  {
>>  	int err = 0;
>>  	struct netdev_hw_addr *ha, *tmp;
>> @@ -260,6 +260,7 @@ static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
>>  	}
>>  	return err;
>>  }
>> +EXPORT_SYMBOL(__hw_addr_sync_multiple);
>>  
>>  /* This function only works where there is a strict 1-1 relationship
>>   * between source and destination of they synch. If you ever need to
>> -- 
>> 2.34.1

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next 3/4] net: ti: icssg-prueth: Add Multicast Filtering support for VLAN in MAC mode

[Michal Swiatkowski]

On Thu, Dec 19, 2024 at 03:06:18PM +0530, MD Danish Anwar wrote:
> 
> 
> On 19/12/24 12:58 pm, Michal Swiatkowski wrote:
> > On Mon, Dec 16, 2024 at 03:30:43PM +0530, MD Danish Anwar wrote:
> >> Add multicast filtering support for VLAN interfaces in dual EMAC mode
> >> for ICSSG driver.
> >>
> >> The driver uses vlan_for_each() API to get the list of available
> >> vlans. The driver then sync mc addr of vlan interface with a locally
> >> mainatined list emac->vlan_mcast_list[vid] using __hw_addr_sync_multiple()
> >> API.
> >>
> >> The driver then calls the sync / unsync callbacks and based on whether
> >> the ndev is vlan or not, driver passes appropriate vid to FDB helper
> >> functions.
> >>
> >> This commit also exports __hw_addr_sync_multiple() in order to use it
> >> from the ICSSG driver.
> >>
> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> >> ---
> >>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 69 ++++++++++++++++----
> >>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  6 ++
> >>  include/linux/netdevice.h                    |  3 +
> >>  net/core/dev_addr_lists.c                    |  7 +-
> >>  4 files changed, 68 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> >> index e031bccf31dc..a18773ef6eab 100644
> >> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> >> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> >> @@ -472,30 +472,43 @@ const struct icss_iep_clockops prueth_iep_clockops = {
> >>  
> >>  static int icssg_prueth_add_mcast(struct net_device *ndev, const u8 *addr)
> >>  {
> >> -	struct prueth_emac *emac = netdev_priv(ndev);
> >> -	int port_mask = BIT(emac->port_id);
> >> +	struct net_device *real_dev;
> >> +	struct prueth_emac *emac;
> >> +	int port_mask;
> >> +	u8 vlan_id;
> >>  
> >> -	port_mask |= icssg_fdb_lookup(emac, addr, 0);
> >> -	icssg_fdb_add_del(emac, addr, 0, port_mask, true);
> >> -	icssg_vtbl_modify(emac, 0, port_mask, port_mask, true);
> >> +	vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) : PRUETH_DFLT_VLAN_MAC;
> >> +	real_dev = is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
> > Looks like a helper for that can be nice to have.
> >
> 
> I don't think that's neccessary. vlan_dev_real_dev() itself is a helper
> function to give the real dev, only down side is vlan_dev_real_dev()
> assumes that the dev is vlan only.
> 
> In this function, we don't know if ndev is vlan or not, hence the check.
> Most drivers are using vlan_dev_real_dev() the same way.
> 

I meant to not repeat the
is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
(it is also used in next patch)

But as you think, I don't have strong opinion on that.

> 
> >> +	emac = netdev_priv(real_dev);
> >> +
> >> +	port_mask = BIT(emac->port_id) | icssg_fdb_lookup(emac, addr, vlan_id);
> >> +	icssg_fdb_add_del(emac, addr, vlan_id, port_mask, true);
> >> +	icssg_vtbl_modify(emac, vlan_id, port_mask, port_mask, true);
> >>  
> >>  	return 0;
> >>  }
> >>  
> >>  static int icssg_prueth_del_mcast(struct net_device *ndev, const u8 *addr)
> >>  {
> >> -	struct prueth_emac *emac = netdev_priv(ndev);
> >> -	int port_mask = BIT(emac->port_id);
> >> +	struct net_device *real_dev;
> >> +	struct prueth_emac *emac;
> >>  	int other_port_mask;
> >> +	int port_mask;
> >> +	u8 vlan_id;
> >> +
> >> +	vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) : PRUETH_DFLT_VLAN_MAC;
> >> +	real_dev = is_vlan_dev(ndev) ? vlan_dev_real_dev(ndev) : ndev;
> >> +	emac = netdev_priv(real_dev);
> >>  
> >> -	other_port_mask = port_mask ^ icssg_fdb_lookup(emac, addr, 0);
> >> +	port_mask = BIT(emac->port_id);
> >> +	other_port_mask = port_mask ^ icssg_fdb_lookup(emac, addr, vlan_id);
> >>  
> >> -	icssg_fdb_add_del(emac, addr, 0, port_mask, false);
> >> -	icssg_vtbl_modify(emac, 0, port_mask, port_mask, false);
> >> +	icssg_fdb_add_del(emac, addr, vlan_id, port_mask, false);
> >> +	icssg_vtbl_modify(emac, vlan_id, port_mask, port_mask, false);
> >>  
> >>  	if (other_port_mask) {
> >> -		icssg_fdb_add_del(emac, addr, 0, other_port_mask, true);
> >> -		icssg_vtbl_modify(emac, 0, other_port_mask, other_port_mask, true);
> >> +		icssg_fdb_add_del(emac, addr, vlan_id, other_port_mask, true);
> >> +		icssg_vtbl_modify(emac, vlan_id, other_port_mask, other_port_mask, true);
> >>  	}
> >>  
> >>  	return 0;
> >> @@ -531,6 +544,28 @@ static int icssg_prueth_hsr_del_mcast(struct net_device *ndev, const u8 *addr)
> >>  	return 0;
> >>  }
> >>  
> >> +static int icssg_update_vlan_mcast(struct net_device *vdev, int vid,
> >> +				   void *args)
> >> +{
> >> +	struct net_device *vport_ndev;
> >> +	struct prueth_emac *emac;
> >> +
> >> +	if (!vdev || !vid)
> >> +		return 0;
> >> +
> >> +	vport_ndev = vlan_dev_real_dev(vdev);
> >> +	emac = netdev_priv(vport_ndev);
> >> +
> >> +	netif_addr_lock_bh(vdev);
> >> +	__hw_addr_sync_multiple(&emac->vlan_mcast_list[vid], &vdev->mc, vdev->addr_len);
> > Only question, why dev_mc_sync_multiple can't be used here?
> > 
> 
> dev_mc_sync_multiple() doesn't work here.
> 
> Let's say we call dev_mc_sync_multiple() with emac->ndev (the netdevice
> for current port say eth1) and vdev (the netdevice for the vlan
> interface say eth1.x with x being the vid).
> 
> Now dev_mc_sync_multiple() will sync the mc list from vdev to ndev. And
> ndev will have the address added to vdev. After this set_rx_mode() gets
> called for ndev. Which eventually calls sync / unsync APIs
> icssg_prueth_add/del_mcast.
> 
> Now in this API, we will have the address to add but we won't have the vid.
> 
> The sync/unsync API will be called for the emac->ndev (the *to* device)
> and emac->ndev will have no information about vid. Since it is not a
> vlan dev and is_vlan_dev() will be false for it as a result we will
> fallback to default vid.
> 
> 	vlan_id = is_vlan_dev(ndev) ? vlan_dev_vlan_id(ndev) :
> PRUETH_DFLT_VLAN_HSR;
> 
> We don't want this. We want to capture the correct vid. Hence sync /
> unsync needs to get called for the vlan device i.e. vdev.
> 
> Due to this dev_mc_sync_multiple() doesn't work and we need to call
> __hw_addr_sync_multiple and __hw_addr_sync_dev on vdev so that our sync
> / unsync APIs are called for vdev.
> 
>

Ok, I understand, thank for explanation.

> >> +	netif_addr_unlock_bh(vdev);
> >> +
> >> +	__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
> >> +			   icssg_prueth_add_mcast, icssg_prueth_del_mcast);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  /**
> >>   * emac_ndo_open - EMAC device open
> >>   * @ndev: network adapter device
> >> @@ -772,12 +807,17 @@ static void emac_ndo_set_rx_mode_work(struct work_struct *work)
> >>  		return;
> >>  	}
> >>  
> >> -	if (emac->prueth->is_hsr_offload_mode)
> >> +	if (emac->prueth->is_hsr_offload_mode) {
> >>  		__dev_mc_sync(ndev, icssg_prueth_hsr_add_mcast,
> >>  			      icssg_prueth_hsr_del_mcast);
> >> -	else
> >> +	} else {
> >>  		__dev_mc_sync(ndev, icssg_prueth_add_mcast,
> >>  			      icssg_prueth_del_mcast);
> >> +		if (rtnl_trylock()) {
> >> +			vlan_for_each(ndev, icssg_update_vlan_mcast, NULL);
> >> +			rtnl_unlock();
> >> +		}
> >> +	}
> >>  }
> >>  
> >>  /**
> >> @@ -828,6 +868,7 @@ static int emac_ndo_vlan_rx_add_vid(struct net_device *ndev,
> >>  	if (prueth->is_hsr_offload_mode)
> >>  		port_mask |= BIT(PRUETH_PORT_HOST);
> >>  
> >> +	__hw_addr_init(&emac->vlan_mcast_list[vid]);
> >>  	netdev_err(emac->ndev, "VID add vid:%u port_mask:%X untag_mask %X\n",
> >>  		   vid, port_mask, untag_mask);
> >>  
> >> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> >> index f5c1d473e9f9..4da8b87408b5 100644
> >> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> >> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> >> @@ -83,6 +83,10 @@
> >>  #define ICSS_CMD_ADD_FILTER 0x7
> >>  #define ICSS_CMD_ADD_MAC 0x8
> >>  
> >> +/* VLAN Filtering Related MACROs */
> >> +#define PRUETH_DFLT_VLAN_MAC	0
> >> +#define MAX_VLAN_ID		256
> >> +
> >>  /* In switch mode there are 3 real ports i.e. 3 mac addrs.
> >>   * however Linux sees only the host side port. The other 2 ports
> >>   * are the switch ports.
> >> @@ -201,6 +205,8 @@ struct prueth_emac {
> >>  	/* RX IRQ Coalescing Related */
> >>  	struct hrtimer rx_hrtimer;
> >>  	unsigned long rx_pace_timeout_ns;
> >> +
> >> +	struct netdev_hw_addr_list vlan_mcast_list[MAX_VLAN_ID];
> >>  };
> >>  
> >>  /**
> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> index d917949bba03..a5c169b19543 100644
> >> --- a/include/linux/netdevice.h
> >> +++ b/include/linux/netdevice.h
> >> @@ -4685,6 +4685,9 @@ int devm_register_netdev(struct device *dev, struct net_device *ndev);
> >>  /* General hardware address lists handling functions */
> >>  int __hw_addr_sync(struct netdev_hw_addr_list *to_list,
> >>  		   struct netdev_hw_addr_list *from_list, int addr_len);
> >> +int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
> >> +			    struct netdev_hw_addr_list *from_list,
> >> +			    int addr_len);
> >>  void __hw_addr_unsync(struct netdev_hw_addr_list *to_list,
> >>  		      struct netdev_hw_addr_list *from_list, int addr_len);
> >>  int __hw_addr_sync_dev(struct netdev_hw_addr_list *list,
> >> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> >> index 166e404f7c03..90716bd736f3 100644
> >> --- a/net/core/dev_addr_lists.c
> >> +++ b/net/core/dev_addr_lists.c
> >> @@ -242,9 +242,9 @@ static void __hw_addr_unsync_one(struct netdev_hw_addr_list *to_list,
> >>  	__hw_addr_del_entry(from_list, ha, false, false);
> >>  }
> >>  
> >> -static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
> >> -				   struct netdev_hw_addr_list *from_list,
> >> -				   int addr_len)
> >> +int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
> >> +			    struct netdev_hw_addr_list *from_list,
> >> +			    int addr_len)
> >>  {
> >>  	int err = 0;
> >>  	struct netdev_hw_addr *ha, *tmp;
> >> @@ -260,6 +260,7 @@ static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
> >>  	}
> >>  	return err;
> >>  }
> >> +EXPORT_SYMBOL(__hw_addr_sync_multiple);
> >>  
> >>  /* This function only works where there is a strict 1-1 relationship
> >>   * between source and destination of they synch. If you ever need to
> >> -- 
> >> 2.34.1
> 
> -- 
> Thanks and Regards,
> Danish

Re: [PATCH net-next 1/4] net: ti: Kconfig: Select HSR for ICSSG Driver

[Larysa Zaremba]

On Thu, Dec 19, 2024 at 10:36:57AM +0530, MD Danish Anwar wrote:
> 
> 
> On 18/12/24 10:29 pm, Larysa Zaremba wrote:
> > On Mon, Dec 16, 2024 at 03:30:41PM +0530, MD Danish Anwar wrote:
> >> HSR offloading is supported by ICSSG driver. Select the symbol HSR for
> >> TI_ICSSG_PRUETH. Also select NET_SWITCHDEV instead of depending on it to
> >> remove recursive dependency.
> >>
> > 
> > 2 things:
> > 1) The explanation from the cover should have been included in the commit 
> >    message.
> 
> I wanted to keep the commit message brief so I provided the actual
> errors in cover letter. I will add the logs here as well.
>

Commit message has to be as verbose as needed to provide enough context for 
whoever needs to explore the code history later.
 
> > 2) Why not `depends on HSR`?
> 
> Adding `depends on HSR` in `config TI_ICSSG_PRUETH` is not setting HSR.
> I have tried below scenarios and only one of them work.
> 
> 1) depends on NET_SWITCHDEV
>    depends on HSR
> 
> 	HSR doesn't get set in .config - `# CONFIG_HSR is not set`. Even the
> CONFIG_TI_ICSSG_PRUETH also gets unset although this is set to =m in
> defconfig. But keeping both as `depends on` makes CONFIG_TI_ICSSG_PRUETH
> disabled.

I do not understand your problem with this option, CONFIG_HSR is a visible 
option that you can enable manually only then you will be able to successfully 
set CONFIG_TI_ICSSG_PRUETH to m/y, this is how the relation with NET_SWITCHDEV 
currently works.

Just 'depends on' is still a preferred way for me, as there is not a single 
driver that does 'select NET_SWITCHDEV'

> 
> 2) select NET_SWITCHDEV
>    depends on HSR
> 
> 	HSR doesn't get set in .config - `# CONFIG_HSR is not set`. Even the
> CONFIG_TI_ICSSG_PRUETH also gets unset although this is set to =m in
> defconfig. But keeping both as `depends on` makes CONFIG_TI_ICSSG_PRUETH
> disabled.
> 
> 3) depends on NET_SWITCHDEV
>    select HSR
> 	
> 	Results in recursive dependency
> 
> error: recursive dependency detected!
> 	symbol NET_DSA depends on HSR
> 	symbol HSR is selected by TI_ICSSG_PRUETH
> 	symbol TI_ICSSG_PRUETH depends on NET_SWITCHDEV
> 	symbol NET_SWITCHDEV is selected by NET_DSA
> For a resolution refer to Documentation/kbuild/kconfig-language.rst
> subsection "Kconfig recursive dependency limitations"
> 
> make[2]: *** [scripts/kconfig/Makefile:95: defconfig] Error 1
> make[1]: *** [/home/danish/workspace/net-next/Makefile:733: defconfig]
> Error 2
> make: *** [Makefile:251: __sub-make] Error 2
> 
> 4) select NET_SWITCHDEV
>    select HSR
> 
> 	HSR is set as `m` along with `CONFIG_TI_ICSSG_PRUETH`
> 
> CONFIG_HSR=m
> CONFIG_NET_SWITCHDEV=y
> CONFIG_TI_ICSSG_PRUETH=m
> 
> #4 is the only secnario where HSR gets built. That's why I sent the
> patch with `select NET_SWITCHDEV` and `select HSR`
> 
> >  
> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> >> ---
> >>  drivers/net/ethernet/ti/Kconfig | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
> >> index 0d5a862cd78a..ad366abfa746 100644
> >> --- a/drivers/net/ethernet/ti/Kconfig
> >> +++ b/drivers/net/ethernet/ti/Kconfig
> >> @@ -187,8 +187,9 @@ config TI_ICSSG_PRUETH
> >>  	select PHYLIB
> >>  	select TI_ICSS_IEP
> >>  	select TI_K3_CPPI_DESC_POOL
> >> +	select NET_SWITCHDEV
> >> +	select HSR
> >>  	depends on PRU_REMOTEPROC
> >> -	depends on NET_SWITCHDEV
> >>  	depends on ARCH_K3 && OF && TI_K3_UDMA_GLUE_LAYER
> >>  	depends on PTP_1588_CLOCK_OPTIONAL
> >>  	help
> >> -- 
> >> 2.34.1
> >>
> >>
> 
> -- 
> Thanks and Regards,
> Danish

Re: [PATCH net-next 1/4] net: ti: Kconfig: Select HSR for ICSSG Driver

[Anwar, Md Danish]

Hi,

On 12/19/2024 10:29 PM, Larysa Zaremba wrote:
> On Thu, Dec 19, 2024 at 10:36:57AM +0530, MD Danish Anwar wrote:
>>
>>
>> On 18/12/24 10:29 pm, Larysa Zaremba wrote:
>>> On Mon, Dec 16, 2024 at 03:30:41PM +0530, MD Danish Anwar wrote:
>>>> HSR offloading is supported by ICSSG driver. Select the symbol HSR for
>>>> TI_ICSSG_PRUETH. Also select NET_SWITCHDEV instead of depending on it to
>>>> remove recursive dependency.
>>>>
>>>
>>> 2 things:
>>> 1) The explanation from the cover should have been included in the commit 
>>>    message.
>>
>> I wanted to keep the commit message brief so I provided the actual
>> errors in cover letter. I will add the logs here as well.
>>
> 
> Commit message has to be as verbose as needed to provide enough context for 
> whoever needs to explore the code history later.
>  

Sure I will update the commit message.

>>> 2) Why not `depends on HSR`?
>>
>> Adding `depends on HSR` in `config TI_ICSSG_PRUETH` is not setting HSR.
>> I have tried below scenarios and only one of them work.
>>
>> 1) depends on NET_SWITCHDEV
>>    depends on HSR
>>
>> 	HSR doesn't get set in .config - `# CONFIG_HSR is not set`. Even the
>> CONFIG_TI_ICSSG_PRUETH also gets unset although this is set to =m in
>> defconfig. But keeping both as `depends on` makes CONFIG_TI_ICSSG_PRUETH
>> disabled.
> 
> I do not understand your problem with this option, CONFIG_HSR is a visible 
> option that you can enable manually only then you will be able to successfully 
> set CONFIG_TI_ICSSG_PRUETH to m/y, this is how the relation with NET_SWITCHDEV 
> currently works.
> 

The only problem with this option is that when I do `make defconfig`, it
will unset CONFIG_TI_ICSSG_PRUETH.

I will have to manually change the arch/arm64/configs/defconfig to set
HSR=m and then only `make defconfig` will enable CONFIG_TI_ICSSG_PRUETH

Currently HSR is not enabled in defconfig. I will have to send out a
patch to set HSR=m in defconfig

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index c62831e61586..ff3e5d960e2a 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -129,6 +129,7 @@ CONFIG_MEMORY_FAILURE=y
 CONFIG_TRANSPARENT_HUGEPAGE=y
 CONFIG_NET=y
 CONFIG_PACKET=y
+CONFIG_HSR=m
 CONFIG_UNIX=y
 CONFIG_INET=y
 CONFIG_IP_MULTICAST=y


Since I am the only one needing HSR, I thought it would be better if I
select HSR and it only gets built if CONFIG_TI_ICSSG_PRUETH is enabled
instead of always getting built.

For this reason I thought selecting HSR would be good choice, since just
selecting HSR wasn't enough and resulted in recursive dependency,  I had
to change NET_SWITCHDEV also to select.

BTW `NET_DSA` selects `NET_SWITCHDEV` (net/dsa/Kconfig:9)

> Just 'depends on' is still a preferred way for me, as there is not a single 
> driver that does 'select NET_SWITCHDEV'
> 
>>
>> 2) select NET_SWITCHDEV
>>    depends on HSR
>>
>> 	HSR doesn't get set in .config - `# CONFIG_HSR is not set`. Even the
>> CONFIG_TI_ICSSG_PRUETH also gets unset although this is set to =m in
>> defconfig. But keeping both as `depends on` makes CONFIG_TI_ICSSG_PRUETH
>> disabled.
>>
>> 3) depends on NET_SWITCHDEV
>>    select HSR
>> 	
>> 	Results in recursive dependency
>>
>> error: recursive dependency detected!
>> 	symbol NET_DSA depends on HSR
>> 	symbol HSR is selected by TI_ICSSG_PRUETH
>> 	symbol TI_ICSSG_PRUETH depends on NET_SWITCHDEV
>> 	symbol NET_SWITCHDEV is selected by NET_DSA
>> For a resolution refer to Documentation/kbuild/kconfig-language.rst
>> subsection "Kconfig recursive dependency limitations"
>>
>> make[2]: *** [scripts/kconfig/Makefile:95: defconfig] Error 1
>> make[1]: *** [/home/danish/workspace/net-next/Makefile:733: defconfig]
>> Error 2
>> make: *** [Makefile:251: __sub-make] Error 2
>>
>> 4) select NET_SWITCHDEV
>>    select HSR
>>
>> 	HSR is set as `m` along with `CONFIG_TI_ICSSG_PRUETH`
>>
>> CONFIG_HSR=m
>> CONFIG_NET_SWITCHDEV=y
>> CONFIG_TI_ICSSG_PRUETH=m
>>
>> #4 is the only secnario where HSR gets built. That's why I sent the
>> patch with `select NET_SWITCHDEV` and `select HSR`
>>

I still think 4 is the best option. Only difference here is that we have
to `select NET_SWITCHDEV` as well.

>>>  
>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>> ---
>>>>  drivers/net/ethernet/ti/Kconfig | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
>>>> index 0d5a862cd78a..ad366abfa746 100644
>>>> --- a/drivers/net/ethernet/ti/Kconfig
>>>> +++ b/drivers/net/ethernet/ti/Kconfig
>>>> @@ -187,8 +187,9 @@ config TI_ICSSG_PRUETH
>>>>  	select PHYLIB
>>>>  	select TI_ICSS_IEP
>>>>  	select TI_K3_CPPI_DESC_POOL
>>>> +	select NET_SWITCHDEV
>>>> +	select HSR
>>>>  	depends on PRU_REMOTEPROC
>>>> -	depends on NET_SWITCHDEV
>>>>  	depends on ARCH_K3 && OF && TI_K3_UDMA_GLUE_LAYER
>>>>  	depends on PTP_1588_CLOCK_OPTIONAL
>>>>  	help
>>>> -- 
>>>> 2.34.1
>>>>
>>>>
>>
>> -- 
>> Thanks and Regards,
>> Danish

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net-next 1/4] net: ti: Kconfig: Select HSR for ICSSG Driver

[kernel test robot]

Hi MD,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 92c932b9946c1e082406aa0515916adb3e662e24]

url:    https://github.com/intel-lab-lkp/linux/commits/MD-Danish-Anwar/net-ti-Kconfig-Select-HSR-for-ICSSG-Driver/20241216-180412
base:   92c932b9946c1e082406aa0515916adb3e662e24
patch link:    https://lore.kernel.org/r/20241216100044.577489-2-danishanwar%40ti.com
patch subject: [PATCH net-next 1/4] net: ti: Kconfig: Select HSR for ICSSG Driver
config: arm64-kismet-CONFIG_NET_SWITCHDEV-CONFIG_TI_ICSSG_PRUETH-0-0 (https://download.01.org/0day-ci/archive/20241221/202412210336.BmgcX3Td-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20241221/202412210336.BmgcX3Td-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412210336.BmgcX3Td-lkp@intel.com/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for NET_SWITCHDEV when selected by TI_ICSSG_PRUETH
   WARNING: unmet direct dependencies detected for NET_SWITCHDEV
     Depends on [n]: NET [=y] && INET [=n]
     Selected by [y]:
     - TI_ICSSG_PRUETH [=y] && NETDEVICES [=y] && ETHERNET [=y] && NET_VENDOR_TI [=y] && PRU_REMOTEPROC [=y] && ARCH_K3 [=y] && OF [=y] && TI_K3_UDMA_GLUE_LAYER [=y] && PTP_1588_CLOCK_OPTIONAL [=y]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 1/4] net: ti: Kconfig: Select HSR for ICSSG Driver

[MD Danish Anwar]

Hi,

On 20/12/24 11:01 am, Anwar, Md Danish wrote:
> Hi,
> 
> On 12/19/2024 10:29 PM, Larysa Zaremba wrote:
>> On Thu, Dec 19, 2024 at 10:36:57AM +0530, MD Danish Anwar wrote:
>>>
>>>
>>> On 18/12/24 10:29 pm, Larysa Zaremba wrote:
>>>> On Mon, Dec 16, 2024 at 03:30:41PM +0530, MD Danish Anwar wrote:
>>>>> HSR offloading is supported by ICSSG driver. Select the symbol HSR for
>>>>> TI_ICSSG_PRUETH. Also select NET_SWITCHDEV instead of depending on it to
>>>>> remove recursive dependency.
>>>>>
>>>>
>>>> 2 things:
>>>> 1) The explanation from the cover should have been included in the commit 
>>>>    message.
>>>
>>> I wanted to keep the commit message brief so I provided the actual
>>> errors in cover letter. I will add the logs here as well.
>>>
>>
>> Commit message has to be as verbose as needed to provide enough context for 
>> whoever needs to explore the code history later.
>>  
> 
> Sure I will update the commit message.
> 
>>>> 2) Why not `depends on HSR`?
>>>
>>> Adding `depends on HSR` in `config TI_ICSSG_PRUETH` is not setting HSR.
>>> I have tried below scenarios and only one of them work.
>>>
>>> 1) depends on NET_SWITCHDEV
>>>    depends on HSR
>>>
>>> 	HSR doesn't get set in .config - `# CONFIG_HSR is not set`. Even the
>>> CONFIG_TI_ICSSG_PRUETH also gets unset although this is set to =m in
>>> defconfig. But keeping both as `depends on` makes CONFIG_TI_ICSSG_PRUETH
>>> disabled.
>>
>> I do not understand your problem with this option, CONFIG_HSR is a visible 
>> option that you can enable manually only then you will be able to successfully 
>> set CONFIG_TI_ICSSG_PRUETH to m/y, this is how the relation with NET_SWITCHDEV 
>> currently works.
>>
> 
> The only problem with this option is that when I do `make defconfig`, it
> will unset CONFIG_TI_ICSSG_PRUETH.
> 
> I will have to manually change the arch/arm64/configs/defconfig to set
> HSR=m and then only `make defconfig` will enable CONFIG_TI_ICSSG_PRUETH
> 
> Currently HSR is not enabled in defconfig. I will have to send out a
> patch to set HSR=m in defconfig
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index c62831e61586..ff3e5d960e2a 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -129,6 +129,7 @@ CONFIG_MEMORY_FAILURE=y
>  CONFIG_TRANSPARENT_HUGEPAGE=y
>  CONFIG_NET=y
>  CONFIG_PACKET=y
> +CONFIG_HSR=m
>  CONFIG_UNIX=y
>  CONFIG_INET=y
>  CONFIG_IP_MULTICAST=y
> 
> 
> Since I am the only one needing HSR, I thought it would be better if I
> select HSR and it only gets built if CONFIG_TI_ICSSG_PRUETH is enabled
> instead of always getting built.
> 
> For this reason I thought selecting HSR would be good choice, since just
> selecting HSR wasn't enough and resulted in recursive dependency,  I had
> to change NET_SWITCHDEV also to select.
> 
> BTW `NET_DSA` selects `NET_SWITCHDEV` (net/dsa/Kconfig:9)
> 
>> Just 'depends on' is still a preferred way for me, as there is not a single 
>> driver that does 'select NET_SWITCHDEV'
>>
>>>
>>> 2) select NET_SWITCHDEV
>>>    depends on HSR
>>>
>>> 	HSR doesn't get set in .config - `# CONFIG_HSR is not set`. Even the
>>> CONFIG_TI_ICSSG_PRUETH also gets unset although this is set to =m in
>>> defconfig. But keeping both as `depends on` makes CONFIG_TI_ICSSG_PRUETH
>>> disabled.
>>>
>>> 3) depends on NET_SWITCHDEV
>>>    select HSR
>>> 	
>>> 	Results in recursive dependency
>>>
>>> error: recursive dependency detected!
>>> 	symbol NET_DSA depends on HSR
>>> 	symbol HSR is selected by TI_ICSSG_PRUETH
>>> 	symbol TI_ICSSG_PRUETH depends on NET_SWITCHDEV
>>> 	symbol NET_SWITCHDEV is selected by NET_DSA
>>> For a resolution refer to Documentation/kbuild/kconfig-language.rst
>>> subsection "Kconfig recursive dependency limitations"
>>>
>>> make[2]: *** [scripts/kconfig/Makefile:95: defconfig] Error 1
>>> make[1]: *** [/home/danish/workspace/net-next/Makefile:733: defconfig]
>>> Error 2
>>> make: *** [Makefile:251: __sub-make] Error 2
>>>
>>> 4) select NET_SWITCHDEV
>>>    select HSR
>>>
>>> 	HSR is set as `m` along with `CONFIG_TI_ICSSG_PRUETH`
>>>
>>> CONFIG_HSR=m
>>> CONFIG_NET_SWITCHDEV=y
>>> CONFIG_TI_ICSSG_PRUETH=m
>>>
>>> #4 is the only secnario where HSR gets built. That's why I sent the
>>> patch with `select NET_SWITCHDEV` and `select HSR`
>>>
> 
> I still think 4 is the best option. Only difference here is that we have
> to `select NET_SWITCHDEV` as well.
> 

I see there is some issue seen
https://lore.kernel.org/all/202412210336.BmgcX3Td-lkp@intel.com/ with
this patch.

For now I'll drop this patch and send out a separate patch to defconfig
to set HSR=m.

Once the defconfig patch gets merged, I will add `depends on HSR` in
Kconfig for TI_ICSSG_PRUETH (the method suggested by you)

Thanks for the review.

>>>>  
>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>>> ---
>>>>>  drivers/net/ethernet/ti/Kconfig | 3 ++-
>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
>>>>> index 0d5a862cd78a..ad366abfa746 100644
>>>>> --- a/drivers/net/ethernet/ti/Kconfig
>>>>> +++ b/drivers/net/ethernet/ti/Kconfig
>>>>> @@ -187,8 +187,9 @@ config TI_ICSSG_PRUETH
>>>>>  	select PHYLIB
>>>>>  	select TI_ICSS_IEP
>>>>>  	select TI_K3_CPPI_DESC_POOL
>>>>> +	select NET_SWITCHDEV
>>>>> +	select HSR
>>>>>  	depends on PRU_REMOTEPROC
>>>>> -	depends on NET_SWITCHDEV
>>>>>  	depends on ARCH_K3 && OF && TI_K3_UDMA_GLUE_LAYER
>>>>>  	depends on PTP_1588_CLOCK_OPTIONAL
>>>>>  	help
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>>>
>>>
>>> -- 
>>> Thanks and Regards,
>>> Danish
> 

-- 
Thanks and Regards,
Danish