[PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Premkumar Jonnala]

Bridge devices have ageing interval used to age out MAC addresses
from FDB.  This ageing interval was not configuratble.

Enable netlink based configuration of ageing interval for bridges and
switch devices.  The ageing interval changes the timer used to purge
inactive FDB entries in bridges.  The ageing interval config is
propagated to switch devices, so that platform or hardware based
ageing works according to configuration.

Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>

---

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 607b5f4..e3b0c45 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1053,7 +1053,16 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
  *	This function is used to pass protocol port error state information
  *	to the switch driver. The switch driver can react to the proto_down
  *      by doing a phys down on the associated switch port.
- *
+ * int (*ndo_bridge_setageing)(const struct net_device *dev,
+ *			       int ageing_interval);
+ *     Called to set FDB aging interval for a given bridge device.
+ * int (*ndo_bridge_getageing_nl)(struct sk_buff *skb,
+ *				const struct net_device *dev,
+ *				struct netlink_callback *cb);
+ *     Called to return the ageing interval for the given bridge device,
+ *     in a format suitable for netlink messaging.
+ * int (*ndo_bridge_getageing)(const struct net_device *dev);
+ *     Called to retrieve the ageing interval for the given bridge device.
  */
 struct net_device_ops {
 	int			(*ndo_init)(struct net_device *dev);
@@ -1226,6 +1235,13 @@ struct net_device_ops {
 	int			(*ndo_get_iflink)(const struct net_device *dev);
 	int			(*ndo_change_proto_down)(struct net_device *dev,
 							 bool proto_down);
+	int	(*ndo_bridge_setageing)(const struct net_device *dev,
+					int ageing_interval);
+	int	(*ndo_bridge_getageing_nl)(struct sk_buff *skb,
+					   const struct net_device *dev,
+					   struct netlink_callback *cb);
+
+	int	(*ndo_bridge_getageing)(const struct net_device *dev);
 };
 
 /**
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 89da893..7186fea 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -129,6 +129,10 @@ int switchdev_port_attr_get(struct net_device *dev,
 			    struct switchdev_attr *attr);
 int switchdev_port_attr_set(struct net_device *dev,
 			    struct switchdev_attr *attr);
+int netdev_switch_ageing_set(struct net_device *dev, int ageing_interval);
+int netdev_switch_ageing_get(struct sk_buff *skb,
+			     const struct net_device *dev,
+			     struct netlink_callback *cb);
 int switchdev_port_obj_add(struct net_device *dev, struct switchdev_obj *obj);
 int switchdev_port_obj_del(struct net_device *dev, struct switchdev_obj *obj);
 int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj);
@@ -163,6 +167,17 @@ void switchdev_port_fwd_mark_set(struct net_device *dev,
 
 #else
 
+static inline int netdev_switch_ageing_set(struct net_device *dev,
+					int ageing_interval)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int netdev_switch_ageing_get(struct net_device *dev)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int switchdev_port_attr_get(struct net_device *dev,
 					  struct switchdev_attr *attr)
 {
diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 3635b77..a32ab4d 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -199,4 +199,23 @@ enum {
 };
 #define MDBA_SET_ENTRY_MAX (__MDBA_SET_ENTRY_MAX - 1)
 
+struct admsg {
+	__u8 adm_family;
+	__u8 adm_pad1;
+	__u16 adm_pad2;
+	__s32 adm_ifindex;
+	__u16 adm_ageing_interval;
+};
+
+/* The value of this macro is based on the value recommended by IEEE
+ * standard 802.1d.
+ */
+#define MIN_AGEING_INTERVAL_SECS (10)
+
+/* The value of DEFAULT_AGEING_INTERVAL_SECS is the default ageing
+ * interval that was used in br_device.c.  This default value is also
+ * recommended by IEEE Standard 802.1d.
+ */
+#define DEFAULT_AGEING_INTERVAL_SECS (300)
+
 #endif /* _UAPI_LINUX_IF_BRIDGE_H */
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 47d24cb..9321818 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -139,6 +139,13 @@ enum {
 	RTM_GETNSID = 90,
 #define RTM_GETNSID RTM_GETNSID
 
+	RTM_SETAGEING = 92,
+#define RTM_SETAGEING RTM_SETAGEING
+	RTM_SETDEFAULTAGEING = 93,
+#define RTM_SETDEFAULTAGEING RTM_SETDEFAULTAGEING
+	RTM_GETAGEING = 94,
+#define RTM_GETAGEING RTM_GETAGEING
+
 	__RTM_MAX,
 #define RTM_MAX		(((__RTM_MAX + 3) & ~3) - 1)
 };
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 4ff77a1..2c81190 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -18,6 +18,7 @@
 #include <linux/ethtool.h>
 #include <linux/list.h>
 #include <linux/netfilter_bridge.h>
+#include <net/netlink.h>
 
 #include <asm/uaccess.h>
 #include "br_private.h"
@@ -309,6 +310,43 @@ static int br_del_slave(struct net_device *dev, struct net_device *slave_dev)
 	return br_del_if(br, slave_dev);
 }
 
+static int br_setageing(const struct net_device *dev, int ageing_interval)
+{
+	struct net_bridge *br = netdev_priv(dev);
+
+	return br_fdb_setageing(br, ageing_interval);
+}
+
+static int br_getageing(const struct net_device *dev)
+{
+	struct net_bridge *br = netdev_priv(dev);
+
+	return br->ageing_time / HZ;
+}
+
+static int br_getageing_nl(struct sk_buff *skb,
+			   const struct net_device *dev,
+			   struct netlink_callback *cb)
+{
+	struct nlmsghdr *nlhdr;
+	struct admsg *amsg;
+
+	struct net_bridge *br = netdev_priv(dev);
+
+	nlhdr = nlmsg_put(skb, NETLINK_CB(cb->skb).portid,
+			  cb->nlh->nlmsg_seq, RTM_GETAGEING, sizeof(*amsg), 0);
+	if (!nlhdr)
+		return -EMSGSIZE;
+
+	amsg = nlmsg_data(nlhdr);
+
+	amsg->adm_family = AF_BRIDGE;
+	amsg->adm_ifindex = dev->ifindex;
+	amsg->adm_ageing_interval = br->ageing_time / HZ;
+
+	return 0;
+}
+
 static const struct ethtool_ops br_ethtool_ops = {
 	.get_drvinfo    = br_getinfo,
 	.get_link	= ethtool_op_get_link,
@@ -339,6 +377,9 @@ static const struct net_device_ops br_netdev_ops = {
 	.ndo_bridge_getlink	 = br_getlink,
 	.ndo_bridge_setlink	 = br_setlink,
 	.ndo_bridge_dellink	 = br_dellink,
+	.ndo_bridge_setageing    = br_setageing,
+	.ndo_bridge_getageing_nl = br_getageing_nl,
+	.ndo_bridge_getageing    = br_getageing,
 };
 
 static void br_dev_free(struct net_device *dev)
@@ -391,7 +432,7 @@ void br_dev_setup(struct net_device *dev)
 	br->bridge_max_age = br->max_age = 20 * HZ;
 	br->bridge_hello_time = br->hello_time = 2 * HZ;
 	br->bridge_forward_delay = br->forward_delay = 15 * HZ;
-	br->ageing_time = 300 * HZ;
+	br->ageing_time = DEFAULT_AGEING_INTERVAL_SECS * HZ;
 
 	br_netfilter_rtable_init(br);
 	br_stp_timer_init(br);
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 9e9875d..c46f734 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -283,6 +283,25 @@ out:
 	spin_unlock_bh(&br->hash_lock);
 }
 
+int br_fdb_setageing(struct net_bridge *br, int ageing_interval)
+{
+	unsigned long next_timer;
+	unsigned long exp_time;
+
+	if (ageing_interval < MIN_AGEING_INTERVAL_SECS)
+		return -EINVAL;
+
+	br->ageing_time = ageing_interval * HZ;
+
+	next_timer = jiffies + hold_time(br);
+	exp_time = br->gc_timer.expires;
+
+	if (exp_time > next_timer)
+		mod_timer(&br->gc_timer, round_jiffies_up(next_timer));
+
+	return 0;
+}
+
 void br_fdb_cleanup(unsigned long _data)
 {
 	struct net_bridge *br = (struct net_bridge *)_data;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 3ad1290..b745e06 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -410,6 +410,7 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
 			      const unsigned char *addr, u16 vid);
 int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
 			      const unsigned char *addr, u16 vid);
+int br_fdb_setageing(struct net_bridge *br, int ageing_interval);
 
 /* br_forward.c */
 void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 5fb4af2..e7c6197 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3277,6 +3277,128 @@ out:
 	return err;
 }
 
+static int configure_ageing_interval(struct net_device  *dev, int interval)
+{
+	int err = -EOPNOTSUPP;
+	int old_ageing_timer = -1;
+	const struct net_device_ops *ops = dev->netdev_ops;
+
+	if (ops->ndo_bridge_getageing)
+		old_ageing_timer = ops->ndo_bridge_getageing(dev);
+
+	if (ops->ndo_bridge_setageing)
+		err = ops->ndo_bridge_setageing(dev, interval);
+
+	if (!err) {
+		err = netdev_switch_ageing_set(dev, interval);
+
+		if (err == -EOPNOTSUPP)
+			return 0;
+
+		if (err && (old_ageing_timer >= MIN_AGEING_INTERVAL_SECS))
+			ops->ndo_bridge_setageing(dev, old_ageing_timer);
+	}
+
+	return err;
+}
+
+static int rtnl_bridge_setageing(struct sk_buff *skb, struct nlmsghdr *nlh)
+{
+	struct net *net = sock_net(skb->sk);
+	struct admsg *adm;
+	struct net_device *dev;
+	int err;
+	struct nlattr *tb[NDA_MAX + 1];
+
+	err = nlmsg_parse(nlh, sizeof(*adm), tb, NDA_MAX, NULL);
+	if (err < 0)
+		return err;
+
+	adm = nlmsg_data(nlh);
+	if (adm->adm_ifindex == 0)
+		return -EINVAL;
+
+	dev = __dev_get_by_index(net, adm->adm_ifindex);
+	if (!dev)
+		return -ENODEV;
+
+	err = -EOPNOTSUPP;
+
+	if (!(dev->priv_flags & IFF_EBRIDGE))
+		return err;
+
+	err = configure_ageing_interval(dev, adm->adm_ageing_interval);
+
+	return err;
+}
+
+static int rtnl_bridge_setdefaultageing(struct sk_buff *skb,
+					struct nlmsghdr *nlh)
+{
+	struct net *net = sock_net(skb->sk);
+	struct admsg *adm;
+	struct net_device *dev;
+	int err;
+	struct nlattr *tb[NDA_MAX + 1];
+
+	err = nlmsg_parse(nlh, sizeof(*adm), tb, NDA_MAX, NULL);
+	if (err < 0)
+		return err;
+
+	adm = nlmsg_data(nlh);
+	if (adm->adm_ifindex == 0)
+		return -EINVAL;
+
+	dev = __dev_get_by_index(net, adm->adm_ifindex);
+	if (!dev)
+		return -ENODEV;
+
+	err = -EOPNOTSUPP;
+
+	if (!(dev->priv_flags & IFF_EBRIDGE))
+		return err;
+
+	err = configure_ageing_interval(dev, DEFAULT_AGEING_INTERVAL_SECS);
+
+	return err;
+}
+
+static int rtnl_bridge_getageing(struct sk_buff *skb,
+				 struct netlink_callback *cb)
+{
+	struct net *net = sock_net(skb->sk);
+	struct admsg *adm;
+	struct net_device *dev;
+	int err;
+	struct nlattr *tb[NDA_MAX + 1];
+
+	err = nlmsg_parse(cb->nlh, sizeof(*adm), tb, NDA_MAX, NULL);
+	if (err < 0)
+		return err;
+
+	adm = nlmsg_data(cb->nlh);
+	if (adm->adm_ifindex == 0)
+		return -EINVAL;
+
+	dev = __dev_get_by_index(net, adm->adm_ifindex);
+	if (!dev)
+		return -ENODEV;
+
+	err = -EOPNOTSUPP;
+
+	if (!(dev->priv_flags & IFF_EBRIDGE))
+		return err;
+
+	if (dev->netdev_ops->ndo_bridge_getageing_nl)
+		err = dev->netdev_ops->ndo_bridge_getageing_nl(skb, dev, cb);
+
+	if (err)
+		return err;
+
+	cb->args[0] = 1;
+	return 0;
+}
+
 /* Process one rtnetlink message. */
 
 static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
