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 54C3A42D08; Tue, 20 Jun 2023 15:10:59 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3F9DC40EDF; Tue, 20 Jun 2023 15:10:59 +0200 (CEST) Received: from agw.arknetworks.am (agw.arknetworks.am [79.141.165.80]) by mails.dpdk.org (Postfix) with ESMTP id 8B4124068E for ; Tue, 20 Jun 2023 15:10:58 +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 0E6DBE08A7; Tue, 20 Jun 2023 17:10:57 +0400 (+04) Message-ID: <3210680e-5822-011f-fbbf-7832739713a5@arknetworks.am> Date: Tue, 20 Jun 2023 17:10:56 +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 Content-Language: en-US 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> <34d912b9-4fc0-246d-b89d-be6e4906f36f@arknetworks.am> From: Artemii Morozov In-Reply-To: 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/20/23 15:50, Andrew Rybchenko wrote: > On 6/20/23 12:55, Artemii Morozov wrote: >> 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 > > I'm talking about a way to make sure that it is always the case. > Not this particular case. > >>> 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. > > This one is too strong. Also it does the check in debug build only. > We need run-time check and less strong. Filters could be initialized, > but it should be no Rx filters installed. > >> 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. > > This one is wrong since it could be filters installed via add. > So, it is not a question about default filters only. All Rx filters. > >> >> 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? > > EF10 counts all installed filters if I remember correctly. So, if number > of installed filters is 0, everything is OK. I think you're talking about eft_unicast_filter_count, eft_multicast_filter_count and eft_encap_filter_count from ef10_filter_table_t. But these fields are specific to ef10 filters. But efx_port_vlan_strip_set is a generic function, and we cannot use fields specific to ef10 inside it, since something else may appear in the future. Maybe we need a new API to find out the number of filters installed? (emo_filter_count or something like that). > >> >>> >>>> 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) >>> >