The current ad_churn_machine implementation only transitions the
actor/partner churn state to churned or none after the churn timer expires.
However, IEEE 802.1AX-2014 specifies that a port should enter the none
state immediately once the actor’s port state enters synchronization.

Another issue is that if the churn timer expires while the churn machine is
not in the monitor state (e.g. already in churn), the state may remain
stuck indefinitely with no further transitions. This becomes visible in
multi-aggregator scenarios. For example:

Ports 1 and 2 are in aggregator 1 (active)
Ports 3 and 4 are in aggregator 2 (backup)

Ports 1 and 2 should be in none
Ports 3 and 4 should be in churned

If a failover occurs due to port 2 link down/up, aggregator 2 becomes active.
Under the current implementation, the resulting states may look like:

agg 1 (backup): port 1 -> none, port 2 -> churned
agg 2 (active): ports 3,4 keep in churned.

The root cause is that ad_churn_machine() only clears the
AD_PORT_CHURNED flag and starts a timer. When a churned port becomes active,
its RX state becomes AD_RX_CURRENT, preventing the churn flag from being set
again, leaving no way to retrigger the timer. Fixing this solely in
ad_rx_machine() is insufficient.

This patch rewrites ad_churn_machine according to IEEE 802.1AX-2014
(Figures 6-23 and 6-24), ensuring correct churn detection, state transitions,
and timer behavior. With new implementation, there is no need to set
AD_PORT_CHURNED in ad_rx_machine().

Fixes: 14c9551a32eb ("bonding: Implement port churn-machine (AD standard 43.4.17).")
Reported-by: Liang Li <liali@redhat.com>
Tested-by: Liang Li <liali@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_3ad.c | 96 +++++++++++++++++++++++++---------
 1 file changed, 71 insertions(+), 25 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index c47f6a69fd2a..68258d61fd1c 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -44,7 +44,6 @@
 #define AD_PORT_STANDBY         0x80
 #define AD_PORT_SELECTED        0x100
 #define AD_PORT_MOVED           0x200
-#define AD_PORT_CHURNED         (AD_PORT_ACTOR_CHURN | AD_PORT_PARTNER_CHURN)
 
 /* Port Key definitions
  * key is determined according to the link speed, duplex and
@@ -1254,7 +1253,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
 	/* first, check if port was reinitialized */
 	if (port->sm_vars & AD_PORT_BEGIN) {
 		port->sm_rx_state = AD_RX_INITIALIZE;
-		port->sm_vars |= AD_PORT_CHURNED;
 	/* check if port is not enabled */
 	} else if (!(port->sm_vars & AD_PORT_BEGIN) && !port->is_enabled)
 		port->sm_rx_state = AD_RX_PORT_DISABLED;
