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 4EA346CC8 for ; Wed, 19 Oct 2016 14:52:21 +0200 (CEST) Received: from pps.filterd (m0083689.ppops.net [127.0.0.1]) by m0083689.ppops.net-00191d01. (8.16.0.17/8.16.0.17) with SMTP id u9JCj0d6040275; Wed, 19 Oct 2016 08:52:20 -0400 Received: from tlpd255.enaf.dadc.sbc.com (sbcsmtp3.sbc.com [144.160.112.28]) by m0083689.ppops.net-00191d01. with ESMTP id 2668vd0k2b-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 19 Oct 2016 08:52:20 -0400 Received: from enaf.dadc.sbc.com (localhost [127.0.0.1]) by tlpd255.enaf.dadc.sbc.com (8.14.5/8.14.5) with ESMTP id u9JCqGsT059780; Wed, 19 Oct 2016 07:52:19 -0500 Received: from dalint02.pst.cso.att.com (dalint02.pst.cso.att.com [135.31.133.160]) by tlpd255.enaf.dadc.sbc.com (8.14.5/8.14.5) with ESMTP id u9JCq6Wm059428 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 19 Oct 2016 07:52:08 -0500 Received: from tlpd252.dadc.sbc.com (tlpd252.dadc.sbc.com [135.31.184.157]) by dalint02.pst.cso.att.com (RSA Interceptor); Wed, 19 Oct 2016 12:51:49 GMT Received: from dadc.sbc.com (localhost [127.0.0.1]) by tlpd252.dadc.sbc.com (8.14.5/8.14.5) with ESMTP id u9JCpYve042828; Wed, 19 Oct 2016 07:51:34 -0500 Received: from mail-green.research.att.com (mail-green.research.att.com [135.207.255.15]) by tlpd252.dadc.sbc.com (8.14.5/8.14.5) with ESMTP id u9JCoN2m040389; Wed, 19 Oct 2016 07:50:33 -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 E0C87E074E; Wed, 19 Oct 2016 08:50:07 -0400 (EDT) Date: Wed, 19 Oct 2016 08:50:18 -0400 (EDT) From: Scott Daniels To: "Iremonger, Bernard" cc: "Lu, Wenzhuo" , "Zhang, Helin" , "dev@dpdk.org" , "ZELEZNIAK, ALEX" In-Reply-To: <8CEF83825BEC744B83065625E567D7C21A093CC1@IRSMSX108.ger.corp.intel.com> Message-ID: References: <1476818007-13659-1-git-send-email-daniels@research.att.com> <6A0DE07E22DDAD4C9103DF62FEBC09093933CE36@shsmsx102.ccr.corp.intel.com> <8CEF83825BEC744B83065625E567D7C21A093CC1@IRSMSX108.ger.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=1015 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:52:21 -0000 ------------------------------------------------------------------------ E. Scott Daniels PMTS - Cloud Software Research AT&T Labs - Research daniels@research.att.com 440.389.0011 On Wed, 19 Oct 2016, Iremonger, Bernard wrote: > Hi Wenzhuo, 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 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. > > 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 use). 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 > > Regards, > > Bernard. >