Re: [PATCH v4 1/3] common/sfc_efx/base: report VLAN stripping capability

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

On 6/1/23 18:30, Artemii Morozov wrote:
> These changes are necessary in order to add support for stripping
> VLAN tags in the future.
> 
> Signed-off-by: Artemii Morozov <artemii.morozov@arknetworks.am>
> Reviewed-by: Ivan Malov <ivan.malov@arknetworks.am>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
>   drivers/common/sfc_efx/base/ef10_nic.c  | 6 ++++++
>   drivers/common/sfc_efx/base/efx.h       | 1 +
>   drivers/common/sfc_efx/base/siena_nic.c | 1 +
>   3 files changed, 8 insertions(+)
> 
> diff --git a/drivers/common/sfc_efx/base/ef10_nic.c b/drivers/common/sfc_efx/base/ef10_nic.c
> index e1709d1200..501ce2e37c 100644
> --- a/drivers/common/sfc_efx/base/ef10_nic.c
> +++ b/drivers/common/sfc_efx/base/ef10_nic.c
> @@ -1227,6 +1227,12 @@ ef10_get_datapath_caps(
>   	else
>   		encp->enc_init_evq_extended_width_supported = B_FALSE;
>   
> +	/* Check if firmware supports VLAN stripping. */
> +	if (CAP_FLAGS1(req, RX_VLAN_STRIPPING))
> +		encp->enc_rx_vlan_stripping = B_TRUE;
> +	else
> +		encp->enc_rx_vlan_stripping = B_FALSE;
> +

I'd like to understand the logic how the place is chosen.
Here it is put between two EvQ related flags. Why?
Also as far as I can see it is not about alphabetical order.
It is not at the end of the list.
I perfectly realize that the order here is far from ideal,
but chosen place is very-very strange.
Since we have no requirement to put at the end of the list,
it should be nearby TX_VLAN_INSERTION since these flags are
close in MCDI header and logically related.

>   	/*
>   	 * Check if the NO_CONT_EV mode for RX events is supported.
>   	 */
> diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
> index 49e29dcc1c..ac89b418e0 100644
> --- a/drivers/common/sfc_efx/base/efx.h
> +++ b/drivers/common/sfc_efx/base/efx.h
> @@ -1638,6 +1638,7 @@ typedef struct efx_nic_cfg_s {
>   	boolean_t		enc_pm_and_rxdp_counters;
>   	boolean_t		enc_mac_stats_40g_tx_size_bins;
>   	uint32_t		enc_tunnel_encapsulations_supported;
> +	boolean_t		enc_rx_vlan_stripping;

Here it is put between two tunnel related flags. Why?
Basically above thoughts are applicable here as well.
Moreover there is a hole just after enc_hw_tx_insert_vlan_enabled.

Naming is a separate question. It is definitely inconsistent
vs naming of boolean flags in the structure. The majority of
flags are either _enabled or _supported. As I understand it
should be _supported in this case.

IMHO, _hw_ is redundant in enc_hw_tx_insert_vlan_enabled, so,
we don't need it here as well.

>   	/*
>   	 * NIC global maximum for unique UDP tunnel ports shared by all
>   	 * functions.
> diff --git a/drivers/common/sfc_efx/base/siena_nic.c b/drivers/common/sfc_efx/base/siena_nic.c
> index 9f14faf271..451ca81bd7 100644
> --- a/drivers/common/sfc_efx/base/siena_nic.c
> +++ b/drivers/common/sfc_efx/base/siena_nic.c
> @@ -189,6 +189,7 @@ siena_board_cfg(
>   	encp->enc_rx_var_packed_stream_supported = B_FALSE;
>   	encp->enc_rx_es_super_buffer_supported = B_FALSE;
>   	encp->enc_fw_subvariant_no_tx_csum_supported = B_FALSE;
> +	encp->enc_rx_vlan_stripping = B_FALSE;
>   
>   	/* Siena supports two 10G ports, and 8 lanes of PCIe Gen2 */
>   	encp->enc_required_pcie_bandwidth_mbps = 2 * 10000;