From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v3] ethdev: modifiy vlan_offload_set_t to return int Date: Fri, 01 Sep 2017 00:04:04 +0200 Message-ID: <2775824.X3T22OWxbu@xps> References: <20170825133319.15207-1-dharton@cisco.com> <20170825134717.18376-1-dharton@cisco.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit 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 To: David Harton 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) In-Reply-To: <20170825134717.18376-1-dharton@cisco.com> 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, 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?