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 489F042D06; Tue, 20 Jun 2023 11:55:29 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3CC2A4113F; Tue, 20 Jun 2023 11:55:29 +0200 (CEST) Received: from agw.arknetworks.am (agw.arknetworks.am [79.141.165.80]) by mails.dpdk.org (Postfix) with ESMTP id 1DBF5400D6 for ; Tue, 20 Jun 2023 11:55:28 +0200 (CEST) Received: from [192.168.2.179] (unknown [141.136.90.242]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by agw.arknetworks.am (Postfix) with ESMTPSA id 35BB5E121A; Tue, 20 Jun 2023 13:55:27 +0400 (+04) Message-ID: <34d912b9-4fc0-246d-b89d-be6e4906f36f@arknetworks.am> Date: Tue, 20 Jun 2023 13:55:19 +0400 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 v5 2/3] common/sfc_efx/base: add support to enable VLAN stripping To: dev@dpdk.org Cc: Ivan Malov , Viacheslav Galaktionov , Andy Moreton , Andrew Rybchenko References: <20230531134122.119508-1-artemii.morozov@arknetworks.am> <20230613151238.59112-1-artemii.morozov@arknetworks.am> <20230613151238.59112-3-artemii.morozov@arknetworks.am> <533f1b4a-2b8a-3b45-2015-f241d8c66efd@oktetlabs.ru> Content-Language: en-US From: Artemii Morozov In-Reply-To: <533f1b4a-2b8a-3b45-2015-f241d8c66efd@oktetlabs.ru> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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/19/23 14:28, Andrew Rybchenko wrote: > 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. This function is called before the sfc_rx_default_rxq_set_filter > efx_port_vlan_strip_set() should check that no filters are installed > (directly or indirectly via default RxQ set). I believe we have the following options: 1. Move the function efx_port_vlan_strip_set before filter initialization(efx_filter_init) and include the statement "EFSYS_ASSERT(!(enp->en_mod_flags & EFX_MOD_FILTER))" at the beginning of the efx_port_vlan_strip_set function. 2. Introduce a new callback function called efx_mac_filter_default_rxq_get. Invoke this function at the start of efx_port_vlan_strip_set and verify that it returns NULL. I don't see any other ways to check that the default filters have been initialized. Could you advise me how to do it better? Am I missing something? > >> 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) >