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

[Hangbin Liu]

On Thu, Dec 12, 2024 at 11:43:15AM +0200, Nikolay Aleksandrov wrote:
> >>> --- 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.

OK, I will drop it.

Thanks
Hangbin