[PATCH net 0/2] bonding: fix negotiation flapping in 802.3ad passive mode

[PATCH net 0/2] bonding: fix negotiation flapping in 802.3ad passive mode

[Hangbin Liu]

This patch fixes unstable LACP negotiation when bonding is configured in
passive mode (`lacp_active=off`).

Previously, the actor would stop sending LACPDUs after initial negotiation
succeeded, leading to the partner timing out and restarting the negotiation
cycle. This resulted in continuous LACP state flapping.

The fix ensures the passive actor starts sending periodic LACPDUs after
receiving the first LACPDU from the partner, in accordance with IEEE
802.1AX-2020 section 6.4.1.

Out of topic:
Although this patch addresses a functional bug and could be considered for
`net`, I'm slightly concerned about potential regressions, as it changes
the current bonding LACP protocol behavior.

It might be safer to merge this through `net-next` first to allow broader
testing. Thoughts?

Hangbin Liu (2):
  bonding: send LACPDUs periodically in passive mode after receiving
    partner's LACPDU
  selftests: bonding: add test for passive LACP mode

 drivers/net/bonding/bond_3ad.c                | 72 ++++++++++----
 drivers/net/bonding/bond_options.c            |  1 +
 include/net/bond_3ad.h                        |  1 +
 .../selftests/drivers/net/bonding/Makefile    |  3 +-
 .../drivers/net/bonding/bond_passive_lacp.sh  | 93 +++++++++++++++++++
 5 files changed, 151 insertions(+), 19 deletions(-)
 create mode 100755 tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh

-- 
2.46.0

[PATCH net 1/2] bonding: send LACPDUs periodically in passive mode after receiving partner's LACPDU

[Hangbin Liu]

When `lacp_active` is set to `off`, the bond operates in passive mode, meaning
it only "speaks when spoken to." However, the current kernel implementation
only sends an LACPDU in response when the partner's state changes.

As a result, once LACP negotiation succeeds, the actor stops sending LACPDUs
until the partner times out and sends an "expired" LACPDU. This causes
continuous LACP state flapping.

According to IEEE 802.1AX-2014, 6.4.13 Periodic Transmission machine. The
values of Partner_Oper_Port_State.LACP_Activity and
Actor_Oper_Port_State.LACP_Activity determine whether periodic transmissions
take place. If either or both parameters are set to Active LACP, then periodic
transmissions occur; if both are set to Passive LACP, then periodic
transmissions do not occur.

To comply with this, we remove the `!bond->params.lacp_active` check in
`ad_periodic_machine()`. Instead, we initialize the actor's port's
`LACP_STATE_LACP_ACTIVITY` state based on `lacp_active` setting.

Additionally, we avoid setting the partner's state to
`LACP_STATE_LACP_ACTIVITY` in the EXPIRED state, since we should not assume
the partner is active by default.

This ensures that in passive mode, the bond starts sending periodic LACPDUs
after receiving one from the partner, and avoids flapping due to inactivity.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_3ad.c     | 72 ++++++++++++++++++++++--------
 drivers/net/bonding/bond_options.c |  1 +
 include/net/bond_3ad.h             |  1 +
 3 files changed, 56 insertions(+), 18 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 2fca8e84ab10..aa64b397bb19 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -95,13 +95,13 @@ static int ad_marker_send(struct port *port, struct bond_marker *marker);
 static void ad_mux_machine(struct port *port, bool *update_slave_arr);
 static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port);
 static void ad_tx_machine(struct port *port);
-static void ad_periodic_machine(struct port *port, struct bond_params *bond_params);
+static void ad_periodic_machine(struct port *port);
 static void ad_port_selection_logic(struct port *port, bool *update_slave_arr);
 static void ad_agg_selection_logic(struct aggregator *aggregator,
 				   bool *update_slave_arr);
 static void ad_clear_agg(struct aggregator *aggregator);
 static void ad_initialize_agg(struct aggregator *aggregator);
-static void ad_initialize_port(struct port *port, int lacp_fast);
+static void ad_initialize_port(struct port *port, struct bond_params *bond_params);
 static void ad_enable_collecting(struct port *port);
 static void ad_disable_distributing(struct port *port,
 				    bool *update_slave_arr);
@@ -1307,10 +1307,16 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
 			 * case of EXPIRED even if LINK_DOWN didn't arrive for
 			 * the port.
 			 */
-			port->partner_oper.port_state &= ~LACP_STATE_SYNCHRONIZATION;
 			port->sm_vars &= ~AD_PORT_MATCHED;
+			/* Based on IEEE 8021AX-2014, Figure 6-18 - Receive
+			 * machine state diagram, the statue should be
+			 * Partner_Oper_Port_State.Synchronization = FALSE;
+			 * Partner_Oper_Port_State.LACP_Timeout = Short Timeout;
+			 * start current_while_timer(Short Timeout);
+			 * Actor_Oper_Port_State.Expired = TRUE;
+			 */
+			port->partner_oper.port_state &= ~LACP_STATE_SYNCHRONIZATION;
 			port->partner_oper.port_state |= LACP_STATE_LACP_TIMEOUT;
-			port->partner_oper.port_state |= LACP_STATE_LACP_ACTIVITY;
 			port->sm_rx_timer_counter = __ad_timer_to_ticks(AD_CURRENT_WHILE_TIMER, (u16)(AD_SHORT_TIMEOUT));
 			port->actor_oper_port_state |= LACP_STATE_EXPIRED;
 			port->sm_vars |= AD_PORT_CHURNED;
@@ -1417,11 +1423,10 @@ static void ad_tx_machine(struct port *port)
 /**
  * ad_periodic_machine - handle a port's periodic state machine
  * @port: the port we're looking at
- * @bond_params: bond parameters we will use
  *
  * Turn ntt flag on priodically to perform periodic transmission of lacpdu's.
  */
