Re: [dpdk-dev] [PATCH] net/bonding: fix link properties with autoneg

[Matan Azrad <matan@mellanox.com>]

Hi Chas

From: Chas Williams, Monday, April 16, 2018 7:44 PM
> On Mon, Apr 16, 2018 at 4:06 AM, Matan Azrad <matan@mellanox.com>
> wrote:
> > Hi Chas
> >
> > From: Chas Williams, Wednesday, February 14, 2018 12:55 AM
> >> If a link is carrier down and using autonegotiation, then the PMD may
> >> not have detected a speed yet.  In this case the best we can do is
> >> ignore the link speed and duplex since they aren't valid.
> >
> > Ok for this.
> >
> >>  To be completely correct, there
> >> should be additional checks to prevent a slave that negotiates a
> >> different speed from being activated.
> >
> > Looks like every changing in the link properties should cause LSC interrupt.
> > In the bonding LCS interrupt you could handle and to deactivate the device.
> > Also you should deal with the case of the first slave, what is happen if the
> first slave has invalid link properties?
> > How can you know that the speed\duplex_mode is invalid?
> > Are we sure LACP mode can run with auto negotiation?
> 
> Yes, I am pretty sure bonding doesn't get this right when the interfaces
> aren't link up.  While what bonding is doing is likely wrong, it doesn't mean
> that the behavior of the PMDs are correct in leaving the link_status unset
> until the first LSC interrupt.
> 
> I plan to get around to looking at this bonding problem in a little bit.  Luckily it
> seems that we always tend to get matched links and even if bonding is
> advertising the wrong aggregate speed and duplex we are find for now.  It
> wouldn't pass close inspection by a protocol analyzer though.
> 

So, Are you going to fix it,
If no, I think you can open a bug in Bugzilla.

> >>
> >> Signed-off-by: Chas Williams <chas3@att.com>
> >> ---
> >>  drivers/net/bonding/rte_eth_bond_pmd.c | 7 ++++---
> >>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> >> b/drivers/net/bonding/rte_eth_bond_pmd.c
> >> index 92ad688..5559879 100644
> >> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> >> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> >> @@ -1545,9 +1545,10 @@ link_properties_valid(struct rte_eth_dev
> >> *ethdev,
> >>       if (bond_ctx->mode == BONDING_MODE_8023AD) {
> >>               struct rte_eth_link *bond_link = &bond_ctx-
> >> >mode4.slave_link;
> >>
> >> -             if (bond_link->link_duplex != slave_link->link_duplex ||
> >> -                     bond_link->link_autoneg != slave_link->link_autoneg
> >> ||
> >> -                     bond_link->link_speed != slave_link->link_speed)
> >> +             if (bond_link->link_autoneg != slave_link->link_autoneg ||
> >> +                 (bond_link->link_autoneg != ETH_LINK_AUTONEG &&
> >> +                  (bond_link->link_duplex != slave_link->link_duplex ||
> >> +                   bond_link->link_speed !=
> >> + slave_link->link_speed)))
> >>                       return -1;
> >>       }
> >>
> >> --
> >> 2.9.5
> >