From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from alln-iport-4.cisco.com (alln-iport-4.cisco.com [173.37.142.91]) by dpdk.org (Postfix) with ESMTP id 06F713B5 for ; Fri, 1 Sep 2017 02:41:15 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=4026; q=dns/txt; s=iport; t=1504226476; x=1505436076; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=4B3qu+QEiqOq0IFxiom4ivhGaulbb1qzdfUQYusD3+U=; b=EMM5I4/X+kvtDN8I8FL/AuuwU1+qUDYrxCWzqCXm136FEoTSRjJhme99 2S/T0u7DjZDwOMPl3V8gkMJikueQDtaG6jkV9lxpDDt+cq/mhNBEOHDjb rSySAs8YUlHVfMRot+GWC8VMUNPn2IZS3f/VdlZk5DCwVuKYvpx5DODsL 0=; X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0CfAABvrKhZ/4wNJK1dGQEBAQEBAQEBA?= =?us-ascii?q?QEBBwEBAQEBgy0tgXkHjhCQH4FxkUKEZQ6CBIVHAoQPPxgBAgEBAQEBAQFrKIU?= =?us-ascii?q?YAQEBAQIBJxM/BQcEAgEIEQQBAR8JBzIUCQgCBA4FCIohCLFAOotiAQEBAQEBA?= =?us-ascii?q?QEBAQEBAQEBAQEBAQEBHYMqggKBToULhF6GCwWYMYg+ApRGghyFZ4p0SJV8AR8?= =?us-ascii?q?4gQ13FR+FQhyBZ3YBiUqBDwEBAQ?= X-IronPort-AV: E=Sophos;i="5.41,456,1498521600"; d="scan'208";a="477691476" Received: from alln-core-7.cisco.com ([173.36.13.140]) by alln-iport-4.cisco.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 01 Sep 2017 00:40:57 +0000 Received: from XCH-RCD-010.cisco.com (xch-rcd-010.cisco.com [173.37.102.20]) by alln-core-7.cisco.com (8.14.5/8.14.5) with ESMTP id v810evST004433 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=FAIL); Fri, 1 Sep 2017 00:40:57 GMT Received: from xch-rcd-016.cisco.com (173.37.102.26) by XCH-RCD-010.cisco.com (173.37.102.20) with Microsoft SMTP Server (TLS) id 15.0.1263.5; Thu, 31 Aug 2017 19:40:57 -0500 Received: from xch-rcd-016.cisco.com ([173.37.102.26]) by XCH-RCD-016.cisco.com ([173.37.102.26]) with mapi id 15.00.1263.000; Thu, 31 Aug 2017 19:40:56 -0500 From: "David Harton (dharton)" To: Thomas Monjalon CC: "dev@dpdk.org" , "ferruh.yigit@intel.com" , "ajit.khaparde@broadcom.com" , "John Daley (johndale)" , "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" Thread-Topic: [dpdk-dev] [PATCH v3] ethdev: modifiy vlan_offload_set_t to return int Thread-Index: AQHTHairirj3sf6LE0SEXf1TU9hn16KfYp4A///UptA= Date: Fri, 1 Sep 2017 00:40:56 +0000 Message-ID: References: <20170825133319.15207-1-dharton@cisco.com> <20170825134717.18376-1-dharton@cisco.com> <2775824.X3T22OWxbu@xps> In-Reply-To: <2775824.X3T22OWxbu@xps> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.82.235.18] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Fri, 01 Sep 2017 00:41:16 -0000 Hi Thomas, > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Thursday, August 31, 2017 6:04 PM > To: David Harton (dharton) > Cc: dev@dpdk.org; ferruh.yigit@intel.com; ajit.khaparde@broadcom.com; Joh= n > Daley (johndale) ; 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 > Subject: Re: [dpdk-dev] [PATCH v3] ethdev: modifiy vlan_offload_set_t to > return int >=20 > Hi, >=20 > 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. >=20 > I agree with allowing to return an error. >=20 > Comments on details below. >=20 > The title could be changed to better reflect the purpose: > ethdev: allow returning error on VLAN configuration Sure. >=20 > > --- 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. > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > > - > > +* Changed return type of ``vlan_offload_set_t`` from ``void`` to ``int= ``. >=20 > It should be referenced as an API change (instead of ABI change). > (and line spacing must be kept) Sure I'll move it to API. Line spacing? You mean 80 char width? I followed an example from a previo= us release so I'm not sure what you mean. >=20 > We also need to bump the library version but it can be done with > bigger changes in ethdev. Ok. >=20 > > --- a/lib/librte_ether/rte_ethdev.c > > +++ b/lib/librte_ether/rte_ethdev.c > > @@ -2049,10 +2049,16 @@ struct rte_eth_dev * > > int ret =3D 0; > > int mask =3D 0; > > int cur, org =3D 0; > > + uint8_t org_strip, org_filter, org_extend; > > > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > > dev =3D &rte_eth_devices[port_id]; > > > > + /* save original values in case of failure */ > > + org_strip =3D dev->data->dev_conf.rxmode.hw_vlan_strip; > > + org_filter =3D dev->data->dev_conf.rxmode.hw_vlan_filter; > > + org_extend =3D dev->data->dev_conf.rxmode.hw_vlan_extend; >=20 > Please waste one more char to write "orig" instead of "org". Will do. >=20 > > - (*dev->dev_ops->vlan_offload_set)(dev, mask); > > + ret =3D (*dev->dev_ops->vlan_offload_set)(dev, mask); > > + if (ret) { > > + /* hit an error restore original values */ > > + dev->data->dev_conf.rxmode.hw_vlan_strip =3D org_strip; > > + dev->data->dev_conf.rxmode.hw_vlan_filter =3D org_filter; > > + dev->data->dev_conf.rxmode.hw_vlan_extend =3D org_extend; > > + } >=20 > 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? By setting the dev_conf.rxmode.hw_vlan_* fields this function is claiming=20 ownership of that data and therefore it should be the one to reset it. > Do we want to document this behaviour with the ops prototype? Why? Shouldn't any function that doesn't do what it was asked to do=20 leave the system in its original state and in fact if it doesn't that=20 is when it should be documented what the behavior is? Otherwise every=20 caller has to implement some cleanup code and would have to be given=20 information to know how to perform that cleanup as well which seems=20 more complicated. Thanks, Dave