[PATCH net-next] team: account for oper state

[PATCH net-next] team: account for oper state

[George Wilkie]

Account for operational state when determining port linkup state,
as per Documentation/networking/operstates.txt.

Signed-off-by: George Wilkie <gwilkie@vyatta.att-mail.com>
---
 drivers/net/team/team.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index a6c6ce19eeee..231264a05e55 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2918,7 +2918,8 @@ static int team_device_event(struct notifier_block *unused,
 	case NETDEV_CHANGE:
 		if (netif_running(port->dev))
 			team_port_change_check(port,
-					       !!netif_carrier_ok(port->dev));
+					       !!(netif_carrier_ok(port->dev) &&
+						  netif_oper_up(port->dev)));
 		break;
 	case NETDEV_UNREGISTER:
 		team_del_slave(port->team->dev, dev);
-- 
2.11.0

Re: [PATCH net-next] team: account for oper state

[Jiri Pirko]

Wed, Apr 18, 2018 at 12:29:50PM CEST, gwilkie@vyatta.att-mail.com wrote:
>Account for operational state when determining port linkup state,
>as per Documentation/networking/operstates.txt.

Could you please point me to the exact place in the document where this
is suggested?


>
>Signed-off-by: George Wilkie <gwilkie@vyatta.att-mail.com>
>---
> drivers/net/team/team.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>index a6c6ce19eeee..231264a05e55 100644
>--- a/drivers/net/team/team.c
>+++ b/drivers/net/team/team.c
>@@ -2918,7 +2918,8 @@ static int team_device_event(struct notifier_block *unused,
> 	case NETDEV_CHANGE:
> 		if (netif_running(port->dev))
> 			team_port_change_check(port,
>-					       !!netif_carrier_ok(port->dev));
>+					       !!(netif_carrier_ok(port->dev) &&
>+						  netif_oper_up(port->dev)));
> 		break;
> 	case NETDEV_UNREGISTER:
> 		team_del_slave(port->team->dev, dev);
>-- 
>2.11.0
>

Re: [PATCH net-next] team: account for oper state

[George Wilkie]

On Wed, Apr 18, 2018 at 02:56:44PM +0200, Jiri Pirko wrote:
> Wed, Apr 18, 2018 at 12:29:50PM CEST, gwilkie@vyatta.att-mail.com wrote:
> >Account for operational state when determining port linkup state,
> >as per Documentation/networking/operstates.txt.
> 
> Could you please point me to the exact place in the document where this
> is suggested?
> 

Various places cover it I think.

In 1. Introduction:
"interface is not usable just because the admin enabled it"
"userspace must be granted the possibility to
influence operational state"

In 4. Setting from userspace:
"the userspace application can set IFLA_OPERSTATE
to IF_OPER_DORMANT or IF_OPER_UP as long as the driver does not set
netif_carrier_off() or netif_dormant_on()"

We have a use case where we want to set the oper state of the team ports based
on whether they are actually usable or not (as opposed to just admin up).

Cheers.

> 
> >
> >Signed-off-by: George Wilkie <gwilkie@vyatta.att-mail.com>
> >---
> > drivers/net/team/team.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> >index a6c6ce19eeee..231264a05e55 100644
> >--- a/drivers/net/team/team.c
> >+++ b/drivers/net/team/team.c
> >@@ -2918,7 +2918,8 @@ static int team_device_event(struct notifier_block *unused,
> > 	case NETDEV_CHANGE:
> > 		if (netif_running(port->dev))
> > 			team_port_change_check(port,
> >-					       !!netif_carrier_ok(port->dev));
> >+					       !!(netif_carrier_ok(port->dev) &&
> >+						  netif_oper_up(port->dev)));
> > 		break;
> > 	case NETDEV_UNREGISTER:
> > 		team_del_slave(port->team->dev, dev);
> >-- 
> >2.11.0
> >

Re: [PATCH net-next] team: account for oper state

[Jiri Pirko]

Wed, Apr 18, 2018 at 03:35:49PM CEST, gwilkie@vyatta.att-mail.com wrote:
>On Wed, Apr 18, 2018 at 02:56:44PM +0200, Jiri Pirko wrote:
>> Wed, Apr 18, 2018 at 12:29:50PM CEST, gwilkie@vyatta.att-mail.com wrote:
>> >Account for operational state when determining port linkup state,
>> >as per Documentation/networking/operstates.txt.
>> 
>> Could you please point me to the exact place in the document where this
>> is suggested?
>> 
>
>Various places cover it I think.
>
>In 1. Introduction:
>"interface is not usable just because the admin enabled it"
>"userspace must be granted the possibility to
>influence operational state"
>
>In 4. Setting from userspace:
>"the userspace application can set IFLA_OPERSTATE
>to IF_OPER_DORMANT or IF_OPER_UP as long as the driver does not set
>netif_carrier_off() or netif_dormant_on()"
>
>We have a use case where we want to set the oper state of the team ports based
>on whether they are actually usable or not (as opposed to just admin up).

