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

[Timur Stukan]

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().

Thanks,
Timur.