* Re: [PATCH v2] i40e: modify the meaning of single VLAN type
2016-06-13 8:03 ` [PATCH v2] i40e: modify the meaning of single " Beilei Xing
@ 2016-06-14 3:33 ` Wu, Jingjing
2016-06-21 10:29 ` Bruce Richardson
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Wu, Jingjing @ 2016-06-14 3:33 UTC (permalink / raw)
To: Xing, Beilei; +Cc: dev
> -----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>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] i40e: modify the meaning of single VLAN type
2016-06-13 8:03 ` [PATCH v2] i40e: modify the meaning of single " Beilei Xing
2016-06-14 3:33 ` Wu, Jingjing
@ 2016-06-21 10:29 ` Bruce Richardson
2016-06-21 11:06 ` Panu Matilainen
2016-06-21 11:30 ` Bruce Richardson
2016-06-22 2:53 ` [PATCH v3] i40e: fix the type issue of a " Beilei Xing
3 siblings, 1 reply; 11+ messages in thread
From: Bruce Richardson @ 2016-06-21 10:29 UTC (permalink / raw)
To: Beilei Xing; +Cc: jingjing.wu, dev, thomas.monjalon, nhorman, pmatilai
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] i40e: modify the meaning of single VLAN type
2016-06-21 10:29 ` Bruce Richardson
@ 2016-06-21 11:06 ` Panu Matilainen
2016-06-21 11:28 ` Bruce Richardson
0 siblings, 1 reply; 11+ messages in thread
From: Panu Matilainen @ 2016-06-21 11:06 UTC (permalink / raw)
To: Bruce Richardson, Beilei Xing; +Cc: jingjing.wu, dev, thomas.monjalon, nhorman
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 -
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] i40e: modify the meaning of single VLAN type
2016-06-21 11:06 ` Panu Matilainen
@ 2016-06-21 11:28 ` Bruce Richardson
0 siblings, 0 replies; 11+ messages in thread
From: Bruce Richardson @ 2016-06-21 11:28 UTC (permalink / raw)
To: Panu Matilainen; +Cc: Beilei Xing, jingjing.wu, dev, thomas.monjalon, nhorman
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] i40e: modify the meaning of single VLAN type
2016-06-13 8:03 ` [PATCH v2] i40e: modify the meaning of single " Beilei Xing
2016-06-14 3:33 ` Wu, Jingjing
2016-06-21 10:29 ` Bruce Richardson
@ 2016-06-21 11:30 ` Bruce Richardson
2016-06-22 2:53 ` [PATCH v3] i40e: fix the type issue of a " Beilei Xing
3 siblings, 0 replies; 11+ messages in thread
From: Bruce Richardson @ 2016-06-21 11:30 UTC (permalink / raw)
To: Beilei Xing; +Cc: jingjing.wu, dev
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] i40e: fix the type issue of a single VLAN type
2016-06-13 8:03 ` [PATCH v2] i40e: modify the meaning of single " Beilei Xing
` (2 preceding siblings ...)
2016-06-21 11:30 ` Bruce Richardson
@ 2016-06-22 2:53 ` Beilei Xing
2016-06-24 12:09 ` Bruce Richardson
3 siblings, 1 reply; 11+ messages in thread
From: Beilei Xing @ 2016-06-22 2:53 UTC (permalink / raw)
To: jingjing.wu; +Cc: dev, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3] i40e: fix the type issue of a single VLAN type
2016-06-22 2:53 ` [PATCH v3] i40e: fix the type issue of a " Beilei Xing
@ 2016-06-24 12:09 ` Bruce Richardson
0 siblings, 0 replies; 11+ messages in thread
From: Bruce Richardson @ 2016-06-24 12:09 UTC (permalink / raw)
To: Beilei Xing; +Cc: jingjing.wu, dev
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
^ permalink raw reply [flat|nested] 11+ messages in thread