Re: [PATCH v3 net-next] net: hsr: sync hw addr of slave2 according to slave1 hw addr on PRP

["Łukasz Majewski" <lukma@nabladev.com>]

Hi Luka,

> On Sun Mar 29, 2026 at 7:22 PM CEST, Fernando Fernandez Mancera wrote:
> > On 3/29/26 6:58 PM, Fernando Fernandez Mancera wrote:  
> >> On 3/29/26 5:07 PM, Luka Gejak wrote:  
> >>> On Wed Apr 9, 2025 at 12:19 PM CEST, Fernando Fernandez Mancera
> >>> wrote:  
> >>>> In order to work properly PRP requires slave1 and slave2 to
> >>>> share the same MAC address. To ease the configuration process on
> >>>> userspace tools, sync the slave2 MAC address with slave1. In
> >>>> addition, when deleting the port from the list, restore the
> >>>> original MAC address.
> >>>>
> >>>> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> >>>> ---
> >>>> v3: restore the original MAC address on slave removal.
> >>>> ---
> >>>>   net/hsr/hsr_device.c | 5 +++++
> >>>>   net/hsr/hsr_main.c   | 9 +++++++++
> >>>>   net/hsr/hsr_main.h   | 1 +
> >>>>   net/hsr/hsr_slave.c  | 2 ++
> >>>>   4 files changed, 17 insertions(+)
> >>>>
> >>>> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> >>>> index 439cfb7ad5d1..e8922e8d9398 100644
> >>>> --- a/net/hsr/hsr_device.c
> >>>> +++ b/net/hsr/hsr_device.c
> >>>> @@ -761,6 +761,11 @@ int hsr_dev_finalize(struct net_device
> >>>> *hsr_dev, struct net_device *slave[2],
> >>>>       if (res)
> >>>>           goto err_unregister;
> >>>> +    if (protocol_version == PRP_V1) {
> >>>> +        eth_hw_addr_set(slave[1], slave[0]->dev_addr);
> >>>> +        call_netdevice_notifiers(NETDEV_CHANGEADDR, slave[1]);
> >>>> +    }
> >>>> +
> >>>>       if (interlink) {
> >>>>           res = hsr_add_port(hsr, interlink, HSR_PT_INTERLINK,
> >>>> extack); if (res)
> >>>> diff --git a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
> >>>> index d7ae32473c41..192893c3f2ec 100644
> >>>> --- a/net/hsr/hsr_main.c
> >>>> +++ b/net/hsr/hsr_main.c
> >>>> @@ -78,6 +78,15 @@ static int hsr_netdev_notify(struct
> >>>> notifier_block *nb, unsigned long event,
> >>>>               eth_hw_addr_set(master->dev, dev->dev_addr);
> >>>>               call_netdevice_notifiers(NETDEV_CHANGEADDR,
> >>>>                            master->dev);
> >>>> +
> >>>> +            if (hsr->prot_version == PRP_V1) {
> >>>> +                port = hsr_port_get_hsr(hsr, HSR_PT_SLAVE_B);
> >>>> +                if (port) {
> >>>> +                    eth_hw_addr_set(port->dev, dev->dev_addr);
> >>>> +                    call_netdevice_notifiers(NETDEV_CHANGEADDR,
> >>>> +                                 port->dev);
> >>>> +                }
> >>>> +            }
> >>>>           }
> >>>>           /* Make sure we recognize frames from ourselves in 
> >>>> hsr_rcv() */
> >>>> diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
> >>>> index 1bc47b17a296..135ec5fce019 100644
> >>>> --- a/net/hsr/hsr_main.h
> >>>> +++ b/net/hsr/hsr_main.h
> >>>> @@ -155,6 +155,7 @@ struct hsr_port {
> >>>>       struct hsr_priv        *hsr;
> >>>>       enum hsr_port_type    type;
> >>>>       struct rcu_head        rcu;
> >>>> +    unsigned char        original_macaddress[ETH_ALEN];
> >>>>   };
> >>>>   struct hsr_frame_info;
> >>>> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> >>>> index 2a802a5de2ac..b87b6a6fe070 100644
> >>>> --- a/net/hsr/hsr_slave.c
> >>>> +++ b/net/hsr/hsr_slave.c
> >>>> @@ -196,6 +196,7 @@ int hsr_add_port(struct hsr_priv *hsr,
> >>>> struct net_device *dev,
> >>>>       port->hsr = hsr;
> >>>>       port->dev = dev;
> >>>>       port->type = type;
> >>>> +    ether_addr_copy(port->original_macaddress, dev->dev_addr);
> >>>>       if (type != HSR_PT_MASTER) {
> >>>>           res = hsr_portdev_setup(hsr, dev, port, extack);
> >>>> @@ -232,6 +233,7 @@ void hsr_del_port(struct hsr_port *port)
> >>>>           if (!port->hsr->fwd_offloaded)
> >>>>               dev_set_promiscuity(port->dev, -1);
> >>>>           netdev_upper_dev_unlink(port->dev, master->dev);
> >>>> +        eth_hw_addr_set(port->dev, port->original_macaddress);
> >>>>       }
> >>>>       kfree_rcu(port, rcu);  
> >>>
> >>> Hi Fernando,
> >>>
> >>> Thanks for the patch. The logic to sync the slave2 MAC address
> >>> with slave1 for PRP is sound, but there are a few implementation
> >>> issues here that will cause hardware filtering problems and style
> >>> violations.
> >>>
> >>> 1. Hardware MAC Filtering
> >>> In your current implementation:
> >>>
> >>>    if (protocol_version == PRP_V1) {
> >>>
> >>>        eth_hw_addr_set(slave[1], slave[0]->dev_addr);
> >>>
> >>>        call_netdevice_notifiers(NETDEV_CHANGEADDR, slave[1]);
> >>>
> >>>    }
> >>>
> >>> eth_hw_addr_set() is appropriate for virtual devices (like the
> >>> hsr_dev itself), but slave[1] is a physical net_device. By using
> >>> this helper, you are bypassing
> >>> dev->netdev_ops->ndo_set_mac_address. This means the physical
> >>> NIC's hardware MAC filter will not be programmed, and it will
> >>> silently drop unicast traffic destined for the new PRP MAC
> >>> address unless the interface happens to be in promiscuous mode.
> >>> You should use dev_set_mac_address() here instead. Note that
> >>> unlike eth_hw_addr_set(), dev_set_mac_address() can fail and
> >>> returns an error code, so you will need to check its return value
> >>> and handle the error path accordingly.
> >>>
> >>> 2. Teardown Notifiers
> >>> In hsr_del_port():
> >>>
> >>>    netdev_upper_dev_unlink(port->dev, master->dev);
> >>>    eth_hw_addr_set(port->dev, port->original_macaddress);
> >>>
> >>> You have the same issue with eth_hw_addr_set() here. Additionally,
> >>> unlike the setup paths, this version omits
> >>> call_netdevice_notifiers (NETDEV_CHANGEADDR, port->dev).
> >>> Switching to dev_set_mac_address() for the restoration will
> >>> handle both the hardware programming and the notifier chain
> >>> automatically. 
> >> 
> >> These points makes sense. I guess I didn't notice the that 
> >> eth_hw_addr_set() due to my interface being set to promiscuous
> >> mode at hsr_slave.c.
> >> 
> >> Let me research a bit more about the impact here and will get back
> >> to you either with a new patch or an reply here.
> >>   
> >
> > Does this actually makes sense? I am not an expert on how hardware
> > is handling offloading here. But if I am not wrong, as the
> > interface is set on promiscuous or offload enabled. For promiscuous
> > this does not matter and for the offloaded scenario shouldn't the
> > decision be handled by hardware and not depend on
> > ndo_set_mac_address()?
> >
> > Let me know if I am missing something here.
> >
> >
> > Anyway, the missing notification makes sense and should be fixed
> > for SLAVE_B.
> >
> > Thanks,
> > Fernando.  
> 
> Hi Fernando,
> You raise a fair point, and I should have looked more carefully at
> the existing code paths.
> 
> You are correct that in the non-offloaded case, hsr_slave.c already 
> sets promiscuous mode via dev_set_promiscuity(dev, 1), so the
> hardware MAC filter is bypassed and eth_hw_addr_set() is sufficient
> for frame reception.
> 
> For the offloaded case, the comment in hsr_portdev_setup() notes that 
> "L2 frame forward happens at the offloaded hardware" — so the
> hardware is configured through its own HSR/PRP offload mechanisms,
> not through ndo_set_mac_address().
> 
> So the practical impact of my original concern is limited to edge
> cases where promiscuous mode might be disabled or fail. The more
> important issue, which you've already acknowledged, is the missing
> notifier in hsr_del_port(). That should definitely be fixed for
> correctness.
> 
> One additional consideration: even though promiscuous mode handles
> the receive path, dev->dev_addr is still used as the source MAC for 
> outgoing frames. Using dev_set_mac_address() would ensure the
> software state (dev->dev_addr) and any driver-level state remain
> consistent. But I agree this is less critical than I initially
> suggested. Thanks for pushing back on this — it's a good
> clarification.
> 

Thanks for your patches - as denx.de is not operational anymore (now I
do work for nabladev.com) I've just stumbled upon your patches.

I'm very happy that PRP now also gains support :-)

> Best regards,
> Luka


-- 
Best regards,

Lukasz Majewski

--
Nabla Software Engineering GmbH
HRB 40522 Augsburg
Phone: +49 821 45592596
E-Mail: office@nabladev.com
Managing Director : Stefano Babic