[PATCH net-next v2 0/5] bonding: various 802.3ad fixes

[PATCH net-next v2 0/5] bonding: various 802.3ad fixes

[Jonathan Toppins]

This patch series is a forward porting of patches we (Cumulus) are shipping
in our 3.2 series kernels. These fixes attempt to make 802.3ad bonding mode
more predictable in certian state machine transtions in addition to fixing
802.3ad bond carrier determination when bonding min_links option is changed.
Specific notes are contained within each patch.

For this patch series there are no userspace facing changes, a diff between
the modinfo output showed no difference. However, there are behavioral
facing changes, primarily in the bond carrier state. Please make sure to
review carefully.

v2:
 * fixed some style issues
 * dropped a portion of patch 1 in favor of more testing on my side

Jonathan Toppins (2):
  bonding: update bond carrier state when min_links option changes
  bonding: cleanup and remove dead code

Satish Ashok (1):
  bonding: fix LACP PDU not sent on slave port sometimes

Wilson Kok (2):
  bonding: fix bond_open() don't always set slave active flag
  bonding: fix incorrect lacp mux state when agg not active

 drivers/net/bonding/bond_3ad.c     |   55 +++++++++++++++++++++++++++---------
 drivers/net/bonding/bond_main.c    |    6 ++--
 drivers/net/bonding/bond_options.c |    1 +
 include/net/bond_3ad.h             |    1 -
 include/net/bonding.h              |    1 +
 5 files changed, 47 insertions(+), 17 deletions(-)

-- 
1.7.10.4

[PATCH net-next v2 1/5] bonding: update bond carrier state when min_links option changes

[Jonathan Toppins]

Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 drivers/net/bonding/bond_main.c    |    2 +-
 drivers/net/bonding/bond_options.c |    1 +
 include/net/bonding.h              |    1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 0dceba1..02ffedb 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -334,7 +334,7 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
  *
  * Returns zero if carrier state does not change, nonzero if it does.
  */
-static int bond_set_carrier(struct bonding *bond)
+int bond_set_carrier(struct bonding *bond)
 {
 	struct list_head *iter;
 	struct slave *slave;
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 9bd538d4..4df2894 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond,
 	netdev_info(bond->dev, "Setting min links value to %llu\n",
 		    newval->value);
 	bond->params.min_links = newval->value;
+	bond_set_carrier(bond);
 
 	return 0;
 }
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 983a94b..29f53ea 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -525,6 +525,7 @@ void bond_sysfs_slave_del(struct slave *slave);
 int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev);
 int bond_release(struct net_device *bond_dev, struct net_device *slave_dev);
 u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb);
+int bond_set_carrier(struct bonding *bond);
 void bond_select_active_slave(struct bonding *bond);
 void bond_change_active_slave(struct bonding *bond, struct slave *new_active);
 void bond_create_debugfs(void);
-- 
1.7.10.4

Re: [PATCH net-next v2 1/5] bonding: update bond carrier state when min_links option changes

[Jay Vosburgh]

Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:

>Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
>Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>

Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>


> drivers/net/bonding/bond_main.c    |    2 +-
> drivers/net/bonding/bond_options.c |    1 +
> include/net/bonding.h              |    1 +
> 3 files changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 0dceba1..02ffedb 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -334,7 +334,7 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
>  *
>  * Returns zero if carrier state does not change, nonzero if it does.
>  */
>-static int bond_set_carrier(struct bonding *bond)
>+int bond_set_carrier(struct bonding *bond)
> {
> 	struct list_head *iter;
> 	struct slave *slave;
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index 9bd538d4..4df2894 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond,
> 	netdev_info(bond->dev, "Setting min links value to %llu\n",
> 		    newval->value);
> 	bond->params.min_links = newval->value;
>+	bond_set_carrier(bond);
> 
> 	return 0;
> }
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index 983a94b..29f53ea 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -525,6 +525,7 @@ void bond_sysfs_slave_del(struct slave *slave);
> int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev);
> int bond_release(struct net_device *bond_dev, struct net_device *slave_dev);
> u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb);
>+int bond_set_carrier(struct bonding *bond);
> void bond_select_active_slave(struct bonding *bond);
> void bond_change_active_slave(struct bonding *bond, struct slave *new_active);
> void bond_create_debugfs(void);
>-- 
>1.7.10.4
>

