Linux Ethernet Bridging - Mar 2018 - [Bridge] [PATCH net-next 1/5] bridge: initialize port flags with switchdev defaults

Main goal of the patchset is to allow for flooding offload to switchdev
in scenarios when HW switchdev ports and SW ports are operating in a
single bridge.

In case a data frame that needs to be flooded is ingressed on a SW port,
    it needs to be flooded to a single HW port of any given
    switchdev-capable hardware only. Switchdev hardware will than take care
    about flooding to the rest of the ports that it manages.


Igor Mitsyanko (5):
  bridge: initialize port flags with switchdev defaults
  bridge: propagate BR_ flags updates through sysfs to switchdev
  bridge: allow switchdev port to handle flooding by itself
  bridge: provide sysfs and netlink interface to set BR_FLOOD_OFFLOAD
  bridge: verify "HW only" flags can't be set without HW support

 include/linux/if_bridge.h    |  1 +
 include/uapi/linux/if_link.h |  1 +
 net/bridge/br_forward.c      |  2 ++
 net/bridge/br_if.c           | 17 ++++++++++++++++-
 net/bridge/br_netlink.c      |  8 +++++++-
 net/bridge/br_private.h      | 13 ++++++++++++-
 net/bridge/br_switchdev.c    | 14 +++++++++++++-
 net/bridge/br_sysfs_if.c     | 17 +++++++++++++----
 8 files changed, 65 insertions(+), 8 deletions(-)

-- 
2.9.5

Default bridge port flags for switchdev devices can be different from
what is used in bridging core. Get default value from switchdev itself
on port initialization.

Signed-off-by: Igor Mitsyanko <igor.mitsyanko.os at quantenna.com>
---
 net/bridge/br_if.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 9ba4ed6..d658b55 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -342,6 +342,21 @@ static int find_portno(struct net_bridge *br)
 	return (index >= BR_MAX_PORTS) ? -EXFULL : index;
 }
 
+static unsigned long nbp_flags_get_default(struct net_device *dev)
+{
+	struct switchdev_attr attr = {
+		.orig_dev = dev,
+		.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS,
+	};
+	int ret;
+
+	ret = switchdev_port_attr_get(dev, &attr);
+	if (ret)
+		return BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
+	else
+		return attr.u.brport_flags;
+}
+
 /* called with RTNL but without bridge lock */
 static struct net_bridge_port *new_nbp(struct net_bridge *br,
 				       struct net_device *dev)
@@ -363,7 +378,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge
*br,
 	p->path_cost = port_cost(dev);
 	p->priority = 0x8000 >> BR_PORT_BITS;
 	p->port_no = index;
-	p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
+	p->flags = nbp_flags_get_default(dev);
 	br_init_port(p);
 	br_set_state(p, BR_STATE_DISABLED);
 	br_stp_port_timer_init(p);
-- 
2.9.5

sysfs interface to configure bridge flags only updates SW copy but does
not notify hardware through switchdev interface. Make sure it is.

