[RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[Horatiu Vultur]

Media Redundancy Protocol is a data network protocol standardized by
International Electrotechnical Commission as IEC 62439-2. It allows rings of
Ethernet switches to overcome any single failure with recovery time faster than
STP. It is primarily used in Industrial Ethernet applications.

Based on the previous RFC[1][2], the MRP state machine and all the
timers were moved to userspace. A generic netlink interface is added to
allow configuring the HW, and logic added to to implement the MRP
specific forwarding rules.

The userspace application that is using the new netlink can be found here[3].

The current implementation both in kernel and userspace supports only 2 roles:

  MRM - this one is responsible to send MRP_Test and MRP_Topo frames on both
  ring ports. It needs to process MRP_Test to know if the ring is open or
  closed. This operation is desired to be offloaded to the HW because it
  requires to generate and process up to 4000 frames per second. Whenever it
  detects that the ring open it sends MRP_Topo frames to notify all MRC about
  changes in the topology. MRM needs also to process MRP_LinkChange frames,
  these frames are generated by the MRC. When the ring is open the the state
  of both ports is to forward frames and when the ring is closed then the
  secondary port is blocked.

  MRC - this one is responsible to forward MRP frames between the ring ports.
  In case one of the ring ports gets a link down or up, then MRC will generate
  a MRP_LinkChange frames. This node should also process MRP_Topo frames and to
  clear its FDB when it receives this frame.

 Userspace
               Deamon +----------+ Client
                +
                |
 +--------------|-----------------------------------------+
  Kernel        |
                + Netlink

                |                              + Interrupt
                |                              |
 +--------------|------------------------------|----------+
  HW            | Switchdev                    |
                +                              |

The user interacts using the client (called 'mrp'), the client talks to the
deamon (called 'mrp_server'), which talks with the kernel using netlink. The
kernel will try to offload the requests to the HW via switchdev API. For this a
new generic netlink interface was added to the bridge.

If the kernel cannot offload MRP to HW (maybe it does not have a switchdev
driver, or it is just not supported), then all the netlink calls will return
-EOPNOTSUPP. In this case the user-space deamon fallback to SW only
implementation.

There are required changes to the SW bridge to be able to run the MRP. First the
bridge needs to initialize the netlink interface. And second it needs to know if
a MRP frame was received on a MRP ring port. In case it was received the SW
bridge should not forward the frame it needs to redirected to upper layes. In
case it was not received on a ring port then it just forwards it as usual.

To be able to offload this to the HW, it was required to extend the switchdev
API.

If this will be accepted then in the future the netlink interface can be
expended with multiple attributes which are required by different roles of the
MRP. Like Media Redundancy Automanager(MRA), Media Interconnect Manager(MIM) and
Media Interconnect Client(MIC).

[1] https://www.spinics.net/lists/netdev/msg623647.html
[2] https://www.spinics.net/lists/netdev/msg624378.html
[3] https://github.com/microchip-ung/mrp/tree/patch-v3

Horatiu Vultur (10):
  net: bridge: mrp: Expose mrp attributes.
  net: bridge: mrp: Expose function br_mrp_port_open
  net: bridge: mrp: Add MRP interface used by netlink
  net: bridge: mrp: Add generic netlink interface to configure MRP
  net: bridge: mrp: Update MRP interface to add switchdev support
  net: bridge: mrp: switchdev: Extend switchdev API to offload MRP
  net: bridge: mrp: switchdev: Implement MRP API for switchdev
  net: bridge: mrp: Connect MRP api with the switchev API
  net: bridge: mrp: Integrate MRP into the bridge
  net: bridge: mrp: Update Kconfig and Makefile

 include/linux/mrp_bridge.h      |  25 ++
 include/net/switchdev.h         |  51 +++
 include/uapi/linux/if_ether.h   |   1 +
 include/uapi/linux/mrp_bridge.h | 118 ++++++
 net/bridge/Kconfig              |  12 +
 net/bridge/Makefile             |   2 +
 net/bridge/br.c                 |  11 +
 net/bridge/br_device.c          |   3 +
 net/bridge/br_if.c              |   6 +
 net/bridge/br_input.c           |  14 +
 net/bridge/br_mrp.c             | 193 ++++++++++
 net/bridge/br_mrp_netlink.c     | 655 ++++++++++++++++++++++++++++++++
 net/bridge/br_mrp_switchdev.c   | 147 +++++++
 net/bridge/br_private.h         |  14 +
 net/bridge/br_private_mrp.h     |  58 +++
 15 files changed, 1310 insertions(+)
 create mode 100644 include/linux/mrp_bridge.h
 create mode 100644 include/uapi/linux/mrp_bridge.h
 create mode 100644 net/bridge/br_mrp.c
 create mode 100644 net/bridge/br_mrp_netlink.c
 create mode 100644 net/bridge/br_mrp_switchdev.c
 create mode 100644 net/bridge/br_private_mrp.h

-- 
2.17.1

[RFC net-next v3 01/10] net: bridge: mrp: Expose mrp attributes.

[Horatiu Vultur]

Expose the attributes and the commands used by the generic netlink interface to
configure the kernel from userspace. The following commands are available:

BR_MRP_GENL_ADD - creates an MRP instance. This doesn't have yet any MRP
  ports attach to it.

BR_MRP_GENL_ADD_PORT - adds a port to an MRP instance. A port can't be part
  of multiple MRP instances.

BR_MRP_GENL_DEL - deletes an MRP instance. By deleting the instance also the
  MRP ports are removed.

BR_MRP_GENL_DEL_PORT - removes a port from an MRP instance. When the port is
  removed then the state machine of the MRP instance is reset because there is
  no point in having only 1 port in a MRP ring.

BR_MRP_GENL_SET_PORT_STATE - sets the port state. The port state can be
  forwarding, which means that the port will forward frames and blocked, which
  will block non-MRP frames.

BR_MRP_GENL_SET_PORT_ROLE - sets the port role, it can be primary or secondary.

BR_MRP_GENL_SET_RING_STATE - sets the ring state. The ring state can be closed
  or open. This information is needed in the MRP_Test frames

BR_MRP_GENL_SET_RING_ROLE - sets the ring role. The supported roles are MRM
  and MRC. A node can have MRM role only if, it can terminate MRP frames,
  redirect MRP frames(except MRP_Test) to the CPU and can notify that it
  stopped receiving frames. A node can have MRC role only if it can copy
  MRP_Topology frames and forward frames between ring ports.

BR_MRP_GENL_SET_START_TEST - this command is used to notify the kernel to start
  to generate MRP frames. A value of 0 for the attribute
  BR_MRP_ATTR_TEST_INTERVAL means that it should stop to generate frames.

BR_MRP_GENL_FLUSH - this command is used to flush the fdb.

BR_MRP_GENL_RING_OPEN - it is used by the kernel to notify userspace
  applications that one of the MRP ring ports stopped receiving MRP_Test frames

The calls to BR_MRP_GENL_SET_PORT_ROLE and BR_MRP_GENL_SET_RING_STATE are
required in case the driver can generate MRP_Test frames because these frames
contains this information. And there is no other way for the HW to get it.

MRP_Test frames contain also the number of times a ring went to open state.
There is no command for that because the driver can count this by counting the
number of times the ring state is set to open.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 include/uapi/linux/if_ether.h   |   1 +
 include/uapi/linux/mrp_bridge.h | 118 ++++++++++++++++++++++++++++++++
 2 files changed, 119 insertions(+)
 create mode 100644 include/uapi/linux/mrp_bridge.h

diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
index f6ceb2e63d1e..d6de2b167448 100644
--- a/include/uapi/linux/if_ether.h
+++ b/include/uapi/linux/if_ether.h
@@ -92,6 +92,7 @@
 #define ETH_P_PREAUTH	0x88C7		/* 802.11 Preauthentication */
 #define ETH_P_TIPC	0x88CA		/* TIPC 			*/
 #define ETH_P_LLDP	0x88CC		/* Link Layer Discovery Protocol */
+#define ETH_P_MRP	0x88E3		/* Media Redundancy Protocol	*/
 #define ETH_P_MACSEC	0x88E5		/* 802.1ae MACsec */
 #define ETH_P_8021AH	0x88E7          /* 802.1ah Backbone Service Tag */
 #define ETH_P_MVRP	0x88F5          /* 802.1Q MVRP                  */
diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h
new file mode 100644
index 000000000000..c5a3d0a2b400
--- /dev/null
+++ b/include/uapi/linux/mrp_bridge.h
@@ -0,0 +1,118 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+
+#ifndef _UAPI_LINUX_MRP_BRIDGE_H
+#define _UAPI_LINUX_MRP_BRIDGE_H
+
+/* These are the attributes used by the generic netlink interface for MRP
+ * BR_MRP_ATTR_BR_IFINDEX - must be the ifindex of the bridge
+ * BR_MRP_ATTR_PORT_IFINDEX - represents the index of one of the ports under the
+ *			      bridge, this is used in both cases to set the
+ *			      primary and secondary port.
+ * BR_MRP_ATTR_RING_NR - represents the ID of each MRP instance. On a bridge can
+ *			 multiple MRP instances.
+ * BR_MRP_ATTR_RING_ROLE - represents the role of the MRP instance. This
+ *			   attribute corresponds to the enum
+ *			   br_mrp_ring_role_type.
+ * BR_MRP_ATTR_RING_STATE - represents the state of the MRP instance. This
+ *			    attribute corresponds to the enum
+ *			    br_mrp_ring_state_type.
+ * BR_MRP_ATTR_PORT_STATE - represents the state of the MRP port. Add means if
+ *			    the port is allowed to forward or not non-MRP
+ *			    frames. It corresponds to enum
+ *			    br_mrp_port_state_type.
+ * BR_MRP_ATTR_PORT_ROLE - represents the role of the MRP port. It corresponds
+ *			   to enum br_mrp_port_role_type.
+ * BR_MRP_ATTR_TEST_INTERVAL - represents the interval of the generated MRP_Test
+ *			       frames. The value is in us.
+ * BR_MRP_ATTR_TEST_MAX_MISS - represents the number of MRP_Test frames that the
+ *			       port can miss before it notifies that the ring
+ *			       is open.
+ * BR_MRP_ATTR_RING_OPEN - represents the Loss of continuity of a port. A value
+ *			   of 1 means that the port stopped receiving MRP_Test
+ *			   while a value of 0 means that the port started to
+ *			   receive MRP_Test frames
+ *
+ * Kernel needs to know about the attributes BR_MRP_ATTR_RING_STATE and
+ * BR_MRP_ATTR_PORT_ROLE because in case it can offload the generation of
+ * MRP_Test frames to HW then the HW needs to know these attributes to be able
+ * to generate correctly MRP_Test frames.
+ */
+enum br_mrp_attr {
+	BR_MRP_ATTR_NONE,
+	BR_MRP_ATTR_BR_IFINDEX,
+	BR_MRP_ATTR_PORT_IFINDEX,
+	BR_MRP_ATTR_RING_NR,
+	BR_MRP_ATTR_RING_ROLE,
+	BR_MRP_ATTR_RING_STATE,
+	BR_MRP_ATTR_PORT_STATE,
+	BR_MRP_ATTR_PORT_ROLE,
+	BR_MRP_ATTR_TEST_INTERVAL,
+	BR_MRP_ATTR_TEST_MAX_MISS,
+	BR_MRP_ATTR_RING_OPEN,
+
+	/* This must be the last entry */
+	BR_MRP_ATTR_END,
+};
+
+#define BR_MRP_ATTR_MAX		(BR_MRP_ATTR_END - 1)
+
+/* These are the commands used by the generic netlink interface for MRP
+ * BR_MRP_GENL_ADD - creates an MRP instance. This doesn't have yet any MRP
+ *		     ports attach to it.
+ * BR_MRP_GENL_ADD_PORT - adds a port to an MRP instance. A port can't be part
+ *			  of multiple MRP instances.
+ * BR_MRP_GENL_DEL - deletes an MRP instance. By deleting the instance also the
+ *		     MRP ports are removed.
+ * BR_MRP_GENL_DEL_PORT - removes a port from an MRP instance.
+ * BR_MRP_GENL_SET_PORT_STATE - sets the port state.
+ * BR_MRP_GENL_SET_PORT_ROLE - sets the port role.
+ * BR_MRP_GENL_SET_RING_STATE - sets the ring state.
+ * BR_MRP_GENL_SET_RING_ROLE - sets the ring role.
+ * BR_MRP_GENL_SET_START_TEST - this command is used to notify the Kernel to
+ *				start to generate MRP frames. A value of 0 for
+ *				the attribute BR_MRP_ATTR_TEST_INTERVAL means
+ *				that it should stop to generate frames.
+ * BR_MRP_GENL_FLUSH - this command is used to flush the fdb.
+ * BR_MRP_GENL_RING_OPEN - it is used by the kernel to notify userspace
+ * 			   applications that one of the MRP ring ports stopped
+ * 			   receiving MRP_Test frames
+ */
+enum br_mrp_genl {
+	BR_MRP_GENL_ADD,
+	BR_MRP_GENL_ADD_PORT,
+	BR_MRP_GENL_DEL,
+	BR_MRP_GENL_DEL_PORT,
+	BR_MRP_GENL_SET_PORT_STATE,
+	BR_MRP_GENL_SET_PORT_ROLE,
+	BR_MRP_GENL_SET_RING_STATE,
+	BR_MRP_GENL_SET_RING_ROLE,
+	BR_MRP_GENL_START_TEST,
+	BR_MRP_GENL_FLUSH,
+	BR_MRP_GENL_RING_OPEN,
+};
+
+enum br_mrp_ring_role_type {
+	BR_MRP_RING_ROLE_DISABLED,
+	BR_MRP_RING_ROLE_MRC,
+	BR_MRP_RING_ROLE_MRM,
+};
+
+enum br_mrp_ring_state_type {
+	BR_MRP_RING_STATE_OPEN,
+	BR_MRP_RING_STATE_CLOSED,
+};
+
+enum br_mrp_port_state_type {
+	BR_MRP_PORT_STATE_DISABLED,
+	BR_MRP_PORT_STATE_BLOCKED,
+	BR_MRP_PORT_STATE_FORWARDING,
+	BR_MRP_PORT_STATE_NOT_CONNECTED,
+};
+
+enum br_mrp_port_role_type {
+	BR_MRP_PORT_ROLE_PRIMARY,
+	BR_MRP_PORT_ROLE_SECONDARY,
+	BR_MRP_PORT_ROLE_NONE,
+};
+
+#endif
-- 
2.17.1

[RFC net-next v3 02/10] net: bridge: mrp: Expose function br_mrp_port_open

[Horatiu Vultur]

In case the HW is capable to detect when the MRP ring is open or closed. It is
expected that the network driver will notify the bridge that the ring is open or
closed.

The function br_mrp_port_open is used to notify the kernel that one of the ports
stopped receiving MRP_Test frames. The argument 'loc' has a value of '1' when
the port stopped receiving MRP_Test and '0' when it started to receive MRP_Test.

This notification will eventually reach the userspace via netlink interface and
the userspace application will process this information.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 include/linux/mrp_bridge.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 include/linux/mrp_bridge.h

diff --git a/include/linux/mrp_bridge.h b/include/linux/mrp_bridge.h
new file mode 100644
index 000000000000..0508bbe6ecdb
--- /dev/null
+++ b/include/linux/mrp_bridge.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _LINUX_MRP_BRIDGE_H
+#define _LINUX_MRO_BRIDGE_H
+
+#include <linux/netdevice.h>
+
+/* The drivers are responsible to call this function when it detects that the
+ * MRP port stopped receiving MRP_Test frames or it started to receive MRP_Test.
+ * The argument dev represents the port and loc(Lost of Continuity) has a value
+ * of 1 when it stopped receiving MRP_Test frames and a value of 0 when it
+ * started to receive frames.
+ *
+ * This eventually eventually notify the userspace which is required to react on
+ * thise changes.
+ */
+
+#ifdef CONFIG_BRIDGE_MRP
+void br_mrp_port_open(struct net_device *dev, u8 loc);
+#else
+inline void br_mrp_port_open(struct net_device *dev, u8 loc)  {}
+#endif
+
+#endif
+
-- 
2.17.1

Re: [RFC net-next v3 02/10] net: bridge: mrp: Expose function br_mrp_port_open

[Andrew Lunn]

On Fri, Jan 24, 2020 at 05:18:20PM +0100, Horatiu Vultur wrote:
> In case the HW is capable to detect when the MRP ring is open or closed. It is
> expected that the network driver will notify the bridge that the ring is open or
> closed.
> 
> The function br_mrp_port_open is used to notify the kernel that one of the ports
> stopped receiving MRP_Test frames. The argument 'loc' has a value of '1' when
> the port stopped receiving MRP_Test and '0' when it started to receive MRP_Test.

Hi Horatiu

Given the name of the function, br_mrp_port_open(), how about replacing
loc with a bool with the name open?

    Andrew

Re: [RFC net-next v3 02/10] net: bridge: mrp: Expose function br_mrp_port_open

[Horatiu Vultur]

The 01/24/2020 18:37, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Jan 24, 2020 at 05:18:20PM +0100, Horatiu Vultur wrote:
> > In case the HW is capable to detect when the MRP ring is open or closed. It is
> > expected that the network driver will notify the bridge that the ring is open or
> > closed.
> >
> > The function br_mrp_port_open is used to notify the kernel that one of the ports
> > stopped receiving MRP_Test frames. The argument 'loc' has a value of '1' when
> > the port stopped receiving MRP_Test and '0' when it started to receive MRP_Test.
> 
> Hi Horatiu
> 
> Given the name of the function, br_mrp_port_open(), how about replacing
> loc with a bool with the name open?

Hi Andrew,

Well spotted, yes I will replace this in the next series.

> 
>     Andrew

-- 
/Horatiu

[RFC net-next v3 03/10] net: bridge: mrp: Add MRP interface used by netlink

[Horatiu Vultur]

Define the MRP interface that will be used by the generic netlink to offload the
calls to the HW.

For this it is required for the kernel to hold in a list all the MRP instances
that are created and all the ports that are part of the MRP rings. Therefore add
the structure 'br_mrp'. This contains the following:
- a list with all MRP instances
- pointer to the net bridge on which the MRP instance is attach to
- pointers to the net bridge ports, which represents the ring ports
- a ring nr which represents the ID of the ring.

The interface has the following functions:

br_mrp_add - it creates a br_mrp instances and adds it to the list of mrp
  instances.
br_mrp_del - it removes a br_mrp instances from the list based on the ring nr of
  the instance.
These functions are used just by the SW to know which rings and which ports are
to which ring. These function will not call eventually the switchdev API. If
there is a HW required to know about these calls then the switchdev API can be
extended.

br_mrp_add_port - adds a port to a MRP instance. This will eventually call the
  switchdev API to notify the HW that the port is part of a ring so it needs to
  process or forward MRP frames on the other port.
br_mrp_del_port - deletes a port from a MRP instance. This will eventually call
  switchdev API to notify the HW that the port is not part of a ring anymore. So
  it would not need to do special processing to MRP frames
Whenever a port is added to the MRP instance, the also the SW needs to know this
information in case the HW can't support MRP. This information is required when
the SW bridge receives MRP frames. Because in case a frame arrived on an MRP
port the SW bridge should not forward the frame.

br_mrp_port_state - changes the port state. The port can be in forwarding state,
  which means that the frames can pass through or in blocked state which means
  that the frames can't pass through except MRP frames. This will eventually
  call the switchdev API to notify the HW. This information is used also by the
  SW bridge to know how to forward frames in case the HW doesn't have this
  capability.

br_mrp_port_role - a port role can be primary or secondary. This information is
  required to be pushed to HW in case the HW can generate MRP_Test frames.
  Because the MRP_Test frames contains a file with this information. Otherwise
  the HW will not be able to generate the frames correctly.

br_mrp_ring_state - a ring can be in state open or closed. State open means that
  the mrp port stopped receiving MRP_Test frames, while closed means that the
  mrp port received MRP_Test frames. Similar with br_mrp_port_role, this
  information is pushed in HW because the MRP_Test frames contain this
  information.

For all the previous commands the userspace doesn't need to check the return
value because it is not affected if the HW supports these or not.

br_mrp_ring_role - a ring can have the following roles MRM or MRC. For the role
  MRM it is expected that the HW can terminate the MRP frames, notify the SW
  that it stopped receiving MRP_Test frames and trapp all the other MRP frames.
  While for MRC mode it is expected that the HW can forward the MRP frames only
  between the MRP ports and copy MRP_Topology frames to CPU. In case the HW
  doesn't support a role it needs to return an error code different than
  -EOPNOTSUPP.

Because the userspace doesn't know if the kernel has HW offload capabilities
it is using the return value of the netlink calls to know if there was a problem
setting the role to the HW, or it should run the role in userspace. For example
if the node doesn't have a switchdev driver than the return code is -EOPNOTSUPP
that means that the state machine can run only in SW. If the node has switchdev
support then, if the node doesn't support the role it needs to return an error
code different than -EOPNOTSUPP. In this case the entire userspace application
will stop. If the node support the role then it returns 0.

br_mrp_start_test - this starts/stops the generation of MRP_Test frames. To stop
  the generation of frames the interval needs to have a value of 0. In this case
  the userspace needs to know if the HW supports this or not. Not to have
  duplicate frames(generated by HW and SW). Because if the HW supports this then
  the SW will not generate anymore frames and will expect that the HW will
  notify when it stopped receiving MRP frames using the function
  br_mrp_port_open.

br_mrp_flush - will flush the FDB.

br_mrp_port_open - this function is used by drivers to notify the userspace via
  a netlink callback that one of the ports stopped receiving MRP_Test frames.
  This function is called only when the node has the role MRM. It is not
  supposed to be called from userspace.

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

diff --git a/net/bridge/br_private_mrp.h b/net/bridge/br_private_mrp.h
new file mode 100644
index 000000000000..bea4ece4411c
--- /dev/null
+++ b/net/bridge/br_private_mrp.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _BR_PRIVATE_MRP_H_
+#define _BR_PRIVATE_MRP_H_
+
+#include "br_private.h"
+#include <uapi/linux/mrp_bridge.h>
+
+struct br_mrp {
+	/* list of mrp instances */
+	struct list_head		list;
+
+	struct net_bridge		*br;
+	struct net_bridge_port		*p_port;
+	struct net_bridge_port		*s_port;
+
+	u32				ring_nr;
+};
+
+/* br_mrp.c */
+int br_mrp_add(struct net_bridge *br, u32 ring_nr);
+int br_mrp_add_port(struct net_bridge *br, u32 ring_nr,
+		    struct net_bridge_port *p);
+int br_mrp_del(struct net_bridge *br, u32 ring_nr);
+int br_mrp_del_port(struct net_bridge_port *p);
+int br_mrp_set_port_state(struct net_bridge_port *p,
+			  enum br_mrp_port_state_type state);
+int br_mrp_set_port_role(struct net_bridge_port *p, u32 ring_nr,
+			 enum br_mrp_port_role_type role);
+int br_mrp_set_ring_state(struct net_bridge *br, u32 ring_nr,
+			  enum br_mrp_ring_state_type state);
+int br_mrp_set_ring_role(struct net_bridge *br, u32 ring_nr,
+			 enum br_mrp_ring_role_type role);
+int br_mrp_start_test(struct net_bridge *br, u32 ring_nr, u32 interval,
+		      u8 max_miss);
+int br_mrp_flush(struct net_bridge *br, u32 ring_nr);
+
+/* br_mrp_netlink.c */
+void br_mrp_port_open(struct net_device *dev, u8 loc);
+
+#endif /* _BR_PRIVATE_MRP_H */
+
-- 
2.17.1

Re: [RFC net-next v3 03/10] net: bridge: mrp: Add MRP interface used by netlink

[Andrew Lunn]

> br_mrp_flush - will flush the FDB.

How does this differ from a normal bridge flush? I assume there is a
way for user space to flush the bridge FDB.

    Andrew

Re: [RFC net-next v3 03/10] net: bridge: mrp: Add MRP interface used by netlink

[Horatiu Vultur]

The 01/24/2020 18:43, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > br_mrp_flush - will flush the FDB.
> 
> How does this differ from a normal bridge flush? I assume there is a
> way for user space to flush the bridge FDB.

Hi,

If I seen corectly the normal bridge flush will clear the entire FDB for
all the ports of the bridge. In this case it is require to clear FDB
entries only for the ring ports. In the next series I will add a better
description of this function and update also the implementation.

The user space doesn't know and doesn't contain a FDB. The user space
will just call the kernel(via netlink interface) to clear the FDB. And
the netlink call will eventually call this function.

> 
>     Andrew

-- 
/Horatiu

Re: [RFC net-next v3 03/10] net: bridge: mrp: Add MRP interface used by netlink

[Andrew Lunn]

On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
> The 01/24/2020 18:43, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > > br_mrp_flush - will flush the FDB.
> > 
> > How does this differ from a normal bridge flush? I assume there is a
> > way for user space to flush the bridge FDB.
> 
> Hi,
> 
> If I seen corectly the normal bridge flush will clear the entire FDB for
> all the ports of the bridge. In this case it is require to clear FDB
> entries only for the ring ports.

Maybe it would be better to extend the current bridge netlink call to
be able to pass an optional interface to be flushed?  I'm not sure it
is a good idea to have two APIs doing very similar things.

   Andrew

Re: [RFC net-next v3 03/10] net: bridge: mrp: Add MRP interface used by netlink

[Allan W. Nielsen]

On 25.01.2020 16:20, Andrew Lunn wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
>> The 01/24/2020 18:43, Andrew Lunn wrote:
>> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> >
>> > > br_mrp_flush - will flush the FDB.
>> >
>> > How does this differ from a normal bridge flush? I assume there is a
>> > way for user space to flush the bridge FDB.
>>
>> Hi,
>>
>> If I seen corectly the normal bridge flush will clear the entire FDB for
>> all the ports of the bridge. In this case it is require to clear FDB
>> entries only for the ring ports.
>
>Maybe it would be better to extend the current bridge netlink call to
>be able to pass an optional interface to be flushed?  I'm not sure it
>is a good idea to have two APIs doing very similar things.
I agree.

And when looking at this again, I start to think that we should have
extended the existing netlink interface with new commands, instead of
adding a generic netlink.

/Allan

Re: [RFC net-next v3 03/10] net: bridge: mrp: Add MRP interface used by netlink

[Horatiu Vultur]

The 01/25/2020 20:16, Allan W. Nielsen wrote:
> On 25.01.2020 16:20, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
> > > The 01/24/2020 18:43, Andrew Lunn wrote:
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > >
> > > > > br_mrp_flush - will flush the FDB.
> > > >
> > > > How does this differ from a normal bridge flush? I assume there is a
> > > > way for user space to flush the bridge FDB.
> > > 
> > > Hi,
> > > 
> > > If I seen corectly the normal bridge flush will clear the entire FDB for
> > > all the ports of the bridge. In this case it is require to clear FDB
> > > entries only for the ring ports.
> > 
> > Maybe it would be better to extend the current bridge netlink call to
> > be able to pass an optional interface to be flushed?  I'm not sure it
> > is a good idea to have two APIs doing very similar things.
> I agree.
I would look over this.

> 
> And when looking at this again, I start to think that we should have
> extended the existing netlink interface with new commands, instead of
> adding a generic netlink.
We could do also that. The main reason why I have added a new generic
netlink was that I thought it would be clearer what commands are for MRP
configuration. But if you think that we should go forward by extending
existing netlink interface, that is perfectly fine for me.

> 
> /Allan
> 

-- 
/Horatiu

Re: [RFC net-next v3 03/10] net: bridge: mrp: Add MRP interface used by netlink

[Andrew Lunn]

> We could do also that. The main reason why I have added a new generic
> netlink was that I thought it would be clearer what commands are for MRP
> configuration.

The naming makes this clear, having _MRP_ in the attribute names etc.

But it would be good have input from the Bridge maintainers.

    Andrew

Re: [RFC net-next v3 03/10] net: bridge: mrp: Add MRP interface used by netlink

[Nikolay Aleksandrov]

On 26/01/2020 15:28, Horatiu Vultur wrote:
> The 01/25/2020 20:16, Allan W. Nielsen wrote:
>> On 25.01.2020 16:20, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
>>>> The 01/24/2020 18:43, Andrew Lunn wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>>> br_mrp_flush - will flush the FDB.
>>>>>
>>>>> How does this differ from a normal bridge flush? I assume there is a
>>>>> way for user space to flush the bridge FDB.
>>>>
>>>> Hi,
>>>>
>>>> If I seen corectly the normal bridge flush will clear the entire FDB for
>>>> all the ports of the bridge. In this case it is require to clear FDB
>>>> entries only for the ring ports.
>>>
>>> Maybe it would be better to extend the current bridge netlink call to
>>> be able to pass an optional interface to be flushed?  I'm not sure it
>>> is a good idea to have two APIs doing very similar things.
>> I agree.
> I would look over this.
> 

There's already a way to flush FDBs per-port - IFLA_BRPORT_FLUSH.

>>
>> And when looking at this again, I start to think that we should have
>> extended the existing netlink interface with new commands, instead of
>> adding a generic netlink.
> We could do also that. The main reason why I have added a new generic
> netlink was that I thought it would be clearer what commands are for MRP
> configuration. But if you think that we should go forward by extending
> existing netlink interface, that is perfectly fine for me.
> 
>>
>> /Allan
>>
> 

I don't mind extending the current netlink interface but the bridge already has
a huge (the largest) set of options and each time we add a new option we have
to adjust RTNL_MAX_TYPE. If you do decide to go this way maybe look into nesting
all the MRP options under one master MRP element into the bridge options, example:
[IFLA_BR_MRP]
  [IFLA_BR_MRP_X]
  [IFLA_BR_MRP_Y]
  ...

To avoid increasing the stack usage and bumping the max rtnl type too much.

Cheers,
 Nik

Re: [RFC net-next v3 03/10] net: bridge: mrp: Add MRP interface used by netlink

[Allan W. Nielsen]

On 20.02.2020 11:08, Nikolay Aleksandrov wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On 26/01/2020 15:28, Horatiu Vultur wrote:
>> The 01/25/2020 20:16, Allan W. Nielsen wrote:
>>> On 25.01.2020 16:20, Andrew Lunn wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
>>>>> The 01/24/2020 18:43, Andrew Lunn wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>>> br_mrp_flush - will flush the FDB.
>>>>>>
>>>>>> How does this differ from a normal bridge flush? I assume there is a
>>>>>> way for user space to flush the bridge FDB.
>>>>>
>>>>> Hi,
>>>>>
>>>>> If I seen corectly the normal bridge flush will clear the entire FDB for
>>>>> all the ports of the bridge. In this case it is require to clear FDB
>>>>> entries only for the ring ports.
>>>>
>>>> Maybe it would be better to extend the current bridge netlink call to
>>>> be able to pass an optional interface to be flushed?  I'm not sure it
>>>> is a good idea to have two APIs doing very similar things.
>>> I agree.
>> I would look over this.
>>
>
>There's already a way to flush FDBs per-port - IFLA_BRPORT_FLUSH.
>
>>>
>>> And when looking at this again, I start to think that we should have
>>> extended the existing netlink interface with new commands, instead of
>>> adding a generic netlink.
>> We could do also that. The main reason why I have added a new generic
>> netlink was that I thought it would be clearer what commands are for MRP
>> configuration. But if you think that we should go forward by extending
>> existing netlink interface, that is perfectly fine for me.
>>
>>>
>>> /Allan
>>>
>>
>
>I don't mind extending the current netlink interface but the bridge already has
>a huge (the largest) set of options and each time we add a new option we have
>to adjust RTNL_MAX_TYPE. If you do decide to go this way maybe look into nesting
>all the MRP options under one master MRP element into the bridge options, example:
>[IFLA_BR_MRP]
>  [IFLA_BR_MRP_X]
>  [IFLA_BR_MRP_Y]
>  ...
Ahh, did not see this mail before responsing to the other one.

We can make it part of the BR netlink then.

/Allan

[RFC net-next v3 04/10] net: bridge: mrp: Add generic netlink interface to configure MRP

[Horatiu Vultur]

Implement the generic netlink interface to configure MRP. The implementation
will do sanity checks over the attributes and then eventually call the MRP
interface which eventually will call the switchdev API.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_mrp_netlink.c | 655 ++++++++++++++++++++++++++++++++++++
 1 file changed, 655 insertions(+)
 create mode 100644 net/bridge/br_mrp_netlink.c

diff --git a/net/bridge/br_mrp_netlink.c b/net/bridge/br_mrp_netlink.c
new file mode 100644
index 000000000000..cb676387e89b
--- /dev/null
+++ b/net/bridge/br_mrp_netlink.c
@@ -0,0 +1,655 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <net/genetlink.h>
+
+#include <uapi/linux/mrp_bridge.h>
+#include "br_private.h"
+#include "br_private_mrp.h"
+
+static struct genl_family br_mrp_genl_family;
+
+static struct nla_policy br_mrp_genl_policy[BR_MRP_ATTR_END] = {
+	[BR_MRP_ATTR_NONE] = { .type = NLA_UNSPEC },
+	[BR_MRP_ATTR_BR_IFINDEX] = { .type = NLA_U32 },
+	[BR_MRP_ATTR_PORT_IFINDEX] = { .type = NLA_U32 },
+	[BR_MRP_ATTR_RING_NR] = { .type = NLA_U32 },
+	[BR_MRP_ATTR_RING_ROLE] = { .type = NLA_U32 },
+	[BR_MRP_ATTR_RING_STATE] = { .type = NLA_U32 },
+	[BR_MRP_ATTR_PORT_STATE] = { .type = NLA_U32 },
+	[BR_MRP_ATTR_PORT_ROLE] = { .type = NLA_U32 },
+	[BR_MRP_ATTR_TEST_INTERVAL] = { .type = NLA_U32 },
+	[BR_MRP_ATTR_TEST_MAX_MISS] = { .type = NLA_U8 },
+	[BR_MRP_ATTR_RING_OPEN] = { .type = NLA_U8 },
+};
+
+enum br_mrp_multicast_groups {
+	BR_MRP_MCGRP_OPEN,
+};
+
+static const struct genl_multicast_group br_mrp_mcgrps[] = {
+	[BR_MRP_MCGRP_OPEN] = { .name = "open", },
+};
+
+static int br_mrp_genl_add(struct sk_buff *skb, struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	struct net_device *dev_br;
+	struct net_bridge *br;
+	int err = 0;
+
+	if (!info->attrs[BR_MRP_ATTR_BR_IFINDEX]) {
+		pr_err("ATTR_BR_IFINDEX is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_RING_NR]) {
+		pr_err("ATTR_RING_NR is missing\n");
+		return -EINVAL;
+	}
+
+	rtnl_lock();
+
+	dev_br = __dev_get_by_index(net,
+				    nla_get_u32(info->attrs[BR_MRP_ATTR_BR_IFINDEX]));
+	if (!dev_br) {
+		pr_err("Invalid ATTR_BR_IFINDEX\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	if (!(dev_br->priv_flags & IFF_EBRIDGE)) {
+		pr_err("Port is not a bridge\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+	br = netdev_priv(dev_br);
+
+	err = br_mrp_add(br, nla_get_u32(info->attrs[BR_MRP_ATTR_RING_NR]));
+
+invalid_info:
+	rtnl_unlock();
+
+	return err;
+}
+
+static int br_mrp_genl_add_port(struct sk_buff *skb, struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	struct net_device *dev_br, *dev;
+	struct net_bridge_port *port;
+	struct net_bridge *br;
+	int err = 0;
+
+	if (!info->attrs[BR_MRP_ATTR_BR_IFINDEX]) {
+		pr_err("ATTR_BR_IFINDEX is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_RING_NR]) {
+		pr_err("ATTR_RING_NR is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_PORT_IFINDEX]) {
+		pr_err("ATTR_PORT_IFINDEX is missing\n");
+		return -EINVAL;
+	}
+
+	rtnl_lock();
+
+	dev_br = __dev_get_by_index(net,
+				    nla_get_u32(info->attrs[BR_MRP_ATTR_BR_IFINDEX]));
+	if (!dev_br) {
+		pr_err("Invalid ATTR_BR_IFINDEX\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	if (!(dev_br->priv_flags & IFF_EBRIDGE)) {
+		pr_err("Port is not a bridge\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+	br = netdev_priv(dev_br);
+
+	dev = __dev_get_by_index(net,
+				 nla_get_u32(info->attrs[BR_MRP_ATTR_PORT_IFINDEX]));
+	if (!dev) {
+		pr_err("Invalid ATTR_PORT_IFIINDEX\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+	if (!(dev->priv_flags & IFF_BRIDGE_PORT)) {
+		pr_err("ATTR_PORT_IFINDEX is not a bridge port");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	port = br_port_get_rtnl(dev);
+	if (port->br != br) {
+		pr_err("Port is not under the bridge\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	err = br_mrp_add_port(br, nla_get_u32(info->attrs[BR_MRP_ATTR_RING_NR]),
+			      port);
+
+invalid_info:
+	rtnl_unlock();
+
+	return err;
+}
+
+static int br_mrp_genl_del(struct sk_buff *skb, struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	struct net_device *dev_br;
+	struct net_bridge *br;
+	int err = 0;
+
+	if (!info->attrs[BR_MRP_ATTR_BR_IFINDEX]) {
+		pr_err("ATTR_BR_IFINDEX is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_RING_NR]) {
+		pr_err("ATTR_RING_NR is missing\n");
+		return -EINVAL;
+	}
+
+	rtnl_lock();
+
+	dev_br = __dev_get_by_index(net,
+				    nla_get_u32(info->attrs[BR_MRP_ATTR_BR_IFINDEX]));
+	if (!dev_br) {
+		pr_err("Invalid ATTR_BR_IFINDEX\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	if (!(dev_br->priv_flags & IFF_EBRIDGE)) {
+		pr_err("Port is not a bridge\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+	br = netdev_priv(dev_br);
+
+	err = br_mrp_del(br, nla_get_u32(info->attrs[BR_MRP_ATTR_RING_NR]));
+
+invalid_info:
+	rtnl_unlock();
+
+	return err;
+}
+
+static int br_mrp_genl_del_port(struct sk_buff *skb, struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	struct net_bridge_port *port;
+	struct net_device *dev;
+	int err = 0;
+
+	if (!info->attrs[BR_MRP_ATTR_PORT_IFINDEX]) {
+		pr_err("ATTR_PORT_IFINDEX is missing\n");
+		return -EINVAL;
+	}
+
+	rtnl_lock();
+
+	dev = __dev_get_by_index(net,
+				 nla_get_u32(info->attrs[BR_MRP_ATTR_PORT_IFINDEX]));
+	if (!dev) {
+		pr_err("Invalid ATTR_PORT_IFIINDEX\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	if (!(dev->priv_flags & IFF_BRIDGE_PORT)) {
+		pr_err("ATTR_PORT_IFINDEX is not a bridge port");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	port = br_port_get_rtnl(dev);
+
+	err = br_mrp_del_port(port);
+
+invalid_info:
+	rtnl_unlock();
+
+	return err;
+}
+
+static int br_mrp_genl_set_port_state(struct sk_buff *skb,
+				      struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	enum br_mrp_port_state_type state;
+	struct net_bridge_port *port;
+	struct net_device *dev;
+	int err = 0;
+
+	if (!info->attrs[BR_MRP_ATTR_PORT_IFINDEX]) {
+		pr_err("ATTR_PORT_IFINDEX is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_PORT_STATE]) {
+		pr_err("ATTR_PORT_STATE is missing\n");
+		return -EINVAL;
+	}
+
+	rtnl_lock();
+
+	dev = __dev_get_by_index(net,
+				 nla_get_u32(info->attrs[BR_MRP_ATTR_PORT_IFINDEX]));
+	if (!dev) {
+		pr_err("Invalid ATTR_PORT_IFIINDEX\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	if (!(dev->priv_flags & IFF_BRIDGE_PORT)) {
+		pr_err("ATTR_PORT_IFINDEX is not a bridge port");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	port = br_port_get_rtnl(dev);
+	state = nla_get_u32(info->attrs[BR_MRP_ATTR_PORT_STATE]);
+
+	err = br_mrp_set_port_state(port, state);
+
+invalid_info:
+	rtnl_unlock();
+
+	return err;
+}
+
+static int br_mrp_genl_set_port_role(struct sk_buff *skb,
+				     struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	enum br_mrp_port_role_type role;
+	struct net_bridge_port *port;
+	struct net_device *dev;
+	u32 ring_nr;
+	int err = 0;
+
+	if (!info->attrs[BR_MRP_ATTR_PORT_IFINDEX]) {
+		pr_err("ATTR_PORT_IFINDEX is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_PORT_ROLE]) {
+		pr_err("ATTR_PORT_STATE is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_RING_NR]) {
+		pr_err("ATTR_RING_NR is missing\n");
+		return -EINVAL;
+	}
+
+	rtnl_lock();
+
+	dev = __dev_get_by_index(net,
+				 nla_get_u32(info->attrs[BR_MRP_ATTR_PORT_IFINDEX]));
+	if (!dev) {
+		pr_err("Invalid ATTR_PORT_IFIINDEX\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	if (!(dev->priv_flags & IFF_BRIDGE_PORT)) {
+		pr_err("ATTR_PORT_IFINDEX is not a bridge port");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	port = br_port_get_rtnl(dev);
+	role = nla_get_u32(info->attrs[BR_MRP_ATTR_PORT_ROLE]);
+	ring_nr = nla_get_u32(info->attrs[BR_MRP_ATTR_RING_NR]);
+
+	err = br_mrp_set_port_role(port, ring_nr, role);
+
+invalid_info:
+	rtnl_unlock();
+
+	return err;
+}
+
+static int br_mrp_genl_set_ring_state(struct sk_buff *skb,
+				      struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	enum br_mrp_ring_state_type state;
+	struct net_device *dev_br;
+	struct net_bridge *br;
+	int err = 0;
+
+	if (!info->attrs[BR_MRP_ATTR_BR_IFINDEX]) {
+		pr_err("ATTR_BR_IFINDEX is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_RING_NR]) {
+		pr_err("ATTR_RING_NR is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_RING_STATE]) {
+		pr_err("ATTR_RING_STATE is missing\n");
+		return -EINVAL;
+	}
+
+	rtnl_lock();
+
+	dev_br = __dev_get_by_index(net,
+				    nla_get_u32(info->attrs[BR_MRP_ATTR_BR_IFINDEX]));
+	if (!dev_br) {
+		pr_err("Invalid ATTR_BR_IFINDEX\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	if (!(dev_br->priv_flags & IFF_EBRIDGE)) {
+		pr_err("Port is not a bridge\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+	br = netdev_priv(dev_br);
+
+	state = nla_get_u32(info->attrs[BR_MRP_ATTR_RING_STATE]);
+
+	err = br_mrp_set_ring_state(br,
+				    nla_get_u32(info->attrs[BR_MRP_ATTR_RING_NR]),
+				    state);
+
+invalid_info:
+	rtnl_unlock();
+
+	return err;
+}
+
+static int br_mrp_genl_set_ring_role(struct sk_buff *skb,
+				     struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	enum br_mrp_ring_role_type role;
+	struct net_device *dev_br;
+	struct net_bridge *br;
+	int err = 0;
+
+	if (!info->attrs[BR_MRP_ATTR_BR_IFINDEX]) {
+		pr_err("ATTR_BR_IFINDEX is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_RING_NR]) {
+		pr_err("ATTR_RING_NR is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_RING_ROLE]) {
+		pr_err("ATTR_RING_ROLE is missing\n");
+		return -EINVAL;
+	}
+
+	rtnl_lock();
+
+	dev_br = __dev_get_by_index(net,
+				    nla_get_u32(info->attrs[BR_MRP_ATTR_BR_IFINDEX]));
+	if (!dev_br) {
+		pr_err("Invalid ATTR_BR_IFINDEX\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	if (!(dev_br->priv_flags & IFF_EBRIDGE)) {
+		pr_err("Port is not a bridge\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+	br = netdev_priv(dev_br);
+
+	role = nla_get_u32(info->attrs[BR_MRP_ATTR_RING_ROLE]);
+
+	err = br_mrp_set_ring_role(br,
+				   nla_get_u32(info->attrs[BR_MRP_ATTR_RING_NR]),
+				   role);
+
+invalid_info:
+	rtnl_unlock();
+
+	return err;
+}
+
+static int br_mrp_genl_start_test(struct sk_buff *skb, struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	struct net_device *dev_br;
+	struct net_bridge *br;
+	u32 interval;
+	int err = 0;
+	u8 max_miss;
+
+	if (!info->attrs[BR_MRP_ATTR_BR_IFINDEX]) {
+		pr_err("ATTR_BR_IFINDEX is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_RING_NR]) {
+		pr_err("ATTR_RING_NR is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_TEST_INTERVAL]) {
+		pr_err("ATTR_TEST_INTERVAL is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_TEST_MAX_MISS]) {
+		pr_err("ATTR_TEST_MAX_MISS is missing\n");
+		return -EINVAL;
+	}
+
+	rtnl_lock();
+
+	dev_br = __dev_get_by_index(net,
+				    nla_get_u32(info->attrs[BR_MRP_ATTR_BR_IFINDEX]));
+	if (!dev_br) {
+		pr_err("Invalid ATTR_BR_IFINDEX\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	if (!(dev_br->priv_flags & IFF_EBRIDGE)) {
+		pr_err("Port is not a bridge\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+	br = netdev_priv(dev_br);
+
+	interval = nla_get_u32(info->attrs[BR_MRP_ATTR_TEST_INTERVAL]);
+	max_miss = nla_get_u8(info->attrs[BR_MRP_ATTR_TEST_MAX_MISS]);
+
+	err = br_mrp_start_test(br, nla_get_u32(info->attrs[BR_MRP_ATTR_RING_NR]),
+				interval, max_miss);
+
+invalid_info:
+	rtnl_unlock();
+
+	return err;
+}
+
+static int br_mrp_genl_flush(struct sk_buff *skb, struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	struct net_device *dev_br;
+	struct net_bridge *br;
+	int err = 0;
+
+	if (!info->attrs[BR_MRP_ATTR_BR_IFINDEX]) {
+		pr_err("ATTR_BR_IFINDEX is missing\n");
+		return -EINVAL;
+	}
+
+	if (!info->attrs[BR_MRP_ATTR_RING_NR]) {
+		pr_err("ATTR_RING_NR is missing\n");
+		return -EINVAL;
+	}
+
+	rtnl_lock();
+
+	dev_br = __dev_get_by_index(net,
+				    nla_get_u32(info->attrs[BR_MRP_ATTR_BR_IFINDEX]));
+	if (!dev_br) {
+		pr_err("Invalid ATTR_BR_IFINDEX\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+
+	if (!(dev_br->priv_flags & IFF_EBRIDGE)) {
+		pr_err("Port is not a bridge\n");
+		err = -EINVAL;
+		goto invalid_info;
+	}
+	br = netdev_priv(dev_br);
+
+	err = br_mrp_flush(br, nla_get_u32(info->attrs[BR_MRP_ATTR_RING_NR]));
+
+invalid_info:
+	rtnl_unlock();
+
+	return err;
+}
+
+void br_mrp_port_open(struct net_device *dev, u8 loc)
+{
+	struct sk_buff *msg;
+	void *hdr;
+
+	msg = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
+	if (!msg) {
+		pr_err("Allocate netlink msg failed\n");
+		return;
+	}
+
+	hdr = genlmsg_put(msg, 0, 0, &br_mrp_genl_family, 0,
+			  BR_MRP_GENL_RING_OPEN);
+	if (!hdr) {
+		pr_err("Create msg hdr failed \n");
+		goto err_msg_free;
+	}
+
+	if (nla_put_u32(msg, BR_MRP_ATTR_PORT_IFINDEX, dev->ifindex) ||
+	    nla_put_u8(msg, BR_MRP_ATTR_RING_OPEN, loc)) {
+		pr_err("Failed nla_put\n");
+		goto nla_put_failure;
+	}
+
+	genlmsg_end(msg, hdr);
+
+	genlmsg_multicast(&br_mrp_genl_family, msg, 0, 0, GFP_ATOMIC);
+	return;
+
+nla_put_failure:
+	genlmsg_cancel(msg, hdr);
+
+err_msg_free:
+	nlmsg_free(msg);
+}
+EXPORT_SYMBOL(br_mrp_port_open);
+
+static struct genl_ops br_mrp_genl_ops[] = {
+	{
+		.cmd    = BR_MRP_GENL_ADD,
+		.doit   = br_mrp_genl_add,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.flags  = GENL_ADMIN_PERM,
+	},
+	{
+		.cmd    = BR_MRP_GENL_ADD_PORT,
+		.doit   = br_mrp_genl_add_port,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.flags  = GENL_ADMIN_PERM,
+	},
+	{
+		.cmd    = BR_MRP_GENL_DEL,
+		.doit   = br_mrp_genl_del,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.flags  = GENL_ADMIN_PERM,
+	},
+	{
+		.cmd    = BR_MRP_GENL_DEL_PORT,
+		.doit   = br_mrp_genl_del_port,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.flags  = GENL_ADMIN_PERM,
+	},
+	{
+		.cmd    = BR_MRP_GENL_SET_PORT_STATE,
+		.doit   = br_mrp_genl_set_port_state,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.flags  = GENL_ADMIN_PERM,
+	},
+	{
+		.cmd    = BR_MRP_GENL_SET_PORT_ROLE,
+		.doit   = br_mrp_genl_set_port_role,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.flags  = GENL_ADMIN_PERM,
+	},
+	{
+		.cmd    = BR_MRP_GENL_SET_RING_STATE,
+		.doit   = br_mrp_genl_set_ring_state,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.flags  = GENL_ADMIN_PERM,
+	},
+	{
+		.cmd    = BR_MRP_GENL_SET_RING_ROLE,
+		.doit   = br_mrp_genl_set_ring_role,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.flags  = GENL_ADMIN_PERM,
+	},
+	{
+		.cmd    = BR_MRP_GENL_START_TEST,
+		.doit   = br_mrp_genl_start_test,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.flags  = GENL_ADMIN_PERM,
+	},
+	{
+		.cmd    = BR_MRP_GENL_FLUSH,
+		.doit   = br_mrp_genl_flush,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.flags  = GENL_ADMIN_PERM,
+	},
+};
+
+static struct genl_family br_mrp_genl_family = {
+	.name		= "br_mrp_netlink",
+	.hdrsize	= 0,
+	.version	= 1,
+	.maxattr	= BR_MRP_ATTR_MAX,
+	.policy		= br_mrp_genl_policy,
+	.ops		= br_mrp_genl_ops,
+	.n_ops		= ARRAY_SIZE(br_mrp_genl_ops),
+	.mcgrps		= br_mrp_mcgrps,
+	.n_mcgrps	= ARRAY_SIZE(br_mrp_mcgrps),
+};
+
+int br_mrp_netlink_init(void)
+{
+	int err;
+
+	err = genl_register_family(&br_mrp_genl_family);
+	if (err)
+		pr_err("genl_register_family failed\n");
+
+	return err;
+}
+
+void br_mrp_netlink_uninit(void)
+{
+	genl_unregister_family(&br_mrp_genl_family);
+}
-- 
2.17.1

Re: [RFC net-next v3 04/10] net: bridge: mrp: Add generic netlink interface to configure MRP

[Andrew Lunn]

On Fri, Jan 24, 2020 at 05:18:22PM +0100, Horatiu Vultur wrote:
> Implement the generic netlink interface to configure MRP. The implementation
> will do sanity checks over the attributes and then eventually call the MRP
> interface which eventually will call the switchdev API.

Hi Horatiu

What was your thinking between adding a new generic netlink interface,
and extending the current one?

I've not looked at your user space code yet, but i assume it has to
make use of both? It needs to create the bridge and add the
interfaces. And then it needs to control the MRP state.

Allan mentioned you might get around to implementing 802.1CB? Would
that be another generic netlink interface, or would you extend the MRP
interface?

Thanks
	Andrew			

Re: [RFC net-next v3 04/10] net: bridge: mrp: Add generic netlink interface to configure MRP

[Allan W. Nielsen]

On 25.01.2020 16:34, Andrew Lunn wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On Fri, Jan 24, 2020 at 05:18:22PM +0100, Horatiu Vultur wrote:
>> Implement the generic netlink interface to configure MRP. The implementation
>> will do sanity checks over the attributes and then eventually call the MRP
>> interface which eventually will call the switchdev API.
>What was your thinking between adding a new generic netlink interface,
>and extending the current one?
>
>I've not looked at your user space code yet, but i assume it has to
>make use of both? It needs to create the bridge and add the
>interfaces. And then it needs to control the MRP state.
>
>Allan mentioned you might get around to implementing 802.1CB? Would
>that be another generic netlink interface, or would you extend the MRP
>interface?
Horatiu, if you have given this any thoughts, then please share them.

Here are my thoughts on 802.1CB: If we look at this with the traditional
NIC/host POW, then it would be natural to look at the HSR interface as
Vinicius suggested, and expose it as a new interface (HSR0). But when
looking at how 802.1CB say a bridge should act, and also what the
capabilities of the HW are, then it seem more natural to extend the TC
system. In HW it is a TCAM classifying the traffic, and it has some
actions to either replicate the matched frames, or eliminate the
additional copies.

The HW also supports CFM (see [1]), which we need (partly) to complete
the MRP implementation with MIM/MIC roles. This is also useful for none
MRP users (like ERPS).

This seems like an argument for moving this to the existing netlink
interfaces instead of having it as a generic netlink.

[1] https://en.wikipedia.org/wiki/IEEE_802.1ag

/Allan

Re: [RFC net-next v3 04/10] net: bridge: mrp: Add generic netlink interface to configure MRP

[Horatiu Vultur]

Hi,
The 01/25/2020 20:28, Allan W. Nielsen wrote:
> On 25.01.2020 16:34, Andrew Lunn wrote:
> > On Fri, Jan 24, 2020 at 05:18:22PM +0100, Horatiu Vultur wrote:
> > > Implement the generic netlink interface to configure MRP. The implementation
> > > will do sanity checks over the attributes and then eventually call the MRP
> > > interface which eventually will call the switchdev API.
> > What was your thinking between adding a new generic netlink interface,
> > and extending the current one?
I thought is more clear to see which commands are used to configure MRP
by adding a new generic netlink than extending the existing one.

> > 
> > I've not looked at your user space code yet, but i assume it has to
> > make use of both? It needs to create the bridge and add the
> > interfaces. And then it needs to control the MRP state.
Actually the userspace application doesn't create the bridge and add the
interfaces. It expects that user does this using iproute2 or other
commands.
And once the bridge and interfaces are added then it would just
configure the bridge.

> > 
> > Allan mentioned you might get around to implementing 802.1CB? Would
> > that be another generic netlink interface, or would you extend the MRP
> > interface?
> Horatiu, if you have given this any thoughts, then please share them.
I have not look yet over this, I will try to have a look ASAP.

> 
> Here are my thoughts on 802.1CB: If we look at this with the traditional
> NIC/host POW, then it would be natural to look at the HSR interface as
> Vinicius suggested, and expose it as a new interface (HSR0). But when
> looking at how 802.1CB say a bridge should act, and also what the
> capabilities of the HW are, then it seem more natural to extend the TC
> system. In HW it is a TCAM classifying the traffic, and it has some
> actions to either replicate the matched frames, or eliminate the
> additional copies.
> 
> The HW also supports CFM (see [1]), which we need (partly) to complete
> the MRP implementation with MIM/MIC roles. This is also useful for none
> MRP users (like ERPS).
> 
> This seems like an argument for moving this to the existing netlink
> interfaces instead of having it as a generic netlink.
> 
> [1] https://en.wikipedia.org/wiki/IEEE_802.1ag
> 
> /Allan

-- 
/Horatiu

[RFC net-next v3 05/10] net: bridge: mrp: Update MRP interface to add switchdev support

[Horatiu Vultur]

Extend the MRP interface to allow switchdev support. The following functions are
added:

br_mrp_port_switchdev_add - this corresponds to the function br_mrp_add_port,
  and will notify the HW that a port is added to a MRP ring. The function gets
  as parameter the port and the ID of the ring.

br_mrp_port_switchdev_del - this corresponds to the function br_mrp_del_port
  and will notify the HW that a port is removed from a MRP ring. The function
  gets as parameter the port and the ID of the ring.

br_mrp_port_switchdev_set_state - this corresponds to the function
  br_mrp_port_state. It would notify the HW if it should block or not non-MRP
  frames.

br_mrp_port_switchdev_set_port - this corresponds to the function
  br_mrp_port_role. It would set the port role, primary or secondary.

br_mrp_switchdev_set_role - this corresponds to the function br_mrp_ring_role
  and would set one of the role MRM or MRC.

br_mrp_switchdev_set_ring_state - this corresponds to the function
  br_mrp_ring_state and would set the ring to be open or closed.

br_mrp_switchdev_send_ring_test - this corresponds to the function
  br_mrp_start_test. This will notify the HW to start or stop generating
  MRP_Test frames. Value 0 for the interval parameter means to stop generating
  the frames.

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

diff --git a/net/bridge/br_private_mrp.h b/net/bridge/br_private_mrp.h
index bea4ece4411c..de5ba7f730f6 100644
--- a/net/bridge/br_private_mrp.h
+++ b/net/bridge/br_private_mrp.h
@@ -35,6 +35,22 @@ int br_mrp_start_test(struct net_bridge *br, u32 ring_nr, u32 interval,
 		      u8 max_miss);
 int br_mrp_flush(struct net_bridge *br, u32 ring_nr);
 
+/* br_mrp_switchdev.c */
+int br_mrp_port_switchdev_add(struct net_bridge_port *p, u32 ring_nr);
+int br_mrp_port_switchdev_del(struct net_bridge_port *p, u32 ring_nr);
+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_ring_role(struct br_mrp *mrp,
+				   enum br_mrp_ring_role_type role);
+int br_mrp_switchdev_set_ring_state(struct br_mrp *mrp,
+				    enum br_mrp_ring_state_type state);
+
+int br_mrp_switchdev_send_ring_test(struct br_mrp *mrp, u32 interval,
+				    u8 max_miss);
+
 /* br_mrp_netlink.c */
 void br_mrp_port_open(struct net_device *dev, u8 loc);
 
-- 
2.17.1

[RFC net-next v3 06/10] net: bridge: mrp: switchdev: Extend switchdev API to offload MRP

[Horatiu Vultur]

Extend switchdev API to add support for MRP. The HW is notified in
following cases:

SWITCHDEV_OBJ_ID_PORT_MRP: This is used when a port is added/removed from the
  mrp ring.

SWITCHDEV_OBJ_ID_RING_ROLE_MRP: This is used when the role of the node
  changes. The current supported roles are Media Redundancy Manager and Media
  Redundancy Client.

SWITCHDEV_OBJ_ID_RING_TEST_MRP: This is used when to start/stop sending
  MRP_Test frames on the mrp ring ports. This is called only on nodes that have
  the role Media Redundancy Manager.

SWITCHDEV_ATTR_ID_MRP_PORT_STATE: This is used when the port's state is
  changed. It can be in blocking/forwarding mode.

SWITCHDEV_ATTR_ID_MRP_PORT_ROLE: This is used when port's role changes. The
  roles of the port can be primary/secondary. This is required to notify HW
  because the MRP_Test frame contains the field MRP_PortRole that contains this
  information.

SWITCHDEV_ATTR_ID_MRP_RING_STATE: This is used when the ring changes it states
  to open or closed. This is required to notify HW because the MRP_Test frame
  contains the field MRP_InState which contains this information.

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

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index aee86a189432..b1ed170fc01d 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -40,6 +40,11 @@ enum switchdev_attr_id {
 	SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING,
 	SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
 	SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
+#ifdef CONFIG_BRIDGE_MRP
+	SWITCHDEV_ATTR_ID_MRP_PORT_STATE,
+	SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
+	SWITCHDEV_ATTR_ID_MRP_RING_STATE,
+#endif
 };
 
 struct switchdev_attr {
@@ -55,6 +60,11 @@ struct switchdev_attr {
 		clock_t ageing_time;			/* BRIDGE_AGEING_TIME */
 		bool vlan_filtering;			/* BRIDGE_VLAN_FILTERING */
 		bool mc_disabled;			/* MC_DISABLED */
+#ifdef CONFIG_BRIDGE_MRP
+		u8 mrp_port_state;			/* MRP_PORT_STATE */
+		u8 mrp_port_role;			/* MRP_PORT_ROLE */
+		u8 mrp_ring_state;			/* MRP_RING_STATE */
+#endif
 	} u;
 };
 
@@ -63,6 +73,11 @@ enum switchdev_obj_id {
 	SWITCHDEV_OBJ_ID_PORT_VLAN,
 	SWITCHDEV_OBJ_ID_PORT_MDB,
 	SWITCHDEV_OBJ_ID_HOST_MDB,
+#ifdef CONFIG_BRIDGE_MRP
+	SWITCHDEV_OBJ_ID_PORT_MRP,
+	SWITCHDEV_OBJ_ID_RING_TEST_MRP,
+	SWITCHDEV_OBJ_ID_RING_ROLE_MRP,
+#endif
 };
 
 struct switchdev_obj {
@@ -94,6 +109,42 @@ struct switchdev_obj_port_mdb {
 #define SWITCHDEV_OBJ_PORT_MDB(OBJ) \
 	container_of((OBJ), struct switchdev_obj_port_mdb, obj)
 
+
+#ifdef CONFIG_BRIDGE_MRP
+/* SWITCHDEV_OBJ_ID_PORT_MRP */
+struct switchdev_obj_port_mrp {
+	struct switchdev_obj obj;
+	struct net_device *port;
+	u32 ring_nr;
+};
+
+#define SWITCHDEV_OBJ_PORT_MRP(OBJ) \
+	container_of((OBJ), struct switchdev_obj_port_mrp, obj)
+
+/* SWITCHDEV_OBJ_ID_RING_TEST_MRP */
+struct switchdev_obj_ring_test_mrp {
+	struct switchdev_obj obj;
+	/* The value is in us and a value of 0 represents to stop */
+	u32 interval;
+	u8 max_miss;
+	u32 ring_nr;
+};
+
+#define SWITCHDEV_OBJ_RING_TEST_MRP(OBJ) \
+	container_of((OBJ), struct switchdev_obj_ring_test_mrp, obj)
+
+/* SWICHDEV_OBJ_ID_RING_ROLE_MRP */
+struct switchdev_obj_ring_role_mrp {
+	struct switchdev_obj obj;
+	u8 ring_role;
+	u32 ring_nr;
+};
+
+#define SWITCHDEV_OBJ_RING_ROLE_MRP(OBJ) \
+	container_of((OBJ), struct switchdev_obj_ring_role_mrp, obj)
+
+#endif
+
 typedef int switchdev_obj_dump_cb_t(struct switchdev_obj *obj);
 
 enum switchdev_notifier_type {
-- 
2.17.1

Re: [RFC net-next v3 06/10] net: bridge: mrp: switchdev: Extend switchdev API to offload MRP

[Andrew Lunn]

> SWITCHDEV_OBJ_ID_RING_TEST_MRP: This is used when to start/stop sending
>   MRP_Test frames on the mrp ring ports. This is called only on nodes that have
>   the role Media Redundancy Manager.

How do you handle the 'headless chicken' scenario? User space tells
the port to start sending MRP_Test frames. It then dies. The hardware
continues sending these messages, and the neighbours thinks everything
is O.K, but in reality the state machine is dead, and when the ring
breaks, the daemon is not there to fix it?

And it is not just the daemon that could die. The kernel could opps or
deadlock, etc.

For a robust design, it seems like SWITCHDEV_OBJ_ID_RING_TEST_MRP
should mean: start sending MRP_Test frames for the next X seconds, and
then stop. And the request is repeated every X-1 seconds.

     Andrew

Re: [RFC net-next v3 06/10] net: bridge: mrp: switchdev: Extend switchdev API to offload MRP

[Horatiu Vultur]

The 01/25/2020 17:35, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > SWITCHDEV_OBJ_ID_RING_TEST_MRP: This is used when to start/stop sending
> >   MRP_Test frames on the mrp ring ports. This is called only on nodes that have
> >   the role Media Redundancy Manager.
> 
> How do you handle the 'headless chicken' scenario? User space tells
> the port to start sending MRP_Test frames. It then dies. The hardware
> continues sending these messages, and the neighbours thinks everything
> is O.K, but in reality the state machine is dead, and when the ring
> breaks, the daemon is not there to fix it?
> 
> And it is not just the daemon that could die. The kernel could opps or
> deadlock, etc.
> 
> For a robust design, it seems like SWITCHDEV_OBJ_ID_RING_TEST_MRP
> should mean: start sending MRP_Test frames for the next X seconds, and
> then stop. And the request is repeated every X-1 seconds.

I totally missed this case, I will update this as you suggest.

> 
>      Andrew

-- 
/Horatiu

Re: [RFC net-next v3 06/10] net: bridge: mrp: switchdev: Extend switchdev API to offload MRP

[Andrew Lunn]

On Sun, Jan 26, 2020 at 02:22:13PM +0100, Horatiu Vultur wrote:
> The 01/25/2020 17:35, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > > SWITCHDEV_OBJ_ID_RING_TEST_MRP: This is used when to start/stop sending
> > >   MRP_Test frames on the mrp ring ports. This is called only on nodes that have
> > >   the role Media Redundancy Manager.
> > 
> > How do you handle the 'headless chicken' scenario? User space tells
> > the port to start sending MRP_Test frames. It then dies. The hardware
> > continues sending these messages, and the neighbours thinks everything
> > is O.K, but in reality the state machine is dead, and when the ring
> > breaks, the daemon is not there to fix it?
> > 
> > And it is not just the daemon that could die. The kernel could opps or
> > deadlock, etc.
> > 
> > For a robust design, it seems like SWITCHDEV_OBJ_ID_RING_TEST_MRP
> > should mean: start sending MRP_Test frames for the next X seconds, and
> > then stop. And the request is repeated every X-1 seconds.
> 
> I totally missed this case, I will update this as you suggest.

Hi Horatiu

What does your hardware actually provide?

Given the design of the protocol, if the hardware decides the OS etc
is dead, it should stop sending MRP_TEST frames and unblock the ports.
If then becomes a 'dumb switch', and for a short time there will be a
broadcast storm. Hopefully one of the other nodes will then take over
the role and block a port.

    Andrew

Re: [RFC net-next v3 06/10] net: bridge: mrp: switchdev: Extend switchdev API to offload MRP

[Allan W. Nielsen]

On 26.01.2020 16:59, Andrew Lunn wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On Sun, Jan 26, 2020 at 02:22:13PM +0100, Horatiu Vultur wrote:
>> The 01/25/2020 17:35, Andrew Lunn wrote:
>> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> >
>> > > SWITCHDEV_OBJ_ID_RING_TEST_MRP: This is used when to start/stop sending
>> > >   MRP_Test frames on the mrp ring ports. This is called only on nodes that have
>> > >   the role Media Redundancy Manager.
>> >
>> > How do you handle the 'headless chicken' scenario? User space tells
>> > the port to start sending MRP_Test frames. It then dies. The hardware
>> > continues sending these messages, and the neighbours thinks everything
>> > is O.K, but in reality the state machine is dead, and when the ring
>> > breaks, the daemon is not there to fix it?
I agree, we need to find a solution to this issue.

>> > And it is not just the daemon that could die. The kernel could opps or
>> > deadlock, etc.
>> >
>> > For a robust design, it seems like SWITCHDEV_OBJ_ID_RING_TEST_MRP
>> > should mean: start sending MRP_Test frames for the next X seconds, and
>> > then stop. And the request is repeated every X-1 seconds.
Sounds like a good idea to me.

>> I totally missed this case, I will update this as you suggest.
>
>What does your hardware actually provide?
>
>Given the design of the protocol, if the hardware decides the OS etc
>is dead, it should stop sending MRP_TEST frames and unblock the ports.
>If then becomes a 'dumb switch', and for a short time there will be a
>broadcast storm. Hopefully one of the other nodes will then take over
>the role and block a port.
As far as I know, the only feature HW has to prevent this is a
watch-dog timer. Which will reset the entire system (not a bad idea if
the kernel has dead-locked).

/Allan

Re: [RFC net-next v3 06/10] net: bridge: mrp: switchdev: Extend switchdev API to offload MRP

[Jürgen Lambrecht]

On 1/27/20 12:04 PM, Allan W. Nielsen wrote:
> CAUTION: This Email originated from outside Televic. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> On 26.01.2020 16:59, Andrew Lunn wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On Sun, Jan 26, 2020 at 02:22:13PM +0100, Horatiu Vultur wrote:
>>> The 01/25/2020 17:35, Andrew Lunn wrote:
>>> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>> >
>>> > > SWITCHDEV_OBJ_ID_RING_TEST_MRP: This is used when to start/stop sending
>>> > >   MRP_Test frames on the mrp ring ports. This is called only on nodes that have
>>> > >   the role Media Redundancy Manager.
>>> >
>>> > How do you handle the 'headless chicken' scenario? User space tells
>>> > the port to start sending MRP_Test frames. It then dies. The hardware
>>> > continues sending these messages, and the neighbours thinks everything
>>> > is O.K, but in reality the state machine is dead, and when the ring
>>> > breaks, the daemon is not there to fix it?
> I agree, we need to find a solution to this issue.
>
>>> > And it is not just the daemon that could die. The kernel could opps or
>>> > deadlock, etc.
>>> >
>>> > For a robust design, it seems like SWITCHDEV_OBJ_ID_RING_TEST_MRP
>>> > should mean: start sending MRP_Test frames for the next X seconds, and
>>> > then stop. And the request is repeated every X-1 seconds.
> Sounds like a good idea to me.

Indeed, and it should then do the same as mentioned below and "... come a 'dumb switch' ", except that I propose to make it configurable how to fallback: with auto-recovery ('dumb switch') or safe mode that keeps the ports blocked, and then some higher layer protocol should fix it.

>
>>> I totally missed this case, I will update this as you suggest.
>>
>> What does your hardware actually provide?
>>
>> Given the design of the protocol, if the hardware decides the OS etc
>> is dead, it should stop sending MRP_TEST frames and unblock the ports.
>> If then becomes a 'dumb switch', and for a short time there will be a
>> broadcast storm. Hopefully one of the other nodes will then take over
>> the role and block a port.
> As far as I know, the only feature HW has to prevent this is a
> watch-dog timer. Which will reset the entire system (not a bad idea if
> the kernel has dead-locked).
Indeed. Our designs always have a watchdog.

And then I again propose to have 2 bootup options.

I refer here also to my answer on Allan's answer on my email of 12:29PM.

Kind regards,

Jürgen

>
> /Allan
>

Re: [RFC net-next v3 06/10] net: bridge: mrp: switchdev: Extend switchdev API to offload MRP

[Andrew Lunn]

On Mon, Jan 27, 2020 at 03:26:38PM +0100, Jürgen Lambrecht wrote:
> On 1/27/20 12:04 PM, Allan W. Nielsen wrote:
> 
>             > How do you handle the 'headless chicken' scenario? User space
>             tells
>             > the port to start sending MRP_Test frames. It then dies. The
>             hardware
> 
> Andrew, I am a bit confused here - maybe I missed an email-thread, I'm sorry
> then.
> 
> In previous emails you and others talked about hardware support to send packets
> (inside the switch). But somebody also talked about data-plane and
> control-plane (about STP in-kernel being a bad idea), and that data-plane is
> in-kernel, and control plane is a mrp-daemon (in user space).
> And in my mind, the "hardware" you mention is a frame-injector and can be both
> real hardware and a driver in the kernel.
> 
> Do I see it right?

Hi Jürgen

It i still unclear where the MRP_Test frames should be generated,
forward and consumed, either in kernel, or in user space.

The userspace RSTP daemon generates and consumes all the BPDUs in
userspace. But BPDUs are never forwarded. However MRP_Test frames are
forwarded by client nodes. Are the MRP_Test frames then part of the
data plane in a client?

What i think is clear is that the state machine is in user space.

For the rest, we are still exploring possibilities.

    Andrew

Re: [RFC net-next v3 06/10] net: bridge: mrp: switchdev: Extend switchdev API to offload MRP

[Jürgen Lambrecht]

[sending again to vger.kernel.org, because previous was rejected]

On 1/27/20 12:04 PM, Allan W. Nielsen wrote:
>>> > How do you handle the 'headless chicken' scenario? User space tells
>>> > the port to start sending MRP_Test frames. It then dies. The hardware

Andrew, I am a bit confused here - maybe I missed an email-thread, I'm sorry then.

In previous emails you and others talked about hardware support to send packets (inside the switch). But somebody also talked about data-plane and control-plane (about STP in-kernel being a bad idea), and that data-plane is in-kernel, and control plane is a mrp-daemon (in user space).
And in my mind, the "hardware" you mention is a frame-injector and can be both real hardware and a driver in the kernel.

Do I see it right?

>>> > continues sending these messages, and the neighbours thinks everything
>>> > is O.K, but in reality the state machine is dead, and when the ring
>>> > breaks, the daemon is not there to fix it?
> I agree, we need to find a solution to this issue.
>

Re: [RFC net-next v3 06/10] net: bridge: mrp: switchdev: Extend switchdev API to offload MRP

[Jürgen Lambrecht]

On 1/26/20 4:59 PM, Andrew Lunn wrote:
> Given the design of the protocol, if the hardware decides the OS etc
> is dead, it should stop sending MRP_TEST frames and unblock the ports.
> If then becomes a 'dumb switch', and for a short time there will be a
> broadcast storm. Hopefully one of the other nodes will then take over
> the role and block a port.

In my experience a closed loop should never happen. It can make software crash and give other problems.
An other node should first take over before unblocking the ring ports. (If this is possible - I only follow this discussion halfly)

What is your opinion?

(FYI:
I made that mistake once doing a proof-of-concept ring design: during testing, when a "broken" Ethernet cable was "fixed" I had for a short time a loop, and then it happened often that that port of the (Marvell 88E6063) switch was blocked.  (To unblock, only solution was to bring that port down and up again, and then all "lost" packets came out in a burst.)
That problem was caused by flow control (with pause frames), and disabling flow control fixed it, but flow-control is default on as far as I know.
)

Kind regards,

Jürgen

Re: [RFC net-next v3 06/10] net: bridge: mrp: switchdev: Extend switchdev API to offload MRP

[Allan W. Nielsen]

Hi Jürgen,

On 27.01.2020 12:29, Jürgen Lambrecht wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On 1/26/20 4:59 PM, Andrew Lunn wrote:
>> Given the design of the protocol, if the hardware decides the OS etc
>> is dead, it should stop sending MRP_TEST frames and unblock the ports.
>> If then becomes a 'dumb switch', and for a short time there will be a
>> broadcast storm. Hopefully one of the other nodes will then take over
>> the role and block a port.

>In my experience a closed loop should never happen. It can make
>software crash and give other problems.  An other node should first
>take over before unblocking the ring ports. (If this is possible - I
>only follow this discussion halfly)
>
>What is your opinion?
Having loops in the network is never a good thing - but to be honest, I
think it is more important that we ensure the design can survive and
recover from loops.

With the current design, it will be really hard to void loops when the
network boot. MRP will actually start with the ports blocked, but they
will be unblocked in the period from when the bridge is created and
until MRP is enabled. If we want to change this (which I'm not too keen
on), then we need to be able to block the ports while the bridge is
down.

And even if we do this, then we can not guarantee to avoid loops. Lets
assume we have a small ring with just 2 nodes: a MRM and a MRC. Lets
assume the MRM boots first. It will unblock both ports as the ring is
open. Now the MRC boots, and make the ring closed, and create a loop.
This will take some time (milliseconds) before the MRM notice this and
block one of the ports.

But while we are at this topic, we need to add some functionality to
the user-space application such that it can set the priority of the MRP
frames. We will get that fixed.

>(FYI: I made that mistake once doing a proof-of-concept ring design:
>during testing, when a "broken" Ethernet cable was "fixed" I had for a
>short time a loop, and then it happened often that that port of the
>(Marvell 88E6063) switch was blocked.  (To unblock, only solution was
>to bring that port down and up again, and then all "lost" packets came
>out in a burst.) That problem was caused by flow control (with pause
>frames), and disabling flow control fixed it, but flow-control is
>default on as far as I know.)
I see. It could be fun to see if what we have proposed so far will with
with such a switch.

/Allan

Re: [RFC net-next v3 06/10] net: bridge: mrp: switchdev: Extend switchdev API to offload MRP

[Jürgen Lambrecht]

On 1/27/20 1:27 PM, Allan W. Nielsen wrote:
> CAUTION: This Email originated from outside Televic. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> Hi Jürgen,
>
> On 27.01.2020 12:29, Jürgen Lambrecht wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 1/26/20 4:59 PM, Andrew Lunn wrote:
>>> Given the design of the protocol, if the hardware decides the OS etc
>>> is dead, it should stop sending MRP_TEST frames and unblock the ports.
>>> If then becomes a 'dumb switch', and for a short time there will be a
>>> broadcast storm. Hopefully one of the other nodes will then take over
>>> the role and block a port.
This can probably be a configuration option in the hardware, how to fall-back.
>
>> In my experience a closed loop should never happen. It can make
>> software crash and give other problems.  An other node should first
>> take over before unblocking the ring ports. (If this is possible - I
>> only follow this discussion halfly)
>>
>> What is your opinion?
> Having loops in the network is never a good thing - but to be honest, I
> think it is more important that we ensure the design can survive and
> recover from loops.
Indeed
>
> With the current design, it will be really hard to void loops when the
> network boot. MRP will actually start with the ports blocked, but they
> will be unblocked in the period from when the bridge is created and
> until MRP is enabled. If we want to change this (which I'm not too keen
> on), then we need to be able to block the ports while the bridge is
> down.
Our ring network is part of a bigger network. Loops are really not allowed.
>
> And even if we do this, then we can not guarantee to avoid loops. Lets
> assume we have a small ring with just 2 nodes: a MRM and a MRC. Lets
> assume the MRM boots first. It will unblock both ports as the ring is
> open. Now the MRC boots, and make the ring closed, and create a loop.
> This will take some time (milliseconds) before the MRM notice this and
> block one of the ports.

In my view there is a bring-up and tear-down module needed. I don't know if it should be part of MRP or not? Probably not, so something on top of the mrp daemon.

>
> But while we are at this topic, we need to add some functionality to
> the user-space application such that it can set the priority of the MRP
> frames. We will get that fixed.

Indeed! In my old design I had to give high priority, else the loop was wrongly closed at high network load.

I guess you mean the priority in the VLAN header?
I think to remember one talked about the bride code being VLAN-agnostic.

>
>> (FYI: I made that mistake once doing a proof-of-concept ring design:
>> during testing, when a "broken" Ethernet cable was "fixed" I had for a
>> short time a loop, and then it happened often that that port of the
>> (Marvell 88E6063) switch was blocked.  (To unblock, only solution was
>> to bring that port down and up again, and then all "lost" packets came
>> out in a burst.) That problem was caused by flow control (with pause
>> frames), and disabling flow control fixed it, but flow-control is
>> default on as far as I know.)
> I see. It could be fun to see if what we have proposed so far will with
> with such a switch.

Depending on the projects I could work on it later this year (or only next year or not..)

Kind regards,

Jürgen

>
> /Allan
>

Re: [RFC net-next v3 06/10] net: bridge: mrp: switchdev: Extend switchdev API to offload MRP

[Allan W. Nielsen]

On 27.01.2020 15:39, Jürgen Lambrecht wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On 1/27/20 1:27 PM, Allan W. Nielsen wrote:
>> Hi Jürgen,
>>
>> On 27.01.2020 12:29, Jürgen Lambrecht wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 1/26/20 4:59 PM, Andrew Lunn wrote:
>>>> Given the design of the protocol, if the hardware decides the OS etc
>>>> is dead, it should stop sending MRP_TEST frames and unblock the ports.
>>>> If then becomes a 'dumb switch', and for a short time there will be a
>>>> broadcast storm. Hopefully one of the other nodes will then take over
>>>> the role and block a port.
>This can probably be a configuration option in the hardware, how to fall-back.
>>
>>> In my experience a closed loop should never happen. It can make
>>> software crash and give other problems.  An other node should first
>>> take over before unblocking the ring ports. (If this is possible - I
>>> only follow this discussion halfly)
>>>
>>> What is your opinion?
>> Having loops in the network is never a good thing - but to be honest, I
>> think it is more important that we ensure the design can survive and
>> recover from loops.
>Indeed
>>
>> With the current design, it will be really hard to void loops when the
>> network boot. MRP will actually start with the ports blocked, but they
>> will be unblocked in the period from when the bridge is created and
>> until MRP is enabled. If we want to change this (which I'm not too keen
>> on), then we need to be able to block the ports while the bridge is
>> down.
>Our ring network is part of a bigger network. Loops are really not allowed.
That is understood, and should be avoided. But I assume that switches
which crashes is not allowed either ;-)

We will consider if we somehow can block the ports before/after a
user-space protocol kicks in. I can not promise anything, but we will
see what can be done.

>> And even if we do this, then we can not guarantee to avoid loops. Lets
>> assume we have a small ring with just 2 nodes: a MRM and a MRC. Lets
>> assume the MRM boots first. It will unblock both ports as the ring is
>> open. Now the MRC boots, and make the ring closed, and create a loop.
>> This will take some time (milliseconds) before the MRM notice this and
>> block one of the ports.
>In my view there is a bring-up and tear-down module needed. I don't
>know if it should be part of MRP or not? Probably not, so something on
>top of the mrp daemon.
If we need this kind of policies, then I agree it should be on top of or
out-side the user-space MRP daemon.

>> But while we are at this topic, we need to add some functionality to
>> the user-space application such that it can set the priority of the MRP
>> frames. We will get that fixed.
>Indeed! In my old design I had to give high priority, else the loop was
>wrongly closed at high network load.
Yes, I'm not surprised to hear that.

>I guess you mean the priority in the VLAN header?
>I think to remember one talked about the bride code being VLAN-agnostic.
Yes, if it has a VLAN header (which is optional). But even without the
VLAN header these frames needs to be classified to a high priority
queue.

>>> (FYI: I made that mistake once doing a proof-of-concept ring design:
>>> during testing, when a "broken" Ethernet cable was "fixed" I had for a
>>> short time a loop, and then it happened often that that port of the
>>> (Marvell 88E6063) switch was blocked.  (To unblock, only solution was
>>> to bring that port down and up again, and then all "lost" packets came
>>> out in a burst.) That problem was caused by flow control (with pause
>>> frames), and disabling flow control fixed it, but flow-control is
>>> default on as far as I know.)
>> I see. It could be fun to see if what we have proposed so far will with
>> with such a switch.
>
>Depending on the projects I could work on it later this year (or only next year or not..)
Sounds good - no hurry.

/Allan

[RFC net-next v3 07/10] net: bridge: mrp: switchdev: Implement MRP API for switchdev

[Horatiu Vultur]

Implement the MRP api for switchdev.
These functions will just eventually call the switchdev functions:
switchdev_port_obj_add/del and switchdev_port_attr_set.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_mrp_switchdev.c | 147 ++++++++++++++++++++++++++++++++++
 1 file changed, 147 insertions(+)
 create mode 100644 net/bridge/br_mrp_switchdev.c

diff --git a/net/bridge/br_mrp_switchdev.c b/net/bridge/br_mrp_switchdev.c
new file mode 100644
index 000000000000..2226d98806de
--- /dev/null
+++ b/net/bridge/br_mrp_switchdev.c
@@ -0,0 +1,147 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <net/switchdev.h>
+
+#include "br_private_mrp.h"
+
+int br_mrp_port_switchdev_add(struct net_bridge_port *p, u32 ring_nr)
+{
+	struct net_bridge *br = p->br;
+	struct switchdev_obj_port_mrp mrp = {
+		.obj.orig_dev = br->dev,
+		.obj.id = SWITCHDEV_OBJ_ID_PORT_MRP,
+		.port = p->dev,
+		.ring_nr = ring_nr,
+	};
+	int err = 0;
+
+	err = switchdev_port_obj_add(br->dev, &mrp.obj, NULL);
+
+	if (err && err != -EOPNOTSUPP)
+		return err;
+
+	return 0;
+}
+
+int br_mrp_switchdev_set_ring_role(struct br_mrp *mrp,
+				   enum br_mrp_ring_role_type role)
+{
+	struct switchdev_obj_ring_role_mrp mrp_role = {
+		.obj.orig_dev = mrp->br->dev,
+		.obj.id = SWITCHDEV_OBJ_ID_RING_ROLE_MRP,
+		.ring_role = role,
+		.ring_nr = mrp->ring_nr,
+	};
+	int err = 0;
+
+	pr_info("%s role: %d\n", __func__, role);
+
+	if (role == BR_MRP_RING_ROLE_DISABLED)
+		err = switchdev_port_obj_del(mrp->br->dev, &mrp_role.obj);
+	else
+		err = switchdev_port_obj_add(mrp->br->dev, &mrp_role.obj, NULL);
+
+	if (err && err != -EOPNOTSUPP)
+		return err;
+
+	return 0;
+}
+
+int br_mrp_switchdev_send_ring_test(struct br_mrp *mrp, u32 interval,
+				    u8 max_miss)
+{
+	struct switchdev_obj_ring_test_mrp test = {
+		.obj.orig_dev = mrp->br->dev,
+		.obj.id = SWITCHDEV_OBJ_ID_RING_TEST_MRP,
+		.interval = interval,
+		.max_miss = max_miss,
+		.ring_nr = mrp->ring_nr,
+	};
+	int err = 0;
+
+	if (interval == 0)
+		err = switchdev_port_obj_del(mrp->br->dev, &test.obj);
+	else
+		err = switchdev_port_obj_add(mrp->br->dev, &test.obj, NULL);
+
+	return err;
+}
+
+int br_mrp_port_switchdev_del(struct net_bridge_port *p, u32 ring_nr)
+{
+	struct net_bridge *br = p->br;
+	struct switchdev_obj_port_mrp mrp = {
+		.obj.orig_dev = br->dev,
+		.obj.id = SWITCHDEV_OBJ_ID_PORT_MRP,
+		.port = p->dev,
+		.ring_nr = ring_nr,
+	};
+	int err = 0;
+
+	err = switchdev_port_obj_del(br->dev, &mrp.obj);
+
+	if (err && err != -EOPNOTSUPP)
+		return err;
+
+	return 0;
+}
+
+int br_mrp_port_switchdev_set_state(struct net_bridge_port *p,
+				    enum br_mrp_port_state_type state)
+{
+	struct switchdev_attr attr = {
+		.orig_dev = p->dev,
+		.id = SWITCHDEV_ATTR_ID_MRP_PORT_STATE,
+		.u.mrp_port_state = state,
+	};
+	int err = 0;
+
+	pr_info("%s port: %s, state: %d\n", __func__, p->dev->name, state);
+
+	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);
+
+	return err;
+}
+
+int br_mrp_port_switchdev_set_role(struct net_bridge_port *p,
+				   enum br_mrp_port_role_type role)
+{
+	struct switchdev_attr attr = {
+		.orig_dev = p->dev,
+		.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;
+
+	return 0;
+}
+
+int br_mrp_switchdev_set_ring_state(struct br_mrp *mrp,
+				    enum br_mrp_ring_state_type state)
+{
+	struct switchdev_attr attr = {
+		.id = SWITCHDEV_ATTR_ID_MRP_RING_STATE,
+		.u.mrp_ring_state = state,
+	};
+	int err = 0;
+
+	attr.orig_dev = mrp->p_port->dev,
+	err = switchdev_port_attr_set(mrp->p_port->dev, &attr);
+	if (err && err != -EOPNOTSUPP)
+		return err;
+
+	attr.orig_dev = mrp->s_port->dev;
+	err = switchdev_port_attr_set(mrp->s_port->dev, &attr);
+	if (err && err != -EOPNOTSUPP)
+		return err;
+
+	return err;
+}
+
-- 
2.17.1

[RFC net-next v3 08/10] net: bridge: mrp: Connect MRP api with the switchev API

[Horatiu Vultur]

Implement the MRP api.
The functions are just a wrapper over the switchdev API with extra checks
regarding the MRP instances and ports.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_mrp.c | 193 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 193 insertions(+)
 create mode 100644 net/bridge/br_mrp.c

diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
new file mode 100644
index 000000000000..69e1a3e526d5
--- /dev/null
+++ b/net/bridge/br_mrp.c
@@ -0,0 +1,193 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include "br_private_mrp.h"
+
+static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_nr)
+{
+	struct br_mrp *mrp;
+
+	list_for_each_entry(mrp, &br->mrp_list, list) {
+		if (mrp->ring_nr == ring_nr)
+			return mrp;
+	}
+
+	return NULL;
+}
+
+static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
+				       struct net_bridge_port *p)
+{
+	struct br_mrp *mrp;
+
+	list_for_each_entry(mrp, &br->mrp_list, list) {
+		if (mrp->p_port == p || mrp->s_port == p)
+			return mrp;
+	}
+
+	return NULL;
+}
+
+int br_mrp_add(struct net_bridge *br, u32 ring_nr)
+{
+	struct br_mrp *mrp;
+
+	/* If the ring exists, it is not possible to create another one with the
+	 * same ring_nr
+	 */
+	mrp = br_mrp_find_id(br, ring_nr);
+	if (mrp)
+		return -EINVAL;
+
+	mrp = devm_kzalloc(&br->dev->dev, sizeof(struct br_mrp), GFP_KERNEL);
+	if (!mrp)
+		return -ENOMEM;
+
+	mrp->br = br;
+	mrp->p_port = NULL;
+	mrp->s_port = NULL;
+	mrp->ring_nr = ring_nr;
+
+	list_add_tail(&mrp->list, &br->mrp_list);
+
+	return 0;
+}
+
+int br_mrp_add_port(struct net_bridge *br, u32 ring_nr,
+		    struct net_bridge_port *p)
+{
+	struct br_mrp *mrp = br_mrp_find_id(br, ring_nr);
+
+	if (!mrp)
+		return -EINVAL;
+
+	p->state = BR_STATE_FORWARDING;
+	p->mrp_aware = true;
+
+	br_mrp_port_switchdev_add(p, mrp->ring_nr);
+
+	return 0;
+}
+
+int br_mrp_del(struct net_bridge *br, u32 ring_nr)
+{
+	struct br_mrp *mrp = br_mrp_find_id(br, ring_nr);
+
+	if (!mrp)
+		return -EINVAL;
+
+	/* Stop sending MRP_Test frames */
+	br_mrp_switchdev_send_ring_test(mrp, 0, 0);
+
+	/* Destroy the ring */
+	mrp->br = NULL;
+	mrp->p_port = NULL;
+	mrp->s_port = NULL;
+
+	list_del(&mrp->list);
+	devm_kfree(&br->dev->dev, mrp);
+
+	return 0;
+}
+
+int br_mrp_del_port(struct net_bridge_port *p)
+{
+	struct net_bridge *br;
+	struct br_mrp *mrp;
+
+	br = p->br;
+	mrp = br_mrp_find_port(br, p);
+	if (!mrp)
+		return 0;
+
+	/* Stop sending MRP_Test frames */
+	br_mrp_switchdev_send_ring_test(mrp, 0, 0);
+
+	p->state = BR_STATE_FORWARDING;
+	p->mrp_aware = false;
+	if (p == mrp->p_port)
+		mrp->p_port = NULL;
+	if (p == mrp->s_port)
+		mrp->s_port = NULL;
+
+	br_mrp_port_switchdev_del(p, mrp->ring_nr);
+
+	return 0;
+}
+
+int br_mrp_set_port_state(struct net_bridge_port *p,
+			  enum br_mrp_port_state_type state)
+{
+	struct net_bridge *br;
+
+	br = p->br;
+	if (state == BR_MRP_PORT_STATE_FORWARDING)
+		p->state = BR_STATE_FORWARDING;
+	else
+		p->state = BR_STATE_BLOCKING;
+
+	br_mrp_port_switchdev_set_state(p, state);
+
+	return 0;
+}
+
+int br_mrp_set_port_role(struct net_bridge_port *p, u32 ring_nr,
+			 enum br_mrp_port_role_type role)
+{
+	struct br_mrp *mrp = br_mrp_find_id(p->br, ring_nr);
+
+	if (!mrp)
+		return -EINVAL;
+
+	if (role == BR_MRP_PORT_ROLE_PRIMARY)
+		mrp->p_port = p;
+	if (role == BR_MRP_PORT_ROLE_SECONDARY)
+		mrp->s_port = p;
+
+	br_mrp_port_switchdev_set_role(p, role);
+
+	return 0;
+}
+
+int br_mrp_set_ring_state(struct net_bridge *br, u32 ring_nr,
+			  enum br_mrp_ring_state_type state)
+{
+	struct br_mrp *mrp = br_mrp_find_id(br, ring_nr);
+
+	if (!mrp)
+		return -EINVAL;
+
+	br_mrp_switchdev_set_ring_state(mrp, state);
+
+	return 0;
+}
+
+int br_mrp_set_ring_role(struct net_bridge *br, u32 ring_nr,
+			 enum br_mrp_ring_role_type role)
+{
+	struct br_mrp *mrp = br_mrp_find_id(br, ring_nr);
+
+	if (!mrp)
+		return -EINVAL;
+
+	br_mrp_switchdev_set_ring_role(mrp, role);
+
+	return 0;
+}
+
+int br_mrp_start_test(struct net_bridge *br, u32 ring_nr, u32 interval,
+		      u8 max_miss)
+{
+	struct br_mrp *mrp = br_mrp_find_id(br, ring_nr);
+
+	if (!mrp)
+		return -EINVAL;
+
+	return br_mrp_switchdev_send_ring_test(mrp, interval, max_miss);
+}
+
+int br_mrp_flush(struct net_bridge *br, u32 ring_nr)
+{
+	br_fdb_flush(br);
+	return 0;
+}
+
-- 
2.17.1

[RFC net-next v3 09/10] net: bridge: mrp: Integrate MRP into the bridge

[Horatiu Vultur]

To integrate MRP into the bridge, the bridge needs to do the following:
- initialized and destroy the generic netlink used by MRP
- detect if the MRP frame was received on a port that is part of a MRP ring. In
  case it was not, then forward the frame as usual, otherwise redirect the frame
  to the upper layer.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br.c         | 11 +++++++++++
 net/bridge/br_device.c  |  3 +++
 net/bridge/br_if.c      |  6 ++++++
 net/bridge/br_input.c   | 14 ++++++++++++++
 net/bridge/br_private.h | 14 ++++++++++++++
 5 files changed, 48 insertions(+)

diff --git a/net/bridge/br.c b/net/bridge/br.c
index b6fe30e3768f..d5e556eed4ba 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -344,6 +344,12 @@ static int __init br_init(void)
 	if (err)
 		goto err_out5;
 
+#ifdef CONFIG_BRIDGE_MRP
+	err = br_mrp_netlink_init();
+	if (err)
+		goto err_out6;
+#endif
+
 	brioctl_set(br_ioctl_deviceless_stub);
 
 #if IS_ENABLED(CONFIG_ATM_LANE)
@@ -358,6 +364,11 @@ static int __init br_init(void)
 
 	return 0;
 
+#ifdef CONFIG_BRIDGE_MRP
+err_out6:
+	br_netlink_fini();
+#endif
+
 err_out5:
 	unregister_switchdev_notifier(&br_switchdev_notifier);
 err_out4:
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index fb38add21b37..29966754d86a 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -464,6 +464,9 @@ void br_dev_setup(struct net_device *dev)
 	spin_lock_init(&br->lock);
 	INIT_LIST_HEAD(&br->port_list);
 	INIT_HLIST_HEAD(&br->fdb_list);
+#ifdef CONFIG_BRIDGE_MRP
+	INIT_LIST_HEAD(&br->mrp_list);
+#endif
 	spin_lock_init(&br->hash_lock);
 
 	br->bridge_id.prio[0] = 0x80;
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 4fe30b182ee7..9b8bb41c0574 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -331,6 +331,9 @@ static void del_nbp(struct net_bridge_port *p)
 
 	spin_lock_bh(&br->lock);
 	br_stp_disable_port(p);
+#ifdef CONFIG_BRIDGE_MRP
+	p->mrp_aware = false;
+#endif
 	spin_unlock_bh(&br->lock);
 
 	br_ifinfo_notify(RTM_DELLINK, NULL, p);
@@ -427,6 +430,9 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
 	p->port_no = index;
 	p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
 	br_init_port(p);
+#ifdef CONFIG_BRIDGE_MRP
+	p->mrp_aware = false;
+#endif
 	br_set_state(p, BR_STATE_DISABLED);
 	br_stp_port_timer_init(p);
 	err = br_multicast_add_port(p);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 8944ceb47fe9..de7066b077e2 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -21,6 +21,9 @@
 #include <linux/rculist.h>
 #include "br_private.h"
 #include "br_private_tunnel.h"
+#ifdef CONFIG_BRIDGE_MRP
+#include "br_private_mrp.h"
+#endif
 
 static int
 br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb)
@@ -338,6 +341,17 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
 			return RX_HANDLER_CONSUMED;
 		}
 	}
+#ifdef CONFIG_BRIDGE_MRP
+	/* If there is no MRP instance do normal forwarding */
+	if (!p->mrp_aware)
+		goto forward;
+
+	if (skb->protocol == htons(ETH_P_MRP))
+		return RX_HANDLER_PASS;
+
+	if (p->state == BR_STATE_BLOCKING)
+		goto drop;
+#endif
 
 forward:
 	switch (p->state) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f540f3bdf294..a5d01a394f54 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -285,6 +285,10 @@ struct net_bridge_port {
 	u16				backup_redirected_cnt;
 
 	struct bridge_stp_xstats	stp_xstats;
+
+#ifdef CONFIG_BRIDGE_MRP
+	bool				mrp_aware;
+#endif
 };
 
 #define kobj_to_brport(obj)	container_of(obj, struct net_bridge_port, kobj)
@@ -424,6 +428,10 @@ struct net_bridge {
 	int offload_fwd_mark;
 #endif
 	struct hlist_head		fdb_list;
+
+#ifdef CONFIG_BRIDGE_MRP
+	struct list_head		mrp_list;
+#endif
 };
 
 struct br_input_skb_cb {
@@ -1165,6 +1173,12 @@ unsigned long br_timer_value(const struct timer_list *timer);
 extern int (*br_fdb_test_addr_hook)(struct net_device *dev, unsigned char *addr);
 #endif
 
+/* br_mrp.c */
+#ifdef CONFIG_BRIDGE_MRP
+int br_mrp_netlink_init(void);
+void br_mrp_netlink_uninit(void);
+#endif
+
 /* br_netlink.c */
 extern struct rtnl_link_ops br_link_ops;
 int br_netlink_init(void);
-- 
2.17.1

Re: [RFC net-next v3 09/10] net: bridge: mrp: Integrate MRP into the bridge

[Andrew Lunn]

On Fri, Jan 24, 2020 at 05:18:27PM +0100, Horatiu Vultur wrote:
> To integrate MRP into the bridge, the bridge needs to do the following:
> - initialized and destroy the generic netlink used by MRP
> - detect if the MRP frame was received on a port that is part of a MRP ring. In
>   case it was not, then forward the frame as usual, otherwise redirect the frame
>   to the upper layer.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  net/bridge/br.c         | 11 +++++++++++
>  net/bridge/br_device.c  |  3 +++
>  net/bridge/br_if.c      |  6 ++++++
>  net/bridge/br_input.c   | 14 ++++++++++++++
>  net/bridge/br_private.h | 14 ++++++++++++++
>  5 files changed, 48 insertions(+)
> 
> diff --git a/net/bridge/br.c b/net/bridge/br.c
> index b6fe30e3768f..d5e556eed4ba 100644
> --- a/net/bridge/br.c
> +++ b/net/bridge/br.c
> @@ -344,6 +344,12 @@ static int __init br_init(void)
>  	if (err)
>  		goto err_out5;
>  
> +#ifdef CONFIG_BRIDGE_MRP
> +	err = br_mrp_netlink_init();
> +	if (err)
> +		goto err_out6;
> +#endif

Please try to avoid #ifdef's like this in C code. Add a stub function
to br_private_mrp.h.

If you really cannot avoid #ifdef, please use #if IS_ENABLED(CONFIG_BRIDGE_MRP).
That expands to

	if (0) {

        }

So the compiler will compile it and then optimize it out. That gives
us added benefit of build testing, we don't suddenly find the code no
longer compiles when we enable the option.

> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -21,6 +21,9 @@
>  #include <linux/rculist.h>
>  #include "br_private.h"
>  #include "br_private_tunnel.h"
> +#ifdef CONFIG_BRIDGE_MRP
> +#include "br_private_mrp.h"
> +#endif

It should always be safe to include a header file.

   Andrew

Re: [RFC net-next v3 09/10] net: bridge: mrp: Integrate MRP into the bridge

[Horatiu Vultur]

The 01/25/2020 16:42, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Jan 24, 2020 at 05:18:27PM +0100, Horatiu Vultur wrote:
> > To integrate MRP into the bridge, the bridge needs to do the following:
> > - initialized and destroy the generic netlink used by MRP
> > - detect if the MRP frame was received on a port that is part of a MRP ring. In
> >   case it was not, then forward the frame as usual, otherwise redirect the frame
> >   to the upper layer.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  net/bridge/br.c         | 11 +++++++++++
> >  net/bridge/br_device.c  |  3 +++
> >  net/bridge/br_if.c      |  6 ++++++
> >  net/bridge/br_input.c   | 14 ++++++++++++++
> >  net/bridge/br_private.h | 14 ++++++++++++++
> >  5 files changed, 48 insertions(+)
> >
> > diff --git a/net/bridge/br.c b/net/bridge/br.c
> > index b6fe30e3768f..d5e556eed4ba 100644
> > --- a/net/bridge/br.c
> > +++ b/net/bridge/br.c
> > @@ -344,6 +344,12 @@ static int __init br_init(void)
> >       if (err)
> >               goto err_out5;
> >
> > +#ifdef CONFIG_BRIDGE_MRP
> > +     err = br_mrp_netlink_init();
> > +     if (err)
> > +             goto err_out6;
> > +#endif
> 
> Please try to avoid #ifdef's like this in C code. Add a stub function
> to br_private_mrp.h.
> 
> If you really cannot avoid #ifdef, please use #if IS_ENABLED(CONFIG_BRIDGE_MRP).
> That expands to
> 
>         if (0) {
> 
>         }
> 
> So the compiler will compile it and then optimize it out. That gives
> us added benefit of build testing, we don't suddenly find the code no
> longer compiles when we enable the option.
> 
> > --- a/net/bridge/br_input.c
> > +++ b/net/bridge/br_input.c
> > @@ -21,6 +21,9 @@
> >  #include <linux/rculist.h>
> >  #include "br_private.h"
> >  #include "br_private_tunnel.h"
> > +#ifdef CONFIG_BRIDGE_MRP
> > +#include "br_private_mrp.h"
> > +#endif
> 
> It should always be safe to include a header file.
> 
>    Andrew

Thanks for pointing out these mistakes. I will try to avoid all these
#ifdef's in the next patch series.

-- 
/Horatiu

Re: [RFC net-next v3 09/10] net: bridge: mrp: Integrate MRP into the bridge

[Andrew Lunn]

>  br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb)
> @@ -338,6 +341,17 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
>  			return RX_HANDLER_CONSUMED;
>  		}
>  	}
> +#ifdef CONFIG_BRIDGE_MRP
> +	/* If there is no MRP instance do normal forwarding */
> +	if (!p->mrp_aware)
> +		goto forward;
> +
> +	if (skb->protocol == htons(ETH_P_MRP))
> +		return RX_HANDLER_PASS;

What MAC address is used for these MRP frames? It would make sense to
use a L2 link local destination address, since i assume they are not
supposed to be forwarded by the bridge. If so, you could extend the
if (unlikely(is_link_local_ether_addr(dest))) condition.

> +
> +	if (p->state == BR_STATE_BLOCKING)
> +		goto drop;
> +#endif

Is this needed? The next block of code is a switch statement on
p->state. The default case, which BR_STATE_BLOCKING should hit, is
drop.

This function is on the hot path. So we should try to optimize it as
much as possible.

     Andrew

Re: [RFC net-next v3 09/10] net: bridge: mrp: Integrate MRP into the bridge

[Horatiu Vultur]

The 01/25/2020 17:16, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> >  br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb)
> > @@ -338,6 +341,17 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >                       return RX_HANDLER_CONSUMED;
> >               }
> >       }
> > +#ifdef CONFIG_BRIDGE_MRP
> > +     /* If there is no MRP instance do normal forwarding */
> > +     if (!p->mrp_aware)
> > +             goto forward;
> > +
> > +     if (skb->protocol == htons(ETH_P_MRP))
> > +             return RX_HANDLER_PASS;
> 
> What MAC address is used for these MRP frames? It would make sense to
> use a L2 link local destination address, since i assume they are not
> supposed to be forwarded by the bridge. If so, you could extend the
> if (unlikely(is_link_local_ether_addr(dest))) condition.

The MAC addresses used by MRP frames are:
0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 - used by MRP_Test frames
0x1, 0x15, 0x4e, 0x0, 0x0, 0x2 - used by the rest of MRP frames.

If we will add support also for MIM/MIC. These requires 2 more MAC
addresses:
0x1, 0x15, 0x4e, 0x0, 0x0, 0x3 - used by MRP_InTest frames.
0x1, 0x15, 0x4e, 0x0, 0x0, 0x4 - used by the other MRP interconnect
frames.

Then maybe I shoukd change the check to be something like:
if (unlikely(skb->protocol == htons(ETH_P_MRP)))

> 
> > +
> > +     if (p->state == BR_STATE_BLOCKING)
> > +             goto drop;
> > +#endif
> 
> Is this needed? The next block of code is a switch statement on
> p->state. The default case, which BR_STATE_BLOCKING should hit, is
> drop.

Yes you are rigth, it is not needed anymore.

> 
> This function is on the hot path. So we should try to optimize it as
> much as possible.
> 
>      Andrew

-- 
/Horatiu

Re: [RFC net-next v3 09/10] net: bridge: mrp: Integrate MRP into the bridge

[Andrew Lunn]

On Sun, Jan 26, 2020 at 02:01:11PM +0100, Horatiu Vultur wrote:
> The 01/25/2020 17:16, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > >  br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb)
> > > @@ -338,6 +341,17 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> > >                       return RX_HANDLER_CONSUMED;
> > >               }
> > >       }
> > > +#ifdef CONFIG_BRIDGE_MRP
> > > +     /* If there is no MRP instance do normal forwarding */
> > > +     if (!p->mrp_aware)
> > > +             goto forward;
> > > +
> > > +     if (skb->protocol == htons(ETH_P_MRP))
> > > +             return RX_HANDLER_PASS;
> > 
> > What MAC address is used for these MRP frames? It would make sense to
> > use a L2 link local destination address, since i assume they are not
> > supposed to be forwarded by the bridge. If so, you could extend the
> > if (unlikely(is_link_local_ether_addr(dest))) condition.
> 
> The MAC addresses used by MRP frames are:
> 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 - used by MRP_Test frames
> 0x1, 0x15, 0x4e, 0x0, 0x0, 0x2 - used by the rest of MRP frames.
> 
> If we will add support also for MIM/MIC. These requires 2 more MAC
> addresses:
> 0x1, 0x15, 0x4e, 0x0, 0x0, 0x3 - used by MRP_InTest frames.
> 0x1, 0x15, 0x4e, 0x0, 0x0, 0x4 - used by the other MRP interconnect
> frames.

Hi Horatiu

I made the wrong guess about how this protocol worked when i said L2
link local. These MAC addresses are L2 multicast.

And you are using a raw socket to receive them into userspace when
needed.

'Thinking allowed' here.

    +------------------------------------------+
    |                                          |
    +-->|H1|<---------->|H2|<---------->|H3|<--+
    eth0    eth1    eth0    eth1    eth0    eth1
     ^
     |
  Blocked


There are three major classes of user case here:

1) Pure software solution

You need the software bridge in the client to forward these frames
from the left side to the right side. (Does the standard give these
two ports names)? In the master, the left port is blocked, so the
bridge drops them anyway. You have a RAW socket open on both eth0 and
eth1, so you get to see the frames, even if the bridge drops them.

2) Hardware offload to an MRP unaware switch.

I'm thinking about a plain switch supported by DSA, Marvell, Broadcom,
etc. It has no special knowledge of MRP.

Ideally, you want the switch to forward MRP_Test frames left to right
for a client. In a master, i think you have a problem, since the port
is blocked. The hardware is unlikely to recognise these frames as
special, since they are not in the 01-80-C2-XX-XX-XX block, and let
them through. So your raw socket is never going to see them, and you
cannot detect open/closed ring.

I don't know how realistic it is to support MRP in this case, and i
also don't think you can fall back to a pure software solution,
because the software bridge is going to offload the basic bridge
operation to the hardware. It would be nice if you could detect this,
and return -EOPNOTSUPP.

3) Hardware offload to an MRP aware switch.

For a client, you tell it which port is left, which is right, and
assume it forwards the frames. For a master, you again tell it which
is left, which is right, and ask it send MRP_Test frames out right,
and report changes in open/closed on the right port. You don't need
the CPU to see the MRP_Test frames, so the switch has no need to
forward them to the CPU.

We should think about the general case of a bridge with many ports,
and many pairs of ports using MRP. This makes the forwarding of these
frames interesting. Given that they are multicast, the default action
of the software bridge is that it will flood them. Does the protocol
handle seeing MRP_Test from some other loop? Do we need to avoid this?
You could avoid this by adding MDB entries to the bridge. However,
this does not scale to more then one ring. I don't think an MDB is
associated to an ingress port. So you cannot say 

0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 ingress port1 egress port2
0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 ingress port3 egress port4

The best you can say is

0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 egress port2, port4

I'm sure there are other issues i'm missing, but it is interesting to
think about all this.

Andrew

Re: [RFC net-next v3 09/10] net: bridge: mrp: Integrate MRP into the bridge

[Allan W. Nielsen]

On 26.01.2020 18:12, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Sun, Jan 26, 2020 at 02:01:11PM +0100, Horatiu Vultur wrote:
> > The 01/25/2020 17:16, Andrew Lunn wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > >
> > > >  br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb)
> > > > @@ -338,6 +341,17 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> > > >                       return RX_HANDLER_CONSUMED;
> > > >               }
> > > >       }
> > > > +#ifdef CONFIG_BRIDGE_MRP
> > > > +     /* If there is no MRP instance do normal forwarding */
> > > > +     if (!p->mrp_aware)
> > > > +             goto forward;
> > > > +
> > > > +     if (skb->protocol == htons(ETH_P_MRP))
> > > > +             return RX_HANDLER_PASS;
> > >
> > > What MAC address is used for these MRP frames? It would make sense to
> > > use a L2 link local destination address, since i assume they are not
> > > supposed to be forwarded by the bridge. If so, you could extend the
> > > if (unlikely(is_link_local_ether_addr(dest))) condition.
> >
> > The MAC addresses used by MRP frames are:
> > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 - used by MRP_Test frames
> > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x2 - used by the rest of MRP frames.
> >
> > If we will add support also for MIM/MIC. These requires 2 more MAC
> > addresses:
> > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x3 - used by MRP_InTest frames.
> > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x4 - used by the other MRP interconnect
> > frames.
> 
> Hi Horatiu
> 
> I made the wrong guess about how this protocol worked when i said L2
> link local. These MAC addresses are L2 multicast.
> 
> And you are using a raw socket to receive them into userspace when
> needed.
> 
> 'Thinking allowed' here.
> 
>     +------------------------------------------+
>     |                                          |
>     +-->|H1|<---------->|H2|<---------->|H3|<--+
>     eth0    eth1    eth0    eth1    eth0    eth1
>      ^
>      |
>   Blocked
> 
> 
> There are three major classes of user case here:
> 
> 1) Pure software solution
> You need the software bridge in the client to forward these frames
> from the left side to the right side.
As far as I understand it is not the bridge which forward these frames -
it is the user-space tool. This was to put as much functionality in
user-space and only use the kernel to configure the HW. We can (and
should) discuss if this is the right decision.

> (Does the standard give these two ports names)?
Horatiu?

> In the master, the left port is blocked, so the bridge drops them
> anyway. You have a RAW socket open on both eth0 and eth1, so you get
> to see the frames, even if the bridge drops them.
Yes, in the current patch-set such frames are forwarded by the
user-space daemon.

We would properly have better performance if we do this in kernel-space.


> 2) Hardware offload to an MRP unaware switch.
> 
> I'm thinking about a plain switch supported by DSA, Marvell, Broadcom,
> etc. It has no special knowledge of MRP.
We have implemented this on Ocelot - which is not MRP aware at all. Not
sure what facilities Marvell and Broadcom has, but it is not a lot which
is needed.

> Ideally, you want the switch to forward MRP_Test frames left to right
> for a client.
Yes. If we have only 1 ring, then we can do that with a MAC table entry.
If we have more than 1 ring, then we will need a TCAM rule of some kind.

In the what we have today on Ocelot, we do not do this is HW, we do the
forwarding in SW.

BTW: It is not only from left to right, it is also from right to left.
The MRM will inject packets on both ring ports, and monitor both. This
is to detect asymmetrical link down or similar. The two ports are
treated the same. But you can set a priority (the primary/secondary) to
state your preference on what port to use if both are up and the ring is
closed.

> In a master, i think you have a problem, since the port
> is blocked. The hardware is unlikely to recognise these frames as
> special, since they are not in the 01-80-C2-XX-XX-XX block, and let
> them through. So your raw socket is never going to see them, and you
> cannot detect open/closed ring.
Again, I do not know how other HW is designed, but all the SOC's we are
working with, does allow us to add a TCAM rule which can redirect these
frames to the CPU even on a blocked port.

> I don't know how realistic it is to support MRP in this case, and i
> also don't think you can fall back to a pure software solution,
> because the software bridge is going to offload the basic bridge
> operation to the hardware. It would be nice if you could detect this,
> and return -EOPNOTSUPP.
We do want to support this on Ocelot, but you are right, if the current
running bridge, cannot block a port, and still get the MRP frames on
that port, then it cannot support MRM. And this we need to detect in
some way.

> 3) Hardware offload to an MRP aware switch.
> 
> For a client, you tell it which port is left, which is right, and
> assume it forwards the frames. For a master, you again tell it which
> is left, which is right, and ask it send MRP_Test frames out right,
> and report changes in open/closed on the right port. You don't need
> the CPU to see the MRP_Test frames, so the switch has no need to
> forward them to the CPU.
> 
> We should think about the general case of a bridge with many ports,
> and many pairs of ports using MRP. This makes the forwarding of these
> frames interesting. Given that they are multicast, the default action
> of the software bridge is that it will flood them. Does the protocol
> handle seeing MRP_Test from some other loop? Do we need to avoid this?
Yes, we need to avoid. We cannot "just" do normal flooding.

> You could avoid this by adding MDB entries to the bridge. However,
> this does not scale to more then one ring.
I would prefer a solution where the individual drivers can do what is
best on the given HW.

- If we have a 2 ported switch, then flooding seems like a perfect valid
   approach. There will be only 1 ring.
- If we have a many ported switch, then we could use MAC-table entry -
   if the user only configure 1 ring.
   - When adding more rings, it either needs to return error, or use
     other HW facilities.

> I don't think an MDB is associated to an ingress port. So you cannot
> say
Agree.

> 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 ingress port1 egress port2
> 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 ingress port3 egress port4
> 
> The best you can say is
> 
> 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 egress port2, port4
> 
> I'm sure there are other issues i'm missing, but it is interesting to
> think about all this.
Yes, the solution Horatiu has chosen, is not to forward MRP frames,
received in MRP ring ports at all. This is done by the user-space tool.

Again, not sure if this is the right way to do it, but it is what patch
v3 does.

The alternative to this would be to learn the bridge how to forward MRP
frames when it is a MRC. The user-space tool then never needs to do
this, it know that the kernel will take care of this part (either in SW
or in HW).

/Allan

Re: [RFC net-next v3 09/10] net: bridge: mrp: Integrate MRP into the bridge

[Horatiu Vultur]

The 01/27/2020 11:57, Allan W. Nielsen wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 26.01.2020 18:12, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Sun, Jan 26, 2020 at 02:01:11PM +0100, Horatiu Vultur wrote:
> > > The 01/25/2020 17:16, Andrew Lunn wrote:
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > >
> > > > >  br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb)
> > > > > @@ -338,6 +341,17 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> > > > >                       return RX_HANDLER_CONSUMED;
> > > > >               }
> > > > >       }
> > > > > +#ifdef CONFIG_BRIDGE_MRP
> > > > > +     /* If there is no MRP instance do normal forwarding */
> > > > > +     if (!p->mrp_aware)
> > > > > +             goto forward;
> > > > > +
> > > > > +     if (skb->protocol == htons(ETH_P_MRP))
> > > > > +             return RX_HANDLER_PASS;
> > > >
> > > > What MAC address is used for these MRP frames? It would make sense to
> > > > use a L2 link local destination address, since i assume they are not
> > > > supposed to be forwarded by the bridge. If so, you could extend the
> > > > if (unlikely(is_link_local_ether_addr(dest))) condition.
> > >
> > > The MAC addresses used by MRP frames are:
> > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 - used by MRP_Test frames
> > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x2 - used by the rest of MRP frames.
> > >
> > > If we will add support also for MIM/MIC. These requires 2 more MAC
> > > addresses:
> > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x3 - used by MRP_InTest frames.
> > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x4 - used by the other MRP interconnect
> > > frames.
> > 
> > Hi Horatiu
> > 
> > I made the wrong guess about how this protocol worked when i said L2
> > link local. These MAC addresses are L2 multicast.
> > 
> > And you are using a raw socket to receive them into userspace when
> > needed.
> > 
> > 'Thinking allowed' here.
> > 
> >     +------------------------------------------+
> >     |                                          |
> >     +-->|H1|<---------->|H2|<---------->|H3|<--+
> >     eth0    eth1    eth0    eth1    eth0    eth1
> >      ^
> >      |
> >   Blocked
> > 
> > 
> > There are three major classes of user case here:
> > 
> > 1) Pure software solution
> > You need the software bridge in the client to forward these frames
> > from the left side to the right side.
> As far as I understand it is not the bridge which forward these frames -
> it is the user-space tool. This was to put as much functionality in
> user-space and only use the kernel to configure the HW. We can (and
> should) discuss if this is the right decision.
> 
> > (Does the standard give these two ports names)?
> Horatiu?

They don't have a specific name, the standard names them as "ring
ports". And to differentiate between them, they have roles: primary or
secondary. These roles are used to know which port needs to be
blocked(the secondary) and which needs to forward the frames. One
observation, these roles are not fix for the entire time. The ring ports
can interchange their roles. For example if eth0 is the primary port and
eth1 is the secondary port and then the eth0 link goes down then eth0
will have the secondary role and eth1 will become primary port.

> 
> > In the master, the left port is blocked, so the bridge drops them
> > anyway. You have a RAW socket open on both eth0 and eth1, so you get
> > to see the frames, even if the bridge drops them.
> Yes, in the current patch-set such frames are forwarded by the
> user-space daemon.
> 
> We would properly have better performance if we do this in kernel-space.
> 
> 
> > 2) Hardware offload to an MRP unaware switch.
> > 
> > I'm thinking about a plain switch supported by DSA, Marvell, Broadcom,
> > etc. It has no special knowledge of MRP.
> We have implemented this on Ocelot - which is not MRP aware at all. Not
> sure what facilities Marvell and Broadcom has, but it is not a lot which
> is needed.

Here is a small confusion. The implementation that we have done on
Ocelot doesn't have at all HW offload(I have hacked the network driver
to remove this support, so basically is just 4 NICs). Therefor all the
non-MRP frames switching were done by the SW bridge and forwarding of
MRP frames were done in the userspace.

> 
> > Ideally, you want the switch to forward MRP_Test frames left to right
> > for a client.
> Yes. If we have only 1 ring, then we can do that with a MAC table entry.
> If we have more than 1 ring, then we will need a TCAM rule of some kind.
> 
> In the what we have today on Ocelot, we do not do this is HW, we do the
> forwarding in SW.
> 
> BTW: It is not only from left to right, it is also from right to left.
> The MRM will inject packets on both ring ports, and monitor both. This
> is to detect asymmetrical link down or similar. The two ports are
> treated the same. But you can set a priority (the primary/secondary) to
> state your preference on what port to use if both are up and the ring is
> closed.

A small observation, the primary/secondary are defined in the standard
as roles and not priority. And yes it uses this role(primary/secondary)
to decide which port to block.

> 
> > In a master, i think you have a problem, since the port
> > is blocked. The hardware is unlikely to recognise these frames as
> > special, since they are not in the 01-80-C2-XX-XX-XX block, and let
> > them through. So your raw socket is never going to see them, and you
> > cannot detect open/closed ring.
> Again, I do not know how other HW is designed, but all the SOC's we are
> working with, does allow us to add a TCAM rule which can redirect these
> frames to the CPU even on a blocked port.
> 
> > I don't know how realistic it is to support MRP in this case, and i
> > also don't think you can fall back to a pure software solution,
> > because the software bridge is going to offload the basic bridge
> > operation to the hardware. It would be nice if you could detect this,
> > and return -EOPNOTSUPP.
> We do want to support this on Ocelot, but you are right, if the current
> running bridge, cannot block a port, and still get the MRP frames on
> that port, then it cannot support MRM. And this we need to detect in
> some way.
> 
> > 3) Hardware offload to an MRP aware switch.
> > 
> > For a client, you tell it which port is left, which is right, and
> > assume it forwards the frames. For a master, you again tell it which
> > is left, which is right, and ask it send MRP_Test frames out right,
> > and report changes in open/closed on the right port. You don't need
> > the CPU to see the MRP_Test frames, so the switch has no need to
> > forward them to the CPU.
> > 
> > We should think about the general case of a bridge with many ports,
> > and many pairs of ports using MRP. This makes the forwarding of these
> > frames interesting. Given that they are multicast, the default action
> > of the software bridge is that it will flood them. Does the protocol
> > handle seeing MRP_Test from some other loop? Do we need to avoid this?
> Yes, we need to avoid. We cannot "just" do normal flooding.
> 
> > You could avoid this by adding MDB entries to the bridge. However,
> > this does not scale to more then one ring.
> I would prefer a solution where the individual drivers can do what is
> best on the given HW.
> 
> - If we have a 2 ported switch, then flooding seems like a perfect valid
>   approach. There will be only 1 ring.
> - If we have a many ported switch, then we could use MAC-table entry -
>   if the user only configure 1 ring.
>   - When adding more rings, it either needs to return error, or use
>     other HW facilities.
> 
> > I don't think an MDB is associated to an ingress port. So you cannot
> > say
> Agree.
> 
> > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 ingress port1 egress port2
> > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 ingress port3 egress port4
> > 
> > The best you can say is
> > 
> > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 egress port2, port4
> > 
> > I'm sure there are other issues i'm missing, but it is interesting to
> > think about all this.
> Yes, the solution Horatiu has chosen, is not to forward MRP frames,
> received in MRP ring ports at all. This is done by the user-space tool.
> 
> Again, not sure if this is the right way to do it, but it is what patch
> v3 does.
> 
> The alternative to this would be to learn the bridge how to forward MRP
> frames when it is a MRC. The user-space tool then never needs to do
> this, it know that the kernel will take care of this part (either in SW
> or in HW).
> 
> /Allan

-- 
/Horatiu

Re: [RFC net-next v3 09/10] net: bridge: mrp: Integrate MRP into the bridge

[Andrew Lunn]

> > 'Thinking allowed' here.
> > 
> >     +------------------------------------------+
> >     |                                          |
> >     +-->|H1|<---------->|H2|<---------->|H3|<--+
> >     eth0    eth1    eth0    eth1    eth0    eth1
> >      ^
> >      |
> >   Blocked
> > 
> > 
> > There are three major classes of user case here:
> > 
> > 1) Pure software solution
> > You need the software bridge in the client to forward these frames
> > from the left side to the right side.

> As far as I understand it is not the bridge which forward these frames -
> it is the user-space tool. This was to put as much functionality in
> user-space and only use the kernel to configure the HW. We can (and
> should) discuss if this is the right decision.

So i need to flip the point around. How does the software switch know
not to forward the frames? Are you adding an MDB?

> We would properly have better performance if we do this in kernel-space.

Yes, that is what i think. And if you can do it without any additional
code, using the forwarding tables, so much the better.

> BTW: It is not only from left to right, it is also from right to left.
> The MRM will inject packets on both ring ports, and monitor both.

Using the same MAC address in both directions? I need to think what
that implies for MDB entries. It probably just works, since you never
flood back out the ingress port.

> Again, I do not know how other HW is designed, but all the SOC's we are
> working with, does allow us to add a TCAM rule which can redirect these
> frames to the CPU even on a blocked port.

It is not in scope for what you are doing, but i wonder how we
describe this in a generic Linux way? And then how we push it down to
the hardware?

For the Marvell Switches, it might be possible to do this without the
TCAM. You can add forwarding DB entries marked as Management. It is
unclear if this overrides the blocked state, but it would be a bit odd
if it did not.

> > You could avoid this by adding MDB entries to the bridge. However,
> > this does not scale to more then one ring.
> I would prefer a solution where the individual drivers can do what is
> best on the given HW.

The nice thing about adding MDB is that it is making use of the
software bridge facilities. In general, the software bridge and
hardware bridges are pretty similar. If you can solve the problem
using generic software bridge features, not additional special cases
in code, you have good chance of being able to offload it to a
hardware bridge which is not MRP aware. The switchdev API for MRP
specific features should then allow you to make use of any additional
features the hardware might have.

> Yes, the solution Horatiu has chosen, is not to forward MRP frames,
> received in MRP ring ports at all. This is done by the user-space tool.
> 
> Again, not sure if this is the right way to do it, but it is what patch
> v3 does.
> 
> The alternative to this would be to learn the bridge how to forward MRP
> frames when it is a MRC. The user-space tool then never needs to do
> this, it know that the kernel will take care of this part (either in SW
> or in HW).

I think that should be considered. I'm not saying it is the best way,
just that some thought should be put into it to figure out what it
actually implies.

	 Andrew

Re: [RFC net-next v3 09/10] net: bridge: mrp: Integrate MRP into the bridge

[Jürgen Lambrecht]

On 1/27/20 2:40 PM, Andrew Lunn wrote:
>> Again, I do not know how other HW is designed, but all the SOC's we are
>> working with, does allow us to add a TCAM rule which can redirect these
>> frames to the CPU even on a blocked port.
> It is not in scope for what you are doing, but i wonder how we
> describe this in a generic Linux way? And then how we push it down to
> the hardware?
>
> For the Marvell Switches, it might be possible to do this without the
> TCAM. You can add forwarding DB entries marked as Management. It is
> unclear if this overrides the blocked state, but it would be a bit odd
> if it did not.
A MGMT frame does override the blocked state according the the datasheet.
And any MAC address can be loaded, not only 01:80:C2:00:00:0x (802.1D) and 01:80:C2:00:00:2x (GARP). Then the ATU is used instead of something specialized.
(referring to Andrew's email of 20200126 6:12 PM)
(I only checked again 88E6250/88E6220/88E6071/88E6070/88E6020 Functional Specification)


Kind regards,

Jürgen

Re: [RFC net-next v3 09/10] net: bridge: mrp: Integrate MRP into the bridge

[Allan W. Nielsen]

On 27.01.2020 14:40, Andrew Lunn wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> > 'Thinking allowed' here.
>> >
>> >     +------------------------------------------+
>> >     |                                          |
>> >     +-->|H1|<---------->|H2|<---------->|H3|<--+
>> >     eth0    eth1    eth0    eth1    eth0    eth1
>> >      ^
>> >      |
>> >   Blocked
>> >
>> >
>> > There are three major classes of user case here:
>> >
>> > 1) Pure software solution
>> > You need the software bridge in the client to forward these frames
>> > from the left side to the right side.
>
>> As far as I understand it is not the bridge which forward these frames -
>> it is the user-space tool. This was to put as much functionality in
>> user-space and only use the kernel to configure the HW. We can (and
>> should) discuss if this is the right decision.
>
>So i need to flip the point around. How does the software switch know
>not to forward the frames? Are you adding an MDB?
In the current implementation (patch v3) this is done here:
https://github.com/microchip-ung/mrp/blob/patch-v3/kernel-patches/v3-0009-net-bridge-mrp-Integrate-MRP-into-the-bridge.patch#L112

We simply ask the bridge not to forward any MRP frames, on MRP enabled
ports, and let "someone" else do that.

>> We would properly have better performance if we do this in kernel-space.
>
>Yes, that is what i think. And if you can do it without any additional
>code, using the forwarding tables, so much the better.
I understand the motivation of using the existing forwarding mechanism,
but I do not think we have all the hooks needed. But we can certainly
limit the impact on the existing code as much as possible.

>> BTW: It is not only from left to right, it is also from right to left.
>> The MRM will inject packets on both ring ports, and monitor both.
>
>Using the same MAC address in both directions? I need to think what
>that implies for MDB entries. It probably just works, since you never
>flood back out the ingress port.
Seems to work fine :-D

>> Again, I do not know how other HW is designed, but all the SOC's we are
>> working with, does allow us to add a TCAM rule which can redirect these
>> frames to the CPU even on a blocked port.
>
>It is not in scope for what you are doing, but i wonder how we
>describe this in a generic Linux way? And then how we push it down to
>the hardware?
>
>For the Marvell Switches, it might be possible to do this without the
>TCAM. You can add forwarding DB entries marked as Management. It is
>unclear if this overrides the blocked state, but it would be a bit odd
>if it did not.
Based on this, and also on the input from Jürgen, I think there is a
good chnage we can make this work for existing silicon from several
vendors.

>> > You could avoid this by adding MDB entries to the bridge. However,
>> > this does not scale to more then one ring.
>> I would prefer a solution where the individual drivers can do what is
>> best on the given HW.
>The nice thing about adding MDB is that it is making use of the
>software bridge facilities. In general, the software bridge and
>hardware bridges are pretty similar. If you can solve the problem
>using generic software bridge features, not additional special cases
>in code, you have good chance of being able to offload it to a
>hardware bridge which is not MRP aware. The switchdev API for MRP
>specific features should then allow you to make use of any additional
>features the hardware might have.
Yes, but the issues in using the MDB API for this is that it does not
allow to look at source ports, and it does not allow to update the
priority of the frames.

>> Yes, the solution Horatiu has chosen, is not to forward MRP frames,
>> received in MRP ring ports at all. This is done by the user-space tool.
>>
>> Again, not sure if this is the right way to do it, but it is what patch
>> v3 does.
>>
>> The alternative to this would be to learn the bridge how to forward MRP
>> frames when it is a MRC. The user-space tool then never needs to do
>> this, it know that the kernel will take care of this part (either in SW
>> or in HW).
>I think that should be considered. I'm not saying it is the best way,
>just that some thought should be put into it to figure out what it
>actually implies.
Sounds good - I will try to explain and illustrate this a bit better,
such that we all have the same understanding of the problem we need to
solve.

/Allan

[RFC net-next v3 10/10] net: bridge: mrp: Update Kconfig and Makefile

[Horatiu Vultur]

Add the option BRIDGE_MRP to allow to build in or not MRP support.
The default value is N.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/Kconfig  | 12 ++++++++++++
 net/bridge/Makefile |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/net/bridge/Kconfig b/net/bridge/Kconfig
index e4fb050e2078..51a6414145d2 100644
--- a/net/bridge/Kconfig
+++ b/net/bridge/Kconfig
@@ -61,3 +61,15 @@ config BRIDGE_VLAN_FILTERING
 	  Say N to exclude this support and reduce the binary size.
 
 	  If unsure, say Y.
+
+config BRIDGE_MRP
+	bool "MRP protocol"
+	depends on BRIDGE
+	default n
+	help
+	  If you say Y here, then the Ethernet bridge will be able to run MRP
+	  protocol to detect loops
+
+	  Say N to exclude this support and reduce the binary size.
+
+	  If unsure, say N.
diff --git a/net/bridge/Makefile b/net/bridge/Makefile
index ac9ef337f0fa..b2e11b819af5 100644
--- a/net/bridge/Makefile
+++ b/net/bridge/Makefile
@@ -25,3 +25,5 @@ bridge-$(CONFIG_BRIDGE_VLAN_FILTERING) += br_vlan.o br_vlan_tunnel.o
 bridge-$(CONFIG_NET_SWITCHDEV) += br_switchdev.o
 
 obj-$(CONFIG_NETFILTER) += netfilter/
+
+bridge-$(CONFIG_BRIDGE_MRP)	+= br_mrp.o br_mrp_netlink.o br_mrp_switchdev.o
-- 
2.17.1

Re: [RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[Allan W. Nielsen]

On 24.01.2020 17:18, Horatiu Vultur wrote:
>Media Redundancy Protocol is a data network protocol standardized by
>International Electrotechnical Commission as IEC 62439-2. It allows rings of
>Ethernet switches to overcome any single failure with recovery time faster than
>STP. It is primarily used in Industrial Ethernet applications.
>
>Based on the previous RFC[1][2], the MRP state machine and all the
>timers were moved to userspace. A generic netlink interface is added to
>allow configuring the HW, and logic added to to implement the MRP
>specific forwarding rules.
>
>The userspace application that is using the new netlink can be found here[3].
>
>The current implementation both in kernel and userspace supports only 2 roles:
>
>  MRM - this one is responsible to send MRP_Test and MRP_Topo frames on both
>  ring ports. It needs to process MRP_Test to know if the ring is open or
>  closed. This operation is desired to be offloaded to the HW because it
>  requires to generate and process up to 4000 frames per second. Whenever it
>  detects that the ring open it sends MRP_Topo frames to notify all MRC about
>  changes in the topology. MRM needs also to process MRP_LinkChange frames,
>  these frames are generated by the MRC. When the ring is open the the state
>  of both ports is to forward frames and when the ring is closed then the
>  secondary port is blocked.
>
>  MRC - this one is responsible to forward MRP frames between the ring ports.
>  In case one of the ring ports gets a link down or up, then MRC will generate
>  a MRP_LinkChange frames. This node should also process MRP_Topo frames and to
>  clear its FDB when it receives this frame.
>
> Userspace
>               Deamon +----------+ Client
>                +
>                |
> +--------------|-----------------------------------------+
>  Kernel        |
>                + Netlink
>
>                |                              + Interrupt
>                |                              |
> +--------------|------------------------------|----------+
>  HW            | Switchdev                    |
>                +                              |
>
>The user interacts using the client (called 'mrp'), the client talks to the
>deamon (called 'mrp_server'), which talks with the kernel using netlink. The
>kernel will try to offload the requests to the HW via switchdev API. For this a
>new generic netlink interface was added to the bridge.
>
>If the kernel cannot offload MRP to HW (maybe it does not have a switchdev
>driver, or it is just not supported), then all the netlink calls will return
>-EOPNOTSUPP. In this case the user-space deamon fallback to SW only
>implementation.
Horatiu and I have spend a bit of time discussing what you be best here.
An alternative to this would be to do the SW fallback in the kernel,
instead of user-land. This would mean that the user application does not
need to know if the function is offloaded (or partly offloaded) to HW.

We went with this approch to make the kernel part as simple as possible.
The alternative would still be much simpler than the first version
posted - but it would require a bit more.

Both options has pros and cons, and we looking forward to the
community's view on this.

>There are required changes to the SW bridge to be able to run the MRP. First the
>bridge needs to initialize the netlink interface. And second it needs to know if
>a MRP frame was received on a MRP ring port. In case it was received the SW
>bridge should not forward the frame it needs to redirected to upper layes. In
>case it was not received on a ring port then it just forwards it as usual.
>
>To be able to offload this to the HW, it was required to extend the switchdev
>API.
>
>If this will be accepted then in the future the netlink interface can be
>expended with multiple attributes which are required by different roles of the
>MRP. Like Media Redundancy Automanager(MRA), Media Interconnect Manager(MIM) and
>Media Interconnect Client(MIC).
>
>[1] https://www.spinics.net/lists/netdev/msg623647.html
>[2] https://www.spinics.net/lists/netdev/msg624378.html
>[3] https://github.com/microchip-ung/mrp/tree/patch-v3
>
>Horatiu Vultur (10):
>  net: bridge: mrp: Expose mrp attributes.
>  net: bridge: mrp: Expose function br_mrp_port_open
>  net: bridge: mrp: Add MRP interface used by netlink
>  net: bridge: mrp: Add generic netlink interface to configure MRP
>  net: bridge: mrp: Update MRP interface to add switchdev support
>  net: bridge: mrp: switchdev: Extend switchdev API to offload MRP
>  net: bridge: mrp: switchdev: Implement MRP API for switchdev
>  net: bridge: mrp: Connect MRP api with the switchev API
>  net: bridge: mrp: Integrate MRP into the bridge
>  net: bridge: mrp: Update Kconfig and Makefile
>
> include/linux/mrp_bridge.h      |  25 ++
> include/net/switchdev.h         |  51 +++
> include/uapi/linux/if_ether.h   |   1 +
> include/uapi/linux/mrp_bridge.h | 118 ++++++
> net/bridge/Kconfig              |  12 +
> net/bridge/Makefile             |   2 +
> net/bridge/br.c                 |  11 +
> net/bridge/br_device.c          |   3 +
> net/bridge/br_if.c              |   6 +
> net/bridge/br_input.c           |  14 +
> net/bridge/br_mrp.c             | 193 ++++++++++
> net/bridge/br_mrp_netlink.c     | 655 ++++++++++++++++++++++++++++++++
> net/bridge/br_mrp_switchdev.c   | 147 +++++++
> net/bridge/br_private.h         |  14 +
> net/bridge/br_private_mrp.h     |  58 +++
> 15 files changed, 1310 insertions(+)
> create mode 100644 include/linux/mrp_bridge.h
> create mode 100644 include/uapi/linux/mrp_bridge.h
> create mode 100644 net/bridge/br_mrp.c
> create mode 100644 net/bridge/br_mrp_netlink.c
> create mode 100644 net/bridge/br_mrp_switchdev.c
> create mode 100644 net/bridge/br_private_mrp.h
>
>-- 
>2.17.1
>
/Allan

Re: [RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[Vinicius Costa Gomes]

Hi,

"Allan W. Nielsen" <allan.nielsen@microchip.com> writes:

> On 24.01.2020 17:18, Horatiu Vultur wrote:
>>Media Redundancy Protocol is a data network protocol standardized by
>>International Electrotechnical Commission as IEC 62439-2. It allows rings of
>>Ethernet switches to overcome any single failure with recovery time faster than
>>STP. It is primarily used in Industrial Ethernet applications.
>>
>>Based on the previous RFC[1][2], the MRP state machine and all the
>>timers were moved to userspace. A generic netlink interface is added to
>>allow configuring the HW, and logic added to to implement the MRP
>>specific forwarding rules.
>>
>>The userspace application that is using the new netlink can be found here[3].
>>
>>The current implementation both in kernel and userspace supports only 2 roles:
>>
>>  MRM - this one is responsible to send MRP_Test and MRP_Topo frames on both
>>  ring ports. It needs to process MRP_Test to know if the ring is open or
>>  closed. This operation is desired to be offloaded to the HW because it
>>  requires to generate and process up to 4000 frames per second. Whenever it
>>  detects that the ring open it sends MRP_Topo frames to notify all MRC about
>>  changes in the topology. MRM needs also to process MRP_LinkChange frames,
>>  these frames are generated by the MRC. When the ring is open the the state
>>  of both ports is to forward frames and when the ring is closed then the
>>  secondary port is blocked.
>>
>>  MRC - this one is responsible to forward MRP frames between the ring ports.
>>  In case one of the ring ports gets a link down or up, then MRC will generate
>>  a MRP_LinkChange frames. This node should also process MRP_Topo frames and to
>>  clear its FDB when it receives this frame.
>>
>> Userspace
>>               Deamon +----------+ Client
>>                +
>>                |
>> +--------------|-----------------------------------------+
>>  Kernel        |
>>                + Netlink
>>
>>                |                              + Interrupt
>>                |                              |
>> +--------------|------------------------------|----------+
>>  HW            | Switchdev                    |
>>                +                              |
>>
>>The user interacts using the client (called 'mrp'), the client talks to the
>>deamon (called 'mrp_server'), which talks with the kernel using netlink. The
>>kernel will try to offload the requests to the HW via switchdev API. For this a
>>new generic netlink interface was added to the bridge.
>>
>>If the kernel cannot offload MRP to HW (maybe it does not have a switchdev
>>driver, or it is just not supported), then all the netlink calls will return
>>-EOPNOTSUPP. In this case the user-space deamon fallback to SW only
>>implementation.
> Horatiu and I have spend a bit of time discussing what you be best here.
> An alternative to this would be to do the SW fallback in the kernel,
> instead of user-land. This would mean that the user application does not
> need to know if the function is offloaded (or partly offloaded) to HW.
>
> We went with this approch to make the kernel part as simple as possible.
> The alternative would still be much simpler than the first version
> posted - but it would require a bit more.
>
> Both options has pros and cons, and we looking forward to the
> community's view on this.

I have one idea and one question.

The idea is:

'net/hsr' already has a software implementation of the HSR replication
tag (and some of the handling necessary). So what came to mind is to
add the necessary switchdev functions to the master HSR device. If
that's done, then it sounds that the rest will mostly work.

For the user the flow would be something like:

 - User takes two (or more interfaces) and set them as slaves of the HSR
   master device, say 'hsr0';

 - 'hsr0' implements some of the switchdev functionality so we can use
   the MRP userspace components on it;

Does it look like something that could work?

The question that I have is: what's the relation of IEC 62439-2 to IEEE
802.1CB? 


Cheers,
--
Vinicius

Re: [RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[Allan W. Nielsen]

Hi Vinicius,

On 24.01.2020 13:05, Vinicius Costa Gomes wrote:
>I have one idea and one question.

Let me answer the question before dicussing the idea.

>The question that I have is: what's the relation of IEC 62439-2 to IEEE
>802.1CB?
HSR and 802.1CB (often called FRER - Frame Replication and Elimination
for Reliability) shares a lot of functionallity. It is a while since I
read the 802.1CB standard, and I have only skimmed the HSR standard, but
as far as I understand 802.1CB is a super set of HSR. Also, I have not
studdied the HSR implementation.

Both HSR and 802.1CB replicate the frame and eliminate the additional
copies. If just 1 of the replicated fraems arrives, then higher layer
applications will not see any traffic lose.

MRP is different, it is a ring protocol, much more like ERPS defined in
G.8032 by ITU. Also, MRP only make sense in switches, it does not make
sense in a host (like HSR does).

In MRP, the higher layer application frames are not replicated. They are
send on either 1 port or the other.

Consider this exaple, with 3 nodes creating a ring. All notes has a br0
device which includes the 2 NICs.

     +------------------------------------------+
     |                                          |
     +-->|H1|<---------->|H2|<---------->|H3|<--+
     eth0    eth1    eth0    eth1    eth0    eth1

Lets say that H1 is the manager (MRM), and H2 + H3 is the client (MRC).

The MRM will now block one of the ports, lets say eth0, to prevent a
loop:

     +------------------------------------------+
     |                                          |
     +-->|H1|<---------->|H2|<---------->|H3|<--+
     eth0    eth1    eth0    eth1    eth0    eth1
      ^
      |
   Blocked


This mean that H1 can reach H2 and H3 via eth1
This mean that H2 can reach H1 eth0
This mean that H2 can reach H3 eth1
This mean that H3 can reach H1 and H2 via eth0

This is normal forwarding, doen by the MAC table.

Lets say that the link between H1 and H2 goes down:

     +------------------------------------------+
     |                                          |
     +-->|H1|<---  / --->|H2|<---------->|H3|<--+
     eth0    eth1    eth0    eth1    eth0    eth1

H1 will now observe that the test packets it sends on eth1, is not
received in eth0, meaninf that the ring is open, and it will unblock the
eth0 device, and send a message to all the nodes that they need to flush
the mac-table.

This mean that H1 can reach H2 and H3 via eth0
This mean that H2 can reach H1 and H3 via eth1
This mean that H3 can reach H2 eth0
This mean that H3 can reach H1 eth1

In all cases, higher layer application will use the br0 device to send
and receive frames. These higher layer applications will not see any
interruption (except during the few milliseconds it takes to unblock, and
flush the mac tables).

Sorry for the long explanation, but it is important to understand this
when discussion the design.

>The idea is:
>
>'net/hsr' already has a software implementation of the HSR replication
>tag (and some of the handling necessary). So what came to mind is to
>add the necessary switchdev functions to the master HSR device. If
>that's done, then it sounds that the rest will mostly work.
Maybe something could be done here, but it will not help MRP, as they do
not really share any functionality ;-)

>For the user the flow would be something like:
> - User takes two (or more interfaces) and set them as slaves of the HSR
>   master device, say 'hsr0';
> - 'hsr0' implements some of the switchdev functionality so we can use
>   the MRP userspace components on it;
For MRP to work, it really need the bridge interface, and the higher
layer applications needs to use the br0 device.

>Does it look like something that could work?
It would make much more sense if we discussed implementing 802.1CB in
some form (which we might get to).

/Allan

Re: [RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[Andrew Lunn]

> Lets say that the link between H1 and H2 goes down:
> 
>     +------------------------------------------+
>     |                                          |
>     +-->|H1|<---  / --->|H2|<---------->|H3|<--+
>     eth0    eth1    eth0    eth1    eth0    eth1
> 
> H1 will now observe that the test packets it sends on eth1, is not
> received in eth0, meaninf that the ring is open

Hi Allan

Is H1 the only device sending test packets? It is assumed that H2 and
H3 will forward them? Or does each device send test packets, and when
it stops hearing these packets from a neighbour, it assumes the link
is open?

   Andrew

Re: [RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[Allan W. Nielsen]

On 25.01.2020 17:23, Andrew Lunn wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> Lets say that the link between H1 and H2 goes down:
>>
>>     +------------------------------------------+
>>     |                                          |
>>     +-->|H1|<---  / --->|H2|<---------->|H3|<--+
>>     eth0    eth1    eth0    eth1    eth0    eth1
>>
>> H1 will now observe that the test packets it sends on eth1, is not
>> received in eth0, meaninf that the ring is open
>
>Is H1 the only device sending test packets? It is assumed that H2 and
>H3 will forward them?
Horatiu, please correct me if I'm wrong, you have been spending more
time with the standard.

It is only the manager/MRM (in this case H1) which sends test-frames.
The other nodes (the MRC's) must forward the MRP-test frames, but only
on the ports which is part of the ring.

> Or does each device send test packets, and when it stops hearing these
> packets from a neighbour, it assumes the link is open?
No.

This also means that most non-MRP aware switches can properly act as
MRC with HW offload. It is good to have that in mind when reviewing the
netlink interface.

It is worth mentioning that the client shall send a frame if they see a
link up/down on one of the ring ports. This is to allow the manager to
react faster.

Also, in this first patch we have only defined the MRM and MRC roles. In
future version we would also like to support a MRA (auto manager), where
the clients is monitoring the test frames, and if there are no manager
(it disappear, or they are all MRA) then they can negotiate who should
take the role as manager.

/Allan

Re: [RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[Vinicius Costa Gomes]

Hi,

"Allan W. Nielsen" <allan.nielsen@microchip.com> writes:

> Hi Vinicius,
>
> On 24.01.2020 13:05, Vinicius Costa Gomes wrote:
>>I have one idea and one question.
>
> Let me answer the question before dicussing the idea.
>
>>The question that I have is: what's the relation of IEC 62439-2 to IEEE
>>802.1CB?
> HSR and 802.1CB (often called FRER - Frame Replication and Elimination
> for Reliability) shares a lot of functionallity. It is a while since I
> read the 802.1CB standard, and I have only skimmed the HSR standard, but
> as far as I understand 802.1CB is a super set of HSR. Also, I have not
> studdied the HSR implementation.
>
> Both HSR and 802.1CB replicate the frame and eliminate the additional
> copies. If just 1 of the replicated fraems arrives, then higher layer
> applications will not see any traffic lose.
>
> MRP is different, it is a ring protocol, much more like ERPS defined in
> G.8032 by ITU. Also, MRP only make sense in switches, it does not make
> sense in a host (like HSR does).
>
> In MRP, the higher layer application frames are not replicated. They are
> send on either 1 port or the other.
>
> Consider this exaple, with 3 nodes creating a ring. All notes has a br0
> device which includes the 2 NICs.
>
>      +------------------------------------------+
>      |                                          |
>      +-->|H1|<---------->|H2|<---------->|H3|<--+
>      eth0    eth1    eth0    eth1    eth0    eth1
>
> Lets say that H1 is the manager (MRM), and H2 + H3 is the client (MRC).
>
> The MRM will now block one of the ports, lets say eth0, to prevent a
> loop:
>
>      +------------------------------------------+
>      |                                          |
>      +-->|H1|<---------->|H2|<---------->|H3|<--+
>      eth0    eth1    eth0    eth1    eth0    eth1
>       ^
>       |
>    Blocked
>
>
> This mean that H1 can reach H2 and H3 via eth1
> This mean that H2 can reach H1 eth0
> This mean that H2 can reach H3 eth1
> This mean that H3 can reach H1 and H2 via eth0
>
> This is normal forwarding, doen by the MAC table.
>
> Lets say that the link between H1 and H2 goes down:
>
>      +------------------------------------------+
>      |                                          |
>      +-->|H1|<---  / --->|H2|<---------->|H3|<--+
>      eth0    eth1    eth0    eth1    eth0    eth1
>
> H1 will now observe that the test packets it sends on eth1, is not
> received in eth0, meaninf that the ring is open, and it will unblock the
> eth0 device, and send a message to all the nodes that they need to flush
> the mac-table.
>
> This mean that H1 can reach H2 and H3 via eth0
> This mean that H2 can reach H1 and H3 via eth1
> This mean that H3 can reach H2 eth0
> This mean that H3 can reach H1 eth1
>
> In all cases, higher layer application will use the br0 device to send
> and receive frames. These higher layer applications will not see any
> interruption (except during the few milliseconds it takes to unblock, and
> flush the mac tables).
>
> Sorry for the long explanation, but it is important to understand this
> when discussion the design.

Not at all, thanks a lot. Now it's clear to me that MRP and 802.1CB are
really different beasts, with different use cases/limitations:

 - MRP: now that we have a ring, let's break the loop, and use the
   redudancy provided by the ring to detect the problem and "repair" the
   network if something bad happens;

 - 802.1CB: now that we have a ring, let's send packets through
   two different paths, and find a way to discard duplicated ones, so
   even if something bad happens the packet will reach its destination;

(I know that it's more complicated than that in reality :-)

>
>>The idea is:
>>
>>'net/hsr' already has a software implementation of the HSR replication
>>tag (and some of the handling necessary). So what came to mind is to
>>add the necessary switchdev functions to the master HSR device. If
>>that's done, then it sounds that the rest will mostly work.
> Maybe something could be done here, but it will not help MRP, as they do
> not really share any functionality ;-)
>
>>For the user the flow would be something like:
>> - User takes two (or more interfaces) and set them as slaves of the HSR
>>   master device, say 'hsr0';
>> - 'hsr0' implements some of the switchdev functionality so we can use
>>   the MRP userspace components on it;
> For MRP to work, it really need the bridge interface, and the higher
> layer applications needs to use the br0 device.
>
>>Does it look like something that could work?
> It would make much more sense if we discussed implementing 802.1CB in
> some form (which we might get to).

I see. Agreed.

>
> /Allan


Cheers,
--
Vinicius

Re: [RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[Jürgen Lambrecht]

On 1/25/20 10:18 PM, Vinicius Costa Gomes wrote:
> CAUTION: This Email originated from outside Televic. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> Hi,
>
> "Allan W. Nielsen" <allan.nielsen@microchip.com> writes:
>
>> Hi Vinicius,
>>
>> On 24.01.2020 13:05, Vinicius Costa Gomes wrote:
>>> I have one idea and one question.
>> Let me answer the question before dicussing the idea.
>>
>>> The question that I have is: what's the relation of IEC 62439-2 to IEEE
>>> 802.1CB?
>> HSR and 802.1CB (often called FRER - Frame Replication and Elimination
>> for Reliability) shares a lot of functionallity. It is a while since I
>> read the 802.1CB standard, and I have only skimmed the HSR standard, but
>> as far as I understand 802.1CB is a super set of HSR. Also, I have not
>> studdied the HSR implementation.
>> Both HSR and 802.1CB replicate the frame and eliminate the additional
>> copies. If just 1 of the replicated fraems arrives, then higher layer
>> applications will not see any traffic lose.
>>
>> MRP is different, it is a ring protocol, much more like ERPS defined in
>> G.8032 by ITU. Also, MRP only make sense in switches, it does not make
>> sense in a host (like HSR does).
>>
>> [snip MPR explanation]
>>
>> Sorry for the long explanation, but it is important to understand this
>> when discussion the design.
> Not at all, thanks a lot. Now it's clear to me that MRP and 802.1CB are
> really different beasts, with different use cases/limitations:
>
>  - MRP: now that we have a ring, let's break the loop, and use the
>    redudancy provided by the ring to detect the problem and "repair" the
>    network if something bad happens;
indeed. MRP is IEC 62439-2
>
>  - 802.1CB: now that we have a ring, let's send packets through
>    two different paths, and find a way to discard duplicated ones, so
>    even if something bad happens the packet will reach its destination;

Not exactly, 802.1CB is independent of the network layout, according to the abstract on https://ieeexplore.ieee.org/document/8091139.

The IEC 62439-3 standard mentions 2 network layouts: 2 parallel paths and a ring:

- IEC 62439-3.4 Parallel Redundancy Protocol (PRP): this runs on 2 separated parallel paths in the network

- IEC 62439-3.5 HSR (High availability seamless redundancy): this runs on a ring: each host sends all data in 2 directions, and when it receives its own data back, it discards it (to avoid a loop).

(and it is better to implement IEEE, because the standard costs only 151$, and the IEC ones cost 2x410$)

Kind regards,

Jürgen

Re: [RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[Allan W. Nielsen]

Hi All,

Its been a while since posting this serie. We got some good and very
specific comments, but there has not been much discussion on the overall
architecture.

Here is the list of items we have noted to be fixed in the next version:
- The headless chicken (it keeps sending test frames if user-space
   daemon dies)
- Avoid loops when bringing up the network - meaning we need to let MRP
   do its work before the br0 device is set to up, and we need to
   preserve that state.
- Unnessecary ifdef on the include.
- Extend the existing mac-table flush instead of adding
   BR_MRP_GENL_FLUSH
- Further optimize the changes in br_handle_frame

In v1 & v2 we had the entire protocol implemented in kernel-space.
Everybody told us this is a bad idea, and in v3 we have moved as much as
possible to user-space, and only kept the HW offload facilites in
kernel-space. The protocol is then implemented in user-space.

This is nice because it simplifies the code in the kernel and moves it
to user-space where such complexity is easier to handle. The downside of
this is that it makes the netlink interface more specific to our HW.

The way v3 is implemented, the netlink API returns an error if a given
operation cannot be HW offloaded. If the netlink calls return Ok,
user-space will trust that HW do the offloading as requested, if the
netlink calls return an error, it will implement all the functionallity
in user-space.

This works at-least in 2 scenarios: The HW we have with full MRP offload
capabilities, and a pure SW bridge.

But we should try make sure this also works in a backwards compatible
way with future MRP aware HW, and with existing (and future) SwitchDev
offloaded HW. At the very least we want to make this run on Ocelot, HW
offload the MRC role, but do the MRM in SW (as the HW is not capable of
this).

If we use the kernel to abstract the MRP forwarding (not the entire
protocol like we did in v1/v2, not just the HW like we did in v3) then
we will have more flxibility to support other HW with a different set of
offload facilities, we can most likely achieve better performance, and
it would be a cleaner design.

This will mean, that if user-space ask for MRP frame to be generated,
the kernel should make sure it will happen. The kernel can try to
offload this via the switchdev API, or it can do it in kernel-space.

Again, it will mean putting back some code into kernel space, but I
think it is worth it.

What do you think, what is the right design.

/Allan


On 24.01.2020 17:18, Horatiu Vultur wrote:
>Media Redundancy Protocol is a data network protocol standardized by
>International Electrotechnical Commission as IEC 62439-2. It allows rings of
>Ethernet switches to overcome any single failure with recovery time faster than
>STP. It is primarily used in Industrial Ethernet applications.
>
>Based on the previous RFC[1][2], the MRP state machine and all the
>timers were moved to userspace. A generic netlink interface is added to
>allow configuring the HW, and logic added to to implement the MRP
>specific forwarding rules.
>
>The userspace application that is using the new netlink can be found here[3].
>
>The current implementation both in kernel and userspace supports only 2 roles:
>
>  MRM - this one is responsible to send MRP_Test and MRP_Topo frames on both
>  ring ports. It needs to process MRP_Test to know if the ring is open or
>  closed. This operation is desired to be offloaded to the HW because it
>  requires to generate and process up to 4000 frames per second. Whenever it
>  detects that the ring open it sends MRP_Topo frames to notify all MRC about
>  changes in the topology. MRM needs also to process MRP_LinkChange frames,
>  these frames are generated by the MRC. When the ring is open the the state
>  of both ports is to forward frames and when the ring is closed then the
>  secondary port is blocked.
>
>  MRC - this one is responsible to forward MRP frames between the ring ports.
>  In case one of the ring ports gets a link down or up, then MRC will generate
>  a MRP_LinkChange frames. This node should also process MRP_Topo frames and to
>  clear its FDB when it receives this frame.
>
> Userspace
>               Deamon +----------+ Client
>                +
>                |
> +--------------|-----------------------------------------+
>  Kernel        |
>                + Netlink
>
>                |                              + Interrupt
>                |                              |
> +--------------|------------------------------|----------+
>  HW            | Switchdev                    |
>                +                              |
>
>The user interacts using the client (called 'mrp'), the client talks to the
>deamon (called 'mrp_server'), which talks with the kernel using netlink. The
>kernel will try to offload the requests to the HW via switchdev API. For this a
>new generic netlink interface was added to the bridge.
>
>If the kernel cannot offload MRP to HW (maybe it does not have a switchdev
>driver, or it is just not supported), then all the netlink calls will return
>-EOPNOTSUPP. In this case the user-space deamon fallback to SW only
>implementation.
>
>There are required changes to the SW bridge to be able to run the MRP. First the
>bridge needs to initialize the netlink interface. And second it needs to know if
>a MRP frame was received on a MRP ring port. In case it was received the SW
>bridge should not forward the frame it needs to redirected to upper layes. In
>case it was not received on a ring port then it just forwards it as usual.
>
>To be able to offload this to the HW, it was required to extend the switchdev
>API.
>
>If this will be accepted then in the future the netlink interface can be
>expended with multiple attributes which are required by different roles of the
>MRP. Like Media Redundancy Automanager(MRA), Media Interconnect Manager(MIM) and
>Media Interconnect Client(MIC).
>
>[1] https://www.spinics.net/lists/netdev/msg623647.html
>[2] https://www.spinics.net/lists/netdev/msg624378.html
>[3] https://github.com/microchip-ung/mrp/tree/patch-v3
>
>Horatiu Vultur (10):
>  net: bridge: mrp: Expose mrp attributes.
>  net: bridge: mrp: Expose function br_mrp_port_open
>  net: bridge: mrp: Add MRP interface used by netlink
>  net: bridge: mrp: Add generic netlink interface to configure MRP
>  net: bridge: mrp: Update MRP interface to add switchdev support
>  net: bridge: mrp: switchdev: Extend switchdev API to offload MRP
>  net: bridge: mrp: switchdev: Implement MRP API for switchdev
>  net: bridge: mrp: Connect MRP api with the switchev API
>  net: bridge: mrp: Integrate MRP into the bridge
>  net: bridge: mrp: Update Kconfig and Makefile
>
> include/linux/mrp_bridge.h      |  25 ++
> include/net/switchdev.h         |  51 +++
> include/uapi/linux/if_ether.h   |   1 +
> include/uapi/linux/mrp_bridge.h | 118 ++++++
> net/bridge/Kconfig              |  12 +
> net/bridge/Makefile             |   2 +
> net/bridge/br.c                 |  11 +
> net/bridge/br_device.c          |   3 +
> net/bridge/br_if.c              |   6 +
> net/bridge/br_input.c           |  14 +
> net/bridge/br_mrp.c             | 193 ++++++++++
> net/bridge/br_mrp_netlink.c     | 655 ++++++++++++++++++++++++++++++++
> net/bridge/br_mrp_switchdev.c   | 147 +++++++
> net/bridge/br_private.h         |  14 +
> net/bridge/br_private_mrp.h     |  58 +++
> 15 files changed, 1310 insertions(+)
> create mode 100644 include/linux/mrp_bridge.h
> create mode 100644 include/uapi/linux/mrp_bridge.h
> create mode 100644 net/bridge/br_mrp.c
> create mode 100644 net/bridge/br_mrp_netlink.c
> create mode 100644 net/bridge/br_mrp_switchdev.c
> create mode 100644 net/bridge/br_private_mrp.h
>
>-- 
>2.17.1
>
/Allan

Re: [RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[Jakub Kicinski]

On Tue, 18 Feb 2020 13:18:11 +0100 Allan W. Nielsen wrote:
> But we should try make sure this also works in a backwards compatible
> way with future MRP aware HW, and with existing (and future) SwitchDev
> offloaded HW. At the very least we want to make this run on Ocelot, HW
> offload the MRC role, but do the MRM in SW (as the HW is not capable of
> this).
> 
> If we use the kernel to abstract the MRP forwarding (not the entire
> protocol like we did in v1/v2, not just the HW like we did in v3) then
> we will have more flxibility to support other HW with a different set of
> offload facilities, we can most likely achieve better performance, and
> it would be a cleaner design.
> 
> This will mean, that if user-space ask for MRP frame to be generated,
> the kernel should make sure it will happen. The kernel can try to
> offload this via the switchdev API, or it can do it in kernel-space.
> 
> Again, it will mean putting back some code into kernel space, but I
> think it is worth it.

FWIW having the guarantee that the kernel can always perform requested
service/operation (either thru offload or in SW in kernel space) seems
appealing and in line what we've been doing for other offloads. 

IOW it'd be nice to have a kernel space software fallback for the
offloaded operations.

Re: [RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[Nikolay Aleksandrov]

On 18/02/2020 14:18, Allan W. Nielsen wrote:
> 
> Hi All,
> 
> Its been a while since posting this serie. We got some good and very
> specific comments, but there has not been much discussion on the overall
> architecture.
> 
> Here is the list of items we have noted to be fixed in the next version:
> - The headless chicken (it keeps sending test frames if user-space
>   daemon dies)
> - Avoid loops when bringing up the network - meaning we need to let MRP
>   do its work before the br0 device is set to up, and we need to
>   preserve that state.
> - Unnessecary ifdef on the include.
> - Extend the existing mac-table flush instead of adding
>   BR_MRP_GENL_FLUSH
> - Further optimize the changes in br_handle_frame
> 
> In v1 & v2 we had the entire protocol implemented in kernel-space.
> Everybody told us this is a bad idea, and in v3 we have moved as much as
> possible to user-space, and only kept the HW offload facilites in
> kernel-space. The protocol is then implemented in user-space.
> 
> This is nice because it simplifies the code in the kernel and moves it
> to user-space where such complexity is easier to handle. The downside of
> this is that it makes the netlink interface more specific to our HW.
> 
> The way v3 is implemented, the netlink API returns an error if a given
> operation cannot be HW offloaded. If the netlink calls return Ok,
> user-space will trust that HW do the offloading as requested, if the
> netlink calls return an error, it will implement all the functionallity
> in user-space.
> 
> This works at-least in 2 scenarios: The HW we have with full MRP offload
> capabilities, and a pure SW bridge.
> 
> But we should try make sure this also works in a backwards compatible
> way with future MRP aware HW, and with existing (and future) SwitchDev
> offloaded HW. At the very least we want to make this run on Ocelot, HW
> offload the MRC role, but do the MRM in SW (as the HW is not capable of
> this).
> 
> If we use the kernel to abstract the MRP forwarding (not the entire
> protocol like we did in v1/v2, not just the HW like we did in v3) then
> we will have more flxibility to support other HW with a different set of
> offload facilities, we can most likely achieve better performance, and
> it would be a cleaner design.
> 
> This will mean, that if user-space ask for MRP frame to be generated,
> the kernel should make sure it will happen. The kernel can try to
> offload this via the switchdev API, or it can do it in kernel-space.
> 
> Again, it will mean putting back some code into kernel space, but I
> think it is worth it.
> 
> What do you think, what is the right design.
> 
> /Allan
> 
> 

In light of all the discussions and details that were explained, and as you've
noted above, I think more code should be put in kernel space at the very least
the performance/latency critical parts would benefit from being executed in the
kernel (kind of control/data-plane separation). It seems from the switchdev calls there's
a minimal state working set which define the behaviour and can be used to make
decisions (similar to STP) in the kernel, but the complex logic how to set them can be
executed in user-space meaning that maybe these hw-offload calls can have a simple SW
fallback logic based on their current values. I think it is worth considering if this can
be achieved before going to full in-kernel implementation of the state machine.
Since you intend to hw-offload it then putting in some SW fallback logic would be good
when the HW can't offload everything, what you suggest above also sounds good to me and
I think you'll have to extend mdb and the multicast code to do it, but IIRC you already
have code to do that based on previous discussions.

As was already suggested you can put the MRP options in the bridge's options and
process them from the bridge netlink code, that should simplify your code. You could
also make the port's "mrp_aware" bool into an internal port flag (use net_bridge_port's
flags field) so it can be quickly tested and in a hot cache line.

> On 24.01.2020 17:18, Horatiu Vultur wrote:
>> Media Redundancy Protocol is a data network protocol standardized by
>> International Electrotechnical Commission as IEC 62439-2. It allows rings of
>> Ethernet switches to overcome any single failure with recovery time faster than
>> STP. It is primarily used in Industrial Ethernet applications.
>>
>> Based on the previous RFC[1][2], the MRP state machine and all the
>> timers were moved to userspace. A generic netlink interface is added to
>> allow configuring the HW, and logic added to to implement the MRP
>> specific forwarding rules.
>>
>> The userspace application that is using the new netlink can be found here[3].
>>
>> The current implementation both in kernel and userspace supports only 2 roles:
>>
>>  MRM - this one is responsible to send MRP_Test and MRP_Topo frames on both
>>  ring ports. It needs to process MRP_Test to know if the ring is open or
>>  closed. This operation is desired to be offloaded to the HW because it
>>  requires to generate and process up to 4000 frames per second. Whenever it
>>  detects that the ring open it sends MRP_Topo frames to notify all MRC about
>>  changes in the topology. MRM needs also to process MRP_LinkChange frames,
>>  these frames are generated by the MRC. When the ring is open the the state
>>  of both ports is to forward frames and when the ring is closed then the
>>  secondary port is blocked.
>>
>>  MRC - this one is responsible to forward MRP frames between the ring ports.
>>  In case one of the ring ports gets a link down or up, then MRC will generate
>>  a MRP_LinkChange frames. This node should also process MRP_Topo frames and to
>>  clear its FDB when it receives this frame.
>>
>> Userspace
>>               Deamon +----------+ Client
>>                +
>>                |
>> +--------------|-----------------------------------------+
>>  Kernel        |
>>                + Netlink
>>
>>                |                              + Interrupt
>>                |                              |
>> +--------------|------------------------------|----------+
>>  HW            | Switchdev                    |
>>                +                              |
>>
>> The user interacts using the client (called 'mrp'), the client talks to the
>> deamon (called 'mrp_server'), which talks with the kernel using netlink. The
>> kernel will try to offload the requests to the HW via switchdev API. For this a
>> new generic netlink interface was added to the bridge.
>>
>> If the kernel cannot offload MRP to HW (maybe it does not have a switchdev
>> driver, or it is just not supported), then all the netlink calls will return
>> -EOPNOTSUPP. In this case the user-space deamon fallback to SW only
>> implementation.
>>
>> There are required changes to the SW bridge to be able to run the MRP. First the
>> bridge needs to initialize the netlink interface. And second it needs to know if
>> a MRP frame was received on a MRP ring port. In case it was received the SW
>> bridge should not forward the frame it needs to redirected to upper layes. In
>> case it was not received on a ring port then it just forwards it as usual.
>>
>> To be able to offload this to the HW, it was required to extend the switchdev
>> API.
>>
>> If this will be accepted then in the future the netlink interface can be
>> expended with multiple attributes which are required by different roles of the
>> MRP. Like Media Redundancy Automanager(MRA), Media Interconnect Manager(MIM) and
>> Media Interconnect Client(MIC).
>>
>> [1] https://www.spinics.net/lists/netdev/msg623647.html
>> [2] https://www.spinics.net/lists/netdev/msg624378.html
>> [3] https://github.com/microchip-ung/mrp/tree/patch-v3
>>
>> Horatiu Vultur (10):
>>  net: bridge: mrp: Expose mrp attributes.
>>  net: bridge: mrp: Expose function br_mrp_port_open
>>  net: bridge: mrp: Add MRP interface used by netlink
>>  net: bridge: mrp: Add generic netlink interface to configure MRP
>>  net: bridge: mrp: Update MRP interface to add switchdev support
>>  net: bridge: mrp: switchdev: Extend switchdev API to offload MRP
>>  net: bridge: mrp: switchdev: Implement MRP API for switchdev
>>  net: bridge: mrp: Connect MRP api with the switchev API
>>  net: bridge: mrp: Integrate MRP into the bridge
>>  net: bridge: mrp: Update Kconfig and Makefile
>>
>> include/linux/mrp_bridge.h      |  25 ++
>> include/net/switchdev.h         |  51 +++
>> include/uapi/linux/if_ether.h   |   1 +
>> include/uapi/linux/mrp_bridge.h | 118 ++++++
>> net/bridge/Kconfig              |  12 +
>> net/bridge/Makefile             |   2 +
>> net/bridge/br.c                 |  11 +
>> net/bridge/br_device.c          |   3 +
>> net/bridge/br_if.c              |   6 +
>> net/bridge/br_input.c           |  14 +
>> net/bridge/br_mrp.c             | 193 ++++++++++
>> net/bridge/br_mrp_netlink.c     | 655 ++++++++++++++++++++++++++++++++
>> net/bridge/br_mrp_switchdev.c   | 147 +++++++
>> net/bridge/br_private.h         |  14 +
>> net/bridge/br_private_mrp.h     |  58 +++
>> 15 files changed, 1310 insertions(+)
>> create mode 100644 include/linux/mrp_bridge.h
>> create mode 100644 include/uapi/linux/mrp_bridge.h
>> create mode 100644 net/bridge/br_mrp.c
>> create mode 100644 net/bridge/br_mrp_netlink.c
>> create mode 100644 net/bridge/br_mrp_switchdev.c
>> create mode 100644 net/bridge/br_private_mrp.h
>>
>> -- 
>> 2.17.1
>>
> /Allan

Re: [RFC net-next v3 00/10] net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)

[Allan W. Nielsen]

Hi Nik,

On 20.02.2020 12:48, Nikolay Aleksandrov wrote:
> In light of all the discussions and details that were explained, and as you've
> noted above, I think more code should be put in kernel space at the very least
> the performance/latency critical parts would benefit from being executed in the
> kernel (kind of control/data-plane separation). It seems from the switchdev calls there's
> a minimal state working set which define the behaviour and can be used to make
> decisions (similar to STP) in the kernel, but the complex logic how to set them can be
> executed in user-space meaning that maybe these hw-offload calls can have a simple SW
> fallback logic based on their current values. I think it is worth considering if this can
> be achieved before going to full in-kernel implementation of the state machine.
> Since you intend to hw-offload it then putting in some SW fallback logic would be good
> when the HW can't offload everything, what you suggest above also sounds good to me and
> I think you'll have to extend mdb and the multicast code to do it, but IIRC you already
> have code to do that based on previous discussions.
Sounds good. We will continue working on defining a good control/data-plane separation
and only keep the data-plane in the kernel. Also it seems that we agree that the SW fallback
of the data-plane should stay in the kernel - we will do that.

> As was already suggested you can put the MRP options in the bridge's options and
> process them from the bridge netlink code, that should simplify your code.
I'm okay with this.

The main argument I see for creating a seperate MRP netlink interface
instead of extending the bridge, is that MRP is properly not the last
bridge protocol we will want to work on.

To complete the MRP-2018 implementation, we will also need some CFM
support (defined in 802.1Qag or the latest version of 802.1Q). And
furhter out we will properly want to implement the full CFM protocol.

DLR may also be relevant at some point, and there may be other.

My main point is, that at some point we will properly want to do
seperate NETLINK interfaces for this. Not sure if that is now or later.

> You could also make the port's "mrp_aware" bool into an internal port
> flag (use net_bridge_port's flags field) so it can be quickly tested
> and in a hot cache line.
Good point, we will do that.

/Allan