[PATCH 0/4] bridge patches

[PATCH 0/4] bridge patches

[Stephen Hemminger]

Several patches to bridge. #1 is a nit, #2 is a bug fix,
#3 and #4 are manageability enhancements (they can wait for
2.6.22 if needed).

--
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 1/4] bridge: get rid of miscdevice include

[Stephen Hemminger]

[-- Attachment #1: bridge-get-rid-miscdevice --]
[-- Type: text/plain, Size: 557 bytes --]

The bridge hasn't used miscdevice for a long long time.

Signed-off-by: StephenHemminger <shemminger@linux-foundtion.org>

---
 net/bridge/br_private.h |    1 -
 1 file changed, 1 deletion(-)

--- bridge.orig/net/bridge/br_private.h	2007-02-21 10:57:26.000000000 -0800
+++ bridge/net/bridge/br_private.h	2007-02-21 10:57:30.000000000 -0800
@@ -16,7 +16,6 @@
 #define _BR_PRIVATE_H
 
 #include <linux/netdevice.h>
-#include <linux/miscdevice.h>
 #include <linux/if_bridge.h>
 
 #define BR_HASH_BITS 8

--
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 2/4] bridge: eliminate workqueue for carrier check

[Stephen Hemminger]

[-- Attachment #1: bridge-port-carrier.patch --]
[-- Type: text/plain, Size: 4593 bytes --]

Having a work queue for checking carrier leads to lots of race issues.
Simpler to just get the cost when data structure is created and
update on change.

Signed-off-by: StephenHemminger <shemminger@linux-foundtion.org>

---
 net/bridge/br_if.c      |   30 +++++-------------------------
 net/bridge/br_notify.c  |   25 +++++++++++--------------
 net/bridge/br_private.h |    4 +---
 3 files changed, 17 insertions(+), 42 deletions(-)

--- bridge.orig/net/bridge/br_if.c	2007-02-21 10:57:06.000000000 -0800
+++ bridge/net/bridge/br_if.c	2007-02-21 14:28:51.000000000 -0800
@@ -77,26 +77,15 @@
  * Called from work queue to allow for calling functions that
  * might sleep (such as speed check), and to debounce.
  */
-static void port_carrier_check(struct work_struct *work)
+void br_port_carrier_check(struct net_bridge_port *p)
 {
-	struct net_bridge_port *p;
-	struct net_device *dev;
-	struct net_bridge *br;
-
-	dev = container_of(work, struct net_bridge_port,
-			   carrier_check.work)->dev;
-	work_release(work);
-
-	rtnl_lock();
-	p = dev->br_port;
-	if (!p)
-		goto done;
-	br = p->br;
+	struct net_device *dev = p->dev;
+	struct net_bridge *br = p->br;
 
 	if (netif_carrier_ok(dev))
 		p->path_cost = port_cost(dev);
 
-	if (br->dev->flags & IFF_UP) {
+	if (netif_running(br->dev)) {
 		spin_lock_bh(&br->lock);
 		if (netif_carrier_ok(dev)) {
 			if (p->state == BR_STATE_DISABLED)
@@ -107,9 +96,6 @@
 		}
 		spin_unlock_bh(&br->lock);
 	}
-done:
-	dev_put(dev);
-	rtnl_unlock();
 }
 
 static void release_nbp(struct kobject *kobj)
@@ -162,9 +148,6 @@
 
 	dev_set_promiscuity(dev, -1);
 
-	if (cancel_delayed_work(&p->carrier_check))
-		dev_put(dev);
-
 	spin_lock_bh(&br->lock);
 	br_stp_disable_port(p);
 	spin_unlock_bh(&br->lock);
@@ -282,7 +265,6 @@
 	p->port_no = index;
 	br_init_port(p);
 	p->state = BR_STATE_DISABLED;
-	INIT_DELAYED_WORK_NAR(&p->carrier_check, port_carrier_check);
 	br_stp_port_timer_init(p);
 
 	kobject_init(&p->kobj);
@@ -446,12 +428,10 @@
 	spin_lock_bh(&br->lock);
 	br_stp_recalculate_bridge_id(br);
 	br_features_recompute(br);
-	if (schedule_delayed_work(&p->carrier_check, BR_PORT_DEBOUNCE))
-		dev_hold(dev);
-
 	spin_unlock_bh(&br->lock);
 
 	dev_set_mtu(br->dev, br_min_mtu(br));
+
 	kobject_uevent(&p->kobj, KOBJ_ADD);
 
 	return 0;
--- bridge.orig/net/bridge/br_notify.c	2007-02-21 10:57:06.000000000 -0800
+++ bridge/net/bridge/br_notify.c	2007-02-21 10:57:37.000000000 -0800
@@ -42,51 +42,48 @@
 
 	br = p->br;
 
-	spin_lock_bh(&br->lock);
 	switch (event) {
 	case NETDEV_CHANGEMTU:
 		dev_set_mtu(br->dev, br_min_mtu(br));
 		break;
 
 	case NETDEV_CHANGEADDR:
+		spin_lock_bh(&br->lock);
 		br_fdb_changeaddr(p, dev->dev_addr);
 		br_ifinfo_notify(RTM_NEWLINK, p);
 		br_stp_recalculate_bridge_id(br);
+		spin_unlock_bh(&br->lock);
 		break;
 
 	case NETDEV_CHANGE:
-		if (br->dev->flags & IFF_UP)
-			if (schedule_delayed_work(&p->carrier_check,
-						BR_PORT_DEBOUNCE))
-				dev_hold(dev);
+		br_port_carrier_check(p);
 		break;
 
 	case NETDEV_FEAT_CHANGE:
-		if (br->dev->flags & IFF_UP)
+		spin_lock_bh(&br->lock);
+		if (netif_running(br->dev))
 			br_features_recompute(br);
-
-		/* could do recursive feature change notification
-		 * but who would care??
-		 */
+		spin_unlock_bh(&br->lock);
 		break;
 
 	case NETDEV_DOWN:
+		spin_lock_bh(&br->lock);
 		if (br->dev->flags & IFF_UP)
 			br_stp_disable_port(p);
+		spin_unlock_bh(&br->lock);
 		break;
 
 	case NETDEV_UP:
+		spin_lock_bh(&br->lock);
 		if (netif_carrier_ok(dev) && (br->dev->flags & IFF_UP))
 			br_stp_enable_port(p);
+		spin_unlock_bh(&br->lock);
 		break;
 
 	case NETDEV_UNREGISTER:
-		spin_unlock_bh(&br->lock);
 		br_del_if(br, dev);
-		goto done;
+		break;
 	}
-	spin_unlock_bh(&br->lock);
 
- done:
 	return NOTIFY_DONE;
 }
--- bridge.orig/net/bridge/br_private.h	2007-02-21 10:57:30.000000000 -0800
+++ bridge/net/bridge/br_private.h	2007-02-21 14:29:09.000000000 -0800
@@ -26,8 +26,6 @@
 #define BR_PORT_BITS	10
 #define BR_MAX_PORTS	(1<<BR_PORT_BITS)
 
-#define BR_PORT_DEBOUNCE (HZ/10)
-
 #define BR_VERSION	"2.2"
 
 typedef struct bridge_id bridge_id;
@@ -81,7 +79,6 @@
 	struct timer_list		hold_timer;
 	struct timer_list		message_age_timer;
 	struct kobject			kobj;
-	struct delayed_work		carrier_check;
 	struct rcu_head			rcu;
 };
 
