From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 9AE912C58 for ; Wed, 19 Oct 2016 14:57:44 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP; 19 Oct 2016 05:57:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,514,1473145200"; d="scan'208";a="1046931625" Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by orsmga001.jf.intel.com with ESMTP; 19 Oct 2016 05:57:43 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.164]) by IRSMSX152.ger.corp.intel.com ([169.254.6.13]) with mapi id 14.03.0248.002; Wed, 19 Oct 2016 13:57:41 +0100 From: "Iremonger, Bernard" To: Scott Daniels CC: "Lu, Wenzhuo" , "Zhang, Helin" , "dev@dpdk.org" , "ZELEZNIAK, ALEX" Thread-Topic: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type and its use Thread-Index: AQHSKceTp8nQzcnvQE+8RUqtfcuQjKCvmI3AgAAR0ACAABHYEA== Date: Wed, 19 Oct 2016 12:57:40 +0000 Message-ID: <8CEF83825BEC744B83065625E567D7C21A093DA3@IRSMSX108.ger.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: Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjNiZGNhZjgtMDJiNC00NGI5LWE0NTMtYzMxNzM0NWE5ZDExIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IktcL3ZmbkdUdTdXOHk2M2tpSjZ6TTBFN1wvUUN4NzVzQldPeGRjV09GWGxlaz0ifQ== x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.181] 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: Wed, 19 Oct 2016 12:57:45 -0000 Hi Scott, > >>> 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. > > > > Reading the 82599 data sheet (sect 8.2.3.27.13), the change from Scott = is > correct. > > The NACK is not correct. > > > > However changing the API means that where it is called needs to be > changed too. > > It is called at present from app/testpmd/cmdline.c line 4731. > > > > > Bernard, > Should I add this change to the patch? (Didn't occur to me to look for us= e). >=20 > Scott No, it would be better to make the testpmd change in a separate patch, and = send a v2 patchset with two patches. > >>> 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 Regards, Bernard.