|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 correctness bugs, coding style compliance, and guideline violations.

Commit: 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 VLAN stripping with outer VLAN TPIDs
0x88a8 (802.1ad) and 0x8100 (802.1Q) in the iavf driver. It introduces
a new callback for VLAN TPID configuration and extends VLAN offload
handling to support QinQ stripping.

> 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 need to be more specific? The field stores the outer
VLAN TPID for QinQ configuration, not just any VLAN tag identifier.
Consider: "Outer VLAN TPID for QinQ (0x8100 or 0x88a8)" for clarity.

>  };
>  
>  /* IAVF_DEV_PRIVATE_TO */
> @@ -456,6 +457,7 @@ int iavf_configure_rss_key(struct iavf_adapter *adapter);
>  int iavf_configure_queues(struct iavf_adapter *adapter, uint16_t num_queue_pairs);
>  int iavf_get_supported_rxdid(struct iavf_adapter *adapter);
>  int iavf_config_vlan_strip_v2(struct iavf_adapter *adapter, bool enable);
> +int iavf_config_outer_vlan_strip_v2(struct iavf_adapter *adapter, bool enable);
>  int iavf_config_vlan_insert_v2(struct iavf_adapter *adapter, bool enable);
>  int iavf_add_del_vlan_v2(struct iavf_adapter *adapter, uint16_t vlanid,
>  			 bool add);
> 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
> @@ -127,6 +127,8 @@ static int iavf_dev_add_mac_addr(struct rte_eth_dev *dev,
>  static void iavf_dev_del_mac_addr(struct rte_eth_dev *dev, uint32_t index);
>  static int iavf_dev_vlan_filter_set(struct rte_eth_dev *dev,
>  				   uint16_t vlan_id, int on);
> +static int iavf_vlan_tpid_set(struct rte_eth_dev *dev,
> +			     enum rte_vlan_type vlan_type, uint16_t tpid);

[ ... ]

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

Line exceeds 79 characters. Should be wrapped.

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

Violates coding style: implicit comparison against zero. Should be:

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

or better as a boolean:

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

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

Does this code have a redundant check? The function already returns if
vlan_type is not RTE_ETH_VLAN_TYPE_OUTER above. Can this condition be
simplified to just "if (qinq)"?

> +
> +	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 implicit comparison issue as above. Should be:

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

>  	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) {
> +			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;
> +	}
> +
>  	return 0;
>  }

[ ... ]

> @@ -2812,6 +2861,7 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
>  	adapter->dev_data = eth_dev->data;
>  	adapter->stopped = 1;
>  	adapter->mac_primary_set = false;
> +	adapter->tpid = RTE_ETHER_TYPE_VLAN; /* VLAN TPID set to 0x8100 by default */

Line exceeds 79 characters. Should be wrapped.

>  
>  	if (iavf_dev_event_handler_init())
>  		goto init_vf_err;
> 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 variable is
declared on the stack and then immediately cleared with memset. Can it be
initialized to zero at declaration like msg_buf?

> +	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 properly validate adapter->tpid before use? The tpid field
is set in iavf_vlan_tpid_set() which validates the value, but what happens
if this function is called without tpid being explicitly set via
iavf_vlan_tpid_set()? Is the default value RTE_ETHER_TYPE_VLAN always
appropriate, or should there be a check here?

> +
> +	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)
> +		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 implicit comparison issue. Should be an explicit comparison or use
the !! operator.

>  	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