[dpdk-dev] [PATCH v3] net/bonding: add add/remove mac addrs

[dpdk-dev] [PATCH v3] net/bonding: add add/remove mac addrs

[Alex Kiselev]

add functions to add/remove MAC addresses
Signed-off-by: Alex Kiselev <alex@therouter.net>
---
 drivers/net/bonding/rte_eth_bond_api.c     |  12 ++-
 drivers/net/bonding/rte_eth_bond_pmd.c     | 127 ++++++++++++++++++++++++++++-
 drivers/net/bonding/rte_eth_bond_private.h |   8 ++
 3 files changed, 142 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index d558df8b9..9123f4614 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -373,6 +373,13 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
 		return -1;
 	}
 
+	/* Add additional MAC addresses to the slave */
+	if (slave_add_mac_addresses(bonded_eth_dev, slave_port_id) != 0) {
+		RTE_BOND_LOG(ERR, "Failed to add mac address(es) to slave %hu",
+				slave_port_id);
+		return -1;
+	}
+
 	internals->slave_count++;
 
 	if (bonded_eth_dev->data->dev_started) {
@@ -387,7 +394,7 @@ __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t slave_port_id)
 	/* Add slave details to bonded device */
 	slave_eth_dev->data->dev_flags |= RTE_ETH_DEV_BONDED_SLAVE;
 
