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

[Hangbin Liu]

On Mon, Oct 26, 2026 at 12:20:37PM +0200, Timur Stukan wrote:
> On Mon, 26 Oct 2026 at 02:56, Hangbin Liu <hangbin.liu@xxxxxxxxx> wrote:
> >
> > On Fri, Oct 23, 2026 at 03:35:57PM +0300, Timur Stukan wrote:
> > > On Fri, 23 Oct 2026 at 10:48, Hangbin Liu <hangbin.liu@xxxxxxxxx> wrote:
> > >
> > > > >  static void bond_miimon_commit(struct bonding *bond)
> > > > >  {
> > > > >       struct slave *slave, *primary, *active;
> > > > > @@ -2807,10 +2828,21 @@ static void bond_miimon_commit(struct bonding *bond)
> > > > >                       err = bond_update_speed_duplex(slave);
> > > > >                       netdev_unlock_ops(slave->dev);
> > > > >                       if (err && bond_needs_speed_duplex(bond)) {
> > > > > -                             slave->link = BOND_LINK_DOWN;
> > > >
> > > > Hmm, this issue only happens when link_state is on
> > > >
> > > >         netif_running(slave->dev) && netif_carrier_ok(slave->dev);
> > > >
> > > > while !netif_device_present. Should we set the link up when device detached?
> > >
> > >
> > > Hi Hangbin,
> > >
> > > Thanks for the review and the suggestion.
> > >
> > > A detached device is one way to get there, but it does not cover all
> > > the ways to hit this bug. In particular, it does not cover the case i
> > > reproduced: during inspect both link_state and netif_device_present()
> > > are true, but the carrier is lost after inspect and before commit, so
> > > reading speed/duplex fails in commit.
> >
> > Hi Timur,
> >
> > Thanks for the explain, but the same question to this patch. How do you
> > make sure the carrier is not lost during bond_miimon_inspect() and
> > this slave->link_prev_state setting?
> >
> > Thanks
> > Hangbin
> 
> Hi Hangbin,
> 
> Thanks for the question. To answer directly: there is no way to make
> sure the carrier is not lost between bond_miimon_inspect() and setting
> link_prev_state, but the fix does not need that.
> 
> Let's split the miimon logic with this patch into 3 steps, executed in
> this order:
> - bond_miimon_inspect() - carrier is checked here and new state is proposed.
> - setting slave->link_prev_state - just a copy of slave->link, it is
> not read from the carrier.
> - bond_miimon_commit(), case UP - speed/duplex check.
> 
> And there is the case where downdelay is set and active slave flaps,
> so it is currently in FAIL. Possible scenarios:
> a) The carrier is present during inspect, so bond_miimon_inspect()
> proposes link_new_state as UP for active slave. Then the carrier is
> lost somewhere between bond_miimon_inspect() and bond_miimon_commit().
> It does not matter whether this happens before (your question) or
> after setting link_prev_state, it is set to FAIL anyway. Then in
> bond_miimon_commit() in case UP, speed/duplex check fails because
> carrier is lost and because link_prev_state is FAIL, the active slave
> goes through a regular down path with failover.
> b) The carrier is lost before bond_miimon_inspect(), so UP is not
> proposed at all, and after downdelay the slave goes DOWN through the
> same regular down path.
> c) The carrier is lost after bond_miimon_commit(), so the slave is
> already UP. On the next miimon tick bond_miimon_inspect() proposes
> FAIL and then it follows the same path as in b).
> 
> So slave->link_prev_state is not affected by carrier changes between
> inspect and commit, that is just a copy of slave->link, needed to do
> the right thing in bond_miimon_commit() if the speed/duplex check
> fails (e.g. because the carrier is lost).
> 
> The same applies to the approach without link_prev_state I proposed to
> Jay, since there slave->link itself still holds the previous state in
> bond_miimon_commit().

Ah, yes. I miss-read that. The bond_propose_link_state() only set the
link_new_state to up. The slave->link state is still BOND_LINK_FAIL.

Thanks
Hangbin