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 EF7D86CD7 for ; Wed, 19 Oct 2016 15:57:21 +0200 (CEST) Received: from pps.filterd (m0049463.ppops.net [127.0.0.1]) by m0049463.ppops.net-00191d01. (8.16.0.17/8.16.0.17) with SMTP id u9JDsp7h006441; Wed, 19 Oct 2016 09:57:21 -0400 Received: from alpi155.enaf.aldc.att.com (sbcsmtp7.sbc.com [144.160.229.24]) by m0049463.ppops.net-00191d01. with ESMTP id 2669qv10hh-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 19 Oct 2016 09:57:21 -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 u9JDvJII031722; Wed, 19 Oct 2016 09:57:20 -0400 Received: from mlpi407.sfdc.sbc.com (mlpi407.sfdc.sbc.com [130.9.128.239]) by alpi155.enaf.aldc.att.com (8.14.5/8.14.5) with ESMTP id u9JDv80j031520 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 19 Oct 2016 09:57:14 -0400 Received: from clpi183.sldc.sbc.com (clpi183.sldc.sbc.com [135.41.1.46]) by mlpi407.sfdc.sbc.com (RSA Interceptor); Wed, 19 Oct 2016 13:56:47 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 u9JDukeo014732; Wed, 19 Oct 2016 08:56:47 -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 u9JDuglX014514; Wed, 19 Oct 2016 08:56:42 -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 87B58E07BF; Wed, 19 Oct 2016 09:56:32 -0400 (EDT) Date: Wed, 19 Oct 2016 09:56:43 -0400 (EDT) From: Scott Daniels To: "Iremonger, Bernard" cc: "Lu, Wenzhuo" , "Zhang, Helin" , "dev@dpdk.org" , "ZELEZNIAK, ALEX" In-Reply-To: <8CEF83825BEC744B83065625E567D7C21A093DA3@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> <8CEF83825BEC744B83065625E567D7C21A093DA3@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-1610190250 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 13:57:22 -0000 On Wed, 19 Oct 2016, Iremonger, Bernard wrote: > 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 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 > > No, it would be better to make the testpmd change in a separate patch, and send a v2 patchset with two patches. > Will do, thanks. New to the patch world, so I appreciate your patience! 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. > >