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;
