[PATCH] ethdev: change comments of VLAN type

[PATCH] ethdev: change comments of VLAN type

[Beilei Xing]

If the packet carries a single VLAN header, it is treated as the
outer header.
So change the comments of inner VLAN and outer VLAN.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 doc/guides/rel_notes/release_16_07.rst | 3 +++
 lib/librte_ether/rte_ethdev.h          | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/doc/guides/rel_notes/release_16_07.rst b/doc/guides/rel_notes/release_16_07.rst
index 30e78d4..29db86c 100644
--- a/doc/guides/rel_notes/release_16_07.rst
+++ b/doc/guides/rel_notes/release_16_07.rst
@@ -116,6 +116,9 @@ API Changes
   ibadcrc, ibadlen, imcasts, fdirmatch, fdirmiss,
   tx_pause_xon, rx_pause_xon, tx_pause_xoff, rx_pause_xoff.
 
+* The comments of ``ETH_VLAN_TYPE_INNER`` and ``ETH_VLAN_TYPE_OUTER`` in
+  ``rte_vlan_type`` are changed.
+
 
 ABI Changes
 -----------
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 2757510..c5c29fb 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -363,8 +363,8 @@ struct rte_eth_rxmode {
  */
 enum rte_vlan_type {
 	ETH_VLAN_TYPE_UNKNOWN = 0,
-	ETH_VLAN_TYPE_INNER, /**< Single VLAN, or inner VLAN. */
-	ETH_VLAN_TYPE_OUTER, /**< Outer VLAN. */
+	ETH_VLAN_TYPE_INNER, /**< Inner VLAN. */
+	ETH_VLAN_TYPE_OUTER, /**< Single VLAN, or outer VLAN. */
 	ETH_VLAN_TYPE_MAX,
 };
 
-- 
2.5.0

Re: [PATCH] ethdev: change comments of VLAN type

[Lu, Wenzhuo]

Hi Beilei,


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Beilei Xing
> Sent: Thursday, May 26, 2016 3:28 PM
> To: Wu, Jingjing
> Cc: dev@dpdk.org; Xing, Beilei
> Subject: [dpdk-dev] [PATCH] ethdev: change comments of VLAN type
> 
> If the packet carries a single VLAN header, it is treated as the outer header.
> So change the comments of inner VLAN and outer VLAN.
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
>  doc/guides/rel_notes/release_16_07.rst | 3 +++
>  lib/librte_ether/rte_ethdev.h          | 4 ++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_16_07.rst
> b/doc/guides/rel_notes/release_16_07.rst
> index 30e78d4..29db86c 100644
> --- a/doc/guides/rel_notes/release_16_07.rst
> +++ b/doc/guides/rel_notes/release_16_07.rst
> @@ -116,6 +116,9 @@ API Changes
>    ibadcrc, ibadlen, imcasts, fdirmatch, fdirmiss,
>    tx_pause_xon, rx_pause_xon, tx_pause_xoff, rx_pause_xoff.
> 
> +* The comments of ``ETH_VLAN_TYPE_INNER`` and
> ``ETH_VLAN_TYPE_OUTER``
> +in
> +  ``rte_vlan_type`` are changed.
I think we need more explanation here. At least the info in the commit log.

Re: [PATCH] ethdev: change comments of VLAN type

[Xing, Beilei]

> -----Original Message-----
> From: Lu, Wenzhuo
> Sent: Friday, June 3, 2016 10:53 AM
> To: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] ethdev: change comments of VLAN type
> 
> Hi Beilei,
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Beilei Xing
> > Sent: Thursday, May 26, 2016 3:28 PM
> > To: Wu, Jingjing
> > Cc: dev@dpdk.org; Xing, Beilei
> > Subject: [dpdk-dev] [PATCH] ethdev: change comments of VLAN type
> >
> > If the packet carries a single VLAN header, it is treated as the outer header.
> > So change the comments of inner VLAN and outer VLAN.
> >
> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > ---
> >  doc/guides/rel_notes/release_16_07.rst | 3 +++
> >  lib/librte_ether/rte_ethdev.h          | 4 ++--
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/release_16_07.rst
> > b/doc/guides/rel_notes/release_16_07.rst
> > index 30e78d4..29db86c 100644
> > --- a/doc/guides/rel_notes/release_16_07.rst
> > +++ b/doc/guides/rel_notes/release_16_07.rst
> > @@ -116,6 +116,9 @@ API Changes
> >    ibadcrc, ibadlen, imcasts, fdirmatch, fdirmiss,
> >    tx_pause_xon, rx_pause_xon, tx_pause_xoff, rx_pause_xoff.
> >
> > +* The comments of ``ETH_VLAN_TYPE_INNER`` and
> > ``ETH_VLAN_TYPE_OUTER``
> > +in
> > +  ``rte_vlan_type`` are changed.
> I think we need more explanation here. At least the info in the commit log.

