[PATCH] net/ice: fix VLAN tag reporting on Rx

[PATCH] net/ice: fix VLAN tag reporting on Rx

[Bruce Richardson]

The ice driver expects the first, or outer, VLAN tag in a packet to be
written to the L2TAG1 field of the descriptor, as configured by the
l2tsel field when configuring the queue context initially for the
device. However, when configuring the actual VLAN or QinQ strip
behaviour, that l2tsel field was changed, sending the single/outer vlan
tag to the L2TAG2 field in the descriptor. This meant that it was not
getting picked up correctly by the Rx paths.

This issue has been around for a long time, but was previously
partially hidden by the issue fixed in [1], since due to that bug,
the l2tsel field was not getting overridden in the single-queue case
(since the single queue was the final queue).

Fix the issue by just removing the code updating the l2tsel field, and
leave it as set by default in the initial queue configuration.

[1] https://github.com/DPDK/dpdk/commit/4cd8c72f6

Fixes: de5da9d16430 ("net/ice: support double VLAN")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/intel/ice/ice_ethdev.c | 75 ++----------------------------
 1 file changed, 3 insertions(+), 72 deletions(-)

diff --git a/drivers/net/intel/ice/ice_ethdev.c b/drivers/net/intel/ice/ice_ethdev.c
index 513777e372..4a6e580628 100644
--- a/drivers/net/intel/ice/ice_ethdev.c
+++ b/drivers/net/intel/ice/ice_ethdev.c
@@ -4965,49 +4965,12 @@ ice_vsi_config_vlan_stripping(struct ice_vsi *vsi, bool ena)
 	return ret;
 }
 
-/**
- * ice_vsi_update_l2tsel - update l2tsel field for all Rx rings on this VSI
- * @vsi: VSI used to update l2tsel on
- * @l2tsel: l2tsel setting requested
- *
- * Use the l2tsel setting to update all of the Rx queue context bits for l2tsel.
- * This will modify which descriptor field the first offloaded VLAN will be
- * stripped into.
- */
-static void ice_vsi_update_l2tsel(struct ice_vsi *vsi, enum ice_l2tsel l2tsel)
-{
-	struct ice_hw *hw = ICE_VSI_TO_HW(vsi);
-	struct ice_pf *pf = ICE_VSI_TO_PF(vsi);
-	struct rte_eth_dev_data *dev_data = pf->dev_data;
-	u32 l2tsel_bit;
-	uint16_t i;
-
-	if (l2tsel == ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG2_2ND)
-		l2tsel_bit = 0;
-	else
-		l2tsel_bit = BIT(ICE_L2TSEL_BIT_OFFSET);
-
-	for (i = 0; i < dev_data->nb_rx_queues; i++) {
-		const struct ci_rx_queue *rxq = dev_data->rx_queues[i];
-		u32 qrx_context_offset;
-		u32 regval;
-
-		qrx_context_offset = QRX_CONTEXT(ICE_L2TSEL_QRX_CONTEXT_REG_IDX, rxq->reg_idx);
-
-		regval = rd32(hw, qrx_context_offset);
-		regval &= ~BIT(ICE_L2TSEL_BIT_OFFSET);
-		regval |= l2tsel_bit;
-		wr32(hw, qrx_context_offset, regval);
-	}
-}
-
 /* Configure outer vlan stripping on or off in QinQ mode */
 static int
 ice_vsi_config_outer_vlan_stripping(struct ice_vsi *vsi, bool on)
 {
 	uint16_t outer_ethertype = vsi->adapter->pf.outer_ethertype;
 	struct ice_hw *hw = ICE_VSI_TO_HW(vsi);
-	int err = 0;
 
 	if (vsi->vsi_id >= ICE_MAX_NUM_VSIS) {
 		PMD_DRV_LOG(ERR, "VSI ID exceeds the maximum");
@@ -5019,41 +4982,9 @@ ice_vsi_config_outer_vlan_stripping(struct ice_vsi *vsi, bool on)
 		return -EOPNOTSUPP;
 	}
 
-	if (on) {
-		err = ice_vsi_ena_outer_stripping(vsi, outer_ethertype);
-		if (!err) {
-			enum ice_l2tsel l2tsel =
-				ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG2_2ND;
-
-			/* PF tells the VF that the outer VLAN tag is always
-			 * extracted to VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2_2 and
-			 * inner is always extracted to
-			 * VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1. This is needed to
-			 * support outer stripping so the first tag always ends
-			 * up in L2TAG2_2ND and the second/inner tag, if
-			 * enabled, is extracted in L2TAG1.
-			 */
-			ice_vsi_update_l2tsel(vsi, l2tsel);
-		}
-	} else {
-		err = ice_vsi_dis_outer_stripping(vsi);
-		if (!err) {
-			enum ice_l2tsel l2tsel =
-				ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG1;
-
-			/* PF tells the VF that the outer VLAN tag is always
-			 * extracted to VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2_2 and
-			 * inner is always extracted to
-			 * VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1. This is needed to
-			 * support inner stripping while outer stripping is
-			 * disabled so that the first and only tag is extracted
-			 * in L2TAG1.
-			 */
-			ice_vsi_update_l2tsel(vsi, l2tsel);
-		}
-	}
-
-	return err;
+	return on ?
+		ice_vsi_ena_outer_stripping(vsi, outer_ethertype) :
+		ice_vsi_dis_outer_stripping(vsi);
 }
 
 static int