Signed-off-by: Igor Mitsyanko <igor.mitsyanko.os at quantenna.com>
---
 net/bridge/br_sysfs_if.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 126a8ea..9bdd177 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -51,6 +51,7 @@ static int store_flag(struct net_bridge_port *p, unsigned long
v,
 		      unsigned long mask)
 {
 	unsigned long flags;
+	int err;
 
 	flags = p->flags;
 
@@ -59,10 +60,16 @@ static int store_flag(struct net_bridge_port *p, unsigned
long v,
 	else
 		flags &= ~mask;
 
-	if (flags != p->flags) {
-		p->flags = flags;
-		br_port_flags_change(p, mask);
-	}
+	if (flags == p->flags)
+		return 0;
+
+	err = br_switchdev_set_port_flag(p, flags, mask);
+	if (err)
+		return err;
+
+	p->flags = flags;
+	br_port_flags_change(p, mask);
+
 	return 0;
 }
 
-- 
2.9.5

Introduce BR_FLOOD_OFFLOAD bridge port flag that can be used by
switchdev-capable hardware to advertize that it wants to handle all
flooding by itself.
In that case there is no need for a driver to set skb::offload_fwd_mark
on each offloaded packet as it is implied by BR_FLOOD_OFFLOAD bridge
port flag.

BR_FLOOD_OFFLOAD port flags helps in two scenarios:
  1. Mixed bridge configuration with SW ports and switchdev-capable ports.
In case a data frame that needs to be flooded is ingressed on a SW port,
it needs to be flooded to a single HW port of any given
switchdev-capable hardware only. Switchdev hardware will than take care
about flooding to the rest of the ports that it manages.
  2. Switch driver does not have to identify frames that were flooded by
hardware and explicitly mark them with skb::offload_fwd_mark. Assuming
it knows that hardware will always handle flooding by itself, it can
simply advertise BR_FLOOD_OFFLOAD port flag.

Note: current implementation can only handle a single switchdev-capable
device in a single port. Frame will be flooded as usual to all ports of
any additional switchdev present in a given bridge.

Signed-off-by: Igor Mitsyanko <igor.mitsyanko.os at quantenna.com>
---
 include/linux/if_bridge.h |  1 +
 net/bridge/br_forward.c   |  2 ++
 net/bridge/br_private.h   |  8 ++++++++
 net/bridge/br_switchdev.c | 14 +++++++++++++-
 4 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index 02639eb..5d0e277 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -50,6 +50,7 @@ struct br_ip_list {
 #define BR_VLAN_TUNNEL		BIT(13)
 #define BR_BCAST_FLOOD		BIT(14)
 #define BR_NEIGH_SUPPRESS	BIT(15)
+#define BR_FLOOD_OFFLOAD	BIT(16)
 
 #define BR_DEFAULT_AGEING_TIME	(300 * HZ)
 
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index b4eed11..ac761a9 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -163,6 +163,8 @@ static struct net_bridge_port *maybe_deliver(
 	if (!should_deliver(p, skb))
 		return prev;
 
+	nbp_switchdev_offload_fwd_track(p, skb);
+
 	if (!prev)
 		goto out;
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 8e13a64..a6d2f2b 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1111,6 +1111,8 @@ void nbp_switchdev_frame_mark(const struct net_bridge_port
*p,
 			      struct sk_buff *skb);
 bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p,
 				  const struct sk_buff *skb);
+void nbp_switchdev_offload_fwd_track(const struct net_bridge_port *p,
+				     struct sk_buff *skb);
 int br_switchdev_set_port_flag(struct net_bridge_port *p,
 			       unsigned long flags,
 			       unsigned long mask);
@@ -1138,6 +1140,12 @@ static inline bool nbp_switchdev_allowed_egress(const
struct net_bridge_port *p,
 	return true;
 }
 
+static inline void
+nbp_switchdev_offload_fwd_track(const struct net_bridge_port *p,
+				struct sk_buff *skb)
+{
+}
+
 static inline int br_switchdev_set_port_flag(struct net_bridge_port *p,
 					     unsigned long flags,
 					     unsigned long mask)
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index ee775f4..aee3c01 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -46,6 +46,9 @@ int nbp_switchdev_mark_set(struct net_bridge_port *p)
 void nbp_switchdev_frame_mark(const struct net_bridge_port *p,
 			      struct sk_buff *skb)
 {
+	if (p->flags & BR_FLOOD_OFFLOAD)
+		skb->offload_fwd_mark = 1;
+
 	if (skb->offload_fwd_mark &&
!WARN_ON_ONCE(!p->offload_fwd_mark))
 		BR_INPUT_SKB_CB(skb)->offload_fwd_mark = p->offload_fwd_mark;
 }
@@ -57,8 +60,17 @@ bool nbp_switchdev_allowed_egress(const struct
net_bridge_port *p,
 	       BR_INPUT_SKB_CB(skb)->offload_fwd_mark != p->offload_fwd_mark;
 }
 
+void nbp_switchdev_offload_fwd_track(const struct net_bridge_port *p,
+				     struct sk_buff *skb)
+{
+	if (skb->offload_fwd_mark || !(p->flags & BR_FLOOD_OFFLOAD))
+		return;
+
+	nbp_switchdev_frame_mark(p, skb);
+}
+
 /* Flags that can be offloaded to hardware */
-#define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | \
+#define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | BR_FLOOD_OFFLOAD | \
 				  BR_MCAST_FLOOD | BR_BCAST_FLOOD)
 
 int br_switchdev_set_port_flag(struct net_bridge_port *p,
-- 
2.9.5

Allow to modify BR_FLOOD_OFFLOAD flag value through both sysfs and
netlink interface.

Signed-off-by: Igor Mitsyanko <igor.mitsyanko.os at quantenna.com>
---
 include/uapi/linux/if_link.h | 1 +
 net/bridge/br_netlink.c      | 8 +++++++-
 net/bridge/br_sysfs_if.c     | 2 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 11d0c0e..5d3eb7f 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -333,6 +333,7 @@ enum {
 	IFLA_BRPORT_BCAST_FLOOD,
 	IFLA_BRPORT_GROUP_FWD_MASK,
 	IFLA_BRPORT_NEIGH_SUPPRESS,
+	IFLA_BRPORT_FLOOD_OFFLOAD,
 	__IFLA_BRPORT_MAX
 };
 #define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 015f465c..26f7c51 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -213,7 +213,9 @@ static int br_port_fill_attrs(struct sk_buff *skb,
 							BR_VLAN_TUNNEL)) ||
 	    nla_put_u16(skb, IFLA_BRPORT_GROUP_FWD_MASK, p->group_fwd_mask) ||
 	    nla_put_u8(skb, IFLA_BRPORT_NEIGH_SUPPRESS,
-		       !!(p->flags & BR_NEIGH_SUPPRESS)))
+		       !!(p->flags & BR_NEIGH_SUPPRESS)) ||
+	    nla_put_u8(skb, IFLA_BRPORT_FLOOD_OFFLOAD,
+		       !!(p->flags & BR_FLOOD_OFFLOAD)))
 		return -EMSGSIZE;
 
 	timerval = br_timer_value(&p->message_age_timer);