[PATCH net-next v2 2/5] bonding: fix bond_open() don't always set slave active flag

[Jonathan Toppins]

From: Wilson Kok <wkok@cumulusnetworks.com>

Mode 802.3ad, fix incorrect bond slave active state when slave is not in
active aggregator. During bond_open(), the bonding driver always sets
the slave active flag to true if the bond is not in active-backup, alb,
or tlb modes. Bonding should let the aggregator selection logic set the
active flag when in 802.3ad mode.

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>
---
 drivers/net/bonding/bond_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 02ffedb..c475d90 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3066,7 +3066,7 @@ static int bond_open(struct net_device *bond_dev)
 			    slave != rcu_access_pointer(bond->curr_active_slave)) {
 				bond_set_slave_inactive_flags(slave,
 							      BOND_SLAVE_NOTIFY_NOW);
-			} else {
+			} else if (BOND_MODE(bond) != BOND_MODE_8023AD) {
 				bond_set_slave_active_flags(slave,
 							    BOND_SLAVE_NOTIFY_NOW);
 			}
-- 
1.7.10.4

Re: [PATCH net-next v2 2/5] bonding: fix bond_open() don't always set slave active flag

[Jay Vosburgh]

Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:

>From: Wilson Kok <wkok@cumulusnetworks.com>
>
>Mode 802.3ad, fix incorrect bond slave active state when slave is not in
>active aggregator. During bond_open(), the bonding driver always sets
>the slave active flag to true if the bond is not in active-backup, alb,
>or tlb modes. Bonding should let the aggregator selection logic set the
>active flag when in 802.3ad mode.
>
>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: Jay Vosburgh <jay.vosburgh@canonical.com>

>---
> drivers/net/bonding/bond_main.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 02ffedb..c475d90 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3066,7 +3066,7 @@ static int bond_open(struct net_device *bond_dev)
> 			    slave != rcu_access_pointer(bond->curr_active_slave)) {
> 				bond_set_slave_inactive_flags(slave,
> 							      BOND_SLAVE_NOTIFY_NOW);
>-			} else {
>+			} else if (BOND_MODE(bond) != BOND_MODE_8023AD) {
> 				bond_set_slave_active_flags(slave,
> 							    BOND_SLAVE_NOTIFY_NOW);
> 			}
>-- 
>1.7.10.4
>

[PATCH net-next v2 3/5] bonding: fix incorrect lacp mux state when agg not active

[Jonathan Toppins]