@@ -1262,8 +1260,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
 	else if (lacpdu && ((port->sm_rx_state == AD_RX_EXPIRED) ||
 		 (port->sm_rx_state == AD_RX_DEFAULTED) ||
 		 (port->sm_rx_state == AD_RX_CURRENT))) {
-		if (port->sm_rx_state != AD_RX_CURRENT)
-			port->sm_vars |= AD_PORT_CHURNED;
 		port->sm_rx_timer_counter = 0;
 		port->sm_rx_state = AD_RX_CURRENT;
 	} else {
@@ -1347,7 +1343,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
 			port->partner_oper.port_state |= LACP_STATE_LACP_TIMEOUT;
 			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;
 			break;
 		case AD_RX_DEFAULTED:
 			__update_default_selected(port);
@@ -1379,11 +1374,41 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
  * ad_churn_machine - handle port churn's state machine
  * @port: the port we're looking at
  *
+ * IEEE 802.1AX-2014 Figure 6-23 - Actor Churn Detection machine state diagram
+ *
+ *                                                     BEGIN || (! port_enabled)
+ *                                                               |
+ *                                      (3)                (1)   v
+ *   +----------------------+     ActorPort.Sync     +-------------------------+
+ *   |    NO_ACTOR_CHURN    | <--------------------- |   ACTOR_CHURN_MONITOR   |
+ *   |======================|                        |=========================|
+ *   | actor_churn = FALSE; |    ! ActorPort.Sync    | actor_churn = FALSE;    |
+ *   |                      | ---------------------> | Start actor_churn_timer |
+ *   +----------------------+           (4)          +-------------------------+
+ *             ^                                                 |
+ *             |                                                 |
+ *             |                                      actor_churn_timer expired
+ *             |                                                 |
+ *       ActorPort.Sync                                          |  (2)
+ *             |              +--------------------+             |
+ *        (3)  |              |   ACTOR_CHURN      |             |
+ *             |              |====================|             |
+ *             +------------- | actor_churn = True | <-----------+
+ *                            |                    |
+ *                            +--------------------+
+ *
+ * Similar for the Figure 6-24 - Partner Churn Detection machine state diagram
+ *
+ * We don’t need to check actor_churn, because it can only be true when the
+ * state is ACTOR_CHURN.
  */
 static void ad_churn_machine(struct port *port)
 {
-	if (port->sm_vars & AD_PORT_CHURNED) {
-		port->sm_vars &= ~AD_PORT_CHURNED;
+	bool partner_synced = port->partner_oper.port_state & LACP_STATE_SYNCHRONIZATION;
+	bool actor_synced = port->actor_oper_port_state & LACP_STATE_SYNCHRONIZATION;
+
+	/* ---- 1. begin or port not enabled ---- */
+	if ((port->sm_vars & AD_PORT_BEGIN) || !port->is_enabled) {
 		port->sm_churn_actor_state = AD_CHURN_MONITOR;
 		port->sm_churn_partner_state = AD_CHURN_MONITOR;
 		port->sm_churn_actor_timer_counter =
@@ -1392,25 +1417,46 @@ static void ad_churn_machine(struct port *port)
 			 __ad_timer_to_ticks(AD_PARTNER_CHURN_TIMER, 0);
 		return;
 	}
-	if (port->sm_churn_actor_timer_counter &&
-	    !(--port->sm_churn_actor_timer_counter) &&
-	    port->sm_churn_actor_state == AD_CHURN_MONITOR) {
-		if (port->actor_oper_port_state & LACP_STATE_SYNCHRONIZATION) {
-			port->sm_churn_actor_state = AD_NO_CHURN;
-		} else {
-			port->churn_actor_count++;
-			port->sm_churn_actor_state = AD_CHURN;
-		}
+
+	if (port->sm_churn_actor_timer_counter)
+		port->sm_churn_actor_timer_counter--;
+
+	if (port->sm_churn_partner_timer_counter)
+		port->sm_churn_partner_timer_counter--;
+
+	/* ---- 2. timer expired, enter CHURN ---- */
+	if (port->sm_churn_actor_state == AD_CHURN_MONITOR &&
+	    !port->sm_churn_actor_timer_counter) {
+		port->sm_churn_actor_state = AD_CHURN;
+		port->churn_actor_count++;
 	}
-	if (port->sm_churn_partner_timer_counter &&
-	    !(--port->sm_churn_partner_timer_counter) &&
-	    port->sm_churn_partner_state == AD_CHURN_MONITOR) {
-		if (port->partner_oper.port_state & LACP_STATE_SYNCHRONIZATION) {
-			port->sm_churn_partner_state = AD_NO_CHURN;
-		} else {
-			port->churn_partner_count++;
-			port->sm_churn_partner_state = AD_CHURN;
-		}
+
+	if (port->sm_churn_partner_state == AD_CHURN_MONITOR &&
+	    !port->sm_churn_partner_timer_counter) {
+		port->sm_churn_partner_state = AD_CHURN;
+		port->churn_partner_count++;
+	}
+
+	/* ---- 3. CHURN_MONITOR/CHURN + sync -> NO_CHURN ---- */
+	if ((port->sm_churn_actor_state == AD_CHURN_MONITOR ||
+	     port->sm_churn_actor_state == AD_CHURN) && actor_synced)
+		port->sm_churn_actor_state = AD_NO_CHURN;
+
+	if ((port->sm_churn_partner_state == AD_CHURN_MONITOR ||
+	     port->sm_churn_partner_state == AD_CHURN) && partner_synced)
+		port->sm_churn_partner_state = AD_NO_CHURN;
+
+	/* ---- 4. NO_CHURN + !sync -> MONITOR ---- */
+	if (port->sm_churn_actor_state == AD_NO_CHURN && !actor_synced) {
+		port->sm_churn_actor_state = AD_CHURN_MONITOR;
+		port->sm_churn_actor_timer_counter =
+			__ad_timer_to_ticks(AD_ACTOR_CHURN_TIMER, 0);
+	}
+
+	if (port->sm_churn_partner_state == AD_NO_CHURN && !partner_synced) {
+		port->sm_churn_partner_state = AD_CHURN_MONITOR;
+		port->sm_churn_partner_timer_counter =
+			__ad_timer_to_ticks(AD_PARTNER_CHURN_TIMER, 0);
 	}
 }
 
-- 
2.50.1

Hangbin Liu <liuhangbin@gmail.com> wrote:

>The current ad_churn_machine implementation only transitions the
>actor/partner churn state to churned or none after the churn timer expires.
>However, IEEE 802.1AX-2014 specifies that a port should enter the none
>state immediately once the actor’s port state enters synchronization.
>
>Another issue is that if the churn timer expires while the churn machine is
>not in the monitor state (e.g. already in churn), the state may remain
>stuck indefinitely with no further transitions. This becomes visible in
>multi-aggregator scenarios. For example:
>
>Ports 1 and 2 are in aggregator 1 (active)
>Ports 3 and 4 are in aggregator 2 (backup)
>
>Ports 1 and 2 should be in none
>Ports 3 and 4 should be in churned
>
>If a failover occurs due to port 2 link down/up, aggregator 2 becomes active.
>Under the current implementation, the resulting states may look like:
>
>agg 1 (backup): port 1 -> none, port 2 -> churned
>agg 2 (active): ports 3,4 keep in churned.
>
>The root cause is that ad_churn_machine() only clears the
>AD_PORT_CHURNED flag and starts a timer. When a churned port becomes active,
>its RX state becomes AD_RX_CURRENT, preventing the churn flag from being set
>again, leaving no way to retrigger the timer. Fixing this solely in
>ad_rx_machine() is insufficient.
>
>This patch rewrites ad_churn_machine according to IEEE 802.1AX-2014
>(Figures 6-23 and 6-24), ensuring correct churn detection, state transitions,
>and timer behavior. With new implementation, there is no need to set
>AD_PORT_CHURNED in ad_rx_machine().
>
>Fixes: 14c9551a32eb ("bonding: Implement port churn-machine (AD standard 43.4.17).")
>Reported-by: Liang Li <liali@redhat.com>
>Tested-by: Liang Li <liali@redhat.com>
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

	I missed this last time it was posted, but reading it now I
think the functional change looks good, but I question the usefulness of
including the 25 line ASCII art version of the state diagram.

	The standard is publicly available, so a comment saying that the
state machine logic conforms to IEEE 802.1AX-2014 figures 6-23 and 6-24
should be sufficient.  Anyone seriously checking the code against the
standard will need to read the relevant text, so they'll be looking it
up anyway.

	-J

>---
> drivers/net/bonding/bond_3ad.c | 96 +++++++++++++++++++++++++---------
> 1 file changed, 71 insertions(+), 25 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index c47f6a69fd2a..68258d61fd1c 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -44,7 +44,6 @@
> #define AD_PORT_STANDBY         0x80
> #define AD_PORT_SELECTED        0x100
> #define AD_PORT_MOVED           0x200
>-#define AD_PORT_CHURNED         (AD_PORT_ACTOR_CHURN | AD_PORT_PARTNER_CHURN)
> 
> /* Port Key definitions
>  * key is determined according to the link speed, duplex and
>@@ -1254,7 +1253,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
> 	/* first, check if port was reinitialized */
> 	if (port->sm_vars & AD_PORT_BEGIN) {
> 		port->sm_rx_state = AD_RX_INITIALIZE;
>-		port->sm_vars |= AD_PORT_CHURNED;
> 	/* check if port is not enabled */
> 	} else if (!(port->sm_vars & AD_PORT_BEGIN) && !port->is_enabled)
> 		port->sm_rx_state = AD_RX_PORT_DISABLED;
>@@ -1262,8 +1260,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
> 	else if (lacpdu && ((port->sm_rx_state == AD_RX_EXPIRED) ||
> 		 (port->sm_rx_state == AD_RX_DEFAULTED) ||
> 		 (port->sm_rx_state == AD_RX_CURRENT))) {
>-		if (port->sm_rx_state != AD_RX_CURRENT)
>-			port->sm_vars |= AD_PORT_CHURNED;
> 		port->sm_rx_timer_counter = 0;
> 		port->sm_rx_state = AD_RX_CURRENT;
> 	} else {
>@@ -1347,7 +1343,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
> 			port->partner_oper.port_state |= LACP_STATE_LACP_TIMEOUT;
> 			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;
> 			break;
> 		case AD_RX_DEFAULTED:
> 			__update_default_selected(port);
>@@ -1379,11 +1374,41 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
>  * ad_churn_machine - handle port churn's state machine
>  * @port: the port we're looking at
>  *
>+ * IEEE 802.1AX-2014 Figure 6-23 - Actor Churn Detection machine state diagram
>+ *
>+ *                                                     BEGIN || (! port_enabled)
>+ *                                                               |
>+ *                                      (3)                (1)   v
>+ *   +----------------------+     ActorPort.Sync     +-------------------------+
>+ *   |    NO_ACTOR_CHURN    | <--------------------- |   ACTOR_CHURN_MONITOR   |
>+ *   |======================|                        |=========================|
>+ *   | actor_churn = FALSE; |    ! ActorPort.Sync    | actor_churn = FALSE;    |
>+ *   |                      | ---------------------> | Start actor_churn_timer |
>+ *   +----------------------+           (4)          +-------------------------+
>+ *             ^                                                 |
>+ *             |                                                 |
>+ *             |                                      actor_churn_timer expired
>+ *             |                                                 |
>+ *       ActorPort.Sync                                          |  (2)
>+ *             |              +--------------------+             |
>+ *        (3)  |              |   ACTOR_CHURN      |             |
>+ *             |              |====================|             |
>+ *             +------------- | actor_churn = True | <-----------+
>+ *                            |                    |
>+ *                            +--------------------+
>+ *
>+ * Similar for the Figure 6-24 - Partner Churn Detection machine state diagram
>+ *
>+ * We don’t need to check actor_churn, because it can only be true when the
>+ * state is ACTOR_CHURN.
>  */
> static void ad_churn_machine(struct port *port)
> {
>-	if (port->sm_vars & AD_PORT_CHURNED) {
>-		port->sm_vars &= ~AD_PORT_CHURNED;
>+	bool partner_synced = port->partner_oper.port_state & LACP_STATE_SYNCHRONIZATION;
>+	bool actor_synced = port->actor_oper_port_state & LACP_STATE_SYNCHRONIZATION;
>+
>+	/* ---- 1. begin or port not enabled ---- */
>+	if ((port->sm_vars & AD_PORT_BEGIN) || !port->is_enabled) {
> 		port->sm_churn_actor_state = AD_CHURN_MONITOR;
> 		port->sm_churn_partner_state = AD_CHURN_MONITOR;
> 		port->sm_churn_actor_timer_counter =
>@@ -1392,25 +1417,46 @@ static void ad_churn_machine(struct port *port)
> 			 __ad_timer_to_ticks(AD_PARTNER_CHURN_TIMER, 0);
> 		return;
> 	}
>-	if (port->sm_churn_actor_timer_counter &&
>-	    !(--port->sm_churn_actor_timer_counter) &&
>-	    port->sm_churn_actor_state == AD_CHURN_MONITOR) {
>-		if (port->actor_oper_port_state & LACP_STATE_SYNCHRONIZATION) {
>-			port->sm_churn_actor_state = AD_NO_CHURN;
>-		} else {
>-			port->churn_actor_count++;
>-			port->sm_churn_actor_state = AD_CHURN;
>-		}
>+
>+	if (port->sm_churn_actor_timer_counter)
>+		port->sm_churn_actor_timer_counter--;
>+
>+	if (port->sm_churn_partner_timer_counter)
>+		port->sm_churn_partner_timer_counter--;
>+
>+	/* ---- 2. timer expired, enter CHURN ---- */
>+	if (port->sm_churn_actor_state == AD_CHURN_MONITOR &&
>+	    !port->sm_churn_actor_timer_counter) {
>+		port->sm_churn_actor_state = AD_CHURN;
>+		port->churn_actor_count++;
> 	}
>-	if (port->sm_churn_partner_timer_counter &&
>-	    !(--port->sm_churn_partner_timer_counter) &&
>-	    port->sm_churn_partner_state == AD_CHURN_MONITOR) {
>-		if (port->partner_oper.port_state & LACP_STATE_SYNCHRONIZATION) {
>-			port->sm_churn_partner_state = AD_NO_CHURN;
>-		} else {
>-			port->churn_partner_count++;
>-			port->sm_churn_partner_state = AD_CHURN;
>-		}
>+
>+	if (port->sm_churn_partner_state == AD_CHURN_MONITOR &&
>+	    !port->sm_churn_partner_timer_counter) {
>+		port->sm_churn_partner_state = AD_CHURN;
>+		port->churn_partner_count++;
>+	}
>+
>+	/* ---- 3. CHURN_MONITOR/CHURN + sync -> NO_CHURN ---- */
>+	if ((port->sm_churn_actor_state == AD_CHURN_MONITOR ||
>+	     port->sm_churn_actor_state == AD_CHURN) && actor_synced)
>+		port->sm_churn_actor_state = AD_NO_CHURN;
>+
>+	if ((port->sm_churn_partner_state == AD_CHURN_MONITOR ||
>+	     port->sm_churn_partner_state == AD_CHURN) && partner_synced)
>+		port->sm_churn_partner_state = AD_NO_CHURN;
>+
>+	/* ---- 4. NO_CHURN + !sync -> MONITOR ---- */
>+	if (port->sm_churn_actor_state == AD_NO_CHURN && !actor_synced) {
>+		port->sm_churn_actor_state = AD_CHURN_MONITOR;
>+		port->sm_churn_actor_timer_counter =
>+			__ad_timer_to_ticks(AD_ACTOR_CHURN_TIMER, 0);
>+	}
>+
>+	if (port->sm_churn_partner_state == AD_NO_CHURN && !partner_synced) {
>+		port->sm_churn_partner_state = AD_CHURN_MONITOR;
>+		port->sm_churn_partner_timer_counter =
>+			__ad_timer_to_ticks(AD_PARTNER_CHURN_TIMER, 0);
> 	}
> }
> 
>-- 
>2.50.1
>

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

On Thu, Feb 26, 2026 at 04:36:46PM -0800, Jay Vosburgh wrote:
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> >The current ad_churn_machine implementation only transitions the
> >actor/partner churn state to churned or none after the churn timer expires.
> >However, IEEE 802.1AX-2014 specifies that a port should enter the none
> >state immediately once the actor’s port state enters synchronization.
> >
> >Another issue is that if the churn timer expires while the churn machine is
> >not in the monitor state (e.g. already in churn), the state may remain
> >stuck indefinitely with no further transitions. This becomes visible in
> >multi-aggregator scenarios. For example:
> >
> >Ports 1 and 2 are in aggregator 1 (active)
> >Ports 3 and 4 are in aggregator 2 (backup)
> >
> >Ports 1 and 2 should be in none
> >Ports 3 and 4 should be in churned
> >
> >If a failover occurs due to port 2 link down/up, aggregator 2 becomes active.
> >Under the current implementation, the resulting states may look like:
> >
> >agg 1 (backup): port 1 -> none, port 2 -> churned
> >agg 2 (active): ports 3,4 keep in churned.
> >
> >The root cause is that ad_churn_machine() only clears the
> >AD_PORT_CHURNED flag and starts a timer. When a churned port becomes active,
> >its RX state becomes AD_RX_CURRENT, preventing the churn flag from being set
> >again, leaving no way to retrigger the timer. Fixing this solely in
> >ad_rx_machine() is insufficient.
> >
> >This patch rewrites ad_churn_machine according to IEEE 802.1AX-2014
> >(Figures 6-23 and 6-24), ensuring correct churn detection, state transitions,
> >and timer behavior. With new implementation, there is no need to set
> >AD_PORT_CHURNED in ad_rx_machine().
> >
> >Fixes: 14c9551a32eb ("bonding: Implement port churn-machine (AD standard 43.4.17).")
> >Reported-by: Liang Li <liali@redhat.com>
> >Tested-by: Liang Li <liali@redhat.com>
> >Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> 	I missed this last time it was posted, but reading it now I
> think the functional change looks good, but I question the usefulness of
> including the 25 line ASCII art version of the state diagram.
> 
> 	The standard is publicly available, so a comment saying that the
> state machine logic conforms to IEEE 802.1AX-2014 figures 6-23 and 6-24
> should be sufficient.  Anyone seriously checking the code against the
> standard will need to read the relevant text, so they'll be looking it
> up anyway.

I added it here to help new readers and reviewers understand the logic
quickly. If you think there’s no need to include it in the code, maybe
we can move it to the commit description?

Thanks
Hangbin

Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Thu, Feb 26, 2026 at 04:36:46PM -0800, Jay Vosburgh wrote:
>> Hangbin Liu <liuhangbin@gmail.com> wrote:
>> 
>> >The current ad_churn_machine implementation only transitions the
>> >actor/partner churn state to churned or none after the churn timer expires.
>> >However, IEEE 802.1AX-2014 specifies that a port should enter the none
>> >state immediately once the actor’s port state enters synchronization.
>> >
>> >Another issue is that if the churn timer expires while the churn machine is
>> >not in the monitor state (e.g. already in churn), the state may remain
>> >stuck indefinitely with no further transitions. This becomes visible in
>> >multi-aggregator scenarios. For example:
>> >
>> >Ports 1 and 2 are in aggregator 1 (active)
>> >Ports 3 and 4 are in aggregator 2 (backup)
>> >
>> >Ports 1 and 2 should be in none
>> >Ports 3 and 4 should be in churned
>> >
>> >If a failover occurs due to port 2 link down/up, aggregator 2 becomes active.
>> >Under the current implementation, the resulting states may look like:
>> >
>> >agg 1 (backup): port 1 -> none, port 2 -> churned
>> >agg 2 (active): ports 3,4 keep in churned.
>> >
>> >The root cause is that ad_churn_machine() only clears the
>> >AD_PORT_CHURNED flag and starts a timer. When a churned port becomes active,
>> >its RX state becomes AD_RX_CURRENT, preventing the churn flag from being set
>> >again, leaving no way to retrigger the timer. Fixing this solely in
>> >ad_rx_machine() is insufficient.
>> >
>> >This patch rewrites ad_churn_machine according to IEEE 802.1AX-2014
>> >(Figures 6-23 and 6-24), ensuring correct churn detection, state transitions,
>> >and timer behavior. With new implementation, there is no need to set
>> >AD_PORT_CHURNED in ad_rx_machine().
>> >
>> >Fixes: 14c9551a32eb ("bonding: Implement port churn-machine (AD standard 43.4.17).")
>> >Reported-by: Liang Li <liali@redhat.com>
>> >Tested-by: Liang Li <liali@redhat.com>
>> >Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>> 
>> 	I missed this last time it was posted, but reading it now I
>> think the functional change looks good, but I question the usefulness of
>> including the 25 line ASCII art version of the state diagram.
>> 
>> 	The standard is publicly available, so a comment saying that the
>> state machine logic conforms to IEEE 802.1AX-2014 figures 6-23 and 6-24
>> should be sufficient.  Anyone seriously checking the code against the
>> standard will need to read the relevant text, so they'll be looking it
>> up anyway.
>
>I added it here to help new readers and reviewers understand the logic
>quickly. If you think there’s no need to include it in the code, maybe
>we can move it to the commit description?

	Personally, I'd leave it out, and just put a reference.

	The churn machine isn't that critical, even the standards
committee didn't like it and removed it from the 2020 edition of
802.1AX.

	Still, whatever we provide should work in accordance with the
2014 standard we're nominally conforming to, so functionally the patch
looks fine to me.

	-J

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

On Thu, Feb 26, 2026 at 05:42:13PM -0800, Jay Vosburgh wrote:
> >I added it here to help new readers and reviewers understand the logic
> >quickly. If you think there’s no need to include it in the code, maybe
> >we can move it to the commit description?
> 
> 	Personally, I'd leave it out, and just put a reference.
> 
> 	The churn machine isn't that critical, even the standards
> committee didn't like it and removed it from the 2020 edition of
> 802.1AX.

Oh, I didn't notice this. I posted a patch[1] to net-next to export the
churn state via netlink recently. Should I revert this patch?

> 
> 	Still, whatever we provide should work in accordance with the
> 2014 standard we're nominally conforming to, so functionally the patch
> looks fine to me.

Got it, I will remove it.

[1] http://lore.kernel.org/netdev/20260224020215.6012-1-liuhangbin@gmail.com

Thanks
Hangbin