Hi Wenzhuo,
Thanks for your comments, and plan to combine this patch with i40 driver modification, it should be better.
Beilei

[PATCH v2] i40e: modify the meaning of single VLAN type

[Beilei Xing]

In current i40e codebase, if single VLAN header is added in a packet,
it's treated as inner VLAN. Generally, a single VLAN header is
treated as the outer VLAN header. So change corresponding register
for single VLAN.
At the meanwhile, change the meanings of inner VLAN and outer VLAN.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
v2 changes:
 Combine corresponding i40e driver changes into this patch.

 doc/guides/rel_notes/release_16_07.rst |  3 +++
 drivers/net/i40e/i40e_ethdev.c         | 29 ++++++++++++++++++++---------
 lib/librte_ether/rte_ethdev.h          |  4 ++--
 3 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/doc/guides/rel_notes/release_16_07.rst b/doc/guides/rel_notes/release_16_07.rst
index c0f6b02..ae02824 100644
--- a/doc/guides/rel_notes/release_16_07.rst
+++ b/doc/guides/rel_notes/release_16_07.rst
@@ -135,6 +135,9 @@ API Changes
   ibadcrc, ibadlen, imcasts, fdirmatch, fdirmiss,
   tx_pause_xon, rx_pause_xon, tx_pause_xoff, rx_pause_xoff.
 
+* The meanings of ``ETH_VLAN_TYPE_INNER`` and ``ETH_VLAN_TYPE_OUTER`` in
+  ``rte_vlan_type`` are changed.
+
 
 ABI Changes
 -----------
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 24777d5..3672c52 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -920,12 +920,6 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 			     "VLAN ether type");
 		goto err_setup_pf_switch;
 	}
-	ret = i40e_vlan_tpid_set(dev, ETH_VLAN_TYPE_INNER, ETHER_TYPE_VLAN);
-	if (ret != I40E_SUCCESS) {
-		PMD_INIT_LOG(ERR, "Failed to set the default outer "
-			     "VLAN ether type");
-		goto err_setup_pf_switch;
-	}
 
 	/* PF setup, which includes VSI setup */
 	ret = i40e_pf_setup(pf);
