All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/3] MAC address fail to be added shouldn't be stored
@ 2017-04-12  9:02 Wei Dai
  2017-04-12  9:02 ` [PATCH v3 1/3] ethdev: fix adding invalid MAC addr Wei Dai
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Wei Dai @ 2017-04-12  9:02 UTC (permalink / raw)
  To: dev, thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, wenzhuo.lu, helin.zhang, konstantin.ananyev,
	jingjing.wu, jing.d.chen, adrien.mazarguil, nelio.laranjeiro,
	bruce.richardson, yuanhan.liu, maxime.coquelin
  Cc: Wei Dai

Current ethdev always stores MAC address even it fails to be added.
Other function may regard the failed MAC address valid and lead to
some errors. So There is a need to check if the addr is added
successfully or not and discard it if it fails.

In 3rd patch, add a command "add_more_mac_addr port_id base_mac_addr count"
to add more than one MAC address one time.
This command can simplify the test for the first patch.
Normally a MAC address may fails to be added only after many MAC
addresses have been added.
Without this command, a tester may only trigger failed MAC address
by running many times of testpmd command 'mac_addr add' .

---
Changes
v3:
  1. Change return value for some specific NIC according to feedbacks
     from the community;
  2. Add ABI change in release note;
  3. Add more detailed commit message.

v2:
  fix warnings and erros from check-git-log.sh and checkpatch.pl

Wei Dai (3):
  ethdev: fix adding invalid MAC addr
  doc: change type of return value of adding MAC addr
  app/testpmd: add a command to add many MAC addrs

 app/test-pmd/cmdline.c                 | 55 ++++++++++++++++++++++++++++++++++
 doc/guides/rel_notes/release_17_05.rst |  6 ++++
 drivers/net/bnx2x/bnx2x_ethdev.c       |  7 +++--
 drivers/net/bnxt/bnxt_ethdev.c         | 12 ++++----
 drivers/net/e1000/base/e1000_api.c     |  2 +-
 drivers/net/e1000/em_ethdev.c          |  6 ++--
 drivers/net/e1000/igb_ethdev.c         |  5 ++--
 drivers/net/enic/enic.h                |  2 +-
 drivers/net/enic/enic_ethdev.c         |  4 +--
 drivers/net/enic/enic_main.c           |  6 ++--
 drivers/net/fm10k/fm10k_ethdev.c       |  3 +-
 drivers/net/i40e/i40e_ethdev.c         | 11 +++----
 drivers/net/i40e/i40e_ethdev_vf.c      |  8 ++---
 drivers/net/ixgbe/ixgbe_ethdev.c       | 27 +++++++++++------
 drivers/net/mlx4/mlx4.c                | 14 +++++----
 drivers/net/mlx5/mlx5.h                |  4 +--
 drivers/net/mlx5/mlx5_mac.c            | 12 +++++---
 drivers/net/qede/qede_ethdev.c         |  6 ++--
 drivers/net/ring/rte_eth_ring.c        |  3 +-
 drivers/net/virtio/virtio_ethdev.c     | 13 ++++----
 lib/librte_ether/rte_ethdev.c          | 15 ++++++----
 lib/librte_ether/rte_ethdev.h          |  2 +-
 22 files changed, 157 insertions(+), 66 deletions(-)

--
2.7.4

^ permalink raw reply	[flat|nested] 19+ messages in thread
* [PATCH v3 1/3] ethdev: fix adding invalid MAC addr
  2017-04-12  9:02 [PATCH v3 0/3] MAC address fail to be added shouldn't be stored Wei Dai
@ 2017-04-12  9:02 ` Wei Dai
  2017-04-12  9:30   ` Nélio Laranjeiro
  2017-04-12  9:02 ` [PATCH v3 2/3] doc: change type of return value of adding " Wei Dai
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Wei Dai @ 2017-04-12  9:02 UTC (permalink / raw)
  To: dev, thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, wenzhuo.lu, helin.zhang, konstantin.ananyev,
	jingjing.wu, jing.d.chen, adrien.mazarguil, nelio.laranjeiro,
	bruce.richardson, yuanhan.liu, maxime.coquelin
  Cc: Wei Dai, stable

some customers find adding mac addr to VF sometimes can fail,
but it is still stored in dev->data->mac_addrs[ ]. So this
can lead to some errors that assumes the non-zero entry in
dev->data->mac_addrs[ ] is valid.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 drivers/net/bnx2x/bnx2x_ethdev.c   |  7 +++++--
 drivers/net/bnxt/bnxt_ethdev.c     | 12 ++++++------
 drivers/net/e1000/base/e1000_api.c |  2 +-
 drivers/net/e1000/em_ethdev.c      |  6 +++---
 drivers/net/e1000/igb_ethdev.c     |  5 +++--
 drivers/net/enic/enic.h            |  2 +-
 drivers/net/enic/enic_ethdev.c     |  4 ++--
 drivers/net/enic/enic_main.c       |  6 +++---
 drivers/net/fm10k/fm10k_ethdev.c   |  3 ++-
 drivers/net/i40e/i40e_ethdev.c     | 11 ++++++-----
 drivers/net/i40e/i40e_ethdev_vf.c  |  8 ++++----
 drivers/net/ixgbe/ixgbe_ethdev.c   | 27 ++++++++++++++++++---------
 drivers/net/mlx4/mlx4.c            | 14 +++++++++-----
 drivers/net/mlx5/mlx5.h            |  4 ++--
 drivers/net/mlx5/mlx5_mac.c        | 12 ++++++++----
 drivers/net/qede/qede_ethdev.c     |  6 ++++--
 drivers/net/ring/rte_eth_ring.c    |  3 ++-
 drivers/net/virtio/virtio_ethdev.c | 13 +++++++------
 lib/librte_ether/rte_ethdev.c      | 15 +++++++++------
 lib/librte_ether/rte_ethdev.h      |  2 +-
 20 files changed, 96 insertions(+), 66 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_ethdev.c b/drivers/net/bnx2x/bnx2x_ethdev.c
index a0b0dfa..6b80c96 100644
--- a/drivers/net/bnx2x/bnx2x_ethdev.c
+++ b/drivers/net/bnx2x/bnx2x_ethdev.c
@@ -449,14 +449,17 @@ bnx2x_dev_infos_get(struct rte_eth_dev *dev, __rte_unused struct rte_eth_dev_inf
 	dev_info->speed_capa = ETH_LINK_SPEED_10G | ETH_LINK_SPEED_20G;
 }
 
-static void
+static int
 bnx2x_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		uint32_t index, uint32_t pool)
 {
 	struct bnx2x_softc *sc = dev->data->dev_private;
 
-	if (sc->mac_ops.mac_addr_add)
+	if (sc->mac_ops.mac_addr_add) {
 		sc->mac_ops.mac_addr_add(dev, mac_addr, index, pool);
+		return 0;
+	}
+	return -ENOTSUP;
 }
 
 static void
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 6167443..9cb2667 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -613,7 +613,7 @@ static void bnxt_mac_addr_remove_op(struct rte_eth_dev *eth_dev,
 	}
 }
 
