|SUCCESS| [v4] net/iavf: add support for QinQ strip

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162962

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-21

## Review of patch: net/iavf: add support for QinQ strip

git sha: 20260409101951.114492-1-anurag.mandal at intel.com
Author: Anurag Mandal <anurag.mandal at intel.com>
Subject: [PATCH v4] net/iavf: add support for QinQ strip

This patch adds support for QinQ stripping with outer VLAN TPIDs 0x88a8
(802.1ad) and 0x8100 (802.1Q) to the iavf driver.

> diff --git a/doc/guides/nics/intel_vf.rst b/doc/guides/nics/intel_vf.rst
> index 5fa2ddc9ea..dfff69b982 100644
> --- a/doc/guides/nics/intel_vf.rst
> +++ b/doc/guides/nics/intel_vf.rst

[ ... ]

> diff --git a/drivers/net/intel/iavf/iavf.h b/drivers/net/intel/iavf/iavf.h
> index f8008d0fda..6a77dacf59 100644
> --- a/drivers/net/intel/iavf/iavf.h
> +++ b/drivers/net/intel/iavf/iavf.h
> @@ -387,6 +387,7 @@ struct iavf_adapter {
>  	uint16_t fdir_ref_cnt;
>  	struct iavf_devargs devargs;
>  	bool mac_primary_set;
> +	uint16_t tpid;  /* VLAN tag identifier */

Does this comment accurately describe the variable's purpose? The tpid
field stores the outer VLAN TPID for QinQ configuration, not just any
VLAN tag identifier. Should the comment be more specific about it being
the "outer VLAN TPID for QinQ configuration"?

[ ... ]

> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
> index 3126d9b644..5da2cd43c2 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c

[ ... ]

> @@ -1365,6 +1368,36 @@ iavf_dev_del_mac_addr(struct rte_eth_dev *dev, uint32_t index)
>  	vf->mac_num--;
>  }
>  
> +static int
> +iavf_vlan_tpid_set(struct rte_eth_dev *dev, enum rte_vlan_type vlan_type, uint16_t tpid)
> +{
> +	struct iavf_adapter *adapter =
> +		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
> +	int qinq = dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;

According to the coding style guide, does this follow the rule for
boolean comparisons? The result should be compared explicitly with 0
rather than using implicit boolean conversion. Should this be:
"int qinq = !!(dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_VLAN_EXTEND);"?

> +
> +	if (vlan_type != RTE_ETH_VLAN_TYPE_OUTER) {
> +		PMD_DRV_LOG(ERR, "Unsupported vlan type.");
> +		return -EINVAL;
> +	} else if (!qinq) {

Should this comparison be "qinq == 0" for explicit boolean comparison
per the coding style?

> +		PMD_DRV_LOG(ERR, "VLAN-extend disabled.");
> +		return -ENOSYS;
> +	} else if (tpid != RTE_ETHER_TYPE_VLAN &&
> +		   tpid != RTE_ETHER_TYPE_QINQ) {
> +		PMD_DRV_LOG(ERR, "tpid supported 0x8100/0x88A8");
> +		return -ENOTSUP;
> +	}
> +
> +	/* This API only fills internal iavf_adapter structure
> +	 * and does not send any signal to hardware.
> +	 * Inner VLAN always 0x8100, so not set explicitly.
> +	 */
> +	if (qinq && vlan_type == RTE_ETH_VLAN_TYPE_OUTER)

Should this comparison be "qinq != 0" per the coding style? Also, is the
second condition check redundant since we already verified vlan_type ==
RTE_ETH_VLAN_TYPE_OUTER earlier?

> +		adapter->tpid = tpid; /* Outer VLAN can be 0x88a8 or 0x8100 */
> +
> +	return 0;
> +}

[ ... ]

> @@ -1453,6 +1486,8 @@ iavf_dev_vlan_offload_set_v2(struct rte_eth_dev *dev, int mask)
>  	struct iavf_adapter *adapter =
>  		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
>  	bool enable;
> +	int qinq = dev->data->dev_conf.rxmode.offloads &
> +		RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;

