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 1459DA04FD; Thu, 2 Jun 2022 08:58:53 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B451340694; Thu, 2 Jun 2022 08:58:52 +0200 (CEST) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id AACA14021E for ; Thu, 2 Jun 2022 08:58:51 +0200 (CEST) Received: from kwepemi500012.china.huawei.com (unknown [172.30.72.53]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4LDGz85d81z1K9Mp for ; Thu, 2 Jun 2022 14:57:08 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by kwepemi500012.china.huawei.com (7.221.188.12) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Thu, 2 Jun 2022 14:58:49 +0800 Subject: Re: [PATCH v4] kni: fix device address set To: Andrew Rybchenko , References: <20220406082213.45750-1-humin29@huawei.com> <20220601015925.35304-1-humin29@huawei.com> <8b6be4bb-2057-218c-af68-d3e21d9d85c2@oktetlabs.ru> From: "Min Hu (Connor)" Message-ID: Date: Thu, 2 Jun 2022 14:58:48 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <8b6be4bb-2057-218c-af68-d3e21d9d85c2@oktetlabs.ru> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To kwepemi500012.china.huawei.com (7.221.188.12) 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 Hi, Andrew, 在 2022/6/1 17:02, Andrew Rybchenko 写道: > On 6/1/22 04:59, Min Hu (Connor) wrote: >> Currently, run KNI APP When Kernel version is 5.17. When quit the APP, >> dmesg buffer get calltrace, info like: >> [ 5965.847401] rte_kni: Creating kni... >> [ 6225.627205] vEth0 (unregistered): Current addr:  70 fd 45 d0 72 a7 >> 00.. >> [ 6225.640113] vEth0 (unregistered): Expected addr: 00 00 00 00 00 00 >> 00.. >> [ 6225.653010] ------------[ cut here ]------------ >> [ 6225.657609] netdevice: vEth0 (unregistered): Incorrect >> netdev->dev_addr >> [ 6225.832647] Call trace: >> [ 6225.835083]  dev_addr_check+0xa0/0x144 >> [ 6225.838816]  dev_addr_flush+0x30/0x9c >> [ 6225.842462]  free_netdev+0x8c/0x1e0 >> [ 6225.845939]  kni_release+0xc0/0x1d0 [rte_kni] >> [ 6225.850281]  __fput+0x78/0x220 >> [ 6225.853327]  ____fput+0x1c/0x30 >> [ 6225.856455]  task_work_run+0x88/0xc0 >> [ 6225.860017]  do_exit+0x2fc/0x940 >> [ 6225.863232]  do_group_exit+0x40/0xac >> [ 6225.866791]  get_signal+0x190/0x960 >> [ 6225.870265]  do_notify_resume+0x26c/0x1360 >> [ 6225.874346]  el0_interrupt+0x60/0xe0 >> [ 6225.877910]  __el0_irq_handler_common+0x18/0x24 >> [ 6225.882420]  el0t_64_irq_handler+0x14/0x20 >> [ 6225.886499]  el0t_64_irq+0x1a0/0x1a4 >> [ 6225.890059] ---[ end trace 0000000000000000 ]--- >> [ 6245.598157] rte_kni: Creating kni... >> >> The reason is that 5.17 kernel introduce 'dev_addr_shadow' in function >> 'dev_addr_set' to ensure that netdev->dev_addr should only be modified >> via helpers('dev_addr_set'). 'dev_addr_check' will check if >> netdev->dev_addr is modified by other ways, like 'memcpy'. >> >> More info could get by referring to kernel patch: >> https://patchwork.kernel.org/project/netdevbpf/patch/ >> 20211118041501.3102861-8-kuba@kernel.org/ >> https://www.spinics.net/lists/netdev/msg764992.html >> >> Fixes: ea6b39b5b847 ("kni: remove ethtool support") >> >> Signed-off-by: Min Hu (Connor) >> --- >> v3,v4: >> * fix compiling errors. >> >> v2: >> * change 'memcpy' to 'ether_addr_copy' to copy device addr. >> --- >>   kernel/linux/kni/kni_misc.c | 10 ++++++++-- >>   1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c >> index 780187d8bf..9c5d8c5576 100644 >> --- a/kernel/linux/kni/kni_misc.c >> +++ b/kernel/linux/kni/kni_misc.c >> @@ -299,6 +299,7 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num, >>       struct kni_net *knet = net_generic(net, kni_net_id); >>       int ret; >>       struct rte_kni_device_info dev_info; >> +    unsigned char mac_addr[ETH_ALEN]; >>       struct net_device *net_dev = NULL; >>       struct kni_dev *kni, *dev, *n; >> @@ -403,10 +404,15 @@ kni_ioctl_create(struct net *net, uint32_t >> ioctl_num, >>       /* if user has provided a valid mac address */ >>       if (is_valid_ether_addr(dev_info.mac_addr)) >> -        memcpy(net_dev->dev_addr, dev_info.mac_addr, ETH_ALEN); >> +        ether_addr_copy(mac_addr, dev_info.mac_addr); >>       else >>           /* Generate random MAC address. */ >> -        eth_random_addr(net_dev->dev_addr); >> +        eth_random_addr(mac_addr); > > Isn't it better to use function provided by the kernel, if available? > I mean eth_hw_addr_random() used in [1] Well, here 'mac_addr' is my object, this object will be used in 'dev_addr_set' in the next code. using eth_hw_addr_random will directly operate net-dev->dev_addr. > > [1] > https://patches.dpdk.org/project/dpdk/patch/20220601054525.7573-1-ke1x.zhang@intel.com/ > > >> +#if KERNEL_VERSION(5, 17, 0) > LINUX_VERSION_CODE > > I think usage of HAVE_* defines like in [1] look much better. > It is easier to read and will limit scope if condition is finally > more complicated. Yes, fixed in v5. > > [1] > https://patches.dpdk.org/project/dpdk/patch/20220525102641.20982-1-jslaby@suse.cz/ > > >> +    ether_addr_copy(net_dev->dev_addr, mac_addr); >> +#else >> +    dev_addr_set(net_dev, mac_addr); >> +#endif >>       if (dev_info.mtu) >>           net_dev->mtu = dev_info.mtu; > > .