-	/* Update all slave devices MACs*/
+	/* Update all slave devices MACs */
 	mac_address_slaves_update(bonded_eth_dev);
 
 	/* Register link status change callback with bonded device pointer as
@@ -491,6 +498,9 @@ __eth_bond_slave_remove_lock_free(uint16_t bonded_port_id,
 	rte_eth_dev_default_mac_addr_set(slave_port_id,
 			&(internals->slaves[slave_idx].persisted_mac_addr));
 
+	/* remove additional MAC addresses from the slave */
+	slave_remove_mac_addresses(bonded_eth_dev, slave_port_id);
+
 	/*
 	 * Remove bond device flows from slave device.
 	 * Note: don't restore flow isolate mode.
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 02d94b1b1..97d689261 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -25,6 +25,7 @@
 
 #define REORDER_PERIOD_MS 10
 #define DEFAULT_POLLING_INTERVAL_10_MS (10)
+#define BOND_MAX_MAC_ADDRS 16
 
 #define HASH_L4_PORTS(h) ((h)->src_port ^ (h)->dst_port)
 
@@ -1588,6 +1589,57 @@ mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr)
 	return 0;
 }
 
+static const struct ether_addr null_mac_addr;
+
+/*
+ * Add additional MAC addresses to the slave
+ */
+int
+slave_add_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
+		uint16_t slave_port_id)
+{
+	int i, ret;
+	struct ether_addr *mac_addr;
+
+	for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
+		mac_addr = &bonded_eth_dev->data->mac_addrs[i];
+		if (is_same_ether_addr(mac_addr, &null_mac_addr))
+			break;
+
+		ret = rte_eth_dev_mac_addr_add(slave_port_id, mac_addr, 0);
+		if (ret < 0) {
+			/* rollback */
+			for (i--; i > 0; i--)
+				rte_eth_dev_mac_addr_remove(slave_port_id,
+					&bonded_eth_dev->data->mac_addrs[i]);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Remove additional MAC addresses from the slave
+ */
+int
+slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
+		uint16_t slave_port_id)
+{
+	int i, ret = 0;
+	struct ether_addr *mac_addr;
+
+	for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
+		mac_addr = &bonded_eth_dev->data->mac_addrs[i];
+		if (is_same_ether_addr(mac_addr, &null_mac_addr))
+			break;
+
+		ret = rte_eth_dev_mac_addr_remove(slave_port_id, mac_addr);
+	}
+
+	return ret;
+}
+
 int
 mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)
 {
@@ -2219,7 +2271,7 @@ bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	uint16_t max_nb_rx_queues = UINT16_MAX;
 	uint16_t max_nb_tx_queues = UINT16_MAX;
 
-	dev_info->max_mac_addrs = 1;
+	dev_info->max_mac_addrs = BOND_MAX_MAC_ADDRS;
 
 	dev_info->max_rx_pktlen = internals->candidate_max_rx_pktlen ?
 			internals->candidate_max_rx_pktlen :
@@ -2905,6 +2957,68 @@ bond_filter_ctrl(struct rte_eth_dev *dev __rte_unused,
 	return -ENOTSUP;
 }
 
+static int
+bond_ethdev_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
+				__rte_unused uint32_t index, uint32_t vmdq)
+{
+	struct rte_eth_dev *slave_eth_dev;
+	struct bond_dev_private *internals = dev->data->dev_private;
+	int ret, i;
+
+	rte_spinlock_lock(&internals->lock);
+
+	for (i = 0; i < internals->slave_count; i++) {
+		slave_eth_dev = &rte_eth_devices[internals->slaves[i].port_id];
+		if (*slave_eth_dev->dev_ops->mac_addr_add == NULL ||
+			 *slave_eth_dev->dev_ops->mac_addr_remove == NULL) {
+			ret = -ENOTSUP;
+			goto end;
+		}
+	}
+
+	for (i = 0; i < internals->slave_count; i++) {
+		ret = rte_eth_dev_mac_addr_add(internals->slaves[i].port_id,
+				mac_addr, vmdq);
+		if (ret < 0) {
+			/* rollback */
+			for (i--; i >= 0; i--)
+				rte_eth_dev_mac_addr_remove(
+					internals->slaves[i].port_id, mac_addr);
+			goto end;
+		}
+	}
+
+	ret = 0;
+end:
+	rte_spinlock_unlock(&internals->lock);
+	return ret;
+}
+
+static void
+bond_ethdev_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
+{
+	struct rte_eth_dev *slave_eth_dev;
+	struct bond_dev_private *internals = dev->data->dev_private;
+	int i;
+
+	rte_spinlock_lock(&internals->lock);
+
+	for (i = 0; i < internals->slave_count; i++) {
+		slave_eth_dev = &rte_eth_devices[internals->slaves[i].port_id];
+		if (*slave_eth_dev->dev_ops->mac_addr_remove == NULL)
+			goto end;
+	}
+
+	struct ether_addr *mac_addr = &dev->data->mac_addrs[index];
+
+	for (i = 0; i < internals->slave_count; i++)
+		rte_eth_dev_mac_addr_remove(internals->slaves[i].port_id,
+				mac_addr);
+
+end:
+	rte_spinlock_unlock(&internals->lock);
+}
+
 const struct eth_dev_ops default_dev_ops = {
 	.dev_start            = bond_ethdev_start,
 	.dev_stop             = bond_ethdev_stop,
@@ -2927,6 +3041,8 @@ const struct eth_dev_ops default_dev_ops = {
 	.rss_hash_conf_get    = bond_ethdev_rss_hash_conf_get,
 	.mtu_set              = bond_ethdev_mtu_set,
 	.mac_addr_set         = bond_ethdev_mac_address_set,
+	.mac_addr_add         = bond_ethdev_mac_addr_add,
+	.mac_addr_remove      = bond_ethdev_mac_addr_remove,
 	.filter_ctrl          = bond_filter_ctrl
 };
 
@@ -2954,10 +3070,13 @@ bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
 	eth_dev->data->nb_rx_queues = (uint16_t)1;
 	eth_dev->data->nb_tx_queues = (uint16_t)1;
 
-	eth_dev->data->mac_addrs = rte_zmalloc_socket(name, ETHER_ADDR_LEN, 0,
-			socket_id);
+	/* Allocate memory for storing MAC addresses */
+	eth_dev->data->mac_addrs = rte_zmalloc_socket(name, ETHER_ADDR_LEN *
+			BOND_MAX_MAC_ADDRS, 0, socket_id);
 	if (eth_dev->data->mac_addrs == NULL) {
-		RTE_BOND_LOG(ERR, "Unable to malloc mac_addrs");
+		RTE_BOND_LOG(ERR,
+			     "Failed to allocate %u bytes needed to store MAC addresses",
+			     ETHER_ADDR_LEN * BOND_MAX_MAC_ADDRS);
 		goto err;
 	}
 
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 65445b863..43e0e448d 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -230,6 +230,14 @@ mac_address_get(struct rte_eth_dev *eth_dev, struct ether_addr *dst_mac_addr);
 int
 mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev);
 
