DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] e1000: configure VLAN TPID
@ 2016-04-21  8:55 Beilei Xing
  2016-06-02  6:43 ` Wang, Xiao W
  2016-06-03  2:59 ` [dpdk-dev] [PATCH v2] " Beilei Xing
  0 siblings, 2 replies; 13+ messages in thread
From: Beilei Xing @ 2016-04-21  8:55 UTC (permalink / raw)
  To: wenzhuo.lu; +Cc: dev, Beilei Xing

This patch enables configuring the ether types of both inner and
outer VLANs. Note that TPID of single or inner VLAN is read only.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/e1000/igb_ethdev.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index e0053fe..c957fbe 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
+
+#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);
@@ -2242,13 +2250,33 @@ 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)
+			PMD_DRV_LOG(WARNING,
+				    "inner vlan ether type is read-only\n");
+		else {
+			PMD_DRV_LOG(ERR, "not set QinQ on yet\n");
+			ret = -EIO;
+		}
+		break;
+	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
+			PMD_DRV_LOG(WARNING,
+				    "single vlan ether type is read-only\n");
+
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.5.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] e1000: configure VLAN TPID
  2016-04-21  8:55 [dpdk-dev] [PATCH] e1000: configure VLAN TPID Beilei Xing
@ 2016-06-02  6:43 ` Wang, Xiao W
  2016-06-03  3:10   ` Xing, Beilei
  2016-06-03  2:59 ` [dpdk-dev] [PATCH v2] " Beilei Xing
  1 sibling, 1 reply; 13+ messages in thread
From: Wang, Xiao W @ 2016-06-02  6:43 UTC (permalink / raw)
  To: Xing, Beilei, Lu, Wenzhuo; +Cc: dev

Hi Beilei,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Beilei Xing
> Sent: Thursday, April 21, 2016 4:56 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>
> Subject: [dpdk-dev] [PATCH] e1000: configure VLAN TPID
> 
> This patch enables configuring the ether types of both inner and outer VLANs.
> Note that TPID of single or inner VLAN is read only.
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
>  drivers/net/e1000/igb_ethdev.c | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
> index e0053fe..c957fbe 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
> +
> +#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); @@ -2242,13 +2250,33 @@ 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)
> +			PMD_DRV_LOG(WARNING,
> +				    "inner vlan ether type is read-only\n");

Add: ret = -ENOTSUP or something else, so that the programmer can handle it.
The same for below.

> +		else {
> +			PMD_DRV_LOG(ERR, "not set QinQ on yet\n");
> +			ret = -EIO;
> +		}
> +		break;
> +	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
> +			PMD_DRV_LOG(WARNING,
> +				    "single vlan ether type is read-only\n");
> +
>  		break;
>  	default:
>  		ret = -EINVAL;
> --
> 2.5.0

Since both inner and outer tpid are considered in this patch, the comment in
rte_ethdev.h "vlan_tpid_set;      /**< Outer VLAN TPID Setup. */" should be changed
accordingly.

Best Regards,
Xiao

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [dpdk-dev] [PATCH v2] e1000: configure VLAN TPID
  2016-04-21  8:55 [dpdk-dev] [PATCH] e1000: configure VLAN TPID Beilei Xing
  2016-06-02  6:43 ` Wang, Xiao W
@ 2016-06-03  2:59 ` Beilei Xing
  2016-06-03  3:59   ` Wang, Xiao W
                     ` (2 more replies)
  1 sibling, 3 replies; 13+ messages in thread
From: Beilei Xing @ 2016-06-03  2:59 UTC (permalink / raw)
  To: wenzhuo.lu; +Cc: dev, Beilei Xing

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.

Signed-off-by: Beilei Xing <beilei.xing@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
+
+#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;
+	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;
-- 
2.5.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] e1000: configure VLAN TPID
  2016-06-02  6:43 ` Wang, Xiao W
