bonding: fix incorrect lacp mux state when agg not active
This patch attempts to fix the following problems when an actor or
partner's aggregator is not active:
1. a slave's lacp port state is marked as AD_STATE_SYNCHRONIZATION
even if it is attached to an inactive aggregator. LACP advertises
this state to the partner, making the partner think he can move
into COLLECTING_DISTRIBUTING state even though this link will not
pass traffic on the local side
2. a slave goes into COLLECTING_DISTRIBUTING state without checking
if the aggregator is actually active
3. when in COLLECTING_DISTRIBUTING state, the partner parameters may
change, e.g. the partner_oper_port_state.SYNCHRONIZATION. The
local mux machine is not reacting to the change and continue to
keep the slave and bond up
4. When bond slave leaves an inactive aggregator and joins an active
aggregator, the actor oper port state need to update to SYNC state.
v2:
* fix style issues in bond_3ad.c
Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 8baa87d..e3c96b2 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -467,11 +467,14 @@
/* set the partner sync. to on if the partner is sync,
* and the port is matched
*/
- if ((port->sm_vars & AD_PORT_MATCHED)
- && (lacpdu->actor_state & AD_STATE_SYNCHRONIZATION))
+ if ((port->sm_vars & AD_PORT_MATCHED) &&
+ (lacpdu->actor_state & AD_STATE_SYNCHRONIZATION)) {
partner->port_state |= AD_STATE_SYNCHRONIZATION;
- else
+ pr_debug("%s partner sync=1\n", port->slave->dev->name);
+ } else {
partner->port_state &= ~AD_STATE_SYNCHRONIZATION;
+ pr_debug("%s partner sync=0\n", port->slave->dev->name);
+ }
}
}
@@ -726,6 +729,8 @@
lacpdu->actor_port_priority = htons(port->actor_port_priority);
lacpdu->actor_port = htons(port->actor_port_number);
lacpdu->actor_state = port->actor_oper_port_state;
+ pr_debug("update lacpdu: %s, actor port state %x\n",
+ port->slave->dev->name, port->actor_oper_port_state);
/* lacpdu->reserved_3_1 initialized
* lacpdu->tlv_type_partner_info initialized
@@ -898,7 +903,9 @@
if ((port->sm_vars & AD_PORT_SELECTED) &&
(port->partner_oper.port_state & AD_STATE_SYNCHRONIZATION) &&
!__check_agg_selection_timer(port)) {
- port->sm_mux_state = AD_MUX_COLLECTING_DISTRIBUTING;
+ if (port->aggregator->is_active)
+ port->sm_mux_state =
+ AD_MUX_COLLECTING_DISTRIBUTING;
} else if (!(port->sm_vars & AD_PORT_SELECTED) ||
(port->sm_vars & AD_PORT_STANDBY)) {
/* if UNSELECTED or STANDBY */
@@ -910,12 +917,16 @@
*/
__set_agg_ports_ready(port->aggregator, __agg_ports_are_ready(port->aggregator));
port->sm_mux_state = AD_MUX_DETACHED;
+ } else if (port->aggregator->is_active) {
+ port->actor_oper_port_state |=
+ AD_STATE_SYNCHRONIZATION;
}
break;
case AD_MUX_COLLECTING_DISTRIBUTING:
if (!(port->sm_vars & AD_PORT_SELECTED) ||
(port->sm_vars & AD_PORT_STANDBY) ||
- !(port->partner_oper.port_state & AD_STATE_SYNCHRONIZATION)) {
+ !(port->partner_oper.port_state & AD_STATE_SYNCHRONIZATION) ||
+ !(port->actor_oper_port_state & AD_STATE_SYNCHRONIZATION)) {
port->sm_mux_state = AD_MUX_ATTACHED;
} else {
/* if port state hasn't changed make
@@ -937,8 +948,10 @@
/* check if the state machine was changed */
if (port->sm_mux_state != last_state) {
- pr_debug("Mux Machine: Port=%d, Last State=%d, Curr State=%d\n",
- port->actor_port_number, last_state,
+ pr_debug("Mux Machine: Port=%d (%s), Last State=%d, Curr State=%d\n",
+ port->actor_port_number,
+ port->slave->dev->name,
+ last_state,
port->sm_mux_state);
switch (port->sm_mux_state) {
case AD_MUX_DETACHED:
@@ -953,7 +966,12 @@
port->sm_mux_timer_counter = __ad_timer_to_ticks(AD_WAIT_WHILE_TIMER, 0);
break;
case AD_MUX_ATTACHED:
- port->actor_oper_port_state |= AD_STATE_SYNCHRONIZATION;
+ if (port->aggregator->is_active)
+ port->actor_oper_port_state |=
+ AD_STATE_SYNCHRONIZATION;
+ else
+ port->actor_oper_port_state &=
+ ~AD_STATE_SYNCHRONIZATION;
port->actor_oper_port_state &= ~AD_STATE_COLLECTING;
port->actor_oper_port_state &= ~AD_STATE_DISTRIBUTING;
ad_disable_collecting_distributing(port,
@@ -963,6 +981,7 @@
case AD_MUX_COLLECTING_DISTRIBUTING:
port->actor_oper_port_state |= AD_STATE_COLLECTING;
port->actor_oper_port_state |= AD_STATE_DISTRIBUTING;
+ port->actor_oper_port_state |= AD_STATE_SYNCHRONIZATION;
ad_enable_collecting_distributing(port,
update_slave_arr);
port->ntt = true;
@@ -1044,8 +1063,10 @@
/* check if the State machine was changed or new lacpdu arrived */
if ((port->sm_rx_state != last_state) || (lacpdu)) {
- pr_debug("Rx Machine: Port=%d, Last State=%d, Curr State=%d\n",
- port->actor_port_number, last_state,
+ pr_debug("Rx Machine: Port=%d (%s), Last State=%d, Curr State=%d\n",
+ port->actor_port_number,
+ port->slave->dev->name,
+ last_state,
port->sm_rx_state);
switch (port->sm_rx_state) {
case AD_RX_INITIALIZE:
@@ -1394,6 +1415,9 @@
aggregator = __get_first_agg(port);
ad_agg_selection_logic(aggregator, update_slave_arr);
+
+ if (!port->aggregator->is_active)
+ port->actor_oper_port_state &= ~AD_STATE_SYNCHRONIZATION;
}
/* Decide if "agg" is a better choice for the new active aggregator that