@@ -3427,5 +3549,11 @@ void __init rtnetlink_init(void)
 	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, NULL);
 	rtnl_register(PF_BRIDGE, RTM_DELLINK, rtnl_bridge_dellink, NULL, NULL);
 	rtnl_register(PF_BRIDGE, RTM_SETLINK, rtnl_bridge_setlink, NULL, NULL);
+	rtnl_register(PF_BRIDGE, RTM_GETAGEING, NULL,
+		      rtnl_bridge_getageing, NULL);
+	rtnl_register(PF_BRIDGE, RTM_SETAGEING,
+		      rtnl_bridge_setageing, NULL, NULL);
+	rtnl_register(PF_BRIDGE, RTM_SETDEFAULTAGEING,
+		      rtnl_bridge_setdefaultageing, NULL, NULL);
 }
 
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 33bafa2..501e892 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -1142,3 +1142,34 @@ void switchdev_port_fwd_mark_set(struct net_device *dev,
 	dev->offload_fwd_mark = mark;
 }
 EXPORT_SYMBOL_GPL(switchdev_port_fwd_mark_set);
+
+int netdev_switch_ageing_set(struct net_device *dev,
+			     int ageing_interval)
+{
+	int err = 0;
+
+	struct net_device *pd;
+	struct list_head *iter;
+
+	netdev_for_each_lower_dev(dev, pd, iter) {
+		if (!pd || !pd->netdev_ops ||
+		    !pd->netdev_ops->ndo_bridge_setageing)
+			continue;
+
+		err = pd->netdev_ops->ndo_bridge_setageing(pd,
+							ageing_interval);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(netdev_switch_ageing_set);
+
+int netdev_switch_ageing_get(struct sk_buff *skb,
+			     const struct net_device *dev,
+			     struct netlink_callback *cb)
+{
+	return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL_GPL(netdev_switch_ageing_get);
diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
index 2bbb418..e29836e 100644
--- a/security/selinux/nlmsgtab.c
+++ b/security/selinux/nlmsgtab.c
@@ -154,7 +154,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
 	switch (sclass) {
 	case SECCLASS_NETLINK_ROUTE_SOCKET:
 		/* RTM_MAX always point to RTM_SETxxxx, ie RTM_NEWxxx + 3 */
-		BUILD_BUG_ON(RTM_MAX != (RTM_NEWNSID + 3));
+		BUILD_BUG_ON(RTM_MAX != (RTM_SETAGEING + 3));
 		err = nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms,
 				 sizeof(nlmsg_route_perms));
 		break;

Re: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[roopa]

On 8/13/15, 11:23 PM, Premkumar Jonnala wrote:
> Bridge devices have ageing interval used to age out MAC addresses
> from FDB.  This ageing interval was not configuratble.
>
> Enable netlink based configuration of ageing interval for bridges and
> switch devices.  The ageing interval changes the timer used to purge
> inactive FDB entries in bridges.  The ageing interval config is
> propagated to switch devices, so that platform or hardware based
> ageing works according to configuration.
>
> Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>

How is this different from netlink attribute IFLA_BR_AGEING_TIME ?
>
> ---
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 607b5f4..e3b0c45 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1053,7 +1053,16 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
>    *	This function is used to pass protocol port error state information
>    *	to the switch driver. The switch driver can react to the proto_down
>    *      by doing a phys down on the associated switch port.
> - *
> + * int (*ndo_bridge_setageing)(const struct net_device *dev,
> + *			       int ageing_interval);
> + *     Called to set FDB aging interval for a given bridge device.
> + * int (*ndo_bridge_getageing_nl)(struct sk_buff *skb,
> + *				const struct net_device *dev,
> + *				struct netlink_callback *cb);
> + *     Called to return the ageing interval for the given bridge device,
> + *     in a format suitable for netlink messaging.
> + * int (*ndo_bridge_getageing)(const struct net_device *dev);
> + *     Called to retrieve the ageing interval for the given bridge device.
>    */
>   struct net_device_ops {
>   	int			(*ndo_init)(struct net_device *dev);
> @@ -1226,6 +1235,13 @@ struct net_device_ops {
>   	int			(*ndo_get_iflink)(const struct net_device *dev);
>   	int			(*ndo_change_proto_down)(struct net_device *dev,
>   							 bool proto_down);
> +	int	(*ndo_bridge_setageing)(const struct net_device *dev,
> +					int ageing_interval);
> +	int	(*ndo_bridge_getageing_nl)(struct sk_buff *skb,
> +					   const struct net_device *dev,
> +					   struct netlink_callback *cb);
> +
> +	int	(*ndo_bridge_getageing)(const struct net_device *dev);
>   };
>   
you cannot add new ndo's for each of these. It should be covered as part of
existing br_link_ops

>   /**
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index 89da893..7186fea 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -129,6 +129,10 @@ int switchdev_port_attr_get(struct net_device *dev,
>   			    struct switchdev_attr *attr);
>   int switchdev_port_attr_set(struct net_device *dev,
>   			    struct switchdev_attr *attr);
> +int netdev_switch_ageing_set(struct net_device *dev, int ageing_interval);
> +int netdev_switch_ageing_get(struct sk_buff *skb,
> +			     const struct net_device *dev,
> +			     struct netlink_callback *cb);
>   int switchdev_port_obj_add(struct net_device *dev, struct switchdev_obj *obj);
>   int switchdev_port_obj_del(struct net_device *dev, struct switchdev_obj *obj);
>   int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj);
> @@ -163,6 +167,17 @@ void switchdev_port_fwd_mark_set(struct net_device *dev,
>   
>   #else
>   
> +static inline int netdev_switch_ageing_set(struct net_device *dev,
> +					int ageing_interval)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int netdev_switch_ageing_get(struct net_device *dev)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>   static inline int switchdev_port_attr_get(struct net_device *dev,
>   					  struct switchdev_attr *attr)
>   {
> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> index 3635b77..a32ab4d 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -199,4 +199,23 @@ enum {
>   };
>   #define MDBA_SET_ENTRY_MAX (__MDBA_SET_ENTRY_MAX - 1)
>   
> +struct admsg {
> +	__u8 adm_family;
> +	__u8 adm_pad1;
> +	__u16 adm_pad2;
> +	__s32 adm_ifindex;
> +	__u16 adm_ageing_interval;
> +};
> +
> +/* The value of this macro is based on the value recommended by IEEE
> + * standard 802.1d.
> + */
> +#define MIN_AGEING_INTERVAL_SECS (10)
> +
> +/* The value of DEFAULT_AGEING_INTERVAL_SECS is the default ageing
> + * interval that was used in br_device.c.  This default value is also
> + * recommended by IEEE Standard 802.1d.
> + */
> +#define DEFAULT_AGEING_INTERVAL_SECS (300)
> +
>   #endif /* _UAPI_LINUX_IF_BRIDGE_H */
> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
> index 47d24cb..9321818 100644
> --- a/include/uapi/linux/rtnetlink.h
> +++ b/include/uapi/linux/rtnetlink.h
> @@ -139,6 +139,13 @@ enum {
>   	RTM_GETNSID = 90,
>   #define RTM_GETNSID RTM_GETNSID
>   
> +	RTM_SETAGEING = 92,
> +#define RTM_SETAGEING RTM_SETAGEING
> +	RTM_SETDEFAULTAGEING = 93,
> +#define RTM_SETDEFAULTAGEING RTM_SETDEFAULTAGEING
> +	RTM_GETAGEING = 94,
> +#define RTM_GETAGEING RTM_GETAGEING
> +

you cannot add a new RTM message for a single attribute either.
>   	__RTM_MAX,
>   #define RTM_MAX		(((__RTM_MAX + 3) & ~3) - 1)
>   };
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 4ff77a1..2c81190 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -18,6 +18,7 @@
>   #include <linux/ethtool.h>
>   #include <linux/list.h>
>   #include <linux/netfilter_bridge.h>
> +#include <net/netlink.h>
>   
>   #include <asm/uaccess.h>
>   #include "br_private.h"
> @@ -309,6 +310,43 @@ static int br_del_slave(struct net_device *dev, struct net_device *slave_dev)
>   	return br_del_if(br, slave_dev);
>   }
>   
> +static int br_setageing(const struct net_device *dev, int ageing_interval)
> +{
> +	struct net_bridge *br = netdev_priv(dev);
> +
> +	return br_fdb_setageing(br, ageing_interval);
> +}
> +
> +static int br_getageing(const struct net_device *dev)
> +{
> +	struct net_bridge *br = netdev_priv(dev);
> +
> +	return br->ageing_time / HZ;
> +}
> +
> +static int br_getageing_nl(struct sk_buff *skb,
> +			   const struct net_device *dev,
> +			   struct netlink_callback *cb)
> +{
> +	struct nlmsghdr *nlhdr;
> +	struct admsg *amsg;
> +
> +	struct net_bridge *br = netdev_priv(dev);
> +
> +	nlhdr = nlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> +			  cb->nlh->nlmsg_seq, RTM_GETAGEING, sizeof(*amsg), 0);
> +	if (!nlhdr)
> +		return -EMSGSIZE;
> +
> +	amsg = nlmsg_data(nlhdr);
> +
> +	amsg->adm_family = AF_BRIDGE;
> +	amsg->adm_ifindex = dev->ifindex;
> +	amsg->adm_ageing_interval = br->ageing_time / HZ;
> +
> +	return 0;
> +}
> +
>   static const struct ethtool_ops br_ethtool_ops = {
>   	.get_drvinfo    = br_getinfo,
>   	.get_link	= ethtool_op_get_link,
> @@ -339,6 +377,9 @@ static const struct net_device_ops br_netdev_ops = {
>   	.ndo_bridge_getlink	 = br_getlink,
>   	.ndo_bridge_setlink	 = br_setlink,
>   	.ndo_bridge_dellink	 = br_dellink,
> +	.ndo_bridge_setageing    = br_setageing,
> +	.ndo_bridge_getageing_nl = br_getageing_nl,
> +	.ndo_bridge_getageing    = br_getageing,
>   };
>   
>   static void br_dev_free(struct net_device *dev)
> @@ -391,7 +432,7 @@ void br_dev_setup(struct net_device *dev)
>   	br->bridge_max_age = br->max_age = 20 * HZ;
>   	br->bridge_hello_time = br->hello_time = 2 * HZ;
>   	br->bridge_forward_delay = br->forward_delay = 15 * HZ;
> -	br->ageing_time = 300 * HZ;
> +	br->ageing_time = DEFAULT_AGEING_INTERVAL_SECS * HZ;
>   
>   	br_netfilter_rtable_init(br);
>   	br_stp_timer_init(br);
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 9e9875d..c46f734 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -283,6 +283,25 @@ out:
>   	spin_unlock_bh(&br->hash_lock);
>   }
>   
> +int br_fdb_setageing(struct net_bridge *br, int ageing_interval)
> +{
> +	unsigned long next_timer;
> +	unsigned long exp_time;
> +
> +	if (ageing_interval < MIN_AGEING_INTERVAL_SECS)
> +		return -EINVAL;
> +
> +	br->ageing_time = ageing_interval * HZ;
> +
> +	next_timer = jiffies + hold_time(br);
> +	exp_time = br->gc_timer.expires;
> +
> +	if (exp_time > next_timer)
> +		mod_timer(&br->gc_timer, round_jiffies_up(next_timer));
> +
> +	return 0;
> +}
> +
>   void br_fdb_cleanup(unsigned long _data)
>   {
>   	struct net_bridge *br = (struct net_bridge *)_data;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 3ad1290..b745e06 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -410,6 +410,7 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
>   			      const unsigned char *addr, u16 vid);
>   int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
>   			      const unsigned char *addr, u16 vid);
> +int br_fdb_setageing(struct net_bridge *br, int ageing_interval);
>   
>   /* br_forward.c */
>   void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb);
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 5fb4af2..e7c6197 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3277,6 +3277,128 @@ out:
>   	return err;
>   }
>   
> +static int configure_ageing_interval(struct net_device  *dev, int interval)
> +{
> +	int err = -EOPNOTSUPP;
> +	int old_ageing_timer = -1;
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +
> +	if (ops->ndo_bridge_getageing)
> +		old_ageing_timer = ops->ndo_bridge_getageing(dev);
> +
> +	if (ops->ndo_bridge_setageing)
> +		err = ops->ndo_bridge_setageing(dev, interval);
> +
> +	if (!err) {
> +		err = netdev_switch_ageing_set(dev, interval);
> +
> +		if (err == -EOPNOTSUPP)
> +			return 0;
> +
> +		if (err && (old_ageing_timer >= MIN_AGEING_INTERVAL_SECS))
> +			ops->ndo_bridge_setageing(dev, old_ageing_timer);
> +	}
> +
> +	return err;
> +}
> +
> +static int rtnl_bridge_setageing(struct sk_buff *skb, struct nlmsghdr *nlh)
> +{
> +	struct net *net = sock_net(skb->sk);
> +	struct admsg *adm;
> +	struct net_device *dev;
> +	int err;
> +	struct nlattr *tb[NDA_MAX + 1];
> +
> +	err = nlmsg_parse(nlh, sizeof(*adm), tb, NDA_MAX, NULL);
> +	if (err < 0)
> +		return err;
> +
> +	adm = nlmsg_data(nlh);
> +	if (adm->adm_ifindex == 0)
> +		return -EINVAL;
> +
> +	dev = __dev_get_by_index(net, adm->adm_ifindex);
> +	if (!dev)
> +		return -ENODEV;
> +
> +	err = -EOPNOTSUPP;
> +
> +	if (!(dev->priv_flags & IFF_EBRIDGE))
> +		return err;
> +
> +	err = configure_ageing_interval(dev, adm->adm_ageing_interval);
> +
> +	return err;
> +}
> +
> +static int rtnl_bridge_setdefaultageing(struct sk_buff *skb,
> +					struct nlmsghdr *nlh)
> +{
> +	struct net *net = sock_net(skb->sk);
> +	struct admsg *adm;
> +	struct net_device *dev;
> +	int err;
> +	struct nlattr *tb[NDA_MAX + 1];
> +
> +	err = nlmsg_parse(nlh, sizeof(*adm), tb, NDA_MAX, NULL);
> +	if (err < 0)
> +		return err;
> +
> +	adm = nlmsg_data(nlh);
> +	if (adm->adm_ifindex == 0)
> +		return -EINVAL;
> +
> +	dev = __dev_get_by_index(net, adm->adm_ifindex);
> +	if (!dev)
> +		return -ENODEV;
> +
> +	err = -EOPNOTSUPP;
> +
> +	if (!(dev->priv_flags & IFF_EBRIDGE))
> +		return err;
> +
> +	err = configure_ageing_interval(dev, DEFAULT_AGEING_INTERVAL_SECS);
> +
> +	return err;
> +}
> +
> +static int rtnl_bridge_getageing(struct sk_buff *skb,
> +				 struct netlink_callback *cb)
> +{
> +	struct net *net = sock_net(skb->sk);
> +	struct admsg *adm;
> +	struct net_device *dev;
> +	int err;
> +	struct nlattr *tb[NDA_MAX + 1];
> +
> +	err = nlmsg_parse(cb->nlh, sizeof(*adm), tb, NDA_MAX, NULL);
> +	if (err < 0)
> +		return err;
> +
> +	adm = nlmsg_data(cb->nlh);
> +	if (adm->adm_ifindex == 0)
> +		return -EINVAL;
> +
> +	dev = __dev_get_by_index(net, adm->adm_ifindex);
> +	if (!dev)
> +		return -ENODEV;
> +
> +	err = -EOPNOTSUPP;
> +
> +	if (!(dev->priv_flags & IFF_EBRIDGE))
> +		return err;
> +
> +	if (dev->netdev_ops->ndo_bridge_getageing_nl)
> +		err = dev->netdev_ops->ndo_bridge_getageing_nl(skb, dev, cb);
> +
> +	if (err)
> +		return err;
> +
> +	cb->args[0] = 1;
> +	return 0;
> +}
> +
>   /* Process one rtnetlink message. */
>   
>   static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> @@ -3427,5 +3549,11 @@ void __init rtnetlink_init(void)
>   	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, NULL);
>   	rtnl_register(PF_BRIDGE, RTM_DELLINK, rtnl_bridge_dellink, NULL, NULL);
>   	rtnl_register(PF_BRIDGE, RTM_SETLINK, rtnl_bridge_setlink, NULL, NULL);
> +	rtnl_register(PF_BRIDGE, RTM_GETAGEING, NULL,
> +		      rtnl_bridge_getageing, NULL);
> +	rtnl_register(PF_BRIDGE, RTM_SETAGEING,
> +		      rtnl_bridge_setageing, NULL, NULL);
> +	rtnl_register(PF_BRIDGE, RTM_SETDEFAULTAGEING,
> +		      rtnl_bridge_setdefaultageing, NULL, NULL);
>   }
>   
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 33bafa2..501e892 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -1142,3 +1142,34 @@ void switchdev_port_fwd_mark_set(struct net_device *dev,
>   	dev->offload_fwd_mark = mark;
>   }
>   EXPORT_SYMBOL_GPL(switchdev_port_fwd_mark_set);
> +
> +int netdev_switch_ageing_set(struct net_device *dev,
> +			     int ageing_interval)
> +{
> +	int err = 0;
> +
> +	struct net_device *pd;
> +	struct list_head *iter;
> +
> +	netdev_for_each_lower_dev(dev, pd, iter) {
> +		if (!pd || !pd->netdev_ops ||
> +		    !pd->netdev_ops->ndo_bridge_setageing)
> +			continue;
> +
> +		err = pd->netdev_ops->ndo_bridge_setageing(pd,
> +							ageing_interval);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(netdev_switch_ageing_set);
> +
> +int netdev_switch_ageing_get(struct sk_buff *skb,
> +			     const struct net_device *dev,
> +			     struct netlink_callback *cb)
> +{
> +	return -EOPNOTSUPP;
> +}
> +EXPORT_SYMBOL_GPL(netdev_switch_ageing_get);
> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> index 2bbb418..e29836e 100644
> --- a/security/selinux/nlmsgtab.c
> +++ b/security/selinux/nlmsgtab.c
> @@ -154,7 +154,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
>   	switch (sclass) {
>   	case SECCLASS_NETLINK_ROUTE_SOCKET:
>   		/* RTM_MAX always point to RTM_SETxxxx, ie RTM_NEWxxx + 3 */
> -		BUILD_BUG_ON(RTM_MAX != (RTM_NEWNSID + 3));
> +		BUILD_BUG_ON(RTM_MAX != (RTM_SETAGEING + 3));
>   		err = nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms,
>   				 sizeof(nlmsg_route_perms));
>   		break;
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Premkumar Jonnala]

Hello Roopa,

Thank you for the comments.  Some thoughts/comments on IFLA_BR_AGEING_TIME.

1. Ageing interval using IFLA_BR_AGEING_TIME is set using 'ip link ..' command.  Shouldn't
bridge command be more appropriate for this?  The earlier patch seems to allow configuration of
other bridge related parameters using 'ip link' command.  

> ip link set dev br0 type bridge priority 10
> ip link set dev br0 type bridge ageing_time 10
> ip link set dev br0 type bridge stp_state 1

2. Another change as part of the proposed patch is to propagate the ageing interval to switchdev
devices so that hardware based aging can be setup correctly.

3. 'ip link' command doesn't seem to do any checks on the values given for ageing.  IEEE 802.1d 
Recommends that the time be between 10 sec and 1M seconds.

4. The ageing timer in kernel isn't restarted until the next expiry.  While this doesn't cause any
harm, it may be nice if the timer is restarted upon configuration change immediately.  

5. Also, similar to #3 above, the kernel doesn't check for any valid values of ageing time that 
it receives via netlink.

-Prem

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of roopa
Sent: Friday, August 14, 2015 12:13 PM
To: Premkumar Jonnala
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

On 8/13/15, 11:23 PM, Premkumar Jonnala wrote:
> Bridge devices have ageing interval used to age out MAC addresses
> from FDB.  This ageing interval was not configuratble.
>
> Enable netlink based configuration of ageing interval for bridges and
> switch devices.  The ageing interval changes the timer used to purge
> inactive FDB entries in bridges.  The ageing interval config is
> propagated to switch devices, so that platform or hardware based
> ageing works according to configuration.
>
> Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>

How is this different from netlink attribute IFLA_BR_AGEING_TIME ?
>
> ---
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 607b5f4..e3b0c45 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1053,7 +1053,16 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
>    *	This function is used to pass protocol port error state information
>    *	to the switch driver. The switch driver can react to the proto_down
>    *      by doing a phys down on the associated switch port.
> - *
> + * int (*ndo_bridge_setageing)(const struct net_device *dev,
> + *			       int ageing_interval);
> + *     Called to set FDB aging interval for a given bridge device.
> + * int (*ndo_bridge_getageing_nl)(struct sk_buff *skb,
> + *				const struct net_device *dev,
> + *				struct netlink_callback *cb);
> + *     Called to return the ageing interval for the given bridge device,
> + *     in a format suitable for netlink messaging.
> + * int (*ndo_bridge_getageing)(const struct net_device *dev);
> + *     Called to retrieve the ageing interval for the given bridge device.
>    */
>   struct net_device_ops {
>   	int			(*ndo_init)(struct net_device *dev);
> @@ -1226,6 +1235,13 @@ struct net_device_ops {
>   	int			(*ndo_get_iflink)(const struct net_device *dev);
>   	int			(*ndo_change_proto_down)(struct net_device *dev,
>   							 bool proto_down);
> +	int	(*ndo_bridge_setageing)(const struct net_device *dev,
> +					int ageing_interval);
> +	int	(*ndo_bridge_getageing_nl)(struct sk_buff *skb,
> +					   const struct net_device *dev,
> +					   struct netlink_callback *cb);
> +
> +	int	(*ndo_bridge_getageing)(const struct net_device *dev);
>   };
>   
you cannot add new ndo's for each of these. It should be covered as part of
existing br_link_ops

>   /**
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index 89da893..7186fea 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -129,6 +129,10 @@ int switchdev_port_attr_get(struct net_device *dev,
>   			    struct switchdev_attr *attr);
>   int switchdev_port_attr_set(struct net_device *dev,
>   			    struct switchdev_attr *attr);
> +int netdev_switch_ageing_set(struct net_device *dev, int ageing_interval);
> +int netdev_switch_ageing_get(struct sk_buff *skb,
> +			     const struct net_device *dev,
> +			     struct netlink_callback *cb);
>   int switchdev_port_obj_add(struct net_device *dev, struct switchdev_obj *obj);
>   int switchdev_port_obj_del(struct net_device *dev, struct switchdev_obj *obj);
>   int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj);
> @@ -163,6 +167,17 @@ void switchdev_port_fwd_mark_set(struct net_device *dev,
>   
>   #else
>   
> +static inline int netdev_switch_ageing_set(struct net_device *dev,
> +					int ageing_interval)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int netdev_switch_ageing_get(struct net_device *dev)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>   static inline int switchdev_port_attr_get(struct net_device *dev,
>   					  struct switchdev_attr *attr)
>   {
> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> index 3635b77..a32ab4d 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -199,4 +199,23 @@ enum {
>   };
>   #define MDBA_SET_ENTRY_MAX (__MDBA_SET_ENTRY_MAX - 1)
>   
> +struct admsg {
> +	__u8 adm_family;
> +	__u8 adm_pad1;
> +	__u16 adm_pad2;
> +	__s32 adm_ifindex;
> +	__u16 adm_ageing_interval;
> +};
> +
> +/* The value of this macro is based on the value recommended by IEEE
> + * standard 802.1d.
> + */
> +#define MIN_AGEING_INTERVAL_SECS (10)
> +
> +/* The value of DEFAULT_AGEING_INTERVAL_SECS is the default ageing
> + * interval that was used in br_device.c.  This default value is also
> + * recommended by IEEE Standard 802.1d.
> + */
> +#define DEFAULT_AGEING_INTERVAL_SECS (300)
> +
>   #endif /* _UAPI_LINUX_IF_BRIDGE_H */
> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
> index 47d24cb..9321818 100644
> --- a/include/uapi/linux/rtnetlink.h
> +++ b/include/uapi/linux/rtnetlink.h
> @@ -139,6 +139,13 @@ enum {
>   	RTM_GETNSID = 90,
>   #define RTM_GETNSID RTM_GETNSID
>   
> +	RTM_SETAGEING = 92,
> +#define RTM_SETAGEING RTM_SETAGEING
> +	RTM_SETDEFAULTAGEING = 93,
> +#define RTM_SETDEFAULTAGEING RTM_SETDEFAULTAGEING
> +	RTM_GETAGEING = 94,
> +#define RTM_GETAGEING RTM_GETAGEING
> +

you cannot add a new RTM message for a single attribute either.
>   	__RTM_MAX,
>   #define RTM_MAX		(((__RTM_MAX + 3) & ~3) - 1)
>   };
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 4ff77a1..2c81190 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -18,6 +18,7 @@
>   #include <linux/ethtool.h>
>   #include <linux/list.h>
>   #include <linux/netfilter_bridge.h>
> +#include <net/netlink.h>
>   
>   #include <asm/uaccess.h>
>   #include "br_private.h"
> @@ -309,6 +310,43 @@ static int br_del_slave(struct net_device *dev, struct net_device *slave_dev)
>   	return br_del_if(br, slave_dev);
>   }
>   
> +static int br_setageing(const struct net_device *dev, int ageing_interval)
> +{
> +	struct net_bridge *br = netdev_priv(dev);
> +
> +	return br_fdb_setageing(br, ageing_interval);
> +}
> +
> +static int br_getageing(const struct net_device *dev)
> +{
> +	struct net_bridge *br = netdev_priv(dev);
> +
> +	return br->ageing_time / HZ;
> +}
> +
> +static int br_getageing_nl(struct sk_buff *skb,
> +			   const struct net_device *dev,
> +			   struct netlink_callback *cb)
> +{
> +	struct nlmsghdr *nlhdr;
> +	struct admsg *amsg;
> +
> +	struct net_bridge *br = netdev_priv(dev);
> +
> +	nlhdr = nlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> +			  cb->nlh->nlmsg_seq, RTM_GETAGEING, sizeof(*amsg), 0);
> +	if (!nlhdr)
> +		return -EMSGSIZE;
> +
> +	amsg = nlmsg_data(nlhdr);
> +
> +	amsg->adm_family = AF_BRIDGE;
> +	amsg->adm_ifindex = dev->ifindex;
> +	amsg->adm_ageing_interval = br->ageing_time / HZ;
> +
> +	return 0;
> +}
> +
>   static const struct ethtool_ops br_ethtool_ops = {
>   	.get_drvinfo    = br_getinfo,
>   	.get_link	= ethtool_op_get_link,
> @@ -339,6 +377,9 @@ static const struct net_device_ops br_netdev_ops = {
>   	.ndo_bridge_getlink	 = br_getlink,
>   	.ndo_bridge_setlink	 = br_setlink,
>   	.ndo_bridge_dellink	 = br_dellink,
> +	.ndo_bridge_setageing    = br_setageing,
> +	.ndo_bridge_getageing_nl = br_getageing_nl,
> +	.ndo_bridge_getageing    = br_getageing,
>   };
>   
>   static void br_dev_free(struct net_device *dev)
> @@ -391,7 +432,7 @@ void br_dev_setup(struct net_device *dev)
>   	br->bridge_max_age = br->max_age = 20 * HZ;
>   	br->bridge_hello_time = br->hello_time = 2 * HZ;
>   	br->bridge_forward_delay = br->forward_delay = 15 * HZ;
> -	br->ageing_time = 300 * HZ;
> +	br->ageing_time = DEFAULT_AGEING_INTERVAL_SECS * HZ;
>   
>   	br_netfilter_rtable_init(br);
>   	br_stp_timer_init(br);
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 9e9875d..c46f734 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -283,6 +283,25 @@ out:
>   	spin_unlock_bh(&br->hash_lock);
>   }
>   
> +int br_fdb_setageing(struct net_bridge *br, int ageing_interval)
> +{
> +	unsigned long next_timer;
> +	unsigned long exp_time;
> +
> +	if (ageing_interval < MIN_AGEING_INTERVAL_SECS)
> +		return -EINVAL;
> +
> +	br->ageing_time = ageing_interval * HZ;
> +
> +	next_timer = jiffies + hold_time(br);
> +	exp_time = br->gc_timer.expires;
> +
> +	if (exp_time > next_timer)
> +		mod_timer(&br->gc_timer, round_jiffies_up(next_timer));
> +
> +	return 0;
> +}
> +
>   void br_fdb_cleanup(unsigned long _data)
>   {
>   	struct net_bridge *br = (struct net_bridge *)_data;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 3ad1290..b745e06 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -410,6 +410,7 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
>   			      const unsigned char *addr, u16 vid);
>   int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
>   			      const unsigned char *addr, u16 vid);
> +int br_fdb_setageing(struct net_bridge *br, int ageing_interval);
>   
>   /* br_forward.c */
>   void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb);
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 5fb4af2..e7c6197 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3277,6 +3277,128 @@ out:
>   	return err;
>   }
>   
> +static int configure_ageing_interval(struct net_device  *dev, int interval)
> +{
> +	int err = -EOPNOTSUPP;
> +	int old_ageing_timer = -1;
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +
> +	if (ops->ndo_bridge_getageing)
> +		old_ageing_timer = ops->ndo_bridge_getageing(dev);
> +
> +	if (ops->ndo_bridge_setageing)
> +		err = ops->ndo_bridge_setageing(dev, interval);
> +
> +	if (!err) {
> +		err = netdev_switch_ageing_set(dev, interval);
> +
> +		if (err == -EOPNOTSUPP)
> +			return 0;
> +
> +		if (err && (old_ageing_timer >= MIN_AGEING_INTERVAL_SECS))
> +			ops->ndo_bridge_setageing(dev, old_ageing_timer);
> +	}
> +
> +	return err;
> +}
> +
> +static int rtnl_bridge_setageing(struct sk_buff *skb, struct nlmsghdr *nlh)
> +{
> +	struct net *net = sock_net(skb->sk);
> +	struct admsg *adm;
> +	struct net_device *dev;
> +	int err;
> +	struct nlattr *tb[NDA_MAX + 1];
> +
> +	err = nlmsg_parse(nlh, sizeof(*adm), tb, NDA_MAX, NULL);
> +	if (err < 0)
> +		return err;
> +
> +	adm = nlmsg_data(nlh);
> +	if (adm->adm_ifindex == 0)
> +		return -EINVAL;
> +
> +	dev = __dev_get_by_index(net, adm->adm_ifindex);
> +	if (!dev)
> +		return -ENODEV;
> +
> +	err = -EOPNOTSUPP;
> +
> +	if (!(dev->priv_flags & IFF_EBRIDGE))
> +		return err;
> +
> +	err = configure_ageing_interval(dev, adm->adm_ageing_interval);
> +
> +	return err;
> +}
> +
> +static int rtnl_bridge_setdefaultageing(struct sk_buff *skb,
> +					struct nlmsghdr *nlh)
> +{
> +	struct net *net = sock_net(skb->sk);
> +	struct admsg *adm;
> +	struct net_device *dev;
> +	int err;
> +	struct nlattr *tb[NDA_MAX + 1];
> +
> +	err = nlmsg_parse(nlh, sizeof(*adm), tb, NDA_MAX, NULL);
> +	if (err < 0)
> +		return err;
> +
> +	adm = nlmsg_data(nlh);
> +	if (adm->adm_ifindex == 0)
> +		return -EINVAL;
> +
> +	dev = __dev_get_by_index(net, adm->adm_ifindex);
> +	if (!dev)
> +		return -ENODEV;
> +
> +	err = -EOPNOTSUPP;
> +
> +	if (!(dev->priv_flags & IFF_EBRIDGE))
> +		return err;
> +
> +	err = configure_ageing_interval(dev, DEFAULT_AGEING_INTERVAL_SECS);
> +
> +	return err;
> +}
> +
> +static int rtnl_bridge_getageing(struct sk_buff *skb,
> +				 struct netlink_callback *cb)
> +{
> +	struct net *net = sock_net(skb->sk);
> +	struct admsg *adm;
> +	struct net_device *dev;
> +	int err;
> +	struct nlattr *tb[NDA_MAX + 1];
> +
> +	err = nlmsg_parse(cb->nlh, sizeof(*adm), tb, NDA_MAX, NULL);
> +	if (err < 0)
> +		return err;
> +
> +	adm = nlmsg_data(cb->nlh);
> +	if (adm->adm_ifindex == 0)
> +		return -EINVAL;
> +
> +	dev = __dev_get_by_index(net, adm->adm_ifindex);
> +	if (!dev)
> +		return -ENODEV;
> +
> +	err = -EOPNOTSUPP;
> +
> +	if (!(dev->priv_flags & IFF_EBRIDGE))
> +		return err;
> +
> +	if (dev->netdev_ops->ndo_bridge_getageing_nl)
> +		err = dev->netdev_ops->ndo_bridge_getageing_nl(skb, dev, cb);
> +
> +	if (err)
> +		return err;
> +
> +	cb->args[0] = 1;
> +	return 0;
> +}
> +
>   /* Process one rtnetlink message. */
>   
>   static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> @@ -3427,5 +3549,11 @@ void __init rtnetlink_init(void)
>   	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, NULL);
>   	rtnl_register(PF_BRIDGE, RTM_DELLINK, rtnl_bridge_dellink, NULL, NULL);
>   	rtnl_register(PF_BRIDGE, RTM_SETLINK, rtnl_bridge_setlink, NULL, NULL);
> +	rtnl_register(PF_BRIDGE, RTM_GETAGEING, NULL,
> +		      rtnl_bridge_getageing, NULL);
> +	rtnl_register(PF_BRIDGE, RTM_SETAGEING,
> +		      rtnl_bridge_setageing, NULL, NULL);
> +	rtnl_register(PF_BRIDGE, RTM_SETDEFAULTAGEING,
> +		      rtnl_bridge_setdefaultageing, NULL, NULL);
>   }
>   
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 33bafa2..501e892 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -1142,3 +1142,34 @@ void switchdev_port_fwd_mark_set(struct net_device *dev,
>   	dev->offload_fwd_mark = mark;
>   }
>   EXPORT_SYMBOL_GPL(switchdev_port_fwd_mark_set);
> +
> +int netdev_switch_ageing_set(struct net_device *dev,
> +			     int ageing_interval)
> +{
> +	int err = 0;
> +
> +	struct net_device *pd;
> +	struct list_head *iter;
> +
> +	netdev_for_each_lower_dev(dev, pd, iter) {
> +		if (!pd || !pd->netdev_ops ||
> +		    !pd->netdev_ops->ndo_bridge_setageing)
> +			continue;
> +
> +		err = pd->netdev_ops->ndo_bridge_setageing(pd,
> +							ageing_interval);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(netdev_switch_ageing_set);
> +
> +int netdev_switch_ageing_get(struct sk_buff *skb,
> +			     const struct net_device *dev,
> +			     struct netlink_callback *cb)
> +{
> +	return -EOPNOTSUPP;
> +}
> +EXPORT_SYMBOL_GPL(netdev_switch_ageing_get);
> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> index 2bbb418..e29836e 100644
> --- a/security/selinux/nlmsgtab.c
> +++ b/security/selinux/nlmsgtab.c
> @@ -154,7 +154,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
>   	switch (sclass) {
>   	case SECCLASS_NETLINK_ROUTE_SOCKET:
>   		/* RTM_MAX always point to RTM_SETxxxx, ie RTM_NEWxxx + 3 */
> -		BUILD_BUG_ON(RTM_MAX != (RTM_NEWNSID + 3));
> +		BUILD_BUG_ON(RTM_MAX != (RTM_SETAGEING + 3));
>   		err = nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms,
>   				 sizeof(nlmsg_route_perms));
>   		break;
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Rosen, Rami]

Hi,

First, I agree about the need to propagate the ageing interval to switchdev devices, so that hardware based aging can be setup correctly.

Second, in this occasion, I want to mention the need to 
turn off bridge ageing in the kernel as part of using switchdev devices. This is mentioned in
https://kernel.googlesource.com/pub/scm/linux/kernel/git/davem/net-next/+/master/Documentation/networking/switchdev.txt:

...
XXX: how to turn off ageing in kernel on a per-port basis or
otherwise prevent the kernel from ageing out the FDB entry?
...

One can think of the option of using value 0 of the ageing interval as an indication to turn off bridge ageing in the kernel, and any other value bigger than MIN_AGEING_INTERVAL_SECS to turn on bridge ageing.

As another option for a *per-port* boolean flag for enabling/disabling ageing, one can think of adding an IFLA_BRPORT_AGEING bool flag (and BR_AGEING) for IFLA_PROTINFO.

Regards,
Rami Rosen
Intel Corporation

Re: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Scott Feldman]

