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

[PATCH net-next v3 0/3] 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/3 - Adds support for VLAN in dual EMAC mode
Patch 2/3 - Adds MC filtering support for VLAN in dual EMAC mode
Patch 3/3 - Adds MC filtering support for VLAN in HSR mode

Changes from v2 to v3:
*) Rebased on latest net-next and re-spun the series as net-next is now open.
*) No functional change.

Changes from v1 to v2:
*) Changed netdev_err to netdev_dbg in emac_ndo_vlan_rx_del_vid() in patch 1/3
as suggested by Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
*) Dropped patch [1] from previous version as the patch created issue [2].
Will send out a separate patch to set HSR=m in arch/arm64/configs/defconfig.
Once the defconfig patch gets merged, I will add `depends on HSR` in Kconfig
for TI_ICSSG_PRUETH as suggested by Larysa Zaremba <larysa.zaremba@intel.com>

[1] https://lore.kernel.org/all/20241216100044.577489-2-danishanwar@ti.com/
[2] https://lore.kernel.org/all/202412210336.BmgcX3Td-lkp@intel.com/#t
v1 https://lore.kernel.org/all/20241216100044.577489-1-danishanwar@ti.com/
v2 https://lore.kernel.org/all/20241223092557.2077526-1-danishanwar@ti.com/

MD Danish Anwar (3):
  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/icssg/icssg_prueth.c | 175 ++++++++++++++-----
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |   8 +
 include/linux/if_hsr.h                       |  18 ++
 include/linux/netdevice.h                    |   3 +
 net/core/dev_addr_lists.c                    |   7 +-
 net/hsr/hsr_device.c                         |  13 ++
 net/hsr/hsr_main.h                           |   9 -
 7 files changed, 174 insertions(+), 59 deletions(-)


base-commit: 94c16fd4df9089931f674fb9aaec41ea20b0fd7a
-- 
2.34.1