From: Wilson Kok <wkok@cumulusnetworks.com>

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>
---
 drivers/net/bonding/bond_3ad.c |   44 +++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 10 deletions(-)

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 @@ static void __record_pdu(struct lacpdu *lacpdu, struct port *port)
 		/* 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 @@ static inline void __update_lacpdu_from_port(struct port *port)
 	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 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
 			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 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
 				 */
 				__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 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
 
 	/* 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 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
 			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 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
 		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 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
 
 	/* 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 @@ static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
 
 	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
-- 
1.7.10.4

Re: [PATCH net-next v2 3/5] bonding: fix incorrect lacp mux state when agg not active

[Jay Vosburgh]

Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:

>From: Wilson Kok <wkok@cumulusnetworks.com>
>
>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.

	This is a lot of subtle changes for one patch, and it's
difficult to read the patch and figure out which of the above points
corresponds to which code change(s).  I think this would have been
better as 2 or 3 patches, perhaps without the first blocks that merely
add debug statements.

	In general, though, I believe I understand the problems and the
solutions, and the solutions appear sound.

	For point 1, what it's doing is introducing an "ATTACHED-ish"
state, ultimately because the standard lacks the concept of "active" or
"inactive" aggregator.  From my reading, when the port transitions to
ATTACHED state, 5.4.15 "Mux machine" requires that it set SYNC at that
time.

	I understand why we would want to not enable SYNC for a port
that's a member of an aggregator that will not currently send or receive
traffic (is inactive in bonding-speak), but in my opinion we are
violating the standard and really should note that fact, and why we're
doing so, in the code.

	Point 2 is similar to point 1 (although here we're not
transitioning from ATTACHED to COLLECTING / COLLECTING_DISTRIBUTING
when the partner's SYNC is true); same comment applies.

	Points 3 and 4 look to be independent issues from points 1 and
2, and could be simple independent patches.

	-J


>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>
>---
> drivers/net/bonding/bond_3ad.c |   44 +++++++++++++++++++++++++++++++---------
> 1 file changed, 34 insertions(+), 10 deletions(-)
>
>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 @@ static void __record_pdu(struct lacpdu *lacpdu, struct port *port)
> 		/* 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 @@ static inline void __update_lacpdu_from_port(struct port *port)
> 	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 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
> 			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 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
> 				 */
> 				__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 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
> 
> 	/* 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 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
> 			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 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
> 		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 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
> 
> 	/* 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 @@ static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
> 
> 	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
>-- 
>1.7.10.4
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

[PATCH net-next v2 4/5] bonding: fix LACP PDU not sent on slave port sometimes

[Jonathan Toppins]

From: Satish Ashok <sashok@cumulusnetworks.com>

When a slave is added to a bond and it is not in full duplex mode,
AD_PORT_LACP_ENABLED flag is cleared, due to this LACP PDU is not sent
on slave. When the duplex is changed to full, the flag needs to be set
to send LACP PDU.

Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
Signed-off-by: Satish Ashok <sashok@cumulusnetworks.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 drivers/net/bonding/bond_3ad.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index e3c96b2..cfc4a9c 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2219,8 +2219,10 @@ static int bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave,
 		switch (lacpdu->subtype) {
 		case AD_TYPE_LACPDU:
 			ret = RX_HANDLER_CONSUMED;
-			netdev_dbg(slave->bond->dev, "Received LACPDU on port %d\n",
-				   port->actor_port_number);
+			netdev_dbg(slave->bond->dev,
+				   "Received LACPDU on port %d slave %s\n",
+				   port->actor_port_number,
+				   slave->dev->name);
 			/* Protect against concurrent state machines */
 			spin_lock(&slave->bond->mode_lock);
 			ad_rx_machine(lacpdu, port);
@@ -2312,7 +2314,10 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave)
 	port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
 	port->actor_oper_port_key = port->actor_admin_port_key |=
 		__get_duplex(port);
-	netdev_dbg(slave->bond->dev, "Port %d changed duplex\n", port->actor_port_number);
+	netdev_dbg(slave->bond->dev, "Port %d slave %s changed duplex\n",
+		   port->actor_port_number, slave->dev->name);
+	if (port->actor_oper_port_key & AD_DUPLEX_KEY_MASKS)
+		port->sm_vars |= AD_PORT_LACP_ENABLED;
 	/* there is no need to reselect a new aggregator, just signal the
 	 * state machines to reinitialize
 	 */
-- 
1.7.10.4

Re: [PATCH net-next v2 4/5] bonding: fix LACP PDU not sent on slave port sometimes

[Sergei Shtylyov]

Hello.

On 1/26/2015 9:17 AM, Jonathan Toppins wrote:

> From: Satish Ashok <sashok@cumulusnetworks.com>

> When a slave is added to a bond and it is not in full duplex mode,
> AD_PORT_LACP_ENABLED flag is cleared, due to this LACP PDU is not sent

    s/is not/not being/.

> on slave. When the duplex is changed to full, the flag needs to be set
> to send LACP PDU.

> Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
> Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
> Signed-off-by: Satish Ashok <sashok@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
>   drivers/net/bonding/bond_3ad.c |   11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index e3c96b2..cfc4a9c 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -2219,8 +2219,10 @@ static int bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave,
>   		switch (lacpdu->subtype) {
>   		case AD_TYPE_LACPDU:
>   			ret = RX_HANDLER_CONSUMED;
> -			netdev_dbg(slave->bond->dev, "Received LACPDU on port %d\n",
> -				   port->actor_port_number);
> +			netdev_dbg(slave->bond->dev,
> +				   "Received LACPDU on port %d slave %s\n",
> +				   port->actor_port_number,
> +				   slave->dev->name);
>   			/* Protect against concurrent state machines */
>   			spin_lock(&slave->bond->mode_lock);
>   			ad_rx_machine(lacpdu, port);
> @@ -2312,7 +2314,10 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave)
>   	port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
>   	port->actor_oper_port_key = port->actor_admin_port_key |=
>   		__get_duplex(port);
> -	netdev_dbg(slave->bond->dev, "Port %d changed duplex\n", port->actor_port_number);
> +	netdev_dbg(slave->bond->dev, "Port %d slave %s changed duplex\n",
> +		   port->actor_port_number, slave->dev->name);

    The above 2 changes seem unrelated/undocumented in the change log...

[...]

WBR, Sergei

Re: [PATCH net-next v2 4/5] bonding: fix LACP PDU not sent on slave port sometimes

[Jay Vosburgh]

Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:

>Hello.
>
>On 1/26/2015 9:17 AM, Jonathan Toppins wrote:
>
>> From: Satish Ashok <sashok@cumulusnetworks.com>
>
>> When a slave is added to a bond and it is not in full duplex mode,
>> AD_PORT_LACP_ENABLED flag is cleared, due to this LACP PDU is not sent
>
>   s/is not/not being/.

	I don't have an issue with the original text, or the updating of
nearby debug messages to include the device name (below).  Worst case
would be to respin and add a mention of this to the commit message.

	-J

Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>


>> on slave. When the duplex is changed to full, the flag needs to be set
>> to send LACP PDU.
>
>> Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
>> Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
>> Signed-off-by: Satish Ashok <sashok@cumulusnetworks.com>
>> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
>> ---
>>   drivers/net/bonding/bond_3ad.c |   11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index e3c96b2..cfc4a9c 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -2219,8 +2219,10 @@ static int bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave,
>>   		switch (lacpdu->subtype) {
>>   		case AD_TYPE_LACPDU:
>>   			ret = RX_HANDLER_CONSUMED;
>> -			netdev_dbg(slave->bond->dev, "Received LACPDU on port %d\n",
>> -				   port->actor_port_number);
>> +			netdev_dbg(slave->bond->dev,
>> +				   "Received LACPDU on port %d slave %s\n",
>> +				   port->actor_port_number,
>> +				   slave->dev->name);
>>   			/* Protect against concurrent state machines */
>>   			spin_lock(&slave->bond->mode_lock);
>>   			ad_rx_machine(lacpdu, port);
>> @@ -2312,7 +2314,10 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave)
>>   	port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
>>   	port->actor_oper_port_key = port->actor_admin_port_key |=
>>   		__get_duplex(port);
>> -	netdev_dbg(slave->bond->dev, "Port %d changed duplex\n", port->actor_port_number);
>> +	netdev_dbg(slave->bond->dev, "Port %d slave %s changed duplex\n",
>> +		   port->actor_port_number, slave->dev->name);
>
>   The above 2 changes seem unrelated/undocumented in the change log...
>
>[...]
>
>WBR, Sergei
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

[PATCH net-next v2 5/5] bonding: cleanup and remove dead code

[Jonathan Toppins]

fix sparse warning about non-static function

drivers/net/bonding/bond_main.c:3737:5: warning: symbol
'bond_3ad_xor_xmit' was not declared. Should it be static?

Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 drivers/net/bonding/bond_main.c |    2 +-
 include/net/bond_3ad.h          |    1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index c475d90..67437f3 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3734,7 +3734,7 @@ out:
  * usable slave array is formed in the control path. The xmit function
  * just calculates hash and sends the packet out.
  */
-int bond_3ad_xor_xmit(struct sk_buff *skb, struct net_device *dev)
+static int bond_3ad_xor_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct bonding *bond = netdev_priv(dev);
 	struct slave *slave;
diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h
index e01d903..f04cdbb 100644
--- a/include/net/bond_3ad.h
+++ b/include/net/bond_3ad.h
@@ -274,7 +274,6 @@ void bond_3ad_handle_link_change(struct slave *slave, char link);
 int  bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info);
 int  __bond_3ad_get_active_agg_info(struct bonding *bond,
 				    struct ad_info *ad_info);