-- 
2.48.1

RE: [PATCH] net/ice: fix VLAN tag reporting on Rx

[Loftus, Ciara]

> 
> The ice driver expects the first, or outer, VLAN tag in a packet to be
> written to the L2TAG1 field of the descriptor, as configured by the
> l2tsel field when configuring the queue context initially for the
> device. However, when configuring the actual VLAN or QinQ strip
> behaviour, that l2tsel field was changed, sending the single/outer vlan
> tag to the L2TAG2 field in the descriptor. This meant that it was not
> getting picked up correctly by the Rx paths.
> 
> This issue has been around for a long time, but was previously
> partially hidden by the issue fixed in [1], since due to that bug,
> the l2tsel field was not getting overridden in the single-queue case
> (since the single queue was the final queue).
> 
> Fix the issue by just removing the code updating the l2tsel field, and
> leave it as set by default in the initial queue configuration.
> 
> [1] https://github.com/DPDK/dpdk/commit/4cd8c72f6
> 
> Fixes: de5da9d16430 ("net/ice: support double VLAN")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/net/intel/ice/ice_ethdev.c | 75 ++----------------------------
>  1 file changed, 3 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/net/intel/ice/ice_ethdev.c
> b/drivers/net/intel/ice/ice_ethdev.c
> index 513777e372..4a6e580628 100644
> --- a/drivers/net/intel/ice/ice_ethdev.c
> +++ b/drivers/net/intel/ice/ice_ethdev.c
> @@ -4965,49 +4965,12 @@ ice_vsi_config_vlan_stripping(struct ice_vsi *vsi,
> bool ena)
>  	return ret;
>  }
> 
> -/**
> - * ice_vsi_update_l2tsel - update l2tsel field for all Rx rings on this VSI
> - * @vsi: VSI used to update l2tsel on
> - * @l2tsel: l2tsel setting requested
> - *
> - * Use the l2tsel setting to update all of the Rx queue context bits for l2tsel.
> - * This will modify which descriptor field the first offloaded VLAN will be
> - * stripped into.
> - */
> -static void ice_vsi_update_l2tsel(struct ice_vsi *vsi, enum ice_l2tsel l2tsel)
> -{
> -	struct ice_hw *hw = ICE_VSI_TO_HW(vsi);
> -	struct ice_pf *pf = ICE_VSI_TO_PF(vsi);
> -	struct rte_eth_dev_data *dev_data = pf->dev_data;
> -	u32 l2tsel_bit;
> -	uint16_t i;
> -
> -	if (l2tsel == ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG2_2ND)
> -		l2tsel_bit = 0;
> -	else
> -		l2tsel_bit = BIT(ICE_L2TSEL_BIT_OFFSET);
> -
> -	for (i = 0; i < dev_data->nb_rx_queues; i++) {
> -		const struct ci_rx_queue *rxq = dev_data->rx_queues[i];
> -		u32 qrx_context_offset;
> -		u32 regval;
> -
> -		qrx_context_offset =
> QRX_CONTEXT(ICE_L2TSEL_QRX_CONTEXT_REG_IDX, rxq->reg_idx);
> -
> -		regval = rd32(hw, qrx_context_offset);
> -		regval &= ~BIT(ICE_L2TSEL_BIT_OFFSET);
> -		regval |= l2tsel_bit;
> -		wr32(hw, qrx_context_offset, regval);
> -	}
> -}
> -
>  /* Configure outer vlan stripping on or off in QinQ mode */
>  static int
>  ice_vsi_config_outer_vlan_stripping(struct ice_vsi *vsi, bool on)
>  {
>  	uint16_t outer_ethertype = vsi->adapter->pf.outer_ethertype;
>  	struct ice_hw *hw = ICE_VSI_TO_HW(vsi);
> -	int err = 0;
> 
>  	if (vsi->vsi_id >= ICE_MAX_NUM_VSIS) {
>  		PMD_DRV_LOG(ERR, "VSI ID exceeds the maximum");
> @@ -5019,41 +4982,9 @@ ice_vsi_config_outer_vlan_stripping(struct ice_vsi
> *vsi, bool on)
>  		return -EOPNOTSUPP;
>  	}
> 
> -	if (on) {
> -		err = ice_vsi_ena_outer_stripping(vsi, outer_ethertype);
> -		if (!err) {
> -			enum ice_l2tsel l2tsel =
> -
> 	ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG2_2ND;
> -
> -			/* PF tells the VF that the outer VLAN tag is always
> -			 * extracted to
> VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2_2 and
> -			 * inner is always extracted to
> -			 * VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1. This is
> needed to
> -			 * support outer stripping so the first tag always ends
> -			 * up in L2TAG2_2ND and the second/inner tag, if
> -			 * enabled, is extracted in L2TAG1.
> -			 */
> -			ice_vsi_update_l2tsel(vsi, l2tsel);
> -		}
> -	} else {
> -		err = ice_vsi_dis_outer_stripping(vsi);
> -		if (!err) {
> -			enum ice_l2tsel l2tsel =
> -				ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG1;
> -
> -			/* PF tells the VF that the outer VLAN tag is always
> -			 * extracted to
> VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2_2 and
> -			 * inner is always extracted to
> -			 * VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1. This is
> needed to
> -			 * support inner stripping while outer stripping is
> -			 * disabled so that the first and only tag is extracted
> -			 * in L2TAG1.
> -			 */
> -			ice_vsi_update_l2tsel(vsi, l2tsel);
> -		}
> -	}
> -
> -	return err;
> +	return on ?
> +		ice_vsi_ena_outer_stripping(vsi, outer_ethertype) :
> +		ice_vsi_dis_outer_stripping(vsi);
>  }
> 
>  static int
> --
> 2.48.1

