diff mbox series

[RFC,v2,net-next] sfc: support offloading TC VLAN push/pop actions to the MAE

Message ID 20230223235026.26066-1-edward.cree@amd.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [RFC,v2,net-next] sfc: support offloading TC VLAN push/pop actions to the MAE | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/cc_maintainers warning 4 maintainers not CCed: davem@davemloft.net edumazet@google.com pabeni@redhat.com kuba@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/checkpatch warning WARNING: line length of 112 exceeds 80 columns WARNING: line length of 114 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 1 this patch: 1
netdev/source_inline success Was 0 now: 0

Commit Message

edward.cree@amd.com Feb. 23, 2023, 11:50 p.m. UTC 
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>
---
Changed in v2: reworked act->vlan_push/pop to be counts rather than bitmasks,
 and simplified the corresponding efx_tc_action_order handling.

 drivers/net/ethernet/sfc/mae.c  | 16 +++++++++++++
 drivers/net/ethernet/sfc/mcdi.h |  5 +++++
 drivers/net/ethernet/sfc/tc.c   | 40 +++++++++++++++++++++++++++++++++
 drivers/net/ethernet/sfc/tc.h   |  4 ++++
 4 files changed, 65 insertions(+)

Comments

Simon Horman Feb. 24, 2023, 10:07 a.m. UTC | #1 
On Thu, Feb 23, 2023 at 11:50:26PM +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>
> ---
> Changed in v2: reworked act->vlan_push/pop to be counts rather than bitmasks,
>  and simplified the corresponding efx_tc_action_order handling.

This looks good to me.

As you'll need to repost as a non-RFC I've added a few nits inline.
But those notwithstanding,

Reviewed-by: Simon Horman <simon.horman@corigine.com>

...

> diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
> index deeaab9ee761..12b34320bc81 100644
> --- a/drivers/net/ethernet/sfc/tc.c
> +++ b/drivers/net/ethernet/sfc/tc.c

...

> @@ -494,6 +511,29 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
>  			}
>  			*act = save;
>  			break;
> +		case FLOW_ACTION_VLAN_POP:
> +			if (act->vlan_push) {
> +				act->vlan_push--;
> +			} else if (efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN_POP)) {
> +				act->vlan_pop++;
> +			} else {
> +				NL_SET_ERR_MSG_MOD(extack, "More than two VLAN pops, or action order violated");

nit: I'm not sure if there is anything to be done about it,
     but checkpatch complains about ling lines here...

> +				rc = -EINVAL;
> +				goto release;
> +			}
> +			break;
> +		case FLOW_ACTION_VLAN_PUSH:
> +			if (!efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN_PUSH)) {
> +				rc = -EINVAL;
> +				NL_SET_ERR_MSG_MOD(extack, "More than two VLAN pushes, or action order violated");

... and here.

> +				goto release;
> +			}
> +			tci = fa->vlan.vid & 0x0fff;
> +			tci |= fa->vlan.prio << 13;

nit: Maybe VLAN_PRIO_SHIFT and VLAN_VID_MASK can be used here.

> +			act->vlan_tci[act->vlan_push] = cpu_to_be16(tci);
> +			act->vlan_proto[act->vlan_push] = fa->vlan.proto;
> +			act->vlan_push++;
> +			break;
>  		default:
>  			NL_SET_ERR_MSG_FMT_MOD(extack, "Unhandled action %u",
>  					       fa->id);

...
Edward Cree Feb. 27, 2023, 9:33 p.m. UTC | #2 
On 24/02/2023 10:07, Simon Horman wrote:
> On Thu, Feb 23, 2023 at 11:50:26PM +0000, edward.cree@amd.com wrote:
>> +				NL_SET_ERR_MSG_MOD(extack, "More than two VLAN pops, or action order violated");
> 
> nit: I'm not sure if there is anything to be done about it,
>      but checkpatch complains about ling lines here...

Yeah I don't think these can be helped.  Breaking up the
 containing function (to reduce indent depth) would be
 rather synthetic imho, most of it wouldn't even be able
 to be shared with the decap and conntrack versions when
 those get added.)

