[RFC net-next v4 0/9] net: bridge: mrp: Add support for Media Redundancy Protocol(MRP)

[RFC net-next v4 0/9] 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][3], the MRP state machine and all the timers
were moved to userspace, except for the timers used to generate MRP Test frames.
In this way the userspace doesn't know and should not know if the HW or the
kernel will generate the MRP Test frames. The following changes were added to
the bridge to support the MRP:
- the existing netlink interface was extended with MRP support,
- allow to detect when a MRP frame was received on a MRP ring port
- allow MRP instance to forward/terminate MRP frames
- generate MRP Test frames in case the HW doesn't have support for this

To be able to offload MRP support to HW, the switchdev API  was extend.

With these changes the userspace doesn't do the following because already the
kernel/HW will do:
- doesn't need to forward/terminate MRP frames
- doesn't need to generate MRP Test frames
- doesn't need to detect when the ring is open/closed.

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

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 is 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 then 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.

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://www.spinics.net/lists/netdev/msg627500.html
[4] https://github.com/microchip-ung/mrp/tree/patch-v4

-v3:
  - move MRP state machine in userspace
  - create generic netlink interface for configuring the HW using switchdev API

-v2:
  - extend switchdev API to offload to HW



Horatiu Vultur (9):
  bridge: uapi: mrp: Add mrp attributes.
  bridge: mrp: Expose function br_mrp_port_open
  bridge: mrp: Add MRP interface.
  bridge: mrp: Implement netlink interface to configure MRP
  switchdev: mrp: Extend switchdev API to offload MRP
  bridge: switchdev: mrp Implement MRP API for switchdev
  bridge: mrp: Connect MRP api with the switchev API
  bridge: mrp: Integrate MRP into the bridge
  bridge: mrp: Update Kconfig and Makefile

 include/linux/if_bridge.h       |   1 +
 include/linux/mrp_bridge.h      |  24 ++
 include/net/switchdev.h         |  53 ++++
 include/uapi/linux/if_bridge.h  |  42 +++
 include/uapi/linux/if_ether.h   |   1 +
 include/uapi/linux/mrp_bridge.h |  84 ++++++
 net/bridge/Kconfig              |  12 +
 net/bridge/Makefile             |   2 +
 net/bridge/br_device.c          |   3 +
 net/bridge/br_input.c           |   3 +
 net/bridge/br_mrp.c             | 514 ++++++++++++++++++++++++++++++++
 net/bridge/br_mrp_netlink.c     | 176 +++++++++++
 net/bridge/br_mrp_switchdev.c   | 150 ++++++++++
 net/bridge/br_netlink.c         |   5 +
 net/bridge/br_private.h         |  22 ++
 net/bridge/br_private_mrp.h     |  67 +++++
 net/bridge/br_stp.c             |   6 +
 17 files changed, 1165 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 v4 1/9] bridge: uapi: mrp: Add mrp attributes.

[Horatiu Vultur]

Add new nested netlink attribute to configure the MRP. These attributes are used
by the userspace to add/delete/configure MRP instances and by the kernel to
notify the userspace when the MRP ring gets open/closed. MRP nested attribute
has the following attributes:

IFLA_BRIDGE_MRP_INSTANCE - the parameter type is br_mrp_instance which contains
  the instance id, and the ifindex of the two ports. The ports can't be part of
  multiple instances. This is used to create/delete MRP instances.

IFLA_BRIDGE_MRP_PORT_STATE - the parameter type is u32. Which can be forwarding,
  blocking or disabled.

IFLA_BRIDGE_MRP_PORT_ROLE - the parameter type is br_mrp_port_role which
  contains the instance id and the role. The role can be primary or secondary.

IFLA_BRIDGE_MRP_RING_STATE - the parameter type is br_mrp_ring_state which
  contains the instance id and the state. The state can be open or closed.

IFLA_BRIDGE_MRP_RING_ROLE - the parameter type is br_mrp_ring_role which
  contains the instance id and the ring role. The role can be MRM or MRC.

IFLA_BRIDGE_MRP_START_TEST - the parameter type is br_mrp_start_test which
  contains the instance id, the interval at which to send the MRP_Test frames,
  how many test frames can be missed before declaring the ring open and the
  period which represent for how long to send the test frames.

IFLA_BRIDGE_MRP_RING_OPEN - the parameter type is u32 and has a value of 1 if
  the port stopped receiving test frames and a value 0 is started to receive
  them.  This attribut is used by the kernel to notify the userspace when the
  ring gets open or closed.

Also add the file include/uapi/linux/mrp_bridge.h which defines all the types
used by MRP that are also needed by the userpace.

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

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index bfe621ea51b3..4d488e1837da 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -120,6 +120,7 @@ enum {
 	IFLA_BRIDGE_MODE,
 	IFLA_BRIDGE_VLAN_INFO,
 	IFLA_BRIDGE_VLAN_TUNNEL_INFO,
+	IFLA_BRIDGE_MRP,
 	__IFLA_BRIDGE_MAX,
 };
 #define IFLA_BRIDGE_MAX (__IFLA_BRIDGE_MAX - 1)
@@ -157,6 +158,47 @@ struct bridge_vlan_xstats {
 	__u32 pad2;
 };
 
+enum {
+	IFLA_BRIDGE_MRP_INSTANCE,
+	IFLA_BRIDGE_MRP_PORT_STATE,
+	IFLA_BRIDGE_MRP_PORT_ROLE,
+	IFLA_BRIDGE_MRP_RING_STATE,
+	IFLA_BRIDGE_MRP_RING_ROLE,
+	IFLA_BRIDGE_MRP_START_TEST,
+	IFLA_BRIDGE_MRP_RING_OPEN,
+	__IFLA_BRIDGE_MRP_MAX,
+};
+
+struct br_mrp_instance {
+	__u32 ring_id;
+	__u32 p_ifindex;
+	__u32 s_ifindex;
+};
+
+struct br_mrp_port_role {
+	__u32 ring_id;
+	__u32 role;
+};
+
+struct br_mrp_ring_state {
+	__u32 ring_id;
+	__u32 ring_state;
+};
+
+struct br_mrp_ring_role {
+	__u32 ring_id;
+	__u32 ring_role;
+};
+
+struct br_mrp_start_test {
+	__u32 ring_id;
+	__u32 interval;
+	__u32 max_miss;
+	__u32 period;
+};
+
+#define IFLA_BRIDGE_MRP_MAX (__IFLA_BRIDGE_MRP_MAX - 1)
+
 struct bridge_stp_xstats {
 	__u64 transition_blk;
 	__u64 transition_fwd;
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..2600cdf5a284
--- /dev/null
+++ b/include/uapi/linux/mrp_bridge.h
@@ -0,0 +1,84 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+
+#ifndef _UAPI_LINUX_MRP_BRIDGE_H_
+#define _UAPI_LINUX_MRP_BRIDGE_H_
+
+#include <linux/types.h>
+#include <linux/if_ether.h>
+
+#define MRP_MAX_FRAME_LENGTH		200
+#define MRP_DEFAULT_PRIO		0x8000
+#define MRP_DOMAIN_UUID_LENGTH		16
+#define MRP_VERSION			1
+#define MRP_FRAME_PRIO			7
+
+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,
+};
+
+enum br_mrp_tlv_header_type {
+	BR_MRP_TLV_HEADER_END = 0x0,
+	BR_MRP_TLV_HEADER_COMMON = 0x1,
+	BR_MRP_TLV_HEADER_RING_TEST = 0x2,
+	BR_MRP_TLV_HEADER_RING_TOPO = 0x3,
+	BR_MRP_TLV_HEADER_RING_LINK_DOWN = 0x4,
+	BR_MRP_TLV_HEADER_RING_LINK_UP = 0x5,
+};
+
+struct br_mrp_tlv_hdr {
+	__u8 type;
+	__u8 length;
+};
+
+struct br_mrp_end_hdr {
+	struct br_mrp_tlv_hdr hdr;
+};
+
+struct br_mrp_common_hdr {
+	__u16 seq_id;
+	__u8 domain[MRP_DOMAIN_UUID_LENGTH];
+};
+
+struct br_mrp_ring_test_hdr {
+	__u16 prio;
+	__u8 sa[ETH_ALEN];
+	__u16 port_role;
+	__u16 state;
+	__u16 transitions;
+	__u32 timestamp;
+};
+
+struct br_mrp_ring_topo_hdr {
+	__u16 prio;
+	__u8 sa[ETH_ALEN];
+	__u16 interval;
+};
+
+struct br_mrp_ring_link_hdr {
+	__u8 sa[ETH_ALEN];
+	__u16 port_role;
+	__u16 interval;
+	__u16 blocked;
+};
+
+#endif
-- 
2.17.1

[RFC net-next v4 2/9] 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 SW 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.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 include/linux/mrp_bridge.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 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..45448853e705
--- /dev/null
+++ b/include/linux/mrp_bridge.h
@@ -0,0 +1,24 @@
+/* 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 notify the userspace which is required to react on these
+ * changes.
+ */
+
+#if IS_ENABLED(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

[RFC net-next v4 3/9] bridge: mrp: Add MRP interface.

[Horatiu Vultur]

Define the MRP interface.
This interface is used by the netlink to update the MRP instances and by the MRP
to make the calls to switchdev to offload it to HW.

It defines an MRP instance 'struct br_mrp' which is a list of MRP instances.
Which will be part of the 'struct net_bridge'. Each instance has 2 ring ports,
a bridge and an ID.

In case the HW can't generate MRP Test frames then the SW will generate those.

br_mrp_add - adds a new MRP instance.

br_mrp_del - deletes an existing MRP instance. Each instance has an ID(ring_id).

br_mrp_set_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_set_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_set_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.

br_mrp_set_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.

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_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.

br_mrp_port_switchdev_add - this corresponds to the function br_mrp_add,
  and will notify the HW that a MRP instance is added. The function gets
  as parameter the MRP instance.

br_mrp_port_switchdev_del - this corresponds to the function br_mrp_del,
  and will notify the HW that a MRP instance is removed. The function
  gets as parameter the ID of the MRP instance that is removed.

br_mrp_port_switchdev_set_state - this corresponds to the function
  br_mrp_set_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_set_port_role. It would set the port role, primary or secondary.

br_mrp_switchdev_set_role - this corresponds to the function
  br_mrp_set_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_set_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 | 67 +++++++++++++++++++++++++++++++++++++
 1 file changed, 67 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..444d8d06ccac
--- /dev/null
+++ b/net/bridge/br_private_mrp.h
@@ -0,0 +1,67 @@
+/* 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		__rcu list;
+
+	struct net_bridge		*br;
+	struct net_bridge_port __rcu	*p_port;
+	struct net_bridge_port __rcu	*s_port;
+
+	u32				ring_id;
+
+	enum br_mrp_ring_role_type	ring_role;
+	u8				ring_role_offloaded;
+	enum br_mrp_ring_state_type	ring_state;
+	u32				ring_transitions;
+
+	struct delayed_work		test_work;
+	u32				test_interval;
+	unsigned long			test_end;
+	u32				test_count_miss;
+	u32				test_max_miss;
+
+	u32				seq_id;
+
+	/* lock for accessing the MRP instance */
+	spinlock_t			lock;
+};
+
+/* br_mrp.c */
+int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance);
+int br_mrp_del(struct net_bridge *br, struct br_mrp_instance *instance);
+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_id, enum br_mrp_port_role_type role);
+int br_mrp_set_ring_state(struct net_bridge *br, u32 ring_id,
+			  enum br_mrp_ring_state_type state);
+int br_mrp_set_ring_role(struct net_bridge *br, u32 ring_id,
+			 enum br_mrp_ring_role_type role);
+int br_mrp_start_test(struct net_bridge *br, u32 ring_id, u32 interval,
+		      u8 max_miss, u32 period);
+
+/* br_mrp_switchdev.c */
+int br_mrp_switchdev_add(struct br_mrp *mrp);
+int br_mrp_switchdev_del(struct br_mrp *mrp);
+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, u32 period);
+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);
+
+/* br_mrp_netlink.c */
+void br_mrp_port_open(struct net_device *dev, u8 loc);
+
+#endif /* _BR_PRIVATE_MRP_H */
-- 
2.17.1

[RFC net-next v4 4/9] bridge: mrp: Implement netlink interface to configure MRP

[Horatiu Vultur]

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

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_mrp_netlink.c | 176 ++++++++++++++++++++++++++++++++++++
 1 file changed, 176 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..043b7f6cddbe
--- /dev/null
+++ b/net/bridge/br_mrp_netlink.c
@@ -0,0 +1,176 @@
+// 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 int br_mrp_parse_instance(struct net_bridge *br, struct nlattr *attr,
+				 int cmd)
+{
+	struct br_mrp_instance *instance;
+
+	if (nla_len(attr) != sizeof(*instance))
+		return -EINVAL;
+
+	instance = nla_data(attr);
+
+	if (cmd == RTM_SETLINK)
+		return br_mrp_add(br, instance);
+	else
+		return br_mrp_del(br, instance);
+}
+
+static int br_mrp_parse_port_state(struct net_bridge *br,
+				   struct net_bridge_port *p,
+				   struct nlattr *attr)
+{
+	enum br_mrp_port_state_type state;
+
+	if (nla_len(attr) != sizeof(u32) || !p)
+		return -EINVAL;
+
+	state = nla_get_u32(attr);
+
+	return br_mrp_set_port_state(p, state);
+}
+
+static int br_mrp_parse_port_role(struct net_bridge *br,
+				  struct net_bridge_port *p,
+				  struct nlattr *attr)
+{
+	struct br_mrp_port_role *role;
+
+	if (nla_len(attr) != sizeof(*role) || !p)
+		return -EINVAL;
+
+	role = nla_data(attr);
+
+	return br_mrp_set_port_role(p, role->ring_id, role->role);
+}
+
+static int br_mrp_parse_ring_state(struct net_bridge *br, struct nlattr *attr)
+{
+	struct br_mrp_ring_state *state;
+
+	if (nla_len(attr) != sizeof(*state))
+		return -EINVAL;
+
+	state = nla_data(attr);
+
+	return br_mrp_set_ring_state(br, state->ring_id, state->ring_state);
+}
+
+static int br_mrp_parse_ring_role(struct net_bridge *br, struct nlattr *attr)
+{
+	struct br_mrp_ring_role *role;
+
+	if (nla_len(attr) != sizeof(*role))
+		return -EINVAL;
+
+	role = nla_data(attr);
+
+	return br_mrp_set_ring_role(br, role->ring_id, role->ring_role);
+}
+
+static int br_mrp_parse_start_test(struct net_bridge *br, struct nlattr *attr)
+{
+	struct br_mrp_start_test *test;
+
+	if (nla_len(attr) != sizeof(*test))
+		return -EINVAL;
+
+	test = nla_data(attr);
+
+	return br_mrp_start_test(br, test->ring_id, test->interval,
+				 test->max_miss, test->period);
+}
+
+int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
+		 struct nlattr *attr, int cmd)
+{
+	struct nlattr *mrp;
+	int rem, err;
+
+	nla_for_each_nested(mrp, attr, rem) {
+		err = 0;
+		switch (nla_type(mrp)) {
+		case IFLA_BRIDGE_MRP_INSTANCE:
+			err = br_mrp_parse_instance(br, mrp, cmd);
+			if (err)
+				return err;
+			break;
+		case IFLA_BRIDGE_MRP_PORT_STATE:
+			err = br_mrp_parse_port_state(br, p, mrp);
+			if (err)
+				return err;
+			break;
+		case IFLA_BRIDGE_MRP_PORT_ROLE:
+			err = br_mrp_parse_port_role(br, p, mrp);
+			if (err)
+				return err;
+			break;
+		case IFLA_BRIDGE_MRP_RING_STATE:
+			err = br_mrp_parse_ring_state(br, mrp);
+			if (err)
+				return err;
+			break;
+		case IFLA_BRIDGE_MRP_RING_ROLE:
+			err = br_mrp_parse_ring_role(br, mrp);
+			if (err)
+				return err;
+			break;
+		case IFLA_BRIDGE_MRP_START_TEST:
+			err = br_mrp_parse_start_test(br, mrp);
+			if (err)
+				return err;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+void br_mrp_port_open(struct net_device *dev, u8 loc)
+{
+	struct nlattr *af, *mrp;
+	struct ifinfomsg *hdr;
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	int err = -ENOBUFS;
+	struct net *net;
+
+	net = dev_net(dev);
+
+	skb = nlmsg_new(1024, GFP_ATOMIC);
+	if (!skb)
+		goto errout;
+
+	nlh = nlmsg_put(skb, 0, 0, RTM_NEWLINK, sizeof(*hdr), 0);
+	if (!nlh)
+		goto errout;
+
+	hdr = nlmsg_data(nlh);
+	hdr->ifi_family = AF_BRIDGE;
+	hdr->__ifi_pad = 0;
+	hdr->ifi_type = dev->type;
+	hdr->ifi_index = dev->ifindex;
+	hdr->ifi_flags = dev_get_flags(dev);
+	hdr->ifi_change = 0;
+
+	af = nla_nest_start_noflag(skb, IFLA_AF_SPEC);
+	mrp = nla_nest_start_noflag(skb, IFLA_BRIDGE_MRP);
+
+	nla_put_u32(skb, IFLA_BRIDGE_MRP_RING_OPEN, loc);
+
+	nla_nest_end(skb, mrp);
+	nla_nest_end(skb, af);
+	nlmsg_end(skb, nlh);
+
+	rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL, GFP_ATOMIC);
+	return;
+errout:
+	rtnl_set_sk_err(net, RTNLGRP_LINK, err);
+}
+EXPORT_SYMBOL(br_mrp_port_open);
-- 
2.17.1

Re: [RFC net-next v4 4/9] bridge: mrp: Implement netlink interface to configure MRP

[Nikolay Aleksandrov]

On 27/03/2020 11:21, Horatiu Vultur wrote:
> Implement netlink interface to configure MRP. The implementation
> will do sanity checks over the attributes and then eventually call the MRP
> interface.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  net/bridge/br_mrp_netlink.c | 176 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 176 insertions(+)
>  create mode 100644 net/bridge/br_mrp_netlink.c
> 

Hi Horatiu,

> diff --git a/net/bridge/br_mrp_netlink.c b/net/bridge/br_mrp_netlink.c
> new file mode 100644
> index 000000000000..043b7f6cddbe
> --- /dev/null
> +++ b/net/bridge/br_mrp_netlink.c
> @@ -0,0 +1,176 @@
> +// 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 int br_mrp_parse_instance(struct net_bridge *br, struct nlattr *attr,
> +				 int cmd)
> +{
> +	struct br_mrp_instance *instance;
> +
> +	if (nla_len(attr) != sizeof(*instance))
> +		return -EINVAL;
> +
> +	instance = nla_data(attr);
> +
> +	if (cmd == RTM_SETLINK)
> +		return br_mrp_add(br, instance);
> +	else
> +		return br_mrp_del(br, instance);
> +}
> +
> +static int br_mrp_parse_port_state(struct net_bridge *br,
> +				   struct net_bridge_port *p,
> +				   struct nlattr *attr)
> +{
> +	enum br_mrp_port_state_type state;
> +
> +	if (nla_len(attr) != sizeof(u32) || !p)
> +		return -EINVAL;
> +
> +	state = nla_get_u32(attr);
> +
> +	return br_mrp_set_port_state(p, state);
> +}
> +
> +static int br_mrp_parse_port_role(struct net_bridge *br,
> +				  struct net_bridge_port *p,
> +				  struct nlattr *attr)
> +{
> +	struct br_mrp_port_role *role;
> +
> +	if (nla_len(attr) != sizeof(*role) || !p)
> +		return -EINVAL;
> +
> +	role = nla_data(attr);
> +
> +	return br_mrp_set_port_role(p, role->ring_id, role->role);
> +}
> +
> +static int br_mrp_parse_ring_state(struct net_bridge *br, struct nlattr *attr)
> +{
> +	struct br_mrp_ring_state *state;
> +
> +	if (nla_len(attr) != sizeof(*state))
> +		return -EINVAL;
> +
> +	state = nla_data(attr);
> +
> +	return br_mrp_set_ring_state(br, state->ring_id, state->ring_state);
> +}
> +
> +static int br_mrp_parse_ring_role(struct net_bridge *br, struct nlattr *attr)
> +{
> +	struct br_mrp_ring_role *role;
> +
> +	if (nla_len(attr) != sizeof(*role))
> +		return -EINVAL;
> +
> +	role = nla_data(attr);
> +
> +	return br_mrp_set_ring_role(br, role->ring_id, role->ring_role);
> +}
> +
> +static int br_mrp_parse_start_test(struct net_bridge *br, struct nlattr *attr)
> +{
> +	struct br_mrp_start_test *test;
> +
> +	if (nla_len(attr) != sizeof(*test))
> +		return -EINVAL;
> +
> +	test = nla_data(attr);
> +
> +	return br_mrp_start_test(br, test->ring_id, test->interval,
> +				 test->max_miss, test->period);
> +}
> +
> +int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
> +		 struct nlattr *attr, int cmd)
> +{
> +	struct nlattr *mrp;
> +	int rem, err;
> +
> +	nla_for_each_nested(mrp, attr, rem) {
> +		err = 0;
> +		switch (nla_type(mrp)) {
> +		case IFLA_BRIDGE_MRP_INSTANCE:
> +			err = br_mrp_parse_instance(br, mrp, cmd);
> +			if (err)
> +				return err;
> +			break;
> +		case IFLA_BRIDGE_MRP_PORT_STATE:
> +			err = br_mrp_parse_port_state(br, p, mrp);
> +			if (err)
> +				return err;
> +			break;
> +		case IFLA_BRIDGE_MRP_PORT_ROLE:
> +			err = br_mrp_parse_port_role(br, p, mrp);
> +			if (err)
> +				return err;
> +			break;
> +		case IFLA_BRIDGE_MRP_RING_STATE:
> +			err = br_mrp_parse_ring_state(br, mrp);
> +			if (err)
> +				return err;
> +			break;
> +		case IFLA_BRIDGE_MRP_RING_ROLE:
> +			err = br_mrp_parse_ring_role(br, mrp);
> +			if (err)
> +				return err;
> +			break;
> +		case IFLA_BRIDGE_MRP_START_TEST:
> +			err = br_mrp_parse_start_test(br, mrp);
> +			if (err)
> +				return err;
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}

All of the above can be implemented via nla_parse_nested() with a proper policy.
You don't have to manually check for the attribute size. Then your code follows
the netlink normal (e.g. check the bridge netlink handling) of:
 nla_parse_nested(tb)
 if (tb[attr])
    do_something_with(tb[attr])
 ...


> +
> +void br_mrp_port_open(struct net_device *dev, u8 loc)
> +{
> +	struct nlattr *af, *mrp;
> +	struct ifinfomsg *hdr;
> +	struct nlmsghdr *nlh;
> +	struct sk_buff *skb;
> +	int err = -ENOBUFS;
> +	struct net *net;
> +
> +	net = dev_net(dev);
> +
> +	skb = nlmsg_new(1024, GFP_ATOMIC);

Please add a function which calculates the size based on the attribute sizes.

> +	if (!skb)
> +		goto errout;
> +
> +	nlh = nlmsg_put(skb, 0, 0, RTM_NEWLINK, sizeof(*hdr), 0);
> +	if (!nlh)
> +		goto errout;
> +
> +	hdr = nlmsg_data(nlh);
> +	hdr->ifi_family = AF_BRIDGE;
> +	hdr->__ifi_pad = 0;
> +	hdr->ifi_type = dev->type;
> +	hdr->ifi_index = dev->ifindex;
> +	hdr->ifi_flags = dev_get_flags(dev);
> +	hdr->ifi_change = 0;
> +
> +	af = nla_nest_start_noflag(skb, IFLA_AF_SPEC);
> +	mrp = nla_nest_start_noflag(skb, IFLA_BRIDGE_MRP);
> +

These can return an error which has to be handled.

> +	nla_put_u32(skb, IFLA_BRIDGE_MRP_RING_OPEN, loc);
> +

Same for this.

> +	nla_nest_end(skb, mrp);
> +	nla_nest_end(skb, af);
> +	nlmsg_end(skb, nlh);
> +
> +	rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL, GFP_ATOMIC);
> +	return;
> +errout:
> +	rtnl_set_sk_err(net, RTNLGRP_LINK, err);
> +}
> +EXPORT_SYMBOL(br_mrp_port_open);
> 

Re: [RFC net-next v4 4/9] bridge: mrp: Implement netlink interface to configure MRP

[Horatiu Vultur]

The 03/30/2020 18:30, Nikolay Aleksandrov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 27/03/2020 11:21, Horatiu Vultur wrote:
> > Implement netlink interface to configure MRP. The implementation
> > will do sanity checks over the attributes and then eventually call the MRP
> > interface.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  net/bridge/br_mrp_netlink.c | 176 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 176 insertions(+)
> >  create mode 100644 net/bridge/br_mrp_netlink.c
> >
> 
> Hi Horatiu,

Hi Nik,

Thanks for the feedback, I will update all this in the new patch series.

> 
> > diff --git a/net/bridge/br_mrp_netlink.c b/net/bridge/br_mrp_netlink.c
> > new file mode 100644
> > index 000000000000..043b7f6cddbe
> > --- /dev/null
> > +++ b/net/bridge/br_mrp_netlink.c
> > @@ -0,0 +1,176 @@
> > +// 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 int br_mrp_parse_instance(struct net_bridge *br, struct nlattr *attr,
> > +                              int cmd)
> > +{
> > +     struct br_mrp_instance *instance;
> > +
> > +     if (nla_len(attr) != sizeof(*instance))
> > +             return -EINVAL;
> > +
> > +     instance = nla_data(attr);
> > +
> > +     if (cmd == RTM_SETLINK)
> > +             return br_mrp_add(br, instance);
> > +     else
> > +             return br_mrp_del(br, instance);
> > +}
> > +
> > +static int br_mrp_parse_port_state(struct net_bridge *br,
> > +                                struct net_bridge_port *p,
> > +                                struct nlattr *attr)
> > +{
> > +     enum br_mrp_port_state_type state;
> > +
> > +     if (nla_len(attr) != sizeof(u32) || !p)
> > +             return -EINVAL;
> > +
> > +     state = nla_get_u32(attr);
> > +
> > +     return br_mrp_set_port_state(p, state);
> > +}
> > +
> > +static int br_mrp_parse_port_role(struct net_bridge *br,
> > +                               struct net_bridge_port *p,
> > +                               struct nlattr *attr)
> > +{
> > +     struct br_mrp_port_role *role;
> > +
> > +     if (nla_len(attr) != sizeof(*role) || !p)
> > +             return -EINVAL;
> > +
> > +     role = nla_data(attr);
> > +
> > +     return br_mrp_set_port_role(p, role->ring_id, role->role);
> > +}
> > +
> > +static int br_mrp_parse_ring_state(struct net_bridge *br, struct nlattr *attr)
> > +{
> > +     struct br_mrp_ring_state *state;
> > +
> > +     if (nla_len(attr) != sizeof(*state))
> > +             return -EINVAL;
> > +
> > +     state = nla_data(attr);
> > +
> > +     return br_mrp_set_ring_state(br, state->ring_id, state->ring_state);
> > +}
> > +
> > +static int br_mrp_parse_ring_role(struct net_bridge *br, struct nlattr *attr)
> > +{
> > +     struct br_mrp_ring_role *role;
> > +
> > +     if (nla_len(attr) != sizeof(*role))
> > +             return -EINVAL;
> > +
> > +     role = nla_data(attr);
> > +
> > +     return br_mrp_set_ring_role(br, role->ring_id, role->ring_role);
> > +}
> > +
> > +static int br_mrp_parse_start_test(struct net_bridge *br, struct nlattr *attr)
> > +{
> > +     struct br_mrp_start_test *test;
> > +
> > +     if (nla_len(attr) != sizeof(*test))
> > +             return -EINVAL;
> > +
> > +     test = nla_data(attr);
> > +
> > +     return br_mrp_start_test(br, test->ring_id, test->interval,
> > +                              test->max_miss, test->period);
> > +}
> > +
> > +int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
> > +              struct nlattr *attr, int cmd)
> > +{
> > +     struct nlattr *mrp;
> > +     int rem, err;
> > +
> > +     nla_for_each_nested(mrp, attr, rem) {
> > +             err = 0;
> > +             switch (nla_type(mrp)) {
> > +             case IFLA_BRIDGE_MRP_INSTANCE:
> > +                     err = br_mrp_parse_instance(br, mrp, cmd);
> > +                     if (err)
> > +                             return err;
> > +                     break;
> > +             case IFLA_BRIDGE_MRP_PORT_STATE:
> > +                     err = br_mrp_parse_port_state(br, p, mrp);
> > +                     if (err)
> > +                             return err;
> > +                     break;
> > +             case IFLA_BRIDGE_MRP_PORT_ROLE:
> > +                     err = br_mrp_parse_port_role(br, p, mrp);
> > +                     if (err)
> > +                             return err;
> > +                     break;
> > +             case IFLA_BRIDGE_MRP_RING_STATE:
> > +                     err = br_mrp_parse_ring_state(br, mrp);
> > +                     if (err)
> > +                             return err;
> > +                     break;
> > +             case IFLA_BRIDGE_MRP_RING_ROLE:
> > +                     err = br_mrp_parse_ring_role(br, mrp);
> > +                     if (err)
> > +                             return err;
> > +                     break;
> > +             case IFLA_BRIDGE_MRP_START_TEST:
> > +                     err = br_mrp_parse_start_test(br, mrp);
> > +                     if (err)
> > +                             return err;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> 
> All of the above can be implemented via nla_parse_nested() with a proper policy.
> You don't have to manually check for the attribute size. Then your code follows
> the netlink normal (e.g. check the bridge netlink handling) of:
>  nla_parse_nested(tb)
>  if (tb[attr])
>     do_something_with(tb[attr])
>  ...
> 
> 
> > +
> > +void br_mrp_port_open(struct net_device *dev, u8 loc)
> > +{
> > +     struct nlattr *af, *mrp;
> > +     struct ifinfomsg *hdr;
> > +     struct nlmsghdr *nlh;
> > +     struct sk_buff *skb;
> > +     int err = -ENOBUFS;
> > +     struct net *net;
> > +
> > +     net = dev_net(dev);
> > +
> > +     skb = nlmsg_new(1024, GFP_ATOMIC);
> 
> Please add a function which calculates the size based on the attribute sizes.
> 
> > +     if (!skb)
> > +             goto errout;
> > +
> > +     nlh = nlmsg_put(skb, 0, 0, RTM_NEWLINK, sizeof(*hdr), 0);
> > +     if (!nlh)
> > +             goto errout;
> > +
> > +     hdr = nlmsg_data(nlh);
> > +     hdr->ifi_family = AF_BRIDGE;
> > +     hdr->__ifi_pad = 0;
> > +     hdr->ifi_type = dev->type;
> > +     hdr->ifi_index = dev->ifindex;
> > +     hdr->ifi_flags = dev_get_flags(dev);
> > +     hdr->ifi_change = 0;
> > +
> > +     af = nla_nest_start_noflag(skb, IFLA_AF_SPEC);
> > +     mrp = nla_nest_start_noflag(skb, IFLA_BRIDGE_MRP);
> > +
> 
> These can return an error which has to be handled.
> 
> > +     nla_put_u32(skb, IFLA_BRIDGE_MRP_RING_OPEN, loc);
> > +
> 
> Same for this.
> 
> > +     nla_nest_end(skb, mrp);
> > +     nla_nest_end(skb, af);
> > +     nlmsg_end(skb, nlh);
> > +
> > +     rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL, GFP_ATOMIC);
> > +     return;
> > +errout:
> > +     rtnl_set_sk_err(net, RTNLGRP_LINK, err);
> > +}
> > +EXPORT_SYMBOL(br_mrp_port_open);
> >
> 

-- 
/Horatiu

[RFC net-next v4 5/9] switchdev: mrp: 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_MRP: This is used when a MRP instance 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 MRM and MRC.

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 MRM. In case this fails then the SW will generate the frames.

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 | 53 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index aee86a189432..3323779122c7 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,
+#if IS_ENABLED(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 */
+#if IS_ENABLED(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,
+#if IS_ENABLED(CONFIG_BRIDGE_MRP)
+	SWITCHDEV_OBJ_ID_MRP,
+	SWITCHDEV_OBJ_ID_RING_TEST_MRP,
+	SWITCHDEV_OBJ_ID_RING_ROLE_MRP,
+#endif
 };
 
 struct switchdev_obj {
@@ -94,6 +109,44 @@ struct switchdev_obj_port_mdb {
 #define SWITCHDEV_OBJ_PORT_MDB(OBJ) \
 	container_of((OBJ), struct switchdev_obj_port_mdb, obj)
 
+
+#if IS_ENABLED(CONFIG_BRIDGE_MRP)
+/* SWITCHDEV_OBJ_ID_MRP */
+struct switchdev_obj_mrp {
+	struct switchdev_obj obj;
+	struct net_device *p_port;
+	struct net_device *s_port;
+	u32 ring_id;
+};
+
+#define SWITCHDEV_OBJ_MRP(OBJ) \
+	container_of((OBJ), struct switchdev_obj_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_id;
+	u32 period;
+};
+
+#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_id;
+};
+
+#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

[RFC net-next v4 6/9] bridge: switchdev: mrp 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 | 150 ++++++++++++++++++++++++++++++++++
 1 file changed, 150 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..7b27721fd4cf
--- /dev/null
+++ b/net/bridge/br_mrp_switchdev.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <net/switchdev.h>
+
+#include "br_private_mrp.h"
+
+int br_mrp_switchdev_add(struct br_mrp *mrp)
+{
+	struct net_bridge *br = mrp->br;
+	struct switchdev_obj_mrp mrp_obj = {
+		.obj.orig_dev = br->dev,
+		.obj.id = SWITCHDEV_OBJ_ID_MRP,
+		.p_port = rcu_dereference(mrp->p_port)->dev,
+		.s_port = rcu_dereference(mrp->s_port)->dev,
+		.ring_id = mrp->ring_id,
+	};
+	int err = 0;
+
+	err = switchdev_port_obj_add(br->dev, &mrp_obj.obj, NULL);
+
+	if (err && err != -EOPNOTSUPP)
+		return err;
+
+	return 0;
+}
+
+int br_mrp_switchdev_del(struct br_mrp *mrp)
+{
+	struct net_bridge *br = mrp->br;
+	struct switchdev_obj_mrp mrp_obj = {
+		.obj.orig_dev = br->dev,
+		.obj.id = SWITCHDEV_OBJ_ID_MRP,
+		.p_port = rcu_dereference(mrp->p_port)->dev,
+		.s_port = rcu_dereference(mrp->s_port)->dev,
+		.ring_id = mrp->ring_id,
+	};
+	int err = 0;
+
+	err = switchdev_port_obj_del(br->dev, &mrp_obj.obj);
+
+	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_id = mrp->ring_id,
+	};
+	int err = 0;
+
+	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, u32 period)
+{
+	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_id = mrp->ring_id,
+		.period = period,
+	};
+	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_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;
+
+	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;
+
+	rcu_read_lock();
+
+	attr.orig_dev = rcu_dereference(mrp->p_port)->dev,
+	err = switchdev_port_attr_set(attr.orig_dev, &attr);
+	if (err && err != -EOPNOTSUPP)
+		goto unlock;
+
+	attr.orig_dev = rcu_dereference(mrp->s_port)->dev;
+	err = switchdev_port_attr_set(attr.orig_dev, &attr);
+	if (err && err != -EOPNOTSUPP)
+		goto unlock;
+
+unlock:
+	rcu_read_unlock();
+
+	return err;
+}
-- 
2.17.1

