[v5,2/3] common/sfc_efx/base: add support to enable VLAN stripping
Checks
Commit Message
To enable VLAN stripping, two conditions must be met:
the corresponding flag must be set and the appropriate
RX prefix should be requested.
VLAN stripping is supported for ef100 datapath only.
Signed-off-by: Artemii Morozov <artemii.morozov@arknetworks.am>
Reviewed-by: Ivan Malov <ivan.malov@arknetworks.am>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
drivers/common/sfc_efx/base/ef10_filter.c | 6 ++++++
drivers/common/sfc_efx/base/efx.h | 12 ++++++++++++
drivers/common/sfc_efx/base/efx_impl.h | 1 +
drivers/common/sfc_efx/base/efx_port.c | 18 ++++++++++++++++++
drivers/common/sfc_efx/base/efx_rx.c | 14 ++++++++++++++
drivers/common/sfc_efx/base/rhead_rx.c | 3 +++
drivers/common/sfc_efx/version.map | 1 +
7 files changed, 55 insertions(+)
Comments
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.
Am I missing something?
efx_port_vlan_strip_set() should check that no filters are installed
(directly or indirectly via default RxQ set).
> 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)
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)
>
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)
>>
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)
>>>
>
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)
>>>>
>>
@@ -171,6 +171,7 @@ efx_mcdi_filter_op_add(
EFX_MCDI_DECLARE_BUF(payload, MC_CMD_FILTER_OP_V3_IN_LEN,
MC_CMD_FILTER_OP_EXT_OUT_LEN);
efx_filter_match_flags_t match_flags;
+ efx_port_t *epp = &(enp->en_port);
uint32_t port_id;
efx_rc_t rc;
@@ -338,6 +339,11 @@ efx_mcdi_filter_op_add(
FILTER_OP_V3_IN_MATCH_SET_FLAG, 1);
}
+ if (epp->ep_vlan_strip) {
+ MCDI_IN_SET_DWORD_FIELD(req, FILTER_OP_V3_IN_MATCH_ACTION_FLAGS,
+ FILTER_OP_V3_IN_MATCH_STRIP_VLAN, 1);
+ }
+
efx_mcdi_execute(enp, &req);
if (req.emr_rc != 0) {
@@ -1147,6 +1147,12 @@ efx_port_poll(
__in efx_nic_t *enp,
__out_opt efx_link_mode_t *link_modep);
+LIBEFX_API
+extern __checkReturn efx_rc_t
+efx_port_vlan_strip_set(
+ __in efx_nic_t *enp,
+ __in boolean_t enabled);
+
LIBEFX_API
extern void
efx_port_fini(
@@ -3101,6 +3107,12 @@ typedef enum efx_rxq_type_e {
* Request user flag field in the Rx prefix of a queue.
*/
#define EFX_RXQ_FLAG_USER_FLAG 0x20
+/*
+ * Request VLAN TCI field in the Rx prefix. The flag just
+ * controls delivery of the stripped VLAN TCI if VLAN stripping
+ * is enabled and done.
+ */
+#define EFX_RXQ_FLAG_VLAN_STRIPPED_TCI 0x40
LIBEFX_API
extern __checkReturn efx_rc_t
@@ -363,6 +363,7 @@ typedef struct efx_port_s {
uint32_t ep_default_adv_cap_mask;
uint32_t ep_phy_cap_mask;
boolean_t ep_mac_drain;
+ boolean_t ep_vlan_strip;
#if EFSYS_OPT_BIST
efx_bist_type_t ep_current_bist;
#endif
@@ -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;
+
+ epp->ep_vlan_strip = enabled;
+
+ return 0;
+}
+
void
efx_port_fini(
__in efx_nic_t *enp)
@@ -941,11 +941,25 @@ efx_rx_qcreate_internal(
goto fail5;
}
+ if (flags & EFX_RXQ_FLAG_VLAN_STRIPPED_TCI) {
+ const efx_rx_prefix_layout_t *erplp = &erp->er_prefix_layout;
+ const efx_rx_prefix_field_info_t *vlan_tci_field;
+
+ vlan_tci_field =
+ &erplp->erpl_fields[EFX_RX_PREFIX_FIELD_VLAN_STRIP_TCI];
+ if (vlan_tci_field->erpfi_width_bits == 0) {
+ rc = ENOTSUP;
+ goto fail6;
+ }
+ }
+
enp->en_rx_qcount++;
*erpp = erp;
return (0);
+fail6:
+ EFSYS_PROBE(fail6);
fail5:
EFSYS_PROBE(fail5);
@@ -640,6 +640,9 @@ rhead_rx_qcreate(
if (flags & EFX_RXQ_FLAG_USER_FLAG)
fields_mask |= 1U << EFX_RX_PREFIX_FIELD_USER_FLAG;
+ if (flags & EFX_RXQ_FLAG_VLAN_STRIPPED_TCI)
+ fields_mask |= 1U << EFX_RX_PREFIX_FIELD_VLAN_STRIP_TCI;
+
/*
* LENGTH is required in EF100 host interface, as receive events
* do not include the packet length.
@@ -203,6 +203,7 @@ INTERNAL {
efx_port_init;
efx_port_loopback_set;
efx_port_poll;
+ efx_port_vlan_strip_set;
efx_pseudo_hdr_hash_get;
efx_pseudo_hdr_pkt_length_get;