+int
+slave_add_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
+		uint16_t slave_port_id);
+
+int
+slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
+		uint16_t slave_port_id);
+
 int
 bond_ethdev_mode_set(struct rte_eth_dev *eth_dev, int mode);
 
-- 
2.16.1.windows.1

Re: [dpdk-dev] [PATCH v3] net/bonding: add add/remove mac addrs

[Matan Azrad]

Hi Alex

Please see comments below.

From: Alex Kiselev
> add functions to add/remove MAC addresses
> Signed-off-by: Alex Kiselev <alex@therouter.net>
> ---
>  drivers/net/bonding/rte_eth_bond_api.c     |  12 ++-
>  drivers/net/bonding/rte_eth_bond_pmd.c     | 127
> ++++++++++++++++++++++++++++-
>  drivers/net/bonding/rte_eth_bond_private.h |   8 ++
>  3 files changed, 142 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_api.c
> b/drivers/net/bonding/rte_eth_bond_api.c
> index d558df8b9..9123f4614 100644
> --- a/drivers/net/bonding/rte_eth_bond_api.c
> +++ b/drivers/net/bonding/rte_eth_bond_api.c
> @@ -373,6 +373,13 @@ __eth_bond_slave_add_lock_free(uint16_t
> bonded_port_id, uint16_t slave_port_id)
>  		return -1;
>  	}
> 
> +	/* Add additional MAC addresses to the slave */
> +	if (slave_add_mac_addresses(bonded_eth_dev, slave_port_id) != 0) {
> +		RTE_BOND_LOG(ERR, "Failed to add mac address(es) to slave
> %hu",
> +				slave_port_id);
> +		return -1;
> +	}
> +
>  	internals->slave_count++;
> 
>  	if (bonded_eth_dev->data->dev_started) { @@ -387,7 +394,7 @@
> __eth_bond_slave_add_lock_free(uint16_t bonded_port_id, uint16_t
> slave_port_id)
>  	/* Add slave details to bonded device */
>  	slave_eth_dev->data->dev_flags |= RTE_ETH_DEV_BONDED_SLAVE;
> 
> -	/* Update all slave devices MACs*/
> +	/* Update all slave devices MACs */
>  	mac_address_slaves_update(bonded_eth_dev);
> 
>  	/* Register link status change callback with bonded device pointer as
> @@ -491,6 +498,9 @@ __eth_bond_slave_remove_lock_free(uint16_t
> bonded_port_id,
>  	rte_eth_dev_default_mac_addr_set(slave_port_id,
>  			&(internals->slaves[slave_idx].persisted_mac_addr));
> 
> +	/* remove additional MAC addresses from the slave */
> +	slave_remove_mac_addresses(bonded_eth_dev, slave_port_id);
> +
>  	/*
>  	 * Remove bond device flows from slave device.
>  	 * Note: don't restore flow isolate mode.
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 02d94b1b1..97d689261 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -25,6 +25,7 @@
> 
>  #define REORDER_PERIOD_MS 10
>  #define DEFAULT_POLLING_INTERVAL_10_MS (10)
> +#define BOND_MAX_MAC_ADDRS 16
> 
>  #define HASH_L4_PORTS(h) ((h)->src_port ^ (h)->dst_port)
> 
> @@ -1588,6 +1589,57 @@ mac_address_set(struct rte_eth_dev *eth_dev,
> struct ether_addr *new_mac_addr)
>  	return 0;
>  }
> 
> +static const struct ether_addr null_mac_addr;
> +
> +/*
> + * Add additional MAC addresses to the slave  */ int
> +slave_add_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
> +		uint16_t slave_port_id)
> +{
> +	int i, ret;
> +	struct ether_addr *mac_addr;
> +
> +	for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
> +		mac_addr = &bonded_eth_dev->data->mac_addrs[i];
> +		if (is_same_ether_addr(mac_addr, &null_mac_addr))
> +			break;
> +
> +		ret = rte_eth_dev_mac_addr_add(slave_port_id, mac_addr, 0);
> +		if (ret < 0) {
> +			/* rollback */
> +			for (i--; i > 0; i--)
> +

In case of failure in the first mac address(i=1) you are going to remove the default mac address(i=0) from the slave.

> 	rte_eth_dev_mac_addr_remove(slave_port_id,
> +					&bonded_eth_dev->data-
> >mac_addrs[i]);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Remove additional MAC addresses from the slave  */ int
> +slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
> +		uint16_t slave_port_id)
> +{
> +	int i, ret = 0;
> +	struct ether_addr *mac_addr;
> +
> +	for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
> +		mac_addr = &bonded_eth_dev->data->mac_addrs[i];
> +		if (is_same_ether_addr(mac_addr, &null_mac_addr))
> +			break;
> +
> +		ret = rte_eth_dev_mac_addr_remove(slave_port_id,
> mac_addr);
> +	}

I suggest to return the first error, also in case of all success with last failure, the code here wrongly returns success. 

> +
> +	return ret;
> +}
> +
>  int
>  mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev)  { @@ -
> 2219,7 +2271,7 @@ bond_ethdev_info(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *dev_info)
>  	uint16_t max_nb_rx_queues = UINT16_MAX;
>  	uint16_t max_nb_tx_queues = UINT16_MAX;
> 
> -	dev_info->max_mac_addrs = 1;
> +	dev_info->max_mac_addrs = BOND_MAX_MAC_ADDRS;
> 
>  	dev_info->max_rx_pktlen = internals->candidate_max_rx_pktlen ?
>  			internals->candidate_max_rx_pktlen :
> @@ -2905,6 +2957,68 @@ bond_filter_ctrl(struct rte_eth_dev *dev
> __rte_unused,
>  	return -ENOTSUP;
>  }
> 
> +static int
> +bond_ethdev_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr
> *mac_addr,
> +				__rte_unused uint32_t index, uint32_t vmdq) {
> +	struct rte_eth_dev *slave_eth_dev;
> +	struct bond_dev_private *internals = dev->data->dev_private;
> +	int ret, i;
> +
> +	rte_spinlock_lock(&internals->lock);
> +
> +	for (i = 0; i < internals->slave_count; i++) {
> +		slave_eth_dev = &rte_eth_devices[internals->slaves[i].port_id];
> +		if (*slave_eth_dev->dev_ops->mac_addr_add == NULL ||
> +			 *slave_eth_dev->dev_ops->mac_addr_remove ==
> NULL) {
> +			ret = -ENOTSUP;
> +			goto end;
> +		}
> +	}
> +
> +	for (i = 0; i < internals->slave_count; i++) {
> +		ret = rte_eth_dev_mac_addr_add(internals->slaves[i].port_id,
> +				mac_addr, vmdq);
> +		if (ret < 0) {
> +			/* rollback */
> +			for (i--; i >= 0; i--)

In case of failure in the first slave(i=0) you are going probably to get memory error (i=-1).

> +				rte_eth_dev_mac_addr_remove(
> +					internals->slaves[i].port_id,
> mac_addr);
> +			goto end;
> +		}
> +	}
> +
> +	ret = 0;
> +end:
> +	rte_spinlock_unlock(&internals->lock);
> +	return ret;
> +}
> +
> +static void
> +bond_ethdev_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index) {
> +	struct rte_eth_dev *slave_eth_dev;
> +	struct bond_dev_private *internals = dev->data->dev_private;
> +	int i;
> +
> +	rte_spinlock_lock(&internals->lock);
> +
> +	for (i = 0; i < internals->slave_count; i++) {
> +		slave_eth_dev = &rte_eth_devices[internals->slaves[i].port_id];
> +		if (*slave_eth_dev->dev_ops->mac_addr_remove == NULL)
> +			goto end;
> +	}
> +
> +	struct ether_addr *mac_addr = &dev->data->mac_addrs[index];
> +
> +	for (i = 0; i < internals->slave_count; i++)
> +		rte_eth_dev_mac_addr_remove(internals->slaves[i].port_id,
> +				mac_addr);
> +
> +end:
> +	rte_spinlock_unlock(&internals->lock);
> +}
> +
>  const struct eth_dev_ops default_dev_ops = {
>  	.dev_start            = bond_ethdev_start,
>  	.dev_stop             = bond_ethdev_stop,
> @@ -2927,6 +3041,8 @@ const struct eth_dev_ops default_dev_ops = {
>  	.rss_hash_conf_get    = bond_ethdev_rss_hash_conf_get,
>  	.mtu_set              = bond_ethdev_mtu_set,
>  	.mac_addr_set         = bond_ethdev_mac_address_set,
> +	.mac_addr_add         = bond_ethdev_mac_addr_add,
> +	.mac_addr_remove      = bond_ethdev_mac_addr_remove,
>  	.filter_ctrl          = bond_filter_ctrl
>  };
> 
> @@ -2954,10 +3070,13 @@ bond_alloc(struct rte_vdev_device *dev, uint8_t
> mode)
>  	eth_dev->data->nb_rx_queues = (uint16_t)1;
>  	eth_dev->data->nb_tx_queues = (uint16_t)1;
> 
> -	eth_dev->data->mac_addrs = rte_zmalloc_socket(name,
> ETHER_ADDR_LEN, 0,
> -			socket_id);
> +	/* Allocate memory for storing MAC addresses */
> +	eth_dev->data->mac_addrs = rte_zmalloc_socket(name,
> ETHER_ADDR_LEN *
> +			BOND_MAX_MAC_ADDRS, 0, socket_id);
>  	if (eth_dev->data->mac_addrs == NULL) {
> -		RTE_BOND_LOG(ERR, "Unable to malloc mac_addrs");
> +		RTE_BOND_LOG(ERR,
> +			     "Failed to allocate %u bytes needed to store MAC
> addresses",
> +			     ETHER_ADDR_LEN * BOND_MAX_MAC_ADDRS);
>  		goto err;
>  	}
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_private.h
> b/drivers/net/bonding/rte_eth_bond_private.h
> index 65445b863..43e0e448d 100644
> --- a/drivers/net/bonding/rte_eth_bond_private.h
> +++ b/drivers/net/bonding/rte_eth_bond_private.h
> @@ -230,6 +230,14 @@ mac_address_get(struct rte_eth_dev *eth_dev, struct
> ether_addr *dst_mac_addr);  int  mac_address_slaves_update(struct
> rte_eth_dev *bonded_eth_dev);
> 
> +int
> +slave_add_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
> +		uint16_t slave_port_id);
> +
> +int
> +slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
> +		uint16_t slave_port_id);
> +
>  int
>  bond_ethdev_mode_set(struct rte_eth_dev *eth_dev, int mode);
> 
> --
> 2.16.1.windows.1

