[PATCH v4 2/3] common/sfc_efx/base: add support to enable VLAN stripping

[Andrew Rybchenko]

On 6/1/23 18:30, Artemii Morozov wrote:
> To enable VLAN stripping, two conditions must be met:
> the corresponding flag must be set and the appropriate
> RX prefix should be requested.

RX -> Rx

> VLAN stripping is supported for ef100 datapath only.

When you read below notes carefully, you'll understand that the
patch is very raw yet. There are too many questions below.

The major decision to be made is if libefx API should enforce
device level offload or should just provide transparent API and
driver must be responsible for consistency.

If driver must be responsible for consistency, don't try to
enforce VLAN stripping enable via filter table. RxQ flags
should control stripped VLAN delivery. Filter flag and default
RxQ set flag (API should be extended) should control VLAN
stripping enabling.

If libefx must guarantee consistency, we need a new way to
enable device level offload early (efx_rx_init()-like, may
be a new API). If so, it should be impossible to control
VLAN stripping per Rx filter. It still could be possible
to control stripped VLAN delivery per RxQ.

Anyway current solution is really inconsistent.

> 
> Signed-off-by: Artemii Morozov <artemii.morozov at arknetworks.am>
> Reviewed-by: Ivan Malov <ivan.malov at arknetworks.am>
> Reviewed-by: Andy Moreton <amoreton at xilinx.com>
> ---
>   drivers/common/sfc_efx/base/ef10_filter.c | 21 +++++++++++++++++++++
>   drivers/common/sfc_efx/base/ef10_impl.h   |  1 +
>   drivers/common/sfc_efx/base/efx.h         |  9 +++++++++
>   drivers/common/sfc_efx/base/efx_filter.c  |  3 ++-
>   drivers/common/sfc_efx/base/efx_impl.h    |  1 +
>   drivers/common/sfc_efx/base/efx_rx.c      | 17 +++++++++++++++++
>   drivers/common/sfc_efx/base/rhead_rx.c    |  3 +++
>   7 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/common/sfc_efx/base/ef10_filter.c b/drivers/common/sfc_efx/base/ef10_filter.c
> index 6d19797d16..6371d8d489 100644
> --- a/drivers/common/sfc_efx/base/ef10_filter.c
> +++ b/drivers/common/sfc_efx/base/ef10_filter.c
> @@ -338,6 +338,11 @@ efx_mcdi_filter_op_add(
>   		    MC_CMD_FILTER_OP_V3_IN_MATCH_ACTION_FLAG);
>   	}
>   
> +	if (spec->efs_flags & EFX_FILTER_FLAG_VLAN_STRIP) {
> +		MCDI_IN_SET_DWORD_FIELD(req, FILTER_OP_V3_IN_MATCH_ACTION_FLAGS,
> +			FILTER_OP_V3_IN_MATCH_STRIP_VLAN, 1);
> +	}
> +
>   	efx_mcdi_execute(enp, &req);
>   
>   	if (req.emr_rc != 0) {
> @@ -852,6 +857,16 @@ ef10_filter_add_internal(
>   
>   	hash = ef10_filter_hash(spec);
>   
> +	/*
> +	 * VLAN stripping is offload at the device level, and it should be
> +	 * applied to all filters if it has been applied at least once before.
> +	 * Knowledge of device level vlan strip offload being enabled comes from

vlan -> VLAN

> +	 * the default RxQ flags, albeit the flag may be set on any queue.
> +	 * But only default RxQ affects this 'eft_strip_vlan' decision.
> +	 */
> +	if (eftp->eft_strip_vlan)
> +		spec->efs_flags |= EFX_FILTER_FLAG_VLAN_STRIP;
> +
>   	/*
>   	 * FIXME: Add support for inserting filters of different priorities
>   	 * and removing lower priority multicast filters (bug 42378)
> @@ -2010,6 +2025,9 @@ ef10_filter_reconfigure(
>   	else
>   		filter_flags = 0;
>   
> +	if (table->eft_strip_vlan)
> +		filter_flags |= EFX_FILTER_FLAG_VLAN_STRIP;
> +
>   	/* Mark old filters which may need to be removed */
>   	ef10_filter_mark_old_filters(enp);
>   
> @@ -2142,6 +2160,8 @@ ef10_filter_default_rxq_set(
>   	EFSYS_ASSERT(using_rss == B_FALSE);
>   	table->eft_using_rss = B_FALSE;
>   #endif
> +	if (erp->er_flags & EFX_RXQ_FLAG_VLAN_STRIP)
> +		table->eft_strip_vlan = B_TRUE;

How to enable VLAN stripping in isolated mode where
API to set default RxQ is not called?

>   	table->eft_default_rxq = erp;
>   }
>   
> @@ -2153,6 +2173,7 @@ ef10_filter_default_rxq_clear(
>   
>   	table->eft_default_rxq = NULL;
>   	table->eft_using_rss = B_FALSE;
> +	table->eft_strip_vlan = B_FALSE;
>   }
>   
>   
> diff --git a/drivers/common/sfc_efx/base/ef10_impl.h b/drivers/common/sfc_efx/base/ef10_impl.h
> index 877aedad45..017e561f19 100644
> --- a/drivers/common/sfc_efx/base/ef10_impl.h
> +++ b/drivers/common/sfc_efx/base/ef10_impl.h
> @@ -1287,6 +1287,7 @@ typedef struct ef10_filter_table_s {
>   	ef10_filter_entry_t	eft_entry[EFX_EF10_FILTER_TBL_ROWS];
>   	efx_rxq_t		*eft_default_rxq;
>   	boolean_t		eft_using_rss;
> +	boolean_t		eft_strip_vlan;
>   	uint32_t		eft_unicst_filter_indexes[
>   	    EFX_EF10_FILTER_UNICAST_FILTERS_MAX];
>   	uint32_t		eft_unicst_filter_count;
> diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
> index ac89b418e0..508d0a802d 100644
> --- a/drivers/common/sfc_efx/base/efx.h
> +++ b/drivers/common/sfc_efx/base/efx.h
> @@ -3101,6 +3101,10 @@ typedef enum efx_rxq_type_e {
>    * Request user flag field in the Rx prefix of a queue.
>    */
>   #define	EFX_RXQ_FLAG_USER_FLAG		0x20
> +/*
> + * Request vlan tci field in the Rx prefix of a queue.

vlan -> VLAN, tci -> TCI

The name and comment are a bit misleading. Name sounds like it
controls VLAN stripping on the RxQ.

Consider EFX_RXQ_FLAG_STRIPPED_VLAN to highlight that it
just requests stripped VLAN TCI. Moreover, comment should
say that it does *not* control VLAN stripping. It just
controls delivery of the stripped VLAN TCI if VLAN stripping
is enabled and done for a packet.

However, as far as I can see finally the flag is used to
enable VLAN stripping as well and it is terribly bad since
it adds false expectations on per-RxQ control for the feature.
It the flag should really control Rx VLAN stripping,
comment should say so and explain limitations.

> + */
> +#define	EFX_RXQ_FLAG_VLAN_STRIP		0x40
>   
>   LIBEFX_API
>   extern	__checkReturn	efx_rc_t
> @@ -3455,6 +3459,11 @@ efx_tx_qdestroy(
>   #define	EFX_FILTER_FLAG_ACTION_FLAG	0x20
>   /* Set match mark on the received packet */
>   #define	EFX_FILTER_FLAG_ACTION_MARK	0x40
> +/*
> + * Request that the first tag in the outer L2 header be stripped
> + * and TCI be indicated in Rx prefix.
> + */
> +#define	EFX_FILTER_FLAG_VLAN_STRIP	0x80

The flag allows driver to request Rx VLAN stripping in
some filters only. Is it really intended?

>   
>   typedef uint8_t efx_filter_flags_t;
>   
> diff --git a/drivers/common/sfc_efx/base/efx_filter.c b/drivers/common/sfc_efx/base/efx_filter.c
> index 83c37ff859..778ed0c370 100644
> --- a/drivers/common/sfc_efx/base/efx_filter.c
> +++ b/drivers/common/sfc_efx/base/efx_filter.c
> @@ -322,7 +322,8 @@ efx_filter_spec_init_rx(
>   	EFSYS_ASSERT3P(spec, !=, NULL);
>   	EFSYS_ASSERT3P(erp, !=, NULL);
>   	EFSYS_ASSERT((flags & ~(EFX_FILTER_FLAG_RX_RSS |
> -				EFX_FILTER_FLAG_RX_SCATTER)) == 0);
> +				EFX_FILTER_FLAG_RX_SCATTER |
> +				EFX_FILTER_FLAG_VLAN_STRIP)) == 0);
>   
>   	memset(spec, 0, sizeof (*spec));
>   	spec->efs_priority = priority;
> diff --git a/drivers/common/sfc_efx/base/efx_impl.h b/drivers/common/sfc_efx/base/efx_impl.h
> index 45e99d01c5..c45669e077 100644
> --- a/drivers/common/sfc_efx/base/efx_impl.h
> +++ b/drivers/common/sfc_efx/base/efx_impl.h
> @@ -1045,6 +1045,7 @@ struct efx_rxq_s {
>   	efsys_mem_t			*er_esmp;
>   	efx_evq_rxq_state_t		*er_ev_qstate;
>   	efx_rx_prefix_layout_t		er_prefix_layout;
> +	uint16_t			er_flags;

Why is it 16-bit width. It shares typedefs with RxQ create
flags which are unsigned int?

>   };
>   
>   #define	EFX_RXQ_MAGIC	0x15022005
> diff --git a/drivers/common/sfc_efx/base/efx_rx.c b/drivers/common/sfc_efx/base/efx_rx.c
> index 68f42f5cac..5726085953 100644
> --- a/drivers/common/sfc_efx/base/efx_rx.c
> +++ b/drivers/common/sfc_efx/base/efx_rx.c
> @@ -925,6 +925,7 @@ efx_rx_qcreate_internal(
>   	erp->er_index = index;
>   	erp->er_mask = ndescs - 1;
>   	erp->er_esmp = esmp;
> +	erp->er_flags = 0;
>   
>   	if ((rc = erxop->erxo_qcreate(enp, index, label, type, type_data, esmp,
>   	    ndescs, id, flags, eep, erp)) != 0)
> @@ -941,11 +942,27 @@ efx_rx_qcreate_internal(

As far as I can see it is a bug here since rc is 0, but the
function should fail. Please, submit separate patch with
appropriate Fixes tag.

>   			goto fail5;
>   	}
>   
> +	if (flags & EFX_RXQ_FLAG_VLAN_STRIP) {
> +		const efx_rx_prefix_layout_t *erplp = &erp->er_prefix_layout;
> +		const efx_rx_prefix_field_info_t *vlan_tci_field;
> +
> +		vlan_tci_field =
> +		    &erplp->erpl_fields[EFX_RX_PREFIX_FIELD_VLAN_STRIP_TCI];
> +		if (vlan_tci_field->erpfi_width_bits == 0) {
> +			rc = ENOTSUP;
> +			goto fail6;
> +		}

IMHO support for Rx VLAN stripping capability should be
checked here as well. It is different aspects of the HW
support:
  1. Support for Rx VLAN stripping
  2. Delivery of the stripped VLAN tag

> +
> +		erp->er_flags |= EFX_RXQ_FLAG_VLAN_STRIP;
> +	}
> +
>   	enp->en_rx_qcount++;
>   	*erpp = erp;
>   
>   	return (0);
>   
> +fail6:
> +	EFSYS_PROBE(fail6);
>   fail5:
>   	EFSYS_PROBE(fail5);
>