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

[Thomas Graf <tgraf@suug.ch>]

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.