This certainly seems more correct to me. I can't think of a reason that
you shouldn't be able to enslave whatever ports you like. Whether those
ports can become active is a different question. I need to think about
this a bit more before approval but I don't think it makes anything worse.
The current problems I see:
- There's still an activate_slave() at the bottom of
__eth_bond_slave_add_lock_free() -- We should be able to avoid this if
we fix LSC change to not miss slaves being added to the configuration (make
sure we get last_link_status right). That section is a little racy with
respect to stop/start and timer. If we don't do that, that activate still
need the guard against mis-matched link properties.
- What happens if you enslave multiple link down ports? The link speed is
taken from the first slave. What's speed of down link? How do other
interfaces match this property when they come up?
- We need to make sure deactivate/active happens on the LSC for the enslaved
interfaces all the time. A link may fail to activate, but if the down goes
down and comes back the "right" link speed, we need it to then activate.
- If all the links go down, should the "learned" default link properties be
cleared? We are a blank slate at this point, there's no reason not to
learn a new default from the first interface that comes up.
On Fri, Jun 1, 2018 at 6:05 AM, Radu Nicolau <radu.nico...@intel.com> wrote:
> Moved the link status validity check from the slave add to the slave
> activation step. Otherwise slave add will fail for mode 4 if
> the ports are all stopped but only one of them checked.
>
> Fixes: b77d21cc2364 ("ethdev: add link status get/set helper functions")
> Bugzilla ID: 52
>
> Signed-off-by: Radu Nicolau <radu.nico...@intel.com>
> ---
> v4: reworked patch
> v3: updated commit msg
> v2: add fix and Bugzilla references
>
> drivers/net/bonding/rte_eth_bond_api.c | 8 --------
> drivers/net/bonding/rte_eth_bond_pmd.c | 9 ++++++++-
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_api.c
> b/drivers/net/bonding/rte_eth_bond_api.c
> index d558df8..853b23b 100644
> --- a/drivers/net/bonding/rte_eth_bond_api.c
> +++ b/drivers/net/bonding/rte_eth_bond_api.c
> @@ -345,14 +345,6 @@ __eth_bond_slave_add_lock_free(uint16_t
> bonded_port_id, uint16_t slave_port_id)
> internals->tx_queue_offload_capa &=
> dev_info.tx_queue_offload_capa;
> internals->flow_type_rss_offloads &=
> dev_info.flow_type_rss_offloads;
>
> - if (link_properties_valid(bonded_eth_dev,
> - &slave_eth_dev->data->dev_link) != 0) {
> - RTE_BOND_LOG(ERR, "Invalid link properties for
> slave %d"
> - " in bonding mode %d",
> slave_port_id,
> - internals->mode);
> - return -1;
> - }
> -
> /* RETA size is GCD of all slaves RETA sizes, so, if all
> sizes will be
> * the power of 2, the lower one is GCD
> */
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 02d94b1..b84af43 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -2679,7 +2679,14 @@ bond_ethdev_lsc_event_callback(uint16_t port_id,
> enum rte_eth_event_type type,
> mac_address_slaves_update(bonded_eth_dev);
> }
>
> - activate_slave(bonded_eth_dev, port_id);
> + /* check link state properties */
> + if (link_properties_valid(bonded_eth_dev, &link)) {
> + activate_slave(bonded_eth_dev, port_id);
> + } else {
> + RTE_BOND_LOG(ERR, "Invalid link properties for
> slave %d"
> + " in bonding mode %d", port_id,
> + internals->mode);
> + }
>
> /* If user has defined the primary port then default to
> using it */
> if (internals->user_defined_primary_port &&
> --
> 2.7.5
>
>
