Re: [PATCH net v5 3/4] bonding: 3ad: fix mux port state on oper down

[Jay Vosburgh <jv@jvosburgh.net>]

Louis Scalbert <louis.scalbert@6wind.com> wrote:

>When the bonding interface has carrier down due to the absence of
>usable slaves and a slave transitions from down to up, the bonding
>interface briefly goes carrier up, then down again, and finally up
>once LACP negotiates collecting and distributing on the port.
>
>When lacp_strict mode is on, the interface should not transition to
>carrier up until LACP negotiation is complete.
>
>This happens because the actor and partner port states remain in
>Collecting_Distributing when the port goes down. When the port
>comes back up, it temporarily remains in this state until LACP
>renegotiation occurs.
>
>Previously this was mostly cosmetic, but since the bonding carrier
>state may depend on the LACP negotiation state, it causes the
>interface to flap.
>
>Move an operationally down port to the Mux WAITING state and clear the
>Synchronization, Collecting, and Distributing states, in accordance with
>the 802.1AX Mux state machine diagram.
>
>Fixes: 655f8919d549 ("bonding: add min links parameter to 802.3ad")
>Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
>---
> drivers/net/bonding/bond_3ad.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index 1247a1e048df..b531f68a24b0 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -1055,6 +1055,8 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
> 	aggregator = rcu_dereference(port->aggregator);
> 	if (port->sm_vars & AD_PORT_BEGIN) {
> 		port->sm_mux_state = AD_MUX_DETACHED;
>+	} else if (!port->is_enabled && port->sm_mux_state != AD_MUX_DETACHED) {
>+		port->sm_mux_state = AD_MUX_WAITING;

	Repeating my comment here from the prior version of the patch
series, that this doesn't seem to comply with the standard.  Bonding
should comply with IEEE 802.1AX-2014, and I do not want to mix and match
from different versions of the standard.  The state machines differ
between versions, and I don't know if a half-and-half implementation
will function correctly.  The 2020 edition of the standard diverges
significantly from prior versions, and bonding is very close to the 2014
standard.

	-J



> 	} else {
> 		switch (port->sm_mux_state) {
> 		case AD_MUX_DETACHED:
>@@ -1202,6 +1204,11 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
> 			break;
> 		case AD_MUX_WAITING:
> 			port->sm_mux_timer_counter = __ad_timer_to_ticks(AD_WAIT_WHILE_TIMER, 0);
>+			port->actor_oper_port_state &= ~LACP_STATE_SYNCHRONIZATION;
>+			ad_disable_collecting_distributing(port,
>+							   update_slave_arr);
>+			port->actor_oper_port_state &= ~LACP_STATE_COLLECTING;
>+			port->actor_oper_port_state &= ~LACP_STATE_DISTRIBUTING;
> 			break;
> 		case AD_MUX_ATTACHED:
> 			if (aggregator->is_active)
>-- 
>2.39.2
>

---
	-Jay Vosburgh, jv@jvosburgh.net