Patchwork [net-next,0/4] net: allow to change carrier from userspace

login
register
mail settings
Submitter stephen hemminger
Date Dec. 14, 2012, 4:59 p.m.
Message ID <20121214085918.6a2f3535@nehalam.linuxnetplumber.net>
Download mbox | patch
Permalink /patch/206528/
State RFC
Delegated to: David Miller
Headers show

Comments

stephen hemminger - Dec. 14, 2012, 4:59 p.m.
On Fri, 14 Dec 2012 17:35:32 +0100
Jiri Pirko <jiri@resnulli.us> wrote:

> Fri, Dec 14, 2012 at 05:12:01PM CET, shemminger@vyatta.com wrote:
> >On Fri, 14 Dec 2012 15:41:34 +0100
> >Jiri Pirko <jiri@resnulli.us> wrote:
> >
> >> Thu, Dec 13, 2012 at 07:20:51PM CET, shemminger@vyatta.com wrote:
> >> >On Thu, 13 Dec 2012 16:17:33 -0200
> >> >Flavio Leitner <fbl@redhat.com> wrote:
> >> >
> >> >> On Thu, 13 Dec 2012 10:09:33 -0800
> >> >> Stephen Hemminger <shemminger@vyatta.com> wrote:
> >> >> 
> >> >> > On Thu, 13 Dec 2012 15:54:23 -0200
> >> >> > Flavio Leitner <fbl@redhat.com> wrote:
> >> >> > 
> >> >> > > I am saying this because people are used to and there are scripts out
> >> >> > > there using something like:
> >> >> > > # ethtool <iface> | grep 'Link'
> >> >> > > to react an interface failure.
> >> >> > 
> >> >> > Then the script is broken. It is asking about hardware state.
> >> >> 
> >> >> I was talking about the team master interface, so it makes sense
> >> >> to check its 'hardware' state. Just think on 'bond0' interface
> >> >> with no slaves. It should report Link detected: no.
> >> >> 
> >> >> See bond_release(), what happens if bond->slave_cnt == 0, for instance.
> >> >> 
> >> >
> >> >I was thinking more that ethtool operation for reporting link on
> >> >the team device should use the proper check rather than just using netif_carrier_ok(),
> >> >the team ethtool operation for get_link should be check IFF_RUNNING flag
> >> >in dev->flags which is controlled by operstate transistions.
> >> 
> >> I admit I'm bit confused now.
> >> 
> >> For example in bridge code:
> >> in br_add_if() - netif_carrier_ok() is checked and by the value it is
> >> decided if br_stp_enable_port() is called or not. Wouldn't it make more
> >> sense to check IFF_RUNNING (or netif_oper_up()) here?
> >> 
> >> The reason I'm asing is that if team device is in bridge, carrier is
> >> always ON and I'm fiddling with IF_OPER_UP and IF_OPER_DORMANT from
> >> userspace, in current code, bridge wouldn't know the difference...
> >> 
> >> There are more exmaples of similar usage of netif_carrier_ok() in
> >> bridge (called on ports), bonding (called on slaves), team code (called on ports).
> >
> >Yes the bridge should be fixed to work with user controlled devices.
> 
> Okay. I'll try to figure out some patchset over the weekend.
> 
> Thanks.
> 

Something like this seems needed.

--
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
Jiri Pirko - Dec. 14, 2012, 5:13 p.m.
Fri, Dec 14, 2012 at 05:59:18PM CET, shemminger@vyatta.com wrote:
>On Fri, 14 Dec 2012 17:35:32 +0100
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Fri, Dec 14, 2012 at 05:12:01PM CET, shemminger@vyatta.com wrote:
>> >On Fri, 14 Dec 2012 15:41:34 +0100
>> >Jiri Pirko <jiri@resnulli.us> wrote:
>> >
>> >> Thu, Dec 13, 2012 at 07:20:51PM CET, shemminger@vyatta.com wrote:
>> >> >On Thu, 13 Dec 2012 16:17:33 -0200
>> >> >Flavio Leitner <fbl@redhat.com> wrote:
>> >> >
>> >> >> On Thu, 13 Dec 2012 10:09:33 -0800
>> >> >> Stephen Hemminger <shemminger@vyatta.com> wrote:
>> >> >> 
>> >> >> > On Thu, 13 Dec 2012 15:54:23 -0200
>> >> >> > Flavio Leitner <fbl@redhat.com> wrote:
>> >> >> > 
>> >> >> > > I am saying this because people are used to and there are scripts out
>> >> >> > > there using something like:
>> >> >> > > # ethtool <iface> | grep 'Link'
>> >> >> > > to react an interface failure.
>> >> >> > 
>> >> >> > Then the script is broken. It is asking about hardware state.
>> >> >> 
>> >> >> I was talking about the team master interface, so it makes sense
>> >> >> to check its 'hardware' state. Just think on 'bond0' interface
>> >> >> with no slaves. It should report Link detected: no.
>> >> >> 
>> >> >> See bond_release(), what happens if bond->slave_cnt == 0, for instance.
>> >> >> 
>> >> >
>> >> >I was thinking more that ethtool operation for reporting link on
>> >> >the team device should use the proper check rather than just using netif_carrier_ok(),
>> >> >the team ethtool operation for get_link should be check IFF_RUNNING flag
>> >> >in dev->flags which is controlled by operstate transistions.
>> >> 
>> >> I admit I'm bit confused now.
>> >> 
>> >> For example in bridge code:
>> >> in br_add_if() - netif_carrier_ok() is checked and by the value it is
>> >> decided if br_stp_enable_port() is called or not. Wouldn't it make more
>> >> sense to check IFF_RUNNING (or netif_oper_up()) here?
>> >> 
>> >> The reason I'm asing is that if team device is in bridge, carrier is
>> >> always ON and I'm fiddling with IF_OPER_UP and IF_OPER_DORMANT from
>> >> userspace, in current code, bridge wouldn't know the difference...
>> >> 
>> >> There are more exmaples of similar usage of netif_carrier_ok() in
>> >> bridge (called on ports), bonding (called on slaves), team code (called on ports).
>> >
>> >Yes the bridge should be fixed to work with user controlled devices.
>> 
>> Okay. I'll try to figure out some patchset over the weekend.
>> 
>> Thanks.
>> 
>
>Something like this seems needed.
>
>--- 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);


