From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 41B79C4DC for ; Thu, 16 Jun 2016 03:40:10 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP; 15 Jun 2016 18:40:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,478,1459839600"; d="scan'208";a="988417352" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga001.fm.intel.com with ESMTP; 15 Jun 2016 18:40:09 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 15 Jun 2016 18:40:09 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 15 Jun 2016 18:40:08 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.181]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.8]) with mapi id 14.03.0248.002; Thu, 16 Jun 2016 09:40:06 +0800 From: "Xing, Beilei" To: "Richardson, Bruce" CC: "Lu, Wenzhuo" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2] e1000: configure VLAN TPID Thread-Index: AQHRvUQBDuLPvqzvK02niknJmZmafZ/p6peAgAF338A= Date: Thu, 16 Jun 2016 01:40:06 +0000 Message-ID: <94479800C636CB44BD422CB454846E01397DB3@SHSMSX103.ccr.corp.intel.com> References: <1461228948-18820-1-git-send-email-beilei.xing@intel.com> <1464922755-2888-1-git-send-email-beilei.xing@intel.com> <20160615110359.GE10172@bricha3-MOBL3> In-Reply-To: <20160615110359.GE10172@bricha3-MOBL3> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2] e1000: configure VLAN TPID 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: Thu, 16 Jun 2016 01:40:10 -0000 Hi Bruce, > -----Original Message----- > From: Richardson, Bruce > Sent: Wednesday, June 15, 2016 7:04 PM > To: Xing, Beilei > Cc: Lu, Wenzhuo ; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2] e1000: configure VLAN TPID >=20 > On Fri, Jun 03, 2016 at 10:59:15AM +0800, Beilei Xing wrote: > > This patch enables configuring the ether types of both inner and outer > > VLANs. Note that outer TPID of single VLAN and inner TPID of double > > VLAN are read only. > > >=20 > Commit message does not actually match the code, and I there is something > very strange about the code, see my comments below. >=20 > > Signed-off-by: Beilei Xing > > --- > > v2 changes: > > Modify return value. Cause inner tpid is not supported by single > > vlan, return -ENOTSUP. > > Add return value. If want to set inner TPID of double vlan or set > > outer tpid of single vlan, return -ENOTSUP. > > > > drivers/net/e1000/igb_ethdev.c | 38 > > +++++++++++++++++++++++++++++++++++--- > > 1 file changed, 35 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/e1000/igb_ethdev.c > > b/drivers/net/e1000/igb_ethdev.c index f0921ee..5d37e2c 100644 > > --- a/drivers/net/e1000/igb_ethdev.c > > +++ b/drivers/net/e1000/igb_ethdev.c > > @@ -86,6 +86,14 @@ > > #define E1000_INCVALUE_82576 (16 << IGB_82576_TSYNC_SHIFT) > > #define E1000_TSAUXC_DISABLE_SYSTIME 0x80000000 > > > > +/* CTRL_EXT bit mask*/ > > +#define E1000_CTRL_EXT_EXT_VLAN (1 << 26) > > + > > +/* VLAN Ether Type bit mask */ > > +#define E1000_VET_VET_EXT 0xFFFF0000 > > + >=20 > No need for a blank line here. Update the comment above to refer to both > mask and shift values and put them on one line after the other. OK, I'll delete the blank line in next version. >=20 > > +#define E1000_VET_VET_EXT_SHIFT 16 > > + > > static int eth_igb_configure(struct rte_eth_dev *dev); static int > > eth_igb_start(struct rte_eth_dev *dev); static void > > eth_igb_stop(struct rte_eth_dev *dev); @@ -2237,13 +2245,37 @@ > > eth_igb_vlan_tpid_set(struct rte_eth_dev *dev, { > > struct e1000_hw *hw =3D > > E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > - uint32_t reg =3D ETHER_TYPE_VLAN; > > + uint32_t reg; > > + uint32_t qinq; > > int ret =3D 0; > > > > + qinq =3D E1000_READ_REG(hw, E1000_CTRL_EXT); > > + qinq &=3D E1000_CTRL_EXT_EXT_VLAN; > > + > > switch (vlan_type) { > > case ETH_VLAN_TYPE_INNER: > > - reg |=3D (tpid << 16); > > - E1000_WRITE_REG(hw, E1000_VET, reg); > > + if (qinq) { > > + ret =3D -ENOTSUP; > > + PMD_DRV_LOG(WARNING, > > + "Inner vlan ether type is read-only\n"); > > + } else { > > + ret =3D -ENOTSUP; > > + PMD_DRV_LOG(ERR, "Inner type is not supported" > > + " by single vlan\n"); > > + } > > + break; >=20 > Both branches of the if statment return -ENOTSUP and do not do anything t= o > set any VLAN parameters. This means that although we have the ability to > set the outer vlan, we can no longer set the inner type. This contradicts= what > the commit message says. Yes, actually inner VLAN couldn't be changed. >=20 > > + case ETH_VLAN_TYPE_OUTER: > > + if (qinq) { > > + reg =3D E1000_READ_REG(hw, E1000_VET); > > + reg =3D (reg & (~E1000_VET_VET_EXT)) | > > + ((uint32_t)tpid << > E1000_VET_VET_EXT_SHIFT); > > + E1000_WRITE_REG(hw, E1000_VET, reg); > > + } else { > > + ret =3D -ENOTSUP; > > + PMD_DRV_LOG(WARNING, > > + "Single vlan ether type is read-only\n"); > > + } > > + >=20 > So according to the code, the only time you can change a vlan ether type = is to > set the outer type for qinq? If this is the correct behaviour, please upd= ate > the commit title and message accordingly. Yes, you're right, thanks for all your comments. >=20 > > break; > > default: > > ret =3D -EINVAL; > > -- > > 2.5.0 > > >=20 > Regards, > /Bruce