[RFC net-next v4 7/9] bridge: mrp: Connect MRP api with the switchev API

[Horatiu Vultur]

Implement the MRP api.

In case the HW can't generate MRP Test frames then the SW will try to generate
the frames. In case that also the SW will fail in generating the frames then a
error is return to the userspace. The userspace is responsible to generate all
the other MRP frames regardless if the test frames are generated by HW or SW.

The forwarding/termination of MRP frames is happening in the kernel and is done
by the MRP instance. The userspace application doesn't do the forwarding.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_mrp.c | 514 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 514 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..f1de792d7a6e
--- /dev/null
+++ b/net/bridge/br_mrp.c
@@ -0,0 +1,514 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include "br_private_mrp.h"
+
+static const u8 mrp_test_dmac[ETH_ALEN] = { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 };
+
+static struct net_bridge_port *br_mrp_get_port(struct net_bridge *br,
+					       u32 ifindex)
+{
+	struct net_bridge_port *res = NULL;
+	struct net_bridge_port *port;
+
+	spin_lock_bh(&br->lock);
+
+	list_for_each_entry(port, &br->port_list, list) {
+		if (port->dev->ifindex == ifindex) {
+			res = port;
+			break;
+		}
+	}
+
+	spin_unlock_bh(&br->lock);
+
+	return res;
+}
+
+static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_id)
+{
+	struct br_mrp *res = NULL;
+	struct br_mrp *mrp;
+
+	rcu_read_lock();
+
+	list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
+		if (mrp->ring_id == ring_id) {
+			res = mrp;
+			break;
+		}
+	}
+
+	rcu_read_unlock();
+
+	return res;
+}
+
+static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
+				       struct net_bridge_port *p)
+{
+	struct br_mrp *res = NULL;
+	struct br_mrp *mrp;
+
+	rcu_read_lock();
+
+	list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
+		if (rcu_dereference(mrp->p_port) == p ||
+		    rcu_dereference(mrp->s_port) == p) {
+			res = mrp;
+			break;
+		}
+	}
+
+	rcu_read_unlock();
+
+	return res;
+}
+
+static int br_mrp_next_seq(struct br_mrp *mrp)
+{
+	mrp->seq_id++;
+	return mrp->seq_id;
+}
+
+static struct sk_buff *br_mrp_skb_alloc(struct net_bridge_port *p,
+					const u8 *src, const u8 *dst)
+{
+	struct ethhdr *eth_hdr;
+	struct sk_buff *skb;
+	u16 *version;
+
+	skb = dev_alloc_skb(MRP_MAX_FRAME_LENGTH);
+	if (!skb)
+		return NULL;
+
+	skb->dev = p->dev;
+	skb->protocol = htons(ETH_P_MRP);
+	skb->priority = MRP_FRAME_PRIO;
+	skb_reserve(skb, sizeof(*eth_hdr));
+
+	eth_hdr = skb_push(skb, sizeof(*eth_hdr));
+	ether_addr_copy(eth_hdr->h_dest, dst);
+	ether_addr_copy(eth_hdr->h_source, src);
+	eth_hdr->h_proto = htons(ETH_P_MRP);
+
+	version = skb_put(skb, sizeof(*version));
+	*version = cpu_to_be16(MRP_VERSION);
+
+	return skb;
+}
+
+static void br_mrp_skb_tlv(struct sk_buff *skb,
+			   enum br_mrp_tlv_header_type type,
+			   u8 length)
+{
+	struct br_mrp_tlv_hdr *hdr;
+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->type = type;
+	hdr->length = length;
+}
+
+static void br_mrp_skb_common(struct sk_buff *skb, struct br_mrp *mrp)
+{
+	struct br_mrp_common_hdr *hdr;
+
+	br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_COMMON, sizeof(*hdr));
+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->seq_id = cpu_to_be16(br_mrp_next_seq(mrp));
+	memset(hdr->domain, 0xff, MRP_DOMAIN_UUID_LENGTH);
+}
+
+static struct sk_buff *br_mrp_alloc_test_skb(struct br_mrp *mrp,
+					     struct net_device *dev,
+					     enum br_mrp_port_role_type port_role)
+{
+	struct net_bridge_port *p = br_port_get_rtnl(dev);
+	struct br_mrp_ring_test_hdr *hdr = NULL;
+	struct net_bridge *br = p->br;
+	struct sk_buff *skb = NULL;
+
+	if (!p)
+		return NULL;
+
+	br = p->br;
+
+	skb = br_mrp_skb_alloc(p, p->dev->dev_addr, mrp_test_dmac);
+	if (!skb)
+		return NULL;
+
+	br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_RING_TEST, sizeof(*hdr));
+	hdr = skb_put(skb, sizeof(*hdr));
+
+	hdr->prio = cpu_to_be16(MRP_DEFAULT_PRIO);
+	ether_addr_copy(hdr->sa, p->br->dev->dev_addr);
+	hdr->port_role = cpu_to_be16(port_role);
+	hdr->state = cpu_to_be16(mrp->ring_state);
+	hdr->transitions = cpu_to_be16(mrp->ring_transitions);
+	hdr->timestamp = cpu_to_be32(jiffies_to_msecs(jiffies));
+
+	br_mrp_skb_common(skb, mrp);
+	br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_END, 0x0);
+
+	return skb;
+}
+
+static void br_mrp_test_work_expired(struct work_struct *work)
+{
+	struct delayed_work *del_work = to_delayed_work(work);
+	struct br_mrp *mrp = container_of(del_work, struct br_mrp, test_work);
+	bool notify_open = false;
+	struct sk_buff *skb;
+
+	if (time_before_eq(mrp->test_end, jiffies))
+		return;
+
+	if (mrp->test_count_miss < mrp->test_max_miss) {
+		mrp->test_count_miss++;
+	} else {
+		/* Notify that the ring is open only if the ring state is
+		 * closed, otherwise it would continue to notify at every
+		 * interval.
+		 */
+		if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED)
+			notify_open = true;
+	}
+
+	local_bh_disable();
+	rcu_read_lock();
+
+	if (!rcu_access_pointer(mrp->p_port) ||
+	    !rcu_access_pointer(mrp->s_port))
+		goto out;
+
+	/* Is it possible here to get call to delete the bridge port? If yes
+	 * I need to protect it
+	 */
+	dev_hold(rcu_dereference(mrp->p_port)->dev);
+
+	skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->p_port)->dev,
+				    BR_MRP_PORT_ROLE_PRIMARY);
+	if (!skb)
+		goto out;
+
+	skb_reset_network_header(skb);
+	dev_queue_xmit(skb);
+
+	if (notify_open && !mrp->ring_role_offloaded)
+		br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, true);
+
+	dev_put(rcu_dereference(mrp->p_port)->dev);
+
+	dev_hold(rcu_dereference(mrp->s_port)->dev);
+
+	skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->s_port)->dev,
+				    BR_MRP_PORT_ROLE_SECONDARY);
+	if (!skb)
+		goto out;
+
+	skb_reset_network_header(skb);
+	dev_queue_xmit(skb);
+
+	if (notify_open && !mrp->ring_role_offloaded)
+		br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, true);
+
+	dev_put(rcu_dereference(mrp->s_port)->dev);
+
+out:
+	rcu_read_unlock();
+	local_bh_enable();
+
+	queue_delayed_work(system_wq, &mrp->test_work,
+			   usecs_to_jiffies(mrp->test_interval));
+}
+
+/* Adds a new MRP instance.
+ * note: called under rtnl_lock
+ */
+int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
+{
+	struct net_bridge_port *p;
+	struct br_mrp *mrp;
+
+	/* If the ring exists, it is not possible to create another one with the
+	 * same ring_id
+	 */
+	mrp = br_mrp_find_id(br, instance->ring_id);
+	if (mrp)
+		return -EINVAL;
+
+	if (!br_mrp_get_port(br, instance->p_ifindex) ||
+	    !br_mrp_get_port(br, instance->s_ifindex))
+		return -EINVAL;
+
+	mrp = devm_kzalloc(&br->dev->dev, sizeof(struct br_mrp), GFP_KERNEL);
+	if (!mrp)
+		return -ENOMEM;
+
+	/* I think is not needed because this can be replaced with rtnl lock*/
+	spin_lock_init(&mrp->lock);
+	spin_lock(&mrp->lock);
+
+	mrp->br = br;
+	mrp->ring_id = instance->ring_id;
+
+	p = br_mrp_get_port(br, instance->p_ifindex);
+	p->state = BR_STATE_FORWARDING;
+	p->flags |= BR_MRP_AWARE;
+	rcu_assign_pointer(mrp->p_port, p);
+
+	p = br_mrp_get_port(br, instance->s_ifindex);
+	p->state = BR_STATE_FORWARDING;
+	p->flags |= BR_MRP_AWARE;
+	rcu_assign_pointer(mrp->s_port, p);
+
+	br_mrp_switchdev_add(mrp);
+
+	spin_unlock(&mrp->lock);
+	synchronize_rcu();
+
+	list_add_tail_rcu(&mrp->list, &br->mrp_list);
+	INIT_DELAYED_WORK(&mrp->test_work, br_mrp_test_work_expired);
+
+	return 0;
+}
+
+/* Deletes existing MRP instance.
+ * note: called under rtnl_lock
+ */
+int br_mrp_del(struct net_bridge *br, struct br_mrp_instance *instance)
+{
+	struct br_mrp *mrp = br_mrp_find_id(br, instance->ring_id);
+	struct net_bridge_port *p;
+
+	if (!mrp)
+		return -EINVAL;
+
+	/* Stop sending MRP_Test frames */
+	cancel_delayed_work(&mrp->test_work);
+	br_mrp_switchdev_send_ring_test(mrp, 0, 0, 0);
+
+	spin_lock(&mrp->lock);
+
+	br_mrp_switchdev_del(mrp);
+
+	/* Reset the ports */
+	p = rcu_dereference_protected(mrp->p_port, lockdep_is_held(&mrp->lock));
+	if (p) {
+		spin_lock(&br->lock);
+		p->state = BR_STATE_FORWARDING;
+		p->flags &= ~BR_MRP_AWARE;
+		br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
+		rcu_assign_pointer(mrp->p_port, NULL);
+		spin_unlock(&br->lock);
+	}
+
+	p = rcu_dereference_protected(mrp->s_port, lockdep_is_held(&mrp->lock));
+	if (p) {
+		spin_lock(&br->lock);
+		p->state = BR_STATE_FORWARDING;
+		p->flags &= ~BR_MRP_AWARE;
+		br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
+		rcu_assign_pointer(mrp->s_port, NULL);
+		spin_unlock(&br->lock);
+	}
+
+	/* Destroy the ring */
+	mrp->br = NULL;
+
+	spin_unlock(&mrp->lock);
+	synchronize_rcu();
+
+	list_del_rcu(&mrp->list);
+	devm_kfree(&br->dev->dev, mrp);
+
+	return 0;
+}
+
+int br_mrp_set_port_state(struct net_bridge_port *p,
+			  enum br_mrp_port_state_type state)
+{
+	spin_lock(&p->br->lock);
+
+	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);
+
+	spin_unlock(&p->br->lock);
+
+	return 0;
+}
+
+int br_mrp_set_port_role(struct net_bridge_port *p,
+			 u32 ring_id, enum br_mrp_port_role_type role)
+{
+	struct br_mrp *mrp = br_mrp_find_id(p->br, ring_id);
+
+	if (!mrp)
+		return -EINVAL;
+
+	spin_lock(&mrp->lock);
+
+	if (role == BR_MRP_PORT_ROLE_PRIMARY)
+		rcu_assign_pointer(mrp->p_port, p);
+	if (role == BR_MRP_PORT_ROLE_SECONDARY)
+		rcu_assign_pointer(mrp->s_port, p);
+
+	br_mrp_port_switchdev_set_role(p, role);
+
+	spin_unlock(&mrp->lock);
+	synchronize_rcu();
+
+	return 0;
+}
+
+int br_mrp_set_ring_state(struct net_bridge *br, u32 ring_id,
+			  enum br_mrp_ring_state_type state)
+{
+	struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
+
+	if (!mrp)
+		return -EINVAL;
+
+	if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED &&
+	    state != BR_MRP_RING_STATE_CLOSED)
+		mrp->ring_transitions++;
+
+	mrp->ring_state = state;
+
+	br_mrp_switchdev_set_ring_state(mrp, state);
+
+	return 0;
+}
+
+int br_mrp_set_ring_role(struct net_bridge *br, u32 ring_id,
+			 enum br_mrp_ring_role_type role)
+{
+	struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
+	int err;
+
+	if (!mrp)
+		return -EINVAL;
+
+	mrp->ring_role = role;
+
+	/* If there is an error just bailed out */
+	err = br_mrp_switchdev_set_ring_role(mrp, role);
+	if (err && err != -EOPNOTSUPP)
+		return err;
+
+	/* Now detect if the HW actually applied the role or not. If the HW
+	 * applied the role it means that the SW will not to do those operations
+	 * anymore. For example if the role ir MRM then the HW will notify the
+	 * SW when ring is open, but if the is not pushed to the HW the SW will
+	 * need to detect when the ring is open
+	 */
+	mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
+
+	return 0;
+}
+
+int br_mrp_start_test(struct net_bridge *br, u32 ring_id, u32 interval,
+		      u8 max_miss, u32 period)
+{
+	struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
+
+	if (!mrp)
+		return -EINVAL;
+
+	/* Try to push is to the HW and if it fails then continue to generate in
+	 * SW and if that also fails then return error
+	 */
+	if (!br_mrp_switchdev_send_ring_test(mrp, interval, max_miss, period))
+		return 0;
+
+	mrp->test_interval = interval;
+	mrp->test_end = jiffies + usecs_to_jiffies(period);
+	mrp->test_max_miss = max_miss;
+	mrp->test_count_miss = 0;
+	queue_delayed_work(system_wq, &mrp->test_work,
+			   usecs_to_jiffies(interval));
+
+	return 0;
+}
+
+/* Process only MRP Test frame. All the other MRP frames are processed by
+ * userspace application
+ * note: already called with rcu_read_lock
+ */
+static void br_mrp_mrm_process(struct br_mrp *mrp, struct sk_buff *skb)
+{
+	struct br_mrp_tlv_hdr *hdr;
+
+	hdr = (struct br_mrp_tlv_hdr *)(skb->data + sizeof(uint16_t));
+
+	if (hdr->type != BR_MRP_TLV_HEADER_RING_TEST)
+		return;
+
+	mrp->test_count_miss = 0;
+
+	br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, false);
+	br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, false);
+}
+
+/* This will just forward the frame to the other mrp ring port(MRC role) or will
+ * not do anything.
+ * note: already called with rcu_read_lock
+ */
+static int br_mrp_rcv(struct net_bridge_port *p,
+		      struct sk_buff *skb, struct net_device *dev)
+{
+	struct net_device *s_dev, *p_dev, *d_dev;
+	struct net_bridge *br;
+	struct sk_buff *nskb;
+	struct br_mrp *mrp;
+
+	/* If port is disable don't accept any frames */
+	if (p->state == BR_STATE_DISABLED)
+		return 0;
+
+	br = p->br;
+	mrp =  br_mrp_find_port(br, p);
+	if (unlikely(!mrp))
+		return 0;
+
+	/* If the role is MRM then don't forward the frames */
+	if (mrp->ring_role == BR_MRP_RING_ROLE_MRM) {
+		br_mrp_mrm_process(mrp, skb);
+		return 1;
+	}
+
+	nskb = skb_clone(skb, GFP_ATOMIC);
+	if (!nskb)
+		return 0;
+
+	p_dev = rcu_dereference(mrp->p_port)->dev;
+	s_dev = rcu_dereference(mrp->s_port)->dev;
+
+	if (p_dev == dev)
+		d_dev = s_dev;
+	else
+		d_dev = p_dev;
+
+	nskb->dev = d_dev;
+	skb_push(nskb, ETH_HLEN);
+	dev_queue_xmit(nskb);
+
+	return 1;
+}
+
+int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
+{
+	/* If there is no MRP instance do normal forwarding */
+	if (unlikely(!(p->flags & BR_MRP_AWARE)))
+		goto out;
+
+	if (unlikely(skb->protocol == htons(ETH_P_MRP)))
+		return br_mrp_rcv(p, skb, p->dev);
+
+out:
+	return 0;
+}
-- 
2.17.1

Re: [RFC net-next v4 7/9] bridge: mrp: Connect MRP api with the switchev API

[Nikolay Aleksandrov]

On 27/03/2020 11:21, Horatiu Vultur wrote:
> Implement the MRP api.
> 
> In case the HW can't generate MRP Test frames then the SW will try to generate
> the frames. In case that also the SW will fail in generating the frames then a
> error is return to the userspace. The userspace is responsible to generate all
> the other MRP frames regardless if the test frames are generated by HW or SW.
> 
> The forwarding/termination of MRP frames is happening in the kernel and is done
> by the MRP instance. The userspace application doesn't do the forwarding.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  net/bridge/br_mrp.c | 514 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 514 insertions(+)
>  create mode 100644 net/bridge/br_mrp.c
> 

Hi,
In general the RCU usage needs more work. Also I might've missed it, but where do you
handle bridge port delete which is used in mrp ?
Also do you actually need the mrp->lock ?

> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
> new file mode 100644
> index 000000000000..f1de792d7a6e
> --- /dev/null
> +++ b/net/bridge/br_mrp.c
> @@ -0,0 +1,514 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include "br_private_mrp.h"
> +
> +static const u8 mrp_test_dmac[ETH_ALEN] = { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 };
> +
> +static struct net_bridge_port *br_mrp_get_port(struct net_bridge *br,
> +					       u32 ifindex)
> +{
> +	struct net_bridge_port *res = NULL;
> +	struct net_bridge_port *port;
> +
> +	spin_lock_bh(&br->lock);
> +

This is called under RTNL, you don't need the br->lock.

> +	list_for_each_entry(port, &br->port_list, list) {
> +		if (port->dev->ifindex == ifindex) {
> +			res = port;
> +			break;
> +		}
> +	}
> +
> +	spin_unlock_bh(&br->lock);
> +
> +	return res;
> +}
> +
> +static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_id)
> +{
> +	struct br_mrp *res = NULL;
> +	struct br_mrp *mrp;
> +
> +	rcu_read_lock();
> +

This is generally a bad pattern because it can hide legitimate bugs and make
it harder to debug.

> +	list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
> +		if (mrp->ring_id == ring_id) {
> +			res = mrp;
> +			break;
> +		}
> +	}
> +
> +	rcu_read_unlock();
> +
> +	return res;
> +}
> +
> +static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
> +				       struct net_bridge_port *p)
> +{
> +	struct br_mrp *res = NULL;
> +	struct br_mrp *mrp;
> +
> +	rcu_read_lock();
> +
> +	list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
> +		if (rcu_dereference(mrp->p_port) == p ||
> +		    rcu_dereference(mrp->s_port) == p) {

rcu_access_pointer() is ok for comparisons

> +			res = mrp;
> +			break;
> +		}
> +	}
> +
> +	rcu_read_unlock();
> +
> +	return res;
> +}
> +
> +static int br_mrp_next_seq(struct br_mrp *mrp)
> +{
> +	mrp->seq_id++;
> +	return mrp->seq_id;
> +}
> +
> +static struct sk_buff *br_mrp_skb_alloc(struct net_bridge_port *p,
> +					const u8 *src, const u8 *dst)
> +{
> +	struct ethhdr *eth_hdr;
> +	struct sk_buff *skb;
> +	u16 *version;
> +
> +	skb = dev_alloc_skb(MRP_MAX_FRAME_LENGTH);
> +	if (!skb)
> +		return NULL;
> +
> +	skb->dev = p->dev;
> +	skb->protocol = htons(ETH_P_MRP);
> +	skb->priority = MRP_FRAME_PRIO;
> +	skb_reserve(skb, sizeof(*eth_hdr));
> +
> +	eth_hdr = skb_push(skb, sizeof(*eth_hdr));
> +	ether_addr_copy(eth_hdr->h_dest, dst);
> +	ether_addr_copy(eth_hdr->h_source, src);
> +	eth_hdr->h_proto = htons(ETH_P_MRP);
> +
> +	version = skb_put(skb, sizeof(*version));
> +	*version = cpu_to_be16(MRP_VERSION);
> +
> +	return skb;
> +}
> +
> +static void br_mrp_skb_tlv(struct sk_buff *skb,
> +			   enum br_mrp_tlv_header_type type,
> +			   u8 length)
> +{
> +	struct br_mrp_tlv_hdr *hdr;
> +
> +	hdr = skb_put(skb, sizeof(*hdr));
> +	hdr->type = type;
> +	hdr->length = length;
> +}
> +
> +static void br_mrp_skb_common(struct sk_buff *skb, struct br_mrp *mrp)
> +{
> +	struct br_mrp_common_hdr *hdr;
> +
> +	br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_COMMON, sizeof(*hdr));
> +
> +	hdr = skb_put(skb, sizeof(*hdr));
> +	hdr->seq_id = cpu_to_be16(br_mrp_next_seq(mrp));
> +	memset(hdr->domain, 0xff, MRP_DOMAIN_UUID_LENGTH);
> +}
> +
> +static struct sk_buff *br_mrp_alloc_test_skb(struct br_mrp *mrp,
> +					     struct net_device *dev,
> +					     enum br_mrp_port_role_type port_role)
> +{
> +	struct net_bridge_port *p = br_port_get_rtnl(dev);
> +	struct br_mrp_ring_test_hdr *hdr = NULL;
> +	struct net_bridge *br = p->br;
> +	struct sk_buff *skb = NULL;
> +
> +	if (!p)
> +		return NULL;
> +
> +	br = p->br;
> +
> +	skb = br_mrp_skb_alloc(p, p->dev->dev_addr, mrp_test_dmac);
> +	if (!skb)
> +		return NULL;
> +
> +	br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_RING_TEST, sizeof(*hdr));
> +	hdr = skb_put(skb, sizeof(*hdr));
> +
> +	hdr->prio = cpu_to_be16(MRP_DEFAULT_PRIO);
> +	ether_addr_copy(hdr->sa, p->br->dev->dev_addr);
> +	hdr->port_role = cpu_to_be16(port_role);
> +	hdr->state = cpu_to_be16(mrp->ring_state);
> +	hdr->transitions = cpu_to_be16(mrp->ring_transitions);
> +	hdr->timestamp = cpu_to_be32(jiffies_to_msecs(jiffies));
> +
> +	br_mrp_skb_common(skb, mrp);
> +	br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_END, 0x0);
> +
> +	return skb;
> +}
> +
> +static void br_mrp_test_work_expired(struct work_struct *work)
> +{
> +	struct delayed_work *del_work = to_delayed_work(work);
> +	struct br_mrp *mrp = container_of(del_work, struct br_mrp, test_work);
> +	bool notify_open = false;
> +	struct sk_buff *skb;
> +

Since this runs asynchronously what happens if the port is deleted ?

> +	if (time_before_eq(mrp->test_end, jiffies))
> +		return;
> +
> +	if (mrp->test_count_miss < mrp->test_max_miss) {
> +		mrp->test_count_miss++;
> +	} else {
> +		/* Notify that the ring is open only if the ring state is
> +		 * closed, otherwise it would continue to notify at every
> +		 * interval.
> +		 */
> +		if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED)
> +			notify_open = true;
> +	}
> +
> +	local_bh_disable();
> +	rcu_read_lock();
> +
> +	if (!rcu_access_pointer(mrp->p_port) ||
> +	    !rcu_access_pointer(mrp->s_port))
> +		goto out;
> +
> +	/* Is it possible here to get call to delete the bridge port? If yes
> +	 * I need to protect it
> +	 */
> +	dev_hold(rcu_dereference(mrp->p_port)->dev);
> +

