[RFC PATCH 4/4] bridge: make hw offload conditional on bridge and bridge port offload flags

[RFC PATCH 4/4] bridge: make hw offload conditional on bridge and bridge port offload flags

[roopa]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

If bridge has NETIF_F_HW_OFFLOAD feature flag set, offload all bridge and
bridge port attributes to hardware.

Two new flags BRPORT_KERNEL and BRPORT_HW_OFFLOAD to control offloading of
a few bridge port flags to hardware. These can be encoded in the upper bits
of all bridge port flag netlink attributes.

Control/Override bridge port flag (learning, flooding etc) offloading
with BRPORT_KERNEL and BRPORT_HW_OFFLOAD flags:
    If the brport offload flags are not present (current default)
	    - set bridge port flag attribute only in the kernel
    else:
        if BRPORT_KERNEL and BRPORT_HW_OFFLOAD:
            - set bridge port flag both in kernel and hw
        elif BRPORT_KERNEL:
            - set bridge port flag attribute only in kernel
        elif BRPORT_HW_OFFLOAD:
            - set bridge port flag attribute only in hw

The 'gets' needs more work. The idea is that the gets can also be controlled
by the KERNEL or HW_OFFLOAD flags.

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 net/bridge/br_netlink.c |   50 +++++++++++++++++++++++++++++++++++++----------
 net/bridge/br_private.h |    2 ++
 net/bridge/br_stp.c     |    9 ++++++---
 net/bridge/br_stp_if.c  |    8 ++++++--
 4 files changed, 54 insertions(+), 15 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 13fecf1..e92e810 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -57,12 +57,19 @@ static int br_port_fill_attrs(struct sk_buff *skb,
 	    nla_put_u16(skb, IFLA_BRPORT_PRIORITY, p->priority) ||
 	    nla_put_u32(skb, IFLA_BRPORT_COST, p->path_cost) ||
 	    nla_put_u8(skb, IFLA_BRPORT_MODE, mode) ||
-	    nla_put_u8(skb, IFLA_BRPORT_GUARD, !!(p->flags & BR_BPDU_GUARD)) ||
-	    nla_put_u8(skb, IFLA_BRPORT_PROTECT, !!(p->flags & BR_ROOT_BLOCK)) ||
-	    nla_put_u8(skb, IFLA_BRPORT_FAST_LEAVE, !!(p->flags & BR_MULTICAST_FAST_LEAVE)) ||
-	    nla_put_u8(skb, IFLA_BRPORT_LEARNING, !!(p->flags & BR_LEARNING)) ||
-	    nla_put_u8(skb, IFLA_BRPORT_UNICAST_FLOOD, !!(p->flags & BR_FLOOD)) ||
-	    nla_put_u8(skb, IFLA_BRPORT_PROXYARP, !!(p->flags & BR_PROXYARP)))
+	    nla_put_u8(skb, IFLA_BRPORT_GUARD,
+		           br_get_port_flag(p, IFLA_BRPORT_GUARD, BR_BPDU_GUARD)) ||
+	    nla_put_u8(skb, IFLA_BRPORT_PROTECT,
+	               br_get_port_flag(p, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK)) ||
+	    nla_put_u8(skb, IFLA_BRPORT_FAST_LEAVE,
+	               br_get_port_flag(p, IFLA_BRPORT_FAST_LEAVE,
+	               BR_MULTICAST_FAST_LEAVE)) ||
+	    nla_put_u8(skb, IFLA_BRPORT_LEARNING,
+	               br_get_port_flag(p, IFLA_BRPORT_LEARNING, BR_LEARNING)) ||
+	    nla_put_u8(skb, IFLA_BRPORT_UNICAST_FLOOD,
+			       br_get_port_flag(p, IFLA_UNICAST_FLOOD, BR_FLOOD)) ||
+	    nla_put_u8(skb, IFLA_BRPORT_PROXYARP, 
+	               br_get_port_flag(p, IFLA_UNICAST_FLOOD, BR_PROXYARP)))
 		return -EMSGSIZE;
 
 	return 0;
@@ -305,7 +312,9 @@ static int br_set_port_state(struct net_bridge_port *p, u8 state)
 
 	br_set_state(p, state);
 	br_log_state(p);
-	netdev_sw_port_stp_update(p->dev, p->state);
+
+	if (BR_HW_OFFLOAD(p->br))
+	    netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state);
 	br_port_state_selection(p->br);
 	return 0;
 }
@@ -316,13 +325,34 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[],
 {
 	if (tb[attrtype]) {
 		u8 flag = nla_get_u8(tb[attrtype]);
-		if (flag)
-			p->flags |= mask;
-		else
+		if (flag) {
+			flag_upper = flag & 0xf0
+			if (!flag_upper || (flag_upper & BRPORT_KERNEL))
+				p->flags |= mask;
+			if ((flag_upper & BRPORT_HW_OFFLOAD) ||
+				(BR_HW_OFFLOAD(p->br)))
+				/* Also set the port flag in hw */
+			netdev_sw_port_set_attr(p->dev, attrtype, 1);
+		} else {
 			p->flags &= ~mask;
+			if (BR_HW_OFFLOAD(p->br))
+				netdev_sw_port_set_attr(p->dev, attrtype, 0);
+		}
 	}
 }
 
