Re: [PATCH net-next v2 0/3] switchdev: support stp updates on stacked netdevices

On Sun, Mar 15, 2015 at 12:17 PM, Jiri Pirko [off-list ref] wrote:

Sun, Mar 15, 2015 at 07:08:02PM CET, sfeldma@gmail.com wrote:

quoted

On Fri, Mar 13, 2015 at 9:31 AM, David Miller [off-list ref] wrote:

quoted

From: roopa <redacted>
Date: Fri, 13 Mar 2015 06:25:24 -0700

quoted

David, if you mean not touch bond and team but have the switchdev
api do it transparently, yes, i had it that way initially.  And i do
liked it that way as well. But the feedback i received (during the
initial introduction of this for setlink/dellink) was to make it
explicit for each master.

I think the concern is that we only want to do this for devices
for which it is safe to "traverse" down like this.

But frankly I cannot think of any layered device where we would
not want to do this.

Let's go back to the simple scheme where we unconditionally traverse
and if we hit a problem case we'll figure out how to deal with it
then, ok?

Here's a way to do it without touching team/bonding/vlan drivers, but
also giving us flexibility to down the road to intercept the call in
the team/bonding/vlan driver and change the behavior by implementing
swdev_port_stp_update in the middle driver.

I would prefer this approach not only for STP updates but also for
port attr set/get calls.


/**
*      netdev_switch_port_stp_update - Notify switch device port of STP
*                                      state change
*      @dev: bridge port device
*      @state: bridge port STP state
*
*      Notify switch device port of bridge port STP state change.
*/
int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
{
       const struct swdev_ops *ops = dev->swdev_ops;
       struct net_device *lower_dev;
       struct list_head *iter;
       int err = -EOPNOTSUPP;

       if (ops && ops->swdev_port_stp_update)
               return ops->swdev_port_stp_update(dev, state);

       /* Bridged switch device port(s) may be stacked under
        * bond/team/vlan dev, so recurse down to stp-update
        * them one at a time.
        */

       netdev_for_each_lower_dev(dev, lower_dev, iter) {
               err = netdev_switch_port_stp_update(lower_dev, state);
               if (err && err != -EOPNOTSUPP)
                       return err;
       }

I don't like this blind approach. Note that with this, you enable this
for lower devices of:
bond
ipvlan
macvlan
team
vlan
batman-adv

Only when they're used under a bridge because
netdev_switch_port_stp_update() is currently only called from the
bridge.  So this is what we want, to propagate the stp state change
down to the base port, regardless of middle drivers.  If you want to
exclude a middle driver from propagating stp down to the base port, or
otherwise change the default propagation, add stub
swdev_port_stp_update to middle driver.

Does that make sense? I think it is better just pick what we want to
support now and implement it there. That makes things clearer and
confusion-prone.

I'm proposing a simple, permissive solution, with a provision for an
out on a case-by-case basis.  Right now I don't see any reason to
special case any middle driver; just let STP state propagate down to
the port device.

-scott