From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Daniels Subject: Re: [PATCH] net/ixgbe: fix vlan insert parameter type and its use Date: Wed, 19 Oct 2016 08:45:57 -0400 (EDT) Message-ID: References: <1476818007-13659-1-git-send-email-daniels@research.att.com> <6A0DE07E22DDAD4C9103DF62FEBC09093933CE36@shsmsx102.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Cc: "Zhang, Helin" , "Iremonger, Bernard" , "dev@dpdk.org" , "ZELEZNIAK, ALEX" To: "Lu, Wenzhuo" Return-path: Received: from mx0a-00191d01.pphosted.com (mx0b-00191d01.pphosted.com [67.231.157.136]) by dpdk.org (Postfix) with ESMTP id A8A5958DD for ; Wed, 19 Oct 2016 14:46:32 +0200 (CEST) In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC09093933CE36@shsmsx102.ccr.corp.intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" ------------------------------------------------------------------------ E. Scott Daniels PMTS - Cloud Software Research AT&T Labs - Research daniels@research.att.com 440.389.0011 On Wed, 19 Oct 2016, Lu, Wenzhuo wrote: > Hi Daniels, > > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of E. Scott Daniels >> Sent: Wednesday, October 19, 2016 3:13 AM >> To: Zhang, Helin; Iremonger, Bernard >> Cc: dev@dpdk.org; az5157@att.com; E. Scott Daniels >> 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 setting). >> >> 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 >= dev_info.max_vfs) >> return -EINVAL; >> >> - if (on > 1) >> + if (vlan_id > 4095) >> return -EINVAL; >> >> hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); >> ctrl = IXGBE_READ_REG(hw, IXGBE_VMVIR(vf)); >> - if (on) { >> - ctrl = on; >> + if (vlan_id) { >> + ctrl = 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. > I'm a bit confused. According to the data sheet (section 8.2.3.27.13) this register accepts the "port VLAN tag to insert if the VLANA field = 01b" in the right most 16 bits. This allows the given tag to be inserted in outgoing packets. The current implementation always sets this bit (via the IXGBE_VMVIR_VLANA_DEFAULT constant) which causes the tag in the right most bits to be inserted. The current code, which I belive is broken, only ever inserts a 1 or 0 into the right most bits as it takes the value passed in and ORs it with the constant, effectively only allowing the VLAN tag of 1 to be inserted. The value passed in is not being used to set/reset the always/never insert VLAN action bit. The only way to insert VLAN tags 2 through 4095 is to modify the code to accept the tag and insert it as described in the patch. Scott >> ctrl |= IXGBE_VMVIR_VLANA_DEFAULT; >> } else { >> ctrl = 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 > >