From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 915E32C15 for ; Fri, 1 Sep 2017 00:04:06 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 2DF5420B5B; Thu, 31 Aug 2017 18:04:06 -0400 (EDT) Received: from frontend1 ([10.202.2.160]) by compute1.internal (MEProxy); Thu, 31 Aug 2017 18:04:06 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=Wul4Q9WOM99r5CB SFgEJ4EsjcFl6wy95PwNMph5yFWM=; b=ULudnNyIfTBzA96YihnpAQA2ainG1Tl +ClhRPQWYHPUU6+DkTH1YOIcuLT6BFFUpSafRcR27OpYanT9wK5mEJLFlobt4BBQ iSgezz20hNIZj3YDhpTmD1TxUq38eN0trJ4GmD1h7t6ckMw61bIZQwdksvlTYiJR 01rYrWbJvcPo= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-sender:x-me-sender:x-sasl-enc:x-sasl-enc; s= fm1; bh=Wul4Q9WOM99r5CBSFgEJ4EsjcFl6wy95PwNMph5yFWM=; b=a/5xp2ZU 86B8e4fpwHgatlY6lUE9K+MXz/NLtCVJvkqhnzB7e93HFUSehPZeT2OaDS8LGJJo xGrdW6ZVmP/4cN/+zB7KbrGpS5nhadtYYhC6tfGbSMcA17t39ohVaXeqlWwWcvm2 taPFEVQnw/SJEejarnvXe/x02neJVXYE75N0G5kASLED9WlTIAWUm33Yuz04K+i0 0bId9XA/YU/92ijKcvZ3w3loNlv8XSKoTTQxxLhXLKPQSFv6K0GG22mIulYJjjwT d2uCqwMZYO0hgFBfOLsQOff5Mq5pgmvL2f0Qhw4lehjzXvhwuRJHdBt8YmUeboUR ugajK1CdOhFIGg== X-ME-Sender: X-Sasl-enc: rByuUrwyP3FVGwu0ipcDVDQs4iwDCffhCiuULxrWAmRF 1504217045 Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id ACCD07FA6A; Thu, 31 Aug 2017 18:04:05 -0400 (EDT) From: Thomas Monjalon To: David Harton Cc: dev@dpdk.org, ferruh.yigit@intel.com, ajit.khaparde@broadcom.com, johndale@cisco.com, konstantin.ananyev@intel.com, jingjing.wu@intel.com, beilei.xing@intel.com, jing.d.chen@intel.com, adrien.mazarguil@6wind.com, nelio.laranjeiro@6wind.com, alejandro.lucero@netronome.com, hemant.agrawal@nxp.com, rasesh.mody@cavium.com, harish.patil@cavium.com, skhare@vmware.com, yliu@fridaylinux.org, maxime.coquelin@redhat.com, allain.legacy@windriver.com Date: Fri, 01 Sep 2017 00:04:04 +0200 Message-ID: <2775824.X3T22OWxbu@xps> In-Reply-To: <20170825134717.18376-1-dharton@cisco.com> References: <20170825133319.15207-1-dharton@cisco.com> <20170825134717.18376-1-dharton@cisco.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v3] ethdev: modifiy vlan_offload_set_t to return int 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: , X-List-Received-Date: Thu, 31 Aug 2017 22:04:06 -0000 Hi, 25/08/2017 15:47, David Harton: > Some devices may not support or fail setting VLAN offload > configuration based on dynamic circurmstances so the > vlan_offload_set_t vector is modified to return an int so > the caller can determine success or not. I agree with allowing to return an error. Comments on details below. The title could be changed to better reflect the purpose: ethdev: allow returning error on VLAN configuration > --- a/doc/guides/rel_notes/release_17_11.rst > +++ b/doc/guides/rel_notes/release_17_11.rst > @@ -124,7 +124,7 @@ ABI Changes > Also, make sure to start the actual text at the margin. > ========================================================= > > - > +* Changed return type of ``vlan_offload_set_t`` from ``void`` to ``int``. It should be referenced as an API change (instead of ABI change). (and line spacing must be kept) We also need to bump the library version but it can be done with bigger changes in ethdev. > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -2049,10 +2049,16 @@ struct rte_eth_dev * > int ret = 0; > int mask = 0; > int cur, org = 0; > + uint8_t org_strip, org_filter, org_extend; > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > dev = &rte_eth_devices[port_id]; > > + /* save original values in case of failure */ > + org_strip = dev->data->dev_conf.rxmode.hw_vlan_strip; > + org_filter = dev->data->dev_conf.rxmode.hw_vlan_filter; > + org_extend = dev->data->dev_conf.rxmode.hw_vlan_extend; Please waste one more char to write "orig" instead of "org". > - (*dev->dev_ops->vlan_offload_set)(dev, mask); > + ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); > + if (ret) { > + /* hit an error restore original values */ > + dev->data->dev_conf.rxmode.hw_vlan_strip = org_strip; > + dev->data->dev_conf.rxmode.hw_vlan_filter = org_filter; > + dev->data->dev_conf.rxmode.hw_vlan_extend = org_extend; > + } Isn't it the responsibility of the PMD to restore values in case of error? I understand it is there to factorize error handling code, right? Do we want to document this behaviour with the ops prototype?