2020-01-13 12:48:30

by Horatiu Vultur

[permalink] [raw]
Subject: [RFC net-next Patch v2 0/4] net: bridge: mrp: Add support for Media Redundancy Protocol(MRP)

Based on the discussion on the first RFC[1], we have created a new RFC showing
what we were expecting to offload to HW.

This patch series contains the same patches plus another one which adds MRP
support to switchdev. MRP can now offload to HW the process of sending and
terminating MRP_Test frames. But based on the discussions from the previous
version, we decided to try to implement this in user space and extend bridge
netlink interface to be able to offload to HW the creation, sending and the
termination of the MRP_Test frames. Therefor this patch series is more like a
future reference.

We were thinking to extend the bridge netlink in such a way to be able to
offload to HW using switchdev. We were thinking to extend the families IFLA_BR_
and IFLA_BRPORT to add MRP support. Do you think that would be OK? Or should we
create a new family for the MRP?

changes from V2:
- Extend the patch series with another patch. The new patches extends switchdev
interface for MRP. MRP will offload to HW the creating and sending of the
MRP_Test frames.

[1] https://www.spinics.net/lists/netdev/msg623647.html

Horatiu Vultur (4):
net: bridge: mrp: Add support for Media Redundancy Protocol
net: bridge: mrp: Integrate MRP into the bridge
net: bridge: mrp: Add netlink support to configure MRP
net: bridge: mrp: switchdev: Add HW offload

include/net/switchdev.h | 52 ++
include/uapi/linux/if_bridge.h | 27 +
include/uapi/linux/if_ether.h | 1 +
include/uapi/linux/rtnetlink.h | 7 +
net/bridge/Kconfig | 12 +
net/bridge/Makefile | 2 +
net/bridge/br.c | 19 +
net/bridge/br_device.c | 3 +
net/bridge/br_forward.c | 1 +
net/bridge/br_if.c | 10 +
net/bridge/br_input.c | 22 +
net/bridge/br_mrp.c | 1543 ++++++++++++++++++++++++++++++++
net/bridge/br_mrp_switchdev.c | 180 ++++
net/bridge/br_mrp_timer.c | 258 ++++++
net/bridge/br_netlink.c | 9 +
net/bridge/br_private.h | 30 +
net/bridge/br_private_mrp.h | 224 +++++
security/selinux/nlmsgtab.c | 5 +-
18 files changed, 2404 insertions(+), 1 deletion(-)
create mode 100644 net/bridge/br_mrp.c
create mode 100644 net/bridge/br_mrp_switchdev.c
create mode 100644 net/bridge/br_mrp_timer.c
create mode 100644 net/bridge/br_private_mrp.h

--
2.17.1


2020-01-13 12:48:37

by Horatiu Vultur

[permalink] [raw]
Subject: [RFC net-next Patch v2 2/4] net: bridge: mrp: Integrate MRP into the bridge

To integrate MRP into the bridge, the bridge needs to do the following:
- notify when the link of one of the ports goes down or up, because MRP instance
needs to react to link changes by sending MRP frames.
- notify when one of the ports are removed from the bridge or when the bridge
is destroyed, because if the port is part of the MRP ring then MRP state
machine should be stopped.
- add a handler to allow MRP instance to process MRP frames, if MRP is enabled.
This is similar with STP design.
- add logic for MRP frames inside the bridge. The bridge will just detect MRP
frames and it would forward them to the upper layer to allow to process it.
- update the logic to update non-MRP frames. If MRP is enabled, then look at
the state of the port to decide to forward or not.

Signed-off-by: Horatiu Vultur <[email protected]>
---
net/bridge/br.c | 19 +++++++++++++++++++
net/bridge/br_device.c | 3 +++
net/bridge/br_forward.c | 1 +
net/bridge/br_if.c | 10 ++++++++++
net/bridge/br_input.c | 22 ++++++++++++++++++++++
net/bridge/br_private.h | 28 ++++++++++++++++++++++++++++
6 files changed, 83 insertions(+)

diff --git a/net/bridge/br.c b/net/bridge/br.c
index b6fe30e3768f..9053378ca1e4 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -94,6 +94,7 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
spin_lock_bh(&br->lock);
if (br->dev->flags & IFF_UP) {
br_stp_disable_port(p);
+ br_mrp_port_link_change(p, false);
notified = true;
}
spin_unlock_bh(&br->lock);
@@ -103,6 +104,7 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
if (netif_running(br->dev) && netif_oper_up(dev)) {
spin_lock_bh(&br->lock);
br_stp_enable_port(p);
+ br_mrp_port_link_change(p, true);
notified = true;
spin_unlock_bh(&br->lock);
}
@@ -308,6 +310,13 @@ static const struct stp_proto br_stp_proto = {
.rcv = br_stp_rcv,
};

+#if IS_ENABLED(CONFIG_BRIDGE_MRP)
+static struct packet_type mrp_packet_type __read_mostly = {
+ .type = cpu_to_be16(ETH_P_MRP),
+ .func = br_mrp_recv,
+};
+#endif
+
static int __init br_init(void)
{
int err;
@@ -320,6 +329,13 @@ static int __init br_init(void)
return err;
}

+#if IS_ENABLED(CONFIG_BRIDGE_MRP)
+ /* Allow all MRP frames to be processed by the upper layer. The MRP
+ * frames can be dropped or forward on other MRP ports
+ */
+ dev_add_pack(&mrp_packet_type);
+#endif
+
err = br_fdb_init();
if (err)
goto err_out;
@@ -376,6 +392,9 @@ static int __init br_init(void)
static void __exit br_deinit(void)
{
stp_proto_unregister(&br_stp_proto);
+#if IS_ENABLED(CONFIG_BRIDGE_MRP)
+ dev_remove_pack(&mrp_packet_type);
+#endif
br_netlink_fini();
unregister_switchdev_notifier(&br_switchdev_notifier);
unregister_netdevice_notifier(&br_device_notifier);
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index fb38add21b37..29966754d86a 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -464,6 +464,9 @@ void br_dev_setup(struct net_device *dev)
spin_lock_init(&br->lock);
INIT_LIST_HEAD(&br->port_list);
INIT_HLIST_HEAD(&br->fdb_list);
+#ifdef CONFIG_BRIDGE_MRP
+ INIT_LIST_HEAD(&br->mrp_list);
+#endif
spin_lock_init(&br->hash_lock);

br->bridge_id.prio[0] = 0x80;
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index 86637000f275..306425bc9899 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -27,6 +27,7 @@ static inline int should_deliver(const struct net_bridge_port *p,
return ((p->flags & BR_HAIRPIN_MODE) || skb->dev != p->dev) &&
br_allowed_egress(vg, skb) && p->state == BR_STATE_FORWARDING &&
nbp_switchdev_allowed_egress(p, skb) &&
+ br_mrp_allow_egress(p, skb) &&
!br_skb_isolated(p, skb);
}

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 4fe30b182ee7..bf7a467b5f33 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -80,11 +80,13 @@ void br_port_carrier_check(struct net_bridge_port *p, bool *notified)
br_stp_enable_port(p);
*notified = true;
}
+ br_mrp_port_link_change(p, true);
} else {
if (p->state != BR_STATE_DISABLED) {
br_stp_disable_port(p);
*notified = true;
}
+ br_mrp_port_link_change(p, false);
}
spin_unlock_bh(&br->lock);
}
@@ -331,6 +333,9 @@ static void del_nbp(struct net_bridge_port *p)

spin_lock_bh(&br->lock);
br_stp_disable_port(p);
+#ifdef CONFIG_BRIDGE_MRP
+ br_mrp_port_uninit(p);
+#endif
spin_unlock_bh(&br->lock);

br_ifinfo_notify(RTM_DELLINK, NULL, p);
@@ -373,6 +378,11 @@ void br_dev_delete(struct net_device *dev, struct list_head *head)
del_nbp(p);
}

+#ifdef CONFIG_BRIDGE_MRP
+ /* Remove MRP instance. This function will remove also the MRP ports */
+ br_mrp_uninit(br);
+#endif
+
br_recalculate_neigh_suppress_enabled(br);

br_fdb_delete_by_port(br, NULL, 0, 1);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 8944ceb47fe9..c65049586dbd 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -21,6 +21,9 @@
#include <linux/rculist.h>
#include "br_private.h"
#include "br_private_tunnel.h"
+#ifdef CONFIG_BRIDGE_MRP
+#include "br_private_mrp.h"
+#endif

static int
br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb)
@@ -338,6 +341,25 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
return RX_HANDLER_CONSUMED;
}
}
+#ifdef CONFIG_BRIDGE_MRP
+ /* If the port is part of the MRP ring and the state of the port is
+ * disabled then all the frames must be dropped
+ */
+ if (p->mrp_port && p->mrp_port->state == BR_MRP_PORT_STATE_DISABLED)
+ goto drop;
+
+ /* MRP frames need special processing, therefor allow the upper level
+ * to decide what to do with the frame
+ */
+ if (p->mrp_port && skb->protocol == ntohs(ETH_P_MRP))
+ return RX_HANDLER_PASS;
+
+ /* Frames received on a blocked port, shall be dropped, except
+ * MRP frames and frames specified in IEEE 802.1D-2004 Table 7-10.
+ */
+ if (p->mrp_port && p->mrp_port->state == BR_MRP_PORT_STATE_BLOCKED)
+ goto drop;
+#endif

