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

[Paolo Abeni <pabeni@redhat.com>]

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