Yes. I have this already in my queue. I just spotted a problem though.

Lets say teamd sets operstate of the team device by values IF_OPER_UP
and IF_OPER_DORMANT depending on teamd states of ports.
What if one would like to use 802.1X supplicant on the same device?
That would not be possible.

This proves that the layering would not be correct. It look like the
carrier userspace set would be the correct thing to do after all...

What do you think?

Jiri

--
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
stephen hemminger - Dec. 14, 2012, 5:23 p.m.
On Fri, 14 Dec 2012 18:13:45 +0100
Jiri Pirko <jiri@resnulli.us> wrote:

> Fri, Dec 14, 2012 at 05:59:18PM CET, shemminger@vyatta.com wrote:
> >On Fri, 14 Dec 2012 17:35:32 +0100
> >Jiri Pirko <jiri@resnulli.us> wrote:
> >
> >> Fri, Dec 14, 2012 at 05:12:01PM CET, shemminger@vyatta.com wrote:
> >> >On Fri, 14 Dec 2012 15:41:34 +0100
> >> >Jiri Pirko <jiri@resnulli.us> wrote:
> >> >
> >> >> Thu, Dec 13, 2012 at 07:20:51PM CET, shemminger@vyatta.com wrote:
> >> >> >On Thu, 13 Dec 2012 16:17:33 -0200
> >> >> >Flavio Leitner <fbl@redhat.com> wrote:
> >> >> >
> >> >> >> On Thu, 13 Dec 2012 10:09:33 -0800
> >> >> >> Stephen Hemminger <shemminger@vyatta.com> wrote:
> >> >> >> 
> >> >> >> > On Thu, 13 Dec 2012 15:54:23 -0200
> >> >> >> > Flavio Leitner <fbl@redhat.com> wrote:
> >> >> >> > 
> >> >> >> > > I am saying this because people are used to and there are scripts out
> >> >> >> > > there using something like:
> >> >> >> > > # ethtool <iface> | grep 'Link'
> >> >> >> > > to react an interface failure.
> >> >> >> > 
> >> >> >> > Then the script is broken. It is asking about hardware state.
> >> >> >> 
> >> >> >> I was talking about the team master interface, so it makes sense
> >> >> >> to check its 'hardware' state. Just think on 'bond0' interface
> >> >> >> with no slaves. It should report Link detected: no.
> >> >> >> 
> >> >> >> See bond_release(), what happens if bond->slave_cnt == 0, for instance.
> >> >> >> 
> >> >> >
> >> >> >I was thinking more that ethtool operation for reporting link on
> >> >> >the team device should use the proper check rather than just using netif_carrier_ok(),
> >> >> >the team ethtool operation for get_link should be check IFF_RUNNING flag
> >> >> >in dev->flags which is controlled by operstate transistions.
> >> >> 
> >> >> I admit I'm bit confused now.
> >> >> 
> >> >> For example in bridge code:
> >> >> in br_add_if() - netif_carrier_ok() is checked and by the value it is
> >> >> decided if br_stp_enable_port() is called or not. Wouldn't it make more
> >> >> sense to check IFF_RUNNING (or netif_oper_up()) here?
> >> >> 
> >> >> The reason I'm asing is that if team device is in bridge, carrier is
> >> >> always ON and I'm fiddling with IF_OPER_UP and IF_OPER_DORMANT from
> >> >> userspace, in current code, bridge wouldn't know the difference...
> >> >> 
> >> >> There are more exmaples of similar usage of netif_carrier_ok() in
> >> >> bridge (called on ports), bonding (called on slaves), team code (called on ports).
> >> >
> >> >Yes the bridge should be fixed to work with user controlled devices.
> >> 
> >> Okay. I'll try to figure out some patchset over the weekend.
> >> 
> >> Thanks.
> >> 
> >
> >Something like this seems needed.
> >
> >--- 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);
> 
> 
> Yes. I have this already in my queue. I just spotted a problem though.
> 
> Lets say teamd sets operstate of the team device by values IF_OPER_UP
> and IF_OPER_DORMANT depending on teamd states of ports.
> What if one would like to use 802.1X supplicant on the same device?
> That would not be possible.
> 
> This proves that the layering would not be correct. It look like the
> carrier userspace set would be the correct thing to do after all...
> 
> What do you think?