@@ -763,6 +765,10 @@ static int br_setport(struct net_bridge_port *p, struct
nlattr *tb[])
 	if (err)
 		return err;
 
+	err = br_set_port_flag(p, tb, IFLA_BRPORT_FLOOD_OFFLOAD, BR_FLOOD_OFFLOAD);
+	if (err)
+		return err;
+
 	if (br_vlan_tunnel_old && !(p->flags & BR_VLAN_TUNNEL))
 		nbp_vlan_tunnel_info_flush(p);
 
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 9bdd177..3be600d 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -199,6 +199,7 @@ BRPORT_ATTR_FLAG(proxyarp_wifi, BR_PROXYARP_WIFI);
 BRPORT_ATTR_FLAG(multicast_flood, BR_MCAST_FLOOD);
 BRPORT_ATTR_FLAG(broadcast_flood, BR_BCAST_FLOOD);
 BRPORT_ATTR_FLAG(neigh_suppress, BR_NEIGH_SUPPRESS);
+BRPORT_ATTR_FLAG(flood_offload, BR_FLOOD_OFFLOAD);
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 static ssize_t show_multicast_router(struct net_bridge_port *p, char *buf)
@@ -250,6 +251,7 @@ static const struct brport_attribute *brport_attrs[] = {
 	&brport_attr_broadcast_flood,
 	&brport_attr_group_fwd_mask,
 	&brport_attr_neigh_suppress,
+	&brport_attr_flood_offload,
 	NULL
 };
 
-- 
2.9.5

Setting bridge flag BR_FLOOD_OFFLOAD only makes sense if underlying
port hardware advertises support for it. Make sure kernel checks that
condition before allowing to update the flag value.

Signed-off-by: Igor Mitsyanko <igor.mitsyanko.os at quantenna.com>
---
 net/bridge/br_private.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index a6d2f2b..6d2f8b1 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -50,6 +50,9 @@ enum {
 /* Path to usermode spanning tree program */
 #define BR_STP_PROG	"/sbin/bridge-stp"
 
+/* Flags that can only be set if HW supports it */
+#define BR_PORT_FLAGS_HW_ONLY	BR_FLOOD_OFFLOAD
+
 typedef struct bridge_id bridge_id;
 typedef struct mac_addr mac_addr;
 typedef __u16 port_id;
@@ -1150,7 +1153,7 @@ static inline int br_switchdev_set_port_flag(struct
net_bridge_port *p,
 					     unsigned long flags,
 					     unsigned long mask)
 {
-	return 0;
+	return mask & BR_PORT_FLAGS_HW_ONLY ? -EOPNOTSUPP : 0;
 }
 
 static inline void
-- 
2.9.5

On Fri, Mar 09, 2018 at 07:03:03PM -0800, Igor Mitsyanko
wrote:
> Main goal of the patchset is to allow for flooding offload to switchdev
> in scenarios when HW switchdev ports and SW ports are operating in a
> single bridge.
> 
> In case a data frame that needs to be flooded is ingressed on a SW port,
>     it needs to be flooded to a single HW port of any given
>     switchdev-capable hardware only. Switchdev hardware will than take care
>     about flooding to the rest of the ports that it manages.
Hi Igor

You don't appear to be adding any user of this. Please also make one
of the switchdev drivers actually use this new functionality.

   Andrew