On Fri, Sep 29, 2017 at 1:08 PM, Stephen Hemminger
<step...@networkplumber.org> wrote:
> On Wed, 27 Sep 2017 18:03:49 -0700
> Mahesh Bandewar <mah...@bandewar.net> wrote:
>
>> From: Mahesh Bandewar <mahe...@google.com>
>>
>> Some NIC drivers don't have correct speed/duplex settings at the
>> time they send NETDEV_UP notification and that messes up the
>> bonding state. Especially 802.3ad mode which is very sensitive
>> to these settings. In the current implementation we invoke
>> bond_update_speed_duplex() when we receive NETDEV_UP, however,
>> ignore the return value. If the values we get are invalid
>> (UNKNOWN), then slave gets removed from the aggregator with
>> speed and duplex set to UNKNOWN while link is still marked as UP.
>>
>> This patch fixes this scenario. Also 802.3ad mode is sensitive to
>> these conditions while other modes are not, so making sure that it
>> doesn't change the behavior for other modes.
>>
>> Signed-off-by: Mahesh Bandewar <mahe...@google.com>
>> ---
>> drivers/net/bonding/bond_main.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c
>> b/drivers/net/bonding/bond_main.c
>> index b7313c1d9dcd..177be373966b 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -3076,7 +3076,16 @@ static int bond_slave_netdev_event(unsigned long
>> event,
>> break;
>> case NETDEV_UP:
>> case NETDEV_CHANGE:
>> - bond_update_speed_duplex(slave);
>> + /* For 802.3ad mode only:
>> + * Getting invalid Speed/Duplex values here will put slave
>> + * in weird state. So mark it as link-down for the time
>> + * being and let link-monitoring (miimon) set it right when
>> + * correct speeds/duplex are available.
>> + */
>> + if (bond_update_speed_duplex(slave) &&
>> + BOND_MODE(bond) == BOND_MODE_8023AD)
>> + slave->link = BOND_LINK_DOWN;
>> +
>> if (BOND_MODE(bond) == BOND_MODE_8023AD)
>> bond_3ad_adapter_speed_duplex_changed(slave);
>> /* Fallthrough */
>
> Then fix the drivers. Trying to workaround it here isn't helping.
>
This is not a workaround. It avoids bonding state being weird.
> The problem is that miimon is not required. Bonding can be purely
> event driven.
>
really? Here is a code-snippet from bonding itself -
/* reset values for 802.3ad/TLB/ALB */
if (!bond_mode_uses_arp(bond_mode)) {
if (!miimon) {
pr_warn("Warning: miimon must be specified,
otherwise bonding will not detect link failure, speed and duplex which
are essential for 802.3ad operation\n");
pr_warn("Forcing miimon to 100msec\n");
miimon = BOND_DEFAULT_MIIMON;
}
}
