Current switchdev users implement notifier blocks to filter global netdev
events, in order to correctly offload bridging to their hardware ports.
Such code could be replaced with the support of a simple switchdev attribute
set when adding/deleting a port to/from a bridge.
Also, we can imagine a switch driver or network layer wanting to restrict the
number of logical bridges on top of a physical device. That could be done by
returning -EOPNOTSUPP when setting such attribute.
The first patch adds a new SWITCHDEV_ATTR_ID_PORT_BRIDGE_IF switchdev attribute
containing a boolean, set when joining or leaving a bridge.
The second patch shows the benefit of supporting such attribute in the DSA
layer. Similar change should be doable to other switchdev users, like Rocker.
Note: I send this as an RFC since I am not really sure about the attribute
flags, and the exact place to set it in del_nbp(). Comments needed :-)
Thanks,
Vivien Didelot (2):
net: bridge: add switchdev attr for port bridging
net: dsa: support SWITCHDEV_ATTR_ID_PORT_BRIDGE_IF
include/net/switchdev.h | 2 +
net/bridge/br_if.c | 27 ++++++++++++
net/dsa/dsa.c | 7 ---
net/dsa/dsa_priv.h | 2 -
net/dsa/slave.c | 113 +++++++++++++-----------------------------------
5 files changed, 59 insertions(+), 92 deletions(-)
--
2.7.2
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.
Signed-off-by: Vivien Didelot <[email protected]>
---
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,
+ .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 (err)
+ return ERR_PTR(err);
index = find_portno(br);
if (index < 0)
--
2.7.2
Add a new dsa_slave_bridge_if function to handle the
SWITCHDEV_ATTR_ID_PORT_BRIDGE_IF switchdev attribute.
Thus remove the code related to the netdev notifier block.
Signed-off-by: Vivien Didelot <[email protected]>
---
net/dsa/dsa.c | 7 ----
net/dsa/dsa_priv.h | 2 -
net/dsa/slave.c | 113 ++++++++++++++---------------------------------------
3 files changed, 30 insertions(+), 92 deletions(-)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index fa4daba..cfb678b 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -977,10 +977,6 @@ static struct packet_type dsa_pack_type __read_mostly = {
.func = dsa_switch_rcv,
};
-static struct notifier_block dsa_netdevice_nb __read_mostly = {
- .notifier_call = dsa_slave_netdevice_event,
-};
-
#ifdef CONFIG_PM_SLEEP
static int dsa_suspend(struct device *d)
{
@@ -1047,8 +1043,6 @@ static int __init dsa_init_module(void)
{
int rc;
- register_netdevice_notifier(&dsa_netdevice_nb);
-
rc = platform_driver_register(&dsa_driver);
if (rc)
return rc;
@@ -1061,7 +1055,6 @@ module_init(dsa_init_module);
static void __exit dsa_cleanup_module(void)
{
- unregister_netdevice_notifier(&dsa_netdevice_nb);
dev_remove_pack(&dsa_pack_type);
platform_driver_unregister(&dsa_driver);
}
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 1d1a546..34d1951 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -64,8 +64,6 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
void dsa_slave_destroy(struct net_device *slave_dev);
int dsa_slave_suspend(struct net_device *slave_dev);
int dsa_slave_resume(struct net_device *slave_dev);
-int dsa_slave_netdevice_event(struct notifier_block *unused,
- unsigned long event, void *ptr);
/* tag_dsa.c */
extern const struct dsa_device_ops dsa_netdev_ops;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 27bf03d..90ef149 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -305,16 +305,38 @@ static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
return -EOPNOTSUPP;
}
-static int dsa_slave_stp_update(struct net_device *dev, u8 state)
+static int dsa_slave_bridge_if(struct net_device *dev,
+ const struct switchdev_attr *attr,
+ struct switchdev_trans *trans)
{
struct dsa_slave_priv *p = netdev_priv(dev);
struct dsa_switch *ds = p->parent;
- int ret = -EOPNOTSUPP;
+ int err;
- if (ds->drv->port_stp_update)
- ret = ds->drv->port_stp_update(ds, p->port, state);
+ if (switchdev_trans_ph_prepare(trans)) {
+ if (!ds->drv->port_join_bridge || !ds->drv->port_leave_bridge)
+ return -EOPNOTSUPP;
+ return 0;
+ }
- return ret;
+ if (attr->u.join) {
+ err = ds->drv->port_join_bridge(ds, p->port, attr->orig_dev);
+ if (!err)
+ p->bridge_dev = attr->orig_dev;
+ } else {
+ err = ds->drv->port_leave_bridge(ds, p->port);
+
+ /* When a port leaves a bridge, the bridge layer sets its STP
+ * state to DISABLED. Restore FORWARDING to keep it functional.
+ */
+ if (ds->drv->port_stp_update)
+ ds->drv->port_stp_update(ds, p->port,
+ BR_STATE_FORWARDING);
+
+ p->bridge_dev = NULL;
+ }
+
+ return err;
}
static int dsa_slave_vlan_filtering(struct net_device *dev,
@@ -354,6 +376,9 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
case SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING:
ret = dsa_slave_vlan_filtering(dev, attr, trans);
break;
+ case SWITCHDEV_ATTR_ID_PORT_BRIDGE_IF:
+ ret = dsa_slave_bridge_if(dev, attr, trans);
+ break;
default:
ret = -EOPNOTSUPP;
break;
@@ -439,41 +464,6 @@ static int dsa_slave_port_obj_dump(struct net_device *dev,
return err;
}
-static int dsa_slave_bridge_port_join(struct net_device *dev,
- struct net_device *br)
-{
- struct dsa_slave_priv *p = netdev_priv(dev);
- struct dsa_switch *ds = p->parent;
- int ret = -EOPNOTSUPP;
-
- p->bridge_dev = br;
-
- if (ds->drv->port_join_bridge)
- ret = ds->drv->port_join_bridge(ds, p->port, br);
-
- return ret;
-}
-
-static int dsa_slave_bridge_port_leave(struct net_device *dev)
-{
- struct dsa_slave_priv *p = netdev_priv(dev);
- struct dsa_switch *ds = p->parent;
- int ret = -EOPNOTSUPP;
-
-
- if (ds->drv->port_leave_bridge)
- ret = ds->drv->port_leave_bridge(ds, p->port);
-
- p->bridge_dev = NULL;
-
- /* Port left the bridge, put in BR_STATE_DISABLED by the bridge layer,
- * so allow it to be in BR_STATE_FORWARDING to be kept functional
- */
- dsa_slave_stp_update(dev, BR_STATE_FORWARDING);
-
- return ret;
-}
-
static int dsa_slave_port_attr_get(struct net_device *dev,
struct switchdev_attr *attr)
{
@@ -1136,46 +1126,3 @@ void dsa_slave_destroy(struct net_device *slave_dev)
unregister_netdev(slave_dev);
free_netdev(slave_dev);
}
-
-static bool dsa_slave_dev_check(struct net_device *dev)
-{
- return dev->netdev_ops == &dsa_slave_netdev_ops;
-}
-
-static int dsa_slave_master_changed(struct net_device *dev)
-{
- struct net_device *master = netdev_master_upper_dev_get(dev);
- struct dsa_slave_priv *p = netdev_priv(dev);
- int err = 0;
-
- if (master && master->rtnl_link_ops &&
- !strcmp(master->rtnl_link_ops->kind, "bridge"))
- err = dsa_slave_bridge_port_join(dev, master);
- else if (dsa_port_is_bridged(p))
- err = dsa_slave_bridge_port_leave(dev);
-
- return err;
-}
-
-int dsa_slave_netdevice_event(struct notifier_block *unused,
- unsigned long event, void *ptr)
-{
- struct net_device *dev;
- int err = 0;
-
- switch (event) {
- case NETDEV_CHANGEUPPER:
- dev = netdev_notifier_info_to_dev(ptr);
- if (!dsa_slave_dev_check(dev))
- goto out;
-
- err = dsa_slave_master_changed(dev);
- if (err && err != -EOPNOTSUPP)
- netdev_warn(dev, "failed to reflect master change\n");
-
- break;
- }
-
-out:
- return NOTIFY_DONE;
-}
--
2.7.2
Hi Vivien
> -static bool dsa_slave_dev_check(struct net_device *dev)
> -{
> - return dev->netdev_ops == &dsa_slave_netdev_ops;
> -}
Where is the equivalent of this happening? Where do we check that the
interface added to the bridge is part of the switch?
> -int dsa_slave_netdevice_event(struct notifier_block *unused,
> - unsigned long event, void *ptr)
> -{
> - struct net_device *dev;
> - int err = 0;
> -
> - switch (event) {
> - case NETDEV_CHANGEUPPER:
> - dev = netdev_notifier_info_to_dev(ptr);
> - if (!dsa_slave_dev_check(dev))
> - goto out;
> -
> - err = dsa_slave_master_changed(dev);
> - if (err && err != -EOPNOTSUPP)
> - netdev_warn(dev, "failed to reflect master change\n");
> -
> - break;
> - }
> -
> -out:
> - return NOTIFY_DONE;
> -}
How about team/bonding? We are not ready to implement it yet with the
Marvell devices, but at some point we probably will. Won't we need the
events then? We need to know when a switch port has been added to a
team?
Or do you think a switchdev object will be added for this case?
Mellanox already have the ability to add switch interfaces to a team,
and then add the team to a bridge. So we need to ensure your solution
works for such stacked systems.
Andrew
Wed, Mar 09, 2016 at 07:32:13PM CET, [email protected] wrote:
>Hi Vivien
>
>> -static bool dsa_slave_dev_check(struct net_device *dev)
>> -{
>> - return dev->netdev_ops == &dsa_slave_netdev_ops;
>> -}
>
>Where is the equivalent of this happening? Where do we check that the
>interface added to the bridge is part of the switch?
>
>> -int dsa_slave_netdevice_event(struct notifier_block *unused,
>> - unsigned long event, void *ptr)
>> -{
>> - struct net_device *dev;
>> - int err = 0;
>> -
>> - switch (event) {
>> - case NETDEV_CHANGEUPPER:
>> - dev = netdev_notifier_info_to_dev(ptr);
>> - if (!dsa_slave_dev_check(dev))
>> - goto out;
>> -
>> - err = dsa_slave_master_changed(dev);
>> - if (err && err != -EOPNOTSUPP)
>> - netdev_warn(dev, "failed to reflect master change\n");
>> -
>> - break;
>> - }
>> -
>> -out:
>> - return NOTIFY_DONE;
>> -}
>
>How about team/bonding? We are not ready to implement it yet with the
>Marvell devices, but at some point we probably will. Won't we need the
>events then? We need to know when a switch port has been added to a
>team?
>
>Or do you think a switchdev object will be added for this case?
>Mellanox already have the ability to add switch interfaces to a team,
>and then add the team to a bridge. So we need to ensure your solution
>works for such stacked systems.
I have to look at this more closer tomorrow, but I'm missing motivation
behind this. Using existing notifiers, drivers can easily monitor what
is going on with their uppers. Why do we need this to be changed?
Hello.
On 03/09/2016 08:42 PM, Vivien Didelot 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.
>
> Signed-off-by: Vivien Didelot <[email protected]>
> ---
> include/net/switchdev.h | 2 ++
> net/bridge/br_if.c | 27 +++++++++++++++++++++++++++
> 2 files changed, 29 insertions(+)
>
[...]
> 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,
> + .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)
Enough to only do the latter comparison.
> + return err;
> +
> + return 0;
> +}
> +
> /*
> * Determine initial path cost based on speed.
> * using recommendations from 802.1d standard
[...]
MBR, Sergei
Hi Andrew,
Andrew Lunn <[email protected]> writes:
>> -static bool dsa_slave_dev_check(struct net_device *dev)
>> -{
>> - return dev->netdev_ops == &dsa_slave_netdev_ops;
>> -}
>
> Where is the equivalent of this happening? Where do we check that the
> interface added to the bridge is part of the switch?
Why should we check that? In this RFC, br_if.c tries to set the new
attribute to the net_device, when creating and deleting the net bridge
port. If it supports attr_set and this attribute, then we're good. Or am
I missing something?
>> -int dsa_slave_netdevice_event(struct notifier_block *unused,
>> - unsigned long event, void *ptr)
>> -{
>> - struct net_device *dev;
>> - int err = 0;
>> -
>> - switch (event) {
>> - case NETDEV_CHANGEUPPER:
>> - dev = netdev_notifier_info_to_dev(ptr);
>> - if (!dsa_slave_dev_check(dev))
>> - goto out;
>> -
>> - err = dsa_slave_master_changed(dev);
>> - if (err && err != -EOPNOTSUPP)
>> - netdev_warn(dev, "failed to reflect master change\n");
>> -
>> - break;
>> - }
>> -
>> -out:
>> - return NOTIFY_DONE;
>> -}
>
> How about team/bonding? We are not ready to implement it yet with the
> Marvell devices, but at some point we probably will. Won't we need the
> events then? We need to know when a switch port has been added to a
> team?
>
> Or do you think a switchdev object will be added for this case?
> Mellanox already have the ability to add switch interfaces to a team,
> and then add the team to a bridge. So we need to ensure your solution
> works for such stacked systems.
Indeed these features can be propagated through new switchdev attributes
or objects.
I think it'd be preferable to factorize the switch related operations
into the switchdev API, instead of having every single switchdev user
implement its custom (but similar) listeners and checks for global
netdev events. What do you think?
Best,
Vivien
On Wed, Mar 09, 2016 at 02:32:05PM -0500, Vivien Didelot wrote:
> Hi Andrew,
>
> Andrew Lunn <[email protected]> writes:
>
> >> -static bool dsa_slave_dev_check(struct net_device *dev)
> >> -{
> >> - return dev->netdev_ops == &dsa_slave_netdev_ops;
> >> -}
> >
> > Where is the equivalent of this happening? Where do we check that the
> > interface added to the bridge is part of the switch?
>
> Why should we check that? In this RFC, br_if.c tries to set the new
> attribute to the net_device, when creating and deleting the net bridge
> port. If it supports attr_set and this attribute, then we're good. Or am
> I missing something?
One of us is missing something...
What happens if i have two dsa clusters? We probably want to limit the
object to only being passed to the DSA cluster which contains the
port, or once we receive the object, we verify it belongs to the
cluster processing it.
What happens with a team/bind interface is added to the bridge. In the
future we need to know about this, so we can add the trunk in Marvells
terms to the bridge.
> > How about team/bonding? We are not ready to implement it yet with the
> > Marvell devices, but at some point we probably will. Won't we need the
> > events then? We need to know when a switch port has been added to a
> > team?
> >
> > Or do you think a switchdev object will be added for this case?
> > Mellanox already have the ability to add switch interfaces to a team,
> > and then add the team to a bridge. So we need to ensure your solution
> > works for such stacked systems.
>
> Indeed these features can be propagated through new switchdev attributes
> or objects.
>
> I think it'd be preferable to factorize the switch related operations
> into the switchdev API, instead of having every single switchdev user
> implement its custom (but similar) listeners and checks for global
> netdev events. What do you think?
Centralizing the code would be good. But DSA is way behind what
Mellanox can do, so you need to look at how your changes fit into
their driver.
During a netdev 1.1 BOF there was a conversation about the stack of
interfaces, teams/bonds, bridges, etc. If the video is available, you
might find it interesting.
Andrew
Hi Vivien,
Wed, Mar 09, 2016 at 07:42:47PM IST, [email protected] 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 <[email protected]>
>---
> 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!
Hi Jiri,
Jiri Pirko <[email protected]> writes:
> Wed, Mar 09, 2016 at 07:32:13PM CET, [email protected] wrote:
>>Hi Vivien
>>
>>> -static bool dsa_slave_dev_check(struct net_device *dev)
>>> -{
>>> - return dev->netdev_ops == &dsa_slave_netdev_ops;
>>> -}
>>
>>Where is the equivalent of this happening? Where do we check that the
>>interface added to the bridge is part of the switch?
>>
>>> -int dsa_slave_netdevice_event(struct notifier_block *unused,
>>> - unsigned long event, void *ptr)
>>> -{
>>> - struct net_device *dev;
>>> - int err = 0;
>>> -
>>> - switch (event) {
>>> - case NETDEV_CHANGEUPPER:
>>> - dev = netdev_notifier_info_to_dev(ptr);
>>> - if (!dsa_slave_dev_check(dev))
>>> - goto out;
>>> -
>>> - err = dsa_slave_master_changed(dev);
>>> - if (err && err != -EOPNOTSUPP)
>>> - netdev_warn(dev, "failed to reflect master change\n");
>>> -
>>> - break;
>>> - }
>>> -
>>> -out:
>>> - return NOTIFY_DONE;
>>> -}
>>
>>How about team/bonding? We are not ready to implement it yet with the
>>Marvell devices, but at some point we probably will. Won't we need the
>>events then? We need to know when a switch port has been added to a
>>team?
>>
>>Or do you think a switchdev object will be added for this case?
>>Mellanox already have the ability to add switch interfaces to a team,
>>and then add the team to a bridge. So we need to ensure your solution
>>works for such stacked systems.
>
> I have to look at this more closer tomorrow, but I'm missing motivation
> behind this. Using existing notifiers, drivers can easily monitor what
> is going on with their uppers. Why do we need this to be changed?
Yes with notifiers, drivers can monitor these changes with the
NETDEV_CHANGEUPPER even. They can also forbid such bridging by returning
NOTIFY_BAD in the NETDEV_PRECHANGEUPPER event if I'm not mistaken.
But looking at DSA slave, Mellanox Spectrum, and Rocker, they all
implement this similar heavy code, while they could support a common
switchdev attribute and reduce boilerplate.
But maybe I'm wrong, what why I sent that as an RFC :-)
Thanks,
Vivien
Hi Ido,
Ido Schimmel <[email protected]> writes:
> Wed, Mar 09, 2016 at 07:42:47PM IST, [email protected] 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 <[email protected]>
>>---
>> 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