-static void ad_periodic_machine(struct port *port, struct bond_params *bond_params)
+static void ad_periodic_machine(struct port *port)
 {
 	periodic_states_t last_state;
 
@@ -1430,8 +1435,7 @@ static void ad_periodic_machine(struct port *port, struct bond_params *bond_para
 
 	/* check if port was reinitialized */
 	if (((port->sm_vars & AD_PORT_BEGIN) || !(port->sm_vars & AD_PORT_LACP_ENABLED) || !port->is_enabled) ||
-	    (!(port->actor_oper_port_state & LACP_STATE_LACP_ACTIVITY) && !(port->partner_oper.port_state & LACP_STATE_LACP_ACTIVITY)) ||
-	    !bond_params->lacp_active) {
+	    (!(port->actor_oper_port_state & LACP_STATE_LACP_ACTIVITY) && !(port->partner_oper.port_state & LACP_STATE_LACP_ACTIVITY))) {
 		port->sm_periodic_state = AD_NO_PERIODIC;
 	}
 	/* check if state machine should change state */
@@ -1955,16 +1959,16 @@ static void ad_initialize_agg(struct aggregator *aggregator)
 /**
  * ad_initialize_port - initialize a given port's parameters
  * @port: the port we're looking at
- * @lacp_fast: boolean. whether fast periodic should be used
+ * @bond_params: bond parameters we will use
  */
-static void ad_initialize_port(struct port *port, int lacp_fast)
+static void ad_initialize_port(struct port *port, struct bond_params *bond_params)
 {
 	static const struct port_params tmpl = {
 		.system_priority = 0xffff,
 		.key             = 1,
 		.port_number     = 1,
 		.port_priority   = 0xff,
-		.port_state      = 1,
+		.port_state      = 0,
 	};
 	static const struct lacpdu lacpdu = {
 		.subtype		= 0x01,
@@ -1982,12 +1986,14 @@ static void ad_initialize_port(struct port *port, int lacp_fast)
 		port->actor_port_priority = 0xff;
 		port->actor_port_aggregator_identifier = 0;
 		port->ntt = false;
-		port->actor_admin_port_state = LACP_STATE_AGGREGATION |
-					       LACP_STATE_LACP_ACTIVITY;
-		port->actor_oper_port_state  = LACP_STATE_AGGREGATION |
-					       LACP_STATE_LACP_ACTIVITY;
+		port->actor_admin_port_state = LACP_STATE_AGGREGATION;
+		port->actor_oper_port_state  = LACP_STATE_AGGREGATION;
+		if (bond_params->lacp_active) {
+			port->actor_admin_port_state |= LACP_STATE_LACP_ACTIVITY;
+			port->actor_oper_port_state  |= LACP_STATE_LACP_ACTIVITY;
+		}
 
-		if (lacp_fast)
+		if (bond_params->lacp_fast)
 			port->actor_oper_port_state |= LACP_STATE_LACP_TIMEOUT;
 
 		memcpy(&port->partner_admin, &tmpl, sizeof(tmpl));
@@ -2201,7 +2207,7 @@ void bond_3ad_bind_slave(struct slave *slave)
 		/* port initialization */
 		port = &(SLAVE_AD_INFO(slave)->port);
 
-		ad_initialize_port(port, bond->params.lacp_fast);
+		ad_initialize_port(port, &bond->params);
 
 		port->slave = slave;
 		port->actor_port_number = SLAVE_AD_INFO(slave)->id;
@@ -2513,7 +2519,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
 		}
 
 		ad_rx_machine(NULL, port);
-		ad_periodic_machine(port, &bond->params);
+		ad_periodic_machine(port);
 		ad_port_selection_logic(port, &update_slave_arr);
 		ad_mux_machine(port, &update_slave_arr);
 		ad_tx_machine(port);
@@ -2883,6 +2889,36 @@ void bond_3ad_update_lacp_rate(struct bonding *bond)
 	spin_unlock_bh(&bond->mode_lock);
 }
 
+/**
+ * bond_3ad_update_lacp_active - change the lacp active
+ * @bond: bonding struct
+ *
+ * When modify lacp_active parameter via sysfs,
+ * update actor_oper_port_state of each port.
+ *
+ * Hold bond->mode_lock,
+ * so we can modify port->actor_oper_port_state,
+ * no matter bond is up or down.
+ */
+void bond_3ad_update_lacp_active(struct bonding *bond)
+{
+	struct port *port = NULL;
+	struct list_head *iter;
+	struct slave *slave;
+	int lacp_active;
+
+	lacp_active = bond->params.lacp_active;
+	spin_lock_bh(&bond->mode_lock);
+	bond_for_each_slave(bond, slave, iter) {
+		port = &(SLAVE_AD_INFO(slave)->port);
+		if (lacp_active)
+			port->actor_oper_port_state |= LACP_STATE_LACP_ACTIVITY;
+		else
+			port->actor_oper_port_state &= ~LACP_STATE_LACP_ACTIVITY;
+	}
+	spin_unlock_bh(&bond->mode_lock);
+}
+
 size_t bond_3ad_stats_size(void)
 {
 	return nla_total_size_64bit(sizeof(u64)) + /* BOND_3AD_STAT_LACPDU_RX */
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 1d639a3be6ba..3b6f815c55ff 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1660,6 +1660,7 @@ static int bond_option_lacp_active_set(struct bonding *bond,
 	netdev_dbg(bond->dev, "Setting LACP active to %s (%llu)\n",
 		   newval->string, newval->value);
 	bond->params.lacp_active = newval->value;
+	bond_3ad_update_lacp_active(bond);
 
 	return 0;
 }
diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h
index 2053cd8e788a..dba369a2cf27 100644
--- a/include/net/bond_3ad.h
+++ b/include/net/bond_3ad.h
@@ -307,6 +307,7 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
 			 struct slave *slave);
 int bond_3ad_set_carrier(struct bonding *bond);
 void bond_3ad_update_lacp_rate(struct bonding *bond);
+void bond_3ad_update_lacp_active(struct bonding *bond);
 void bond_3ad_update_ad_actor_settings(struct bonding *bond);
 int bond_3ad_stats_fill(struct sk_buff *skb, struct bond_3ad_stats *stats);
 size_t bond_3ad_stats_size(void);
-- 
2.46.0

Re: [PATCH net 1/2] bonding: send LACPDUs periodically in passive mode after receiving partner's LACPDU

[Nikolay Aleksandrov]

On 7/25/25 09:28, Hangbin Liu wrote:
> When `lacp_active` is set to `off`, the bond operates in passive mode, meaning
> it only "speaks when spoken to." However, the current kernel implementation
> only sends an LACPDU in response when the partner's state changes.
> 
> As a result, once LACP negotiation succeeds, the actor stops sending LACPDUs
> until the partner times out and sends an "expired" LACPDU. This causes
> continuous LACP state flapping.
> 
> According to IEEE 802.1AX-2014, 6.4.13 Periodic Transmission machine. The
> values of Partner_Oper_Port_State.LACP_Activity and
> Actor_Oper_Port_State.LACP_Activity determine whether periodic transmissions
> take place. If either or both parameters are set to Active LACP, then periodic
> transmissions occur; if both are set to Passive LACP, then periodic
> transmissions do not occur.
> 
> To comply with this, we remove the `!bond->params.lacp_active` check in
> `ad_periodic_machine()`. Instead, we initialize the actor's port's
> `LACP_STATE_LACP_ACTIVITY` state based on `lacp_active` setting.
> 
> Additionally, we avoid setting the partner's state to
> `LACP_STATE_LACP_ACTIVITY` in the EXPIRED state, since we should not assume
> the partner is active by default.
> 
> This ensures that in passive mode, the bond starts sending periodic LACPDUs
> after receiving one from the partner, and avoids flapping due to inactivity.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

Shouldn't the fixes tag be lacp_active commit id?
E.g. 3a755cd8b7c6 ("bonding: add new option lacp_active")

> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/bonding/bond_3ad.c     | 72 ++++++++++++++++++++++--------
>  drivers/net/bonding/bond_options.c |  1 +
>  include/net/bond_3ad.h             |  1 +
>  3 files changed, 56 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 2fca8e84ab10..aa64b397bb19 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -95,13 +95,13 @@ static int ad_marker_send(struct port *port, struct bond_marker *marker);
>  static void ad_mux_machine(struct port *port, bool *update_slave_arr);
>  static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port);
>  static void ad_tx_machine(struct port *port);
> -static void ad_periodic_machine(struct port *port, struct bond_params *bond_params);
> +static void ad_periodic_machine(struct port *port);
>  static void ad_port_selection_logic(struct port *port, bool *update_slave_arr);
>  static void ad_agg_selection_logic(struct aggregator *aggregator,
>  				   bool *update_slave_arr);
>  static void ad_clear_agg(struct aggregator *aggregator);
>  static void ad_initialize_agg(struct aggregator *aggregator);
> -static void ad_initialize_port(struct port *port, int lacp_fast);
> +static void ad_initialize_port(struct port *port, struct bond_params *bond_params);
>  static void ad_enable_collecting(struct port *port);
>  static void ad_disable_distributing(struct port *port,
>  				    bool *update_slave_arr);
> @@ -1307,10 +1307,16 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
>  			 * case of EXPIRED even if LINK_DOWN didn't arrive for
>  			 * the port.
>  			 */
> -			port->partner_oper.port_state &= ~LACP_STATE_SYNCHRONIZATION;
>  			port->sm_vars &= ~AD_PORT_MATCHED;
> +			/* Based on IEEE 8021AX-2014, Figure 6-18 - Receive
> +			 * machine state diagram, the statue should be
> +			 * Partner_Oper_Port_State.Synchronization = FALSE;
> +			 * Partner_Oper_Port_State.LACP_Timeout = Short Timeout;
> +			 * start current_while_timer(Short Timeout);
> +			 * Actor_Oper_Port_State.Expired = TRUE;
> +			 */
> +			port->partner_oper.port_state &= ~LACP_STATE_SYNCHRONIZATION;
>  			port->partner_oper.port_state |= LACP_STATE_LACP_TIMEOUT;
> -			port->partner_oper.port_state |= LACP_STATE_LACP_ACTIVITY;
>  			port->sm_rx_timer_counter = __ad_timer_to_ticks(AD_CURRENT_WHILE_TIMER, (u16)(AD_SHORT_TIMEOUT));
>  			port->actor_oper_port_state |= LACP_STATE_EXPIRED;
>  			port->sm_vars |= AD_PORT_CHURNED;
> @@ -1417,11 +1423,10 @@ static void ad_tx_machine(struct port *port)
>  /**
>   * ad_periodic_machine - handle a port's periodic state machine
>   * @port: the port we're looking at
> - * @bond_params: bond parameters we will use
>   *
>   * Turn ntt flag on priodically to perform periodic transmission of lacpdu's.
>   */
> -static void ad_periodic_machine(struct port *port, struct bond_params *bond_params)
> +static void ad_periodic_machine(struct port *port)
>  {
>  	periodic_states_t last_state;
>  
> @@ -1430,8 +1435,7 @@ static void ad_periodic_machine(struct port *port, struct bond_params *bond_para
>  
>  	/* check if port was reinitialized */
>  	if (((port->sm_vars & AD_PORT_BEGIN) || !(port->sm_vars & AD_PORT_LACP_ENABLED) || !port->is_enabled) ||
> -	    (!(port->actor_oper_port_state & LACP_STATE_LACP_ACTIVITY) && !(port->partner_oper.port_state & LACP_STATE_LACP_ACTIVITY)) ||
> -	    !bond_params->lacp_active) {
> +	    (!(port->actor_oper_port_state & LACP_STATE_LACP_ACTIVITY) && !(port->partner_oper.port_state & LACP_STATE_LACP_ACTIVITY))) {
>  		port->sm_periodic_state = AD_NO_PERIODIC;
>  	}
>  	/* check if state machine should change state */
> @@ -1955,16 +1959,16 @@ static void ad_initialize_agg(struct aggregator *aggregator)
>  /**
>   * ad_initialize_port - initialize a given port's parameters
>   * @port: the port we're looking at
> - * @lacp_fast: boolean. whether fast periodic should be used
> + * @bond_params: bond parameters we will use
>   */
> -static void ad_initialize_port(struct port *port, int lacp_fast)
> +static void ad_initialize_port(struct port *port, struct bond_params *bond_params)
>  {
>  	static const struct port_params tmpl = {
>  		.system_priority = 0xffff,
>  		.key             = 1,
>  		.port_number     = 1,
>  		.port_priority   = 0xff,
> -		.port_state      = 1,
> +		.port_state      = 0,

1 == LACP_STATE_ACTIVITY :)
Just noting that this lets LACP_STATE_ACTIVITY to be set based on lacp_active.

>  	};
>  	static const struct lacpdu lacpdu = {
>  		.subtype		= 0x01,
> @@ -1982,12 +1986,14 @@ static void ad_initialize_port(struct port *port, int lacp_fast)
>  		port->actor_port_priority = 0xff;
>  		port->actor_port_aggregator_identifier = 0;
>  		port->ntt = false;
> -		port->actor_admin_port_state = LACP_STATE_AGGREGATION |
> -					       LACP_STATE_LACP_ACTIVITY;
> -		port->actor_oper_port_state  = LACP_STATE_AGGREGATION |
> -					       LACP_STATE_LACP_ACTIVITY;
> +		port->actor_admin_port_state = LACP_STATE_AGGREGATION;
> +		port->actor_oper_port_state  = LACP_STATE_AGGREGATION;
> +		if (bond_params->lacp_active) {
> +			port->actor_admin_port_state |= LACP_STATE_LACP_ACTIVITY;
> +			port->actor_oper_port_state  |= LACP_STATE_LACP_ACTIVITY;
> +		}
>  
> -		if (lacp_fast)
> +		if (bond_params->lacp_fast)
>  			port->actor_oper_port_state |= LACP_STATE_LACP_TIMEOUT;
>  
>  		memcpy(&port->partner_admin, &tmpl, sizeof(tmpl));
> @@ -2201,7 +2207,7 @@ void bond_3ad_bind_slave(struct slave *slave)
>  		/* port initialization */
>  		port = &(SLAVE_AD_INFO(slave)->port);
>  
> -		ad_initialize_port(port, bond->params.lacp_fast);
> +		ad_initialize_port(port, &bond->params);
>  
>  		port->slave = slave;
>  		port->actor_port_number = SLAVE_AD_INFO(slave)->id;
> @@ -2513,7 +2519,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
>  		}
>  
>  		ad_rx_machine(NULL, port);
> -		ad_periodic_machine(port, &bond->params);
> +		ad_periodic_machine(port);
>  		ad_port_selection_logic(port, &update_slave_arr);
>  		ad_mux_machine(port, &update_slave_arr);
>  		ad_tx_machine(port);
> @@ -2883,6 +2889,36 @@ void bond_3ad_update_lacp_rate(struct bonding *bond)
>  	spin_unlock_bh(&bond->mode_lock);
>  }
>  
> +/**
> + * bond_3ad_update_lacp_active - change the lacp active
> + * @bond: bonding struct
> + *
> + * When modify lacp_active parameter via sysfs,
> + * update actor_oper_port_state of each port.

It could also be netlink, right? I'd just say something like:
Update actor_oper_port_state when lacp_active is modified.

> + *
> + * Hold bond->mode_lock,
> + * so we can modify port->actor_oper_port_state,
> + * no matter bond is up or down.

nit: this last part about bond up/down is unnecessary

> + */
> +void bond_3ad_update_lacp_active(struct bonding *bond)
> +{
> +	struct port *port = NULL;
> +	struct list_head *iter;
> +	struct slave *slave;
> +	int lacp_active;
> +
> +	lacp_active = bond->params.lacp_active;
> +	spin_lock_bh(&bond->mode_lock);
> +	bond_for_each_slave(bond, slave, iter) {
> +		port = &(SLAVE_AD_INFO(slave)->port);
> +		if (lacp_active)
> +			port->actor_oper_port_state |= LACP_STATE_LACP_ACTIVITY;
> +		else
> +			port->actor_oper_port_state &= ~LACP_STATE_LACP_ACTIVITY;
> +	}
> +	spin_unlock_bh(&bond->mode_lock);
> +}
> +
>  size_t bond_3ad_stats_size(void)
>  {
>  	return nla_total_size_64bit(sizeof(u64)) + /* BOND_3AD_STAT_LACPDU_RX */
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 1d639a3be6ba..3b6f815c55ff 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -1660,6 +1660,7 @@ static int bond_option_lacp_active_set(struct bonding *bond,
>  	netdev_dbg(bond->dev, "Setting LACP active to %s (%llu)\n",
>  		   newval->string, newval->value);
>  	bond->params.lacp_active = newval->value;
> +	bond_3ad_update_lacp_active(bond);

To me it looks like this fix needs to be in a separate patch.

>  
>  	return 0;
>  }
> diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h
> index 2053cd8e788a..dba369a2cf27 100644
> --- a/include/net/bond_3ad.h
> +++ b/include/net/bond_3ad.h
> @@ -307,6 +307,7 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
>  			 struct slave *slave);
>  int bond_3ad_set_carrier(struct bonding *bond);
>  void bond_3ad_update_lacp_rate(struct bonding *bond);
> +void bond_3ad_update_lacp_active(struct bonding *bond);
>  void bond_3ad_update_ad_actor_settings(struct bonding *bond);
>  int bond_3ad_stats_fill(struct sk_buff *skb, struct bond_3ad_stats *stats);
>  size_t bond_3ad_stats_size(void);

Re: [PATCH net 1/2] bonding: send LACPDUs periodically in passive mode after receiving partner's LACPDU

[Hangbin Liu]

On Fri, Jul 25, 2025 at 11:49:00AM +0300, Nikolay Aleksandrov wrote:
> On 7/25/25 09:28, Hangbin Liu wrote:
> > When `lacp_active` is set to `off`, the bond operates in passive mode, meaning
> > it only "speaks when spoken to." However, the current kernel implementation
> > only sends an LACPDU in response when the partner's state changes.
> > 
> > As a result, once LACP negotiation succeeds, the actor stops sending LACPDUs
> > until the partner times out and sends an "expired" LACPDU. This causes
> > continuous LACP state flapping.
> > 
> > According to IEEE 802.1AX-2014, 6.4.13 Periodic Transmission machine. The
> > values of Partner_Oper_Port_State.LACP_Activity and
> > Actor_Oper_Port_State.LACP_Activity determine whether periodic transmissions
> > take place. If either or both parameters are set to Active LACP, then periodic
> > transmissions occur; if both are set to Passive LACP, then periodic
> > transmissions do not occur.
> > 
> > To comply with this, we remove the `!bond->params.lacp_active` check in
> > `ad_periodic_machine()`. Instead, we initialize the actor's port's
> > `LACP_STATE_LACP_ACTIVITY` state based on `lacp_active` setting.
> > 
> > Additionally, we avoid setting the partner's state to
> > `LACP_STATE_LACP_ACTIVITY` in the EXPIRED state, since we should not assume
> > the partner is active by default.
> > 
> > This ensures that in passive mode, the bond starts sending periodic LACPDUs
> > after receiving one from the partner, and avoids flapping due to inactivity.
> > 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> 
> Shouldn't the fixes tag be lacp_active commit id?
> E.g. 3a755cd8b7c6 ("bonding: add new option lacp_active")

Totally forgot that I added this option -_-!!

Thanks
Hangbin

[PATCH net 2/2] selftests: bonding: add test for passive LACP mode

[Hangbin Liu]

Add a selftest to verify bonding behavior when `lacp_active` is set to `off`.

The test checks the following:
- The passive LACP bond should not send LACPDUs before receiving a partner's
  LACPDU.
- The transmitted LACPDUs must not include the active flag.
- After transitioning to EXPIRED and DEFAULTED states, the passive side should
  still not initiate LACPDUs.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 .../selftests/drivers/net/bonding/Makefile    |  3 +-
 .../drivers/net/bonding/bond_passive_lacp.sh  | 93 +++++++++++++++++++
 2 files changed, 95 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh

diff --git a/tools/testing/selftests/drivers/net/bonding/Makefile b/tools/testing/selftests/drivers/net/bonding/Makefile
index 2b10854e4b1e..44b98f17f8ff 100644
--- a/tools/testing/selftests/drivers/net/bonding/Makefile
+++ b/tools/testing/selftests/drivers/net/bonding/Makefile
@@ -10,7 +10,8 @@ TEST_PROGS := \
 	mode-2-recovery-updelay.sh \
 	bond_options.sh \
 	bond-eth-type-change.sh \
-	bond_macvlan_ipvlan.sh
+	bond_macvlan_ipvlan.sh \
+	bond_passive_lacp.sh
 
 TEST_FILES := \
 	lag_lib.sh \
diff --git a/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
new file mode 100755
index 000000000000..017d96a319a3
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
@@ -0,0 +1,93 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Test if a bond interface works with lacp_active=off.
+
+# shellcheck disable=SC2034
+REQUIRE_MZ=no
+NUM_NETIFS=0
+lib_dir=$(dirname "$0")
+# shellcheck disable=SC1091
+source "$lib_dir"/../../../net/forwarding/lib.sh
+
+check_port_state()
+{
+	local netns=$1
+	local port=$2
+	local state=$3
+
+	ip -n "${netns}" -d -j link show "$port" | \
+		jq -e ".[].linkinfo.info_slave_data.ad_actor_oper_port_state_str | index(\"${state}\") != null" > /dev/null
+}
+
+trap cleanup EXIT
+setup_ns c_ns s_ns
+defer cleanup_all_ns
+
+# shellcheck disable=SC2154
+ip -n "${c_ns}" link add eth0 type veth peer name eth0 netns "${s_ns}"
+ip -n "${c_ns}" link add eth1 type veth peer name eth1 netns "${s_ns}"
+ip -n "${s_ns}" link set eth0 up
+ip -n "${s_ns}" link set eth1 up
+ip -n "${c_ns}" link add bond0 type bond mode 802.3ad lacp_active off lacp_rate fast
+ip -n "${c_ns}" link set eth0 master bond0
+ip -n "${c_ns}" link set eth1 master bond0
+ip -n "${c_ns}" link set bond0 up
+
+# 1. The passive side shouldn't send LACPDU.
+RET=0
+client_mac=$(cmd_jq "ip -j -n ${c_ns} link show bond0" ".[].address")
+# Wait for the first LACPDU due to state change.
+sleep 2
+timeout 62 ip netns exec "${c_ns}" tcpdump --immediate-mode -c 1 -i eth0 \
+	-nn -l -vvv ether proto 0x8809 2> /dev/null > /tmp/client_init.out
+grep -q "System $client_mac" /tmp/client_init.out && RET=1
+log_test "802.3ad" "init port pkt lacp_active off"
+
+# 2. The passive side should not have the 'active' flag.
+RET=0
+check_port_state "${c_ns}" "eth0" "active" && RET=1
+log_test "802.3ad" "port state lacp_active off"
+
+# Set up the switch side with active mode.
+ip -n "${s_ns}" link set eth0 down
+ip -n "${s_ns}" link set eth1 down
+ip -n "${s_ns}" link add bond0 type bond mode 802.3ad lacp_active on lacp_rate fast
+ip -n "${s_ns}" link set eth0 master bond0
+ip -n "${s_ns}" link set eth1 master bond0
+ip -n "${s_ns}" link set bond0 up
+
+# 3. The active side should have the 'active' flag.
+RET=0
+check_port_state "${s_ns}" "eth0" "active" || RET=1
+log_test "802.3ad" "port state lacp_active on"
+
+# 4. Make sure the connection has not expired.
+RET=0
+slowwait 15 check_port_state "${s_ns}" "eth0" "expired" && RET=1
+slowwait 15 check_port_state "${s_ns}" "eth1" "expired" && RET=1
+log_test "bond 802.3ad" "port connect lacp_active off"
+
+# After testing, disconnect one port on each side to check the state.
+ip -n "${s_ns}" link set eth0 nomaster
+ip -n "${s_ns}" link set eth0 up
+ip -n "${c_ns}" link set eth1 nomaster
+ip -n "${c_ns}" link set eth1 up
+# 5. The passive side shouldn't send LACPDU anymore.
+RET=0
+# Wait for LACPDU due to state change.
+sleep 5
+timeout 62 ip netns exec "${c_ns}" tcpdump --immediate-mode -c 1 -i eth0 \
+	-nn -l -vvv ether proto 0x8809 2> /dev/null > /tmp/client_dis.out
+grep -q "System $client_mac" /tmp/client_dis.out && RET=1
+log_test "bond 802.3ad" "disconnect port pkt lacp_active off"
+
+# 6. The active side keeps sending LACPDU.
+RET=0
+switch_mac=$(cmd_jq "ip -j -n ${s_ns} link show bond0" ".[].address")
+timeout 62 ip netns exec "${s_ns}" tcpdump --immediate-mode -c 1 -i eth1 \
+	-nn -l -vvv ether proto 0x8809 2> /dev/null > /tmp/switch.out
+grep -q "System $switch_mac" /tmp/switch.out || RET=1
+log_test "bond 802.3ad" "disconnect port pkt lacp_active on"
+
+exit "$EXIT_STATUS"
-- 
2.46.0

Re: [PATCH net 2/2] selftests: bonding: add test for passive LACP mode

[Jakub Kicinski]

On Fri, 25 Jul 2025 06:28:48 +0000 Hangbin Liu wrote:
> Add a selftest to verify bonding behavior when `lacp_active` is set to `off`.
> 
> The test checks the following:
> - The passive LACP bond should not send LACPDUs before receiving a partner's
>   LACPDU.
> - The transmitted LACPDUs must not include the active flag.
> - After transitioning to EXPIRED and DEFAULTED states, the passive side should
>   still not initiate LACPDUs.

Strangely this test fails on a normal kernel build but passes with
debug options enabled:

https://netdev.bots.linux.dev/contest.html?branch=net-next-2025-07-25--09-00&test=bond-passive-lacp-sh

Re: [PATCH net 2/2] selftests: bonding: add test for passive LACP mode

[Hangbin Liu]

On Fri, Jul 25, 2025 at 07:21:12AM -0700, Jakub Kicinski wrote:
> On Fri, 25 Jul 2025 06:28:48 +0000 Hangbin Liu wrote:
> > Add a selftest to verify bonding behavior when `lacp_active` is set to `off`.
> > 
> > The test checks the following:
> > - The passive LACP bond should not send LACPDUs before receiving a partner's
> >   LACPDU.
> > - The transmitted LACPDUs must not include the active flag.
> > - After transitioning to EXPIRED and DEFAULTED states, the passive side should
> >   still not initiate LACPDUs.
> 
> Strangely this test fails on a normal kernel build but passes with
> debug options enabled:
> 
> https://netdev.bots.linux.dev/contest.html?branch=net-next-2025-07-25--09-00&test=bond-passive-lacp-sh

I guess on the normal kernel, the checking starts too soon before negotiation
finished. I will add some sleep time before checking.

Thanks
Hangbin

[PATCH net 0/2] bonding: fix LACP negotiation issues in passive mode

[Hangbin Liu]

This patchset fixes an issue where bonding fails to establish a stable LACP
negotiation when operating in passive mode (lacp_active=off).

In passive mode, the current implementation only replies when the partner's
state changes, which results in LACP timeout and unstable aggregator formation.

With this change, the bond responds to each received LACPDU in passive mode
by setting ntt = true, ensuring timely replies and stable LACP negotiation.

Hangbin Liu (2):
  bonding: update ntt to true in passive mode
  selftests: bonding: add test for passive LACP mode

 drivers/net/bonding/bond_3ad.c                |  6 ++
 .../drivers/net/bonding/bond_passive_lacp.sh  | 21 +++++
 .../drivers/net/bonding/bond_topo_lacp.sh     | 77 +++++++++++++++++++
 3 files changed, 104 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
 create mode 100644 tools/testing/selftests/drivers/net/bonding/bond_topo_lacp.sh

-- 
2.46.0

[PATCH net 2/2] selftests: bonding: add test for passive LACP mode

[Hangbin Liu]

Add a selftest to verify bonding behavior when lacp_active is set to off.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 .../drivers/net/bonding/bond_passive_lacp.sh  | 21 +++++
 .../drivers/net/bonding/bond_topo_lacp.sh     | 77 +++++++++++++++++++
 2 files changed, 98 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
 create mode 100644 tools/testing/selftests/drivers/net/bonding/bond_topo_lacp.sh

diff --git a/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
new file mode 100755
index 000000000000..4cf8a5999aaa
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# Testing if bond works with lacp_active = off
+
+lib_dir=$(dirname "$0")
+source ${lib_dir}/bond_topo_lacp.sh
+
+trap cleanup EXIT
+setup_topo_lacp
+
+lacp_bond_reset "${c_ns}" "lacp_active off"
+# make sure the switch state is not expired [A,T,G,S,Ex]
+if slowwait 15 ip netns exec ${s_ns} grep -q 'port state: 143' /proc/net/bonding/bond0; then
+	RET=1
+else
+	RET=0
+fi
+log_test "bond 802.3ad" "lacp_active off"
+
+exit $EXIT_STATUS
diff --git a/tools/testing/selftests/drivers/net/bonding/bond_topo_lacp.sh b/tools/testing/selftests/drivers/net/bonding/bond_topo_lacp.sh
new file mode 100644
index 000000000000..33dca6ebdc4b
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/bonding/bond_topo_lacp.sh
@@ -0,0 +1,77 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Topology for Bond mode 802.3ad testing
+#
+#  +-------------------------+
+#  |          bond0          |  Switch
+#  |            +            |  192.0.2.254/24
+#  |      eth0  |  eth1      |  2001:db8::254/24
+#  |        +---+---+        |
+#  |        |       |        |
+#  +-------------------------+
+#           |       |
+#  +-------------------------+
+#  |        |       |        |
+#  |        +---+---+        |  Client
+#  |      eth0  |  eth1      |  192.0.2.1/24
+#  |            +            |  2001:db8::1/24
+#  |          bond0          |
+#  +-------------------------+
+
+REQUIRE_MZ=no
+NUM_NETIFS=0
+lib_dir=$(dirname "$0")
+source "$lib_dir"/../../../net/forwarding/lib.sh
+
+s_ip4="192.0.2.254"
+c_ip4="192.0.2.1"
+s_ip6="2001:db8::254"
+c_ip6="2001:db8::1"
+
+switch_create()
+{
+	ip netns exec ${s_ns} sysctl -q net.ipv6.conf.default.keep_addr_on_down=1
+	ip -n ${s_ns} link add eth0 type veth peer name eth0 netns ${c_ns}
+	ip -n ${s_ns} link add eth1 type veth peer name eth1 netns ${c_ns}
+	ip -n ${s_ns} link add bond0 type bond mode 802.3ad miimon 100 lacp_active on lacp_rate fast
+	ip -n ${s_ns} link set eth0 master bond0
+	ip -n ${s_ns} link set eth1 master bond0
+	ip -n ${s_ns} addr add ${s_ip4}/24 dev bond0
+	ip -n ${s_ns} addr add ${s_ip6}/24 dev bond0
+	ip -n ${s_ns} link set bond0 up
+}
+
+client_create()
+{
+	ip netns exec ${c_ns} sysctl -q net.ipv6.conf.default.keep_addr_on_down=1
+	ip -n ${c_ns} link add bond0 type bond mode 802.3ad miimon 100 lacp_active on lacp_rate fast
+	ip -n ${c_ns} link set eth0 master bond0
+	ip -n ${c_ns} link set eth1 master bond0
+	ip -n ${c_ns} addr add ${c_ip4}/24 dev bond0
+	ip -n ${c_ns} addr add ${c_ip6}/24 dev bond0
+	ip -n ${c_ns} link set bond0 up
+}
+
+setup_topo_lacp()
+{
+	setup_ns s_ns c_ns
+	defer cleanup_all_ns
+
+	switch_create
+	client_create
+}
+
+# Reset bond with and options
+lacp_bond_reset()
+{
+	local netns="$1"
+	local param="$2"
+
+	ip -n ${netns} link set bond0 down
+	ip -n ${netns} link set bond0 type bond $param
+	ip -n ${netns} link set bond0 up
+
+	# Wait for IPv6 address ready as it needs DAD
+	slowwait 10 ip netns exec ${c_ns} ping6 ${s_ip6} -c 1 -W 0.1 &> /dev/null
+}
-- 
2.46.0

Re: [PATCH net 2/2] selftests: bonding: add test for passive LACP mode

[Paolo Abeni]

On 7/9/25 11:03 AM, Hangbin Liu wrote:
> Add a selftest to verify bonding behavior when lacp_active is set to off.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  .../drivers/net/bonding/bond_passive_lacp.sh  | 21 +++++
>  .../drivers/net/bonding/bond_topo_lacp.sh     | 77 +++++++++++++++++++
>  2 files changed, 98 insertions(+)
>  create mode 100755 tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
>  create mode 100644 tools/testing/selftests/drivers/net/bonding/bond_topo_lacp.sh

New test should be listed in the relevant makefile
> diff --git a/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
> new file mode 100755
> index 000000000000..4cf8a5999aaa
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Testing if bond works with lacp_active = off
> +
> +lib_dir=$(dirname "$0")
> +source ${lib_dir}/bond_topo_lacp.sh

shellcheck is not super happy about 'source' usage:

In bond_passive_lacp.sh line 7:
source ${lib_dir}/bond_topo_lacp.sh
^-- SC3046 (warning): In POSIX sh, 'source' in place of '.' is undefined.
^-- SC3051 (warning): In POSIX sh, 'source' in place of '.' is undefined.

either switch to '. ' or use bash instead of 'sh'.

> +lacp_bond_reset "${c_ns}" "lacp_active off"
> +# make sure the switch state is not expired [A,T,G,S,Ex]
> +if slowwait 15 ip netns exec ${s_ns} grep -q 'port state: 143' /proc/net/bonding/bond0; then

Shellcheck wants double quote everywhere. Since in many cases (all the
blamed ones in this patch) we know the variable is really a single word,
I think you could simply disable the warning with:

#shellcheck disable=SC2086

(same in the other test file)

> +	RET=1
> +else
> +	RET=0
> +fi

/P

Re: [PATCH net 2/2] selftests: bonding: add test for passive LACP mode

[Hangbin Liu]

On Tue, Jul 15, 2025 at 11:37:54AM +0200, Paolo Abeni wrote:
> On 7/9/25 11:03 AM, Hangbin Liu wrote:
> > Add a selftest to verify bonding behavior when lacp_active is set to off.
> > 
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > ---
> >  .../drivers/net/bonding/bond_passive_lacp.sh  | 21 +++++
> >  .../drivers/net/bonding/bond_topo_lacp.sh     | 77 +++++++++++++++++++
> >  2 files changed, 98 insertions(+)
> >  create mode 100755 tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
> >  create mode 100644 tools/testing/selftests/drivers/net/bonding/bond_topo_lacp.sh
> 
> New test should be listed in the relevant makefile

Ah, yes, I forgot this.

> > diff --git a/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
> > new file mode 100755
> > index 000000000000..4cf8a5999aaa
> > --- /dev/null
> > +++ b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
> > @@ -0,0 +1,21 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Testing if bond works with lacp_active = off
> > +
> > +lib_dir=$(dirname "$0")
> > +source ${lib_dir}/bond_topo_lacp.sh
> 
> shellcheck is not super happy about 'source' usage:
> 
> In bond_passive_lacp.sh line 7:
> source ${lib_dir}/bond_topo_lacp.sh
> ^-- SC3046 (warning): In POSIX sh, 'source' in place of '.' is undefined.
> ^-- SC3051 (warning): In POSIX sh, 'source' in place of '.' is undefined.
> 
> either switch to '. ' or use bash instead of 'sh'.

OK, I will fix this and other warns.

Thanks
Hangbin
> 
> > +lacp_bond_reset "${c_ns}" "lacp_active off"
> > +# make sure the switch state is not expired [A,T,G,S,Ex]
> > +if slowwait 15 ip netns exec ${s_ns} grep -q 'port state: 143' /proc/net/bonding/bond0; then
> 
> Shellcheck wants double quote everywhere. Since in many cases (all the
> blamed ones in this patch) we know the variable is really a single word,
> I think you could simply disable the warning with:
> 
> #shellcheck disable=SC2086
> 
> (same in the other test file)
> 
> > +	RET=1
> > +else
> > +	RET=0
> > +fi
> 
> /P
> 

Re: [PATCH net 2/2] selftests: bonding: add test for passive LACP mode

[Hangbin Liu]

On Tue, Jul 15, 2025 at 11:37:54AM +0200, Paolo Abeni wrote:
> > diff --git a/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
> > new file mode 100755
> > index 000000000000..4cf8a5999aaa
> > --- /dev/null
> > +++ b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
> > @@ -0,0 +1,21 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Testing if bond works with lacp_active = off
> > +
> > +lib_dir=$(dirname "$0")
> > +source ${lib_dir}/bond_topo_lacp.sh
> 
> shellcheck is not super happy about 'source' usage:
> 
> In bond_passive_lacp.sh line 7:
> source ${lib_dir}/bond_topo_lacp.sh
> ^-- SC3046 (warning): In POSIX sh, 'source' in place of '.' is undefined.
> ^-- SC3051 (warning): In POSIX sh, 'source' in place of '.' is undefined.
> 
> either switch to '. ' or use bash instead of 'sh'.

Hi Paolo,

I updated the case and remove the source file bond_topo_lacp.sh.
Instead I source the forwarding lib directly like:

lib_dir=$(dirname "$0")
source "$lib_dir"/../../../net/forwarding/lib.sh

But this cause shell check unable to find the lib.sh as $lib_dir is get
dynamically. This usage is common in selftest. How should we resolves this
problem?

Thanks
Hangbin

Re: [PATCH net 2/2] selftests: bonding: add test for passive LACP mode

[Hangbin Liu]

On Thu, Jul 24, 2025 at 04:06:03AM +0000, Hangbin Liu wrote:
> On Tue, Jul 15, 2025 at 11:37:54AM +0200, Paolo Abeni wrote:
> > > diff --git a/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
> > > new file mode 100755
> > > index 000000000000..4cf8a5999aaa
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
> > > @@ -0,0 +1,21 @@
> > > +#!/bin/sh
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +#
> > > +# Testing if bond works with lacp_active = off
> > > +
> > > +lib_dir=$(dirname "$0")
> > > +source ${lib_dir}/bond_topo_lacp.sh
> > 
> > shellcheck is not super happy about 'source' usage:
> > 
> > In bond_passive_lacp.sh line 7:
> > source ${lib_dir}/bond_topo_lacp.sh
> > ^-- SC3046 (warning): In POSIX sh, 'source' in place of '.' is undefined.
> > ^-- SC3051 (warning): In POSIX sh, 'source' in place of '.' is undefined.
> > 
> > either switch to '. ' or use bash instead of 'sh'.
> 
> Hi Paolo,
> 
> I updated the case and remove the source file bond_topo_lacp.sh.
> Instead I source the forwarding lib directly like:
> 
> lib_dir=$(dirname "$0")
> source "$lib_dir"/../../../net/forwarding/lib.sh
> 
> But this cause shell check unable to find the lib.sh as $lib_dir is get
> dynamically. This usage is common in selftest. How should we resolves this
> problem?

OK, I just disabled this warning.

# shellcheck disable=SC1091

Hangbin

Re: [PATCH net 2/2] selftests: bonding: add test for passive LACP mode

[Petr Machata]

Hangbin Liu <liuhangbin@gmail.com> writes:

> On Thu, Jul 24, 2025 at 04:06:03AM +0000, Hangbin Liu wrote:
>> On Tue, Jul 15, 2025 at 11:37:54AM +0200, Paolo Abeni wrote:
>> > > diff --git a/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
>> > > b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
>> > > new file mode 100755
>> > > index 000000000000..4cf8a5999aaa
>> > > --- /dev/null
>> > > +++ b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
>> > > @@ -0,0 +1,21 @@
>> > > +#!/bin/sh
>> > > +# SPDX-License-Identifier: GPL-2.0
>> > > +#
>> > > +# Testing if bond works with lacp_active = off
>> > > +
>> > > +lib_dir=$(dirname "$0")
>> > > +source ${lib_dir}/bond_topo_lacp.sh
>> > 
>> > shellcheck is not super happy about 'source' usage:
>> > 
>> > In bond_passive_lacp.sh line 7:
>> > source ${lib_dir}/bond_topo_lacp.sh
>> > ^-- SC3046 (warning): In POSIX sh, 'source' in place of '.' is undefined.
>> > ^-- SC3051 (warning): In POSIX sh, 'source' in place of '.' is undefined.
>> > 
>> > either switch to '. ' or use bash instead of 'sh'.
>> 
>> Hi Paolo,
>> 
>> I updated the case and remove the source file bond_topo_lacp.sh.
>> Instead I source the forwarding lib directly like:
>> 
>> lib_dir=$(dirname "$0")
>> source "$lib_dir"/../../../net/forwarding/lib.sh
>> 
>> But this cause shell check unable to find the lib.sh as $lib_dir is get
>> dynamically. This usage is common in selftest. How should we resolves this
>> problem?
>
> OK, I just disabled this warning.
>
> # shellcheck disable=SC1091

I believe the point was only about using "." instead of "source". The
following should have fixed it:

. ${lib_dir}/bond_topo_lacp.sh

... or just use bash as the interpreter, I suspect lib.sh is not
actually POSIX clean.

Re: [PATCH net 2/2] selftests: bonding: add test for passive LACP mode

[Hangbin Liu]

On Fri, Jul 25, 2025 at 10:27:48AM +0200, Petr Machata wrote:
> 
> Hangbin Liu <liuhangbin@gmail.com> writes:
> 
> > On Thu, Jul 24, 2025 at 04:06:03AM +0000, Hangbin Liu wrote:
> >> On Tue, Jul 15, 2025 at 11:37:54AM +0200, Paolo Abeni wrote:
> >> > > diff --git a/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
> >> > > b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
> >> > > new file mode 100755
> >> > > index 000000000000..4cf8a5999aaa
> >> > > --- /dev/null
> >> > > +++ b/tools/testing/selftests/drivers/net/bonding/bond_passive_lacp.sh
> >> > > @@ -0,0 +1,21 @@
> >> > > +#!/bin/sh
> >> > > +# SPDX-License-Identifier: GPL-2.0
> >> > > +#
> >> > > +# Testing if bond works with lacp_active = off
> >> > > +
> >> > > +lib_dir=$(dirname "$0")
> >> > > +source ${lib_dir}/bond_topo_lacp.sh
> >> > 
> >> > shellcheck is not super happy about 'source' usage:
> >> > 
> >> > In bond_passive_lacp.sh line 7:
> >> > source ${lib_dir}/bond_topo_lacp.sh
> >> > ^-- SC3046 (warning): In POSIX sh, 'source' in place of '.' is undefined.
> >> > ^-- SC3051 (warning): In POSIX sh, 'source' in place of '.' is undefined.
> >> > 
> >> > either switch to '. ' or use bash instead of 'sh'.
> >> 
> >> Hi Paolo,
> >> 
> >> I updated the case and remove the source file bond_topo_lacp.sh.
> >> Instead I source the forwarding lib directly like:
> >> 
> >> lib_dir=$(dirname "$0")
> >> source "$lib_dir"/../../../net/forwarding/lib.sh
> >> 
> >> But this cause shell check unable to find the lib.sh as $lib_dir is get
> >> dynamically. This usage is common in selftest. How should we resolves this
> >> problem?
> >
> > OK, I just disabled this warning.
> >
> > # shellcheck disable=SC1091
> 
> I believe the point was only about using "." instead of "source". The
> following should have fixed it:
> 
> . ${lib_dir}/bond_topo_lacp.sh
> 
> ... or just use bash as the interpreter, I suspect lib.sh is not
> actually POSIX clean.

Thanks Petr, I know Paolo means to use "." to fix this. The issue is that
I changed the script to source forwarding lib. And shell check could only
analyse static path. Unless use -x to supply the real source path. But I guess
the CI can't do this. So I disabled the SC1091 checking as a workaround.

Regards
Hangbin