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

[Timur Stukan]

On Thu, 29 Oct 2026 at 17:51, Jay Vosburgh <jv@xxxxxxxxxxxxx> wrote:

>         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.
>
>         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);
>                         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@xxxxxxxxxxxxx


Hi Jay,

Thanks for the review.

I had the same thought and was planning to send it later as a separate
patch, but doing it here makes sense.

I went through the possible consequences. Without the loop, DOWN is
also applied only when bond_miimon_commit() reaches that slave. I
don't think other slaves handled earlier in the same pass can be
affected, because when each such slave goes DOWN, its own link change
rebuilds the arrays and redoes the aggregator selection anyway, so
nothing stale is left after the pass.

I tested it in 802.3ad, alb and active-backup with downdelay 0,
including two slaves going down, and the original FAIL->UP case with
downdelay. I'll send v3 with this change

Thanks,
Timur.