This looks all wrong, p_port can become NULL here and you'll deref it.

> +	skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->p_port)->dev,
> +				    BR_MRP_PORT_ROLE_PRIMARY);
> +	if (!skb)
> +		goto out;
> +
> +	skb_reset_network_header(skb);
> +	dev_queue_xmit(skb);
> +
> +	if (notify_open && !mrp->ring_role_offloaded)
> +		br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, true);
> +
> +	dev_put(rcu_dereference(mrp->p_port)->dev);
> +
> +	dev_hold(rcu_dereference(mrp->s_port)->dev);
> +

same here

> +	skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->s_port)->dev,
> +				    BR_MRP_PORT_ROLE_SECONDARY);
> +	if (!skb)
> +		goto out;
> +
> +	skb_reset_network_header(skb);
> +	dev_queue_xmit(skb);
> +
> +	if (notify_open && !mrp->ring_role_offloaded)
> +		br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, true);
> +
> +	dev_put(rcu_dereference(mrp->s_port)->dev);
> +
> +out:
> +	rcu_read_unlock();
> +	local_bh_enable();
> +
> +	queue_delayed_work(system_wq, &mrp->test_work,
> +			   usecs_to_jiffies(mrp->test_interval));
> +}
> +
> +/* Adds a new MRP instance.
> + * note: called under rtnl_lock
> + */
> +int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
> +{
> +	struct net_bridge_port *p;
> +	struct br_mrp *mrp;
> +
> +	/* If the ring exists, it is not possible to create another one with the
> +	 * same ring_id
> +	 */
> +	mrp = br_mrp_find_id(br, instance->ring_id);
> +	if (mrp)
> +		return -EINVAL;
> +
> +	if (!br_mrp_get_port(br, instance->p_ifindex) ||
> +	    !br_mrp_get_port(br, instance->s_ifindex))
> +		return -EINVAL;
> +
> +	mrp = devm_kzalloc(&br->dev->dev, sizeof(struct br_mrp), GFP_KERNEL);
> +	if (!mrp)
> +		return -ENOMEM;
> +
> +	/* I think is not needed because this can be replaced with rtnl lock*/
> +	spin_lock_init(&mrp->lock);
> +	spin_lock(&mrp->lock);
> +
> +	mrp->br = br;

Is this field (mrp->br) used anywhere ?

> +	mrp->ring_id = instance->ring_id;
> +
> +	p = br_mrp_get_port(br, instance->p_ifindex);
> +	p->state = BR_STATE_FORWARDING;
> +	p->flags |= BR_MRP_AWARE;
> +	rcu_assign_pointer(mrp->p_port, p);
> +
> +	p = br_mrp_get_port(br, instance->s_ifindex);
> +	p->state = BR_STATE_FORWARDING;
> +	p->flags |= BR_MRP_AWARE;
> +	rcu_assign_pointer(mrp->s_port, p);
> +
> +	br_mrp_switchdev_add(mrp);
> +
> +	spin_unlock(&mrp->lock);
> +	synchronize_rcu();

Why do you need the synchronize here?

> +
> +	list_add_tail_rcu(&mrp->list, &br->mrp_list);
> +	INIT_DELAYED_WORK(&mrp->test_work, br_mrp_test_work_expired);
> +
> +	return 0;
> +}
> +
> +/* Deletes existing MRP instance.
> + * note: called under rtnl_lock
> + */
> +int br_mrp_del(struct net_bridge *br, struct br_mrp_instance *instance)
> +{
> +	struct br_mrp *mrp = br_mrp_find_id(br, instance->ring_id);
> +	struct net_bridge_port *p;
> +
> +	if (!mrp)
> +		return -EINVAL;
> +
> +	/* Stop sending MRP_Test frames */
> +	cancel_delayed_work(&mrp->test_work);

cancel_delayed_work_sync() if you'd like to make sure it's stopped and finished (if it was running
during this)

> +	br_mrp_switchdev_send_ring_test(mrp, 0, 0, 0);
> +
> +	spin_lock(&mrp->lock);
> +
> +	br_mrp_switchdev_del(mrp);
> +
> +	/* Reset the ports */
> +	p = rcu_dereference_protected(mrp->p_port, lockdep_is_held(&mrp->lock));
> +	if (p) {
> +		spin_lock(&br->lock);
> +		p->state = BR_STATE_FORWARDING;
> +		p->flags &= ~BR_MRP_AWARE;
> +		br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
> +		rcu_assign_pointer(mrp->p_port, NULL);
> +		spin_unlock(&br->lock);
> +	}
> +
> +	p = rcu_dereference_protected(mrp->s_port, lockdep_is_held(&mrp->lock));
> +	if (p) {
> +		spin_lock(&br->lock);
> +		p->state = BR_STATE_FORWARDING;
> +		p->flags &= ~BR_MRP_AWARE;
> +		br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
> +		rcu_assign_pointer(mrp->s_port, NULL);
> +		spin_unlock(&br->lock);
> +	}
> +
> +	/* Destroy the ring */
> +	mrp->br = NULL;
> +
> +	spin_unlock(&mrp->lock);
> +	synchronize_rcu();
> +
> +	list_del_rcu(&mrp->list);
> +	devm_kfree(&br->dev->dev, mrp);
> +
> +	return 0;
> +}
> +
> +int br_mrp_set_port_state(struct net_bridge_port *p,
> +			  enum br_mrp_port_state_type state)
> +{
> +	spin_lock(&p->br->lock);
> +
> +	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);
> +
> +	spin_unlock(&p->br->lock);
> +
> +	return 0;
> +}
> +
> +int br_mrp_set_port_role(struct net_bridge_port *p,
> +			 u32 ring_id, enum br_mrp_port_role_type role)
> +{
> +	struct br_mrp *mrp = br_mrp_find_id(p->br, ring_id);
> +
> +	if (!mrp)
> +		return -EINVAL;
> +
> +	spin_lock(&mrp->lock);
> +
> +	if (role == BR_MRP_PORT_ROLE_PRIMARY)
> +		rcu_assign_pointer(mrp->p_port, p);
> +	if (role == BR_MRP_PORT_ROLE_SECONDARY)
> +		rcu_assign_pointer(mrp->s_port, p);
> +
> +	br_mrp_port_switchdev_set_role(p, role);
> +
> +	spin_unlock(&mrp->lock);
> +	synchronize_rcu();

Why do you need to synchronize here?

> +
> +	return 0;
> +}
> +
> +int br_mrp_set_ring_state(struct net_bridge *br, u32 ring_id,
> +			  enum br_mrp_ring_state_type state)
> +{
> +	struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
> +
> +	if (!mrp)
> +		return -EINVAL;
> +
> +	if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED &&
> +	    state != BR_MRP_RING_STATE_CLOSED)
> +		mrp->ring_transitions++;
> +
> +	mrp->ring_state = state;
> +
> +	br_mrp_switchdev_set_ring_state(mrp, state);
> +
> +	return 0;
> +}
> +
> +int br_mrp_set_ring_role(struct net_bridge *br, u32 ring_id,
> +			 enum br_mrp_ring_role_type role)
> +{
> +	struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
> +	int err;
> +
> +	if (!mrp)
> +		return -EINVAL;
> +
> +	mrp->ring_role = role;
> +
> +	/* If there is an error just bailed out */
> +	err = br_mrp_switchdev_set_ring_role(mrp, role);
> +	if (err && err != -EOPNOTSUPP)
> +		return err;
> +
> +	/* Now detect if the HW actually applied the role or not. If the HW
> +	 * applied the role it means that the SW will not to do those operations
> +	 * anymore. For example if the role ir MRM then the HW will notify the
> +	 * SW when ring is open, but if the is not pushed to the HW the SW will
> +	 * need to detect when the ring is open
> +	 */
> +	mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
> +
> +	return 0;
> +}
> +
> +int br_mrp_start_test(struct net_bridge *br, u32 ring_id, u32 interval,
> +		      u8 max_miss, u32 period)
> +{
> +	struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
> +
> +	if (!mrp)
> +		return -EINVAL;
> +
> +	/* Try to push is to the HW and if it fails then continue to generate in
> +	 * SW and if that also fails then return error
> +	 */
> +	if (!br_mrp_switchdev_send_ring_test(mrp, interval, max_miss, period))
> +		return 0;
> +
> +	mrp->test_interval = interval;
> +	mrp->test_end = jiffies + usecs_to_jiffies(period);
> +	mrp->test_max_miss = max_miss;
> +	mrp->test_count_miss = 0;
> +	queue_delayed_work(system_wq, &mrp->test_work,
> +			   usecs_to_jiffies(interval));
> +
> +	return 0;
> +}
> +
> +/* Process only MRP Test frame. All the other MRP frames are processed by
> + * userspace application
> + * note: already called with rcu_read_lock
> + */
> +static void br_mrp_mrm_process(struct br_mrp *mrp, struct sk_buff *skb)
> +{
> +	struct br_mrp_tlv_hdr *hdr;
> +
> +	hdr = (struct br_mrp_tlv_hdr *)(skb->data + sizeof(uint16_t));
> +
> +	if (hdr->type != BR_MRP_TLV_HEADER_RING_TEST)
> +		return;
> +
> +	mrp->test_count_miss = 0;
> +
> +	br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, false);
> +	br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, false);
> +}
> +
> +/* This will just forward the frame to the other mrp ring port(MRC role) or will
> + * not do anything.
> + * note: already called with rcu_read_lock
> + */
> +static int br_mrp_rcv(struct net_bridge_port *p,
> +		      struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct net_device *s_dev, *p_dev, *d_dev;
> +	struct net_bridge *br;
> +	struct sk_buff *nskb;
> +	struct br_mrp *mrp;
> +
> +	/* If port is disable don't accept any frames */
> +	if (p->state == BR_STATE_DISABLED)
> +		return 0;
> +
> +	br = p->br;
> +	mrp =  br_mrp_find_port(br, p);
> +	if (unlikely(!mrp))
> +		return 0;
> +
> +	/* If the role is MRM then don't forward the frames */
> +	if (mrp->ring_role == BR_MRP_RING_ROLE_MRM) {
> +		br_mrp_mrm_process(mrp, skb);
> +		return 1;
> +	}
> +
> +	nskb = skb_clone(skb, GFP_ATOMIC);
> +	if (!nskb)
> +		return 0;
> +
> +	p_dev = rcu_dereference(mrp->p_port)->dev;
> +	s_dev = rcu_dereference(mrp->s_port)->dev;
> +

Not safe, could deref null.

> +	if (p_dev == dev)
> +		d_dev = s_dev;
> +	else
> +		d_dev = p_dev;
> +
> +	nskb->dev = d_dev;
> +	skb_push(nskb, ETH_HLEN);
> +	dev_queue_xmit(nskb);
> +
> +	return 1;
> +}
> +
> +int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
> +{
> +	/* If there is no MRP instance do normal forwarding */
> +	if (unlikely(!(p->flags & BR_MRP_AWARE)))

Shouldn't this one be likely() ?

> +		goto out;
> +
> +	if (unlikely(skb->protocol == htons(ETH_P_MRP)))
> +		return br_mrp_rcv(p, skb, p->dev);
> +
> +out:
> +	return 0;
> +}
> 

Re: [RFC net-next v4 7/9] bridge: mrp: Connect MRP api with the switchev API

[Horatiu Vultur]

The 03/30/2020 19:11, Nikolay Aleksandrov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 27/03/2020 11:21, Horatiu Vultur wrote:
> > Implement the MRP api.
> >
> > In case the HW can't generate MRP Test frames then the SW will try to generate
> > the frames. In case that also the SW will fail in generating the frames then a
> > error is return to the userspace. The userspace is responsible to generate all
> > the other MRP frames regardless if the test frames are generated by HW or SW.
> >
> > The forwarding/termination of MRP frames is happening in the kernel and is done
> > by the MRP instance. The userspace application doesn't do the forwarding.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  net/bridge/br_mrp.c | 514 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 514 insertions(+)
> >  create mode 100644 net/bridge/br_mrp.c
> >
> 
> Hi,

Hi Nik,

> In general the RCU usage needs more work.

Thanks for the detailed review, this is my first time when I use the RCU,
so I might need to spend more time on time.

> Also I might've missed it, but where do you
> handle bridge port delete which is used in mrp ?

When a port is deleted, then the userspace application will be notified
and then the userspace will remove the MRP instance. Because there is no
point to have a MRP instance with only 1 port. And the function that
delets the MRP instance is br_mrp_del.

> Also do you actually need the mrp->lock ?

I think I should be fine not to use mrp->lock because already the rtnl
lock is taken.

> 
> > diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
> > new file mode 100644
> > index 000000000000..f1de792d7a6e
> > --- /dev/null
> > +++ b/net/bridge/br_mrp.c
> > @@ -0,0 +1,514 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include "br_private_mrp.h"
> > +
> > +static const u8 mrp_test_dmac[ETH_ALEN] = { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 };
> > +
> > +static struct net_bridge_port *br_mrp_get_port(struct net_bridge *br,
> > +                                            u32 ifindex)
> > +{
> > +     struct net_bridge_port *res = NULL;
> > +     struct net_bridge_port *port;
> > +
> > +     spin_lock_bh(&br->lock);
> > +
> 
> This is called under RTNL, you don't need the br->lock.

Will be fix in the next patch series.
> 
> > +     list_for_each_entry(port, &br->port_list, list) {
> > +             if (port->dev->ifindex == ifindex) {
> > +                     res = port;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     spin_unlock_bh(&br->lock);
> > +
> > +     return res;
> > +}
> > +
> > +static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_id)
> > +{
> > +     struct br_mrp *res = NULL;
> > +     struct br_mrp *mrp;
> > +
> > +     rcu_read_lock();
> > +
> 
> This is generally a bad pattern because it can hide legitimate bugs and make
> it harder to debug.

Can you give me a little more details why is a bad pattern?
I have tried to read about rcu from here[1][2]. But I couldn't see
anything about this.

> 
> > +     list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
> > +             if (mrp->ring_id == ring_id) {
> > +                     res = mrp;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     rcu_read_unlock();
> > +
> > +     return res;
> > +}
> > +
> > +static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
> > +                                    struct net_bridge_port *p)
> > +{
> > +     struct br_mrp *res = NULL;
> > +     struct br_mrp *mrp;
> > +
> > +     rcu_read_lock();
> > +
> > +     list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
> > +             if (rcu_dereference(mrp->p_port) == p ||
> > +                 rcu_dereference(mrp->s_port) == p) {
> 
> rcu_access_pointer() is ok for comparisons

Will be fix in the next patch series.
> 
> > +                     res = mrp;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     rcu_read_unlock();
> > +
> > +     return res;
> > +}
> > +
> > +static int br_mrp_next_seq(struct br_mrp *mrp)
> > +{
> > +     mrp->seq_id++;
> > +     return mrp->seq_id;
> > +}
> > +
> > +static struct sk_buff *br_mrp_skb_alloc(struct net_bridge_port *p,
> > +                                     const u8 *src, const u8 *dst)
> > +{
> > +     struct ethhdr *eth_hdr;
> > +     struct sk_buff *skb;
> > +     u16 *version;
> > +
> > +     skb = dev_alloc_skb(MRP_MAX_FRAME_LENGTH);
> > +     if (!skb)
> > +             return NULL;
> > +
> > +     skb->dev = p->dev;
> > +     skb->protocol = htons(ETH_P_MRP);
> > +     skb->priority = MRP_FRAME_PRIO;
> > +     skb_reserve(skb, sizeof(*eth_hdr));
> > +
> > +     eth_hdr = skb_push(skb, sizeof(*eth_hdr));
> > +     ether_addr_copy(eth_hdr->h_dest, dst);
> > +     ether_addr_copy(eth_hdr->h_source, src);
> > +     eth_hdr->h_proto = htons(ETH_P_MRP);
> > +
> > +     version = skb_put(skb, sizeof(*version));
> > +     *version = cpu_to_be16(MRP_VERSION);
> > +
> > +     return skb;
> > +}
> > +
> > +static void br_mrp_skb_tlv(struct sk_buff *skb,
> > +                        enum br_mrp_tlv_header_type type,
> > +                        u8 length)
> > +{
> > +     struct br_mrp_tlv_hdr *hdr;
> > +
> > +     hdr = skb_put(skb, sizeof(*hdr));
> > +     hdr->type = type;
> > +     hdr->length = length;
> > +}
> > +
> > +static void br_mrp_skb_common(struct sk_buff *skb, struct br_mrp *mrp)
> > +{
> > +     struct br_mrp_common_hdr *hdr;
> > +
> > +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_COMMON, sizeof(*hdr));
> > +
> > +     hdr = skb_put(skb, sizeof(*hdr));
> > +     hdr->seq_id = cpu_to_be16(br_mrp_next_seq(mrp));
> > +     memset(hdr->domain, 0xff, MRP_DOMAIN_UUID_LENGTH);
> > +}
> > +
> > +static struct sk_buff *br_mrp_alloc_test_skb(struct br_mrp *mrp,
> > +                                          struct net_device *dev,
> > +                                          enum br_mrp_port_role_type port_role)
> > +{
> > +     struct net_bridge_port *p = br_port_get_rtnl(dev);
> > +     struct br_mrp_ring_test_hdr *hdr = NULL;
> > +     struct net_bridge *br = p->br;
> > +     struct sk_buff *skb = NULL;
> > +
> > +     if (!p)
> > +             return NULL;
> > +
> > +     br = p->br;
> > +
> > +     skb = br_mrp_skb_alloc(p, p->dev->dev_addr, mrp_test_dmac);
> > +     if (!skb)
> > +             return NULL;
> > +
> > +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_RING_TEST, sizeof(*hdr));
> > +     hdr = skb_put(skb, sizeof(*hdr));
> > +
> > +     hdr->prio = cpu_to_be16(MRP_DEFAULT_PRIO);
> > +     ether_addr_copy(hdr->sa, p->br->dev->dev_addr);
> > +     hdr->port_role = cpu_to_be16(port_role);
> > +     hdr->state = cpu_to_be16(mrp->ring_state);
> > +     hdr->transitions = cpu_to_be16(mrp->ring_transitions);
> > +     hdr->timestamp = cpu_to_be32(jiffies_to_msecs(jiffies));
> > +
> > +     br_mrp_skb_common(skb, mrp);
> > +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_END, 0x0);
> > +
> > +     return skb;
> > +}
> > +
> > +static void br_mrp_test_work_expired(struct work_struct *work)
> > +{
> > +     struct delayed_work *del_work = to_delayed_work(work);
> > +     struct br_mrp *mrp = container_of(del_work, struct br_mrp, test_work);
> > +     bool notify_open = false;
> > +     struct sk_buff *skb;
> > +
> 
> Since this runs asynchronously what happens if the port is deleted ?

Later I have checks to see if the port is no NULL. Is not good enough?
I have these rcu_access_pointer checks and before that I disable the
interrupts and get the rcu lock.

> 
> > +     if (time_before_eq(mrp->test_end, jiffies))
> > +             return;
> > +
> > +     if (mrp->test_count_miss < mrp->test_max_miss) {
> > +             mrp->test_count_miss++;
> > +     } else {
> > +             /* Notify that the ring is open only if the ring state is
> > +              * closed, otherwise it would continue to notify at every
> > +              * interval.
> > +              */
> > +             if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED)
> > +                     notify_open = true;
> > +     }
> > +
> > +     local_bh_disable();
> > +     rcu_read_lock();
> > +
> > +     if (!rcu_access_pointer(mrp->p_port) ||
> > +         !rcu_access_pointer(mrp->s_port))
> > +             goto out;
> > +
> > +     /* Is it possible here to get call to delete the bridge port? If yes
> > +      * I need to protect it
> > +      */
> > +     dev_hold(rcu_dereference(mrp->p_port)->dev);
> > +
> 
> This looks all wrong, p_port can become NULL here and you'll deref it.

By disabling the interrupts and taking the rcu read lock, will I not be
sure that no one can access the p_port?
If is not true, how the p_port can become NULL?

