From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <bernard.iremonger@intel.com>
Received: from mga04.intel.com (mga04.intel.com [192.55.52.120])
by dpdk.org (Postfix) with ESMTP id 27A566936
for <dev@dpdk.org>; Wed, 19 Oct 2016 12:56:59 +0200 (CEST)
Received: from fmsmga003.fm.intel.com ([10.253.24.29])
by fmsmga104.fm.intel.com with ESMTP; 19 Oct 2016 03:56:58 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.31,514,1473145200"; d="scan'208";a="774500238"
Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66])
by FMSMGA003.fm.intel.com with ESMTP; 19 Oct 2016 03:56:57 -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 11:55:54 +0100
From: "Iremonger, Bernard" <bernard.iremonger@intel.com>
To: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "E. Scott Daniels"
<daniels@research.att.com>, "Zhang, Helin" <helin.zhang@intel.com>
CC: "dev@dpdk.org" <dev@dpdk.org>, "az5157@att.com" <az5157@att.com>
Thread-Topic: [dpdk-dev] [PATCH] net/ixgbe: fix vlan insert parameter type
and its use
Thread-Index: AQHSKceTp8nQzcnvQE+8RUqtfcuQjKCvmI3A
Date: Wed, 19 Oct 2016 10:55:54 +0000
Message-ID: <8CEF83825BEC744B83065625E567D7C21A093CC1@IRSMSX108.ger.corp.intel.com>
References: <1476818007-13659-1-git-send-email-daniels@research.att.com>
<6A0DE07E22DDAD4C9103DF62FEBC09093933CE36@shsmsx102.ccr.corp.intel.com>
In-Reply-To: <6A0DE07E22DDAD4C9103DF62FEBC09093933CE36@shsmsx102.ccr.corp.intel.com>
Accept-Language: en-GB, en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNDlmNWQ1ZDItNjFiYi00NTA1LWFiZmQtMWNmOGVkNzk4ZjA0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlFybVpRd0U2WUh6amZHQkFPTEZqNFhvZFJrVGhaU2RnWGpXQ0gwcnlybnc9In0=
x-ctpclassification: CTP_IC
x-originating-ip: [163.33.239.182]
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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Wed, 19 Oct 2016 10:56:59 -0000
Hi Wenzhuo, Scott,
<snip>
> > 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 <daniels@research.att.com>
> > ---
> > 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 c=
orrect.
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.
=20
> > 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.