Re: [PATCH net-next v2 2/4] net: hsr: fix VLAN add unwind on slave errors

[Luka Gejak]

On March 27, 2026 12:52:02 PM GMT+01:00, Felix Maurer <fmaurer@xxxxxxxxxx> wrote:
>Thank you for updating this patch!
>
>On Thu, Mar 26, 2026 at 04:47:13PM +0100, luka.gejak@xxxxxxxxx wrote:
>> From: Luka Gejak <luka.gejak@xxxxxxxxx>
>>
>> When vlan_vid_add() fails for a secondary slave, the error path calls
>> vlan_vid_del() on the failing port instead of the peer slave that had
>> already succeeded. This results in asymmetric VLAN state across the HSR
>> pair.
>>
>> Fix this by switching to a centralized unwind path that removes the VID
>> from any slave device that was already programmed.
>>
>> Signed-off-by: Luka Gejak <luka.gejak@xxxxxxxxx>
>> ---
>>  net/hsr/hsr_device.c | 46 ++++++++++++++++++++++++++------------------
>>  1 file changed, 27 insertions(+), 19 deletions(-)
>>
>> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
>> index 5c3eca2235ce..75c491279df8 100644
>> --- a/net/hsr/hsr_device.c
>> +++ b/net/hsr/hsr_device.c
>> @@ -532,8 +532,8 @@ static void hsr_change_rx_flags(struct net_device *dev, int change)
>>  static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
>>  				   __be16 proto, u16 vid)
>>  {
>> -	bool is_slave_a_added = false;
>> -	bool is_slave_b_added = false;
>> +	struct net_device *slave_a_dev = NULL;
>> +	struct net_device *slave_b_dev = NULL;
>>  	struct hsr_port *port;
>>  	struct hsr_priv *hsr;
>>  	int ret = 0;
>> @@ -546,29 +546,28 @@ static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
>>  			continue;
>>
>>  		ret = vlan_vid_add(port->dev, proto, vid);
>> -		switch (port->type) {
>> -		case HSR_PT_SLAVE_A:
>> -			if (ret) {
>> -				/* clean up Slave-B */
>> +		if (ret) {
>> +			switch (port->type) {
>> +			case HSR_PT_SLAVE_A:
>>  				netdev_err(dev, "add vid failed for Slave-A\n");
>> -				if (is_slave_b_added)
>> -					vlan_vid_del(port->dev, proto, vid);
>> -				return ret;
>> +				break;
>> +			case HSR_PT_SLAVE_B:
>> +				netdev_err(dev, "add vid failed for Slave-B\n");
>> +				break;
>> +			default:
>> +				break;
>>  			}
>>
>> -			is_slave_a_added = true;
>> +			goto unwind;
>> +		}
>> +
>> +		switch (port->type) {
>> +		case HSR_PT_SLAVE_A:
>> +			slave_a_dev = port->dev;
>>  			break;
>>
>
>nit: superflous empty line (it's inconsistent with the other case
>blocks)
>
>>  		case HSR_PT_SLAVE_B:
>> -			if (ret) {
>> -				/* clean up Slave-A */
>> -				netdev_err(dev, "add vid failed for Slave-B\n");
>> -				if (is_slave_a_added)
>> -					vlan_vid_del(port->dev, proto, vid);
>> -				return ret;
>> -			}
>> -
>> -			is_slave_b_added = true;
>> +			slave_b_dev = port->dev;
>>  			break;
>>  		default:
>>  			break;
>
>I think this would look cleaner with the good and the bad paths in the
>same switch-case, but that may be a matter of (my) taste. What do you
>think?
>
>Thanks,
>   Felix
>

Hi Felix,
Thanks for taking a look at v2.
>nit: superflous empty line (it's inconsistent with the other case 
>blocks)
Good catch. I'll drop the extra newline in v3.
>I think this would look cleaner with the good and the bad paths in 
>the same switch-case, but that may be a matter of (my) taste. What do
> you think?
Very well. I will take your preference into consideration and will 
make mentioned changes in v3. However I am not currently home and will
send v3 once I come home in 1-2 days.
Best regards,
Luka Gejak