From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id EEAB76CC4 for ; Thu, 20 Oct 2016 02:51:28 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP; 19 Oct 2016 17:51:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,516,1473145200"; d="scan'208";a="1056364543" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga001.fm.intel.com with ESMTP; 19 Oct 2016 17:51:28 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 19 Oct 2016 17:51:27 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 19 Oct 2016 17:51:26 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.206]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.96]) with mapi id 14.03.0248.002; Thu, 20 Oct 2016 08:51:23 +0800 From: "Lu, Wenzhuo" To: "Iremonger, Bernard" , "E. Scott Daniels" , "Zhang, Helin" CC: "dev@dpdk.org" , "az5157@att.com" Thread-Topic: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use Thread-Index: AQHSKXPXOvVcnmh5aUyiGr/RJAUmQqCvOwSA///asQCAAW6x4A== Date: Thu, 20 Oct 2016 00:51:22 +0000 Message-ID: <6A0DE07E22DDAD4C9103DF62FEBC09093933D3BE@shsmsx102.ccr.corp.intel.com> References: <1476818007-13659-1-git-send-email-daniels@research.att.com> <6A0DE07E22DDAD4C9103DF62FEBC09093933CE36@shsmsx102.ccr.corp.intel.com> <8CEF83825BEC744B83065625E567D7C21A093CC1@IRSMSX108.ger.corp.intel.com> In-Reply-To: <8CEF83825BEC744B83065625E567D7C21A093CC1@IRSMSX108.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Oct 2016 00:51:29 -0000 Hi Bernard, > -----Original Message----- > From: Iremonger, Bernard > Sent: Wednesday, October 19, 2016 6:56 PM > To: Lu, Wenzhuo; E. Scott Daniels; Zhang, Helin > Cc: dev@dpdk.org; az5157@att.com > Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type= and its > use >=20 > Hi Wenzhuo, Scott, >=20 > >=20 > > > Subject: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter > > > type and its use > > > > > > The final parameter to rte_pmd_ixgbe_set_vf_vlan_insert is uint8_t > > > and treated as a binary flag when it needs to be a uint16_t and > > > treated as a VLAN id. The data sheet (sect 8.2.3.27.13) describes > > > the right most > > > 16 bits as the VLAN id that is to be inserted; the > > > 16.11 code is accepting only a 1 or 0 thus effectively only > > > allowing the VLAN id 1 to be inserted (0 disables the insertion setti= ng). > > > > > > This patch changes the final parm name to represent the data that is > > > being accepted (vlan_id), changes the type to permit all valid VLAN > > > ids, and validates the parameter based on the range of 0 to 4095. > > > Corresponding changes to prototype and documentation in the .h file. > > > > > > Fixes: 49e248223e9f71 ("net/ixgbe: add API for VF management") > > > > > > Signed-off-by: E. Scott Daniels > > > --- > > > drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++++---- > > > drivers/net/ixgbe/rte_pmd_ixgbe.h | 9 +++++---- > > > 2 files changed, 9 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > > > b/drivers/net/ixgbe/ixgbe_ethdev.c > > > index 4ca5747..316af73 100644 > > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > > > @@ -4727,7 +4727,7 @@ rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t > > > port, uint16_t vf, uint8_t on) } > > > > > > int > > > -rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, uint8_t > > > on) > > > +rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, > > > +uint16_t > > > +vlan_id) > > > { > > > struct ixgbe_hw *hw; > > > uint32_t ctrl; > > > @@ -4742,13 +4742,13 @@ rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t > > port, > > > uint16_t vf, uint8_t on) > > > if (vf >=3D dev_info.max_vfs) > > > return -EINVAL; > > > > > > - if (on > 1) > > > + if (vlan_id > 4095) > > > return -EINVAL; > > > > > > hw =3D IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > > ctrl =3D IXGBE_READ_REG(hw, IXGBE_VMVIR(vf)); > > > - if (on) { > > > - ctrl =3D on; > > > + if (vlan_id) { > > > + ctrl =3D vlan_id; > > I believe this patch is a good idea of an enhancement. But you cannot > > leverage the existing code like this. > > The register IXGBE_VMVIR is only for enable/disable. We cannot write > > the VLAN ID into it. > > If you want to merge the 2 things, enabling/disabling and setting VLAN > > ID together. I think we need a totally new implementation. So NACK. >=20 > Reading the 82599 data sheet (sect 8.2.3.27.13), the change from Scott is > correct. > The NACK is not correct. You're right. I made a mistake about the words 'default vlan'. It's not fix= ed but the value of 'Port VLAN ID'. So the previous patch is not good, we need to fix it. Thanks for correcting= me. >=20 > However changing the API means that where it is called needs to be change= d > too. > It is called at present from app/testpmd/cmdline.c line 4731. >=20 >=20 >=20 > > > ctrl |=3D IXGBE_VMVIR_VLANA_DEFAULT; > > > } else { > > > ctrl =3D 0; > > > diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.h > > > b/drivers/net/ixgbe/rte_pmd_ixgbe.h > > > index 2fdf530..c2fb826 100644 > > > --- a/drivers/net/ixgbe/rte_pmd_ixgbe.h > > > +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.h > > > @@ -99,16 +99,17 @@ int rte_pmd_ixgbe_set_vf_mac_anti_spoof(uint8_t > > > port, uint16_t vf, uint8_t on); > > > * The port identifier of the Ethernet device. > > > * @param vf > > > * ID specifying VF. > > > - * @param on > > > - * 1 - Enable VF's vlan insert. > > > - * 0 - Disable VF's vlan insert > > > + * @param vlan_id > > > + * 0 - Disable VF's vlan insert. > > > + * n - Enable; n is inserted as the vlan id. > > > * > > > * @return > > > * - (0) if successful. > > > * - (-ENODEV) if *port* invalid. > > > * - (-EINVAL) if bad parameter. > > > */ > > > -int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, > > > uint8_t on); > > > +int rte_pmd_ixgbe_set_vf_vlan_insert(uint8_t port, uint16_t vf, > > > + uint16_t vlan_id); > > > > > > /** > > > * Enable/Disable tx loopback > > > -- > > > 1.9.1 >=20 > Regards, >=20 > Bernard.