On Fri, 14 Aug 2015, Premkumar Jonnala wrote:

> Bridge devices have ageing interval used to age out MAC addresses
> from FDB.  This ageing interval was not configuratble.
>
> Enable netlink based configuration of ageing interval for bridges and
> switch devices.  The ageing interval changes the timer used to purge
> inactive FDB entries in bridges.  The ageing interval config is
> propagated to switch devices, so that platform or hardware based
> ageing works according to configuration.
>
> Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>

Hi Premkumar,

I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.

I hope you don't mind if share a patch for setting bridge-level attributes 
down to the switch hardware ports.  It uses the switchdev_port_attr_set() 
call on the bridge interface itself when any IFLA_BR_xxx attribute is set 
via netlink.   switchdev_port_attr_set() will do a recursive walk of all 
lower devs from the bridge, stopping at each to set the attribute.  I 
added one small tweak to switchdev_port_attr_set() to allow skipping over 
ports that return -EOPNOTSUPP.  The advantage to using 
switchdev_port_attr_set() is it automatically knows how to find the leaf 
ports in a stacked driver setup, and it uses the prepare/commit 
transaction model to make sure all ports can support new attr setting 
before committing setting to hardware.

I gave an example using rocker to set IFLA_BR_AGEING_TIME.  It's stubbed 
out, but you get the idea.  Other IFLA_BR_xxx attrs can be added to this 
model.

