Re: [PATCH net v2] bonding: fix missing down path on speed/duplex failure after FAIL->UP

[Hangbin Liu <hangbin.liu@linux.dev>]

Hi Jay,
On Thu, Oct 29, 2026 at 10:51:11AM -0500, Jay Vosburgh wrote:
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index 7019e5d8fdc8..9bafcddbb453 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> > 
> > 		case BOND_LINK_FAIL:
> >@@ -2919,6 +2935,12 @@ static void bond_mii_monitor(struct work_struct *work)
> > 
> > 		if (commit) {
> > 			bond_for_each_slave(bond, slave, iter) {
> >+				/* UP state is committed by bond_miimon_commit()
> >+				 * after the speed and duplex check, so that it
> >+				 * can still see the previous link state.
> >+				 */
> >+				if (slave->link_new_state == BOND_LINK_UP)
> >+					continue;
> > 				bond_commit_link_state(slave,
> > 						       BOND_SLAVE_NOTIFY_LATER);
> 
> 	Reading bond_commit_link_state(), for this call site, all it
> really does is slave->link = slave->link_new_state, when link_new_state
> != BOND_LINK_NOCHAGE.  The changes to ->should_notify_link only matter
> for the ARP monitor, so that part is effectively a no-op here.

+1

> 
> 	Therefore, I wonder if it would be cleaner overall to not do the
> bond_commit_link_state here at all, but instead keep what you've added
> in the BOND_LINK_UP part of bond_miimon_commit, and then add calls to
> bond_set_slave_link_state(slave, ..., BOND_SLAVE_NOTIFY_LATER) to the
> BOND_LINK_FAIL/BOND_LINK_BACK case to make the change stick.
> 
> 	A quick hack patch that I have not tested to hopefully
> illustrate what I mean:
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 756d79281ffc..5fb215fa85f6 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2877,6 +2877,8 @@ static void bond_miimon_commit(struct bonding *bond)
>  
>  		case BOND_LINK_FAIL:
>  		case BOND_LINK_BACK:
> +			bond_set_slave_link_state(slave, slave->link_new_state,
> +						  BOND_SLAVE_NOTIFY_LATER);

- bond_set_slave_link_state(slave, slave->link_new_state, BOND_SLAVE_NOTIFY_LATER)
  - bond_propose_link_state(slave, slave->link_new_state)
    - slave->link_new_state = slave->link_new_state

This looks like a no-op here.

Maybe just call bond_commit_link_state()?

The bond_set_slave_link_state() in bond_miimon_commit() seems also have this
issue.

Thanks
Hangbin
>  			slave_dbg(bond->dev, slave->dev, "link_new_state %d on slave\n",
>  				  slave->link_new_state);
>  			continue;
> @@ -2933,19 +2935,8 @@ static void bond_mii_monitor(struct work_struct *work)
>  			goto re_arm;
>  		}
>  
> -		if (commit) {
> -			bond_for_each_slave(bond, slave, iter) {
> -				/* UP state is committed by bond_miimon_commit()
> -				 * after the speed and duplex check, so that it
> -				 * can still see the previous link state.
> -				 */
> -				if (slave->link_new_state == BOND_LINK_UP)
> -					continue;
> -				bond_commit_link_state(slave,
> -						       BOND_SLAVE_NOTIFY_LATER);
> -			}
> +		if (commit)
>  			bond_miimon_commit(bond);
> -		}
>  
>  		if (bond->send_peer_notif)
>  			bond_peer_notify_may_events(bond, true);
> 
> 
> 	Thoughts?
> 
> 	-J
> 
> ---
> 	-Jay Vosburgh, jv@jvosburgh.net