|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 DPDK Patch: net/iavf: add support for QinQ strip

Commit: series.patch
Author: Anurag Mandal <anurag.mandal at intel.com>
Subject: [PATCH v4] net/iavf: add support for QinQ strip

This patch adds support for QinQ strip with both Outer VLAN TPIDs 0x88a8
(802.1ad) & 0x8100 (802.1Q). VLAN Extend should be enabled for the same
and Outer VLAN TPID should be set properly.

> 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 */
>  };

The comment formatting follows DPDK style for single-line comments. The
placement is appropriate as the last field.

> 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

[ ... ]

> +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;

Does this code have an implicit boolean comparison issue? The variable
qinq is declared as int but used as a boolean. According to DPDK coding
style guidelines, boolean comparisons should be explicit. Should this be:

    bool qinq = !!(dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_VLAN_EXTEND);

or:

    int qinq = dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;
    if (!qinq) { ... }

> +
> +	if (vlan_type != RTE_ETH_VLAN_TYPE_OUTER) {
> +		PMD_DRV_LOG(ERR, "Unsupported vlan type.");
> +		return -EINVAL;
> +	} else if (!qinq) {
> +		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)
> +		adapter->tpid = tpid; /* Outer VLAN can be 0x88a8 or 0x8100 */
> +
> +	return 0;
> +}

Does this function have an unnecessary conditional check? The code already
validates that vlan_type == RTE_ETH_VLAN_TYPE_OUTER at the top of the
function, returning -EINVAL if it is not. Is the redundant check
"vlan_type == RTE_ETH_VLAN_TYPE_OUTER" at the end unnecessary?

[ ... ]

> @@ -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;
>  	int err;

Does this code have the same implicit boolean comparison issue as the
earlier function? The variable qinq is declared as int but used in
boolean context. Should this use explicit boolean type or explicit
comparison?

[ ... ]

> +	if (mask & RTE_ETH_QINQ_STRIP_MASK) {
> +		if (!qinq) {
> +			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)
> +			err = 0;
> +		if (err)
> +			return -EIO;
> +	}

The enable assignment uses explicit boolean conversion !!(). However, the
qinq variable used in the check does not. Is this intentional
inconsistency?

> 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

[ ... ]

> +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));

Does this code have redundant initialization? The vlan_strip variable is
zeroed with memset() but is not marked with any initializer. Should the
declaration use = {0} or is memset() preferred in this driver?

> +	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 need validation of adapter->tpid? The function
iavf_vlan_tpid_set() validates the tpid value, but does this function
assume adapter->tpid has been set correctly? What happens if
iavf_config_outer_vlan_strip_v2() is called before iavf_vlan_tpid_set()?

[ ... ]

> @@ -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;
>  	int ret;

Does this code also have the implicit boolean comparison issue? The qinq
variable is declared as int but used in boolean contexts throughout this
function.

[ ... ]

> @@ -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)
> +		vlan_setting->tpid = RTE_ETHER_TYPE_QINQ;
> +	else
> +		vlan_setting->tpid = RTE_ETHER_TYPE_VLAN;
>  	vlan_setting->tci = vlanid;

Does this code need input validation? Similar to the earlier concern, is
adapter->tpid guaranteed to be valid at this point, or should there be
explicit validation?