@@ -172,6 +169,7 @@
 		      int clone);
 
 /* br_if.c */
+extern void br_port_carrier_check(struct net_bridge_port *p);
 extern int br_add_bridge(const char *name);
 extern int br_del_bridge(const char *name);
 extern void br_cleanup_bridges(void);

--
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 3/4] bridge: make path cost setting persistent

[Stephen Hemminger]

[-- Attachment #1: bridge-admin-path-cost.patch --]
[-- Type: text/plain, Size: 3845 bytes --]

This is to keep an STP port path cost which was set by a user from
replaced by the link-speed based path cost whenever the link goes down
and comes back up. 
An admin_cost field is added to struct net_bridge_port to
indicate whether there is a user specified path cost.

Signed-Off-By: Srinivas Aji <Aji_Srinivas@emc.com>
Signed-off-by: StephenHemminger <shemminger@linux-foundtion.org>

---
 net/bridge/br_if.c       |    8 ++++----
 net/bridge/br_private.h  |    2 ++
 net/bridge/br_stp.c      |    1 -
 net/bridge/br_stp_if.c   |   13 ++++++++++---
 net/bridge/br_sysfs_if.c |   10 +++++++---
 5 files changed, 23 insertions(+), 11 deletions(-)

--- bridge.orig/net/bridge/br_if.c	2007-02-21 14:28:51.000000000 -0800
+++ bridge/net/bridge/br_if.c	2007-02-21 14:29:25.000000000 -0800
@@ -33,7 +33,7 @@
  * ethtool, use ethtool_ops.  Also, since driver might sleep need to
  * not be holding any locks.
  */
-static int port_cost(struct net_device *dev)
+int br_port_cost(struct net_device *dev)
 {
 	struct ethtool_cmd ecmd = { ETHTOOL_GSET };
 	struct ifreq ifr;
@@ -82,8 +82,8 @@
 	struct net_device *dev = p->dev;
 	struct net_bridge *br = p->br;
 
-	if (netif_carrier_ok(dev))
-		p->path_cost = port_cost(dev);
+	if (netif_carrier_ok(dev) && p->admin_cost == 0)
+		p->path_cost = br_port_cost(dev);
 
 	if (netif_running(br->dev)) {
 		spin_lock_bh(&br->lock);
@@ -260,7 +260,7 @@
 	p->br = br;
 	dev_hold(dev);
 	p->dev = dev;
-	p->path_cost = port_cost(dev);
+	p->path_cost = br_port_cost(dev);
 	p->priority = 0x8000 >> BR_PORT_BITS;
 	p->port_no = index;
 	br_init_port(p);
--- bridge.orig/net/bridge/br_private.h	2007-02-21 14:29:09.000000000 -0800
+++ bridge/net/bridge/br_private.h	2007-02-21 14:29:25.000000000 -0800
@@ -73,6 +73,7 @@
 	bridge_id			designated_root;
 	bridge_id			designated_bridge;
 	u32				path_cost;
+	u32				admin_cost;
 	u32				designated_cost;
 
 	struct timer_list		forward_delay_timer;
@@ -169,6 +170,7 @@
 		      int clone);
 
 /* br_if.c */
+extern int br_port_cost(struct net_device *dev);
 extern void br_port_carrier_check(struct net_bridge_port *p);
 extern int br_add_bridge(const char *name);
 extern int br_del_bridge(const char *name);
--- bridge.orig/net/bridge/br_stp.c	2007-02-21 14:28:51.000000000 -0800
+++ bridge/net/bridge/br_stp.c	2007-02-21 14:29:25.000000000 -0800
@@ -39,7 +39,6 @@
 
 }
 
-/* called under bridge lock */
 struct net_bridge_port *br_get_port(struct net_bridge *br, u16 port_no)
 {
 	struct net_bridge_port *p;
--- bridge.orig/net/bridge/br_stp_if.c	2007-02-21 14:28:51.000000000 -0800
+++ bridge/net/bridge/br_stp_if.c	2007-02-21 14:29:25.000000000 -0800
@@ -212,12 +212,19 @@
 	}
 }
 