-int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
 int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
 			 struct slave *slave);
 int bond_3ad_set_carrier(struct bonding *bond);
-- 
1.7.10.4

Re: [PATCH net-next v2 5/5] bonding: cleanup and remove dead code

[Jay Vosburgh]

Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote:

>fix sparse warning about non-static function
>
>drivers/net/bonding/bond_main.c:3737:5: warning: symbol
>'bond_3ad_xor_xmit' was not declared. Should it be static?
>
>Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
>Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>

Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>

>---
> drivers/net/bonding/bond_main.c |    2 +-
> include/net/bond_3ad.h          |    1 -
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index c475d90..67437f3 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3734,7 +3734,7 @@ out:
>  * usable slave array is formed in the control path. The xmit function
>  * just calculates hash and sends the packet out.
>  */
>-int bond_3ad_xor_xmit(struct sk_buff *skb, struct net_device *dev)
>+static int bond_3ad_xor_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> 	struct bonding *bond = netdev_priv(dev);
> 	struct slave *slave;
>diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h
>index e01d903..f04cdbb 100644
>--- a/include/net/bond_3ad.h
>+++ b/include/net/bond_3ad.h
>@@ -274,7 +274,6 @@ void bond_3ad_handle_link_change(struct slave *slave, char link);
> int  bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info);
> int  __bond_3ad_get_active_agg_info(struct bonding *bond,
> 				    struct ad_info *ad_info);
>-int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
> int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
> 			 struct slave *slave);
> int bond_3ad_set_carrier(struct bonding *bond);
>-- 
>1.7.10.4
>

Re: [PATCH net-next v2 0/5] bonding: various 802.3ad fixes

[Andy Gospodarek]

On Mon, Jan 26, 2015 at 01:16:56AM -0500, Jonathan Toppins wrote:
> This patch series is a forward porting of patches we (Cumulus) are shipping
> in our 3.2 series kernels. These fixes attempt to make 802.3ad bonding mode
> more predictable in certian state machine transtions in addition to fixing
> 802.3ad bond carrier determination when bonding min_links option is changed.
> Specific notes are contained within each patch.
> 
> For this patch series there are no userspace facing changes, a diff between
> the modinfo output showed no difference. However, there are behavioral
> facing changes, primarily in the bond carrier state. Please make sure to
> review carefully.
> 
> v2:
>  * fixed some style issues
>  * dropped a portion of patch 1 in favor of more testing on my side
> 
> Jonathan Toppins (2):
>   bonding: update bond carrier state when min_links option changes
>   bonding: cleanup and remove dead code
> 
> Satish Ashok (1):
>   bonding: fix LACP PDU not sent on slave port sometimes
> 
> Wilson Kok (2):
>   bonding: fix bond_open() don't always set slave active flag
>   bonding: fix incorrect lacp mux state when agg not active
> 
>  drivers/net/bonding/bond_3ad.c     |   55 +++++++++++++++++++++++++++---------
>  drivers/net/bonding/bond_main.c    |    6 ++--
>  drivers/net/bonding/bond_options.c |    1 +
>  include/net/bond_3ad.h             |    1 -
>  include/net/bonding.h              |    1 +
>  5 files changed, 47 insertions(+), 17 deletions(-)

Great work on this, Jon.

For the series:

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

Re: [PATCH net-next v2 0/5] bonding: various 802.3ad fixes

[David Miller]

From: Jonathan Toppins <jtoppins@cumulusnetworks.com>
Date: Mon, 26 Jan 2015 01:16:56 -0500

> This patch series is a forward porting of patches we (Cumulus) are shipping
> in our 3.2 series kernels. These fixes attempt to make 802.3ad bonding mode
> more predictable in certian state machine transtions in addition to fixing
> 802.3ad bond carrier determination when bonding min_links option is changed.
> Specific notes are contained within each patch.
> 
> For this patch series there are no userspace facing changes, a diff between
> the modinfo output showed no difference. However, there are behavioral
> facing changes, primarily in the bond carrier state. Please make sure to
> review carefully.
> 
> v2:
>  * fixed some style issues
>  * dropped a portion of patch 1 in favor of more testing on my side

Series applied, thanks.