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

["Zhang, Helin" <helin.zhang@intel.com>]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, March 7, 2016 5:29 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 0/3] i40e setting ether type of VLANs
> 
> 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.
Won't it be reviewed by Bruce, and applied on his maintainer branch first?

> 
> > 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.
Let me think it a little bit more. I don't think I have understood that
versioning quite well.

> 
> About the API change, you want to be able to insert a QinQ inner-vlan, right?
No, I just want to configure the ether type of vlan the hardware can recognize,
but not to insert a vlan.

> 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.
Yes, I agree with you the comments should be reworked.

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

> 
> 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?
No, they are different. I am trying to configure the hardware recognized
ether type of both inner and outer vlan, but not to configure the vlan itself.