diff mbox series

[V6] ethdev: fix one address occupies two entries in MAC addrs

Message ID 20230131064154.10571-1-lihuisong@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [V6] ethdev: fix one address occupies two entries in MAC addrs |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-testing fail Testing issues
ci/iol-x86_64-unit-testing fail Testing issues
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

lihuisong (C) Jan. 31, 2023, 6:41 a.m. UTC 
  The dev->data->mac_addrs[0] will be changed to a new MAC address when
applications modify the default MAC address by .mac_addr_set(). However,
if the new default one has been added as a non-default MAC address by
.mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
list. As a result, one MAC address occupies two entries in the list. Like:
add(MAC1)
add(MAC2)
add(MAC3)
add(MAC4)
set_default(MAC3)
default=MAC3, the rest of list=MAC1, MAC2, MAC3, MAC4
Note: MAC3 occupies two entries.

In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
old default MAC when set default MAC. If user continues to do
set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
but packets with MAC3 aren't actually received by the PMD.

So need to ensure that the new default address is removed from the rest of
the list.

Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
v6: fix commit log and some code comments.                                 
v5:                                                                        
 - merge the second patch into the first patch.                            
 - add error log when rollback failed.                                     
v4:                                                                        
  - fix broken in the patchwork                                            
v3:                                                                        
  - first explicitly remove the non-default MAC, then set default one.     
  - document default and non-default MAC address                           
v2:                                                                        
  - fixed commit log.

---
 lib/ethdev/ethdev_driver.h |  6 +++++-
 lib/ethdev/rte_ethdev.c    | 35 +++++++++++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 3 deletions(-)

Comments

Thomas Monjalon Feb. 1, 2023, 10:42 a.m. UTC | #1 
31/01/2023 07:41, Huisong Li:
> The dev->data->mac_addrs[0] will be changed to a new MAC address when
> applications modify the default MAC address by .mac_addr_set(). However,
> if the new default one has been added as a non-default MAC address by
> .mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
> list. As a result, one MAC address occupies two entries in the list. Like:
> add(MAC1)
> add(MAC2)
> add(MAC3)
> add(MAC4)
> set_default(MAC3)
> default=MAC3, the rest of list=MAC1, MAC2, MAC3, MAC4
> Note: MAC3 occupies two entries.
> 
> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
> old default MAC when set default MAC. If user continues to do
> set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
> MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
> but packets with MAC3 aren't actually received by the PMD.
> 
> So need to ensure that the new default address is removed from the rest of
> the list.
> 
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  lib/ethdev/ethdev_driver.h |  6 +++++-
>  lib/ethdev/rte_ethdev.c    | 35 +++++++++++++++++++++++++++++++++--
>  2 files changed, 38 insertions(+), 3 deletions(-)

This is a behavior change.
It must be noted in the release notes in the API section.

> -	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
> +	/**
> +	 * Device Ethernet link addresses. The first entry (index zero) is the
> +	 * default address. The rest of list cannot be the same as the default
> +	 * address.
> +	 */

Please split the lines after a dot.

rest of "the" list

Maybe better for this field:
	All entries are unique.
	The first entry (index zero) is the default address.

>  	struct rte_ether_addr *mac_addrs;

and for the function rte_eth_dev_default_mac_addr_set:
	Set the default MAC address.
	It replaces the address at index 0 of the address list.
	If the address was already in the MAC address list,
	it is removed from the rest of the list.
lihuisong (C) Feb. 1, 2023, 12:26 p.m. UTC | #2 
在 2023/2/1 18:42, Thomas Monjalon 写道:
> 31/01/2023 07:41, Huisong Li:
>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>> applications modify the default MAC address by .mac_addr_set(). However,
>> if the new default one has been added as a non-default MAC address by
>> .mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
>> list. As a result, one MAC address occupies two entries in the list. Like:
>> add(MAC1)
>> add(MAC2)
>> add(MAC3)
>> add(MAC4)
>> set_default(MAC3)
>> default=MAC3, the rest of list=MAC1, MAC2, MAC3, MAC4
>> Note: MAC3 occupies two entries.
>>
>> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
>> old default MAC when set default MAC. If user continues to do
>> set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
>> MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
>> but packets with MAC3 aren't actually received by the PMD.
>>
>> So need to ensure that the new default address is removed from the rest of
>> the list.
>>
>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>   lib/ethdev/ethdev_driver.h |  6 +++++-
>>   lib/ethdev/rte_ethdev.c    | 35 +++++++++++++++++++++++++++++++++--
>>   2 files changed, 38 insertions(+), 3 deletions(-)
> This is a behavior change.
> It must be noted in the release notes in the API section.
ok. I will add an announcement.
>> -	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
>> +	/**
>> +	 * Device Ethernet link addresses. The first entry (index zero) is the
>> +	 * default address. The rest of list cannot be the same as the default
>> +	 * address.
>> +	 */
> Please split the lines after a dot.
>
> rest of "the" list
>
> Maybe better for this field:
> 	All entries are unique.
> 	The first entry (index zero) is the default address.
It's more simpler. Thank you for your suggestion.
>>   	struct rte_ether_addr *mac_addrs;
> and for the function rte_eth_dev_default_mac_addr_set:
> 	Set the default MAC address.
> 	It replaces the address at index 0 of the address list.
> 	If the address was already in the MAC address list,
> 	it is removed from the rest of the list.
Indeed, It's better to document it.
>
>
>
> .
diff mbox series

Patch

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index dde3ec84ef..77acdfb95f 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -117,7 +117,11 @@  struct rte_eth_dev_data {
 
 	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
 
-	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
+	/**
+	 * Device Ethernet link addresses. The first entry (index zero) is the
+	 * default address. The rest of list cannot be the same as the default
+	 * address.
+	 */
 	struct rte_ether_addr *mac_addrs;
 	/** Bitmap associating MAC addresses to pools */
 	uint64_t mac_pool_sel[RTE_ETH_NUM_RECEIVE_MAC_ADDR];
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 86ca303ab5..de25183619 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -4498,7 +4498,10 @@  rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
 int
 rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
 {
+	uint64_t mac_pool_sel_bk = 0;
 	struct rte_eth_dev *dev;
+	uint32_t pool;
+	int index;
 	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
@@ -4517,16 +4520,44 @@  rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
 	if (*dev->dev_ops->mac_addr_set == NULL)
 		return -ENOTSUP;
 
+	/* Keep address unique in dev->data->mac_addrs[]. */
+	index = eth_dev_get_mac_addr_index(port_id, addr);
+	if (index > 0) {
+		/* Remove address in dev data structure */
+		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
+		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
+		if (ret < 0) {
+			RTE_ETHDEV_LOG(ERR, "Cannot remove the port %u address from the rest of list.\n",
+				       port_id);
+			return ret;
+		}
+	}
 	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	/* Update default address in NIC data structure */
 	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
 
 	return 0;
-}
 
+out:
+	if (index > 0) {
+		pool = 0;
+		do {
+			if (mac_pool_sel_bk & UINT64_C(1)) {
+				if (rte_eth_dev_mac_addr_add(port_id, addr,
+							     pool) != 0)
+					RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool id(%u) in port %u.\n",
+						       pool, port_id);
+			}
+			mac_pool_sel_bk >>= 1;
+			pool++;
+		} while (mac_pool_sel_bk != 0);
+	}
+
+	return ret;
+}
 
 /*
  * Returns index into MAC address array of addr. Use 00:00:00:00:00:00 to find