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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <546FD9E6.6080205@cumulusnetworks.com>
Date:	Fri, 21 Nov 2014 16:33:42 -0800
From:	Roopa Prabhu <roopa@...ulusnetworks.com>
To:	Thomas Graf <tgraf@...g.ch>
CC:	jiri@...nulli.us, sfeldma@...il.com, jhs@...atatu.com,
	bcrl@...ck.org, john.fastabend@...il.com,
	stephen@...workplumber.org, linville@...driver.com,
	nhorman@...driver.com, nicolas.dichtel@...nd.com,
	vyasevic@...hat.com, f.fainelli@...il.com, buytenh@...tstofly.org,
	aviadr@...lanox.com, netdev@...r.kernel.org, davem@...emloft.net,
	Shrijeet Mukherjee <shm@...ulusnetworks.com>,
	gospo@...ulusnetworks.com
Subject: Re: [RFC PATCH 4/4] bridge: make hw offload conditional on bridge
 and bridge port offload flags

On 11/21/14, 3:30 PM, Thomas Graf wrote:
> On 11/21/14 at 02:49pm, roopa@...ulusnetworks.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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html