* [PATCH net-next] sfc: support offloading TC VLAN push/pop actions to the MAE
@ 2023-02-16 16:04 edward.cree
2023-02-17 9:00 ` Martin Habets
2023-02-19 9:21 ` Leon Romanovsky
0 siblings, 2 replies; 5+ messages in thread
From: edward.cree @ 2023-02-16 16:04 UTC (permalink / raw)
To: linux-net-drivers, davem, kuba, pabeni, edumazet
Cc: Edward Cree, netdev, habetsm.xilinx
From: Edward Cree <ecree.xilinx@gmail.com>
EF100 can pop and/or push up to two VLAN tags.
Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
drivers/net/ethernet/sfc/mae.c | 43 ++++++++++++++++++++++++++
drivers/net/ethernet/sfc/mcdi.h | 5 ++++
drivers/net/ethernet/sfc/tc.c | 53 +++++++++++++++++++++++++++++++++
drivers/net/ethernet/sfc/tc.h | 4 +++
4 files changed, 105 insertions(+)
diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
index 6321fd393fc3..7ae5b22af624 100644
--- a/drivers/net/ethernet/sfc/mae.c
+++ b/drivers/net/ethernet/sfc/mae.c
@@ -679,9 +679,40 @@ int efx_mae_alloc_action_set(struct efx_nic *efx, struct efx_tc_action_set *act)
{
MCDI_DECLARE_BUF(outbuf, MC_CMD_MAE_ACTION_SET_ALLOC_OUT_LEN);
MCDI_DECLARE_BUF(inbuf, MC_CMD_MAE_ACTION_SET_ALLOC_IN_LEN);
+ unsigned char vlan_push, vlan_pop;
size_t outlen;
int rc;
+ /* Translate vlan actions from bitmask to count */
+ switch (act->vlan_push) {
+ case 0:
+ case 1:
+ vlan_push = act->vlan_push;
+ break;
+ case 2: /* can't happen */
+ default:
+ return -EINVAL;
+ case 3:
+ vlan_push = 2;
+ break;
+ }
+ switch (act->vlan_pop) {
+ case 0:
+ case 1:
+ vlan_pop = act->vlan_pop;
+ break;
+ case 2: /* can't happen */
+ default:
+ return -EINVAL;
+ case 3:
+ vlan_pop = 2;
+ break;
+ }
+
+ MCDI_POPULATE_DWORD_2(inbuf, MAE_ACTION_SET_ALLOC_IN_FLAGS,
+ MAE_ACTION_SET_ALLOC_IN_VLAN_PUSH, vlan_push,
+ MAE_ACTION_SET_ALLOC_IN_VLAN_POP, vlan_pop);
+
MCDI_SET_DWORD(inbuf, MAE_ACTION_SET_ALLOC_IN_SRC_MAC_ID,
MC_CMD_MAE_MAC_ADDR_ALLOC_OUT_MAC_ID_NULL);
MCDI_SET_DWORD(inbuf, MAE_ACTION_SET_ALLOC_IN_DST_MAC_ID,
@@ -694,6 +725,18 @@ int efx_mae_alloc_action_set(struct efx_nic *efx, struct efx_tc_action_set *act)
MC_CMD_MAE_COUNTER_ALLOC_OUT_COUNTER_ID_NULL);
MCDI_SET_DWORD(inbuf, MAE_ACTION_SET_ALLOC_IN_COUNTER_LIST_ID,
MC_CMD_MAE_COUNTER_LIST_ALLOC_OUT_COUNTER_LIST_ID_NULL);
+ if (act->vlan_push & 1) {
+ MCDI_SET_WORD_BE(inbuf, MAE_ACTION_SET_ALLOC_IN_VLAN0_TCI_BE,
+ act->vlan_tci[0]);
+ MCDI_SET_WORD_BE(inbuf, MAE_ACTION_SET_ALLOC_IN_VLAN0_PROTO_BE,
+ act->vlan_proto[0]);
+ }
+ if (act->vlan_push & 2) {
+ MCDI_SET_WORD_BE(inbuf, MAE_ACTION_SET_ALLOC_IN_VLAN1_TCI_BE,
+ act->vlan_tci[1]);
+ MCDI_SET_WORD_BE(inbuf, MAE_ACTION_SET_ALLOC_IN_VLAN1_PROTO_BE,
+ act->vlan_proto[1]);
+ }
MCDI_SET_DWORD(inbuf, MAE_ACTION_SET_ALLOC_IN_ENCAP_HEADER_ID,
MC_CMD_MAE_ENCAP_HEADER_ALLOC_OUT_ENCAP_HEADER_ID_NULL);
if (act->deliver)
diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h
index b139b76febff..454e9d51a4c2 100644
--- a/drivers/net/ethernet/sfc/mcdi.h
+++ b/drivers/net/ethernet/sfc/mcdi.h
@@ -233,6 +233,11 @@ void efx_mcdi_sensor_event(struct efx_nic *efx, efx_qword_t *ev);
((void)BUILD_BUG_ON_ZERO(_field ## _LEN != 2), \
le16_to_cpu(*(__force const __le16 *)MCDI_STRUCT_PTR(_buf, _field)))
/* Write a 16-bit field defined in the protocol as being big-endian. */
+#define MCDI_SET_WORD_BE(_buf, _field, _value) do { \
+ BUILD_BUG_ON(MC_CMD_ ## _field ## _LEN != 2); \
+ BUILD_BUG_ON(MC_CMD_ ## _field ## _OFST & 1); \
+ *(__force __be16 *)MCDI_PTR(_buf, _field) = (_value); \
+ } while (0)
#define MCDI_STRUCT_SET_WORD_BE(_buf, _field, _value) do { \
BUILD_BUG_ON(_field ## _LEN != 2); \
BUILD_BUG_ON(_field ## _OFST & 1); \
diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
index deeaab9ee761..195c288736be 100644
--- a/drivers/net/ethernet/sfc/tc.c
+++ b/drivers/net/ethernet/sfc/tc.c
@@ -286,6 +286,10 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
/* For details of action order constraints refer to SF-123102-TC-1§12.6.1 */
enum efx_tc_action_order {
+ EFX_TC_AO_VLAN1_POP,
+ EFX_TC_AO_VLAN0_POP,
+ EFX_TC_AO_VLAN0_PUSH,
+ EFX_TC_AO_VLAN1_PUSH,
EFX_TC_AO_COUNT,
EFX_TC_AO_DELIVER
};
@@ -294,6 +298,22 @@ static bool efx_tc_flower_action_order_ok(const struct efx_tc_action_set *act,
enum efx_tc_action_order new)
{
switch (new) {
+ case EFX_TC_AO_VLAN0_POP:
+ if (act->vlan_pop & 1)
+ return false;
+ fallthrough;
+ case EFX_TC_AO_VLAN1_POP:
+ if (act->vlan_pop & 2)
+ return false;
+ fallthrough;
+ case EFX_TC_AO_VLAN0_PUSH:
+ if (act->vlan_push & 1)
+ return false;
+ fallthrough;
+ case EFX_TC_AO_VLAN1_PUSH:
+ if (act->vlan_push & 2)
+ return false;
+ fallthrough;
case EFX_TC_AO_COUNT:
if (act->count)
return false;
@@ -393,6 +413,8 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
flow_action_for_each(i, fa, &fr->action) {
struct efx_tc_action_set save;
+ int depth;
+ u16 tci;
if (!act) {
/* more actions after a non-pipe action */
@@ -494,6 +516,37 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
}
*act = save;
break;
+ case FLOW_ACTION_VLAN_POP:
+ if (act->vlan_push & 2) {
+ act->vlan_push &= ~2;
+ } else if (act->vlan_push & 1) {
+ act->vlan_push &= ~1;
+ } else if (efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN0_POP)) {
+ act->vlan_pop |= 1;
+ } else if (efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN1_POP)) {
+ act->vlan_pop |= 2;
+ } else {
+ NL_SET_ERR_MSG_MOD(extack, "More than two VLAN pops, or action order violated");
+ rc = -EINVAL;
+ goto release;
+ }
+ break;
+ case FLOW_ACTION_VLAN_PUSH:
+ if (efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN0_PUSH)) {
+ depth = 0;
+ } else if (efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN1_PUSH)) {
+ depth = 1;
+ } else {
+ rc = -EINVAL;
+ NL_SET_ERR_MSG_MOD(extack, "More than two VLAN pushes, or action order violated");
+ goto release;
+ }
+ tci = fa->vlan.vid & 0x0fff;
+ tci |= fa->vlan.prio << 13;
+ act->vlan_push |= (1 << depth);
+ act->vlan_tci[depth] = cpu_to_be16(tci);
+ act->vlan_proto[depth] = fa->vlan.proto;
+ break;
default:
NL_SET_ERR_MSG_FMT_MOD(extack, "Unhandled action %u",
fa->id);
diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h
index 418ce8c13a06..542853f60c2a 100644
--- a/drivers/net/ethernet/sfc/tc.h
+++ b/drivers/net/ethernet/sfc/tc.h
@@ -19,7 +19,11 @@
#define IS_ALL_ONES(v) (!(typeof (v))~(v))
struct efx_tc_action_set {
+ u16 vlan_push:2;
+ u16 vlan_pop:2;
u16 deliver:1;
+ __be16 vlan_tci[2]; /* TCIs for vlan_push */
+ __be16 vlan_proto[2]; /* Ethertypes for vlan_push */
struct efx_tc_counter_index *count;
u32 dest_mport;
u32 fw_id; /* index of this entry in firmware actions table */
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] sfc: support offloading TC VLAN push/pop actions to the MAE
2023-02-16 16:04 [PATCH net-next] sfc: support offloading TC VLAN push/pop actions to the MAE edward.cree
@ 2023-02-17 9:00 ` Martin Habets
2023-02-19 9:21 ` Leon Romanovsky
1 sibling, 0 replies; 5+ messages in thread
From: Martin Habets @ 2023-02-17 9:00 UTC (permalink / raw)
To: edward.cree
Cc: linux-net-drivers, davem, kuba, pabeni, edumazet, Edward Cree,
netdev
On Thu, Feb 16, 2023 at 04:04:42PM +0000, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
>
> EF100 can pop and/or push up to two VLAN tags.
>
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
> drivers/net/ethernet/sfc/mae.c | 43 ++++++++++++++++++++++++++
> drivers/net/ethernet/sfc/mcdi.h | 5 ++++
> drivers/net/ethernet/sfc/tc.c | 53 +++++++++++++++++++++++++++++++++
> drivers/net/ethernet/sfc/tc.h | 4 +++
> 4 files changed, 105 insertions(+)
>
> diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
> index 6321fd393fc3..7ae5b22af624 100644
> --- a/drivers/net/ethernet/sfc/mae.c
> +++ b/drivers/net/ethernet/sfc/mae.c
> @@ -679,9 +679,40 @@ int efx_mae_alloc_action_set(struct efx_nic *efx, struct efx_tc_action_set *act)
> {
> MCDI_DECLARE_BUF(outbuf, MC_CMD_MAE_ACTION_SET_ALLOC_OUT_LEN);
> MCDI_DECLARE_BUF(inbuf, MC_CMD_MAE_ACTION_SET_ALLOC_IN_LEN);
> + unsigned char vlan_push, vlan_pop;
> size_t outlen;
> int rc;
>
> + /* Translate vlan actions from bitmask to count */
> + switch (act->vlan_push) {
> + case 0:
> + case 1:
> + vlan_push = act->vlan_push;
> + break;
> + case 2: /* can't happen */
Use fallthrough here.
> + default:
> + return -EINVAL;
> + case 3:
> + vlan_push = 2;
> + break;
> + }
> + switch (act->vlan_pop) {
> + case 0:
> + case 1:
> + vlan_pop = act->vlan_pop;
> + break;
> + case 2: /* can't happen */
and here.
Martin
> + default:
> + return -EINVAL;
> + case 3:
> + vlan_pop = 2;
> + break;
> + }
> +
> + MCDI_POPULATE_DWORD_2(inbuf, MAE_ACTION_SET_ALLOC_IN_FLAGS,
> + MAE_ACTION_SET_ALLOC_IN_VLAN_PUSH, vlan_push,
> + MAE_ACTION_SET_ALLOC_IN_VLAN_POP, vlan_pop);
> +
> MCDI_SET_DWORD(inbuf, MAE_ACTION_SET_ALLOC_IN_SRC_MAC_ID,
> MC_CMD_MAE_MAC_ADDR_ALLOC_OUT_MAC_ID_NULL);
> MCDI_SET_DWORD(inbuf, MAE_ACTION_SET_ALLOC_IN_DST_MAC_ID,
> @@ -694,6 +725,18 @@ int efx_mae_alloc_action_set(struct efx_nic *efx, struct efx_tc_action_set *act)
> MC_CMD_MAE_COUNTER_ALLOC_OUT_COUNTER_ID_NULL);
> MCDI_SET_DWORD(inbuf, MAE_ACTION_SET_ALLOC_IN_COUNTER_LIST_ID,
> MC_CMD_MAE_COUNTER_LIST_ALLOC_OUT_COUNTER_LIST_ID_NULL);
> + if (act->vlan_push & 1) {
> + MCDI_SET_WORD_BE(inbuf, MAE_ACTION_SET_ALLOC_IN_VLAN0_TCI_BE,
> + act->vlan_tci[0]);
> + MCDI_SET_WORD_BE(inbuf, MAE_ACTION_SET_ALLOC_IN_VLAN0_PROTO_BE,
> + act->vlan_proto[0]);
> + }
> + if (act->vlan_push & 2) {
> + MCDI_SET_WORD_BE(inbuf, MAE_ACTION_SET_ALLOC_IN_VLAN1_TCI_BE,
> + act->vlan_tci[1]);
> + MCDI_SET_WORD_BE(inbuf, MAE_ACTION_SET_ALLOC_IN_VLAN1_PROTO_BE,
> + act->vlan_proto[1]);
> + }
> MCDI_SET_DWORD(inbuf, MAE_ACTION_SET_ALLOC_IN_ENCAP_HEADER_ID,
> MC_CMD_MAE_ENCAP_HEADER_ALLOC_OUT_ENCAP_HEADER_ID_NULL);
> if (act->deliver)
> diff --git a/drivers/net/ethernet/sfc/mcdi.h b/drivers/net/ethernet/sfc/mcdi.h
> index b139b76febff..454e9d51a4c2 100644
> --- a/drivers/net/ethernet/sfc/mcdi.h
> +++ b/drivers/net/ethernet/sfc/mcdi.h
> @@ -233,6 +233,11 @@ void efx_mcdi_sensor_event(struct efx_nic *efx, efx_qword_t *ev);
> ((void)BUILD_BUG_ON_ZERO(_field ## _LEN != 2), \
> le16_to_cpu(*(__force const __le16 *)MCDI_STRUCT_PTR(_buf, _field)))
> /* Write a 16-bit field defined in the protocol as being big-endian. */
> +#define MCDI_SET_WORD_BE(_buf, _field, _value) do { \
> + BUILD_BUG_ON(MC_CMD_ ## _field ## _LEN != 2); \
> + BUILD_BUG_ON(MC_CMD_ ## _field ## _OFST & 1); \
> + *(__force __be16 *)MCDI_PTR(_buf, _field) = (_value); \
> + } while (0)
> #define MCDI_STRUCT_SET_WORD_BE(_buf, _field, _value) do { \
> BUILD_BUG_ON(_field ## _LEN != 2); \
> BUILD_BUG_ON(_field ## _OFST & 1); \
> diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
> index deeaab9ee761..195c288736be 100644
> --- a/drivers/net/ethernet/sfc/tc.c
> +++ b/drivers/net/ethernet/sfc/tc.c
> @@ -286,6 +286,10 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
>
> /* For details of action order constraints refer to SF-123102-TC-1§12.6.1 */
> enum efx_tc_action_order {
> + EFX_TC_AO_VLAN1_POP,
> + EFX_TC_AO_VLAN0_POP,
> + EFX_TC_AO_VLAN0_PUSH,
> + EFX_TC_AO_VLAN1_PUSH,
> EFX_TC_AO_COUNT,
> EFX_TC_AO_DELIVER
> };
> @@ -294,6 +298,22 @@ static bool efx_tc_flower_action_order_ok(const struct efx_tc_action_set *act,
> enum efx_tc_action_order new)
> {
> switch (new) {
> + case EFX_TC_AO_VLAN0_POP:
> + if (act->vlan_pop & 1)
> + return false;
> + fallthrough;
> + case EFX_TC_AO_VLAN1_POP:
> + if (act->vlan_pop & 2)
> + return false;
> + fallthrough;
> + case EFX_TC_AO_VLAN0_PUSH:
> + if (act->vlan_push & 1)
> + return false;
> + fallthrough;
> + case EFX_TC_AO_VLAN1_PUSH:
> + if (act->vlan_push & 2)
> + return false;
> + fallthrough;
> case EFX_TC_AO_COUNT:
> if (act->count)
> return false;
> @@ -393,6 +413,8 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
>
> flow_action_for_each(i, fa, &fr->action) {
> struct efx_tc_action_set save;
> + int depth;
> + u16 tci;
>
> if (!act) {
> /* more actions after a non-pipe action */
> @@ -494,6 +516,37 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
> }
> *act = save;
> break;
> + case FLOW_ACTION_VLAN_POP:
> + if (act->vlan_push & 2) {
> + act->vlan_push &= ~2;
> + } else if (act->vlan_push & 1) {
> + act->vlan_push &= ~1;
> + } else if (efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN0_POP)) {
> + act->vlan_pop |= 1;
> + } else if (efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN1_POP)) {
> + act->vlan_pop |= 2;
> + } else {
> + NL_SET_ERR_MSG_MOD(extack, "More than two VLAN pops, or action order violated");
> + rc = -EINVAL;
> + goto release;
> + }
> + break;
> + case FLOW_ACTION_VLAN_PUSH:
> + if (efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN0_PUSH)) {
> + depth = 0;
> + } else if (efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN1_PUSH)) {
> + depth = 1;
> + } else {
> + rc = -EINVAL;
> + NL_SET_ERR_MSG_MOD(extack, "More than two VLAN pushes, or action order violated");
> + goto release;
> + }
> + tci = fa->vlan.vid & 0x0fff;
> + tci |= fa->vlan.prio << 13;
> + act->vlan_push |= (1 << depth);
> + act->vlan_tci[depth] = cpu_to_be16(tci);
> + act->vlan_proto[depth] = fa->vlan.proto;
> + break;
> default:
> NL_SET_ERR_MSG_FMT_MOD(extack, "Unhandled action %u",
> fa->id);
> diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h
> index 418ce8c13a06..542853f60c2a 100644
> --- a/drivers/net/ethernet/sfc/tc.h
> +++ b/drivers/net/ethernet/sfc/tc.h
> @@ -19,7 +19,11 @@
> #define IS_ALL_ONES(v) (!(typeof (v))~(v))
>
> struct efx_tc_action_set {
> + u16 vlan_push:2;
> + u16 vlan_pop:2;
> u16 deliver:1;
> + __be16 vlan_tci[2]; /* TCIs for vlan_push */
> + __be16 vlan_proto[2]; /* Ethertypes for vlan_push */
> struct efx_tc_counter_index *count;
> u32 dest_mport;
> u32 fw_id; /* index of this entry in firmware actions table */
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] sfc: support offloading TC VLAN push/pop actions to the MAE
2023-02-16 16:04 [PATCH net-next] sfc: support offloading TC VLAN push/pop actions to the MAE edward.cree
2023-02-17 9:00 ` Martin Habets
@ 2023-02-19 9:21 ` Leon Romanovsky
2023-02-21 20:32 ` Edward Cree
1 sibling, 1 reply; 5+ messages in thread
From: Leon Romanovsky @ 2023-02-19 9:21 UTC (permalink / raw)
To: edward.cree
Cc: linux-net-drivers, davem, kuba, pabeni, edumazet, Edward Cree,
netdev, habetsm.xilinx
On Thu, Feb 16, 2023 at 04:04:42PM +0000, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
>
> EF100 can pop and/or push up to two VLAN tags.
>
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ---
> drivers/net/ethernet/sfc/mae.c | 43 ++++++++++++++++++++++++++
> drivers/net/ethernet/sfc/mcdi.h | 5 ++++
> drivers/net/ethernet/sfc/tc.c | 53 +++++++++++++++++++++++++++++++++
> drivers/net/ethernet/sfc/tc.h | 4 +++
> 4 files changed, 105 insertions(+)
>
> diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
> index 6321fd393fc3..7ae5b22af624 100644
> --- a/drivers/net/ethernet/sfc/mae.c
> +++ b/drivers/net/ethernet/sfc/mae.c
> @@ -679,9 +679,40 @@ int efx_mae_alloc_action_set(struct efx_nic *efx, struct efx_tc_action_set *act)
> {
> MCDI_DECLARE_BUF(outbuf, MC_CMD_MAE_ACTION_SET_ALLOC_OUT_LEN);
> MCDI_DECLARE_BUF(inbuf, MC_CMD_MAE_ACTION_SET_ALLOC_IN_LEN);
> + unsigned char vlan_push, vlan_pop;
> size_t outlen;
> int rc;
>
> + /* Translate vlan actions from bitmask to count */
> + switch (act->vlan_push) {
> + case 0:
> + case 1:
> + vlan_push = act->vlan_push;
> + break;
> + case 2: /* can't happen */
There is no need in case here as "default" will catch.
> + default:
> + return -EINVAL;
> + case 3:
> + vlan_push = 2;
> + break;
> + }
> + switch (act->vlan_pop) {
> + case 0:
> + case 1:
> + vlan_pop = act->vlan_pop;
> + break;
> + case 2: /* can't happen */
> + default:
> + return -EINVAL;
Please rely switch-case semantics and don't put default in the middle.
> + case 3:
> + vlan_pop = 2;
> + break;
> + }
Thanks
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] sfc: support offloading TC VLAN push/pop actions to the MAE
2023-02-19 9:21 ` Leon Romanovsky
@ 2023-02-21 20:32 ` Edward Cree
2023-02-22 8:56 ` Martin Habets
0 siblings, 1 reply; 5+ messages in thread
From: Edward Cree @ 2023-02-21 20:32 UTC (permalink / raw)
To: Leon Romanovsky, edward.cree
Cc: linux-net-drivers, davem, kuba, pabeni, edumazet, netdev,
habetsm.xilinx
On 19/02/2023 09:21, Leon Romanovsky wrote:
> On Thu, Feb 16, 2023 at 04:04:42PM +0000, edward.cree@amd.com wrote:
>> From: Edward Cree <ecree.xilinx@gmail.com>
>>
>> EF100 can pop and/or push up to two VLAN tags.
>>
>> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
...
>> + /* Translate vlan actions from bitmask to count */
>> + switch (act->vlan_push) {
>> + case 0:
>> + case 1:
>> + vlan_push = act->vlan_push;
>> + break;
>> + case 2: /* can't happen */
>
> There is no need in case here as "default" will catch.
>
>> + default:
>> + return -EINVAL;
>> + case 3:
>> + vlan_push = 2;
>> + break;
>> + }
>> + switch (act->vlan_pop) {
>> + case 0:
>> + case 1:
>> + vlan_pop = act->vlan_pop;
>> + break;
>> + case 2: /* can't happen */
>> + default:
>> + return -EINVAL;
>
> Please rely switch-case semantics and don't put default in the middle.
It's legal C and as far as I can tell there's nothing in coding-style.rst
about it; I did it this way so as to put the cases in the logical(?)
ascending order and try to make the code self-document the possible
values of the act-> fields.
Arguably it's the 'default:' rather than the 'case 2:' that's unnecessary
as the switch argument is an unsigned:2 bitfield, so it can only take on
these four values.
Although on revisiting this code I wonder if it makes more sense just to
use the 'count' (rather than 'bitmask') form throughout, including in
act->vlan_push/pop; it makes the tc.c side of the code slightly more
involved, but gets rid of this translation entirely. WDYT?
-ed
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] sfc: support offloading TC VLAN push/pop actions to the MAE
2023-02-21 20:32 ` Edward Cree
@ 2023-02-22 8:56 ` Martin Habets
0 siblings, 0 replies; 5+ messages in thread
From: Martin Habets @ 2023-02-22 8:56 UTC (permalink / raw)
To: Edward Cree
Cc: Leon Romanovsky, edward.cree, linux-net-drivers, davem, kuba,
pabeni, edumazet, netdev
On Tue, Feb 21, 2023 at 08:32:13PM +0000, Edward Cree wrote:
> On 19/02/2023 09:21, Leon Romanovsky wrote:
> > On Thu, Feb 16, 2023 at 04:04:42PM +0000, edward.cree@amd.com wrote:
> >> From: Edward Cree <ecree.xilinx@gmail.com>
> >>
> >> EF100 can pop and/or push up to two VLAN tags.
> >>
> >> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
> ...
> >> + /* Translate vlan actions from bitmask to count */
> >> + switch (act->vlan_push) {
> >> + case 0:
> >> + case 1:
> >> + vlan_push = act->vlan_push;
> >> + break;
> >> + case 2: /* can't happen */
> >
> > There is no need in case here as "default" will catch.
> >
> >> + default:
> >> + return -EINVAL;
> >> + case 3:
> >> + vlan_push = 2;
> >> + break;
> >> + }
> >> + switch (act->vlan_pop) {
> >> + case 0:
> >> + case 1:
> >> + vlan_pop = act->vlan_pop;
> >> + break;
> >> + case 2: /* can't happen */
> >> + default:
> >> + return -EINVAL;
> >
> > Please rely switch-case semantics and don't put default in the middle.
>
> It's legal C and as far as I can tell there's nothing in coding-style.rst
> about it; I did it this way so as to put the cases in the logical(?)
> ascending order and try to make the code self-document the possible
> values of the act-> fields.
> Arguably it's the 'default:' rather than the 'case 2:' that's unnecessary
> as the switch argument is an unsigned:2 bitfield, so it can only take on
> these four values.
Can you replace the switch statement with
vlan_push = act->vlan_push & 1 + act->vlan_push & 2;
Even then it would seem prudent to guard against act->vlan_push == 2.
Martin
> Although on revisiting this code I wonder if it makes more sense just to
> use the 'count' (rather than 'bitmask') form throughout, including in
> act->vlan_push/pop; it makes the tc.c side of the code slightly more
> involved, but gets rid of this translation entirely. WDYT?
>
> -ed
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-02-22 8:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-16 16:04 [PATCH net-next] sfc: support offloading TC VLAN push/pop actions to the MAE edward.cree
2023-02-17 9:00 ` Martin Habets
2023-02-19 9:21 ` Leon Romanovsky
2023-02-21 20:32 ` Edward Cree
2023-02-22 8:56 ` Martin Habets
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).