Hi Bruce, > -----Original Message----- > From: Richardson, Bruce > Sent: Wednesday, June 15, 2016 7:04 PM > To: Xing, Beilei <beilei.xing at intel.com> > Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2] e1000: configure VLAN TPID > > 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. > > > > Commit message does not actually match the code, and I there is something > very strange about the code, see my comments below. > > > Signed-off-by: Beilei Xing <beilei.xing at intel.com> > > --- > > 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 > > + > > 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. > > > +#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 = > > 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; > > Both branches of the if statment return -ENOTSUP and do not do anything to > 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. > > > + 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"); > > + } > > + > > 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 update > the commit title and message accordingly. Yes, you're right, thanks for all your comments. > > > break; > > default: > > ret = -EINVAL; > > -- > > 2.5.0 > > > > Regards, > /Bruce