Re: [PATCH net 1/2] bonding: fix xfrm offload feature setup on active-backup mode

[Nikolay Aleksandrov]

On 12/12/24 11:39, Hangbin Liu wrote:
> On Thu, Dec 12, 2024 at 11:19:33AM +0200, Nikolay Aleksandrov wrote:
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 49dd4fe195e5..7daeab67e7b5 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -4389,7 +4389,7 @@ void bond_work_init_all(struct bonding *bond)
>>>  	INIT_DELAYED_WORK(&bond->slave_arr_work, bond_slave_arr_handler);
>>>  }
>>>  
>>> -static void bond_work_cancel_all(struct bonding *bond)
>>> +void bond_work_cancel_all(struct bonding *bond)
>>>  {
>>>  	cancel_delayed_work_sync(&bond->mii_work);
>>>  	cancel_delayed_work_sync(&bond->arp_work);
>>> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>>> index 2a6a424806aa..7fe8c62366eb 100644
>>> --- a/drivers/net/bonding/bond_netlink.c
>>> +++ b/drivers/net/bonding/bond_netlink.c
>>> @@ -568,18 +568,21 @@ static int bond_newlink(struct net *src_net, struct net_device *bond_dev,
>>>  			struct nlattr *tb[], struct nlattr *data[],
>>>  			struct netlink_ext_ack *extack)
>>>  {
>>> +	struct bonding *bond = netdev_priv(bond_dev);
>>>  	int err;
>>>  
>>> -	err = bond_changelink(bond_dev, tb, data, extack);
>>> -	if (err < 0)
>>> +	err = register_netdevice(bond_dev);
>>> +	if (err)
>>>  		return err;
>>>  
>>> -	err = register_netdevice(bond_dev);
>>> -	if (!err) {
>>> -		struct bonding *bond = netdev_priv(bond_dev);
>>> +	netif_carrier_off(bond_dev);
>>> +	bond_work_init_all(bond);
>>>  
>>> -		netif_carrier_off(bond_dev);
>>> -		bond_work_init_all(bond);
>>> +	err = bond_changelink(bond_dev, tb, data, extack);
>>> +	if (err) {
>>> +		bond_work_cancel_all(bond);
>>> +		netif_carrier_on(bond_dev);
>>
>> The patch looks good, but I'm curious why the carrier on here?
> 
> The current code set netif_carrier_off(bond_dev) after register_netdevice()
> success, So I make it on if register failed.
> 
> Thanks
> hangbin

I don't like adding code just for symmetry alone, I think you should drop it
unless there is an actual reason to turn carrier on.

>>
>>> +		unregister_netdevice(bond_dev);
>>>  	}
>>>  
>>>  	return err;