[PATCH] bonding: allow to create master bonding

[PATCH] bonding: allow to create master bonding

[Tomasz Kulasek]

This patch removes restrictions in bonded device library which prevent a
bonded device to be added to another bonded device.

There's two limitations to create master bonding:

 - Total depth of nesting is limited to two levels,
 - 802.3ad mode is not supported if one or more slaves is a bond device,


Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
---
 drivers/net/bonding/rte_eth_bond_api.c     | 63 ++++++++++++++++++++++++------
 drivers/net/bonding/rte_eth_bond_pmd.c     |  6 +++
 drivers/net/bonding/rte_eth_bond_private.h |  5 ++-
 3 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 36ec65d..05f565d 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2010-2017 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -63,13 +63,45 @@
 }
 
 int
-valid_slave_port_id(uint8_t port_id)
+check_for_master_bonded_ethdev(const struct rte_eth_dev *eth_dev) {
+	int i;
+	struct bond_dev_private *internals;
+
+	if (check_for_bonded_ethdev(eth_dev) != 0)
+		return 0;
+
+	internals = eth_dev->data->dev_private;
+
+	/* Check if any of slave devices is a bonded device */
+	for (i = 0; i < internals->slave_count; i++)
+		if (valid_bonded_port_id(internals->slaves[i].port_id) == 0)
+			return 1;
+
+	return 0;
+}
+
+int
+valid_slave_port_id(uint8_t port_id, uint8_t mode)
 {
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -1);
 
 	/* Verify that port_id refers to a non bonded port */
-	if (check_for_bonded_ethdev(&rte_eth_devices[port_id]) == 0)
-		return -1;
+	if (check_for_bonded_ethdev(&rte_eth_devices[port_id]) == 0) {
+		if (mode == BONDING_MODE_8023AD) {
+			RTE_BOND_LOG(ERR, "One or more slaves is a bond device,"
+					" there 802.3ad mode can not be"
+					" supported on this bond device.");
+			return -1;
+		}
+
+		if (check_for_master_bonded_ethdev(&rte_eth_devices[port_id])) {
+			RTE_BOND_LOG(ERR, "Too many levels of bonding");
+			return -1;
+		}
+
+		/* Slave is in master bonding */
+		return 1;
+	}
 
 	return 0;
 }
@@ -250,14 +282,19 @@
 	struct bond_dev_private *internals;
 	struct rte_eth_link link_props;
 	struct rte_eth_dev_info dev_info;
-
-	if (valid_slave_port_id(slave_port_id) != 0)
-		return -1;
+	int status;
 
 	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
 	internals = bonded_eth_dev->data->dev_private;
 
+	status = valid_slave_port_id(slave_port_id, internals->mode);
+
+	/* Slave is invalid */
+	if (status < 0)
+		return -1;
+
 	slave_eth_dev = &rte_eth_devices[slave_port_id];
+
 	if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_BONDED_SLAVE) {
 		RTE_BOND_LOG(ERR, "Slave device is already a slave of a bonded device");
 		return -1;
@@ -402,12 +439,12 @@
 	struct rte_eth_dev *slave_eth_dev;
 	int i, slave_idx;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
-		return -1;
-
 	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
 	internals = bonded_eth_dev->data->dev_private;
 
+	if (valid_slave_port_id(slave_port_id, internals->mode) < 0)
+		return -1;
+
 	/* first remove from active slave list */
 	slave_idx = find_slave_by_id(internals->active_slaves,
 		internals->active_slave_count, slave_port_id);
@@ -528,11 +565,11 @@
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
-		return -1;
-
 	internals =  rte_eth_devices[bonded_port_id].data->dev_private;
 
+	if (valid_slave_port_id(slave_port_id, internals->mode) < 0)
+		return -1;
+
 	internals->user_defined_primary_port = 1;
 	internals->primary_port = slave_port_id;
 
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 82959ab..d477f37 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1292,6 +1292,12 @@ struct bwg_slave {
 		eth_dev->rx_pkt_burst = bond_ethdev_rx_burst;
 		break;
 	case BONDING_MODE_8023AD:
+		if (check_for_master_bonded_ethdev(eth_dev) == 1) {
+			RTE_BOND_LOG(ERR, "One or more slaves is a bond device,"
+					" there 802.3ad mode can not be"
+					" supported on this bond device.");
+			return -1;
+		}
 		if (bond_mode_8023ad_enable(eth_dev) != 0)
 			return -1;
 
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index c8db090..1a10564 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -184,6 +184,9 @@ struct bond_dev_private {
 int
 check_for_bonded_ethdev(const struct rte_eth_dev *eth_dev);
 
+int
+check_for_master_bonded_ethdev(const struct rte_eth_dev *eth_dev);
+
 /* Search given slave array to find possition of given id.
  * Return slave pos or slaves_count if not found. */
 static inline uint8_t
@@ -205,7 +208,7 @@ struct bond_dev_private {
 valid_bonded_port_id(uint8_t port_id);
 
 int
-valid_slave_port_id(uint8_t port_id);
+valid_slave_port_id(uint8_t port_id, uint8_t mode);
 
 void
 deactivate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id);
-- 
1.9.1

[PATCH v2] bonding: allow to create master bonding

[Tomasz Kulasek]

This patch removes restrictions in bonded device library which prevent a
bonded device to be added to another bonded device.

Added link speed calculation for a bonding with fallowed rules:
 - BROADCAST - Minimal slaves link speed
 - ACTIVE_BACKUP - Current primary slave
 - ROUND_ROBIN, BALANCE, 8023AD, TLB, ALB - Sum of slaves link speeds

There's two limitations to create master bonding:

 - Total depth of nesting is limited to two levels,
 - 802.3ad mode is not supported if one or more slaves is a bond device,

---
v2 changes:
 - added dynamic link speed recalculation for a bonding

Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
---
 drivers/net/bonding/rte_eth_bond_api.c     | 63 ++++++++++++++++++++------
 drivers/net/bonding/rte_eth_bond_pmd.c     | 73 +++++++++++++++++++++++++++---
 drivers/net/bonding/rte_eth_bond_private.h |  7 ++-
 3 files changed, 123 insertions(+), 20 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 55d71d9..e1e7564 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2010-2017 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -63,13 +63,45 @@
 }
 
 int
-valid_slave_port_id(uint8_t port_id)
+check_for_master_bonded_ethdev(const struct rte_eth_dev *eth_dev) {
+	int i;
+	struct bond_dev_private *internals;
+
+	if (check_for_bonded_ethdev(eth_dev) != 0)
+		return 0;
+
+	internals = eth_dev->data->dev_private;
+
+	/* Check if any of slave devices is a bonded device */
+	for (i = 0; i < internals->slave_count; i++)
+		if (valid_bonded_port_id(internals->slaves[i].port_id) == 0)
+			return 1;
+
+	return 0;
+}
+
+int
+valid_slave_port_id(uint8_t port_id, uint8_t mode)
 {
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -1);
 
 	/* Verify that port_id refers to a non bonded port */
-	if (check_for_bonded_ethdev(&rte_eth_devices[port_id]) == 0)
-		return -1;
+	if (check_for_bonded_ethdev(&rte_eth_devices[port_id]) == 0) {
+		if (mode == BONDING_MODE_8023AD) {
+			RTE_BOND_LOG(ERR, "One or more slaves is a bond device,"
+					" there 802.3ad mode can not be"
+					" supported on this bond device.");
+			return -1;
+		}
+
+		if (check_for_master_bonded_ethdev(&rte_eth_devices[port_id])) {
+			RTE_BOND_LOG(ERR, "Too many levels of bonding");
+			return -1;
+		}
+
+		/* Slave is in master bonding */
+		return 1;
+	}
 
 	return 0;
 }
@@ -234,14 +266,19 @@
 	struct bond_dev_private *internals;
 	struct rte_eth_link link_props;
 	struct rte_eth_dev_info dev_info;
-
-	if (valid_slave_port_id(slave_port_id) != 0)
-		return -1;
+	int status;
 
 	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
 	internals = bonded_eth_dev->data->dev_private;
 
+	status = valid_slave_port_id(slave_port_id, internals->mode);
+
+	/* Slave is invalid */
+	if (status < 0)
+		return -1;
+
 	slave_eth_dev = &rte_eth_devices[slave_port_id];
+
 	if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_BONDED_SLAVE) {
 		RTE_BOND_LOG(ERR, "Slave device is already a slave of a bonded device");
 		return -1;
@@ -386,12 +423,12 @@
 	struct rte_eth_dev *slave_eth_dev;
 	int i, slave_idx;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
-		return -1;
-
 	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
 	internals = bonded_eth_dev->data->dev_private;
 
+	if (valid_slave_port_id(slave_port_id, internals->mode) < 0)
+		return -1;
+
 	/* first remove from active slave list */
 	slave_idx = find_slave_by_id(internals->active_slaves,
 		internals->active_slave_count, slave_port_id);
@@ -512,11 +549,11 @@
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
-		return -1;
-
 	internals =  rte_eth_devices[bonded_port_id].data->dev_private;
 
+	if (valid_slave_port_id(slave_port_id, internals->mode) < 0)
+		return -1;
+
 	internals->user_defined_primary_port = 1;
 	internals->primary_port = slave_port_id;
 
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index a8d9780..73e1032 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -79,6 +79,50 @@
 	return vlan_offset;
 }
 