> 
> > +     skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->p_port)->dev,
> > +                                 BR_MRP_PORT_ROLE_PRIMARY);
> > +     if (!skb)
> > +             goto out;
> > +
> > +     skb_reset_network_header(skb);
> > +     dev_queue_xmit(skb);
> > +
> > +     if (notify_open && !mrp->ring_role_offloaded)
> > +             br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, true);
> > +
> > +     dev_put(rcu_dereference(mrp->p_port)->dev);
> > +
> > +     dev_hold(rcu_dereference(mrp->s_port)->dev);
> > +
> 
> same here
> 
> > +     skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->s_port)->dev,
> > +                                 BR_MRP_PORT_ROLE_SECONDARY);
> > +     if (!skb)
> > +             goto out;
> > +
> > +     skb_reset_network_header(skb);
> > +     dev_queue_xmit(skb);
> > +
> > +     if (notify_open && !mrp->ring_role_offloaded)
> > +             br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, true);
> > +
> > +     dev_put(rcu_dereference(mrp->s_port)->dev);
> > +
> > +out:
> > +     rcu_read_unlock();
> > +     local_bh_enable();
> > +
> > +     queue_delayed_work(system_wq, &mrp->test_work,
> > +                        usecs_to_jiffies(mrp->test_interval));
> > +}
> > +
> > +/* Adds a new MRP instance.
> > + * note: called under rtnl_lock
> > + */
> > +int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
> > +{
> > +     struct net_bridge_port *p;
> > +     struct br_mrp *mrp;
> > +
> > +     /* If the ring exists, it is not possible to create another one with the
> > +      * same ring_id
> > +      */
> > +     mrp = br_mrp_find_id(br, instance->ring_id);
> > +     if (mrp)
> > +             return -EINVAL;
> > +
> > +     if (!br_mrp_get_port(br, instance->p_ifindex) ||
> > +         !br_mrp_get_port(br, instance->s_ifindex))
> > +             return -EINVAL;
> > +
> > +     mrp = devm_kzalloc(&br->dev->dev, sizeof(struct br_mrp), GFP_KERNEL);
> > +     if (!mrp)
> > +             return -ENOMEM;
> > +
> > +     /* I think is not needed because this can be replaced with rtnl lock*/
> > +     spin_lock_init(&mrp->lock);
> > +     spin_lock(&mrp->lock);
> > +
> > +     mrp->br = br;
> 
> Is this field (mrp->br) used anywhere ?

Not anymore. I can remove it in the next patch series.

> 
> > +     mrp->ring_id = instance->ring_id;
> > +
> > +     p = br_mrp_get_port(br, instance->p_ifindex);
> > +     p->state = BR_STATE_FORWARDING;
> > +     p->flags |= BR_MRP_AWARE;
> > +     rcu_assign_pointer(mrp->p_port, p);
> > +
> > +     p = br_mrp_get_port(br, instance->s_ifindex);
> > +     p->state = BR_STATE_FORWARDING;
> > +     p->flags |= BR_MRP_AWARE;
> > +     rcu_assign_pointer(mrp->s_port, p);
> > +
> > +     br_mrp_switchdev_add(mrp);
> > +
> > +     spin_unlock(&mrp->lock);
> > +     synchronize_rcu();
> 
> Why do you need the synchronize here?

Actually this shouldn't be after the list_add_tail_rcu? Because I am
thinking that some can read the list at the same time I am change it.

> 
> > +
> > +     list_add_tail_rcu(&mrp->list, &br->mrp_list);
> > +     INIT_DELAYED_WORK(&mrp->test_work, br_mrp_test_work_expired);
> > +
> > +     return 0;
> > +}
> > +
> > +/* Deletes existing MRP instance.
> > + * note: called under rtnl_lock
> > + */
> > +int br_mrp_del(struct net_bridge *br, struct br_mrp_instance *instance)
> > +{
> > +     struct br_mrp *mrp = br_mrp_find_id(br, instance->ring_id);
> > +     struct net_bridge_port *p;
> > +
> > +     if (!mrp)
> > +             return -EINVAL;
> > +
> > +     /* Stop sending MRP_Test frames */
> > +     cancel_delayed_work(&mrp->test_work);
> 
> cancel_delayed_work_sync() if you'd like to make sure it's stopped and finished (if it was running
> during this)

Will be fixed in the next patch series.
> 
> > +     br_mrp_switchdev_send_ring_test(mrp, 0, 0, 0);
> > +
> > +     spin_lock(&mrp->lock);
> > +
> > +     br_mrp_switchdev_del(mrp);
> > +
> > +     /* Reset the ports */
> > +     p = rcu_dereference_protected(mrp->p_port, lockdep_is_held(&mrp->lock));
> > +     if (p) {
> > +             spin_lock(&br->lock);
> > +             p->state = BR_STATE_FORWARDING;
> > +             p->flags &= ~BR_MRP_AWARE;
> > +             br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
> > +             rcu_assign_pointer(mrp->p_port, NULL);
> > +             spin_unlock(&br->lock);
> > +     }
> > +
> > +     p = rcu_dereference_protected(mrp->s_port, lockdep_is_held(&mrp->lock));
> > +     if (p) {
> > +             spin_lock(&br->lock);
> > +             p->state = BR_STATE_FORWARDING;
> > +             p->flags &= ~BR_MRP_AWARE;
> > +             br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
> > +             rcu_assign_pointer(mrp->s_port, NULL);
> > +             spin_unlock(&br->lock);
> > +     }
> > +
> > +     /* Destroy the ring */
> > +     mrp->br = NULL;
> > +
> > +     spin_unlock(&mrp->lock);
> > +     synchronize_rcu();
> > +
> > +     list_del_rcu(&mrp->list);
> > +     devm_kfree(&br->dev->dev, mrp);
> > +
> > +     return 0;
> > +}
> > +
> > +int br_mrp_set_port_state(struct net_bridge_port *p,
> > +                       enum br_mrp_port_state_type state)
> > +{
> > +     spin_lock(&p->br->lock);
> > +
> > +     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);
> > +
> > +     spin_unlock(&p->br->lock);
> > +
> > +     return 0;
> > +}
> > +
> > +int br_mrp_set_port_role(struct net_bridge_port *p,
> > +                      u32 ring_id, enum br_mrp_port_role_type role)
> > +{
> > +     struct br_mrp *mrp = br_mrp_find_id(p->br, ring_id);
> > +
> > +     if (!mrp)
> > +             return -EINVAL;
> > +
> > +     spin_lock(&mrp->lock);
> > +
> > +     if (role == BR_MRP_PORT_ROLE_PRIMARY)
> > +             rcu_assign_pointer(mrp->p_port, p);
> > +     if (role == BR_MRP_PORT_ROLE_SECONDARY)
> > +             rcu_assign_pointer(mrp->s_port, p);
> > +
> > +     br_mrp_port_switchdev_set_role(p, role);
> > +
> > +     spin_unlock(&mrp->lock);
> > +     synchronize_rcu();
> 
> Why do you need to synchronize here?

Actually this is not needed.
> 
> > +
> > +     return 0;
> > +}
> > +
> > +int br_mrp_set_ring_state(struct net_bridge *br, u32 ring_id,
> > +                       enum br_mrp_ring_state_type state)
> > +{
> > +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
> > +
> > +     if (!mrp)
> > +             return -EINVAL;
> > +
> > +     if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED &&
> > +         state != BR_MRP_RING_STATE_CLOSED)
> > +             mrp->ring_transitions++;
> > +
> > +     mrp->ring_state = state;
> > +
> > +     br_mrp_switchdev_set_ring_state(mrp, state);
> > +
> > +     return 0;
> > +}
> > +
> > +int br_mrp_set_ring_role(struct net_bridge *br, u32 ring_id,
> > +                      enum br_mrp_ring_role_type role)
> > +{
> > +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
> > +     int err;
> > +
> > +     if (!mrp)
> > +             return -EINVAL;
> > +
> > +     mrp->ring_role = role;
> > +
> > +     /* If there is an error just bailed out */
> > +     err = br_mrp_switchdev_set_ring_role(mrp, role);
> > +     if (err && err != -EOPNOTSUPP)
> > +             return err;
> > +
> > +     /* Now detect if the HW actually applied the role or not. If the HW
> > +      * applied the role it means that the SW will not to do those operations
> > +      * anymore. For example if the role ir MRM then the HW will notify the
> > +      * SW when ring is open, but if the is not pushed to the HW the SW will
> > +      * need to detect when the ring is open
> > +      */
> > +     mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
> > +
> > +     return 0;
> > +}
> > +
> > +int br_mrp_start_test(struct net_bridge *br, u32 ring_id, u32 interval,
> > +                   u8 max_miss, u32 period)
> > +{
> > +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
> > +
> > +     if (!mrp)
> > +             return -EINVAL;
> > +
> > +     /* Try to push is to the HW and if it fails then continue to generate in
> > +      * SW and if that also fails then return error
> > +      */
> > +     if (!br_mrp_switchdev_send_ring_test(mrp, interval, max_miss, period))
> > +             return 0;
> > +
> > +     mrp->test_interval = interval;
> > +     mrp->test_end = jiffies + usecs_to_jiffies(period);
> > +     mrp->test_max_miss = max_miss;
> > +     mrp->test_count_miss = 0;
> > +     queue_delayed_work(system_wq, &mrp->test_work,
> > +                        usecs_to_jiffies(interval));
> > +
> > +     return 0;
> > +}
> > +
> > +/* Process only MRP Test frame. All the other MRP frames are processed by
> > + * userspace application
> > + * note: already called with rcu_read_lock
> > + */
> > +static void br_mrp_mrm_process(struct br_mrp *mrp, struct sk_buff *skb)
> > +{
> > +     struct br_mrp_tlv_hdr *hdr;
> > +
> > +     hdr = (struct br_mrp_tlv_hdr *)(skb->data + sizeof(uint16_t));
> > +
> > +     if (hdr->type != BR_MRP_TLV_HEADER_RING_TEST)
> > +             return;
> > +
> > +     mrp->test_count_miss = 0;
> > +
> > +     br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, false);
> > +     br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, false);
> > +}
> > +
> > +/* This will just forward the frame to the other mrp ring port(MRC role) or will
> > + * not do anything.
> > + * note: already called with rcu_read_lock
> > + */
> > +static int br_mrp_rcv(struct net_bridge_port *p,
> > +                   struct sk_buff *skb, struct net_device *dev)
> > +{
> > +     struct net_device *s_dev, *p_dev, *d_dev;
> > +     struct net_bridge *br;
> > +     struct sk_buff *nskb;
> > +     struct br_mrp *mrp;
> > +
> > +     /* If port is disable don't accept any frames */
> > +     if (p->state == BR_STATE_DISABLED)
> > +             return 0;
> > +
> > +     br = p->br;
> > +     mrp =  br_mrp_find_port(br, p);
> > +     if (unlikely(!mrp))
> > +             return 0;
> > +
> > +     /* If the role is MRM then don't forward the frames */
> > +     if (mrp->ring_role == BR_MRP_RING_ROLE_MRM) {
> > +             br_mrp_mrm_process(mrp, skb);
> > +             return 1;
> > +     }
> > +
> > +     nskb = skb_clone(skb, GFP_ATOMIC);
> > +     if (!nskb)
> > +             return 0;
> > +
> > +     p_dev = rcu_dereference(mrp->p_port)->dev;
> > +     s_dev = rcu_dereference(mrp->s_port)->dev;
> > +
> 
> Not safe, could deref null.

Will be fixed in the next patch series.

> 
> > +     if (p_dev == dev)
> > +             d_dev = s_dev;
> > +     else
> > +             d_dev = p_dev;
> > +
> > +     nskb->dev = d_dev;
> > +     skb_push(nskb, ETH_HLEN);
> > +     dev_queue_xmit(nskb);
> > +
> > +     return 1;
> > +}
> > +
> > +int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
> > +{
> > +     /* If there is no MRP instance do normal forwarding */
> > +     if (unlikely(!(p->flags & BR_MRP_AWARE)))
> 
> Shouldn't this one be likely() ?

Yes, this should be likely.
> 
> > +             goto out;
> > +
> > +     if (unlikely(skb->protocol == htons(ETH_P_MRP)))
> > +             return br_mrp_rcv(p, skb, p->dev);
> > +
> > +out:
> > +     return 0;
> > +}
> >
> 

[1] https://lwn.net/Articles/262464/
[2] https://www.kernel.org/doc/html/latest/RCU/listRCU.html

-- 
/Horatiu

Re: [RFC net-next v4 7/9] bridge: mrp: Connect MRP api with the switchev API

[Nikolay Aleksandrov]

On 01/04/2020 19:06, Horatiu Vultur wrote:
> The 03/30/2020 19:11, Nikolay Aleksandrov wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 27/03/2020 11:21, Horatiu Vultur wrote:
>>> Implement the MRP api.
>>>
>>> In case the HW can't generate MRP Test frames then the SW will try to generate
>>> the frames. In case that also the SW will fail in generating the frames then a
>>> error is return to the userspace. The userspace is responsible to generate all
>>> the other MRP frames regardless if the test frames are generated by HW or SW.
>>>
>>> The forwarding/termination of MRP frames is happening in the kernel and is done
>>> by the MRP instance. The userspace application doesn't do the forwarding.
>>>
>>> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
>>> ---
>>>  net/bridge/br_mrp.c | 514 ++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 514 insertions(+)
>>>  create mode 100644 net/bridge/br_mrp.c
>>>
>>
>> Hi,
> 
> Hi Nik,
> 
>> In general the RCU usage needs more work.
> 
> Thanks for the detailed review, this is my first time when I use the RCU,
> so I might need to spend more time on time.
> 
>> Also I might've missed it, but where do you
>> handle bridge port delete which is used in mrp ?
> 
> When a port is deleted, then the userspace application will be notified
> and then the userspace will remove the MRP instance. Because there is no
> point to have a MRP instance with only 1 port. And the function that
> delets the MRP instance is br_mrp_del.
> 

How would you execute br_mrp_del() if the port is already deleted from the bridge ?
Nothing prevents the port to disappear and then you lose all bridge callbacks.

>> Also do you actually need the mrp->lock ?
> 
> I think I should be fine not to use mrp->lock because already the rtnl
> lock is taken.
> >>
>>> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
>>> new file mode 100644
>>> index 000000000000..f1de792d7a6e
>>> --- /dev/null
>>> +++ b/net/bridge/br_mrp.c
>>> @@ -0,0 +1,514 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +
>>> +#include "br_private_mrp.h"
>>> +
>>> +static const u8 mrp_test_dmac[ETH_ALEN] = { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 };
>>> +
>>> +static struct net_bridge_port *br_mrp_get_port(struct net_bridge *br,
>>> +                                            u32 ifindex)
>>> +{
>>> +     struct net_bridge_port *res = NULL;
>>> +     struct net_bridge_port *port;
>>> +
>>> +     spin_lock_bh(&br->lock);
>>> +
>>
>> This is called under RTNL, you don't need the br->lock.
> 
> Will be fix in the next patch series.
>>
>>> +     list_for_each_entry(port, &br->port_list, list) {
>>> +             if (port->dev->ifindex == ifindex) {
>>> +                     res = port;
>>> +                     break;
>>> +             }
>>> +     }
>>> +
>>> +     spin_unlock_bh(&br->lock);
>>> +
>>> +     return res;
>>> +}
>>> +
>>> +static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_id)
>>> +{
>>> +     struct br_mrp *res = NULL;
>>> +     struct br_mrp *mrp;
>>> +
>>> +     rcu_read_lock();
>>> +
>>
>> This is generally a bad pattern because it can hide legitimate bugs and make
>> it harder to debug.
> 
> Can you give me a little more details why is a bad pattern?
> I have tried to read about rcu from here[1][2]. But I couldn't see
> anything about this.
> 

In general you should know the context the function is used in, you cannot use the
pointer obtained from this search after the rcu_read_unlock(). If this function is
ever used in context which doesn't have rcu read lock or the writer lock then you'll
mask the bug here. If you know it is always called from RCU context then just drop
these, if not then add the proper lockdep annotations so they can be checked.

>>
>>> +     list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
>>> +             if (mrp->ring_id == ring_id) {
>>> +                     res = mrp;
>>> +                     break;
>>> +             }
>>> +     }
>>> +
>>> +     rcu_read_unlock();
>>> +
>>> +     return res;
>>> +}
>>> +
>>> +static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
>>> +                                    struct net_bridge_port *p)
>>> +{
>>> +     struct br_mrp *res = NULL;
>>> +     struct br_mrp *mrp;
>>> +
>>> +     rcu_read_lock();
>>> +
>>> +     list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
>>> +             if (rcu_dereference(mrp->p_port) == p ||
>>> +                 rcu_dereference(mrp->s_port) == p) {
>>
>> rcu_access_pointer() is ok for comparisons
> 
> Will be fix in the next patch series.
>>
>>> +                     res = mrp;
>>> +                     break;
>>> +             }
>>> +     }
>>> +
>>> +     rcu_read_unlock();
>>> +
>>> +     return res;
>>> +}
>>> +
>>> +static int br_mrp_next_seq(struct br_mrp *mrp)
>>> +{
>>> +     mrp->seq_id++;
>>> +     return mrp->seq_id;
>>> +}
>>> +
>>> +static struct sk_buff *br_mrp_skb_alloc(struct net_bridge_port *p,
>>> +                                     const u8 *src, const u8 *dst)
>>> +{
>>> +     struct ethhdr *eth_hdr;
>>> +     struct sk_buff *skb;
>>> +     u16 *version;
>>> +
>>> +     skb = dev_alloc_skb(MRP_MAX_FRAME_LENGTH);
>>> +     if (!skb)
>>> +             return NULL;
>>> +
>>> +     skb->dev = p->dev;
>>> +     skb->protocol = htons(ETH_P_MRP);
>>> +     skb->priority = MRP_FRAME_PRIO;
>>> +     skb_reserve(skb, sizeof(*eth_hdr));
>>> +
>>> +     eth_hdr = skb_push(skb, sizeof(*eth_hdr));
>>> +     ether_addr_copy(eth_hdr->h_dest, dst);
>>> +     ether_addr_copy(eth_hdr->h_source, src);
>>> +     eth_hdr->h_proto = htons(ETH_P_MRP);
>>> +
>>> +     version = skb_put(skb, sizeof(*version));
>>> +     *version = cpu_to_be16(MRP_VERSION);
>>> +
>>> +     return skb;
>>> +}
>>> +
>>> +static void br_mrp_skb_tlv(struct sk_buff *skb,
>>> +                        enum br_mrp_tlv_header_type type,
>>> +                        u8 length)
>>> +{
>>> +     struct br_mrp_tlv_hdr *hdr;
>>> +
>>> +     hdr = skb_put(skb, sizeof(*hdr));
>>> +     hdr->type = type;
>>> +     hdr->length = length;
>>> +}
>>> +
>>> +static void br_mrp_skb_common(struct sk_buff *skb, struct br_mrp *mrp)
>>> +{
>>> +     struct br_mrp_common_hdr *hdr;
>>> +
>>> +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_COMMON, sizeof(*hdr));
>>> +
>>> +     hdr = skb_put(skb, sizeof(*hdr));
>>> +     hdr->seq_id = cpu_to_be16(br_mrp_next_seq(mrp));
>>> +     memset(hdr->domain, 0xff, MRP_DOMAIN_UUID_LENGTH);
>>> +}
>>> +
>>> +static struct sk_buff *br_mrp_alloc_test_skb(struct br_mrp *mrp,
>>> +                                          struct net_device *dev,
>>> +                                          enum br_mrp_port_role_type port_role)
>>> +{
>>> +     struct net_bridge_port *p = br_port_get_rtnl(dev);
>>> +     struct br_mrp_ring_test_hdr *hdr = NULL;
>>> +     struct net_bridge *br = p->br;
>>> +     struct sk_buff *skb = NULL;
>>> +
>>> +     if (!p)
>>> +             return NULL;
>>> +
>>> +     br = p->br;
>>> +
>>> +     skb = br_mrp_skb_alloc(p, p->dev->dev_addr, mrp_test_dmac);
>>> +     if (!skb)
>>> +             return NULL;
>>> +
>>> +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_RING_TEST, sizeof(*hdr));
>>> +     hdr = skb_put(skb, sizeof(*hdr));
>>> +
>>> +     hdr->prio = cpu_to_be16(MRP_DEFAULT_PRIO);
>>> +     ether_addr_copy(hdr->sa, p->br->dev->dev_addr);
>>> +     hdr->port_role = cpu_to_be16(port_role);
>>> +     hdr->state = cpu_to_be16(mrp->ring_state);
>>> +     hdr->transitions = cpu_to_be16(mrp->ring_transitions);
>>> +     hdr->timestamp = cpu_to_be32(jiffies_to_msecs(jiffies));
>>> +
>>> +     br_mrp_skb_common(skb, mrp);
>>> +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_END, 0x0);
>>> +
>>> +     return skb;
>>> +}
>>> +
>>> +static void br_mrp_test_work_expired(struct work_struct *work)
>>> +{
>>> +     struct delayed_work *del_work = to_delayed_work(work);
>>> +     struct br_mrp *mrp = container_of(del_work, struct br_mrp, test_work);
>>> +     bool notify_open = false;
>>> +     struct sk_buff *skb;
>>> +
>>
>> Since this runs asynchronously what happens if the port is deleted ?
> 
> Later I have checks to see if the port is no NULL. Is not good enough?
> I have these rcu_access_pointer checks and before that I disable the
> interrupts and get the rcu lock.
> 

That is not safe because you dereference the pointer again after the check
and it may become NULL between those. You could do ptr = rcu_dereference();
if (!ptr) and if non-null then continue accessing that memory through ptr. 

>>
>>> +     if (time_before_eq(mrp->test_end, jiffies))
>>> +             return;
>>> +
>>> +     if (mrp->test_count_miss < mrp->test_max_miss) {
>>> +             mrp->test_count_miss++;
>>> +     } else {
>>> +             /* Notify that the ring is open only if the ring state is
>>> +              * closed, otherwise it would continue to notify at every
>>> +              * interval.
>>> +              */
>>> +             if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED)
>>> +                     notify_open = true;
>>> +     }
>>> +
>>> +     local_bh_disable();
>>> +     rcu_read_lock();
>>> +
>>> +     if (!rcu_access_pointer(mrp->p_port) ||
>>> +         !rcu_access_pointer(mrp->s_port))
>>> +             goto out;
>>> +
>>> +     /* Is it possible here to get call to delete the bridge port? If yes
>>> +      * I need to protect it
>>> +      */
>>> +     dev_hold(rcu_dereference(mrp->p_port)->dev);
>>> +
>>
>> This looks all wrong, p_port can become NULL here and you'll deref it.
> 
> By disabling the interrupts and taking the rcu read lock, will I not be
> sure that no one can access the p_port?

No. You should read more about how RCU operates.

> If is not true, how the p_port can become NULL?
> 

Readers and writer run concurrently.

