| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <45fac9f0-b31a-495d-bd1b-ccf0fbe19653@redhat.com>
Date: Tue, 7 Jan 2025 19:27:54 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: MD Danish Anwar <danishanwar@...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,
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.
> 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
Powered by blists - more mailing lists