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

STALE2030d

Revision v1 of 4 in this series.

7 messages, 2 authors, 2021-01-27 · open the first message on its own page

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

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: 2021-01-23 16:21:35

This patch series extends MRP switchdev to allow the SW to have a better
understanding if the HW can implment 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 determin 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.

Horatiu Vultur (4):
  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

 include/net/switchdev.h       |   2 +
 net/bridge/br_mrp.c           |  43 +++++---
 net/bridge/br_mrp_switchdev.c | 189 +++++++++++++++++++++++++---------
 net/bridge/br_private_mrp.h   |  38 +++++--
 4 files changed, 195 insertions(+), 77 deletions(-)

-- 
2.27.0

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

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: 2021-01-23 16:21:58

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, then 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 then 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 can 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. In
  case the driver returns 0 then the SW will continue to configure the
  node accordingly.

In this way is more clear when the SW needs to stop configuring the
node, or when the SW is used as a backup or the HW can implement the
functionality.

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

diff --git a/net/bridge/br_mrp_switchdev.c b/net/bridge/br_mrp_switchdev.c
index ed547e03ace1..e93a84532a4e 100644
--- a/net/bridge/br_mrp_switchdev.c
+++ b/net/bridge/br_mrp_switchdev.c

@@ -14,14 +14,12 @@ 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 (err && err != -EOPNOTSUPP)
-		return err;
+	/* If switchdev is not enabled then just run in SW */
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return 0;
 
-	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 +31,69 @@ 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 switchdev is not enabled then just run in SW */
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return 0;
 
-	if (err && err != -EOPNOTSUPP)
-		return err;
-
-	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,
 	};
 	int err;
 
+	/* If switchdev is not enabled then just run in SW */
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return BR_MRP_SW;
+
+	/* First try to see if HW can implement comptletly the role in HW */
 	if (role == BR_MRP_RING_ROLE_DISABLED)
 		err = switchdev_port_obj_del(br->dev, &mrp_role.obj);
 	else
 		err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);
 
-	return err;
+	/* In case of success then just return and notify the SW that doesn't
+	 * need to do anything
+	 */
+	if (!err)
+		return BR_MRP_HW;
+
+	/* There was some issue then is not possible at all to have this role so
+	 * just return failire
+	 */
+	if (err != -EOPNOTSUPP)
+		return BR_MRP_NONE;
+
+	/* In case the HW can't run complety in HW the protocol, we try again
+	 * and this time to allow the SW to help, but the HW needs to redirect
+	 * the frames to CPU.
+	 */
+	mrp_role.sw_backup = true;
+	err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);
+
+	/* In case of success then notify the SW that it needs to help with the
+	 * protocol
+	 */
+	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,

@@ -79,12 +106,29 @@ int br_mrp_switchdev_send_ring_test(struct net_bridge *br,
 	};
 	int err;
 
+	/* If switchdev is not enabled then just run in SW */
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return BR_MRP_SW;
+
 	if (interval == 0)
 		err = switchdev_port_obj_del(br->dev, &test.obj);
 	else
 		err = switchdev_port_obj_add(br->dev, &test.obj, NULL);
 
-	return err;
+	/* If everything succeed then the HW can send this frames, so the SW
+	 * doesn't need to generate them
+	 */
+	if (!err)
+		return BR_MRP_HW;
+
+	/* There was an error when the HW was configured so the SW return
+	 * failure
+	 */
+	if (err != -EOPNOTSUPP)
+		return BR_MRP_NONE;
+
+	/* So the HW can't generate these frames so allow the SW to do that */
+	return BR_MRP_SW;
 }
 
 int br_mrp_switchdev_set_ring_state(struct net_bridge *br,

@@ -97,19 +141,18 @@ 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 switchdev is not enabled then just run in SW */
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return 0;
 
-	if (err && err != -EOPNOTSUPP)
-		return err;
-
-	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 +161,46 @@ 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,
 	};
 	int err;
 
+	/* If switchdev is not enabled then just run in SW */
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return BR_MRP_SW;
+
+	/* First try to see if HW can implement comptletly the role in HW */
 	if (role == BR_MRP_IN_ROLE_DISABLED)
 		err = switchdev_port_obj_del(br->dev, &mrp_role.obj);
 	else
 		err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);
 