Does this look like something that would work?  If so, would you mind 
running with this patch and maybe even extending it for other IFLA_BR_xxx 
attrs?

Again, I hope I'm not stepping on toes here by rewriting your patch, but 
it was the best way to communicate the idea of how to use the switchdev 
frame work for these bridge attrs.

-scott




diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 0cc9e8f..830f8e6 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -4680,6 +4680,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:
+		{
+			u32 ageing_time = bridge->val;
+			/* XXX push ageing_time down to device */
+		}
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
  static int rocker_port_attr_set(struct net_device *dev,
  				struct switchdev_attr *attr)
  {
@@ -4707,6 +4725,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;
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 319baab..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,
@@ -28,6 +29,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 +40,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 ff659f0..0630053 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,
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 0f2408f..01401ea 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -759,9 +759,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 3d95647..9807a6f 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -807,6 +807,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..5de27bc 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -566,6 +566,25 @@ 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,
+	};
+	int err;
+
+	err = switchdev_port_attr_set(br->dev, &attr);
+	if (err)
+		return err;
+
+	br->ageing_time = clock_t_to_jiffies(ageing_time);
+
+	return 0;
+}
+
  void __br_set_forward_delay(struct net_bridge *br, unsigned long t)
  {
  	br->bridge_forward_delay = t;
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 16c1c43..b990301 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;
  }

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Premkumar Jonnala]

