[PATCH net-next v2 0/4] switchdev: push bridge attributes down

[PATCH net-next v2 0/4] switchdev: push bridge attributes down

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

Push bridge-level attributes down to switchdev drivers.  This patchset
adds the infrastructure and then pushes, as an example, ageing_time attribute
down from bridge to switchdev (rocker) driver.  Add some range-checking
for ageing_time.

# ip link set dev br0 type bridge ageing_time 1000

# ip link set dev br0 type bridge ageing_time 999
RTNETLINK answers: Numerical result out of range

Up until now, switchdev attrs where port-level attrs, so the netdev used in
switchdev_attr_set() would be a switch port or bond of switch ports.  With
bridge-level attrs, the netdev passed to switchdev_attr_set() is the bridge
netdev.  The same recusive algo is used to visit the leaves of the stacked
drivers to set the attr, it's just in this case we start one layer higher in
the stack.  One note is not all ports in the bridge may support setting a
bridge-level attribute, so rather than failing the entire set, we'll skip over
those ports returning -EOPNOTSUPP.

v1->v2: rebase w/ net-next

Scott Feldman (4):
  switchdev: add bridge attributes
  switchdev: skip over ports returning -EOPNOTSUPP when recursing ports
  bridge: push bridge setting ageing_time down to switchdev
  rocker: handle setting bridge ageing_time

 drivers/net/ethernet/rocker/rocker.c |   22 ++++++++++++++++++++++
 include/net/switchdev.h              |    6 ++++++
 include/uapi/linux/if_link.h         |    2 +-
 net/bridge/br_ioctl.c                |    3 +--
 net/bridge/br_netlink.c              |    6 +++---
 net/bridge/br_private.h              |    1 +
 net/bridge/br_stp.c                  |   24 ++++++++++++++++++++++++
 net/bridge/br_sysfs_br.c             |    3 +--
 net/switchdev/switchdev.c            |    9 ++++++++-
 9 files changed, 67 insertions(+), 9 deletions(-)

-- 
1.7.10.4

[PATCH net-next v2 1/4] switchdev: add bridge attributes

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