Are you running a supplicant there or what is the use-case?

How is this handle in other drivers like bond, openvswitch, bridge, etc?

>
>Cheers.
>
>> 
>> >
>> >Signed-off-by: George Wilkie <gwilkie@vyatta.att-mail.com>
>> >---
>> > drivers/net/team/team.c | 3 ++-
>> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>> >index a6c6ce19eeee..231264a05e55 100644
>> >--- a/drivers/net/team/team.c
>> >+++ b/drivers/net/team/team.c
>> >@@ -2918,7 +2918,8 @@ static int team_device_event(struct notifier_block *unused,
>> > 	case NETDEV_CHANGE:
>> > 		if (netif_running(port->dev))
>> > 			team_port_change_check(port,
>> >-					       !!netif_carrier_ok(port->dev));
>> >+					       !!(netif_carrier_ok(port->dev) &&
>> >+						  netif_oper_up(port->dev)));
>> > 		break;
>> > 	case NETDEV_UNREGISTER:
>> > 		team_del_slave(port->team->dev, dev);
>> >-- 
>> >2.11.0
>> >

Re: [PATCH net-next] team: account for oper state

[George Wilkie]

On Wed, Apr 18, 2018 at 04:58:22PM +0200, Jiri Pirko wrote:
> Wed, Apr 18, 2018 at 03:35:49PM CEST, gwilkie@vyatta.att-mail.com wrote:
> >On Wed, Apr 18, 2018 at 02:56:44PM +0200, Jiri Pirko wrote:
> >> Wed, Apr 18, 2018 at 12:29:50PM CEST, gwilkie@vyatta.att-mail.com wrote:
> >> >Account for operational state when determining port linkup state,
> >> >as per Documentation/networking/operstates.txt.
> >> 
> >> Could you please point me to the exact place in the document where this
> >> is suggested?
> >> 
> >
> >Various places cover it I think.
> >
> >In 1. Introduction:
> >"interface is not usable just because the admin enabled it"
> >"userspace must be granted the possibility to
> >influence operational state"
> >
> >In 4. Setting from userspace:
> >"the userspace application can set IFLA_OPERSTATE
> >to IF_OPER_DORMANT or IF_OPER_UP as long as the driver does not set
> >netif_carrier_off() or netif_dormant_on()"
> >
> >We have a use case where we want to set the oper state of the team ports based
> >on whether they are actually usable or not (as opposed to just admin up).
> 
> Are you running a supplicant there or what is the use-case?
> 

We are using tun/tap interfaces for the team ports with the physical interfaces
under the control of a user process.

> How is this handle in other drivers like bond, openvswitch, bridge, etc?

It looks like bridge is using it, looking at br_port_carrier_check() and
br_add_if().

Cheers.

> 
> >
> >Cheers.
> >
> >> 
> >> >
> >> >Signed-off-by: George Wilkie <gwilkie@vyatta.att-mail.com>
> >> >---
> >> > drivers/net/team/team.c | 3 ++-
> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >> >
> >> >diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> >> >index a6c6ce19eeee..231264a05e55 100644
> >> >--- a/drivers/net/team/team.c
> >> >+++ b/drivers/net/team/team.c
> >> >@@ -2918,7 +2918,8 @@ static int team_device_event(struct notifier_block *unused,
> >> > 	case NETDEV_CHANGE:
> >> > 		if (netif_running(port->dev))
> >> > 			team_port_change_check(port,
> >> >-					       !!netif_carrier_ok(port->dev));
> >> >+					       !!(netif_carrier_ok(port->dev) &&
> >> >+						  netif_oper_up(port->dev)));
> >> > 		break;
> >> > 	case NETDEV_UNREGISTER:
> >> > 		team_del_slave(port->team->dev, dev);
> >> >-- 
> >> >2.11.0
> >> >

Re: [PATCH net-next] team: account for oper state

[George Wilkie]