You can also remove the following #defines and enum related to the code you have removed as they are not used anywhere else as far as I can see.

#define ICE_L2TSEL_QRX_CONTEXT_REG_IDX	3
#define ICE_L2TSEL_BIT_OFFSET		   23
enum ice_l2tsel {
	ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG2_2ND,
	ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG1,
};

Acked-by: Ciara Loftus <ciara.loftus@intel.com>

Re: [PATCH] net/ice: fix VLAN tag reporting on Rx

[Bruce Richardson]

On Thu, Aug 07, 2025 at 02:12:37PM +0100, Loftus, Ciara wrote:
> > 
> > The ice driver expects the first, or outer, VLAN tag in a packet to be
> > written to the L2TAG1 field of the descriptor, as configured by the
> > l2tsel field when configuring the queue context initially for the
> > device. However, when configuring the actual VLAN or QinQ strip
> > behaviour, that l2tsel field was changed, sending the single/outer vlan
> > tag to the L2TAG2 field in the descriptor. This meant that it was not
> > getting picked up correctly by the Rx paths.
> > 
> > This issue has been around for a long time, but was previously
> > partially hidden by the issue fixed in [1], since due to that bug,
> > the l2tsel field was not getting overridden in the single-queue case
> > (since the single queue was the final queue).
> > 
> > Fix the issue by just removing the code updating the l2tsel field, and
> > leave it as set by default in the initial queue configuration.
> > 
> > [1] https://github.com/DPDK/dpdk/commit/4cd8c72f6
> > 
> > Fixes: de5da9d16430 ("net/ice: support double VLAN")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  drivers/net/intel/ice/ice_ethdev.c | 75 ++----------------------------
> >  1 file changed, 3 insertions(+), 72 deletions(-)
> > 
> > diff --git a/drivers/net/intel/ice/ice_ethdev.c
> > b/drivers/net/intel/ice/ice_ethdev.c
> > index 513777e372..4a6e580628 100644
> > --- a/drivers/net/intel/ice/ice_ethdev.c
> > +++ b/drivers/net/intel/ice/ice_ethdev.c
> > @@ -4965,49 +4965,12 @@ ice_vsi_config_vlan_stripping(struct ice_vsi *vsi,
> > bool ena)
> >  	return ret;
> >  }
> > 
> > -/**
> > - * ice_vsi_update_l2tsel - update l2tsel field for all Rx rings on this VSI
> > - * @vsi: VSI used to update l2tsel on
> > - * @l2tsel: l2tsel setting requested
> > - *
> > - * Use the l2tsel setting to update all of the Rx queue context bits for l2tsel.
> > - * This will modify which descriptor field the first offloaded VLAN will be
> > - * stripped into.
> > - */
> > -static void ice_vsi_update_l2tsel(struct ice_vsi *vsi, enum ice_l2tsel l2tsel)
> > -{

<snip

> > -
> > -	return err;
> > +	return on ?
> > +		ice_vsi_ena_outer_stripping(vsi, outer_ethertype) :
v> > +		ice_vsi_dis_outer_stripping(vsi);
> >  }
> > 
> >  static int
> > --
> > 2.48.1
> 
> You can also remove the following #defines and enum related to the code you have removed as they are not used anywhere else as far as I can see.
> 
> #define ICE_L2TSEL_QRX_CONTEXT_REG_IDX	3
> #define ICE_L2TSEL_BIT_OFFSET		   23
> enum ice_l2tsel {
> 	ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG2_2ND,
> 	ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG1,
> };
> 
> Acked-by: Ciara Loftus <ciara.loftus@intel.com>
>
Sure. We can always reintroduce those defines again if it proves necessary
in future, but for now, I'm happy enough to remove all unused code! I'll do
up a new revision.

