Re: [PATCH net v4 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 9cf064243d58..bc2964ea11f5 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -1053,6 +1053,8 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
> 
> 	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;

	Technically, this is not exactly following the state machines.

	The mux machine should transition to WAITING from DETACHED when
Selected == SELECTED or STANDBY, not for !is_enabled ("port_enabled" in
the standard).  The check for !is_enabled happens in the receive
machine, and it would transition to PORT_DISABLED state (which clears
Synchronization).

	I'm not sure if this is actually an issue or not; I need to read
the relevant bits again to make sure I understand how it's supposed to
work.

	-J

> 	} else {
> 		switch (port->sm_mux_state) {
> 		case AD_MUX_DETACHED:
>@@ -1200,6 +1202,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 (port->aggregator->is_active)
>-- 
>2.39.2
>

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