Re: [PATCH net-next] rocker: check for BRIDGE_FLAGS_SELF in bridge setlink handler

[roopa <roopa@cumulusnetworks.com>]

On 3/8/15, 4:17 PM, Scott Feldman wrote:
> On Sun, Mar 8, 2015 at 7:19 AM, roopa <roopa@cumulusnetworks.com> wrote:
>> On 3/6/15, 1:52 AM, Scott Feldman wrote:
> [cut]
>
>>> I'm saying port driver's ndo_bridge_setlink shouldn't be called unless
>>> SELF is set.  The port driver shouldn't have to check if it's SELF,
>>> that's my main beef with this patch.  If the app (iproute2) wants to
>>> mirror an attribute, then it can set both MASTER and SELF.  This is
>>> consistent with ndo_fdb ops, where MASTER targets the bridge's FDB and
>>> SELF targets the device's FDB.
>>
>> bridge setlink/dellink ndo sets/clears bridge port flags, vlans.
>>
>> 4 ways to the ndo op gets called (value in brackets is the bridge flags):
>>
>> 1. ndo_bridge_setlink ()
>> 2. ndo_bridge_setlink (master)
>> 3. ndo_bridge_setlink (self)
>> 4. ndo_bridge_setlink (master | self)
>>
>>
>> in case of 1) and 2) today, bridge driver also calls into the port driver
>> (as part of NETIF_F_HW_SWITCH_OFFLOAD) because vlans and other port flags
>> need to be offloaded if they are set on master (I understand that rocker
>> does not use bridge setlink ndo op for vlans. But it is completely valid for
>> a switch driver to use the ndo setlink op for offloading vlan config)
>>
>> One thing i can do is the NETIF_F_HW_SWITCH_OFFLOAD case only applies in
>> case 1 above. ie when bridge flags is zero.
>>
>> which leads to the following commands to disable learning in the bridge
>> driver and enable it in hw,
>>
>> 1. bridge link learning on - enable learning in both rocker and bridge
>> driver due to the NETIF_F_HW_SWITCH_OFFLOAD flag
>> 2. bridge link learning off master - learning disable only in the bridge
>> driver
>>
>> With this i think we hit a compromise, rocker setlink can be called when
>> flags is zero, but we still get a valid sequence of commands to
>> enable/disable certain port flags only in the bridge driver or port driver
>> without having an explicit self check in rocker.
>>
>> (my work laptop is broken..., I will re-post the patch tomorrow if you are
>> ok with the plan)
> I'm not OK with the plan; it doesn't address the issue.  I'm saying
> port driver's ndo_bridge_setlink shouldn't be called unless SELF is
> set.

but, why scott ?. With l2, l3 and all other offloads, we try not to 
change user commands.
Before switchdev 'self' was always used to go to nic drivers directly to 
program fdb.
l2 never used the bridge driver.

with switchdev l2 offloads, we are using the bridge driver.
But there is a need to turn off/on some attributes in hw independently of
attributes in the kernel bridge driver. To that i had suggested we 
continue to use 'self'
for those attributes without introducing a new hw mode.
>
> We have this currently, looking from the user's cmd:
>
> bridge link set                               calls into bridge and port driver
> bridge link set master                    calls into bridge and port driver
> bridge link set self                         calls into port driver
> bridge link set self master              calls into bridge and port driver
>
> Your proposal above changes it to:
>
> bridge link set                               calls into bridge and port driver
> bridge link set master                    calls into bridge
> bridge link set self                         calls into port driver
> bridge link set self master              calls into bridge and port driver
>
> Which is still no good because the default case (neither self or
> master explicitly set) calls into the port driver.  Before all of
> these changes, we had:
>
> bridge link set                               calls into bridge
> bridge link set master                    calls into bridge
> bridge link set hwmode veb|vepa     calls into port driver
>
> hwmode was a way to set SELF.  We wanted to add "self" flag to command
> to be more like bridge fdb cmd.  I assumed the default case (bridge
> set link) would continue to work as before.  So we'd have:
>
> bridge link set                               calls into bridge
> bridge link set master                    calls into bridge
> bridge link set self                         calls into port driver
> bridge link set self master              calls into bridge and port driver
>
> But that's not what we got.  Your changes changed the default case
> (and the bridge link set master case) to call into both bridge and
> port driver even though user didn't specify self.

I have made sure that no default case has changed. It works this way 
only when
the driver advertises offload using NETIF_F_HW_SWITCH_OFFLOAD.
Which means the driver is interested in involving the bridge driver in 
all l2 offloads
(And this includes all bridge ndo ops setlink/dellink/stp/fdb).

http://www.spinics.net/lists/netdev/msg314407.html
>
> How hard would it be to get back to original default behavior?

Am not sure which default behavior you are talking about. If you are 
talking about default behavior before switchdev,
that has not changed. For switch devices which want the bridge driver 
involved and who advertise the NETIF_F_HW_SWITCH_OFFLOAD flag, this is 
new behavior. And rocker does advertise this flag today.