/Bruce

[PATCH v2] net/ice: fix VLAN tag reporting on Rx

[Bruce Richardson]

The ice driver expects the first, or outer, VLAN tag in a packet to be
written to the L2TAG1 field of the descriptor, as configured by the
l2tsel field when configuring the queue context initially for the
device. However, when configuring the actual VLAN or QinQ strip
behaviour, that l2tsel field was changed, sending the single/outer vlan
tag to the L2TAG2 field in the descriptor. This meant that it was not
getting picked up correctly by the Rx paths.

This issue has been around for a long time, but was previously
partially hidden by the issue fixed in [1], since due to that bug,
the l2tsel field was not getting overridden in the single-queue case
(since the single queue was the final queue).

Fix the issue by just removing the code updating the l2tsel field, and
leave it as set by default in the initial queue configuration.

[1] https://github.com/DPDK/dpdk/commit/4cd8c72f6

Fixes: de5da9d16430 ("net/ice: support double VLAN")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Loftus <ciara.loftus@intel.com>
---
V2: remove extra unnecessary defines
---
 drivers/net/intel/ice/ice_ethdev.c | 83 ++----------------------------
 1 file changed, 3 insertions(+), 80 deletions(-)

diff --git a/drivers/net/intel/ice/ice_ethdev.c b/drivers/net/intel/ice/ice_ethdev.c
index 513777e372..6c65a8341a 100644
--- a/drivers/net/intel/ice/ice_ethdev.c
+++ b/drivers/net/intel/ice/ice_ethdev.c
@@ -65,14 +65,6 @@ static const char * const ice_valid_args[] = {
 /* Maximum number of VSI */
 #define ICE_MAX_NUM_VSIS          (768UL)
 
-/* The 119 bit offset of the LAN Rx queue context is the L2TSEL control bit. */
-#define ICE_L2TSEL_QRX_CONTEXT_REG_IDX	3
-#define ICE_L2TSEL_BIT_OFFSET		   23
-enum ice_l2tsel {
-	ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG2_2ND,
-	ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG1,
-};
-
 struct proto_xtr_ol_flag {
 	const struct rte_mbuf_dynflag param;
 	bool required;
@@ -4965,49 +4957,12 @@ ice_vsi_config_vlan_stripping(struct ice_vsi *vsi, bool ena)
 	return ret;
 }
 
-/**
- * ice_vsi_update_l2tsel - update l2tsel field for all Rx rings on this VSI
- * @vsi: VSI used to update l2tsel on
- * @l2tsel: l2tsel setting requested
- *
- * Use the l2tsel setting to update all of the Rx queue context bits for l2tsel.
- * This will modify which descriptor field the first offloaded VLAN will be
- * stripped into.
- */
-static void ice_vsi_update_l2tsel(struct ice_vsi *vsi, enum ice_l2tsel l2tsel)
-{
-	struct ice_hw *hw = ICE_VSI_TO_HW(vsi);
-	struct ice_pf *pf = ICE_VSI_TO_PF(vsi);
-	struct rte_eth_dev_data *dev_data = pf->dev_data;
-	u32 l2tsel_bit;
-	uint16_t i;
-
-	if (l2tsel == ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG2_2ND)
-		l2tsel_bit = 0;
-	else
-		l2tsel_bit = BIT(ICE_L2TSEL_BIT_OFFSET);
-
-	for (i = 0; i < dev_data->nb_rx_queues; i++) {
-		const struct ci_rx_queue *rxq = dev_data->rx_queues[i];
-		u32 qrx_context_offset;
-		u32 regval;
-
-		qrx_context_offset = QRX_CONTEXT(ICE_L2TSEL_QRX_CONTEXT_REG_IDX, rxq->reg_idx);
-
-		regval = rd32(hw, qrx_context_offset);
-		regval &= ~BIT(ICE_L2TSEL_BIT_OFFSET);
-		regval |= l2tsel_bit;
-		wr32(hw, qrx_context_offset, regval);
-	}
-}
-
 /* Configure outer vlan stripping on or off in QinQ mode */
 static int
 ice_vsi_config_outer_vlan_stripping(struct ice_vsi *vsi, bool on)
 {
 	uint16_t outer_ethertype = vsi->adapter->pf.outer_ethertype;
 	struct ice_hw *hw = ICE_VSI_TO_HW(vsi);
-	int err = 0;
 
 	if (vsi->vsi_id >= ICE_MAX_NUM_VSIS) {
 		PMD_DRV_LOG(ERR, "VSI ID exceeds the maximum");
@@ -5019,41 +4974,9 @@ ice_vsi_config_outer_vlan_stripping(struct ice_vsi *vsi, bool on)
 		return -EOPNOTSUPP;
 	}
 
-	if (on) {
-		err = ice_vsi_ena_outer_stripping(vsi, outer_ethertype);
-		if (!err) {
-			enum ice_l2tsel l2tsel =
-				ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG2_2ND;
-
-			/* PF tells the VF that the outer VLAN tag is always
-			 * extracted to VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2_2 and
-			 * inner is always extracted to
-			 * VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1. This is needed to
-			 * support outer stripping so the first tag always ends
-			 * up in L2TAG2_2ND and the second/inner tag, if
-			 * enabled, is extracted in L2TAG1.
-			 */
-			ice_vsi_update_l2tsel(vsi, l2tsel);
-		}
-	} else {
-		err = ice_vsi_dis_outer_stripping(vsi);
-		if (!err) {
-			enum ice_l2tsel l2tsel =
-				ICE_L2TSEL_EXTRACT_FIRST_TAG_L2TAG1;
-
-			/* PF tells the VF that the outer VLAN tag is always
-			 * extracted to VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2_2 and
-			 * inner is always extracted to
-			 * VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1. This is needed to
-			 * support inner stripping while outer stripping is
-			 * disabled so that the first and only tag is extracted
-			 * in L2TAG1.
-			 */
-			ice_vsi_update_l2tsel(vsi, l2tsel);
-		}
-	}
-
-	return err;
+	return on ?
+		ice_vsi_ena_outer_stripping(vsi, outer_ethertype) :
+		ice_vsi_dis_outer_stripping(vsi);
 }
 
 static int
-- 
2.48.1

Re: [PATCH v2] net/ice: fix VLAN tag reporting on Rx

[Bruce Richardson]

On Thu, Aug 07, 2025 at 08:08:26PM +0100, Bruce Richardson wrote:
> The ice driver expects the first, or outer, VLAN tag in a packet to be
> written to the L2TAG1 field of the descriptor, as configured by the
> l2tsel field when configuring the queue context initially for the
> device. However, when configuring the actual VLAN or QinQ strip
> behaviour, that l2tsel field was changed, sending the single/outer vlan
> tag to the L2TAG2 field in the descriptor. This meant that it was not
> getting picked up correctly by the Rx paths.
> 
> This issue has been around for a long time, but was previously
> partially hidden by the issue fixed in [1], since due to that bug,
> the l2tsel field was not getting overridden in the single-queue case
> (since the single queue was the final queue).
> 
> Fix the issue by just removing the code updating the l2tsel field, and
> leave it as set by default in the initial queue configuration.
> 
> [1] https://github.com/DPDK/dpdk/commit/4cd8c72f6
> 
> Fixes: de5da9d16430 ("net/ice: support double VLAN")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Ciara Loftus <ciara.loftus@intel.com>
> ---
Applied to dpdk-next-net-intel.

/Bruce