This patchset renames the bridging routines of the DSA layer, make the
unbridging routine return void, and rework the DSA netdev notifier handler,
similar to what the Mellanox Spectrum driver does.
Vivien Didelot (3):
net: dsa: rename port_*_bridge routines
net: dsa: make port_bridge_leave return void
net: dsa: refine netdev event notifier
Documentation/networking/dsa/dsa.txt | 4 +--
drivers/net/dsa/bcm_sf2.c | 8 ++---
drivers/net/dsa/mv88e6171.c | 4 +--
drivers/net/dsa/mv88e6352.c | 4 +--
drivers/net/dsa/mv88e6xxx.c | 28 +++++----------
drivers/net/dsa/mv88e6xxx.h | 2 +-
include/net/dsa.h | 4 +--
net/dsa/slave.c | 68 +++++++++++++++++++-----------------
8 files changed, 57 insertions(+), 65 deletions(-)
--
2.7.2
netdev_upper_dev_unlink() which notifies NETDEV_CHANGEUPPER, returns
void, as well as del_nbp(). So there's no advantage to catch an eventual
error from the port_bridge_leave routine at the DSA level.
Make this routine void for the DSA layer and its existing drivers.
Signed-off-by: Vivien Didelot <[email protected]>
---
drivers/net/dsa/bcm_sf2.c | 4 +---
drivers/net/dsa/mv88e6xxx.c | 28 +++++++++-------------------
drivers/net/dsa/mv88e6xxx.h | 2 +-
include/net/dsa.h | 2 +-
net/dsa/slave.c | 9 +++------
5 files changed, 15 insertions(+), 30 deletions(-)
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 4bcc9eb..95944d5 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -516,7 +516,7 @@ static int bcm_sf2_sw_br_join(struct dsa_switch *ds, int port,
return 0;
}
-static int bcm_sf2_sw_br_leave(struct dsa_switch *ds, int port)
+static void bcm_sf2_sw_br_leave(struct dsa_switch *ds, int port)
{
struct bcm_sf2_priv *priv = ds_to_priv(ds);
struct net_device *bridge = priv->port_sts[port].bridge_dev;
@@ -543,8 +543,6 @@ static int bcm_sf2_sw_br_leave(struct dsa_switch *ds, int port)
core_writel(priv, p_ctl, CORE_PORT_VLAN_CTL_PORT(port));
priv->port_sts[port].vlan_ctl_mask = p_ctl;
priv->port_sts[port].bridge_dev = NULL;
-
- return 0;
}
static int bcm_sf2_sw_br_set_stp_state(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 5f07524..448d4ef 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -2219,39 +2219,29 @@ unlock:
return err;
}
-int mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port)
+void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port)
{
struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
struct net_device *bridge = ps->ports[port].bridge_dev;
u16 fid;
- int i, err;
+ int i;
mutex_lock(&ps->smi_mutex);
/* Give the port a fresh Filtering Information Database */
- err = _mv88e6xxx_fid_new(ds, &fid);
- if (err)
- goto unlock;
-
- err = _mv88e6xxx_port_fid_set(ds, port, fid);
- if (err)
- goto unlock;
+ if (_mv88e6xxx_fid_new(ds, &fid) ||
+ _mv88e6xxx_port_fid_set(ds, port, fid))
+ netdev_warn(ds->ports[port], "failed to assign a new FID\n");
/* Unassign the bridge and remap each port's VLANTable */
ps->ports[port].bridge_dev = NULL;
- for (i = 0; i < ps->num_ports; ++i) {
- if (i == port || ps->ports[i].bridge_dev == bridge) {
- err = _mv88e6xxx_port_based_vlan_map(ds, i);
- if (err)
- break;
- }
- }
+ for (i = 0; i < ps->num_ports; ++i)
+ if (i == port || ps->ports[i].bridge_dev == bridge)
+ if (_mv88e6xxx_port_based_vlan_map(ds, i))
+ netdev_warn(ds->ports[i], "failed to remap\n");
-unlock:
mutex_unlock(&ps->smi_mutex);
-
- return err;
}
static void mv88e6xxx_bridge_work(struct work_struct *work)
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 3425616..afd495c 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -488,7 +488,7 @@ int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
struct phy_device *phydev, struct ethtool_eee *e);
int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
struct net_device *bridge);
-int mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port);
+void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port);
int mv88e6xxx_port_stp_update(struct dsa_switch *ds, int port, u8 state);
int mv88e6xxx_port_vlan_filtering(struct dsa_switch *ds, int port,
bool vlan_filtering);
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 004e034..6463bb2 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -298,7 +298,7 @@ struct dsa_switch_driver {
*/
int (*port_bridge_join)(struct dsa_switch *ds, int port,
struct net_device *bridge);
- int (*port_bridge_leave)(struct dsa_switch *ds, int port);
+ void (*port_bridge_leave)(struct dsa_switch *ds, int port);
int (*port_stp_update)(struct dsa_switch *ds, int port,
u8 state);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index b997ee1..54976c4 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -454,15 +454,14 @@ static int dsa_slave_bridge_port_join(struct net_device *dev,
return ret;
}
-static int dsa_slave_bridge_port_leave(struct net_device *dev)
+static void 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_bridge_leave)
- ret = ds->drv->port_bridge_leave(ds, p->port);
+ ds->drv->port_bridge_leave(ds, p->port);
p->bridge_dev = NULL;
@@ -470,8 +469,6 @@ static int dsa_slave_bridge_port_leave(struct net_device *dev)
* 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,
@@ -1152,7 +1149,7 @@ static int dsa_slave_master_changed(struct net_device *dev)
!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);
+ dsa_slave_bridge_port_leave(dev);
return err;
}
--
2.7.2
Rework the netdev event handler, similar to what the Mellanox Spectrum
driver does, to eventually welcome NETDEV_PRECHANGEUPPER actions and use
netdev helpers, such as netif_is_bridge_master.
Signed-off-by: Vivien Didelot <[email protected]>
---
net/dsa/slave.c | 55 +++++++++++++++++++++++++++++++------------------------
1 file changed, 31 insertions(+), 24 deletions(-)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 54976c4..27f7886 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -451,7 +451,7 @@ static int dsa_slave_bridge_port_join(struct net_device *dev,
if (ds->drv->port_bridge_join)
ret = ds->drv->port_bridge_join(ds, p->port, br);
- return ret;
+ return ret == -EOPNOTSUPP ? 0 : ret;
}
static void dsa_slave_bridge_port_leave(struct net_device *dev)
@@ -1139,40 +1139,47 @@ 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)
+static int dsa_slave_port_upper_event(struct net_device *dev,
+ unsigned long event, void *ptr)
{
- struct net_device *master = netdev_master_upper_dev_get(dev);
- struct dsa_slave_priv *p = netdev_priv(dev);
+ struct netdev_notifier_changeupper_info *info = ptr;
+ struct net_device *upper = info->upper_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))
- dsa_slave_bridge_port_leave(dev);
+ switch (event) {
+ case NETDEV_CHANGEUPPER:
+ if (netif_is_bridge_master(upper)) {
+ if (info->linking)
+ err = dsa_slave_bridge_port_join(dev, upper);
+ else
+ dsa_slave_bridge_port_leave(dev);
+ }
- return err;
+ break;
+ }
+
+ return notifier_from_errno(err);
}
-int dsa_slave_netdevice_event(struct notifier_block *unused,
- unsigned long event, void *ptr)
+static int dsa_slave_port_event(struct net_device *dev, unsigned long event,
+ void *ptr)
{
- struct net_device *dev;
- int err = 0;
-
switch (event) {
+ case NETDEV_PRECHANGEUPPER:
case NETDEV_CHANGEUPPER:
- dev = netdev_notifier_info_to_dev(ptr);
- if (!dsa_slave_dev_check(dev))
- goto out;
+ return dsa_slave_port_upper_event(dev, event, ptr);
+ }
- err = dsa_slave_master_changed(dev);
- if (err && err != -EOPNOTSUPP)
- netdev_warn(dev, "failed to reflect master change\n");
+ return NOTIFY_DONE;
+}
- break;
- }
+int dsa_slave_netdevice_event(struct notifier_block *unused,
+ unsigned long event, void *ptr)
+{
+ struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+
+ if (dsa_slave_dev_check(dev))
+ return dsa_slave_port_event(dev, event, ptr);
-out:
return NOTIFY_DONE;
}
--
2.7.2
Rename DSA port_join_bridge and port_leave_bridge routines to
respectively port_bridge_join and port_bridge_leave in order to respect
an implicit Port::Bridge namespace.
Signed-off-by: Vivien Didelot <[email protected]>
---
Documentation/networking/dsa/dsa.txt | 4 ++--
drivers/net/dsa/bcm_sf2.c | 4 ++--
drivers/net/dsa/mv88e6171.c | 4 ++--
drivers/net/dsa/mv88e6352.c | 4 ++--
include/net/dsa.h | 4 ++--
net/dsa/slave.c | 8 ++++----
6 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/Documentation/networking/dsa/dsa.txt b/Documentation/networking/dsa/dsa.txt
index 974e9c3..3b196c3 100644
--- a/Documentation/networking/dsa/dsa.txt
+++ b/Documentation/networking/dsa/dsa.txt
@@ -521,12 +521,12 @@ See Documentation/hwmon/sysfs-interface for details.
Bridge layer
------------
-- port_join_bridge: bridge layer function invoked when a given switch port is
+- port_bridge_join: bridge layer function invoked when a given switch port is
added to a bridge, this function should be doing the necessary at the switch
level to permit the joining port from being added to the relevant logical
domain for it to ingress/egress traffic with other members of the bridge.
-- port_leave_bridge: bridge layer function invoked when a given switch port is
+- port_bridge_leave: bridge layer function invoked when a given switch port is
removed from a bridge, this function should be doing the necessary at the
switch level to deny the leaving port from ingress/egress traffic from the
remaining bridge members. When the port leaves the bridge, it should be aged
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 3f62759..4bcc9eb 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -1387,8 +1387,8 @@ static struct dsa_switch_driver bcm_sf2_switch_driver = {
.port_disable = bcm_sf2_port_disable,
.get_eee = bcm_sf2_sw_get_eee,
.set_eee = bcm_sf2_sw_set_eee,
- .port_join_bridge = bcm_sf2_sw_br_join,
- .port_leave_bridge = bcm_sf2_sw_br_leave,
+ .port_bridge_join = bcm_sf2_sw_br_join,
+ .port_bridge_leave = bcm_sf2_sw_br_leave,
.port_stp_update = bcm_sf2_sw_br_set_stp_state,
.port_fdb_prepare = bcm_sf2_sw_fdb_prepare,
.port_fdb_add = bcm_sf2_sw_fdb_add,
diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c
index d72ccbd..c0164b9 100644
--- a/drivers/net/dsa/mv88e6171.c
+++ b/drivers/net/dsa/mv88e6171.c
@@ -103,8 +103,8 @@ struct dsa_switch_driver mv88e6171_switch_driver = {
#endif
.get_regs_len = mv88e6xxx_get_regs_len,
.get_regs = mv88e6xxx_get_regs,
- .port_join_bridge = mv88e6xxx_port_bridge_join,
- .port_leave_bridge = mv88e6xxx_port_bridge_leave,
+ .port_bridge_join = mv88e6xxx_port_bridge_join,
+ .port_bridge_leave = mv88e6xxx_port_bridge_leave,
.port_stp_update = mv88e6xxx_port_stp_update,
.port_vlan_filtering = mv88e6xxx_port_vlan_filtering,
.port_vlan_prepare = mv88e6xxx_port_vlan_prepare,
diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index a41fa50..5f528ab 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -324,8 +324,8 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
.set_eeprom = mv88e6352_set_eeprom,
.get_regs_len = mv88e6xxx_get_regs_len,
.get_regs = mv88e6xxx_get_regs,
- .port_join_bridge = mv88e6xxx_port_bridge_join,
- .port_leave_bridge = mv88e6xxx_port_bridge_leave,
+ .port_bridge_join = mv88e6xxx_port_bridge_join,
+ .port_bridge_leave = mv88e6xxx_port_bridge_leave,
.port_stp_update = mv88e6xxx_port_stp_update,
.port_vlan_filtering = mv88e6xxx_port_vlan_filtering,
.port_vlan_prepare = mv88e6xxx_port_vlan_prepare,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 26c0a3f..004e034 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -296,9 +296,9 @@ struct dsa_switch_driver {
/*
* Bridge integration
*/
- int (*port_join_bridge)(struct dsa_switch *ds, int port,
+ int (*port_bridge_join)(struct dsa_switch *ds, int port,
struct net_device *bridge);
- int (*port_leave_bridge)(struct dsa_switch *ds, int port);
+ int (*port_bridge_leave)(struct dsa_switch *ds, int port);
int (*port_stp_update)(struct dsa_switch *ds, int port,
u8 state);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 27bf03d..b997ee1 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -448,8 +448,8 @@ static int dsa_slave_bridge_port_join(struct net_device *dev,
p->bridge_dev = br;
- if (ds->drv->port_join_bridge)
- ret = ds->drv->port_join_bridge(ds, p->port, br);
+ if (ds->drv->port_bridge_join)
+ ret = ds->drv->port_bridge_join(ds, p->port, br);
return ret;
}
@@ -461,8 +461,8 @@ static int dsa_slave_bridge_port_leave(struct net_device *dev)
int ret = -EOPNOTSUPP;
- if (ds->drv->port_leave_bridge)
- ret = ds->drv->port_leave_bridge(ds, p->port);
+ if (ds->drv->port_bridge_leave)
+ ret = ds->drv->port_bridge_leave(ds, p->port);
p->bridge_dev = NULL;
--
2.7.2
Hi David,
Vivien Didelot <[email protected]> writes:
> This patchset renames the bridging routines of the DSA layer, make the
> unbridging routine return void, and rework the DSA netdev notifier handler,
> similar to what the Mellanox Spectrum driver does.
This is not an RFC, but a normal patchset. I badly reused a previous
git-format-patch command. Please let me know if I need to resend.
Thank you,
Vivien
Hi Vivien,
Sun, Mar 13, 2016 at 08:42:26AM IST, [email protected] wrote:
>Rework the netdev event handler, similar to what the Mellanox Spectrum
>driver does, to eventually welcome NETDEV_PRECHANGEUPPER actions and use
>netdev helpers, such as netif_is_bridge_master.
>
>Signed-off-by: Vivien Didelot <[email protected]>
>---
> net/dsa/slave.c | 55 +++++++++++++++++++++++++++++++------------------------
> 1 file changed, 31 insertions(+), 24 deletions(-)
>
>diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>index 54976c4..27f7886 100644
>--- a/net/dsa/slave.c
>+++ b/net/dsa/slave.c
>@@ -451,7 +451,7 @@ static int dsa_slave_bridge_port_join(struct net_device *dev,
> if (ds->drv->port_bridge_join)
> ret = ds->drv->port_bridge_join(ds, p->port, br);
>
>- return ret;
>+ return ret == -EOPNOTSUPP ? 0 : ret;
> }
>
> static void dsa_slave_bridge_port_leave(struct net_device *dev)
>@@ -1139,40 +1139,47 @@ 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)
>+static int dsa_slave_port_upper_event(struct net_device *dev,
>+ unsigned long event, void *ptr)
> {
>- struct net_device *master = netdev_master_upper_dev_get(dev);
>- struct dsa_slave_priv *p = netdev_priv(dev);
>+ struct netdev_notifier_changeupper_info *info = ptr;
>+ struct net_device *upper = info->upper_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))
>- dsa_slave_bridge_port_leave(dev);
>+ switch (event) {
>+ case NETDEV_CHANGEUPPER:
>+ if (netif_is_bridge_master(upper)) {
>+ if (info->linking)
>+ err = dsa_slave_bridge_port_join(dev, upper);
>+ else
>+ dsa_slave_bridge_port_leave(dev);
>+ }
>
>- return err;
>+ break;
>+ }
>+
>+ return notifier_from_errno(err);
> }
>
>-int dsa_slave_netdevice_event(struct notifier_block *unused,
>- unsigned long event, void *ptr)
>+static int dsa_slave_port_event(struct net_device *dev, unsigned long event,
>+ void *ptr)
> {
>- struct net_device *dev;
>- int err = 0;
>-
> switch (event) {
>+ case NETDEV_PRECHANGEUPPER:
Why do you need this here? It seems you are always ignoring it in
dsa_slave_port_upper_event()? Probably better to introduce it when you
actually need it.
Other than that, it looks good to me.
> case NETDEV_CHANGEUPPER:
>- dev = netdev_notifier_info_to_dev(ptr);
>- if (!dsa_slave_dev_check(dev))
>- goto out;
>+ return dsa_slave_port_upper_event(dev, event, ptr);
>+ }
>
>- err = dsa_slave_master_changed(dev);
>- if (err && err != -EOPNOTSUPP)
>- netdev_warn(dev, "failed to reflect master change\n");
>+ return NOTIFY_DONE;
>+}
>
>- break;
>- }
>+int dsa_slave_netdevice_event(struct notifier_block *unused,
>+ unsigned long event, void *ptr)
>+{
>+ struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>+
>+ if (dsa_slave_dev_check(dev))
>+ return dsa_slave_port_event(dev, event, ptr);
>
>-out:
> return NOTIFY_DONE;
> }
>--
>2.7.2
>
Hi Ido,
Ido Schimmel <[email protected]> writes:
>>+ case NETDEV_PRECHANGEUPPER:
>
> Why do you need this here? It seems you are always ignoring it in
> dsa_slave_port_upper_event()? Probably better to introduce it when you
> actually need it.
>
> Other than that, it looks good to me.
I've prepare a real v1 without this switch case locally, I'll send it in
a few moment unless there are other comments.
Thanks Ido!
Vivien
On Sun, Mar 13, 2016 at 01:42:23AM -0500, Vivien Didelot wrote:
> This patchset renames the bridging routines of the DSA layer, make the
> unbridging routine return void, and rework the DSA netdev notifier handler,
> similar to what the Mellanox Spectrum driver does.
Tested-by: Andrew Lunn <[email protected]>
Do you think there could be some consolidation of code with Mellanox,
and other switchdev devices? Moving it into the switchdev core?
Andrew
Hi Andrew,
Andrew Lunn <[email protected]> writes:
> On Sun, Mar 13, 2016 at 01:42:23AM -0500, Vivien Didelot wrote:
>> This patchset renames the bridging routines of the DSA layer, make the
>> unbridging routine return void, and rework the DSA netdev notifier handler,
>> similar to what the Mellanox Spectrum driver does.
>
> Tested-by: Andrew Lunn <[email protected]>
>
> Do you think there could be some consolidation of code with Mellanox,
> and other switchdev devices? Moving it into the switchdev core?
We cannot move all this code to switchdev core, because switchdev is
stateless, so there is no place to register the netdevice notifier(s).
But it might be possible to provide a generic switchdev_netdevice_event
helper, using new switchdev operations to check a port device, and
add/del upper devices. I'll think about that.
Thanks!
Vivien