Same question here: should this use explicit boolean comparison with
"!!" operator?

>  	int err;
>  
>  	if (mask & RTE_ETH_VLAN_FILTER_MASK) {
> @@ -1472,6 +1507,20 @@ iavf_dev_vlan_offload_set_v2(struct rte_eth_dev *dev, int mask)
>  			return -EIO;
>  	}
>  
> +	if (mask & RTE_ETH_QINQ_STRIP_MASK) {
> +		if (!qinq) {

Should this be "qinq == 0" for explicit comparison?

> +			PMD_DRV_LOG(ERR, "VLAN-extend disabled");
> +			return -ENOSYS;
> +		}
> +		enable = !!(rxmode->offloads & RTE_ETH_RX_OFFLOAD_QINQ_STRIP);
> +		err = iavf_config_outer_vlan_strip_v2(adapter, enable);
> +		/* If not support, the stripping is already disabled by PF */
> +		if (err == -ENOTSUP && !enable)

Should the "!enable" comparison be "enable == false" per the coding
style?

> +			err = 0;
> +		if (err)

Should this be "err != 0" for explicit comparison?

> +			return -EIO;
> +	}
> +
>  	return 0;
>  }

[ ... ]

> diff --git a/drivers/net/intel/iavf/iavf_vchnl.c b/drivers/net/intel/iavf/iavf_vchnl.c
> index 23d115298c..c2f340db81 100644
> --- a/drivers/net/intel/iavf/iavf_vchnl.c
> +++ b/drivers/net/intel/iavf/iavf_vchnl.c
> @@ -829,6 +829,50 @@ iavf_get_supported_rxdid(struct iavf_adapter *adapter)
>  	return 0;
>  }
>  
> +int
> +iavf_config_outer_vlan_strip_v2(struct iavf_adapter *adapter, bool enable)
> +{
> +	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
> +	struct virtchnl_vlan_supported_caps *stripping_caps;
> +	struct virtchnl_vlan_setting vlan_strip;
> +	uint8_t msg_buf[IAVF_AQ_BUF_SZ] = {0};
> +	struct iavf_cmd_info args;
> +	uint32_t *ethertype;
> +	int ret;
> +
> +	memset(&vlan_strip, 0, sizeof(vlan_strip));
> +	stripping_caps = &vf->vlan_v2_caps.offloads.stripping_support;
> +	if ((stripping_caps->outer & VIRTCHNL_VLAN_ETHERTYPE_88A8) &&
> +	    (stripping_caps->outer & VIRTCHNL_VLAN_TOGGLE) &&
> +	    adapter->tpid == RTE_ETHER_TYPE_QINQ) {
> +		ethertype = &vlan_strip.outer_ethertype_setting;
> +		*ethertype = VIRTCHNL_VLAN_ETHERTYPE_88A8;
> +	} else if ((stripping_caps->outer & VIRTCHNL_VLAN_ETHERTYPE_8100) &&
> +		   (stripping_caps->outer & VIRTCHNL_VLAN_TOGGLE) &&
> +		   adapter->tpid == RTE_ETHER_TYPE_VLAN) {
> +		ethertype = &vlan_strip.outer_ethertype_setting;
> +		*ethertype = VIRTCHNL_VLAN_ETHERTYPE_8100;
> +	} else {
> +		return -ENOTSUP;
> +	}

Does this function validate that adapter->tpid has been initialized
before use? In iavf_config_outer_vlan_strip_v2(), if the tpid field was
never explicitly set via iavf_vlan_tpid_set(), will adapter->tpid have a
valid value?

> +
> +	vlan_strip.vport_id = vf->vsi_res->vsi_id;
> +
> +	args.ops = enable ? VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2 :
> +			    VIRTCHNL_OP_DISABLE_VLAN_STRIPPING_V2;
> +	args.in_args = (uint8_t *)&vlan_strip;
> +	args.in_args_size = sizeof(vlan_strip);
> +	args.out_buffer = msg_buf;
> +	args.out_size = IAVF_AQ_BUF_SZ;
> +	ret = iavf_execute_vf_cmd_safe(adapter, &args);
> +	if (ret)

Should this be "ret != 0" for explicit comparison?

> +		PMD_DRV_LOG(ERR, "fail to execute command %s",
> +			    enable ? "VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2" :
> +				     "VIRTCHNL_OP_DISABLE_VLAN_STRIPPING_V2");
> +
> +	return ret;
> +}

