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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

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

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

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

> diff --git a/drivers/net/intel/iavf/iavf.h b/drivers/net/intel/iavf/iavf.h
> index f8008d0fda..35d82b7cc0 100644
> --- a/drivers/net/intel/iavf/iavf.h
> +++ b/drivers/net/intel/iavf/iavf.h
> @@ -292,6 +292,7 @@ struct iavf_info {
>  	bool in_reset_recovery;
>  
>  	uint32_t ptp_caps;
> +	uint16_t tpid;  /* VLAN tag identifier */

In the iavf_info structure, does this new tpid field need to be reset or
cleared during device reconfiguration or reset? The field is initialized
in iavf_dev_configure() but is there cleanup needed in stop/close paths?

[ ... ]

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

[ ... ]

> @@ -717,6 +720,7 @@ iavf_dev_configure(struct rte_eth_dev *dev)
>  	}
>  
>  	iavf_dev_init_vlan(dev);
> +	vf->tpid = RTE_ETHER_TYPE_VLAN; /* VLAN TPID set to 0x8100 by default */

In iavf_dev_configure(), does setting vf->tpid override any previously
configured value? If iavf_dev_configure() can be called multiple times
without a full device reset, could this reset the tpid to default even if
the user had configured it to 0x88a8 via iavf_vlan_tpid_set()?

> +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 iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
> +	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
> +	int qinq = dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;
> +
> +	if ((vlan_type != RTE_ETH_VLAN_TYPE_INNER &&
> +	    vlan_type != RTE_ETH_VLAN_TYPE_OUTER) ||
> +	    (!qinq && vlan_type == RTE_ETH_VLAN_TYPE_INNER)) {
> +		PMD_DRV_LOG(ERR, "Unsupported vlan type.");
> +		return -EINVAL;
> +	}
> +
> +	/* This API only fills internal iavf_info structure
> +	 * and does not send any signal to hardware
> +	 */
> +	if (qinq && vlan_type == RTE_ETH_VLAN_TYPE_OUTER)
> +		vf->tpid = rte_cpu_to_le_16(tpid); /* Outer VLAN can be 0x88a8 or 0x8100 */
> +	else
> +		vf->tpid = RTE_ETHER_TYPE_VLAN; /* Inner VLAN 0x8100 */
> +	return 0;
> +}

In iavf_vlan_tpid_set(), should there be validation that the tpid value
is one of the supported values (0x8100 or 0x88a8)? The function accepts
any uint16_t value without checking if it's valid.

Also in iavf_vlan_tpid_set(), does mixing rte_cpu_to_le_16(tpid) for the
outer VLAN case but direct assignment of RTE_ETHER_TYPE_VLAN for the
inner VLAN case create inconsistency? RTE_ETHER_TYPE_VLAN is defined as
0x8100 in host byte order, so should vf->tpid always store values in the
same byte order?

[ ... ]

> @@ -1453,6 +1483,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;
>  
>  	if (mask & RTE_ETH_VLAN_FILTER_MASK) {
> @@ -1472,6 +1504,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;
> +	}
> +

In iavf_dev_vlan_offload_set_v2(), does the code handle partial failures
correctly? If the RTE_ETH_VLAN_STRIP_MASK case succeeds but the
RTE_ETH_QINQ_STRIP_MASK case fails, has the device been left in an
inconsistent state?

> diff --git a/drivers/net/intel/iavf/iavf_vchnl.c b/drivers/net/intel/iavf/iavf_vchnl.c
> index 23d115298c..bae5d54553 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));
> +	stripping_caps = &vf->vlan_v2_caps.offloads.stripping_support;
> +	if ((stripping_caps->outer & VIRTCHNL_VLAN_ETHERTYPE_88A8) &&
> +	    (stripping_caps->outer & VIRTCHNL_VLAN_TOGGLE) &&
> +	    vf->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) &&
> +		   vf->tpid == RTE_ETHER_TYPE_VLAN) {
> +		ethertype = &vlan_strip.outer_ethertype_setting;
> +		*ethertype = VIRTCHNL_VLAN_ETHERTYPE_8100;
> +	} else {
> +		return -ENOTSUP;
> +	}

In iavf_config_outer_vlan_strip_v2(), the comparisons check vf->tpid
against RTE_ETHER_TYPE_QINQ and RTE_ETHER_TYPE_VLAN, but earlier in
iavf_vlan_tpid_set() the value is stored using rte_cpu_to_le_16(tpid).
Does this create a byte-order mismatch? RTE_ETHER_TYPE_QINQ is 0x88a8
and RTE_ETHER_TYPE_VLAN is 0x8100 in host byte order, but vf->tpid may
be in little-endian.

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

In iavf_add_del_vlan_v2(), does the same byte-order issue apply when
comparing vf->tpid to RTE_ETHER_TYPE_QINQ? Is there a possibility of
mismatched endianness causing incorrect behavior?