>> +			}
>> +			tci = fa->vlan.vid & 0x0fff;
>> +			tci |= fa->vlan.prio << 13;
> 
> nit: Maybe VLAN_PRIO_SHIFT and VLAN_VID_MASK can be used here.

Yep good suggestion, incorporated for v3.
Thanks for the review.

-ed
Martin Habets Feb. 28, 2023, 9:01 a.m. UTC | #3 
On Mon, Feb 27, 2023 at 09:33:49PM +0000, Edward Cree wrote:
> On 24/02/2023 10:07, Simon Horman wrote:
> > On Thu, Feb 23, 2023 at 11:50:26PM +0000, edward.cree@amd.com wrote:
> >> +				NL_SET_ERR_MSG_MOD(extack, "More than two VLAN pops, or action order violated");
> > 
> > nit: I'm not sure if there is anything to be done about it,
> >      but checkpatch complains about ling lines here...
> 
> Yeah I don't think these can be helped.  Breaking up the
>  containing function (to reduce indent depth) would be
>  rather synthetic imho, most of it wouldn't even be able
>  to be shared with the decap and conntrack versions when
>  those get added.)

You can put the string on it's own line, i.e. align it under
extack. I think that will pacify checkpatch.

Martin

> 
> >> +			}
> >> +			tci = fa->vlan.vid & 0x0fff;
> >> +			tci |= fa->vlan.prio << 13;
> > 
> > nit: Maybe VLAN_PRIO_SHIFT and VLAN_VID_MASK can be used here.
> 
> Yep good suggestion, incorporated for v3.
> Thanks for the review.
> 
> -ed
diff mbox series

Patch

diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
index 6321fd393fc3..142b3d6ae6aa 100644
--- a/drivers/net/ethernet/sfc/mae.c
+++ b/drivers/net/ethernet/sfc/mae.c
@@ -682,6 +682,10 @@  int efx_mae_alloc_action_set(struct efx_nic *efx, struct efx_tc_action_set *act)
 	size_t outlen;
 	int rc;
 
+	MCDI_POPULATE_DWORD_2(inbuf, MAE_ACTION_SET_ALLOC_IN_FLAGS,
+			      MAE_ACTION_SET_ALLOC_IN_VLAN_PUSH, act->vlan_push,
+			      MAE_ACTION_SET_ALLOC_IN_VLAN_POP, act->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 +698,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) {
+		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..12b34320bc81 100644
--- a/drivers/net/ethernet/sfc/tc.c
+++ b/drivers/net/ethernet/sfc/tc.c
@@ -286,6 +286,8 @@  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_VLAN_POP,
+	EFX_TC_AO_VLAN_PUSH,
 	EFX_TC_AO_COUNT,
 	EFX_TC_AO_DELIVER
 };
@@ -294,6 +296,20 @@  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_VLAN_POP:
+		if (act->vlan_pop >= 2)
+			return false;
+		/* If we've already pushed a VLAN, we can't then pop it;
+		 * the hardware would instead try to pop an existing VLAN
+		 * before pushing the new one.
+		 */
+		if (act->vlan_push)
+			return false;
+		fallthrough;
+	case EFX_TC_AO_VLAN_PUSH:
+		if (act->vlan_push >= 2)
+			return false;
+		fallthrough;
 	case EFX_TC_AO_COUNT:
 		if (act->count)
 			return false;
@@ -393,6 +409,7 @@  static int efx_tc_flower_replace(struct efx_nic *efx,
 
 	flow_action_for_each(i, fa, &fr->action) {
 		struct efx_tc_action_set save;
+		u16 tci;
 
 		if (!act) {
 			/* more actions after a non-pipe action */
@@ -494,6 +511,29 @@  static int efx_tc_flower_replace(struct efx_nic *efx,
 			}
 			*act = save;
 			break;
+		case FLOW_ACTION_VLAN_POP:
+			if (act->vlan_push) {
+				act->vlan_push--;
+			} else if (efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN_POP)) {
+				act->vlan_pop++;
+			} 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_VLAN_PUSH)) {
+				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_tci[act->vlan_push] = cpu_to_be16(tci);
+			act->vlan_proto[act->vlan_push] = fa->vlan.proto;
+			act->vlan_push++;
+			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 */