Re: [PATCH net v3 2/5] bonding: 3ad: fix carrier when no valid slaves

[Jay Vosburgh <jv@jvosburgh.net>]

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

>Apply the "lacp_fallback" configuration from the previous commit.
>
>"lacp_fallback" mode "strict" asserts that the bonding master carrier
>only when at least 'min_links' slaves are in the collecting/distributing
>state (or collecting only if the coupled_control default behavior is
>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     | 26 ++++++++++++++++++++++++--
> drivers/net/bonding/bond_options.c |  1 +
> 2 files changed, 25 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index af7f74cfdc08..b79a76296966 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -745,6 +745,22 @@ static void __set_agg_ports_ready(struct aggregator *aggregator, int val)
> 	}
> }
> 
>+static int __agg_valid_ports(struct aggregator *agg)
>+{
>+	struct port *port;
>+	int valid = 0;
>+
>+	for (port = agg->lag_ports; port;
>+	     port = port->next_port_in_aggregator) {
>+		if (port->actor_oper_port_state & LACP_STATE_COLLECTING &&
>+		    (!port->slave->bond->params.coupled_control ||
>+		     port->actor_oper_port_state & LACP_STATE_DISTRIBUTING))
>+			valid++;

	Do we need to test coupled_control?  I.e., can the test be

		if ((port->actor_oper_port_state & LACP_STATE_COLLECTING) &&
		    (port->actor_oper_port_state & LACP_STATE_DISTRIBUTING))

	To my reading, ad_mux_machine will set _COLLECTING and
_DISTRIBUTING appropriately regardless of the coupled_control selection.

>+	}
>+
>+	return valid;
>+}
>+
> static int __agg_active_ports(struct aggregator *agg)
> {
> 	struct port *port;
>@@ -2120,6 +2136,7 @@ static void ad_enable_collecting_distributing(struct port *port,
> 			  port->actor_port_number,
> 			  port->aggregator->aggregator_identifier);
> 		__enable_port(port);
>+		bond_3ad_set_carrier(port->slave->bond);
> 		/* Slave array needs update */
> 		*update_slave_arr = true;
> 		/* Should notify peers if possible */
>@@ -2141,6 +2158,7 @@ static void ad_disable_collecting_distributing(struct port *port,
> 			  port->actor_port_number,
> 			  port->aggregator->aggregator_identifier);
> 		__disable_port(port);
>+		bond_3ad_set_carrier(port->slave->bond);
> 		/* Slave array needs an update */
> 		*update_slave_arr = true;
> 	}
>@@ -2819,8 +2837,12 @@ int bond_3ad_set_carrier(struct bonding *bond)
> 	}
> 	active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
> 	if (active) {
>-		/* are enough slaves available to consider link up? */
>-		if (__agg_active_ports(active) < bond->params.min_links) {
>+		/* are enough slaves in collecting (and distributing) state to consider
>+		 * link up?
>+		 */
>+		if ((bond->params.lacp_fallback ? __agg_valid_ports(active)
>+					: __agg_active_ports(active)) <
>+		    bond->params.min_links) {

	I think the original comment is better; if the new option is
off, it doesn't require collecting / distributing state.

	-J

> 			if (netif_carrier_ok(bond->dev)) {
> 				netif_carrier_off(bond->dev);
> 				goto out;
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index b672b8a881bb..d64a5d2f80b6 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -1706,6 +1706,7 @@ static int bond_option_lacp_fallback_set(struct bonding *bond,
> 	netdev_dbg(bond->dev, "Setting LACP fallback to %s (%llu)\n",
> 		   newval->string, newval->value);
> 	bond->params.lacp_fallback = newval->value;
>+	bond_3ad_set_carrier(bond);
> 
> 	return 0;
> }
>-- 
>2.39.2
>

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