| Message ID | 20110307103406.27330529@nehalam |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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); > -- 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
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.
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. -- 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
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 -- 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
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. -- 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
--- 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);
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(-) -- 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