forward:
switch (p->state) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f540f3bdf294..0c008b3d24cc 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -285,6 +285,9 @@ struct net_bridge_port {
u16 backup_redirected_cnt;

struct bridge_stp_xstats stp_xstats;
+#ifdef CONFIG_BRIDGE_MRP
+ struct br_mrp_port *mrp_port;
+#endif
};

#define kobj_to_brport(obj) container_of(obj, struct net_bridge_port, kobj)
@@ -424,6 +427,10 @@ struct net_bridge {
int offload_fwd_mark;
#endif
struct hlist_head fdb_list;
+
+#ifdef CONFIG_BRIDGE_MRP
+ struct list_head mrp_list;
+#endif
};

struct br_input_skb_cb {
@@ -1160,6 +1167,27 @@ void br_stp_timer_init(struct net_bridge *br);
void br_stp_port_timer_init(struct net_bridge_port *p);
unsigned long br_timer_value(const struct timer_list *timer);

+#if IS_ENABLED(CONFIG_BRIDGE_MRP)
+/* br_mrp.c */
+void br_mrp_uninit(struct net_bridge *br);
+void br_mrp_port_uninit(struct net_bridge_port *p);
+void br_mrp_port_link_change(struct net_bridge_port *br, bool up);
+int br_mrp_recv(struct sk_buff *skb, struct net_device *dev,
+ struct packet_type *pt, struct net_device *orig_dev);
+bool br_mrp_allow_egress(const struct net_bridge_port *p,
+ const struct sk_buff *skb);
+#else
+static inline bool br_mrp_allow_egress(const struct net_bridge_port *p,
+ const struct sk_buff *skb)
+{
+ return true;
+}
+
+static inline void br_mrp_port_link_change(struct net_bridge_port *br, bool up)
+{
+}
+#endif
+
/* br.c */
#if IS_ENABLED(CONFIG_ATM_LANE)
extern int (*br_fdb_test_addr_hook)(struct net_device *dev, unsigned char *addr);
--
2.17.1

2020-01-13 12:48:41

by Horatiu Vultur

[permalink] [raw]
Subject: [RFC net-next Patch v2 4/4] net: bridge: mrp: switchdev: Add HW offload

Extend switchdev interface to add support for MRP. The HW is notified in
following cases:
- SWITCHDEV_OBJ_ID_PORT_MRP. This is used when a port is added/removed from the
mrp ring.
- SWITCHDEV_OBJ_ID_RING_ROLE_MRP. This is used when the role of the node
changes. The current supported roles are Media Redundancy Manager and Media
Redundancy Client.
- SWITCHDEV_OBJ_ID_RING_TEST_MRP. This is used when to start/stop sending
MRP_Test frames on the mrp ring ports. This is called only on nodes that have
the role Media Redundancy Manager.
- SWITCHDEV_ATTR_ID_MRP_PORT_STATE. This is used when the port's state is
changed. It can be in blocking/forwarding mode.
- SWITCHDEV_ATTR_ID_MRP_PORT_ROLE. This is used when ports'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_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.
- SWITCHDEV_ATTR_ID_RING_TRANS. This is used to count the number of times the
ring goes in open state. This is required to notify the HW because the
MRP_Test frame contains the field MRP_Transition which contains this
information.

Signed-off-by: Horatiu Vultur <[email protected]>
---
include/net/switchdev.h | 52 ++++++++++
net/bridge/Makefile | 2 +-
net/bridge/br_mrp.c | 96 +++++++++++-------
net/bridge/br_mrp_switchdev.c | 180 ++++++++++++++++++++++++++++++++++
net/bridge/br_mrp_timer.c | 37 ++++++-
net/bridge/br_private_mrp.h | 16 +++
6 files changed, 344 insertions(+), 39 deletions(-)
create mode 100644 net/bridge/br_mrp_switchdev.c

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index aee86a189432..c4cd180fbf5a 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -40,6 +40,12 @@ enum switchdev_attr_id {
SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING,
SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
+#ifdef CONFIG_BRIDGE_MRP
+ SWITCHDEV_ATTR_ID_MRP_PORT_STATE,
+ SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
+ SWITCHDEV_ATTR_ID_MRP_RING_STATE,
+ SWITCHDEV_ATTR_ID_MRP_RING_TRANS,
+#endif
};

struct switchdev_attr {
@@ -55,6 +61,12 @@ struct switchdev_attr {
clock_t ageing_time; /* BRIDGE_AGEING_TIME */
bool vlan_filtering; /* BRIDGE_VLAN_FILTERING */
bool mc_disabled; /* MC_DISABLED */
+#ifdef CONFIG_BRIDGE_MRP
+ u8 mrp_port_state; /* MRP_PORT_STATE */
+ u8 mrp_port_role; /* MRP_PORT_ROLE */
+ u8 mrp_ring_state; /* MRP_RING_STATE */
+ u32 mrp_ring_trans; /* MRP_RING_TRANSITION */
+#endif
} u;
};

@@ -63,6 +75,11 @@ enum switchdev_obj_id {
SWITCHDEV_OBJ_ID_PORT_VLAN,
SWITCHDEV_OBJ_ID_PORT_MDB,
SWITCHDEV_OBJ_ID_HOST_MDB,
+#ifdef CONFIG_BRIDGE_MRP
+ SWITCHDEV_OBJ_ID_PORT_MRP,
+ SWITCHDEV_OBJ_ID_RING_TEST_MRP,
+ SWITCHDEV_OBJ_ID_RING_ROLE_MRP,
+#endif
};

struct switchdev_obj {
@@ -94,6 +111,41 @@ struct switchdev_obj_port_mdb {
#define SWITCHDEV_OBJ_PORT_MDB(OBJ) \
container_of((OBJ), struct switchdev_obj_port_mdb, obj)

+#ifdef CONFIG_BRIDGE_MRP
+/* SWITCHDEV_OBJ_ID_PORT_MRP */
+struct switchdev_obj_port_mrp {
+ struct switchdev_obj obj;
+ struct net_device *port;
+ u32 ring_nr;
+};
+
+#define SWITCHDEV_OBJ_PORT_MRP(OBJ) \
+ container_of((OBJ), struct switchdev_obj_port_mrp, obj)
+
+/* SWITCHDEV_OBJ_ID_RING_TEST_MRP */
+struct switchdev_obj_ring_test_mrp {
+ struct switchdev_obj obj;
+ /* The value is in us and a value of 0 represents to stop */
+ u32 interval;
+ u8 max;
+ u32 ring_nr;
+};
+
+#define SWITCHDEV_OBJ_RING_TEST_MRP(OBJ) \
+ container_of((OBJ), struct switchdev_obj_ring_test_mrp, obj)
+
+/* SWITCHDEV_OBJ_ID_RING_ROLE_MRP */
+struct switchdev_obj_ring_role_mrp {
+ struct switchdev_obj obj;
+ u8 ring_role;
+ u32 ring_nr;
+};
+
+#define SWITCHDEV_OBJ_RING_ROLE_MRP(OBJ) \
+ container_of((OBJ), struct switchdev_obj_ring_role_mrp, obj)
+
+#endif
+
typedef int switchdev_obj_dump_cb_t(struct switchdev_obj *obj);

enum switchdev_notifier_type {
diff --git a/net/bridge/Makefile b/net/bridge/Makefile
index 917826c9d8de..a51a9fc112ed 100644
--- a/net/bridge/Makefile
+++ b/net/bridge/Makefile
@@ -26,4 +26,4 @@ bridge-$(CONFIG_NET_SWITCHDEV) += br_switchdev.o

obj-$(CONFIG_NETFILTER) += netfilter/

-bridge-$(CONFIG_BRIDGE_MRP) += br_mrp.o br_mrp_timer.o
+bridge-$(CONFIG_BRIDGE_MRP) += br_mrp.o br_mrp_timer.o br_mrp_switchdev.o
diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
index 4173021d3bfa..d109fce226d5 100644
--- a/net/bridge/br_mrp.c
+++ b/net/bridge/br_mrp.c
@@ -78,6 +78,10 @@ void br_mrp_set_mrm_state(struct br_mrp *mrp,
{
br_debug(mrp->br, "mrm_state: %s\n", br_mrp_get_mrm_state(state));
mrp->mrm_state = state;
+
+ br_mrp_switchdev_set_ring_state(mrp, state == BR_MRP_MRM_STATE_CHK_RC ?
+ BR_MRP_RING_STATE_CLOSED :
+ BR_MRP_RING_STATE_OPEN);
}

void br_mrp_set_mrc_state(struct br_mrp *mrp,
@@ -101,9 +105,9 @@ static int br_mrp_set_mrm_role(struct br_mrp *mrp)

br_mrp_set_mrm_state(mrp, BR_MRP_MRM_STATE_AC_STAT1);

- mrp->p_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
- mrp->ring_role = BR_MRP_RING_ROLE_MRM;
+ br_mrp_port_switchdev_set_state(mrp->p_port, BR_MRP_PORT_STATE_BLOCKED);
+ br_mrp_port_switchdev_set_state(mrp->s_port, BR_MRP_PORT_STATE_BLOCKED);
+ br_mrp_switchdev_set_ring_role(mrp, BR_MRP_RING_ROLE_MRM);

if (br_mrp_is_port_up(mrp->p_port))
br_mrp_port_link_change(mrp->p_port, true);
@@ -128,9 +132,9 @@ static int br_mrp_set_mrc_role(struct br_mrp *mrp)

br_mrp_set_mrc_state(mrp, BR_MRP_MRC_STATE_AC_STAT1);

- mrp->p_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
- mrp->ring_role = BR_MRP_RING_ROLE_MRC;
+ br_mrp_port_switchdev_set_state(mrp->p_port, BR_MRP_PORT_STATE_BLOCKED);
+ br_mrp_port_switchdev_set_state(mrp->s_port, BR_MRP_PORT_STATE_BLOCKED);
+ br_mrp_switchdev_set_ring_role(mrp, BR_MRP_RING_ROLE_MRC);

if (br_mrp_is_port_up(mrp->p_port))
br_mrp_port_link_change(mrp->p_port, true);
@@ -385,7 +389,8 @@ static void br_mrp_mrm_recv_ring_test(struct br_mrp *mrp)
br_mrp_set_mrm_state(mrp, BR_MRP_MRM_STATE_CHK_RC);
break;
case BR_MRP_MRM_STATE_CHK_RO:
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
+ br_mrp_port_switchdev_set_state(mrp->s_port,
+ BR_MRP_PORT_STATE_BLOCKED);

mrp->ring_test_curr_max = mrp->ring_test_conf_max - 1;
mrp->ring_test_curr = 0;
@@ -455,7 +460,8 @@ static void br_mrp_recv_ring_topo(struct net_bridge_port *port,
case BR_MRP_MRC_STATE_PT:
mrp->ring_link_curr_max = mrp->ring_link_conf_max;
br_mrp_ring_link_up_stop(mrp);
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_FORWARDING;
+ br_mrp_port_switchdev_set_state(mrp->s_port,
+ BR_MRP_PORT_STATE_FORWARDING);
br_mrp_clear_fdb_start(mrp, ntohs(hdr->interval));
br_mrp_set_mrc_state(mrp, BR_MRP_MRC_STATE_PT_IDLE);
break;
@@ -538,7 +544,8 @@ static void br_mrp_recv_ring_link(struct net_bridge_port *port,
}

if (type == BR_MRP_TLV_HEADER_RING_LINK_UP && !mrp->blocked) {
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
+ br_mrp_port_switchdev_set_state(mrp->s_port,
+ BR_MRP_PORT_STATE_BLOCKED);
mrp->ring_test_curr_max = mrp->ring_test_conf_max - 1;
mrp->ring_test_curr = 0;

@@ -570,8 +577,9 @@ static void br_mrp_recv_ring_link(struct net_bridge_port *port,

if (type == BR_MRP_TLV_HEADER_RING_LINK_DOWN &&
mrp->react_on_link_change) {
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_FORWARDING;
- mrp->ring_transitions++;
+ br_mrp_port_switchdev_set_state(mrp->s_port,
+ BR_MRP_PORT_STATE_FORWARDING);
+ br_mrp_switchdev_update_ring_transitions(mrp);
br_mrp_ring_topo_req(mrp, 0);
br_mrp_set_mrm_state(mrp, BR_MRP_MRM_STATE_CHK_RO);
break;
@@ -808,14 +816,16 @@ static void br_mrp_mrm_port_link(struct net_bridge_port *p, bool up)
switch (mrp->mrm_state) {
case BR_MRP_MRM_STATE_AC_STAT1:
if (up && p == mrp->p_port) {
- mrp->p_port->mrp_port->state = BR_MRP_PORT_STATE_FORWARDING;
+ br_mrp_port_switchdev_set_state(mrp->p_port,
+ BR_MRP_PORT_STATE_FORWARDING);
br_mrp_ring_test_req(mrp, mrp->ring_test_conf_interval);
br_mrp_set_mrm_state(mrp, BR_MRP_MRM_STATE_PRM_UP);
}
if (up && p != mrp->p_port) {
mrp->s_port = mrp->p_port;
mrp->p_port = p;
- mrp->p_port->mrp_port->state = BR_MRP_PORT_STATE_FORWARDING;
+ br_mrp_port_switchdev_set_state(mrp->p_port,
+ BR_MRP_PORT_STATE_FORWARDING);
br_mrp_ring_test_req(mrp, mrp->ring_test_conf_interval);
br_mrp_set_mrm_state(mrp, BR_MRP_MRM_STATE_PRM_UP);
}
@@ -823,7 +833,8 @@ static void br_mrp_mrm_port_link(struct net_bridge_port *p, bool up)
case BR_MRP_MRM_STATE_PRM_UP:
if (!up && p == mrp->p_port) {
br_mrp_ring_test_stop(mrp);
- mrp->p_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
+ br_mrp_port_switchdev_set_state(mrp->p_port,
+ BR_MRP_PORT_STATE_BLOCKED);
br_mrp_set_mrm_state(mrp, BR_MRP_MRM_STATE_AC_STAT1);
}
if (up && p != mrp->p_port) {
@@ -838,14 +849,16 @@ static void br_mrp_mrm_port_link(struct net_bridge_port *p, bool up)
if (!up && p == mrp->p_port) {
mrp->s_port = mrp->p_port;
mrp->p_port = p;
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
+ br_mrp_port_switchdev_set_state(mrp->s_port,
+ BR_MRP_PORT_STATE_BLOCKED);
br_mrp_ring_test_req(mrp, mrp->ring_test_conf_interval);
br_mrp_ring_topo_req(mrp, topo_interval);
br_mrp_set_mrm_state(mrp, BR_MRP_MRM_STATE_PRM_UP);
break;
}
if (!up && p != mrp->p_port) {
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
+ br_mrp_port_switchdev_set_state(mrp->s_port,
+ BR_MRP_PORT_STATE_BLOCKED);
br_mrp_set_mrm_state(mrp, BR_MRP_MRM_STATE_PRM_UP);
}
break;
@@ -853,16 +866,18 @@ static void br_mrp_mrm_port_link(struct net_bridge_port *p, bool up)
if (!up && p == mrp->p_port) {
mrp->p_port = mrp->s_port;
mrp->s_port = p;
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
- mrp->p_port->mrp_port->state = BR_MRP_PORT_STATE_FORWARDING;
+ br_mrp_port_switchdev_set_role(mrp->s_port,
+ BR_MRP_PORT_STATE_BLOCKED);
+ br_mrp_port_switchdev_set_role(mrp->p_port,
+ BR_MRP_PORT_STATE_FORWARDING);
br_mrp_ring_test_req(mrp, mrp->ring_test_conf_interval);
br_mrp_ring_topo_req(mrp, topo_interval);
- mrp->ring_transitions++;
+ br_mrp_switchdev_update_ring_transitions(mrp);
br_mrp_set_mrm_state(mrp, BR_MRP_MRM_STATE_PRM_UP);
break;
}
if (!up && p != mrp->p_port) {
- mrp->ring_transitions++;
+ br_mrp_switchdev_update_ring_transitions(mrp);
br_mrp_set_mrm_state(mrp, BR_MRP_MRM_STATE_PRM_UP);
break;
}
@@ -887,13 +902,15 @@ static void br_mrp_mrc_port_link(struct net_bridge_port *p, bool up)
switch (mrp->mrc_state) {
case BR_MRP_MRC_STATE_AC_STAT1:
if (up && p == mrp->p_port) {
- mrp->p_port->mrp_port->state = BR_MRP_PORT_STATE_FORWARDING;
+ br_mrp_port_switchdev_set_state(mrp->p_port,
+ BR_MRP_PORT_STATE_FORWARDING);
br_mrp_set_mrc_state(mrp, BR_MRP_MRC_STATE_DE_IDLE);
}
if (up && p != mrp->p_port) {
mrp->s_port = mrp->p_port;
mrp->p_port = p;
- mrp->p_port->mrp_port->state = BR_MRP_PORT_STATE_FORWARDING;
+ br_mrp_port_switchdev_set_state(mrp->p_port,
+ BR_MRP_PORT_STATE_FORWARDING);
br_mrp_set_mrc_state(mrp, BR_MRP_MRC_STATE_DE_IDLE);
}
break;
@@ -908,7 +925,8 @@ static void br_mrp_mrc_port_link(struct net_bridge_port *p, bool up)
br_mrp_set_mrc_state(mrp, BR_MRP_MRC_STATE_PT);
}
if (!up && p == mrp->p_port) {
- mrp->p_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
+ br_mrp_port_switchdev_set_state(mrp->p_port,
+ BR_MRP_PORT_STATE_BLOCKED);
br_mrp_set_mrc_state(mrp, BR_MRP_MRC_STATE_AC_STAT1);
}
break;
@@ -916,7 +934,8 @@ static void br_mrp_mrc_port_link(struct net_bridge_port *p, bool up)
if (!up && p != mrp->p_port) {
mrp->ring_link_curr_max = mrp->ring_link_conf_max;
br_mrp_ring_link_up_stop(mrp);
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
+ br_mrp_port_switchdev_set_state(mrp->s_port,
+ BR_MRP_PORT_STATE_BLOCKED);
br_mrp_ring_link_down_start(mrp,
mrp->ring_link_conf_interval);
br_mrp_ring_link_req(mrp->p_port, up,
@@ -930,8 +949,10 @@ static void br_mrp_mrc_port_link(struct net_bridge_port *p, bool up)
br_mrp_ring_link_up_stop(mrp);
mrp->p_port = mrp->s_port;
mrp->s_port = p;
- mrp->p_port->mrp_port->state = BR_MRP_PORT_STATE_FORWARDING;
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
+ br_mrp_port_switchdev_set_state(mrp->p_port,
+ BR_MRP_PORT_STATE_FORWARDING);
+ br_mrp_port_switchdev_set_state(mrp->s_port,
+ BR_MRP_PORT_STATE_BLOCKED);
br_mrp_ring_link_down_start(mrp,
mrp->ring_link_conf_interval);
br_mrp_ring_link_req(mrp->p_port, up,
@@ -953,7 +974,8 @@ static void br_mrp_mrc_port_link(struct net_bridge_port *p, bool up)
}
if (!up && p == mrp->p_port) {
mrp->ring_link_curr_max = mrp->ring_link_conf_max;
- mrp->p_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
+ br_mrp_port_switchdev_set_state(mrp->p_port,
+ BR_MRP_PORT_STATE_BLOCKED);
br_mrp_ring_link_down_stop(mrp);
br_mrp_set_mrc_state(mrp, BR_MRP_MRC_STATE_AC_STAT1);
}
@@ -961,7 +983,8 @@ static void br_mrp_mrc_port_link(struct net_bridge_port *p, bool up)
case BR_MRP_MRC_STATE_PT_IDLE:
if (!up && p != mrp->p_port) {
mrp->ring_link_curr_max = mrp->ring_link_conf_max;
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
+ br_mrp_port_switchdev_set_state(mrp->s_port,
+ BR_MRP_PORT_STATE_BLOCKED);
br_mrp_ring_link_down_start(mrp,
mrp->ring_link_conf_interval);
br_mrp_ring_link_req(mrp->p_port, up,
@@ -973,7 +996,8 @@ static void br_mrp_mrc_port_link(struct net_bridge_port *p, bool up)
mrp->ring_link_curr_max = mrp->ring_link_conf_max;
mrp->p_port = mrp->s_port;
mrp->s_port = p;
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_BLOCKED;
+ br_mrp_port_switchdev_set_state(mrp->s_port,
+ BR_MRP_PORT_STATE_BLOCKED);
br_mrp_ring_link_down_start(mrp,
mrp->ring_link_conf_interval);
br_mrp_ring_link_req(mrp->p_port, up,
@@ -1183,7 +1207,7 @@ static int br_mrp_port_init(struct net_bridge_port *port, struct br_mrp *mrp,
* to set again the role(MRM or MRC)
*/
br_mrp_reset_ring_state(mrp);
- mrp->ring_role = BR_MRP_RING_ROLE_DISABLED;
+ br_mrp_switchdev_set_ring_role(mrp, BR_MRP_RING_ROLE_DISABLED);

if (!port->mrp_port) {
port->mrp_port = devm_kzalloc(&port->br->dev->dev,
@@ -1194,8 +1218,9 @@ static int br_mrp_port_init(struct net_bridge_port *port, struct br_mrp *mrp,
}

port->mrp_port->mrp = mrp;
- port->mrp_port->state = BR_MRP_PORT_STATE_FORWARDING;
- port->mrp_port->role = role;
+ br_mrp_port_switchdev_set_role(port, BR_MRP_PORT_STATE_FORWARDING);
+ br_mrp_port_switchdev_set_role(port, role);
+ br_mrp_port_switchdev_add(port);

if (role == BR_MRP_PORT_ROLE_PRIMARY)
mrp->p_port = port;
@@ -1218,15 +1243,16 @@ void br_mrp_port_uninit(struct net_bridge_port *port)
mutex_lock(&mrp->lock);

br_mrp_reset_ring_state(mrp);
- mrp->ring_role = BR_MRP_RING_ROLE_DISABLED;
+ br_mrp_switchdev_set_ring_role(mrp, BR_MRP_RING_ROLE_DISABLED);

if (port->mrp_port->role == BR_MRP_PORT_ROLE_PRIMARY)
mrp->p_port = NULL;
if (port->mrp_port->role == BR_MRP_PORT_ROLE_SECONDARY)
mrp->s_port = NULL;

- port->mrp_port->state = BR_MRP_PORT_STATE_FORWARDING;
- port->mrp_port->role = BR_MRP_PORT_ROLE_NONE;
+ br_mrp_port_switchdev_set_state(port, BR_MRP_PORT_STATE_FORWARDING);
+ br_mrp_port_switchdev_set_role(port, BR_MRP_PORT_ROLE_NONE);
+ br_mrp_port_switchdev_del(port);
port->mrp_port->mrp = NULL;

devm_kfree(&port->br->dev->dev, port->mrp_port);
diff --git a/net/bridge/br_mrp_switchdev.c b/net/bridge/br_mrp_switchdev.c
new file mode 100644
index 000000000000..c02f46e0ca47
--- /dev/null
+++ b/net/bridge/br_mrp_switchdev.c
@@ -0,0 +1,180 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <net/switchdev.h>
+
+#include "br_private_mrp.h"
+
+int br_mrp_port_switchdev_add(struct net_bridge_port *p)
+{
+ struct net_bridge *br = p->br;
+ struct switchdev_obj_port_mrp mrp = {
+ .obj.orig_dev = br->dev,
+ .obj.id = SWITCHDEV_OBJ_ID_PORT_MRP,
+ .port = p->dev,
+ .ring_nr = p->mrp_port->mrp->ring_nr,
+ };
+ int err = 0;
+
+ err = switchdev_port_obj_add(br->dev, &mrp.obj, NULL);
+
+ if (err && err != -EOPNOTSUPP)
+ return err;
+
+ return 0;
+}
+
+int br_mrp_switchdev_set_ring_role(struct br_mrp *mrp,
+ enum br_mrp_ring_role_type role)
+{
+ struct switchdev_obj_ring_role_mrp mrp_role = {
+ .obj.orig_dev = mrp->br->dev,
+ .obj.id = SWITCHDEV_OBJ_ID_RING_ROLE_MRP,
+ .ring_role = role,
+ .ring_nr = mrp->ring_nr,
+ };
+ int err = 0;
+
+ mrp->ring_role = role;
+
+ pr_info("%s role: %d\n", __func__, role);
+
+ if (role == BR_MRP_RING_ROLE_DISABLED)
+ err = switchdev_port_obj_del(mrp->br->dev, &mrp_role.obj);
+ else
+ err = switchdev_port_obj_add(mrp->br->dev, &mrp_role.obj, NULL);
+
+ if (err && err != -EOPNOTSUPP)
+ return err;
+
+ return 0;
+}
+
+int br_mrp_switchdev_send_ring_test(struct br_mrp *mrp, u32 interval, u8 max)
+{
+ struct switchdev_obj_ring_test_mrp test = {
+ .obj.orig_dev = mrp->br->dev,
+ .obj.id = SWITCHDEV_OBJ_ID_RING_TEST_MRP,
+ .interval = interval,
+ .max = max,
+ .ring_nr = mrp->ring_nr,
+ };
+ int err = 0;
+
+ if (interval == 0)
+ err = switchdev_port_obj_del(mrp->br->dev, &test.obj);
+ else
+ err = switchdev_port_obj_add(mrp->br->dev, &test.obj, NULL);
+
+ return err;
+}
+
+int br_mrp_port_switchdev_del(struct net_bridge_port *p)
+{
+ struct net_bridge *br = p->br;
+ struct switchdev_obj_port_mrp mrp = {
+ .obj.orig_dev = br->dev,
+ .obj.id = SWITCHDEV_OBJ_ID_PORT_MRP,
+ .port = p->dev,
+ };
+ int err = 0;
+
+ if (!p->mrp_port->mrp)
+ return 0;
+
+ mrp.ring_nr = p->mrp_port->mrp->ring_nr;
+
+ err = switchdev_port_obj_del(br->dev, &mrp.obj);
+
+ if (err && err != -EOPNOTSUPP)
+ return err;
+
+ return 0;
+}
+
+int br_mrp_port_switchdev_set_state(struct net_bridge_port *p,
+ enum br_mrp_port_state_type state)
+{
+ struct switchdev_attr attr = {
+ .orig_dev = p->dev,
+ .id = SWITCHDEV_ATTR_ID_MRP_PORT_STATE,
+ .u.mrp_port_state = state,
+ };
+ int err = 0;
+
+ p->mrp_port->state = state;
+
+ pr_info("%s port: %s, state: %d\n", __func__, p->dev->name, state);
+
+ err = switchdev_port_attr_set(p->dev, &attr);
+ if (err && err != -EOPNOTSUPP)
+ br_warn(p->br, "error setting offload MRP state on port %u(%s)\n",
+ (unsigned int)p->port_no, p->dev->name);
+
+ return err;
+}
+
+int br_mrp_port_switchdev_set_role(struct net_bridge_port *p,
+ enum br_mrp_port_role_type role)
+{
+ struct switchdev_attr attr = {
+ .orig_dev = p->dev,
+ .id = SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
+ .u.mrp_port_role = role,
+ };
+ int err;
+
+ p->mrp_port->role = role;
+
+ err = switchdev_port_attr_set(p->dev, &attr);
+ if (err && err != -EOPNOTSUPP)
+ return err;
+
+ return 0;
+}
+
+int br_mrp_switchdev_set_ring_state(struct br_mrp *mrp,
+ enum br_mrp_ring_state_type state)
+{
+ struct switchdev_attr attr = {
+ .id = SWITCHDEV_ATTR_ID_MRP_RING_STATE,
+ .u.mrp_ring_state = state,
+ };
+ int err = 0;
+
+ attr.orig_dev = mrp->p_port->dev,
+ err = switchdev_port_attr_set(mrp->p_port->dev, &attr);
+ if (err && err != -EOPNOTSUPP)
+ return err;
+
+ attr.orig_dev = mrp->s_port->dev;
+ err = switchdev_port_attr_set(mrp->s_port->dev, &attr);
+ if (err && err != -EOPNOTSUPP)
+ return err;
+
+ return err;
+}
+
+int br_mrp_switchdev_update_ring_transitions(struct br_mrp *mrp)
+{
+ struct switchdev_attr attr = {
+ .id = SWITCHDEV_ATTR_ID_MRP_RING_TRANS,
+ };
+ int err;
+
+ mrp->ring_transitions++;
+
+ attr.u.mrp_ring_trans = mrp->ring_transitions;
+
+ attr.orig_dev = mrp->p_port->dev,
+ err = switchdev_port_attr_set(mrp->p_port->dev, &attr);
+ if (err && err != -EOPNOTSUPP)
+ return err;
+
+ attr.orig_dev = mrp->s_port->dev;
+ err = switchdev_port_attr_set(mrp->s_port->dev, &attr);
+ if (err && err != -EOPNOTSUPP)
+ return err;
+
+ return 0;
+}
+
diff --git a/net/bridge/br_mrp_timer.c b/net/bridge/br_mrp_timer.c
index 59aa8c05724f..6493fc94bd49 100644
--- a/net/bridge/br_mrp_timer.c
+++ b/net/bridge/br_mrp_timer.c
@@ -6,7 +6,8 @@

static void br_mrp_ring_open(struct br_mrp *mrp)
{
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_FORWARDING;
+ br_mrp_port_switchdev_set_state(mrp->s_port,
+ BR_MRP_PORT_STATE_FORWARDING);

mrp->ring_test_curr_max = mrp->ring_test_conf_max - 1;
mrp->ring_test_curr = 0;
@@ -18,7 +19,7 @@ static void br_mrp_ring_open(struct br_mrp *mrp)

br_mrp_ring_test_req(mrp, mrp->ring_test_conf_interval);

- mrp->ring_transitions++;
+ br_mrp_switchdev_update_ring_transitions(mrp);
br_mrp_set_mrm_state(mrp, BR_MRP_MRM_STATE_CHK_RO);
}

@@ -111,7 +112,8 @@ static void br_mrp_ring_link_up_expired(struct work_struct *work)
br_mrp_ring_link_req(mrp->p_port, true, interval);
} else {
mrp->ring_link_curr_max = mrp->ring_link_conf_max;
- mrp->s_port->mrp_port->state = BR_MRP_PORT_STATE_FORWARDING;
+ br_mrp_port_switchdev_set_state(mrp->s_port,
+ BR_MRP_PORT_STATE_FORWARDING);
br_mrp_set_mrc_state(mrp, BR_MRP_MRC_STATE_PT_IDLE);
}

@@ -152,12 +154,41 @@ static void br_mrp_ring_link_down_expired(struct work_struct *work)

void br_mrp_ring_test_start(struct br_mrp *mrp, u32 interval)
{
+ int err;
+
+ err = br_mrp_switchdev_send_ring_test(mrp, interval,
+ mrp->ring_test_conf_max);
+ /* If HW can transmit the test frames then don't start anymore the
+ * SW timers
+ */
+ if (!err) {
+ pr_info("HW timers started\n");
+ return;
+ } else if (err != -EOPNOTSUPP) {
+ pr_info("HW can't start timers error: %d\n", err);
+ return;
+ }
+
queue_delayed_work(mrp->timers_queue, &mrp->ring_test_work,
usecs_to_jiffies(interval));
}

void br_mrp_ring_test_stop(struct br_mrp *mrp)
{
+ int err;
+
+ err = br_mrp_switchdev_send_ring_test(mrp, 0, 0);
+ /* If HW can stop the transmission of the test frames then the SW timers
+ * were not start so just exit
+ */
+ if (!err) {
+ pr_info("HW timers stopped\n");
+ return;
+ } else if (err != -EOPNOTSUPP) {
+ pr_info("HW can't stop timers error: %d\n", err);
+ return;
+ }
+
cancel_delayed_work(&mrp->ring_test_work);
}

diff --git a/net/bridge/br_private_mrp.h b/net/bridge/br_private_mrp.h
index 13fd2330ccfc..f3f34749774d 100644
--- a/net/bridge/br_private_mrp.h
+++ b/net/bridge/br_private_mrp.h
@@ -205,4 +205,20 @@ void br_mrp_ring_link_up_stop(struct br_mrp *mrp);
void br_mrp_ring_link_down_start(struct br_mrp *mrp, u32 interval);
void br_mrp_ring_link_down_stop(struct br_mrp *mrp);

+/* br_mrp_switchdev.c */
+int br_mrp_port_switchdev_add(struct net_bridge_port *p);
+int br_mrp_port_switchdev_del(struct net_bridge_port *p);
+int br_mrp_port_switchdev_set_state(struct net_bridge_port *p,
+ enum br_mrp_port_state_type state);
+int br_mrp_port_switchdev_set_role(struct net_bridge_port *p,
+ enum br_mrp_port_role_type role);
+
+int br_mrp_switchdev_set_ring_role(struct br_mrp *mrp,
+ enum br_mrp_ring_role_type role);
+int br_mrp_switchdev_set_ring_state(struct br_mrp *mrp,
+ enum br_mrp_ring_state_type state);
+int br_mrp_switchdev_update_ring_transitions(struct br_mrp *mrp);
+
+int br_mrp_switchdev_send_ring_test(struct br_mrp *mrp, u32 interval, u8 max);
+
#endif /* BR_PRIVATE_MRP_H_ */
--
2.17.1

2020-01-13 12:50:10

by Horatiu Vultur

[permalink] [raw]
Subject: [RFC net-next Patch v2 3/4] net: bridge: mrp: Add netlink support to configure MRP

Extend br_netlink to be able to create/delete MRP instances. The current
configurations options for each instance are:
- set primary port
- set secondary port
- set MRP ring role (MRM or MRC)
- set MRP ring id.

To create a MRP instance on the bridge:
$ bridge mrp add dev br0 p_port eth0 s_port eth1 ring_role 2 ring_id 1

Where:
p_port, s_port: can be any port under the bridge
ring_role: can have the value 1(MRC - Media Redundancy Client) or
2(MRM - Media Redundancy Manager). In a ring can be only one MRM.
ring_id: unique id for each MRP instance.

It is possible to create multiple instances. Each instance has to have it's own
ring_id and a port can't be part of multiple instances:
$ bridge mrp add dev br0 p_port eth2 s_port eth3 ring_role 1 ring_id 2

To see current MRP instances and their status:
$ bridge mrp show
dev br0 p_port eth2 s_port eth3 ring_role 1 ring_id 2 ring_state 3
dev br0 p_port eth0 s_port eth1 ring_role 2 ring_id 1 ring_state 4

Where:
p_port, s_port, ring_role, ring_id: represent the configuration values. It is
possible for primary port to change the role with the secondary port.
It depends on the states through which the node goes.
ring_state: depends on the ring_role. If mrp_ring_role is 1(MRC) then the values
of mrp_ring_state can be: 0(AC_STAT1), 1(DE_IDLE), 2(PT), 3(DE), 4(PT_IDLE).
If mrp_ring_role is 2(MRM) then the values of mrp_ring_state can be:
0(AC_STAT1), 1(PRM_UP), 2(CHK_RO), 3(CHK_RC).

Signed-off-by: Horatiu Vultur <[email protected]>
---
include/uapi/linux/if_bridge.h | 27 ++++
include/uapi/linux/rtnetlink.h | 7 +
net/bridge/br_mrp.c | 281 +++++++++++++++++++++++++++++++++
net/bridge/br_netlink.c | 9 ++
net/bridge/br_private.h | 2 +
net/bridge/br_private_mrp.h | 9 ++
security/selinux/nlmsgtab.c | 5 +-
7 files changed, 339 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 4a58e3d7de46..00f4f465d62a 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -265,6 +265,33 @@ enum {
};
#define MDBA_SET_ENTRY_MAX (__MDBA_SET_ENTRY_MAX - 1)

+#ifdef CONFIG_BRIDGE_MRP
+enum {
+ MRPA_UNSPEC,
+ MRPA_MRP,
+ __MRPA_MAX,
+};
+#define MRPA_MAX (__MRPA_MAX - 1)
+
+enum {
+ MRPA_MRP_UNSPEC,
+ MRPA_MRP_ENTRY,
+ __MRPA_MRP_MAX,
+};
+#define MRPA_MRP_MAX (__MRPA_MRP_MAX - 1)
+
+enum {
+ MRP_ATTR_UNSPEC,
+ MRP_ATTR_P_IFINDEX,
+ MRP_ATTR_S_IFINDEX,
+ MRP_ATTR_RING_ROLE,
+ MRP_ATTR_RING_NR,
+ MRP_ATTR_RING_STATE,
+ __MRP_ATTR_MAX,
+};
+#define MRP_ATTR_MAX (__MRP_ATTR_MAX - 1)
+#endif
+
/* Embedded inside LINK_XSTATS_TYPE_BRIDGE */
enum {
BRIDGE_XSTATS_UNSPEC,
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 1418a8362bb7..b1d72a5309cd 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -171,6 +171,13 @@ enum {
RTM_GETLINKPROP,
#define RTM_GETLINKPROP RTM_GETLINKPROP

+ RTM_NEWMRP = 112,
+#define RTM_NEWMRP RTM_NEWMRP
+ RTM_DELMRP,
+#define RTM_DELMRP RTM_DELMRP
+ RTM_GETMRP,
+#define RTM_GETMRP RTM_GETMRP
+
__RTM_MAX,
#define RTM_MAX (((__RTM_MAX + 3) & ~3) - 1)
};
diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
index a84aab3f7114..4173021d3bfa 100644
--- a/net/bridge/br_mrp.c
+++ b/net/bridge/br_mrp.c
@@ -1234,3 +1234,284 @@ void br_mrp_port_uninit(struct net_bridge_port *port)

mutex_unlock(&mrp->lock);
}
+
+/* Do sanity checks and obtain device and the ring */
+static int br_mrp_parse(struct sk_buff *skb, struct nlmsghdr *nlh,
+ struct net_device **pdev, struct br_mrp_config *conf)
+{
+ struct nlattr *tb[MRP_ATTR_MAX + 1];
+ struct net *net = sock_net(skb->sk);
+ struct br_port_msg *bpm;
+ struct net_device *dev;
+ int err;
+
+ err = nlmsg_parse_deprecated(nlh, sizeof(*bpm), tb,
+ MRP_ATTR_MAX, NULL, NULL);
+ if (err < 0)
+ return err;
+
+ bpm = nlmsg_data(nlh);
+ if (bpm->ifindex == 0) {
+ pr_info("PF_BRIDGE: %s with invalid ifindex\n", __func__);
+ return -EINVAL;
+ }
+
+ dev = __dev_get_by_index(net, bpm->ifindex);
+ if (!dev) {
+ pr_info("PF_BRIDGE: %s with unknown ifindex\n", __func__);
+ return -ENODEV;
+ }
+
+ if (!(dev->priv_flags & IFF_EBRIDGE)) {
+ pr_info("PF_BRIDGE: %s with non-bridge\n", __func__);
+ return -EOPNOTSUPP;
+ }
+
+ *pdev = dev;
+
+ if (tb[MRP_ATTR_P_IFINDEX])
+ conf->p_ifindex = nla_get_u32(tb[MRP_ATTR_P_IFINDEX]);
+ if (tb[MRP_ATTR_S_IFINDEX])
+ conf->s_ifindex = nla_get_u32(tb[MRP_ATTR_S_IFINDEX]);
+ if (tb[MRP_ATTR_RING_ROLE])
+ conf->ring_role = nla_get_u8(tb[MRP_ATTR_RING_ROLE]);
+ if (tb[MRP_ATTR_RING_NR])
+ conf->ring_nr = nla_get_u8(tb[MRP_ATTR_RING_NR]);
+
+ return 0;
+}
+
+static int br_mrp_fill_entry(struct sk_buff *skb, struct netlink_callback *cb,
+ struct net_device *dev)
+{
+ int idx = 0, s_idx = cb->args[1], err = 0;
+ struct net_bridge *br = netdev_priv(dev);
+ struct br_mrp *mrp;
+ struct nlattr *nest, *nest2;
+
+ nest = nla_nest_start_noflag(skb, MRPA_MRP);
+ if (!nest)
+ return -EMSGSIZE;
+
+ list_for_each_entry_rcu(mrp, &br->mrp_list, list) {
+ if (idx < s_idx)
+ goto skip;
+
+ nest2 = nla_nest_start_noflag(skb, MRPA_MRP_ENTRY);
+ if (!nest2) {
+ err = -EMSGSIZE;
+ mutex_unlock(&mrp->lock);
+ break;
+ }
+
+ mutex_lock(&mrp->lock);
+
+ if (mrp->p_port)
+ nla_put_u32(skb, MRP_ATTR_P_IFINDEX,
+ mrp->p_port->dev->ifindex);
+ if (mrp->s_port)
+ nla_put_u32(skb, MRP_ATTR_S_IFINDEX,
+ mrp->s_port->dev->ifindex);
+
+ nla_put_u32(skb, MRP_ATTR_RING_NR, mrp->ring_nr);
+ nla_put_u32(skb, MRP_ATTR_RING_ROLE, mrp->ring_role);
+
+ if (mrp->ring_role == BR_MRP_RING_ROLE_MRM)
+ nla_put_u32(skb, MRP_ATTR_RING_STATE, mrp->mrm_state);
+ if (mrp->ring_role == BR_MRP_RING_ROLE_MRC)
+ nla_put_u32(skb, MRP_ATTR_RING_STATE, mrp->mrc_state);
+
+ mutex_unlock(&mrp->lock);
+
+ nla_nest_end(skb, nest2);
+skip:
+ idx++;
+ }
+
+ cb->args[1] = idx;
+ nla_nest_end(skb, nest);
+ return err;
+}
+
+static int br_mrp_dump(struct sk_buff *skb, struct netlink_callback *cb)
+{
+ struct net *net = sock_net(skb->sk);
+ struct nlmsghdr *nlh = NULL;
+ struct net_device *dev;
+ int idx = 0, s_idx;
+
+ s_idx = cb->args[0];
+
+ rcu_read_lock();
+
+ cb->seq = net->dev_base_seq;
+
+ for_each_netdev_rcu(net, dev) {
+ if (dev->priv_flags & IFF_EBRIDGE) {
+ struct br_port_msg *bpm;
+
+ if (idx < s_idx)
+ goto skip;
+
+ nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid,
+ cb->nlh->nlmsg_seq, RTM_GETMRP,
+ sizeof(*bpm), NLM_F_MULTI);
+ if (!nlh)
+ break;
+
+ bpm = nlmsg_data(nlh);
+ memset(bpm, 0, sizeof(*bpm));
+ bpm->ifindex = dev->ifindex;
+ if (br_mrp_fill_entry(skb, cb, dev) < 0)
+ goto out;
+
+ cb->args[1] = 0;
+ nlmsg_end(skb, nlh);
+skip:
+ idx++;
+ }
+ }
+
+out:
+ if (nlh)
+ nlmsg_end(skb, nlh);
+ rcu_read_unlock();
+ cb->args[0] = idx;
+ return skb->len;
+}
+
+static int br_mrp_add(struct sk_buff *skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack)
+{
+ struct net_bridge_port *p_port, *s_port;
+ struct net *net = sock_net(skb->sk);
+ enum br_mrp_ring_role_type role;
+ struct br_mrp_config conf;
+ struct net_device *dev;
+ struct net_bridge *br;
+ struct br_mrp *mrp;
+ u32 ring_nr;
+ int err;
+
+ err = br_mrp_parse(skb, nlh, &dev, &conf);
+ if (err < 0)
+ return err;
+
+ br = netdev_priv(dev);
+
+ /* Get priority and secondary ports */
+ dev = __dev_get_by_index(net, conf.p_ifindex);
+ if (!dev)
+ return -ENODEV;
+
+ p_port = br_port_get_rtnl(dev);
+ if (!p_port || p_port->br != br)
+ return -EINVAL;
+
+ dev = __dev_get_by_index(net, conf.s_ifindex);
+ if (!dev)
+ return -ENODEV;
+
+ s_port = br_port_get_rtnl(dev);
+ if (!s_port || s_port->br != br)
+ return -EINVAL;
+
+ /* Get role */
+ role = conf.ring_role;
+
+ /* Get ring number */
+ ring_nr = conf.ring_nr;
+
+ /* It is not possible to have MRP instances with the same ID */
+ mrp = br_mrp_find(br, ring_nr);
+ if (mrp)
+ return -EINVAL;
+
+ /* Create the mrp instance */
+ err = br_mrp_create(br, ring_nr);
+ if (err < 0)
+ return err;
+
+ mrp = br_mrp_find(br, ring_nr);
+
+ mutex_lock(&mrp->lock);
+
+ /* Initialize the ports */
+ err = br_mrp_port_init(p_port, mrp, BR_MRP_PORT_ROLE_PRIMARY);
+ if (err < 0) {
+ mutex_unlock(&mrp->lock);
+ goto delete_mrp;
+ }
+
+ err = br_mrp_port_init(s_port, mrp, BR_MRP_PORT_ROLE_SECONDARY);
+ if (err < 0) {
+ mutex_unlock(&mrp->lock);
+ goto delete_port;
+ }
+
+ if (role == BR_MRP_RING_ROLE_MRM)
+ br_mrp_set_mrm_role(mrp);
+ if (role == BR_MRP_RING_ROLE_MRC)
+ br_mrp_set_mrc_role(mrp);
+
+ mutex_unlock(&mrp->lock);
+
+ return 0;
+
+delete_port:
+ br_mrp_port_uninit(p_port);
+
+delete_mrp:
+ br_mrp_destroy(br, ring_nr);
+ return err;
+}
+
+static int br_mrp_del(struct sk_buff *skb, struct nlmsghdr *nlh,
+ struct netlink_ext_ack *extack)
+{
+ struct br_mrp_config conf;
+ struct net_device *dev;
+ struct net_bridge *br;
+ struct br_mrp *mrp;
+ u32 ring_nr;
+ int err;
+
+ err = br_mrp_parse(skb, nlh, &dev, &conf);
+ if (err < 0)
+ return err;
+
+ br = netdev_priv(dev);
+
+ /* Get ring number */
+ ring_nr = conf.ring_nr;
+
+ mrp = br_mrp_find(br, ring_nr);
+ if (!mrp) {
+ pr_info("PF_BRIDGE: %s with invalid ring_nr\n", __func__);
+ return -EINVAL;
+ }
+
+ br_mrp_port_uninit(mrp->p_port);
+ br_mrp_port_uninit(mrp->s_port);
+
+ br_mrp_destroy(br, ring_nr);
+
+ return 0;
+}
+
+void br_mrp_netlink_init(void)
+{
+ rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_GETMRP, NULL,
+ br_mrp_dump, 0);
+ rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_NEWMRP, br_mrp_add,
+ NULL, 0);
+ rtnl_register_module(THIS_MODULE, PF_BRIDGE, RTM_DELMRP, br_mrp_del,
+ NULL, 0);
+}
+
+void br_mrp_netlink_uninit(void)
+{
+ rtnl_unregister(PF_BRIDGE, RTM_GETMRP);
+ rtnl_unregister(PF_BRIDGE, RTM_NEWMRP);
+ rtnl_unregister(PF_BRIDGE, RTM_DELMRP);
+}
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 60136575aea4..6d8f84ed8b0d 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1664,6 +1664,9 @@ int __init br_netlink_init(void)
int err;

br_mdb_init();
+#ifdef CONFIG_BRIDGE_MRP
+ br_mrp_netlink_init();
+#endif
rtnl_af_register(&br_af_ops);

err = rtnl_link_register(&br_link_ops);
@@ -1674,12 +1677,18 @@ int __init br_netlink_init(void)

out_af:
rtnl_af_unregister(&br_af_ops);
+#ifdef CONFIG_BRIDGE_MRP
+ br_mrp_netlink_uninit();
+#endif
br_mdb_uninit();
return err;
}

void br_netlink_fini(void)
{
+#ifdef CONFIG_BRIDGE_MRP
+ br_mrp_netlink_uninit();
+#endif
br_mdb_uninit();
rtnl_af_unregister(&br_af_ops);
rtnl_link_unregister(&br_link_ops);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 0c008b3d24cc..9a060c3c7713 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1169,6 +1169,8 @@ unsigned long br_timer_value(const struct timer_list *timer);

#if IS_ENABLED(CONFIG_BRIDGE_MRP)
/* br_mrp.c */
+void br_mrp_netlink_init(void);
+void br_mrp_netlink_uninit(void);
void br_mrp_uninit(struct net_bridge *br);
void br_mrp_port_uninit(struct net_bridge_port *p);
void br_mrp_port_link_change(struct net_bridge_port *br, bool up);
diff --git a/net/bridge/br_private_mrp.h b/net/bridge/br_private_mrp.h
index 00ee20582ac9..13fd2330ccfc 100644
--- a/net/bridge/br_private_mrp.h
+++ b/net/bridge/br_private_mrp.h
@@ -174,6 +174,15 @@ struct br_mrp {
u16 react_on_link_change;
};

+/* Represents the configuration of the MRP instance */
+struct br_mrp_config {
+ u32 p_ifindex;
+ u32 s_ifindex;
+ u32 ring_role;
+ u32 ring_nr;
+ u32 ring_state;
+};
+
/* br_mrp.c */
void br_mrp_ring_test_req(struct br_mrp *mrp, u32 interval);
void br_mrp_ring_topo_req(struct br_mrp *mrp, u32 interval);
diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
index c97fdae8f71b..7c110fdb9e1e 100644
--- a/security/selinux/nlmsgtab.c
+++ b/security/selinux/nlmsgtab.c
@@ -85,6 +85,9 @@ static const struct nlmsg_perm nlmsg_route_perms[] =
{ RTM_GETNEXTHOP, NETLINK_ROUTE_SOCKET__NLMSG_READ },
{ RTM_NEWLINKPROP, NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
{ RTM_DELLINKPROP, NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
+ { RTM_NEWMRP, NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
+ { RTM_DELMRP, NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
+ { RTM_GETMRP, NETLINK_ROUTE_SOCKET__NLMSG_READ },
};

static const struct nlmsg_perm nlmsg_tcpdiag_perms[] =
@@ -168,7 +171,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
* structures at the top of this file with the new mappings
* before updating the BUILD_BUG_ON() macro!
*/
- BUILD_BUG_ON(RTM_MAX != (RTM_NEWLINKPROP + 3));
+ BUILD_BUG_ON(RTM_MAX != (RTM_DELMRP + 3));
err = nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms,
sizeof(nlmsg_route_perms));
break;
--
2.17.1

2020-01-13 14:02:56

by Andrew Lunn

[permalink] [raw]
Subject: Re: [RFC net-next Patch v2 4/4] net: bridge: mrp: switchdev: Add HW offload

On Mon, Jan 13, 2020 at 01:46:20PM +0100, Horatiu Vultur wrote:
> +#ifdef CONFIG_BRIDGE_MRP
> +/* SWITCHDEV_OBJ_ID_PORT_MRP */
> +struct switchdev_obj_port_mrp {
> + struct switchdev_obj obj;
> + struct net_device *port;
> + u32 ring_nr;
> +};
> +
> +#define SWITCHDEV_OBJ_PORT_MRP(OBJ) \
> + container_of((OBJ), struct switchdev_obj_port_mrp, obj)
> +
> +/* SWITCHDEV_OBJ_ID_RING_TEST_MRP */
> +struct switchdev_obj_ring_test_mrp {
> + struct switchdev_obj obj;
> + /* The value is in us and a value of 0 represents to stop */
> + u32 interval;
> + u8 max;
> + u32 ring_nr;
> +};
> +
> +#define SWITCHDEV_OBJ_RING_TEST_MRP(OBJ) \
> + container_of((OBJ), struct switchdev_obj_ring_test_mrp, obj)
> +
> +/* SWITCHDEV_OBJ_ID_RING_ROLE_MRP */
> +struct switchdev_obj_ring_role_mrp {
> + struct switchdev_obj obj;
> + u8 ring_role;
> + u32 ring_nr;
> +};

Hi Horatiu

The structures above should give me enough information to build this,
correct?

Ethernet II, Src: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1), Dst: Iec_00:00:01 (01:15:4e:00:00:01)
Destination: Iec_00:00:01 (01:15:4e:00:00:01)
Source: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1)
Type: MRP (0x88e3)
PROFINET MRP MRP_Test, MRP_Common, MRP_End
MRP_Version: 1
MRP_TLVHeader.Type: MRP_Test (0x02)
MRP_TLVHeader.Type: MRP_Test (0x02)
MRP_TLVHeader.Length: 18
MRP_Prio: 0x1f40 High priorities
MRP_SA: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1)
MRP_PortRole: Primary ring port (0x0000)
MRP_RingState: Ring closed (0x0001)
MRP_Transition: 0x0001
MRP_TimeStamp [ms]: 0x000cf574 <---------- Updated automatic
MRP_TLVHeader.Type: MRP_Common (0x01)
MRP_TLVHeader.Type: MRP_Common (0x01)
MRP_TLVHeader.Length: 18
MRP_SequenceID: 0x00e9 <---------- Updated automatic
MRP_DomainUUID: ffffffff-ffff-ffff-ffff-ffffffffffff
MRP_TLVHeader.Type: MRP_End (0x00)
MRP_TLVHeader.Type: MRP_End (0x00)
MRP_TLVHeader.Length: 0

There are a couple of fields i don't see. MRP_SA, MRP_Transition.

What are max and ring_nr used for?

Do you need to set the first value MRP_SequenceID uses? Often, in
order to detect a reset, a random value is used to initialise the
sequence number. Also, does the time stamp need initializing?

Thanks
Andrew

2020-01-13 22:59:19

by Horatiu Vultur

[permalink] [raw]
Subject: Re: [RFC net-next Patch v2 4/4] net: bridge: mrp: switchdev: Add HW offload

The 01/13/2020 15:00, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Mon, Jan 13, 2020 at 01:46:20PM +0100, Horatiu Vultur wrote:
> > +#ifdef CONFIG_BRIDGE_MRP
> > +/* SWITCHDEV_OBJ_ID_PORT_MRP */
> > +struct switchdev_obj_port_mrp {
> > + struct switchdev_obj obj;
> > + struct net_device *port;
> > + u32 ring_nr;
> > +};
> > +
> > +#define SWITCHDEV_OBJ_PORT_MRP(OBJ) \
> > + container_of((OBJ), struct switchdev_obj_port_mrp, obj)
> > +
> > +/* SWITCHDEV_OBJ_ID_RING_TEST_MRP */
> > +struct switchdev_obj_ring_test_mrp {
> > + struct switchdev_obj obj;
> > + /* The value is in us and a value of 0 represents to stop */
> > + u32 interval;
> > + u8 max;
> > + u32 ring_nr;
> > +};
> > +
> > +#define SWITCHDEV_OBJ_RING_TEST_MRP(OBJ) \
> > + container_of((OBJ), struct switchdev_obj_ring_test_mrp, obj)
> > +
> > +/* SWITCHDEV_OBJ_ID_RING_ROLE_MRP */
> > +struct switchdev_obj_ring_role_mrp {
> > + struct switchdev_obj obj;
> > + u8 ring_role;
> > + u32 ring_nr;
> > +};
>
> Hi Horatiu
>
> The structures above should give me enough information to build this,
> correct?

Hi Andrew,

You will need also these attributes to build a minimum MRP_Test frame:
SWITCHDEV_ATTR_ID_MRP_PORT_STATE,
SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
SWITCHDEV_ATTR_ID_MRP_RING_STATE,
SWITCHDEV_ATTR_ID_MRP_RING_TRANS,

>
> Ethernet II, Src: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1), Dst: Iec_00:00:01 (01:15:4e:00:00:01)
> Destination: Iec_00:00:01 (01:15:4e:00:00:01)
> Source: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1)
> Type: MRP (0x88e3)
> PROFINET MRP MRP_Test, MRP_Common, MRP_End
> MRP_Version: 1
> MRP_TLVHeader.Type: MRP_Test (0x02)
> MRP_TLVHeader.Type: MRP_Test (0x02)
> MRP_TLVHeader.Length: 18
> MRP_Prio: 0x1f40 High priorities
> MRP_SA: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1)
> MRP_PortRole: Primary ring port (0x0000)
> MRP_RingState: Ring closed (0x0001)
> MRP_Transition: 0x0001
> MRP_TimeStamp [ms]: 0x000cf574 <---------- Updated automatic
> MRP_TLVHeader.Type: MRP_Common (0x01)
> MRP_TLVHeader.Type: MRP_Common (0x01)
> MRP_TLVHeader.Length: 18
> MRP_SequenceID: 0x00e9 <---------- Updated automatic
> MRP_DomainUUID: ffffffff-ffff-ffff-ffff-ffffffffffff
> MRP_TLVHeader.Type: MRP_End (0x00)
> MRP_TLVHeader.Type: MRP_End (0x00)
> MRP_TLVHeader.Length: 0
>
> There are a couple of fields i don't see. MRP_SA, MRP_Transition.

Regarding the MRP_SA, which represents the bridge MAC address, we could
get this information from listening to the notifications in the driver.
So I don't think we need a special call for this.

The same could be for MRP_Transition, which counts the number of times
the ring goes in open state. In theory we could get information by
counting in the driver how many times the ring gets in the open state.
And we get this information through the attribute
SWITCHDEV_ATTR_ID_MRP_RING_STATE.

The other fields that are missing are MRP_Prio and MRP_DomainUUID. But
these values could be set to a default values for now because they are
used by MRA(Media Redundancy Auto-manager), which is not part of this
patch series.

>
> What are max and ring_nr used for?

The max represents the number of MRP_Test frames that can be missed
by receiver before it declares the ring open. For example if the
receiver expects a MRP_Frame every 10ms and it sets the max to 3. Then
it means that if it didn't receive a frame in 30ms, it would set that
the port didn't receive MRP_Test.
The ring_nr represents the ID of the MRP instance. For example, on a
switch which has 8 ports, there can be 4 MRP instances. Because each
instance requires 2 ports. And to be able to differences them, each
instance has it's own ID, which is this ring_nr.

>
> Do you need to set the first value MRP_SequenceID uses? Often, in
> order to detect a reset, a random value is used to initialise the
> sequence number. Also, does the time stamp need initializing?

I couldn't see in the standard if they required an initial for
MRP_SequenceID. From what I have seen on some switches that have their
own MRP implementation, they set the initial value of MRP_SequenceID to
0 and they increase for it frame.
Regarding the timestamp, again the standard doesn't say anything about
initial value. This timestamp is used by MRM to determine the maximum
travel time of the MRP_Test frames in a ring.
>
> Thanks
> Andrew

--
/Horatiu

2020-01-13 23:36:31

by Andrew Lunn

[permalink] [raw]
Subject: Re: [RFC net-next Patch v2 4/4] net: bridge: mrp: switchdev: Add HW offload

Hi Horatiu

It has been said a few times what the basic state machine should be in
user space. A pure software solution can use raw sockets to send and
receive MRP_Test test frames. When considering hardware acceleration,
the switchdev API you have proposed here seems quite simple. It should
not be too hard to map it to a set of netlink messages from userspace.

Yet your argument for kernel, not user space, is you are worried about
the parameters which need to be passed to the hardware offload engine.
In order to win the argument for a kernel solution, we are going to
need a better idea what you think this problem is. The MRP_Test is TLV
based. Are there other things which could be in this message? Is that
what you are worried about?

Thanks
Andrew

2020-01-14 08:10:33

by Horatiu Vultur

[permalink] [raw]
Subject: Re: [RFC net-next Patch v2 4/4] net: bridge: mrp: switchdev: Add HW offload

The 01/14/2020 00:30, Andrew Lunn wrote:
>
> Hi Horatiu
>
> It has been said a few times what the basic state machine should be in
> user space. A pure software solution can use raw sockets to send and
> receive MRP_Test test frames. When considering hardware acceleration,
> the switchdev API you have proposed here seems quite simple. It should
> not be too hard to map it to a set of netlink messages from userspace.

Yes and we will try to go with this approach, to have a user space
application that contains the state machines and then in the kernel to
extend the netlink messages to map to the switchdev API.
So we will create a new RFC once we will have the user space and the
definition of the netlink messages.

>
> Yet your argument for kernel, not user space, is you are worried about
> the parameters which need to be passed to the hardware offload engine.
> In order to win the argument for a kernel solution, we are going to
> need a better idea what you think this problem is. The MRP_Test is TLV
> based. Are there other things which could be in this message? Is that
> what you are worried about?

>
> Thanks
> Andrew

--
/Horatiu

2020-01-14 13:23:11

by Andrew Lunn

[permalink] [raw]
Subject: Re: [RFC net-next Patch v2 4/4] net: bridge: mrp: switchdev: Add HW offload

On Tue, Jan 14, 2020 at 09:08:56AM +0100, Horatiu Vultur wrote:
> The 01/14/2020 00:30, Andrew Lunn wrote:
> >
> > Hi Horatiu
> >
> > It has been said a few times what the basic state machine should be in
> > user space. A pure software solution can use raw sockets to send and
> > receive MRP_Test test frames. When considering hardware acceleration,
> > the switchdev API you have proposed here seems quite simple. It should
> > not be too hard to map it to a set of netlink messages from userspace.
>
> Yes and we will try to go with this approach, to have a user space
> application that contains the state machines and then in the kernel to
> extend the netlink messages to map to the switchdev API.
> So we will create a new RFC once we will have the user space and the
> definition of the netlink messages.

Cool.

Before you get too far, we might want to discuss exactly how you pass
these netlink messages. Do we want to make this part of the new
ethtool Netlink implementation? Part of devlink? Extend the current
bridge netlink interface used by userspae RSTP daemons? A new generic
netlink socket?

Extending the bridge netlink interface might seem the most logical.
The argument against it, is that the kernel bridge code probably does
not need to know anything about this offloading. But it does allow you
to make use of the switchdev API, so we have a uniform API between the
network stack and drivers implementing offloading.

Andrew

2020-01-14 16:47:39

by Horatiu Vultur

[permalink] [raw]
Subject: Re: [RFC net-next Patch v2 4/4] net: bridge: mrp: switchdev: Add HW offload

The 01/14/2020 14:20, Andrew Lunn wrote:
>
> On Tue, Jan 14, 2020 at 09:08:56AM +0100, Horatiu Vultur wrote:
> > The 01/14/2020 00:30, Andrew Lunn wrote:
> > >
> > > Hi Horatiu
> > >
> > > It has been said a few times what the basic state machine should be in
> > > user space. A pure software solution can use raw sockets to send and
> > > receive MRP_Test test frames. When considering hardware acceleration,
> > > the switchdev API you have proposed here seems quite simple. It should
> > > not be too hard to map it to a set of netlink messages from userspace.
> >
> > Yes and we will try to go with this approach, to have a user space
> > application that contains the state machines and then in the kernel to
> > extend the netlink messages to map to the switchdev API.
> > So we will create a new RFC once we will have the user space and the
> > definition of the netlink messages.
>
> Cool.
>
> Before you get too far, we might want to discuss exactly how you pass
> these netlink messages. Do we want to make this part of the new
> ethtool Netlink implementation? Part of devlink? Extend the current
> bridge netlink interface used by userspae RSTP daemons? A new generic
> netlink socket?

We are not yet 100% sure. We were thinking to choose between extending
the bridge netlink interface or adding a new netlink socket. I was
leaning to create a new netlink socket, because I think that would be
clearer and easier to understand. But I don't have much experience with
this, so in both cases I need to sit down and actually try to implement
it to see exactly.

>
> Extending the bridge netlink interface might seem the most logical.
> The argument against it, is that the kernel bridge code probably does
> not need to know anything about this offloading. But it does allow you
> to make use of the switchdev API, so we have a uniform API between the
> network stack and drivers implementing offloading.
>
> Andrew

--
/Horatiu