Re: [PATCH net v3 3/5] 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
>valid 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.

	Instead of "valid," I would suggest "usable."

>The interface should not transition to carrier up until LACP
>negotiation is complete.

	If the new option is off, i.e., does not require successful LACP
negotiation, it should wait some time before asserting carrier up.  If
negotiation fails, however, how long should it wait?

>This happens because the actor and partner port states remain in
>collecting (and 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 now depends on the LACP negotiation state, it causes the
>interface to flap.

	"now depends" -> "may depend"

>Fix this by unsetting the SELECTED flag when a port goes down so that
>the mux state machine transitions through ATTACHED and DETACHED,
>which clears the actor collecting and distributing flags. Do not
>attempt to set the SELECTED flag if the port is still disabled.
>
>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 b79a76296966..3a94fbcbf721 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -1570,6 +1570,12 @@ static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
> 	struct slave *slave;
> 	int found = 0;
> 
>+	/* Disabled ports cannot be SELECTED.
>+	 * Do not attempt to set the SELECTED flag if the port is still disabled.
>+	 */
>+	if (!port->is_enabled)
>+		return;
>+

	I think the change is fine, but the comment seems redundant to
me.

	-J

> 	/* if the port is already Selected, do nothing */
> 	if (port->sm_vars & AD_PORT_SELECTED)
> 		return;
>@@ -2794,6 +2800,7 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
> 		/* link has failed */
> 		port->is_enabled = false;
> 		ad_update_actor_keys(port, true);
>+		port->sm_vars &= ~AD_PORT_SELECTED;
> 	}
> 	agg = __get_first_agg(port);
> 	ad_agg_selection_logic(agg, &dummy);
>-- 
>2.39.2
>

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