On Wed, Apr 18, 2018 at 04:33:12PM +0100, George Wilkie wrote:
> On Wed, Apr 18, 2018 at 04:58:22PM +0200, Jiri Pirko wrote:
> > Wed, Apr 18, 2018 at 03:35:49PM CEST, gwilkie@vyatta.att-mail.com wrote:
> > >On Wed, Apr 18, 2018 at 02:56:44PM +0200, Jiri Pirko wrote:
> > >> Wed, Apr 18, 2018 at 12:29:50PM CEST, gwilkie@vyatta.att-mail.com wrote:
> > >> >Account for operational state when determining port linkup state,
> > >> >as per Documentation/networking/operstates.txt.
> > >> 
> > >> Could you please point me to the exact place in the document where this
> > >> is suggested?
> > >> 
> > >
> > >Various places cover it I think.
> > >
> > >In 1. Introduction:
> > >"interface is not usable just because the admin enabled it"
> > >"userspace must be granted the possibility to
> > >influence operational state"
> > >
> > >In 4. Setting from userspace:
> > >"the userspace application can set IFLA_OPERSTATE
> > >to IF_OPER_DORMANT or IF_OPER_UP as long as the driver does not set
> > >netif_carrier_off() or netif_dormant_on()"
> > >
> > >We have a use case where we want to set the oper state of the team ports based
> > >on whether they are actually usable or not (as opposed to just admin up).
> > 
> > Are you running a supplicant there or what is the use-case?
> > 
> 
> We are using tun/tap interfaces for the team ports with the physical interfaces
> under the control of a user process.
> 
> > How is this handle in other drivers like bond, openvswitch, bridge, etc?
> 
> It looks like bridge is using it, looking at br_port_carrier_check() and
> br_add_if().
> 

commit 576eb62598f10c8c7fd75703fe89010cdcfff596
Author:     stephen hemminger <shemminger@vyatta.com>
AuthorDate: Fri Dec 28 18:15:22 2012 +0000
Commit:     David S. Miller <davem@davemloft.net>
CommitDate: Sun Dec 30 02:31:43 2012 -0800

    bridge: respect RFC2863 operational state
    
    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.

> Cheers.
> 
> > 
> > >
> > >Cheers.
> > >
> > >> 
> > >> >
> > >> >Signed-off-by: George Wilkie <gwilkie@vyatta.att-mail.com>
> > >> >---
> > >> > drivers/net/team/team.c | 3 ++-
> > >> > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >> >
> > >> >diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> > >> >index a6c6ce19eeee..231264a05e55 100644
> > >> >--- a/drivers/net/team/team.c
> > >> >+++ b/drivers/net/team/team.c
> > >> >@@ -2918,7 +2918,8 @@ static int team_device_event(struct notifier_block *unused,
> > >> > 	case NETDEV_CHANGE:
> > >> > 		if (netif_running(port->dev))
> > >> > 			team_port_change_check(port,
> > >> >-					       !!netif_carrier_ok(port->dev));
> > >> >+					       !!(netif_carrier_ok(port->dev) &&
> > >> >+						  netif_oper_up(port->dev)));
> > >> > 		break;
> > >> > 	case NETDEV_UNREGISTER:
> > >> > 		team_del_slave(port->team->dev, dev);
> > >> >-- 
> > >> >2.11.0
> > >> >

Re: [PATCH net-next] team: account for oper state

[Jiri Pirko]

Wed, Apr 18, 2018 at 05:33:12PM CEST, gwilkie@vyatta.att-mail.com wrote:
>On Wed, Apr 18, 2018 at 04:58:22PM +0200, Jiri Pirko wrote:
>> Wed, Apr 18, 2018 at 03:35:49PM CEST, gwilkie@vyatta.att-mail.com wrote:
>> >On Wed, Apr 18, 2018 at 02:56:44PM +0200, Jiri Pirko wrote:
>> >> Wed, Apr 18, 2018 at 12:29:50PM CEST, gwilkie@vyatta.att-mail.com wrote:
>> >> >Account for operational state when determining port linkup state,
>> >> >as per Documentation/networking/operstates.txt.
>> >> 
>> >> Could you please point me to the exact place in the document where this
>> >> is suggested?
>> >> 
>> >
>> >Various places cover it I think.
>> >
>> >In 1. Introduction:
>> >"interface is not usable just because the admin enabled it"
>> >"userspace must be granted the possibility to
>> >influence operational state"
>> >
>> >In 4. Setting from userspace:
>> >"the userspace application can set IFLA_OPERSTATE
>> >to IF_OPER_DORMANT or IF_OPER_UP as long as the driver does not set
>> >netif_carrier_off() or netif_dormant_on()"
>> >
>> >We have a use case where we want to set the oper state of the team ports based
>> >on whether they are actually usable or not (as opposed to just admin up).
>> 
>> Are you running a supplicant there or what is the use-case?
>> 
>
>We are using tun/tap interfaces for the team ports with the physical interfaces
>under the control of a user process.
>
>> How is this handle in other drivers like bond, openvswitch, bridge, etc?
>
>It looks like bridge is using it, looking at br_port_carrier_check() and
>br_add_if().

Okay, so why do you still need to check netif_carrier_ok?
Looks like netif_oper_up is enough, right?


