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

[PATCH net-next 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.

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 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 319baab..54b2faa 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -28,6 +28,7 @@ enum switchdev_attr_id {
 	SWITCHDEV_ATTR_PORT_PARENT_ID,
 	SWITCHDEV_ATTR_PORT_STP_STATE,
 	SWITCHDEV_ATTR_PORT_BRIDGE_FLAGS,
+	SWITCHDEV_ATTR_BRIDGE,
 };
 
 struct switchdev_attr {
@@ -38,6 +39,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 3a5f263..8d0ef1c 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 1/4] switchdev: add bridge attributes

[Premkumar Jonnala]

Acked-by: Premkumar Jonnala

> -----Original Message-----
> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]
> Sent: Friday, September 25, 2015 2:29 AM
> To: netdev@vger.kernel.org
> Cc: jiri@resnulli.us; siva.mannem.lnx@gmail.com; Premkumar Jonnala;
> stephen@networkplumber.org; roopa@cumulusnetworks.com;
> andrew@lunn.ch; f.fainelli@gmail.com; vivien.didelot@savoirfairelinux.com
> Subject: [PATCH net-next 1/4] switchdev: add bridge attributes
> 
> 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 319baab..54b2faa 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -28,6 +28,7 @@ enum switchdev_attr_id {
>  	SWITCHDEV_ATTR_PORT_PARENT_ID,
>  	SWITCHDEV_ATTR_PORT_STP_STATE,
>  	SWITCHDEV_ATTR_PORT_BRIDGE_FLAGS,
> +	SWITCHDEV_ATTR_BRIDGE,
>  };
> 
>  struct switchdev_attr {
> @@ -38,6 +39,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 3a5f263..8d0ef1c 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 1/4] switchdev: add bridge attributes

[David Miller]

From: Premkumar Jonnala <pjonnala@broadcom.com>
Date: Fri, 25 Sep 2015 04:32:16 +0000

> Acked-by: Premkumar Jonnala

This is not the correct way to ACK a patch.

First of all, you should provide your full email address
after your name in the Acked-by:, as I do, like this:

"Acked-by: David S. Miller <davem@davemloft.net>"

Second of all, never "top post" on mailing lists at vger.kernel.org,
it is extremely irritating especially for those of us who participate
regularly on these list and have done so for decades.

