Re: [PATCH v5 2/3] common/sfc_efx/base: add support to enable VLAN stripping

[Artemii Morozov <Artemii.Morozov@arknetworks.am>]

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)
>>>
>