>>
>>> +     skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->p_port)->dev,
>>> +                                 BR_MRP_PORT_ROLE_PRIMARY);
>>> +     if (!skb)
>>> +             goto out;
>>> +
>>> +     skb_reset_network_header(skb);
>>> +     dev_queue_xmit(skb);
>>> +
>>> +     if (notify_open && !mrp->ring_role_offloaded)
>>> +             br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, true);
>>> +
>>> +     dev_put(rcu_dereference(mrp->p_port)->dev);
>>> +
>>> +     dev_hold(rcu_dereference(mrp->s_port)->dev);
>>> +
>>
>> same here
>>
>>> +     skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->s_port)->dev,
>>> +                                 BR_MRP_PORT_ROLE_SECONDARY);
>>> +     if (!skb)
>>> +             goto out;
>>> +
>>> +     skb_reset_network_header(skb);
>>> +     dev_queue_xmit(skb);
>>> +
>>> +     if (notify_open && !mrp->ring_role_offloaded)
>>> +             br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, true);
>>> +
>>> +     dev_put(rcu_dereference(mrp->s_port)->dev);
>>> +
>>> +out:
>>> +     rcu_read_unlock();
>>> +     local_bh_enable();
>>> +
>>> +     queue_delayed_work(system_wq, &mrp->test_work,
>>> +                        usecs_to_jiffies(mrp->test_interval));
>>> +}
>>> +
>>> +/* Adds a new MRP instance.
>>> + * note: called under rtnl_lock
>>> + */
>>> +int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
>>> +{
>>> +     struct net_bridge_port *p;
>>> +     struct br_mrp *mrp;
>>> +
>>> +     /* If the ring exists, it is not possible to create another one with the
>>> +      * same ring_id
>>> +      */
>>> +     mrp = br_mrp_find_id(br, instance->ring_id);
>>> +     if (mrp)
>>> +             return -EINVAL;
>>> +
>>> +     if (!br_mrp_get_port(br, instance->p_ifindex) ||
>>> +         !br_mrp_get_port(br, instance->s_ifindex))
>>> +             return -EINVAL;
>>> +
>>> +     mrp = devm_kzalloc(&br->dev->dev, sizeof(struct br_mrp), GFP_KERNEL);
>>> +     if (!mrp)
>>> +             return -ENOMEM;
>>> +
>>> +     /* I think is not needed because this can be replaced with rtnl lock*/
>>> +     spin_lock_init(&mrp->lock);
>>> +     spin_lock(&mrp->lock);
>>> +
>>> +     mrp->br = br;
>>
>> Is this field (mrp->br) used anywhere ?
> 
> Not anymore. I can remove it in the next patch series.
> 
>>
>>> +     mrp->ring_id = instance->ring_id;
>>> +
>>> +     p = br_mrp_get_port(br, instance->p_ifindex);
>>> +     p->state = BR_STATE_FORWARDING;
>>> +     p->flags |= BR_MRP_AWARE;
>>> +     rcu_assign_pointer(mrp->p_port, p);
>>> +
>>> +     p = br_mrp_get_port(br, instance->s_ifindex);
>>> +     p->state = BR_STATE_FORWARDING;
>>> +     p->flags |= BR_MRP_AWARE;
>>> +     rcu_assign_pointer(mrp->s_port, p);
>>> +
>>> +     br_mrp_switchdev_add(mrp);
>>> +
>>> +     spin_unlock(&mrp->lock);
>>> +     synchronize_rcu();
>>
>> Why do you need the synchronize here?
> 
> Actually this shouldn't be after the list_add_tail_rcu? Because I am
> thinking that some can read the list at the same time I am change it.

That doesn't help, rcu primitives are already safe to run concurrently with readers.

> 
>>
>>> +
>>> +     list_add_tail_rcu(&mrp->list, &br->mrp_list);
>>> +     INIT_DELAYED_WORK(&mrp->test_work, br_mrp_test_work_expired);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +/* Deletes existing MRP instance.
>>> + * note: called under rtnl_lock
>>> + */
>>> +int br_mrp_del(struct net_bridge *br, struct br_mrp_instance *instance)
>>> +{
>>> +     struct br_mrp *mrp = br_mrp_find_id(br, instance->ring_id);
>>> +     struct net_bridge_port *p;
>>> +
>>> +     if (!mrp)
>>> +             return -EINVAL;
>>> +
>>> +     /* Stop sending MRP_Test frames */
>>> +     cancel_delayed_work(&mrp->test_work);
>>
>> cancel_delayed_work_sync() if you'd like to make sure it's stopped and finished (if it was running
>> during this)
> 
> Will be fixed in the next patch series.
>>
>>> +     br_mrp_switchdev_send_ring_test(mrp, 0, 0, 0);
>>> +
>>> +     spin_lock(&mrp->lock);
>>> +
>>> +     br_mrp_switchdev_del(mrp);
>>> +
>>> +     /* Reset the ports */
>>> +     p = rcu_dereference_protected(mrp->p_port, lockdep_is_held(&mrp->lock));
>>> +     if (p) {
>>> +             spin_lock(&br->lock);
>>> +             p->state = BR_STATE_FORWARDING;
>>> +             p->flags &= ~BR_MRP_AWARE;
>>> +             br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
>>> +             rcu_assign_pointer(mrp->p_port, NULL);
>>> +             spin_unlock(&br->lock);
>>> +     }
>>> +
>>> +     p = rcu_dereference_protected(mrp->s_port, lockdep_is_held(&mrp->lock));
>>> +     if (p) {
>>> +             spin_lock(&br->lock);
>>> +             p->state = BR_STATE_FORWARDING;
>>> +             p->flags &= ~BR_MRP_AWARE;
>>> +             br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
>>> +             rcu_assign_pointer(mrp->s_port, NULL);
>>> +             spin_unlock(&br->lock);
>>> +     }
>>> +
>>> +     /* Destroy the ring */
>>> +     mrp->br = NULL;
>>> +
>>> +     spin_unlock(&mrp->lock);
>>> +     synchronize_rcu();
>>> +
>>> +     list_del_rcu(&mrp->list);
>>> +     devm_kfree(&br->dev->dev, mrp);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +int br_mrp_set_port_state(struct net_bridge_port *p,
>>> +                       enum br_mrp_port_state_type state)
>>> +{
>>> +     spin_lock(&p->br->lock);
>>> +
>>> +     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);
>>> +
>>> +     spin_unlock(&p->br->lock);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +int br_mrp_set_port_role(struct net_bridge_port *p,
>>> +                      u32 ring_id, enum br_mrp_port_role_type role)
>>> +{
>>> +     struct br_mrp *mrp = br_mrp_find_id(p->br, ring_id);
>>> +
>>> +     if (!mrp)
>>> +             return -EINVAL;
>>> +
>>> +     spin_lock(&mrp->lock);
>>> +
>>> +     if (role == BR_MRP_PORT_ROLE_PRIMARY)
>>> +             rcu_assign_pointer(mrp->p_port, p);
>>> +     if (role == BR_MRP_PORT_ROLE_SECONDARY)
>>> +             rcu_assign_pointer(mrp->s_port, p);
>>> +
>>> +     br_mrp_port_switchdev_set_role(p, role);
>>> +
>>> +     spin_unlock(&mrp->lock);
>>> +     synchronize_rcu();
>>
>> Why do you need to synchronize here?
> 
> Actually this is not needed.
>>
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +int br_mrp_set_ring_state(struct net_bridge *br, u32 ring_id,
>>> +                       enum br_mrp_ring_state_type state)
>>> +{
>>> +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
>>> +
>>> +     if (!mrp)
>>> +             return -EINVAL;
>>> +
>>> +     if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED &&
>>> +         state != BR_MRP_RING_STATE_CLOSED)
>>> +             mrp->ring_transitions++;
>>> +
>>> +     mrp->ring_state = state;
>>> +
>>> +     br_mrp_switchdev_set_ring_state(mrp, state);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +int br_mrp_set_ring_role(struct net_bridge *br, u32 ring_id,
>>> +                      enum br_mrp_ring_role_type role)
>>> +{
>>> +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
>>> +     int err;
>>> +
>>> +     if (!mrp)
>>> +             return -EINVAL;
>>> +
>>> +     mrp->ring_role = role;
>>> +
>>> +     /* If there is an error just bailed out */
>>> +     err = br_mrp_switchdev_set_ring_role(mrp, role);
>>> +     if (err && err != -EOPNOTSUPP)
>>> +             return err;
>>> +
>>> +     /* Now detect if the HW actually applied the role or not. If the HW
>>> +      * applied the role it means that the SW will not to do those operations
>>> +      * anymore. For example if the role ir MRM then the HW will notify the
>>> +      * SW when ring is open, but if the is not pushed to the HW the SW will
>>> +      * need to detect when the ring is open
>>> +      */
>>> +     mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +int br_mrp_start_test(struct net_bridge *br, u32 ring_id, u32 interval,
>>> +                   u8 max_miss, u32 period)
>>> +{
>>> +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
>>> +
>>> +     if (!mrp)
>>> +             return -EINVAL;
>>> +
>>> +     /* Try to push is to the HW and if it fails then continue to generate in
>>> +      * SW and if that also fails then return error
>>> +      */
>>> +     if (!br_mrp_switchdev_send_ring_test(mrp, interval, max_miss, period))
>>> +             return 0;
>>> +
>>> +     mrp->test_interval = interval;
>>> +     mrp->test_end = jiffies + usecs_to_jiffies(period);
>>> +     mrp->test_max_miss = max_miss;
>>> +     mrp->test_count_miss = 0;
>>> +     queue_delayed_work(system_wq, &mrp->test_work,
>>> +                        usecs_to_jiffies(interval));
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +/* Process only MRP Test frame. All the other MRP frames are processed by
>>> + * userspace application
>>> + * note: already called with rcu_read_lock
>>> + */
>>> +static void br_mrp_mrm_process(struct br_mrp *mrp, struct sk_buff *skb)
>>> +{
>>> +     struct br_mrp_tlv_hdr *hdr;
>>> +
>>> +     hdr = (struct br_mrp_tlv_hdr *)(skb->data + sizeof(uint16_t));
>>> +
>>> +     if (hdr->type != BR_MRP_TLV_HEADER_RING_TEST)
>>> +             return;
>>> +
>>> +     mrp->test_count_miss = 0;
>>> +
>>> +     br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, false);
>>> +     br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, false);
>>> +}
>>> +
>>> +/* This will just forward the frame to the other mrp ring port(MRC role) or will
>>> + * not do anything.
>>> + * note: already called with rcu_read_lock
>>> + */
>>> +static int br_mrp_rcv(struct net_bridge_port *p,
>>> +                   struct sk_buff *skb, struct net_device *dev)
>>> +{
>>> +     struct net_device *s_dev, *p_dev, *d_dev;
>>> +     struct net_bridge *br;
>>> +     struct sk_buff *nskb;
>>> +     struct br_mrp *mrp;
>>> +
>>> +     /* If port is disable don't accept any frames */
>>> +     if (p->state == BR_STATE_DISABLED)
>>> +             return 0;
>>> +
>>> +     br = p->br;
>>> +     mrp =  br_mrp_find_port(br, p);
>>> +     if (unlikely(!mrp))
>>> +             return 0;
>>> +
>>> +     /* If the role is MRM then don't forward the frames */
>>> +     if (mrp->ring_role == BR_MRP_RING_ROLE_MRM) {
>>> +             br_mrp_mrm_process(mrp, skb);
>>> +             return 1;
>>> +     }
>>> +
>>> +     nskb = skb_clone(skb, GFP_ATOMIC);
>>> +     if (!nskb)
>>> +             return 0;
>>> +
>>> +     p_dev = rcu_dereference(mrp->p_port)->dev;
>>> +     s_dev = rcu_dereference(mrp->s_port)->dev;
>>> +
>>
>> Not safe, could deref null.
> 
> Will be fixed in the next patch series.
> 
>>
>>> +     if (p_dev == dev)
>>> +             d_dev = s_dev;
>>> +     else
>>> +             d_dev = p_dev;
>>> +
>>> +     nskb->dev = d_dev;
>>> +     skb_push(nskb, ETH_HLEN);
>>> +     dev_queue_xmit(nskb);
>>> +
>>> +     return 1;
>>> +}
>>> +
>>> +int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
>>> +{
>>> +     /* If there is no MRP instance do normal forwarding */
>>> +     if (unlikely(!(p->flags & BR_MRP_AWARE)))
>>
>> Shouldn't this one be likely() ?
> 
> Yes, this should be likely.
>>
>>> +             goto out;
>>> +
>>> +     if (unlikely(skb->protocol == htons(ETH_P_MRP)))
>>> +             return br_mrp_rcv(p, skb, p->dev);
>>> +
>>> +out:
>>> +     return 0;
>>> +}
>>>
>>
> 
> [1] https://lwn.net/Articles/262464/
> [2] https://www.kernel.org/doc/html/latest/RCU/listRCU.html
> 

Re: [RFC net-next v4 7/9] bridge: mrp: Connect MRP api with the switchev API

[Horatiu Vultur]

The 04/01/2020 19:32, Nikolay Aleksandrov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 01/04/2020 19:06, Horatiu Vultur wrote:
> > The 03/30/2020 19:11, Nikolay Aleksandrov wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>
> >> On 27/03/2020 11:21, Horatiu Vultur wrote:
> >>> Implement the MRP api.
> >>>
> >>> In case the HW can't generate MRP Test frames then the SW will try to generate
> >>> the frames. In case that also the SW will fail in generating the frames then a
> >>> error is return to the userspace. The userspace is responsible to generate all
> >>> the other MRP frames regardless if the test frames are generated by HW or SW.
> >>>
> >>> The forwarding/termination of MRP frames is happening in the kernel and is done
> >>> by the MRP instance. The userspace application doesn't do the forwarding.
> >>>
> >>> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> >>> ---
> >>>  net/bridge/br_mrp.c | 514 ++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 514 insertions(+)
> >>>  create mode 100644 net/bridge/br_mrp.c
> >>>
> >>
> >> Hi,
> >
> > Hi Nik,
> >
> >> In general the RCU usage needs more work.
> >
> > Thanks for the detailed review, this is my first time when I use the RCU,
> > so I might need to spend more time on time.
> >
> >> Also I might've missed it, but where do you
> >> handle bridge port delete which is used in mrp ?
> >
> > When a port is deleted, then the userspace application will be notified
> > and then the userspace will remove the MRP instance. Because there is no
> > point to have a MRP instance with only 1 port. And the function that
> > delets the MRP instance is br_mrp_del.
> >
> 
> How would you execute br_mrp_del() if the port is already deleted from the bridge ?
> Nothing prevents the port to disappear and then you lose all bridge callbacks.

The br_mrp_del() is called on the bridge interface and not on the port.
The flow as I see it: the port is deleted from the bridge, the userspace
will be notified by this, the userspace will determine the bridge on
which was the port and then the userspace will make netlink call on the
bridge interface. In this way it would not loose the callback when the
port is deleted from the bridge.
The question is when the bridge is deleted, I can see that first it
deletes the ports and then it would delete the bridge. In this case I am
loosing the callbacks.
Should the function br_dev_delete be exteded for this?

> 
> >> Also do you actually need the mrp->lock ?
> >
> > I think I should be fine not to use mrp->lock because already the rtnl
> > lock is taken.
> > >>
> >>> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
> >>> new file mode 100644
> >>> index 000000000000..f1de792d7a6e
> >>> --- /dev/null
> >>> +++ b/net/bridge/br_mrp.c
> >>> @@ -0,0 +1,514 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-or-later
> >>> +
> >>> +#include "br_private_mrp.h"
> >>> +
> >>> +static const u8 mrp_test_dmac[ETH_ALEN] = { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 };
> >>> +
> >>> +static struct net_bridge_port *br_mrp_get_port(struct net_bridge *br,
> >>> +                                            u32 ifindex)
> >>> +{
> >>> +     struct net_bridge_port *res = NULL;
> >>> +     struct net_bridge_port *port;
> >>> +
> >>> +     spin_lock_bh(&br->lock);
> >>> +
> >>
> >> This is called under RTNL, you don't need the br->lock.
> >
> > Will be fix in the next patch series.
> >>
> >>> +     list_for_each_entry(port, &br->port_list, list) {
> >>> +             if (port->dev->ifindex == ifindex) {
> >>> +                     res = port;
> >>> +                     break;
> >>> +             }
> >>> +     }
> >>> +
> >>> +     spin_unlock_bh(&br->lock);
> >>> +
> >>> +     return res;
> >>> +}
> >>> +
> >>> +static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_id)
> >>> +{
> >>> +     struct br_mrp *res = NULL;
> >>> +     struct br_mrp *mrp;
> >>> +
> >>> +     rcu_read_lock();
> >>> +
> >>
> >> This is generally a bad pattern because it can hide legitimate bugs and make
> >> it harder to debug.
> >
> > Can you give me a little more details why is a bad pattern?
> > I have tried to read about rcu from here[1][2]. But I couldn't see
> > anything about this.
> >
> 
> In general you should know the context the function is used in, you cannot use the
> pointer obtained from this search after the rcu_read_unlock(). If this function is
> ever used in context which doesn't have rcu read lock or the writer lock then you'll
> mask the bug here. If you know it is always called from RCU context then just drop
> these, if not then add the proper lockdep annotations so they can be checked.

Thanks for the details. I will fix it in the next patch series.
> 
> >>
> >>> +     list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
> >>> +             if (mrp->ring_id == ring_id) {
> >>> +                     res = mrp;
> >>> +                     break;
> >>> +             }
> >>> +     }
> >>> +
> >>> +     rcu_read_unlock();
> >>> +
> >>> +     return res;
> >>> +}
> >>> +
> >>> +static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
> >>> +                                    struct net_bridge_port *p)
> >>> +{
> >>> +     struct br_mrp *res = NULL;
> >>> +     struct br_mrp *mrp;
> >>> +
> >>> +     rcu_read_lock();
> >>> +
> >>> +     list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
> >>> +             if (rcu_dereference(mrp->p_port) == p ||
> >>> +                 rcu_dereference(mrp->s_port) == p) {
> >>
> >> rcu_access_pointer() is ok for comparisons
> >
> > Will be fix in the next patch series.
> >>
> >>> +                     res = mrp;
> >>> +                     break;
> >>> +             }
> >>> +     }
> >>> +
> >>> +     rcu_read_unlock();
> >>> +
> >>> +     return res;
> >>> +}
> >>> +
> >>> +static int br_mrp_next_seq(struct br_mrp *mrp)
> >>> +{
> >>> +     mrp->seq_id++;
> >>> +     return mrp->seq_id;
> >>> +}
> >>> +
> >>> +static struct sk_buff *br_mrp_skb_alloc(struct net_bridge_port *p,
> >>> +                                     const u8 *src, const u8 *dst)
> >>> +{
> >>> +     struct ethhdr *eth_hdr;
> >>> +     struct sk_buff *skb;
> >>> +     u16 *version;
> >>> +
> >>> +     skb = dev_alloc_skb(MRP_MAX_FRAME_LENGTH);
> >>> +     if (!skb)
> >>> +             return NULL;
> >>> +
> >>> +     skb->dev = p->dev;
> >>> +     skb->protocol = htons(ETH_P_MRP);
> >>> +     skb->priority = MRP_FRAME_PRIO;
> >>> +     skb_reserve(skb, sizeof(*eth_hdr));
> >>> +
> >>> +     eth_hdr = skb_push(skb, sizeof(*eth_hdr));
> >>> +     ether_addr_copy(eth_hdr->h_dest, dst);
> >>> +     ether_addr_copy(eth_hdr->h_source, src);
> >>> +     eth_hdr->h_proto = htons(ETH_P_MRP);
> >>> +
> >>> +     version = skb_put(skb, sizeof(*version));
> >>> +     *version = cpu_to_be16(MRP_VERSION);
> >>> +
> >>> +     return skb;
> >>> +}
> >>> +
> >>> +static void br_mrp_skb_tlv(struct sk_buff *skb,
> >>> +                        enum br_mrp_tlv_header_type type,
> >>> +                        u8 length)
> >>> +{
> >>> +     struct br_mrp_tlv_hdr *hdr;
> >>> +
> >>> +     hdr = skb_put(skb, sizeof(*hdr));
> >>> +     hdr->type = type;
> >>> +     hdr->length = length;
> >>> +}
> >>> +
> >>> +static void br_mrp_skb_common(struct sk_buff *skb, struct br_mrp *mrp)
> >>> +{
> >>> +     struct br_mrp_common_hdr *hdr;
> >>> +
> >>> +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_COMMON, sizeof(*hdr));
> >>> +
> >>> +     hdr = skb_put(skb, sizeof(*hdr));
> >>> +     hdr->seq_id = cpu_to_be16(br_mrp_next_seq(mrp));
> >>> +     memset(hdr->domain, 0xff, MRP_DOMAIN_UUID_LENGTH);
> >>> +}
> >>> +
> >>> +static struct sk_buff *br_mrp_alloc_test_skb(struct br_mrp *mrp,
> >>> +                                          struct net_device *dev,
> >>> +                                          enum br_mrp_port_role_type port_role)
> >>> +{
> >>> +     struct net_bridge_port *p = br_port_get_rtnl(dev);
> >>> +     struct br_mrp_ring_test_hdr *hdr = NULL;
> >>> +     struct net_bridge *br = p->br;
> >>> +     struct sk_buff *skb = NULL;
> >>> +
> >>> +     if (!p)
> >>> +             return NULL;
> >>> +
> >>> +     br = p->br;
> >>> +
> >>> +     skb = br_mrp_skb_alloc(p, p->dev->dev_addr, mrp_test_dmac);
> >>> +     if (!skb)
> >>> +             return NULL;
> >>> +
> >>> +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_RING_TEST, sizeof(*hdr));
> >>> +     hdr = skb_put(skb, sizeof(*hdr));
> >>> +
> >>> +     hdr->prio = cpu_to_be16(MRP_DEFAULT_PRIO);
> >>> +     ether_addr_copy(hdr->sa, p->br->dev->dev_addr);
> >>> +     hdr->port_role = cpu_to_be16(port_role);
> >>> +     hdr->state = cpu_to_be16(mrp->ring_state);
> >>> +     hdr->transitions = cpu_to_be16(mrp->ring_transitions);
> >>> +     hdr->timestamp = cpu_to_be32(jiffies_to_msecs(jiffies));
> >>> +
> >>> +     br_mrp_skb_common(skb, mrp);
> >>> +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_END, 0x0);
> >>> +
> >>> +     return skb;
> >>> +}
> >>> +
> >>> +static void br_mrp_test_work_expired(struct work_struct *work)
> >>> +{
> >>> +     struct delayed_work *del_work = to_delayed_work(work);
> >>> +     struct br_mrp *mrp = container_of(del_work, struct br_mrp, test_work);
> >>> +     bool notify_open = false;
> >>> +     struct sk_buff *skb;
> >>> +
> >>
> >> Since this runs asynchronously what happens if the port is deleted ?
> >
> > Later I have checks to see if the port is no NULL. Is not good enough?
> > I have these rcu_access_pointer checks and before that I disable the
> > interrupts and get the rcu lock.
> >
> 
> That is not safe because you dereference the pointer again after the check
> and it may become NULL between those. You could do ptr = rcu_dereference();
> if (!ptr) and if non-null then continue accessing that memory through ptr.

I see, I will try to use rcu_dereference as you suggested.

> 
> >>
> >>> +     if (time_before_eq(mrp->test_end, jiffies))
> >>> +             return;
> >>> +
> >>> +     if (mrp->test_count_miss < mrp->test_max_miss) {
> >>> +             mrp->test_count_miss++;
> >>> +     } else {
> >>> +             /* Notify that the ring is open only if the ring state is
> >>> +              * closed, otherwise it would continue to notify at every
> >>> +              * interval.
> >>> +              */
> >>> +             if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED)
> >>> +                     notify_open = true;
> >>> +     }
> >>> +
> >>> +     local_bh_disable();
> >>> +     rcu_read_lock();
> >>> +
> >>> +     if (!rcu_access_pointer(mrp->p_port) ||
> >>> +         !rcu_access_pointer(mrp->s_port))
> >>> +             goto out;
> >>> +
> >>> +     /* Is it possible here to get call to delete the bridge port? If yes
> >>> +      * I need to protect it
> >>> +      */
> >>> +     dev_hold(rcu_dereference(mrp->p_port)->dev);
> >>> +
> >>
> >> This looks all wrong, p_port can become NULL here and you'll deref it.
> >
> > By disabling the interrupts and taking the rcu read lock, will I not be
> > sure that no one can access the p_port?
> 
> No. You should read more about how RCU operates.

Yes, I should definetly do that.

