Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> The dev->data->mac_addrs[0] will be changed to a new MAC address when
> applications modify the default MAC address by
> rte_eth_dev_default_mac_addr_set() API. However, If the new default
> MAC address has been added as a non-default MAC address by
> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
> address occupies two index capacities in dev->data->mac_addrs[].
> 

Hi Connor,

I see the problem, but can you please clarify what is the impact to the end user?

If application does as following:
   rte_eth_dev_mac_addr_add(MAC1);
   rte_eth_dev_mac_addr_add(MAC2);
   rte_eth_dev_mac_addr_add(MAC3);
   rte_eth_dev_default_mac_addr_set(MAC2);

The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which has 'MAC2' duplicated.

Will this cause any problem for the application to receive the packets
with 'MAC2' address?

Or is the only problem one extra space used in 'dev->data->mac_addrs[]'
without any other impact to the application?

> This patch adds the logic of removing MAC addresses for this scenario.
> 
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> v2:
> * fixed commit log.
> ---
>   lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 028907bc4b..7faff17d9a 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -4340,6 +4340,7 @@ int
>   rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>   {
>   	struct rte_eth_dev *dev;
> +	int index;
>   	int ret;
>   
>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> @@ -4361,6 +4362,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>   	if (ret < 0)
>   		return ret;
>   
> +	/*
> +	 * If the address has been added as a non-default MAC address by
> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
> +	 * dev->data->mac_addrs[].
> +	 */
> +	index = eth_dev_get_mac_addr_index(port_id, addr);
> +	if (index > 0) {
> +		/* remove address in NIC data structure */
> +		rte_ether_addr_copy(&null_mac_addr,
> +				    &dev->data->mac_addrs[index]);
> +		/* reset pool bitmap */
> +		dev->data->mac_pool_sel[index] = 0;
> +	}
> +

Here only 'dev->data->mac_addrs[]' array is updated and it assumes
driver removes similar duplication itself, but I am not sure if this is
valid for all drivers.

If driver is not removing the duplicate in the HW configuration, the driver
config and 'dev->data->mac_addrs[]' will diverge, which is not good.

What about following logic to be sure HW configuration and
'dev->data->mac_addrs[]' is same:

   index = eth_dev_get_mac_addr_index(port_id, addr);
   if (index > 0)
       rte_eth_dev_mac_addr_remove(port_id, addr);
   (*dev->dev_ops->mac_addr_set)(dev, addr);

>   	/* Update default address in NIC data structure */
>   	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>   
>