> -----Original Message-----
> From: Rosen, Rami [mailto:rami.rosen@intel.com]
> Sent: Tuesday, August 18, 2015 10:25 AM
> To: Premkumar Jonnala; roopa
> Cc: netdev@vger.kernel.org
> Subject: RE: [PATCH] bridge: Enable configuration of ageing interval for bridges
> and switch devices.
> 
> Hi,
> 
> First, I agree about the need to propagate the ageing interval to switchdev
> devices, so that hardware based aging can be setup correctly.
> 
> Second, in this occasion, I want to mention the need to
> turn off bridge ageing in the kernel as part of using switchdev devices. This is
> mentioned in
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/davem/net-
> next/+/master/Documentation/networking/switchdev.txt:
> 
> ...
> XXX: how to turn off ageing in kernel on a per-port basis or
> otherwise prevent the kernel from ageing out the FDB entry?
> ...
> 
> One can think of the option of using value 0 of the ageing interval as an
> indication to turn off bridge ageing in the kernel, and any other value bigger
> than MIN_AGEING_INTERVAL_SECS to turn on bridge ageing.

I recall that there was a patch proposed to prevent ageing of fdb entried by bridge in kernel,
when the fdb entry was added due to notification by switch device.  Please see:
http://www.spinics.net/lists/netdev/msg314770.html