-/* called under bridge lock */
-void br_stp_set_path_cost(struct net_bridge_port *p, u32 path_cost)
+void br_stp_set_path_cost(struct net_bridge_port *p, u32 cost)
 {
-	p->path_cost = path_cost;
+	struct net_bridge *br = p->br;
+
+	ASSERT_RTNL();
+
+	p->admin_cost = cost;
+	p->path_cost = cost ? : br_port_cost(p->dev);
+
+	spin_lock_bh(&br->lock);
 	br_configuration_update(p->br);
 	br_port_state_selection(p->br);
+	spin_unlock_bh(&br->lock);
 }
 
 ssize_t br_show_bridge_id(char *buf, const struct bridge_id *id)
--- bridge.orig/net/bridge/br_sysfs_if.c	2007-02-21 14:28:51.000000000 -0800
+++ bridge/net/bridge/br_sysfs_if.c	2007-02-21 14:29:25.000000000 -0800
@@ -184,9 +184,13 @@
 	if (endp != buf) {
 		rtnl_lock();
 		if (p->dev && p->br && brport_attr->store) {
-			spin_lock_bh(&p->br->lock);
-			ret = brport_attr->store(p, val);
-			spin_unlock_bh(&p->br->lock);
+			if (brport_attr->store == store_path_cost)
+				ret = store_path_cost(p, val);
+			else {
+				spin_lock_bh(&p->br->lock);
+				ret = brport_attr->store(p, val);
+				spin_unlock_bh(&p->br->lock);
+			}
 			if (ret == 0)
 				ret = count;
 		}

--
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 4/4] bridge: add flush attribute