Re: [dpdk-dev] [PATCH v3] net/bonding: add add/remove mac addrs

[Alex Kiselev]

Hi Matan.

> Hi Alex

> Please see comments below.


>> +
>> +             ret = rte_eth_dev_mac_addr_add(slave_port_id, mac_addr, 0);
>> +             if (ret < 0) {
>> +                     /* rollback */
>> +                     for (i--; i > 0; i--)
>> +

> In case of failure in the first mac address(i=1) you are going to
> remove the default mac address(i=0) from the slave.
In that case i will be incremented first and will be equal to 0, then for condition will fail
and the loop body will not be executed.


>>       rte_eth_dev_mac_addr_remove(slave_port_id,
>> +                                     &bonded_eth_dev->data-
>> >mac_addrs[i]);
>> +                     return ret;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +/*
>> + * Remove additional MAC addresses from the slave  */ int
>> +slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
>> +             uint16_t slave_port_id)
>> +{
>> +     int i, ret = 0;
>> +     struct ether_addr *mac_addr;
>> +
>> +     for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
>> +             mac_addr = &bonded_eth_dev->data->mac_addrs[i];
>> +             if (is_same_ether_addr(mac_addr, &null_mac_addr))
>> +                     break;
>> +
>> +             ret = rte_eth_dev_mac_addr_remove(slave_port_id,
>> mac_addr);
>> +     }