+static uint32_t
+get_link_speed(struct rte_eth_dev *bonded_eth_dev)
+{
+	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	int i;
+
+	struct rte_eth_link link_status;
+	uint32_t link_speed;
+
+	if (internals->active_slave_count < 1)
+		return 0;
+
+	switch (internals->mode) {
+	case BONDING_MODE_BROADCAST:
+		/* Minimal slaves link speed */
+		rte_eth_link_get(internals->active_slaves[0], &link_status);
+		link_speed = link_status.link_speed;
+		for (i = 1; i < internals->active_slave_count; i++) {
+			rte_eth_link_get(internals->active_slaves[i], &link_status);
+			if (link_status.link_speed < link_speed)
+				link_speed = link_status.link_speed;
+		}
+		break;
+	case BONDING_MODE_ACTIVE_BACKUP:
+		/* Current primary slave */
+		rte_eth_link_get(internals->current_primary_port, &link_status);
+		link_speed = link_status.link_speed;
+		break;
+	case BONDING_MODE_ROUND_ROBIN:
+	case BONDING_MODE_BALANCE:
+	case BONDING_MODE_8023AD:
+	case BONDING_MODE_TLB:
+	case BONDING_MODE_ALB:
+	default:
+		/* Sum of slaves link speeds */
+		link_speed = 0;
+		for (i = 0; i < internals->active_slave_count; i++) {
+			rte_eth_link_get(internals->active_slaves[i], &link_status);
+			link_speed += link_status.link_speed;
+		}
+	}
+	return link_speed;
+}
+
 static uint16_t
 bond_ethdev_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 {
@@ -1139,9 +1183,10 @@ struct bwg_slave {
 
 	if (slave_dev_link->link_status &&
 		bonded_eth_dev->data->dev_started) {
-		bonded_dev_link->link_duplex = slave_dev_link->link_duplex;
-		bonded_dev_link->link_speed = slave_dev_link->link_speed;
-
+		internals->slave_link.link_duplex = slave_dev_link->link_duplex;
+		internals->slave_link.link_speed = slave_dev_link->link_speed;
+		memcpy(bonded_dev_link, &internals->slave_link,
+				sizeof(struct rte_eth_link));
 		internals->link_props_set = 1;
 	}
 }
@@ -1153,6 +1198,8 @@ struct bwg_slave {
 
 	memset(&(bonded_eth_dev->data->dev_link), 0,
 			sizeof(bonded_eth_dev->data->dev_link));
+	memset(&(internals->slave_link), 0,
+			sizeof(bonded_eth_dev->data->dev_link));
 
 	internals->link_props_set = 0;
 }
@@ -1292,6 +1339,12 @@ struct bwg_slave {
 		eth_dev->rx_pkt_burst = bond_ethdev_rx_burst;
 		break;
 	case BONDING_MODE_8023AD:
+		if (check_for_master_bonded_ethdev(eth_dev) == 1) {
+			RTE_BOND_LOG(ERR, "One or more slaves is a bond device,"
+					" there 802.3ad mode can not be"
+					" supported on this bond device.");
+			return -1;
+		}
 		if (bond_mode_8023ad_enable(eth_dev) != 0)
 			return -1;
 
@@ -1887,6 +1940,8 @@ struct bwg_slave {
 		}
 
 		bonded_eth_dev->data->dev_link.link_status = link_up;
+		bonded_eth_dev->data->dev_link.link_speed = get_link_speed(
+				bonded_eth_dev);
 	}
 
 	return 0;
@@ -2009,6 +2064,7 @@ struct bwg_slave {
 	int i, valid_slave = 0;
 	uint8_t active_pos;
 	uint8_t lsc_flag = 0;
+	uint8_t master_bonding;
 
 	if (type != RTE_ETH_EVENT_INTR_LSC || param == NULL)
 		return;
@@ -2058,9 +2114,11 @@ struct bwg_slave {
 			link_properties_set(bonded_eth_dev,
 					&(slave_eth_dev->data->dev_link));
 		} else {
-			if (link_properties_valid(
-				&bonded_eth_dev->data->dev_link, &link) != 0) {
-				slave_eth_dev->data->dev_flags &=
+			master_bonding = check_for_master_bonded_ethdev(
+					bonded_eth_dev);
+			if ((master_bonding == 0) && link_properties_valid(
+				&internals->slave_link, &link) != 0) {
+					slave_eth_dev->data->dev_flags &=
 					(~RTE_ETH_DEV_BONDED_SLAVE);
 				RTE_LOG(ERR, PMD,
 					"port %u invalid speed/duplex\n",
@@ -2128,6 +2186,9 @@ struct bwg_slave {
 						RTE_ETH_EVENT_INTR_LSC, NULL);
 		}
 	}
+
+	bonded_eth_dev->data->dev_link.link_speed = get_link_speed(
+			bonded_eth_dev);
 }
 
 static int
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 50d908d..e27ceb8 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -135,6 +135,8 @@ struct bond_dev_private {
 	uint8_t link_props_set;
 	/**< flag to denote if the link properties are set */
 
+	struct rte_eth_link slave_link;
+
 	uint8_t link_status_polling_enabled;
 	uint32_t link_status_polling_interval_ms;
 
@@ -184,6 +186,9 @@ struct bond_dev_private {
 int
 check_for_bonded_ethdev(const struct rte_eth_dev *eth_dev);
 
+int
+check_for_master_bonded_ethdev(const struct rte_eth_dev *eth_dev);
+
 /* Search given slave array to find position of given id.
  * Return slave pos or slaves_count if not found. */
 static inline uint8_t
@@ -205,7 +210,7 @@ struct bond_dev_private {
 valid_bonded_port_id(uint8_t port_id);
 
 int
-valid_slave_port_id(uint8_t port_id);
+valid_slave_port_id(uint8_t port_id, uint8_t mode);
 
 void
 deactivate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id);
-- 
1.9.1

[PATCH v3 1/2] net/bond: fixes for link properties management

[Declan Doherty]

From: Tomasz Kulasek <tomaszx.kulasek@intel.com>

This patch fixes the management of link properties in the bonded device.

In all mode except mode 4 a bonded device link will default to reporting
the link as full duplex and auto-neg. The link speed for a bond port is
calculated on it's active slaves and the particular mode it is running
in. The bonding link speed is reported based on the transmit link as in
some modes link speed between egress/ingress is not symmetrical.

- round-robin, balance, 802.3ad, TLB and ALB modes all report the link
  speed as the sum of the speed of each active slave.
- active backup link speed is reported as the speed of the current
  primary slave
- broadcast is reported as the minimum of value of the active slaves
  link speeds.

In mode 4 (link aggregation 802.3ad) the properties of the first slave
added to the bonded device are slave and subsequent slaves are verified
to have the same properties.

Finally in the bond_ethdev_lsc_event_callback function the link
properties of the device are updated after any change to the number of
active slaves.

Signed-off-by: Declan Doherty <declan.doherty@intel.com>
Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
---
 drivers/net/bonding/rte_eth_bond_8023ad_private.h |   3 +
 drivers/net/bonding/rte_eth_bond_api.c            |   6 +-
 drivers/net/bonding/rte_eth_bond_pmd.c            | 170 +++++++++++++---------
 drivers/net/bonding/rte_eth_bond_private.h        |   8 +-
 4 files changed, 112 insertions(+), 75 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad_private.h b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
index ca8858b..3fd4f40 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad_private.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
@@ -175,6 +175,9 @@ struct mode8023ad_private {
 	uint64_t update_timeout_us;
 	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
 	uint8_t external_sm;
+
+	struct rte_eth_link slave_link;
+	/***< slave link properties */
 };
 
 /**
diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 55d71d9..82c4499 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -297,6 +297,9 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 		internals->tx_offload_capa &= dev_info.tx_offload_capa;
 		internals->flow_type_rss_offloads &= dev_info.flow_type_rss_offloads;
 
+		link_properties_valid(bonded_eth_dev,
+				&slave_eth_dev->data->dev_link);
+
 		/* RETA size is GCD of all slaves RETA sizes, so, if all sizes will be
 		 * the power of 2, the lower one is GCD
 		 */
@@ -439,9 +442,6 @@ __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 	}
 
 	if (internals->active_slave_count < 1) {
-		/* reset device link properties as no slaves are active */
-		link_properties_reset(&rte_eth_devices[bonded_port_id]);
-
 		/* if no slaves are any longer attached to bonded device and MAC is not
 		 * user defined then clear MAC of bonded device as it will be reset
 		 * when a new slave is added */
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index dccc016..2b78a5e 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1131,39 +1131,44 @@ bond_ethdev_tx_burst_broadcast(void *queue, struct rte_mbuf **bufs,
 }
 
 void
-link_properties_set(struct rte_eth_dev *bonded_eth_dev,
-		struct rte_eth_link *slave_dev_link)
+link_properties_set(struct rte_eth_dev *ethdev, struct rte_eth_link *slave_link)
 {
-	struct rte_eth_link *bonded_dev_link = &bonded_eth_dev->data->dev_link;
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct bond_dev_private *bond_ctx = ethdev->data->dev_private;
 
-	if (slave_dev_link->link_status &&
-		bonded_eth_dev->data->dev_started) {
-		bonded_dev_link->link_duplex = slave_dev_link->link_duplex;
-		bonded_dev_link->link_speed = slave_dev_link->link_speed;
+	if (bond_ctx->mode == BONDING_MODE_8023AD) {
+		/**
+		 * If in mode 4 then save the link properties of the first
+		 * slave, all subsequent slaves must match these properties
+		 */
+		struct rte_eth_link *bond_link = &bond_ctx->mode4.slave_link;
 
-		internals->link_props_set = 1;
+		bond_link->link_autoneg = slave_link->link_autoneg;
+		bond_link->link_duplex = slave_link->link_duplex;
+		bond_link->link_speed = slave_link->link_speed;
+	} else {
+		/**
+		 * In any other mode the link properties are set to default
+		 * values of AUTONEG/DUPLEX
+		 */
+		ethdev->data->dev_link.link_autoneg = ETH_LINK_AUTONEG;
+		ethdev->data->dev_link.link_duplex = ETH_LINK_FULL_DUPLEX;
 	}
 }
 
-void
-link_properties_reset(struct rte_eth_dev *bonded_eth_dev)
+int
+link_properties_valid(struct rte_eth_dev *ethdev,
+		struct rte_eth_link *slave_link)
 {
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct bond_dev_private *bond_ctx = ethdev->data->dev_private;
 
-	memset(&(bonded_eth_dev->data->dev_link), 0,
-			sizeof(bonded_eth_dev->data->dev_link));
+	if (bond_ctx->mode == BONDING_MODE_8023AD) {
+		struct rte_eth_link *bond_link = &bond_ctx->mode4.slave_link;
 
-	internals->link_props_set = 0;
-}
-
-int
-link_properties_valid(struct rte_eth_link *bonded_dev_link,
-		struct rte_eth_link *slave_dev_link)
-{
-	if (bonded_dev_link->link_duplex != slave_dev_link->link_duplex ||
-		bonded_dev_link->link_speed !=  slave_dev_link->link_speed)
-		return -1;
+		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)
+			return -1;
+	}
 
 	return 0;
 }
@@ -1864,36 +1869,90 @@ bond_ethdev_slave_link_status_change_monitor(void *cb_arg)
 }
 
 static int
-bond_ethdev_link_update(struct rte_eth_dev *bonded_eth_dev,
-		int wait_to_complete)
+bond_ethdev_link_update(struct rte_eth_dev *ethdev, int wait_to_complete)
 {
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	void (*link_update)(uint8_t port_id, struct rte_eth_link *eth_link);
+
+	struct bond_dev_private *bond_ctx;
+	struct rte_eth_link slave_link;
+
+	uint32_t idx;
+
+	bond_ctx = ethdev->data->dev_private;
 
-	if (!bonded_eth_dev->data->dev_started ||
-		internals->active_slave_count == 0) {
-		bonded_eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
+	ethdev->data->dev_link.link_speed = ETH_SPEED_NUM_NONE;
+
+	if (ethdev->data->dev_started == 0 ||
+			bond_ctx->active_slave_count == 0) {
+		ethdev->data->dev_link.link_status = ETH_LINK_DOWN;
 		return 0;
-	} else {
-		struct rte_eth_dev *slave_eth_dev;
-		int i, link_up = 0;
+	}
 
-		for (i = 0; i < internals->active_slave_count; i++) {
-			slave_eth_dev = &rte_eth_devices[internals->active_slaves[i]];
+	ethdev->data->dev_link.link_status = ETH_LINK_UP;
 
-			(*slave_eth_dev->dev_ops->link_update)(slave_eth_dev,
-					wait_to_complete);
-			if (slave_eth_dev->data->dev_link.link_status == ETH_LINK_UP) {
-				link_up = 1;
-				break;
-			}
+	if (wait_to_complete)
+		link_update = rte_eth_link_get;
+	else
+		link_update = rte_eth_link_get_nowait;
+
+	switch (bond_ctx->mode) {
+	case BONDING_MODE_BROADCAST:
+		/**
+		 * Setting link speed to UINT32_MAX to ensure we pick up the
+		 * value of the first active slave
+		 */
+		ethdev->data->dev_link.link_speed = UINT32_MAX;
+
+		/**
+		 * link speed is minimum value of all the slaves link speed as
+		 * packet loss will occur on this slave if transmission at rates
+		 * greater than this are attempted
+		 */
+		for (idx = 1; idx < bond_ctx->active_slave_count; idx++) {
+			link_update(bond_ctx->active_slaves[0],	&slave_link);
+
+			if (slave_link.link_speed <
+					ethdev->data->dev_link.link_speed)
+				ethdev->data->dev_link.link_speed =
+						slave_link.link_speed;
 		}
+		break;
+	case BONDING_MODE_ACTIVE_BACKUP:
+		/* Current primary slave */
+		link_update(bond_ctx->current_primary_port, &slave_link);
+
+		ethdev->data->dev_link.link_speed = slave_link.link_speed;
+		break;
+	case BONDING_MODE_8023AD:
+		ethdev->data->dev_link.link_autoneg =
+				bond_ctx->mode4.slave_link.link_autoneg;
+		ethdev->data->dev_link.link_duplex =
+				bond_ctx->mode4.slave_link.link_duplex;
+		/* fall through to update link speed */
+	case BONDING_MODE_ROUND_ROBIN:
+	case BONDING_MODE_BALANCE:
+	case BONDING_MODE_TLB:
+	case BONDING_MODE_ALB:
+	default:
+		/**
+		 * In theses mode the maximum theoretical link speed is the sum
+		 * of all the slaves
+		 */
+		ethdev->data->dev_link.link_speed = ETH_SPEED_NUM_NONE;
 
-		bonded_eth_dev->data->dev_link.link_status = link_up;
+		for (idx = 0; idx < bond_ctx->active_slave_count; idx++) {
+			link_update(bond_ctx->active_slaves[idx], &slave_link);
+
+			ethdev->data->dev_link.link_speed +=
+					slave_link.link_speed;
+		}
 	}
 
+
 	return 0;
 }
 
+
 static void
 bond_ethdev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
@@ -2056,20 +2115,6 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 			lsc_flag = 1;
 
 			mac_address_slaves_update(bonded_eth_dev);
-
-			/* Inherit eth dev link properties from first active slave */
-			link_properties_set(bonded_eth_dev,
-					&(slave_eth_dev->data->dev_link));
-		} else {
-			if (link_properties_valid(
-				&bonded_eth_dev->data->dev_link, &link) != 0) {
-				slave_eth_dev->data->dev_flags &=
-					(~RTE_ETH_DEV_BONDED_SLAVE);
-				RTE_LOG(ERR, PMD,
-					"port %u invalid speed/duplex\n",
-					port_id);
-				return rc;
-			}
 		}
 
 		activate_slave(bonded_eth_dev, port_id);
@@ -2085,15 +2130,6 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		/* Remove from active slave list */
 		deactivate_slave(bonded_eth_dev, port_id);
 
-		/* No active slaves, change link status to down and reset other
-		 * link properties */
-		if (internals->active_slave_count < 1) {
-			lsc_flag = 1;
-			bonded_eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
-
-			link_properties_reset(bonded_eth_dev);
-		}
-
 		/* Update primary id, take first active slave from list or if none
 		 * available set to -1 */
 		if (port_id == internals->current_primary_port) {
@@ -2105,6 +2141,9 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		}
 	}
 
+	/* update bonded device link after any change to active slaves */
+	bond_ethdev_link_update(bonded_eth_dev, 0);
+
 	if (lsc_flag) {
 		/* Cancel any possible outstanding interrupts if delays are enabled */
 		if (internals->link_up_delay_ms > 0 ||
@@ -2308,7 +2347,6 @@ bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
 	internals->balance_xmit_policy = BALANCE_XMIT_POLICY_LAYER2;
 	internals->xmit_hash = xmit_l2_hash;
 	internals->user_defined_mac = 0;
-	internals->link_props_set = 0;
 
 	internals->link_status_polling_enabled = 0;
 
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 53470f6..7295192 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -132,8 +132,7 @@ struct bond_dev_private {
 	/**< Flag for whether MAC address is user defined or not */
 	uint8_t promiscuous_en;
 	/**< Enabled/disable promiscuous mode on bonding device */
-	uint8_t link_props_set;
-	/**< flag to denote if the link properties are set */
+
 
 	uint8_t link_status_polling_enabled;
 	uint32_t link_status_polling_interval_ms;
@@ -216,11 +215,8 @@ activate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id);
 void
 link_properties_set(struct rte_eth_dev *bonded_eth_dev,
 		struct rte_eth_link *slave_dev_link);
-void
-link_properties_reset(struct rte_eth_dev *bonded_eth_dev);
-
 int
-link_properties_valid(struct rte_eth_link *bonded_dev_link,
+link_properties_valid(struct rte_eth_dev *bonded_eth_dev,
 		struct rte_eth_link *slave_dev_link);
 
 int
-- 
2.9.4

[PATCH v3 2/2] net/bond: allow slaves to also be bonded devices

[Declan Doherty]

From: Tomasz Kulasek <tomaszx.kulasek@intel.com>

This patch removes restrictions in bonded device library which prevent a
bonded device to be added to another bonded device with the limitation
that 802.3ad mode is not supported if one or more slaves is also a
bonded device,

---
v3: reworked link management changes and split into separate patch

v2 changes:
 - added dynamic link speed recalculation for a bonding

Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Signed-off-by: Declan Doherty <declan.doherty@intel.com>
---
 drivers/net/bonding/rte_eth_bond_api.c     | 28 +++++++++++++++++-----------
 drivers/net/bonding/rte_eth_bond_private.h |  2 +-
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 82c4499..824ab4f 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2010-2017 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -63,13 +63,18 @@ valid_bonded_port_id(uint8_t port_id)
 }
 
 int
-valid_slave_port_id(uint8_t port_id)
+valid_slave_port_id(uint8_t port_id, uint8_t mode)
 {
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -1);
 
 	/* Verify that port_id refers to a non bonded port */
-	if (check_for_bonded_ethdev(&rte_eth_devices[port_id]) == 0)
+	if (check_for_bonded_ethdev(&rte_eth_devices[port_id]) == 0 &&
+			mode == BONDING_MODE_8023AD) {
+		RTE_BOND_LOG(ERR, "Cannot add slave to bonded device in 802.3ad"
+				" mode as slave is also a bonded device, only "
+				"physical devices can be support in this mode.");
 		return -1;
+	}
 
 	return 0;
 }
@@ -235,12 +240,12 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 	struct rte_eth_link link_props;
 	struct rte_eth_dev_info dev_info;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
-		return -1;
-
 	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
 	internals = bonded_eth_dev->data->dev_private;
 
+	if (valid_slave_port_id(slave_port_id, internals->mode) != 0)
+		return -1;
+
 	slave_eth_dev = &rte_eth_devices[slave_port_id];
 	if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_BONDED_SLAVE) {
 		RTE_BOND_LOG(ERR, "Slave device is already a slave of a bonded device");
@@ -389,12 +394,12 @@ __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 	struct rte_eth_dev *slave_eth_dev;
 	int i, slave_idx;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
-		return -1;
-
 	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
 	internals = bonded_eth_dev->data->dev_private;
 
+	if (valid_slave_port_id(slave_port_id, internals->mode) < 0)
+		return -1;
+
 	/* first remove from active slave list */
 	slave_idx = find_slave_by_id(internals->active_slaves,
 		internals->active_slave_count, slave_port_id);
@@ -509,13 +514,14 @@ rte_eth_bond_primary_set(uint8_t bonded_port_id, uint8_t slave_port_id)
 {
 	struct bond_dev_private *internals;
 
+	internals =  rte_eth_devices[bonded_port_id].data->dev_private;
+
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
+	if (valid_slave_port_id(slave_port_id, internals->mode) != 0)
 		return -1;
 
-	internals =  rte_eth_devices[bonded_port_id].data->dev_private;
 
 	internals->user_defined_primary_port = 1;
 	internals->primary_port = slave_port_id;
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 7295192..8fbf4bf 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -204,7 +204,7 @@ int
 valid_bonded_port_id(uint8_t port_id);
 
 int
-valid_slave_port_id(uint8_t port_id);
+valid_slave_port_id(uint8_t port_id, uint8_t mode);
 
 void
 deactivate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id);
-- 
2.9.4

[PATCH v4 1/2] net/bond: fixes for link properties management

[Declan Doherty]

From: Tomasz Kulasek <tomaszx.kulasek@intel.com>

This patch fixes the management of link properties in the bonded device.

In all mode except mode 4 a bonded device link will default to reporting
the link as full duplex and auto-neg. The link speed for a bond port is
calculated on it's active slaves and the particular mode it is running
in. The bonding link speed is reported based on the transmit link as in
some modes link speed between egress/ingress is not symmetrical.

- round-robin, balance, 802.3ad, TLB and ALB modes all report the link
  speed as the sum of the speed of each active slave.
- active backup link speed is reported as the speed of the current
  primary slave
- broadcast is reported as the minimum of value of the active slaves
  link speeds.

In mode 4 (link aggregation 802.3ad) the properties of the first slave
added to the bonded device are slave and subsequent slaves are verified
to have the same properties.

Finally in the bond_ethdev_lsc_event_callback function the link
properties of the device are updated after any change to the number of
active slaves.

Signed-off-by: Declan Doherty <declan.doherty@intel.com>
---
v4: rebased
v3: reworked link management changes and split into separate patch

 drivers/net/bonding/rte_eth_bond_api.c     |   6 +-
 drivers/net/bonding/rte_eth_bond_pmd.c     | 170 ++++++++++++++++++-----------
 drivers/net/bonding/rte_eth_bond_private.h |   8 +-
 3 files changed, 109 insertions(+), 75 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 55d71d9..82c4499 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -297,6 +297,9 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 		internals->tx_offload_capa &= dev_info.tx_offload_capa;
 		internals->flow_type_rss_offloads &= dev_info.flow_type_rss_offloads;
 
+		link_properties_valid(bonded_eth_dev,
+				&slave_eth_dev->data->dev_link);
+
 		/* RETA size is GCD of all slaves RETA sizes, so, if all sizes will be
 		 * the power of 2, the lower one is GCD
 		 */
@@ -439,9 +442,6 @@ __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 	}
 
 	if (internals->active_slave_count < 1) {
-		/* reset device link properties as no slaves are active */
-		link_properties_reset(&rte_eth_devices[bonded_port_id]);
-
 		/* if no slaves are any longer attached to bonded device and MAC is not
 		 * user defined then clear MAC of bonded device as it will be reset
 		 * when a new slave is added */
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 60569c6..a836631 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1386,39 +1386,44 @@ bond_ethdev_tx_burst_broadcast(void *queue, struct rte_mbuf **bufs,
 }
 
 void
-link_properties_set(struct rte_eth_dev *bonded_eth_dev,
-		struct rte_eth_link *slave_dev_link)
+link_properties_set(struct rte_eth_dev *ethdev, struct rte_eth_link *slave_link)
 {
-	struct rte_eth_link *bonded_dev_link = &bonded_eth_dev->data->dev_link;
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct bond_dev_private *bond_ctx = ethdev->data->dev_private;
 
-	if (slave_dev_link->link_status &&
-		bonded_eth_dev->data->dev_started) {
-		bonded_dev_link->link_duplex = slave_dev_link->link_duplex;
-		bonded_dev_link->link_speed = slave_dev_link->link_speed;
+	if (bond_ctx->mode == BONDING_MODE_8023AD) {
+		/**
+		 * If in mode 4 then save the link properties of the first
+		 * slave, all subsequent slaves must match these properties
+		 */
+		struct rte_eth_link *bond_link = &bond_ctx->mode4.slave_link;
 
-		internals->link_props_set = 1;
+		bond_link->link_autoneg = slave_link->link_autoneg;
+		bond_link->link_duplex = slave_link->link_duplex;
+		bond_link->link_speed = slave_link->link_speed;
+	} else {
+		/**
+		 * In any other mode the link properties are set to default
+		 * values of AUTONEG/DUPLEX
+		 */
+		ethdev->data->dev_link.link_autoneg = ETH_LINK_AUTONEG;
+		ethdev->data->dev_link.link_duplex = ETH_LINK_FULL_DUPLEX;
 	}
 }
 
-void
-link_properties_reset(struct rte_eth_dev *bonded_eth_dev)
+int
+link_properties_valid(struct rte_eth_dev *ethdev,
+		struct rte_eth_link *slave_link)
 {
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct bond_dev_private *bond_ctx = ethdev->data->dev_private;
 
-	memset(&(bonded_eth_dev->data->dev_link), 0,
-			sizeof(bonded_eth_dev->data->dev_link));
+	if (bond_ctx->mode == BONDING_MODE_8023AD) {
+		struct rte_eth_link *bond_link = &bond_ctx->mode4.slave_link;
 
-	internals->link_props_set = 0;
-}
-
-int
-link_properties_valid(struct rte_eth_link *bonded_dev_link,
-		struct rte_eth_link *slave_dev_link)
-{
-	if (bonded_dev_link->link_duplex != slave_dev_link->link_duplex ||
-		bonded_dev_link->link_speed !=  slave_dev_link->link_speed)
-		return -1;
+		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)
+			return -1;
+	}
 
 	return 0;
 }
@@ -2270,36 +2275,90 @@ bond_ethdev_slave_link_status_change_monitor(void *cb_arg)
 }
 
 static int
-bond_ethdev_link_update(struct rte_eth_dev *bonded_eth_dev,
-		int wait_to_complete)
+bond_ethdev_link_update(struct rte_eth_dev *ethdev, int wait_to_complete)
 {
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	void (*link_update)(uint8_t port_id, struct rte_eth_link *eth_link);
+
+	struct bond_dev_private *bond_ctx;
+	struct rte_eth_link slave_link;
+
+	uint32_t idx;
+
+	bond_ctx = ethdev->data->dev_private;
 
-	if (!bonded_eth_dev->data->dev_started ||
-		internals->active_slave_count == 0) {
-		bonded_eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
+	ethdev->data->dev_link.link_speed = ETH_SPEED_NUM_NONE;
+
+	if (ethdev->data->dev_started == 0 ||
+			bond_ctx->active_slave_count == 0) {
+		ethdev->data->dev_link.link_status = ETH_LINK_DOWN;
 		return 0;
-	} else {
-		struct rte_eth_dev *slave_eth_dev;
-		int i, link_up = 0;
+	}
 
-		for (i = 0; i < internals->active_slave_count; i++) {
-			slave_eth_dev = &rte_eth_devices[internals->active_slaves[i]];
+	ethdev->data->dev_link.link_status = ETH_LINK_UP;
 
-			(*slave_eth_dev->dev_ops->link_update)(slave_eth_dev,
-					wait_to_complete);
-			if (slave_eth_dev->data->dev_link.link_status == ETH_LINK_UP) {
-				link_up = 1;
-				break;
-			}
+	if (wait_to_complete)
+		link_update = rte_eth_link_get;
+	else
+		link_update = rte_eth_link_get_nowait;
+
+	switch (bond_ctx->mode) {
+	case BONDING_MODE_BROADCAST:
+		/**
+		 * Setting link speed to UINT32_MAX to ensure we pick up the
+		 * value of the first active slave
+		 */
+		ethdev->data->dev_link.link_speed = UINT32_MAX;
+
+		/**
+		 * link speed is minimum value of all the slaves link speed as
+		 * packet loss will occur on this slave if transmission at rates
+		 * greater than this are attempted
+		 */
+		for (idx = 1; idx < bond_ctx->active_slave_count; idx++) {
+			link_update(bond_ctx->active_slaves[0],	&slave_link);
+
+			if (slave_link.link_speed <
+					ethdev->data->dev_link.link_speed)
+				ethdev->data->dev_link.link_speed =
+						slave_link.link_speed;
 		}
+		break;
+	case BONDING_MODE_ACTIVE_BACKUP:
+		/* Current primary slave */
+		link_update(bond_ctx->current_primary_port, &slave_link);
+
+		ethdev->data->dev_link.link_speed = slave_link.link_speed;
+		break;
+	case BONDING_MODE_8023AD:
+		ethdev->data->dev_link.link_autoneg =
+				bond_ctx->mode4.slave_link.link_autoneg;
+		ethdev->data->dev_link.link_duplex =
+				bond_ctx->mode4.slave_link.link_duplex;
+		/* fall through to update link speed */
+	case BONDING_MODE_ROUND_ROBIN:
+	case BONDING_MODE_BALANCE:
+	case BONDING_MODE_TLB:
+	case BONDING_MODE_ALB:
+	default:
+		/**
+		 * In theses mode the maximum theoretical link speed is the sum
+		 * of all the slaves
+		 */
+		ethdev->data->dev_link.link_speed = ETH_SPEED_NUM_NONE;
 
-		bonded_eth_dev->data->dev_link.link_status = link_up;
+		for (idx = 0; idx < bond_ctx->active_slave_count; idx++) {
+			link_update(bond_ctx->active_slaves[idx], &slave_link);
+
+			ethdev->data->dev_link.link_speed +=
+					slave_link.link_speed;
+		}
 	}
 
+
 	return 0;
 }
 
+
 static void
 bond_ethdev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
@@ -2462,20 +2521,6 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 			lsc_flag = 1;
 
 			mac_address_slaves_update(bonded_eth_dev);
-
-			/* Inherit eth dev link properties from first active slave */
-			link_properties_set(bonded_eth_dev,
-					&(slave_eth_dev->data->dev_link));
-		} else {
-			if (link_properties_valid(
-				&bonded_eth_dev->data->dev_link, &link) != 0) {
-				slave_eth_dev->data->dev_flags &=
-					(~RTE_ETH_DEV_BONDED_SLAVE);
-				RTE_LOG(ERR, PMD,
-					"port %u invalid speed/duplex\n",
-					port_id);
-				return rc;
-			}
 		}
 
 		activate_slave(bonded_eth_dev, port_id);
@@ -2491,15 +2536,6 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		/* Remove from active slave list */
 		deactivate_slave(bonded_eth_dev, port_id);
 
-		/* No active slaves, change link status to down and reset other
-		 * link properties */
-		if (internals->active_slave_count < 1) {
-			lsc_flag = 1;
-			bonded_eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
-
-			link_properties_reset(bonded_eth_dev);
-		}
-
 		/* Update primary id, take first active slave from list or if none
 		 * available set to -1 */
 		if (port_id == internals->current_primary_port) {
@@ -2511,6 +2547,9 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		}
 	}
 
+	/* update bonded device link after any change to active slaves */
+	bond_ethdev_link_update(bonded_eth_dev, 0);
+
 	if (lsc_flag) {
 		/* Cancel any possible outstanding interrupts if delays are enabled */
 		if (internals->link_up_delay_ms > 0 ||
@@ -2714,7 +2753,6 @@ bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
 	internals->balance_xmit_policy = BALANCE_XMIT_POLICY_LAYER2;
 	internals->xmit_hash = xmit_l2_hash;
 	internals->user_defined_mac = 0;
-	internals->link_props_set = 0;
 
 	internals->link_status_polling_enabled = 0;
 
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 53470f6..7295192 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -132,8 +132,7 @@ struct bond_dev_private {
 	/**< Flag for whether MAC address is user defined or not */
 	uint8_t promiscuous_en;
 	/**< Enabled/disable promiscuous mode on bonding device */
-	uint8_t link_props_set;
-	/**< flag to denote if the link properties are set */
+
 
 	uint8_t link_status_polling_enabled;
 	uint32_t link_status_polling_interval_ms;
@@ -216,11 +215,8 @@ activate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id);
 void
 link_properties_set(struct rte_eth_dev *bonded_eth_dev,
 		struct rte_eth_link *slave_dev_link);
-void
-link_properties_reset(struct rte_eth_dev *bonded_eth_dev);
-
 int
-link_properties_valid(struct rte_eth_link *bonded_dev_link,
+link_properties_valid(struct rte_eth_dev *bonded_eth_dev,
 		struct rte_eth_link *slave_dev_link);
 
 int
-- 
2.9.4

[PATCH v4 2/2] net/bond: allow slaves to also be bonded devices

[Declan Doherty]

From: Tomasz Kulasek <tomaszx.kulasek@intel.com>

This patch removes restrictions in bonded device library which prevent a
bonded device to be added to another bonded device with the limitation
that 802.3ad mode is not supported if one or more slaves is also a
bonded device,

Signed-off-by: Declan Doherty <declan.doherty@intel.com>
---
v4:
 - rebase
 
v3:
 - reworked link management changes and split into separate patch

v2 changes:

 - added dynamic link speed recalculation for a bonding

drivers/net/bonding/rte_eth_bond_api.c     | 28 +++++++++++++++++-----------
 drivers/net/bonding/rte_eth_bond_private.h |  2 +-
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 82c4499..824ab4f 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2010-2017 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -63,13 +63,18 @@ valid_bonded_port_id(uint8_t port_id)
 }
 
 int
-valid_slave_port_id(uint8_t port_id)
+valid_slave_port_id(uint8_t port_id, uint8_t mode)
 {
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -1);
 
 	/* Verify that port_id refers to a non bonded port */
-	if (check_for_bonded_ethdev(&rte_eth_devices[port_id]) == 0)
+	if (check_for_bonded_ethdev(&rte_eth_devices[port_id]) == 0 &&
+			mode == BONDING_MODE_8023AD) {
+		RTE_BOND_LOG(ERR, "Cannot add slave to bonded device in 802.3ad"
+				" mode as slave is also a bonded device, only "
+				"physical devices can be support in this mode.");
 		return -1;
+	}
 
 	return 0;
 }
@@ -235,12 +240,12 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 	struct rte_eth_link link_props;
 	struct rte_eth_dev_info dev_info;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
-		return -1;
-
 	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
 	internals = bonded_eth_dev->data->dev_private;
 
+	if (valid_slave_port_id(slave_port_id, internals->mode) != 0)
+		return -1;
+
 	slave_eth_dev = &rte_eth_devices[slave_port_id];
 	if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_BONDED_SLAVE) {
 		RTE_BOND_LOG(ERR, "Slave device is already a slave of a bonded device");
@@ -389,12 +394,12 @@ __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 	struct rte_eth_dev *slave_eth_dev;
 	int i, slave_idx;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
-		return -1;
-
 	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
 	internals = bonded_eth_dev->data->dev_private;
 
+	if (valid_slave_port_id(slave_port_id, internals->mode) < 0)
+		return -1;
+
 	/* first remove from active slave list */
 	slave_idx = find_slave_by_id(internals->active_slaves,
 		internals->active_slave_count, slave_port_id);
@@ -509,13 +514,14 @@ rte_eth_bond_primary_set(uint8_t bonded_port_id, uint8_t slave_port_id)
 {
 	struct bond_dev_private *internals;
 
+	internals =  rte_eth_devices[bonded_port_id].data->dev_private;
+
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
+	if (valid_slave_port_id(slave_port_id, internals->mode) != 0)
 		return -1;
 
-	internals =  rte_eth_devices[bonded_port_id].data->dev_private;
 
 	internals->user_defined_primary_port = 1;
 	internals->primary_port = slave_port_id;
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 7295192..8fbf4bf 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -204,7 +204,7 @@ int
 valid_bonded_port_id(uint8_t port_id);
 
 int
-valid_slave_port_id(uint8_t port_id);
+valid_slave_port_id(uint8_t port_id, uint8_t mode);
 
 void
 deactivate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id);
-- 
2.9.4

[PATCH v5] net/bond: fixes for link properties management

[Declan Doherty]

From: Tomasz Kulasek <tomaszx.kulasek@intel.com>

This patch fixes the management of link properties in the bonded device.

In all mode except mode 4 a bonded device link will default to reporting
the link as full duplex and auto-neg. The link speed for a bond port is
calculated on it's active slaves and the particular mode it is running
in. The bonding link speed is reported based on the transmit link as in
some modes link speed between egress/ingress is not symmetrical.

- round-robin, balance, 802.3ad, TLB and ALB modes all report the link
  speed as the sum of the speed of each active slave.
- active backup link speed is reported as the speed of the current
  primary slave
- broadcast is reported as the minimum of value of the active slaves
  link speeds.

In mode 4 (link aggregation 802.3ad) the properties of the first slave
added to the bonded device are slave and subsequent slaves are verified
to have the same properties.

Finally in the bond_ethdev_lsc_event_callback function the link
properties of the device are updated after any change to the number of
active slaves.

Signed-off-by: Declan Doherty <declan.doherty@intel.com>
---
v5:
- add back in change to drivers/net/bonding/rte_eth_bond_8023ad_private.h lost
in rebase :)
v4:
- rebased
v3:
- reworked link management changes and split into separate patch

 drivers/net/bonding/rte_eth_bond_8023ad_private.h |   3 +
 drivers/net/bonding/rte_eth_bond_api.c            |   6 +-
 drivers/net/bonding/rte_eth_bond_pmd.c            | 170 +++++++++++++---------
 drivers/net/bonding/rte_eth_bond_private.h        |   8 +-
 4 files changed, 112 insertions(+), 75 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad_private.h b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
index c16dba8..802551d 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad_private.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
@@ -180,6 +180,9 @@ struct mode8023ad_private {
 	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
 	uint8_t external_sm;
 
+	struct rte_eth_link slave_link;
+	/***< slave link properties */
+
 	/**
 	 * Configuration of dedicated hardware queues for control plane
 	 * traffic
diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 55d71d9..82c4499 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -297,6 +297,9 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 		internals->tx_offload_capa &= dev_info.tx_offload_capa;
 		internals->flow_type_rss_offloads &= dev_info.flow_type_rss_offloads;
 
+		link_properties_valid(bonded_eth_dev,
+				&slave_eth_dev->data->dev_link);
+
 		/* RETA size is GCD of all slaves RETA sizes, so, if all sizes will be
 		 * the power of 2, the lower one is GCD
 		 */
@@ -439,9 +442,6 @@ __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 	}
 
 	if (internals->active_slave_count < 1) {
-		/* reset device link properties as no slaves are active */
-		link_properties_reset(&rte_eth_devices[bonded_port_id]);
-
 		/* if no slaves are any longer attached to bonded device and MAC is not
 		 * user defined then clear MAC of bonded device as it will be reset
 		 * when a new slave is added */
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 60569c6..a836631 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1386,39 +1386,44 @@ bond_ethdev_tx_burst_broadcast(void *queue, struct rte_mbuf **bufs,
 }
 
 void
-link_properties_set(struct rte_eth_dev *bonded_eth_dev,
-		struct rte_eth_link *slave_dev_link)
+link_properties_set(struct rte_eth_dev *ethdev, struct rte_eth_link *slave_link)
 {
-	struct rte_eth_link *bonded_dev_link = &bonded_eth_dev->data->dev_link;
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct bond_dev_private *bond_ctx = ethdev->data->dev_private;
 
-	if (slave_dev_link->link_status &&
-		bonded_eth_dev->data->dev_started) {
-		bonded_dev_link->link_duplex = slave_dev_link->link_duplex;
-		bonded_dev_link->link_speed = slave_dev_link->link_speed;
+	if (bond_ctx->mode == BONDING_MODE_8023AD) {
+		/**
+		 * If in mode 4 then save the link properties of the first
+		 * slave, all subsequent slaves must match these properties
+		 */
+		struct rte_eth_link *bond_link = &bond_ctx->mode4.slave_link;
 
-		internals->link_props_set = 1;
+		bond_link->link_autoneg = slave_link->link_autoneg;
+		bond_link->link_duplex = slave_link->link_duplex;
+		bond_link->link_speed = slave_link->link_speed;
+	} else {
+		/**
+		 * In any other mode the link properties are set to default
+		 * values of AUTONEG/DUPLEX
+		 */
+		ethdev->data->dev_link.link_autoneg = ETH_LINK_AUTONEG;
+		ethdev->data->dev_link.link_duplex = ETH_LINK_FULL_DUPLEX;
 	}
 }
 
-void
-link_properties_reset(struct rte_eth_dev *bonded_eth_dev)
+int
+link_properties_valid(struct rte_eth_dev *ethdev,
+		struct rte_eth_link *slave_link)
 {
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct bond_dev_private *bond_ctx = ethdev->data->dev_private;
 
-	memset(&(bonded_eth_dev->data->dev_link), 0,
-			sizeof(bonded_eth_dev->data->dev_link));
+	if (bond_ctx->mode == BONDING_MODE_8023AD) {
+		struct rte_eth_link *bond_link = &bond_ctx->mode4.slave_link;
 
-	internals->link_props_set = 0;
-}
-
-int
-link_properties_valid(struct rte_eth_link *bonded_dev_link,
-		struct rte_eth_link *slave_dev_link)
-{
-	if (bonded_dev_link->link_duplex != slave_dev_link->link_duplex ||
-		bonded_dev_link->link_speed !=  slave_dev_link->link_speed)
-		return -1;
+		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)
+			return -1;
+	}
 
 	return 0;
 }
@@ -2270,36 +2275,90 @@ bond_ethdev_slave_link_status_change_monitor(void *cb_arg)
 }
 
 static int
-bond_ethdev_link_update(struct rte_eth_dev *bonded_eth_dev,
-		int wait_to_complete)
+bond_ethdev_link_update(struct rte_eth_dev *ethdev, int wait_to_complete)
 {
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	void (*link_update)(uint8_t port_id, struct rte_eth_link *eth_link);
+
+	struct bond_dev_private *bond_ctx;
+	struct rte_eth_link slave_link;
+
+	uint32_t idx;
+
+	bond_ctx = ethdev->data->dev_private;
 
-	if (!bonded_eth_dev->data->dev_started ||
-		internals->active_slave_count == 0) {
-		bonded_eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
+	ethdev->data->dev_link.link_speed = ETH_SPEED_NUM_NONE;
+
+	if (ethdev->data->dev_started == 0 ||
+			bond_ctx->active_slave_count == 0) {
+		ethdev->data->dev_link.link_status = ETH_LINK_DOWN;
 		return 0;
-	} else {
-		struct rte_eth_dev *slave_eth_dev;
-		int i, link_up = 0;
+	}
 
-		for (i = 0; i < internals->active_slave_count; i++) {
-			slave_eth_dev = &rte_eth_devices[internals->active_slaves[i]];
+	ethdev->data->dev_link.link_status = ETH_LINK_UP;
 
-			(*slave_eth_dev->dev_ops->link_update)(slave_eth_dev,
-					wait_to_complete);
-			if (slave_eth_dev->data->dev_link.link_status == ETH_LINK_UP) {
-				link_up = 1;
-				break;
-			}
+	if (wait_to_complete)
+		link_update = rte_eth_link_get;
+	else
+		link_update = rte_eth_link_get_nowait;
+
+	switch (bond_ctx->mode) {
+	case BONDING_MODE_BROADCAST:
+		/**
+		 * Setting link speed to UINT32_MAX to ensure we pick up the
+		 * value of the first active slave
+		 */
+		ethdev->data->dev_link.link_speed = UINT32_MAX;
+
+		/**
+		 * link speed is minimum value of all the slaves link speed as
+		 * packet loss will occur on this slave if transmission at rates
+		 * greater than this are attempted
+		 */
+		for (idx = 1; idx < bond_ctx->active_slave_count; idx++) {
+			link_update(bond_ctx->active_slaves[0],	&slave_link);
+
+			if (slave_link.link_speed <
+					ethdev->data->dev_link.link_speed)
+				ethdev->data->dev_link.link_speed =
+						slave_link.link_speed;
 		}
+		break;
+	case BONDING_MODE_ACTIVE_BACKUP:
+		/* Current primary slave */
+		link_update(bond_ctx->current_primary_port, &slave_link);
+
+		ethdev->data->dev_link.link_speed = slave_link.link_speed;
+		break;
+	case BONDING_MODE_8023AD:
+		ethdev->data->dev_link.link_autoneg =
+				bond_ctx->mode4.slave_link.link_autoneg;
+		ethdev->data->dev_link.link_duplex =
+				bond_ctx->mode4.slave_link.link_duplex;
+		/* fall through to update link speed */
+	case BONDING_MODE_ROUND_ROBIN:
+	case BONDING_MODE_BALANCE:
+	case BONDING_MODE_TLB:
+	case BONDING_MODE_ALB:
+	default:
+		/**
+		 * In theses mode the maximum theoretical link speed is the sum
+		 * of all the slaves
+		 */
+		ethdev->data->dev_link.link_speed = ETH_SPEED_NUM_NONE;
 
-		bonded_eth_dev->data->dev_link.link_status = link_up;
+		for (idx = 0; idx < bond_ctx->active_slave_count; idx++) {
+			link_update(bond_ctx->active_slaves[idx], &slave_link);
+
+			ethdev->data->dev_link.link_speed +=
+					slave_link.link_speed;
+		}
 	}
 
+
 	return 0;
 }
 
+
 static void
 bond_ethdev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
@@ -2462,20 +2521,6 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 			lsc_flag = 1;
 
 			mac_address_slaves_update(bonded_eth_dev);
-
-			/* Inherit eth dev link properties from first active slave */
-			link_properties_set(bonded_eth_dev,
-					&(slave_eth_dev->data->dev_link));
-		} else {
-			if (link_properties_valid(
-				&bonded_eth_dev->data->dev_link, &link) != 0) {
-				slave_eth_dev->data->dev_flags &=
-					(~RTE_ETH_DEV_BONDED_SLAVE);
-				RTE_LOG(ERR, PMD,
-					"port %u invalid speed/duplex\n",
-					port_id);
-				return rc;
-			}
 		}
 
 		activate_slave(bonded_eth_dev, port_id);
@@ -2491,15 +2536,6 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		/* Remove from active slave list */
 		deactivate_slave(bonded_eth_dev, port_id);
 
-		/* No active slaves, change link status to down and reset other
-		 * link properties */
-		if (internals->active_slave_count < 1) {
-			lsc_flag = 1;
-			bonded_eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
-
-			link_properties_reset(bonded_eth_dev);
-		}
-
 		/* Update primary id, take first active slave from list or if none
 		 * available set to -1 */
 		if (port_id == internals->current_primary_port) {
@@ -2511,6 +2547,9 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		}
 	}
 
+	/* update bonded device link after any change to active slaves */
+	bond_ethdev_link_update(bonded_eth_dev, 0);
+
 	if (lsc_flag) {
 		/* Cancel any possible outstanding interrupts if delays are enabled */
 		if (internals->link_up_delay_ms > 0 ||
@@ -2714,7 +2753,6 @@ bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
 	internals->balance_xmit_policy = BALANCE_XMIT_POLICY_LAYER2;
 	internals->xmit_hash = xmit_l2_hash;
 	internals->user_defined_mac = 0;
-	internals->link_props_set = 0;
 
 	internals->link_status_polling_enabled = 0;
 
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 53470f6..7295192 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -132,8 +132,7 @@ struct bond_dev_private {
 	/**< Flag for whether MAC address is user defined or not */
 	uint8_t promiscuous_en;
 	/**< Enabled/disable promiscuous mode on bonding device */
-	uint8_t link_props_set;
-	/**< flag to denote if the link properties are set */
+
 
 	uint8_t link_status_polling_enabled;
 	uint32_t link_status_polling_interval_ms;
@@ -216,11 +215,8 @@ activate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id);
 void
 link_properties_set(struct rte_eth_dev *bonded_eth_dev,
 		struct rte_eth_link *slave_dev_link);
-void
-link_properties_reset(struct rte_eth_dev *bonded_eth_dev);
-
 int
-link_properties_valid(struct rte_eth_link *bonded_dev_link,
+link_properties_valid(struct rte_eth_dev *bonded_eth_dev,
 		struct rte_eth_link *slave_dev_link);
 
 int
-- 
2.9.4

Re: [PATCH v5] net/bond: fixes for link properties management

[Ferruh Yigit]

On 7/5/2017 6:09 PM, Declan Doherty wrote:
> From: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> 
> This patch fixes the management of link properties in the bonded device.
> 
> In all mode except mode 4 a bonded device link will default to reporting
> the link as full duplex and auto-neg. The link speed for a bond port is
> calculated on it's active slaves and the particular mode it is running
> in. The bonding link speed is reported based on the transmit link as in
> some modes link speed between egress/ingress is not symmetrical.
> 
> - round-robin, balance, 802.3ad, TLB and ALB modes all report the link
>   speed as the sum of the speed of each active slave.
> - active backup link speed is reported as the speed of the current
>   primary slave
> - broadcast is reported as the minimum of value of the active slaves
>   link speeds.
> 
> In mode 4 (link aggregation 802.3ad) the properties of the first slave
> added to the bonded device are slave and subsequent slaves are verified
> to have the same properties.
> 
> Finally in the bond_ethdev_lsc_event_callback function the link
> properties of the device are updated after any change to the number of
> active slaves.
> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>

Gcc is throwing another warning [1].

And can you please send next version as full patchset, for the sake of
the traceability of merged patches in the future.


[1]
.../dpdk/drivers/net/bonding/rte_eth_bond_pmd.c: In function
‘bond_ethdev_lsc_event_callback’:
.../dpdk/drivers/net/bonding/rte_eth_bond_pmd.c:2472:39: error: variable
‘slave_eth_dev’ set but not used [-Werror=unused-but-set-variable]
  struct rte_eth_dev *bonded_eth_dev, *slave_eth_dev;
                                       ^~~~~~~~~~~~~

[PATCH v6 1/2] net/bond: fixes for link properties management

[Declan Doherty]

From: Tomasz Kulasek <tomaszx.kulasek@intel.com>

This patch fixes the management of link properties in the bonded device.

In all mode except mode 4 a bonded device link will default to reporting
the link as full duplex and auto-neg. The link speed for a bond port is
calculated on it's active slaves and the particular mode it is running
in. The bonding link speed is reported based on the transmit link as in
some modes link speed between egress/ingress is not symmetrical.

- round-robin, balance, 802.3ad, TLB and ALB modes all report the link
  speed as the sum of the speed of each active slave.
- active backup link speed is reported as the speed of the current
  primary slave
- broadcast is reported as the minimum of value of the active slaves
  link speeds.

In mode 4 (link aggregation 802.3ad) the properties of the first slave
added to the bonded device are slave and subsequent slaves are verified
to have the same properties.

Finally in the bond_ethdev_lsc_event_callback function the link
properties of the device are updated after any change to the number of
active slaves.

Signed-off-by: Declan Doherty <declan.doherty@intel.com>
---
v6:
- fix unused variable warning
v5:
- add back in change to drivers/net/bonding/rte_eth_bond_8023ad_private.h lost
in rebase 
v4:

- rebased
v3:

- reworked link management changes and split into separate patch

 drivers/net/bonding/rte_eth_bond_8023ad_private.h |   3 +
 drivers/net/bonding/rte_eth_bond_api.c            |   6 +-
 drivers/net/bonding/rte_eth_bond_pmd.c            | 176 +++++++++++++---------
 drivers/net/bonding/rte_eth_bond_private.h        |   8 +-
 4 files changed, 116 insertions(+), 77 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad_private.h b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
index c16dba8..802551d 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad_private.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
@@ -180,6 +180,9 @@ struct mode8023ad_private {
 	rte_eth_bond_8023ad_ext_slowrx_fn slowrx_cb;
 	uint8_t external_sm;
 
+	struct rte_eth_link slave_link;
+	/***< slave link properties */
+
 	/**
 	 * Configuration of dedicated hardware queues for control plane
 	 * traffic
diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 55d71d9..82c4499 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -297,6 +297,9 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 		internals->tx_offload_capa &= dev_info.tx_offload_capa;
 		internals->flow_type_rss_offloads &= dev_info.flow_type_rss_offloads;
 
+		link_properties_valid(bonded_eth_dev,
+				&slave_eth_dev->data->dev_link);
+
 		/* RETA size is GCD of all slaves RETA sizes, so, if all sizes will be
 		 * the power of 2, the lower one is GCD
 		 */
@@ -439,9 +442,6 @@ __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 	}
 
 	if (internals->active_slave_count < 1) {
-		/* reset device link properties as no slaves are active */
-		link_properties_reset(&rte_eth_devices[bonded_port_id]);
-
 		/* if no slaves are any longer attached to bonded device and MAC is not
 		 * user defined then clear MAC of bonded device as it will be reset
 		 * when a new slave is added */
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 60569c6..d3cbc9d 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1386,39 +1386,44 @@ bond_ethdev_tx_burst_broadcast(void *queue, struct rte_mbuf **bufs,
 }
 
 void
-link_properties_set(struct rte_eth_dev *bonded_eth_dev,
-		struct rte_eth_link *slave_dev_link)
+link_properties_set(struct rte_eth_dev *ethdev, struct rte_eth_link *slave_link)
 {
-	struct rte_eth_link *bonded_dev_link = &bonded_eth_dev->data->dev_link;
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct bond_dev_private *bond_ctx = ethdev->data->dev_private;
 
-	if (slave_dev_link->link_status &&
-		bonded_eth_dev->data->dev_started) {
-		bonded_dev_link->link_duplex = slave_dev_link->link_duplex;
-		bonded_dev_link->link_speed = slave_dev_link->link_speed;
+	if (bond_ctx->mode == BONDING_MODE_8023AD) {
+		/**
+		 * If in mode 4 then save the link properties of the first
+		 * slave, all subsequent slaves must match these properties
+		 */
+		struct rte_eth_link *bond_link = &bond_ctx->mode4.slave_link;
 
-		internals->link_props_set = 1;
+		bond_link->link_autoneg = slave_link->link_autoneg;
+		bond_link->link_duplex = slave_link->link_duplex;
+		bond_link->link_speed = slave_link->link_speed;
+	} else {
+		/**
+		 * In any other mode the link properties are set to default
+		 * values of AUTONEG/DUPLEX
+		 */
+		ethdev->data->dev_link.link_autoneg = ETH_LINK_AUTONEG;
+		ethdev->data->dev_link.link_duplex = ETH_LINK_FULL_DUPLEX;
 	}
 }
 
-void
-link_properties_reset(struct rte_eth_dev *bonded_eth_dev)
+int
+link_properties_valid(struct rte_eth_dev *ethdev,
+		struct rte_eth_link *slave_link)
 {
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	struct bond_dev_private *bond_ctx = ethdev->data->dev_private;
 
-	memset(&(bonded_eth_dev->data->dev_link), 0,
-			sizeof(bonded_eth_dev->data->dev_link));
+	if (bond_ctx->mode == BONDING_MODE_8023AD) {
+		struct rte_eth_link *bond_link = &bond_ctx->mode4.slave_link;
 
-	internals->link_props_set = 0;
-}
-
-int
-link_properties_valid(struct rte_eth_link *bonded_dev_link,
-		struct rte_eth_link *slave_dev_link)
-{
-	if (bonded_dev_link->link_duplex != slave_dev_link->link_duplex ||
-		bonded_dev_link->link_speed !=  slave_dev_link->link_speed)
-		return -1;
+		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)
+			return -1;
+	}
 
 	return 0;
 }
@@ -2270,36 +2275,90 @@ bond_ethdev_slave_link_status_change_monitor(void *cb_arg)
 }
 
 static int
-bond_ethdev_link_update(struct rte_eth_dev *bonded_eth_dev,
-		int wait_to_complete)
+bond_ethdev_link_update(struct rte_eth_dev *ethdev, int wait_to_complete)
 {
-	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
+	void (*link_update)(uint8_t port_id, struct rte_eth_link *eth_link);
 
-	if (!bonded_eth_dev->data->dev_started ||
-		internals->active_slave_count == 0) {
-		bonded_eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
+	struct bond_dev_private *bond_ctx;
+	struct rte_eth_link slave_link;
+
+	uint32_t idx;
+
+	bond_ctx = ethdev->data->dev_private;
+
+	ethdev->data->dev_link.link_speed = ETH_SPEED_NUM_NONE;
+
+	if (ethdev->data->dev_started == 0 ||
+			bond_ctx->active_slave_count == 0) {
+		ethdev->data->dev_link.link_status = ETH_LINK_DOWN;
 		return 0;
-	} else {
-		struct rte_eth_dev *slave_eth_dev;
-		int i, link_up = 0;
+	}
 
-		for (i = 0; i < internals->active_slave_count; i++) {
-			slave_eth_dev = &rte_eth_devices[internals->active_slaves[i]];
+	ethdev->data->dev_link.link_status = ETH_LINK_UP;
 
-			(*slave_eth_dev->dev_ops->link_update)(slave_eth_dev,
-					wait_to_complete);
-			if (slave_eth_dev->data->dev_link.link_status == ETH_LINK_UP) {
-				link_up = 1;
-				break;
-			}
+	if (wait_to_complete)
+		link_update = rte_eth_link_get;
+	else
+		link_update = rte_eth_link_get_nowait;
+
+	switch (bond_ctx->mode) {
+	case BONDING_MODE_BROADCAST:
+		/**
+		 * Setting link speed to UINT32_MAX to ensure we pick up the
+		 * value of the first active slave
+		 */
+		ethdev->data->dev_link.link_speed = UINT32_MAX;
+
+		/**
+		 * link speed is minimum value of all the slaves link speed as
+		 * packet loss will occur on this slave if transmission at rates
+		 * greater than this are attempted
+		 */
+		for (idx = 1; idx < bond_ctx->active_slave_count; idx++) {
+			link_update(bond_ctx->active_slaves[0],	&slave_link);
+
+			if (slave_link.link_speed <
+					ethdev->data->dev_link.link_speed)
+				ethdev->data->dev_link.link_speed =
+						slave_link.link_speed;
 		}
+		break;
+	case BONDING_MODE_ACTIVE_BACKUP:
+		/* Current primary slave */
+		link_update(bond_ctx->current_primary_port, &slave_link);
+
+		ethdev->data->dev_link.link_speed = slave_link.link_speed;
+		break;
+	case BONDING_MODE_8023AD:
+		ethdev->data->dev_link.link_autoneg =
+				bond_ctx->mode4.slave_link.link_autoneg;
+		ethdev->data->dev_link.link_duplex =
+				bond_ctx->mode4.slave_link.link_duplex;
+		/* fall through to update link speed */
+	case BONDING_MODE_ROUND_ROBIN:
+	case BONDING_MODE_BALANCE:
+	case BONDING_MODE_TLB:
+	case BONDING_MODE_ALB:
+	default:
+		/**
+		 * In theses mode the maximum theoretical link speed is the sum
+		 * of all the slaves
+		 */
+		ethdev->data->dev_link.link_speed = ETH_SPEED_NUM_NONE;
+
+		for (idx = 0; idx < bond_ctx->active_slave_count; idx++) {
+			link_update(bond_ctx->active_slaves[idx], &slave_link);
 
-		bonded_eth_dev->data->dev_link.link_status = link_up;
+			ethdev->data->dev_link.link_speed +=
+					slave_link.link_speed;
+		}
 	}
 
+
 	return 0;
 }
 
+
 static void
 bond_ethdev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
@@ -2410,7 +2469,7 @@ int
 bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		void *param, void *ret_param __rte_unused)
 {
-	struct rte_eth_dev *bonded_eth_dev, *slave_eth_dev;
+	struct rte_eth_dev *bonded_eth_dev;
 	struct bond_dev_private *internals;
 	struct rte_eth_link link;
 	int rc = -1;
@@ -2423,7 +2482,6 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		return rc;
 
 	bonded_eth_dev = &rte_eth_devices[*(uint8_t *)param];
-	slave_eth_dev = &rte_eth_devices[port_id];
 
 	if (check_for_bonded_ethdev(bonded_eth_dev))
 		return rc;
@@ -2462,20 +2520,6 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 			lsc_flag = 1;
 
 			mac_address_slaves_update(bonded_eth_dev);
-
-			/* Inherit eth dev link properties from first active slave */
-			link_properties_set(bonded_eth_dev,
-					&(slave_eth_dev->data->dev_link));
-		} else {
-			if (link_properties_valid(
-				&bonded_eth_dev->data->dev_link, &link) != 0) {
-				slave_eth_dev->data->dev_flags &=
-					(~RTE_ETH_DEV_BONDED_SLAVE);
-				RTE_LOG(ERR, PMD,
-					"port %u invalid speed/duplex\n",
-					port_id);
-				return rc;
-			}
 		}
 
 		activate_slave(bonded_eth_dev, port_id);
@@ -2491,15 +2535,6 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		/* Remove from active slave list */
 		deactivate_slave(bonded_eth_dev, port_id);
 
-		/* No active slaves, change link status to down and reset other
-		 * link properties */
-		if (internals->active_slave_count < 1) {
-			lsc_flag = 1;
-			bonded_eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
-
-			link_properties_reset(bonded_eth_dev);
-		}
-
 		/* Update primary id, take first active slave from list or if none
 		 * available set to -1 */
 		if (port_id == internals->current_primary_port) {
@@ -2511,6 +2546,12 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type,
 		}
 	}
 
+	/**
+	 * Update bonded device link properties after any change to active
+	 * slaves
+	 */
+	bond_ethdev_link_update(bonded_eth_dev, 0);
+
 	if (lsc_flag) {
 		/* Cancel any possible outstanding interrupts if delays are enabled */
 		if (internals->link_up_delay_ms > 0 ||
@@ -2714,7 +2755,6 @@ bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
 	internals->balance_xmit_policy = BALANCE_XMIT_POLICY_LAYER2;
 	internals->xmit_hash = xmit_l2_hash;
 	internals->user_defined_mac = 0;
-	internals->link_props_set = 0;
 
 	internals->link_status_polling_enabled = 0;
 
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 53470f6..7295192 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -132,8 +132,7 @@ struct bond_dev_private {
 	/**< Flag for whether MAC address is user defined or not */
 	uint8_t promiscuous_en;
 	/**< Enabled/disable promiscuous mode on bonding device */
-	uint8_t link_props_set;
-	/**< flag to denote if the link properties are set */
+
 
 	uint8_t link_status_polling_enabled;
 	uint32_t link_status_polling_interval_ms;
@@ -216,11 +215,8 @@ activate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id);
 void
 link_properties_set(struct rte_eth_dev *bonded_eth_dev,
 		struct rte_eth_link *slave_dev_link);
-void
-link_properties_reset(struct rte_eth_dev *bonded_eth_dev);
-
 int
-link_properties_valid(struct rte_eth_link *bonded_dev_link,
+link_properties_valid(struct rte_eth_dev *bonded_eth_dev,
 		struct rte_eth_link *slave_dev_link);
 
 int
-- 
2.9.4

[PATCH v6 2/2] net/bond: allow slaves to also be bonded devices

[Declan Doherty]

From: Tomasz Kulasek <tomaszx.kulasek@intel.com>

This patch removes restrictions in bonded device library which prevent a
bonded device to be added to another bonded device with the limitation
that 802.3ad mode is not supported if one or more slaves is also a
bonded device,

Signed-off-by: Declan Doherty <declan.doherty@intel.com>
---
v4:
 - rebase
v3:
 - reworked link management changes and split into separate patch
v2:
- added dynamic link speed recalculation for a bonding

 drivers/net/bonding/rte_eth_bond_api.c     | 28 +++++++++++++++++-----------
 drivers/net/bonding/rte_eth_bond_private.h |  2 +-
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index 82c4499..824ab4f 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2010-2017 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -63,13 +63,18 @@ valid_bonded_port_id(uint8_t port_id)
 }
 
 int
-valid_slave_port_id(uint8_t port_id)
+valid_slave_port_id(uint8_t port_id, uint8_t mode)
 {
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -1);
 
 	/* Verify that port_id refers to a non bonded port */
-	if (check_for_bonded_ethdev(&rte_eth_devices[port_id]) == 0)
+	if (check_for_bonded_ethdev(&rte_eth_devices[port_id]) == 0 &&
+			mode == BONDING_MODE_8023AD) {
+		RTE_BOND_LOG(ERR, "Cannot add slave to bonded device in 802.3ad"
+				" mode as slave is also a bonded device, only "
+				"physical devices can be support in this mode.");
 		return -1;
+	}
 
 	return 0;
 }
@@ -235,12 +240,12 @@ __eth_bond_slave_add_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 	struct rte_eth_link link_props;
 	struct rte_eth_dev_info dev_info;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
-		return -1;
-
 	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
 	internals = bonded_eth_dev->data->dev_private;
 
+	if (valid_slave_port_id(slave_port_id, internals->mode) != 0)
+		return -1;
+
 	slave_eth_dev = &rte_eth_devices[slave_port_id];
 	if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_BONDED_SLAVE) {
 		RTE_BOND_LOG(ERR, "Slave device is already a slave of a bonded device");
@@ -389,12 +394,12 @@ __eth_bond_slave_remove_lock_free(uint8_t bonded_port_id, uint8_t slave_port_id)
 	struct rte_eth_dev *slave_eth_dev;
 	int i, slave_idx;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
-		return -1;
-
 	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
 	internals = bonded_eth_dev->data->dev_private;
 
+	if (valid_slave_port_id(slave_port_id, internals->mode) < 0)
+		return -1;
+
 	/* first remove from active slave list */
 	slave_idx = find_slave_by_id(internals->active_slaves,
 		internals->active_slave_count, slave_port_id);
@@ -509,13 +514,14 @@ rte_eth_bond_primary_set(uint8_t bonded_port_id, uint8_t slave_port_id)
 {
 	struct bond_dev_private *internals;
 
+	internals =  rte_eth_devices[bonded_port_id].data->dev_private;
+
 	if (valid_bonded_port_id(bonded_port_id) != 0)
 		return -1;
 
-	if (valid_slave_port_id(slave_port_id) != 0)
+	if (valid_slave_port_id(slave_port_id, internals->mode) != 0)
 		return -1;
 
-	internals =  rte_eth_devices[bonded_port_id].data->dev_private;
 
 	internals->user_defined_primary_port = 1;
 	internals->primary_port = slave_port_id;
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 7295192..8fbf4bf 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -204,7 +204,7 @@ int
 valid_bonded_port_id(uint8_t port_id);
 
 int
-valid_slave_port_id(uint8_t port_id);
+valid_slave_port_id(uint8_t port_id, uint8_t mode);
 
 void
 deactivate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id);
-- 
2.9.4

Re: [PATCH v6 1/2] net/bond: fixes for link properties management

[Ferruh Yigit]

On 7/5/2017 7:54 PM, Declan Doherty wrote:
> From: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> 
> This patch fixes the management of link properties in the bonded device.
> 
> In all mode except mode 4 a bonded device link will default to reporting
> the link as full duplex and auto-neg. The link speed for a bond port is
> calculated on it's active slaves and the particular mode it is running
> in. The bonding link speed is reported based on the transmit link as in
> some modes link speed between egress/ingress is not symmetrical.
> 
> - round-robin, balance, 802.3ad, TLB and ALB modes all report the link
>   speed as the sum of the speed of each active slave.
> - active backup link speed is reported as the speed of the current
>   primary slave
> - broadcast is reported as the minimum of value of the active slaves
>   link speeds.
> 
> In mode 4 (link aggregation 802.3ad) the properties of the first slave
> added to the bonded device are slave and subsequent slaves are verified
> to have the same properties.
> 
> Finally in the bond_ethdev_lsc_event_callback function the link
> properties of the device are updated after any change to the number of
> active slaves.

Fixes: 2efb58cbab6e ("bond: new link bonding library")
Cc: stable@dpdk.org

> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>


Series applied to dpdk-next-net/master, thanks.

(Signed-off from Tomasz, exists in older versions, added back)