[PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

[PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

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

Signed-off-by: Adam Majer <adamm@zombino.com>
---
 net/bridge/br_if.c      |   20 ++++++++++++++++++++
 net/bridge/br_private.h |    2 ++
 net/bridge/br_stp.c     |    2 ++
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d9d1e2b..9604d52 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -25,6 +25,22 @@
 
 #include "br_private.h"
 
+
+/*
+ * Notifies that the bridge has changed state
+ */
+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();
+}
+
+
 /*
  * Determine initial path cost based on speed.
  * using recommendations from 802.1d standard
@@ -163,6 +179,7 @@ static void del_br(struct net_bridge *br, struct list_head *head)
 {
 	struct net_bridge_port *p, *n;
 
+	flush_work(&br->change_notification_worker);
 	list_for_each_entry_safe(p, n, &br->port_list, list) {
 		del_nbp(p);
 	}
@@ -203,6 +220,9 @@ static struct net_device *new_bridge_dev(struct net *net, const char *name)
 
 	memcpy(br->group_addr, br_group_address, ETH_ALEN);
 
+	INIT_WORK(&br->change_notification_worker,
+		  &br_port_change_notifier_handler);
+
 	br->feature_mask = dev->features;
 	br->stp_enabled = BR_NO_STP;
 	br->designated_root = br->bridge_id;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4e1b620..7f8ef41 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -179,6 +179,8 @@ struct net_bridge
 	struct list_head		port_list;
 	struct net_device		*dev;
 
+	struct work_struct		change_notification_worker;
+
 	struct br_cpu_netstats __percpu *stats;
 	spinlock_t			hash_lock;
 	struct hlist_head		hash[BR_HASH_SIZE];
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 57186d8..b5be3e3 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -296,6 +296,8 @@ void br_topology_change_detection(struct net_bridge *br)
 {
 	int isroot = br_is_root_bridge(br);
 
+	queue_work(system_long_wq, &br->change_notification_worker);
+
 	if (br->stp_enabled != BR_KERNEL_STP)
 		return;
 
-- 
1.7.2.3

[PATCH 2/2] Retry autoconfiguration on interface after NETDEV_CHANGE notification

[Adam Majer]

A bridged interface will timeout trying to receive Router Advert as
these packets are not forwarded when bridge is UP but not in the
FORWARDING state (eg. LEARNING state). Bridge code issues
NETDEV_CHANGE when bridge's internal state is changed. It is then
possible to retry Router Solicitation.

Signed-off-by: Adam Majer <adamm@zombino.com>
---
 net/ipv6/addrconf.c |   74 +++++++++++++++++++++++++++++++++-----------------
 1 files changed, 49 insertions(+), 25 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index fd6782e..f8018b3 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -152,6 +152,7 @@ static void addrconf_dad_start(struct inet6_ifaddr *ifp, u32 flags);
 static void addrconf_dad_timer(unsigned long data);
 static void addrconf_dad_completed(struct inet6_ifaddr *ifp);
 static void addrconf_dad_run(struct inet6_dev *idev);
+static void addrconf_rs_start(struct inet6_ifaddr *ifp);
 static void addrconf_rs_timer(unsigned long data);
 static void __ipv6_ifa_notify(int event, struct inet6_ifaddr *ifa);
 static void ipv6_ifa_notify(int event, struct inet6_ifaddr *ifa);
@@ -2368,6 +2369,18 @@ static void addrconf_add_linklocal(struct inet6_dev *idev, struct in6_addr *addr
 		addrconf_prefix_route(&ifp->addr, ifp->prefix_len, idev->dev, 0, 0);
 		addrconf_dad_start(ifp, 0);
 		in6_ifa_put(ifp);
+	} else if (PTR_ERR(ifp) == -EEXIST &&
+		list_is_singular(&idev->addr_list)) {
+		/*
+		 * first address must be link local, but no router - re-solicit.
+		 * This code path is called when bridge exits LEARNING state
+		 */
+
+		ifp = list_first_entry(&idev->addr_list,
+				       struct inet6_ifaddr,
+				       if_list);
+
+		addrconf_rs_start(ifp);
 	}
 }
 