> I suggest to return the first error, also in case of all success
> with last failure, the code here wrongly returns success. 
Yeah, you are right. I'll fix it.

>> +
>> +     for (i = 0; i < internals->slave_count; i++) {
>> +             ret = rte_eth_dev_mac_addr_add(internals->slaves[i].port_id,
>> +                             mac_addr, vmdq);
>> +             if (ret < 0) {
>> +                     /* rollback */
>> +                     for (i--; i >= 0; i--)

> In case of failure in the first slave(i=0) you are going probably to get memory error (i=-1).
The same logic apply here. When i ==-1 the condition will fail and the loop body will not be executed;





-- 
Alex

Re: [dpdk-dev] [PATCH v3] net/bonding: add add/remove mac addrs

[Matan Azrad]

 From: Alex Kiselev
> Hi Matan.
> 
> > Hi Alex
> 
> > Please see comments below.
> 
> 
> >> +
> >> +             ret = rte_eth_dev_mac_addr_add(slave_port_id, mac_addr, 0);
> >> +             if (ret < 0) {
> >> +                     /* rollback */
> >> +                     for (i--; i > 0; i--)
> >> +
> 
> > In case of failure in the first mac address(i=1) you are going to
> > remove the default mac address(i=0) from the slave.
> In that case i will be incremented first and will be equal to 0, then for condition
> will fail and the loop body will not be executed.
> 

Yes, my mistake.

> 
> >>       rte_eth_dev_mac_addr_remove(slave_port_id,
> >> +                                     &bonded_eth_dev->data-
> >> >mac_addrs[i]);
> >> +                     return ret;
> >> +             }
> >> +     }
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +/*
> >> + * Remove additional MAC addresses from the slave  */ int
> >> +slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
> >> +             uint16_t slave_port_id) {
> >> +     int i, ret = 0;
> >> +     struct ether_addr *mac_addr;
> >> +
> >> +     for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
> >> +             mac_addr = &bonded_eth_dev->data->mac_addrs[i];
> >> +             if (is_same_ether_addr(mac_addr, &null_mac_addr))
> >> +                     break;
> >> +
> >> +             ret = rte_eth_dev_mac_addr_remove(slave_port_id,
> >> mac_addr);
> >> +     }
> 
> > I suggest to return the first error, also in case of all success with
> > last failure, the code here wrongly returns success.
> Yeah, you are right. I'll fix it.
> 
> >> +
> >> +     for (i = 0; i < internals->slave_count; i++) {
> >> +             ret = rte_eth_dev_mac_addr_add(internals->slaves[i].port_id,
> >> +                             mac_addr, vmdq);
> >> +             if (ret < 0) {
> >> +                     /* rollback */
> >> +                     for (i--; i >= 0; i--)
> 
> > In case of failure in the first slave(i=0) you are going probably to get memory
> error (i=-1).
> The same logic apply here. When i ==-1 the condition will fail and the loop body
> will not be executed;

Yes, my mistake.

> 
> 
> 
> 
> --
> Alex

Re: [dpdk-dev] [PATCH v3] net/bonding: add add/remove mac addrs

[Alex Kiselev]

> Please see comments below.

From: Matan Azrad
>> +/*
>> + * Remove additional MAC addresses from the slave  */ int
>> +slave_remove_mac_addresses(struct rte_eth_dev *bonded_eth_dev,
>> +             uint16_t slave_port_id)
>> +{
>> +     int i, ret = 0;
>> +     struct ether_addr *mac_addr;
>> +
>> +     for (i = 1; i < BOND_MAX_MAC_ADDRS; i++) {
>> +             mac_addr = &bonded_eth_dev->data->mac_addrs[i];
>> +             if (is_same_ether_addr(mac_addr, &null_mac_addr))
>> +                     break;
>> +
>> +             ret = rte_eth_dev_mac_addr_remove(slave_port_id,
>> mac_addr);
>> +     }

> I suggest to return the first error, also in case of all success
> with last failure, the code here wrongly returns success. 

Fixed in the patch v4.


-- 
Alex