Re: [dpdk-dev] [PATCH v2 0/3] i40e setting ether type of VLANs

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2016-03-07 16:12, Helin Zhang:
> The patch set was branched off rel_16_04 of repo dpdk-next-net,
> on below commit.
>  - commit 4ac366ba647909c3b71818f9be9db86ba5e871da
>      nfp: fix non-x86 build

Currently, changes on ethdev are directly applied on dpdk.git.

> v2:
>  - Used RTE_NEXT_ABI to avoid ABI change issue.

RTE_NEXT_ABI must be used only when it is really too difficult to keep
the compatibility with librte_compat.
Here you are just adding a parameter to some functions, so you should
try versionning the functions with the help of macros in librte_compat.

About the API change, you want to be able to insert a QinQ inner-vlan, right?
The current comment of rte_eth_dev_set_vlan_ether_type is:
 * Set the Outer VLAN Ether Type by an Ethernet device, it can be inserted to
 * the VLAN Header. This is a register setup available on some Intel NIC, not
 * but all, please check the data sheet for availability.

2 comments:
- you haven't changed "Outer VLAN" so the API description is wrong
- it is announced as something Intel-specific

About the new enum:
+ * VLAN types to indicate if it is for single VLAN, inner VLAN or outer VLAN.
+ * Note that most of time single VLAN is treated the same as inner VLAN.

You cannot say "most of time" in an API.

More generally, I am not convinced by the current VLAN API that you are extending.
Why this function is not merged with rte_eth_dev_set_vlan_pvid?