@@ -2532,8 +2545,11 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			}
 
 			if (idev) {
-				if (idev->if_flags & IF_READY)
-					/* device is already configured. */
+				if (idev->if_flags & IF_READY &&
+				    idev->if_flags & IF_RA_RCVD)
+					/* device is already configured and
+					 * RA was received.
+					 */
 					break;
 				idev->if_flags |= IF_READY;
 			}
@@ -2775,6 +2791,35 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 	return 0;
 }
 
+static void addrconf_rs_start(struct inet6_ifaddr *ifp)
+{
+	/* If added prefix is link local and forwarding is off,
+	   start sending router solicitations.
+	 */
+	struct net_device *dev = ifp->idev->dev;
+
+	if ((ifp->idev->cnf.forwarding == 0 ||
+	     ifp->idev->cnf.forwarding == 2) &&
+	    ifp->idev->cnf.rtr_solicits > 0 &&
+	    (dev->flags&IFF_LOOPBACK) == 0 &&
+	    (ipv6_addr_type(&ifp->addr) & IPV6_ADDR_LINKLOCAL)) {
+		/*
+		 *	If a host as already performed a random delay
+		 *	[...] as part of DAD [...] there is no need
+		 *	to delay again before sending the first RS
+		 */
+		ndisc_send_rs(ifp->idev->dev, &ifp->addr,
+			      &in6addr_linklocal_allrouters);
+
+		spin_lock_bh(&ifp->lock);
+		ifp->probes = 1;
+		ifp->idev->if_flags |= IF_RS_SENT;
+		addrconf_mod_timer(ifp, AC_RS,
+				   ifp->idev->cnf.rtr_solicit_interval);
+		spin_unlock_bh(&ifp->lock);
+	}
+}
+
 static void addrconf_rs_timer(unsigned long data)
 {
 	struct inet6_ifaddr *ifp = (struct inet6_ifaddr *) data;
@@ -2935,36 +2980,15 @@ out:
 
 static void addrconf_dad_completed(struct inet6_ifaddr *ifp)
 {
-	struct net_device *dev = ifp->idev->dev;
-
 	/*
 	 *	Configure the address for reception. Now it is valid.
 	 */
 
 	ipv6_ifa_notify(RTM_NEWADDR, ifp);
 
-	/* If added prefix is link local and forwarding is off,
-	   start sending router solicitations.
-	 */
+	/* start sending router solicitations. */
 
-	if ((ifp->idev->cnf.forwarding == 0 ||
-	     ifp->idev->cnf.forwarding == 2) &&
-	    ifp->idev->cnf.rtr_solicits > 0 &&
-	    (dev->flags&IFF_LOOPBACK) == 0 &&
-	    (ipv6_addr_type(&ifp->addr) & IPV6_ADDR_LINKLOCAL)) {
-		/*
-		 *	If a host as already performed a random delay
-		 *	[...] as part of DAD [...] there is no need
-		 *	to delay again before sending the first RS
-		 */
-		ndisc_send_rs(ifp->idev->dev, &ifp->addr, &in6addr_linklocal_allrouters);
-
-		spin_lock_bh(&ifp->lock);
-		ifp->probes = 1;
-		ifp->idev->if_flags |= IF_RS_SENT;
-		addrconf_mod_timer(ifp, AC_RS, ifp->idev->cnf.rtr_solicit_interval);
-		spin_unlock_bh(&ifp->lock);
-	}
+	addrconf_rs_start(ifp);
 }
 
 static void addrconf_dad_run(struct inet6_dev *idev)
-- 
1.7.2.3

Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

[Adam Majer]

Hello,

As background for these 2 patches, the current bridge code does not
play very nicely with IPv6 autoconfiguration. What happens is,

[   35.117030] device eth0 entered promiscuous mode
[   35.120898] br0: port 1(eth0) entering learning state
[   35.120901] br0: port 1(eth0) entering learning state

Here bridge is setup and both interface eth0 and br0 are indicated as
up via NETDEV_UP notification. The IPv6 autoconfiguration code
configures linklocal address and start to issue RS packets. These
timeout because bridge is in learning state, not forwarding state.

[   45.168007] br0: no IPv6 routers present
[   45.312020] eth0: no IPv6 routers present

RS timeouts and IPv6 is abandoned at this point.

[   50.144011] br0: port 1(eth0) entering forwarding state

Now bridge entered forwarding state. Userland sets up IPv4 via DHCP or
otherwise.

[   50.144487] ADDRCONF(NETDEV_CHANGE): br0: link becomes ready

With the patches, NETDEV_CHANGE is issued by the bridge once it
changes state. The autoconfiguration code then checks that no RA
packets are received and only linklocal interface is setup. It then
retries soliciting router information.


This is my first attempt at a kernel patch so please be gentle :) Part
of the autoconf patch splits out the addrconf_rs_start() code from the
addrconf_dad_completed() function.