Setting the stage to push bridge-level attributes down to port driver so
hardware can be programmed accordingly.  Bridge-level attribute example is
ageing_time.  This is a per-bridge attribute, not a per-bridge-port attr.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 include/net/switchdev.h      |    5 +++++
 include/uapi/linux/if_link.h |    2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 89266a3..8d92cd0 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -43,6 +43,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_BRIDGE,
 };
 
 struct switchdev_attr {
@@ -52,6 +53,10 @@ 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 */
+		struct switchdev_attr_bridge {		/* BRIDGE */
+			enum ifla_br attr;
+			u32 val;
+		} bridge;
 	} u;
 };
 
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index e3b6217..30177b3 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -222,7 +222,7 @@ enum in6_addr_gen_mode {
 
 /* Bridge section */
 
-enum {
+enum ifla_br {
 	IFLA_BR_UNSPEC,
 	IFLA_BR_FORWARD_DELAY,
 	IFLA_BR_HELLO_TIME,
-- 
1.7.10.4

Re: [PATCH net-next v2 1/4] switchdev: add bridge attributes

[Jiri Pirko]

Thu, Oct 08, 2015 at 08:04:40AM CEST, sfeldma@gmail.com wrote:
>From: Scott Feldman <sfeldma@gmail.com>
>
>Setting the stage to push bridge-level attributes down to port driver so
>hardware can be programmed accordingly.  Bridge-level attribute example is
>ageing_time.  This is a per-bridge attribute, not a per-bridge-port attr.
>
>Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>---
> include/net/switchdev.h      |    5 +++++
> include/uapi/linux/if_link.h |    2 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>index 89266a3..8d92cd0 100644
>--- a/include/net/switchdev.h
>+++ b/include/net/switchdev.h
>@@ -43,6 +43,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_BRIDGE,
> };
> 
> struct switchdev_attr {
>@@ -52,6 +53,10 @@ 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 */
>+		struct switchdev_attr_bridge {		/* BRIDGE */
>+			enum ifla_br attr;

I don't like pushing down IFLA_BR_* values throught switchdev. I think
it might better to just intruduce:
SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME

and "u32 ageing_time" here. Something similar to stp_state. Much easier
to read and does not give blank cheque for passing any bridge IFLA_BR_*
down to drivers.

It also aligns with bridge code nicely, I believe.



>+			u32 val;
>+		} bridge;
> 	} u;
> };
> 
>diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>index e3b6217..30177b3 100644
>--- a/include/uapi/linux/if_link.h
>+++ b/include/uapi/linux/if_link.h
>@@ -222,7 +222,7 @@ enum in6_addr_gen_mode {
> 
> /* Bridge section */
> 
>-enum {
>+enum ifla_br {
> 	IFLA_BR_UNSPEC,
> 	IFLA_BR_FORWARD_DELAY,
> 	IFLA_BR_HELLO_TIME,
>-- 
>1.7.10.4
>

Re: [PATCH net-next v2 1/4] switchdev: add bridge attributes

[Vivien Didelot]

On Oct. Thursday 08 (41) 10:39 AM, Jiri Pirko wrote:
> Thu, Oct 08, 2015 at 08:04:40AM CEST, sfeldma@gmail.com wrote:
> >From: Scott Feldman <sfeldma@gmail.com>
> >
> >Setting the stage to push bridge-level attributes down to port driver so
> >hardware can be programmed accordingly.  Bridge-level attribute example is
> >ageing_time.  This is a per-bridge attribute, not a per-bridge-port attr.
> >
> >Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> >---
> > include/net/switchdev.h      |    5 +++++
> > include/uapi/linux/if_link.h |    2 +-
> > 2 files changed, 6 insertions(+), 1 deletion(-)
> >
> >diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> >index 89266a3..8d92cd0 100644
> >--- a/include/net/switchdev.h
> >+++ b/include/net/switchdev.h
> >@@ -43,6 +43,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_BRIDGE,
> > };
> > 
> > struct switchdev_attr {
> >@@ -52,6 +53,10 @@ 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 */
> >+		struct switchdev_attr_bridge {		/* BRIDGE */
> >+			enum ifla_br attr;
> 
> I don't like pushing down IFLA_BR_* values throught switchdev. I think
> it might better to just intruduce:
> SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME
> 
> and "u32 ageing_time" here. Something similar to stp_state. Much easier
> to read and does not give blank cheque for passing any bridge IFLA_BR_*
> down to drivers.
> 
> It also aligns with bridge code nicely, I believe.

I would add that pushing bridge-specific aspects down to the drivers
does not really make sense since such notion does not exist for them. A
Linux-bridge is just an untagged-VLAN for the switch chip point-of-view.

Thanks,
-v

Re: [PATCH net-next v2 1/4] switchdev: add bridge attributes

[Scott Feldman]

On Thu, Oct 8, 2015 at 1:39 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 08, 2015 at 08:04:40AM CEST, sfeldma@gmail.com wrote:
>>From: Scott Feldman <sfeldma@gmail.com>
>>
>>Setting the stage to push bridge-level attributes down to port driver so
>>hardware can be programmed accordingly.  Bridge-level attribute example is
>>ageing_time.  This is a per-bridge attribute, not a per-bridge-port attr.
>>
>>Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>>---
>> include/net/switchdev.h      |    5 +++++
>> include/uapi/linux/if_link.h |    2 +-
>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>
>>diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>>index 89266a3..8d92cd0 100644
>>--- a/include/net/switchdev.h
>>+++ b/include/net/switchdev.h
>>@@ -43,6 +43,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_BRIDGE,
>> };
>>
>> struct switchdev_attr {
>>@@ -52,6 +53,10 @@ 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 */
>>+              struct switchdev_attr_bridge {          /* BRIDGE */
>>+                      enum ifla_br attr;
>
> I don't like pushing down IFLA_BR_* values throught switchdev. I think
> it might better to just intruduce:
> SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME
>
> and "u32 ageing_time" here. Something similar to stp_state. Much easier
> to read and does not give blank cheque for passing any bridge IFLA_BR_*
> down to drivers.
>
> It also aligns with bridge code nicely, I believe.

Done, v3 sent with your suggested change.

[PATCH net-next v2 2/4] switchdev: skip over ports returning -EOPNOTSUPP when recursing ports

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

This allows us to recurse over all the ports, skipping over unsupporting
ports.  Without the change, the recursion would stop at first unsupported
port.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 include/net/switchdev.h   |    1 +
 net/switchdev/switchdev.c |    9 ++++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 8d92cd0..f3de6f4 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -16,6 +16,7 @@
 #include <linux/list.h>
 
 #define SWITCHDEV_F_NO_RECURSE		BIT(0)
+#define SWITCHDEV_F_SKIP_EOPNOTSUPP	BIT(1)
 
 struct switchdev_trans_item {
 	struct list_head list;
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 6e4a4f9..7a9ab90 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -147,7 +147,7 @@ static int __switchdev_port_attr_set(struct net_device *dev,
 		return ops->switchdev_port_attr_set(dev, attr, trans);
 
 	if (attr->flags & SWITCHDEV_F_NO_RECURSE)
-		return err;
+		goto done;
 
 	/* Switch device port(s) may be stacked under
 	 * bond/team/vlan dev, so recurse down to set attr on
@@ -156,10 +156,17 @@ static int __switchdev_port_attr_set(struct net_device *dev,
 
 	netdev_for_each_lower_dev(dev, lower_dev, iter) {
 		err = __switchdev_port_attr_set(lower_dev, attr, trans);
+		if (err == -EOPNOTSUPP &&
+		    attr->flags & SWITCHDEV_F_SKIP_EOPNOTSUPP)
+			continue;
 		if (err)
 			break;
 	}
 
+done:
+	if (err == -EOPNOTSUPP && attr->flags & SWITCHDEV_F_SKIP_EOPNOTSUPP)
+		err = 0;
+
 	return err;
 }
 
-- 
1.7.10.4

[PATCH net-next v2 3/4] bridge: push bridge setting ageing_time down to switchdev

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
support setting ageing_time (or setting bridge attrs in general).

If push fails, don't update ageing_time in bridge and return err to user.

If push succeeds, update ageing_time in bridge and run gc_timer now to
recalabrate when to run gc_timer next, based on new ageing_time.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/bridge/br_ioctl.c    |    3 +--
 net/bridge/br_netlink.c  |    6 +++---
 net/bridge/br_private.h  |    1 +
 net/bridge/br_stp.c      |   24 ++++++++++++++++++++++++
 net/bridge/br_sysfs_br.c |    3 +--
 5 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index 8d423bc..263b4de 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -200,8 +200,7 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 		if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
 
-		br->ageing_time = clock_t_to_jiffies(args[1]);
-		return 0;
+		return br_set_ageing_time(br, args[1]);
 
 	case BRCTL_GET_PORT_INFO:
 	{
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index d78b442..544ab96 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -870,9 +870,9 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 	}
 
 	if (data[IFLA_BR_AGEING_TIME]) {
-		u32 ageing_time = nla_get_u32(data[IFLA_BR_AGEING_TIME]);
-
-		br->ageing_time = clock_t_to_jiffies(ageing_time);
+		err = br_set_ageing_time(br, nla_get_u32(data[IFLA_BR_AGEING_TIME]));
+		if (err)
+			return err;
 	}
 
 	if (data[IFLA_BR_STP_STATE]) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 09d3ecb..ba0c67b 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -882,6 +882,7 @@ void __br_set_forward_delay(struct net_bridge *br, unsigned long t);
 int br_set_forward_delay(struct net_bridge *br, unsigned long x);
 int br_set_hello_time(struct net_bridge *br, unsigned long x);
 int br_set_max_age(struct net_bridge *br, unsigned long x);
+int br_set_ageing_time(struct net_bridge *br, u32 ageing_time);
 
 
 /* br_stp_if.c */
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 3a982c0..ae3286b 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -566,6 +566,30 @@ int br_set_max_age(struct net_bridge *br, unsigned long val)
 
 }
 
+int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
+{
+	struct switchdev_attr attr = {
+		.id = SWITCHDEV_ATTR_ID_BRIDGE,
+		.flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
+		.u.bridge.attr = IFLA_BR_AGEING_TIME,
+		.u.bridge.val = ageing_time,
+	};
+	unsigned long t = clock_t_to_jiffies(ageing_time);
+	int err;
+
+	if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
+		return -ERANGE;
+
+	err = switchdev_port_attr_set(br->dev, &attr);
+	if (err)
+		return err;
+
+	br->ageing_time = t;
+	mod_timer(&br->gc_timer, jiffies);
+
+	return 0;
+}
+
 void __br_set_forward_delay(struct net_bridge *br, unsigned long t)
 {
 	br->bridge_forward_delay = t;
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 4c97fc5..04ef192 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -102,8 +102,7 @@ static ssize_t ageing_time_show(struct device *d,
 
 static int set_ageing_time(struct net_bridge *br, unsigned long val)
 {
-	br->ageing_time = clock_t_to_jiffies(val);
-	return 0;
+	return br_set_ageing_time(br, val);
 }
 
 static ssize_t ageing_time_store(struct device *d,
-- 
1.7.10.4

[PATCH net-next v2 4/4] rocker: handle setting bridge ageing_time

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

The FDB cleanup timer will get rescheduled to re-evaluate FDB entries
based on new ageing_time.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 drivers/net/ethernet/rocker/rocker.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index cf91ffc..3c7f9ae 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -4361,6 +4361,24 @@ static int rocker_port_brport_flags_set(struct rocker_port *rocker_port,
 	return err;
 }
 
+static int rocker_port_bridge_set(struct rocker_port *rocker_port,
+				  struct switchdev_trans *trans,
+				  struct switchdev_attr_bridge *bridge)
+{
+	switch (bridge->attr) {
+	case IFLA_BR_AGEING_TIME:
+		if (switchdev_trans_ph_prepare(trans))
+			return 0;
+		rocker_port->ageing_time = clock_t_to_jiffies(bridge->val);
+		mod_timer(&rocker_port->rocker->fdb_cleanup_timer, jiffies);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int rocker_port_attr_set(struct net_device *dev,
 				struct switchdev_attr *attr,
 				struct switchdev_trans *trans)
@@ -4378,6 +4396,10 @@ static int rocker_port_attr_set(struct net_device *dev,
 		err = rocker_port_brport_flags_set(rocker_port, trans,
 						   attr->u.brport_flags);
 		break;
+	case SWITCHDEV_ATTR_ID_BRIDGE:
+		err = rocker_port_bridge_set(rocker_port, trans,
+					     &attr->u.bridge);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
-- 
1.7.10.4