@ 2016-06-03  3:10   ` Xing, Beilei
  0 siblings, 0 replies; 13+ messages in thread
From: Xing, Beilei @ 2016-06-03  3:10 UTC (permalink / raw)
  To: Wang, Xiao W, Lu, Wenzhuo; +Cc: dev


> -----Original Message-----
> From: Wang, Xiao W
> Sent: Thursday, June 2, 2016 2:44 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] e1000: configure VLAN TPID
> 
> Hi Beilei,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Beilei Xing
> > Sent: Thursday, April 21, 2016 4:56 PM
> > To: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>
> > Subject: [dpdk-dev] [PATCH] e1000: configure VLAN TPID
> >
> > This patch enables configuring the ether types of both inner and outer
> VLANs.
> > Note that TPID of single or inner VLAN is read only.
> >
> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > ---
> >  drivers/net/e1000/igb_ethdev.c | 34
> > +++++++++++++++++++++++++++++++---
> >  1 file changed, 31 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/e1000/igb_ethdev.c
> > b/drivers/net/e1000/igb_ethdev.c index e0053fe..c957fbe 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
> > +
> > +#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); @@ -2242,13 +2250,33 @@
> > 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)
> > +			PMD_DRV_LOG(WARNING,
> > +				    "inner vlan ether type is read-only\n");
> 
> Add: ret = -ENOTSUP or something else, so that the programmer can handle
> it.
> The same for below.

Yes, I think -ENOTSUP should be more appropriate here.

> 
> > +		else {
> > +			PMD_DRV_LOG(ERR, "not set QinQ on yet\n");
> > +			ret = -EIO;
> > +		}
> > +		break;
> > +	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
> > +			PMD_DRV_LOG(WARNING,
> > +				    "single vlan ether type is read-only\n");
> > +
> >  		break;
> >  	default:
> >  		ret = -EINVAL;
> > --
> > 2.5.0
> 
> Since both inner and outer tpid are considered in this patch, the comment in
> rte_ethdev.h "vlan_tpid_set;      /**< Outer VLAN TPID Setup. */" should be
> changed
> accordingly.

Hi Xiao,
Thanks for your review, I've sent a new patch.

> 
> Best Regards,
> Xiao

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v2] e1000: configure VLAN TPID
  2016-06-03  2:59 ` [dpdk-dev] [PATCH v2] " Beilei Xing
@ 2016-06-03  3:59   ` Wang, Xiao W
  2016-06-15 11:03   ` Bruce Richardson
  2016-06-16  3:15   ` [dpdk-dev] [PATCH v3] " Beilei Xing
  2 siblings, 0 replies; 13+ messages in thread
From: Wang, Xiao W @ 2016-06-03  3:59 UTC (permalink / raw)
  To: Xing, Beilei, Lu, Wenzhuo; +Cc: dev, Xing, Beilei

Hi,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Beilei Xing
> Sent: Friday, June 3, 2016 10:59 AM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>
> Subject: [dpdk-dev] [PATCH v2] e1000: configure VLAN TPID
> 
> 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.
> 
> Signed-off-by: Beilei Xing <beilei.xing@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.
> 
Acked-by: Xiao Wang <xiao.w.wang@intel.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v2] e1000: configure VLAN TPID
  2016-06-03  2:59 ` [dpdk-dev] [PATCH v2] " Beilei Xing
  2016-06-03  3:59   ` Wang, Xiao W
@ 2016-06-15 11:03   ` Bruce Richardson
  2016-06-16  1:40     ` Xing, Beilei
  2016-06-16  3:15   ` [dpdk-dev] [PATCH v3] " Beilei Xing
  2 siblings, 1 reply; 13+ messages in thread
From: Bruce Richardson @ 2016-06-15 11:03 UTC (permalink / raw)
  To: Beilei Xing; +Cc: wenzhuo.lu, dev

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@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.

> +#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.

> +	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.

>  		break;
>  	default:
>  		ret = -EINVAL;
> -- 
> 2.5.0
> 

Regards,
/Bruce

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v2] e1000: configure VLAN TPID
  2016-06-15 11:03   ` Bruce Richardson
@ 2016-06-16  1:40     ` Xing, Beilei
  0 siblings, 0 replies; 13+ messages in thread
From: Xing, Beilei @ 2016-06-16  1:40 UTC (permalink / raw)
  To: Richardson, Bruce; +Cc: Lu, Wenzhuo, dev

Hi Bruce,

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Wednesday, June 15, 2016 7:04 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@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@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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [dpdk-dev] [PATCH v3] e1000: configure VLAN TPID
  2016-06-03  2:59 ` [dpdk-dev] [PATCH v2] " Beilei Xing
  2016-06-03  3:59   ` Wang, Xiao W
  2016-06-15 11:03   ` Bruce Richardson
@ 2016-06-16  3:15   ` Beilei Xing
  2016-06-16  6:57     ` Lu, Wenzhuo
                       ` (2 more replies)
  2 siblings, 3 replies; 13+ messages in thread
From: Beilei Xing @ 2016-06-16  3:15 UTC (permalink / raw)
  To: wenzhuo.lu; +Cc: dev, Beilei Xing

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.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
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;
+	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;
-- 
2.5.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v3] e1000: configure VLAN TPID
  2016-06-16  3:15   ` [dpdk-dev] [PATCH v3] " Beilei Xing
@ 2016-06-16  6:57     ` Lu, Wenzhuo
  2016-06-16  9:42     ` Bruce Richardson
  2016-06-16 13:36     ` [dpdk-dev] [PATCH v4] " Beilei Xing
  2 siblings, 0 replies; 13+ messages in thread
From: Lu, Wenzhuo @ 2016-06-16  6:57 UTC (permalink / raw)
  To: Xing, Beilei; +Cc: dev

Hi,

> -----Original Message-----
> From: Xing, Beilei
> Sent: Thursday, June 16, 2016 11:16 AM
> To: Lu, Wenzhuo
> Cc: dev@dpdk.org; Xing, Beilei
> Subject: [PATCH v3] e1000: configure VLAN TPID
> 
> 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.
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v3] e1000: configure VLAN TPID
  2016-06-16  3:15   ` [dpdk-dev] [PATCH v3] " Beilei Xing
  2016-06-16  6:57     ` Lu, Wenzhuo