[Stephen Hemminger]

[-- Attachment #1: bridge-fdb-flush.patch --]
[-- Type: text/plain, Size: 2903 bytes --]

Add a flush attribute to sysfs to allow flushing forwarding table.
This can be used by user level spanning tree protocol to clear state.

---
 net/bridge/br_fdb.c      |   11 ++++++++---
 net/bridge/br_sysfs_br.c |   14 ++++++++++++++
 net/bridge/br_sysfs_if.c |    8 ++++++++
 3 files changed, 30 insertions(+), 3 deletions(-)

--- bridge.orig/net/bridge/br_sysfs_br.c	2007-02-21 14:28:49.000000000 -0800
+++ bridge/net/bridge/br_sysfs_br.c	2007-02-21 14:39:50.000000000 -0800
@@ -310,6 +310,19 @@
 		   show_group_addr, store_group_addr);
 
 
+static ssize_t store_flush(struct device *d, struct device_attribute *attr,
+			   const char *buf, size_t len)
+{
+	struct net_bridge *br = to_bridge(d);
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	br_fdb_delete_by_port(br, NULL, 0);
+	return len;
+}
+static DEVICE_ATTR(flush, S_IWUSR, NULL, store_flush);
+
 static struct attribute *bridge_attrs[] = {
 	&dev_attr_forward_delay.attr,
 	&dev_attr_hello_time.attr,
@@ -328,6 +341,7 @@
 	&dev_attr_topology_change_timer.attr,
 	&dev_attr_gc_timer.attr,
 	&dev_attr_group_addr.attr,
+	&dev_attr_flush.attr,
 	NULL
 };
 
--- bridge.orig/net/bridge/br_sysfs_if.c	2007-02-21 14:29:25.000000000 -0800
+++ bridge/net/bridge/br_sysfs_if.c	2007-02-21 14:29:27.000000000 -0800
@@ -137,6 +137,13 @@
 }
 static BRPORT_ATTR(hold_timer, S_IRUGO, show_hold_timer, NULL);
 
+static ssize_t store_flush(struct net_bridge_port *p, unsigned long v)
+{
+	br_fdb_delete_by_port(p->br, p, 0); // Don't delete local entry
+	return 0;
+}
+static BRPORT_ATTR(flush, S_IWUSR, NULL, store_flush);
+
 static struct brport_attribute *brport_attrs[] = {
 	&brport_attr_path_cost,
 	&brport_attr_priority,
@@ -152,6 +159,7 @@
 	&brport_attr_message_age_timer,
 	&brport_attr_forward_delay_timer,
 	&brport_attr_hold_timer,
+	&brport_attr_flush,
 	NULL
 };
 
--- bridge.orig/net/bridge/br_fdb.c	2007-02-21 14:28:49.000000000 -0800
+++ bridge/net/bridge/br_fdb.c	2007-02-21 14:29:27.000000000 -0800
@@ -128,7 +128,11 @@
 	mod_timer(&br->gc_timer, jiffies + HZ/10);
 }
 
-
+/*
+ * Flush all forwarding entries for a port.
+ * if p is NULL, it means flush for all ports.
+ * if do_all is non-zero, then flush static entries as well
+ */
 void br_fdb_delete_by_port(struct net_bridge *br,
 			   const struct net_bridge_port *p,
 			   int do_all)