Cheers,
Adam


-- 
Adam Majer
adamm@zombino.com

Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

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

Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

[Adam Majer]

On Sat, Mar 05, 2011 at 10:43:03PM -0800, Stephen Hemminger wrote:
> 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.

-- 
Adam Majer
adamm@zombino.com

Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

[Stephen Hemminger]

On Sun, 6 Mar 2011 02:03:28 -0600
Adam Majer <adamm@zombino.com> wrote:

> On Sat, Mar 05, 2011 at 10:43:03PM -0800, Stephen Hemminger wrote:
> > 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.

-- 

Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

[Jan Ceuleers]

On 06/03/11 09:03, Adam Majer wrote:
> On Sat, Mar 05, 2011 at 10:43:03PM -0800, Stephen Hemminger wrote:
>> 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.

I don't think that Stephen is saying that you should hard-code the 
forwarding delay as zero (i.e. to remove the delay from the kernel 
completely), but rather use brctl to set the forwarding delay to zero on 
bridge instances in which you're not using STP.

HTH, Jan

Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

[Adam Majer]

On Sun, Mar 06, 2011 at 09:45:41AM -0800, Stephen Hemminger wrote:
> 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.


-- 
Adam Majer
adamm@zombino.com

Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

[Stephen Hemminger]

> On Sun, Mar 06, 2011 at 09:45:41AM -0800, Stephen Hemminger wrote:
> > 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.

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. 

Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

[Nicolas de Pesloüan]

Le 07/03/2011 07:41, Stephen Hemminger a écrit :
>
>> On Sun, Mar 06, 2011 at 09:45:41AM -0800, Stephen Hemminger wrote:
>>> 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.
>
> 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.

I think it is the right way to manage this situation. And bonding should behave the same, if not 
already true.

	Nicolas.

[PATCH] bridge: control carrier based on ports online

[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 <shemminger@vyatta.com>

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

--- a/net/bridge/br_device.c	2011-03-07 08:40:08.913599513 -0800
+++ b/net/bridge/br_device.c	2011-03-07 08:40:48.382377389 -0800
@@ -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);
 
--- a/net/bridge/br_stp.c	2011-03-07 08:41:58.619783678 -0800
+++ b/net/bridge/br_stp.c	2011-03-07 08:53:58.953558810 -0800
@@ -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) {
+			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_FORWARDING)
+			++liveports;
 	}
+
+	if (liveports == 0)
+		netif_carrier_off(br->dev);
+	else
+		netif_carrier_on(br->dev);
 }
 
 /* called under bridge lock */
--- a/net/bridge/br_stp_timer.c	2011-03-07 08:53:25.728770710 -0800
+++ b/net/bridge/br_stp_timer.c	2011-03-07 08:53:40.273116636 -0800
@@ -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);

Re: [PATCH] bridge: control carrier based on ports online

[Nicolas de Pesloüan]