Somehow the patch is not visible in the net-next pull.

-Prem

> 
> As another option for a *per-port* boolean flag for enabling/disabling ageing,
> one can think of adding an IFLA_BRPORT_AGEING bool flag (and BR_AGEING)
> for IFLA_PROTINFO.
> 
> Regards,
> Rami Rosen
> Intel Corporation

Re: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Michal Kubecek]

On Tue, Aug 18, 2015 at 09:08:42AM +0000, Premkumar Jonnala wrote:
> > -----Original Message-----
> > From: Rosen, Rami [mailto:rami.rosen@intel.com]
> > Sent: Tuesday, August 18, 2015 10:25 AM
> > To: Premkumar Jonnala; roopa
> > Cc: netdev@vger.kernel.org
> > Subject: RE: [PATCH] bridge: Enable configuration of ageing interval for bridges
> > and switch devices.
> > 
> > Hi,
> > 
> > First, I agree about the need to propagate the ageing interval to switchdev
> > devices, so that hardware based aging can be setup correctly.
> > 
> > Second, in this occasion, I want to mention the need to
> > turn off bridge ageing in the kernel as part of using switchdev devices. This is
> > mentioned in
> > https://kernel.googlesource.com/pub/scm/linux/kernel/git/davem/net-
> > next/+/master/Documentation/networking/switchdev.txt:
> > 
> > ...
> > XXX: how to turn off ageing in kernel on a per-port basis or
> > otherwise prevent the kernel from ageing out the FDB entry?
> > ...
> > 
> > One can think of the option of using value 0 of the ageing interval as an
> > indication to turn off bridge ageing in the kernel, and any other value bigger
> > than MIN_AGEING_INTERVAL_SECS to turn on bridge ageing.
> 
> I recall that there was a patch proposed to prevent ageing of fdb entried by bridge in kernel,
> when the fdb entry was added due to notification by switch device.  Please see:
> http://www.spinics.net/lists/netdev/msg314770.html
> 
> Somehow the patch is not visible in the net-next pull.

It has been reverted by commit 3fcf90111887 as requested by

  http://www.spinics.net/lists/netdev/msg315236.html

Michal Kubecek

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Rosen, Rami]

Hi, Prem, 

>I recall that there was a patch proposed to prevent ageing of fdb entried by bridge in kernel, when the fdb entry was added due to >notification by switch device.  Please see:
>http://www.spinics.net/lists/netdev/msg314770.html

>Somehow the patch is not visible in the net-next pull.

It seems that this patch was reverted:
http://lists.openwall.net/netdev/2015/02/05/129
https://lkml.org/lkml/2015/2/11/12

Regards,
Rami Rosen
Intel Corporation

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Premkumar Jonnala]

Hello Scott,

Thank you for the diff and comments.   Please see my comments inline.

> -----Original Message-----
> From: Scott Feldman [mailto:sfeldma@gmail.com]
> Sent: Tuesday, August 18, 2015 12:48 PM
> To: Premkumar Jonnala
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges
> and switch devices.
> 
> 
> 
> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
> 
> > Bridge devices have ageing interval used to age out MAC addresses
> > from FDB.  This ageing interval was not configuratble.
> >
> > Enable netlink based configuration of ageing interval for bridges and
> > switch devices.  The ageing interval changes the timer used to purge
> > inactive FDB entries in bridges.  The ageing interval config is
> > propagated to switch devices, so that platform or hardware based
> > ageing works according to configuration.
> >
> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
> 
> Hi Premkumar,
> 
> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.

What is the motivation for using 'ip link' command to configure bridge attributes?  IMHO,
bridge command is better suited for that. 

> 
> I hope you don't mind if share a patch for setting bridge-level attributes
> down to the switch hardware ports.  It uses the switchdev_port_attr_set()
> call on the bridge interface itself when any IFLA_BR_xxx attribute is set
> via netlink.   switchdev_port_attr_set() will do a recursive walk of all
> lower devs from the bridge, stopping at each to set the attribute.  I
> added one small tweak to switchdev_port_attr_set() to allow skipping over
> ports that return -EOPNOTSUPP.  The advantage to using
> switchdev_port_attr_set() is it automatically knows how to find the leaf
> ports in a stacked driver setup, and it uses the prepare/commit
> transaction model to make sure all ports can support new attr setting
> before committing setting to hardware.
> 
> I gave an example using rocker to set IFLA_BR_AGEING_TIME.  It's stubbed
> out, but you get the idea.  Other IFLA_BR_xxx attrs can be added to this
> model.
> 
> Does this look like something that would work?  If so, would you mind
> running with this patch and maybe even extending it for other IFLA_BR_xxx
> attrs?

Sure, let me go with this patch.  Couple of comments inline.

> 
> Again, I hope I'm not stepping on toes here by rewriting your patch, but
> it was the best way to communicate the idea of how to use the switchdev
> frame work for these bridge attrs.
> 
> -scott
> 
> 
> 
> 
> diff --git a/drivers/net/ethernet/rocker/rocker.c
> b/drivers/net/ethernet/rocker/rocker.c
> index 0cc9e8f..830f8e6 100644
> --- a/drivers/net/ethernet/rocker/rocker.c
> +++ b/drivers/net/ethernet/rocker/rocker.c
> @@ -4680,6 +4680,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:
> +		{
> +			u32 ageing_time = bridge->val;
> +			/* XXX push ageing_time down to device */
> +		}
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
>   static int rocker_port_attr_set(struct net_device *dev,
>   				struct switchdev_attr *attr)
>   {
> @@ -4707,6 +4725,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;
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index 319baab..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,
> @@ -28,6 +29,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 +40,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 ff659f0..0630053 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,
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 0f2408f..01401ea 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -759,9 +759,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]);

Should we do some range checking here to ensure that the value is within a certain range.
IEEE 802.1d recommends that the ageing time be between 10 sec and 1 million seconds.

> -
> -		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 3d95647..9807a6f 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -807,6 +807,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..5de27bc 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -566,6 +566,25 @@ 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,
> +	};
> +	int err;
> +
> +	err = switchdev_port_attr_set(br->dev, &attr);
> +	if (err)
> +		return err;
> +
> +	br->ageing_time = clock_t_to_jiffies(ageing_time);

Should we restart the timer here the new time takes effect?

> +
> +	return 0;
> +}
> +
>   void __br_set_forward_delay(struct net_bridge *br, unsigned long t)
>   {
>   	br->bridge_forward_delay = t;
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 16c1c43..b990301 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;
>   }

-Prem

Re: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Scott Feldman]

On Wed, Aug 19, 2015 at 2:34 AM, Premkumar Jonnala
<pjonnala@broadcom.com> wrote:
> Hello Scott,
>
> Thank you for the diff and comments.   Please see my comments inline.
>
>> -----Original Message-----
>> From: Scott Feldman [mailto:sfeldma@gmail.com]
>> Sent: Tuesday, August 18, 2015 12:48 PM
>> To: Premkumar Jonnala
>> Cc: netdev@vger.kernel.org
>> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges
>> and switch devices.
>>
>>
>>
>> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
>>
>> > Bridge devices have ageing interval used to age out MAC addresses
>> > from FDB.  This ageing interval was not configuratble.
>> >
>> > Enable netlink based configuration of ageing interval for bridges and
>> > switch devices.  The ageing interval changes the timer used to purge
>> > inactive FDB entries in bridges.  The ageing interval config is
>> > propagated to switch devices, so that platform or hardware based
>> > ageing works according to configuration.
>> >
>> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
>>
>> Hi Premkumar,
>>
>> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
>
> What is the motivation for using 'ip link' command to configure bridge attributes?  IMHO,
> bridge command is better suited for that.

Can you extend bridge command to allow setting/getting these bridge
attrs?  Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No
changes needed to the kernel.

bridge link set dev br0 ageing_time 1000

 --or--

ip link set dev br0 type bridge ageing_time 1000

>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 0f2408f..01401ea 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -759,9 +759,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]);
>
> Should we do some range checking here to ensure that the value is within a certain range.
> IEEE 802.1d recommends that the ageing time be between 10 sec and 1 million seconds.

Sure, but make that a separate patch.

>> +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,
>> +     };
>> +     int err;
>> +
>> +     err = switchdev_port_attr_set(br->dev, &attr);
>> +     if (err)
>> +             return err;
>> +
>> +     br->ageing_time = clock_t_to_jiffies(ageing_time);
>
> Should we restart the timer here the new time takes effect?

I don't know...I just copied what the original code did.  If it does
need to be restarted, break that out as a separate patch.

-scott

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Wilson, Daniel G]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Scott Feldman
> Sent: Wednesday, August 19, 2015 12:54 PM
> To: Premkumar Jonnala
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges
> and switch devices.
> 
> On Wed, Aug 19, 2015 at 2:34 AM, Premkumar Jonnala
> <pjonnala@broadcom.com> wrote:
> > Hello Scott,
> >
> > Thank you for the diff and comments.   Please see my comments inline.
> >
> >> -----Original Message-----
> >> From: Scott Feldman [mailto:sfeldma@gmail.com]
> >> Sent: Tuesday, August 18, 2015 12:48 PM
> >> To: Premkumar Jonnala
> >> Cc: netdev@vger.kernel.org
> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval
> >> for bridges and switch devices.
> >>
> >>
> >>
> >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
> >>
> >> > Bridge devices have ageing interval used to age out MAC addresses
> >> > from FDB.  This ageing interval was not configuratble.
> >> >
> >> > Enable netlink based configuration of ageing interval for bridges
> >> > and switch devices.  The ageing interval changes the timer used to
> >> > purge inactive FDB entries in bridges.  The ageing interval config
> >> > is propagated to switch devices, so that platform or hardware based
> >> > ageing works according to configuration.
> >> >
> >> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
> >>
> >> Hi Premkumar,
> >>
> >> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
> >
> > What is the motivation for using 'ip link' command to configure bridge
> > attributes?  IMHO, bridge command is better suited for that.
> 
> Can you extend bridge command to allow setting/getting these bridge attrs?
> Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No changes
> needed to the kernel.
> 
> bridge link set dev br0 ageing_time 1000
> 
>  --or--
> 
> ip link set dev br0 type bridge ageing_time 1000

Being able to set these attributes via both bridge and ip would be great.

> >> +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,
> >> +     };
> >> +     int err;
> >> +
> >> +     err = switchdev_port_attr_set(br->dev, &attr);
> >> +     if (err)
> >> +             return err;
> >> +
> >> +     br->ageing_time = clock_t_to_jiffies(ageing_time);
> >
> > Should we restart the timer here the new time takes effect?
> 
> I don't know...I just copied what the original code did.  If it does need to be
> restarted, break that out as a separate patch.

