From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from alln-iport-6.cisco.com (alln-iport-6.cisco.com [173.37.142.93]) by dpdk.org (Postfix) with ESMTP id 30B97199A9 for ; Thu, 7 Sep 2017 14:09:23 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=5839; q=dns/txt; s=iport; t=1504786163; x=1505995763; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=Do2fE/pM5yzt+eUnYStJeFWGTEu0URpIoq/iMbGNPqk=; b=T1HdD2qOYFC50t4zVEyFsbym0U01dKK82gD+NgtoIVhobTuhJ0fs4Bz0 NcQ5tyVM7u86KUiUaUfXuqBdBZjE0aROnqt9hmjiBpvOcfT0NRmK8+EAW 4VX0kw06/3bljf5M9+9M+N8ygcsibpNUMdPXIDzld9uYzhdmnktwpKtMh E=; X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0DzAABhNrFZ/4oNJK1cGQEBAQEBAQEBA?= =?us-ascii?q?QEBBwEBAQEBg1qBUicHjhGQIYFxliiCEgqFPgKEAj8YAQIBAQEBAQEBayiFGAE?= =?us-ascii?q?BAQECAScTPwwEAgEIFQMeCQcyFBECBAENBQgMihUIr1g6hBUBAYcxAQEBAQEBA?= =?us-ascii?q?QEBAQEBAQEBAQEBAQEBHYMqggKBToFjgyiKaQWJf4kQjWUClEaSepR+AhEZAYE?= =?us-ascii?q?4AR84gQ13FYVhHIEsATp2iXuBDwEBAQ?= X-IronPort-AV: E=Sophos;i="5.42,358,1500940800"; d="scan'208";a="482616471" Received: from alln-core-5.cisco.com ([173.36.13.138]) by alln-iport-6.cisco.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 07 Sep 2017 12:09:21 +0000 Received: from XCH-ALN-009.cisco.com (xch-aln-009.cisco.com [173.36.7.19]) by alln-core-5.cisco.com (8.14.5/8.14.5) with ESMTP id v87C9LEe011582 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=FAIL); Thu, 7 Sep 2017 12:09:21 GMT Received: from xch-rcd-016.cisco.com (173.37.102.26) by XCH-ALN-009.cisco.com (173.36.7.19) with Microsoft SMTP Server (TLS) id 15.0.1263.5; Thu, 7 Sep 2017 07:09:21 -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, 7 Sep 2017 07:09:20 -0500 From: "David Harton (dharton)" To: Hemant Agrawal , "thomas@monjalon.net" , "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" , "rasesh.mody@cavium.com" , "harish.patil@cavium.com" , "skhare@vmware.com" , "yliu@fridaylinux.org" , "maxime.coquelin@redhat.com" CC: "dev@dpdk.org" Thread-Topic: [PATCH v4] ethdev: allow returning error on VLAN offload configuration Thread-Index: AQHTIssf+K6PEnUXC0G/S0Koh/YxtaKf+aGAgAACh2CACYvYgP//z73g Date: Thu, 7 Sep 2017 12:09:20 +0000 Message-ID: <3862084d353f4101a0b94110d869e390@XCH-RCD-016.cisco.com> References: <20170825134717.18376-1-dharton@cisco.com> <20170901023628.21308-1-dharton@cisco.com> In-Reply-To: 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.226.96] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v4] ethdev: allow returning error on VLAN offload configuration 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, 07 Sep 2017 12:09:23 -0000 > -----Original Message----- > From: Hemant Agrawal [mailto:hemant.agrawal@nxp.com] >=20 > On 9/1/2017 6:24 PM, David Harton (dharton) wrote: > > > >> -----Original Message----- > >> From: Hemant Agrawal [mailto:hemant.agrawal@nxp.com] > >> > >> On 9/1/2017 8:06 AM, David Harton wrote: > >>> 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. > >>> > >>> rte_eth_dev_set_vlan_offload is updated to return the value provided > >>> by the vector when called along with restoring the original offload > >>> configs on failure. > >>> > >>> Existing vlan_offload_set_t vectors are modified to return an int. > >>> Majority of cases return 0 but a few that actually can fail now > >>> return their failure codes. > >>> > >>> Finally, a vlan_offload_set_t vector is added to virtio to > >>> facilitate dynamically turning VLAN strip on or off. > >>> > >>> Signed-off-by: David Harton > >>> --- >=20 > .... >=20 > >>> diff --git a/lib/librte_ether/rte_ethdev.c > >> b/lib/librte_ether/rte_ethdev.c > >>> index 0597641..05e52b8 100644 > >>> --- a/lib/librte_ether/rte_ethdev.c > >>> +++ b/lib/librte_ether/rte_ethdev.c > >>> @@ -2048,29 +2048,35 @@ struct rte_eth_dev * > >>> struct rte_eth_dev *dev; > >>> int ret =3D 0; > >>> int mask =3D 0; > >>> - int cur, org =3D 0; > >>> + int cur, orig =3D 0; > >>> + uint8_t orig_strip, orig_filter, orig_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 */ > >>> + orig_strip =3D dev->data->dev_conf.rxmode.hw_vlan_strip; > >>> + orig_filter =3D dev->data->dev_conf.rxmode.hw_vlan_filter; > >>> + orig_extend =3D dev->data->dev_conf.rxmode.hw_vlan_extend; > >>> + > >>> /*check which option changed by application*/ > >>> cur =3D !!(offload_mask & ETH_VLAN_STRIP_OFFLOAD); > >>> - org =3D !!(dev->data->dev_conf.rxmode.hw_vlan_strip); > >>> - if (cur !=3D org) { > >>> + orig =3D !!(dev->data->dev_conf.rxmode.hw_vlan_strip); > >>> + if (cur !=3D orig) { > >>> dev->data->dev_conf.rxmode.hw_vlan_strip =3D (uint8_t)cur; > >>> mask |=3D ETH_VLAN_STRIP_MASK; > >>> } > >>> > >>> cur =3D !!(offload_mask & ETH_VLAN_FILTER_OFFLOAD); > >>> - org =3D !!(dev->data->dev_conf.rxmode.hw_vlan_filter); > >>> - if (cur !=3D org) { > >>> + orig =3D !!(dev->data->dev_conf.rxmode.hw_vlan_filter); > >>> + if (cur !=3D orig) { > >>> dev->data->dev_conf.rxmode.hw_vlan_filter =3D (uint8_t)cur; > >>> mask |=3D ETH_VLAN_FILTER_MASK; > >>> } > >>> > >>> cur =3D !!(offload_mask & ETH_VLAN_EXTEND_OFFLOAD); > >>> - org =3D !!(dev->data->dev_conf.rxmode.hw_vlan_extend); > >>> - if (cur !=3D org) { > >>> + orig =3D !!(dev->data->dev_conf.rxmode.hw_vlan_extend); > >>> + if (cur !=3D orig) { > >>> dev->data->dev_conf.rxmode.hw_vlan_extend =3D (uint8_t)cur; > >>> mask |=3D ETH_VLAN_EXTEND_MASK; > >>> } > >>> @@ -2080,7 +2086,13 @@ struct rte_eth_dev * > >>> return ret; > >>> > >>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); > >>> - (*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 orig_strip; > >>> + dev->data->dev_conf.rxmode.hw_vlan_filter =3D orig_filter; > >>> + dev->data->dev_conf.rxmode.hw_vlan_extend =3D orig_extend; > >>> + } > >>> > >> Currently vlan offload is combining three functions: > >> strip, filter and extend. > >> > >> Not all PMDs in DPDK support all of three. > >> Should not the error we extended to reflect, which of the VLAN > >> offload is not supported? > > > > There are many examples of APIs that not all PMDs support. > > There are also other APIs that manipulate many attributes but do not > > communicate which attribute fails on set. > > Solving these issues I believe it outside the scope of this change and > > it requires a larger community discussion. > > > > This proposed change at least adheres to the existing paradigm. >=20 > I agree that solving this issue is outside the scope of this patch. >=20 > However this patch may leave the drivers in incorrect state. I agree. When failures happen in other APIs as well, whether reported=20 to the caller or not, how the failure is handled is inconsistent. Also, drivers could be in the incorrect state after this API call with the current implementation. >=20 > Earlier this API was not returning error or failing. With this change, th= e > driver may want to implement the error handling if 2nd or 3rd attribute > failed. Note that you are also assuming and reverting back to the origina= l > condition. The real thrust of this change is to report the failure so the caller can attempt to take some corrective action. Is there a policy one way or the other on whether DPDK restores original state on failures or just leaves the system in a failed state and reports the condition? I've always come from the ilk that if an API call fails then the state of=20 the system should, as much as possible, remain the same as prior to the=20 API call which is why I restored the flags on failure. With that, I can make ethdev call the device again with the "restored" values and still return the first failure. Or I could leave ethdev alone and leave the "bad state" as the previous=20 implementation did? =20 >=20 > The change is fine w.r.t dpaa2 driver. Thanks. And thanks for the good discussion.