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

[Huisong Li <lihuisong@huawei.com>]

在 2021/9/22 16:02, Andrew Rybchenko 写道:
> On 9/22/21 10:43 AM, Huisong Li wrote:
>> 在 2021/9/22 14:39, Andrew Rybchenko 写道:
>>> On 9/22/21 6:36 AM, Min Hu (Connor) wrote:
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>> Use the testpmd to perform the following operations:
>>>> 1) mac_addr add 0 00:18:2D:00:00:90
>>>> 2) mac_addr add 0 00:18:2D:00:00:91
>>>> 3) mac_addr add 0 00:18:2D:00:00:92
>>>> 4) mac_addr set 0 00:18:2D:00:00:91
>>>> 5) show port 0 macs
>>>> Number of MAC address added: 4
>>>>     00:18:2D:00:00:91
>>>>     00:18:2D:00:00:90
>>>>     00:18:2D:00:00:91
>>>>     00:18:2D:00:00:92
>>>>
>>>> This is due to the reason that if the address has been added as a
>>>> non-default MAC address by rte_eth_dev_mac_addr_add API, it doesn't
>>>> remove
>>>> from dev->data->mac_addrs[] when set default MAC address with the same
>>>> address.
>>>>
>>>> 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>
>>>> ---
>>>>    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 daf5ca9242..77657a3314 100644
>>>> --- a/lib/ethdev/rte_ethdev.c
>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>> @@ -4360,6 +4360,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);
>>>> @@ -4381,6 +4382,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;
>>>> +    }
>>>> +
>>>>        /* Update default address in NIC data structure */
>>>>        rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>>   
>>> If I change default MAC to something else later, should the old
>>> default MAC be returned to some specific pools? I guess it is
>> Since the old default MAC address is invalid, which has been removed
>> from hardware.
>>
>> This MAC address does not need to be added to some specific pools, and
>> occupies
>>
>> one position in mac addrs.
>>
>>> hard to do. If the change is accepted, the behaviour must be
>>> documented in rte_eth_dev_default_mac_addr_set() description.
>>> .
>> I think it's not necessary. Because old default MAC is no longer valid
>> if we modify
>>
>> default MAC with a new MAC address. This is a definition of
>> rte_eth_dev_default_mac_addr_set().
>>
>> The current modification does not change the definition of the interface.
> Not sure that I understand:
>
> set default MAC-A
> add MAC-B to pool 1
> set default MAC-B
> set default MAC-C
>
> since I've not removed MAC-B from pool 1, I'd expect it to be
> there.
> .

Yes. You are right.

But MAC-B is removed from hardware after executing "set default MAC-C" 
cmd, and

new default MAC-C takes effect.

View from interface rte_eth_dev_mac_addr_remove(), if one MAC address is 
removed,

the API removes the MAC from dev->data->mac_addrs[] and reset 
"mac_pool_sel"(that is,

all pools do not have the MAC address.).

This also means that above MAC-B should not exist in any of the pools and

in dev->data->mac_addrs[] after executing "set default MAC-C" cmd.