> 
> > If is not true, how the p_port can become NULL?
> >
> 
> Readers and writer run concurrently.
> 
> >>
> >>> +     skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->p_port)->dev,
> >>> +                                 BR_MRP_PORT_ROLE_PRIMARY);
> >>> +     if (!skb)
> >>> +             goto out;
> >>> +
> >>> +     skb_reset_network_header(skb);
> >>> +     dev_queue_xmit(skb);
> >>> +
> >>> +     if (notify_open && !mrp->ring_role_offloaded)
> >>> +             br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, true);
> >>> +
> >>> +     dev_put(rcu_dereference(mrp->p_port)->dev);
> >>> +
> >>> +     dev_hold(rcu_dereference(mrp->s_port)->dev);
> >>> +
> >>
> >> same here
> >>
> >>> +     skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->s_port)->dev,
> >>> +                                 BR_MRP_PORT_ROLE_SECONDARY);
> >>> +     if (!skb)
> >>> +             goto out;
> >>> +
> >>> +     skb_reset_network_header(skb);
> >>> +     dev_queue_xmit(skb);
> >>> +
> >>> +     if (notify_open && !mrp->ring_role_offloaded)
> >>> +             br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, true);
> >>> +
> >>> +     dev_put(rcu_dereference(mrp->s_port)->dev);
> >>> +
> >>> +out:
> >>> +     rcu_read_unlock();
> >>> +     local_bh_enable();
> >>> +
> >>> +     queue_delayed_work(system_wq, &mrp->test_work,
> >>> +                        usecs_to_jiffies(mrp->test_interval));
> >>> +}
> >>> +
> >>> +/* Adds a new MRP instance.
> >>> + * note: called under rtnl_lock
> >>> + */
> >>> +int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
> >>> +{
> >>> +     struct net_bridge_port *p;
> >>> +     struct br_mrp *mrp;
> >>> +
> >>> +     /* If the ring exists, it is not possible to create another one with the
> >>> +      * same ring_id
> >>> +      */
> >>> +     mrp = br_mrp_find_id(br, instance->ring_id);
> >>> +     if (mrp)
> >>> +             return -EINVAL;
> >>> +
> >>> +     if (!br_mrp_get_port(br, instance->p_ifindex) ||
> >>> +         !br_mrp_get_port(br, instance->s_ifindex))
> >>> +             return -EINVAL;
> >>> +
> >>> +     mrp = devm_kzalloc(&br->dev->dev, sizeof(struct br_mrp), GFP_KERNEL);
> >>> +     if (!mrp)
> >>> +             return -ENOMEM;
> >>> +
> >>> +     /* I think is not needed because this can be replaced with rtnl lock*/
> >>> +     spin_lock_init(&mrp->lock);
> >>> +     spin_lock(&mrp->lock);
> >>> +
> >>> +     mrp->br = br;
> >>
> >> Is this field (mrp->br) used anywhere ?
> >
> > Not anymore. I can remove it in the next patch series.
> >
> >>
> >>> +     mrp->ring_id = instance->ring_id;
> >>> +
> >>> +     p = br_mrp_get_port(br, instance->p_ifindex);
> >>> +     p->state = BR_STATE_FORWARDING;
> >>> +     p->flags |= BR_MRP_AWARE;
> >>> +     rcu_assign_pointer(mrp->p_port, p);
> >>> +
> >>> +     p = br_mrp_get_port(br, instance->s_ifindex);
> >>> +     p->state = BR_STATE_FORWARDING;
> >>> +     p->flags |= BR_MRP_AWARE;
> >>> +     rcu_assign_pointer(mrp->s_port, p);
> >>> +
> >>> +     br_mrp_switchdev_add(mrp);
> >>> +
> >>> +     spin_unlock(&mrp->lock);
> >>> +     synchronize_rcu();
> >>
> >> Why do you need the synchronize here?
> >
> > Actually this shouldn't be after the list_add_tail_rcu? Because I am
> > thinking that some can read the list at the same time I am change it.
> 
> That doesn't help, rcu primitives are already safe to run concurrently with readers.
> 
> >
> >>
> >>> +
> >>> +     list_add_tail_rcu(&mrp->list, &br->mrp_list);
> >>> +     INIT_DELAYED_WORK(&mrp->test_work, br_mrp_test_work_expired);
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +/* Deletes existing MRP instance.
> >>> + * note: called under rtnl_lock
> >>> + */
> >>> +int br_mrp_del(struct net_bridge *br, struct br_mrp_instance *instance)
> >>> +{
> >>> +     struct br_mrp *mrp = br_mrp_find_id(br, instance->ring_id);
> >>> +     struct net_bridge_port *p;
> >>> +
> >>> +     if (!mrp)
> >>> +             return -EINVAL;
> >>> +
> >>> +     /* Stop sending MRP_Test frames */
> >>> +     cancel_delayed_work(&mrp->test_work);
> >>
> >> cancel_delayed_work_sync() if you'd like to make sure it's stopped and finished (if it was running
> >> during this)
> >
> > Will be fixed in the next patch series.
> >>
> >>> +     br_mrp_switchdev_send_ring_test(mrp, 0, 0, 0);
> >>> +
> >>> +     spin_lock(&mrp->lock);
> >>> +
> >>> +     br_mrp_switchdev_del(mrp);
> >>> +
> >>> +     /* Reset the ports */
> >>> +     p = rcu_dereference_protected(mrp->p_port, lockdep_is_held(&mrp->lock));
> >>> +     if (p) {
> >>> +             spin_lock(&br->lock);
> >>> +             p->state = BR_STATE_FORWARDING;
> >>> +             p->flags &= ~BR_MRP_AWARE;
> >>> +             br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
> >>> +             rcu_assign_pointer(mrp->p_port, NULL);
> >>> +             spin_unlock(&br->lock);
> >>> +     }
> >>> +
> >>> +     p = rcu_dereference_protected(mrp->s_port, lockdep_is_held(&mrp->lock));
> >>> +     if (p) {
> >>> +             spin_lock(&br->lock);
> >>> +             p->state = BR_STATE_FORWARDING;
> >>> +             p->flags &= ~BR_MRP_AWARE;
> >>> +             br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
> >>> +             rcu_assign_pointer(mrp->s_port, NULL);
> >>> +             spin_unlock(&br->lock);
> >>> +     }
> >>> +
> >>> +     /* Destroy the ring */
> >>> +     mrp->br = NULL;
> >>> +
> >>> +     spin_unlock(&mrp->lock);
> >>> +     synchronize_rcu();
> >>> +
> >>> +     list_del_rcu(&mrp->list);
> >>> +     devm_kfree(&br->dev->dev, mrp);
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +int br_mrp_set_port_state(struct net_bridge_port *p,
> >>> +                       enum br_mrp_port_state_type state)
> >>> +{
> >>> +     spin_lock(&p->br->lock);
> >>> +
> >>> +     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);
> >>> +
> >>> +     spin_unlock(&p->br->lock);
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +int br_mrp_set_port_role(struct net_bridge_port *p,
> >>> +                      u32 ring_id, enum br_mrp_port_role_type role)
> >>> +{
> >>> +     struct br_mrp *mrp = br_mrp_find_id(p->br, ring_id);
> >>> +
> >>> +     if (!mrp)
> >>> +             return -EINVAL;
> >>> +
> >>> +     spin_lock(&mrp->lock);
> >>> +
> >>> +     if (role == BR_MRP_PORT_ROLE_PRIMARY)
> >>> +             rcu_assign_pointer(mrp->p_port, p);
> >>> +     if (role == BR_MRP_PORT_ROLE_SECONDARY)
> >>> +             rcu_assign_pointer(mrp->s_port, p);
> >>> +
> >>> +     br_mrp_port_switchdev_set_role(p, role);
> >>> +
> >>> +     spin_unlock(&mrp->lock);
> >>> +     synchronize_rcu();
> >>
> >> Why do you need to synchronize here?
> >
> > Actually this is not needed.
> >>
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +int br_mrp_set_ring_state(struct net_bridge *br, u32 ring_id,
> >>> +                       enum br_mrp_ring_state_type state)
> >>> +{
> >>> +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
> >>> +
> >>> +     if (!mrp)
> >>> +             return -EINVAL;
> >>> +
> >>> +     if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED &&
> >>> +         state != BR_MRP_RING_STATE_CLOSED)
> >>> +             mrp->ring_transitions++;
> >>> +
> >>> +     mrp->ring_state = state;
> >>> +
> >>> +     br_mrp_switchdev_set_ring_state(mrp, state);
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +int br_mrp_set_ring_role(struct net_bridge *br, u32 ring_id,
> >>> +                      enum br_mrp_ring_role_type role)
> >>> +{
> >>> +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
> >>> +     int err;
> >>> +
> >>> +     if (!mrp)
> >>> +             return -EINVAL;
> >>> +
> >>> +     mrp->ring_role = role;
> >>> +
> >>> +     /* If there is an error just bailed out */
> >>> +     err = br_mrp_switchdev_set_ring_role(mrp, role);
> >>> +     if (err && err != -EOPNOTSUPP)
> >>> +             return err;
> >>> +
> >>> +     /* Now detect if the HW actually applied the role or not. If the HW
> >>> +      * applied the role it means that the SW will not to do those operations
> >>> +      * anymore. For example if the role ir MRM then the HW will notify the
> >>> +      * SW when ring is open, but if the is not pushed to the HW the SW will
> >>> +      * need to detect when the ring is open
> >>> +      */
> >>> +     mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +int br_mrp_start_test(struct net_bridge *br, u32 ring_id, u32 interval,
> >>> +                   u8 max_miss, u32 period)
> >>> +{
> >>> +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
> >>> +
> >>> +     if (!mrp)
> >>> +             return -EINVAL;
> >>> +
> >>> +     /* Try to push is to the HW and if it fails then continue to generate in
> >>> +      * SW and if that also fails then return error
> >>> +      */
> >>> +     if (!br_mrp_switchdev_send_ring_test(mrp, interval, max_miss, period))
> >>> +             return 0;
> >>> +
> >>> +     mrp->test_interval = interval;
> >>> +     mrp->test_end = jiffies + usecs_to_jiffies(period);
> >>> +     mrp->test_max_miss = max_miss;
> >>> +     mrp->test_count_miss = 0;
> >>> +     queue_delayed_work(system_wq, &mrp->test_work,
> >>> +                        usecs_to_jiffies(interval));
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +/* Process only MRP Test frame. All the other MRP frames are processed by
> >>> + * userspace application
> >>> + * note: already called with rcu_read_lock
> >>> + */
> >>> +static void br_mrp_mrm_process(struct br_mrp *mrp, struct sk_buff *skb)
> >>> +{
> >>> +     struct br_mrp_tlv_hdr *hdr;
> >>> +
> >>> +     hdr = (struct br_mrp_tlv_hdr *)(skb->data + sizeof(uint16_t));
> >>> +
> >>> +     if (hdr->type != BR_MRP_TLV_HEADER_RING_TEST)
> >>> +             return;
> >>> +
> >>> +     mrp->test_count_miss = 0;
> >>> +
> >>> +     br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, false);
> >>> +     br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, false);
> >>> +}
> >>> +
> >>> +/* This will just forward the frame to the other mrp ring port(MRC role) or will
> >>> + * not do anything.
> >>> + * note: already called with rcu_read_lock
> >>> + */
> >>> +static int br_mrp_rcv(struct net_bridge_port *p,
> >>> +                   struct sk_buff *skb, struct net_device *dev)
> >>> +{
> >>> +     struct net_device *s_dev, *p_dev, *d_dev;
> >>> +     struct net_bridge *br;
> >>> +     struct sk_buff *nskb;
> >>> +     struct br_mrp *mrp;
> >>> +
> >>> +     /* If port is disable don't accept any frames */
> >>> +     if (p->state == BR_STATE_DISABLED)
> >>> +             return 0;
> >>> +
> >>> +     br = p->br;
> >>> +     mrp =  br_mrp_find_port(br, p);
> >>> +     if (unlikely(!mrp))
> >>> +             return 0;
> >>> +
> >>> +     /* If the role is MRM then don't forward the frames */
> >>> +     if (mrp->ring_role == BR_MRP_RING_ROLE_MRM) {
> >>> +             br_mrp_mrm_process(mrp, skb);
> >>> +             return 1;
> >>> +     }
> >>> +
> >>> +     nskb = skb_clone(skb, GFP_ATOMIC);
> >>> +     if (!nskb)
> >>> +             return 0;
> >>> +
> >>> +     p_dev = rcu_dereference(mrp->p_port)->dev;
> >>> +     s_dev = rcu_dereference(mrp->s_port)->dev;
> >>> +
> >>
> >> Not safe, could deref null.
> >
> > Will be fixed in the next patch series.
> >
> >>
> >>> +     if (p_dev == dev)
> >>> +             d_dev = s_dev;
> >>> +     else
> >>> +             d_dev = p_dev;
> >>> +
> >>> +     nskb->dev = d_dev;
> >>> +     skb_push(nskb, ETH_HLEN);
> >>> +     dev_queue_xmit(nskb);
> >>> +
> >>> +     return 1;
> >>> +}
> >>> +
> >>> +int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
> >>> +{
> >>> +     /* If there is no MRP instance do normal forwarding */
> >>> +     if (unlikely(!(p->flags & BR_MRP_AWARE)))
> >>
> >> Shouldn't this one be likely() ?
> >
> > Yes, this should be likely.
> >>
> >>> +             goto out;
> >>> +
> >>> +     if (unlikely(skb->protocol == htons(ETH_P_MRP)))
> >>> +             return br_mrp_rcv(p, skb, p->dev);
> >>> +
> >>> +out:
> >>> +     return 0;
> >>> +}
> >>>
> >>
> >
> > [1] https://lwn.net/Articles/262464/
> > [2] https://www.kernel.org/doc/html/latest/RCU/listRCU.html
> >
> 

-- 
/Horatiu

Re: [RFC net-next v4 7/9] bridge: mrp: Connect MRP api with the switchev API

[Nikolay Aleksandrov]

On 02/04/2020 17:18, Horatiu Vultur wrote:
> The 04/01/2020 19:32, Nikolay Aleksandrov wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 01/04/2020 19:06, Horatiu Vultur wrote:
>>> The 03/30/2020 19:11, Nikolay Aleksandrov wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 27/03/2020 11:21, Horatiu Vultur wrote:
>>>>> Implement the MRP api.
>>>>>
>>>>> In case the HW can't generate MRP Test frames then the SW will try to generate
>>>>> the frames. In case that also the SW will fail in generating the frames then a
>>>>> error is return to the userspace. The userspace is responsible to generate all
>>>>> the other MRP frames regardless if the test frames are generated by HW or SW.
>>>>>
>>>>> The forwarding/termination of MRP frames is happening in the kernel and is done
>>>>> by the MRP instance. The userspace application doesn't do the forwarding.
>>>>>
>>>>> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
>>>>> ---
>>>>>  net/bridge/br_mrp.c | 514 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 514 insertions(+)
>>>>>  create mode 100644 net/bridge/br_mrp.c
>>>>>
>>>>
>>>> Hi,
>>>
>>> Hi Nik,
>>>
>>>> In general the RCU usage needs more work.
>>>
>>> Thanks for the detailed review, this is my first time when I use the RCU,
>>> so I might need to spend more time on time.
>>>
>>>> Also I might've missed it, but where do you
>>>> handle bridge port delete which is used in mrp ?
>>>
>>> When a port is deleted, then the userspace application will be notified
>>> and then the userspace will remove the MRP instance. Because there is no
>>> point to have a MRP instance with only 1 port. And the function that
>>> delets the MRP instance is br_mrp_del.
>>>
>>
>> How would you execute br_mrp_del() if the port is already deleted from the bridge ?
>> Nothing prevents the port to disappear and then you lose all bridge callbacks.
> 
> The br_mrp_del() is called on the bridge interface and not on the port.
> The flow as I see it: the port is deleted from the bridge, the userspace
> will be notified by this, the userspace will determine the bridge on
> which was the port and then the userspace will make netlink call on the
> bridge interface. In this way it would not loose the callback when the
> port is deleted from the bridge.
> The question is when the bridge is deleted, I can see that first it
> deletes the ports and then it would delete the bridge. In this case I am
> loosing the callbacks.
> Should the function br_dev_delete be exteded for this?
> 

That is correct, but I don't think the above would work. There are pointers to
ports in MRP and if they get deleted those ptrs are no longer valid. Even in
br_mrp_del() there's code like:
 /* Reset the ports */
	p = rcu_dereference_protected(mrp->p_port, lockdep_is_held(&mrp->lock));

but p could've been deleted before so now br_mrp_del() will deref an invalid ptr.

By the way I just noticed another bug in br_mrp_del():
+	synchronize_rcu();
+
+	list_del_rcu(&mrp->list);

this is the wrong way around, you should delete it from the list so it can't be found
by any new readers and then do synchronize_rcu() before freeing it. Also why do you
use the devm_ calls ? You're allocating/freeing the memory so you don't need the managed
alloc, just cleanup properly on port/bridge del.

>>
>>>> Also do you actually need the mrp->lock ?
>>>
>>> I think I should be fine not to use mrp->lock because already the rtnl
>>> lock is taken.
>>>>>
>>>>> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
>>>>> new file mode 100644
>>>>> index 000000000000..f1de792d7a6e
>>>>> --- /dev/null
>>>>> +++ b/net/bridge/br_mrp.c
>>>>> @@ -0,0 +1,514 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>>>> +
>>>>> +#include "br_private_mrp.h"
>>>>> +
>>>>> +static const u8 mrp_test_dmac[ETH_ALEN] = { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 };
>>>>> +
>>>>> +static struct net_bridge_port *br_mrp_get_port(struct net_bridge *br,
>>>>> +                                            u32 ifindex)
>>>>> +{
>>>>> +     struct net_bridge_port *res = NULL;
>>>>> +     struct net_bridge_port *port;
>>>>> +
>>>>> +     spin_lock_bh(&br->lock);
>>>>> +
>>>>
>>>> This is called under RTNL, you don't need the br->lock.
>>>
>>> Will be fix in the next patch series.
>>>>
>>>>> +     list_for_each_entry(port, &br->port_list, list) {
>>>>> +             if (port->dev->ifindex == ifindex) {
>>>>> +                     res = port;
>>>>> +                     break;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     spin_unlock_bh(&br->lock);
>>>>> +
>>>>> +     return res;
>>>>> +}
>>>>> +
>>>>> +static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_id)
>>>>> +{
>>>>> +     struct br_mrp *res = NULL;
>>>>> +     struct br_mrp *mrp;
>>>>> +
>>>>> +     rcu_read_lock();
>>>>> +
>>>>
>>>> This is generally a bad pattern because it can hide legitimate bugs and make
>>>> it harder to debug.
>>>
>>> Can you give me a little more details why is a bad pattern?
>>> I have tried to read about rcu from here[1][2]. But I couldn't see
>>> anything about this.
>>>
>>
>> In general you should know the context the function is used in, you cannot use the
>> pointer obtained from this search after the rcu_read_unlock(). If this function is
>> ever used in context which doesn't have rcu read lock or the writer lock then you'll
>> mask the bug here. If you know it is always called from RCU context then just drop
>> these, if not then add the proper lockdep annotations so they can be checked.
> 
> Thanks for the details. I will fix it in the next patch series.
>>
>>>>
>>>>> +     list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
>>>>> +             if (mrp->ring_id == ring_id) {
>>>>> +                     res = mrp;
>>>>> +                     break;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     rcu_read_unlock();
>>>>> +
>>>>> +     return res;
>>>>> +}
>>>>> +
>>>>> +static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
>>>>> +                                    struct net_bridge_port *p)
>>>>> +{
>>>>> +     struct br_mrp *res = NULL;
>>>>> +     struct br_mrp *mrp;
>>>>> +
>>>>> +     rcu_read_lock();
>>>>> +
>>>>> +     list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
>>>>> +             if (rcu_dereference(mrp->p_port) == p ||
>>>>> +                 rcu_dereference(mrp->s_port) == p) {
>>>>
>>>> rcu_access_pointer() is ok for comparisons
>>>
>>> Will be fix in the next patch series.
>>>>
>>>>> +                     res = mrp;
>>>>> +                     break;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     rcu_read_unlock();
>>>>> +
>>>>> +     return res;
>>>>> +}
>>>>> +
>>>>> +static int br_mrp_next_seq(struct br_mrp *mrp)
>>>>> +{
>>>>> +     mrp->seq_id++;
>>>>> +     return mrp->seq_id;
>>>>> +}
>>>>> +
>>>>> +static struct sk_buff *br_mrp_skb_alloc(struct net_bridge_port *p,
>>>>> +                                     const u8 *src, const u8 *dst)
>>>>> +{
>>>>> +     struct ethhdr *eth_hdr;
>>>>> +     struct sk_buff *skb;
>>>>> +     u16 *version;
>>>>> +
>>>>> +     skb = dev_alloc_skb(MRP_MAX_FRAME_LENGTH);
>>>>> +     if (!skb)
>>>>> +             return NULL;
>>>>> +
>>>>> +     skb->dev = p->dev;
>>>>> +     skb->protocol = htons(ETH_P_MRP);
>>>>> +     skb->priority = MRP_FRAME_PRIO;
>>>>> +     skb_reserve(skb, sizeof(*eth_hdr));
>>>>> +
>>>>> +     eth_hdr = skb_push(skb, sizeof(*eth_hdr));
>>>>> +     ether_addr_copy(eth_hdr->h_dest, dst);
>>>>> +     ether_addr_copy(eth_hdr->h_source, src);
>>>>> +     eth_hdr->h_proto = htons(ETH_P_MRP);
>>>>> +
>>>>> +     version = skb_put(skb, sizeof(*version));
>>>>> +     *version = cpu_to_be16(MRP_VERSION);
>>>>> +
>>>>> +     return skb;
>>>>> +}
>>>>> +
>>>>> +static void br_mrp_skb_tlv(struct sk_buff *skb,
>>>>> +                        enum br_mrp_tlv_header_type type,
>>>>> +                        u8 length)
>>>>> +{
>>>>> +     struct br_mrp_tlv_hdr *hdr;
>>>>> +
>>>>> +     hdr = skb_put(skb, sizeof(*hdr));
>>>>> +     hdr->type = type;
>>>>> +     hdr->length = length;
>>>>> +}
>>>>> +
>>>>> +static void br_mrp_skb_common(struct sk_buff *skb, struct br_mrp *mrp)
>>>>> +{
>>>>> +     struct br_mrp_common_hdr *hdr;
>>>>> +
>>>>> +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_COMMON, sizeof(*hdr));
>>>>> +
>>>>> +     hdr = skb_put(skb, sizeof(*hdr));
>>>>> +     hdr->seq_id = cpu_to_be16(br_mrp_next_seq(mrp));
>>>>> +     memset(hdr->domain, 0xff, MRP_DOMAIN_UUID_LENGTH);
>>>>> +}
>>>>> +
>>>>> +static struct sk_buff *br_mrp_alloc_test_skb(struct br_mrp *mrp,
>>>>> +                                          struct net_device *dev,
>>>>> +                                          enum br_mrp_port_role_type port_role)
>>>>> +{
>>>>> +     struct net_bridge_port *p = br_port_get_rtnl(dev);
>>>>> +     struct br_mrp_ring_test_hdr *hdr = NULL;
>>>>> +     struct net_bridge *br = p->br;
>>>>> +     struct sk_buff *skb = NULL;
>>>>> +
>>>>> +     if (!p)
>>>>> +             return NULL;
>>>>> +
>>>>> +     br = p->br;
>>>>> +
>>>>> +     skb = br_mrp_skb_alloc(p, p->dev->dev_addr, mrp_test_dmac);
>>>>> +     if (!skb)
>>>>> +             return NULL;
>>>>> +
>>>>> +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_RING_TEST, sizeof(*hdr));
>>>>> +     hdr = skb_put(skb, sizeof(*hdr));
>>>>> +
>>>>> +     hdr->prio = cpu_to_be16(MRP_DEFAULT_PRIO);
>>>>> +     ether_addr_copy(hdr->sa, p->br->dev->dev_addr);
>>>>> +     hdr->port_role = cpu_to_be16(port_role);
>>>>> +     hdr->state = cpu_to_be16(mrp->ring_state);
>>>>> +     hdr->transitions = cpu_to_be16(mrp->ring_transitions);
>>>>> +     hdr->timestamp = cpu_to_be32(jiffies_to_msecs(jiffies));
>>>>> +
>>>>> +     br_mrp_skb_common(skb, mrp);
>>>>> +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_END, 0x0);
>>>>> +
>>>>> +     return skb;
>>>>> +}
>>>>> +
>>>>> +static void br_mrp_test_work_expired(struct work_struct *work)
>>>>> +{
>>>>> +     struct delayed_work *del_work = to_delayed_work(work);
>>>>> +     struct br_mrp *mrp = container_of(del_work, struct br_mrp, test_work);
>>>>> +     bool notify_open = false;
>>>>> +     struct sk_buff *skb;
>>>>> +
>>>>
>>>> Since this runs asynchronously what happens if the port is deleted ?
>>>
>>> Later I have checks to see if the port is no NULL. Is not good enough?
>>> I have these rcu_access_pointer checks and before that I disable the
>>> interrupts and get the rcu lock.
>>>
>>
>> That is not safe because you dereference the pointer again after the check
>> and it may become NULL between those. You could do ptr = rcu_dereference();
>> if (!ptr) and if non-null then continue accessing that memory through ptr.
> 
> I see, I will try to use rcu_dereference as you suggested.
> 
>>
>>>>
>>>>> +     if (time_before_eq(mrp->test_end, jiffies))
>>>>> +             return;
>>>>> +
>>>>> +     if (mrp->test_count_miss < mrp->test_max_miss) {
>>>>> +             mrp->test_count_miss++;
>>>>> +     } else {
>>>>> +             /* Notify that the ring is open only if the ring state is
>>>>> +              * closed, otherwise it would continue to notify at every
>>>>> +              * interval.
>>>>> +              */
>>>>> +             if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED)
>>>>> +                     notify_open = true;
>>>>> +     }
>>>>> +
>>>>> +     local_bh_disable();
>>>>> +     rcu_read_lock();
>>>>> +
>>>>> +     if (!rcu_access_pointer(mrp->p_port) ||
>>>>> +         !rcu_access_pointer(mrp->s_port))
>>>>> +             goto out;
>>>>> +
>>>>> +     /* Is it possible here to get call to delete the bridge port? If yes
>>>>> +      * I need to protect it
>>>>> +      */
>>>>> +     dev_hold(rcu_dereference(mrp->p_port)->dev);
>>>>> +
>>>>
>>>> This looks all wrong, p_port can become NULL here and you'll deref it.
>>>
>>> By disabling the interrupts and taking the rcu read lock, will I not be
>>> sure that no one can access the p_port?
>>
>> No. You should read more about how RCU operates.
> 
> Yes, I should definetly do that.
> 
>>
>>> If is not true, how the p_port can become NULL?
>>>
>>
>> Readers and writer run concurrently.
>>
>>>>
>>>>> +     skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->p_port)->dev,
>>>>> +                                 BR_MRP_PORT_ROLE_PRIMARY);
>>>>> +     if (!skb)
>>>>> +             goto out;
>>>>> +
>>>>> +     skb_reset_network_header(skb);
>>>>> +     dev_queue_xmit(skb);
>>>>> +
>>>>> +     if (notify_open && !mrp->ring_role_offloaded)
>>>>> +             br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, true);
>>>>> +
>>>>> +     dev_put(rcu_dereference(mrp->p_port)->dev);
>>>>> +
>>>>> +     dev_hold(rcu_dereference(mrp->s_port)->dev);
>>>>> +
>>>>
>>>> same here
>>>>
>>>>> +     skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->s_port)->dev,
>>>>> +                                 BR_MRP_PORT_ROLE_SECONDARY);
>>>>> +     if (!skb)
>>>>> +             goto out;
>>>>> +
>>>>> +     skb_reset_network_header(skb);
>>>>> +     dev_queue_xmit(skb);
>>>>> +
>>>>> +     if (notify_open && !mrp->ring_role_offloaded)
>>>>> +             br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, true);
>>>>> +
>>>>> +     dev_put(rcu_dereference(mrp->s_port)->dev);
>>>>> +
>>>>> +out:
>>>>> +     rcu_read_unlock();
>>>>> +     local_bh_enable();
>>>>> +
>>>>> +     queue_delayed_work(system_wq, &mrp->test_work,
>>>>> +                        usecs_to_jiffies(mrp->test_interval));
>>>>> +}
>>>>> +
>>>>> +/* Adds a new MRP instance.
>>>>> + * note: called under rtnl_lock
>>>>> + */
>>>>> +int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
>>>>> +{
>>>>> +     struct net_bridge_port *p;
>>>>> +     struct br_mrp *mrp;
>>>>> +
>>>>> +     /* If the ring exists, it is not possible to create another one with the
>>>>> +      * same ring_id
>>>>> +      */
>>>>> +     mrp = br_mrp_find_id(br, instance->ring_id);
>>>>> +     if (mrp)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     if (!br_mrp_get_port(br, instance->p_ifindex) ||
>>>>> +         !br_mrp_get_port(br, instance->s_ifindex))
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     mrp = devm_kzalloc(&br->dev->dev, sizeof(struct br_mrp), GFP_KERNEL);
>>>>> +     if (!mrp)
>>>>> +             return -ENOMEM;
>>>>> +
>>>>> +     /* I think is not needed because this can be replaced with rtnl lock*/
>>>>> +     spin_lock_init(&mrp->lock);
>>>>> +     spin_lock(&mrp->lock);
>>>>> +
>>>>> +     mrp->br = br;
>>>>
>>>> Is this field (mrp->br) used anywhere ?
>>>
>>> Not anymore. I can remove it in the next patch series.
>>>
>>>>
>>>>> +     mrp->ring_id = instance->ring_id;
>>>>> +
>>>>> +     p = br_mrp_get_port(br, instance->p_ifindex);
>>>>> +     p->state = BR_STATE_FORWARDING;
>>>>> +     p->flags |= BR_MRP_AWARE;
>>>>> +     rcu_assign_pointer(mrp->p_port, p);
>>>>> +
>>>>> +     p = br_mrp_get_port(br, instance->s_ifindex);
>>>>> +     p->state = BR_STATE_FORWARDING;
>>>>> +     p->flags |= BR_MRP_AWARE;
>>>>> +     rcu_assign_pointer(mrp->s_port, p);
>>>>> +
>>>>> +     br_mrp_switchdev_add(mrp);
>>>>> +
>>>>> +     spin_unlock(&mrp->lock);
>>>>> +     synchronize_rcu();
>>>>
>>>> Why do you need the synchronize here?
>>>
>>> Actually this shouldn't be after the list_add_tail_rcu? Because I am
>>> thinking that some can read the list at the same time I am change it.
>>
>> That doesn't help, rcu primitives are already safe to run concurrently with readers.
>>
>>>
>>>>
>>>>> +
>>>>> +     list_add_tail_rcu(&mrp->list, &br->mrp_list);
>>>>> +     INIT_DELAYED_WORK(&mrp->test_work, br_mrp_test_work_expired);
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +/* Deletes existing MRP instance.
>>>>> + * note: called under rtnl_lock
>>>>> + */
>>>>> +int br_mrp_del(struct net_bridge *br, struct br_mrp_instance *instance)
>>>>> +{
>>>>> +     struct br_mrp *mrp = br_mrp_find_id(br, instance->ring_id);
>>>>> +     struct net_bridge_port *p;
>>>>> +
>>>>> +     if (!mrp)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     /* Stop sending MRP_Test frames */
>>>>> +     cancel_delayed_work(&mrp->test_work);
>>>>
>>>> cancel_delayed_work_sync() if you'd like to make sure it's stopped and finished (if it was running
>>>> during this)
>>>
>>> Will be fixed in the next patch series.
>>>>
>>>>> +     br_mrp_switchdev_send_ring_test(mrp, 0, 0, 0);
>>>>> +
>>>>> +     spin_lock(&mrp->lock);
>>>>> +
>>>>> +     br_mrp_switchdev_del(mrp);
>>>>> +
>>>>> +     /* Reset the ports */
>>>>> +     p = rcu_dereference_protected(mrp->p_port, lockdep_is_held(&mrp->lock));
>>>>> +     if (p) {
>>>>> +             spin_lock(&br->lock);
>>>>> +             p->state = BR_STATE_FORWARDING;
>>>>> +             p->flags &= ~BR_MRP_AWARE;
>>>>> +             br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
>>>>> +             rcu_assign_pointer(mrp->p_port, NULL);
>>>>> +             spin_unlock(&br->lock);
>>>>> +     }
>>>>> +
>>>>> +     p = rcu_dereference_protected(mrp->s_port, lockdep_is_held(&mrp->lock));
>>>>> +     if (p) {
>>>>> +             spin_lock(&br->lock);
>>>>> +             p->state = BR_STATE_FORWARDING;
>>>>> +             p->flags &= ~BR_MRP_AWARE;
>>>>> +             br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
>>>>> +             rcu_assign_pointer(mrp->s_port, NULL);
>>>>> +             spin_unlock(&br->lock);
>>>>> +     }
>>>>> +
>>>>> +     /* Destroy the ring */
>>>>> +     mrp->br = NULL;
>>>>> +
>>>>> +     spin_unlock(&mrp->lock);
>>>>> +     synchronize_rcu();
>>>>> +
>>>>> +     list_del_rcu(&mrp->list);
>>>>> +     devm_kfree(&br->dev->dev, mrp);
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +int br_mrp_set_port_state(struct net_bridge_port *p,
>>>>> +                       enum br_mrp_port_state_type state)
>>>>> +{
>>>>> +     spin_lock(&p->br->lock);
>>>>> +
>>>>> +     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);
>>>>> +
>>>>> +     spin_unlock(&p->br->lock);
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +int br_mrp_set_port_role(struct net_bridge_port *p,
>>>>> +                      u32 ring_id, enum br_mrp_port_role_type role)
>>>>> +{
>>>>> +     struct br_mrp *mrp = br_mrp_find_id(p->br, ring_id);
>>>>> +
>>>>> +     if (!mrp)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     spin_lock(&mrp->lock);
>>>>> +
>>>>> +     if (role == BR_MRP_PORT_ROLE_PRIMARY)
>>>>> +             rcu_assign_pointer(mrp->p_port, p);
>>>>> +     if (role == BR_MRP_PORT_ROLE_SECONDARY)
>>>>> +             rcu_assign_pointer(mrp->s_port, p);
>>>>> +
>>>>> +     br_mrp_port_switchdev_set_role(p, role);
>>>>> +
>>>>> +     spin_unlock(&mrp->lock);
>>>>> +     synchronize_rcu();
>>>>
>>>> Why do you need to synchronize here?
>>>
>>> Actually this is not needed.
>>>>
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +int br_mrp_set_ring_state(struct net_bridge *br, u32 ring_id,
>>>>> +                       enum br_mrp_ring_state_type state)
>>>>> +{
>>>>> +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
>>>>> +
>>>>> +     if (!mrp)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED &&
>>>>> +         state != BR_MRP_RING_STATE_CLOSED)
>>>>> +             mrp->ring_transitions++;
>>>>> +
>>>>> +     mrp->ring_state = state;
>>>>> +
>>>>> +     br_mrp_switchdev_set_ring_state(mrp, state);
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +int br_mrp_set_ring_role(struct net_bridge *br, u32 ring_id,
>>>>> +                      enum br_mrp_ring_role_type role)
>>>>> +{
>>>>> +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
>>>>> +     int err;
>>>>> +
>>>>> +     if (!mrp)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     mrp->ring_role = role;
>>>>> +
>>>>> +     /* If there is an error just bailed out */
>>>>> +     err = br_mrp_switchdev_set_ring_role(mrp, role);
>>>>> +     if (err && err != -EOPNOTSUPP)
>>>>> +             return err;
>>>>> +
>>>>> +     /* Now detect if the HW actually applied the role or not. If the HW
>>>>> +      * applied the role it means that the SW will not to do those operations
>>>>> +      * anymore. For example if the role ir MRM then the HW will notify the
>>>>> +      * SW when ring is open, but if the is not pushed to the HW the SW will
>>>>> +      * need to detect when the ring is open
>>>>> +      */
>>>>> +     mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +int br_mrp_start_test(struct net_bridge *br, u32 ring_id, u32 interval,
>>>>> +                   u8 max_miss, u32 period)
>>>>> +{
>>>>> +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
>>>>> +
>>>>> +     if (!mrp)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     /* Try to push is to the HW and if it fails then continue to generate in
>>>>> +      * SW and if that also fails then return error
>>>>> +      */
>>>>> +     if (!br_mrp_switchdev_send_ring_test(mrp, interval, max_miss, period))
>>>>> +             return 0;
>>>>> +
>>>>> +     mrp->test_interval = interval;
>>>>> +     mrp->test_end = jiffies + usecs_to_jiffies(period);
>>>>> +     mrp->test_max_miss = max_miss;
>>>>> +     mrp->test_count_miss = 0;
>>>>> +     queue_delayed_work(system_wq, &mrp->test_work,
>>>>> +                        usecs_to_jiffies(interval));
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +/* Process only MRP Test frame. All the other MRP frames are processed by
>>>>> + * userspace application
>>>>> + * note: already called with rcu_read_lock
>>>>> + */
>>>>> +static void br_mrp_mrm_process(struct br_mrp *mrp, struct sk_buff *skb)
>>>>> +{
>>>>> +     struct br_mrp_tlv_hdr *hdr;
>>>>> +
>>>>> +     hdr = (struct br_mrp_tlv_hdr *)(skb->data + sizeof(uint16_t));
>>>>> +
>>>>> +     if (hdr->type != BR_MRP_TLV_HEADER_RING_TEST)
>>>>> +             return;
>>>>> +
>>>>> +     mrp->test_count_miss = 0;
>>>>> +
>>>>> +     br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, false);
>>>>> +     br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, false);
>>>>> +}
>>>>> +
>>>>> +/* This will just forward the frame to the other mrp ring port(MRC role) or will
>>>>> + * not do anything.
>>>>> + * note: already called with rcu_read_lock
>>>>> + */
>>>>> +static int br_mrp_rcv(struct net_bridge_port *p,
>>>>> +                   struct sk_buff *skb, struct net_device *dev)
>>>>> +{
>>>>> +     struct net_device *s_dev, *p_dev, *d_dev;
>>>>> +     struct net_bridge *br;
>>>>> +     struct sk_buff *nskb;
>>>>> +     struct br_mrp *mrp;
>>>>> +
>>>>> +     /* If port is disable don't accept any frames */
>>>>> +     if (p->state == BR_STATE_DISABLED)
>>>>> +             return 0;
>>>>> +
>>>>> +     br = p->br;
>>>>> +     mrp =  br_mrp_find_port(br, p);
>>>>> +     if (unlikely(!mrp))
>>>>> +             return 0;
>>>>> +
>>>>> +     /* If the role is MRM then don't forward the frames */
>>>>> +     if (mrp->ring_role == BR_MRP_RING_ROLE_MRM) {
>>>>> +             br_mrp_mrm_process(mrp, skb);
>>>>> +             return 1;
>>>>> +     }
>>>>> +
>>>>> +     nskb = skb_clone(skb, GFP_ATOMIC);
>>>>> +     if (!nskb)
>>>>> +             return 0;
>>>>> +
>>>>> +     p_dev = rcu_dereference(mrp->p_port)->dev;
>>>>> +     s_dev = rcu_dereference(mrp->s_port)->dev;
>>>>> +
>>>>
>>>> Not safe, could deref null.
>>>
>>> Will be fixed in the next patch series.
>>>
>>>>
>>>>> +     if (p_dev == dev)
>>>>> +             d_dev = s_dev;
>>>>> +     else
>>>>> +             d_dev = p_dev;
>>>>> +
>>>>> +     nskb->dev = d_dev;
>>>>> +     skb_push(nskb, ETH_HLEN);
>>>>> +     dev_queue_xmit(nskb);
>>>>> +
>>>>> +     return 1;
>>>>> +}
>>>>> +
>>>>> +int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
>>>>> +{
>>>>> +     /* If there is no MRP instance do normal forwarding */
>>>>> +     if (unlikely(!(p->flags & BR_MRP_AWARE)))
>>>>
>>>> Shouldn't this one be likely() ?
>>>
>>> Yes, this should be likely.
>>>>
>>>>> +             goto out;
>>>>> +
>>>>> +     if (unlikely(skb->protocol == htons(ETH_P_MRP)))
>>>>> +             return br_mrp_rcv(p, skb, p->dev);
>>>>> +
>>>>> +out:
>>>>> +     return 0;
>>>>> +}
>>>>>
>>>>
>>>
>>> [1] https://lwn.net/Articles/262464/
>>> [2] https://www.kernel.org/doc/html/latest/RCU/listRCU.html
>>>
>>
> 

Re: [RFC net-next v4 7/9] bridge: mrp: Connect MRP api with the switchev API

[Horatiu Vultur]

The 04/02/2020 17:29, Nikolay Aleksandrov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 02/04/2020 17:18, Horatiu Vultur wrote:
> > The 04/01/2020 19:32, Nikolay Aleksandrov wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>
> >> On 01/04/2020 19:06, Horatiu Vultur wrote:
> >>> The 03/30/2020 19:11, Nikolay Aleksandrov wrote:
> >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>>
> >>>> On 27/03/2020 11:21, Horatiu Vultur wrote:
> >>>>> Implement the MRP api.
> >>>>>
> >>>>> In case the HW can't generate MRP Test frames then the SW will try to generate
> >>>>> the frames. In case that also the SW will fail in generating the frames then a
> >>>>> error is return to the userspace. The userspace is responsible to generate all
> >>>>> the other MRP frames regardless if the test frames are generated by HW or SW.
> >>>>>
> >>>>> The forwarding/termination of MRP frames is happening in the kernel and is done
> >>>>> by the MRP instance. The userspace application doesn't do the forwarding.
> >>>>>
> >>>>> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> >>>>> ---
> >>>>>  net/bridge/br_mrp.c | 514 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>>  1 file changed, 514 insertions(+)
> >>>>>  create mode 100644 net/bridge/br_mrp.c
> >>>>>
> >>>>
> >>>> Hi,
> >>>
> >>> Hi Nik,
> >>>
> >>>> In general the RCU usage needs more work.
> >>>
> >>> Thanks for the detailed review, this is my first time when I use the RCU,
> >>> so I might need to spend more time on time.
> >>>
> >>>> Also I might've missed it, but where do you
> >>>> handle bridge port delete which is used in mrp ?
> >>>
> >>> When a port is deleted, then the userspace application will be notified
> >>> and then the userspace will remove the MRP instance. Because there is no
> >>> point to have a MRP instance with only 1 port. And the function that
> >>> delets the MRP instance is br_mrp_del.
> >>>
> >>
> >> How would you execute br_mrp_del() if the port is already deleted from the bridge ?
> >> Nothing prevents the port to disappear and then you lose all bridge callbacks.
> >
> > The br_mrp_del() is called on the bridge interface and not on the port.
> > The flow as I see it: the port is deleted from the bridge, the userspace
> > will be notified by this, the userspace will determine the bridge on
> > which was the port and then the userspace will make netlink call on the
> > bridge interface. In this way it would not loose the callback when the
> > port is deleted from the bridge.
> > The question is when the bridge is deleted, I can see that first it
> > deletes the ports and then it would delete the bridge. In this case I am
> > loosing the callbacks.
> > Should the function br_dev_delete be exteded for this?
> >
> 
> That is correct, but I don't think the above would work. There are pointers to
> ports in MRP and if they get deleted those ptrs are no longer valid. Even in
> br_mrp_del() there's code like:
>  /* Reset the ports */
>         p = rcu_dereference_protected(mrp->p_port, lockdep_is_held(&mrp->lock));
> 
> but p could've been deleted before so now br_mrp_del() will deref an invalid ptr.

I understand it now.
What about extending the function del_nbp? Basically just to make a call
to br_mrp_del(of course the br_mrp_del needs to be updated to receive
the port). In this way the pointers to ports in MRP are valid. Because
from the port we can go back to the bridge and from there we can find
which MRP instance has the port that would be deleted.

And the userspace will not need to call back in the kernel to delete the
MRP instance. But if the user wants to delete a MRP instance, it is
still required for the userspace to call in the kernel to delete the MRP
instance.

> 
> By the way I just noticed another bug in br_mrp_del():
> +       synchronize_rcu();
> +
> +       list_del_rcu(&mrp->list);
> 
> this is the wrong way around, you should delete it from the list so it can't be found
> by any new readers and then do synchronize_rcu() before freeing it.
Good catch, also I noticed few other possible bugs in the file
br_mrp_switchdev file, which needs to be fixed.

> Also why do you use the devm_ calls ?
> You're allocating/freeing the memory so you don't need the managed
> alloc, just cleanup properly on port/bridge del.
No specific reason, I will update in the next patch series.

> 
> >>
> >>>> Also do you actually need the mrp->lock ?
> >>>
> >>> I think I should be fine not to use mrp->lock because already the rtnl
> >>> lock is taken.
> >>>>>
> >>>>> diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
> >>>>> new file mode 100644
> >>>>> index 000000000000..f1de792d7a6e
> >>>>> --- /dev/null
> >>>>> +++ b/net/bridge/br_mrp.c
> >>>>> @@ -0,0 +1,514 @@
> >>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
> >>>>> +
> >>>>> +#include "br_private_mrp.h"
> >>>>> +
> >>>>> +static const u8 mrp_test_dmac[ETH_ALEN] = { 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 };
> >>>>> +
> >>>>> +static struct net_bridge_port *br_mrp_get_port(struct net_bridge *br,
> >>>>> +                                            u32 ifindex)
> >>>>> +{
> >>>>> +     struct net_bridge_port *res = NULL;
> >>>>> +     struct net_bridge_port *port;
> >>>>> +
> >>>>> +     spin_lock_bh(&br->lock);
> >>>>> +
> >>>>
> >>>> This is called under RTNL, you don't need the br->lock.
> >>>
> >>> Will be fix in the next patch series.
> >>>>
> >>>>> +     list_for_each_entry(port, &br->port_list, list) {
> >>>>> +             if (port->dev->ifindex == ifindex) {
> >>>>> +                     res = port;
> >>>>> +                     break;
> >>>>> +             }
> >>>>> +     }
> >>>>> +
> >>>>> +     spin_unlock_bh(&br->lock);
> >>>>> +
> >>>>> +     return res;
> >>>>> +}
> >>>>> +
> >>>>> +static struct br_mrp *br_mrp_find_id(struct net_bridge *br, u32 ring_id)
> >>>>> +{
> >>>>> +     struct br_mrp *res = NULL;
> >>>>> +     struct br_mrp *mrp;
> >>>>> +
> >>>>> +     rcu_read_lock();
> >>>>> +
> >>>>
> >>>> This is generally a bad pattern because it can hide legitimate bugs and make
> >>>> it harder to debug.
> >>>
> >>> Can you give me a little more details why is a bad pattern?
> >>> I have tried to read about rcu from here[1][2]. But I couldn't see
> >>> anything about this.
> >>>
> >>
> >> In general you should know the context the function is used in, you cannot use the
> >> pointer obtained from this search after the rcu_read_unlock(). If this function is
> >> ever used in context which doesn't have rcu read lock or the writer lock then you'll
> >> mask the bug here. If you know it is always called from RCU context then just drop
> >> these, if not then add the proper lockdep annotations so they can be checked.
> >
> > Thanks for the details. I will fix it in the next patch series.
> >>
> >>>>
> >>>>> +     list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
> >>>>> +             if (mrp->ring_id == ring_id) {
> >>>>> +                     res = mrp;
> >>>>> +                     break;
> >>>>> +             }
> >>>>> +     }
> >>>>> +
> >>>>> +     rcu_read_unlock();
> >>>>> +
> >>>>> +     return res;
> >>>>> +}
> >>>>> +
> >>>>> +static struct br_mrp *br_mrp_find_port(struct net_bridge *br,
> >>>>> +                                    struct net_bridge_port *p)
> >>>>> +{
> >>>>> +     struct br_mrp *res = NULL;
> >>>>> +     struct br_mrp *mrp;
> >>>>> +
> >>>>> +     rcu_read_lock();
> >>>>> +
> >>>>> +     list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
> >>>>> +             if (rcu_dereference(mrp->p_port) == p ||
> >>>>> +                 rcu_dereference(mrp->s_port) == p) {
> >>>>
> >>>> rcu_access_pointer() is ok for comparisons
> >>>
> >>> Will be fix in the next patch series.
> >>>>
> >>>>> +                     res = mrp;
> >>>>> +                     break;
> >>>>> +             }
> >>>>> +     }
> >>>>> +
> >>>>> +     rcu_read_unlock();
> >>>>> +
> >>>>> +     return res;
> >>>>> +}
> >>>>> +
> >>>>> +static int br_mrp_next_seq(struct br_mrp *mrp)
> >>>>> +{
> >>>>> +     mrp->seq_id++;
> >>>>> +     return mrp->seq_id;
> >>>>> +}
> >>>>> +
> >>>>> +static struct sk_buff *br_mrp_skb_alloc(struct net_bridge_port *p,
> >>>>> +                                     const u8 *src, const u8 *dst)
> >>>>> +{
> >>>>> +     struct ethhdr *eth_hdr;
> >>>>> +     struct sk_buff *skb;
> >>>>> +     u16 *version;
> >>>>> +
> >>>>> +     skb = dev_alloc_skb(MRP_MAX_FRAME_LENGTH);
> >>>>> +     if (!skb)
> >>>>> +             return NULL;
> >>>>> +
> >>>>> +     skb->dev = p->dev;
> >>>>> +     skb->protocol = htons(ETH_P_MRP);
> >>>>> +     skb->priority = MRP_FRAME_PRIO;
> >>>>> +     skb_reserve(skb, sizeof(*eth_hdr));
> >>>>> +
> >>>>> +     eth_hdr = skb_push(skb, sizeof(*eth_hdr));
> >>>>> +     ether_addr_copy(eth_hdr->h_dest, dst);
> >>>>> +     ether_addr_copy(eth_hdr->h_source, src);
> >>>>> +     eth_hdr->h_proto = htons(ETH_P_MRP);
> >>>>> +
> >>>>> +     version = skb_put(skb, sizeof(*version));
> >>>>> +     *version = cpu_to_be16(MRP_VERSION);
> >>>>> +
> >>>>> +     return skb;
> >>>>> +}
> >>>>> +
> >>>>> +static void br_mrp_skb_tlv(struct sk_buff *skb,
> >>>>> +                        enum br_mrp_tlv_header_type type,
> >>>>> +                        u8 length)
> >>>>> +{
> >>>>> +     struct br_mrp_tlv_hdr *hdr;
> >>>>> +
> >>>>> +     hdr = skb_put(skb, sizeof(*hdr));
> >>>>> +     hdr->type = type;
> >>>>> +     hdr->length = length;
> >>>>> +}
> >>>>> +
> >>>>> +static void br_mrp_skb_common(struct sk_buff *skb, struct br_mrp *mrp)
> >>>>> +{
> >>>>> +     struct br_mrp_common_hdr *hdr;
> >>>>> +
> >>>>> +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_COMMON, sizeof(*hdr));
> >>>>> +
> >>>>> +     hdr = skb_put(skb, sizeof(*hdr));
> >>>>> +     hdr->seq_id = cpu_to_be16(br_mrp_next_seq(mrp));
> >>>>> +     memset(hdr->domain, 0xff, MRP_DOMAIN_UUID_LENGTH);
> >>>>> +}
> >>>>> +
> >>>>> +static struct sk_buff *br_mrp_alloc_test_skb(struct br_mrp *mrp,
> >>>>> +                                          struct net_device *dev,
> >>>>> +                                          enum br_mrp_port_role_type port_role)
> >>>>> +{
> >>>>> +     struct net_bridge_port *p = br_port_get_rtnl(dev);
> >>>>> +     struct br_mrp_ring_test_hdr *hdr = NULL;
> >>>>> +     struct net_bridge *br = p->br;
> >>>>> +     struct sk_buff *skb = NULL;
> >>>>> +
> >>>>> +     if (!p)
> >>>>> +             return NULL;
> >>>>> +
> >>>>> +     br = p->br;
> >>>>> +
> >>>>> +     skb = br_mrp_skb_alloc(p, p->dev->dev_addr, mrp_test_dmac);
> >>>>> +     if (!skb)
> >>>>> +             return NULL;
> >>>>> +
> >>>>> +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_RING_TEST, sizeof(*hdr));
> >>>>> +     hdr = skb_put(skb, sizeof(*hdr));
> >>>>> +
> >>>>> +     hdr->prio = cpu_to_be16(MRP_DEFAULT_PRIO);
> >>>>> +     ether_addr_copy(hdr->sa, p->br->dev->dev_addr);
> >>>>> +     hdr->port_role = cpu_to_be16(port_role);
> >>>>> +     hdr->state = cpu_to_be16(mrp->ring_state);
> >>>>> +     hdr->transitions = cpu_to_be16(mrp->ring_transitions);
> >>>>> +     hdr->timestamp = cpu_to_be32(jiffies_to_msecs(jiffies));
> >>>>> +
> >>>>> +     br_mrp_skb_common(skb, mrp);
> >>>>> +     br_mrp_skb_tlv(skb, BR_MRP_TLV_HEADER_END, 0x0);
> >>>>> +
> >>>>> +     return skb;
> >>>>> +}
> >>>>> +
> >>>>> +static void br_mrp_test_work_expired(struct work_struct *work)
> >>>>> +{
> >>>>> +     struct delayed_work *del_work = to_delayed_work(work);
> >>>>> +     struct br_mrp *mrp = container_of(del_work, struct br_mrp, test_work);
> >>>>> +     bool notify_open = false;
> >>>>> +     struct sk_buff *skb;
> >>>>> +
> >>>>
> >>>> Since this runs asynchronously what happens if the port is deleted ?
> >>>
> >>> Later I have checks to see if the port is no NULL. Is not good enough?
> >>> I have these rcu_access_pointer checks and before that I disable the
> >>> interrupts and get the rcu lock.
> >>>
> >>
> >> That is not safe because you dereference the pointer again after the check
> >> and it may become NULL between those. You could do ptr = rcu_dereference();
> >> if (!ptr) and if non-null then continue accessing that memory through ptr.
> >
> > I see, I will try to use rcu_dereference as you suggested.
> >
> >>
> >>>>
> >>>>> +     if (time_before_eq(mrp->test_end, jiffies))
> >>>>> +             return;
> >>>>> +
> >>>>> +     if (mrp->test_count_miss < mrp->test_max_miss) {
> >>>>> +             mrp->test_count_miss++;
> >>>>> +     } else {
> >>>>> +             /* Notify that the ring is open only if the ring state is
> >>>>> +              * closed, otherwise it would continue to notify at every
> >>>>> +              * interval.
> >>>>> +              */
> >>>>> +             if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED)
> >>>>> +                     notify_open = true;
> >>>>> +     }
> >>>>> +
> >>>>> +     local_bh_disable();
> >>>>> +     rcu_read_lock();
> >>>>> +
> >>>>> +     if (!rcu_access_pointer(mrp->p_port) ||
> >>>>> +         !rcu_access_pointer(mrp->s_port))
> >>>>> +             goto out;
> >>>>> +
> >>>>> +     /* Is it possible here to get call to delete the bridge port? If yes
> >>>>> +      * I need to protect it
> >>>>> +      */
> >>>>> +     dev_hold(rcu_dereference(mrp->p_port)->dev);
> >>>>> +
> >>>>
> >>>> This looks all wrong, p_port can become NULL here and you'll deref it.
> >>>
> >>> By disabling the interrupts and taking the rcu read lock, will I not be
> >>> sure that no one can access the p_port?
> >>
> >> No. You should read more about how RCU operates.
> >
> > Yes, I should definetly do that.
> >
> >>
> >>> If is not true, how the p_port can become NULL?
> >>>
> >>
> >> Readers and writer run concurrently.
> >>
> >>>>
> >>>>> +     skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->p_port)->dev,
> >>>>> +                                 BR_MRP_PORT_ROLE_PRIMARY);
> >>>>> +     if (!skb)
> >>>>> +             goto out;
> >>>>> +
> >>>>> +     skb_reset_network_header(skb);
> >>>>> +     dev_queue_xmit(skb);
> >>>>> +
> >>>>> +     if (notify_open && !mrp->ring_role_offloaded)
> >>>>> +             br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, true);
> >>>>> +
> >>>>> +     dev_put(rcu_dereference(mrp->p_port)->dev);
> >>>>> +
> >>>>> +     dev_hold(rcu_dereference(mrp->s_port)->dev);
> >>>>> +
> >>>>
> >>>> same here
> >>>>
> >>>>> +     skb = br_mrp_alloc_test_skb(mrp, rcu_dereference(mrp->s_port)->dev,
> >>>>> +                                 BR_MRP_PORT_ROLE_SECONDARY);
> >>>>> +     if (!skb)
> >>>>> +             goto out;
> >>>>> +
> >>>>> +     skb_reset_network_header(skb);
> >>>>> +     dev_queue_xmit(skb);
> >>>>> +
> >>>>> +     if (notify_open && !mrp->ring_role_offloaded)
> >>>>> +             br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, true);
> >>>>> +
> >>>>> +     dev_put(rcu_dereference(mrp->s_port)->dev);
> >>>>> +
> >>>>> +out:
> >>>>> +     rcu_read_unlock();
> >>>>> +     local_bh_enable();
> >>>>> +
> >>>>> +     queue_delayed_work(system_wq, &mrp->test_work,
> >>>>> +                        usecs_to_jiffies(mrp->test_interval));
> >>>>> +}
> >>>>> +
> >>>>> +/* Adds a new MRP instance.
> >>>>> + * note: called under rtnl_lock
> >>>>> + */
> >>>>> +int br_mrp_add(struct net_bridge *br, struct br_mrp_instance *instance)
> >>>>> +{
> >>>>> +     struct net_bridge_port *p;
> >>>>> +     struct br_mrp *mrp;
> >>>>> +
> >>>>> +     /* If the ring exists, it is not possible to create another one with the
> >>>>> +      * same ring_id
> >>>>> +      */
> >>>>> +     mrp = br_mrp_find_id(br, instance->ring_id);
> >>>>> +     if (mrp)
> >>>>> +             return -EINVAL;
> >>>>> +
> >>>>> +     if (!br_mrp_get_port(br, instance->p_ifindex) ||
> >>>>> +         !br_mrp_get_port(br, instance->s_ifindex))
> >>>>> +             return -EINVAL;
> >>>>> +
> >>>>> +     mrp = devm_kzalloc(&br->dev->dev, sizeof(struct br_mrp), GFP_KERNEL);
> >>>>> +     if (!mrp)
> >>>>> +             return -ENOMEM;
> >>>>> +
> >>>>> +     /* I think is not needed because this can be replaced with rtnl lock*/
> >>>>> +     spin_lock_init(&mrp->lock);
> >>>>> +     spin_lock(&mrp->lock);
> >>>>> +
> >>>>> +     mrp->br = br;
> >>>>
> >>>> Is this field (mrp->br) used anywhere ?
> >>>
> >>> Not anymore. I can remove it in the next patch series.
> >>>
> >>>>
> >>>>> +     mrp->ring_id = instance->ring_id;
> >>>>> +
> >>>>> +     p = br_mrp_get_port(br, instance->p_ifindex);
> >>>>> +     p->state = BR_STATE_FORWARDING;
> >>>>> +     p->flags |= BR_MRP_AWARE;
> >>>>> +     rcu_assign_pointer(mrp->p_port, p);
> >>>>> +
> >>>>> +     p = br_mrp_get_port(br, instance->s_ifindex);
> >>>>> +     p->state = BR_STATE_FORWARDING;
> >>>>> +     p->flags |= BR_MRP_AWARE;
> >>>>> +     rcu_assign_pointer(mrp->s_port, p);
> >>>>> +
> >>>>> +     br_mrp_switchdev_add(mrp);
> >>>>> +
> >>>>> +     spin_unlock(&mrp->lock);
> >>>>> +     synchronize_rcu();
> >>>>
> >>>> Why do you need the synchronize here?
> >>>
> >>> Actually this shouldn't be after the list_add_tail_rcu? Because I am
> >>> thinking that some can read the list at the same time I am change it.
> >>
> >> That doesn't help, rcu primitives are already safe to run concurrently with readers.
> >>
> >>>
> >>>>
> >>>>> +
> >>>>> +     list_add_tail_rcu(&mrp->list, &br->mrp_list);
> >>>>> +     INIT_DELAYED_WORK(&mrp->test_work, br_mrp_test_work_expired);
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +/* Deletes existing MRP instance.
> >>>>> + * note: called under rtnl_lock
> >>>>> + */
> >>>>> +int br_mrp_del(struct net_bridge *br, struct br_mrp_instance *instance)
> >>>>> +{
> >>>>> +     struct br_mrp *mrp = br_mrp_find_id(br, instance->ring_id);
> >>>>> +     struct net_bridge_port *p;
> >>>>> +
> >>>>> +     if (!mrp)
> >>>>> +             return -EINVAL;
> >>>>> +
> >>>>> +     /* Stop sending MRP_Test frames */
> >>>>> +     cancel_delayed_work(&mrp->test_work);
> >>>>
> >>>> cancel_delayed_work_sync() if you'd like to make sure it's stopped and finished (if it was running
> >>>> during this)
> >>>
> >>> Will be fixed in the next patch series.
> >>>>
> >>>>> +     br_mrp_switchdev_send_ring_test(mrp, 0, 0, 0);
> >>>>> +
> >>>>> +     spin_lock(&mrp->lock);
> >>>>> +
> >>>>> +     br_mrp_switchdev_del(mrp);
> >>>>> +
> >>>>> +     /* Reset the ports */
> >>>>> +     p = rcu_dereference_protected(mrp->p_port, lockdep_is_held(&mrp->lock));
> >>>>> +     if (p) {
> >>>>> +             spin_lock(&br->lock);
> >>>>> +             p->state = BR_STATE_FORWARDING;
> >>>>> +             p->flags &= ~BR_MRP_AWARE;
> >>>>> +             br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
> >>>>> +             rcu_assign_pointer(mrp->p_port, NULL);
> >>>>> +             spin_unlock(&br->lock);
> >>>>> +     }
> >>>>> +
> >>>>> +     p = rcu_dereference_protected(mrp->s_port, lockdep_is_held(&mrp->lock));
> >>>>> +     if (p) {
> >>>>> +             spin_lock(&br->lock);
> >>>>> +             p->state = BR_STATE_FORWARDING;
> >>>>> +             p->flags &= ~BR_MRP_AWARE;
> >>>>> +             br_mrp_port_switchdev_set_state(p, BR_STATE_FORWARDING);
> >>>>> +             rcu_assign_pointer(mrp->s_port, NULL);
> >>>>> +             spin_unlock(&br->lock);
> >>>>> +     }
> >>>>> +
> >>>>> +     /* Destroy the ring */
> >>>>> +     mrp->br = NULL;
> >>>>> +
> >>>>> +     spin_unlock(&mrp->lock);
> >>>>> +     synchronize_rcu();
> >>>>> +
> >>>>> +     list_del_rcu(&mrp->list);
> >>>>> +     devm_kfree(&br->dev->dev, mrp);
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +int br_mrp_set_port_state(struct net_bridge_port *p,
> >>>>> +                       enum br_mrp_port_state_type state)
> >>>>> +{
> >>>>> +     spin_lock(&p->br->lock);
> >>>>> +
> >>>>> +     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);
> >>>>> +
> >>>>> +     spin_unlock(&p->br->lock);
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +int br_mrp_set_port_role(struct net_bridge_port *p,
> >>>>> +                      u32 ring_id, enum br_mrp_port_role_type role)
> >>>>> +{
> >>>>> +     struct br_mrp *mrp = br_mrp_find_id(p->br, ring_id);
> >>>>> +
> >>>>> +     if (!mrp)
> >>>>> +             return -EINVAL;
> >>>>> +
> >>>>> +     spin_lock(&mrp->lock);
> >>>>> +
> >>>>> +     if (role == BR_MRP_PORT_ROLE_PRIMARY)
> >>>>> +             rcu_assign_pointer(mrp->p_port, p);
> >>>>> +     if (role == BR_MRP_PORT_ROLE_SECONDARY)
> >>>>> +             rcu_assign_pointer(mrp->s_port, p);
> >>>>> +
> >>>>> +     br_mrp_port_switchdev_set_role(p, role);
> >>>>> +
> >>>>> +     spin_unlock(&mrp->lock);
> >>>>> +     synchronize_rcu();
> >>>>
> >>>> Why do you need to synchronize here?
> >>>
> >>> Actually this is not needed.
> >>>>
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +int br_mrp_set_ring_state(struct net_bridge *br, u32 ring_id,
> >>>>> +                       enum br_mrp_ring_state_type state)
> >>>>> +{
> >>>>> +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
> >>>>> +
> >>>>> +     if (!mrp)
> >>>>> +             return -EINVAL;
> >>>>> +
> >>>>> +     if (mrp->ring_state == BR_MRP_RING_STATE_CLOSED &&
> >>>>> +         state != BR_MRP_RING_STATE_CLOSED)
> >>>>> +             mrp->ring_transitions++;
> >>>>> +
> >>>>> +     mrp->ring_state = state;
> >>>>> +
> >>>>> +     br_mrp_switchdev_set_ring_state(mrp, state);
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +int br_mrp_set_ring_role(struct net_bridge *br, u32 ring_id,
> >>>>> +                      enum br_mrp_ring_role_type role)
> >>>>> +{
> >>>>> +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
> >>>>> +     int err;
> >>>>> +
> >>>>> +     if (!mrp)
> >>>>> +             return -EINVAL;
> >>>>> +
> >>>>> +     mrp->ring_role = role;
> >>>>> +
> >>>>> +     /* If there is an error just bailed out */
> >>>>> +     err = br_mrp_switchdev_set_ring_role(mrp, role);
> >>>>> +     if (err && err != -EOPNOTSUPP)
> >>>>> +             return err;
> >>>>> +
> >>>>> +     /* Now detect if the HW actually applied the role or not. If the HW
> >>>>> +      * applied the role it means that the SW will not to do those operations
> >>>>> +      * anymore. For example if the role ir MRM then the HW will notify the
> >>>>> +      * SW when ring is open, but if the is not pushed to the HW the SW will
> >>>>> +      * need to detect when the ring is open
> >>>>> +      */
> >>>>> +     mrp->ring_role_offloaded = err == -EOPNOTSUPP ? 0 : 1;
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +int br_mrp_start_test(struct net_bridge *br, u32 ring_id, u32 interval,
> >>>>> +                   u8 max_miss, u32 period)
> >>>>> +{
> >>>>> +     struct br_mrp *mrp = br_mrp_find_id(br, ring_id);
> >>>>> +
> >>>>> +     if (!mrp)
> >>>>> +             return -EINVAL;
> >>>>> +
> >>>>> +     /* Try to push is to the HW and if it fails then continue to generate in
> >>>>> +      * SW and if that also fails then return error
> >>>>> +      */
> >>>>> +     if (!br_mrp_switchdev_send_ring_test(mrp, interval, max_miss, period))
> >>>>> +             return 0;
> >>>>> +
> >>>>> +     mrp->test_interval = interval;
> >>>>> +     mrp->test_end = jiffies + usecs_to_jiffies(period);
> >>>>> +     mrp->test_max_miss = max_miss;
> >>>>> +     mrp->test_count_miss = 0;
> >>>>> +     queue_delayed_work(system_wq, &mrp->test_work,
> >>>>> +                        usecs_to_jiffies(interval));
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +/* Process only MRP Test frame. All the other MRP frames are processed by
> >>>>> + * userspace application
> >>>>> + * note: already called with rcu_read_lock
> >>>>> + */
> >>>>> +static void br_mrp_mrm_process(struct br_mrp *mrp, struct sk_buff *skb)
> >>>>> +{
> >>>>> +     struct br_mrp_tlv_hdr *hdr;
> >>>>> +
> >>>>> +     hdr = (struct br_mrp_tlv_hdr *)(skb->data + sizeof(uint16_t));
> >>>>> +
> >>>>> +     if (hdr->type != BR_MRP_TLV_HEADER_RING_TEST)
> >>>>> +             return;
> >>>>> +
> >>>>> +     mrp->test_count_miss = 0;
> >>>>> +
> >>>>> +     br_mrp_port_open(rcu_dereference(mrp->p_port)->dev, false);
> >>>>> +     br_mrp_port_open(rcu_dereference(mrp->s_port)->dev, false);
> >>>>> +}
> >>>>> +
> >>>>> +/* This will just forward the frame to the other mrp ring port(MRC role) or will
> >>>>> + * not do anything.
> >>>>> + * note: already called with rcu_read_lock
> >>>>> + */
> >>>>> +static int br_mrp_rcv(struct net_bridge_port *p,
> >>>>> +                   struct sk_buff *skb, struct net_device *dev)
> >>>>> +{
> >>>>> +     struct net_device *s_dev, *p_dev, *d_dev;
> >>>>> +     struct net_bridge *br;
> >>>>> +     struct sk_buff *nskb;
> >>>>> +     struct br_mrp *mrp;
> >>>>> +
> >>>>> +     /* If port is disable don't accept any frames */
> >>>>> +     if (p->state == BR_STATE_DISABLED)
> >>>>> +             return 0;
> >>>>> +
> >>>>> +     br = p->br;
> >>>>> +     mrp =  br_mrp_find_port(br, p);
> >>>>> +     if (unlikely(!mrp))
> >>>>> +             return 0;
> >>>>> +
> >>>>> +     /* If the role is MRM then don't forward the frames */
> >>>>> +     if (mrp->ring_role == BR_MRP_RING_ROLE_MRM) {
> >>>>> +             br_mrp_mrm_process(mrp, skb);
> >>>>> +             return 1;
> >>>>> +     }
> >>>>> +
> >>>>> +     nskb = skb_clone(skb, GFP_ATOMIC);
> >>>>> +     if (!nskb)
> >>>>> +             return 0;
> >>>>> +
> >>>>> +     p_dev = rcu_dereference(mrp->p_port)->dev;
> >>>>> +     s_dev = rcu_dereference(mrp->s_port)->dev;
> >>>>> +
> >>>>
> >>>> Not safe, could deref null.
> >>>
> >>> Will be fixed in the next patch series.
> >>>
> >>>>
> >>>>> +     if (p_dev == dev)
> >>>>> +             d_dev = s_dev;
> >>>>> +     else
> >>>>> +             d_dev = p_dev;
> >>>>> +
> >>>>> +     nskb->dev = d_dev;
> >>>>> +     skb_push(nskb, ETH_HLEN);
> >>>>> +     dev_queue_xmit(nskb);
> >>>>> +
> >>>>> +     return 1;
> >>>>> +}
> >>>>> +
> >>>>> +int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
> >>>>> +{
> >>>>> +     /* If there is no MRP instance do normal forwarding */
> >>>>> +     if (unlikely(!(p->flags & BR_MRP_AWARE)))
> >>>>
> >>>> Shouldn't this one be likely() ?
> >>>
> >>> Yes, this should be likely.
> >>>>
> >>>>> +             goto out;
> >>>>> +
> >>>>> +     if (unlikely(skb->protocol == htons(ETH_P_MRP)))
> >>>>> +             return br_mrp_rcv(p, skb, p->dev);
> >>>>> +
> >>>>> +out:
> >>>>> +     return 0;
> >>>>> +}
> >>>>>
> >>>>
> >>>
> >>> [1] https://lwn.net/Articles/262464/
> >>> [2] https://www.kernel.org/doc/html/latest/RCU/listRCU.html
> >>>
> >>
> >
> 

-- 
/Horatiu

[RFC net-next v4 8/9] bridge: mrp: Integrate MRP into the bridge

[Horatiu Vultur]

To integrate MRP into the bridge, the bridge needs to do the following:
- add new flag(BR_MPP_AWARE) to the net bridge ports, this bit will be set when
  the port is added to an MRP instance. In this way it knows if the frame was
  received on MRP ring port
- detect if the MRP frame was received on MRP ring port in that case it would be
  processed otherwise just forward it as usual.
- enable parsing of MRP
- before whenever the bridge was set up, it would set all the ports in
  forwarding state. Add an extra check to not set ports in forwarding state if
  the port is an MRP ring port. The reason of this change is that if the MRP
  instance initially sets the port in blocked state by setting the bridge up it
  would overwrite this setting.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 include/linux/if_bridge.h |  1 +
 net/bridge/br_device.c    |  3 +++
 net/bridge/br_input.c     |  3 +++
 net/bridge/br_netlink.c   |  5 +++++
 net/bridge/br_private.h   | 22 ++++++++++++++++++++++
 net/bridge/br_stp.c       |  6 ++++++
 6 files changed, 40 insertions(+)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index 9e57c4411734..10baa9efdae8 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -47,6 +47,7 @@ struct br_ip_list {
 #define BR_BCAST_FLOOD		BIT(14)
 #define BR_NEIGH_SUPPRESS	BIT(15)
 #define BR_ISOLATED		BIT(16)
+#define BR_MRP_AWARE		BIT(17)
 
 #define BR_DEFAULT_AGEING_TIME	(300 * HZ)
 
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 0e3dbc5f3c34..8ec1362588af 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -463,6 +463,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);
+#if IS_ENABLED(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_input.c b/net/bridge/br_input.c
index fcc260840028..d5c34f36f0f4 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -342,6 +342,9 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
 		}
 	}
 
+	if (unlikely(br_mrp_process(p, skb)))
+		return RX_HANDLER_PASS;
+
 forward:
 	switch (p->state) {
 	case BR_STATE_FORWARDING:
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 43dab4066f91..77bc96745be6 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -669,6 +669,11 @@ static int br_afspec(struct net_bridge *br,
 			if (err)
 				return err;
 			break;
+		case IFLA_BRIDGE_MRP:
+			err = br_mrp_parse(br, p, attr, cmd);
+			if (err)
+				return err;
+			break;
 		}
 	}
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1f97703a52ff..38894f2cf98f 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -428,6 +428,10 @@ struct net_bridge {
 	int offload_fwd_mark;
 #endif
 	struct hlist_head		fdb_list;
+
+#if IS_ENABLED(CONFIG_BRIDGE_MRP)
+	struct list_head		__rcu mrp_list;
+#endif
 };
 
 struct br_input_skb_cb {
@@ -1304,6 +1308,24 @@ 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 */
+#if IS_ENABLED(CONFIG_BRIDGE_MRP)
+int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
+		 struct nlattr *attr, int cmd);
+int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb);
+#else
+static inline int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
+			       struct nlattr *attr, int cmd)
+{
+	return -1;
+}
+
+static inline int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
+{
+	return -1;
+}
+#endif
+
 /* br_netlink.c */
 extern struct rtnl_link_ops br_link_ops;
 int br_netlink_init(void);
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 1f14b8455345..3e88be7aa269 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -36,6 +36,12 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
 	};
 	int err;
 
+	/* Don't change the state of the ports if they are driven by a different
+	 * protocol.
+	 */
+	if (p->flags & BR_MRP_AWARE)
+		return;
+
 	p->state = state;
 	err = switchdev_port_attr_set(p->dev, &attr);
 	if (err && err != -EOPNOTSUPP)
-- 
2.17.1

Re: [RFC net-next v4 8/9] bridge: mrp: Integrate MRP into the bridge

[Nikolay Aleksandrov]

On 27/03/2020 11:21, Horatiu Vultur wrote:
> To integrate MRP into the bridge, the bridge needs to do the following:
> - add new flag(BR_MPP_AWARE) to the net bridge ports, this bit will be set when
>   the port is added to an MRP instance. In this way it knows if the frame was
>   received on MRP ring port
> - detect if the MRP frame was received on MRP ring port in that case it would be
>   processed otherwise just forward it as usual.
> - enable parsing of MRP
> - before whenever the bridge was set up, it would set all the ports in
>   forwarding state. Add an extra check to not set ports in forwarding state if
>   the port is an MRP ring port. The reason of this change is that if the MRP
>   instance initially sets the port in blocked state by setting the bridge up it
>   would overwrite this setting.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  include/linux/if_bridge.h |  1 +
>  net/bridge/br_device.c    |  3 +++
>  net/bridge/br_input.c     |  3 +++
>  net/bridge/br_netlink.c   |  5 +++++
>  net/bridge/br_private.h   | 22 ++++++++++++++++++++++
>  net/bridge/br_stp.c       |  6 ++++++
>  6 files changed, 40 insertions(+)
> 
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index 9e57c4411734..10baa9efdae8 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -47,6 +47,7 @@ struct br_ip_list {
>  #define BR_BCAST_FLOOD		BIT(14)
>  #define BR_NEIGH_SUPPRESS	BIT(15)
>  #define BR_ISOLATED		BIT(16)
> +#define BR_MRP_AWARE		BIT(17)
>  
>  #define BR_DEFAULT_AGEING_TIME	(300 * HZ)
>  
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 0e3dbc5f3c34..8ec1362588af 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -463,6 +463,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);
> +#if IS_ENABLED(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_input.c b/net/bridge/br_input.c
> index fcc260840028..d5c34f36f0f4 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -342,6 +342,9 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
>  		}
>  	}
>  
> +	if (unlikely(br_mrp_process(p, skb)))
> +		return RX_HANDLER_PASS;
> +
>  forward:
>  	switch (p->state) {
>  	case BR_STATE_FORWARDING:
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 43dab4066f91..77bc96745be6 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -669,6 +669,11 @@ static int br_afspec(struct net_bridge *br,
>  			if (err)
>  				return err;
>  			break;
> +		case IFLA_BRIDGE_MRP:
> +			err = br_mrp_parse(br, p, attr, cmd);
> +			if (err)
> +				return err;
> +			break;
>  		}
>  	}
>  
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 1f97703a52ff..38894f2cf98f 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -428,6 +428,10 @@ struct net_bridge {
>  	int offload_fwd_mark;
>  #endif
>  	struct hlist_head		fdb_list;
> +
> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> +	struct list_head		__rcu mrp_list;
> +#endif
>  };
>  
>  struct br_input_skb_cb {
> @@ -1304,6 +1308,24 @@ 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 */
> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> +int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
> +		 struct nlattr *attr, int cmd);
> +int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb);
> +#else
> +static inline int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
> +			       struct nlattr *attr, int cmd)
> +{
> +	return -1;

You should return proper error here.

> +}
> +
> +static inline int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
> +{
> +	return -1;

The bridge can't possibly work with MRP disabled with this.

> +}
> +#endif
> +
>  /* br_netlink.c */
>  extern struct rtnl_link_ops br_link_ops;
>  int br_netlink_init(void);
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index 1f14b8455345..3e88be7aa269 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -36,6 +36,12 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
>  	};
>  	int err;
>  
> +	/* Don't change the state of the ports if they are driven by a different
> +	 * protocol.
> +	 */
> +	if (p->flags & BR_MRP_AWARE)
> +		return;
> +