-	return err;
+	/* In case of success then just return and notify the SW that doesn't
+	 * need to do anything
+	 */
+	if (!err)
+		return BR_MRP_HW;
+
+	/* There was some issue then is not possible at all to have this role so
+	 * just return failire
+	 */
+	if (err != -EOPNOTSUPP)
+		return BR_MRP_NONE;
+
+	/* In case the HW can't run complety in HW the protocol, we try again
+	 * and this time to allow the SW to help, but the HW needs to redirect
+	 * the frames to CPU.
+	 */
+	mrp_role.sw_backup = true;
+	err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);
+
+	/* In case of success then notify the SW that it needs to help with the
+	 * protocol
+	 */
+	if (!err)
+		return BR_MRP_SW;
+
+	return BR_MRP_NONE;
 }
 
 int br_mrp_switchdev_set_in_state(struct net_bridge *br, struct br_mrp *mrp,

@@ -138,18 +212,17 @@ 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 switchdev is not enabled then just run in SW */
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return 0;
 
-	if (err && err != -EOPNOTSUPP)
-		return err;
-
-	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,

@@ -161,12 +234,29 @@ int br_mrp_switchdev_send_in_test(struct net_bridge *br, struct br_mrp *mrp,
 	};
 	int err;
 
+	/* If switchdev is not enabled then just run in SW */
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return BR_MRP_SW;
+
 	if (interval == 0)
 		err = switchdev_port_obj_del(br->dev, &test.obj);
 	else
 		err = switchdev_port_obj_add(br->dev, &test.obj, NULL);
 
-	return err;
+	/* If everything succeed then the HW can send this frames, so the SW
+	 * doesn't need to generate them
+	 */
+	if (!err)
+		return BR_MRP_HW;
+
+	/* There was an error when the HW was configured so the SW return
+	 * failure
+	 */
+	if (err != -EOPNOTSUPP)
+		return BR_MRP_NONE;
+
+	/* So the HW can't generate these frames so allow the SW to do that */
+	return BR_MRP_SW;
 }
 
 int br_mrp_port_switchdev_set_state(struct net_bridge_port *p,

@@ -177,14 +267,12 @@ int br_mrp_port_switchdev_set_state(struct net_bridge_port *p,
 		.id = SWITCHDEV_ATTR_ID_MRP_PORT_STATE,
 		.u.mrp_port_state = state,
 	};
-	int err;
 
-	err = switchdev_port_attr_set(p->dev, &attr);
-	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 switchdev is not enabled then just run in SW */
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return 0;
 
-	return err;
+	return switchdev_port_attr_set(p->dev, &attr);
 }
 
 int br_mrp_port_switchdev_set_role(struct net_bridge_port *p,

@@ -195,11 +283,10 @@ 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);
-	if (err && err != -EOPNOTSUPP)
-		return err;
+	/* If switchdev is not enabled then just run in SW */
+	if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+		return 0;
 
-	return 0;
+	return switchdev_port_attr_set(p->dev, &attr);
 }

diff --git a/net/bridge/br_private_mrp.h b/net/bridge/br_private_mrp.h
index 31e666ae6955..e941dad398cf 100644
--- a/net/bridge/br_private_mrp.h
+++ b/net/bridge/br_private_mrp.h

@@ -79,24 +79,28 @@ 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,
 				    enum br_mrp_port_state_type 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 4/4] bridge: mrp: Update br_mrp to use new return values of br_mrp_switchdev

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: 2021-01-23 16:22:50

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 implemtation
- BR_MRP_HW, continue without SW implemtation.

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 fc0a98874bfc..faa4ccb20f0a 100644
--- a/net/bridge/br_mrp.c
+++ b/net/bridge/br_mrp.c

@@ -636,7 +636,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;

@@ -644,9 +644,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

@@ -654,7 +654,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;
 }

@@ -667,6 +667,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;

@@ -674,9 +675,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;

@@ -718,8 +723,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;

@@ -777,10 +782,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

@@ -788,7 +793,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;
 }

@@ -801,6 +806,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;

@@ -811,8 +817,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 2/4] bridge: mrp: Add 'enum br_mrp_hw_support'

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: 2021-01-23 16:22:50

Add the enum br_mrp_hw_support that is used by the br_mrp_switchdev
functions to allow the SW to detect better the cases where the HW can't
implement this or when the 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 1883118aae55..31e666ae6955 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 1/4] switchdev: mrp: Extend ring_role_mrp and in_role_mrp

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: 2021-01-23 16:23:57

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 88fcac140966..3f236eaa4f3e 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h

@@ -132,6 +132,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) \

@@ -166,6 +167,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

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

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2021-01-26 00:08:03

On Sat, Jan 23, 2021 at 11:23 AM Horatiu Vultur
[off-list ref] 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, then 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 then 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 can 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. In
  case the driver returns 0 then the SW will continue to configure the
  node accordingly.

In this way is more clear when the SW needs to stop configuring the
node, or when the SW is used as a backup or the HW can implement the
functionality.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

-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,
        };
        int err;

+       /* If switchdev is not enabled then just run in SW */
+       if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+               return BR_MRP_SW;
+
+       /* First try to see if HW can implement comptletly the role in HW */

typo: completely

        if (role == BR_MRP_RING_ROLE_DISABLED)
                err = switchdev_port_obj_del(br->dev, &mrp_role.obj);
        else
                err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);

-       return err;
+       /* In case of success then just return and notify the SW that doesn't
+        * need to do anything
+        */
+       if (!err)
+               return BR_MRP_HW;
+
+       /* There was some issue then is not possible at all to have this role so
+        * just return failire

typo: failure

+        */
+       if (err != -EOPNOTSUPP)
+               return BR_MRP_NONE;
+
+       /* In case the HW can't run complety in HW the protocol, we try again

typo: completely. Please proofread your comments closely. I saw at
least one typo in the commit messages too.

More in general comments that say what the code does can generally be eschewed.

+        * and this time to allow the SW to help, but the HW needs to redirect
+        * the frames to CPU.
+        */
+       mrp_role.sw_backup = true;
+       err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);

