From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 36763A051A; Fri, 17 Jan 2020 04:04:14 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4387E1D54C; Fri, 17 Jan 2020 04:04:13 +0100 (CET) Received: from m12-11.163.com (m12-11.163.com [220.181.12.11]) by dpdk.org (Postfix) with ESMTP id 39BB61D51E for ; Fri, 17 Jan 2020 04:04:10 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=163.com; s=s110527; h=Subject:From:Message-ID:Date:MIME-Version; bh=uvg4A BMRbkPvaXBvC9kHC+hLrmTddvQ/m3mk0r41REk=; b=g0nT04jsTKcnkBJoPH35o MT5eq0+TwCGFp5qhGxK0TsmePI1wbcgZzUb8CM0b1N6aueAKdBS7puWnMllL0buY xLlWB5Y5edKnekCz3SEmbHY4qn8QBZO0EFxIBTTVzUa977BowI3Sc/tmSd8DKgT2 EzHpYyrygdWLXZwdItqdzs= Received: from [192.168.1.199] (unknown [139.159.243.11]) by smtp7 (Coremail) with SMTP id C8CowABnaQUnJCFe9AEUHg--.6916S2; Fri, 17 Jan 2020 11:04:07 +0800 (CST) To: Ferruh Yigit , "Wei Hu (Xavier)" , dev@dpdk.org Cc: Andrew Rybchenko , Thomas Monjalon References: <20200116122703.9199-1-huwei013@chinasoftinc.com> From: "Wei Hu (Xavier)" Message-ID: <3fdd5077-f672-852b-4dd4-3c5ed50de375@163.com> Date: Fri, 17 Jan 2020 11:04:07 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-CM-TRANSID: C8CowABnaQUnJCFe9AEUHg--.6916S2 X-Coremail-Antispam: 1Uf129KBjvJXoWxZF15AFyxWFykCrW8Aw4kCrg_yoW5Gw1rpF Z5KFWFya1xWF13WFy7uayFk34j9Fs7tF42yrWUJFW7J3s8ZFyxury5K34qqry7Z3s7tr4j vrW8Zrna9FyUZ3JanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDUYxBIdaVFxhVjvjDU0xZFpf9x07bn3ktUUUUU= X-Originating-IP: [139.159.243.11] X-CM-SenderInfo: 50dyxv5ubk34lhl6il2tof0z/xtbBzwito1aD6FojfAAAsx Subject: Re: [dpdk-dev] [PATCH] ethdev: move non null judgment of ops function pointer ahead X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, Ferruh Yigit On 2020/1/17 5:08, Ferruh Yigit wrote: > On 1/16/2020 12:27 PM, Wei Hu (Xavier) wrote: >> From: "Wei Hu (Xavier)" >> >> This patch moves the following judgement statement to the first half of >> the function named rte_eth_dev_set_vlan_offload, so we can avoid changing >> the content of dev->data->dev_conf.rxmode.offloads even when the pointer >> named dev->dev_ops->vlan_offload_set is NULL: >> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); >> >> Signed-off-by: Wei Hu (Xavier) >> Signed-off-by: Chunsong Feng >> Signed-off-by: Min Wang (Jushui) >> Signed-off-by: Min Hu (Connor) >> --- >> lib/librte_ethdev/rte_ethdev.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c >> index aec2d0f70..21f24cd7f 100644 >> --- a/lib/librte_ethdev/rte_ethdev.c >> +++ b/lib/librte_ethdev/rte_ethdev.c >> @@ -3257,6 +3257,7 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) >> >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> dev = &rte_eth_devices[port_id]; >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); >> >> /* save original values in case of failure */ >> orig_offloads = dev->data->dev_conf.rxmode.offloads; >> @@ -3307,7 +3308,6 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) >> if (mask == 0) >> return ret; >> >> - RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); >> ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); >> if (ret) { >> /* hit an error restore original values */ >> > > I agree this side affect is a defect, thanks for spotting. > > Above approach would work, but also I think it was good approach to return > success (0) when no change at all requested, we are loosing it here. > > What do you think, > - Update a local 'dev_offloads' variable, instead of directly updating the > device config data > - if no change requested, "mask == 0", return success > - if change requested, check the 'vlan_offload_set' dev_ops in this stage > - assign the local 'dev_offloads' to 'dev->data->dev_conf.rxmode.offloads' and > call the dev_ops > - On error recover the 'dev->data->dev_conf.rxmode.offloads' to 'orig_offloads' > > This way both side affect can be prevented and API returns success if no change > requested. > I agree your opinion and will send V2. Thanks Xavier