Skip to contentSkip to conversation

i40e: fix i40e_validate_mac_addr to permit multicast addresses

PATCH In review David Harton 4 participants 15 messages Aug 22, 2017 – Sep 14, 2017 Network drivers

lore
Share & tools
lore

David Harton

author
#0 lore raw

PATCH 1/1 i40e: fix i40e_validate_mac_addr to permit multicast addresses

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by removing the multicast address check in i40e_validate_mac_addr.

Signed-off-by David Harton <[email protected]>
---
 drivers/net/i40e/base/i40e_common.c | 12 +++++-------
 drivers/net/i40e/i40e_ethdev.c      |  3 ++-
 2 files changed, 7 insertions(+), 8 deletions(-)
drivers/net/i40e/base/i40e_common.c +5 -7
index 900d379..9779854 100644
@@ -969,10 +969,10 @@ struct i40e_rx_ptype_decoded i40e_ptype_lookup[] = {
 
 
 /**
- * i40e_validate_mac_addr - Validate unicast MAC address
+ * i40e_validate_mac_addr - Validate MAC address
  * @mac_addr: pointer to MAC address
  *
- * Tests a MAC address to ensure it is a valid Individual Address
+ * Tests a MAC address to ensure it is a valid Address
  **/
 enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)
 {

@@ -980,13 +980,11 @@ enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)
 
 	DEBUGFUNC("i40e_validate_mac_addr");
 
-	/* Broadcast addresses ARE multicast addresses
-	 * Make sure it is not a multicast address
+	/*
 	 * Reject the zero address
 	 */
-	if (I40E_IS_MULTICAST(mac_addr) ||
-	    (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
-	      mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0))
+	if (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
+	    mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0)
 		status = I40E_ERR_INVALID_MAC_ADDR;
 
 	return status;
drivers/net/i40e/i40e_ethdev.c +2 -1
index 5f26e24..00b6082 100644
@@ -1199,7 +1199,8 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 
 	/* Get and check the mac address */
 	i40e_get_mac_addr(hw, hw->mac.addr);