Le 07/03/2011 19:34, Stephen Hemminger a écrit :
> 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<shemminger@vyatta.com>
>
> ---
>   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(-)
>
> --- a/net/bridge/br_device.c	2011-03-07 08:40:08.913599513 -0800
> +++ b/net/bridge/br_device.c	2011-03-07 08:40:48.382377389 -0800
> @@ -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);
>
> --- a/net/bridge/br_stp.c	2011-03-07 08:41:58.619783678 -0800
> +++ b/net/bridge/br_stp.c	2011-03-07 08:53:58.953558810 -0800
> @@ -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) {
> +			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 <nicolas.2p.debian@free.fr>

>   		}
>
> +		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 */
> --- a/net/bridge/br_stp_timer.c	2011-03-07 08:53:25.728770710 -0800
> +++ b/net/bridge/br_stp_timer.c	2011-03-07 08:53:40.273116636 -0800
> @@ -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);
>

Re: [PATCH] bridge: control carrier based on ports online

[Stephen Hemminger]

On Mon, 07 Mar 2011 21:48:16 +0100
Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:

> Le 07/03/2011 19:34, Stephen Hemminger a écrit :
> > 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<shemminger@vyatta.com>
> >
> > ---
> >   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(-)
> >
> > --- a/net/bridge/br_device.c	2011-03-07 08:40:08.913599513 -0800
> > +++ b/net/bridge/br_device.c	2011-03-07 08:40:48.382377389 -0800
> > @@ -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);
> >
> > --- a/net/bridge/br_stp.c	2011-03-07 08:41:58.619783678 -0800
> > +++ b/net/bridge/br_stp.c	2011-03-07 08:53:58.953558810 -0800
> > @@ -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) {
> > +			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 <nicolas.2p.debian@free.fr>

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.


-- 

Re: [PATCH] bridge: control carrier based on ports online

[Nicolas de Pesloüan]

Le 07/03/2011 22:44, Stephen Hemminger a écrit :
> On Mon, 07 Mar 2011 21:48:16 +0100
> Nicolas de Pesloüan<nicolas.2p.debian@gmail.com>  wrote:
>
>> Le 07/03/2011 19:34, Stephen Hemminger a écrit :
[snip]
>>>    	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) {
>>> +			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<nicolas.2p.debian@free.fr>
>
> 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.

Sound's good to me. Thanks for clarifying.

	Nicolas.

Re: [PATCH] bridge: control carrier based on ports online

[Adam Majer]

On Mon, Mar 07, 2011 at 10:34:06AM -0800, Stephen Hemminger wrote:
> 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.

Yes, your patch does fix issues with IPv6 autoconfiguration in bridged
networks completely. I've just tested it with and without STP. IPv4
DHCP continues to function correctly. It is definitely much cleaner
solution than my hack.

IPv6 autoconfiguration code already checks if NETDEV_CHANGE or
NETDEV_UP represent a device that is ready to transmit data before
attempting to configure the interface. No additional changes required
there. This patch should also fix duplicate address detection of
linklocal address on bridged interfaces that I believe was also broken
prior to this patch.

If you want, you can add

Tested-By: Adam Majer <adamm@zombino.com>


Cheers,
- Adam

Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

[Américo Wang]

On Sat, Mar 05, 2011 at 11:18:33PM -0600, Adam Majer wrote:
>
>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.
>
...

>+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?

Thanks.

Re: [PATCH 1/2] Issue NETDEV_CHANGE notification when bridge changes state

[Adam Majer]

On Wed, Mar 09, 2011 at 11:09:49PM +0800, Américo Wang wrote:
> On Sat, Mar 05, 2011 at 11:18:33PM -0600, Adam Majer wrote:
> >
> >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.
> >
> ...
> 
> >+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?

Stephen Hemminger's patch (posted as a reply in this thread but only
on the netdev mailing list) is much better as it only brings the link
up on the bridge interface when the bridge enters forwarding mode.

Effectively it does similar thing via NETDEV_CHANGE but requires no
other hacks in other code. It should make userland much happier too.


As for my patch, I've put that in the workqueue because the
notification cannot be called in interrupt context. Is there a better
way of calling code that can sleep from interrupt context?

- Adam

-- 
Adam Majer
adamm@zombino.com

Re: [PATCH] bridge: control carrier based on ports online

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 7 Mar 2011 10:34:06 -0800

> 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 <shemminger@vyatta.com>

Applied to net-next-2.6, thanks Stephen.