[PATCH net-next v4 0/8] bridge: mrp: Extend br_mrp_switchdev_*

[PATCH net-next v4 0/8] bridge: mrp: Extend br_mrp_switchdev_*

[Horatiu Vultur]

This patch series extends MRP switchdev to allow the SW to have a better
understanding if the HW can implement the MRP functionality or it needs
to help the HW to run it. There are 3 cases:
- when HW can't implement at all the functionality.
- when HW can implement a part of the functionality but needs the SW
  implement the rest. For example if it can't detect when it stops
  receiving MRP Test frames but it can copy the MRP frames to CPU to
  allow the SW to determine this.  Another example is generating the MRP
  Test frames. If HW can't do that then the SW is used as backup.
- when HW can implement completely the functionality.

So, initially the SW tries to offload the entire functionality in HW, if
that fails it tries offload parts of the functionality in HW and use the
SW as helper and if also this fails then MRP can't run on this HW.

Based on these new calls, implement the switchdev for Ocelot driver. This
is an example where the HW can't run completely the functionality but it
can help the SW to run it, by trapping all MRP frames to CPU.

Also this patch series adds MRP support to DSA and implements the Felix
driver which just reuse the Ocelot functions. This part was just compiled
tested because I don't have any HW on which to do the actual tests.

v4:
 - remove ifdef MRP from include/net/switchdev.h
 - move MRP implementation for Ocelot in a different file such that
   Felix driver can use it.
 - extend DSA with MRP support
 - implement MRP support for Felix.
v3:
 - implement the switchdev calls needed by Ocelot driver.
v2:
 - fix typos in comments and in commit messages
 - remove some of the comments
 - move repeated code in helper function
 - fix issue when deleting a node when sw_backup was true

Horatiu Vultur (8):
  switchdev: mrp: Remove CONFIG_BRIDGE_MRP
  switchdev: mrp: Extend ring_role_mrp and in_role_mrp
  bridge: mrp: Add 'enum br_mrp_hw_support'
  bridge: mrp: Extend br_mrp_switchdev to detect better the errors
  bridge: mrp: Update br_mrp to use new return values of
    br_mrp_switchdev
  net: mscc: ocelot: Add support for MRP
  net: dsa: add MRP support
  net: dsa: felix: Add support for MRP

 drivers/net/dsa/ocelot/felix.c         |  38 ++++++
 drivers/net/ethernet/mscc/Makefile     |   1 +
 drivers/net/ethernet/mscc/ocelot.c     |  10 +-
 drivers/net/ethernet/mscc/ocelot_mrp.c | 175 +++++++++++++++++++++++++
 drivers/net/ethernet/mscc/ocelot_net.c |  60 +++++++++
 include/linux/dsa/ocelot.h             |   5 +
 include/net/dsa.h                      |  12 ++
 include/net/switchdev.h                |  12 +-
 include/soc/mscc/ocelot.h              |  45 +++++++
 net/bridge/br_mrp.c                    |  43 +++---
 net/bridge/br_mrp_switchdev.c          | 171 ++++++++++++++----------
 net/bridge/br_private_mrp.h            |  38 ++++--
 net/dsa/dsa_priv.h                     |  26 ++++
 net/dsa/port.c                         |  48 +++++++
 net/dsa/slave.c                        |  22 ++++
 net/dsa/switch.c                       | 105 +++++++++++++++
 net/dsa/tag_ocelot.c                   |   8 ++
 17 files changed, 715 insertions(+), 104 deletions(-)
 create mode 100644 drivers/net/ethernet/mscc/ocelot_mrp.c

-- 
2.27.0

[PATCH net-next v4 1/8] switchdev: mrp: Remove CONFIG_BRIDGE_MRP

[Horatiu Vultur]

Remove #IS_ENABLED(CONFIG_BRIDGE_MRP) from switchdev.h. This will
simplify the code implements MRP callbacks and will be similar with the
vlan filtering.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 include/net/switchdev.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 9a5426b61ca5..465362d9d063 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -27,9 +27,7 @@ enum switchdev_attr_id {
 	SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL,
 	SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
 	SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
-#if IS_ENABLED(CONFIG_BRIDGE_MRP)
 	SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
-#endif
 };
 
 struct switchdev_brport_flags {
@@ -51,9 +49,7 @@ struct switchdev_attr {
 		bool vlan_filtering;			/* BRIDGE_VLAN_FILTERING */
 		u16 vlan_protocol;			/* BRIDGE_VLAN_PROTOCOL */
 		bool mc_disabled;			/* MC_DISABLED */
-#if IS_ENABLED(CONFIG_BRIDGE_MRP)
 		u8 mrp_port_role;			/* MRP_PORT_ROLE */
-#endif
 	} u;
 };
 
