Hi Ido,
Ido Schimmel <ido...@mellanox.com> writes:
> Wed, Mar 09, 2016 at 07:42:47PM IST, vivien.dide...@savoirfairelinux.com
> wrote:
>>Add a new SWITCHDEV_ATTR_ID_PORT_BRIDGE_IF switchdev attribute which is
>>set before adding a port to a bridge and deleting a port from a bridge.
>>
>>The main purpose for this attribute is to provide switchdev users a
>>simple and common way to retrieve bridging information, instead of
>>implementing complex notifier blocks to listen to global netdev events.
>>
>>We can also imagine a switchdev user returning an error different from
>>-EOPNOTSUPP in the prepare phase to prevent a port from being bridged.
>
> I don't really understand the motivation for this change. We are already
> doing all these stuff with the notifiers and it's pretty
> straight-forward.
>
> In fact, I believe using an existing mechanism instead of introducing
> more switchdev hooks is more elegant. This RFC only deals with bridge,
> but you'll have to do the same for team, bond and vlan devices. And
> you'll probably place the hooks in the exact locations where the
> notifiers are called from anyway.
>
>>
>>Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
>>---
>> include/net/switchdev.h | 2 ++
>> net/bridge/br_if.c | 27 +++++++++++++++++++++++++++
>> 2 files changed, 29 insertions(+)
>>
>>diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>>index d451122..65f8514 100644
>>--- a/include/net/switchdev.h
>>+++ b/include/net/switchdev.h
>>@@ -46,6 +46,7 @@ enum switchdev_attr_id {
>> SWITCHDEV_ATTR_ID_PORT_PARENT_ID,
>> SWITCHDEV_ATTR_ID_PORT_STP_STATE,
>> SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS,
>>+ SWITCHDEV_ATTR_ID_PORT_BRIDGE_IF,
>> SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>> SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING,
>> };
>>@@ -58,6 +59,7 @@ struct switchdev_attr {
>> struct netdev_phys_item_id ppid; /* PORT_PARENT_ID */
>> u8 stp_state; /* PORT_STP_STATE */
>> unsigned long brport_flags; /* PORT_BRIDGE_FLAGS */
>>+ bool join; /* PORT_BRIDGE_IF */
>> u32 ageing_time; /* BRIDGE_AGEING_TIME */
>> bool vlan_filtering; /*
>> BRIDGE_VLAN_FILTERING */
>> } u;
>>diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>>index a73df33..105b9fd 100644
>>--- a/net/bridge/br_if.c
>>+++ b/net/bridge/br_if.c
>>@@ -28,6 +28,24 @@
>>
>> #include "br_private.h"
>>
>>+static int switchdev_bridge_if(struct net_device *dev, struct net_bridge *br,
>>+ bool join)
>>+{
>>+ struct switchdev_attr attr = {
>>+ .orig_dev = br->dev,
>
> This should be just 'dev', since you need to know for which stacked
> device on top of the port this was called for. This also means you'll
> have to call netdev_master_upper_dev_get() from within your driver if
> you want to limit the number of VLAN filtering bridges (for example).
> However, since this is called before bridge dev and dev itself are
> linked, you'll get NULL.
>
>>+ .id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_IF,
>>+ .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>>+ .u.join = join,
>>+ };
>>+ int err;
>>+
>>+ err = switchdev_port_attr_set(dev, &attr);
>>+ if (err && err != -EOPNOTSUPP)
>>+ return err;
>>+
>>+ return 0;
>>+}
>>+
>> /*
>> * Determine initial path cost based on speed.
>> * using recommendations from 802.1d standard
>>@@ -297,6 +315,10 @@ static void del_nbp(struct net_bridge_port *p)
>> br_netpoll_disable(p);
>>
>> call_rcu(&p->rcu, destroy_nbp_rcu);
>>+
>>+ if (switchdev_bridge_if(dev, br, false))
>>+ br_warn(br, "error unbridging port %u(%s)\n",
>>+ (unsigned int) p->port_no, dev->name);
>> }
>>
>> /* Delete bridge device */
>>@@ -347,6 +369,11 @@ static struct net_bridge_port *new_nbp(struct net_bridge
>>*br,
>> {
>> int index;
>> struct net_bridge_port *p;
>>+ int err;
>>+
>>+ err = switchdev_bridge_if(dev, br, true);
>
> If you look at br_add_if() - where new_nbp() is called from - then
> you'll see that you aren't rollbacking this operation in case of error.
> Same for subsequent errors in this function I believe.
>
>>+ if (err)
>>+ return ERR_PTR(err);
>>
>> index = find_portno(br);
>> if (index < 0)
>>--
>>2.7.2
>>
>
> Maybe this is something we'll have to do in the future, but for now I
> think we are OK with the notifiers. :)
>
> Thanks Vivien!
I didn't have the big picture for team, bond and vlan devices as well.
I can drop this RFC then. Thanks for the details!
Vivien