@ 2016-06-16  9:42     ` Bruce Richardson
  2016-06-16 13:36     ` [dpdk-dev] [PATCH v4] " Beilei Xing
  2 siblings, 0 replies; 13+ messages in thread
From: Bruce Richardson @ 2016-06-16  9:42 UTC (permalink / raw)
  To: Beilei Xing; +Cc: wenzhuo.lu, dev

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 <beilei.xing@intel.com>
> ---
> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [dpdk-dev] [PATCH v4] e1000: configure VLAN TPID
  2016-06-16  3:15   ` [dpdk-dev] [PATCH v3] " Beilei Xing
  2016-06-16  6:57     ` Lu, Wenzhuo
  2016-06-16  9:42     ` Bruce Richardson
@ 2016-06-16 13:36     ` Beilei Xing
  2016-06-16 13:59       ` Zhang, Helin
  2 siblings, 1 reply; 13+ messages in thread
From: Beilei Xing @ 2016-06-16 13:36 UTC (permalink / raw)
  To: helin.zhang; +Cc: dev, Beilei Xing

This patch enables configuring the outer TPID for double VLAN.
Note that all other TPID values are read only.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
v4 changes:
 Optimize the code to be more readable.
v3 changes:
 Update commit log and comments.
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 | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index f0921ee..0ed95c8 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,21 +2244,25 @@ 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;
-	int ret = 0;
+	uint32_t reg, qinq;
+
+	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);
+	/* only outer TPID of double VLAN can be configured*/
+	if (qinq && vlan_type == ETH_VLAN_TYPE_OUTER) {
+		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);
-		break;
-	default:
-		ret = -EINVAL;
-		PMD_DRV_LOG(ERR, "Unsupported vlan type %d\n", vlan_type);
-		break;
+
+		return 0;
 	}
 
-	return ret;
+	/* all other TPID values are read-only*/
+	PMD_DRV_LOG(ERR, "Not supported");
+
+	return -ENOTSUP;
 }
 
 static void
-- 
2.5.0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v4] e1000: configure VLAN TPID
  2016-06-16 13:36     ` [dpdk-dev] [PATCH v4] " Beilei Xing
@ 2016-06-16 13:59       ` Zhang, Helin
  2016-06-24  9:59         ` Bruce Richardson
  0 siblings, 1 reply; 13+ messages in thread
From: Zhang, Helin @ 2016-06-16 13:59 UTC (permalink / raw)
  To: Xing, Beilei; +Cc: dev



> -----Original Message-----
> From: Xing, Beilei
> Sent: Thursday, June 16, 2016 9:36 PM
> To: Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>
> Subject: [PATCH v4] e1000: configure VLAN TPID
> 
> This patch enables configuring the outer TPID for double VLAN.
> Note that all other TPID values are read only.
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH v4] e1000: configure VLAN TPID
  2016-06-16 13:59       ` Zhang, Helin
@ 2016-06-24  9:59         ` Bruce Richardson
  0 siblings, 0 replies; 13+ messages in thread
From: Bruce Richardson @ 2016-06-24  9:59 UTC (permalink / raw)
  To: Zhang, Helin; +Cc: Xing, Beilei, dev

On Thu, Jun 16, 2016 at 01:59:46PM +0000, Zhang, Helin wrote:
> 
> 
> > -----Original Message-----
> > From: Xing, Beilei
> > Sent: Thursday, June 16, 2016 9:36 PM
> > To: Zhang, Helin <helin.zhang@intel.com>
> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>
> > Subject: [PATCH v4] e1000: configure VLAN TPID
> > 
> > This patch enables configuring the outer TPID for double VLAN.
> > Note that all other TPID values are read only.
> > 
> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> Acked-by: Helin Zhang <helin.zhang@intel.com>
> 
Applied to dpdk-next-net/rel_16_07

/Bruce

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-06-24  9:59 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-21  8:55 [dpdk-dev] [PATCH] e1000: configure VLAN TPID Beilei Xing
2016-06-02  6:43 ` Wang, Xiao W
2016-06-03  3:10   ` Xing, Beilei
2016-06-03  2:59 ` [dpdk-dev] [PATCH v2] " Beilei Xing
2016-06-03  3:59   ` Wang, Xiao W
2016-06-15 11:03   ` Bruce Richardson
2016-06-16  1:40     ` Xing, Beilei
2016-06-16  3:15   ` [dpdk-dev] [PATCH v3] " Beilei Xing
2016-06-16  6:57     ` Lu, Wenzhuo
2016-06-16  9:42     ` Bruce Richardson
2016-06-16 13:36     ` [dpdk-dev] [PATCH v4] " Beilei Xing
2016-06-16 13:59       ` Zhang, Helin
2016-06-24  9:59         ` Bruce Richardson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).