@@ -62,7 +58,6 @@ enum switchdev_obj_id {
 	SWITCHDEV_OBJ_ID_PORT_VLAN,
 	SWITCHDEV_OBJ_ID_PORT_MDB,
 	SWITCHDEV_OBJ_ID_HOST_MDB,
-#if IS_ENABLED(CONFIG_BRIDGE_MRP)
 	SWITCHDEV_OBJ_ID_MRP,
 	SWITCHDEV_OBJ_ID_RING_TEST_MRP,
 	SWITCHDEV_OBJ_ID_RING_ROLE_MRP,
@@ -70,8 +65,6 @@ enum switchdev_obj_id {
 	SWITCHDEV_OBJ_ID_IN_TEST_MRP,
 	SWITCHDEV_OBJ_ID_IN_ROLE_MRP,
 	SWITCHDEV_OBJ_ID_IN_STATE_MRP,
-
-#endif
 };
 
 struct switchdev_obj {
@@ -103,7 +96,6 @@ struct switchdev_obj_port_mdb {
 	container_of((OBJ), struct switchdev_obj_port_mdb, obj)
 
 
-#if IS_ENABLED(CONFIG_BRIDGE_MRP)
 /* SWITCHDEV_OBJ_ID_MRP */
 struct switchdev_obj_mrp {
 	struct switchdev_obj obj;
@@ -183,8 +175,6 @@ struct switchdev_obj_in_state_mrp {
 #define SWITCHDEV_OBJ_IN_STATE_MRP(OBJ) \
 	container_of((OBJ), struct switchdev_obj_in_state_mrp, obj)
 
-#endif
-
 typedef int switchdev_obj_dump_cb_t(struct switchdev_obj *obj);
 
 enum switchdev_notifier_type {
-- 
2.27.0

[PATCH net-next v4 2/8] switchdev: mrp: Extend ring_role_mrp and in_role_mrp

[Horatiu Vultur]

Add the member sw_backup to the structures switchdev_obj_ring_role_mrp
and switchdev_obj_in_role_mrp. In this way the SW can call the driver in
2 ways, once when sw_backup is set to false, meaning that the driver
should implement this completely in HW. And if that is not supported the
SW will call again but with sw_backup set to true, meaning that the
HW should help or allow the SW to run the protocol.

For example when role is MRM, if the HW can't detect when it stops
receiving MRP Test frames but it can trap these frames to CPU, then it
needs to return -EOPNOTSUPP when sw_backup is false and return 0 when
sw_backup is true.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 include/net/switchdev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 465362d9d063..b7fc7d0f54e2 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -127,6 +127,7 @@ struct switchdev_obj_ring_role_mrp {
 	struct switchdev_obj obj;
 	u8 ring_role;
 	u32 ring_id;
+	u8 sw_backup;
 };
 
 #define SWITCHDEV_OBJ_RING_ROLE_MRP(OBJ) \
@@ -161,6 +162,7 @@ struct switchdev_obj_in_role_mrp {
 	u32 ring_id;
 	u16 in_id;
 	u8 in_role;
+	u8 sw_backup;
 };
 
 #define SWITCHDEV_OBJ_IN_ROLE_MRP(OBJ) \
-- 
2.27.0

[PATCH net-next v4 3/8] bridge: mrp: Add 'enum br_mrp_hw_support'

[Horatiu Vultur]

Add the enum br_mrp_hw_support that is used by the br_mrp_switchdev
functions to allow the SW to detect the cases where HW can't implement
the functionality or when SW is used as a backup.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_private_mrp.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/net/bridge/br_private_mrp.h b/net/bridge/br_private_mrp.h
index 2514954c1431..966444304c38 100644
--- a/net/bridge/br_private_mrp.h
+++ b/net/bridge/br_private_mrp.h
@@ -46,6 +46,20 @@ struct br_mrp {
 	struct rcu_head			rcu;
 };
 
+/* This type is returned by br_mrp_switchdev functions that allow to have a SW
+ * backup in case the HW can't implement completely the protocol.
+ * BR_MRP_NONE - means the HW can't run at all the protocol, so the SW stops
+ *               configuring the node anymore.
+ * BR_MRP_SW - the HW can help the SW to run the protocol, by redirecting MRP
+ *             frames to CPU.
+ * BR_MRP_HW - the HW can implement completely the protocol.
+ */
+enum br_mrp_hw_support {
+	BR_MRP_NONE,
+	BR_MRP_SW,
+	BR_MRP_HW,
+};
+
 /* br_mrp.c */
 int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance);
 int br_mrp_del(struct net_bridge *br, struct br_mrp_instance *instance);
-- 
2.27.0

[PATCH net-next v4 4/8] bridge: mrp: Extend br_mrp_switchdev to detect better the errors

[Horatiu Vultur]

This patch extends the br_mrp_switchdev functions to be able to have a
better understanding what cause the issue and if the SW needs to be used
as a backup.

There are the following cases:
- when the code is compiled without CONFIG_NET_SWITCHDEV. In this case
  return success so the SW can continue with the protocol. Depending
  on the function, it returns 0 or BR_MRP_SW.
- when code is compiled with CONFIG_NET_SWITCHDEV and the driver doesn't
  implement any MRP callbacks. In this case the HW can't run MRP so it
  just returns -EOPNOTSUPP. So the SW will stop further to configure the
  node.
- when code is compiled with CONFIG_NET_SWITCHDEV and the driver fully
  supports any MRP functionality. In this case the SW doesn't need to do
  anything. The functions will return 0 or BR_MRP_HW.
- when code is compiled with CONFIG_NET_SWITCHDEV and the HW can't run
  completely the protocol but it can help the SW to run it. For
  example, the HW can't support completely MRM role(can't detect when it
  stops receiving MRP Test frames) but it can redirect these frames to
  CPU. In this case it is possible to have a SW fallback. The SW will
  try initially to call the driver with sw_backup set to false, meaning
  that the HW should implement completely the role. If the driver returns
  -EOPNOTSUPP, the SW will try again with sw_backup set to false,
  meaning that the SW will detect when it stops receiving the frames but
  it needs HW support to redirect the frames to CPU. In case the driver
  returns 0 then the SW will continue to configure the node accordingly.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_mrp_switchdev.c | 171 +++++++++++++++++++++-------------
 net/bridge/br_private_mrp.h   |  24 +++--
 2 files changed, 118 insertions(+), 77 deletions(-)

diff --git a/net/bridge/br_mrp_switchdev.c b/net/bridge/br_mrp_switchdev.c
index 3c9a4abcf4ee..cb54b324fa8c 100644
--- a/net/bridge/br_mrp_switchdev.c
+++ b/net/bridge/br_mrp_switchdev.c
@@ -4,6 +4,30 @@
 
 #include "br_private_mrp.h"
 
+static enum br_mrp_hw_support
+br_mrp_switchdev_port_obj(struct net_bridge *br,
+			  const struct switchdev_obj *obj, bool add)
+{
+	int err;
+
+	if (add)
+		err = switchdev_port_obj_add(br->dev, obj, NULL);
+	else
+		err = switchdev_port_obj_del(br->dev, obj);
+
+	/* In case of success just return and notify the SW that doesn't need
+	 * to do anything
+	 */
+	if (!err)
+		return BR_MRP_HW;
+
+	if (err != -EOPNOTSUPP)
+		return BR_MRP_NONE;
+
+	/* Continue with SW backup */
+	return BR_MRP_SW;
+}
+
 int br_mrp_switchdev_add(struct net_bridge *br, struct br_mrp *mrp)
 {
 	struct switchdev_obj_mrp mrp_obj = {
@@ -14,14 +38,11 @@ int br_mrp_switchdev_add(struct net_bridge *br, struct br_mrp *mrp)
 		.ring_id = mrp->ring_id,
 		.prio = mrp->prio,
 	};
-	int err;
 
-	err = switchdev_port_obj_add(br->dev, &mrp_obj.obj, NULL);
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return 0;
 
-	if (err && err != -EOPNOTSUPP)
-		return err;
-
-	return 0;
+	return switchdev_port_obj_add(br->dev, &mrp_obj.obj, NULL);
 }
 
 int br_mrp_switchdev_del(struct net_bridge *br, struct br_mrp *mrp)
@@ -33,40 +54,54 @@ int br_mrp_switchdev_del(struct net_bridge *br, struct br_mrp *mrp)
 		.s_port = NULL,
 		.ring_id = mrp->ring_id,
 	};
-	int err;
-
-	err = switchdev_port_obj_del(br->dev, &mrp_obj.obj);
 
-	if (err && err != -EOPNOTSUPP)
-		return err;
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return 0;
 
-	return 0;
+	return switchdev_port_obj_del(br->dev, &mrp_obj.obj);
 }
 
-int br_mrp_switchdev_set_ring_role(struct net_bridge *br,
-				   struct br_mrp *mrp,
-				   enum br_mrp_ring_role_type role)
+enum br_mrp_hw_support
+br_mrp_switchdev_set_ring_role(struct net_bridge *br, struct br_mrp *mrp,
+			       enum br_mrp_ring_role_type role)
 {
 	struct switchdev_obj_ring_role_mrp mrp_role = {
 		.obj.orig_dev = br->dev,
 		.obj.id = SWITCHDEV_OBJ_ID_RING_ROLE_MRP,
 		.ring_role = role,
 		.ring_id = mrp->ring_id,
+		.sw_backup = false,
 	};
+	enum br_mrp_hw_support support;
 	int err;
 
-	if (role == BR_MRP_RING_ROLE_DISABLED)
-		err = switchdev_port_obj_del(br->dev, &mrp_role.obj);
-	else
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return BR_MRP_SW;
+
+	support = br_mrp_switchdev_port_obj(br, &mrp_role.obj,
+					    role != BR_MRP_RING_ROLE_DISABLED);
+	if (support != BR_MRP_SW)
+		return support;
+
+	/* If the driver can't configure to run completely the protocol in HW,
+	 * then try again to configure the HW so the SW can run the protocol.
+	 */
+	mrp_role.sw_backup = true;
+	if (role != BR_MRP_RING_ROLE_DISABLED)
 		err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);
+	else
+		err = switchdev_port_obj_del(br->dev, &mrp_role.obj);
 
-	return err;
+	if (!err)
+		return BR_MRP_SW;
+
+	return BR_MRP_NONE;
 }
 
-int br_mrp_switchdev_send_ring_test(struct net_bridge *br,
-				    struct br_mrp *mrp, u32 interval,
-				    u8 max_miss, u32 period,
-				    bool monitor)
+enum br_mrp_hw_support
+br_mrp_switchdev_send_ring_test(struct net_bridge *br, struct br_mrp *mrp,
+				u32 interval, u8 max_miss, u32 period,
+				bool monitor)
 {
 	struct switchdev_obj_ring_test_mrp test = {
 		.obj.orig_dev = br->dev,
@@ -77,14 +112,11 @@ int br_mrp_switchdev_send_ring_test(struct net_bridge *br,
 		.period = period,
 		.monitor = monitor,
 	};
-	int err;
 
-	if (interval == 0)
-		err = switchdev_port_obj_del(br->dev, &test.obj);
-	else
-		err = switchdev_port_obj_add(br->dev, &test.obj, NULL);
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return BR_MRP_SW;
 
-	return err;
+	return br_mrp_switchdev_port_obj(br, &test.obj, interval != 0);
 }
 
 int br_mrp_switchdev_set_ring_state(struct net_bridge *br,
@@ -97,19 +129,17 @@ int br_mrp_switchdev_set_ring_state(struct net_bridge *br,
 		.ring_state = state,
 		.ring_id = mrp->ring_id,
 	};
-	int err;
-
-	err = switchdev_port_obj_add(br->dev, &mrp_state.obj, NULL);
 
-	if (err && err != -EOPNOTSUPP)
-		return err;
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return 0;
 
-	return 0;
+	return switchdev_port_obj_add(br->dev, &mrp_state.obj, NULL);
 }
 
-int br_mrp_switchdev_set_in_role(struct net_bridge *br, struct br_mrp *mrp,
-				 u16 in_id, u32 ring_id,
-				 enum br_mrp_in_role_type role)
+enum br_mrp_hw_support
+br_mrp_switchdev_set_in_role(struct net_bridge *br, struct br_mrp *mrp,
+			     u16 in_id, u32 ring_id,
+			     enum br_mrp_in_role_type role)
 {
 	struct switchdev_obj_in_role_mrp mrp_role = {
 		.obj.orig_dev = br->dev,
@@ -118,15 +148,32 @@ int br_mrp_switchdev_set_in_role(struct net_bridge *br, struct br_mrp *mrp,
 		.in_id = mrp->in_id,
 		.ring_id = mrp->ring_id,
 		.i_port = rtnl_dereference(mrp->i_port)->dev,
+		.sw_backup = false,
 	};
+	enum br_mrp_hw_support support;
 	int err;
 
-	if (role == BR_MRP_IN_ROLE_DISABLED)
-		err = switchdev_port_obj_del(br->dev, &mrp_role.obj);
-	else
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return BR_MRP_SW;
+
+	support = br_mrp_switchdev_port_obj(br, &mrp_role.obj,
+					    role != BR_MRP_IN_ROLE_DISABLED);
+	if (support != BR_MRP_NONE)
+		return support;
+
+	/* If the driver can't configure to run completely the protocol in HW,
+	 * then try again to configure the HW so the SW can run the protocol.
+	 */
+	mrp_role.sw_backup = true;
+	if (role != BR_MRP_IN_ROLE_DISABLED)
 		err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);
+	else
+		err = switchdev_port_obj_del(br->dev, &mrp_role.obj);
+
+	if (!err)
+		return BR_MRP_SW;
 
-	return err;
+	return BR_MRP_NONE;
 }
 
 int br_mrp_switchdev_set_in_state(struct net_bridge *br, struct br_mrp *mrp,
@@ -138,18 +185,16 @@ int br_mrp_switchdev_set_in_state(struct net_bridge *br, struct br_mrp *mrp,
 		.in_state = state,
 		.in_id = mrp->in_id,
 	};
-	int err;
-
-	err = switchdev_port_obj_add(br->dev, &mrp_state.obj, NULL);
 
-	if (err && err != -EOPNOTSUPP)
-		return err;
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return 0;
 
-	return 0;
+	return switchdev_port_obj_add(br->dev, &mrp_state.obj, NULL);
 }
 
-int br_mrp_switchdev_send_in_test(struct net_bridge *br, struct br_mrp *mrp,
-				  u32 interval, u8 max_miss, u32 period)
+enum br_mrp_hw_support
+br_mrp_switchdev_send_in_test(struct net_bridge *br, struct br_mrp *mrp,
+			      u32 interval, u8 max_miss, u32 period)
 {
 	struct switchdev_obj_in_test_mrp test = {
 		.obj.orig_dev = br->dev,
@@ -159,14 +204,11 @@ int br_mrp_switchdev_send_in_test(struct net_bridge *br, struct br_mrp *mrp,
 		.in_id = mrp->in_id,
 		.period = period,
 	};
-	int err;
 
-	if (interval == 0)
-		err = switchdev_port_obj_del(br->dev, &test.obj);
-	else
-		err = switchdev_port_obj_add(br->dev, &test.obj, NULL);
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return BR_MRP_SW;
 
-	return err;
+	return br_mrp_switchdev_port_obj(br, &test.obj, interval != 0);
 }
 
 int br_mrp_port_switchdev_set_state(struct net_bridge_port *p, u32 state)
@@ -176,14 +218,11 @@ int br_mrp_port_switchdev_set_state(struct net_bridge_port *p, u32 state)
 		.id = SWITCHDEV_ATTR_ID_PORT_STP_STATE,
 		.u.stp_state = state,
 	};
-	int err;
 
-	err = switchdev_port_attr_set(p->dev, &attr, NULL);
-	if (err && err != -EOPNOTSUPP)
-		br_warn(p->br, "error setting offload MRP state on port %u(%s)\n",
-			(unsigned int)p->port_no, p->dev->name);
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return 0;
 
-	return err;
+	return switchdev_port_attr_set(p->dev, &attr, NULL);
 }
 
 int br_mrp_port_switchdev_set_role(struct net_bridge_port *p,
@@ -194,11 +233,9 @@ int br_mrp_port_switchdev_set_role(struct net_bridge_port *p,
 		.id = SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
 		.u.mrp_port_role = role,
 	};
-	int err;
 
-	err = switchdev_port_attr_set(p->dev, &attr, NULL);
-	if (err && err != -EOPNOTSUPP)
-		return err;
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return 0;
 
-	return 0;
+	return switchdev_port_attr_set(p->dev, &attr, NULL);
 }
diff --git a/net/bridge/br_private_mrp.h b/net/bridge/br_private_mrp.h
index 966444304c38..9559aa2750fb 100644
--- a/net/bridge/br_private_mrp.h
+++ b/net/bridge/br_private_mrp.h
@@ -79,23 +79,27 @@ int br_mrp_start_in_test(struct net_bridge *br,
 /* br_mrp_switchdev.c */
 int br_mrp_switchdev_add(struct net_bridge *br, struct br_mrp *mrp);
 int br_mrp_switchdev_del(struct net_bridge *br, struct br_mrp *mrp);
-int br_mrp_switchdev_set_ring_role(struct net_bridge *br, struct br_mrp *mrp,
-				   enum br_mrp_ring_role_type role);
+enum br_mrp_hw_support
+br_mrp_switchdev_set_ring_role(struct net_bridge *br, struct br_mrp *mrp,
+			       enum br_mrp_ring_role_type role);
 int br_mrp_switchdev_set_ring_state(struct net_bridge *br, struct br_mrp *mrp,
 				    enum br_mrp_ring_state_type state);
-int br_mrp_switchdev_send_ring_test(struct net_bridge *br, struct br_mrp *mrp,
-				    u32 interval, u8 max_miss, u32 period,
-				    bool monitor);
+enum br_mrp_hw_support
+br_mrp_switchdev_send_ring_test(struct net_bridge *br, struct br_mrp *mrp,
+				u32 interval, u8 max_miss, u32 period,
+				bool monitor);
 int br_mrp_port_switchdev_set_state(struct net_bridge_port *p, u32 state);
 int br_mrp_port_switchdev_set_role(struct net_bridge_port *p,
 				   enum br_mrp_port_role_type role);
-int br_mrp_switchdev_set_in_role(struct net_bridge *br, struct br_mrp *mrp,
-				 u16 in_id, u32 ring_id,
-				 enum br_mrp_in_role_type role);
+enum br_mrp_hw_support
+br_mrp_switchdev_set_in_role(struct net_bridge *br, struct br_mrp *mrp,
+			     u16 in_id, u32 ring_id,
+			     enum br_mrp_in_role_type role);
 int br_mrp_switchdev_set_in_state(struct net_bridge *br, struct br_mrp *mrp,
 				  enum br_mrp_in_state_type state);
-int br_mrp_switchdev_send_in_test(struct net_bridge *br, struct br_mrp *mrp,
-				  u32 interval, u8 max_miss, u32 period);
+enum br_mrp_hw_support
+br_mrp_switchdev_send_in_test(struct net_bridge *br, struct br_mrp *mrp,
+			      u32 interval, u8 max_miss, u32 period);
 
 /* br_mrp_netlink.c  */
 int br_mrp_ring_port_open(struct net_device *dev, u8 loc);
-- 
2.27.0

[PATCH net-next v4 5/8] bridge: mrp: Update br_mrp to use new return values of br_mrp_switchdev

[Horatiu Vultur]

Check the return values of the br_mrp_switchdev function.
In case of:
- BR_MRP_NONE, return the error to userspace,
- BR_MRP_SW, continue with SW implementation,
- BR_MRP_HW, continue without SW implementation,

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_mrp.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
index 01c67ed727a9..12487f6fe9b4 100644
--- a/net/bridge/br_mrp.c
+++ b/net/bridge/br_mrp.c
@@ -639,7 +639,7 @@ int br_mrp_set_ring_role(struct net_bridge *br,
 			 struct br_mrp_ring_role *role)
 {
 	struct br_mrp *mrp = br_mrp_find_id(br, role->ring_id);
-	int err;
+	enum br_mrp_hw_support support;
 
 	if (!mrp)
 		return -EINVAL;
@@ -647,9 +647,9 @@ int br_mrp_set_ring_role(struct net_bridge *br,
 	mrp->ring_role = role->ring_role;
 
 	/* If there is an error just bailed out */
-	err = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role);
-	if (err && err != -EOPNOTSUPP)
-		return err;
+	support = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role);
+	if (support == BR_MRP_NONE)
+		return -EOPNOTSUPP;
 
 	/* Now detect if the HW actually applied the role or not. If the HW
 	 * applied the role it means that the SW will not to do those operations
@@ -657,7 +657,7 @@ int br_mrp_set_ring_role(struct net_bridge *br,
 	 * SW when ring is open, but if the is not pushed to the HW the SW will
 	 * need to detect when the ring is open
 	 */
-	mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
+	mrp->ring_role_offloaded = support == BR_MRP_SW ? 0 : 1;
 
 	return 0;
 }
@@ -670,6 +670,7 @@ int br_mrp_start_test(struct net_bridge *br,
 		      struct br_mrp_start_test *test)
 {
 	struct br_mrp *mrp = br_mrp_find_id(br, test->ring_id);
+	enum br_mrp_hw_support support;
 
 	if (!mrp)
 		return -EINVAL;
@@ -677,9 +678,13 @@ int br_mrp_start_test(struct net_bridge *br,
 	/* Try to push it to the HW and if it fails then continue with SW
 	 * implementation and if that also fails then return error.
 	 */
-	if (!br_mrp_switchdev_send_ring_test(br, mrp, test->interval,
-					     test->max_miss, test->period,
-					     test->monitor))
+	support = br_mrp_switchdev_send_ring_test(br, mrp, test->interval,
+						  test->max_miss, test->period,
+						  test->monitor);
+	if (support == BR_MRP_NONE)
+		return -EOPNOTSUPP;
+
+	if (support == BR_MRP_HW)
 		return 0;
 
 	mrp->test_interval = test->interval;
@@ -721,8 +726,8 @@ int br_mrp_set_in_state(struct net_bridge *br, struct br_mrp_in_state *state)
 int br_mrp_set_in_role(struct net_bridge *br, struct br_mrp_in_role *role)
 {
 	struct br_mrp *mrp = br_mrp_find_id(br, role->ring_id);
+	enum br_mrp_hw_support support;
 	struct net_bridge_port *p;
-	int err;
 
 	if (!mrp)
 		return -EINVAL;
@@ -780,10 +785,10 @@ int br_mrp_set_in_role(struct net_bridge *br, struct br_mrp_in_role *role)
 	mrp->in_id = role->in_id;
 
 	/* If there is an error just bailed out */
-	err = br_mrp_switchdev_set_in_role(br, mrp, role->in_id,
-					   role->ring_id, role->in_role);
-	if (err && err != -EOPNOTSUPP)
-		return err;
+	support = br_mrp_switchdev_set_in_role(br, mrp, role->in_id,
+					       role->ring_id, role->in_role);
+	if (support == BR_MRP_NONE)
+		return -EOPNOTSUPP;
 
 	/* Now detect if the HW actually applied the role or not. If the HW
 	 * applied the role it means that the SW will not to do those operations
@@ -791,7 +796,7 @@ int br_mrp_set_in_role(struct net_bridge *br, struct br_mrp_in_role *role)
 	 * SW when interconnect ring is open, but if the is not pushed to the HW
 	 * the SW will need to detect when the interconnect ring is open.
 	 */
-	mrp->in_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
+	mrp->in_role_offloaded = support == BR_MRP_SW ? 0 : 1;
 
 	return 0;
 }
@@ -804,6 +809,7 @@ int br_mrp_start_in_test(struct net_bridge *br,
 			 struct br_mrp_start_in_test *in_test)
 {
 	struct br_mrp *mrp = br_mrp_find_in_id(br, in_test->in_id);
+	enum br_mrp_hw_support support;
 
 	if (!mrp)
 		return -EINVAL;
@@ -814,8 +820,13 @@ int br_mrp_start_in_test(struct net_bridge *br,
 	/* Try to push it to the HW and if it fails then continue with SW
 	 * implementation and if that also fails then return error.
 	 */
-	if (!br_mrp_switchdev_send_in_test(br, mrp, in_test->interval,
-					   in_test->max_miss, in_test->period))
+	support =  br_mrp_switchdev_send_in_test(br, mrp, in_test->interval,
+						 in_test->max_miss,
+						 in_test->period);
+	if (support == BR_MRP_NONE)
+		return -EOPNOTSUPP;
+
+	if (support == BR_MRP_HW)
 		return 0;
 
 	mrp->in_test_interval = in_test->interval;
-- 
2.27.0

[PATCH net-next v4 6/8] net: mscc: ocelot: Add support for MRP

[Horatiu Vultur]

Add basic support for MRP. The HW will just trap all MRP frames on the
ring ports to CPU and allow the SW to process them. In this way it is
possible to for this node to behave both as MRM and MRC.

Current limitations are:
- it doesn't support Interconnect roles.
- it supports only a single ring.
- the HW should be able to do forwarding of MRP Test frames so the SW
  will not need to do this. So it would be able to have the role MRC
  without SW support.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/ethernet/mscc/Makefile     |   1 +
 drivers/net/ethernet/mscc/ocelot.c     |  10 +-
 drivers/net/ethernet/mscc/ocelot_mrp.c | 175 +++++++++++++++++++++++++
 drivers/net/ethernet/mscc/ocelot_net.c |  60 +++++++++
 include/linux/dsa/ocelot.h             |   5 +
 include/soc/mscc/ocelot.h              |  45 +++++++
 6 files changed, 295 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/mscc/ocelot_mrp.c

diff --git a/drivers/net/ethernet/mscc/Makefile b/drivers/net/ethernet/mscc/Makefile
index 346bba2730ad..722c27694b21 100644
--- a/drivers/net/ethernet/mscc/Makefile
+++ b/drivers/net/ethernet/mscc/Makefile
@@ -8,6 +8,7 @@ mscc_ocelot_switch_lib-y := \
 	ocelot_flower.o \
 	ocelot_ptp.o \
 	ocelot_devlink.o
+mscc_ocelot_switch_lib-$(CONFIG_BRIDGE_MRP) += ocelot_mrp.o
 obj-$(CONFIG_MSCC_OCELOT_SWITCH) += mscc_ocelot.o
 mscc_ocelot-y := \
 	ocelot_vsc7514.o \
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 5d13087c85d6..46e5c9136bac 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -687,7 +687,7 @@ static int ocelot_xtr_poll_xfh(struct ocelot *ocelot, int grp, u32 *xfh)
 int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, struct sk_buff **nskb)
 {
 	struct skb_shared_hwtstamps *shhwtstamps;
-	u64 tod_in_ns, full_ts_in_ns;
+	u64 tod_in_ns, full_ts_in_ns, cpuq;
 	u64 timestamp, src_port, len;
 	u32 xfh[OCELOT_TAG_LEN / 4];
 	struct net_device *dev;
@@ -704,6 +704,7 @@ int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, struct sk_buff **nskb)
 	ocelot_xfh_get_src_port(xfh, &src_port);
 	ocelot_xfh_get_len(xfh, &len);
 	ocelot_xfh_get_rew_val(xfh, &timestamp);
+	ocelot_xfh_get_cpuq(xfh, &cpuq);
 
 	if (WARN_ON(src_port >= ocelot->num_phys_ports))
 		return -EINVAL;
@@ -770,6 +771,13 @@ int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, struct sk_buff **nskb)
 		skb->offload_fwd_mark = 1;
 
 	skb->protocol = eth_type_trans(skb, dev);
+
+#if IS_ENABLED(CONFIG_BRIDGE_MRP)
+	if (skb->protocol == cpu_to_be16(ETH_P_MRP) &&
+	    cpuq & BIT(OCELOT_MRP_CPUQ))
+		skb->offload_fwd_mark = 0;
+#endif
+
 	*nskb = skb;
 
 	return 0;
diff --git a/drivers/net/ethernet/mscc/ocelot_mrp.c b/drivers/net/ethernet/mscc/ocelot_mrp.c
new file mode 100644
index 000000000000..683da320bfd8
--- /dev/null
+++ b/drivers/net/ethernet/mscc/ocelot_mrp.c
@@ -0,0 +1,175 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/* Microsemi Ocelot Switch driver
+ *
+ * This contains glue logic between the switchdev driver operations and the
+ * mscc_ocelot_switch_lib.
+ *
+ * Copyright (c) 2017, 2019 Microsemi Corporation
+ * Copyright 2020-2021 NXP Semiconductors
+ */
+
+#include <linux/if_bridge.h>
+#include <linux/mrp_bridge.h>
+#include <soc/mscc/ocelot_vcap.h>
+#include <uapi/linux/mrp_bridge.h>
+#include "ocelot.h"
+#include "ocelot_vcap.h"
+
+static int ocelot_mrp_del_vcap(struct ocelot *ocelot, int port)
+{
+	struct ocelot_vcap_block *block_vcap_is2;
+	struct ocelot_vcap_filter *filter;
+
+	block_vcap_is2 = &ocelot->block[VCAP_IS2];
+	filter = ocelot_vcap_block_find_filter_by_id(block_vcap_is2, port,
+						     false);
+	if (!filter)
+		return 0;
+
+	return ocelot_vcap_filter_del(ocelot, filter);
+}
+
+int ocelot_mrp_add(struct ocelot *ocelot, int port,
+		   const struct switchdev_obj_mrp *mrp)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	struct ocelot_port_private *priv;
+	struct net_device *dev;
+
+	if (!ocelot_port)
+		return -EOPNOTSUPP;
+
+	priv = container_of(ocelot_port, struct ocelot_port_private, port);
+	dev = priv->dev;
+
+	if (mrp->p_port != dev && mrp->s_port != dev)
+		return 0;
+
+	if (ocelot->mrp_ring_id != 0 &&
+	    ocelot->mrp_s_port &&
+	    ocelot->mrp_p_port)
+		return -EINVAL;
+
+	if (mrp->p_port == dev)
+		ocelot->mrp_p_port = dev;
+
+	if (mrp->s_port == dev)
+		ocelot->mrp_s_port = dev;
+
+	ocelot->mrp_ring_id = mrp->ring_id;
+
+	return 0;
+}
+EXPORT_SYMBOL(ocelot_mrp_add);
+
+int ocelot_mrp_del(struct ocelot *ocelot, int port,
+		   const struct switchdev_obj_mrp *mrp)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	struct ocelot_port_private *priv;
+	struct net_device *dev;
+
+	if (!ocelot_port)
+		return -EOPNOTSUPP;
+
+	priv = container_of(ocelot_port, struct ocelot_port_private, port);
+	dev = priv->dev;
+
+	if (ocelot->mrp_p_port != dev && ocelot->mrp_s_port != dev)
+		return 0;
+
+	if (ocelot->mrp_ring_id == 0 &&
+	    !ocelot->mrp_s_port &&
+	    !ocelot->mrp_p_port)
+		return -EINVAL;
+
+	if (ocelot_mrp_del_vcap(ocelot, priv->chip_port))
+		return -EINVAL;
+
+	if (ocelot->mrp_p_port == dev)
+		ocelot->mrp_p_port = NULL;
+
+	if (ocelot->mrp_s_port == dev)
+		ocelot->mrp_s_port = NULL;
+
+	ocelot->mrp_ring_id = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL(ocelot_mrp_del);
+
+int ocelot_mrp_add_ring_role(struct ocelot *ocelot, int port,
+			     const struct switchdev_obj_ring_role_mrp *mrp)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	struct ocelot_vcap_filter *filter;
+	struct ocelot_port_private *priv;
+	struct net_device *dev;
+	int err;
+
+	if (!ocelot_port)
+		return -EOPNOTSUPP;
+
+	priv = container_of(ocelot_port, struct ocelot_port_private, port);
+	dev = priv->dev;
+
+	if (ocelot->mrp_ring_id != mrp->ring_id)
+		return -EINVAL;
+
+	if (!mrp->sw_backup)
+		return -EOPNOTSUPP;
+
+	if (ocelot->mrp_p_port != dev && ocelot->mrp_s_port != dev)
+		return 0;
+
+	filter = kzalloc(sizeof(*filter), GFP_ATOMIC);
+	if (!filter)
+		return -ENOMEM;
+
+	filter->key_type = OCELOT_VCAP_KEY_ETYPE;
+	filter->prio = 1;
+	filter->id.cookie = priv->chip_port;
+	filter->id.tc_offload = false;
+	filter->block_id = VCAP_IS2;
+	filter->type = OCELOT_VCAP_FILTER_OFFLOAD;
+	filter->ingress_port_mask = BIT(priv->chip_port);
+	*(__be16 *)filter->key.etype.etype.value = htons(ETH_P_MRP);
+	*(__be16 *)filter->key.etype.etype.mask = htons(0xffff);
+	filter->action.mask_mode = OCELOT_MASK_MODE_PERMIT_DENY;
+	filter->action.port_mask = 0x0;
+	filter->action.cpu_copy_ena = true;
+	filter->action.cpu_qu_num = OCELOT_MRP_CPUQ;
+
+	err = ocelot_vcap_filter_add(ocelot, filter, NULL);
+	if (err)
+		kfree(filter);
+
+	return err;
+}
+EXPORT_SYMBOL(ocelot_mrp_add_ring_role);
+
+int ocelot_mrp_del_ring_role(struct ocelot *ocelot, int port,
+			     const struct switchdev_obj_ring_role_mrp *mrp)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	struct ocelot_port_private *priv;
+	struct net_device *dev;
+
+	if (!ocelot_port)
+		return -EOPNOTSUPP;
+
+	priv = container_of(ocelot_port, struct ocelot_port_private, port);
+	dev = priv->dev;
+
+	if (ocelot->mrp_ring_id != mrp->ring_id)
+		return -EINVAL;
+
+	if (!mrp->sw_backup)
+		return -EOPNOTSUPP;
+
+	if (ocelot->mrp_p_port != dev && ocelot->mrp_s_port != dev)
+		return 0;
+
+	return ocelot_mrp_del_vcap(ocelot, priv->chip_port);
+}
+EXPORT_SYMBOL(ocelot_mrp_del_ring_role);
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 6518262532f0..12cb6867a2d0 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -1010,6 +1010,52 @@ static int ocelot_port_obj_del_mdb(struct net_device *dev,
 	return ocelot_port_mdb_del(ocelot, port, mdb);
 }
 