@@ -2392,13 +2386,24 @@ i40e_vlan_tpid_set(struct rte_eth_dev *dev,
 	uint64_t reg_r = 0, reg_w = 0;
 	uint16_t reg_id = 0;
 	int ret = 0;
+	int qinq = dev->data->dev_conf.rxmode.hw_vlan_extend;
 
 	switch (vlan_type) {
 	case ETH_VLAN_TYPE_OUTER:
-		reg_id = 2;
+		if (qinq)
+			reg_id = 2;
+		else
+			reg_id = 3;
 		break;
 	case ETH_VLAN_TYPE_INNER:
-		reg_id = 3;
+		if (qinq)
+			reg_id = 3;
+		else {
+			ret = -EINVAL;
+			PMD_DRV_LOG(ERR, "Unsupported vlan type"
+				    "in single vlan.\n");
+			return ret;
+		}
 		break;
 	default:
 		ret = -EINVAL;
@@ -2460,8 +2465,14 @@ i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 	}
 
 	if (mask & ETH_VLAN_EXTEND_MASK) {
-		if (dev->data->dev_conf.rxmode.hw_vlan_extend)
+		if (dev->data->dev_conf.rxmode.hw_vlan_extend) {
 			i40e_vsi_config_double_vlan(vsi, TRUE);
+			/* Set global registers with default ether type value */
+			i40e_vlan_tpid_set(dev, ETH_VLAN_TYPE_OUTER,
+					   ETHER_TYPE_VLAN);
+			i40e_vlan_tpid_set(dev, ETH_VLAN_TYPE_INNER,
+					   ETHER_TYPE_VLAN);
+		}
 		else
 			i40e_vsi_config_double_vlan(vsi, FALSE);
 	}
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 2757510..c5c29fb 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -363,8 +363,8 @@ struct rte_eth_rxmode {
  */
 enum rte_vlan_type {
 	ETH_VLAN_TYPE_UNKNOWN = 0,
-	ETH_VLAN_TYPE_INNER, /**< Single VLAN, or inner VLAN. */
-	ETH_VLAN_TYPE_OUTER, /**< Outer VLAN. */
+	ETH_VLAN_TYPE_INNER, /**< Inner VLAN. */
+	ETH_VLAN_TYPE_OUTER, /**< Single VLAN, or outer VLAN. */
 	ETH_VLAN_TYPE_MAX,
 };
 
-- 
2.5.0

Re: [PATCH v2] i40e: modify the meaning of single VLAN type

[Wu, Jingjing]

> -----Original Message-----
> From: Xing, Beilei
> Sent: Monday, June 13, 2016 4:04 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>
> Subject: [PATCH v2] i40e: modify the meaning of single VLAN type
> 
> In current i40e codebase, if single VLAN header is added in a packet,
> it's treated as inner VLAN. Generally, a single VLAN header is
> treated as the outer VLAN header. So change corresponding register
> for single VLAN.
> At the meanwhile, change the meanings of inner VLAN and outer VLAN.
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
Acked-by: Jingjing Wu <jingjing.wu@intel.com>

Re: [PATCH v2] i40e: modify the meaning of single VLAN type

[Bruce Richardson]

On Mon, Jun 13, 2016 at 04:03:32PM +0800, Beilei Xing wrote:
> In current i40e codebase, if single VLAN header is added in a packet,
> it's treated as inner VLAN. Generally, a single VLAN header is
> treated as the outer VLAN header. So change corresponding register
> for single VLAN.
> At the meanwhile, change the meanings of inner VLAN and outer VLAN.
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>

This patch changes the ABI, since an app written to the original API as specified
e.g. to set a single vlan header, would no longer work with this change.
Therefore, even though the original behaviour was inconsistent with other drivers
it may still need to be preserved.

I'm thinking that we may need to provide appropriately versioned copies of the
vlan_offload_set and vlan_tpid_set functions for backward compatibility with
the old ABI.

Any other comments or thoughts on this? 
Neil, Thomas, Panu - is this fix something that we need to provide backward
version-compatibility for, or given that the functions are being called through
a generic ethdev API mean that this can just go in as a straight bug-fix?

/Bruce

Re: [PATCH v2] i40e: modify the meaning of single VLAN type

[Panu Matilainen]

On 06/21/2016 01:29 PM, Bruce Richardson wrote:
> On Mon, Jun 13, 2016 at 04:03:32PM +0800, Beilei Xing wrote:
>> In current i40e codebase, if single VLAN header is added in a packet,
>> it's treated as inner VLAN. Generally, a single VLAN header is
>> treated as the outer VLAN header. So change corresponding register
>> for single VLAN.
>> At the meanwhile, change the meanings of inner VLAN and outer VLAN.
>>
>> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
>
> This patch changes the ABI, since an app written to the original API as specified
> e.g. to set a single vlan header, would no longer work with this change.
> Therefore, even though the original behaviour was inconsistent with other drivers
> it may still need to be preserved.
>
> I'm thinking that we may need to provide appropriately versioned copies of the
> vlan_offload_set and vlan_tpid_set functions for backward compatibility with
> the old ABI.
>
> Any other comments or thoughts on this?
> Neil, Thomas, Panu - is this fix something that we need to provide backward
> version-compatibility for, or given that the functions are being called through
> a generic ethdev API mean that this can just go in as a straight bug-fix?

Since it's currently inconsistent with everything else, I'd just call it 
a bug-fix and leave it at that.

Besides, I dont think you could version it via the ordinary means even 
if you wanted to, due to the way its called through eth_dev_ops etc.

	- Panu -

Re: [PATCH v2] i40e: modify the meaning of single VLAN type

[Bruce Richardson]

On Tue, Jun 21, 2016 at 02:06:38PM +0300, Panu Matilainen wrote:
> On 06/21/2016 01:29 PM, Bruce Richardson wrote:
> >On Mon, Jun 13, 2016 at 04:03:32PM +0800, Beilei Xing wrote:
> >>In current i40e codebase, if single VLAN header is added in a packet,
> >>it's treated as inner VLAN. Generally, a single VLAN header is
> >>treated as the outer VLAN header. So change corresponding register
> >>for single VLAN.
> >>At the meanwhile, change the meanings of inner VLAN and outer VLAN.
> >>
> >>Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> >
> >This patch changes the ABI, since an app written to the original API as specified
> >e.g. to set a single vlan header, would no longer work with this change.
> >Therefore, even though the original behaviour was inconsistent with other drivers
> >it may still need to be preserved.
> >
> >I'm thinking that we may need to provide appropriately versioned copies of the
> >vlan_offload_set and vlan_tpid_set functions for backward compatibility with
> >the old ABI.
> >
> >Any other comments or thoughts on this?
> >Neil, Thomas, Panu - is this fix something that we need to provide backward
> >version-compatibility for, or given that the functions are being called through
> >a generic ethdev API mean that this can just go in as a straight bug-fix?
> 
> Since it's currently inconsistent with everything else, I'd just call it a
> bug-fix and leave it at that.
> 

Yep, makes sense.

> Besides, I dont think you could version it via the ordinary means even if
> you wanted to, due to the way its called through eth_dev_ops etc.
> 

Good point, never thought of that! :-(

> 	- Panu -

Thanks for the guidance.

/Bruce

Re: [PATCH v2] i40e: modify the meaning of single VLAN type

[Bruce Richardson]

On Mon, Jun 13, 2016 at 04:03:32PM +0800, Beilei Xing wrote:
> In current i40e codebase, if single VLAN header is added in a packet,
> it's treated as inner VLAN. Generally, a single VLAN header is
> treated as the outer VLAN header. So change corresponding register
> for single VLAN.
> At the meanwhile, change the meanings of inner VLAN and outer VLAN.
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
> v2 changes:
>  Combine corresponding i40e driver changes into this patch.
> 
>  doc/guides/rel_notes/release_16_07.rst |  3 +++
>  drivers/net/i40e/i40e_ethdev.c         | 29 ++++++++++++++++++++---------
>  lib/librte_ether/rte_ethdev.h          |  4 ++--
>  3 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_16_07.rst b/doc/guides/rel_notes/release_16_07.rst
> index c0f6b02..ae02824 100644
> --- a/doc/guides/rel_notes/release_16_07.rst
> +++ b/doc/guides/rel_notes/release_16_07.rst
> @@ -135,6 +135,9 @@ API Changes
>    ibadcrc, ibadlen, imcasts, fdirmatch, fdirmiss,
>    tx_pause_xon, rx_pause_xon, tx_pause_xoff, rx_pause_xoff.
>  
> +* The meanings of ``ETH_VLAN_TYPE_INNER`` and ``ETH_VLAN_TYPE_OUTER`` in
> +  ``rte_vlan_type`` are changed.
> +

This change of meaning is not a general change across the whole API but is
just for the i40e driver. Rather than noting it as an API change, I think it's
better documenting it as a "fixed issue" in the i40e driver.

Also, I think it is better explained as a change in the type of a single vlan
tag, rather than referring to it as a switch in meaning of inner and outer
tags generally. Can you reword the message, please.

/Bruce

[PATCH v3] i40e: fix the type issue of a single VLAN type

[Beilei Xing]

In current i40e codebase, if single VLAN header is added in a packet,
it's treated as inner VLAN. Generally, a single VLAN header is
treated as the outer VLAN header. So change corresponding register
for single VLAN.

Fixes: 19b16e2f6442 ("ethdev: add vlan type when setting ether type")

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
v3 changes:
 Note it as a "fixed issue" in the i40e driver.
 Reword the title.
v2 changes:
 Combine corresponding i40e driver changes into this patch.
 
 doc/guides/rel_notes/release_16_07.rst |  7 +++++++
 drivers/net/i40e/i40e_ethdev.c         | 29 ++++++++++++++++++++---------
 lib/librte_ether/rte_ethdev.h          |  4 ++--
 3 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/doc/guides/rel_notes/release_16_07.rst b/doc/guides/rel_notes/release_16_07.rst
index 7aeacb2..f6912a7 100644
--- a/doc/guides/rel_notes/release_16_07.rst
+++ b/doc/guides/rel_notes/release_16_07.rst
@@ -113,6 +113,13 @@ Drivers
   info to descriptor.
   Now this issue is fixed by disabling vlan stripping from inner header.
 
+* **i40e: Fixed the type issue of a single VLAN type.**
+
+  Currently, if a single VLAN header is added in a packet, it's treated
+  as inner VLAN. But generally, a single VLAN header is treated as the
+  outer VLAN header.
+  This issue is fixed by changing corresponding register for single VLAN.
+
 
 Libraries
 ~~~~~~~~~
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index f94ad87..838889e 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -924,12 +924,6 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 			     "VLAN ether type");
 		goto err_setup_pf_switch;
 	}
-	ret = i40e_vlan_tpid_set(dev, ETH_VLAN_TYPE_INNER, ETHER_TYPE_VLAN);
-	if (ret != I40E_SUCCESS) {
-		PMD_INIT_LOG(ERR, "Failed to set the default outer "
-			     "VLAN ether type");
-		goto err_setup_pf_switch;
-	}
 
 	/* PF setup, which includes VSI setup */
 	ret = i40e_pf_setup(pf);
@@ -2442,13 +2436,24 @@ i40e_vlan_tpid_set(struct rte_eth_dev *dev,
 	uint64_t reg_r = 0, reg_w = 0;
 	uint16_t reg_id = 0;
 	int ret = 0;
+	int qinq = dev->data->dev_conf.rxmode.hw_vlan_extend;
 
 	switch (vlan_type) {
 	case ETH_VLAN_TYPE_OUTER:
-		reg_id = 2;
+		if (qinq)
+			reg_id = 2;
+		else
+			reg_id = 3;
 		break;
 	case ETH_VLAN_TYPE_INNER:
-		reg_id = 3;
+		if (qinq)
+			reg_id = 3;
+		else {
+			ret = -EINVAL;
+			PMD_DRV_LOG(ERR, "Unsupported vlan type"
+				    "in single vlan.\n");
+			return ret;
+		}
 		break;
 	default:
 		ret = -EINVAL;
@@ -2510,8 +2515,14 @@ i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 	}
 
 	if (mask & ETH_VLAN_EXTEND_MASK) {
-		if (dev->data->dev_conf.rxmode.hw_vlan_extend)
+		if (dev->data->dev_conf.rxmode.hw_vlan_extend) {
 			i40e_vsi_config_double_vlan(vsi, TRUE);
+			/* Set global registers with default ether type value */
+			i40e_vlan_tpid_set(dev, ETH_VLAN_TYPE_OUTER,
+					   ETHER_TYPE_VLAN);
+			i40e_vlan_tpid_set(dev, ETH_VLAN_TYPE_INNER,
+					   ETHER_TYPE_VLAN);
+		}
 		else
 			i40e_vsi_config_double_vlan(vsi, FALSE);
 	}
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index bd93bf6..6804a86 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -363,8 +363,8 @@ struct rte_eth_rxmode {
  */
 enum rte_vlan_type {
 	ETH_VLAN_TYPE_UNKNOWN = 0,
-	ETH_VLAN_TYPE_INNER, /**< Single VLAN, or inner VLAN. */
-	ETH_VLAN_TYPE_OUTER, /**< Outer VLAN. */
+	ETH_VLAN_TYPE_INNER, /**< Inner VLAN. */
+	ETH_VLAN_TYPE_OUTER, /**< Single VLAN, or outer VLAN. */
 	ETH_VLAN_TYPE_MAX,
 };
 
-- 
2.5.0

Re: [PATCH v3] i40e: fix the type issue of a single VLAN type

[Bruce Richardson]

On Wed, Jun 22, 2016 at 10:53:51AM +0800, Beilei Xing wrote:
> In current i40e codebase, if single VLAN header is added in a packet,
> it's treated as inner VLAN. Generally, a single VLAN header is
> treated as the outer VLAN header. So change corresponding register
> for single VLAN.
> 
> Fixes: 19b16e2f6442 ("ethdev: add vlan type when setting ether type")
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>

Applied to dpdk-next-net/rel_16_07

/Bruce