-	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS) {
+	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS ||
+	    I40E_IS_MULTICAST(hw->mac.addr)) {
 		PMD_INIT_LOG(ERR, "mac address is not valid");
 		ret = -EIO;
 		goto err_get_mac_addr;
Signature · 1 line
2.10.3.dirty
#1 lore raw
Quoted 51 lines · David Harton

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by removing the multicast address check in i40e_validate_mac_addr.

Signed-off-by David Harton <[email protected]>
---
 drivers/net/i40e/base/i40e_common.c | 12 +++++-------
 drivers/net/i40e/i40e_ethdev.c      |  3 ++-
 2 files changed, 7 insertions(+), 8 deletions(-)
drivers/net/i40e/base/i40e_common.c +5 -7
index 900d379..9779854 100644
@@ -969,10 +969,10 @@ struct i40e_rx_ptype_decoded i40e_ptype_lookup[] = {
 
 
 /**
- * i40e_validate_mac_addr - Validate unicast MAC address
+ * i40e_validate_mac_addr - Validate MAC address
  * @mac_addr: pointer to MAC address
  *
- * Tests a MAC address to ensure it is a valid Individual Address
+ * Tests a MAC address to ensure it is a valid Address
  **/
 enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)
 {

@@ -980,13 +980,11 @@ enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)
 
 	DEBUGFUNC("i40e_validate_mac_addr");
 
-	/* Broadcast addresses ARE multicast addresses
-	 * Make sure it is not a multicast address
+	/*
 	 * Reject the zero address
 	 */
-	if (I40E_IS_MULTICAST(mac_addr) ||
-	    (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
-	      mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0))
+	if (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
+	    mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0)
 		status = I40E_ERR_INVALID_MAC_ADDR;
 
 	return status;
drivers/net/i40e/i40e_ethdev.c +0 -0
index 5f26e24..00b6082 100644

drivers/net/i40e/i40e_ethdev.c eth_i40e_dev_init(struct rte_eth_dev *dev) · line 1203

Quoted 6 lines
drivers/net/i40e/i40e_ethdev.c +2 -1
@@ -1199,7 +1199,8 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 
 	/* Get and check the mac address */
 	i40e_get_mac_addr(hw, hw->mac.addr);
-	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS) {
+	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS ||
+	    I40E_IS_MULTICAST(hw->mac.addr)) {

As far as I can see this is to set PF permanent mac address during init(), i40e_macaddr_add() can be used to set multicast address filters. Why do you want to set multicast address as mac permanent address?

Any chance that you want to update i40evf_add_mac_addr() to let multicast addresses?

Quoted 4 lines
drivers/net/i40e/i40e_ethdev.c +0 -0
 		PMD_INIT_LOG(ERR, "mac address is not valid");
 		ret = -EIO;
 		goto err_get_mac_addr;
author
#2 lore raw
Quoted 76 lines

-----Original Message-----
From: Ferruh Yigit [mailto:[email protected]]
Sent: Thursday, August 31, 2017 11:54 AM
To: David Harton (dharton) <[email protected]>; [email protected]; [email protected]
Cc: [email protected]
Subject: Re: [dpdk-dev] [PATCH] i40e: fix i40e_validate_mac_addr to permit multicast addresses

Quoted 53 lines · David Harton

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by removing the multicast address check in i40e_validate_mac_addr.

Signed-off-by David Harton <[email protected]>
---
 drivers/net/i40e/base/i40e_common.c | 12 +++++-------
 drivers/net/i40e/i40e_ethdev.c      |  3 ++-
 2 files changed, 7 insertions(+), 8 deletions(-)
drivers/net/i40e/base/i40e_common.c +2 -3
index 900d379..9779854 100644
@@ -969,10 +969,10 @@ struct i40e_rx_ptype_decoded i40e_ptype_lookup[]
 = {
 
 
 /**
- * i40e_validate_mac_addr - Validate unicast MAC address
+ * i40e_validate_mac_addr - Validate MAC address
  * @mac_addr: pointer to MAC address
  *
- * Tests a MAC address to ensure it is a valid Individual Address
+ * Tests a MAC address to ensure it is a valid Address
  **/
 enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)  { @@
-980,13 +980,11 @@ enum i40e_status_code i40e_validate_mac_addr(u8

*mac_addr)

drivers/net/i40e/base/i40e_common.c +3 -5
 	DEBUGFUNC("i40e_validate_mac_addr");
 
-	/* Broadcast addresses ARE multicast addresses
-	 * Make sure it is not a multicast address
+	/*
 	 * Reject the zero address
 	 */
-	if (I40E_IS_MULTICAST(mac_addr) ||
-	    (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
-	      mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0))
+	if (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
+	    mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0)
 		status = I40E_ERR_INVALID_MAC_ADDR;
 
 	return status;
(quoted diff) +0 -0

b/drivers/net/i40e/i40e_ethdev.c index 5f26e24..00b6082 100644

drivers/net/i40e/i40e_ethdev.c eth_i40e_dev_init(struct rte_eth_dev *dev) · line 1203

Quoted 6 lines
drivers/net/i40e/i40e_ethdev.c +2 -1
@@ -1199,7 +1199,8 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 
 	/* Get and check the mac address */
 	i40e_get_mac_addr(hw, hw->mac.addr);
-	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS) {
+	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS ||
+	    I40E_IS_MULTICAST(hw->mac.addr)) {

As far as I can see this is to set PF permanent mac address during init(), i40e_macaddr_add() can be used to set multicast address filters. Why do you want to set multicast address as mac permanent address?

Quoted 2 lines

Any chance that you want to update i40evf_add_mac_addr() to let multicast addresses?

No. I'm preserving the existing behavior here. The previous call used to ensure that the permanent mac address here was both non-null and not a multi-cast address.

Since I removed the multi cast check from i40e_validate_mac_addr() I added the multicast check here. If "not valid" or a multicast address fail.

Regards,
Dave

Quoted 5 lines
Quoted 5 lines
drivers/net/i40e/i40e_ethdev.c +0 -0
 		PMD_INIT_LOG(ERR, "mac address is not valid");
 		ret = -EIO;
 		goto err_get_mac_addr;
author
#3 lore raw
In reply to Ferruh Yigit

Hi Jingjing/Beilei,

A kind reminder to review the patch and the discussion between Ferruh and myself.

Thanks,
Dave

Quoted 89 lines

-----Original Message-----
From: David Harton (dharton)

Quoted 69 lines

-----Original Message-----
From: Ferruh Yigit [mailto:[email protected]]

Quoted 53 lines · David Harton

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by removing the multicast address check in i40e_validate_mac_addr.

Signed-off-by David Harton <[email protected]>
---
 drivers/net/i40e/base/i40e_common.c | 12 +++++-------
 drivers/net/i40e/i40e_ethdev.c      |  3 ++-
 2 files changed, 7 insertions(+), 8 deletions(-)
drivers/net/i40e/base/i40e_common.c +2 -3
index 900d379..9779854 100644
@@ -969,10 +969,10 @@ struct i40e_rx_ptype_decoded
 i40e_ptype_lookup[] = {
 
 
 /**
- * i40e_validate_mac_addr - Validate unicast MAC address
+ * i40e_validate_mac_addr - Validate MAC address
  * @mac_addr: pointer to MAC address
  *
- * Tests a MAC address to ensure it is a valid Individual Address
+ * Tests a MAC address to ensure it is a valid Address
  **/
 enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)  { @@
-980,13 +980,11 @@ enum i40e_status_code i40e_validate_mac_addr(u8

*mac_addr)

drivers/net/i40e/base/i40e_common.c +3 -5
 	DEBUGFUNC("i40e_validate_mac_addr");
 
-	/* Broadcast addresses ARE multicast addresses
-	 * Make sure it is not a multicast address
+	/*
 	 * Reject the zero address
 	 */
-	if (I40E_IS_MULTICAST(mac_addr) ||
-	    (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
-	      mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0))
+	if (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
+	    mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0)
 		status = I40E_ERR_INVALID_MAC_ADDR;
 
 	return status;
(quoted diff) +0 -0

b/drivers/net/i40e/i40e_ethdev.c index 5f26e24..00b6082 100644

drivers/net/i40e/i40e_ethdev.c eth_i40e_dev_init(struct rte_eth_dev *dev) · line 1203

Quoted 6 lines
drivers/net/i40e/i40e_ethdev.c +2 -1
@@ -1199,7 +1199,8 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 
 	/* Get and check the mac address */
 	i40e_get_mac_addr(hw, hw->mac.addr);
-	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS) {
+	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS ||
+	    I40E_IS_MULTICAST(hw->mac.addr)) {

As far as I can see this is to set PF permanent mac address during init(),
i40e_macaddr_add() can be used to set multicast address filters. Why do you want to set multicast address as mac permanent address?

Quoted 2 lines

Any chance that you want to update i40evf_add_mac_addr() to let multicast addresses?

No. I'm preserving the existing behavior here. The previous call used to ensure that the permanent mac address here was both non-null and not a multi-cast address.

Since I removed the multi cast check from i40e_validate_mac_addr() I added the multicast check here. If "not valid" or a multicast address fail.

Regards,
Dave

Quoted 5 lines
Quoted 5 lines
drivers/net/i40e/i40e_ethdev.c +0 -0
 		PMD_INIT_LOG(ERR, "mac address is not valid");
 		ret = -EIO;
 		goto err_get_mac_addr;
#4 lore raw
Quoted 4 lines · David Harton (dharton)

Hi Jingjing/Beilei,

A kind reminder to review the patch and the discussion between Ferruh and myself.

Hi David,

To clarify, your reply answered my concern, so I am OK, still I believe this should be reviewed by driver maintainer.

Thanks,
ferruh

Quoted 94 lines

Thanks,
Dave

Quoted 89 lines

-----Original Message-----
From: David Harton (dharton)

Quoted 69 lines

-----Original Message-----
From: Ferruh Yigit [mailto:[email protected]]

Quoted 53 lines · David Harton

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by removing the multicast address check in i40e_validate_mac_addr.

Signed-off-by David Harton <[email protected]>
---
 drivers/net/i40e/base/i40e_common.c | 12 +++++-------
 drivers/net/i40e/i40e_ethdev.c      |  3 ++-
 2 files changed, 7 insertions(+), 8 deletions(-)
drivers/net/i40e/base/i40e_common.c +2 -3
index 900d379..9779854 100644
@@ -969,10 +969,10 @@ struct i40e_rx_ptype_decoded
 i40e_ptype_lookup[] = {
 
 
 /**
- * i40e_validate_mac_addr - Validate unicast MAC address
+ * i40e_validate_mac_addr - Validate MAC address
  * @mac_addr: pointer to MAC address
  *
- * Tests a MAC address to ensure it is a valid Individual Address
+ * Tests a MAC address to ensure it is a valid Address
  **/
 enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)  { @@
-980,13 +980,11 @@ enum i40e_status_code i40e_validate_mac_addr(u8

*mac_addr)

drivers/net/i40e/base/i40e_common.c +3 -5
 	DEBUGFUNC("i40e_validate_mac_addr");
 
-	/* Broadcast addresses ARE multicast addresses
-	 * Make sure it is not a multicast address
+	/*
 	 * Reject the zero address
 	 */
-	if (I40E_IS_MULTICAST(mac_addr) ||
-	    (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
-	      mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0))
+	if (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
+	    mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0)
 		status = I40E_ERR_INVALID_MAC_ADDR;
 
 	return status;
(quoted diff) +0 -0

b/drivers/net/i40e/i40e_ethdev.c index 5f26e24..00b6082 100644

drivers/net/i40e/i40e_ethdev.c eth_i40e_dev_init(struct rte_eth_dev *dev) · line 1203

Quoted 6 lines
drivers/net/i40e/i40e_ethdev.c +2 -1
@@ -1199,7 +1199,8 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 
 	/* Get and check the mac address */
 	i40e_get_mac_addr(hw, hw->mac.addr);
-	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS) {
+	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS ||
+	    I40E_IS_MULTICAST(hw->mac.addr)) {

As far as I can see this is to set PF permanent mac address during init(),
i40e_macaddr_add() can be used to set multicast address filters. Why do you want to set multicast address as mac permanent address?

Quoted 2 lines

Any chance that you want to update i40evf_add_mac_addr() to let multicast addresses?

No. I'm preserving the existing behavior here. The previous call used to ensure that the permanent mac address here was both non-null and not a multi-cast address.

Since I removed the multi cast check from i40e_validate_mac_addr() I added the multicast check here. If "not valid" or a multicast address fail.

Regards,
Dave

Quoted 5 lines
Quoted 5 lines
drivers/net/i40e/i40e_ethdev.c +0 -0
 		PMD_INIT_LOG(ERR, "mac address is not valid");
 		ret = -EIO;
 		goto err_get_mac_addr;
#5 lore raw
In reply to David Harton

Hi,

Quoted 8 lines

-----Original Message-----
From: David Harton [mailto:[email protected]]
Sent: Wednesday, August 23, 2017 6:22 AM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; David Harton <[email protected]>
Subject: [PATCH] i40e: fix i40e_validate_mac_addr to permit multicast addresses

Quoted 6 lines

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by removing the multicast address check in i40e_validate_mac_addr.

Maybe fix line is needed here.

Quoted 31 lines
Signed-off-by David Harton <[email protected]>
---
 drivers/net/i40e/base/i40e_common.c | 12 +++++-------
 drivers/net/i40e/i40e_ethdev.c      |  3 ++-
 2 files changed, 7 insertions(+), 8 deletions(-)
drivers/net/i40e/base/i40e_common.c +2 -2
index 900d379..9779854 100644
@@ -969,10 +969,10 @@ struct i40e_rx_ptype_decoded
 i40e_ptype_lookup[] = {
 
 
 /**
- * i40e_validate_mac_addr - Validate unicast MAC address
+ * i40e_validate_mac_addr - Validate MAC address
  * @mac_addr: pointer to MAC address
  *
- * Tests a MAC address to ensure it is a valid Individual Address
+ * Tests a MAC address to ensure it is a valid Address
  **/
 enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)  { @@ -

980,13 +980,11 @@ enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)

drivers/net/i40e/base/i40e_common.c +0 -1
 	DEBUGFUNC("i40e_validate_mac_addr");
 
-	/* Broadcast addresses ARE multicast addresses

drivers/net/i40e/base/i40e_common.c enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr) · line 988

Quoted 10 lines
drivers/net/i40e/base/i40e_common.c +3 -4
-	 * Make sure it is not a multicast address
+	/*
 	 * Reject the zero address
 	 */
-	if (I40E_IS_MULTICAST(mac_addr) ||
-	    (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
-	      mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0))
+	if (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
+	    mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0)
 		status = I40E_ERR_INVALID_MAC_ADDR;

We'd better not to change the base code which is from kernel driver. How about changing the i40evf_add_mac_addr function as following:
- if (i40e_validate_mac_addr(addr->addr_bytes) != I40E_SUCCESS) {
+ if (is_zero_ether_addr (addr->addr_bytes) != I40E_SUCCESS) {

Then following modification in eth_i40e_dev_init function can be removed. But we should also pay attention to other functions which call i40evf_add_mac_addr.

Beilei

Quoted 17 lines
drivers/net/i40e/base/i40e_common.c +0 -0
 	return status;
drivers/net/i40e/i40e_ethdev.c +2 -2
index 5f26e24..00b6082 100644
@@ -1199,7 +1199,8 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 
 	/* Get and check the mac address */
 	i40e_get_mac_addr(hw, hw->mac.addr);
-	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS) {
+	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS ||
+	    I40E_IS_MULTICAST(hw->mac.addr)) {
 		PMD_INIT_LOG(ERR, "mac address is not valid");
 		ret = -EIO;
 		goto err_get_mac_addr;
--

2.10.3.dirty

author
#6 lore raw

Hi Beilei,

Quoted 8 lines

-----Original Message-----
From: Xing, Beilei [mailto:[email protected]]

Hi,

Quoted 3 lines

-----Original Message-----
From: David Harton [mailto:[email protected]]

Quoted 8 lines
Quoted 6 lines

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by removing the multicast address check in i40e_validate_mac_addr.

Maybe fix line is needed here.

Please elaborate. If feel this is a regression and you want me to add a fixline can you offer the offending commit?

Quoted 48 lines
Quoted 32 lines
Signed-off-by David Harton <[email protected]>
---
 drivers/net/i40e/base/i40e_common.c | 12 +++++-------
 drivers/net/i40e/i40e_ethdev.c      |  3 ++-
 2 files changed, 7 insertions(+), 8 deletions(-)
drivers/net/i40e/base/i40e_common.c +2 -2
index 900d379..9779854 100644
@@ -969,10 +969,10 @@ struct i40e_rx_ptype_decoded i40e_ptype_lookup[]
 = {
 
 
 /**
- * i40e_validate_mac_addr - Validate unicast MAC address
+ * i40e_validate_mac_addr - Validate MAC address
  * @mac_addr: pointer to MAC address
  *
- * Tests a MAC address to ensure it is a valid Individual Address
+ * Tests a MAC address to ensure it is a valid Address
  **/
 enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)  { @@ -

980,13 +980,11 @@ enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)

drivers/net/i40e/base/i40e_common.c +0 -1
 	DEBUGFUNC("i40e_validate_mac_addr");
 
-	/* Broadcast addresses ARE multicast addresses

drivers/net/i40e/base/i40e_common.c enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr) · line 988

Quoted 10 lines
drivers/net/i40e/base/i40e_common.c +3 -4
-	 * Make sure it is not a multicast address
+	/*
 	 * Reject the zero address
 	 */
-	if (I40E_IS_MULTICAST(mac_addr) ||
-	    (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
-	      mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0))
+	if (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
+	    mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0)
 		status = I40E_ERR_INVALID_MAC_ADDR;

We'd better not to change the base code which is from kernel driver. How about changing the i40evf_add_mac_addr function as following:

(quoted diff) +1 -1
-	if (i40e_validate_mac_addr(addr->addr_bytes) != I40E_SUCCESS) {
+	if (is_zero_ether_addr (addr->addr_bytes) != I40E_SUCCESS) {
Quoted 3 lines

Then following modification in eth_i40e_dev_init function can be removed. But we should also pay attention to other functions which call i40evf_add_mac_addr.

Sure. I'll post a new patch as suggested once the fixline question is answered.

I was trying to fix the issue the way I did in case i40e passthru wanted to benefit. Your suggestion will work for the VF as the only other caller for i40evf_add_mac_addr() performs its own macaddr validation.

Thanks,
Dave

Quoted 20 lines

Beilei

Quoted 17 lines
drivers/net/i40e/base/i40e_common.c +0 -0
 	return status;
(quoted diff) +0 -0

b/drivers/net/i40e/i40e_ethdev.c index 5f26e24..00b6082 100644

drivers/net/i40e/i40e_ethdev.c +2 -2
@@ -1199,7 +1199,8 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 
 	/* Get and check the mac address */
 	i40e_get_mac_addr(hw, hw->mac.addr);
-	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS) {
+	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS ||
+	    I40E_IS_MULTICAST(hw->mac.addr)) {
 		PMD_INIT_LOG(ERR, "mac address is not valid");
 		ret = -EIO;
 		goto err_get_mac_addr;
--

2.10.3.dirty

#7 lore raw
Quoted 25 lines

-----Original Message-----
From: David Harton (dharton) [mailto:[email protected]]
Sent: Tuesday, September 12, 2017 1:23 AM
To: Xing, Beilei <[email protected]>; Wu, Jingjing <[email protected]>
Cc: [email protected]
Subject: RE: [PATCH] i40e: fix i40e_validate_mac_addr to permit multicast addresses

Hi Beilei,

Quoted 15 lines

-----Original Message-----
From: Xing, Beilei [mailto:[email protected]]

Hi,

Quoted 9 lines

-----Original Message-----
From: David Harton [mailto:[email protected]]

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by removing the multicast address check in i40e_validate_mac_addr.

Quoted 4 lines
Quoted 1 line

Maybe fix line is needed here.

Please elaborate. If feel this is a regression and you want me to add a fixline can you offer the offending commit?

I think these two commit are related:
commit 4861cde4611601ccc9d467675f9d7a10c3095b54
commit 97ac72aa71a93d5c9bc5dc3ef1d1a324f38e61c8

Quoted 82 lines
Quoted 49 lines
Quoted 33 lines
Signed-off-by David Harton <[email protected]>
---
 drivers/net/i40e/base/i40e_common.c | 12 +++++-------
 drivers/net/i40e/i40e_ethdev.c      |  3 ++-
 2 files changed, 7 insertions(+), 8 deletions(-)
drivers/net/i40e/base/i40e_common.c +2 -2
index 900d379..9779854 100644
@@ -969,10 +969,10 @@ struct i40e_rx_ptype_decoded
 i40e_ptype_lookup[] = {
 
 
 /**
- * i40e_validate_mac_addr - Validate unicast MAC address
+ * i40e_validate_mac_addr - Validate MAC address
  * @mac_addr: pointer to MAC address
  *
- * Tests a MAC address to ensure it is a valid Individual Address
+ * Tests a MAC address to ensure it is a valid Address
  **/
 enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)  { @@ -

980,13 +980,11 @@ enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr)

drivers/net/i40e/base/i40e_common.c +0 -1
 	DEBUGFUNC("i40e_validate_mac_addr");
 
-	/* Broadcast addresses ARE multicast addresses

drivers/net/i40e/base/i40e_common.c enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr) · line 988

Quoted 10 lines
drivers/net/i40e/base/i40e_common.c +3 -4
-	 * Make sure it is not a multicast address
+	/*
 	 * Reject the zero address
 	 */
-	if (I40E_IS_MULTICAST(mac_addr) ||
-	    (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
-	      mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0))
+	if (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
+	    mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0)
 		status = I40E_ERR_INVALID_MAC_ADDR;

We'd better not to change the base code which is from kernel driver. How about changing the i40evf_add_mac_addr function as following:

(quoted diff) +1 -1
-	if (i40e_validate_mac_addr(addr->addr_bytes) != I40E_SUCCESS) {
+	if (is_zero_ether_addr (addr->addr_bytes) != I40E_SUCCESS) {
Quoted 3 lines

Then following modification in eth_i40e_dev_init function can be removed. But we should also pay attention to other functions which call i40evf_add_mac_addr.

Sure. I'll post a new patch as suggested once the fixline question is answered.

I was trying to fix the issue the way I did in case i40e passthru wanted to benefit. Your suggestion will work for the VF as the only other caller for i40evf_add_mac_addr() performs its own macaddr validation.

Thanks,
Dave

Quoted 20 lines

Beilei

Quoted 17 lines
drivers/net/i40e/base/i40e_common.c +0 -0
 	return status;
(quoted diff) +0 -0

b/drivers/net/i40e/i40e_ethdev.c index 5f26e24..00b6082 100644

drivers/net/i40e/i40e_ethdev.c +2 -2
@@ -1199,7 +1199,8 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 
 	/* Get and check the mac address */
 	i40e_get_mac_addr(hw, hw->mac.addr);
-	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS) {
+	if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS ||
+	    I40E_IS_MULTICAST(hw->mac.addr)) {
 		PMD_INIT_LOG(ERR, "mac address is not valid");
 		ret = -EIO;
 		goto err_get_mac_addr;
--

2.10.3.dirty

#8 lore raw

PATCH v2 1/1 i40e: fix i40evf_add_mac_addr to permit multicast addresses

In reply to David Harton

From: David Harton <[email protected]>

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by adjusting the check in i40evf_add_mac_addr.

Fixes 4861cde46116 ("i40e: new poll mode driver")
Fixes 97ac72aa71a9 ("i40e: support setting VF MAC address")
Signed-off-by David Harton <[email protected]>

---

v2
* Removed multicast check in i40evf_add_mac_addr.

v1
* Removed multicast check in i40e_validate_mac_addr.

 drivers/net/i40e/i40e_ethdev_vf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/i40e/i40e_ethdev_vf.c +1 -1
index f6d8293..5916d11 100644
@@ -888,7 +888,7 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
 	int err;
 	struct vf_cmd_info args;
 
-	if (i40e_validate_mac_addr(addr->addr_bytes) != I40E_SUCCESS) {
+	if (is_zero_ether_addr(addr) != I40E_SUCCESS) {
 		PMD_DRV_LOG(ERR, "Invalid mac:%x:%x:%x:%x:%x:%x",
 			    addr->addr_bytes[0], addr->addr_bytes[1],
 			    addr->addr_bytes[2], addr->addr_bytes[3],
Signature · 1 line
2.10.3.dirty
#9 lore raw

Re: [PATCH v2] i40e: fix i40evf_add_mac_addr to permit multicast addresses

Hi Harton,

Quoted 36 lines

-----Original Message-----
From: David Harton [mailto:[email protected]]
Sent: Tuesday, September 12, 2017 9:02 PM
To: Xing, Beilei <[email protected]>; Wu, Jingjing <[email protected]>
Cc: [email protected]; David Harton <[email protected]>
Subject: [PATCH v2] i40e: fix i40evf_add_mac_addr to permit multicast addresses

From: David Harton <[email protected]>

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by adjusting the check in i40evf_add_mac_addr.

Fixes 4861cde46116 ("i40e: new poll mode driver")
Fixes 97ac72aa71a9 ("i40e: support setting VF MAC address")
Signed-off-by David Harton <[email protected]>

---

v2
* Removed multicast check in i40evf_add_mac_addr.

v1
* Removed multicast check in i40e_validate_mac_addr.

 drivers/net/i40e/i40e_ethdev_vf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/i40e/i40e_ethdev_vf.c +0 -0
index f6d8293..5916d11 100644

drivers/net/i40e/i40e_ethdev_vf.c i40evf_add_mac_addr(struct rte_eth_dev *dev, · line 891

Quoted 5 lines
drivers/net/i40e/i40e_ethdev_vf.c +1 -1
@@ -888,7 +888,7 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
 	int err;
 	struct vf_cmd_info args;
 
-	if (i40e_validate_mac_addr(addr->addr_bytes) != I40E_SUCCESS) {
+	if (is_zero_ether_addr(addr) != I40E_SUCCESS) {

Thanks for the patch, there's some mistake with my last comment.
* @return
* True (1) if the given ethernet address is filled with zeros;
* false (0) otherwise.
*/
According to the comment of is_zero_ether_addr function, return value should be 0 or 1. So you should use if (!is_zero_ether_addr(addr)) here.

Quoted 5 lines
drivers/net/i40e/i40e_ethdev_vf.c +0 -1
 		PMD_DRV_LOG(ERR, "Invalid mac:%x:%x:%x:%x:%x:%x",
 			    addr->addr_bytes[0], addr->addr_bytes[1],
 			    addr->addr_bytes[2], addr->addr_bytes[3],
--

2.10.3.dirty

author
#10 lore raw

Re: [PATCH v2] i40e: fix i40evf_add_mac_addr to permit multicast addresses

Hi Beilei,

Quoted 40 lines

-----Original Message-----
From: Xing, Beilei [mailto:[email protected]]

Hi Harton,

Quoted 9 lines

-----Original Message-----
From: David Harton [mailto:[email protected]]

From: David Harton <[email protected]>

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by adjusting the check in

i40evf_add_mac_addr.

Quoted 25 lines
Fixes 4861cde46116 ("i40e: new poll mode driver")
Fixes 97ac72aa71a9 ("i40e: support setting VF MAC address")
Signed-off-by David Harton <[email protected]>

---

v2
* Removed multicast check in i40evf_add_mac_addr.

v1
* Removed multicast check in i40e_validate_mac_addr.

 drivers/net/i40e/i40e_ethdev_vf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/i40e/i40e_ethdev_vf.c +0 -0
index f6d8293..5916d11 100644
@@ -888,7 +888,7 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
 	int err;
 	struct vf_cmd_info args;
Quoted 4 lines

drivers/net/i40e/i40e_ethdev_vf.c i40evf_add_mac_addr(struct rte_eth_dev *dev, · line 891

Quoted 2 lines
drivers/net/i40e/i40e_ethdev_vf.c +1 -1
-	if (i40e_validate_mac_addr(addr->addr_bytes) != I40E_SUCCESS) {
+	if (is_zero_ether_addr(addr) != I40E_SUCCESS) {

Thanks for the patch, there's some mistake with my last comment.

Actually, I think the logic above works but I do agree it is confusing but I wanted to use your suggestion to speed up the process. :)

Quoted 7 lines

* @return
* True (1) if the given ethernet address is filled with zeros;
* false (0) otherwise.
*/
According to the comment of is_zero_ether_addr function, return value should be 0 or 1.
So you should use if (!is_zero_ether_addr(addr)) here.

I don't think so. I think the logic that should be used is if (is_zero_ether_addr(addr)) {
because we want to report an error if the addr is all 0's.

Please let me know if you agree and I'll post this patch instead.

Thanks,
Dave

Quoted 6 lines
Quoted 6 lines
drivers/net/i40e/i40e_ethdev_vf.c +0 -1
 		PMD_DRV_LOG(ERR, "Invalid mac:%x:%x:%x:%x:%x:%x",
 			    addr->addr_bytes[0], addr->addr_bytes[1],
 			    addr->addr_bytes[2], addr->addr_bytes[3],
--

2.10.3.dirty

#11 lore raw

Re: [PATCH v2] i40e: fix i40evf_add_mac_addr to permit multicast addresses

Quoted 69 lines

-----Original Message-----
From: David Harton (dharton) [mailto:[email protected]]
Sent: Wednesday, September 13, 2017 10:38 AM
To: Xing, Beilei <[email protected]>; David Harton <dharton@cpp-rtpbld- 31.cpprtplab>; Wu, Jingjing <[email protected]>
Cc: [email protected]
Subject: RE: [PATCH v2] i40e: fix i40evf_add_mac_addr to permit multicast addresses

Hi Beilei,

Quoted 40 lines

-----Original Message-----
From: Xing, Beilei [mailto:[email protected]]

Hi Harton,

Quoted 9 lines

-----Original Message-----
From: David Harton [mailto:[email protected]]

From: David Harton <[email protected]>

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by adjusting the check in

i40evf_add_mac_addr.

Quoted 25 lines
Fixes 4861cde46116 ("i40e: new poll mode driver")
Fixes 97ac72aa71a9 ("i40e: support setting VF MAC address")
Signed-off-by David Harton <[email protected]>

---

v2
* Removed multicast check in i40evf_add_mac_addr.

v1
* Removed multicast check in i40e_validate_mac_addr.

 drivers/net/i40e/i40e_ethdev_vf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/i40e/i40e_ethdev_vf.c +0 -0
index f6d8293..5916d11 100644
@@ -888,7 +888,7 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
 	int err;
 	struct vf_cmd_info args;
Quoted 4 lines

drivers/net/i40e/i40e_ethdev_vf.c i40evf_add_mac_addr(struct rte_eth_dev *dev, · line 891

Quoted 2 lines
drivers/net/i40e/i40e_ethdev_vf.c +1 -1
-	if (i40e_validate_mac_addr(addr->addr_bytes) != I40E_SUCCESS) {
+	if (is_zero_ether_addr(addr) != I40E_SUCCESS) {

Thanks for the patch, there's some mistake with my last comment.

Actually, I think the logic above works but I do agree it is confusing but I wanted to use your suggestion to speed up the process. :)

Quoted 7 lines

* @return
* True (1) if the given ethernet address is filled with zeros;
* false (0) otherwise.
*/
According to the comment of is_zero_ether_addr function, return value should be 0 or 1.
So you should use if (!is_zero_ether_addr(addr)) here.

Quoted 4 lines

I don't think so. I think the logic that should be used is if (is_zero_ether_addr(addr)) { because we want to report an error if the addr is all 0's.

Yes, correct:)

Quoted 11 lines

Please let me know if you agree and I'll post this patch instead.

Thanks,
Dave

Quoted 6 lines
Quoted 6 lines
drivers/net/i40e/i40e_ethdev_vf.c +0 -1
 		PMD_DRV_LOG(ERR, "Invalid mac:%x:%x:%x:%x:%x:%x",
 			    addr->addr_bytes[0], addr->addr_bytes[1],
 			    addr->addr_bytes[2], addr->addr_bytes[3],
--

2.10.3.dirty

#12 lore raw

PATCH v3 1/1 i40e: fix i40evf_add_mac_addr to permit multicast addresses

In reply to David Harton

From: David Harton <[email protected]>

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by adjusting the check in i40evf_add_mac_addr.

Fixes 4861cde46116 ("i40e: new poll mode driver")
Fixes 97ac72aa71a9 ("i40e: support setting VF MAC address")
Signed-off-by David Harton <[email protected]>

---

v3
* Modified diff to use more conventional logic.

v2
* Removed multicast check in i40evf_add_mac_addr.

v1
* Removed multicast check in i40e_validate_mac_addr.

 drivers/net/i40e/i40e_ethdev_vf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/i40e/i40e_ethdev_vf.c +1 -1
index f6d8293..08afd9e 100644
@@ -888,7 +888,7 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
 	int err;
 	struct vf_cmd_info args;
 
-	if (i40e_validate_mac_addr(addr->addr_bytes) != I40E_SUCCESS) {
+	if (is_zero_ether_addr(addr)) {
 		PMD_DRV_LOG(ERR, "Invalid mac:%x:%x:%x:%x:%x:%x",
 			    addr->addr_bytes[0], addr->addr_bytes[1],
 			    addr->addr_bytes[2], addr->addr_bytes[3],
Signature · 1 line
2.10.3.dirty
Acked-by on v3 1/1
#13 lore raw

Re: [PATCH v3] i40e: fix i40evf_add_mac_addr to permit multicast addresses

Quoted 17 lines

-----Original Message-----
From: David Harton [mailto:[email protected]]
Sent: Wednesday, September 13, 2017 11:21 AM
To: Xing, Beilei <[email protected]>; Wu, Jingjing <[email protected]>
Cc: [email protected]; David Harton <[email protected]>
Subject: [PATCH v3] i40e: fix i40evf_add_mac_addr to permit multicast addresses

From: David Harton <[email protected]>

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by adjusting the check in i40evf_add_mac_addr.

Quoted 4 lines
Fixes 4861cde46116 ("i40e: new poll mode driver")
Fixes 97ac72aa71a9 ("i40e: support setting VF MAC address")
Signed-off-by David Harton <[email protected]>
Acked-by Beilei Xing <[email protected]>
Applied
#14 lore raw

Re: [PATCH v3] i40e: fix i40evf_add_mac_addr to permit multicast addresses

Quoted 17 lines · Beilei
Quoted 17 lines

-----Original Message-----
From: David Harton [mailto:[email protected]]
Sent: Wednesday, September 13, 2017 11:21 AM
To: Xing, Beilei <[email protected]>; Wu, Jingjing <[email protected]>
Cc: [email protected]; David Harton <[email protected]>
Subject: [PATCH v3] i40e: fix i40evf_add_mac_addr to permit multicast addresses

From: David Harton <[email protected]>

The i40e maintains a single MAC filter table for both unicast and multicast addresses. The i40e_validate_mac_addr function was preventing multicast addresses from being added to the table via i40evf_add_mac_addr. Fixed the issue by adjusting the check in i40evf_add_mac_addr.

Quoted 6 lines
Quoted 4 lines
Fixes 4861cde46116 ("i40e: new poll mode driver")
Fixes 97ac72aa71a9 ("i40e: support setting VF MAC address")
Signed-off-by David Harton <[email protected]>
Acked-by Beilei Xing <[email protected]>

Applied to dpdk-next-net/master, thanks.