[PATCH net-next v3 1/3] 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..1663941e59e3 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_dbg(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_dbg(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

Re: [PATCH net-next v3 1/3] net: ti: icssg-prueth: Add VLAN support in EMAC mode

[Roger Quadros]

On 03/01/2025 11:20, MD Danish Anwar wrote:
> Add support for vlan filtering in dual EMAC mode.

vlan/VLAN

> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

[PATCH net-next v3 2/3] 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 | 67 ++++++++++++++++----
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  6 ++
 include/linux/netdevice.h                    |  3 +
 net/core/dev_addr_lists.c                    |  7 +-
 4 files changed, 66 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 1663941e59e3..ed8b5a3184d6 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -472,30 +472,44 @@ 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 +545,25 @@ 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 prueth_emac *emac = args;
+
+	if (!vdev || !vid)
+		return 0;
+
+	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 +805,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, emac);
+			rtnl_unlock();
+		}
+	}
 }
 
 /**
@@ -828,6 +866,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_dbg(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 2593019ad5b1..3ee833e9b6f7 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4689,6 +4689,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 v3 2/3] net: ti: icssg-prueth: Add Multicast Filtering support for VLAN in MAC mode

[Paolo Abeni]

On 1/3/25 10:20 AM, 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 | 67 ++++++++++++++++----
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  6 ++
>  include/linux/netdevice.h                    |  3 +
>  net/core/dev_addr_lists.c                    |  7 +-
>  4 files changed, 66 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 1663941e59e3..ed8b5a3184d6 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -472,30 +472,44 @@ 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 +545,25 @@ 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 prueth_emac *emac = args;
> +
> +	if (!vdev || !vid)
> +		return 0;
> +
> +	netif_addr_lock_bh(vdev);
> +	__hw_addr_sync_multiple(&emac->vlan_mcast_list[vid], &vdev->mc,
> +				vdev->addr_len);
> +	netif_addr_unlock_bh(vdev);

At this point, isn't emac->vlan_mcast_list[vid] == vdev->mc?

> +
> +	__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
> +			   icssg_prueth_add_mcast, icssg_prueth_del_mcast);

If so, can this function be reduced to just:

	__dev_mc_sync(vdev, icssg_prueth_add_mcast, icssg_prueth_del_mcast);

?

> 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);

I'm asking because this additional export looks suspect. How other
drivers cope with similar situation?

Thanks!

Paolo

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

[MD Danish Anwar]

Hi Paolo,

On 07/01/25 3:12 pm, Paolo Abeni wrote:
> On 1/3/25 10:20 AM, 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 | 67 ++++++++++++++++----
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  6 ++
>>  include/linux/netdevice.h                    |  3 +
>>  net/core/dev_addr_lists.c                    |  7 +-
>>  4 files changed, 66 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index 1663941e59e3..ed8b5a3184d6 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -472,30 +472,44 @@ 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 +545,25 @@ 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 prueth_emac *emac = args;
>> +
>> +	if (!vdev || !vid)
>> +		return 0;
>> +
>> +	netif_addr_lock_bh(vdev);
>> +	__hw_addr_sync_multiple(&emac->vlan_mcast_list[vid], &vdev->mc,
>> +				vdev->addr_len);
>> +	netif_addr_unlock_bh(vdev);
> 
> At this point, isn't emac->vlan_mcast_list[vid] == vdev->mc?
> 
>> +
>> +	__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
>> +			   icssg_prueth_add_mcast, icssg_prueth_del_mcast);
> 
> If so, can this function be reduced to just:
> 
> 	__dev_mc_sync(vdev, icssg_prueth_add_mcast, icssg_prueth_del_mcast);
> 
> ?
> 

I don't know but for some reason __dev_mc_sync() doesn't work here. My
initial approach was to use __dev_mc_sync(vdev, sync, unsync) however it
didn't work.

When I use __dev_mc_sync() and print the vlan_id in function
icssg_prueth_add_mcast(). It always prints vlan_id as 0 implying
__dev_mc_sync from here never gets called. Whereas when using
__hw_addr_sync_dev() I see the appropriate vlan_id in
icssg_prueth_add_mcast()

Anyways, Even if I use __dev_mc_sync(), we will still need the export. I
am exporting __hw_addr_sync_multiple() not __hw_addr_sync_dev(). The API
being used by me `__hw_addr_sync_dev()` is already exported.

>> 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);
> 
> I'm asking because this additional export looks suspect. How other
> drivers cope with similar situation?
> 

To avoid exporting I will need to use dev_mc_sync_multiple() which was
in fact suggested by Michal Swiatkowski in [1]. However that won't work
in this case as explained by me in the reply to Michal [2]

[1]
https://lore.kernel.org/all/Z2PLDqqrLdXhLtAF@mev-dev.igk.intel.com/#:~:text=Only%20question%2C%20why%20dev_mc_sync_multiple%20can%27t%20be%20used%20here%3F
[2]
https://lore.kernel.org/all/cb319ffd-ac67-42b3-9786-e8c9970086d2@ti.com/#:~:text=nice%20to%20have.%0A%3E-,I,-don%27t%20think%20that%27s

> Thanks!
> 
> Paolo
> 

-- 
Thanks and Regards,
Danish

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

[Paolo Abeni]

Hi,

On 1/7/25 11:47 AM, MD Danish Anwar wrote:
> On 07/01/25 3:12 pm, Paolo Abeni wrote:
>> On 1/3/25 10:20 AM, 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 | 67 ++++++++++++++++----
>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  6 ++
>>>  include/linux/netdevice.h                    |  3 +
>>>  net/core/dev_addr_lists.c                    |  7 +-
>>>  4 files changed, 66 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>> index 1663941e59e3..ed8b5a3184d6 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>> @@ -472,30 +472,44 @@ 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 +545,25 @@ 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 prueth_emac *emac = args;
>>> +
>>> +	if (!vdev || !vid)
>>> +		return 0;
>>> +
>>> +	netif_addr_lock_bh(vdev);
>>> +	__hw_addr_sync_multiple(&emac->vlan_mcast_list[vid], &vdev->mc,
>>> +				vdev->addr_len);
>>> +	netif_addr_unlock_bh(vdev);
>>
>> At this point, isn't emac->vlan_mcast_list[vid] == vdev->mc?
>>
>>> +
>>> +	__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
>>> +			   icssg_prueth_add_mcast, icssg_prueth_del_mcast);
>>
>> If so, can this function be reduced to just:
>>
>> 	__dev_mc_sync(vdev, icssg_prueth_add_mcast, icssg_prueth_del_mcast);
>>
>> ?
>>
> 
> I don't know but for some reason __dev_mc_sync() doesn't work here. My
> initial approach was to use __dev_mc_sync(vdev, sync, unsync) however it
> didn't work.
> 
> When I use __dev_mc_sync() and print the vlan_id in function
> icssg_prueth_add_mcast(). It always prints vlan_id as 0 implying
> __dev_mc_sync from here never gets called. Whereas when using
> __hw_addr_sync_dev() I see the appropriate vlan_id in
> icssg_prueth_add_mcast()

It look like the above needs more investigation, right? is
vlan_mcast_list[vid] different from vdev->mc? why? At very least you
need to provide a clear explaination of the above.

> Anyways, Even if I use __dev_mc_sync(), we will still need the export. I
> am exporting __hw_addr_sync_multiple() not __hw_addr_sync_dev(). The API
> being used by me `__hw_addr_sync_dev()` is already exported.

I fear there is a misunderstanding. I'm suggesting dropping
__hw_addr_sync_multiple() usage entirely. If that is not possible, a
clear and complete explaination of the reason/root cause must be provided.

Thanks,

Paolo

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

[MD Danish Anwar]

Hi Paolo,

On 07/01/25 11:57 pm, Paolo Abeni wrote:
> Hi,
> 
> On 1/7/25 11:47 AM, MD Danish Anwar wrote:
>> On 07/01/25 3:12 pm, Paolo Abeni wrote:
>>> On 1/3/25 10:20 AM, 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 | 67 ++++++++++++++++----
>>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  6 ++
>>>>  include/linux/netdevice.h                    |  3 +
>>>>  net/core/dev_addr_lists.c                    |  7 +-
>>>>  4 files changed, 66 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>> index 1663941e59e3..ed8b5a3184d6 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>> @@ -472,30 +472,44 @@ 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 +545,25 @@ 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 prueth_emac *emac = args;
>>>> +
>>>> +	if (!vdev || !vid)
>>>> +		return 0;
>>>> +
>>>> +	netif_addr_lock_bh(vdev);
>>>> +	__hw_addr_sync_multiple(&emac->vlan_mcast_list[vid], &vdev->mc,
>>>> +				vdev->addr_len);
>>>> +	netif_addr_unlock_bh(vdev);
>>>
>>> At this point, isn't emac->vlan_mcast_list[vid] == vdev->mc?
>>>
>>>> +
>>>> +	__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
>>>> +			   icssg_prueth_add_mcast, icssg_prueth_del_mcast);
>>>
>>> If so, can this function be reduced to just:
>>>
>>> 	__dev_mc_sync(vdev, icssg_prueth_add_mcast, icssg_prueth_del_mcast);
>>>
>>> ?
>>>
>>
>> I don't know but for some reason __dev_mc_sync() doesn't work here. My
>> initial approach was to use __dev_mc_sync(vdev, sync, unsync) however it
>> didn't work.
>>
>> When I use __dev_mc_sync() and print the vlan_id in function
>> icssg_prueth_add_mcast(). It always prints vlan_id as 0 implying
>> __dev_mc_sync from here never gets called. Whereas when using
>> __hw_addr_sync_dev() I see the appropriate vlan_id in
>> icssg_prueth_add_mcast()
> 
> It look like the above needs more investigation, right? is
> vlan_mcast_list[vid] different from vdev->mc? why? At very least you
> need to provide a clear explaination of the above.
> 

I did further debug on this. At this point vlan_mcast_list[vid] is
actually same as vdev->mc in terms of the multicast addresses.

However the sync_cnt and refcount of the addresses in both the lists are
not same. Due to which vdev->mc doesn't work here. I will explain it.

__dev_mc_sync(vdev, sync, unsync) will call __hw_addr_sync_dev(&dev->mc,
dev, sync, unsync)

Now in __hw_addr_sync_dev() sync is only called when ha->sync_cnt == 0
for the given mac address. If ha->sync_cnt is non zero, sync will not
get called.

When we add a multicast address to the vlan interface using below command,
	
	ip maddr add <mac_addr> dev eth1.6

ndo_set_rx_mode() gets called for the vlan interface i.e.
vlan_dev_set_rx_mode() [net/8021q/vlan_dev.c] which syncs mc list from
the vdev to the real_dev of vdev by calling dev_mc_sync(real_dev, vdev)

Now dev_mc_sync() sync addresses from vdev to real_dev using
__hw_addr_sync().

__hw_addr_sync() calls __hw_addr_sync_one() which after successfully
syncing an address from vdev to real_dev increment the ha->sync_cnt. As
a result at this point the ha->sync_cnt == 1 for the above address
passed by command. After this vlan_dev_set_rx_mode() calls the
set_rx_mode() of the real_dev.

Now when icssg_update_vlan_mcast() calls __dev_mc_sync(vdev, sync,
unsync), it calls _hw_addr_sync_dev(&dev->mc, dev, sync, unsync) and
checks the ha->sync_cnt for the given address, since sync_cnt is already
1, it doesn't consider it as an newly added address and sync / unsync
callbacks are not called. [1]

list_for_each_entry_safe(ha, tmp, &list->list, list) {
	if (ha->sync_cnt)
		continue;

Since for addresses in vlan interfaces, sync_cnt will always be set as
vlan_dev_set_rx_mode() will sync the mc list of vlan interface with the
real dev. This will mean that the driver implemented sync / unsync APIs
will only be called for the real_dev and the real_dev won't have any
information about the vlan_id which I need in my sync / unsync APIs to
populate the same in the hardware maintained FDB table.

To overcome this, I am maintaining a local copy of vdev->mc named
emac->vlan_mcast_list[vid]. I will sync address from vdev->mc to this.
and then call __hw_addr_sync_dev() on this list as the sync_cnt for
address in this list will still be zero. Which will then trigger my
sync() callback on the vdev which will help me obtain the vlan_id in the
sync callback. I can now populate the same in the hardware maintained
FDB table.

I hope this explains why I am using

	__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
icssg_prueth_add_mcast, icssg_prueth_del_mcast)

instead of

	__dev_mc_sync(vdev, icssg_prueth_add_mcast, icssg_prueth_del_mcast);

>> Anyways, Even if I use __dev_mc_sync(), we will still need the export. I
>> am exporting __hw_addr_sync_multiple() not __hw_addr_sync_dev(). The API
>> being used by me `__hw_addr_sync_dev()` is already exported.
> 
> I fear there is a misunderstanding. I'm suggesting dropping
> __hw_addr_sync_multiple() usage entirely. If that is not possible, a
> clear and complete explaination of the reason/root cause must be provided.
> 

As explained above, I need to be able to sync addresses from vdev->mc to
my local list. Now this can be done using two APIs.

1. __hw_addr_sync() - This is already exported and is actually the first
choice. However this will fail syncing address from vdev->mc to my local
copy.

__hw_addr_sync() only syncs the address if ha->sync_cnt == 0. If the
sync_cnt is non zero, this will skip the address. As explained above,
for mc addresses of vlan interfaces, sync_cnt will always be set as
vlan_dev_set_rx_mode() will sync the mc list of vlan interface with the
real dev. As a result the addresses will be skipped and __hw_addr_sync()
will not serve the purpose here.

2. __hw_addr_sync_multiple() - This actually works perfectly fine here
as it sync address even if sync_cnt is non zero. As a result I am using
this function in my implementation.

Since this is not public, I have to export it so that the driver can
call this.

So I am afraid dropping __hw_addr_sync_multiple() is not possible here.
I hope the explanation above makes sense to you.

Please let me know if this is OK with you. If you have some other way
through which I can make this work please let me know.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/net/core/dev_addr_lists.c#n314

> Thanks,
> 
> Paolo
> 

-- 
Thanks and Regards,
Danish

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

[Paolo Abeni]

On 1/8/25 10:40 AM, MD Danish Anwar wrote:
> On 07/01/25 11:57 pm, Paolo Abeni wrote:
>> On 1/7/25 11:47 AM, MD Danish Anwar wrote:
>>> On 07/01/25 3:12 pm, Paolo Abeni wrote:
>>>> On 1/3/25 10:20 AM, 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 | 67 ++++++++++++++++----
>>>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  6 ++
>>>>>  include/linux/netdevice.h                    |  3 +
>>>>>  net/core/dev_addr_lists.c                    |  7 +-
>>>>>  4 files changed, 66 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>>> index 1663941e59e3..ed8b5a3184d6 100644
>>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>>> @@ -472,30 +472,44 @@ 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 +545,25 @@ 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 prueth_emac *emac = args;
>>>>> +
>>>>> +	if (!vdev || !vid)
>>>>> +		return 0;
>>>>> +
>>>>> +	netif_addr_lock_bh(vdev);
>>>>> +	__hw_addr_sync_multiple(&emac->vlan_mcast_list[vid], &vdev->mc,
>>>>> +				vdev->addr_len);
>>>>> +	netif_addr_unlock_bh(vdev);
>>>>
>>>> At this point, isn't emac->vlan_mcast_list[vid] == vdev->mc?
>>>>
>>>>> +
>>>>> +	__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
>>>>> +			   icssg_prueth_add_mcast, icssg_prueth_del_mcast);
>>>>
>>>> If so, can this function be reduced to just:
>>>>
>>>> 	__dev_mc_sync(vdev, icssg_prueth_add_mcast, icssg_prueth_del_mcast);
>>>>
>>>> ?
>>>>
>>>
>>> I don't know but for some reason __dev_mc_sync() doesn't work here. My
>>> initial approach was to use __dev_mc_sync(vdev, sync, unsync) however it
>>> didn't work.
>>>
>>> When I use __dev_mc_sync() and print the vlan_id in function
>>> icssg_prueth_add_mcast(). It always prints vlan_id as 0 implying
>>> __dev_mc_sync from here never gets called. Whereas when using
>>> __hw_addr_sync_dev() I see the appropriate vlan_id in
>>> icssg_prueth_add_mcast()
>>
>> It look like the above needs more investigation, right? is
>> vlan_mcast_list[vid] different from vdev->mc? why? At very least you
>> need to provide a clear explaination of the above.
>>
> 
> I did further debug on this. At this point vlan_mcast_list[vid] is
> actually same as vdev->mc in terms of the multicast addresses.
> 
> However the sync_cnt and refcount of the addresses in both the lists are
> not same. Due to which vdev->mc doesn't work here. I will explain it.
> 
> __dev_mc_sync(vdev, sync, unsync) will call __hw_addr_sync_dev(&dev->mc,
> dev, sync, unsync)
> 
> Now in __hw_addr_sync_dev() sync is only called when ha->sync_cnt == 0
> for the given mac address. If ha->sync_cnt is non zero, sync will not
> get called.
> 
> When we add a multicast address to the vlan interface using below command,
> 	
> 	ip maddr add <mac_addr> dev eth1.6
> 
> ndo_set_rx_mode() gets called for the vlan interface i.e.
> vlan_dev_set_rx_mode() [net/8021q/vlan_dev.c] which syncs mc list from
> the vdev to the real_dev of vdev by calling dev_mc_sync(real_dev, vdev)
> 
> Now dev_mc_sync() sync addresses from vdev to real_dev using
> __hw_addr_sync().
> 
> __hw_addr_sync() calls __hw_addr_sync_one() which after successfully
> syncing an address from vdev to real_dev increment the ha->sync_cnt. As
> a result at this point the ha->sync_cnt == 1 for the above address
> passed by command. After this vlan_dev_set_rx_mode() calls the
> set_rx_mode() of the real_dev.
> 
> Now when icssg_update_vlan_mcast() calls __dev_mc_sync(vdev, sync,
> unsync), it calls _hw_addr_sync_dev(&dev->mc, dev, sync, unsync) and
> checks the ha->sync_cnt for the given address, since sync_cnt is already
> 1, it doesn't consider it as an newly added address and sync / unsync
> callbacks are not called. [1]
> 
> list_for_each_entry_safe(ha, tmp, &list->list, list) {
> 	if (ha->sync_cnt)
> 		continue;
> 
> Since for addresses in vlan interfaces, sync_cnt will always be set as
> vlan_dev_set_rx_mode() will sync the mc list of vlan interface with the
> real dev. This will mean that the driver implemented sync / unsync APIs
> will only be called for the real_dev and the real_dev won't have any
> information about the vlan_id which I need in my sync / unsync APIs to
> populate the same in the hardware maintained FDB table.
> 
> To overcome this, I am maintaining a local copy of vdev->mc named
> emac->vlan_mcast_list[vid]. I will sync address from vdev->mc to this.
> and then call __hw_addr_sync_dev() on this list as the sync_cnt for
> address in this list will still be zero. Which will then trigger my
> sync() callback on the vdev which will help me obtain the vlan_id in the
> sync callback. I can now populate the same in the hardware maintained
> FDB table.
> 
> I hope this explains why I am using
> 
> 	__hw_addr_sync_dev(&emac->vlan_mcast_list[vid], vdev,
> icssg_prueth_add_mcast, icssg_prueth_del_mcast)
> 
> instead of
> 
> 	__dev_mc_sync(vdev, icssg_prueth_add_mcast, icssg_prueth_del_mcast);
> 
>>> Anyways, Even if I use __dev_mc_sync(), we will still need the export. I
>>> am exporting __hw_addr_sync_multiple() not __hw_addr_sync_dev(). The API
>>> being used by me `__hw_addr_sync_dev()` is already exported.
>>
>> I fear there is a misunderstanding. I'm suggesting dropping
>> __hw_addr_sync_multiple() usage entirely. If that is not possible, a
>> clear and complete explaination of the reason/root cause must be provided.
>>
> 
> As explained above, I need to be able to sync addresses from vdev->mc to
> my local list. Now this can be done using two APIs.
> 
> 1. __hw_addr_sync() - This is already exported and is actually the first
> choice. However this will fail syncing address from vdev->mc to my local
> copy.
> 
> __hw_addr_sync() only syncs the address if ha->sync_cnt == 0. If the
> sync_cnt is non zero, this will skip the address. As explained above,
> for mc addresses of vlan interfaces, sync_cnt will always be set as
> vlan_dev_set_rx_mode() will sync the mc list of vlan interface with the
> real dev. As a result the addresses will be skipped and __hw_addr_sync()
> will not serve the purpose here.
> 
> 2. __hw_addr_sync_multiple() - This actually works perfectly fine here
> as it sync address even if sync_cnt is non zero. As a result I am using
> this function in my implementation.
> 
> Since this is not public, I have to export it so that the driver can
> call this.
> 
> So I am afraid dropping __hw_addr_sync_multiple() is not possible here.
> I hope the explanation above makes sense to you.
> 
> Please let me know if this is OK with you. If you have some other way> through which I can make this work please let me know.

I think it makes sense and I can't think of simpler ways on top of my
head. Please repost capturing a synopsis of the above in the commit message.

Thanks,

Paolo

[PATCH net-next v3 3/3] 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 | 83 +++++++++++++++-----
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  2 +
 include/linux/if_hsr.h                       | 18 +++++
 net/hsr/hsr_device.c                         | 13 +++
 net/hsr/hsr_main.h                           |  9 ---
 5 files changed, 97 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index ed8b5a3184d6..29e0e7a86a7f 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -515,32 +515,66 @@ 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;
 }
@@ -558,8 +592,14 @@ static int icssg_update_vlan_mcast(struct net_device *vdev, int vid,
 				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;
 }
@@ -808,6 +848,11 @@ 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);
@@ -1194,7 +1239,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);
 		}
@@ -1247,7 +1292,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..0e0bbd6ed082 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,8 @@ 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 +53,13 @@ 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..b6fb18469439 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -663,6 +663,19 @@ 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 v3 3/3] net: ti: icssg-prueth: Add Support for Multicast filtering with VLAN in HSR mode

[Roger Quadros]

On 03/01/2025 11:20, MD Danish Anwar wrote:
> 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 | 83 +++++++++++++++-----
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  2 +
>  include/linux/if_hsr.h                       | 18 +++++
>  net/hsr/hsr_device.c                         | 13 +++
>  net/hsr/hsr_main.h                           |  9 ---

Should we be splitting hsr core changes into separate patch first,
then followed by a patch with icssg driver changes?

>  5 files changed, 97 insertions(+), 28 deletions(-)

-- 
cheers,
-roger

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

[Anwar, Md Danish]

On 1/7/2025 6:53 PM, Roger Quadros wrote:
> 
> 
> On 03/01/2025 11:20, MD Danish Anwar wrote:
>> 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 | 83 +++++++++++++++-----
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  2 +
>>  include/linux/if_hsr.h                       | 18 +++++
>>  net/hsr/hsr_device.c                         | 13 +++
>>  net/hsr/hsr_main.h                           |  9 ---
> 
> Should we be splitting hsr core changes into separate patch first,
> then followed by a patch with icssg driver changes?
> 

I wanted to make sure that the changes to hsr core are done with the
driver change so that any one looking at the commit can understand why
these changes are done.

If we split the changes and someone looks at the commit modifying hsr
core, they will not be able to understand why this is done. We will be
creating and exporting API hsr_get_port_ndev() but there will be no
caller for this.

>>  5 files changed, 97 insertions(+), 28 deletions(-)
> 

-- 
Thanks and Regards,
Md Danish Anwar

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

[Roger Quadros]

On 07/01/2025 16:01, Anwar, Md Danish wrote:
> 
> 
> On 1/7/2025 6:53 PM, Roger Quadros wrote:
>>
>>
>> On 03/01/2025 11:20, MD Danish Anwar wrote:
>>> 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 | 83 +++++++++++++++-----
>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  2 +
>>>  include/linux/if_hsr.h                       | 18 +++++
>>>  net/hsr/hsr_device.c                         | 13 +++
>>>  net/hsr/hsr_main.h                           |  9 ---
>>
>> Should we be splitting hsr core changes into separate patch first,
>> then followed by a patch with icssg driver changes?
>>
> 
> I wanted to make sure that the changes to hsr core are done with the
> driver change so that any one looking at the commit can understand why
> these changes are done.
> 
> If we split the changes and someone looks at the commit modifying hsr
> core, they will not be able to understand why this is done. We will be
> creating and exporting API hsr_get_port_ndev() but there will be no
> caller for this.

I think you can explain in the commit log why you need the API.

> 
>>>  5 files changed, 97 insertions(+), 28 deletions(-)
>>
> 

-- 
cheers,
-roger