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

[Chas Williams <3chas3@gmail.com>]

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.

>>
>> 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
>