+/* Set/clear or port flags based on attribute */
+static u8 br_get_port_flag(struct net_bridge_port *p,
+			   int attrtype, u8 flag)
+{
+	attrvalue = !!(p->flags & flag)
+	if (attrvalue)
+		attrvalue |= BRPORT_KERNEL
+	if (netdev_sw_port_get_flag(p->dev, attrtype))
+		attrvalue |= BRPORT_HW_OFFLOAD
+	return attrvalue;
+}
+
 /* Process bridge protocol info on port */
 static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
 {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 8f3f081..3ebd196 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -41,6 +41,8 @@
 /* Path to usermode spanning tree program */
 #define BR_STP_PROG	"/sbin/bridge-stp"
 
+#define BR_HW_OFFLOAD(br) !!(br->dev->features & NETIF_F_HW_OFFLOAD)
+
 typedef struct bridge_id bridge_id;
 typedef struct mac_addr mac_addr;
 typedef __u16 port_id;
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index c00139b..bb61dc0 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -115,7 +115,8 @@ static void br_root_port_block(const struct net_bridge *br,
 
 	br_set_state(p, BR_STATE_LISTENING);
 	br_log_state(p);
-	netdev_sw_port_stp_update(p->dev, p->state);
+	if (BR_HW_OFFLOAD(p->br))
+	    netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state);
 	br_ifinfo_notify(RTM_NEWLINK, p);
 
 	if (br->forward_delay > 0)
@@ -396,7 +397,8 @@ static void br_make_blocking(struct net_bridge_port *p)
 
 		br_set_state(p, BR_STATE_BLOCKING);
 		br_log_state(p);
-		netdev_sw_port_stp_update(p->dev, p->state);
+		if (BR__HW_OFFLOAD(p->br))
+	        netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state);
 		br_ifinfo_notify(RTM_NEWLINK, p);
 
 		del_timer(&p->forward_delay_timer);
@@ -422,7 +424,8 @@ static void br_make_forwarding(struct net_bridge_port *p)
 
 	br_multicast_enable_port(p);
 	br_log_state(p);
-	netdev_sw_port_stp_update(p->dev, p->state);
+	if (BR_OFFLOAD(p->br))
+	    netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state);
 	br_ifinfo_notify(RTM_NEWLINK, p);
 
 	if (br->forward_delay != 0)
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 91279f8..8435b4d 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -90,7 +90,8 @@ void br_stp_enable_port(struct net_bridge_port *p)
 	br_init_port(p);
 	br_port_state_selection(p->br);
 	br_log_state(p);
-	netdev_sw_port_stp_update(p->dev, p->state);
+	if (BR_HW_OFFLOAD(p->br))
+	    netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state);
 	br_ifinfo_notify(RTM_NEWLINK, p);
 }
 
@@ -107,7 +108,8 @@ void br_stp_disable_port(struct net_bridge_port *p)
 	p->config_pending = 0;
 
 	br_log_state(p);
-	netdev_sw_port_stp_update(p->dev, p->state);
+	if (BR_HW_OFFLOAD(p->br))
+	    netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state);
 	br_ifinfo_notify(RTM_NEWLINK, p);
 
 	del_timer(&p->message_age_timer);
@@ -290,6 +292,8 @@ int br_stp_set_port_priority(struct net_bridge_port *p, unsigned long newprio)
 		br_become_designated_port(p);
 		br_port_state_selection(p->br);
 	}
+	if (BR_HW_OFFLOAD(p->br))
+	    netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_PRIORITY, &newprio)
 
 	return 0;
 }
-- 
1.7.10.4

Re: [RFC PATCH 4/4] bridge: make hw offload conditional on bridge and bridge port offload flags

[Thomas Graf]

On 11/21/14 at 02:49pm, roopa@cumulusnetworks.com wrote:
> +	    nla_put_u8(skb, IFLA_BRPORT_GUARD,
> +		           br_get_port_flag(p, IFLA_BRPORT_GUARD, BR_BPDU_GUARD)) ||

A helper taking nla_put_br_flag(port, skb, attrtype, brflag) would simplify
this code a lot.

> @@ -305,7 +312,9 @@ static int br_set_port_state(struct net_bridge_port *p, u8 state)
>  
>  	br_set_state(p, state);
>  	br_log_state(p);
> -	netdev_sw_port_stp_update(p->dev, p->state);
> +
> +	if (BR_HW_OFFLOAD(p->br))
> +	    netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state);

I assume the yet unfinished netdev_sw_port_set_attr() will call
netdev_sw_port_stp_update()?

