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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 6/13/23 18:12, 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.

"ef100 datapath" does not make sense in libefx.
"VLAN stripping is supported on EF100 only."
However, such string could be confusing in the future. So, I'd drop
"only"

LGTM, but the problem of the patch is that it does not ensure and does
not highlight in any way that efx_port_vlan_strip_set() must be called
before any filter insertion to have things consistent.
Am I missing something?

efx_port_vlan_strip_set() should check that no filters are installed
(directly or indirectly via default RxQ set).

> diff --git a/drivers/common/sfc_efx/base/efx_port.c b/drivers/common/sfc_efx/base/efx_port.c
> index a5f982e335..7804eb76bc 100644
> --- a/drivers/common/sfc_efx/base/efx_port.c
> +++ b/drivers/common/sfc_efx/base/efx_port.c
> @@ -204,6 +204,24 @@ efx_loopback_type_name(
>   
>   #endif	/* EFSYS_OPT_LOOPBACK */
>   
> +	__checkReturn	efx_rc_t
> +efx_port_vlan_strip_set(
> +	__in		efx_nic_t *enp,
> +	__in		boolean_t enabled)
> +{
> +	efx_port_t *epp = &(enp->en_port);
> +	efx_nic_cfg_t *encp = &(enp->en_nic_cfg);
> +
> +	EFSYS_ASSERT3U(enp->en_magic, ==, EFX_NIC_MAGIC);
> +
> +	if (enabled && !encp->enc_rx_vlan_stripping_supported)
> +		return ENOTSUP;

Return value must be in parenthesis in libefx (common/sfc_efx/base).

> +
> +	epp->ep_vlan_strip = enabled;
> +
> +	return 0;

Return value must be in parenthesis in libefx (common/sfc_efx/base).

> +}
> +
>   			void
>   efx_port_fini(
>   	__in		efx_nic_t *enp)