From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v2 0/3] i40e setting ether type of VLANs Date: Mon, 07 Mar 2016 10:28:47 +0100 Message-ID: <1571443.EPqn3bY1d6@xps13> References: <1453426623-8696-1-git-send-email-helin.zhang@intel.com> <1457338370-32744-1-git-send-email-helin.zhang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org To: Helin Zhang Return-path: Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com [74.125.82.42]) by dpdk.org (Postfix) with ESMTP id 566692C65 for ; Mon, 7 Mar 2016 10:30:25 +0100 (CET) Received: by mail-wm0-f42.google.com with SMTP id l68so62701469wml.1 for ; Mon, 07 Mar 2016 01:30:25 -0800 (PST) In-Reply-To: <1457338370-32744-1-git-send-email-helin.zhang@intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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?