> @@ -316,13 +325,34 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[],
>  {
>  	if (tb[attrtype]) {
>  		u8 flag = nla_get_u8(tb[attrtype]);
> -		if (flag)
> -			p->flags |= mask;
> -		else
> +		if (flag) {
> +			flag_upper = flag & 0xf0
> +			if (!flag_upper || (flag_upper & BRPORT_KERNEL))
> +				p->flags |= mask;
> +			if ((flag_upper & BRPORT_HW_OFFLOAD) ||
> +				(BR_HW_OFFLOAD(p->br)))
> +				/* Also set the port flag in hw */
> +			netdev_sw_port_set_attr(p->dev, attrtype, 1);

I'm not sure I understand the || here. HW_OFFLOAD enabled on the
net_device is a conditional for all netdev_sw_port_set_attr() calls
and at the same time implies BRPORT_HW_OFFLOAD on all attributes.
As I read this code now, I don't see how you would offload individual
features to hardware.

> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 8f3f081..3ebd196 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -41,6 +41,8 @@
>  /* Path to usermode spanning tree program */
>  #define BR_STP_PROG	"/sbin/bridge-stp"
>  
> +#define BR_HW_OFFLOAD(br) !!(br->dev->features & NETIF_F_HW_OFFLOAD)

Let's not add more non type safe macros. A static inline seems like
a better fit here.

Re: [RFC PATCH 4/4] bridge: make hw offload conditional on bridge and bridge port offload flags

[Roopa Prabhu]

On 11/21/14, 3:30 PM, Thomas Graf wrote:
> On 11/21/14 at 02:49pm, roopa@cumulusnetworks.com wrote:
>> +	    nla_put_u8(skb, IFLA_BRPORT_GUARD,
>> +		           br_get_port_flag(p, IFLA_BRPORT_GUARD, BR_BPDU_GUARD)) ||
> A helper taking nla_put_br_flag(port, skb, attrtype, brflag) would simplify
> this code a lot.
sure
>
>> @@ -305,7 +312,9 @@ static int br_set_port_state(struct net_bridge_port *p, u8 state)
>>   
>>   	br_set_state(p, state);
>>   	br_log_state(p);
>> -	netdev_sw_port_stp_update(p->dev, p->state);
>> +
>> +	if (BR_HW_OFFLOAD(p->br))
>> +	    netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state);
> I assume the yet unfinished netdev_sw_port_set_attr() will call
> netdev_sw_port_stp_update()?

netdev_sw_port_set_attr will just call the switch driver ndo for bridge port attributes. (ndo_sw_bridge_port_set_attr()).
On that note it would be nice to rename the whole thing  (the other thread on "sw" vs "offload"), this could be
renamed to netdev_offload_bridge_port_set_attr() or something along those lines.

>
>> @@ -316,13 +325,34 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[],
>>   {
>>   	if (tb[attrtype]) {
>>   		u8 flag = nla_get_u8(tb[attrtype]);
>> -		if (flag)
>> -			p->flags |= mask;
>> -		else
>> +		if (flag) {
>> +			flag_upper = flag & 0xf0
>> +			if (!flag_upper || (flag_upper & BRPORT_KERNEL))
>> +				p->flags |= mask;
>> +			if ((flag_upper & BRPORT_HW_OFFLOAD) ||
>> +				(BR_HW_OFFLOAD(p->br)))
>> +				/* Also set the port flag in hw */
>> +			netdev_sw_port_set_attr(p->dev, attrtype, 1);
> I'm not sure I understand the || here. HW_OFFLOAD enabled on the
> net_device is a conditional for all netdev_sw_port_set_attr() calls
> and at the same time implies BRPORT_HW_OFFLOAD on all attributes.

Just realized this needs another condition, I will fix it.

The idea is, the port attribute flags BRPORT_KERNEL and BRPORT_HW_OFFLOAD
can be used to override the HW_OFFLOAD flag on the bridge netdev.

You will only call the swdev api to offload the bridge port flag,
if the bridge netdev has the HW_OFFLOAD flag set and also the per port 
attribute flag
does not override it.

> As I read this code now, I don't see how you would offload individual
> features to hardware.


This is for offloading individual bridge port flags like learning and 
flooding.

You want to be able to HW_OFFLOAD the bridge device, but be able to 
control offloading of some of the port flags
like learning and flooding. For example some of the realtek devices 
might want to turn learning in hw off by default
but learn in software.

>
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 8f3f081..3ebd196 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -41,6 +41,8 @@
>>   /* Path to usermode spanning tree program */
>>   #define BR_STP_PROG	"/sbin/bridge-stp"
>>   
>> +#define BR_HW_OFFLOAD(br) !!(br->dev->features & NETIF_F_HW_OFFLOAD)
> Let's not add more non type safe macros. A static inline seems like
> a better fit here.
sure, ack.

Thanks for the review.