[PATCH net-next 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 54b2faa..22a6dbe 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -15,6 +15,7 @@
 #include <linux/notifier.h>
 
 #define SWITCHDEV_F_NO_RECURSE		BIT(0)
+#define SWITCHDEV_F_SKIP_EOPNOTSUPP	BIT(1)
 
 enum switchdev_trans {
 	SWITCHDEV_TRANS_NONE,
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index fda38f8..5c30da0 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -73,7 +73,7 @@ static int __switchdev_port_attr_set(struct net_device *dev,
 		return ops->switchdev_port_attr_set(dev, attr);
 
 	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
@@ -82,10 +82,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);
+		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

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

[Premkumar Jonnala]

Acked-by: Premkumar Jonnala

> -----Original Message-----
> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]
> Sent: Friday, September 25, 2015 2:29 AM
> To: netdev@vger.kernel.org
> Cc: jiri@resnulli.us; siva.mannem.lnx@gmail.com; Premkumar Jonnala;
> stephen@networkplumber.org; roopa@cumulusnetworks.com;
> andrew@lunn.ch; f.fainelli@gmail.com; vivien.didelot@savoirfairelinux.com
> Subject: [PATCH net-next 2/4] switchdev: skip over ports returning -
> EOPNOTSUPP when recursing ports
> 
> 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 54b2faa..22a6dbe 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -15,6 +15,7 @@
>  #include <linux/notifier.h>
> 
>  #define SWITCHDEV_F_NO_RECURSE		BIT(0)
> +#define SWITCHDEV_F_SKIP_EOPNOTSUPP	BIT(1)
> 
>  enum switchdev_trans {
>  	SWITCHDEV_TRANS_NONE,
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index fda38f8..5c30da0 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -73,7 +73,7 @@ static int __switchdev_port_attr_set(struct net_device
> *dev,
>  		return ops->switchdev_port_attr_set(dev, attr);
> 
>  	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
> @@ -82,10 +82,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);
> +		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 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 ea748c9..6dbdd4d 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -774,9 +774,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 74e99c7..dc4b390 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -808,6 +808,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 ed74ffa..6241bab 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_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 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 32c5429..99ec715 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -4382,6 +4382,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,
+				  enum switchdev_trans trans,
+				  struct switchdev_attr_bridge *bridge)
+{
+	switch (bridge->attr) {
+	case IFLA_BR_AGEING_TIME:
+		if (trans == SWITCHDEV_TRANS_PREPARE)
+			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)
 {
@@ -4409,6 +4427,10 @@ static int rocker_port_attr_set(struct net_device *dev,
 		err = rocker_port_brport_flags_set(rocker_port, attr->trans,
 						   attr->u.brport_flags);
 		break;
+	case SWITCHDEV_ATTR_BRIDGE:
+		err = rocker_port_bridge_set(rocker_port, attr->trans,
+					     &attr->u.bridge);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
-- 
1.7.10.4

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

[Stephen Hemminger]

On Thu, 24 Sep 2015 13:59:26 -0700
sfeldma@gmail.com wrote:

> 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.


Rather than having more in bridge, shouldn't this just be a netlink event?

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

[Scott Feldman]

On Thu, Sep 24, 2015 at 2:05 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Thu, 24 Sep 2015 13:59:26 -0700
> sfeldma@gmail.com wrote:
>
>> 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.
>
>
> Rather than having more in bridge, shouldn't this just be a netlink event?

You lost me?  Oh, do you mean netdev notifier?  Jiri (privately) had
suggested that also.

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

[Stephen Hemminger]

On Thu, 24 Sep 2015 20:25:01 -0700
Scott Feldman <sfeldma@gmail.com> wrote:

> On Thu, Sep 24, 2015 at 2:05 PM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> > On Thu, 24 Sep 2015 13:59:26 -0700
> > sfeldma@gmail.com wrote:
> >
> >> 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.
> >
> >
> > Rather than having more in bridge, shouldn't this just be a netlink event?
> 
> You lost me?  Oh, do you mean netdev notifier?  Jiri (privately) had
> suggested that also.

Yes. I meant a netlink notifier so that user or kernel space driver
can track changes. Otherwise you have to add switchdev callbacks for
each parameter.

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

[Florian Fainelli]

On 24/09/15 13:59, sfeldma@gmail.com wrote:
> 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.

So, without a better device to hold that kind of information (in the
future it could be a global, switch-specific device holding that
information), I agree with your decision to take the bridge device to
hold that attribute, it still feels a bit uncomfortable to have
switchdev_attr_port() take a bridge device parameter, but whatever, here
is a scenario I am wondering how we would want to proceed with:

- suppose we have a switch which is only able to control ageing
globally, not per port or any other kind of logical domain

- we have enabled two software bridges on the same physical switch, with
different ageing timeouts

It does not seem to me like it hurts ageing the other bridge faster than
expected (even though that could be expensive for MDIO devices), but we
would need to have consistent reporting here for the other bridge.

We could therefore have the driver return different things:

- < 0: error, value is too low or too high for the hardware to support that

- == 0: supports ageing in a more fine-grained way that globally

- > 0 (== ageing): supports ageing globally and switchdev/bridge also
needs to update the other bridge devices with the same ageing parameters

Alternatively, as soon as we have more bridges than supported ageing
control knobs, we could have the driver just return -EPERM or something
like that, but that means keeping track of bridges attached to the
switch's ports.

Other than that, this looks good to me, thanks!

> 
> 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(-)
> 


-- 
Florian

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

[Andrew Lunn]

> So, without a better device to hold that kind of information (in the
> future it could be a global, switch-specific device holding that
> information), I agree with your decision to take the bridge device to
> hold that attribute, it still feels a bit uncomfortable to have
> switchdev_attr_port() take a bridge device parameter, but whatever, here
> is a scenario I am wondering how we would want to proceed with:
> 
> - suppose we have a switch which is only able to control ageing
> globally, not per port or any other kind of logical domain

Marvell switches only have global ageing settings, and it has to be a
multiple of 15 seconds, from 0 to 3825 seconds.

	 Andrew

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

[Scott Feldman]

On Thu, Sep 24, 2015 at 6:23 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 24/09/15 13:59, sfeldma@gmail.com wrote:
>> 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.
>
> So, without a better device to hold that kind of information (in the
> future it could be a global, switch-specific device holding that
> information), I agree with your decision to take the bridge device to
> hold that attribute, it still feels a bit uncomfortable to have
> switchdev_attr_port() take a bridge device parameter, but whatever, here
> is a scenario I am wondering how we would want to proceed with:
>
> - suppose we have a switch which is only able to control ageing
> globally, not per port or any other kind of logical domain
>
> - we have enabled two software bridges on the same physical switch, with
> different ageing timeouts
>
> It does not seem to me like it hurts ageing the other bridge faster than
> expected (even though that could be expensive for MDIO devices), but we
> would need to have consistent reporting here for the other bridge.

It could hurt a little bit by ageing out entries prematurely, forcing
relearning.  ;)

I think if the switch can't support multiple ageing timeouts (which is
probably typical), then the switch driver should not implement this
switchdev attr at the port level (this patchset).  So how does ageing
timeout get set for a switch with a global timer?  I believe we need
the switch-specific device to handle those switch-global attr sets.
I'll send out a refresh of my RFC patches for switch device class, and
add this ageing_time attr.

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

[Florian Fainelli]

2015-09-24 21:26 GMT-07:00 Scott Feldman <sfeldma@gmail.com>:
> On Thu, Sep 24, 2015 at 6:23 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 24/09/15 13:59, sfeldma@gmail.com wrote:
>>> 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.
>>
>> So, without a better device to hold that kind of information (in the
>> future it could be a global, switch-specific device holding that
>> information), I agree with your decision to take the bridge device to
>> hold that attribute, it still feels a bit uncomfortable to have
>> switchdev_attr_port() take a bridge device parameter, but whatever, here
>> is a scenario I am wondering how we would want to proceed with:
>>
>> - suppose we have a switch which is only able to control ageing
>> globally, not per port or any other kind of logical domain
>>
>> - we have enabled two software bridges on the same physical switch, with
>> different ageing timeouts
>>
>> It does not seem to me like it hurts ageing the other bridge faster than
>> expected (even though that could be expensive for MDIO devices), but we
>> would need to have consistent reporting here for the other bridge.
>
> It could hurt a little bit by ageing out entries prematurely, forcing
> relearning.  ;)
>
> I think if the switch can't support multiple ageing timeouts (which is
> probably typical), then the switch driver should not implement this
> switchdev attr at the port level (this patchset).  So how does ageing
> timeout get set for a switch with a global timer?

For Broadcom switches, you have a global timer which is 20 bits wide,
allowing both the min and max ageing times to be set. So I would
suspect you would have to setup a combination of hardware and software
timers if you want different ageing timeouts to be made per
bridge/VLAN/port etc.

>  I believe we need
> the switch-specific device to handle those switch-global attr sets.
> I'll send out a refresh of my RFC patches for switch device class, and
> add this ageing_time attr.

Works for me, thanks!
-- 
Florian

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

[David Miller]

From: sfeldma@gmail.com
Date: Thu, 24 Sep 2015 13:59:26 -0700

> 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.

This doesn't apply cleanly to net-next, please respin.

Thanks.