[PATCHv2 net] team: team_port_add should check link_up before enable port

[PATCHv2 net] team: team_port_add should check link_up before enable port

[Xin Long]

Now when users add a nic to team dev, the option 'enable' of the port
is true by default, as team_port_enable enables it after dev_open in
team_port_add.

But even if the port_dev has no carrier, like it's cable was unpluged,
the port is still enabled. It leads to that team dev couldn't work well
if this port was chosen to connect, and has no chance to change to use
other ports if link_watch is ethtool.

This patch is to enable the port only when the port_dev has carrier in
team_port_add.

v1 -> v2:
  use netif_carrier_ok() instead of !!netif_carrier_ok(), as it returns
  bool now.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 drivers/net/team/team.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index a380649..4bc0103 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -1140,6 +1140,7 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
 	struct net_device *dev = team->dev;
 	struct team_port *port;
 	char *portname = port_dev->name;
+	bool linkup;
 	int err;
 
 	if (port_dev->flags & IFF_LOOPBACK) {
@@ -1249,9 +1250,12 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
 
 	port->index = -1;
 	list_add_tail_rcu(&port->list, &team->port_list);
-	team_port_enable(team, port);
+	linkup = netif_carrier_ok(port_dev);
+	if (linkup)
+		team_port_enable(team, port);
+
 	__team_compute_features(team);
-	__team_port_change_port_added(port, !!netif_carrier_ok(port_dev));
+	__team_port_change_port_added(port, linkup);
 	__team_options_change_check(team);
 
 	netdev_info(dev, "Port device %s added\n", portname);
-- 
2.1.0

Re: [PATCHv2 net] team: team_port_add should check link_up before enable port

[Jiri Pirko]

Tue, Dec 06, 2016 at 08:29:08AM CET, lucien.xin@gmail.com wrote:
>Now when users add a nic to team dev, the option 'enable' of the port
>is true by default, as team_port_enable enables it after dev_open in
>team_port_add.
>
>But even if the port_dev has no carrier, like it's cable was unpluged,
>the port is still enabled. It leads to that team dev couldn't work well
>if this port was chosen to connect, and has no chance to change to use
>other ports if link_watch is ethtool.
>
>This patch is to enable the port only when the port_dev has carrier in
>team_port_add.
>
>v1 -> v2:
>  use netif_carrier_ok() instead of !!netif_carrier_ok(), as it returns
>  bool now.
>
>Signed-off-by: Xin Long <lucien.xin@gmail.com>
>---
> drivers/net/team/team.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>index a380649..4bc0103 100644
>--- a/drivers/net/team/team.c
>+++ b/drivers/net/team/team.c
>@@ -1140,6 +1140,7 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
> 	struct net_device *dev = team->dev;
> 	struct team_port *port;
> 	char *portname = port_dev->name;
>+	bool linkup;
> 	int err;
> 
> 	if (port_dev->flags & IFF_LOOPBACK) {
>@@ -1249,9 +1250,12 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
> 
> 	port->index = -1;
> 	list_add_tail_rcu(&port->list, &team->port_list);
>-	team_port_enable(team, port);
>+	linkup = netif_carrier_ok(port_dev);
>+	if (linkup)
>+		team_port_enable(team, port);

This is obviously wrong change. It you use a simple setup without
userspace part (e.g. round robin), The port gets never enabled.
team_port_enabl is called from here and team_port_en_option_set.

By default, all ports should be enabled. Only in case the userspace
daemon decides to disable, it does so.

Could you send me the exact configuration where you see the issue?
This should be definitelly fixed in user part.

Thanks.

Re: [PATCHv2 net] team: team_port_add should check link_up before enable port

[Xin Long]

[-- Attachment #1: Type: text/plain, Size: 2719 bytes --]

On Tue, Dec 6, 2016 at 4:22 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Tue, Dec 06, 2016 at 08:29:08AM CET, lucien.xin@gmail.com wrote:
>>Now when users add a nic to team dev, the option 'enable' of the port
>>is true by default, as team_port_enable enables it after dev_open in
>>team_port_add.
>>
>>But even if the port_dev has no carrier, like it's cable was unpluged,
>>the port is still enabled. It leads to that team dev couldn't work well
>>if this port was chosen to connect, and has no chance to change to use
>>other ports if link_watch is ethtool.
>>
>>This patch is to enable the port only when the port_dev has carrier in
>>team_port_add.
>>
>>v1 -> v2:
>>  use netif_carrier_ok() instead of !!netif_carrier_ok(), as it returns
>>  bool now.
>>
>>Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>---
>> drivers/net/team/team.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>>index a380649..4bc0103 100644
>>--- a/drivers/net/team/team.c
>>+++ b/drivers/net/team/team.c
>>@@ -1140,6 +1140,7 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
>>       struct net_device *dev = team->dev;
>>       struct team_port *port;
>>       char *portname = port_dev->name;
>>+      bool linkup;
>>       int err;
>>
>>       if (port_dev->flags & IFF_LOOPBACK) {
>>@@ -1249,9 +1250,12 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
>>
>>       port->index = -1;
>>       list_add_tail_rcu(&port->list, &team->port_list);
>>-      team_port_enable(team, port);
>>+      linkup = netif_carrier_ok(port_dev);
>>+      if (linkup)
>>+              team_port_enable(team, port);
>
> This is obviously wrong change. It you use a simple setup without
> userspace part (e.g. round robin), The port gets never enabled.
> team_port_enabl is called from here and team_port_en_option_set.
yes, without userspace part, that would be a problem.

>
> By default, all ports should be enabled. Only in case the userspace
> daemon decides to disable, it does so.
>
> Could you send me the exact configuration where you see the issue?
attachment is the scripts,

# ./setup.sh
# ip netns exec ns1 ./team1.sh
# ./team0.sh
# ping 192.168.11.3

the issue is team0 cannot switch to veth2, even if the veth0 has no carrier.

> This should be definitelly fixed in user part.
now it can disable/enable it in lw_ethtool_event_watch_port_changed()
when receive event from kernel, but at the beginning, the first event from
adding port will not going to team_port_en_option_set, as new_link_up
== common_ppriv->link_up in lw_ethtool_event_watch_port_changed().

maybe it should be fixed there ?

>
> Thanks.

[-- Attachment #2: team-scripts.tar.gz --]
[-- Type: application/x-gzip, Size: 561 bytes --]

Re: [PATCHv2 net] team: team_port_add should check link_up before enable port

[Jiri Pirko]

Tue, Dec 06, 2016 at 11:38:53AM CET, lucien.xin@gmail.com wrote:
>On Tue, Dec 6, 2016 at 4:22 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Tue, Dec 06, 2016 at 08:29:08AM CET, lucien.xin@gmail.com wrote:
>>>Now when users add a nic to team dev, the option 'enable' of the port
>>>is true by default, as team_port_enable enables it after dev_open in
>>>team_port_add.
>>>
>>>But even if the port_dev has no carrier, like it's cable was unpluged,
>>>the port is still enabled. It leads to that team dev couldn't work well
>>>if this port was chosen to connect, and has no chance to change to use
>>>other ports if link_watch is ethtool.
>>>
>>>This patch is to enable the port only when the port_dev has carrier in
>>>team_port_add.
>>>
>>>v1 -> v2:
>>>  use netif_carrier_ok() instead of !!netif_carrier_ok(), as it returns
>>>  bool now.
>>>
>>>Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>>---
>>> drivers/net/team/team.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>>>index a380649..4bc0103 100644
>>>--- a/drivers/net/team/team.c
>>>+++ b/drivers/net/team/team.c
>>>@@ -1140,6 +1140,7 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
>>>       struct net_device *dev = team->dev;
>>>       struct team_port *port;
>>>       char *portname = port_dev->name;
>>>+      bool linkup;
>>>       int err;
>>>
>>>       if (port_dev->flags & IFF_LOOPBACK) {
>>>@@ -1249,9 +1250,12 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
>>>
>>>       port->index = -1;
>>>       list_add_tail_rcu(&port->list, &team->port_list);
>>>-      team_port_enable(team, port);
>>>+      linkup = netif_carrier_ok(port_dev);
>>>+      if (linkup)
>>>+              team_port_enable(team, port);
>>
>> This is obviously wrong change. It you use a simple setup without
>> userspace part (e.g. round robin), The port gets never enabled.
>> team_port_enabl is called from here and team_port_en_option_set.
>yes, without userspace part, that would be a problem.
>
>>
>> By default, all ports should be enabled. Only in case the userspace
>> daemon decides to disable, it does so.
>>
>> Could you send me the exact configuration where you see the issue?
>attachment is the scripts,
>
># ./setup.sh
># ip netns exec ns1 ./team1.sh
># ./team0.sh
># ping 192.168.11.3
>
>the issue is team0 cannot switch to veth2, even if the veth0 has no carrier.
>
>> This should be definitelly fixed in user part.
>now it can disable/enable it in lw_ethtool_event_watch_port_changed()
>when receive event from kernel, but at the beginning, the first event from
>adding port will not going to team_port_en_option_set, as new_link_up
>== common_ppriv->link_up in lw_ethtool_event_watch_port_changed().
>
>maybe it should be fixed there ?

Yes please.

>
>>
>> Thanks.