From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6E36642C0E; Fri, 2 Jun 2023 09:22:22 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 01E84406B8; Fri, 2 Jun 2023 09:22:22 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 2FE1140695 for ; Fri, 2 Jun 2023 09:22:20 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 42EC750; Fri, 2 Jun 2023 10:22:19 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 42EC750 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1685690539; bh=GFG05OqzU+qjK4ywItNK3aUd5Sn6R8FCFWakcTclV8c=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=pd67kM9teIKfYt8pxu4AdcadecJeXiIcXzkXAz41pDtLmQmq24ucVYBVexoNoXpr+ jcnd0ev+dv7vzflxEbif7x1/bLDbzBZmsqrxxVSC/AuLbBCPkY7B75Cf8f93QJ4hbV k6Wn8Mx9bedagc4pc4Sz3kK5zyq79AaBnzoQjEfU= Message-ID: <128e2ba9-f855-3ce7-f21a-a285cda6d920@oktetlabs.ru> Date: Fri, 2 Jun 2023 10:22:19 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH v4 1/3] common/sfc_efx/base: report VLAN stripping capability To: Artemii Morozov , dev@dpdk.org Cc: Ivan Malov , Viacheslav Galaktionov , Andy Moreton References: <20230531134122.119508-1-artemii.morozov@arknetworks.am> <20230601153022.99634-1-artemii.morozov@arknetworks.am> <20230601153022.99634-2-artemii.morozov@arknetworks.am> Content-Language: en-US From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20230601153022.99634-2-artemii.morozov@arknetworks.am> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > Reviewed-by: Ivan Malov > Reviewed-by: Andy Moreton > --- > 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;