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

[Luka Gejak <luka.gejak@linux.dev>]

On March 29, 2026 6:58:38 PM GMT+02:00, Fernando Fernandez Mancera <fmancera@suse.de> 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.
>
>> 3. Coding Style
>> In hsr_main.h:
>> 
>>    unsigned char original_macaddress[ETH_ALEN];
>> 
>> Please use u8 instead of unsigned char for byte arrays to align with
>> standard netdev coding styles.
>> 
>
>I used `unsigned char` because it was already used around HSR code already, so I chose consistency. If there is a preference for u8 over unsigned char I could change it.
>
>> Please address these points and submit a v4.
>> 
>
>Well, this is almost a year old so I am sending a fix instead. Is this some kind of automated bot going through all the patches in netdev mailing list? I am a bit confused about receiving a patch review a year later.
>
>> Best regards,
>> Luka Gejak
>> 
>

Hi Fernando,
I was setting up lei for the mailing list and because I didn't set it 
up correctly, patches that were filtered and downloaded by lei were 
older and I noticed it only later after carefully checking why patch 
count was low.
Best regards,
Luka Gejak