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

[Andrew Rybchenko]

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.

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