From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 9FDB241BA5; Thu, 2 Feb 2023 02:11:14 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2EB5840DFB; Thu, 2 Feb 2023 02:11:14 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 044774067B for ; Thu, 2 Feb 2023 02:11:11 +0100 (CET) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.53]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4P6gfH0y6MzRr5x; Thu, 2 Feb 2023 09:08:55 +0800 (CST) Received: from [10.67.103.231] (10.67.103.231) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.34; Thu, 2 Feb 2023 09:11:08 +0800 Message-ID: <86a52aa2-4d1a-9243-4ca9-5ca49ed69652@huawei.com> Date: Thu, 2 Feb 2023 09:11:08 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH V7] ethdev: fix one address occupies two entries in MAC addrs To: Thomas Monjalon CC: , , , , , References: <20221020093102.20679-1-lihuisong@huawei.com> <20230201131501.15698-1-lihuisong@huawei.com> <3915735.ZaRXLXkqSa@thomas> From: "lihuisong (C)" In-Reply-To: <3915735.ZaRXLXkqSa@thomas> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To kwepemm600004.china.huawei.com (7.193.23.242) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 在 2023/2/2 0:37, Thomas Monjalon 写道: > 01/02/2023 14:15, 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 if the address was already in the list. >> >> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier") >> Cc: stable@dpdk.org >> >> Signed-off-by: Huisong Li >> Acked-by: Chengwen Feng > [...] >> --- a/doc/guides/rel_notes/release_23_03.rst >> +++ b/doc/guides/rel_notes/release_23_03.rst >> @@ -101,10 +101,16 @@ API Changes >> Use fixed width quotes for ``function_names`` or ``struct_names``. >> Use the past tense. >> >> + > useless line? will remove. > >> This section is a comment. Do not overwrite or remove it. >> Also, make sure to start the actual text at the margin. >> ======================================================= > Please check the comment: "make sure to start the actual text at the margin." It means to start the actual text  at the beginning of the line, right? > >> + * ethdev: ensured all entries in MAC address list is unique. > is -> are uniques All right. > >> + The function ``rte_eth_dev_default_mac_addr_set`` replaces the address >> + at index 0 of the address list. If the address was already in the >> + address list, it is removed from the rest of the list. > You need to highlight what changed: > > When setting a default MAC address with the function > ``rte_eth_dev_default_mac_addr_set``, > the address is now removed from the rest of the address list > in order to ensure it is only at index 0 of the list. ok, highlight it. > > [...] >> --- 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. >> + * All entries are unique. The first entry (index zero) is the >> + * default address. >> + */ > You remember I asked to split lines after the dot? Sorry for my misunderstanding. Need to occupy one line for each sentence here? Do we generally use this style to make comments? or are we just trying to highlight it here? > >> /** >> * Set the default MAC address. >> + * It replaces the address at index 0 of the MAC address list. >> + * If the address was already in the MAC address list, it is removed from >> + * the rest of the list. > Here you can split after the comma. Ack > > > > .