In my opinion, yes, the timer should be restarted. If the timer had been set to 1 million seconds and is being changed to 1 minute, you wouldn't want to wait for the 1-million-second  timer to expire before resetting it to the newly-configured 1-minute timer value.

Dan.

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Premkumar Jonnala]

Thank you Scott.  Please see inline.

> >> -----Original Message-----
> >> From: Scott Feldman [mailto:sfeldma@gmail.com]
> >> Sent: Tuesday, August 18, 2015 12:48 PM
> >> To: Premkumar Jonnala
> >> Cc: netdev@vger.kernel.org
> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for
> bridges
> >> and switch devices.
> >>
> >>
> >>
> >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
> >>
> >> > Bridge devices have ageing interval used to age out MAC addresses
> >> > from FDB.  This ageing interval was not configuratble.
> >> >
> >> > Enable netlink based configuration of ageing interval for bridges and
> >> > switch devices.  The ageing interval changes the timer used to purge
> >> > inactive FDB entries in bridges.  The ageing interval config is
> >> > propagated to switch devices, so that platform or hardware based
> >> > ageing works according to configuration.
> >> >
> >> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
> >>
> >> Hi Premkumar,
> >>
> >> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
> >
> > What is the motivation for using 'ip link' command to configure bridge
> attributes?  IMHO,
> > bridge command is better suited for that.
> 
> Can you extend bridge command to allow setting/getting these bridge
> attrs?  Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No
> changes needed to the kernel.
> 
> bridge link set dev br0 ageing_time 1000
> 
>  --or--
> 
> ip link set dev br0 type bridge ageing_time 1000

I'd prefer to deprecate/remove all the 6 options on the 'ip link' command and move them to 'bridge' command.  

> 
> >> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> >> index 0f2408f..01401ea 100644
> >> --- a/net/bridge/br_netlink.c
> >> +++ b/net/bridge/br_netlink.c
> >> @@ -759,9 +759,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]);
> >
> > Should we do some range checking here to ensure that the value is within a
> certain range.
> > IEEE 802.1d recommends that the ageing time be between 10 sec and 1 million
> seconds.
> 
> Sure, but make that a separate patch.

Sure.

> 
> >> +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,
> >> +     };
> >> +     int err;
> >> +
> >> +     err = switchdev_port_attr_set(br->dev, &attr);
> >> +     if (err)
> >> +             return err;
> >> +
> >> +     br->ageing_time = clock_t_to_jiffies(ageing_time);
> >
> > Should we restart the timer here the new time takes effect?
> 
> I don't know...I just copied what the original code did.  If it does
> need to be restarted, break that out as a separate patch.

There is another comment on this thread where it was suggested that we restart the timer.  I will
make the change in a separate patch.

-Prem

> 
> -scott

Re: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Scott Feldman]

On Wed, Aug 19, 2015 at 9:56 PM, Premkumar Jonnala
<pjonnala@broadcom.com> wrote:
> Thank you Scott.  Please see inline.
>
>> >> -----Original Message-----
>> >> From: Scott Feldman [mailto:sfeldma@gmail.com]
>> >> Sent: Tuesday, August 18, 2015 12:48 PM
>> >> To: Premkumar Jonnala
>> >> Cc: netdev@vger.kernel.org
>> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for
>> bridges
>> >> and switch devices.
>> >>
>> >>
>> >>
>> >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
>> >>
>> >> > Bridge devices have ageing interval used to age out MAC addresses
>> >> > from FDB.  This ageing interval was not configuratble.
>> >> >
>> >> > Enable netlink based configuration of ageing interval for bridges and
>> >> > switch devices.  The ageing interval changes the timer used to purge
>> >> > inactive FDB entries in bridges.  The ageing interval config is
>> >> > propagated to switch devices, so that platform or hardware based
>> >> > ageing works according to configuration.
>> >> >
>> >> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
>> >>
>> >> Hi Premkumar,
>> >>
>> >> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
>> >
>> > What is the motivation for using 'ip link' command to configure bridge
>> attributes?  IMHO,
>> > bridge command is better suited for that.
>>
>> Can you extend bridge command to allow setting/getting these bridge
>> attrs?  Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No
>> changes needed to the kernel.
>>
>> bridge link set dev br0 ageing_time 1000
>>
>>  --or--
>>
>> ip link set dev br0 type bridge ageing_time 1000
>
> I'd prefer to deprecate/remove all the 6 options on the 'ip link' command and move them to 'bridge' command.

We're probably stuck with the 'ip link' commands, since they're
already release in the wild and folks may have dependency on them.
However, when looking at iproute2 code, look for opportunity to use
same code for both paths.

-scott

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Premkumar Jonnala]

Hi Daniel,

Thank you for the comments.  Please see inline.

> -----Original Message-----
> From: Wilson, Daniel G [mailto:daniel.wilson@intel.com]
> Sent: Wednesday, August 19, 2015 11:33 PM
> To: Scott Feldman; Premkumar Jonnala
> Cc: netdev@vger.kernel.org
> Subject: RE: [PATCH] bridge: Enable configuration of ageing interval for bridges
> and switch devices.
> 
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org]
> > On Behalf Of Scott Feldman
> > Sent: Wednesday, August 19, 2015 12:54 PM
> > To: Premkumar Jonnala
> > Cc: netdev@vger.kernel.org
> > Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for
> bridges
> > and switch devices.
> >
> > On Wed, Aug 19, 2015 at 2:34 AM, Premkumar Jonnala
> > <pjonnala@broadcom.com> wrote:
> > > Hello Scott,
> > >
> > > Thank you for the diff and comments.   Please see my comments inline.
> > >
> > >> -----Original Message-----
> > >> From: Scott Feldman [mailto:sfeldma@gmail.com]
> > >> Sent: Tuesday, August 18, 2015 12:48 PM
> > >> To: Premkumar Jonnala
> > >> Cc: netdev@vger.kernel.org
> > >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval
> > >> for bridges and switch devices.
> > >>
> > >>
> > >>
> > >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
> > >>
> > >> > Bridge devices have ageing interval used to age out MAC addresses
> > >> > from FDB.  This ageing interval was not configuratble.
> > >> >
> > >> > Enable netlink based configuration of ageing interval for bridges
> > >> > and switch devices.  The ageing interval changes the timer used to
> > >> > purge inactive FDB entries in bridges.  The ageing interval config
> > >> > is propagated to switch devices, so that platform or hardware based
> > >> > ageing works according to configuration.
> > >> >
> > >> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
> > >>
> > >> Hi Premkumar,
> > >>
> > >> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
> > >
> > > What is the motivation for using 'ip link' command to configure bridge
> > > attributes?  IMHO, bridge command is better suited for that.
> >
> > Can you extend bridge command to allow setting/getting these bridge attrs?
> > Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No changes
> > needed to the kernel.
> >
> > bridge link set dev br0 ageing_time 1000
> >
> >  --or--
> >
> > ip link set dev br0 type bridge ageing_time 1000

IMHO, we should choose only one command.  Otherwise, we'd have to spend effort in trying to keep both the commands in sync.
My vote would be for the bridge command - since the options/parameters are related to bridges.  If there is no objection,
I'll move all the bridge options from 'ip link' command to 'bridge' command.

> 
> Being able to set these attributes via both bridge and ip would be great.
> 
> > >> +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,
> > >> +     };
> > >> +     int err;
> > >> +
> > >> +     err = switchdev_port_attr_set(br->dev, &attr);
> > >> +     if (err)
> > >> +             return err;
> > >> +
> > >> +     br->ageing_time = clock_t_to_jiffies(ageing_time);
> > >
> > > Should we restart the timer here the new time takes effect?
> >
> > I don't know...I just copied what the original code did.  If it does need to be
> > restarted, break that out as a separate patch.
> 
> In my opinion, yes, the timer should be restarted. If the timer had been set to 1
> million seconds and is being changed to 1 minute, you wouldn't want to wait for
> the 1-million-second  timer to expire before resetting it to the newly-configured
> 1-minute timer value.

I agree with you totally.  This is exactly the reason I want to restart the timer.  I'll
make the changes and post the patch.

-Prem

> 
> Dan.

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Premkumar Jonnala]

> -----Original Message-----
> From: Scott Feldman [mailto:sfeldma@gmail.com]
> Sent: Thursday, August 20, 2015 10:31 AM
> To: Premkumar Jonnala
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges
> and switch devices.
> 
> On Wed, Aug 19, 2015 at 9:56 PM, Premkumar Jonnala
> <pjonnala@broadcom.com> wrote:
> > Thank you Scott.  Please see inline.
> >
> >> >> -----Original Message-----
> >> >> From: Scott Feldman [mailto:sfeldma@gmail.com]
> >> >> Sent: Tuesday, August 18, 2015 12:48 PM
> >> >> To: Premkumar Jonnala
> >> >> Cc: netdev@vger.kernel.org
> >> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for
> >> bridges
> >> >> and switch devices.
> >> >>
> >> >>
> >> >>
> >> >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
> >> >>
> >> >> > Bridge devices have ageing interval used to age out MAC addresses
> >> >> > from FDB.  This ageing interval was not configuratble.
> >> >> >
> >> >> > Enable netlink based configuration of ageing interval for bridges and
> >> >> > switch devices.  The ageing interval changes the timer used to purge
> >> >> > inactive FDB entries in bridges.  The ageing interval config is
> >> >> > propagated to switch devices, so that platform or hardware based
> >> >> > ageing works according to configuration.
> >> >> >
> >> >> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
> >> >>
> >> >> Hi Premkumar,
> >> >>
> >> >> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
> >> >
> >> > What is the motivation for using 'ip link' command to configure bridge
> >> attributes?  IMHO,
> >> > bridge command is better suited for that.
> >>
> >> Can you extend bridge command to allow setting/getting these bridge
> >> attrs?  Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No
> >> changes needed to the kernel.
> >>
> >> bridge link set dev br0 ageing_time 1000
> >>
> >>  --or--
> >>
> >> ip link set dev br0 type bridge ageing_time 1000
> >
> > I'd prefer to deprecate/remove all the 6 options on the 'ip link' command and
> move them to 'bridge' command.
> 
> We're probably stuck with the 'ip link' commands, since they're
> already release in the wild and folks may have dependency on them.
> However, when looking at iproute2 code, look for opportunity to use
> same code for both paths.

Ok.  Then we can have both ip and bridge commands supporting these options, and freeze the 'ip link' command as it
exists today.  Any new options in future should be added to the bridge command.  Does that sound okay?

-Prem

> 
> -scott

Re: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Scott Feldman]

