Message-ID: <b8d46941-aeb2-466a-acd5-8ad5be832649@ti.com>
Date: Wed, 8 Jan 2025 15:10:14 +0530
From: MD Danish Anwar <danishanwar@...com>
To: Paolo Abeni <pabeni@...hat.com>, Jeongjun Park <aha310510@...il.com>,
        Alexander Lobakin <aleksander.lobakin@...el.com>,
        Lukasz Majewski
	<lukma@...x.de>, Meghana Malladi <m-malladi@...com>,
        Diogo Ivo
	<diogo.ivo@...mens.com>, Simon Horman <horms@...nel.org>,
        Jakub Kicinski
	<kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller"
	<davem@...emloft.net>,
        Andrew Lunn <andrew+netdev@...n.ch>,
        Roger Quadros
	<rogerq@...nel.org>
CC: <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <srk@...com>,
        Vignesh Raghavendra
	<vigneshr@...com>,
        Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
        Larysa Zaremba <larysa.zaremba@...el.com>
Subject: Re: [PATCH net-next v3 2/3] net: ti: icssg-prueth: Add Multicast
 Filtering support for VLAN in MAC mode

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@...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

Powered by blists - more mailing lists