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

[Bruce Richardson <bruce.richardson@intel.com>]

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