-static void bnxt_mac_addr_add_op(struct rte_eth_dev *eth_dev,
+static int bnxt_mac_addr_add_op(struct rte_eth_dev *eth_dev,
 				 struct ether_addr *mac_addr,
 				 uint32_t index, uint32_t pool)
 {
@@ -623,30 +623,30 @@ static void bnxt_mac_addr_add_op(struct rte_eth_dev *eth_dev,
 
 	if (BNXT_VF(bp)) {
 		RTE_LOG(ERR, PMD, "Cannot add MAC address to a VF interface\n");
-		return;
+		return -ENOTSUP;
 	}
 
 	if (!vnic) {
 		RTE_LOG(ERR, PMD, "VNIC not found for pool %d!\n", pool);
-		return;
+		return -EINVAL;
 	}
 	/* Attach requested MAC address to the new l2_filter */
 	STAILQ_FOREACH(filter, &vnic->filter, next) {
 		if (filter->mac_index == index) {
 			RTE_LOG(ERR, PMD,
 				"MAC addr already existed for pool %d\n", pool);
-			return;
+			return -EINVAL;
 		}
 	}
 	filter = bnxt_alloc_filter(bp);
 	if (!filter) {
 		RTE_LOG(ERR, PMD, "L2 filter alloc failed\n");
-		return;
+		return -ENODEV;
 	}
 	STAILQ_INSERT_TAIL(&vnic->filter, filter, next);
 	filter->mac_index = index;
 	memcpy(filter->l2_addr, mac_addr, ETHER_ADDR_LEN);
-	bnxt_hwrm_set_filter(bp, vnic, filter);
+	return bnxt_hwrm_set_filter(bp, vnic, filter);
 }
 
 int bnxt_link_update_op(struct rte_eth_dev *eth_dev, int wait_to_complete)
diff --git a/drivers/net/e1000/base/e1000_api.c b/drivers/net/e1000/base/e1000_api.c
index f7cf83b..dcb53f7 100644
--- a/drivers/net/e1000/base/e1000_api.c
+++ b/drivers/net/e1000/base/e1000_api.c
@@ -855,7 +855,7 @@ int e1000_rar_set(struct e1000_hw *hw, u8 *addr, u32 index)
 	if (hw->mac.ops.rar_set)
 		return hw->mac.ops.rar_set(hw, addr, index);
 
-	return E1000_SUCCESS;
+	return E1000_ERR_NO_SPACE;
 }
 
 /**
diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 7110af3..3f124ec 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -120,7 +120,7 @@ static int eth_em_led_on(struct rte_eth_dev *dev);
 static int eth_em_led_off(struct rte_eth_dev *dev);
 
 static int em_get_rx_buffer_size(struct e1000_hw *hw);
-static void eth_em_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
+static int eth_em_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		uint32_t index, uint32_t pool);
 static void eth_em_rar_clear(struct rte_eth_dev *dev, uint32_t index);
 
@@ -1788,13 +1788,13 @@ eth_em_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf)
 	return -EIO;
 }
 
-static void
+static int
 eth_em_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		uint32_t index, __rte_unused uint32_t pool)
 {
 	struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
-	e1000_rar_set(hw, mac_addr->addr_bytes, index);
+	return e1000_rar_set(hw, mac_addr->addr_bytes, index);
 }
 
 static void
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index a73cd7a..35225ba 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -165,7 +165,7 @@ static int eth_igb_led_off(struct rte_eth_dev *dev);
 
 static void igb_intr_disable(struct e1000_hw *hw);
 static int  igb_get_rx_buffer_size(struct e1000_hw *hw);
-static void eth_igb_rar_set(struct rte_eth_dev *dev,
+static int eth_igb_rar_set(struct rte_eth_dev *dev,
 		struct ether_addr *mac_addr,
 		uint32_t index, uint32_t pool);
 static void eth_igb_rar_clear(struct rte_eth_dev *dev, uint32_t index);
@@ -2976,7 +2976,7 @@ eth_igb_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf)
 }
 
 #define E1000_RAH_POOLSEL_SHIFT      (18)
-static void
+static int
 eth_igb_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 	        uint32_t index, __rte_unused uint32_t pool)
 {
@@ -2987,6 +2987,7 @@ eth_igb_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 	rah = E1000_READ_REG(hw, E1000_RAH(index));
 	rah |= (0x1 << (E1000_RAH_POOLSEL_SHIFT + pool));
 	E1000_WRITE_REG(hw, E1000_RAH(index), rah);
+	return 0;
 }
 
 static void
diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index e921de4..d17a35f 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -279,7 +279,7 @@ extern void enic_dev_stats_get(struct enic *enic,
 	struct rte_eth_stats *r_stats);
 extern void enic_dev_stats_clear(struct enic *enic);
 extern void enic_add_packet_filter(struct enic *enic);
-void enic_set_mac_address(struct enic *enic, uint8_t *mac_addr);
+int enic_set_mac_address(struct enic *enic, uint8_t *mac_addr);
 void enic_del_mac_address(struct enic *enic, int mac_index);
 extern unsigned int enic_cleanup_wq(struct enic *enic, struct vnic_wq *wq);
 extern void enic_send_pkt(struct enic *enic, struct vnic_wq *wq,
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index 2c2e29e..9ef9c31 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -532,14 +532,14 @@ static void enicpmd_dev_allmulticast_disable(struct rte_eth_dev *eth_dev)
 	enic_add_packet_filter(enic);
 }
 
-static void enicpmd_add_mac_addr(struct rte_eth_dev *eth_dev,
+static int enicpmd_add_mac_addr(struct rte_eth_dev *eth_dev,
 	struct ether_addr *mac_addr,
 	__rte_unused uint32_t index, __rte_unused uint32_t pool)
 {
 	struct enic *enic = pmd_priv(eth_dev);
 
 	ENICPMD_FUNC_TRACE();
-	enic_set_mac_address(enic, mac_addr->addr_bytes);
+	return enic_set_mac_address(enic, mac_addr->addr_bytes);
 }
 
 static void enicpmd_remove_mac_addr(struct rte_eth_dev *eth_dev, uint32_t index)
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 570b7b6..85490ab 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -202,20 +202,20 @@ void enic_del_mac_address(struct enic *enic, int mac_index)
 		dev_err(enic, "del mac addr failed\n");
 }
 
-void enic_set_mac_address(struct enic *enic, uint8_t *mac_addr)
+int enic_set_mac_address(struct enic *enic, uint8_t *mac_addr)
 {
 	int err;
 
 	if (!is_eth_addr_valid(mac_addr)) {
 		dev_err(enic, "invalid mac address\n");
-		return;
+		return -1;
 	}
 
 	err = vnic_dev_add_addr(enic->vdev, mac_addr);
 	if (err) {
 		dev_err(enic, "add mac addr failed\n");
-		return;
 	}
+	return err;
 }
 
 static void
diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index b6775d8..a2a74d7 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -1689,7 +1689,7 @@ static void fm10k_MAC_filter_set(struct rte_eth_dev *dev,
 }
 
 /* Add a MAC address, and update filters */
-static void
+static int
 fm10k_macaddr_add(struct rte_eth_dev *dev,
 		struct ether_addr *mac_addr,
 		uint32_t index,
@@ -1700,6 +1700,7 @@ fm10k_macaddr_add(struct rte_eth_dev *dev,
 	macvlan = FM10K_DEV_PRIVATE_TO_MACVLAN(dev->data->dev_private);
 	fm10k_MAC_filter_set(dev, mac_addr->addr_bytes, TRUE, pool);
 	macvlan->mac_vmdq_id[index] = pool;
+	return 0;
 }
 
 /* Remove a MAC address, and update filters */
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index cb8bf2b..37a0c70 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -300,7 +300,7 @@ static int i40e_flow_ctrl_set(struct rte_eth_dev *dev,
 			      struct rte_eth_fc_conf *fc_conf);
 static int i40e_priority_flow_ctrl_set(struct rte_eth_dev *dev,
 				       struct rte_eth_pfc_conf *pfc_conf);
-static void i40e_macaddr_add(struct rte_eth_dev *dev,
+static int i40e_macaddr_add(struct rte_eth_dev *dev,
 			  struct ether_addr *mac_addr,
 			  uint32_t index,
 			  uint32_t pool);
@@ -3270,7 +3270,7 @@ i40e_priority_flow_ctrl_set(__rte_unused struct rte_eth_dev *dev,
 }
 
 /* Add a MAC address, and update filters */
-static void
+static int
 i40e_macaddr_add(struct rte_eth_dev *dev,
 		 struct ether_addr *mac_addr,
 		 __rte_unused uint32_t index,
@@ -3287,13 +3287,13 @@ i40e_macaddr_add(struct rte_eth_dev *dev,
 		PMD_DRV_LOG(ERR, "VMDQ not %s, can't set mac to pool %u",
 			pf->flags & I40E_FLAG_VMDQ ? "configured" : "enabled",
 			pool);
-		return;
+		return -ENOTSUP;
 	}
 
 	if (pool > pf->nb_cfg_vmdq_vsi) {
 		PMD_DRV_LOG(ERR, "Pool number %u invalid. Max pool is %u",
 				pool, pf->nb_cfg_vmdq_vsi);
-		return;
+		return -EINVAL;
 	}
 
 	(void)rte_memcpy(&mac_filter.mac_addr, mac_addr, ETHER_ADDR_LEN);
@@ -3310,8 +3310,9 @@ i40e_macaddr_add(struct rte_eth_dev *dev,
 	ret = i40e_vsi_add_mac(vsi, &mac_filter);
 	if (ret != I40E_SUCCESS) {
 		PMD_DRV_LOG(ERR, "Failed to add MACVLAN filter");
-		return;
+		return -ENODEV;
 	}
+	return 0;
 }
 
 /* Remove a MAC address, and update filters */
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index d3659c9..e4daa1c 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -135,7 +135,7 @@ static int i40evf_dev_tx_queue_start(struct rte_eth_dev *dev,
 				     uint16_t tx_queue_id);
 static int i40evf_dev_tx_queue_stop(struct rte_eth_dev *dev,
 				    uint16_t tx_queue_id);
-static void i40evf_add_mac_addr(struct rte_eth_dev *dev,
+static int i40evf_add_mac_addr(struct rte_eth_dev *dev,
 				struct ether_addr *addr,
 				uint32_t index,
 				uint32_t pool);
@@ -854,7 +854,7 @@ i40evf_stop_queues(struct rte_eth_dev *dev)
 	return 0;
 }
 
-static void
+static int
 i40evf_add_mac_addr(struct rte_eth_dev *dev,
 		    struct ether_addr *addr,
 		    __rte_unused uint32_t index,
@@ -872,7 +872,7 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
 			    addr->addr_bytes[0], addr->addr_bytes[1],
 			    addr->addr_bytes[2], addr->addr_bytes[3],
 			    addr->addr_bytes[4], addr->addr_bytes[5]);
-		return;
+		return I40E_ERR_INVALID_MAC_ADDR;
 	}
 
 	list = (struct i40e_virtchnl_ether_addr_list *)cmd_buffer;
@@ -891,7 +891,7 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
 		PMD_DRV_LOG(ERR, "fail to execute command "
 			    "OP_ADD_ETHER_ADDRESS");
 
-	return;
+	return err;
 }
 
 static void
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 0ad59c2..a59b2f0 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -241,7 +241,7 @@ static int ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,
 static void ixgbe_dev_interrupt_handler(struct rte_intr_handle *handle,
 		void *param);
 static void ixgbe_dev_interrupt_delayed_handler(void *param);
-static void ixgbe_add_rar(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
+static int ixgbe_add_rar(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		uint32_t index, uint32_t pool);
 static void ixgbe_remove_rar(struct rte_eth_dev *dev, uint32_t index);
 static void ixgbe_set_default_mac_addr(struct rte_eth_dev *dev,
@@ -298,7 +298,7 @@ static void ixgbe_configure_msix(struct rte_eth_dev *dev);
 static int ixgbe_set_queue_rate_limit(struct rte_eth_dev *dev,
 		uint16_t queue_idx, uint16_t tx_rate);
 
-static void ixgbevf_add_mac_addr(struct rte_eth_dev *dev,
+static int ixgbevf_add_mac_addr(struct rte_eth_dev *dev,
 				 struct ether_addr *mac_addr,
 				 uint32_t index, uint32_t pool);
 static void ixgbevf_remove_mac_addr(struct rte_eth_dev *dev, uint32_t index);
@@ -4359,14 +4359,15 @@ ixgbe_dev_rss_reta_query(struct rte_eth_dev *dev,
 	return 0;
 }
 
-static void
+static int
 ixgbe_add_rar(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 				uint32_t index, uint32_t pool)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	uint32_t enable_addr = 1;
 
-	ixgbe_set_rar(hw, index, mac_addr->addr_bytes, pool, enable_addr);
+	return ixgbe_set_rar(hw, index, mac_addr->addr_bytes,
+				pool, enable_addr);
 }
 
 static void
@@ -5915,7 +5916,7 @@ static int ixgbe_set_queue_rate_limit(struct rte_eth_dev *dev,
 	return 0;
 }
 
-static void
+static int
 ixgbevf_add_mac_addr(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		     __attribute__((unused)) uint32_t index,
 		     __attribute__((unused)) uint32_t pool)
@@ -5929,11 +5930,19 @@ ixgbevf_add_mac_addr(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 	 * set of PF resources used to store VF MAC addresses.
 	 */
 	if (memcmp(hw->mac.perm_addr, mac_addr, sizeof(struct ether_addr)) == 0)
-		return;
+		return -1;
 	diag = ixgbevf_set_uc_addr_vf(hw, 2, mac_addr->addr_bytes);
-	if (diag == 0)
-		return;
-	PMD_DRV_LOG(ERR, "Unable to add MAC address - diag=%d", diag);
+	if (diag != 0)
+		PMD_DRV_LOG(ERR, "Unable to add MAC address "
+				"%02x:%02x:%02x:%02x:%02x:%02x- diag=%d",
+				mac_addr->addr_bytes[0],
+				mac_addr->addr_bytes[1],
+				mac_addr->addr_bytes[2],
+				mac_addr->addr_bytes[3],
+				mac_addr->addr_bytes[4],
+				mac_addr->addr_bytes[5],
+				diag);
+	return diag;
 }
 
 static void
diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index ff903e6..91fc4c4 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -4475,26 +4475,30 @@ mlx4_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
  * @param vmdq
  *   VMDq pool index to associate address with (ignored).
  */
-static void
+static int
 mlx4_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		  uint32_t index, uint32_t vmdq)
 {
 	struct priv *priv = dev->data->dev_private;
+	int re;
 
 	if (mlx4_is_secondary())
-		return;
+		return -ENOTSUP;
 	(void)vmdq;
 	priv_lock(priv);
 	DEBUG("%p: adding MAC address at index %" PRIu32,
 	      (void *)dev, index);
 	/* Last array entry is reserved for broadcast. */
-	if (index >= (elemof(priv->mac) - 1))
-		goto end;
-	priv_mac_addr_add(priv, index,
+	if (index >= (elemof(priv->mac) - 1)) {
+		priv_unlock(priv);
+		return -EINVAL;
+	}
+	re = priv_mac_addr_add(priv, index,
 			  (const uint8_t (*)[ETHER_ADDR_LEN])
 			  mac_addr->addr_bytes);
 end:
 	priv_unlock(priv);
+	return -re;
 }
 
 /**
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index d26d465..3c5de07 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -238,8 +238,8 @@ int hash_rxq_mac_addrs_add(struct hash_rxq *);
 int priv_mac_addr_add(struct priv *, unsigned int,
 		      const uint8_t (*)[ETHER_ADDR_LEN]);
 int priv_mac_addrs_enable(struct priv *);
-void mlx5_mac_addr_add(struct rte_eth_dev *, struct ether_addr *, uint32_t,
-		       uint32_t);
+int mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
+		  uint32_t index, uint32_t vmdq);
 void mlx5_mac_addr_set(struct rte_eth_dev *, struct ether_addr *);
 
 /* mlx5_rss.c */
diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
index 4fcfd3b..3cc6f8b 100644
--- a/drivers/net/mlx5/mlx5_mac.c
+++ b/drivers/net/mlx5/mlx5_mac.c
@@ -470,26 +470,30 @@ priv_mac_addrs_enable(struct priv *priv)
  * @param vmdq
  *   VMDq pool index to associate address with (ignored).
  */
-void
+int
 mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		  uint32_t index, uint32_t vmdq)
 {
 	struct priv *priv = dev->data->dev_private;
+	int re;
 
 	if (mlx5_is_secondary())
-		return;
+		return -ENOTSUP;
 
 	(void)vmdq;
 	priv_lock(priv);
 	DEBUG("%p: adding MAC address at index %" PRIu32,
 	      (void *)dev, index);
-	if (index >= RTE_DIM(priv->mac))
+	if (index >= RTE_DIM(priv->mac)) {
+		re = -EINVAL;
 		goto end;
-	priv_mac_addr_add(priv, index,
+	}
+	re = priv_mac_addr_add(priv, index,
 			  (const uint8_t (*)[ETHER_ADDR_LEN])
 			  mac_addr->addr_bytes);
 end:
 	priv_unlock(priv);
+	return -re;
 }
 
 /**
diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
index 2f1d2b5..2c8629f 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -506,16 +506,18 @@ qede_mac_int_ops(struct rte_eth_dev *eth_dev, struct ecore_filter_ucast *ucast,
 	return rc;
 }
 
-static void
+static int
 qede_mac_addr_add(struct rte_eth_dev *eth_dev, struct ether_addr *mac_addr,
 		  uint32_t index, __rte_unused uint32_t pool)
 {
 	struct ecore_filter_ucast ucast;
+	int re;
 
 	qede_set_ucast_cmn_params(&ucast);
 	ucast.type = ECORE_FILTER_MAC;
 	ether_addr_copy(mac_addr, (struct ether_addr *)&ucast.mac);
-	(void)qede_mac_int_ops(eth_dev, &ucast, 1);
+	re = (int)qede_mac_int_ops(eth_dev, &ucast, 1);
+	return re;
 }
 
 static void
diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index 77ef3a1..4ca4ca8 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -224,12 +224,13 @@ eth_mac_addr_remove(struct rte_eth_dev *dev __rte_unused,
 {
 }
 
-static void
+static int
 eth_mac_addr_add(struct rte_eth_dev *dev __rte_unused,
 	struct ether_addr *mac_addr __rte_unused,
 	uint32_t index __rte_unused,
 	uint32_t vmdq __rte_unused)
 {
+	return -1;
 }
 
 static void
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 6a9568f..462b36b 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -86,7 +86,7 @@ static void virtio_dev_stats_reset(struct rte_eth_dev *dev);
 static void virtio_dev_free_mbufs(struct rte_eth_dev *dev);
 static int virtio_vlan_filter_set(struct rte_eth_dev *dev,
 				uint16_t vlan_id, int on);
-static void virtio_mac_addr_add(struct rte_eth_dev *dev,
+static int virtio_mac_addr_add(struct rte_eth_dev *dev,
 				struct ether_addr *mac_addr,
 				uint32_t index, uint32_t vmdq __rte_unused);
 static void virtio_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);
@@ -1019,7 +1019,7 @@ virtio_get_hwaddr(struct virtio_hw *hw)
 	}
 }
 
-static void
+static int
 virtio_mac_table_set(struct virtio_hw *hw,
 		     const struct virtio_net_ctrl_mac *uc,
 		     const struct virtio_net_ctrl_mac *mc)
@@ -1029,7 +1029,7 @@ virtio_mac_table_set(struct virtio_hw *hw,
 
 	if (!vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
 		PMD_DRV_LOG(INFO, "host does not support mac table");
-		return;
+		return -1;
 	}
 
 	ctrl.hdr.class = VIRTIO_NET_CTRL_MAC;
@@ -1044,9 +1044,10 @@ virtio_mac_table_set(struct virtio_hw *hw,
 	err = virtio_send_command(hw->cvq, &ctrl, len, 2);
 	if (err != 0)
 		PMD_DRV_LOG(NOTICE, "mac table set failed: %d", err);
+	return err;
 }
 
-static void
+static int
 virtio_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		    uint32_t index, uint32_t vmdq __rte_unused)
 {
@@ -1057,7 +1058,7 @@ virtio_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 
 	if (index >= VIRTIO_MAX_MAC_ADDRS) {
 		PMD_DRV_LOG(ERR, "mac address index %u out of range", index);
-		return;
+		return -EINVAL;
 	}
 
 	uc = alloca(VIRTIO_MAX_MAC_ADDRS * ETHER_ADDR_LEN + sizeof(uc->entries));
@@ -1074,7 +1075,7 @@ virtio_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		memcpy(&tbl->macs[tbl->entries++], addr, ETHER_ADDR_LEN);
 	}
 
-	virtio_mac_table_set(hw, uc, mc);
+	return virtio_mac_table_set(hw, uc, mc);
 }
 
 static void
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 4e1e6dc..6b5de73 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -2167,6 +2167,7 @@ rte_eth_dev_mac_addr_add(uint8_t port_id, struct ether_addr *addr,
 	struct rte_eth_dev *dev;
 	int index;
 	uint64_t pool_mask;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
@@ -2199,15 +2200,17 @@ rte_eth_dev_mac_addr_add(uint8_t port_id, struct ether_addr *addr,
 	}
 
 	/* Update NIC */
-	(*dev->dev_ops->mac_addr_add)(dev, addr, index, pool);
+	ret = (*dev->dev_ops->mac_addr_add)(dev, addr, index, pool);
 
-	/* Update address in NIC data structure */
-	ether_addr_copy(addr, &dev->data->mac_addrs[index]);
+	if (ret == 0) {
+		/* Update address in NIC data structure */
+		ether_addr_copy(addr, &dev->data->mac_addrs[index]);
 
-	/* Update pool bitmap in NIC data structure */
-	dev->data->mac_pool_sel[index] |= (1ULL << pool);
+		/* Update pool bitmap in NIC data structure */
+		dev->data->mac_pool_sel[index] |= (1ULL << pool);
+	}
 
-	return 0;
+	return ret;
 }
 
 int
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index d072538..08e6c13 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1277,7 +1277,7 @@ typedef int (*eth_dev_led_off_t)(struct rte_eth_dev *dev);
 typedef void (*eth_mac_addr_remove_t)(struct rte_eth_dev *dev, uint32_t index);
 /**< @internal Remove MAC address from receive address register */
 
-typedef void (*eth_mac_addr_add_t)(struct rte_eth_dev *dev,
+typedef int (*eth_mac_addr_add_t)(struct rte_eth_dev *dev,
 				  struct ether_addr *mac_addr,
 				  uint32_t index,
 				  uint32_t vmdq);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH v3 2/3] doc: change type of return value of adding MAC addr
  2017-04-12  9:02 [PATCH v3 0/3] MAC address fail to be added shouldn't be stored Wei Dai
  2017-04-12  9:02 ` [PATCH v3 1/3] ethdev: fix adding invalid MAC addr Wei Dai
@ 2017-04-12  9:02 ` Wei Dai
  2017-04-12  9:02 ` [PATCH v3 3/3] app/testpmd: add a command to add many MAC addrs Wei Dai
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Wei Dai @ 2017-04-12  9:02 UTC (permalink / raw)
  To: dev, thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, wenzhuo.lu, helin.zhang, konstantin.ananyev,
	jingjing.wu, jing.d.chen, adrien.mazarguil, nelio.laranjeiro,
	bruce.richardson, yuanhan.liu, maxime.coquelin
  Cc: Wei Dai

Add following lines in section of API change in release note.

If a MAC address fails to be added without this change, it is still stored
and may be regarded as a valid one. This may lead to errors in application.
The type of return value of eth_mac_addr_add_t in rte_ethdev.h is changed.
Any specific NIC also follows this change.

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 doc/guides/rel_notes/release_17_05.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/guides/rel_notes/release_17_05.rst b/doc/guides/rel_notes/release_17_05.rst
index aa3e1e0..b3aac0c 100644
--- a/doc/guides/rel_notes/release_17_05.rst
+++ b/doc/guides/rel_notes/release_17_05.rst
@@ -408,6 +408,12 @@ ABI Changes
   The order and size of the fields in the ``mbuf`` structure changed,
   as described in the `New Features`_ section.
 
+* **Return if the MAC address is added successfully or not.**
+
+  If a MAC address fails to be added without this change, it is still stored
+  and may be regarded as a valid one. This may lead to errors in application.
+  The type of return value of eth_mac_addr_add_t in rte_ethdev.h is changed.
+  Any specific NIC also follows this change.
 
 Removed Items
 -------------
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH v3 3/3] app/testpmd: add a command to add many MAC addrs
  2017-04-12  9:02 [PATCH v3 0/3] MAC address fail to be added shouldn't be stored Wei Dai
  2017-04-12  9:02 ` [PATCH v3 1/3] ethdev: fix adding invalid MAC addr Wei Dai
  2017-04-12  9:02 ` [PATCH v3 2/3] doc: change type of return value of adding " Wei Dai
@ 2017-04-12  9:02 ` Wei Dai
  2017-04-13  8:21 ` [PATCH v4 0/3] MAC address fail to be added shouldn't be stored Wei Dai
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Wei Dai @ 2017-04-12  9:02 UTC (permalink / raw)
  To: dev, thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, wenzhuo.lu, helin.zhang, konstantin.ananyev,
	jingjing.wu, jing.d.chen, adrien.mazarguil, nelio.laranjeiro,
	bruce.richardson, yuanhan.liu, maxime.coquelin
  Cc: Wei Dai

This patch is added to introduce a testpmd command which
is used to add more than one MAC addresses one time.
This command can simplify the test for the change where
the type of return value of adding MAC address.
Normally a MAC address may fails to be added only after
many MAC addresses have been added.
Without this command, a tester may only trigger failed
MAC address by running many times of testpmd command
'mac_addr add' .

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 app/test-pmd/cmdline.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index e0d54d4..0aa9ccc 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -6424,6 +6424,60 @@ cmdline_parse_inst_t cmd_mac_addr = {
 	},
 };
 
+/* *** ADD MORE THAN ONE MAC ADDRESS FROM A PORT *** */
+struct cmd_add_more_mac_addr_result {
+	cmdline_fixed_string_t mac_addr_cmd;
+	uint8_t port_num;
+	struct ether_addr address;
+	uint8_t cnt_addr;
+};
+
+static void cmd_add_more_mac_addr_parsed(void *parsed_result,
+		__attribute__((unused)) struct cmdline *cl,
+		__attribute__((unused)) void *data)
+{
+	struct cmd_add_more_mac_addr_result *res = parsed_result;
+	int ret;
+	int k;
+
+	for (k = 0; k < res->cnt_addr; k++) {
+		ret = rte_eth_dev_mac_addr_add(res->port_num, &res->address, 0);
+		if (ret  < 0) {
+			printf("Fail to add mac addr : (%s) after adding %u addresses\n",
+				strerror(-ret), k);
+			return;
+		}
+		res->address.addr_bytes[5]++;
+	}
+	printf("Success to add %u mac addresses\n", k);
+}
+
+cmdline_parse_token_string_t cmd_add_more_mac_addr_cmd =
+	TOKEN_STRING_INITIALIZER(struct cmd_add_more_mac_addr_result,
+		mac_addr_cmd, "add_more_mac_addr");
+cmdline_parse_token_num_t cmd_add_more_mac_addr_portnum =
+	TOKEN_NUM_INITIALIZER(struct cmd_add_more_mac_addr_result,
+		port_num, UINT8);
+cmdline_parse_token_etheraddr_t cmd_add_more_mac_addr_addr =
+	TOKEN_ETHERADDR_INITIALIZER(struct cmd_add_more_mac_addr_result,
+		address);
+cmdline_parse_token_num_t cmd_add_more_mac_addr_cnt_addr =
+	TOKEN_NUM_INITIALIZER(struct cmd_add_more_mac_addr_result,
+		cnt_addr, UINT8);
+
+cmdline_parse_inst_t cmd_add_more_mac_addr = {
+	.f = cmd_add_more_mac_addr_parsed,
+	.data = (void *)0,
+	.help_str = "add_more_mac_addr <port_id> <mac_addr> <cnt_addr>: "
+			"Add cnt_addr MAC addresses on port_id",
+	.tokens = {
+		(void *)&cmd_add_more_mac_addr_cmd,
+		(void *)&cmd_add_more_mac_addr_portnum,
+		(void *)&cmd_add_more_mac_addr_addr,
+		(void *)&cmd_add_more_mac_addr_cnt_addr,
+		NULL,
+	},
+};
 
 /* *** CONFIGURE QUEUE STATS COUNTER MAPPINGS *** */
 struct cmd_set_qmap_result {
@@ -13244,6 +13298,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_read_rxd_txd,
 	(cmdline_parse_inst_t *)&cmd_stop,
 	(cmdline_parse_inst_t *)&cmd_mac_addr,
+	(cmdline_parse_inst_t *)&cmd_add_more_mac_addr,
 	(cmdline_parse_inst_t *)&cmd_set_qmap,
 	(cmdline_parse_inst_t *)&cmd_operate_port,
 	(cmdline_parse_inst_t *)&cmd_operate_specific_port,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] ethdev: fix adding invalid MAC addr
  2017-04-12  9:02 ` [PATCH v3 1/3] ethdev: fix adding invalid MAC addr Wei Dai
@ 2017-04-12  9:30   ` Nélio Laranjeiro
  0 siblings, 0 replies; 19+ messages in thread
From: Nélio Laranjeiro @ 2017-04-12  9:30 UTC (permalink / raw)
  To: Wei Dai
  Cc: dev, thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, wenzhuo.lu, helin.zhang, konstantin.ananyev,
	jingjing.wu, jing.d.chen, adrien.mazarguil, bruce.richardson,
	yuanhan.liu, maxime.coquelin, stable

Hi,

note, please use the --thread option in addition of the in-reply-to to
new series.

Small comments inline,

On Wed, Apr 12, 2017 at 05:02:24PM +0800, Wei Dai wrote:
>[...]
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index ff903e6..91fc4c4 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -4475,26 +4475,30 @@ mlx4_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
>   * @param vmdq
>   *   VMDq pool index to associate address with (ignored).
>   */
> -static void
> +static int
>  mlx4_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  		  uint32_t index, uint32_t vmdq)
>  {
>  	struct priv *priv = dev->data->dev_private;
> +	int re;
>  
>  	if (mlx4_is_secondary())
> -		return;
> +		return -ENOTSUP;
>  	(void)vmdq;
>  	priv_lock(priv);
>  	DEBUG("%p: adding MAC address at index %" PRIu32,
>  	      (void *)dev, index);
>  	/* Last array entry is reserved for broadcast. */
> -	if (index >= (elemof(priv->mac) - 1))
> -		goto end;
> -	priv_mac_addr_add(priv, index,
> +	if (index >= (elemof(priv->mac) - 1)) {
> +		priv_unlock(priv);
> +		return -EINVAL;
> +	}
> +	re = priv_mac_addr_add(priv, index,
>  			  (const uint8_t (*)[ETHER_ADDR_LEN])
>  			  mac_addr->addr_bytes);

Please keep the coding style consistency among the file, this last
snippet should be:

        re = priv_mac_addr_add(priv, index,
                               (const uint8_t (*)[ETHER_ADDR_LEN])
                               mac_addr->addr_bytes);

>  end:
>  	priv_unlock(priv);
> +	return -re;
>  }
>  
>  /**
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index d26d465..3c5de07 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -238,8 +238,8 @@ int hash_rxq_mac_addrs_add(struct hash_rxq *);
>  int priv_mac_addr_add(struct priv *, unsigned int,
>  		      const uint8_t (*)[ETHER_ADDR_LEN]);
>  int priv_mac_addrs_enable(struct priv *);
> -void mlx5_mac_addr_add(struct rte_eth_dev *, struct ether_addr *, uint32_t,
> -		       uint32_t);
> +int mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
> +		  uint32_t index, uint32_t vmdq);

In the whole file, function prototypes does not provide variable names,
please keep it as is.

Same point about the indentation, the second line should be aligned with
the '('.

>  void mlx5_mac_addr_set(struct rte_eth_dev *, struct ether_addr *);
>  
>  /* mlx5_rss.c */
> diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
> index 4fcfd3b..3cc6f8b 100644
> --- a/drivers/net/mlx5/mlx5_mac.c
> +++ b/drivers/net/mlx5/mlx5_mac.c
> @@ -470,26 +470,30 @@ priv_mac_addrs_enable(struct priv *priv)
>   * @param vmdq
>   *   VMDq pool index to associate address with (ignored).
>   */
> -void
> +int
>  mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  		  uint32_t index, uint32_t vmdq)
>  {
>  	struct priv *priv = dev->data->dev_private;
> +	int re;
>  
>  	if (mlx5_is_secondary())
> -		return;
> +		return -ENOTSUP;
>  
>  	(void)vmdq;
>  	priv_lock(priv);
>  	DEBUG("%p: adding MAC address at index %" PRIu32,
>  	      (void *)dev, index);
> -	if (index >= RTE_DIM(priv->mac))
> +	if (index >= RTE_DIM(priv->mac)) {
> +		re = -EINVAL;
>  		goto end;
> -	priv_mac_addr_add(priv, index,
> +	}
> +	re = priv_mac_addr_add(priv, index,
>  			  (const uint8_t (*)[ETHER_ADDR_LEN])
>  			  mac_addr->addr_bytes);

Same remark here about the indentation.

>  end:
>  	priv_unlock(priv);
> +	return -re;
>  }

Thanks,

-- 
Nélio Laranjeiro
6WIND

^ permalink raw reply	[flat|nested] 19+ messages in thread
* [PATCH v4 0/3] MAC address fail to be added shouldn't be stored
  2017-04-12  9:02 [PATCH v3 0/3] MAC address fail to be added shouldn't be stored Wei Dai
                   ` (2 preceding siblings ...)
  2017-04-12  9:02 ` [PATCH v3 3/3] app/testpmd: add a command to add many MAC addrs Wei Dai
@ 2017-04-13  8:21 ` Wei Dai
  2017-04-13 13:54   ` Ananyev, Konstantin
  2017-04-13  8:21 ` [PATCH v4 1/3] ethdev: fix adding invalid MAC addr Wei Dai
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Wei Dai @ 2017-04-13  8:21 UTC (permalink / raw)
  To: thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, wenzhuo.lu, helin.zhang, konstantin.ananyev,
	jingjing.wu, jing.d.chen, adrien.mazarguil, nelio.laranjeiro,
	bruce.richardson, yuanhan.liu, maxime.coquelin
  Cc: dev, Wei Dai

Current ethdev always stores MAC address even it fails to be added.
Other function may regard the failed MAC address valid and lead to
some errors. So There is a need to check if the addr is added
successfully or not and discard it if it fails.

In 3rd patch, add a command "add_more_mac_addr port_id base_mac_addr count"
to add more than one MAC address one time.
This command can simplify the test for the first patch.
Normally a MAC address may fails to be added only after many MAC
addresses have been added.
Without this command, a tester may only trigger failed MAC address
by running many times of testpmd command 'mac_addr add' .

---
Changes
v4:
	1. rebase master branch
	2. follow code style
	
v3:
  1. Change return value for some specific NIC according to feedbacks
     from the community;
  2. Add ABI change in release note;
  3. Add more detailed commit message.

v2:
  fix warnings and erros from check-git-log.sh and checkpatch.pl

Wei Dai (3):
  ethdev: fix adding invalid MAC addr
  doc: change type of return value of adding MAC addr
  app/testpmd: add a command to add many MAC addrs

 app/test-pmd/cmdline.c                 | 55 ++++++++++++++++++++++++++++++++++
 doc/guides/rel_notes/release_17_05.rst |  7 +++++
 drivers/net/bnx2x/bnx2x_ethdev.c       |  7 +++--
 drivers/net/bnxt/bnxt_ethdev.c         | 12 ++++----
 drivers/net/e1000/base/e1000_api.c     |  2 +-
 drivers/net/e1000/em_ethdev.c          |  6 ++--
 drivers/net/e1000/igb_ethdev.c         |  5 ++--
 drivers/net/enic/enic.h                |  2 +-
 drivers/net/enic/enic_ethdev.c         |  4 +--
 drivers/net/enic/enic_main.c           |  6 ++--
 drivers/net/fm10k/fm10k_ethdev.c       |  3 +-
 drivers/net/i40e/i40e_ethdev.c         | 11 +++----
 drivers/net/i40e/i40e_ethdev_vf.c      |  8 ++---
 drivers/net/ixgbe/ixgbe_ethdev.c       | 27 +++++++++++------
 drivers/net/mlx4/mlx4.c                | 18 ++++++-----
 drivers/net/mlx5/mlx5.h                |  4 +--
 drivers/net/mlx5/mlx5_mac.c            | 16 ++++++----
 drivers/net/qede/qede_ethdev.c         |  6 ++--
 drivers/net/ring/rte_eth_ring.c        |  3 +-
 drivers/net/virtio/virtio_ethdev.c     | 13 ++++----
 lib/librte_ether/rte_ethdev.c          | 15 ++++++----
 lib/librte_ether/rte_ethdev.h          |  2 +-
 22 files changed, 162 insertions(+), 70 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 19+ messages in thread
* [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
  2017-04-12  9:02 [PATCH v3 0/3] MAC address fail to be added shouldn't be stored Wei Dai
                   ` (3 preceding siblings ...)
  2017-04-13  8:21 ` [PATCH v4 0/3] MAC address fail to be added shouldn't be stored Wei Dai
@ 2017-04-13  8:21 ` Wei Dai
  2017-04-13  8:44   ` Nélio Laranjeiro
                     ` (3 more replies)
  2017-04-13  8:21 ` [PATCH v4 2/3] doc: change type of return value of adding " Wei Dai
  2017-04-13  8:21 ` [PATCH v4 3/3] app/testpmd: add a command to add many MAC addrs Wei Dai
  6 siblings, 4 replies; 19+ messages in thread
From: Wei Dai @ 2017-04-13  8:21 UTC (permalink / raw)
  To: thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, wenzhuo.lu, helin.zhang, konstantin.ananyev,
	jingjing.wu, jing.d.chen, adrien.mazarguil, nelio.laranjeiro,
	bruce.richardson, yuanhan.liu, maxime.coquelin
  Cc: dev, Wei Dai, stable

some customers find adding mac addr to VF sometimes can fail,
but it is still stored in dev->data->mac_addrs[ ]. So this
can lead to some errors that assumes the non-zero entry in
dev->data->mac_addrs[ ] is valid.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 drivers/net/bnx2x/bnx2x_ethdev.c   |  7 +++++--
 drivers/net/bnxt/bnxt_ethdev.c     | 12 ++++++------
 drivers/net/e1000/base/e1000_api.c |  2 +-
 drivers/net/e1000/em_ethdev.c      |  6 +++---
 drivers/net/e1000/igb_ethdev.c     |  5 +++--
 drivers/net/enic/enic.h            |  2 +-
 drivers/net/enic/enic_ethdev.c     |  4 ++--
 drivers/net/enic/enic_main.c       |  6 +++---
 drivers/net/fm10k/fm10k_ethdev.c   |  3 ++-
 drivers/net/i40e/i40e_ethdev.c     | 11 ++++++-----
 drivers/net/i40e/i40e_ethdev_vf.c  |  8 ++++----
 drivers/net/ixgbe/ixgbe_ethdev.c   | 27 ++++++++++++++++++---------
 drivers/net/mlx4/mlx4.c            | 18 +++++++++++-------
 drivers/net/mlx5/mlx5.h            |  4 ++--
 drivers/net/mlx5/mlx5_mac.c        | 16 ++++++++++------
 drivers/net/qede/qede_ethdev.c     |  6 ++++--
 drivers/net/ring/rte_eth_ring.c    |  3 ++-
 drivers/net/virtio/virtio_ethdev.c | 13 +++++++------
 lib/librte_ether/rte_ethdev.c      | 15 +++++++++------
 lib/librte_ether/rte_ethdev.h      |  2 +-
 20 files changed, 100 insertions(+), 70 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_ethdev.c b/drivers/net/bnx2x/bnx2x_ethdev.c
index 0e8b4d9..425b48d 100644
--- a/drivers/net/bnx2x/bnx2x_ethdev.c
+++ b/drivers/net/bnx2x/bnx2x_ethdev.c
@@ -450,14 +450,17 @@ bnx2x_dev_infos_get(struct rte_eth_dev *dev, __rte_unused struct rte_eth_dev_inf
 	dev_info->speed_capa = ETH_LINK_SPEED_10G | ETH_LINK_SPEED_20G;
 }
 
-static void
+static int
 bnx2x_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		uint32_t index, uint32_t pool)
 {
 	struct bnx2x_softc *sc = dev->data->dev_private;
 
-	if (sc->mac_ops.mac_addr_add)
+	if (sc->mac_ops.mac_addr_add) {
 		sc->mac_ops.mac_addr_add(dev, mac_addr, index, pool);
+		return 0;
+	}
+	return -ENOTSUP;
 }
 
 static void
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 6167443..9cb2667 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -613,7 +613,7 @@ static void bnxt_mac_addr_remove_op(struct rte_eth_dev *eth_dev,
 	}
 }
 
-static void bnxt_mac_addr_add_op(struct rte_eth_dev *eth_dev,
+static int bnxt_mac_addr_add_op(struct rte_eth_dev *eth_dev,
 				 struct ether_addr *mac_addr,
 				 uint32_t index, uint32_t pool)
 {
@@ -623,30 +623,30 @@ static void bnxt_mac_addr_add_op(struct rte_eth_dev *eth_dev,
 
 	if (BNXT_VF(bp)) {
 		RTE_LOG(ERR, PMD, "Cannot add MAC address to a VF interface\n");
-		return;
+		return -ENOTSUP;
 	}
 
 	if (!vnic) {
 		RTE_LOG(ERR, PMD, "VNIC not found for pool %d!\n", pool);
-		return;
+		return -EINVAL;
 	}
 	/* Attach requested MAC address to the new l2_filter */
 	STAILQ_FOREACH(filter, &vnic->filter, next) {
 		if (filter->mac_index == index) {
 			RTE_LOG(ERR, PMD,
 				"MAC addr already existed for pool %d\n", pool);
-			return;
+			return -EINVAL;
 		}
 	}
 	filter = bnxt_alloc_filter(bp);
 	if (!filter) {
 		RTE_LOG(ERR, PMD, "L2 filter alloc failed\n");
-		return;
+		return -ENODEV;
 	}
 	STAILQ_INSERT_TAIL(&vnic->filter, filter, next);
 	filter->mac_index = index;
 	memcpy(filter->l2_addr, mac_addr, ETHER_ADDR_LEN);
-	bnxt_hwrm_set_filter(bp, vnic, filter);
+	return bnxt_hwrm_set_filter(bp, vnic, filter);
 }
 
 int bnxt_link_update_op(struct rte_eth_dev *eth_dev, int wait_to_complete)
diff --git a/drivers/net/e1000/base/e1000_api.c b/drivers/net/e1000/base/e1000_api.c
index f7cf83b..dcb53f7 100644
--- a/drivers/net/e1000/base/e1000_api.c
+++ b/drivers/net/e1000/base/e1000_api.c
@@ -855,7 +855,7 @@ int e1000_rar_set(struct e1000_hw *hw, u8 *addr, u32 index)
 	if (hw->mac.ops.rar_set)
 		return hw->mac.ops.rar_set(hw, addr, index);
 
-	return E1000_SUCCESS;
+	return E1000_ERR_NO_SPACE;
 }
 
 /**
diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index ecefa56..3a80856 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -119,7 +119,7 @@ static int eth_em_led_on(struct rte_eth_dev *dev);
 static int eth_em_led_off(struct rte_eth_dev *dev);
 
 static int em_get_rx_buffer_size(struct e1000_hw *hw);
-static void eth_em_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
+static int eth_em_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		uint32_t index, uint32_t pool);
 static void eth_em_rar_clear(struct rte_eth_dev *dev, uint32_t index);
 
@@ -1786,13 +1786,13 @@ eth_em_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf)
 	return -EIO;
 }
 
-static void
+static int
 eth_em_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		uint32_t index, __rte_unused uint32_t pool)
 {
 	struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
-	e1000_rar_set(hw, mac_addr->addr_bytes, index);
+	return e1000_rar_set(hw, mac_addr->addr_bytes, index);
 }
 
 static void
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index cc2c244..3d95ec4 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -164,7 +164,7 @@ static int eth_igb_led_off(struct rte_eth_dev *dev);
 
 static void igb_intr_disable(struct e1000_hw *hw);
 static int  igb_get_rx_buffer_size(struct e1000_hw *hw);
-static void eth_igb_rar_set(struct rte_eth_dev *dev,
+static int eth_igb_rar_set(struct rte_eth_dev *dev,
 		struct ether_addr *mac_addr,
 		uint32_t index, uint32_t pool);
 static void eth_igb_rar_clear(struct rte_eth_dev *dev, uint32_t index);
@@ -2973,7 +2973,7 @@ eth_igb_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf)
 }
 
 #define E1000_RAH_POOLSEL_SHIFT      (18)
-static void
+static int
 eth_igb_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 	        uint32_t index, __rte_unused uint32_t pool)
 {
@@ -2984,6 +2984,7 @@ eth_igb_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 	rah = E1000_READ_REG(hw, E1000_RAH(index));
 	rah |= (0x1 << (E1000_RAH_POOLSEL_SHIFT + pool));
 	E1000_WRITE_REG(hw, E1000_RAH(index), rah);
+	return 0;
 }
 
 static void
diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index e921de4..d17a35f 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -279,7 +279,7 @@ extern void enic_dev_stats_get(struct enic *enic,
 	struct rte_eth_stats *r_stats);
 extern void enic_dev_stats_clear(struct enic *enic);
 extern void enic_add_packet_filter(struct enic *enic);
-void enic_set_mac_address(struct enic *enic, uint8_t *mac_addr);
+int enic_set_mac_address(struct enic *enic, uint8_t *mac_addr);
 void enic_del_mac_address(struct enic *enic, int mac_index);
 extern unsigned int enic_cleanup_wq(struct enic *enic, struct vnic_wq *wq);
 extern void enic_send_pkt(struct enic *enic, struct vnic_wq *wq,
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index 2c2e29e..9ef9c31 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -532,14 +532,14 @@ static void enicpmd_dev_allmulticast_disable(struct rte_eth_dev *eth_dev)
 	enic_add_packet_filter(enic);
 }
 
-static void enicpmd_add_mac_addr(struct rte_eth_dev *eth_dev,
+static int enicpmd_add_mac_addr(struct rte_eth_dev *eth_dev,
 	struct ether_addr *mac_addr,
 	__rte_unused uint32_t index, __rte_unused uint32_t pool)
 {
 	struct enic *enic = pmd_priv(eth_dev);
 
 	ENICPMD_FUNC_TRACE();
-	enic_set_mac_address(enic, mac_addr->addr_bytes);
+	return enic_set_mac_address(enic, mac_addr->addr_bytes);
 }
 
 static void enicpmd_remove_mac_addr(struct rte_eth_dev *eth_dev, uint32_t index)
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 5f2188b..c6892e9 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -202,20 +202,20 @@ void enic_del_mac_address(struct enic *enic, int mac_index)
 		dev_err(enic, "del mac addr failed\n");
 }
 
-void enic_set_mac_address(struct enic *enic, uint8_t *mac_addr)
+int enic_set_mac_address(struct enic *enic, uint8_t *mac_addr)
 {
 	int err;
 
 	if (!is_eth_addr_valid(mac_addr)) {
 		dev_err(enic, "invalid mac address\n");
-		return;
+		return -1;
 	}
 
 	err = vnic_dev_add_addr(enic->vdev, mac_addr);
 	if (err) {
 		dev_err(enic, "add mac addr failed\n");
-		return;
 	}
+	return err;
 }
 
 static void
diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index de0352e..dfc58fc 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -1689,7 +1689,7 @@ static void fm10k_MAC_filter_set(struct rte_eth_dev *dev,
 }
 
 /* Add a MAC address, and update filters */
-static void
+static int
 fm10k_macaddr_add(struct rte_eth_dev *dev,
 		struct ether_addr *mac_addr,
 		uint32_t index,
@@ -1700,6 +1700,7 @@ fm10k_macaddr_add(struct rte_eth_dev *dev,
 	macvlan = FM10K_DEV_PRIVATE_TO_MACVLAN(dev->data->dev_private);
 	fm10k_MAC_filter_set(dev, mac_addr->addr_bytes, TRUE, pool);
 	macvlan->mac_vmdq_id[index] = pool;
+	return 0;
 }
 
 /* Remove a MAC address, and update filters */
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 6927fde..173aa87 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -300,7 +300,7 @@ static int i40e_flow_ctrl_set(struct rte_eth_dev *dev,
 			      struct rte_eth_fc_conf *fc_conf);
 static int i40e_priority_flow_ctrl_set(struct rte_eth_dev *dev,
 				       struct rte_eth_pfc_conf *pfc_conf);
-static void i40e_macaddr_add(struct rte_eth_dev *dev,
+static int i40e_macaddr_add(struct rte_eth_dev *dev,
 			  struct ether_addr *mac_addr,
 			  uint32_t index,
 			  uint32_t pool);
@@ -3269,7 +3269,7 @@ i40e_priority_flow_ctrl_set(__rte_unused struct rte_eth_dev *dev,
 }
 
 /* Add a MAC address, and update filters */
-static void
+static int
 i40e_macaddr_add(struct rte_eth_dev *dev,
 		 struct ether_addr *mac_addr,
 		 __rte_unused uint32_t index,
@@ -3286,13 +3286,13 @@ i40e_macaddr_add(struct rte_eth_dev *dev,
 		PMD_DRV_LOG(ERR, "VMDQ not %s, can't set mac to pool %u",
 			pf->flags & I40E_FLAG_VMDQ ? "configured" : "enabled",
 			pool);
-		return;
+		return -ENOTSUP;
 	}
 
 	if (pool > pf->nb_cfg_vmdq_vsi) {
 		PMD_DRV_LOG(ERR, "Pool number %u invalid. Max pool is %u",
 				pool, pf->nb_cfg_vmdq_vsi);
-		return;
+		return -EINVAL;
 	}
 
 	(void)rte_memcpy(&mac_filter.mac_addr, mac_addr, ETHER_ADDR_LEN);
@@ -3309,8 +3309,9 @@ i40e_macaddr_add(struct rte_eth_dev *dev,
 	ret = i40e_vsi_add_mac(vsi, &mac_filter);
 	if (ret != I40E_SUCCESS) {
 		PMD_DRV_LOG(ERR, "Failed to add MACVLAN filter");
-		return;
+		return -ENODEV;
 	}
+	return 0;
 }
 
 /* Remove a MAC address, and update filters */
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 7e48fea..fcccc46 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -135,7 +135,7 @@ static int i40evf_dev_tx_queue_start(struct rte_eth_dev *dev,
 				     uint16_t tx_queue_id);
 static int i40evf_dev_tx_queue_stop(struct rte_eth_dev *dev,
 				    uint16_t tx_queue_id);
-static void i40evf_add_mac_addr(struct rte_eth_dev *dev,
+static int i40evf_add_mac_addr(struct rte_eth_dev *dev,
 				struct ether_addr *addr,
 				uint32_t index,
 				uint32_t pool);
@@ -854,7 +854,7 @@ i40evf_stop_queues(struct rte_eth_dev *dev)
 	return 0;
 }
 
-static void
+static int
 i40evf_add_mac_addr(struct rte_eth_dev *dev,
 		    struct ether_addr *addr,
 		    __rte_unused uint32_t index,
@@ -872,7 +872,7 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
 			    addr->addr_bytes[0], addr->addr_bytes[1],
 			    addr->addr_bytes[2], addr->addr_bytes[3],
 			    addr->addr_bytes[4], addr->addr_bytes[5]);
-		return;
+		return I40E_ERR_INVALID_MAC_ADDR;
 	}
 
 	list = (struct i40e_virtchnl_ether_addr_list *)cmd_buffer;
@@ -891,7 +891,7 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
 		PMD_DRV_LOG(ERR, "fail to execute command "
 			    "OP_ADD_ETHER_ADDRESS");
 
-	return;
+	return err;
 }
 
 static void
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 1462324..3ae5b4c 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -240,7 +240,7 @@ static int ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,
 				      struct rte_intr_handle *handle);
 static void ixgbe_dev_interrupt_handler(void *param);
 static void ixgbe_dev_interrupt_delayed_handler(void *param);
-static void ixgbe_add_rar(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
+static int ixgbe_add_rar(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		uint32_t index, uint32_t pool);
 static void ixgbe_remove_rar(struct rte_eth_dev *dev, uint32_t index);
 static void ixgbe_set_default_mac_addr(struct rte_eth_dev *dev,
@@ -297,7 +297,7 @@ static void ixgbe_configure_msix(struct rte_eth_dev *dev);
 static int ixgbe_set_queue_rate_limit(struct rte_eth_dev *dev,
 		uint16_t queue_idx, uint16_t tx_rate);
 
-static void ixgbevf_add_mac_addr(struct rte_eth_dev *dev,
+static int ixgbevf_add_mac_addr(struct rte_eth_dev *dev,
 				 struct ether_addr *mac_addr,
 				 uint32_t index, uint32_t pool);
 static void ixgbevf_remove_mac_addr(struct rte_eth_dev *dev, uint32_t index);
@@ -4356,14 +4356,15 @@ ixgbe_dev_rss_reta_query(struct rte_eth_dev *dev,
 	return 0;
 }
 
-static void
+static int
 ixgbe_add_rar(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 				uint32_t index, uint32_t pool)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	uint32_t enable_addr = 1;
 
-	ixgbe_set_rar(hw, index, mac_addr->addr_bytes, pool, enable_addr);
+	return ixgbe_set_rar(hw, index, mac_addr->addr_bytes,
+				pool, enable_addr);
 }
 
 static void
@@ -5912,7 +5913,7 @@ static int ixgbe_set_queue_rate_limit(struct rte_eth_dev *dev,
 	return 0;
 }
 
-static void
+static int
 ixgbevf_add_mac_addr(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		     __attribute__((unused)) uint32_t index,
 		     __attribute__((unused)) uint32_t pool)
@@ -5926,11 +5927,19 @@ ixgbevf_add_mac_addr(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 	 * set of PF resources used to store VF MAC addresses.
 	 */
 	if (memcmp(hw->mac.perm_addr, mac_addr, sizeof(struct ether_addr)) == 0)
-		return;
+		return -1;
 	diag = ixgbevf_set_uc_addr_vf(hw, 2, mac_addr->addr_bytes);
-	if (diag == 0)
-		return;
-	PMD_DRV_LOG(ERR, "Unable to add MAC address - diag=%d", diag);
+	if (diag != 0)
+		PMD_DRV_LOG(ERR, "Unable to add MAC address "
+				"%02x:%02x:%02x:%02x:%02x:%02x- diag=%d",
+				mac_addr->addr_bytes[0],
+				mac_addr->addr_bytes[1],
+				mac_addr->addr_bytes[2],
+				mac_addr->addr_bytes[3],
+				mac_addr->addr_bytes[4],
+				mac_addr->addr_bytes[5],
+				diag);
+	return diag;
 }
 
 static void
diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index aff9155..8cd643c 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -4475,26 +4475,30 @@ mlx4_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
  * @param vmdq
  *   VMDq pool index to associate address with (ignored).
  */
-static void
+static int
 mlx4_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		  uint32_t index, uint32_t vmdq)
 {
 	struct priv *priv = dev->data->dev_private;
+	int re;
 
 	if (mlx4_is_secondary())
-		return;
+		return -ENOTSUP;
 	(void)vmdq;
 	priv_lock(priv);
 	DEBUG("%p: adding MAC address at index %" PRIu32,
 	      (void *)dev, index);
 	/* Last array entry is reserved for broadcast. */
-	if (index >= (elemof(priv->mac) - 1))
-		goto end;
-	priv_mac_addr_add(priv, index,
-			  (const uint8_t (*)[ETHER_ADDR_LEN])
-			  mac_addr->addr_bytes);
+	if (index >= (elemof(priv->mac) - 1)) {
+		priv_unlock(priv);
+		return -EINVAL;
+	}
+	re = priv_mac_addr_add(priv, index,
+			       (const uint8_t (*)[ETHER_ADDR_LEN])
+			       mac_addr->addr_bytes);
 end:
 	priv_unlock(priv);
+	return -re;
 }
 
 /**
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index e8bf361..7acb33c 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -238,8 +238,8 @@ int hash_rxq_mac_addrs_add(struct hash_rxq *);
 int priv_mac_addr_add(struct priv *, unsigned int,
 		      const uint8_t (*)[ETHER_ADDR_LEN]);
 int priv_mac_addrs_enable(struct priv *);
-void mlx5_mac_addr_add(struct rte_eth_dev *, struct ether_addr *, uint32_t,
-		       uint32_t);
+int mlx5_mac_addr_add(struct rte_eth_dev *, struct ether_addr *, uint32_t,
+		      uint32_t);
 void mlx5_mac_addr_set(struct rte_eth_dev *, struct ether_addr *);
 
 /* mlx5_rss.c */
diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
index 4fcfd3b..7ab3a37 100644
--- a/drivers/net/mlx5/mlx5_mac.c
+++ b/drivers/net/mlx5/mlx5_mac.c
@@ -470,26 +470,30 @@ priv_mac_addrs_enable(struct priv *priv)
  * @param vmdq
  *   VMDq pool index to associate address with (ignored).
  */
-void
+int
 mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		  uint32_t index, uint32_t vmdq)
 {
 	struct priv *priv = dev->data->dev_private;
+	int re;
 
 	if (mlx5_is_secondary())
-		return;
+		return -ENOTSUP;
 
 	(void)vmdq;
 	priv_lock(priv);
 	DEBUG("%p: adding MAC address at index %" PRIu32,
 	      (void *)dev, index);
-	if (index >= RTE_DIM(priv->mac))
+	if (index >= RTE_DIM(priv->mac)) {
+		re = -EINVAL;
 		goto end;
-	priv_mac_addr_add(priv, index,
-			  (const uint8_t (*)[ETHER_ADDR_LEN])
-			  mac_addr->addr_bytes);
+	}
+	re = priv_mac_addr_add(priv, index,
+			       (const uint8_t (*)[ETHER_ADDR_LEN])
+			       mac_addr->addr_bytes);
 end:
 	priv_unlock(priv);
+	return -re;
 }
 
 /**
diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
index 5f469e5..cefb5c1 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -506,16 +506,18 @@ qede_mac_int_ops(struct rte_eth_dev *eth_dev, struct ecore_filter_ucast *ucast,
 	return rc;
 }
 
-static void
+static int
 qede_mac_addr_add(struct rte_eth_dev *eth_dev, struct ether_addr *mac_addr,
 		  uint32_t index, __rte_unused uint32_t pool)
 {
 	struct ecore_filter_ucast ucast;
+	int re;
 
 	qede_set_ucast_cmn_params(&ucast);
 	ucast.type = ECORE_FILTER_MAC;
 	ether_addr_copy(mac_addr, (struct ether_addr *)&ucast.mac);
-	(void)qede_mac_int_ops(eth_dev, &ucast, 1);
+	re = (int)qede_mac_int_ops(eth_dev, &ucast, 1);
+	return re;
 }
 
 static void
diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index 77ef3a1..4ca4ca8 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -224,12 +224,13 @@ eth_mac_addr_remove(struct rte_eth_dev *dev __rte_unused,
 {
 }
 
-static void
+static int
 eth_mac_addr_add(struct rte_eth_dev *dev __rte_unused,
 	struct ether_addr *mac_addr __rte_unused,
 	uint32_t index __rte_unused,
 	uint32_t vmdq __rte_unused)
 {
+	return -1;
 }
 
 static void
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 78cb3e8..8e8d8bf 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -86,7 +86,7 @@ static void virtio_dev_stats_reset(struct rte_eth_dev *dev);
 static void virtio_dev_free_mbufs(struct rte_eth_dev *dev);
 static int virtio_vlan_filter_set(struct rte_eth_dev *dev,
 				uint16_t vlan_id, int on);
-static void virtio_mac_addr_add(struct rte_eth_dev *dev,
+static int virtio_mac_addr_add(struct rte_eth_dev *dev,
 				struct ether_addr *mac_addr,
 				uint32_t index, uint32_t vmdq __rte_unused);
 static void virtio_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);
@@ -1019,7 +1019,7 @@ virtio_get_hwaddr(struct virtio_hw *hw)
 	}
 }
 
-static void
+static int
 virtio_mac_table_set(struct virtio_hw *hw,
 		     const struct virtio_net_ctrl_mac *uc,
 		     const struct virtio_net_ctrl_mac *mc)
@@ -1029,7 +1029,7 @@ virtio_mac_table_set(struct virtio_hw *hw,
 
 	if (!vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
 		PMD_DRV_LOG(INFO, "host does not support mac table");
-		return;
+		return -1;
 	}
 
 	ctrl.hdr.class = VIRTIO_NET_CTRL_MAC;
@@ -1044,9 +1044,10 @@ virtio_mac_table_set(struct virtio_hw *hw,
 	err = virtio_send_command(hw->cvq, &ctrl, len, 2);
 	if (err != 0)
 		PMD_DRV_LOG(NOTICE, "mac table set failed: %d", err);
+	return err;
 }
 
-static void
+static int
 virtio_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		    uint32_t index, uint32_t vmdq __rte_unused)
 {
@@ -1057,7 +1058,7 @@ virtio_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 
 	if (index >= VIRTIO_MAX_MAC_ADDRS) {
 		PMD_DRV_LOG(ERR, "mac address index %u out of range", index);
-		return;
+		return -EINVAL;
 	}
 
 	uc = alloca(VIRTIO_MAX_MAC_ADDRS * ETHER_ADDR_LEN + sizeof(uc->entries));
@@ -1074,7 +1075,7 @@ virtio_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 		memcpy(&tbl->macs[tbl->entries++], addr, ETHER_ADDR_LEN);
 	}
 
-	virtio_mac_table_set(hw, uc, mc);
+	return virtio_mac_table_set(hw, uc, mc);
 }
 
 static void
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index fa6ae44..a6937bd 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -2167,6 +2167,7 @@ rte_eth_dev_mac_addr_add(uint8_t port_id, struct ether_addr *addr,
 	struct rte_eth_dev *dev;
 	int index;
 	uint64_t pool_mask;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
@@ -2199,15 +2200,17 @@ rte_eth_dev_mac_addr_add(uint8_t port_id, struct ether_addr *addr,
 	}
 
 	/* Update NIC */
-	(*dev->dev_ops->mac_addr_add)(dev, addr, index, pool);
+	ret = (*dev->dev_ops->mac_addr_add)(dev, addr, index, pool);
 
-	/* Update address in NIC data structure */
-	ether_addr_copy(addr, &dev->data->mac_addrs[index]);
+	if (ret == 0) {
+		/* Update address in NIC data structure */
+		ether_addr_copy(addr, &dev->data->mac_addrs[index]);
 
-	/* Update pool bitmap in NIC data structure */
-	dev->data->mac_pool_sel[index] |= (1ULL << pool);
+		/* Update pool bitmap in NIC data structure */
+		dev->data->mac_pool_sel[index] |= (1ULL << pool);
+	}
 
-	return 0;
+	return ret;
 }
 
 int
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index d072538..08e6c13 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1277,7 +1277,7 @@ typedef int (*eth_dev_led_off_t)(struct rte_eth_dev *dev);
 typedef void (*eth_mac_addr_remove_t)(struct rte_eth_dev *dev, uint32_t index);
 /**< @internal Remove MAC address from receive address register */
 
-typedef void (*eth_mac_addr_add_t)(struct rte_eth_dev *dev,
+typedef int (*eth_mac_addr_add_t)(struct rte_eth_dev *dev,
 				  struct ether_addr *mac_addr,
 				  uint32_t index,
 				  uint32_t vmdq);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH v4 2/3] doc: change type of return value of adding MAC addr
  2017-04-12  9:02 [PATCH v3 0/3] MAC address fail to be added shouldn't be stored Wei Dai
                   ` (4 preceding siblings ...)
  2017-04-13  8:21 ` [PATCH v4 1/3] ethdev: fix adding invalid MAC addr Wei Dai
@ 2017-04-13  8:21 ` Wei Dai
  2017-04-14 16:03   ` Mcnamara, John
  2017-04-13  8:21 ` [PATCH v4 3/3] app/testpmd: add a command to add many MAC addrs Wei Dai
  6 siblings, 1 reply; 19+ messages in thread
From: Wei Dai @ 2017-04-13  8:21 UTC (permalink / raw)
  To: thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, wenzhuo.lu, helin.zhang, konstantin.ananyev,
	jingjing.wu, jing.d.chen, adrien.mazarguil, nelio.laranjeiro,
	bruce.richardson, yuanhan.liu, maxime.coquelin
  Cc: dev, Wei Dai

Add following lines in section of API change in release note.

If a MAC address fails to be added without this change, it is still stored
and may be regarded as a valid one. This may lead to errors in application.
The type of return value of eth_mac_addr_add_t in rte_ethdev.h is changed.
Any specific NIC also follows this change.

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 doc/guides/rel_notes/release_17_05.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/doc/guides/rel_notes/release_17_05.rst b/doc/guides/rel_notes/release_17_05.rst
index 4968b8f..c9ba484 100644
--- a/doc/guides/rel_notes/release_17_05.rst
+++ b/doc/guides/rel_notes/release_17_05.rst
@@ -438,6 +438,13 @@ ABI Changes
 * The ``rte_cryptodev_info.sym`` structure has new field ``max_nb_sessions_per_qp``
   to support drivers which may support limited number of sessions per queue_pair.
 
+* **Return if the MAC address is added successfully or not.**
+
+  If a MAC address fails to be added without this change, it is still stored
+  and may be regarded as a valid one. This may lead to errors in application.
+  The type of return value of eth_mac_addr_add_t in rte_ethdev.h is changed.
+  Any specific NIC also follows this change.
+
 
 Removed Items
 -------------
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [PATCH v4 3/3] app/testpmd: add a command to add many MAC addrs
  2017-04-12  9:02 [PATCH v3 0/3] MAC address fail to be added shouldn't be stored Wei Dai
                   ` (5 preceding siblings ...)
  2017-04-13  8:21 ` [PATCH v4 2/3] doc: change type of return value of adding " Wei Dai
@ 2017-04-13  8:21 ` Wei Dai
  6 siblings, 0 replies; 19+ messages in thread
From: Wei Dai @ 2017-04-13  8:21 UTC (permalink / raw)
  To: thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, wenzhuo.lu, helin.zhang, konstantin.ananyev,
	jingjing.wu, jing.d.chen, adrien.mazarguil, nelio.laranjeiro,
	bruce.richardson, yuanhan.liu, maxime.coquelin
  Cc: dev, Wei Dai

This patch is added to introduce a testpmd command which
is used to add more than one MAC addresses one time.
This command can simplify the test for the change where
the type of return value of adding MAC address.
Normally a MAC address may fails to be added only after
many MAC addresses have been added.
Without this command, a tester may only trigger failed
MAC address by running many times of testpmd command
'mac_addr add' .

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 app/test-pmd/cmdline.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index e0d54d4..0aa9ccc 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -6424,6 +6424,60 @@ cmdline_parse_inst_t cmd_mac_addr = {
 	},
 };
 
+/* *** ADD MORE THAN ONE MAC ADDRESS FROM A PORT *** */
+struct cmd_add_more_mac_addr_result {
+	cmdline_fixed_string_t mac_addr_cmd;
+	uint8_t port_num;
+	struct ether_addr address;
+	uint8_t cnt_addr;
+};
+
+static void cmd_add_more_mac_addr_parsed(void *parsed_result,
+		__attribute__((unused)) struct cmdline *cl,
+		__attribute__((unused)) void *data)
+{
+	struct cmd_add_more_mac_addr_result *res = parsed_result;
+	int ret;
+	int k;
+
+	for (k = 0; k < res->cnt_addr; k++) {
+		ret = rte_eth_dev_mac_addr_add(res->port_num, &res->address, 0);
+		if (ret  < 0) {
+			printf("Fail to add mac addr : (%s) after adding %u addresses\n",
+				strerror(-ret), k);
+			return;
+		}
+		res->address.addr_bytes[5]++;
+	}
+	printf("Success to add %u mac addresses\n", k);
+}
+
+cmdline_parse_token_string_t cmd_add_more_mac_addr_cmd =
+	TOKEN_STRING_INITIALIZER(struct cmd_add_more_mac_addr_result,
+		mac_addr_cmd, "add_more_mac_addr");
+cmdline_parse_token_num_t cmd_add_more_mac_addr_portnum =
+	TOKEN_NUM_INITIALIZER(struct cmd_add_more_mac_addr_result,
+		port_num, UINT8);
+cmdline_parse_token_etheraddr_t cmd_add_more_mac_addr_addr =
+	TOKEN_ETHERADDR_INITIALIZER(struct cmd_add_more_mac_addr_result,
+		address);
+cmdline_parse_token_num_t cmd_add_more_mac_addr_cnt_addr =
+	TOKEN_NUM_INITIALIZER(struct cmd_add_more_mac_addr_result,
+		cnt_addr, UINT8);
+
+cmdline_parse_inst_t cmd_add_more_mac_addr = {
+	.f = cmd_add_more_mac_addr_parsed,
+	.data = (void *)0,
+	.help_str = "add_more_mac_addr <port_id> <mac_addr> <cnt_addr>: "
+			"Add cnt_addr MAC addresses on port_id",
+	.tokens = {
+		(void *)&cmd_add_more_mac_addr_cmd,
+		(void *)&cmd_add_more_mac_addr_portnum,
+		(void *)&cmd_add_more_mac_addr_addr,
+		(void *)&cmd_add_more_mac_addr_cnt_addr,
+		NULL,
+	},
+};
 
 /* *** CONFIGURE QUEUE STATS COUNTER MAPPINGS *** */
 struct cmd_set_qmap_result {
@@ -13244,6 +13298,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_read_rxd_txd,
 	(cmdline_parse_inst_t *)&cmd_stop,
 	(cmdline_parse_inst_t *)&cmd_mac_addr,
+	(cmdline_parse_inst_t *)&cmd_add_more_mac_addr,
 	(cmdline_parse_inst_t *)&cmd_set_qmap,
 	(cmdline_parse_inst_t *)&cmd_operate_port,
 	(cmdline_parse_inst_t *)&cmd_operate_specific_port,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
  2017-04-13  8:21 ` [PATCH v4 1/3] ethdev: fix adding invalid MAC addr Wei Dai
@ 2017-04-13  8:44   ` Nélio Laranjeiro
  2017-04-13  9:22     ` Dai, Wei
  2017-04-20  5:31   ` Yuanhan Liu
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: Nélio Laranjeiro @ 2017-04-13  8:44 UTC (permalink / raw)
  To: Wei Dai
  Cc: thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, wenzhuo.lu, helin.zhang, konstantin.ananyev,
	jingjing.wu, jing.d.chen, adrien.mazarguil, bruce.richardson,
	yuanhan.liu, maxime.coquelin, dev, stable

On Thu, Apr 13, 2017 at 04:21:04PM +0800, Wei Dai wrote:
> some customers find adding mac addr to VF sometimes can fail,
> but it is still stored in dev->data->mac_addrs[ ]. So this
> can lead to some errors that assumes the non-zero entry in
> dev->data->mac_addrs[ ] is valid.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>
> ---
>  drivers/net/bnx2x/bnx2x_ethdev.c   |  7 +++++--
>  drivers/net/bnxt/bnxt_ethdev.c     | 12 ++++++------
>  drivers/net/e1000/base/e1000_api.c |  2 +-
>  drivers/net/e1000/em_ethdev.c      |  6 +++---
>  drivers/net/e1000/igb_ethdev.c     |  5 +++--
>  drivers/net/enic/enic.h            |  2 +-
>  drivers/net/enic/enic_ethdev.c     |  4 ++--
>  drivers/net/enic/enic_main.c       |  6 +++---
>  drivers/net/fm10k/fm10k_ethdev.c   |  3 ++-
>  drivers/net/i40e/i40e_ethdev.c     | 11 ++++++-----
>  drivers/net/i40e/i40e_ethdev_vf.c  |  8 ++++----
>  drivers/net/ixgbe/ixgbe_ethdev.c   | 27 ++++++++++++++++++---------
>  drivers/net/mlx4/mlx4.c            | 18 +++++++++++-------
>  drivers/net/mlx5/mlx5.h            |  4 ++--
>  drivers/net/mlx5/mlx5_mac.c        | 16 ++++++++++------
>  drivers/net/qede/qede_ethdev.c     |  6 ++++--
>  drivers/net/ring/rte_eth_ring.c    |  3 ++-
>  drivers/net/virtio/virtio_ethdev.c | 13 +++++++------
>  lib/librte_ether/rte_ethdev.c      | 15 +++++++++------
>  lib/librte_ether/rte_ethdev.h      |  2 +-
>  20 files changed, 100 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/net/bnx2x/bnx2x_ethdev.c b/drivers/net/bnx2x/bnx2x_ethdev.c
> index 0e8b4d9..425b48d 100644
> --- a/drivers/net/bnx2x/bnx2x_ethdev.c
> +++ b/drivers/net/bnx2x/bnx2x_ethdev.c
> @@ -450,14 +450,17 @@ bnx2x_dev_infos_get(struct rte_eth_dev *dev, __rte_unused struct rte_eth_dev_inf
>  	dev_info->speed_capa = ETH_LINK_SPEED_10G | ETH_LINK_SPEED_20G;
>  }
>  
> -static void
> +static int
>  bnx2x_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  		uint32_t index, uint32_t pool)
>  {
>  	struct bnx2x_softc *sc = dev->data->dev_private;
>  
> -	if (sc->mac_ops.mac_addr_add)
> +	if (sc->mac_ops.mac_addr_add) {
>  		sc->mac_ops.mac_addr_add(dev, mac_addr, index, pool);
> +		return 0;
> +	}
> +	return -ENOTSUP;
>  }
>  
>  static void
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index 6167443..9cb2667 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -613,7 +613,7 @@ static void bnxt_mac_addr_remove_op(struct rte_eth_dev *eth_dev,
>  	}
>  }
>  
> -static void bnxt_mac_addr_add_op(struct rte_eth_dev *eth_dev,
> +static int bnxt_mac_addr_add_op(struct rte_eth_dev *eth_dev,
>  				 struct ether_addr *mac_addr,
>  				 uint32_t index, uint32_t pool)
>  {
> @@ -623,30 +623,30 @@ static void bnxt_mac_addr_add_op(struct rte_eth_dev *eth_dev,
>  
>  	if (BNXT_VF(bp)) {
>  		RTE_LOG(ERR, PMD, "Cannot add MAC address to a VF interface\n");
> -		return;
> +		return -ENOTSUP;
>  	}
>  
>  	if (!vnic) {
>  		RTE_LOG(ERR, PMD, "VNIC not found for pool %d!\n", pool);
> -		return;
> +		return -EINVAL;
>  	}
>  	/* Attach requested MAC address to the new l2_filter */
>  	STAILQ_FOREACH(filter, &vnic->filter, next) {
>  		if (filter->mac_index == index) {
>  			RTE_LOG(ERR, PMD,
>  				"MAC addr already existed for pool %d\n", pool);
> -			return;
> +			return -EINVAL;
>  		}
>  	}
>  	filter = bnxt_alloc_filter(bp);
>  	if (!filter) {
>  		RTE_LOG(ERR, PMD, "L2 filter alloc failed\n");
> -		return;
> +		return -ENODEV;
>  	}
>  	STAILQ_INSERT_TAIL(&vnic->filter, filter, next);
>  	filter->mac_index = index;
>  	memcpy(filter->l2_addr, mac_addr, ETHER_ADDR_LEN);
> -	bnxt_hwrm_set_filter(bp, vnic, filter);
> +	return bnxt_hwrm_set_filter(bp, vnic, filter);
>  }
>  
>  int bnxt_link_update_op(struct rte_eth_dev *eth_dev, int wait_to_complete)
> diff --git a/drivers/net/e1000/base/e1000_api.c b/drivers/net/e1000/base/e1000_api.c
> index f7cf83b..dcb53f7 100644
> --- a/drivers/net/e1000/base/e1000_api.c
> +++ b/drivers/net/e1000/base/e1000_api.c
> @@ -855,7 +855,7 @@ int e1000_rar_set(struct e1000_hw *hw, u8 *addr, u32 index)
>  	if (hw->mac.ops.rar_set)
>  		return hw->mac.ops.rar_set(hw, addr, index);
>  
> -	return E1000_SUCCESS;
> +	return E1000_ERR_NO_SPACE;
>  }
>  
>  /**
> diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
> index ecefa56..3a80856 100644
> --- a/drivers/net/e1000/em_ethdev.c
> +++ b/drivers/net/e1000/em_ethdev.c
> @@ -119,7 +119,7 @@ static int eth_em_led_on(struct rte_eth_dev *dev);
>  static int eth_em_led_off(struct rte_eth_dev *dev);
>  
>  static int em_get_rx_buffer_size(struct e1000_hw *hw);
> -static void eth_em_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
> +static int eth_em_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  		uint32_t index, uint32_t pool);
>  static void eth_em_rar_clear(struct rte_eth_dev *dev, uint32_t index);
>  
> @@ -1786,13 +1786,13 @@ eth_em_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf)
>  	return -EIO;
>  }
>  
> -static void
> +static int
>  eth_em_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  		uint32_t index, __rte_unused uint32_t pool)
>  {
>  	struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>  
> -	e1000_rar_set(hw, mac_addr->addr_bytes, index);
> +	return e1000_rar_set(hw, mac_addr->addr_bytes, index);
>  }
>  
>  static void
> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
> index cc2c244..3d95ec4 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -164,7 +164,7 @@ static int eth_igb_led_off(struct rte_eth_dev *dev);
>  
>  static void igb_intr_disable(struct e1000_hw *hw);
>  static int  igb_get_rx_buffer_size(struct e1000_hw *hw);
> -static void eth_igb_rar_set(struct rte_eth_dev *dev,
> +static int eth_igb_rar_set(struct rte_eth_dev *dev,
>  		struct ether_addr *mac_addr,
>  		uint32_t index, uint32_t pool);
>  static void eth_igb_rar_clear(struct rte_eth_dev *dev, uint32_t index);
> @@ -2973,7 +2973,7 @@ eth_igb_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf)
>  }
>  
>  #define E1000_RAH_POOLSEL_SHIFT      (18)
> -static void
> +static int
>  eth_igb_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  	        uint32_t index, __rte_unused uint32_t pool)
>  {
> @@ -2984,6 +2984,7 @@ eth_igb_rar_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  	rah = E1000_READ_REG(hw, E1000_RAH(index));
>  	rah |= (0x1 << (E1000_RAH_POOLSEL_SHIFT + pool));
>  	E1000_WRITE_REG(hw, E1000_RAH(index), rah);
> +	return 0;
>  }
>  
>  static void
> diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
> index e921de4..d17a35f 100644
> --- a/drivers/net/enic/enic.h
> +++ b/drivers/net/enic/enic.h
> @@ -279,7 +279,7 @@ extern void enic_dev_stats_get(struct enic *enic,
>  	struct rte_eth_stats *r_stats);
>  extern void enic_dev_stats_clear(struct enic *enic);
>  extern void enic_add_packet_filter(struct enic *enic);
> -void enic_set_mac_address(struct enic *enic, uint8_t *mac_addr);
> +int enic_set_mac_address(struct enic *enic, uint8_t *mac_addr);
>  void enic_del_mac_address(struct enic *enic, int mac_index);
>  extern unsigned int enic_cleanup_wq(struct enic *enic, struct vnic_wq *wq);
>  extern void enic_send_pkt(struct enic *enic, struct vnic_wq *wq,
> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> index 2c2e29e..9ef9c31 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -532,14 +532,14 @@ static void enicpmd_dev_allmulticast_disable(struct rte_eth_dev *eth_dev)
>  	enic_add_packet_filter(enic);
>  }
>  
> -static void enicpmd_add_mac_addr(struct rte_eth_dev *eth_dev,
> +static int enicpmd_add_mac_addr(struct rte_eth_dev *eth_dev,
>  	struct ether_addr *mac_addr,
>  	__rte_unused uint32_t index, __rte_unused uint32_t pool)
>  {
>  	struct enic *enic = pmd_priv(eth_dev);
>  
>  	ENICPMD_FUNC_TRACE();
> -	enic_set_mac_address(enic, mac_addr->addr_bytes);
> +	return enic_set_mac_address(enic, mac_addr->addr_bytes);
>  }
>  
>  static void enicpmd_remove_mac_addr(struct rte_eth_dev *eth_dev, uint32_t index)
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
> index 5f2188b..c6892e9 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -202,20 +202,20 @@ void enic_del_mac_address(struct enic *enic, int mac_index)
>  		dev_err(enic, "del mac addr failed\n");
>  }
>  
> -void enic_set_mac_address(struct enic *enic, uint8_t *mac_addr)
> +int enic_set_mac_address(struct enic *enic, uint8_t *mac_addr)
>  {
>  	int err;
>  
>  	if (!is_eth_addr_valid(mac_addr)) {
>  		dev_err(enic, "invalid mac address\n");
> -		return;
> +		return -1;
>  	}
>  
>  	err = vnic_dev_add_addr(enic->vdev, mac_addr);
>  	if (err) {
>  		dev_err(enic, "add mac addr failed\n");
> -		return;
>  	}
> +	return err;
>  }
>  
>  static void
> diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
> index de0352e..dfc58fc 100644
> --- a/drivers/net/fm10k/fm10k_ethdev.c
> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> @@ -1689,7 +1689,7 @@ static void fm10k_MAC_filter_set(struct rte_eth_dev *dev,
>  }
>  
>  /* Add a MAC address, and update filters */
> -static void
> +static int
>  fm10k_macaddr_add(struct rte_eth_dev *dev,
>  		struct ether_addr *mac_addr,
>  		uint32_t index,
> @@ -1700,6 +1700,7 @@ fm10k_macaddr_add(struct rte_eth_dev *dev,
>  	macvlan = FM10K_DEV_PRIVATE_TO_MACVLAN(dev->data->dev_private);
>  	fm10k_MAC_filter_set(dev, mac_addr->addr_bytes, TRUE, pool);
>  	macvlan->mac_vmdq_id[index] = pool;
> +	return 0;
>  }
>  
>  /* Remove a MAC address, and update filters */
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 6927fde..173aa87 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -300,7 +300,7 @@ static int i40e_flow_ctrl_set(struct rte_eth_dev *dev,
>  			      struct rte_eth_fc_conf *fc_conf);
>  static int i40e_priority_flow_ctrl_set(struct rte_eth_dev *dev,
>  				       struct rte_eth_pfc_conf *pfc_conf);
> -static void i40e_macaddr_add(struct rte_eth_dev *dev,
> +static int i40e_macaddr_add(struct rte_eth_dev *dev,
>  			  struct ether_addr *mac_addr,
>  			  uint32_t index,
>  			  uint32_t pool);
> @@ -3269,7 +3269,7 @@ i40e_priority_flow_ctrl_set(__rte_unused struct rte_eth_dev *dev,
>  }
>  
>  /* Add a MAC address, and update filters */
> -static void
> +static int
>  i40e_macaddr_add(struct rte_eth_dev *dev,
>  		 struct ether_addr *mac_addr,
>  		 __rte_unused uint32_t index,
> @@ -3286,13 +3286,13 @@ i40e_macaddr_add(struct rte_eth_dev *dev,
>  		PMD_DRV_LOG(ERR, "VMDQ not %s, can't set mac to pool %u",
>  			pf->flags & I40E_FLAG_VMDQ ? "configured" : "enabled",
>  			pool);
> -		return;
> +		return -ENOTSUP;
>  	}
>  
>  	if (pool > pf->nb_cfg_vmdq_vsi) {
>  		PMD_DRV_LOG(ERR, "Pool number %u invalid. Max pool is %u",
>  				pool, pf->nb_cfg_vmdq_vsi);
> -		return;
> +		return -EINVAL;
>  	}
>  
>  	(void)rte_memcpy(&mac_filter.mac_addr, mac_addr, ETHER_ADDR_LEN);
> @@ -3309,8 +3309,9 @@ i40e_macaddr_add(struct rte_eth_dev *dev,
>  	ret = i40e_vsi_add_mac(vsi, &mac_filter);
>  	if (ret != I40E_SUCCESS) {
>  		PMD_DRV_LOG(ERR, "Failed to add MACVLAN filter");
> -		return;
> +		return -ENODEV;
>  	}
> +	return 0;
>  }
>  
>  /* Remove a MAC address, and update filters */
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
> index 7e48fea..fcccc46 100644
> --- a/drivers/net/i40e/i40e_ethdev_vf.c
> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> @@ -135,7 +135,7 @@ static int i40evf_dev_tx_queue_start(struct rte_eth_dev *dev,
>  				     uint16_t tx_queue_id);
>  static int i40evf_dev_tx_queue_stop(struct rte_eth_dev *dev,
>  				    uint16_t tx_queue_id);
> -static void i40evf_add_mac_addr(struct rte_eth_dev *dev,
> +static int i40evf_add_mac_addr(struct rte_eth_dev *dev,
>  				struct ether_addr *addr,
>  				uint32_t index,
>  				uint32_t pool);
> @@ -854,7 +854,7 @@ i40evf_stop_queues(struct rte_eth_dev *dev)
>  	return 0;
>  }
>  
> -static void
> +static int
>  i40evf_add_mac_addr(struct rte_eth_dev *dev,
>  		    struct ether_addr *addr,
>  		    __rte_unused uint32_t index,
> @@ -872,7 +872,7 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
>  			    addr->addr_bytes[0], addr->addr_bytes[1],
>  			    addr->addr_bytes[2], addr->addr_bytes[3],
>  			    addr->addr_bytes[4], addr->addr_bytes[5]);
> -		return;
> +		return I40E_ERR_INVALID_MAC_ADDR;
>  	}
>  
>  	list = (struct i40e_virtchnl_ether_addr_list *)cmd_buffer;
> @@ -891,7 +891,7 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
>  		PMD_DRV_LOG(ERR, "fail to execute command "
>  			    "OP_ADD_ETHER_ADDRESS");
>  
> -	return;
> +	return err;
>  }
>  
>  static void
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 1462324..3ae5b4c 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -240,7 +240,7 @@ static int ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,
>  				      struct rte_intr_handle *handle);
>  static void ixgbe_dev_interrupt_handler(void *param);
>  static void ixgbe_dev_interrupt_delayed_handler(void *param);
> -static void ixgbe_add_rar(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
> +static int ixgbe_add_rar(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  		uint32_t index, uint32_t pool);
>  static void ixgbe_remove_rar(struct rte_eth_dev *dev, uint32_t index);
>  static void ixgbe_set_default_mac_addr(struct rte_eth_dev *dev,
> @@ -297,7 +297,7 @@ static void ixgbe_configure_msix(struct rte_eth_dev *dev);
>  static int ixgbe_set_queue_rate_limit(struct rte_eth_dev *dev,
>  		uint16_t queue_idx, uint16_t tx_rate);
>  
> -static void ixgbevf_add_mac_addr(struct rte_eth_dev *dev,
> +static int ixgbevf_add_mac_addr(struct rte_eth_dev *dev,
>  				 struct ether_addr *mac_addr,
>  				 uint32_t index, uint32_t pool);
>  static void ixgbevf_remove_mac_addr(struct rte_eth_dev *dev, uint32_t index);
> @@ -4356,14 +4356,15 @@ ixgbe_dev_rss_reta_query(struct rte_eth_dev *dev,
>  	return 0;
>  }
>  
> -static void
> +static int
>  ixgbe_add_rar(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  				uint32_t index, uint32_t pool)
>  {
>  	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>  	uint32_t enable_addr = 1;
>  
> -	ixgbe_set_rar(hw, index, mac_addr->addr_bytes, pool, enable_addr);
> +	return ixgbe_set_rar(hw, index, mac_addr->addr_bytes,
> +				pool, enable_addr);
>  }
>  
>  static void
> @@ -5912,7 +5913,7 @@ static int ixgbe_set_queue_rate_limit(struct rte_eth_dev *dev,
>  	return 0;
>  }
>  
> -static void
> +static int
>  ixgbevf_add_mac_addr(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  		     __attribute__((unused)) uint32_t index,
>  		     __attribute__((unused)) uint32_t pool)
> @@ -5926,11 +5927,19 @@ ixgbevf_add_mac_addr(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  	 * set of PF resources used to store VF MAC addresses.
>  	 */
>  	if (memcmp(hw->mac.perm_addr, mac_addr, sizeof(struct ether_addr)) == 0)
> -		return;
> +		return -1;
>  	diag = ixgbevf_set_uc_addr_vf(hw, 2, mac_addr->addr_bytes);
> -	if (diag == 0)
> -		return;
> -	PMD_DRV_LOG(ERR, "Unable to add MAC address - diag=%d", diag);
> +	if (diag != 0)
> +		PMD_DRV_LOG(ERR, "Unable to add MAC address "
> +				"%02x:%02x:%02x:%02x:%02x:%02x- diag=%d",
> +				mac_addr->addr_bytes[0],
> +				mac_addr->addr_bytes[1],
> +				mac_addr->addr_bytes[2],
> +				mac_addr->addr_bytes[3],
> +				mac_addr->addr_bytes[4],
> +				mac_addr->addr_bytes[5],
> +				diag);
> +	return diag;
>  }
>  
>  static void
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index aff9155..8cd643c 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -4475,26 +4475,30 @@ mlx4_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
>   * @param vmdq
>   *   VMDq pool index to associate address with (ignored).
>   */
> -static void
> +static int
>  mlx4_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  		  uint32_t index, uint32_t vmdq)
>  {
>  	struct priv *priv = dev->data->dev_private;
> +	int re;
>  
>  	if (mlx4_is_secondary())
> -		return;
> +		return -ENOTSUP;
>  	(void)vmdq;
>  	priv_lock(priv);
>  	DEBUG("%p: adding MAC address at index %" PRIu32,
>  	      (void *)dev, index);
>  	/* Last array entry is reserved for broadcast. */
> -	if (index >= (elemof(priv->mac) - 1))
> -		goto end;
> -	priv_mac_addr_add(priv, index,
> -			  (const uint8_t (*)[ETHER_ADDR_LEN])
> -			  mac_addr->addr_bytes);
> +	if (index >= (elemof(priv->mac) - 1)) {
> +		priv_unlock(priv);
> +		return -EINVAL;
> +	}
> +	re = priv_mac_addr_add(priv, index,
> +			       (const uint8_t (*)[ETHER_ADDR_LEN])
> +			       mac_addr->addr_bytes);
>  end:
>  	priv_unlock(priv);
> +	return -re;
>  }
>  
>  /**
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index e8bf361..7acb33c 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -238,8 +238,8 @@ int hash_rxq_mac_addrs_add(struct hash_rxq *);
>  int priv_mac_addr_add(struct priv *, unsigned int,
>  		      const uint8_t (*)[ETHER_ADDR_LEN]);
>  int priv_mac_addrs_enable(struct priv *);
> -void mlx5_mac_addr_add(struct rte_eth_dev *, struct ether_addr *, uint32_t,
> -		       uint32_t);
> +int mlx5_mac_addr_add(struct rte_eth_dev *, struct ether_addr *, uint32_t,
> +		      uint32_t);
>  void mlx5_mac_addr_set(struct rte_eth_dev *, struct ether_addr *);
>  
>  /* mlx5_rss.c */
> diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
> index 4fcfd3b..7ab3a37 100644
> --- a/drivers/net/mlx5/mlx5_mac.c
> +++ b/drivers/net/mlx5/mlx5_mac.c
> @@ -470,26 +470,30 @@ priv_mac_addrs_enable(struct priv *priv)
>   * @param vmdq
>   *   VMDq pool index to associate address with (ignored).
>   */
> -void
> +int
>  mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  		  uint32_t index, uint32_t vmdq)
>  {
>  	struct priv *priv = dev->data->dev_private;
> +	int re;
>  
>  	if (mlx5_is_secondary())
> -		return;
> +		return -ENOTSUP;
>  
>  	(void)vmdq;
>  	priv_lock(priv);
>  	DEBUG("%p: adding MAC address at index %" PRIu32,
>  	      (void *)dev, index);
> -	if (index >= RTE_DIM(priv->mac))
> +	if (index >= RTE_DIM(priv->mac)) {
> +		re = -EINVAL;
>  		goto end;
> -	priv_mac_addr_add(priv, index,
> -			  (const uint8_t (*)[ETHER_ADDR_LEN])
> -			  mac_addr->addr_bytes);
> +	}
> +	re = priv_mac_addr_add(priv, index,
> +			       (const uint8_t (*)[ETHER_ADDR_LEN])
> +			       mac_addr->addr_bytes);
>  end:
>  	priv_unlock(priv);
> +	return -re;
>  }
>  
>  /**
> diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
> index 5f469e5..cefb5c1 100644
> --- a/drivers/net/qede/qede_ethdev.c
> +++ b/drivers/net/qede/qede_ethdev.c
> @@ -506,16 +506,18 @@ qede_mac_int_ops(struct rte_eth_dev *eth_dev, struct ecore_filter_ucast *ucast,
>  	return rc;
>  }
>  
> -static void
> +static int
>  qede_mac_addr_add(struct rte_eth_dev *eth_dev, struct ether_addr *mac_addr,
>  		  uint32_t index, __rte_unused uint32_t pool)
>  {
>  	struct ecore_filter_ucast ucast;
> +	int re;
>  
>  	qede_set_ucast_cmn_params(&ucast);
>  	ucast.type = ECORE_FILTER_MAC;
>  	ether_addr_copy(mac_addr, (struct ether_addr *)&ucast.mac);
> -	(void)qede_mac_int_ops(eth_dev, &ucast, 1);
> +	re = (int)qede_mac_int_ops(eth_dev, &ucast, 1);
> +	return re;
>  }
>  
>  static void
> diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
> index 77ef3a1..4ca4ca8 100644
> --- a/drivers/net/ring/rte_eth_ring.c
> +++ b/drivers/net/ring/rte_eth_ring.c
> @@ -224,12 +224,13 @@ eth_mac_addr_remove(struct rte_eth_dev *dev __rte_unused,
>  {
>  }
>  
> -static void
> +static int
>  eth_mac_addr_add(struct rte_eth_dev *dev __rte_unused,
>  	struct ether_addr *mac_addr __rte_unused,
>  	uint32_t index __rte_unused,
>  	uint32_t vmdq __rte_unused)
>  {
> +	return -1;
>  }
>  
>  static void
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 78cb3e8..8e8d8bf 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -86,7 +86,7 @@ static void virtio_dev_stats_reset(struct rte_eth_dev *dev);
>  static void virtio_dev_free_mbufs(struct rte_eth_dev *dev);
>  static int virtio_vlan_filter_set(struct rte_eth_dev *dev,
>  				uint16_t vlan_id, int on);
> -static void virtio_mac_addr_add(struct rte_eth_dev *dev,
> +static int virtio_mac_addr_add(struct rte_eth_dev *dev,
>  				struct ether_addr *mac_addr,
>  				uint32_t index, uint32_t vmdq __rte_unused);
>  static void virtio_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);
> @@ -1019,7 +1019,7 @@ virtio_get_hwaddr(struct virtio_hw *hw)
>  	}
>  }
>  
> -static void
> +static int
>  virtio_mac_table_set(struct virtio_hw *hw,
>  		     const struct virtio_net_ctrl_mac *uc,
>  		     const struct virtio_net_ctrl_mac *mc)
> @@ -1029,7 +1029,7 @@ virtio_mac_table_set(struct virtio_hw *hw,
>  
>  	if (!vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
>  		PMD_DRV_LOG(INFO, "host does not support mac table");
> -		return;
> +		return -1;
>  	}
>  
>  	ctrl.hdr.class = VIRTIO_NET_CTRL_MAC;
> @@ -1044,9 +1044,10 @@ virtio_mac_table_set(struct virtio_hw *hw,
>  	err = virtio_send_command(hw->cvq, &ctrl, len, 2);
>  	if (err != 0)
>  		PMD_DRV_LOG(NOTICE, "mac table set failed: %d", err);
> +	return err;
>  }
>  
> -static void
> +static int
>  virtio_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  		    uint32_t index, uint32_t vmdq __rte_unused)
>  {
> @@ -1057,7 +1058,7 @@ virtio_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  
>  	if (index >= VIRTIO_MAX_MAC_ADDRS) {
>  		PMD_DRV_LOG(ERR, "mac address index %u out of range", index);
> -		return;
> +		return -EINVAL;
>  	}
>  
>  	uc = alloca(VIRTIO_MAX_MAC_ADDRS * ETHER_ADDR_LEN + sizeof(uc->entries));
> @@ -1074,7 +1075,7 @@ virtio_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
>  		memcpy(&tbl->macs[tbl->entries++], addr, ETHER_ADDR_LEN);
>  	}
>  
> -	virtio_mac_table_set(hw, uc, mc);
> +	return virtio_mac_table_set(hw, uc, mc);
>  }
>  
>  static void
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index fa6ae44..a6937bd 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2167,6 +2167,7 @@ rte_eth_dev_mac_addr_add(uint8_t port_id, struct ether_addr *addr,
>  	struct rte_eth_dev *dev;
>  	int index;
>  	uint64_t pool_mask;
> +	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  	dev = &rte_eth_devices[port_id];
> @@ -2199,15 +2200,17 @@ rte_eth_dev_mac_addr_add(uint8_t port_id, struct ether_addr *addr,
>  	}
>  
>  	/* Update NIC */
> -	(*dev->dev_ops->mac_addr_add)(dev, addr, index, pool);
> +	ret = (*dev->dev_ops->mac_addr_add)(dev, addr, index, pool);
>  
> -	/* Update address in NIC data structure */
> -	ether_addr_copy(addr, &dev->data->mac_addrs[index]);
> +	if (ret == 0) {
> +		/* Update address in NIC data structure */
> +		ether_addr_copy(addr, &dev->data->mac_addrs[index]);
>  
> -	/* Update pool bitmap in NIC data structure */
> -	dev->data->mac_pool_sel[index] |= (1ULL << pool);
> +		/* Update pool bitmap in NIC data structure */
> +		dev->data->mac_pool_sel[index] |= (1ULL << pool);
> +	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  int
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index d072538..08e6c13 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -1277,7 +1277,7 @@ typedef int (*eth_dev_led_off_t)(struct rte_eth_dev *dev);
>  typedef void (*eth_mac_addr_remove_t)(struct rte_eth_dev *dev, uint32_t index);
>  /**< @internal Remove MAC address from receive address register */
>  
> -typedef void (*eth_mac_addr_add_t)(struct rte_eth_dev *dev,
> +typedef int (*eth_mac_addr_add_t)(struct rte_eth_dev *dev,
>  				  struct ether_addr *mac_addr,
>  				  uint32_t index,
>  				  uint32_t vmdq);
> -- 
> 2.7.4
> 


For mlx changes,

Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

-- 
Nélio Laranjeiro
6WIND

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
  2017-04-13  8:44   ` Nélio Laranjeiro
@ 2017-04-13  9:22     ` Dai, Wei
  0 siblings, 0 replies; 19+ messages in thread
From: Dai, Wei @ 2017-04-13  9:22 UTC (permalink / raw)
  To: Nélio Laranjeiro
  Cc: thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, Lu, Wenzhuo, Zhang, Helin, Ananyev, Konstantin,
	Wu, Jingjing, Chen, Jing D, adrien.mazarguil, Richardson, Bruce,
	yuanhan.liu, maxime.coquelin, dev, stable

Thanks,  Nelio Laranjeiro.
What about other parts, dear maintainers ?


> -----Original Message-----
> From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> Sent: Thursday, April 13, 2017 4:45 PM
> To: Dai, Wei <wei.dai@intel.com>
> Cc: thomas.monjalon@6wind.com; harish.patil@cavium.com;
> rasesh.mody@cavium.com; stephen.hurd@broadcom.com;
> ajit.khaparde@broadcom.com; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang,
> Helin <helin.zhang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Chen,
> Jing D <jing.d.chen@intel.com>; adrien.mazarguil@6wind.com; Richardson,
> Bruce <bruce.richardson@intel.com>; yuanhan.liu@linux.intel.com;
> maxime.coquelin@redhat.com; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
> 
> On Thu, Apr 13, 2017 at 04:21:04PM +0800, Wei Dai wrote:
> > some customers find adding mac addr to VF sometimes can fail, but it
> > is still stored in dev->data->mac_addrs[ ]. So this can lead to some
> > errors that assumes the non-zero entry in
> > dev->data->mac_addrs[ ] is valid.
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wei Dai <wei.dai@intel.com>
> > ---
> >  drivers/net/bnx2x/bnx2x_ethdev.c   |  7 +++++--
> >  drivers/net/bnxt/bnxt_ethdev.c     | 12 ++++++------
> >  drivers/net/e1000/base/e1000_api.c |  2 +-
> >  drivers/net/e1000/em_ethdev.c      |  6 +++---
> >  drivers/net/e1000/igb_ethdev.c     |  5 +++--
> >  drivers/net/enic/enic.h            |  2 +-
> >  drivers/net/enic/enic_ethdev.c     |  4 ++--
> >  drivers/net/enic/enic_main.c       |  6 +++---
> >  drivers/net/fm10k/fm10k_ethdev.c   |  3 ++-
> >  drivers/net/i40e/i40e_ethdev.c     | 11 ++++++-----
> >  drivers/net/i40e/i40e_ethdev_vf.c  |  8 ++++----
> >  drivers/net/ixgbe/ixgbe_ethdev.c   | 27 ++++++++++++++++++---------
> >  drivers/net/mlx4/mlx4.c            | 18 +++++++++++-------
> >  drivers/net/mlx5/mlx5.h            |  4 ++--
> >  drivers/net/mlx5/mlx5_mac.c        | 16 ++++++++++------
> >  drivers/net/qede/qede_ethdev.c     |  6 ++++--
> >  drivers/net/ring/rte_eth_ring.c    |  3 ++-
> >  drivers/net/virtio/virtio_ethdev.c | 13 +++++++------
> >  lib/librte_ether/rte_ethdev.c      | 15 +++++++++------
> >  lib/librte_ether/rte_ethdev.h      |  2 +-
> >  20 files changed, 100 insertions(+), 70 deletions(-)
> >
> 
> 
> For mlx changes,
> 
> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> 
> --
> Nélio Laranjeiro
> 6WIND

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v4 0/3] MAC address fail to be added shouldn't be stored
  2017-04-13  8:21 ` [PATCH v4 0/3] MAC address fail to be added shouldn't be stored Wei Dai
@ 2017-04-13 13:54   ` Ananyev, Konstantin
  0 siblings, 0 replies; 19+ messages in thread
From: Ananyev, Konstantin @ 2017-04-13 13:54 UTC (permalink / raw)
  To: Dai, Wei, thomas.monjalon, harish.patil, rasesh.mody,
	stephen.hurd, ajit.khaparde, Lu, Wenzhuo, Zhang, Helin, Wu,
	Jingjing, Chen, Jing D, adrien.mazarguil, nelio.laranjeiro,
	Richardson, Bruce, yuanhan.liu, maxime.coquelin
  Cc: dev



> 
> Current ethdev always stores MAC address even it fails to be added.
> Other function may regard the failed MAC address valid and lead to
> some errors. So There is a need to check if the addr is added
> successfully or not and discard it if it fails.
> 
> In 3rd patch, add a command "add_more_mac_addr port_id base_mac_addr count"
> to add more than one MAC address one time.
> This command can simplify the test for the first patch.
> Normally a MAC address may fails to be added only after many MAC
> addresses have been added.
> Without this command, a tester may only trigger failed MAC address
> by running many times of testpmd command 'mac_addr add' .
> 
> ---
> Changes
> v4:
> 	1. rebase master branch
> 	2. follow code style
> 
> v3:
>   1. Change return value for some specific NIC according to feedbacks
>      from the community;
>   2. Add ABI change in release note;
>   3. Add more detailed commit message.
> 
> v2:
>   fix warnings and erros from check-git-log.sh and checkpatch.pl
> 
> Wei Dai (3):
>   ethdev: fix adding invalid MAC addr
>   doc: change type of return value of adding MAC addr
>   app/testpmd: add a command to add many MAC addrs
> 
>  app/test-pmd/cmdline.c                 | 55 ++++++++++++++++++++++++++++++++++
>  doc/guides/rel_notes/release_17_05.rst |  7 +++++
>  drivers/net/bnx2x/bnx2x_ethdev.c       |  7 +++--
>  drivers/net/bnxt/bnxt_ethdev.c         | 12 ++++----
>  drivers/net/e1000/base/e1000_api.c     |  2 +-
>  drivers/net/e1000/em_ethdev.c          |  6 ++--
>  drivers/net/e1000/igb_ethdev.c         |  5 ++--
>  drivers/net/enic/enic.h                |  2 +-
>  drivers/net/enic/enic_ethdev.c         |  4 +--
>  drivers/net/enic/enic_main.c           |  6 ++--
>  drivers/net/fm10k/fm10k_ethdev.c       |  3 +-
>  drivers/net/i40e/i40e_ethdev.c         | 11 +++----
>  drivers/net/i40e/i40e_ethdev_vf.c      |  8 ++---
>  drivers/net/ixgbe/ixgbe_ethdev.c       | 27 +++++++++++------
>  drivers/net/mlx4/mlx4.c                | 18 ++++++-----
>  drivers/net/mlx5/mlx5.h                |  4 +--
>  drivers/net/mlx5/mlx5_mac.c            | 16 ++++++----
>  drivers/net/qede/qede_ethdev.c         |  6 ++--
>  drivers/net/ring/rte_eth_ring.c        |  3 +-
>  drivers/net/virtio/virtio_ethdev.c     | 13 ++++----
>  lib/librte_ether/rte_ethdev.c          | 15 ++++++----
>  lib/librte_ether/rte_ethdev.h          |  2 +-
>  22 files changed, 162 insertions(+), 70 deletions(-)
> 
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.7.4

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/3] doc: change type of return value of adding MAC addr
  2017-04-13  8:21 ` [PATCH v4 2/3] doc: change type of return value of adding " Wei Dai
@ 2017-04-14 16:03   ` Mcnamara, John
  0 siblings, 0 replies; 19+ messages in thread
From: Mcnamara, John @ 2017-04-14 16:03 UTC (permalink / raw)
  To: Dai, Wei, thomas.monjalon, harish.patil, rasesh.mody,
	stephen.hurd, ajit.khaparde, Lu, Wenzhuo, Zhang, Helin, Ananyev,
	Konstantin, Wu, Jingjing, Chen, Jing D, adrien.mazarguil,
	nelio.laranjeiro, Richardson, Bruce, yuanhan.liu,
	maxime.coquelin
  Cc: dev, Dai, Wei



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wei Dai
> Sent: Thursday, April 13, 2017 9:21 AM
> To: thomas.monjalon@6wind.com; harish.patil@cavium.com;
> rasesh.mody@cavium.com; stephen.hurd@broadcom.com;
> ajit.khaparde@broadcom.com; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang,
> Helin <helin.zhang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Chen, Jing D <jing.d.chen@intel.com>; adrien.mazarguil@6wind.com;
> nelio.laranjeiro@6wind.com; Richardson, Bruce
> <bruce.richardson@intel.com>; yuanhan.liu@linux.intel.com;
> maxime.coquelin@redhat.com
> Cc: dev@dpdk.org; Dai, Wei <wei.dai@intel.com>
> Subject: [dpdk-dev] [PATCH v4 2/3] doc: change type of return value of
> adding MAC addr
> 
> Add following lines in section of API change in release note.
> 
> If a MAC address fails to be added without this change, it is still stored
> and may be regarded as a valid one. This may lead to errors in
> application.
> The type of return value of eth_mac_addr_add_t in rte_ethdev.h is changed.
> Any specific NIC also follows this change.
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>


Acked-by: John McNamara <john.mcnamara@intel.com>

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
  2017-04-13  8:21 ` [PATCH v4 1/3] ethdev: fix adding invalid MAC addr Wei Dai
  2017-04-13  8:44   ` Nélio Laranjeiro
@ 2017-04-20  5:31   ` Yuanhan Liu
  2017-04-20 21:43   ` [dpdk-stable] " Thomas Monjalon
  2017-04-21  6:43   ` Lu, Wenzhuo
  3 siblings, 0 replies; 19+ messages in thread
From: Yuanhan Liu @ 2017-04-20  5:31 UTC (permalink / raw)
  To: Wei Dai
  Cc: thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, wenzhuo.lu, helin.zhang, konstantin.ananyev,
	jingjing.wu, jing.d.chen, adrien.mazarguil, nelio.laranjeiro,
	bruce.richardson, maxime.coquelin, dev, stable

On Thu, Apr 13, 2017 at 04:21:04PM +0800, Wei Dai wrote:
> some customers find adding mac addr to VF sometimes can fail,
> but it is still stored in dev->data->mac_addrs[ ]. So this
> can lead to some errors that assumes the non-zero entry in
> dev->data->mac_addrs[ ] is valid.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>
> ---
>  drivers/net/bnx2x/bnx2x_ethdev.c   |  7 +++++--
>  drivers/net/bnxt/bnxt_ethdev.c     | 12 ++++++------
>  drivers/net/e1000/base/e1000_api.c |  2 +-
>  drivers/net/e1000/em_ethdev.c      |  6 +++---
>  drivers/net/e1000/igb_ethdev.c     |  5 +++--
>  drivers/net/enic/enic.h            |  2 +-
>  drivers/net/enic/enic_ethdev.c     |  4 ++--
>  drivers/net/enic/enic_main.c       |  6 +++---
>  drivers/net/fm10k/fm10k_ethdev.c   |  3 ++-
>  drivers/net/i40e/i40e_ethdev.c     | 11 ++++++-----
>  drivers/net/i40e/i40e_ethdev_vf.c  |  8 ++++----
>  drivers/net/ixgbe/ixgbe_ethdev.c   | 27 ++++++++++++++++++---------
>  drivers/net/mlx4/mlx4.c            | 18 +++++++++++-------
>  drivers/net/mlx5/mlx5.h            |  4 ++--
>  drivers/net/mlx5/mlx5_mac.c        | 16 ++++++++++------
>  drivers/net/qede/qede_ethdev.c     |  6 ++++--
>  drivers/net/ring/rte_eth_ring.c    |  3 ++-
>  drivers/net/virtio/virtio_ethdev.c | 13 +++++++------

For virtio changes,

Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

	--yliu

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [dpdk-stable] [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
  2017-04-13  8:21 ` [PATCH v4 1/3] ethdev: fix adding invalid MAC addr Wei Dai
  2017-04-13  8:44   ` Nélio Laranjeiro
  2017-04-20  5:31   ` Yuanhan Liu
@ 2017-04-20 21:43   ` Thomas Monjalon
  2017-04-21  6:43   ` Lu, Wenzhuo
  3 siblings, 0 replies; 19+ messages in thread
From: Thomas Monjalon @ 2017-04-20 21:43 UTC (permalink / raw)
  To: Wei Dai
  Cc: stable, thomas.monjalon, harish.patil, rasesh.mody, stephen.hurd,
	ajit.khaparde, wenzhuo.lu, helin.zhang, konstantin.ananyev,
	jingjing.wu, jing.d.chen, adrien.mazarguil, nelio.laranjeiro,
	bruce.richardson, yuanhan.liu, maxime.coquelin, dev

13/04/2017 10:21, Wei Dai:
> some customers find adding mac addr to VF sometimes can fail,
> but it is still stored in dev->data->mac_addrs[ ]. So this
> can lead to some errors that assumes the non-zero entry in
> dev->data->mac_addrs[ ] is valid.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>
> ---
>  drivers/net/bnx2x/bnx2x_ethdev.c   |  7 +++++--
>  drivers/net/bnxt/bnxt_ethdev.c     | 12 ++++++------
>  drivers/net/e1000/base/e1000_api.c |  2 +-
>  drivers/net/e1000/em_ethdev.c      |  6 +++---
>  drivers/net/e1000/igb_ethdev.c     |  5 +++--
>  drivers/net/enic/enic.h            |  2 +-
>  drivers/net/enic/enic_ethdev.c     |  4 ++--
>  drivers/net/enic/enic_main.c       |  6 +++---
>  drivers/net/fm10k/fm10k_ethdev.c   |  3 ++-
>  drivers/net/i40e/i40e_ethdev.c     | 11 ++++++-----
>  drivers/net/i40e/i40e_ethdev_vf.c  |  8 ++++----
>  drivers/net/ixgbe/ixgbe_ethdev.c   | 27 ++++++++++++++++++---------
>  drivers/net/mlx4/mlx4.c            | 18 +++++++++++-------
>  drivers/net/mlx5/mlx5.h            |  4 ++--
>  drivers/net/mlx5/mlx5_mac.c        | 16 ++++++++++------
>  drivers/net/qede/qede_ethdev.c     |  6 ++++--
>  drivers/net/ring/rte_eth_ring.c    |  3 ++-
>  drivers/net/virtio/virtio_ethdev.c | 13 +++++++------
>  lib/librte_ether/rte_ethdev.c      | 15 +++++++++------
>  lib/librte_ether/rte_ethdev.h      |  2 +-
>  20 files changed, 100 insertions(+), 70 deletions(-)

Please, could you rebase and add changes to newly introduced drivers?
Thanks

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
  2017-04-13  8:21 ` [PATCH v4 1/3] ethdev: fix adding invalid MAC addr Wei Dai
                     ` (2 preceding siblings ...)
  2017-04-20 21:43   ` [dpdk-stable] " Thomas Monjalon
@ 2017-04-21  6:43   ` Lu, Wenzhuo
  2017-04-29  6:09     ` Dai, Wei
  3 siblings, 1 reply; 19+ messages in thread
From: Lu, Wenzhuo @ 2017-04-21  6:43 UTC (permalink / raw)
  To: Dai, Wei, thomas.monjalon, harish.patil, rasesh.mody,
	stephen.hurd, ajit.khaparde, Zhang, Helin, Ananyev, Konstantin,
	Wu, Jingjing, Chen, Jing D, adrien.mazarguil, nelio.laranjeiro,
	Richardson, Bruce, yuanhan.liu, maxime.coquelin
  Cc: dev, stable

Hi Wei,

> -----Original Message-----
> From: Dai, Wei
> Sent: Thursday, April 13, 2017 4:21 PM
> To: thomas.monjalon@6wind.com; harish.patil@cavium.com;
> rasesh.mody@cavium.com; stephen.hurd@broadcom.com;
> ajit.khaparde@broadcom.com; Lu, Wenzhuo; Zhang, Helin; Ananyev,
> Konstantin; Wu, Jingjing; Chen, Jing D; adrien.mazarguil@6wind.com;
> nelio.laranjeiro@6wind.com; Richardson, Bruce;
> yuanhan.liu@linux.intel.com; maxime.coquelin@redhat.com
> Cc: dev@dpdk.org; Dai, Wei; stable@dpdk.org
> Subject: [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
> 
> some customers find adding mac addr to VF sometimes can fail, but it is still
> stored in dev->data->mac_addrs[ ]. So this can lead to some errors that
> assumes the non-zero entry in
> dev->data->mac_addrs[ ] is valid.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wei Dai <wei.dai@intel.com>
> ---
>  drivers/net/bnx2x/bnx2x_ethdev.c   |  7 +++++--
>  drivers/net/bnxt/bnxt_ethdev.c     | 12 ++++++------
>  drivers/net/e1000/base/e1000_api.c |  2 +-
>  drivers/net/e1000/em_ethdev.c      |  6 +++---
>  drivers/net/e1000/igb_ethdev.c     |  5 +++--
>  drivers/net/enic/enic.h            |  2 +-
>  drivers/net/enic/enic_ethdev.c     |  4 ++--
>  drivers/net/enic/enic_main.c       |  6 +++---
>  drivers/net/fm10k/fm10k_ethdev.c   |  3 ++-
>  drivers/net/i40e/i40e_ethdev.c     | 11 ++++++-----
>  drivers/net/i40e/i40e_ethdev_vf.c  |  8 ++++----
>  drivers/net/ixgbe/ixgbe_ethdev.c   | 27 ++++++++++++++++++---------
>  drivers/net/mlx4/mlx4.c            | 18 +++++++++++-------
>  drivers/net/mlx5/mlx5.h            |  4 ++--
>  drivers/net/mlx5/mlx5_mac.c        | 16 ++++++++++------
>  drivers/net/qede/qede_ethdev.c     |  6 ++++--
>  drivers/net/ring/rte_eth_ring.c    |  3 ++-
>  drivers/net/virtio/virtio_ethdev.c | 13 +++++++------
>  lib/librte_ether/rte_ethdev.c      | 15 +++++++++------
>  lib/librte_ether/rte_ethdev.h      |  2 +-
>  20 files changed, 100 insertions(+), 70 deletions(-)
> 
> 
>  int bnxt_link_update_op(struct rte_eth_dev *eth_dev, int
> wait_to_complete) diff --git a/drivers/net/e1000/base/e1000_api.c
> b/drivers/net/e1000/base/e1000_api.c
> index f7cf83b..dcb53f7 100644
> --- a/drivers/net/e1000/base/e1000_api.c
> +++ b/drivers/net/e1000/base/e1000_api.c
> @@ -855,7 +855,7 @@ int e1000_rar_set(struct e1000_hw *hw, u8 *addr,
> u32 index)
>  	if (hw->mac.ops.rar_set)
>  		return hw->mac.ops.rar_set(hw, addr, index);
> 
> -	return E1000_SUCCESS;
> +	return E1000_ERR_NO_SPACE;
>  }
NACK here. Normally we try to avoid changing base code. And I don't think the change is necessary.

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
  2017-04-21  6:43   ` Lu, Wenzhuo
@ 2017-04-29  6:09     ` Dai, Wei
  2017-05-02  1:21       ` Lu, Wenzhuo
  0 siblings, 1 reply; 19+ messages in thread
From: Dai, Wei @ 2017-04-29  6:09 UTC (permalink / raw)
  To: Lu, Wenzhuo, thomas.monjalon, harish.patil, rasesh.mody,
	stephen.hurd, ajit.khaparde, Zhang, Helin, Ananyev, Konstantin,
	Wu, Jingjing, Chen, Jing D, adrien.mazarguil, nelio.laranjeiro,
	Richardson, Bruce, yuanhan.liu, maxime.coquelin
  Cc: dev, stable

HI, Wenzhuo

> >
> >  int bnxt_link_update_op(struct rte_eth_dev *eth_dev, int
> > wait_to_complete) diff --git a/drivers/net/e1000/base/e1000_api.c
> > b/drivers/net/e1000/base/e1000_api.c
> > index f7cf83b..dcb53f7 100644
> > --- a/drivers/net/e1000/base/e1000_api.c
> > +++ b/drivers/net/e1000/base/e1000_api.c
> > @@ -855,7 +855,7 @@ int e1000_rar_set(struct e1000_hw *hw, u8 *addr,
> > u32 index)
> >  	if (hw->mac.ops.rar_set)
> >  		return hw->mac.ops.rar_set(hw, addr, index);
> >
> > -	return E1000_SUCCESS;
> > +	return E1000_ERR_NO_SPACE;
> >  }
> NACK here. Normally we try to avoid changing base code. And I don't think the
> change is necessary.

If this code is not changed, the code in ethdev may get wrong return value and assume
the failed MAC addr is added.
Anyway, we can ask the base code to be revised by the associated team.

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
  2017-04-29  6:09     ` Dai, Wei
@ 2017-05-02  1:21       ` Lu, Wenzhuo
  2017-05-02  1:51         ` Dai, Wei
  0 siblings, 1 reply; 19+ messages in thread
From: Lu, Wenzhuo @ 2017-05-02  1:21 UTC (permalink / raw)
  To: Dai, Wei, thomas.monjalon, harish.patil, rasesh.mody,
	stephen.hurd, ajit.khaparde, Zhang, Helin, Ananyev, Konstantin,
	Wu, Jingjing, Chen, Jing D, adrien.mazarguil, nelio.laranjeiro,
	Richardson, Bruce, yuanhan.liu, maxime.coquelin
  Cc: dev, stable

Hi Wei,


> -----Original Message-----
> From: Dai, Wei
> Sent: Saturday, April 29, 2017 2:09 PM
> To: Lu, Wenzhuo; thomas.monjalon@6wind.com; harish.patil@cavium.com;
> rasesh.mody@cavium.com; stephen.hurd@broadcom.com;
> ajit.khaparde@broadcom.com; Zhang, Helin; Ananyev, Konstantin; Wu,
> Jingjing; Chen, Jing D; adrien.mazarguil@6wind.com;
> nelio.laranjeiro@6wind.com; Richardson, Bruce;
> yuanhan.liu@linux.intel.com; maxime.coquelin@redhat.com
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
> 
> HI, Wenzhuo
> 
> > >
> > >  int bnxt_link_update_op(struct rte_eth_dev *eth_dev, int
> > > wait_to_complete) diff --git a/drivers/net/e1000/base/e1000_api.c
> > > b/drivers/net/e1000/base/e1000_api.c
> > > index f7cf83b..dcb53f7 100644
> > > --- a/drivers/net/e1000/base/e1000_api.c
> > > +++ b/drivers/net/e1000/base/e1000_api.c
> > > @@ -855,7 +855,7 @@ int e1000_rar_set(struct e1000_hw *hw, u8 *addr,
> > > u32 index)
> > >  	if (hw->mac.ops.rar_set)
> > >  		return hw->mac.ops.rar_set(hw, addr, index);
> > >
> > > -	return E1000_SUCCESS;
> > > +	return E1000_ERR_NO_SPACE;
> > >  }
> > NACK here. Normally we try to avoid changing base code. And I don't
> > think the change is necessary.
> 
> If this code is not changed, the code in ethdev may get wrong return value
> and assume the failed MAC addr is added.
> Anyway, we can ask the base code to be revised by the associated team.
' hw->mac.ops.rar_set ' cannot be NULL. That's why I think this change is not necessary.

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH v4 1/3] ethdev: fix adding invalid MAC addr
  2017-05-02  1:21       ` Lu, Wenzhuo
@ 2017-05-02  1:51         ` Dai, Wei
  0 siblings, 0 replies; 19+ messages in thread
From: Dai, Wei @ 2017-05-02  1:51 UTC (permalink / raw)
  To: Lu, Wenzhuo, thomas.monjalon, harish.patil, rasesh.mody,
	stephen.hurd, ajit.khaparde, Zhang, Helin, Ananyev, Konstantin,
	Wu, Jingjing, Chen, Jing D, adrien.mazarguil, nelio.laranjeiro,
	Richardson, Bruce, yuanhan.liu, maxime.coquelin
  Cc: dev, stable

> > > >
> > > >  int bnxt_link_update_op(struct rte_eth_dev *eth_dev, int
> > > > wait_to_complete) diff --git a/drivers/net/e1000/base/e1000_api.c
> > > > b/drivers/net/e1000/base/e1000_api.c
> > > > index f7cf83b..dcb53f7 100644
> > > > --- a/drivers/net/e1000/base/e1000_api.c
> > > > +++ b/drivers/net/e1000/base/e1000_api.c
> > > > @@ -855,7 +855,7 @@ int e1000_rar_set(struct e1000_hw *hw, u8
> > > > *addr,
> > > > u32 index)
> > > >  	if (hw->mac.ops.rar_set)
> > > >  		return hw->mac.ops.rar_set(hw, addr, index);
> > > >
> > > > -	return E1000_SUCCESS;
> > > > +	return E1000_ERR_NO_SPACE;
> > > >  }
> > > NACK here. Normally we try to avoid changing base code. And I don't
> > > think the change is necessary.
> >
> > If this code is not changed, the code in ethdev may get wrong return
> > value and assume the failed MAC addr is added.
> > Anyway, we can ask the base code to be revised by the associated team.
> ' hw->mac.ops.rar_set ' cannot be NULL. That's why I think this change is not
> necessary.
OK, I will follow your advice in my v6 patch set.
Thanks

^ permalink raw reply	[flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-05-02  1:51 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-12  9:02 [PATCH v3 0/3] MAC address fail to be added shouldn't be stored Wei Dai
2017-04-12  9:02 ` [PATCH v3 1/3] ethdev: fix adding invalid MAC addr Wei Dai
2017-04-12  9:30   ` Nélio Laranjeiro
2017-04-12  9:02 ` [PATCH v3 2/3] doc: change type of return value of adding " Wei Dai
2017-04-12  9:02 ` [PATCH v3 3/3] app/testpmd: add a command to add many MAC addrs Wei Dai
2017-04-13  8:21 ` [PATCH v4 0/3] MAC address fail to be added shouldn't be stored Wei Dai
2017-04-13 13:54   ` Ananyev, Konstantin
2017-04-13  8:21 ` [PATCH v4 1/3] ethdev: fix adding invalid MAC addr Wei Dai
2017-04-13  8:44   ` Nélio Laranjeiro
2017-04-13  9:22     ` Dai, Wei
2017-04-20  5:31   ` Yuanhan Liu
2017-04-20 21:43   ` [dpdk-stable] " Thomas Monjalon
2017-04-21  6:43   ` Lu, Wenzhuo
2017-04-29  6:09     ` Dai, Wei
2017-05-02  1:21       ` Lu, Wenzhuo
2017-05-02  1:51         ` Dai, Wei
2017-04-13  8:21 ` [PATCH v4 2/3] doc: change type of return value of adding " Wei Dai
2017-04-14 16:03   ` Mcnamara, John
2017-04-13  8:21 ` [PATCH v4 3/3] app/testpmd: add a command to add many MAC addrs Wei Dai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.