+static int ocelot_port_obj_mrp_add(struct net_device *dev,
+				   const struct switchdev_obj_mrp *mrp)
+{
+	struct ocelot_port_private *priv = netdev_priv(dev);
+	struct ocelot_port *ocelot_port = &priv->port;
+	struct ocelot *ocelot = ocelot_port->ocelot;
+	int port = priv->chip_port;
+
+	return ocelot_mrp_add(ocelot, port, mrp);
+}
+
+static int ocelot_port_obj_mrp_del(struct net_device *dev,
+				   const struct switchdev_obj_mrp *mrp)
+{
+	struct ocelot_port_private *priv = netdev_priv(dev);
+	struct ocelot_port *ocelot_port = &priv->port;
+	struct ocelot *ocelot = ocelot_port->ocelot;
+	int port = priv->chip_port;
+
+	return ocelot_mrp_del(ocelot, port, mrp);
+}
+
+static int
+ocelot_port_obj_mrp_add_ring_role(struct net_device *dev,
+				  const struct switchdev_obj_ring_role_mrp *mrp)
+{
+	struct ocelot_port_private *priv = netdev_priv(dev);
+	struct ocelot_port *ocelot_port = &priv->port;
+	struct ocelot *ocelot = ocelot_port->ocelot;
+	int port = priv->chip_port;
+
+	return ocelot_mrp_add_ring_role(ocelot, port, mrp);
+}
+
+static int
+ocelot_port_obj_mrp_del_ring_role(struct net_device *dev,
+				  const struct switchdev_obj_ring_role_mrp *mrp)
+{
+	struct ocelot_port_private *priv = netdev_priv(dev);
+	struct ocelot_port *ocelot_port = &priv->port;
+	struct ocelot *ocelot = ocelot_port->ocelot;
+	int port = priv->chip_port;
+
+	return ocelot_mrp_del_ring_role(ocelot, port, mrp);
+}
+
 static int ocelot_port_obj_add(struct net_device *dev,
 			       const struct switchdev_obj *obj,
 			       struct netlink_ext_ack *extack)