>
>Cheers.
>
>> 
>> >
>> >Cheers.
>> >
>> >> 
>> >> >
>> >> >Signed-off-by: George Wilkie <gwilkie@vyatta.att-mail.com>
>> >> >---
>> >> > drivers/net/team/team.c | 3 ++-
>> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >> >
>> >> >diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>> >> >index a6c6ce19eeee..231264a05e55 100644
>> >> >--- a/drivers/net/team/team.c
>> >> >+++ b/drivers/net/team/team.c
>> >> >@@ -2918,7 +2918,8 @@ static int team_device_event(struct notifier_block *unused,
>> >> > 	case NETDEV_CHANGE:
>> >> > 		if (netif_running(port->dev))
>> >> > 			team_port_change_check(port,
>> >> >-					       !!netif_carrier_ok(port->dev));
>> >> >+					       !!(netif_carrier_ok(port->dev) &&
>> >> >+						  netif_oper_up(port->dev)));
>> >> > 		break;
>> >> > 	case NETDEV_UNREGISTER:
>> >> > 		team_del_slave(port->team->dev, dev);
>> >> >-- 
>> >> >2.11.0
>> >> >

Re: [PATCH net-next] team: account for oper state

[George Wilkie]

On Wed, Apr 18, 2018 at 09:17:32PM +0200, Jiri Pirko wrote:
> Wed, Apr 18, 2018 at 05:33:12PM CEST, gwilkie@vyatta.att-mail.com wrote:
> >On Wed, Apr 18, 2018 at 04:58:22PM +0200, Jiri Pirko wrote:
> >> Wed, Apr 18, 2018 at 03:35:49PM CEST, gwilkie@vyatta.att-mail.com wrote:
> >> >On Wed, Apr 18, 2018 at 02:56:44PM +0200, Jiri Pirko wrote:
> >> >> Wed, Apr 18, 2018 at 12:29:50PM CEST, gwilkie@vyatta.att-mail.com wrote:
> >> >> >Account for operational state when determining port linkup state,
> >> >> >as per Documentation/networking/operstates.txt.
> >> >> 
> >> >> Could you please point me to the exact place in the document where this
> >> >> is suggested?
> >> >> 
> >> >
> >> >Various places cover it I think.
> >> >
> >> >In 1. Introduction:
> >> >"interface is not usable just because the admin enabled it"
> >> >"userspace must be granted the possibility to
> >> >influence operational state"
> >> >
> >> >In 4. Setting from userspace:
> >> >"the userspace application can set IFLA_OPERSTATE
> >> >to IF_OPER_DORMANT or IF_OPER_UP as long as the driver does not set
> >> >netif_carrier_off() or netif_dormant_on()"
> >> >
> >> >We have a use case where we want to set the oper state of the team ports based
> >> >on whether they are actually usable or not (as opposed to just admin up).
> >> 
> >> Are you running a supplicant there or what is the use-case?
> >> 
> >
> >We are using tun/tap interfaces for the team ports with the physical interfaces
> >under the control of a user process.
> >
> >> How is this handle in other drivers like bond, openvswitch, bridge, etc?
> >
> >It looks like bridge is using it, looking at br_port_carrier_check() and
> >br_add_if().
> 
> Okay, so why do you still need to check netif_carrier_ok?
> Looks like netif_oper_up is enough, right?

Yes, I was being overly cautious. Replacing netif_carrier_ok with netif_oper_up
works OK. I'll send updated patch.

Cheers.


> 
> 
> >
> >Cheers.
> >
> >> 
> >> >
> >> >Cheers.
> >> >
> >> >> 
> >> >> >
> >> >> >Signed-off-by: George Wilkie <gwilkie@vyatta.att-mail.com>
> >> >> >---
> >> >> > drivers/net/team/team.c | 3 ++-
> >> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >> >> >
> >> >> >diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> >> >> >index a6c6ce19eeee..231264a05e55 100644
> >> >> >--- a/drivers/net/team/team.c
> >> >> >+++ b/drivers/net/team/team.c
> >> >> >@@ -2918,7 +2918,8 @@ static int team_device_event(struct notifier_block *unused,
> >> >> > 	case NETDEV_CHANGE:
> >> >> > 		if (netif_running(port->dev))
> >> >> > 			team_port_change_check(port,
> >> >> >-					       !!netif_carrier_ok(port->dev));
> >> >> >+					       !!(netif_carrier_ok(port->dev) &&
> >> >> >+						  netif_oper_up(port->dev)));
> >> >> > 		break;
> >> >> > 	case NETDEV_UNREGISTER:
> >> >> > 		team_del_slave(port->team->dev, dev);
> >> >> >-- 
> >> >> >2.11.0
> >> >> >
> 
>