|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-09

I'll review this DPDK patch for the Intel IAVF driver that adds QinQ strip support.

---

Commit: [patch version v4]
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 both Outer VLAN TPIDs
0x88a8 (802.1ad) and 0x8100 (802.1Q). VLAN Extend should be enabled 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 */
>  };

Does this code need synchronization when accessing the tpid field? The
field is set in iavf_vlan_tpid_set() and read in
iavf_config_outer_vlan_strip_v2() and iavf_add_del_vlan_v2() without any
locking mechanism.

> 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, variables should be initialized
separately from their declaration. Should this be: "int qinq;" followed
by "qinq = dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;"?

Also, does this code use implicit comparison? The result of the bitwise
AND is used as a boolean without explicit comparison to zero.

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

Does this code use implicit comparison on the qinq variable?

> +		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 */

Does the second condition (vlan_type == RTE_ETH_VLAN_TYPE_OUTER) need to
be checked here? The function already returned earlier if vlan_type is not
RTE_ETH_VLAN_TYPE_OUTER.

> +
> +	return 0;
> +}
> +
>  static int
>  iavf_disable_vlan_strip_ex(struct rte_eth_dev *dev, int on)

[ ... ]

> @@ -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 initialization-in-declaration and implicit comparison issues as noted
earlier.

>  	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) {

Does this code use implicit comparison?

> +			PMD_DRV_LOG(ERR, "VLAN-extend disabled");
> +			return -ENOSYS;
> +		}
> +		enable = !!(rxmode->offloads & RTE_ETH_RX_OFFLOAD_QINQ_STRIP);

Does this code use the double negation operator (!!) pattern? The coding
style guide generally discourages this pattern in favor of explicit
comparisons.

> +		err = iavf_config_outer_vlan_strip_v2(adapter, enable);
> +		/* If not support, the stripping is already disabled by PF */
> +		if (err == -ENOTSUP && !enable)

Does this code use implicit comparison on the enable variable?

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

Does this code use implicit 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));

Does this code have redundant initialization? The vlan_strip structure is
already zeroed via memset(), yet msg_buf is initialized with {0} in the
declaration. Should both use the same pattern for consistency?

> +	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 code correctly handle the case when adapter->tpid has not been
explicitly set via iavf_vlan_tpid_set()? Is adapter->tpid guaranteed to be
initialized before this function is called?

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

Does this code use implicit 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;
> +}
> +
>  int
>  iavf_config_vlan_strip_v2(struct iavf_adapter *adapter, bool enable)
>  {
> @@ -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;

Same initialization-in-declaration and implicit comparison issues as noted
earlier.

>  	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) &&

Does this code use implicit comparison on the qinq variable?

>  	    (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) &&

Does this code use implicit comparison?

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

[ ... ]

> @@ -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)

Does this code use implicit comparison on the qinq variable?

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