[PATCH net-next v1] bonding: Fix another case of LACPDU not sent on slave

[PATCH net-next v1] bonding: Fix another case of LACPDU not sent on slave

[Mahesh Bandewar]

When mii-mon discovers that the link is up, it will call
bond_3ad_handle_link_change() but we forget to add the LACP_ENABLED
flag when we discover the speed and duplex for the slave link are
normal.

Change-Id: Ie8b268ecfeea0f99bf9fdcd72706c0653f9d9e49
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
 drivers/net/bonding/bond_3ad.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 4309b5a76708..30e8d118664b 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2436,12 +2436,15 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
 		port->actor_admin_port_key &= ~AD_SPEED_KEY_MASKS;
 		port->actor_oper_port_key = port->actor_admin_port_key |=
 			(__get_link_speed(port) << 1);
+		if (port->actor_oper_port_key & AD_DUPLEX_KEY_MASKS)
+			port->sm_vars |= AD_PORT_LACP_ENABLED;
 	} else {
 		/* link has failed */
 		port->is_enabled = false;
 		port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
 		port->actor_oper_port_key = (port->actor_admin_port_key &=
 					     ~AD_SPEED_KEY_MASKS);
+		port->sm_vars &= ~AD_PORT_LACP_ENABLED;
 	}
 	netdev_dbg(slave->bond->dev, "Port %d changed link status to %s\n",
 		   port->actor_port_number,
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH net-next v1] bonding: Fix another case of LACPDU not sent on slave

[Andy Gospodarek]

On Mon, Mar 30, 2015 at 02:31:16PM -0700, Mahesh Bandewar wrote:
> When mii-mon discovers that the link is up, it will call
> bond_3ad_handle_link_change() but we forget to add the LACP_ENABLED
> flag when we discover the speed and duplex for the slave link are
> normal.
> 
> Change-Id: Ie8b268ecfeea0f99bf9fdcd72706c0653f9d9e49
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>

This won't make the state machine start-up more quickly will it?  Either
way...

Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>


> ---
>  drivers/net/bonding/bond_3ad.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 4309b5a76708..30e8d118664b 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -2436,12 +2436,15 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
>  		port->actor_admin_port_key &= ~AD_SPEED_KEY_MASKS;
>  		port->actor_oper_port_key = port->actor_admin_port_key |=
>  			(__get_link_speed(port) << 1);
> +		if (port->actor_oper_port_key & AD_DUPLEX_KEY_MASKS)
> +			port->sm_vars |= AD_PORT_LACP_ENABLED;
>  	} else {
>  		/* link has failed */
>  		port->is_enabled = false;
>  		port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
>  		port->actor_oper_port_key = (port->actor_admin_port_key &=
>  					     ~AD_SPEED_KEY_MASKS);
> +		port->sm_vars &= ~AD_PORT_LACP_ENABLED;
>  	}
>  	netdev_dbg(slave->bond->dev, "Port %d changed link status to %s\n",
>  		   port->actor_port_number,
> -- 
> 2.2.0.rc0.207.ga3a616c
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v1] bonding: Fix another case of LACPDU not sent on slave

[Mahesh Bandewar]

On Tue, Mar 31, 2015 at 8:59 AM, Andy Gospodarek
<gospo@cumulusnetworks.com> wrote:
> On Mon, Mar 30, 2015 at 02:31:16PM -0700, Mahesh Bandewar wrote:
>> When mii-mon discovers that the link is up, it will call
>> bond_3ad_handle_link_change() but we forget to add the LACP_ENABLED
>> flag when we discover the speed and duplex for the slave link are
>> normal.
>>
>> Change-Id: Ie8b268ecfeea0f99bf9fdcd72706c0653f9d9e49
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>
> This won't make the state machine start-up more quickly will it?  Either
> way...
>
I'm seeing some occasions where LACP state for a port is in weird
state. The port has correct speed and duplex but there are no LACPDUs
are exchanged with partner. Seems like there is some sort of race
condition in the code and trying to hunt that down.

This definitely looks like a place where it would happen since miimon
will detect and update the speed and duplex for the port but lack of
ENABLE_LACP flag will prevent state-machine from sending LACPDU. This
is especially a big problem when the partner is in passive mode! This
takes port out of the active aggregation group so unless there is
another link-event on this port, it will not join the aggregator
again.

So the idea behind this patch is not to expedite the state-machine run
but to make it correctly.

> Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>
>
>
>> ---
>>  drivers/net/bonding/bond_3ad.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index 4309b5a76708..30e8d118664b 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -2436,12 +2436,15 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
>>               port->actor_admin_port_key &= ~AD_SPEED_KEY_MASKS;
>>               port->actor_oper_port_key = port->actor_admin_port_key |=
>>                       (__get_link_speed(port) << 1);
>> +             if (port->actor_oper_port_key & AD_DUPLEX_KEY_MASKS)
>> +                     port->sm_vars |= AD_PORT_LACP_ENABLED;
>>       } else {
>>               /* link has failed */
>>               port->is_enabled = false;
>>               port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
>>               port->actor_oper_port_key = (port->actor_admin_port_key &=
>>                                            ~AD_SPEED_KEY_MASKS);
>> +             port->sm_vars &= ~AD_PORT_LACP_ENABLED;
>>       }
>>       netdev_dbg(slave->bond->dev, "Port %d changed link status to %s\n",
>>                  port->actor_port_number,
>> --
>> 2.2.0.rc0.207.ga3a616c
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html