[ ... ]

> @@ -838,14 +882,20 @@ iavf_config_vlan_strip_v2(struct iavf_adapter *adapter, bool enable)
>  	uint8_t msg_buf[IAVF_AQ_BUF_SZ] = {0};
>  	struct iavf_cmd_info args;
>  	uint32_t *ethertype;
> +	int qinq = adapter->dev_data->dev_conf.rxmode.offloads &
> +		   RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;

Should this also use "!!" for explicit boolean conversion?

>  	int ret;
>  
>  	stripping_caps = &vf->vlan_v2_caps.offloads.stripping_support;
>  
> -	if ((stripping_caps->outer & VIRTCHNL_VLAN_ETHERTYPE_8100) &&
> +	/* When VLAN extend is disabled, Single VLAN mode which is Outer VLAN
> +	 * When VLAN extend is enabled, QinQ mode, this API works only on
> +	 * Inner VLAN strip which is always 0x8100.
> +	 */
> +	if (!qinq && (stripping_caps->outer & VIRTCHNL_VLAN_ETHERTYPE_8100) &&

Should this be "qinq == 0" for explicit comparison?

>  	    (stripping_caps->outer & VIRTCHNL_VLAN_TOGGLE))
>  		ethertype = &vlan_strip.outer_ethertype_setting;
> -	else if ((stripping_caps->inner & VIRTCHNL_VLAN_ETHERTYPE_8100) &&
> +	else if (qinq && (stripping_caps->inner & VIRTCHNL_VLAN_ETHERTYPE_8100) &&

Should this be "qinq != 0" for explicit comparison?

>  		 (stripping_caps->inner & VIRTCHNL_VLAN_TOGGLE))
>  		ethertype = &vlan_strip.inner_ethertype_setting;
>  	else

[ ... ]

> @@ -921,6 +971,8 @@ iavf_add_del_vlan_v2(struct iavf_adapter *adapter, uint16_t vlanid, bool add)
>  	struct virtchnl_vlan *vlan_setting;
>  	struct iavf_cmd_info args;
>  	uint32_t filtering_caps;
> +	int qinq = adapter->dev_data->dev_conf.rxmode.offloads &
> +		   RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;

Should this use "!!" for explicit boolean conversion?

>  	int err;
>  
>  	supported_caps = &vf->vlan_v2_caps.filtering.filtering_support;
> @@ -938,7 +990,10 @@ iavf_add_del_vlan_v2(struct iavf_adapter *adapter, uint16_t vlanid, bool add)
>  	memset(&vlan_filter, 0, sizeof(vlan_filter));
>  	vlan_filter.vport_id = vf->vsi_res->vsi_id;
>  	vlan_filter.num_elements = 1;
> -	vlan_setting->tpid = RTE_ETHER_TYPE_VLAN;
> +	if (qinq && adapter->tpid == RTE_ETHER_TYPE_QINQ)

Should this use "qinq != 0" for explicit comparison?

> +		vlan_setting->tpid = RTE_ETHER_TYPE_QINQ;
> +	else
> +		vlan_setting->tpid = RTE_ETHER_TYPE_VLAN;
>  	vlan_setting->tci = vlanid;

Does this code in iavf_add_del_vlan_v2() rely on adapter->tpid being
properly initialized? If iavf_vlan_tpid_set() was never called, will
adapter->tpid still have the default RTE_ETHER_TYPE_VLAN value set in
iavf_dev_init()?