Re: [PATCH net-next] net: ti: icssg-prueth: Add HSR multicast FDB port membership management

[MD Danish Anwar]

Hi Simon,

On 09/03/26 7:41 pm, Simon Horman wrote:
> On Thu, Mar 05, 2026 at 06:22:51PM +0530, MD Danish Anwar wrote:
>> In HSR offload mode, multicast addresses can be added via HSR master
>> (hsr0) or directly to slave ports (eth1/eth2). The FDB must track port
>> membership: P0 (0x1) for HSR master, P1 (0x2) for slave port 1, and P2
>> (0x4) for slave port 2. When the same address is added from multiple
>> paths, memberships must accumulate.
>>
>> Implement a hybrid approach using __dev_mc_sync() callbacks to track
>> basic add/delete operations, checking netdev_hw_addr->synced to
>> distinguish HSR-synced addresses from direct additions. Post-process
>> to handle overlapping memberships by checking refcount:
>> - refcount=2 with synced=1: HSR only (P0)
>> - refcount>=3 with synced=1: HSR + direct (P0|P1/P2)
>> - synced=0 with P0 set: HSR removed, clean up orphaned P0
>>
>> On add operations, accumulate new membership with existing ports. On
>> delete operations, remove only the specific port and clean up orphaned
>> P0 bits if needed.
>>
>> Add error handling for icssg_fdb_lookup() which can return negative
>> error codes (e.g., -ETIMEDOUT). On lookup failure in add/delete path,
>> default to no existing membership. In the post-processing path, skip
>> the address update to avoid corrupting FDB entries with garbage values.
>>
>> VLAN Interface Handling:
>> Add support for multicast addresses added to VLAN interfaces on the HSR
>> master (e.g., hsr0.7). These addresses require P0 (HSR master) bit to be
>> set along with the port bits, since VLAN-tagged packets use separate FDB
>> entries per VLAN ID. Without P0, the HSR master would not receive
>> multicast packets on VLAN interfaces.
>>
>> Track whether the add/del operation came from a VLAN interface path and
>> set P0 when in HSR offload mode with VLAN interfaces. Update orphaned P0
>> cleanup logic to preserve P0 for VLAN interfaces.
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@xxxxxx>
> 
> Hi,
> 
> This patch is doing quite a lot.
> I understand that there is a relationship between those things.
> But I am wondering if you considered breaking the patch up.
> Or is there too much interdependency going on?
> 

I had thought of breaking this patch but there is too much
inter-dependency. It makes more sense to have all these changes in a
single patch

>> ---
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 233 +++++++++++++++++--
>>  1 file changed, 217 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> 
> ...
> 
>> @@ -1077,8 +1191,95 @@ static void emac_ndo_set_rx_mode_work(struct work_struct *work)
>>  	}
>>  
>>  	if (emac->prueth->is_hsr_offload_mode) {
> 
> In a similar vein, this condition is now doing a lot:
> about 90 lines more than it used to.
> 
> I'm wondering if you considered placing the new logic in a separate function?
> 

This logic can definitely go in a separate function. I will make this
change and send a v2 shortly.

Let me know if you have any other comment.

-- 
Thanks and Regards,
Danish