@@ -142,7 +146,8 @@
 		hlist_for_each_safe(h, g, &br->hash[i]) {
 			struct net_bridge_fdb_entry *f
 				= hlist_entry(h, struct net_bridge_fdb_entry, hlist);
-			if (f->dst != p)
+
+			if (p && f->dst != p)
 				continue;
 
 			if (f->is_static && !do_all)
@@ -152,7 +157,7 @@
 			 * then when one port is deleted, assign
 			 * the local entry to other port
 			 */
-			if (f->is_local) {
+			if (p && f->is_local) {
 				struct net_bridge_port *op;
 				list_for_each_entry(op, &br->port_list, list) {
 					if (op != p &&

--
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH 1/4] bridge: get rid of miscdevice include

[Marc.Obbad]

How to get those fixes for testing ?

Thanks,
-Marc.


                                                                                                                        
             Stephen Hemminger                                                                                          
             <shemminger@linux-foundation.org>                                                                          
             Sent by:                                                                                                To 
             bridge-bounces@lists.osdl.org               David Miller <davem@davemloft.net>                             
                                                                                                                     cc 
                                                         netdev@vger.kernel.org, bridge@linux-foundation.org            
             02/21/2007 05:41 PM                                                                                Subject 
                                                         [Bridge] [PATCH 1/4] bridge: get rid of miscdevice include     
                                                                                                                        
                                                                                                                        
                                                                                                                        
                                                                                                                        
                                                                                                                        
                                                                                                                        




The bridge hasn't used miscdevice for a long long time.

Signed-off-by: StephenHemminger <shemminger@linux-foundtion.org>

---
 net/bridge/br_private.h |    1 -
 1 file changed, 1 deletion(-)

--- bridge.orig/net/bridge/br_private.h          2007-02-21 10:57:26.000000000 -0800
+++ bridge/net/bridge/br_private.h         2007-02-21 10:57:30.000000000 -0800
@@ -16,7 +16,6 @@
 #define _BR_PRIVATE_H

 #include <linux/netdevice.h>
-#include <linux/miscdevice.h>
 #include <linux/if_bridge.h>

 #define BR_HASH_BITS 8

--
Stephen Hemminger <shemminger@linux-foundation.org>

_______________________________________________
Bridge mailing list
Bridge@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/bridge

Re: [PATCH 1/4] bridge: get rid of miscdevice include

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Wed, 21 Feb 2007 14:41:08 -0800

> The bridge hasn't used miscdevice for a long long time.

Applied, thanks Stephen.

> Signed-off-by: StephenHemminger <shemminger@linux-foundtion.org>
                      ^^^^                              ^^^^

You don't type these in by hand do you? :-)

Time to put your signoff into a text file and use the insert-from-file
feature of your favorite editor or similar :-)

Re: [PATCH 2/4] bridge: eliminate workqueue for carrier check

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Wed, 21 Feb 2007 14:41:09 -0800

> Having a work queue for checking carrier leads to lots of race issues.
> Simpler to just get the cost when data structure is created and
> update on change.
> 
> Signed-off-by: StephenHemminger <shemminger@linux-foundtion.org>

Applied, I guess the signoff typo is in your file :-)

Re: [PATCH 0/4] bridge patches

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Wed, 21 Feb 2007 14:41:07 -0800

> Several patches to bridge. #1 is a nit, #2 is a bug fix,
> #3 and #4 are manageability enhancements (they can wait for
> 2.6.22 if needed).

I've added #1 and #2, #3 and #4 are for 2.6.22

[PATCH 3+/4] bridge: fix locking of set path cost

[Stephen Hemminger]

This change goes with earlier change to get rid of 
work queue for path cost. Now stp_set_path_cost does its own
locking. This is to allow it to call br_path_cost() which calls
ethtool interfaces (might sleep).


Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
---
 net/bridge/br_ioctl.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- bridge.orig/net/bridge/br_ioctl.c	2007-02-27 09:46:15.000000000 -0800
+++ bridge/net/bridge/br_ioctl.c	2007-02-27 09:43:24.000000000 -0800
@@ -291,12 +291,11 @@
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
 
-		spin_lock_bh(&br->lock);
 		if ((p = br_get_port(br, args[1])) == NULL)
 			ret = -EINVAL;
 		else
 			br_stp_set_path_cost(p, args[2]);
-		spin_unlock_bh(&br->lock);
+
 		return ret;
 	}
 

Re: [PATCH 3+/4] bridge: fix locking of set path cost

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Tue, 27 Feb 2007 09:52:51 -0800

> This change goes with earlier change to get rid of 
> work queue for path cost. Now stp_set_path_cost does its own
> locking. This is to allow it to call br_path_cost() which calls
> ethtool interfaces (might sleep).
> 
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

Applied, thanks Stephen.