On Wed, Aug 19, 2015 at 10:12 PM, Premkumar Jonnala
<pjonnala@broadcom.com> wrote:
>
>
>> -----Original Message-----
>> From: Scott Feldman [mailto:sfeldma@gmail.com]
>> Sent: Thursday, August 20, 2015 10:31 AM
>> To: Premkumar Jonnala
>> Cc: netdev@vger.kernel.org
>> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges
>> and switch devices.
>>
>> On Wed, Aug 19, 2015 at 9:56 PM, Premkumar Jonnala
>> <pjonnala@broadcom.com> wrote:
>> > Thank you Scott.  Please see inline.
>> >
>> >> >> -----Original Message-----
>> >> >> From: Scott Feldman [mailto:sfeldma@gmail.com]
>> >> >> Sent: Tuesday, August 18, 2015 12:48 PM
>> >> >> To: Premkumar Jonnala
>> >> >> Cc: netdev@vger.kernel.org
>> >> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for
>> >> bridges
>> >> >> and switch devices.
>> >> >>
>> >> >>
>> >> >>
>> >> >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
>> >> >>
>> >> >> > Bridge devices have ageing interval used to age out MAC addresses
>> >> >> > from FDB.  This ageing interval was not configuratble.
>> >> >> >
>> >> >> > Enable netlink based configuration of ageing interval for bridges and
>> >> >> > switch devices.  The ageing interval changes the timer used to purge
>> >> >> > inactive FDB entries in bridges.  The ageing interval config is
>> >> >> > propagated to switch devices, so that platform or hardware based
>> >> >> > ageing works according to configuration.
>> >> >> >
>> >> >> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
>> >> >>
>> >> >> Hi Premkumar,
>> >> >>
>> >> >> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
>> >> >
>> >> > What is the motivation for using 'ip link' command to configure bridge
>> >> attributes?  IMHO,
>> >> > bridge command is better suited for that.
>> >>
>> >> Can you extend bridge command to allow setting/getting these bridge
>> >> attrs?  Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No
>> >> changes needed to the kernel.
>> >>
>> >> bridge link set dev br0 ageing_time 1000
>> >>
>> >>  --or--
>> >>
>> >> ip link set dev br0 type bridge ageing_time 1000
>> >
>> > I'd prefer to deprecate/remove all the 6 options on the 'ip link' command and
>> move them to 'bridge' command.
>>
>> We're probably stuck with the 'ip link' commands, since they're
>> already release in the wild and folks may have dependency on them.
>> However, when looking at iproute2 code, look for opportunity to use
>> same code for both paths.
>
> Ok.  Then we can have both ip and bridge commands supporting these options, and freeze the 'ip link' command as it
> exists today.  Any new options in future should be added to the bridge command.  Does that sound okay?

It would be ideal if both command paths use same code so new options
work with either command.  There are other examples where shared code
approach could be used to create synonymous commands:

ip link add name br0 type bridge       --or-- bridge add dev br0
ip link del br0                                  --or-- bridge del dev br0
ip link set dev sw1p1 master br0       --or-- bridge link add dev sw1p1 br0
ip link set dev sw1p1 nomaster br0    --or-- bridge link del sw1p1

There is some precedence of synonymous bridge commands:

ip link set dev sw1p1 type bridge_slave flood on
--or--
bridge link set dev sw1p1 flood on

But I don't know if the same code path is used for both of these.  (It should).

-scott

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Premkumar Jonnala]

> -----Original Message-----
> From: Scott Feldman [mailto:sfeldma@gmail.com]
> Sent: Thursday, August 20, 2015 11:09 AM
> To: Premkumar Jonnala
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges
> and switch devices.
> 
> On Wed, Aug 19, 2015 at 10:12 PM, Premkumar Jonnala
> <pjonnala@broadcom.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Scott Feldman [mailto:sfeldma@gmail.com]
> >> Sent: Thursday, August 20, 2015 10:31 AM
> >> To: Premkumar Jonnala
> >> Cc: netdev@vger.kernel.org
> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for
> bridges
> >> and switch devices.
> >>
> >> On Wed, Aug 19, 2015 at 9:56 PM, Premkumar Jonnala
> >> <pjonnala@broadcom.com> wrote:
> >> > Thank you Scott.  Please see inline.
> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: Scott Feldman [mailto:sfeldma@gmail.com]
> >> >> >> Sent: Tuesday, August 18, 2015 12:48 PM
> >> >> >> To: Premkumar Jonnala
> >> >> >> Cc: netdev@vger.kernel.org
> >> >> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for
> >> >> bridges
> >> >> >> and switch devices.
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
> >> >> >>
> >> >> >> > Bridge devices have ageing interval used to age out MAC addresses
> >> >> >> > from FDB.  This ageing interval was not configuratble.
> >> >> >> >
> >> >> >> > Enable netlink based configuration of ageing interval for bridges and
> >> >> >> > switch devices.  The ageing interval changes the timer used to purge
> >> >> >> > inactive FDB entries in bridges.  The ageing interval config is
> >> >> >> > propagated to switch devices, so that platform or hardware based
> >> >> >> > ageing works according to configuration.
> >> >> >> >
> >> >> >> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
> >> >> >>
> >> >> >> Hi Premkumar,
> >> >> >>
> >> >> >> I agree with Roopa that we should use existing
> IFLA_BR_AGEING_TIME.
> >> >> >
> >> >> > What is the motivation for using 'ip link' command to configure bridge
> >> >> attributes?  IMHO,
> >> >> > bridge command is better suited for that.
> >> >>
> >> >> Can you extend bridge command to allow setting/getting these bridge
> >> >> attrs?  Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.
> No
> >> >> changes needed to the kernel.
> >> >>
> >> >> bridge link set dev br0 ageing_time 1000
> >> >>
> >> >>  --or--
> >> >>
> >> >> ip link set dev br0 type bridge ageing_time 1000
> >> >
> >> > I'd prefer to deprecate/remove all the 6 options on the 'ip link' command
> and
> >> move them to 'bridge' command.
> >>
> >> We're probably stuck with the 'ip link' commands, since they're
> >> already release in the wild and folks may have dependency on them.
> >> However, when looking at iproute2 code, look for opportunity to use
> >> same code for both paths.
> >
> > Ok.  Then we can have both ip and bridge commands supporting these options,
> and freeze the 'ip link' command as it
> > exists today.  Any new options in future should be added to the bridge
> command.  Does that sound okay?
> 
> It would be ideal if both command paths use same code so new options
> work with either command.  There are other examples where shared code
> approach could be used to create synonymous commands:
> 
> ip link add name br0 type bridge       --or-- bridge add dev br0
> ip link del br0                                  --or-- bridge del dev br0
> ip link set dev sw1p1 master br0       --or-- bridge link add dev sw1p1 br0
> ip link set dev sw1p1 nomaster br0    --or-- bridge link del sw1p1
> 
> There is some precedence of synonymous bridge commands:
> 
> ip link set dev sw1p1 type bridge_slave flood on
> --or--
> bridge link set dev sw1p1 flood on
> 
> But I don't know if the same code path is used for both of these.  (It should).

Sounds reasonable.  I'll try to follow this approach - with code sharing requirement in mind.

-Prem
> 
> -scott

Re: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Michal Kubecek]

On Thu, Aug 20, 2015 at 05:08:51AM +0000, Premkumar Jonnala wrote:
> > From: Wilson, Daniel G [mailto:daniel.wilson@intel.com]
> > >
> > > Can you extend bridge command to allow setting/getting these bridge attrs?
> > > Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No changes
> > > needed to the kernel.
> > >
> > > bridge link set dev br0 ageing_time 1000
> > >
> > >  --or--
> > >
> > > ip link set dev br0 type bridge ageing_time 1000
> > 
> > Being able to set these attributes via both bridge and ip would be great.
> > 
> IMHO, we should choose only one command.  Otherwise, we'd have to
> spend effort in trying to keep both the commands in sync.

As long as they are using the same netlink interface, I don't think it's
a serious problem. After all, there will be also other tools (wicked,
perhaps systemd-networkd) setting it directly via netlink rather than
calling either ip or bridge.

> My vote would be for the bridge command - since the options/parameters
> are related to bridges.  If there is no objection, I'll move all the
> bridge options from 'ip link' command to 'bridge' command.

This would break existing scripts using ip to set the parameter. Is the
possibility to use any of the two really that bad?

                                                         Michal Kubecek

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Premkumar Jonnala]

> -----Original Message-----
> From: Michal Kubecek [mailto:mkubecek@suse.cz]
> Sent: Thursday, August 20, 2015 12:00 PM
> To: Premkumar Jonnala
> Cc: Wilson, Daniel G; Scott Feldman; netdev@vger.kernel.org
> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges
> and switch devices.
> 
> On Thu, Aug 20, 2015 at 05:08:51AM +0000, Premkumar Jonnala wrote:
> > > From: Wilson, Daniel G [mailto:daniel.wilson@intel.com]
> > > >
> > > > Can you extend bridge command to allow setting/getting these bridge
> attrs?
> > > > Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No
> changes
> > > > needed to the kernel.
> > > >
> > > > bridge link set dev br0 ageing_time 1000
> > > >
> > > >  --or--
> > > >
> > > > ip link set dev br0 type bridge ageing_time 1000
> > >
> > > Being able to set these attributes via both bridge and ip would be great.
> > >
> > IMHO, we should choose only one command.  Otherwise, we'd have to
> > spend effort in trying to keep both the commands in sync.
> 
> As long as they are using the same netlink interface, I don't think it's
> a serious problem. After all, there will be also other tools (wicked,
> perhaps systemd-networkd) setting it directly via netlink rather than
> calling either ip or bridge.
> 
> > My vote would be for the bridge command - since the options/parameters
> > are related to bridges.  If there is no objection, I'll move all the
> > bridge options from 'ip link' command to 'bridge' command.
> 
> This would break existing scripts using ip to set the parameter. Is the
> possibility to use any of the two really that bad?

There was another email on this thread where Scott indicated existence of other commands
where both ip and bridge are available, and they are for the same function.

I will keep both the ip and bridge commands, and try to share the underlying code as much as possible.

-Prem

> 
>                                                          Michal Kubecek

Re: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Michal Kubecek]

On Thu, Aug 20, 2015 at 06:40:01AM +0000, Premkumar Jonnala wrote:
> > From: Michal Kubecek [mailto:mkubecek@suse.cz]
> > 
> > This would break existing scripts using ip to set the parameter. Is the
> > possibility to use any of the two really that bad?
> 
> There was another email on this thread where Scott indicated existence
> of other commands where both ip and bridge are available, and they are
> for the same function.

Yes, I already noticed it. I'm sorry, I should have checked the whole
thread before replying.

                                                       Michal Kubecek