That is tough, you have two applications conflicting over control of
the same state on the same device.

--
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
Jiri Pirko - Dec. 14, 2012, 5:35 p.m.
Fri, Dec 14, 2012 at 06:23:43PM CET, shemminger@vyatta.com wrote:
>On Fri, 14 Dec 2012 18:13:45 +0100
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Fri, Dec 14, 2012 at 05:59:18PM CET, shemminger@vyatta.com wrote:
>> >On Fri, 14 Dec 2012 17:35:32 +0100
>> >Jiri Pirko <jiri@resnulli.us> wrote:
>> >
>> >> Fri, Dec 14, 2012 at 05:12:01PM CET, shemminger@vyatta.com wrote:
>> >> >On Fri, 14 Dec 2012 15:41:34 +0100
>> >> >Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >
>> >> >> Thu, Dec 13, 2012 at 07:20:51PM CET, shemminger@vyatta.com wrote:
>> >> >> >On Thu, 13 Dec 2012 16:17:33 -0200
>> >> >> >Flavio Leitner <fbl@redhat.com> wrote:
>> >> >> >
>> >> >> >> On Thu, 13 Dec 2012 10:09:33 -0800
>> >> >> >> Stephen Hemminger <shemminger@vyatta.com> wrote:
>> >> >> >> 
>> >> >> >> > On Thu, 13 Dec 2012 15:54:23 -0200
>> >> >> >> > Flavio Leitner <fbl@redhat.com> wrote:
>> >> >> >> > 
>> >> >> >> > > I am saying this because people are used to and there are scripts out
>> >> >> >> > > there using something like:
>> >> >> >> > > # ethtool <iface> | grep 'Link'
>> >> >> >> > > to react an interface failure.
>> >> >> >> > 
>> >> >> >> > Then the script is broken. It is asking about hardware state.
>> >> >> >> 
>> >> >> >> I was talking about the team master interface, so it makes sense
>> >> >> >> to check its 'hardware' state. Just think on 'bond0' interface
>> >> >> >> with no slaves. It should report Link detected: no.
>> >> >> >> 
>> >> >> >> See bond_release(), what happens if bond->slave_cnt == 0, for instance.
>> >> >> >> 
>> >> >> >
>> >> >> >I was thinking more that ethtool operation for reporting link on
>> >> >> >the team device should use the proper check rather than just using netif_carrier_ok(),
>> >> >> >the team ethtool operation for get_link should be check IFF_RUNNING flag
>> >> >> >in dev->flags which is controlled by operstate transistions.
>> >> >> 
>> >> >> I admit I'm bit confused now.
>> >> >> 
>> >> >> For example in bridge code:
>> >> >> in br_add_if() - netif_carrier_ok() is checked and by the value it is
>> >> >> decided if br_stp_enable_port() is called or not. Wouldn't it make more
>> >> >> sense to check IFF_RUNNING (or netif_oper_up()) here?
>> >> >> 
>> >> >> The reason I'm asing is that if team device is in bridge, carrier is
>> >> >> always ON and I'm fiddling with IF_OPER_UP and IF_OPER_DORMANT from
>> >> >> userspace, in current code, bridge wouldn't know the difference...
>> >> >> 
>> >> >> There are more exmaples of similar usage of netif_carrier_ok() in
>> >> >> bridge (called on ports), bonding (called on slaves), team code (called on ports).
>> >> >
>> >> >Yes the bridge should be fixed to work with user controlled devices.
>> >> 
>> >> Okay. I'll try to figure out some patchset over the weekend.
>> >> 
>> >> Thanks.
>> >> 
>> >
>> >Something like this seems needed.
>> >
>> >--- 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);
>> 
>> 
>> Yes. I have this already in my queue. I just spotted a problem though.
>> 
>> Lets say teamd sets operstate of the team device by values IF_OPER_UP
>> and IF_OPER_DORMANT depending on teamd states of ports.
>> What if one would like to use 802.1X supplicant on the same device?
>> That would not be possible.
>> 
>> This proves that the layering would not be correct. It look like the
>> carrier userspace set would be the correct thing to do after all...
>> 
>> What do you think?
>
>That is tough, you have two applications conflicting over control of
>the same state on the same device.


I do not think so. The thing is, they both do something different. Teamd
should care of the low level link and set that up accordingly.
802.1X supplicant is higher. This implies they should not use the same
interface, ergo IMHO Teamd should be able to set carrier directly.

And that is what is provided by this patchset.

ps: I think that the use of 802.1X supplicant over team device is legit
setup.

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

Patch

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