Message ID | 20121227222854.6ec132dd@nehalam.linuxnetplumber.net |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Fri, Dec 28, 2012 at 07:28:54AM CET, shemminger@vyatta.com wrote: >The bridge link detection should follow the operational state >of the lower device, rather than the carrier bit. This allows devices >like tunnels that are controlled by userspace control plane to work >with bridge STP link management. > > >Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> > > >--- a/net/bridge/br_if.c 2012-10-25 09:11:15.627272524 -0700 >+++ b/net/bridge/br_if.c 2012-12-14 08:58:14.329847361 -0800 >@@ -66,14 +66,14 @@ void br_port_carrier_check(struct net_br > struct net_device *dev = p->dev; > struct net_bridge *br = p->br; > >- if (netif_running(dev) && netif_carrier_ok(dev)) >+ if (netif_running(dev) && netif_oper_up(dev)) > p->path_cost = port_cost(dev); > > if (!netif_running(br->dev)) > return; > > spin_lock_bh(&br->lock); >- if (netif_running(dev) && netif_carrier_ok(dev)) { >+ if (netif_running(dev) && netif_oper_up(dev)) > if (p->state == BR_STATE_DISABLED) > br_stp_enable_port(p); > } else { >--- a/net/bridge/br_notify.c 2012-10-25 09:11:15.631272484 -0700 >+++ b/net/bridge/br_notify.c 2012-12-14 08:57:36.954222724 -0800 >@@ -82,7 +82,7 @@ static int br_device_event(struct notifi > break; > > case NETDEV_UP: >- if (netif_carrier_ok(dev) && (br->dev->flags & IFF_UP)) { >+ if (netif_running(br->dev) && netif_oper_up(dev)) { > spin_lock_bh(&br->lock); > br_stp_enable_port(p); > spin_unlock_bh(&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 Reviewed-by: Jiri Pirko <jiri@resnulli.us> -- 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 Thu, Dec 27, 2012 at 10:28:54PM -0800, Stephen Hemminger wrote: > The bridge link detection should follow the operational state > of the lower device, rather than the carrier bit. This allows devices > like tunnels that are controlled by userspace control plane to work > with bridge STP link management. > > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> > > > --- a/net/bridge/br_if.c 2012-10-25 09:11:15.627272524 -0700 > +++ b/net/bridge/br_if.c 2012-12-14 08:58:14.329847361 -0800 > @@ -66,14 +66,14 @@ void br_port_carrier_check(struct net_br > struct net_device *dev = p->dev; > struct net_bridge *br = p->br; > > - if (netif_running(dev) && netif_carrier_ok(dev)) > + if (netif_running(dev) && netif_oper_up(dev)) > p->path_cost = port_cost(dev); > > if (!netif_running(br->dev)) > return; > > spin_lock_bh(&br->lock); > - if (netif_running(dev) && netif_carrier_ok(dev)) { > + if (netif_running(dev) && netif_oper_up(dev)) > if (p->state == BR_STATE_DISABLED) > br_stp_enable_port(p); I found this piece still using netif_carrier_ok(): 321 int br_add_if(struct net_bridge *br, struct net_device *dev) 322 { ... 385 386 if ((dev->flags & IFF_UP) && netif_carrier_ok(dev) && 387 (br->dev->flags & IFF_UP)) 388 br_stp_enable_port(p); 389 spin_unlock_bh(&br->lock); 390 Is there any reason for enabling stp on a port using operstate in br_port_carrier_check() but not in br_add_if() ? The same thing happens with br_stp_enable_bridge(): 56 list_for_each_entry(p, &br->port_list, list) { 57 if ((p->dev->flags & IFF_UP) && netif_carrier_ok(p->dev)) 58 br_stp_enable_port(p); Also, as operstate UP means that packets are flowing, there is no need to check if the device is opened, so checking only for operstate should be enough, right? I.e. - if ((p->dev->flags & IFF_UP) && netif_carrier_ok(p->dev)) + if (netif_oper_up(dev)) > } else { > --- a/net/bridge/br_notify.c 2012-10-25 09:11:15.631272484 -0700 > +++ b/net/bridge/br_notify.c 2012-12-14 08:57:36.954222724 -0800 > @@ -82,7 +82,7 @@ static int br_device_event(struct notifi > break; > > case NETDEV_UP: > - if (netif_carrier_ok(dev) && (br->dev->flags & IFF_UP)) { > + if (netif_running(br->dev) && netif_oper_up(dev)) { > spin_lock_bh(&br->lock); > br_stp_enable_port(p); > spin_unlock_bh(&br->lock); You are not just changing to use operstate, but also to check another flag - before it was IFF_UP and now __LINK_STATE_START. Any reason for that besides being consistent with other checks? thanks!
From: Stephen Hemminger <shemminger@vyatta.com> Date: Thu, 27 Dec 2012 22:28:54 -0800 > spin_lock_bh(&br->lock); > - if (netif_running(dev) && netif_carrier_ok(dev)) { > + if (netif_running(dev) && netif_oper_up(dev)) > if (p->state == BR_STATE_DISABLED) > br_stp_enable_port(p); > } else { You didn't even try to compile this. -- 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_if.c 2012-10-25 09:11:15.627272524 -0700 +++ b/net/bridge/br_if.c 2012-12-14 08:58:14.329847361 -0800 @@ -66,14 +66,14 @@ void br_port_carrier_check(struct net_br struct net_device *dev = p->dev; struct net_bridge *br = p->br; - if (netif_running(dev) && netif_carrier_ok(dev)) + if (netif_running(dev) && netif_oper_up(dev)) p->path_cost = port_cost(dev); if (!netif_running(br->dev)) return; spin_lock_bh(&br->lock); - if (netif_running(dev) && netif_carrier_ok(dev)) { + if (netif_running(dev) && netif_oper_up(dev)) if (p->state == BR_STATE_DISABLED) br_stp_enable_port(p); } else { --- a/net/bridge/br_notify.c 2012-10-25 09:11:15.631272484 -0700 +++ b/net/bridge/br_notify.c 2012-12-14 08:57:36.954222724 -0800 @@ -82,7 +82,7 @@ static int br_device_event(struct notifi break; case NETDEV_UP: - if (netif_carrier_ok(dev) && (br->dev->flags & IFF_UP)) { + if (netif_running(br->dev) && netif_oper_up(dev)) { spin_lock_bh(&br->lock); br_stp_enable_port(p); spin_unlock_bh(&br->lock);
The bridge link detection should follow the operational state of the lower device, rather than the carrier bit. This allows devices like tunnels that are controlled by userspace control plane to work with bridge STP link management. Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> -- 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