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 B481242D08; Tue, 20 Jun 2023 15:53:05 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A51F0410D7; Tue, 20 Jun 2023 15:53:05 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id E0B5440EDF for ; Tue, 20 Jun 2023 15:53:04 +0200 (CEST) Received: from [192.168.1.40] (unknown [188.170.81.161]) (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 shelob.oktetlabs.ru (Postfix) with ESMTPSA id 3587950; Tue, 20 Jun 2023 16:53:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 3587950 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1687269184; bh=C+NXVgxfEh7jrT3QGXvTDFoVNXan3GGDObOMOmfDyGw=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=wPI6wBgeKlXkxuRyN+Kb3sB/q0UyrSx3/4m95LZbQ+Le6vghaPztga5QxLM4t38NH ROa1kS+YDIYQ23w/yXrvwNDx5xvaJAQl3WuZXPThHSan5RkujxT3x0MeZjSCJFtAfG Adlc3a/E1Uk1ks0GFk9nKvPfBZp5+xiiLcheyveA= Message-ID: Date: Tue, 20 Jun 2023 16:53:02 +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 v5 2/3] common/sfc_efx/base: add support to enable VLAN stripping Content-Language: en-US To: Artemii Morozov , dev@dpdk.org Cc: Ivan Malov , Viacheslav Galaktionov , Andy Moreton 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> <3210680e-5822-011f-fbbf-7832739713a5@arknetworks.am> From: Andrew Rybchenko In-Reply-To: <3210680e-5822-011f-fbbf-7832739713a5@arknetworks.am> 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 16:10, Artemii Morozov wrote: > > 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). Of course, I'm talking about solution in principle. Yes, it is required to wrap it in a right code. >> >>> >>>> >>>>> 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) >>>> >>