From mboxrd@z Thu Jan 1 00:00:00 1970 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) Received: from pps.filterd (m0049459.ppops.net [127.0.0.1]) by m0049459.ppops.net-00191d01. (8.16.0.17/8.16.0.17) with SMTP id u9JCjEZj030007; Wed, 19 Oct 2016 08:46:32 -0400 Received: from alpi155.enaf.aldc.att.com (sbcsmtp7.sbc.com [144.160.229.24]) by m0049459.ppops.net-00191d01. with ESMTP id 266908r862-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 19 Oct 2016 08:46:31 -0400 Received: from enaf.aldc.att.com (localhost [127.0.0.1]) by alpi155.enaf.aldc.att.com (8.14.5/8.14.5) with ESMTP id u9JCkVpT014161; Wed, 19 Oct 2016 08:46:31 -0400 Received: from mlpi408.sfdc.sbc.com (mlpi408.sfdc.sbc.com [130.9.128.240]) by alpi155.enaf.aldc.att.com (8.14.5/8.14.5) with ESMTP id u9JCkNfY014066 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 19 Oct 2016 08:46:26 -0400 Received: from clpi183.sldc.sbc.com (clpi183.sldc.sbc.com [135.41.1.46]) by mlpi408.sfdc.sbc.com (RSA Interceptor); Wed, 19 Oct 2016 12:46:05 GMT Received: from sldc.sbc.com (localhost [127.0.0.1]) by clpi183.sldc.sbc.com (8.14.5/8.14.5) with ESMTP id u9JCk53q006777; Wed, 19 Oct 2016 07:46:05 -0500 Received: from mail-green.research.att.com (mail-green.research.att.com [135.207.255.15]) by clpi183.sldc.sbc.com (8.14.5/8.14.5) with ESMTP id u9JCjuI7006028; Wed, 19 Oct 2016 07:45:56 -0500 Received: from mt-230-10.research.att.com (mt-230-10.research.att.com [135.207.230.10]) by mail-green.research.att.com (Postfix) with ESMTP id 581E5E07C2; Wed, 19 Oct 2016 08:45:46 -0400 (EDT) Date: Wed, 19 Oct 2016 08:45:57 -0400 (EDT) From: Scott Daniels To: "Lu, Wenzhuo" cc: "Zhang, Helin" , "Iremonger, Bernard" , "dev@dpdk.org" , "ZELEZNIAK, ALEX" In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC09093933CE36@shsmsx102.ccr.corp.intel.com> Message-ID: References: <1476818007-13659-1-git-send-email-daniels@research.att.com> <6A0DE07E22DDAD4C9103DF62FEBC09093933CE36@shsmsx102.ccr.corp.intel.com> User-Agent: Alpine 2.20.12 (LSU 116 2015-12-14) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed X-RSA-Inspected: yes X-RSA-Classifications: public X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2016-10-19_07:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_policy_notspam policy=outbound_policy score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1610190230 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:46:33 -0000 ------------------------------------------------------------------------ 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 > >