Skip to content

Commit

Permalink
bonding: fix 802.3ad standards compliance error
Browse files Browse the repository at this point in the history 
The language of 802.3ad 43.4.9 requires the "recordPDU" function
to, in part, compare the Partner parameter values in a received LACPDU
to the stored Actor values.  If those match, then the Partner's
synchronization state is set to true.

	The current 802.3ad implementation is performing these steps out
of order; first, the synchronization check is done, then the paramters are
checked to see if they match (the synch check being done against a match
check of a prior LACPDU).  This causes delays in establishing aggregators
in some circumstances.

	This patch modifies the 802.3ad code to call __choose_matched,
the function that does the "match" comparisions, as the first step of
__record_pdu, instead of immediately afterwards.  This new behavior is
in compliance with the language of the standard.

	Some additional commentary relating to code vs. standard is also
added.

	Reported by Martin Patterson <martin@gear6.com> who also supplied
the logic of the fix and verified the patch.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
Jay Vosburgh authored and David S. Miller committed Nov 16, 2009
1 parent b93ab83 commit 2d6682d
Showing 1 changed file with 43 additions and 42 deletions.
85 changes: 43 additions & 42 deletions drivers/net/bonding/bond_3ad.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,48 @@ static u16 __ad_timer_to_ticks(u16 timer_type, u16 par)
// ================= ad_rx_machine helper functions ==================
/////////////////////////////////////////////////////////////////////////////////

/**
* __choose_matched - update a port's matched variable from a received lacpdu
* @lacpdu: the lacpdu we've received
* @port: the port we're looking at
*
* Update the value of the matched variable, using parameter values from a
* newly received lacpdu. Parameter values for the partner carried in the
* received PDU are compared with the corresponding operational parameter
* values for the actor. Matched is set to TRUE if all of these parameters
* match and the PDU parameter partner_state.aggregation has the same value as
* actor_oper_port_state.aggregation and lacp will actively maintain the link
* in the aggregation. Matched is also set to TRUE if the value of
* actor_state.aggregation in the received PDU is set to FALSE, i.e., indicates
* an individual link and lacp will actively maintain the link. Otherwise,
* matched is set to FALSE. LACP is considered to be actively maintaining the
* link if either the PDU's actor_state.lacp_activity variable is TRUE or both
* the actor's actor_oper_port_state.lacp_activity and the PDU's
* partner_state.lacp_activity variables are TRUE.
*
* Note: the AD_PORT_MATCHED "variable" is not specified by 802.3ad; it is
* used here to implement the language from 802.3ad 43.4.9 that requires
* recordPDU to "match" the LACPDU parameters to the stored values.
*/
static void __choose_matched(struct lacpdu *lacpdu, struct port *port)
{
// check if all parameters are alike
if (((ntohs(lacpdu->partner_port) == port->actor_port_number) &&
(ntohs(lacpdu->partner_port_priority) == port->actor_port_priority) &&
!MAC_ADDRESS_COMPARE(&(lacpdu->partner_system), &(port->actor_system)) &&
(ntohs(lacpdu->partner_system_priority) == port->actor_system_priority) &&
(ntohs(lacpdu->partner_key) == port->actor_oper_port_key) &&
((lacpdu->partner_state & AD_STATE_AGGREGATION) == (port->actor_oper_port_state & AD_STATE_AGGREGATION))) ||
// or this is individual link(aggregation == FALSE)
((lacpdu->actor_state & AD_STATE_AGGREGATION) == 0)
) {
// update the state machine Matched variable
port->sm_vars |= AD_PORT_MATCHED;
} else {
port->sm_vars &= ~AD_PORT_MATCHED;
}
}

/**
* __record_pdu - record parameters from a received lacpdu
* @lacpdu: the lacpdu we've received
Expand All @@ -459,6 +501,7 @@ static void __record_pdu(struct lacpdu *lacpdu, struct port *port)
if (lacpdu && port) {
struct port_params *partner = &port->partner_oper;

__choose_matched(lacpdu, port);
// record the new parameter values for the partner operational
partner->port_number = ntohs(lacpdu->actor_port);
partner->port_priority = ntohs(lacpdu->actor_port_priority);
Expand Down Expand Up @@ -562,47 +605,6 @@ static void __update_default_selected(struct port *port)
}
}

/**
* __choose_matched - update a port's matched variable from a received lacpdu
* @lacpdu: the lacpdu we've received
* @port: the port we're looking at
*
* Update the value of the matched variable, using parameter values from a
* newly received lacpdu. Parameter values for the partner carried in the
* received PDU are compared with the corresponding operational parameter
* values for the actor. Matched is set to TRUE if all of these parameters
* match and the PDU parameter partner_state.aggregation has the same value as
* actor_oper_port_state.aggregation and lacp will actively maintain the link
* in the aggregation. Matched is also set to TRUE if the value of
* actor_state.aggregation in the received PDU is set to FALSE, i.e., indicates
* an individual link and lacp will actively maintain the link. Otherwise,
* matched is set to FALSE. LACP is considered to be actively maintaining the
* link if either the PDU's actor_state.lacp_activity variable is TRUE or both
* the actor's actor_oper_port_state.lacp_activity and the PDU's
* partner_state.lacp_activity variables are TRUE.
*/
static void __choose_matched(struct lacpdu *lacpdu, struct port *port)
{
// validate lacpdu and port
if (lacpdu && port) {
// check if all parameters are alike
if (((ntohs(lacpdu->partner_port) == port->actor_port_number) &&
(ntohs(lacpdu->partner_port_priority) == port->actor_port_priority) &&
!MAC_ADDRESS_COMPARE(&(lacpdu->partner_system), &(port->actor_system)) &&
(ntohs(lacpdu->partner_system_priority) == port->actor_system_priority) &&
(ntohs(lacpdu->partner_key) == port->actor_oper_port_key) &&
((lacpdu->partner_state & AD_STATE_AGGREGATION) == (port->actor_oper_port_state & AD_STATE_AGGREGATION))) ||
// or this is individual link(aggregation == FALSE)
((lacpdu->actor_state & AD_STATE_AGGREGATION) == 0)
) {
// update the state machine Matched variable
port->sm_vars |= AD_PORT_MATCHED;
} else {
port->sm_vars &= ~AD_PORT_MATCHED;
}
}
}

/**
* __update_ntt - update a port's ntt variable from a received lacpdu
* @lacpdu: the lacpdu we've received
Expand Down Expand Up @@ -1134,7 +1136,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
__update_selected(lacpdu, port);
__update_ntt(lacpdu, port);
__record_pdu(lacpdu, port);
__choose_matched(lacpdu, port);
port->sm_rx_timer_counter = __ad_timer_to_ticks(AD_CURRENT_WHILE_TIMER, (u16)(port->actor_oper_port_state & AD_STATE_LACP_TIMEOUT));
port->actor_oper_port_state &= ~AD_STATE_EXPIRED;
// verify that if the aggregator is enabled, the port is enabled too.
Expand Down

0 comments on commit 2d6682d

Please sign in to comment.