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 EF7EF2C17 for ; Fri, 1 Sep 2017 00:08:29 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id A4F1C20D9C; Thu, 31 Aug 2017 18:08:29 -0400 (EDT) Received: from frontend2 ([10.202.2.161]) by compute1.internal (MEProxy); Thu, 31 Aug 2017 18:08:29 -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=TbsiJdeJnXY7FDo j7nB0jihTiQSSZg1YeBS9Dcay/JA=; b=rOo1VHTSOWgUx5jEFfckemoeKlVDOyx ZN/NHmd+6MBiV7iBfeOc2+AGhCs6mzV+B22FuulWvdDRaA4vdcACgQWACn1lxYig 55uCjD3OqiV6GUeWGNRVBXIjD1ZIV48MzP+1VtZ4/HckNhWw1a13/BNLao0eV+qp Be7F69VWaCc8= 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=TbsiJdeJnXY7FDoj7nB0jihTiQSSZg1YeBS9Dcay/JA=; b=DHTLtQK4 CxEwVoNxB8AdJDVBY8mOiI4dcM3CNTm1tmejaet733/drhb4SkQwK2AJ51GpDW7l LSN+hcfiB87p0zRcPTMSaejoUSyAhbxrRiU4ioxQfCNnE68xnB7ASbBbTxKD08zf ZpQ3y9oowDNwAunWOzomJEZgU5H97iprJadCtQsigho+IphkIJFuFeBF2oFw1DlJ vWDq47bUQC4HlRVpcQaTQdAoG+JmdjM/g73Vshz6522pgbtTs7jPWlAKW1jGkf7B arXVH+mSr2KNJti8arWFeD+PObbYOZZFkimNbXY3NmRKfCcz7TJ8xc+2dv2ctp5S fxmUmsGggx6fpA== X-ME-Sender: X-Sasl-enc: SLu1uc06Bq5hph4/zybQNGQvNw2A9+WakMXhFnUcip0/ 1504217309 Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 434D924573; Thu, 31 Aug 2017 18:08:29 -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:08:28 +0200 Message-ID: <1822305.AnSnkpAPWV@xps> In-Reply-To: <2775824.X3T22OWxbu@xps> References: <20170825133319.15207-1-dharton@cisco.com> <20170825134717.18376-1-dharton@cisco.com> <2775824.X3T22OWxbu@xps> 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:08:30 -0000 One more comment below 01/09/2017 00:04, Thomas Monjalon: > 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); Which error codes can be returned by this op? It is adding new error codes to rte_eth_dev_set_vlan_offload(), and they must be documented in the doxygen. > > + 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? Thanks