Re: [PATCH net v8 4/6] bonding: 3ad: fix carrier when no usable slaves

[Jay Vosburgh <jv@jvosburgh.net>]

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

>Apply the "lacp_strict" configuration from the previous commit.
>
>"lacp_strict" mode "on" asserts that the bonding master carrier is up
>only when at least 'min_links' slaves are in the Collecting_Distributing
>state.
>
>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, 24 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index f0aa7d2f2171..98abb2fdd7a6 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -745,6 +745,21 @@ static void __set_agg_ports_ready(struct aggregator *aggregator, int val)
> 	}
> }
> 
>+static int __agg_usable_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->actor_oper_port_state & LACP_STATE_DISTRIBUTING)
>+			valid++;
>+	}
>+
>+	return valid;
>+}
>+
> static int __agg_active_ports(struct aggregator *agg)
> {
> 	struct port *port;
>@@ -1179,10 +1194,10 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
> 		switch (port->sm_mux_state) {
> 		case AD_MUX_DETACHED:
> 			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;
>+			ad_disable_collecting_distributing(port,
>+							   update_slave_arr);

	Is there a functional reason to move this?  I don't see anything
obvious in ad_disable_collecting_distributing() that depends on the
actor_oper_port_state.  Since you want this to be backported via stable,
fewer actual changes reduce the chances of an issue applying the patch
to an older version.

	-J


> 			port->ntt = true;
> 			break;
> 		case AD_MUX_WAITING:
>@@ -2105,6 +2120,7 @@ static void ad_disable_distributing(struct port *port, bool *update_slave_arr)
> 			  port->actor_port_number,
> 			  aggregator->aggregator_identifier);
> 		__disable_distributing_port(port);
>+		bond_3ad_set_carrier(port->slave->bond);
> 		/* Slave array needs an update */
> 		*update_slave_arr = true;
> 	}
>@@ -2128,6 +2144,7 @@ static void ad_enable_collecting_distributing(struct port *port,
> 			  port->actor_port_number,
> 			  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 */
>@@ -2151,6 +2168,7 @@ static void ad_disable_collecting_distributing(struct port *port,
> 			  port->actor_port_number,
> 			  aggregator->aggregator_identifier);
> 		__disable_port(port);
>+		bond_3ad_set_carrier(port->slave->bond);
> 		/* Slave array needs an update */
> 		*update_slave_arr = true;
> 	}
>@@ -2830,7 +2848,9 @@ 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) {
>+		if ((bond->params.lacp_strict ? __agg_usable_ports(active)
>+					: __agg_active_ports(active)) <
>+		    bond->params.min_links) {
> 			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 d358b831df77..94b7b0851f16 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -1706,6 +1706,7 @@ static int bond_option_lacp_strict_set(struct bonding *bond,
> 	netdev_dbg(bond->dev, "Setting LACP fallback to %s (%llu)\n",
> 		   newval->string, newval->value);
> 	bond->params.lacp_strict = newval->value;
>+	bond_3ad_set_carrier(bond);
> 
> 	return 0;
> }
>-- 
>2.39.2
>

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