@@ -1024,6 +1070,13 @@ static int ocelot_port_obj_add(struct net_device *dev,
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		ret = ocelot_port_obj_add_mdb(dev, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
+	case SWITCHDEV_OBJ_ID_MRP:
+		ret = ocelot_port_obj_mrp_add(dev, SWITCHDEV_OBJ_MRP(obj));
+		break;
+	case SWITCHDEV_OBJ_ID_RING_ROLE_MRP:
+		ret = ocelot_port_obj_mrp_add_ring_role(dev,
+							SWITCHDEV_OBJ_RING_ROLE_MRP(obj));
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -1044,6 +1097,13 @@ static int ocelot_port_obj_del(struct net_device *dev,
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		ret = ocelot_port_obj_del_mdb(dev, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
+	case SWITCHDEV_OBJ_ID_MRP:
+		ret = ocelot_port_obj_mrp_del(dev, SWITCHDEV_OBJ_MRP(obj));
+		break;
+	case SWITCHDEV_OBJ_ID_RING_ROLE_MRP:
+		ret = ocelot_port_obj_mrp_del_ring_role(dev,
+							SWITCHDEV_OBJ_RING_ROLE_MRP(obj));
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
diff --git a/include/linux/dsa/ocelot.h b/include/linux/dsa/ocelot.h
index c6bc45ae5e03..4265f328681a 100644
--- a/include/linux/dsa/ocelot.h
+++ b/include/linux/dsa/ocelot.h
@@ -160,6 +160,11 @@ static inline void ocelot_xfh_get_src_port(void *extraction, u64 *src_port)
 	packing(extraction, src_port, 46, 43, OCELOT_TAG_LEN, UNPACK, 0);
 }
 
+static inline void ocelot_xfh_get_cpuq(void *extraction, u64 *cpuq)
+{
+	packing(extraction, cpuq, 28, 20, OCELOT_TAG_LEN, UNPACK, 0);
+}
+
 static inline void ocelot_xfh_get_qos_class(void *extraction, u64 *qos_class)
 {
 	packing(extraction, qos_class, 19, 17, OCELOT_TAG_LEN, UNPACK, 0);
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 1f2d90976564..425ff29d9389 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -112,6 +112,8 @@
 #define REG_RESERVED_ADDR		0xffffffff
 #define REG_RESERVED(reg)		REG(reg, REG_RESERVED_ADDR)
 
+#define OCELOT_MRP_CPUQ			7
+
 enum ocelot_target {
 	ANA = 1,
 	QS,
@@ -677,6 +679,12 @@ struct ocelot {
 	/* Protects the PTP clock */
 	spinlock_t			ptp_clock_lock;
 	struct ptp_pin_desc		ptp_pins[OCELOT_PTP_PINS_NUM];
+
+#if IS_ENABLED(CONFIG_BRIDGE_MRP)
+	u16				mrp_ring_id;
+	struct net_device		*mrp_p_port;
+	struct net_device		*mrp_s_port;
+#endif
 };
 
 struct ocelot_policer {
@@ -874,4 +882,41 @@ int ocelot_sb_occ_tc_port_bind_get(struct ocelot *ocelot, int port,
 				   enum devlink_sb_pool_type pool_type,
 				   u32 *p_cur, u32 *p_max);
 
+#if IS_ENABLED(CONFIG_BRIDGE_MRP)
+int ocelot_mrp_add(struct ocelot *ocelot, int port,
+		   const struct switchdev_obj_mrp *mrp);
+int ocelot_mrp_del(struct ocelot *ocelot, int port,
+		   const struct switchdev_obj_mrp *mrp);
+int ocelot_mrp_add_ring_role(struct ocelot *ocelot, int port,
+			     const struct switchdev_obj_ring_role_mrp *mrp);
+int ocelot_mrp_del_ring_role(struct ocelot *ocelot, int port,
+			     const struct switchdev_obj_ring_role_mrp *mrp);
+#else
+static inline int ocelot_mrp_add(struct ocelot *ocelot, int port,
+				 const struct switchdev_obj_mrp *mrp)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int ocelot_mrp_del(struct ocelot *ocelot, int port,
+				 const struct switchdev_obj_mrp *mrp)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int
+ocelot_mrp_add_ring_role(struct ocelot *ocelot, int port,
+			 const struct switchdev_obj_ring_role_mrp *mrp)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int
+ocelot_mrp_del_ring_role(struct ocelot *ocelot, int port,
+			 const struct switchdev_obj_ring_role_mrp *mrp)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
 #endif
-- 
2.27.0

[PATCH net-next v4 7/8] net: dsa: add MRP support

[Horatiu Vultur]

Add support for offloading MRP in HW. Currently implement the switchdev
calls 'SWITCHDEV_OBJ_ID_MRP', 'SWITCHDEV_OBJ_ID_RING_ROLE_MRP',
to allow to create MRP instances and to set the role of these instances.

Add DSA_NOTIFIER_MRP_ADD/DEL and DSA_NOTIFIER_MRP_ADD/DEL_RING_ROLE
which calls to .port_mrp_add/del and .port_mrp_add/del_ring_role in the
DSA driver for the switch.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 include/net/dsa.h  |  12 ++++++
 net/dsa/dsa_priv.h |  26 +++++++++++
 net/dsa/port.c     |  48 +++++++++++++++++++++
 net/dsa/slave.c    |  22 ++++++++++
 net/dsa/switch.c   | 105 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 213 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 68f8159564a3..83a933e563fe 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -792,6 +792,18 @@ struct dsa_switch_ops {
 				 struct net_device *hsr);
 	int	(*port_hsr_leave)(struct dsa_switch *ds, int port,
 				  struct net_device *hsr);
+
+	/*
+	 * MRP integration
+	 */
+	int	(*port_mrp_add)(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_mrp *mrp);
+	int	(*port_mrp_del)(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_mrp *mrp);
+	int	(*port_mrp_add_ring_role)(struct dsa_switch *ds, int port,
+					  const struct switchdev_obj_ring_role_mrp *mrp);
+	int	(*port_mrp_del_ring_role)(struct dsa_switch *ds, int port,
+					  const struct switchdev_obj_ring_role_mrp *mrp);
 };
 
 #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes)		\
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index e9d1e76c42ba..2eeaa42f2e08 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -31,6 +31,10 @@ enum {
 	DSA_NOTIFIER_VLAN_DEL,
 	DSA_NOTIFIER_MTU,
 	DSA_NOTIFIER_TAG_PROTO,
+	DSA_NOTIFIER_MRP_ADD,
+	DSA_NOTIFIER_MRP_DEL,
+	DSA_NOTIFIER_MRP_ADD_RING_ROLE,
+	DSA_NOTIFIER_MRP_DEL_RING_ROLE,
 };
 
 /* DSA_NOTIFIER_AGEING_TIME */
@@ -91,6 +95,20 @@ struct dsa_notifier_tag_proto_info {
 	const struct dsa_device_ops *tag_ops;
 };
 
+/* DSA_NOTIFIER_MRP_* */
+struct dsa_notifier_mrp_info {
+	const struct switchdev_obj_mrp *mrp;
+	int sw_index;
+	int port;
+};
+
+/* DSA_NOTIFIER_MRP_* */
+struct dsa_notifier_mrp_ring_role_info {
+	const struct switchdev_obj_ring_role_mrp *mrp;
+	int sw_index;
+	int port;
+};
+
 struct dsa_switchdev_event_work {
 	struct dsa_switch *ds;
 	int port;
@@ -198,6 +216,14 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 		      struct netlink_ext_ack *extack);
 int dsa_port_vlan_del(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan);
+int dsa_port_mrp_add(const struct dsa_port *dp,
+		     const struct switchdev_obj_mrp *mrp);
+int dsa_port_mrp_del(const struct dsa_port *dp,
+		     const struct switchdev_obj_mrp *mrp);
+int dsa_port_mrp_add_ring_role(const struct dsa_port *dp,
+			       const struct switchdev_obj_ring_role_mrp *mrp);
+int dsa_port_mrp_del_ring_role(const struct dsa_port *dp,
+			       const struct switchdev_obj_ring_role_mrp *mrp);
 int dsa_port_link_register_of(struct dsa_port *dp);
 void dsa_port_link_unregister_of(struct dsa_port *dp);
 int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 14a1d0d77657..c9c6d7ab3f47 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -564,6 +564,54 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 }
 
+int dsa_port_mrp_add(const struct dsa_port *dp,
+		     const struct switchdev_obj_mrp *mrp)
+{
+	struct dsa_notifier_mrp_info info = {
+		.sw_index = dp->ds->index,
+		.port = dp->index,
+		.mrp = mrp,
+	};
+
+	return dsa_port_notify(dp, DSA_NOTIFIER_MRP_ADD, &info);
+}
+
+int dsa_port_mrp_del(const struct dsa_port *dp,
+		     const struct switchdev_obj_mrp *mrp)
+{
+	struct dsa_notifier_mrp_info info = {
+		.sw_index = dp->ds->index,
+		.port = dp->index,
+		.mrp = mrp,
+	};
+
+	return dsa_port_notify(dp, DSA_NOTIFIER_MRP_DEL, &info);
+}
+
+int dsa_port_mrp_add_ring_role(const struct dsa_port *dp,
+			       const struct switchdev_obj_ring_role_mrp *mrp)
+{
+	struct dsa_notifier_mrp_ring_role_info info = {
+		.sw_index = dp->ds->index,
+		.port = dp->index,
+		.mrp = mrp,
+	};
+
+	return dsa_port_notify(dp, DSA_NOTIFIER_MRP_ADD_RING_ROLE, &info);
+}
+
+int dsa_port_mrp_del_ring_role(const struct dsa_port *dp,
+			       const struct switchdev_obj_ring_role_mrp *mrp)
+{
+	struct dsa_notifier_mrp_ring_role_info info = {
+		.sw_index = dp->ds->index,
+		.port = dp->index,
+		.mrp = mrp,
+	};
+
+	return dsa_port_notify(dp, DSA_NOTIFIER_MRP_DEL_RING_ROLE, &info);
+}
+
 void dsa_port_set_tag_protocol(struct dsa_port *cpu_dp,
 			       const struct dsa_device_ops *tag_ops)
 {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 5ecb43a1b6e0..491e3761b5f4 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -404,6 +404,17 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
 		err = dsa_slave_vlan_add(dev, obj, extack);
 		break;
+	case SWITCHDEV_OBJ_ID_MRP:
+		if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
+			return -EOPNOTSUPP;
+		err = dsa_port_mrp_add(dp, SWITCHDEV_OBJ_MRP(obj));
+		break;
+	case SWITCHDEV_OBJ_ID_RING_ROLE_MRP:
+		if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
+			return -EOPNOTSUPP;
+		err = dsa_port_mrp_add_ring_role(dp,
+						 SWITCHDEV_OBJ_RING_ROLE_MRP(obj));
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
@@ -461,6 +472,17 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
 		err = dsa_slave_vlan_del(dev, obj);
 		break;
+	case SWITCHDEV_OBJ_ID_MRP:
+		if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
+			return -EOPNOTSUPP;
+		err = dsa_port_mrp_del(dp, SWITCHDEV_OBJ_MRP(obj));
+		break;
+	case SWITCHDEV_OBJ_ID_RING_ROLE_MRP:
+		if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
+			return -EOPNOTSUPP;
+		err = dsa_port_mrp_del_ring_role(dp,
+						 SWITCHDEV_OBJ_RING_ROLE_MRP(obj));
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index db2a9b221988..4b5da89dc27a 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -372,6 +372,99 @@ static int dsa_switch_change_tag_proto(struct dsa_switch *ds,
 	return 0;
 }
 
+static bool dsa_switch_mrp_match(struct dsa_switch *ds, int port,
+				 struct dsa_notifier_mrp_info *info)
+{
+	if (ds->index == info->sw_index && port == info->port)
+		return true;
+
+	if (dsa_is_dsa_port(ds, port))
+		return true;
+
+	return false;
+}
+
+static int dsa_switch_mrp_add(struct dsa_switch *ds,
+			      struct dsa_notifier_mrp_info *info)
+{
+	int err = 0;
+	int port;
+
+	if (!ds->ops->port_mrp_add)
+		return -EOPNOTSUPP;
+
+	for (port = 0; port < ds->num_ports; port++) {
+		if (dsa_switch_mrp_match(ds, port, info)) {
+			err = ds->ops->port_mrp_add(ds, port, info->mrp);
+			if (err)
+				break;
+		}
+	}
+
+	return err;
+}
+
+static int dsa_switch_mrp_del(struct dsa_switch *ds,
+			      struct dsa_notifier_mrp_info *info)
+{
+	if (!ds->ops->port_mrp_del)
+		return -EOPNOTSUPP;
+
+	if (ds->index == info->sw_index)
+		return ds->ops->port_mrp_del(ds, info->port, info->mrp);
+
+	return 0;
+}
+
+static bool
+dsa_switch_mrp_ring_role_match(struct dsa_switch *ds, int port,
+			       struct dsa_notifier_mrp_ring_role_info *info)
+{
+	if (ds->index == info->sw_index && port == info->port)
+		return true;
+
+	if (dsa_is_dsa_port(ds, port))
+		return true;
+
+	return false;
+}
+
+static int
+dsa_switch_mrp_add_ring_role(struct dsa_switch *ds,
+			     struct dsa_notifier_mrp_ring_role_info *info)
+{
+	int err = 0;
+	int port;
+
+	if (!ds->ops->port_mrp_add)
+		return -EOPNOTSUPP;
+
+	for (port = 0; port < ds->num_ports; port++) {
+		if (dsa_switch_mrp_ring_role_match(ds, port, info)) {
+			err = ds->ops->port_mrp_add_ring_role(ds, port,
+							      info->mrp);
+			if (err)
+				break;
+		}
+	}
+
+	return err;
+}
+
+static int
+dsa_switch_mrp_del_ring_role(struct dsa_switch *ds,
+			     struct dsa_notifier_mrp_ring_role_info *info)
+{
+	if (!ds->ops->port_mrp_del)
+		return -EOPNOTSUPP;
+
+	if (ds->index == info->sw_index)
+		return ds->ops->port_mrp_del_ring_role(ds, info->port,
+						       info->mrp);
+
+	return 0;
+}
+
 static int dsa_switch_event(struct notifier_block *nb,
 			    unsigned long event, void *info)
 {
@@ -427,6 +520,18 @@ static int dsa_switch_event(struct notifier_block *nb,
 	case DSA_NOTIFIER_TAG_PROTO:
 		err = dsa_switch_change_tag_proto(ds, info);
 		break;
+	case DSA_NOTIFIER_MRP_ADD:
+		err = dsa_switch_mrp_add(ds, info);
+		break;
+	case DSA_NOTIFIER_MRP_DEL:
+		err = dsa_switch_mrp_del(ds, info);
+		break;
+	case DSA_NOTIFIER_MRP_ADD_RING_ROLE:
+		err = dsa_switch_mrp_add_ring_role(ds, info);
+		break;
+	case DSA_NOTIFIER_MRP_DEL_RING_ROLE:
+		err = dsa_switch_mrp_del_ring_role(ds, info);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
-- 
2.27.0

[PATCH net-next v4 8/8] net: dsa: felix: Add support for MRP

[Horatiu Vultur]

Implement functions 'port_mrp_add', 'port_mrp_del',
'port_mrp_add_ring_role' and 'port_mrp_del_ring_role' to call the mrp
functions from ocelot.

Also all MRP frames that arrive to CPU on queue number OCELOT_MRP_CPUQ
will be forward by the SW.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/dsa/ocelot/felix.c | 38 ++++++++++++++++++++++++++++++++++
 net/dsa/tag_ocelot.c           |  8 +++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 800f27d65c6c..fa1c3f14bb88 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1561,6 +1561,40 @@ static int felix_sb_occ_tc_port_bind_get(struct dsa_switch *ds, int port,
 					      pool_type, p_cur, p_max);
 }
 
+static int felix_mrp_add(struct dsa_switch *ds, int port,
+			 const struct switchdev_obj_mrp *mrp)
+{
+	struct ocelot *ocelot = ds->priv;
+
+	return ocelot_mrp_add(ocelot, port, mrp);
+}
+
+static int felix_mrp_del(struct dsa_switch *ds, int port,
+			 const struct switchdev_obj_mrp *mrp)
+{
+	struct ocelot *ocelot = ds->priv;
+
+	return ocelot_mrp_add(ocelot, port, mrp);
+}
+
+static int
+felix_mrp_add_ring_role(struct dsa_switch *ds, int port,
+			const struct switchdev_obj_ring_role_mrp *mrp)
+{
+	struct ocelot *ocelot = ds->priv;
+
+	return ocelot_mrp_add_ring_role(ocelot, port, mrp);
+}
+
+static int
+felix_mrp_del_ring_role(struct dsa_switch *ds, int port,
+			const struct switchdev_obj_ring_role_mrp *mrp)
+{
+	struct ocelot *ocelot = ds->priv;
+
+	return ocelot_mrp_del_ring_role(ocelot, port, mrp);
+}
+
 const struct dsa_switch_ops felix_switch_ops = {
 	.get_tag_protocol		= felix_get_tag_protocol,
 	.change_tag_protocol		= felix_change_tag_protocol,
@@ -1615,6 +1649,10 @@ const struct dsa_switch_ops felix_switch_ops = {
 	.devlink_sb_occ_max_clear	= felix_sb_occ_max_clear,
 	.devlink_sb_occ_port_pool_get	= felix_sb_occ_port_pool_get,
 	.devlink_sb_occ_tc_port_bind_get= felix_sb_occ_tc_port_bind_get,
+	.port_mrp_add			= felix_mrp_add,
+	.port_mrp_del			= felix_mrp_del,
+	.port_mrp_add_ring_role		= felix_mrp_add_ring_role,
+	.port_mrp_del_ring_role		= felix_mrp_del_ring_role,
 };
 
 struct net_device *felix_port_to_netdev(struct ocelot *ocelot, int port)
diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
index f9df9cac81c5..743809b5806b 100644
--- a/net/dsa/tag_ocelot.c
+++ b/net/dsa/tag_ocelot.c
@@ -83,6 +83,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
 	struct dsa_port *dp;
 	u8 *extraction;
 	u16 vlan_tpid;
+	u64 cpuq;
 
 	/* Revert skb->data by the amount consumed by the DSA master,
 	 * so it points to the beginning of the frame.
@@ -112,6 +113,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
 	ocelot_xfh_get_qos_class(extraction, &qos_class);
 	ocelot_xfh_get_tag_type(extraction, &tag_type);
 	ocelot_xfh_get_vlan_tci(extraction, &vlan_tci);
+	ocelot_xfh_get_cpuq(extraction, &cpuq);
 
 	skb->dev = dsa_master_find_slave(netdev, 0, src_port);
 	if (!skb->dev)
@@ -126,6 +128,12 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
 	skb->offload_fwd_mark = 1;
 	skb->priority = qos_class;
 
+#if IS_ENABLED(CONFIG_BRIDGE_MRP)
+	if (eth_hdr(skb)->h_proto == cpu_to_be16(ETH_P_MRP) &&
+	    cpuq & BIT(OCELOT_MRP_CPUQ))
+		skb->offload_fwd_mark = 0;
+#endif
+
 	/* Ocelot switches copy frames unmodified to the CPU. However, it is
 	 * possible for the user to request a VLAN modification through
 	 * VCAP_IS1_ACT_VID_REPLACE_ENA. In this case, what will happen is that
-- 
2.27.0

Re: [PATCH net-next v4 0/8] bridge: mrp: Extend br_mrp_switchdev_*

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Tue, 16 Feb 2021 22:41:57 +0100 you wrote:
> This patch series extends MRP switchdev to allow the SW to have a better
> understanding if the HW can implement the MRP functionality or it needs
> to help the HW to run it. There are 3 cases:
> - when HW can't implement at all the functionality.
> - when HW can implement a part of the functionality but needs the SW
>   implement the rest. For example if it can't detect when it stops
>   receiving MRP Test frames but it can copy the MRP frames to CPU to
>   allow the SW to determine this.  Another example is generating the MRP
>   Test frames. If HW can't do that then the SW is used as backup.
> - when HW can implement completely the functionality.
> 
> [...]

Here is the summary with links:
  - [net-next,v4,1/8] switchdev: mrp: Remove CONFIG_BRIDGE_MRP
    https://git.kernel.org/netdev/net-next/c/405be6b46b70
  - [net-next,v4,2/8] switchdev: mrp: Extend ring_role_mrp and in_role_mrp
    https://git.kernel.org/netdev/net-next/c/c513efa20c52
  - [net-next,v4,3/8] bridge: mrp: Add 'enum br_mrp_hw_support'
    https://git.kernel.org/netdev/net-next/c/e1bd99d07e61
  - [net-next,v4,4/8] bridge: mrp: Extend br_mrp_switchdev to detect better the errors
    https://git.kernel.org/netdev/net-next/c/1a3ddb0b7516
  - [net-next,v4,5/8] bridge: mrp: Update br_mrp to use new return values of br_mrp_switchdev
    https://git.kernel.org/netdev/net-next/c/cd605d455a44
  - [net-next,v4,6/8] net: mscc: ocelot: Add support for MRP
    https://git.kernel.org/netdev/net-next/c/d8ea7ff3995e
  - [net-next,v4,7/8] net: dsa: add MRP support
    https://git.kernel.org/netdev/net-next/c/c595c4330da0
  - [net-next,v4,8/8] net: dsa: felix: Add support for MRP
    https://git.kernel.org/netdev/net-next/c/a026c50b599f

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v4 8/8] net: dsa: felix: Add support for MRP

[Vladimir Oltean]

On Tue, Feb 16, 2021 at 10:42:05PM +0100, Horatiu Vultur wrote:
> @@ -112,6 +113,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
>  	ocelot_xfh_get_qos_class(extraction, &qos_class);
>  	ocelot_xfh_get_tag_type(extraction, &tag_type);
>  	ocelot_xfh_get_vlan_tci(extraction, &vlan_tci);
> +	ocelot_xfh_get_cpuq(extraction, &cpuq);
>  
>  	skb->dev = dsa_master_find_slave(netdev, 0, src_port);
>  	if (!skb->dev)
> @@ -126,6 +128,12 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
>  	skb->offload_fwd_mark = 1;
>  	skb->priority = qos_class;
>  
> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> +	if (eth_hdr(skb)->h_proto == cpu_to_be16(ETH_P_MRP) &&
> +	    cpuq & BIT(OCELOT_MRP_CPUQ))

Checking the EtherType seems redundant, since those are the only frames
trapped to the MRP CPU queue.

Also, the cpuq variable is potentially unused when CONFIG_BRIDGE_MRP is
unset. I'm concerned that static analysis people may come in and try to
fix it up with even more ifdeffery, which is definitely not what I would
like to go for.

How about just the following, which is not conditionally compiled:
	if (!(cpuq & BIT(OCELOT_MRP_CPUQ)))
		skb->offload_fwd_mark = 1;

> +		skb->offload_fwd_mark = 0;
> +#endif
> +
>  	/* Ocelot switches copy frames unmodified to the CPU. However, it is
>  	 * possible for the user to request a VLAN modification through
>  	 * VCAP_IS1_ACT_VID_REPLACE_ENA. In this case, what will happen is that
> -- 
> 2.27.0
> 

Re: [PATCH net-next v4 1/8] switchdev: mrp: Remove CONFIG_BRIDGE_MRP

[Vladimir Oltean]

On Tue, Feb 16, 2021 at 10:41:58PM +0100, Horatiu Vultur wrote:
> Remove #IS_ENABLED(CONFIG_BRIDGE_MRP) from switchdev.h. This will
> simplify the code implements MRP callbacks and will be similar with the
> vlan filtering.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v4 2/8] switchdev: mrp: Extend ring_role_mrp and in_role_mrp

[Vladimir Oltean]

On Tue, Feb 16, 2021 at 10:41:59PM +0100, Horatiu Vultur wrote:
> Add the member sw_backup to the structures switchdev_obj_ring_role_mrp
> and switchdev_obj_in_role_mrp. In this way the SW can call the driver in
> 2 ways, once when sw_backup is set to false, meaning that the driver
> should implement this completely in HW. And if that is not supported the
> SW will call again but with sw_backup set to true, meaning that the
> HW should help or allow the SW to run the protocol.
> 
> For example when role is MRM, if the HW can't detect when it stops
> receiving MRP Test frames but it can trap these frames to CPU, then it
> needs to return -EOPNOTSUPP when sw_backup is false and return 0 when
> sw_backup is true.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  include/net/switchdev.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index 465362d9d063..b7fc7d0f54e2 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -127,6 +127,7 @@ struct switchdev_obj_ring_role_mrp {
>  	struct switchdev_obj obj;
>  	u8 ring_role;
>  	u32 ring_id;
> +	u8 sw_backup;
>  };
>  
>  #define SWITCHDEV_OBJ_RING_ROLE_MRP(OBJ) \
> @@ -161,6 +162,7 @@ struct switchdev_obj_in_role_mrp {
>  	u32 ring_id;
>  	u16 in_id;
>  	u8 in_role;
> +	u8 sw_backup;

What was wrong with 'bool'?

>  };
>  
>  #define SWITCHDEV_OBJ_IN_ROLE_MRP(OBJ) \
> -- 
> 2.27.0
> 

If a driver implements full MRP offload for a ring/interconnect
manager/automanager, should it return -EOPNOTSUPP when sw_backup=false?

Re: [PATCH net-next v4 4/8] bridge: mrp: Extend br_mrp_switchdev to detect better the errors

[Vladimir Oltean]

On Tue, Feb 16, 2021 at 10:42:01PM +0100, Horatiu Vultur wrote:
> This patch extends the br_mrp_switchdev functions to be able to have a
> better understanding what cause the issue and if the SW needs to be used
> as a backup.
> 
> There are the following cases:
> - when the code is compiled without CONFIG_NET_SWITCHDEV. In this case
>   return success so the SW can continue with the protocol. Depending
>   on the function, it returns 0 or BR_MRP_SW.
> - when code is compiled with CONFIG_NET_SWITCHDEV and the driver doesn't
>   implement any MRP callbacks. In this case the HW can't run MRP so it
>   just returns -EOPNOTSUPP. So the SW will stop further to configure the
>   node.
> - when code is compiled with CONFIG_NET_SWITCHDEV and the driver fully
>   supports any MRP functionality. In this case the SW doesn't need to do
>   anything. The functions will return 0 or BR_MRP_HW.
> - when code is compiled with CONFIG_NET_SWITCHDEV and the HW can't run
>   completely the protocol but it can help the SW to run it. For
>   example, the HW can't support completely MRM role(can't detect when it
>   stops receiving MRP Test frames) but it can redirect these frames to
>   CPU. In this case it is possible to have a SW fallback. The SW will
>   try initially to call the driver with sw_backup set to false, meaning
>   that the HW should implement completely the role. If the driver returns
>   -EOPNOTSUPP, the SW will try again with sw_backup set to false,
>   meaning that the SW will detect when it stops receiving the frames but
>   it needs HW support to redirect the frames to CPU. In case the driver
>   returns 0 then the SW will continue to configure the node accordingly.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  net/bridge/br_mrp_switchdev.c | 171 +++++++++++++++++++++-------------
>  net/bridge/br_private_mrp.h   |  24 +++--
>  2 files changed, 118 insertions(+), 77 deletions(-)
> 
> diff --git a/net/bridge/br_mrp_switchdev.c b/net/bridge/br_mrp_switchdev.c
> index 3c9a4abcf4ee..cb54b324fa8c 100644
> --- a/net/bridge/br_mrp_switchdev.c
> +++ b/net/bridge/br_mrp_switchdev.c
> @@ -4,6 +4,30 @@
>  
>  #include "br_private_mrp.h"
>  
> +static enum br_mrp_hw_support
> +br_mrp_switchdev_port_obj(struct net_bridge *br,
> +			  const struct switchdev_obj *obj, bool add)
> +{
> +	int err;
> +

Looks like you could have added this check here and simplified all the
callers:

	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
		return BR_MRP_SW;

> +	if (add)
> +		err = switchdev_port_obj_add(br->dev, obj, NULL);
> +	else
> +		err = switchdev_port_obj_del(br->dev, obj);
> +
> +	/* In case of success just return and notify the SW that doesn't need
> +	 * to do anything
> +	 */
> +	if (!err)
> +		return BR_MRP_HW;
> +
> +	if (err != -EOPNOTSUPP)
> +		return BR_MRP_NONE;
> +
> +	/* Continue with SW backup */
> +	return BR_MRP_SW;
> +}
> +

Re: [PATCH net-next v4 5/8] bridge: mrp: Update br_mrp to use new return values of br_mrp_switchdev

[Vladimir Oltean]

On Tue, Feb 16, 2021 at 10:42:02PM +0100, Horatiu Vultur wrote:
> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
> index 01c67ed727a9..12487f6fe9b4 100644
> --- a/net/bridge/br_mrp.c
> +++ b/net/bridge/br_mrp.c
> @@ -639,7 +639,7 @@ int br_mrp_set_ring_role(struct net_bridge *br,
>  			 struct br_mrp_ring_role *role)
>  {
>  	struct br_mrp *mrp = br_mrp_find_id(br, role->ring_id);
> -	int err;
> +	enum br_mrp_hw_support support;
>  
>  	if (!mrp)
>  		return -EINVAL;
> @@ -647,9 +647,9 @@ int br_mrp_set_ring_role(struct net_bridge *br,
>  	mrp->ring_role = role->ring_role;
>  
>  	/* If there is an error just bailed out */
> -	err = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role);
> -	if (err && err != -EOPNOTSUPP)
> -		return err;
> +	support = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role);
> +	if (support == BR_MRP_NONE)
> +		return -EOPNOTSUPP;

It is broken to update the return type and value of a function in one
patch, and check for the updated return value in another patch.

>  
>  	/* Now detect if the HW actually applied the role or not. If the HW
>  	 * applied the role it means that the SW will not to do those operations
> @@ -657,7 +657,7 @@ int br_mrp_set_ring_role(struct net_bridge *br,
>  	 * SW when ring is open, but if the is not pushed to the HW the SW will
>  	 * need to detect when the ring is open
>  	 */
> -	mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
> +	mrp->ring_role_offloaded = support == BR_MRP_SW ? 0 : 1;
>  
>  	return 0;
>  }

Re: [PATCH net-next v4 6/8] net: mscc: ocelot: Add support for MRP

[Vladimir Oltean]

On Tue, Feb 16, 2021 at 10:42:03PM +0100, Horatiu Vultur wrote:
> Add basic support for MRP. The HW will just trap all MRP frames on the
> ring ports to CPU and allow the SW to process them. In this way it is
> possible to for this node to behave both as MRM and MRC.
> 
> Current limitations are:
> - it doesn't support Interconnect roles.
> - it supports only a single ring.
> - the HW should be able to do forwarding of MRP Test frames so the SW
>   will not need to do this. So it would be able to have the role MRC
>   without SW support.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  drivers/net/ethernet/mscc/Makefile     |   1 +
>  drivers/net/ethernet/mscc/ocelot.c     |  10 +-
>  drivers/net/ethernet/mscc/ocelot_mrp.c | 175 +++++++++++++++++++++++++
>  drivers/net/ethernet/mscc/ocelot_net.c |  60 +++++++++
>  include/linux/dsa/ocelot.h             |   5 +
>  include/soc/mscc/ocelot.h              |  45 +++++++
>  6 files changed, 295 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/mscc/ocelot_mrp.c
> 
> diff --git a/drivers/net/ethernet/mscc/Makefile b/drivers/net/ethernet/mscc/Makefile
> index 346bba2730ad..722c27694b21 100644
> --- a/drivers/net/ethernet/mscc/Makefile
> +++ b/drivers/net/ethernet/mscc/Makefile
> @@ -8,6 +8,7 @@ mscc_ocelot_switch_lib-y := \
>  	ocelot_flower.o \
>  	ocelot_ptp.o \
>  	ocelot_devlink.o
> +mscc_ocelot_switch_lib-$(CONFIG_BRIDGE_MRP) += ocelot_mrp.o
>  obj-$(CONFIG_MSCC_OCELOT_SWITCH) += mscc_ocelot.o
>  mscc_ocelot-y := \
>  	ocelot_vsc7514.o \
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 5d13087c85d6..46e5c9136bac 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -687,7 +687,7 @@ static int ocelot_xtr_poll_xfh(struct ocelot *ocelot, int grp, u32 *xfh)
>  int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, struct sk_buff **nskb)
>  {
>  	struct skb_shared_hwtstamps *shhwtstamps;
> -	u64 tod_in_ns, full_ts_in_ns;
> +	u64 tod_in_ns, full_ts_in_ns, cpuq;
>  	u64 timestamp, src_port, len;
>  	u32 xfh[OCELOT_TAG_LEN / 4];
>  	struct net_device *dev;
> @@ -704,6 +704,7 @@ int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, struct sk_buff **nskb)
>  	ocelot_xfh_get_src_port(xfh, &src_port);
>  	ocelot_xfh_get_len(xfh, &len);
>  	ocelot_xfh_get_rew_val(xfh, &timestamp);
> +	ocelot_xfh_get_cpuq(xfh, &cpuq);
>  
>  	if (WARN_ON(src_port >= ocelot->num_phys_ports))
>  		return -EINVAL;
> @@ -770,6 +771,13 @@ int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, struct sk_buff **nskb)
>  		skb->offload_fwd_mark = 1;
>  
>  	skb->protocol = eth_type_trans(skb, dev);
> +
> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> +	if (skb->protocol == cpu_to_be16(ETH_P_MRP) &&
> +	    cpuq & BIT(OCELOT_MRP_CPUQ))
> +		skb->offload_fwd_mark = 0;
> +#endif

Same comment as in DSA, it sounds simpler to me to just do:

	if ((ocelot->bridge_mask & BIT(src_port)) &&
	    !(cpuq & BIT(OCELOT_MRP_CPUQ)))
		skb->offload_fwd_mark = 1;

When we add support for more packet traps, this check will be more
amortized anyway.

> +
>  	*nskb = skb;
>  
>  	return 0;
> diff --git a/drivers/net/ethernet/mscc/ocelot_mrp.c b/drivers/net/ethernet/mscc/ocelot_mrp.c
> new file mode 100644
> index 000000000000..683da320bfd8
> --- /dev/null
> +++ b/drivers/net/ethernet/mscc/ocelot_mrp.c
> @@ -0,0 +1,175 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/* Microsemi Ocelot Switch driver
> + *
> + * This contains glue logic between the switchdev driver operations and the
> + * mscc_ocelot_switch_lib.

Wrong, this _is_ part of the mscc_ocelot_switch_lib. Which is also the
reason why some of the code below will not work.

> + *
> + * Copyright (c) 2017, 2019 Microsemi Corporation
> + * Copyright 2020-2021 NXP Semiconductors
> + */
> +
> +#include <linux/if_bridge.h>
> +#include <linux/mrp_bridge.h>
> +#include <soc/mscc/ocelot_vcap.h>
> +#include <uapi/linux/mrp_bridge.h>
> +#include "ocelot.h"
> +#include "ocelot_vcap.h"
> +
> +static int ocelot_mrp_del_vcap(struct ocelot *ocelot, int port)
> +{
> +	struct ocelot_vcap_block *block_vcap_is2;
> +	struct ocelot_vcap_filter *filter;
> +
> +	block_vcap_is2 = &ocelot->block[VCAP_IS2];
> +	filter = ocelot_vcap_block_find_filter_by_id(block_vcap_is2, port,
> +						     false);
> +	if (!filter)
> +		return 0;
> +
> +	return ocelot_vcap_filter_del(ocelot, filter);
> +}
> +
> +int ocelot_mrp_add(struct ocelot *ocelot, int port,
> +		   const struct switchdev_obj_mrp *mrp)
> +{
> +	struct ocelot_port *ocelot_port = ocelot->ports[port];
> +	struct ocelot_port_private *priv;
> +	struct net_device *dev;
> +
> +	if (!ocelot_port)
> +		return -EOPNOTSUPP;
> +
> +	priv = container_of(ocelot_port, struct ocelot_port_private, port);
> +	dev = priv->dev;

No, no, no.
The struct net_device registered by DSA uses a netdev_priv of
struct dsa_slave_priv. You can't just go ahead and assume that the
caller of this function uses struct ocelot_port_private.

Please go to struct ocelot_port and add:
	bool is_mrp_primary;
	bool is_mrp_secondary;

and replace the checks for a net_device with bools.

> +
> +	if (mrp->p_port != dev && mrp->s_port != dev)
> +		return 0;
> +
> +	if (ocelot->mrp_ring_id != 0 &&
> +	    ocelot->mrp_s_port &&
> +	    ocelot->mrp_p_port)
> +		return -EINVAL;
> +
> +	if (mrp->p_port == dev)
> +		ocelot->mrp_p_port = dev;
> +
> +	if (mrp->s_port == dev)
> +		ocelot->mrp_s_port = dev;
> +
> +	ocelot->mrp_ring_id = mrp->ring_id;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ocelot_mrp_add);
> +
> +int ocelot_mrp_del(struct ocelot *ocelot, int port,
> +		   const struct switchdev_obj_mrp *mrp)
> +{
> +	struct ocelot_port *ocelot_port = ocelot->ports[port];
> +	struct ocelot_port_private *priv;
> +	struct net_device *dev;
> +
> +	if (!ocelot_port)
> +		return -EOPNOTSUPP;
> +
> +	priv = container_of(ocelot_port, struct ocelot_port_private, port);
> +	dev = priv->dev;
> +
> +	if (ocelot->mrp_p_port != dev && ocelot->mrp_s_port != dev)
> +		return 0;
> +
> +	if (ocelot->mrp_ring_id == 0 &&
> +	    !ocelot->mrp_s_port &&
> +	    !ocelot->mrp_p_port)
> +		return -EINVAL;
> +
> +	if (ocelot_mrp_del_vcap(ocelot, priv->chip_port))
> +		return -EINVAL;
> +
> +	if (ocelot->mrp_p_port == dev)
> +		ocelot->mrp_p_port = NULL;
> +
> +	if (ocelot->mrp_s_port == dev)
> +		ocelot->mrp_s_port = NULL;
> +
> +	ocelot->mrp_ring_id = 0;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ocelot_mrp_del);
> +
> +int ocelot_mrp_add_ring_role(struct ocelot *ocelot, int port,
> +			     const struct switchdev_obj_ring_role_mrp *mrp)
> +{
> +	struct ocelot_port *ocelot_port = ocelot->ports[port];
> +	struct ocelot_vcap_filter *filter;
> +	struct ocelot_port_private *priv;
> +	struct net_device *dev;
> +	int err;
> +
> +	if (!ocelot_port)
> +		return -EOPNOTSUPP;
> +
> +	priv = container_of(ocelot_port, struct ocelot_port_private, port);
> +	dev = priv->dev;
> +
> +	if (ocelot->mrp_ring_id != mrp->ring_id)
> +		return -EINVAL;
> +
> +	if (!mrp->sw_backup)
> +		return -EOPNOTSUPP;
> +
> +	if (ocelot->mrp_p_port != dev && ocelot->mrp_s_port != dev)
> +		return 0;
> +
> +	filter = kzalloc(sizeof(*filter), GFP_ATOMIC);
> +	if (!filter)
> +		return -ENOMEM;
> +
> +	filter->key_type = OCELOT_VCAP_KEY_ETYPE;
> +	filter->prio = 1;
> +	filter->id.cookie = priv->chip_port;

You have "port" already. This is also wrong for the reason I stated above:
no "priv" in the common library.

> +	filter->id.tc_offload = false;
> +	filter->block_id = VCAP_IS2;
> +	filter->type = OCELOT_VCAP_FILTER_OFFLOAD;
> +	filter->ingress_port_mask = BIT(priv->chip_port);
> +	*(__be16 *)filter->key.etype.etype.value = htons(ETH_P_MRP);
> +	*(__be16 *)filter->key.etype.etype.mask = htons(0xffff);
> +	filter->action.mask_mode = OCELOT_MASK_MODE_PERMIT_DENY;
> +	filter->action.port_mask = 0x0;
> +	filter->action.cpu_copy_ena = true;
> +	filter->action.cpu_qu_num = OCELOT_MRP_CPUQ;
> +
> +	err = ocelot_vcap_filter_add(ocelot, filter, NULL);
> +	if (err)
> +		kfree(filter);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(ocelot_mrp_add_ring_role);
> +
> +int ocelot_mrp_del_ring_role(struct ocelot *ocelot, int port,
> +			     const struct switchdev_obj_ring_role_mrp *mrp)
> +{
> +	struct ocelot_port *ocelot_port = ocelot->ports[port];
> +	struct ocelot_port_private *priv;
> +	struct net_device *dev;
> +
> +	if (!ocelot_port)
> +		return -EOPNOTSUPP;
> +
> +	priv = container_of(ocelot_port, struct ocelot_port_private, port);
> +	dev = priv->dev;
> +
> +	if (ocelot->mrp_ring_id != mrp->ring_id)
> +		return -EINVAL;
> +
> +	if (!mrp->sw_backup)
> +		return -EOPNOTSUPP;
> +
> +	if (ocelot->mrp_p_port != dev && ocelot->mrp_s_port != dev)
> +		return 0;
> +
> +	return ocelot_mrp_del_vcap(ocelot, priv->chip_port);
> +}
> +EXPORT_SYMBOL(ocelot_mrp_del_ring_role);
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 6518262532f0..12cb6867a2d0 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -1010,6 +1010,52 @@ static int ocelot_port_obj_del_mdb(struct net_device *dev,
>  	return ocelot_port_mdb_del(ocelot, port, mdb);
>  }
>  
> +static int ocelot_port_obj_mrp_add(struct net_device *dev,
> +				   const struct switchdev_obj_mrp *mrp)
> +{
> +	struct ocelot_port_private *priv = netdev_priv(dev);
> +	struct ocelot_port *ocelot_port = &priv->port;
> +	struct ocelot *ocelot = ocelot_port->ocelot;
> +	int port = priv->chip_port;
> +
> +	return ocelot_mrp_add(ocelot, port, mrp);
> +}
> +
> +static int ocelot_port_obj_mrp_del(struct net_device *dev,
> +				   const struct switchdev_obj_mrp *mrp)
> +{
> +	struct ocelot_port_private *priv = netdev_priv(dev);
> +	struct ocelot_port *ocelot_port = &priv->port;
> +	struct ocelot *ocelot = ocelot_port->ocelot;
> +	int port = priv->chip_port;
> +
> +	return ocelot_mrp_del(ocelot, port, mrp);
> +}
> +
> +static int
> +ocelot_port_obj_mrp_add_ring_role(struct net_device *dev,
> +				  const struct switchdev_obj_ring_role_mrp *mrp)
> +{
> +	struct ocelot_port_private *priv = netdev_priv(dev);
> +	struct ocelot_port *ocelot_port = &priv->port;
> +	struct ocelot *ocelot = ocelot_port->ocelot;
> +	int port = priv->chip_port;
> +
> +	return ocelot_mrp_add_ring_role(ocelot, port, mrp);
> +}
> +
> +static int
> +ocelot_port_obj_mrp_del_ring_role(struct net_device *dev,
> +				  const struct switchdev_obj_ring_role_mrp *mrp)
> +{
> +	struct ocelot_port_private *priv = netdev_priv(dev);
> +	struct ocelot_port *ocelot_port = &priv->port;
> +	struct ocelot *ocelot = ocelot_port->ocelot;
> +	int port = priv->chip_port;
> +
> +	return ocelot_mrp_del_ring_role(ocelot, port, mrp);
> +}
> +
>  static int ocelot_port_obj_add(struct net_device *dev,
>  			       const struct switchdev_obj *obj,
>  			       struct netlink_ext_ack *extack)
> @@ -1024,6 +1070,13 @@ static int ocelot_port_obj_add(struct net_device *dev,
>  	case SWITCHDEV_OBJ_ID_PORT_MDB:
>  		ret = ocelot_port_obj_add_mdb(dev, SWITCHDEV_OBJ_PORT_MDB(obj));
>  		break;
> +	case SWITCHDEV_OBJ_ID_MRP:
> +		ret = ocelot_port_obj_mrp_add(dev, SWITCHDEV_OBJ_MRP(obj));
> +		break;
> +	case SWITCHDEV_OBJ_ID_RING_ROLE_MRP:
> +		ret = ocelot_port_obj_mrp_add_ring_role(dev,
> +							SWITCHDEV_OBJ_RING_ROLE_MRP(obj));
> +		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -1044,6 +1097,13 @@ static int ocelot_port_obj_del(struct net_device *dev,
>  	case SWITCHDEV_OBJ_ID_PORT_MDB:
>  		ret = ocelot_port_obj_del_mdb(dev, SWITCHDEV_OBJ_PORT_MDB(obj));
>  		break;
> +	case SWITCHDEV_OBJ_ID_MRP:
> +		ret = ocelot_port_obj_mrp_del(dev, SWITCHDEV_OBJ_MRP(obj));
> +		break;
> +	case SWITCHDEV_OBJ_ID_RING_ROLE_MRP:
> +		ret = ocelot_port_obj_mrp_del_ring_role(dev,
> +							SWITCHDEV_OBJ_RING_ROLE_MRP(obj));
> +		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> diff --git a/include/linux/dsa/ocelot.h b/include/linux/dsa/ocelot.h
> index c6bc45ae5e03..4265f328681a 100644
> --- a/include/linux/dsa/ocelot.h
> +++ b/include/linux/dsa/ocelot.h
> @@ -160,6 +160,11 @@ static inline void ocelot_xfh_get_src_port(void *extraction, u64 *src_port)
>  	packing(extraction, src_port, 46, 43, OCELOT_TAG_LEN, UNPACK, 0);
>  }
>  
> +static inline void ocelot_xfh_get_cpuq(void *extraction, u64 *cpuq)
> +{
> +	packing(extraction, cpuq, 28, 20, OCELOT_TAG_LEN, UNPACK, 0);
> +}
> +
>  static inline void ocelot_xfh_get_qos_class(void *extraction, u64 *qos_class)
>  {
>  	packing(extraction, qos_class, 19, 17, OCELOT_TAG_LEN, UNPACK, 0);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 1f2d90976564..425ff29d9389 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -112,6 +112,8 @@
>  #define REG_RESERVED_ADDR		0xffffffff
>  #define REG_RESERVED(reg)		REG(reg, REG_RESERVED_ADDR)
>  
> +#define OCELOT_MRP_CPUQ			7
> +
>  enum ocelot_target {
>  	ANA = 1,
>  	QS,
> @@ -677,6 +679,12 @@ struct ocelot {
>  	/* Protects the PTP clock */
>  	spinlock_t			ptp_clock_lock;
>  	struct ptp_pin_desc		ptp_pins[OCELOT_PTP_PINS_NUM];
> +
> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> +	u16				mrp_ring_id;
> +	struct net_device		*mrp_p_port;
> +	struct net_device		*mrp_s_port;
> +#endif

I'd rather have this without the ifdeffery, doesn't seem too expensive
to justify compiling it out. We have a 4K array of VLANs in struct
ocelot, for god's sake.

Re: [PATCH net-next v4 7/8] net: dsa: add MRP support

[Vladimir Oltean]

On Tue, Feb 16, 2021 at 10:42:04PM +0100, Horatiu Vultur wrote:
> Add support for offloading MRP in HW. Currently implement the switchdev
> calls 'SWITCHDEV_OBJ_ID_MRP', 'SWITCHDEV_OBJ_ID_RING_ROLE_MRP',
> to allow to create MRP instances and to set the role of these instances.
> 
> Add DSA_NOTIFIER_MRP_ADD/DEL and DSA_NOTIFIER_MRP_ADD/DEL_RING_ROLE
> which calls to .port_mrp_add/del and .port_mrp_add/del_ring_role in the
> DSA driver for the switch.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  include/net/dsa.h  |  12 ++++++
>  net/dsa/dsa_priv.h |  26 +++++++++++
>  net/dsa/port.c     |  48 +++++++++++++++++++++
>  net/dsa/slave.c    |  22 ++++++++++
>  net/dsa/switch.c   | 105 +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 213 insertions(+)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 68f8159564a3..83a933e563fe 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -792,6 +792,18 @@ struct dsa_switch_ops {
>  				 struct net_device *hsr);
>  	int	(*port_hsr_leave)(struct dsa_switch *ds, int port,
>  				  struct net_device *hsr);
> +
> +	/*
> +	 * MRP integration
> +	 */
> +	int	(*port_mrp_add)(struct dsa_switch *ds, int port,
> +				const struct switchdev_obj_mrp *mrp);
> +	int	(*port_mrp_del)(struct dsa_switch *ds, int port,
> +				const struct switchdev_obj_mrp *mrp);
> +	int	(*port_mrp_add_ring_role)(struct dsa_switch *ds, int port,
> +					  const struct switchdev_obj_ring_role_mrp *mrp);
> +	int	(*port_mrp_del_ring_role)(struct dsa_switch *ds, int port,
> +					  const struct switchdev_obj_ring_role_mrp *mrp);
>  };
>  
>  #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes)		\
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index e9d1e76c42ba..2eeaa42f2e08 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -31,6 +31,10 @@ enum {
>  	DSA_NOTIFIER_VLAN_DEL,
>  	DSA_NOTIFIER_MTU,
>  	DSA_NOTIFIER_TAG_PROTO,
> +	DSA_NOTIFIER_MRP_ADD,
> +	DSA_NOTIFIER_MRP_DEL,
> +	DSA_NOTIFIER_MRP_ADD_RING_ROLE,
> +	DSA_NOTIFIER_MRP_DEL_RING_ROLE,
>  };
>  
>  /* DSA_NOTIFIER_AGEING_TIME */
> @@ -91,6 +95,20 @@ struct dsa_notifier_tag_proto_info {
>  	const struct dsa_device_ops *tag_ops;
>  };
>  
> +/* DSA_NOTIFIER_MRP_* */
> +struct dsa_notifier_mrp_info {
> +	const struct switchdev_obj_mrp *mrp;
> +	int sw_index;
> +	int port;
> +};
> +
> +/* DSA_NOTIFIER_MRP_* */
> +struct dsa_notifier_mrp_ring_role_info {
> +	const struct switchdev_obj_ring_role_mrp *mrp;
> +	int sw_index;
> +	int port;
> +};
> +
>  struct dsa_switchdev_event_work {
>  	struct dsa_switch *ds;
>  	int port;
> @@ -198,6 +216,14 @@ int dsa_port_vlan_add(struct dsa_port *dp,
>  		      struct netlink_ext_ack *extack);
>  int dsa_port_vlan_del(struct dsa_port *dp,
>  		      const struct switchdev_obj_port_vlan *vlan);
> +int dsa_port_mrp_add(const struct dsa_port *dp,
> +		     const struct switchdev_obj_mrp *mrp);
> +int dsa_port_mrp_del(const struct dsa_port *dp,
> +		     const struct switchdev_obj_mrp *mrp);
> +int dsa_port_mrp_add_ring_role(const struct dsa_port *dp,
> +			       const struct switchdev_obj_ring_role_mrp *mrp);
> +int dsa_port_mrp_del_ring_role(const struct dsa_port *dp,
> +			       const struct switchdev_obj_ring_role_mrp *mrp);
>  int dsa_port_link_register_of(struct dsa_port *dp);
>  void dsa_port_link_unregister_of(struct dsa_port *dp);
>  int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr);
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 14a1d0d77657..c9c6d7ab3f47 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -564,6 +564,54 @@ int dsa_port_vlan_del(struct dsa_port *dp,
>  	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
>  }
>  
> +int dsa_port_mrp_add(const struct dsa_port *dp,
> +		     const struct switchdev_obj_mrp *mrp)
> +{
> +	struct dsa_notifier_mrp_info info = {
> +		.sw_index = dp->ds->index,
> +		.port = dp->index,
> +		.mrp = mrp,
> +	};
> +
> +	return dsa_port_notify(dp, DSA_NOTIFIER_MRP_ADD, &info);
> +}
> +
> +int dsa_port_mrp_del(const struct dsa_port *dp,
> +		     const struct switchdev_obj_mrp *mrp)
> +{
> +	struct dsa_notifier_mrp_info info = {
> +		.sw_index = dp->ds->index,
> +		.port = dp->index,
> +		.mrp = mrp,
> +	};
> +
> +	return dsa_port_notify(dp, DSA_NOTIFIER_MRP_DEL, &info);
> +}
> +
> +int dsa_port_mrp_add_ring_role(const struct dsa_port *dp,
> +			       const struct switchdev_obj_ring_role_mrp *mrp)
> +{
> +	struct dsa_notifier_mrp_ring_role_info info = {
> +		.sw_index = dp->ds->index,
> +		.port = dp->index,
> +		.mrp = mrp,
> +	};
> +
> +	return dsa_port_notify(dp, DSA_NOTIFIER_MRP_ADD_RING_ROLE, &info);
> +}
> +
> +int dsa_port_mrp_del_ring_role(const struct dsa_port *dp,
> +			       const struct switchdev_obj_ring_role_mrp *mrp)
> +{
> +	struct dsa_notifier_mrp_ring_role_info info = {
> +		.sw_index = dp->ds->index,
> +		.port = dp->index,
> +		.mrp = mrp,
> +	};
> +
> +	return dsa_port_notify(dp, DSA_NOTIFIER_MRP_DEL_RING_ROLE, &info);
> +}
> +
>  void dsa_port_set_tag_protocol(struct dsa_port *cpu_dp,
>  			       const struct dsa_device_ops *tag_ops)
>  {
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 5ecb43a1b6e0..491e3761b5f4 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -404,6 +404,17 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>  	case SWITCHDEV_OBJ_ID_PORT_VLAN:
>  		err = dsa_slave_vlan_add(dev, obj, extack);
>  		break;
> +	case SWITCHDEV_OBJ_ID_MRP:
> +		if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
> +			return -EOPNOTSUPP;
> +		err = dsa_port_mrp_add(dp, SWITCHDEV_OBJ_MRP(obj));
> +		break;
> +	case SWITCHDEV_OBJ_ID_RING_ROLE_MRP:
> +		if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
> +			return -EOPNOTSUPP;
> +		err = dsa_port_mrp_add_ring_role(dp,
> +						 SWITCHDEV_OBJ_RING_ROLE_MRP(obj));
> +		break;
>  	default:
>  		err = -EOPNOTSUPP;
>  		break;
> @@ -461,6 +472,17 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
>  	case SWITCHDEV_OBJ_ID_PORT_VLAN:
>  		err = dsa_slave_vlan_del(dev, obj);
>  		break;
> +	case SWITCHDEV_OBJ_ID_MRP:
> +		if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
> +			return -EOPNOTSUPP;
> +		err = dsa_port_mrp_del(dp, SWITCHDEV_OBJ_MRP(obj));
> +		break;
> +	case SWITCHDEV_OBJ_ID_RING_ROLE_MRP:
> +		if (!dsa_port_offloads_netdev(dp, obj->orig_dev))
> +			return -EOPNOTSUPP;
> +		err = dsa_port_mrp_del_ring_role(dp,
> +						 SWITCHDEV_OBJ_RING_ROLE_MRP(obj));
> +		break;
>  	default:
>  		err = -EOPNOTSUPP;
>  		break;
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index db2a9b221988..4b5da89dc27a 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -372,6 +372,99 @@ static int dsa_switch_change_tag_proto(struct dsa_switch *ds,
>  	return 0;
>  }
>  
> +static bool dsa_switch_mrp_match(struct dsa_switch *ds, int port,
> +				 struct dsa_notifier_mrp_info *info)
> +{
> +	if (ds->index == info->sw_index && port == info->port)
> +		return true;
> +
> +	if (dsa_is_dsa_port(ds, port))
> +		return true;
> +
> +	return false;
> +}
> +
> +static int dsa_switch_mrp_add(struct dsa_switch *ds,
> +			      struct dsa_notifier_mrp_info *info)
> +{
> +	int err = 0;
> +	int port;
> +
> +	if (!ds->ops->port_mrp_add)
> +		return -EOPNOTSUPP;
> +
> +	for (port = 0; port < ds->num_ports; port++) {
> +		if (dsa_switch_mrp_match(ds, port, info)) {
> +			err = ds->ops->port_mrp_add(ds, port, info->mrp);
> +			if (err)
> +				break;
> +		}
> +	}
> +
> +	return err;
> +}
> +
> +static int dsa_switch_mrp_del(struct dsa_switch *ds,
> +			      struct dsa_notifier_mrp_info *info)
> +{
> +	if (!ds->ops->port_mrp_del)
> +		return -EOPNOTSUPP;
> +
> +	if (ds->index == info->sw_index)
> +		return ds->ops->port_mrp_del(ds, info->port, info->mrp);
> +
> +	return 0;
> +}
> +

Why not use dsa_switch_mrp_match here too? (question valid for the ring
role below too)

> +static bool
> +dsa_switch_mrp_ring_role_match(struct dsa_switch *ds, int port,
> +			       struct dsa_notifier_mrp_ring_role_info *info)
> +{
> +	if (ds->index == info->sw_index && port == info->port)
> +		return true;
> +
> +	if (dsa_is_dsa_port(ds, port))
> +		return true;
> +
> +	return false;
> +}
> +
> +static int
> +dsa_switch_mrp_add_ring_role(struct dsa_switch *ds,
> +			     struct dsa_notifier_mrp_ring_role_info *info)
> +{
> +	int err = 0;
> +	int port;
> +
> +	if (!ds->ops->port_mrp_add)
> +		return -EOPNOTSUPP;
> +
> +	for (port = 0; port < ds->num_ports; port++) {
> +		if (dsa_switch_mrp_ring_role_match(ds, port, info)) {
> +			err = ds->ops->port_mrp_add_ring_role(ds, port,
> +							      info->mrp);
> +			if (err)
> +				break;
> +		}
> +	}
> +
> +	return err;
> +}
> +
> +static int
> +dsa_switch_mrp_del_ring_role(struct dsa_switch *ds,
> +			     struct dsa_notifier_mrp_ring_role_info *info)
> +{
> +	if (!ds->ops->port_mrp_del)
> +		return -EOPNOTSUPP;
> +
> +	if (ds->index == info->sw_index)
> +		return ds->ops->port_mrp_del_ring_role(ds, info->port,
> +						       info->mrp);
> +
> +	return 0;
> +}
> +
>  static int dsa_switch_event(struct notifier_block *nb,
>  			    unsigned long event, void *info)
>  {
> @@ -427,6 +520,18 @@ static int dsa_switch_event(struct notifier_block *nb,
>  	case DSA_NOTIFIER_TAG_PROTO:
>  		err = dsa_switch_change_tag_proto(ds, info);
>  		break;
> +	case DSA_NOTIFIER_MRP_ADD:
> +		err = dsa_switch_mrp_add(ds, info);
> +		break;
> +	case DSA_NOTIFIER_MRP_DEL:
> +		err = dsa_switch_mrp_del(ds, info);
> +		break;
> +	case DSA_NOTIFIER_MRP_ADD_RING_ROLE:
> +		err = dsa_switch_mrp_add_ring_role(ds, info);
> +		break;
> +	case DSA_NOTIFIER_MRP_DEL_RING_ROLE:
> +		err = dsa_switch_mrp_del_ring_role(ds, info);
> +		break;
>  	default:
>  		err = -EOPNOTSUPP;
>  		break;
> -- 
> 2.27.0
> 

Re: [PATCH net-next v4 6/8] net: mscc: ocelot: Add support for MRP

[Vladimir Oltean]

On Tue, Feb 16, 2021 at 10:42:03PM +0100, Horatiu Vultur wrote:
> +static inline void ocelot_xfh_get_cpuq(void *extraction, u64 *cpuq)
> +{
> +	packing(extraction, cpuq, 28, 20, OCELOT_TAG_LEN, UNPACK, 0);
> +}
> +

The 8 bits I count for CPUQ are from 27 to 20.
This is spilling over into LRN_FLAGS.

Re: [PATCH net-next v4 2/8] switchdev: mrp: Extend ring_role_mrp and in_role_mrp

[Horatiu Vultur]

The 02/17/2021 10:34, Vladimir Oltean wrote:

Hi Vladimir,

> 
> On Tue, Feb 16, 2021 at 10:41:59PM +0100, Horatiu Vultur wrote:
> > Add the member sw_backup to the structures switchdev_obj_ring_role_mrp
> > and switchdev_obj_in_role_mrp. In this way the SW can call the driver in
> > 2 ways, once when sw_backup is set to false, meaning that the driver
> > should implement this completely in HW. And if that is not supported the
> > SW will call again but with sw_backup set to true, meaning that the
> > HW should help or allow the SW to run the protocol.
> >
> > For example when role is MRM, if the HW can't detect when it stops
> > receiving MRP Test frames but it can trap these frames to CPU, then it
> > needs to return -EOPNOTSUPP when sw_backup is false and return 0 when
> > sw_backup is true.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  include/net/switchdev.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> > index 465362d9d063..b7fc7d0f54e2 100644
> > --- a/include/net/switchdev.h
> > +++ b/include/net/switchdev.h
> > @@ -127,6 +127,7 @@ struct switchdev_obj_ring_role_mrp {
> >       struct switchdev_obj obj;
> >       u8 ring_role;
> >       u32 ring_id;
> > +     u8 sw_backup;
> >  };
> >
> >  #define SWITCHDEV_OBJ_RING_ROLE_MRP(OBJ) \
> > @@ -161,6 +162,7 @@ struct switchdev_obj_in_role_mrp {
> >       u32 ring_id;
> >       u16 in_id;
> >       u8 in_role;
> > +     u8 sw_backup;
> 
> What was wrong with 'bool'?
Yeah, that would be a better match.
> 
> >  };
> >
> >  #define SWITCHDEV_OBJ_IN_ROLE_MRP(OBJ) \
> > --
> > 2.27.0
> >
> 
> If a driver implements full MRP offload for a ring/interconnect
> manager/automanager, should it return -EOPNOTSUPP when sw_backup=false?

In that case it should return 0.
So if the driver can:
- fully support MRP, when sw_backup = false, return 0. Then end of story.
- partially support MRP, when sw_backup = false, return -EOPNOTSUPP,
                         when sw_backup = true, return 0.
- no support at all, return -EOPNOTSUPP.

-- 
/Horatiu

Re: [PATCH net-next v4 4/8] bridge: mrp: Extend br_mrp_switchdev to detect better the errors

[Horatiu Vultur]

The 02/17/2021 10:56, Vladimir Oltean wrote:
> 
> On Tue, Feb 16, 2021 at 10:42:01PM +0100, Horatiu Vultur wrote:
> > This patch extends the br_mrp_switchdev functions to be able to have a
> > better understanding what cause the issue and if the SW needs to be used
> > as a backup.
> >
> > There are the following cases:
> > - when the code is compiled without CONFIG_NET_SWITCHDEV. In this case
> >   return success so the SW can continue with the protocol. Depending
> >   on the function, it returns 0 or BR_MRP_SW.
> > - when code is compiled with CONFIG_NET_SWITCHDEV and the driver doesn't
> >   implement any MRP callbacks. In this case the HW can't run MRP so it
> >   just returns -EOPNOTSUPP. So the SW will stop further to configure the
> >   node.
> > - when code is compiled with CONFIG_NET_SWITCHDEV and the driver fully
> >   supports any MRP functionality. In this case the SW doesn't need to do
> >   anything. The functions will return 0 or BR_MRP_HW.
> > - when code is compiled with CONFIG_NET_SWITCHDEV and the HW can't run
> >   completely the protocol but it can help the SW to run it. For
> >   example, the HW can't support completely MRM role(can't detect when it
> >   stops receiving MRP Test frames) but it can redirect these frames to
> >   CPU. In this case it is possible to have a SW fallback. The SW will
> >   try initially to call the driver with sw_backup set to false, meaning
> >   that the HW should implement completely the role. If the driver returns
> >   -EOPNOTSUPP, the SW will try again with sw_backup set to false,
> >   meaning that the SW will detect when it stops receiving the frames but
> >   it needs HW support to redirect the frames to CPU. In case the driver
> >   returns 0 then the SW will continue to configure the node accordingly.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  net/bridge/br_mrp_switchdev.c | 171 +++++++++++++++++++++-------------
> >  net/bridge/br_private_mrp.h   |  24 +++--
> >  2 files changed, 118 insertions(+), 77 deletions(-)
> >
> > diff --git a/net/bridge/br_mrp_switchdev.c b/net/bridge/br_mrp_switchdev.c
> > index 3c9a4abcf4ee..cb54b324fa8c 100644
> > --- a/net/bridge/br_mrp_switchdev.c
> > +++ b/net/bridge/br_mrp_switchdev.c
> > @@ -4,6 +4,30 @@
> >
> >  #include "br_private_mrp.h"
> >
> > +static enum br_mrp_hw_support
> > +br_mrp_switchdev_port_obj(struct net_bridge *br,
> > +                       const struct switchdev_obj *obj, bool add)
> > +{
> > +     int err;
> > +
> 
> Looks like you could have added this check here and simplified all the
> callers:
> 
>         if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
>                 return BR_MRP_SW;

Yes, good catch!

> 
> > +     if (add)
> > +             err = switchdev_port_obj_add(br->dev, obj, NULL);
> > +     else
> > +             err = switchdev_port_obj_del(br->dev, obj);
> > +
> > +     /* In case of success just return and notify the SW that doesn't need
> > +      * to do anything
> > +      */
> > +     if (!err)
> > +             return BR_MRP_HW;
> > +
> > +     if (err != -EOPNOTSUPP)
> > +             return BR_MRP_NONE;
> > +
> > +     /* Continue with SW backup */
> > +     return BR_MRP_SW;
> > +}
> > +

-- 
/Horatiu

Re: [PATCH net-next v4 2/8] switchdev: mrp: Extend ring_role_mrp and in_role_mrp

[Vladimir Oltean]

On Wed, Feb 17, 2021 at 04:58:45PM +0100, Horatiu Vultur wrote:
> > If a driver implements full MRP offload for a ring/interconnect
> > manager/automanager, should it return -EOPNOTSUPP when sw_backup=false?
> 
> In that case it should return 0.
> So if the driver can:
> - fully support MRP, when sw_backup = false, return 0. Then end of story.
> - partially support MRP, when sw_backup = false, return -EOPNOTSUPP,
>                          when sw_backup = true, return 0.
> - no support at all, return -EOPNOTSUPP.

Damn, I asked the wrong question.
I meant to ask about what it should return when sw_backup=true.
But you answered anyway that if it returns 0 when sw_backup=false, it
can simply not deal with the case where sw_backup=true, because that is
never supposed to happen.

Re: [PATCH net-next v4 5/8] bridge: mrp: Update br_mrp to use new return values of br_mrp_switchdev

[Horatiu Vultur]

The 02/17/2021 10:59, Vladimir Oltean wrote:
> 
> On Tue, Feb 16, 2021 at 10:42:02PM +0100, Horatiu Vultur wrote:
> > diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
> > index 01c67ed727a9..12487f6fe9b4 100644
> > --- a/net/bridge/br_mrp.c
> > +++ b/net/bridge/br_mrp.c
> > @@ -639,7 +639,7 @@ int br_mrp_set_ring_role(struct net_bridge *br,
> >                        struct br_mrp_ring_role *role)
> >  {
> >       struct br_mrp *mrp = br_mrp_find_id(br, role->ring_id);
> > -     int err;
> > +     enum br_mrp_hw_support support;
> >
> >       if (!mrp)
> >               return -EINVAL;
> > @@ -647,9 +647,9 @@ int br_mrp_set_ring_role(struct net_bridge *br,
> >       mrp->ring_role = role->ring_role;
> >
> >       /* If there is an error just bailed out */
> > -     err = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role);
> > -     if (err && err != -EOPNOTSUPP)
> > -             return err;
> > +     support = br_mrp_switchdev_set_ring_role(br, mrp, role->ring_role);
> > +     if (support == BR_MRP_NONE)
> > +             return -EOPNOTSUPP;
> 
> It is broken to update the return type and value of a function in one
> patch, and check for the updated return value in another patch.
> 

Yes, I will be more careful next time. I have tried to compile between
the patches and I have not see any issues here so I though that
everything is good.

> >
> >       /* Now detect if the HW actually applied the role or not. If the HW
> >        * applied the role it means that the SW will not to do those operations
> > @@ -657,7 +657,7 @@ int br_mrp_set_ring_role(struct net_bridge *br,
> >        * SW when ring is open, but if the is not pushed to the HW the SW will
> >        * need to detect when the ring is open
> >        */
> > -     mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
> > +     mrp->ring_role_offloaded = support == BR_MRP_SW ? 0 : 1;
> >
> >       return 0;
> >  }

-- 
/Horatiu

Re: [PATCH net-next v4 6/8] net: mscc: ocelot: Add support for MRP

[Horatiu Vultur]

The 02/17/2021 11:14, Vladimir Oltean wrote:
> 
> On Tue, Feb 16, 2021 at 10:42:03PM +0100, Horatiu Vultur wrote:
> > Add basic support for MRP. The HW will just trap all MRP frames on the
> > ring ports to CPU and allow the SW to process them. In this way it is
> > possible to for this node to behave both as MRM and MRC.
> >
> > Current limitations are:
> > - it doesn't support Interconnect roles.
> > - it supports only a single ring.
> > - the HW should be able to do forwarding of MRP Test frames so the SW
> >   will not need to do this. So it would be able to have the role MRC
> >   without SW support.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  drivers/net/ethernet/mscc/Makefile     |   1 +
> >  drivers/net/ethernet/mscc/ocelot.c     |  10 +-
> >  drivers/net/ethernet/mscc/ocelot_mrp.c | 175 +++++++++++++++++++++++++
> >  drivers/net/ethernet/mscc/ocelot_net.c |  60 +++++++++
> >  include/linux/dsa/ocelot.h             |   5 +
> >  include/soc/mscc/ocelot.h              |  45 +++++++
> >  6 files changed, 295 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/net/ethernet/mscc/ocelot_mrp.c
> >
> > diff --git a/drivers/net/ethernet/mscc/Makefile b/drivers/net/ethernet/mscc/Makefile
> > index 346bba2730ad..722c27694b21 100644
> > --- a/drivers/net/ethernet/mscc/Makefile
> > +++ b/drivers/net/ethernet/mscc/Makefile
> > @@ -8,6 +8,7 @@ mscc_ocelot_switch_lib-y := \
> >       ocelot_flower.o \
> >       ocelot_ptp.o \
> >       ocelot_devlink.o
> > +mscc_ocelot_switch_lib-$(CONFIG_BRIDGE_MRP) += ocelot_mrp.o
> >  obj-$(CONFIG_MSCC_OCELOT_SWITCH) += mscc_ocelot.o
> >  mscc_ocelot-y := \
> >       ocelot_vsc7514.o \
> > diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> > index 5d13087c85d6..46e5c9136bac 100644
> > --- a/drivers/net/ethernet/mscc/ocelot.c
> > +++ b/drivers/net/ethernet/mscc/ocelot.c
> > @@ -687,7 +687,7 @@ static int ocelot_xtr_poll_xfh(struct ocelot *ocelot, int grp, u32 *xfh)
> >  int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, struct sk_buff **nskb)
> >  {
> >       struct skb_shared_hwtstamps *shhwtstamps;
> > -     u64 tod_in_ns, full_ts_in_ns;
> > +     u64 tod_in_ns, full_ts_in_ns, cpuq;
> >       u64 timestamp, src_port, len;
> >       u32 xfh[OCELOT_TAG_LEN / 4];
> >       struct net_device *dev;
> > @@ -704,6 +704,7 @@ int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, struct sk_buff **nskb)
> >       ocelot_xfh_get_src_port(xfh, &src_port);
> >       ocelot_xfh_get_len(xfh, &len);
> >       ocelot_xfh_get_rew_val(xfh, &timestamp);
> > +     ocelot_xfh_get_cpuq(xfh, &cpuq);
> >
> >       if (WARN_ON(src_port >= ocelot->num_phys_ports))
> >               return -EINVAL;
> > @@ -770,6 +771,13 @@ int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, struct sk_buff **nskb)
> >               skb->offload_fwd_mark = 1;
> >
> >       skb->protocol = eth_type_trans(skb, dev);
> > +
> > +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> > +     if (skb->protocol == cpu_to_be16(ETH_P_MRP) &&
> > +         cpuq & BIT(OCELOT_MRP_CPUQ))
> > +             skb->offload_fwd_mark = 0;
> > +#endif

Hi Vladimir,

> 
> Same comment as in DSA, it sounds simpler to me to just do:
> 
>         if ((ocelot->bridge_mask & BIT(src_port)) &&
>             !(cpuq & BIT(OCELOT_MRP_CPUQ)))
>                 skb->offload_fwd_mark = 1;
> 
> When we add support for more packet traps, this check will be more
> amortized anyway.

Yes that looks simpler. But actually I think we can remove this once we
will do the forwarding of the frames in HW. And of course the same will
apply also for DSA driver.

> 
> > +
> >       *nskb = skb;
> >
> >       return 0;
> > diff --git a/drivers/net/ethernet/mscc/ocelot_mrp.c b/drivers/net/ethernet/mscc/ocelot_mrp.c
> > new file mode 100644
> > index 000000000000..683da320bfd8
> > --- /dev/null
> > +++ b/drivers/net/ethernet/mscc/ocelot_mrp.c
> > @@ -0,0 +1,175 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/* Microsemi Ocelot Switch driver
> > + *
> > + * This contains glue logic between the switchdev driver operations and the
> > + * mscc_ocelot_switch_lib.
> 
> Wrong, this _is_ part of the mscc_ocelot_switch_lib. Which is also the
> reason why some of the code below will not work.

Yes, this was the result of copy - paste.

> 
> > + *
> > + * Copyright (c) 2017, 2019 Microsemi Corporation
> > + * Copyright 2020-2021 NXP Semiconductors
> > + */
> > +
> > +#include <linux/if_bridge.h>
> > +#include <linux/mrp_bridge.h>
> > +#include <soc/mscc/ocelot_vcap.h>
> > +#include <uapi/linux/mrp_bridge.h>
> > +#include "ocelot.h"
> > +#include "ocelot_vcap.h"
> > +
> > +static int ocelot_mrp_del_vcap(struct ocelot *ocelot, int port)
> > +{
> > +     struct ocelot_vcap_block *block_vcap_is2;
> > +     struct ocelot_vcap_filter *filter;
> > +
> > +     block_vcap_is2 = &ocelot->block[VCAP_IS2];
> > +     filter = ocelot_vcap_block_find_filter_by_id(block_vcap_is2, port,
> > +                                                  false);
> > +     if (!filter)
> > +             return 0;
> > +
> > +     return ocelot_vcap_filter_del(ocelot, filter);
> > +}
> > +
> > +int ocelot_mrp_add(struct ocelot *ocelot, int port,
> > +                const struct switchdev_obj_mrp *mrp)
> > +{
> > +     struct ocelot_port *ocelot_port = ocelot->ports[port];
> > +     struct ocelot_port_private *priv;
> > +     struct net_device *dev;
> > +
> > +     if (!ocelot_port)
> > +             return -EOPNOTSUPP;
> > +
> > +     priv = container_of(ocelot_port, struct ocelot_port_private, port);
> > +     dev = priv->dev;
> 
> No, no, no.
> The struct net_device registered by DSA uses a netdev_priv of
> struct dsa_slave_priv. You can't just go ahead and assume that the
> caller of this function uses struct ocelot_port_private.
> 
> Please go to struct ocelot_port and add:
>         bool is_mrp_primary;
>         bool is_mrp_secondary;
> 
> and replace the checks for a net_device with bools.

My bad. I will create a new patch with your suggestion.

> 
> > +
> > +     if (mrp->p_port != dev && mrp->s_port != dev)
> > +             return 0;
> > +
> > +     if (ocelot->mrp_ring_id != 0 &&
> > +         ocelot->mrp_s_port &&
> > +         ocelot->mrp_p_port)
> > +             return -EINVAL;
> > +
> > +     if (mrp->p_port == dev)
> > +             ocelot->mrp_p_port = dev;
> > +
> > +     if (mrp->s_port == dev)
> > +             ocelot->mrp_s_port = dev;
> > +
> > +     ocelot->mrp_ring_id = mrp->ring_id;
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(ocelot_mrp_add);
> > +
> > +int ocelot_mrp_del(struct ocelot *ocelot, int port,
> > +                const struct switchdev_obj_mrp *mrp)
> > +{
> > +     struct ocelot_port *ocelot_port = ocelot->ports[port];
> > +     struct ocelot_port_private *priv;
> > +     struct net_device *dev;
> > +
> > +     if (!ocelot_port)
> > +             return -EOPNOTSUPP;
> > +
> > +     priv = container_of(ocelot_port, struct ocelot_port_private, port);
> > +     dev = priv->dev;
> > +
> > +     if (ocelot->mrp_p_port != dev && ocelot->mrp_s_port != dev)
> > +             return 0;
> > +
> > +     if (ocelot->mrp_ring_id == 0 &&
> > +         !ocelot->mrp_s_port &&
> > +         !ocelot->mrp_p_port)
> > +             return -EINVAL;
> > +
> > +     if (ocelot_mrp_del_vcap(ocelot, priv->chip_port))
> > +             return -EINVAL;
> > +
> > +     if (ocelot->mrp_p_port == dev)
> > +             ocelot->mrp_p_port = NULL;
> > +
> > +     if (ocelot->mrp_s_port == dev)
> > +             ocelot->mrp_s_port = NULL;
> > +
> > +     ocelot->mrp_ring_id = 0;
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(ocelot_mrp_del);
> > +
> > +int ocelot_mrp_add_ring_role(struct ocelot *ocelot, int port,
> > +                          const struct switchdev_obj_ring_role_mrp *mrp)
> > +{
> > +     struct ocelot_port *ocelot_port = ocelot->ports[port];
> > +     struct ocelot_vcap_filter *filter;
> > +     struct ocelot_port_private *priv;
> > +     struct net_device *dev;
> > +     int err;
> > +
> > +     if (!ocelot_port)
> > +             return -EOPNOTSUPP;
> > +
> > +     priv = container_of(ocelot_port, struct ocelot_port_private, port);
> > +     dev = priv->dev;
> > +
> > +     if (ocelot->mrp_ring_id != mrp->ring_id)
> > +             return -EINVAL;
> > +
> > +     if (!mrp->sw_backup)
> > +             return -EOPNOTSUPP;
> > +
> > +     if (ocelot->mrp_p_port != dev && ocelot->mrp_s_port != dev)
> > +             return 0;
> > +
> > +     filter = kzalloc(sizeof(*filter), GFP_ATOMIC);
> > +     if (!filter)
> > +             return -ENOMEM;
> > +
> > +     filter->key_type = OCELOT_VCAP_KEY_ETYPE;
> > +     filter->prio = 1;
> > +     filter->id.cookie = priv->chip_port;
> 
> You have "port" already. This is also wrong for the reason I stated above:
> no "priv" in the common library.
> 
> > +     filter->id.tc_offload = false;
> > +     filter->block_id = VCAP_IS2;
> > +     filter->type = OCELOT_VCAP_FILTER_OFFLOAD;
> > +     filter->ingress_port_mask = BIT(priv->chip_port);
> > +     *(__be16 *)filter->key.etype.etype.value = htons(ETH_P_MRP);
> > +     *(__be16 *)filter->key.etype.etype.mask = htons(0xffff);
> > +     filter->action.mask_mode = OCELOT_MASK_MODE_PERMIT_DENY;
> > +     filter->action.port_mask = 0x0;
> > +     filter->action.cpu_copy_ena = true;
> > +     filter->action.cpu_qu_num = OCELOT_MRP_CPUQ;
> > +
> > +     err = ocelot_vcap_filter_add(ocelot, filter, NULL);
> > +     if (err)
> > +             kfree(filter);
> > +
> > +     return err;
> > +}
> > +EXPORT_SYMBOL(ocelot_mrp_add_ring_role);
> > +
> > +int ocelot_mrp_del_ring_role(struct ocelot *ocelot, int port,
> > +                          const struct switchdev_obj_ring_role_mrp *mrp)
> > +{
> > +     struct ocelot_port *ocelot_port = ocelot->ports[port];
> > +     struct ocelot_port_private *priv;
> > +     struct net_device *dev;
> > +
> > +     if (!ocelot_port)
> > +             return -EOPNOTSUPP;
> > +
> > +     priv = container_of(ocelot_port, struct ocelot_port_private, port);
> > +     dev = priv->dev;
> > +
> > +     if (ocelot->mrp_ring_id != mrp->ring_id)
> > +             return -EINVAL;
> > +
> > +     if (!mrp->sw_backup)
> > +             return -EOPNOTSUPP;
> > +
> > +     if (ocelot->mrp_p_port != dev && ocelot->mrp_s_port != dev)
> > +             return 0;
> > +
> > +     return ocelot_mrp_del_vcap(ocelot, priv->chip_port);
> > +}
> > +EXPORT_SYMBOL(ocelot_mrp_del_ring_role);
> > diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> > index 6518262532f0..12cb6867a2d0 100644
> > --- a/drivers/net/ethernet/mscc/ocelot_net.c
> > +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> > @@ -1010,6 +1010,52 @@ static int ocelot_port_obj_del_mdb(struct net_device *dev,
> >       return ocelot_port_mdb_del(ocelot, port, mdb);
> >  }
> >
> > +static int ocelot_port_obj_mrp_add(struct net_device *dev,
> > +                                const struct switchdev_obj_mrp *mrp)
> > +{
> > +     struct ocelot_port_private *priv = netdev_priv(dev);
> > +     struct ocelot_port *ocelot_port = &priv->port;
> > +     struct ocelot *ocelot = ocelot_port->ocelot;
> > +     int port = priv->chip_port;
> > +
> > +     return ocelot_mrp_add(ocelot, port, mrp);
> > +}
> > +
> > +static int ocelot_port_obj_mrp_del(struct net_device *dev,
> > +                                const struct switchdev_obj_mrp *mrp)
> > +{
> > +     struct ocelot_port_private *priv = netdev_priv(dev);
> > +     struct ocelot_port *ocelot_port = &priv->port;
> > +     struct ocelot *ocelot = ocelot_port->ocelot;
> > +     int port = priv->chip_port;
> > +
> > +     return ocelot_mrp_del(ocelot, port, mrp);
> > +}
> > +
> > +static int
> > +ocelot_port_obj_mrp_add_ring_role(struct net_device *dev,
> > +                               const struct switchdev_obj_ring_role_mrp *mrp)
> > +{
> > +     struct ocelot_port_private *priv = netdev_priv(dev);
> > +     struct ocelot_port *ocelot_port = &priv->port;
> > +     struct ocelot *ocelot = ocelot_port->ocelot;
> > +     int port = priv->chip_port;
> > +
> > +     return ocelot_mrp_add_ring_role(ocelot, port, mrp);
> > +}
> > +
> > +static int
> > +ocelot_port_obj_mrp_del_ring_role(struct net_device *dev,
> > +                               const struct switchdev_obj_ring_role_mrp *mrp)
> > +{
> > +     struct ocelot_port_private *priv = netdev_priv(dev);
> > +     struct ocelot_port *ocelot_port = &priv->port;
> > +     struct ocelot *ocelot = ocelot_port->ocelot;
> > +     int port = priv->chip_port;
> > +
> > +     return ocelot_mrp_del_ring_role(ocelot, port, mrp);
> > +}
> > +
> >  static int ocelot_port_obj_add(struct net_device *dev,
> >                              const struct switchdev_obj *obj,
> >                              struct netlink_ext_ack *extack)
> > @@ -1024,6 +1070,13 @@ static int ocelot_port_obj_add(struct net_device *dev,
> >       case SWITCHDEV_OBJ_ID_PORT_MDB:
> >               ret = ocelot_port_obj_add_mdb(dev, SWITCHDEV_OBJ_PORT_MDB(obj));
> >               break;
> > +     case SWITCHDEV_OBJ_ID_MRP:
> > +             ret = ocelot_port_obj_mrp_add(dev, SWITCHDEV_OBJ_MRP(obj));
> > +             break;
> > +     case SWITCHDEV_OBJ_ID_RING_ROLE_MRP:
> > +             ret = ocelot_port_obj_mrp_add_ring_role(dev,
> > +                                                     SWITCHDEV_OBJ_RING_ROLE_MRP(obj));
> > +             break;
> >       default:
> >               return -EOPNOTSUPP;
> >       }
> > @@ -1044,6 +1097,13 @@ static int ocelot_port_obj_del(struct net_device *dev,
> >       case SWITCHDEV_OBJ_ID_PORT_MDB:
> >               ret = ocelot_port_obj_del_mdb(dev, SWITCHDEV_OBJ_PORT_MDB(obj));
> >               break;
> > +     case SWITCHDEV_OBJ_ID_MRP:
> > +             ret = ocelot_port_obj_mrp_del(dev, SWITCHDEV_OBJ_MRP(obj));
> > +             break;
> > +     case SWITCHDEV_OBJ_ID_RING_ROLE_MRP:
> > +             ret = ocelot_port_obj_mrp_del_ring_role(dev,
> > +                                                     SWITCHDEV_OBJ_RING_ROLE_MRP(obj));
> > +             break;
> >       default:
> >               return -EOPNOTSUPP;
> >       }
> > diff --git a/include/linux/dsa/ocelot.h b/include/linux/dsa/ocelot.h
> > index c6bc45ae5e03..4265f328681a 100644
> > --- a/include/linux/dsa/ocelot.h
> > +++ b/include/linux/dsa/ocelot.h
> > @@ -160,6 +160,11 @@ static inline void ocelot_xfh_get_src_port(void *extraction, u64 *src_port)
> >       packing(extraction, src_port, 46, 43, OCELOT_TAG_LEN, UNPACK, 0);
> >  }
> >
> > +static inline void ocelot_xfh_get_cpuq(void *extraction, u64 *cpuq)
> > +{
> > +     packing(extraction, cpuq, 28, 20, OCELOT_TAG_LEN, UNPACK, 0);
> > +}
> > +
> >  static inline void ocelot_xfh_get_qos_class(void *extraction, u64 *qos_class)
> >  {
> >       packing(extraction, qos_class, 19, 17, OCELOT_TAG_LEN, UNPACK, 0);
> > diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> > index 1f2d90976564..425ff29d9389 100644
> > --- a/include/soc/mscc/ocelot.h
> > +++ b/include/soc/mscc/ocelot.h
> > @@ -112,6 +112,8 @@
> >  #define REG_RESERVED_ADDR            0xffffffff
> >  #define REG_RESERVED(reg)            REG(reg, REG_RESERVED_ADDR)
> >
> > +#define OCELOT_MRP_CPUQ                      7
> > +
> >  enum ocelot_target {
> >       ANA = 1,
> >       QS,
> > @@ -677,6 +679,12 @@ struct ocelot {
> >       /* Protects the PTP clock */
> >       spinlock_t                      ptp_clock_lock;
> >       struct ptp_pin_desc             ptp_pins[OCELOT_PTP_PINS_NUM];
> > +
> > +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> > +     u16                             mrp_ring_id;
> > +     struct net_device               *mrp_p_port;
> > +     struct net_device               *mrp_s_port;
> > +#endif
> 
> I'd rather have this without the ifdeffery, doesn't seem too expensive
> to justify compiling it out. We have a 4K array of VLANs in struct
> ocelot, for god's sake.

I will update this in the next patch.

-- 
/Horatiu