Maybe disallow STP type (kernel/user-space/no-stp) changing as well, force it to no-stp.

>  	p->state = state;
>  	err = switchdev_port_attr_set(p->dev, &attr);
>  	if (err && err != -EOPNOTSUPP)
> 

Re: [RFC net-next v4 8/9] bridge: mrp: Integrate MRP into the bridge

[Horatiu Vultur]

Hi Nik,

The 03/30/2020 19:16, Nikolay Aleksandrov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 27/03/2020 11:21, Horatiu Vultur wrote:
> > To integrate MRP into the bridge, the bridge needs to do the following:
> > - add new flag(BR_MPP_AWARE) to the net bridge ports, this bit will be set when
> >   the port is added to an MRP instance. In this way it knows if the frame was
> >   received on MRP ring port
> > - detect if the MRP frame was received on MRP ring port in that case it would be
> >   processed otherwise just forward it as usual.
> > - enable parsing of MRP
> > - before whenever the bridge was set up, it would set all the ports in
> >   forwarding state. Add an extra check to not set ports in forwarding state if
> >   the port is an MRP ring port. The reason of this change is that if the MRP
> >   instance initially sets the port in blocked state by setting the bridge up it
> >   would overwrite this setting.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  include/linux/if_bridge.h |  1 +
> >  net/bridge/br_device.c    |  3 +++
> >  net/bridge/br_input.c     |  3 +++
> >  net/bridge/br_netlink.c   |  5 +++++
> >  net/bridge/br_private.h   | 22 ++++++++++++++++++++++
> >  net/bridge/br_stp.c       |  6 ++++++
> >  6 files changed, 40 insertions(+)
> >
> > diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> > index 9e57c4411734..10baa9efdae8 100644
> > --- a/include/linux/if_bridge.h
> > +++ b/include/linux/if_bridge.h
> > @@ -47,6 +47,7 @@ struct br_ip_list {
> >  #define BR_BCAST_FLOOD               BIT(14)
> >  #define BR_NEIGH_SUPPRESS    BIT(15)
> >  #define BR_ISOLATED          BIT(16)
> > +#define BR_MRP_AWARE         BIT(17)
> >
> >  #define BR_DEFAULT_AGEING_TIME       (300 * HZ)
> >
> > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> > index 0e3dbc5f3c34..8ec1362588af 100644
> > --- a/net/bridge/br_device.c
> > +++ b/net/bridge/br_device.c
> > @@ -463,6 +463,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);
> > +#if IS_ENABLED(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_input.c b/net/bridge/br_input.c
> > index fcc260840028..d5c34f36f0f4 100644
> > --- a/net/bridge/br_input.c
> > +++ b/net/bridge/br_input.c
> > @@ -342,6 +342,9 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >               }
> >       }
> >
> > +     if (unlikely(br_mrp_process(p, skb)))
> > +             return RX_HANDLER_PASS;
> > +
> >  forward:
> >       switch (p->state) {
> >       case BR_STATE_FORWARDING:
> > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> > index 43dab4066f91..77bc96745be6 100644
> > --- a/net/bridge/br_netlink.c
> > +++ b/net/bridge/br_netlink.c
> > @@ -669,6 +669,11 @@ static int br_afspec(struct net_bridge *br,
> >                       if (err)
> >                               return err;
> >                       break;
> > +             case IFLA_BRIDGE_MRP:
> > +                     err = br_mrp_parse(br, p, attr, cmd);
> > +                     if (err)
> > +                             return err;
> > +                     break;
> >               }
> >       }
> >
> > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> > index 1f97703a52ff..38894f2cf98f 100644
> > --- a/net/bridge/br_private.h
> > +++ b/net/bridge/br_private.h
> > @@ -428,6 +428,10 @@ struct net_bridge {
> >       int offload_fwd_mark;
> >  #endif
> >       struct hlist_head               fdb_list;
> > +
> > +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> > +     struct list_head                __rcu mrp_list;
> > +#endif
> >  };
> >
> >  struct br_input_skb_cb {
> > @@ -1304,6 +1308,24 @@ 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 */
> > +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> > +int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
> > +              struct nlattr *attr, int cmd);
> > +int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb);
> > +#else
> > +static inline int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
> > +                            struct nlattr *attr, int cmd)
> > +{
> > +     return -1;
> 
> You should return proper error here.

It will return -EOPNOTSUPP.

> 
> > +}
> > +
> > +static inline int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
> > +{
> > +     return -1;
> 
> The bridge can't possibly work with MRP disabled with this.

Good catch, it will return 0.

> 
> > +}
> > +#endif
> > +
> >  /* br_netlink.c */
> >  extern struct rtnl_link_ops br_link_ops;
> >  int br_netlink_init(void);
> > diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> > index 1f14b8455345..3e88be7aa269 100644
> > --- a/net/bridge/br_stp.c
> > +++ b/net/bridge/br_stp.c
> > @@ -36,6 +36,12 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
> >       };
> >       int err;
> >
> > +     /* Don't change the state of the ports if they are driven by a different
> > +      * protocol.
> > +      */
> > +     if (p->flags & BR_MRP_AWARE)
> > +             return;
> > +
> 
> Maybe disallow STP type (kernel/user-space/no-stp) changing as well, force it to no-stp.

