Issue NETDEV_CHANGE notification when bridge changes state

Why not set forwarding delay to zero? I don't think you are using STP?

P.s: removing linux-kernel mailing list, since there is no point in copying the whole world on this thread.

Quoted 4 lines · Stephen Hemminger

Why not set forwarding delay to zero? I don't think you are using STP?

P.s: removing linux-kernel mailing list, since there is no point in copying the whole world on this thread.

I just sent patches where MAINTAINERS file told me to send it. ;)

No, I'm not relying on STP. I think setting learning->forwarding delay to 0 could cause problems with STP like loops until the loop is detected.

There may be a better spot where to insert the notification that the bridge is forwarding data, though I'm not exactly certain where. The patch will cause bridge to issue NETDEV_CHANGE notification for all topology changes. This may or may not be useful but shouldn't be harmful (I can't imagine these occur very often?) The IPv6 autoconf patch will only act on this if it doesn't have IPv6 address configured manually or via autoconf.

Since this a generic problem, it needs a better solution. Sending NETDEV_CHANGE impacts lots of other pieces, and even user space has similar problems.

Quoted 3 lines

Since this a generic problem, it needs a better solution. Sending NETDEV_CHANGE impacts lots of other pieces, and even user space has similar problems.

It does seem a little broad notification type. I've checked over all the currently defined NETDEV notifiers, and it seems that NETDEV_NOTIFY_PEERS may be a better option to use when bridge has a potential topology change.

Currently it is only used in ipv4/devinet.c: where it is used to issue a gratuitous ARP.

Quoted 11 lines · Stephen Hemminger

Quoted 3 lines

Since this a generic problem, it needs a better solution. Sending NETDEV_CHANGE impacts lots of other pieces, and even user space has similar problems.

It does seem a little broad notification type. I've checked over all the currently defined NETDEV notifiers, and it seems that NETDEV_NOTIFY_PEERS may be a better option to use when bridge has a potential topology change.

Quoted 2 lines

Currently it is only used in ipv4/devinet.c: where it is used to issue a gratuitous ARP.

I was thinking of fixing bridge to not actually bring the link up until in forwarding mode. Other applications (DHCP, etc) see the link up and really don't like being in half duplex during that period.

This makes the bridge device behave like a physical device. In earlier releases the bridge always asserted carrier. This changes the behavior so that bridge device carrier is on only if one or more ports are in the forwarding state. This should help IPv6 autoconfiguration, DHCP, and routing daemons.

I did brief testing with Network and Virt manager and they seem fine, but since this changes behavior of bridge, it should wait until net-next (2.6.39).

Signed-off-by Stephen Hemminger<[email protected]>

---
  net/bridge/br_device.c    |    4 ++++
  net/bridge/br_stp.c       |   35 ++++++++++++++++++++++-------------
  net/bridge/br_stp_timer.c |    1 +
  3 files changed, 27 insertions(+), 13 deletions(-)

net/bridge/br_device.c +4 -0

@@ -78,6 +78,8 @@ static int br_dev_open(struct net_device
  {
  	struct net_bridge *br = netdev_priv(dev);
 
+	netif_carrier_off(dev);
+
  	br_features_recompute(br);
  	netif_start_queue(dev);
  	br_stp_enable_bridge(br);

@@ -94,6 +96,8 @@ static int br_dev_stop(struct net_device
  {
  	struct net_bridge *br = netdev_priv(dev);
 
+	netif_carrier_off(dev);
+
  	br_stp_disable_bridge(br);
  	br_multicast_stop(br);
 

net/bridge/br_stp.c +5 -13

@@ -397,28 +397,37 @@ static void br_make_forwarding(struct ne
  void br_port_state_selection(struct net_bridge *br)
  {
  	struct net_bridge_port *p;
+	unsigned int liveports = 0;
 
  	/* Don't change port states if userspace is handling STP */
  	if (br->stp_enabled == BR_USER_STP)
  		return;
 
  	list_for_each_entry(p,&br->port_list, list) {
-		if (p->state != BR_STATE_DISABLED) {
-			if (p->port_no == br->root_port) {
-				p->config_pending = 0;
-				p->topology_change_ack = 0;
-				br_make_forwarding(p);
-			} else if (br_is_designated_port(p)) {
-				del_timer(&p->message_age_timer);
-				br_make_forwarding(p);
-			} else {
-				p->config_pending = 0;
-				p->topology_change_ack = 0;
-				br_make_blocking(p);
-			}
+		if (p->state == BR_STATE_DISABLED)
+			continue;
+
+		if (p->port_no == br->root_port) {

net/bridge/br_stp.c +7 -0

  		}
 
+		if (p->state == BR_STATE_FORWARDING)
+			++liveports;
  	}
+
+	if (liveports == 0)
+		netif_carrier_off(br->dev);
+	else
+		netif_carrier_on(br->dev);
  }
 
  /* called under bridge lock */

net/bridge/br_stp_timer.c +1 -0

@@ -94,6 +94,7 @@ static void br_forward_delay_timer_expir
  		p->state = BR_STATE_FORWARDING;
  		if (br_is_designated_for_some_port(br))
  			br_topology_change_detection(br);
+		netif_carrier_on(br->dev);
  	}
  	br_log_state(p);
  	spin_unlock(&br->lock);

Quoted 68 lines · Stephen Hemminger

This makes the bridge device behave like a physical device. In earlier releases the bridge always asserted carrier. This changes the behavior so that bridge device carrier is on only if one or more ports are in the forwarding state. This should help IPv6 autoconfiguration, DHCP, and routing daemons.

I did brief testing with Network and Virt manager and they seem fine, but since this changes behavior of bridge, it should wait until net-next (2.6.39).

Signed-off-by Stephen Hemminger<[email protected]>

---
  net/bridge/br_device.c    |    4 ++++
  net/bridge/br_stp.c       |   35 ++++++++++++++++++++++-------------
  net/bridge/br_stp_timer.c |    1 +
  3 files changed, 27 insertions(+), 13 deletions(-)

net/bridge/br_device.c +4 -0

@@ -78,6 +78,8 @@ static int br_dev_open(struct net_device
  {
  	struct net_bridge *br = netdev_priv(dev);
 
+	netif_carrier_off(dev);
+
  	br_features_recompute(br);
  	netif_start_queue(dev);
  	br_stp_enable_bridge(br);

@@ -94,6 +96,8 @@ static int br_dev_stop(struct net_device
  {
  	struct net_bridge *br = netdev_priv(dev);
 
+	netif_carrier_off(dev);
+
  	br_stp_disable_bridge(br);
  	br_multicast_stop(br);
 

net/bridge/br_stp.c +5 -13

@@ -397,28 +397,37 @@ static void br_make_forwarding(struct ne
  void br_port_state_selection(struct net_bridge *br)
  {
  	struct net_bridge_port *p;
+	unsigned int liveports = 0;
 
  	/* Don't change port states if userspace is handling STP */
  	if (br->stp_enabled == BR_USER_STP)
  		return;
 
  	list_for_each_entry(p,&br->port_list, list) {
-		if (p->state != BR_STATE_DISABLED) {
-			if (p->port_no == br->root_port) {
-				p->config_pending = 0;
-				p->topology_change_ack = 0;
-				br_make_forwarding(p);
-			} else if (br_is_designated_port(p)) {
-				del_timer(&p->message_age_timer);
-				br_make_forwarding(p);
-			} else {
-				p->config_pending = 0;
-				p->topology_change_ack = 0;
-				br_make_blocking(p);
-			}
+		if (p->state == BR_STATE_DISABLED)
+			continue;
+
+		if (p->port_no == br->root_port) {

net/bridge/br_stp.c static void br_make_forwarding(struct ne · line 420

Quoted 10 lines

net/bridge/br_stp.c +10 -0

+			p->config_pending = 0;
+			p->topology_change_ack = 0;
+			br_make_forwarding(p);
+		} else if (br_is_designated_port(p)) {
+			del_timer(&p->message_age_timer);
+			br_make_forwarding(p);
+		} else {
+			p->config_pending = 0;
+			p->topology_change_ack = 0;
+			br_make_blocking(p);

Is the above part really related to the purpose of this patch? It looks like (good) cleanup, but should be in a different patch.

Except from this comment,

Reviewed-by Nicolas de Pesloüan <[email protected]>

Quoted 1 line · On Mon, 07 Mar 2011 21:48:16 +0100 Nicolas de…

Le 07/03/2011 19:34, Stephen Hemminger a écrit :

Quoted 34 lines

Quoted 18 lines

   	list_for_each_entry(p,&br->port_list, list) {

net/bridge/br_stp.c +4 -13

-		if (p->state != BR_STATE_DISABLED) {
-			if (p->port_no == br->root_port) {
-				p->config_pending = 0;
-				p->topology_change_ack = 0;
-				br_make_forwarding(p);
-			} else if (br_is_designated_port(p)) {
-				del_timer(&p->message_age_timer);
-				br_make_forwarding(p);
-			} else {
-				p->config_pending = 0;
-				p->topology_change_ack = 0;
-				br_make_blocking(p);
-			}
+		if (p->state == BR_STATE_DISABLED)
+			continue;
+
+		if (p->port_no == br->root_port) {

net/bridge/br_stp.c static void br_make_forwarding(struct ne · line 420

Quoted 10 lines

net/bridge/br_stp.c +10 -0

+			p->config_pending = 0;
+			p->topology_change_ack = 0;
+			br_make_forwarding(p);
+		} else if (br_is_designated_port(p)) {
+			del_timer(&p->message_age_timer);
+			br_make_forwarding(p);
+		} else {
+			p->config_pending = 0;
+			p->topology_change_ack = 0;
+			br_make_blocking(p);

Is the above part really related to the purpose of this patch? It looks like (good) cleanup, but should be in a different patch.

Except from this comment,

Quoted 1 line

Reviewed-by Nicolas de Pesloüan<[email protected]>

The loop is going over the state of ports.
Since the new code at the end of loop has to check for STATE_FORWARDING it is clearer with continue statement. When adding code it is always better to clarify the logic in the process rather than making it more complex.

This makes the bridge device behave like a physical device. In earlier releases the bridge always asserted carrier. This changes the behavior so that bridge device carrier is on only if one or more ports are in the forwarding state. This should help IPv6 autoconfiguration, DHCP, and routing daemons.

This makes the bridge device behave like a physical device. In earlier releases the bridge always asserted carrier. This changes the behavior so that bridge device carrier is on only if one or more ports are in the forwarding state. This should help IPv6 autoconfiguration, DHCP, and routing daemons.

I did brief testing with Network and Virt manager and they seem fine, but since this changes behavior of bridge, it should wait until net-next (2.6.39).

Signed-off-by Stephen Hemminger <[email protected]>

Quoted 1 line · Stephen Hemminger

Why not set forwarding delay to zero? I don't think you are using STP?

No, I'm not relying on STP. I think setting learning->forwarding delay to 0 could cause problems with STP like loops until the loop is detected.

IPv6 address autoconfiguration relies on an UP interface to processes traffic normally. A bridge that is UP enters LEARNING state and this state "eats" any Router Advertising packets sent to the bridge. Issuing a NETDEV_CHANGE notification on the bridge interface when it changes state allows autoconfiguration code to retry querying router information.

Quoted 8 lines · Adam Majer

IPv6 address autoconfiguration relies on an UP interface to processes traffic normally. A bridge that is UP enters LEARNING state and this state "eats" any Router Advertising packets sent to the bridge. Issuing a NETDEV_CHANGE notification on the bridge interface when it changes state allows autoconfiguration code to retry querying router information.

...

net/bridge/br_if.c line 42

Quoted 11 lines

net/bridge/br_if.c +11 -0

+static void br_port_change_notifier_handler(struct work_struct *work)
+{
+	struct net_bridge *br = container_of(work,
+					     struct net_bridge,
+					     change_notification_worker);
+
+	rtnl_lock();
+	netdev_state_change(br->dev);
+	rtnl_unlock();
+}
+

Do you really want user-space to get this notification too? Why do you put it into a workqueue? Maybe it has to be called in process-context?