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

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

[Fernando Fernandez Mancera]

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);
-- 
2.49.0

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed,  9 Apr 2025 12:19:11 +0200 you 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>
> 
> [...]

Here is the summary with links:
  - [v3,net-next] net: hsr: sync hw addr of slave2 according to slave1 hw addr on PRP
    https://git.kernel.org/netdev/net-next/c/b65999e7238e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Luka Gejak]

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.

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.

Please address these points and submit a v4.

Best regards,
Luka Gejak

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

[Fernando Fernandez Mancera]

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
> 

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

[Luka Gejak]

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

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

[Fernando Fernandez Mancera]

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.

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

[Luka Gejak]

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.

Best regards,
Luka

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

[Fernando Fernandez Mancera]

On 3/29/26 7:32 PM, Luka Gejak wrote:
> 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)

I am sorry I do not follow here. eth_hw_addr_set() path ends up calling 
dev_addr_mod() which modifies dev->dev_addr indeed.

Thanks,
Fernando.

  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.
> 
> Best regards,
> Luka
> 

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

[Łukasz Majewski]

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