This calls the same function. I did not see code that changes behavior
based on sw_backup. Will this not give the same result?

Also, this lacks the role test (add or del). Is that because if
falling back onto SW mode during add, this code does not get called at
all on delete?

quoted hunk

+
+       /* In case of success then notify the SW that it needs to help with the
+        * protocol
+        */
+       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,

@@ -79,12 +106,29 @@ int br_mrp_switchdev_send_ring_test(struct net_bridge *br,
        };
        int err;

+       /* If switchdev is not enabled then just run in SW */
+       if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+               return BR_MRP_SW;
+
        if (interval == 0)
                err = switchdev_port_obj_del(br->dev, &test.obj);
        else
                err = switchdev_port_obj_add(br->dev, &test.obj, NULL);

-       return err;
+       /* If everything succeed then the HW can send this frames, so the SW
+        * doesn't need to generate them
+        */
+       if (!err)
+               return BR_MRP_HW;
+
+       /* There was an error when the HW was configured so the SW return
+        * failure
+        */
+       if (err != -EOPNOTSUPP)
+               return BR_MRP_NONE;
+
+       /* So the HW can't generate these frames so allow the SW to do that */
+       return BR_MRP_SW;

This is the same ternary test as above. It recurs a few times. Perhaps
it can use a helper.

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

From: Horatiu Vultur <horatiu.vultur@microchip.com>
Date: 2021-01-27 03:18:15

The 01/25/2021 19:06, Willem de Bruijn wrote:

On Sat, Jan 23, 2021 at 11:23 AM Horatiu Vultur
[off-list ref] wrote:

Hi Willem,

quoted

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, then 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 then 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 can 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. In
  case the driver returns 0 then the SW will continue to configure the
  node accordingly.

In this way is more clear when the SW needs to stop configuring the
node, or when the SW is used as a backup or the HW can implement the
functionality.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

quoted

-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,
        };
        int err;

+       /* If switchdev is not enabled then just run in SW */
+       if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+               return BR_MRP_SW;
+
+       /* First try to see if HW can implement comptletly the role in HW */

typo: completely

quoted

        if (role == BR_MRP_RING_ROLE_DISABLED)
                err = switchdev_port_obj_del(br->dev, &mrp_role.obj);
        else
                err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);

-       return err;
+       /* In case of success then just return and notify the SW that doesn't
+        * need to do anything
+        */
+       if (!err)
+               return BR_MRP_HW;
+
+       /* There was some issue then is not possible at all to have this role so
+        * just return failire

typo: failure

quoted

+        */
+       if (err != -EOPNOTSUPP)
+               return BR_MRP_NONE;
+
+       /* In case the HW can't run complety in HW the protocol, we try again

typo: completely. Please proofread your comments closely. I saw at
least one typo in the commit messages too.

Sorry for that. I will fix those in the next version.

More in general comments that say what the code does can generally be eschewed.

quoted

+        * and this time to allow the SW to help, but the HW needs to redirect
+        * the frames to CPU.
+        */
+       mrp_role.sw_backup = true;
+       err = switchdev_port_obj_add(br->dev, &mrp_role.obj, NULL);

This calls the same function. I did not see code that changes behavior
based on sw_backup. Will this not give the same result?

No, because is the driver responsibility to check that flag and
implement what it can support. If the sw_backup is false, then it is
expected the driver to implement completely the functionality in HW. If
sw_backup is true it just needs to help the SW to run. So the driver can
check this flag and decide what to do.

Also, this lacks the role test (add or del). Is that because if
falling back onto SW mode during add, this code does not get called at
all on delete?

Good catch!. It should have the check.

quoted

+
+       /* In case of success then notify the SW that it needs to help with the
+        * protocol
+        */
+       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,

@@ -79,12 +106,29 @@ int br_mrp_switchdev_send_ring_test(struct net_bridge *br,
        };
        int err;

+       /* If switchdev is not enabled then just run in SW */
+       if (!IS_ENABLED(CONFIG_NET_SWITCHDEV))
+               return BR_MRP_SW;
+
        if (interval == 0)
                err = switchdev_port_obj_del(br->dev, &test.obj);
        else
                err = switchdev_port_obj_add(br->dev, &test.obj, NULL);

-       return err;
+       /* If everything succeed then the HW can send this frames, so the SW
+        * doesn't need to generate them
+        */
+       if (!err)
+               return BR_MRP_HW;
+
+       /* There was an error when the HW was configured so the SW return
+        * failure
+        */
+       if (err != -EOPNOTSUPP)
+               return BR_MRP_NONE;
+
+       /* So the HW can't generate these frames so allow the SW to do that */
+       return BR_MRP_SW;

This is the same ternary test as above. It recurs a few times. Perhaps
it can use a helper.

Yes, I will try to have a look.

-- 
/Horatiu

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help