From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 41649C506 for ; Thu, 16 Jun 2016 11:43:03 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP; 16 Jun 2016 02:43:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,478,1459839600"; d="scan'208";a="998816568" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.220.182]) by orsmga002.jf.intel.com with SMTP; 16 Jun 2016 02:43:00 -0700 Received: by (sSMTP sendmail emulation); Thu, 16 Jun 2016 10:42:58 +0025 Date: Thu, 16 Jun 2016 10:42:58 +0100 From: Bruce Richardson To: Beilei Xing Cc: wenzhuo.lu@intel.com, dev@dpdk.org Message-ID: <20160616094258.GA11016@bricha3-MOBL3> References: <1464922755-2888-1-git-send-email-beilei.xing@intel.com> <1466046930-11021-1-git-send-email-beilei.xing@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1466046930-11021-1-git-send-email-beilei.xing@intel.com> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v3] 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 09:43:04 -0000 On Thu, Jun 16, 2016 at 11:15:30AM +0800, Beilei Xing wrote: > This patch enables configuring the outer TPID for double VLAN. > Note that outer TPID of single VLAN and inner TPID of double VLAN > are read only. While correct, I think it would be clearer to actually state that all other vlan TPID values are readonly. For single VLAN there is no outer and inner VLAN tags as such. > > Signed-off-by: Beilei Xing > --- > v3 changes: > Update commit log and comments. > > drivers/net/e1000/igb_ethdev.c | 37 ++++++++++++++++++++++++++++++++++--- > 1 file changed, 34 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c > index f0921ee..1a5f1f1 100644 > --- a/drivers/net/e1000/igb_ethdev.c > +++ b/drivers/net/e1000/igb_ethdev.c > @@ -86,6 +86,13 @@ > #define E1000_INCVALUE_82576 (16 << IGB_82576_TSYNC_SHIFT) > #define E1000_TSAUXC_DISABLE_SYSTIME 0x80000000 > > +/* External VLAN Enable bit mask */ > +#define E1000_CTRL_EXT_EXT_VLAN (1 << 26) > + > +/* External VLAN Ether Type bit mask and shift */ > +#define E1000_VET_VET_EXT 0xFFFF0000 > +#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 +2244,37 @@ eth_igb_vlan_tpid_set(struct rte_eth_dev *dev, > { > struct e1000_hw *hw = > E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private); > - uint32_t reg = ETHER_TYPE_VLAN; > + uint32_t reg; > + uint32_t qinq; > int ret = 0; > > + qinq = E1000_READ_REG(hw, E1000_CTRL_EXT); > + qinq &= E1000_CTRL_EXT_EXT_VLAN; > + > switch (vlan_type) { > case ETH_VLAN_TYPE_INNER: > - reg |= (tpid << 16); > - E1000_WRITE_REG(hw, E1000_VET, reg); > + if (qinq) { > + ret = -ENOTSUP; > + PMD_DRV_LOG(WARNING, > + "Inner vlan ether type is read-only\n"); > + } else { > + ret = -ENOTSUP; > + PMD_DRV_LOG(ERR, "Inner type is not supported" > + " by single vlan\n"); > + } > + break; This would be clearer if you reworked it. The "ret = -ENOTSUP" should come before the if statement (allowing you to remove the braces for the if). In fact, for single vlan case, you could do the check without even going into the switch statement, since you can never set a TPID if qinq is not enabled. That would also make the code more readable. > + case ETH_VLAN_TYPE_OUTER: > + if (qinq) { > + reg = E1000_READ_REG(hw, E1000_VET); > + reg = (reg & (~E1000_VET_VET_EXT)) | > + ((uint32_t)tpid << E1000_VET_VET_EXT_SHIFT); > + E1000_WRITE_REG(hw, E1000_VET, reg); > + } else { > + ret = -ENOTSUP; > + PMD_DRV_LOG(WARNING, > + "Single vlan ether type is read-only\n"); > + } > + > break; > default: > ret = -EINVAL; > -- Two other issues I spot: * Please also check the capitalization of VLAN - it should be all-caps since it's an acronym. * According to the macro for PMD_DRV_LOG in e1000_logs.h, a "\n" is automatically appended to the log messages, so omit the "\n" at the end of the log messages in this patch. Regards, /Bruce