Date:	Sun, 15 Mar 2015 13:03:06 -0700
From:	Scott Feldman <sfeldma@...il.com>
To:	Jiri Pirko <jiri@...nulli.us>
Cc:	David Miller <davem@...emloft.net>,
	Roopa Prabhu <roopa@...ulusnetworks.com>,
	Netdev <netdev@...r.kernel.org>
Subject: 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 <jiri@...nulli.us> wrote:
> Sun, Mar 15, 2015 at 07:08:02PM CET, sfeldma@...il.com wrote:
>>On Fri, Mar 13, 2015 at 9:31 AM, David Miller <davem@...emloft.net> wrote:
>>> From: roopa <roopa@...ulusnetworks.com>
>>> Date: Fri, 13 Mar 2015 06:25:24 -0700
>>>
>>>> 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
--
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

Powered by blists - more mailing lists