I am not sure that I understand completely here, do you want me to
disable STP if MRP is started?

> 
> >       p->state = state;
> >       err = switchdev_port_attr_set(p->dev, &attr);
> >       if (err && err != -EOPNOTSUPP)
> >
> 

-- 
/Horatiu

Re: [RFC net-next v4 8/9] bridge: mrp: Integrate MRP into the bridge

[Nikolay Aleksandrov]

On 01/04/2020 19:10, Horatiu Vultur wrote:
> Hi Nik,
> 
> The 03/30/2020 19:16, Nikolay Aleksandrov wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 27/03/2020 11:21, Horatiu Vultur wrote:
>>> To integrate MRP into the bridge, the bridge needs to do the following:
>>> - add new flag(BR_MPP_AWARE) to the net bridge ports, this bit will be set when
>>>   the port is added to an MRP instance. In this way it knows if the frame was
>>>   received on MRP ring port
>>> - detect if the MRP frame was received on MRP ring port in that case it would be
>>>   processed otherwise just forward it as usual.
>>> - enable parsing of MRP
>>> - before whenever the bridge was set up, it would set all the ports in
>>>   forwarding state. Add an extra check to not set ports in forwarding state if
>>>   the port is an MRP ring port. The reason of this change is that if the MRP
>>>   instance initially sets the port in blocked state by setting the bridge up it
>>>   would overwrite this setting.
>>>
>>> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
>>> ---
>>>  include/linux/if_bridge.h |  1 +
>>>  net/bridge/br_device.c    |  3 +++
>>>  net/bridge/br_input.c     |  3 +++
>>>  net/bridge/br_netlink.c   |  5 +++++
>>>  net/bridge/br_private.h   | 22 ++++++++++++++++++++++
>>>  net/bridge/br_stp.c       |  6 ++++++
>>>  6 files changed, 40 insertions(+)
>>>
>>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>>> index 9e57c4411734..10baa9efdae8 100644
>>> --- a/include/linux/if_bridge.h
>>> +++ b/include/linux/if_bridge.h
>>> @@ -47,6 +47,7 @@ struct br_ip_list {
>>>  #define BR_BCAST_FLOOD               BIT(14)
>>>  #define BR_NEIGH_SUPPRESS    BIT(15)
>>>  #define BR_ISOLATED          BIT(16)
>>> +#define BR_MRP_AWARE         BIT(17)
>>>
>>>  #define BR_DEFAULT_AGEING_TIME       (300 * HZ)
>>>
>>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>>> index 0e3dbc5f3c34..8ec1362588af 100644
>>> --- a/net/bridge/br_device.c
>>> +++ b/net/bridge/br_device.c
>>> @@ -463,6 +463,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);
>>> +#if IS_ENABLED(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_input.c b/net/bridge/br_input.c
>>> index fcc260840028..d5c34f36f0f4 100644
>>> --- a/net/bridge/br_input.c
>>> +++ b/net/bridge/br_input.c
>>> @@ -342,6 +342,9 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
>>>               }
>>>       }
>>>
>>> +     if (unlikely(br_mrp_process(p, skb)))
>>> +             return RX_HANDLER_PASS;
>>> +
>>>  forward:
>>>       switch (p->state) {
>>>       case BR_STATE_FORWARDING:
>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>> index 43dab4066f91..77bc96745be6 100644
>>> --- a/net/bridge/br_netlink.c
>>> +++ b/net/bridge/br_netlink.c
>>> @@ -669,6 +669,11 @@ static int br_afspec(struct net_bridge *br,
>>>                       if (err)
>>>                               return err;
>>>                       break;
>>> +             case IFLA_BRIDGE_MRP:
>>> +                     err = br_mrp_parse(br, p, attr, cmd);
>>> +                     if (err)
>>> +                             return err;
>>> +                     break;
>>>               }
>>>       }
>>>
>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>> index 1f97703a52ff..38894f2cf98f 100644
>>> --- a/net/bridge/br_private.h
>>> +++ b/net/bridge/br_private.h
>>> @@ -428,6 +428,10 @@ struct net_bridge {
>>>       int offload_fwd_mark;
>>>  #endif
>>>       struct hlist_head               fdb_list;
>>> +
>>> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
>>> +     struct list_head                __rcu mrp_list;
>>> +#endif
>>>  };
>>>
>>>  struct br_input_skb_cb {
>>> @@ -1304,6 +1308,24 @@ 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 */
>>> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
>>> +int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
>>> +              struct nlattr *attr, int cmd);
>>> +int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb);
>>> +#else
>>> +static inline int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
>>> +                            struct nlattr *attr, int cmd)
>>> +{
>>> +     return -1;
>>
>> You should return proper error here.
> 
> It will return -EOPNOTSUPP.
> 
>>
>>> +}
>>> +
>>> +static inline int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
>>> +{
>>> +     return -1;
>>
>> The bridge can't possibly work with MRP disabled with this.
> 
> Good catch, it will return 0.
> 
>>
>>> +}
>>> +#endif
>>> +
>>>  /* br_netlink.c */
>>>  extern struct rtnl_link_ops br_link_ops;
>>>  int br_netlink_init(void);
>>> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
>>> index 1f14b8455345..3e88be7aa269 100644
>>> --- a/net/bridge/br_stp.c
>>> +++ b/net/bridge/br_stp.c
>>> @@ -36,6 +36,12 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
>>>       };
>>>       int err;
>>>
>>> +     /* Don't change the state of the ports if they are driven by a different
>>> +      * protocol.
>>> +      */
>>> +     if (p->flags & BR_MRP_AWARE)
>>> +             return;
>>> +
>>
>> Maybe disallow STP type (kernel/user-space/no-stp) changing as well, force it to no-stp.
> 
> I am not sure that I understand completely here, do you want me to
> disable STP if MRP is started?
> 

I think it should be one of the two since they can step on each other's toes. If MRP is
enabled then STP must be already disabled and should not be possible to enable while MRP
is active and vice-versa.

>>
>>>       p->state = state;
>>>       err = switchdev_port_attr_set(p->dev, &attr);
>>>       if (err && err != -EOPNOTSUPP)
>>>
>>
> 

Re: [RFC net-next v4 8/9] bridge: mrp: Integrate MRP into the bridge

[Horatiu Vultur]

The 04/01/2020 19:33, Nikolay Aleksandrov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 01/04/2020 19:10, Horatiu Vultur wrote:
> > Hi Nik,
> >
> > The 03/30/2020 19:16, Nikolay Aleksandrov wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>
> >> On 27/03/2020 11:21, Horatiu Vultur wrote:
> >>> To integrate MRP into the bridge, the bridge needs to do the following:
> >>> - add new flag(BR_MPP_AWARE) to the net bridge ports, this bit will be set when
> >>>   the port is added to an MRP instance. In this way it knows if the frame was
> >>>   received on MRP ring port
> >>> - detect if the MRP frame was received on MRP ring port in that case it would be
> >>>   processed otherwise just forward it as usual.
> >>> - enable parsing of MRP
> >>> - before whenever the bridge was set up, it would set all the ports in
> >>>   forwarding state. Add an extra check to not set ports in forwarding state if
> >>>   the port is an MRP ring port. The reason of this change is that if the MRP
> >>>   instance initially sets the port in blocked state by setting the bridge up it
> >>>   would overwrite this setting.
> >>>
> >>> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> >>> ---
> >>>  include/linux/if_bridge.h |  1 +
> >>>  net/bridge/br_device.c    |  3 +++
> >>>  net/bridge/br_input.c     |  3 +++
> >>>  net/bridge/br_netlink.c   |  5 +++++
> >>>  net/bridge/br_private.h   | 22 ++++++++++++++++++++++
> >>>  net/bridge/br_stp.c       |  6 ++++++
> >>>  6 files changed, 40 insertions(+)
> >>>
> >>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> >>> index 9e57c4411734..10baa9efdae8 100644
> >>> --- a/include/linux/if_bridge.h
> >>> +++ b/include/linux/if_bridge.h
> >>> @@ -47,6 +47,7 @@ struct br_ip_list {
> >>>  #define BR_BCAST_FLOOD               BIT(14)
> >>>  #define BR_NEIGH_SUPPRESS    BIT(15)
> >>>  #define BR_ISOLATED          BIT(16)
> >>> +#define BR_MRP_AWARE         BIT(17)
> >>>
> >>>  #define BR_DEFAULT_AGEING_TIME       (300 * HZ)
> >>>
> >>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> >>> index 0e3dbc5f3c34..8ec1362588af 100644
> >>> --- a/net/bridge/br_device.c
> >>> +++ b/net/bridge/br_device.c
> >>> @@ -463,6 +463,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);
> >>> +#if IS_ENABLED(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_input.c b/net/bridge/br_input.c
> >>> index fcc260840028..d5c34f36f0f4 100644
> >>> --- a/net/bridge/br_input.c
> >>> +++ b/net/bridge/br_input.c
> >>> @@ -342,6 +342,9 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >>>               }
> >>>       }
> >>>
> >>> +     if (unlikely(br_mrp_process(p, skb)))
> >>> +             return RX_HANDLER_PASS;
> >>> +
> >>>  forward:
> >>>       switch (p->state) {
> >>>       case BR_STATE_FORWARDING:
> >>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> >>> index 43dab4066f91..77bc96745be6 100644
> >>> --- a/net/bridge/br_netlink.c
> >>> +++ b/net/bridge/br_netlink.c
> >>> @@ -669,6 +669,11 @@ static int br_afspec(struct net_bridge *br,
> >>>                       if (err)
> >>>                               return err;
> >>>                       break;
> >>> +             case IFLA_BRIDGE_MRP:
> >>> +                     err = br_mrp_parse(br, p, attr, cmd);
> >>> +                     if (err)
> >>> +                             return err;
> >>> +                     break;
> >>>               }
> >>>       }
> >>>
> >>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >>> index 1f97703a52ff..38894f2cf98f 100644
> >>> --- a/net/bridge/br_private.h
> >>> +++ b/net/bridge/br_private.h
> >>> @@ -428,6 +428,10 @@ struct net_bridge {
> >>>       int offload_fwd_mark;
> >>>  #endif
> >>>       struct hlist_head               fdb_list;
> >>> +
> >>> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> >>> +     struct list_head                __rcu mrp_list;
> >>> +#endif
> >>>  };
> >>>
> >>>  struct br_input_skb_cb {
> >>> @@ -1304,6 +1308,24 @@ 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 */
> >>> +#if IS_ENABLED(CONFIG_BRIDGE_MRP)
> >>> +int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
> >>> +              struct nlattr *attr, int cmd);
> >>> +int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb);
> >>> +#else
> >>> +static inline int br_mrp_parse(struct net_bridge *br, struct net_bridge_port *p,
> >>> +                            struct nlattr *attr, int cmd)
> >>> +{
> >>> +     return -1;
> >>
> >> You should return proper error here.
> >
> > It will return -EOPNOTSUPP.
> >
> >>
> >>> +}
> >>> +
> >>> +static inline int br_mrp_process(struct net_bridge_port *p, struct sk_buff *skb)
> >>> +{
> >>> +     return -1;
> >>
> >> The bridge can't possibly work with MRP disabled with this.
> >
> > Good catch, it will return 0.
> >
> >>
> >>> +}
> >>> +#endif
> >>> +
> >>>  /* br_netlink.c */
> >>>  extern struct rtnl_link_ops br_link_ops;
> >>>  int br_netlink_init(void);
> >>> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> >>> index 1f14b8455345..3e88be7aa269 100644
> >>> --- a/net/bridge/br_stp.c
> >>> +++ b/net/bridge/br_stp.c
> >>> @@ -36,6 +36,12 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
> >>>       };
> >>>       int err;
> >>>
> >>> +     /* Don't change the state of the ports if they are driven by a different
> >>> +      * protocol.
> >>> +      */
> >>> +     if (p->flags & BR_MRP_AWARE)
> >>> +             return;
> >>> +
> >>
> >> Maybe disallow STP type (kernel/user-space/no-stp) changing as well, force it to no-stp.
> >
> > I am not sure that I understand completely here, do you want me to
> > disable STP if MRP is started?
> >
> 
> I think it should be one of the two since they can step on each other's toes. If MRP is
> enabled then STP must be already disabled and should not be possible to enable while MRP
> is active and vice-versa.

I will update this in the next patch series.

> 
> >>
> >>>       p->state = state;
> >>>       err = switchdev_port_attr_set(p->dev, &attr);
> >>>       if (err && err != -EOPNOTSUPP)
> >>>
> >>
> >
> 

-- 
/Horatiu

[RFC net-next v4 9/9] 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 49da7ae6f077..9bf3e1be3328 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 br_vlan_opt
 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 v4 0/9] net: bridge: mrp: Add support for Media Redundancy Protocol(MRP)

[Nikolay Aleksandrov]

On 27/03/2020 11:21, 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][3], the MRP state machine and all the timers
> were moved to userspace, except for the timers used to generate MRP Test frames.
> In this way the userspace doesn't know and should not know if the HW or the
> kernel will generate the MRP Test frames. The following changes were added to
> the bridge to support the MRP:
> - the existing netlink interface was extended with MRP support,
> - allow to detect when a MRP frame was received on a MRP ring port
> - allow MRP instance to forward/terminate MRP frames
> - generate MRP Test frames in case the HW doesn't have support for this
> 
> To be able to offload MRP support to HW, the switchdev API  was extend.
> 
> With these changes the userspace doesn't do the following because already the
> kernel/HW will do:
> - doesn't need to forward/terminate MRP frames
> - doesn't need to generate MRP Test frames
> - doesn't need to detect when the ring is open/closed.
> 
> The userspace application that is using the new netlink can be found here[4].
> 

Hi Horatiu,
One issue in general - some functions are used before they're defined (the switchdev
API integration ones) patch 4 vs 7 which doesn't make sense. Also I see that the BRIDGE_MRP is used
(ifdef) before it's added to the Kconfig which doesn't make much sense either.
I think you should rearrange the patches and maybe combine some of them.

Thanks,
 Nik

Re: [RFC net-next v4 0/9] net: bridge: mrp: Add support for Media Redundancy Protocol(MRP)

[Horatiu Vultur]

The 03/30/2020 19:21, Nikolay Aleksandrov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 27/03/2020 11:21, 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][3], the MRP state machine and all the timers
> > were moved to userspace, except for the timers used to generate MRP Test frames.
> > In this way the userspace doesn't know and should not know if the HW or the
> > kernel will generate the MRP Test frames. The following changes were added to
> > the bridge to support the MRP:
> > - the existing netlink interface was extended with MRP support,
> > - allow to detect when a MRP frame was received on a MRP ring port
> > - allow MRP instance to forward/terminate MRP frames
> > - generate MRP Test frames in case the HW doesn't have support for this
> >
> > To be able to offload MRP support to HW, the switchdev API  was extend.
> >
> > With these changes the userspace doesn't do the following because already the
> > kernel/HW will do:
> > - doesn't need to forward/terminate MRP frames
> > - doesn't need to generate MRP Test frames
> > - doesn't need to detect when the ring is open/closed.
> >
> > The userspace application that is using the new netlink can be found here[4].
> >
> 
> Hi Horatiu,

Hi Nik,

> One issue in general - some functions are used before they're defined (the switchdev
> API integration ones) patch 4 vs 7 which doesn't make sense. Also I see that the BRIDGE_MRP is used
> (ifdef) before it's added to the Kconfig which doesn't make much sense either.
> I think you should rearrange the patches and maybe combine some of them.

Thanks for the feedback, in the next patch series I will make sure that
everything is defined before it is used.


> 
> Thanks,
>  Nik
> 

-- 
/Horatiu