[dpdk-dev,v2] i40e: modify the meaning of single VLAN type
Commit Message
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(-)
Comments
> -----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>
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
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 -
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
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
@@ -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
-----------
@@ -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);
}
@@ -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,
};