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

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

[Jiri Pirko]

This is basically a repost of my previous patchset:
"[patch net-next-2.6 0/2] net: allow to change carrier via sysfs" from Aug 30

The way net-sysfs stores values changed and this patchset reflects it.
Also, I exposed carrier via rtnetlink iface.

So far, only dummy driver uses carrier change ndo. In very near future
team driver will use that as well.

Jiri Pirko (4):
  net: add change_carrier netdev op
  net: allow to change carrier via sysfs
  rtnl: expose carrier value with possibility to set it
  dummy: implement carrier change

 drivers/net/dummy.c          | 10 ++++++++++
 include/linux/netdevice.h    |  7 +++++++
 include/uapi/linux/if_link.h |  1 +
 net/core/dev.c               | 19 +++++++++++++++++++
 net/core/net-sysfs.c         | 15 ++++++++++++++-
 net/core/rtnetlink.c         | 10 ++++++++++
 6 files changed, 61 insertions(+), 1 deletion(-)

-- 
1.8.0

[patch net-next 1/4] net: add change_carrier netdev op

[Jiri Pirko]

This allows a driver to register change_carrier callback which will be
called whenever user will like to change carrier state. This is useful
for devices like dummy, gre, team and so on.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 include/linux/netdevice.h |  7 +++++++
 net/core/dev.c            | 19 +++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c6a14d4..e1a5c16 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -891,6 +891,9 @@ struct netdev_fcoe_hbainfo {
  * int (*ndo_bridge_setlink)(struct net_device *dev, struct nlmsghdr *nlh)
  * int (*ndo_bridge_getlink)(struct sk_buff *skb, u32 pid, u32 seq,
  *			     struct net_device *dev)
+ *
+ * int (*ndo_change_carrier)(struct net_device *dev, bool new_carrier);
+ *	Called to update device carrier.
  */
 struct net_device_ops {
 	int			(*ndo_init)(struct net_device *dev);
@@ -1008,6 +1011,8 @@ struct net_device_ops {
 	int			(*ndo_bridge_getlink)(struct sk_buff *skb,
 						      u32 pid, u32 seq,
 						      struct net_device *dev);
+	int			(*ndo_change_carrier)(struct net_device *dev,
+						      bool new_carrier);
 };
 
 /*
@@ -2191,6 +2196,8 @@ extern int		dev_set_mtu(struct net_device *, int);
 extern void		dev_set_group(struct net_device *, int);
 extern int		dev_set_mac_address(struct net_device *,
 					    struct sockaddr *);
+extern int		dev_change_carrier(struct net_device *,
+					   bool new_carrier);
 extern int		dev_hard_start_xmit(struct sk_buff *skb,
 					    struct net_device *dev,
 					    struct netdev_queue *txq);
diff --git a/net/core/dev.c b/net/core/dev.c
index 4783850..cc6426b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5025,6 +5025,25 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
 }
 EXPORT_SYMBOL(dev_set_mac_address);
 
+/**
+ *	dev_change_carrier - Change device carrier
+ *	@dev: device
+ *	@new_carries: new value
+ *
+ *	Change device carrier
+ */
+int dev_change_carrier(struct net_device *dev, bool new_carrier)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+
+	if (!ops->ndo_change_carrier)
+		return -EOPNOTSUPP;
+	if (!netif_device_present(dev))
+		return -ENODEV;
+	return ops->ndo_change_carrier(dev, new_carrier);
+}
+EXPORT_SYMBOL(dev_change_carrier);
+
 /*
  *	Perform the SIOCxIFxxx calls, inside rcu_read_lock()
  */
-- 
1.8.0

[patch net-next 2/4] net: allow to change carrier via sysfs

[Jiri Pirko]

Make carrier writable

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/core/net-sysfs.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 334efd5..7eda40a 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -126,6 +126,19 @@ static ssize_t show_broadcast(struct device *dev,
 	return -EINVAL;
 }
 
+static int change_carrier(struct net_device *net, unsigned long new_carrier)
+{
+	if (!netif_running(net))
+		return -EINVAL;
+	return dev_change_carrier(net, (bool) new_carrier);
+}
+
+static ssize_t store_carrier(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t len)
+{
+	return netdev_store(dev, attr, buf, len, change_carrier);
+}
+
 static ssize_t show_carrier(struct device *dev,
 			    struct device_attribute *attr, char *buf)
 {
@@ -331,7 +344,7 @@ static struct device_attribute net_class_attributes[] = {
 	__ATTR(link_mode, S_IRUGO, show_link_mode, NULL),
 	__ATTR(address, S_IRUGO, show_address, NULL),
 	__ATTR(broadcast, S_IRUGO, show_broadcast, NULL),
-	__ATTR(carrier, S_IRUGO, show_carrier, NULL),
+	__ATTR(carrier, S_IRUGO | S_IWUSR, show_carrier, store_carrier),
 	__ATTR(speed, S_IRUGO, show_speed, NULL),
 	__ATTR(duplex, S_IRUGO, show_duplex, NULL),
 	__ATTR(dormant, S_IRUGO, show_dormant, NULL),
-- 
1.8.0

[patch net-next 3/4] rtnl: expose carrier value with possibility to set it

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 include/uapi/linux/if_link.h |  1 +
 net/core/rtnetlink.c         | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 60f3b6b..c4edfe1 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -142,6 +142,7 @@ enum {
 #define IFLA_PROMISCUITY IFLA_PROMISCUITY
 	IFLA_NUM_TX_QUEUES,
 	IFLA_NUM_RX_QUEUES,
+	IFLA_CARRIER,
 	__IFLA_MAX
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 1868625..2ef7a56 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -780,6 +780,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + nla_total_size(4) /* IFLA_MTU */
 	       + nla_total_size(4) /* IFLA_LINK */
 	       + nla_total_size(4) /* IFLA_MASTER */
+	       + nla_total_size(1) /* IFLA_CARRIER */
 	       + nla_total_size(4) /* IFLA_PROMISCUITY */
 	       + nla_total_size(4) /* IFLA_NUM_TX_QUEUES */
 	       + nla_total_size(4) /* IFLA_NUM_RX_QUEUES */
@@ -909,6 +910,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	     nla_put_u32(skb, IFLA_LINK, dev->iflink)) ||
 	    (dev->master &&
 	     nla_put_u32(skb, IFLA_MASTER, dev->master->ifindex)) ||
+	    nla_put_u8(skb, IFLA_CARRIER, netif_carrier_ok(dev)) ||
 	    (dev->qdisc &&
 	     nla_put_string(skb, IFLA_QDISC, dev->qdisc->ops->id)) ||
 	    (dev->ifalias &&
@@ -1108,6 +1110,7 @@ const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_MTU]		= { .type = NLA_U32 },
 	[IFLA_LINK]		= { .type = NLA_U32 },
 	[IFLA_MASTER]		= { .type = NLA_U32 },
+	[IFLA_CARRIER]		= { .type = NLA_U8 },
 	[IFLA_TXQLEN]		= { .type = NLA_U32 },
 	[IFLA_WEIGHT]		= { .type = NLA_U32 },
 	[IFLA_OPERSTATE]	= { .type = NLA_U8 },
@@ -1438,6 +1441,13 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 		modified = 1;
 	}
 
+	if (tb[IFLA_CARRIER]) {
+		err = dev_change_carrier(dev, nla_get_u8(tb[IFLA_CARRIER]));
+		if (err)
+			goto errout;
+		modified = 1;
+	}
+
 	if (tb[IFLA_TXQLEN])
 		dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
 
-- 
1.8.0

[patch net-next 4/4] dummy: implement carrier change

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 drivers/net/dummy.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index c260af5..42aa54a 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -100,6 +100,15 @@ static void dummy_dev_uninit(struct net_device *dev)
 	free_percpu(dev->dstats);
 }
 
+static int dummy_change_carrier(struct net_device *dev, bool new_carrier)
+{
+	if (new_carrier)
+		netif_carrier_on(dev);
+	else
+		netif_carrier_off(dev);
+	return 0;
+}
+
 static const struct net_device_ops dummy_netdev_ops = {
 	.ndo_init		= dummy_dev_init,
 	.ndo_uninit		= dummy_dev_uninit,
@@ -108,6 +117,7 @@ static const struct net_device_ops dummy_netdev_ops = {
 	.ndo_set_rx_mode	= set_multicast_list,
 	.ndo_set_mac_address	= eth_mac_addr,
 	.ndo_get_stats64	= dummy_get_stats64,
+	.ndo_change_carrier	= dummy_change_carrier,
 };
 
 static void dummy_setup(struct net_device *dev)
-- 
1.8.0

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Stephen Hemminger]

On Wed, 12 Dec 2012 11:58:03 +0100
Jiri Pirko <jiri@resnulli.us> wrote:

> This is basically a repost of my previous patchset:
> "[patch net-next-2.6 0/2] net: allow to change carrier via sysfs" from Aug 30
> 
> The way net-sysfs stores values changed and this patchset reflects it.
> Also, I exposed carrier via rtnetlink iface.
> 
> So far, only dummy driver uses carrier change ndo. In very near future
> team driver will use that as well.
> 
> Jiri Pirko (4):
>   net: add change_carrier netdev op
>   net: allow to change carrier via sysfs
>   rtnl: expose carrier value with possibility to set it
>   dummy: implement carrier change
> 
>  drivers/net/dummy.c          | 10 ++++++++++
>  include/linux/netdevice.h    |  7 +++++++
>  include/uapi/linux/if_link.h |  1 +
>  net/core/dev.c               | 19 +++++++++++++++++++
>  net/core/net-sysfs.c         | 15 ++++++++++++++-
>  net/core/rtnetlink.c         | 10 ++++++++++
>  6 files changed, 61 insertions(+), 1 deletion(-)
> 

I needed to do the same thing for a project we are working on and discovered
that there already is a working documented interface for doing that via
operstate mode. Therefore I can't recommend that the additional complexity
of a new API for this is required.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

Wed, Dec 12, 2012 at 05:15:00PM CET, shemminger@vyatta.com wrote:
>On Wed, 12 Dec 2012 11:58:03 +0100
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> This is basically a repost of my previous patchset:
>> "[patch net-next-2.6 0/2] net: allow to change carrier via sysfs" from Aug 30
>> 
>> The way net-sysfs stores values changed and this patchset reflects it.
>> Also, I exposed carrier via rtnetlink iface.
>> 
>> So far, only dummy driver uses carrier change ndo. In very near future
>> team driver will use that as well.
>> 
>> Jiri Pirko (4):
>>   net: add change_carrier netdev op
>>   net: allow to change carrier via sysfs
>>   rtnl: expose carrier value with possibility to set it
>>   dummy: implement carrier change
>> 
>>  drivers/net/dummy.c          | 10 ++++++++++
>>  include/linux/netdevice.h    |  7 +++++++
>>  include/uapi/linux/if_link.h |  1 +
>>  net/core/dev.c               | 19 +++++++++++++++++++
>>  net/core/net-sysfs.c         | 15 ++++++++++++++-
>>  net/core/rtnetlink.c         | 10 ++++++++++
>>  6 files changed, 61 insertions(+), 1 deletion(-)
>> 
>
>I needed to do the same thing for a project we are working on and discovered
>that there already is a working documented interface for doing that via
>operstate mode. Therefore I can't recommend that the additional complexity
>of a new API for this is required.

I might be missing something, but I'm unable to find how operstate set
can affect value returned by netif_carrier_ok()

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Stephen Hemminger]

On Wed, 12 Dec 2012 18:05:20 +0100
Jiri Pirko <jiri@resnulli.us> wrote:

> Wed, Dec 12, 2012 at 05:15:00PM CET, shemminger@vyatta.com wrote:
> >On Wed, 12 Dec 2012 11:58:03 +0100
> >Jiri Pirko <jiri@resnulli.us> wrote:
> >
> >> This is basically a repost of my previous patchset:
> >> "[patch net-next-2.6 0/2] net: allow to change carrier via sysfs" from Aug 30
> >> 
> >> The way net-sysfs stores values changed and this patchset reflects it.
> >> Also, I exposed carrier via rtnetlink iface.
> >> 
> >> So far, only dummy driver uses carrier change ndo. In very near future
> >> team driver will use that as well.
> >> 
> >> Jiri Pirko (4):
> >>   net: add change_carrier netdev op
> >>   net: allow to change carrier via sysfs
> >>   rtnl: expose carrier value with possibility to set it
> >>   dummy: implement carrier change
> >> 
> >>  drivers/net/dummy.c          | 10 ++++++++++
> >>  include/linux/netdevice.h    |  7 +++++++
> >>  include/uapi/linux/if_link.h |  1 +
> >>  net/core/dev.c               | 19 +++++++++++++++++++
> >>  net/core/net-sysfs.c         | 15 ++++++++++++++-
> >>  net/core/rtnetlink.c         | 10 ++++++++++
> >>  6 files changed, 61 insertions(+), 1 deletion(-)
> >> 
> >
> >I needed to do the same thing for a project we are working on and discovered
> >that there already is a working documented interface for doing that via
> >operstate mode. Therefore I can't recommend that the additional complexity
> >of a new API for this is required.
> 
> I might be missing something, but I'm unable to find how operstate set
> can affect value returned by netif_carrier_ok()
> --
> 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

Here is an example using dummy device using libmnl. It is also possible
with ip commands.

# modprobe dummy
# ip li show dev dummy0
12: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT 
    link/ether ce:90:46:83:6e:f8 brd ff:ff:ff:ff:ff:ff
# ./dummy dummy0 init
# ip li show dev dummy0
12: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DORMANT 
    link/ether ce:90:46:83:6e:f8 brd ff:ff:ff:ff:ff:ff
# ip li set dummy0 up
# ip li show dev dummy0
12: dummy0: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN mode DORMANT 
    link/ether ce:90:46:83:6e:f8 brd ff:ff:ff:ff:ff:ff
# ./dummy dummy0 down
# ip li show dev dummy0
12: dummy0: <NO-CARRIER,BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state DORMANT mode DORMANT 
    link/ether ce:90:46:83:6e:f8 brd ff:ff:ff:ff:ff:ff
# ./dummy dummy0 up
# ip li show dev dummy0
12: dummy0: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DORMANT 
    link/ether ce:90:46:83:6e:f8 brd ff:ff:ff:ff:ff:ff


/* Sample program to control link mode and link state */
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <time.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/fcntl.h>
#include <sys/ioctl.h>
#include <libmnl/libmnl.h>

#include <linux/if.h>
#include <linux/if_tun.h>
#include <linux/rtnetlink.h>

static void panic(const char *str)
{
	perror(str);
	exit(1);
}

static void usage(const char *cmd)
{
	fprintf(stderr, "Usage: %s dummyX [up|down|init]\n", cmd);
	exit(1);
}

/* Send request and parse response */
static void mnl_talk(struct mnl_socket *nl, struct nlmsghdr *nlh)
{
	unsigned portid = mnl_socket_get_portid(nl);
	uint32_t seq = time(NULL);
	char buf[MNL_SOCKET_BUFFER_SIZE];

	nlh->nlmsg_flags |= NLM_F_ACK;
	nlh->nlmsg_seq = seq;

	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0)
		panic("mnl_socket_sendto failed");

	int ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
	if (ret < 0)
		panic("mnl_socket_recvfrom");

	if ( mnl_cb_run(buf, ret, seq, portid, NULL, NULL) < 0)
		panic("mnl_cb_run");
}

static void linkstate(struct mnl_socket *nl,
		      const char *ifname, unsigned int state)
{
	char buf[MNL_SOCKET_BUFFER_SIZE];
	struct nlmsghdr *nlh = mnl_nlmsg_put_header(buf);
	nlh->nlmsg_type = RTM_NEWLINK;
	nlh->nlmsg_flags = NLM_F_REQUEST;

	struct ifinfomsg *ifi;
	ifi = mnl_nlmsg_put_extra_header(nlh, sizeof(struct ifinfomsg));
	ifi->ifi_family = AF_UNSPEC;

	mnl_attr_put_strz(nlh, IFLA_IFNAME, ifname);
	mnl_attr_put_u8(nlh, IFLA_OPERSTATE, state);

	mnl_talk(nl, nlh);
}

/* Set device link mode */
static void init(struct mnl_socket *nl, const char *ifname)
{
	char buf[MNL_SOCKET_BUFFER_SIZE];
	struct nlmsghdr *nlh = mnl_nlmsg_put_header(buf);
	nlh->nlmsg_type = RTM_NEWLINK;
	nlh->nlmsg_flags = NLM_F_REQUEST;

	struct ifinfomsg *ifi;
	ifi = mnl_nlmsg_put_extra_header(nlh, sizeof(struct ifinfomsg));
	ifi->ifi_family = AF_UNSPEC;
	
	mnl_attr_put_strz(nlh, IFLA_IFNAME, ifname);
	mnl_attr_put_u8(nlh, IFLA_LINKMODE, IF_LINK_MODE_DORMANT);
	mnl_talk(nl, nlh);
}

int main(int argc, char **argv)
{
	if (argc != 3)
		usage(argv[0]);

	struct mnl_socket *nl = mnl_socket_open(NETLINK_ROUTE);
	if (!nl)
		panic("mnl_socket_open");

	if (mnl_socket_bind(nl, 0, MNL_SOCKET_AUTOPID) < 0)
		panic("mnl_socket_bind");
	

	if (strcmp(argv[2], "init") == 0)
		init(nl, argv[1]);
	else if (strcmp(argv[2], "up") == 0)
		linkstate(nl, argv[1], IF_OPER_UP);
	else if (strcmp(argv[2], "down") == 0)
		linkstate(nl, argv[1], IF_OPER_DORMANT);
	else
		usage(argv[0]);

	return 0;
}

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

Wed, Dec 12, 2012 at 06:27:00PM CET, shemminger@vyatta.com wrote:
>On Wed, 12 Dec 2012 18:05:20 +0100
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Wed, Dec 12, 2012 at 05:15:00PM CET, shemminger@vyatta.com wrote:
>> >On Wed, 12 Dec 2012 11:58:03 +0100
>> >Jiri Pirko <jiri@resnulli.us> wrote:
>> >
>> >> This is basically a repost of my previous patchset:
>> >> "[patch net-next-2.6 0/2] net: allow to change carrier via sysfs" from Aug 30
>> >> 
>> >> The way net-sysfs stores values changed and this patchset reflects it.
>> >> Also, I exposed carrier via rtnetlink iface.
>> >> 
>> >> So far, only dummy driver uses carrier change ndo. In very near future
>> >> team driver will use that as well.
>> >> 
>> >> Jiri Pirko (4):
>> >>   net: add change_carrier netdev op
>> >>   net: allow to change carrier via sysfs
>> >>   rtnl: expose carrier value with possibility to set it
>> >>   dummy: implement carrier change
>> >> 
>> >>  drivers/net/dummy.c          | 10 ++++++++++
>> >>  include/linux/netdevice.h    |  7 +++++++
>> >>  include/uapi/linux/if_link.h |  1 +
>> >>  net/core/dev.c               | 19 +++++++++++++++++++
>> >>  net/core/net-sysfs.c         | 15 ++++++++++++++-
>> >>  net/core/rtnetlink.c         | 10 ++++++++++
>> >>  6 files changed, 61 insertions(+), 1 deletion(-)
>> >> 
>> >
>> >I needed to do the same thing for a project we are working on and discovered
>> >that there already is a working documented interface for doing that via
>> >operstate mode. Therefore I can't recommend that the additional complexity
>> >of a new API for this is required.
>> 
>> I might be missing something, but I'm unable to find how operstate set
>> can affect value returned by netif_carrier_ok()
>> --
>> 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
>
>Here is an example using dummy device using libmnl. It is also possible
>with ip commands.
>
># modprobe dummy
># ip li show dev dummy0
>12: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT 
>    link/ether ce:90:46:83:6e:f8 brd ff:ff:ff:ff:ff:ff
># ./dummy dummy0 init
># ip li show dev dummy0
>12: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DORMANT 
>    link/ether ce:90:46:83:6e:f8 brd ff:ff:ff:ff:ff:ff
># ip li set dummy0 up
># ip li show dev dummy0
>12: dummy0: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN mode DORMANT 
>    link/ether ce:90:46:83:6e:f8 brd ff:ff:ff:ff:ff:ff
># ./dummy dummy0 down
># ip li show dev dummy0
>12: dummy0: <NO-CARRIER,BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state DORMANT mode DORMANT 

if you mean this "NO-CARRIER"
it has no direct relation with netif_carrier_ok().


>    link/ether ce:90:46:83:6e:f8 brd ff:ff:ff:ff:ff:ff
># ./dummy dummy0 up
># ip li show dev dummy0
>12: dummy0: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DORMANT 
>    link/ether ce:90:46:83:6e:f8 brd ff:ff:ff:ff:ff:ff
>
>
>/* Sample program to control link mode and link state */
>#include <stdio.h>
>#include <stdlib.h>
>#include <unistd.h>
>#include <string.h>
>#include <time.h>
>#include <errno.h>
>#include <sys/types.h>
>#include <sys/fcntl.h>
>#include <sys/ioctl.h>
>#include <libmnl/libmnl.h>
>
>#include <linux/if.h>
>#include <linux/if_tun.h>
>#include <linux/rtnetlink.h>
>
>static void panic(const char *str)
>{
>	perror(str);
>	exit(1);
>}
>
>static void usage(const char *cmd)
>{
>	fprintf(stderr, "Usage: %s dummyX [up|down|init]\n", cmd);
>	exit(1);
>}
>
>/* Send request and parse response */
>static void mnl_talk(struct mnl_socket *nl, struct nlmsghdr *nlh)
>{
>	unsigned portid = mnl_socket_get_portid(nl);
>	uint32_t seq = time(NULL);
>	char buf[MNL_SOCKET_BUFFER_SIZE];
>
>	nlh->nlmsg_flags |= NLM_F_ACK;
>	nlh->nlmsg_seq = seq;
>
>	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0)
>		panic("mnl_socket_sendto failed");
>
>	int ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
>	if (ret < 0)
>		panic("mnl_socket_recvfrom");
>
>	if ( mnl_cb_run(buf, ret, seq, portid, NULL, NULL) < 0)
>		panic("mnl_cb_run");
>}
>
>static void linkstate(struct mnl_socket *nl,
>		      const char *ifname, unsigned int state)
>{
>	char buf[MNL_SOCKET_BUFFER_SIZE];
>	struct nlmsghdr *nlh = mnl_nlmsg_put_header(buf);
>	nlh->nlmsg_type = RTM_NEWLINK;
>	nlh->nlmsg_flags = NLM_F_REQUEST;
>
>	struct ifinfomsg *ifi;
>	ifi = mnl_nlmsg_put_extra_header(nlh, sizeof(struct ifinfomsg));
>	ifi->ifi_family = AF_UNSPEC;
>
>	mnl_attr_put_strz(nlh, IFLA_IFNAME, ifname);
>	mnl_attr_put_u8(nlh, IFLA_OPERSTATE, state);
>
>	mnl_talk(nl, nlh);
>}
>
>/* Set device link mode */
>static void init(struct mnl_socket *nl, const char *ifname)
>{
>	char buf[MNL_SOCKET_BUFFER_SIZE];
>	struct nlmsghdr *nlh = mnl_nlmsg_put_header(buf);
>	nlh->nlmsg_type = RTM_NEWLINK;
>	nlh->nlmsg_flags = NLM_F_REQUEST;
>
>	struct ifinfomsg *ifi;
>	ifi = mnl_nlmsg_put_extra_header(nlh, sizeof(struct ifinfomsg));
>	ifi->ifi_family = AF_UNSPEC;
>	
>	mnl_attr_put_strz(nlh, IFLA_IFNAME, ifname);
>	mnl_attr_put_u8(nlh, IFLA_LINKMODE, IF_LINK_MODE_DORMANT);
>	mnl_talk(nl, nlh);
>}
>
>int main(int argc, char **argv)
>{
>	if (argc != 3)
>		usage(argv[0]);
>
>	struct mnl_socket *nl = mnl_socket_open(NETLINK_ROUTE);
>	if (!nl)
>		panic("mnl_socket_open");
>
>	if (mnl_socket_bind(nl, 0, MNL_SOCKET_AUTOPID) < 0)
>		panic("mnl_socket_bind");
>	
>
>	if (strcmp(argv[2], "init") == 0)
>		init(nl, argv[1]);
>	else if (strcmp(argv[2], "up") == 0)
>		linkstate(nl, argv[1], IF_OPER_UP);
>	else if (strcmp(argv[2], "down") == 0)
>		linkstate(nl, argv[1], IF_OPER_DORMANT);
>	else
>		usage(argv[0]);
>
>	return 0;
>}
>
>

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Stephen Hemminger]

On Wed, 12 Dec 2012 19:10:17 +0100
Jiri Pirko <jiri@resnulli.us> wrote:

> ># ip li show dev dummy0
> >12: dummy0: <NO-CARRIER,BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state DORMANT mode DORMANT   
> 
> if you mean this "NO-CARRIER"
> it has no direct relation with netif_carrier_ok().

It is the same value (IFF_RUNNING) that is visible from user space.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

Wed, Dec 12, 2012 at 07:12:08PM CET, shemminger@vyatta.com wrote:
>On Wed, 12 Dec 2012 19:10:17 +0100
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> ># ip li show dev dummy0
>> >12: dummy0: <NO-CARRIER,BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state DORMANT mode DORMANT   
>> 
>> if you mean this "NO-CARRIER"
>> it has no direct relation with netif_carrier_ok().
>
>It is the same value (IFF_RUNNING) that is visible from user space.

static inline bool netif_carrier_ok(const struct net_device *dev)
{
	        return !test_bit(__LINK_STATE_NOCARRIER, &dev->state);
}

So netif_carrier[ok/on/off] are working with on __LINK_STATE_NOCARRIER
bit. Not with IFF_RUNNING flag.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Stephen Hemminger]

On Wed, 12 Dec 2012 19:25:56 +0100
Jiri Pirko <jiri@resnulli.us> wrote:

> Wed, Dec 12, 2012 at 07:12:08PM CET, shemminger@vyatta.com wrote:
> >On Wed, 12 Dec 2012 19:10:17 +0100
> >Jiri Pirko <jiri@resnulli.us> wrote:
> >
> >> ># ip li show dev dummy0
> >> >12: dummy0: <NO-CARRIER,BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state DORMANT mode DORMANT   
> >> 
> >> if you mean this "NO-CARRIER"
> >> it has no direct relation with netif_carrier_ok().
> >
> >It is the same value (IFF_RUNNING) that is visible from user space.
> 
> static inline bool netif_carrier_ok(const struct net_device *dev)
> {
> 	        return !test_bit(__LINK_STATE_NOCARRIER, &dev->state);
> }
> 
> So netif_carrier[ok/on/off] are working with on __LINK_STATE_NOCARRIER
> bit. Not with IFF_RUNNING flag.

What is the code path that you are worried about netif_carrier_ok being set or clear?
The interaction here is complex, and right now LINK_STATE_NOCARRIER is purely
controlled by the driver, your patch changes that, but before acking I want
to make sure why it is required.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

Wed, Dec 12, 2012 at 07:36:32PM CET, shemminger@vyatta.com wrote:
>On Wed, 12 Dec 2012 19:25:56 +0100
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Wed, Dec 12, 2012 at 07:12:08PM CET, shemminger@vyatta.com wrote:
>> >On Wed, 12 Dec 2012 19:10:17 +0100
>> >Jiri Pirko <jiri@resnulli.us> wrote:
>> >
>> >> ># ip li show dev dummy0
>> >> >12: dummy0: <NO-CARRIER,BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state DORMANT mode DORMANT   
>> >> 
>> >> if you mean this "NO-CARRIER"
>> >> it has no direct relation with netif_carrier_ok().
>> >
>> >It is the same value (IFF_RUNNING) that is visible from user space.
>> 
>> static inline bool netif_carrier_ok(const struct net_device *dev)
>> {
>> 	        return !test_bit(__LINK_STATE_NOCARRIER, &dev->state);
>> }
>> 
>> So netif_carrier[ok/on/off] are working with on __LINK_STATE_NOCARRIER
>> bit. Not with IFF_RUNNING flag.
>
>What is the code path that you are worried about netif_carrier_ok being set or clear?
>The interaction here is complex, and right now LINK_STATE_NOCARRIER is purely
>controlled by the driver, your patch changes that, but before acking I want
>to make sure why it is required.

This patchset would provide a possibility to set or clear the carrier
from userspace. For dummy device it would serve for direct emulation
of link fail.

Also for team deriver, that would serve for teamd (userspace part) to
set the carrier actually on or off (in case of LACP runner for example
this is required).

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Stephen Hemminger]

On Wed, 12 Dec 2012 19:49:26 +0100
Jiri Pirko <jiri@resnulli.us> wrote:

> Wed, Dec 12, 2012 at 07:36:32PM CET, shemminger@vyatta.com wrote:
> >On Wed, 12 Dec 2012 19:25:56 +0100
> >Jiri Pirko <jiri@resnulli.us> wrote:
> >
> >> Wed, Dec 12, 2012 at 07:12:08PM CET, shemminger@vyatta.com wrote:
> >> >On Wed, 12 Dec 2012 19:10:17 +0100
> >> >Jiri Pirko <jiri@resnulli.us> wrote:
> >> >
> >> >> ># ip li show dev dummy0
> >> >> >12: dummy0: <NO-CARRIER,BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state DORMANT mode DORMANT   
> >> >> 
> >> >> if you mean this "NO-CARRIER"
> >> >> it has no direct relation with netif_carrier_ok().
> >> >
> >> >It is the same value (IFF_RUNNING) that is visible from user space.
> >> 
> >> static inline bool netif_carrier_ok(const struct net_device *dev)
> >> {
> >> 	        return !test_bit(__LINK_STATE_NOCARRIER, &dev->state);
> >> }
> >> 
> >> So netif_carrier[ok/on/off] are working with on __LINK_STATE_NOCARRIER
> >> bit. Not with IFF_RUNNING flag.
> >
> >What is the code path that you are worried about netif_carrier_ok being set or clear?
> >The interaction here is complex, and right now LINK_STATE_NOCARRIER is purely
> >controlled by the driver, your patch changes that, but before acking I want
> >to make sure why it is required.
> 
> This patchset would provide a possibility to set or clear the carrier
> from userspace. For dummy device it would serve for direct emulation
> of link fail.
> 
> Also for team deriver, that would serve for teamd (userspace part) to
> set the carrier actually on or off (in case of LACP runner for example
> this is required).
> 

You want to able to control the dummy device, so that you can test carrier
management in the team device. Another alternative is to use carrier control
on a virtual device. Vmware can do it, there were patches to do this with KVM/QEMU
not sure if they ever got incorporated.

Since this is a specific feature of the dummy device which is specialized for
testing, maybe it should just be done by adding device specific ioctl rather
than letting it creep in as a general facility.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

Wed, Dec 12, 2012 at 07:54:48PM CET, shemminger@vyatta.com wrote:
>On Wed, 12 Dec 2012 19:49:26 +0100
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Wed, Dec 12, 2012 at 07:36:32PM CET, shemminger@vyatta.com wrote:
>> >On Wed, 12 Dec 2012 19:25:56 +0100
>> >Jiri Pirko <jiri@resnulli.us> wrote:
>> >
>> >> Wed, Dec 12, 2012 at 07:12:08PM CET, shemminger@vyatta.com wrote:
>> >> >On Wed, 12 Dec 2012 19:10:17 +0100
>> >> >Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >
>> >> >> ># ip li show dev dummy0
>> >> >> >12: dummy0: <NO-CARRIER,BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state DORMANT mode DORMANT   
>> >> >> 
>> >> >> if you mean this "NO-CARRIER"
>> >> >> it has no direct relation with netif_carrier_ok().
>> >> >
>> >> >It is the same value (IFF_RUNNING) that is visible from user space.
>> >> 
>> >> static inline bool netif_carrier_ok(const struct net_device *dev)
>> >> {
>> >> 	        return !test_bit(__LINK_STATE_NOCARRIER, &dev->state);
>> >> }
>> >> 
>> >> So netif_carrier[ok/on/off] are working with on __LINK_STATE_NOCARRIER
>> >> bit. Not with IFF_RUNNING flag.
>> >
>> >What is the code path that you are worried about netif_carrier_ok being set or clear?
>> >The interaction here is complex, and right now LINK_STATE_NOCARRIER is purely
>> >controlled by the driver, your patch changes that, but before acking I want
>> >to make sure why it is required.
>> 
>> This patchset would provide a possibility to set or clear the carrier
>> from userspace. For dummy device it would serve for direct emulation
>> of link fail.
>> 
>> Also for team deriver, that would serve for teamd (userspace part) to
>> set the carrier actually on or off (in case of LACP runner for example
>> this is required).
>> 
>
>You want to able to control the dummy device, so that you can test carrier
>management in the team device. Another alternative is to use carrier control
>on a virtual device. Vmware can do it, there were patches to do this with KVM/QEMU
>not sure if they ever got incorporated.
>
>Since this is a specific feature of the dummy device which is specialized for
>testing, maybe it should just be done by adding device specific ioctl rather
>than letting it creep in as a general facility.

Ugh, specific ioctl stinks...
But this is not only for dummy. As I said, we need this for team driver.
Maybe I did not explain that correctly. Given the fact that the whole
Team logic is in userspace, teamd (userspace daemon) needs to set the
carrier state as if it was done in kernel. Yes, we would be able to do
this by specific Team option in team driver, but I thought this would be
nicer to do that more generally.

Also, in previous discussion Michał Mirosław wrote he would like this
feature also for GRE tunnel devices.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Stephen Hemminger]

On Wed, 12 Dec 2012 20:06:13 +0100
Jiri Pirko <jiri@resnulli.us> wrote:

> Wed, Dec 12, 2012 at 07:54:48PM CET, shemminger@vyatta.com wrote:
> >On Wed, 12 Dec 2012 19:49:26 +0100
> >Jiri Pirko <jiri@resnulli.us> wrote:
> >
> >> Wed, Dec 12, 2012 at 07:36:32PM CET, shemminger@vyatta.com wrote:
> >> >On Wed, 12 Dec 2012 19:25:56 +0100
> >> >Jiri Pirko <jiri@resnulli.us> wrote:
> >> >
> >> >> Wed, Dec 12, 2012 at 07:12:08PM CET, shemminger@vyatta.com wrote:
> >> >> >On Wed, 12 Dec 2012 19:10:17 +0100
> >> >> >Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> >
> >> >> >> ># ip li show dev dummy0
> >> >> >> >12: dummy0: <NO-CARRIER,BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state DORMANT mode DORMANT   
> >> >> >> 
> >> >> >> if you mean this "NO-CARRIER"
> >> >> >> it has no direct relation with netif_carrier_ok().
> >> >> >
> >> >> >It is the same value (IFF_RUNNING) that is visible from user space.
> >> >> 
> >> >> static inline bool netif_carrier_ok(const struct net_device *dev)
> >> >> {
> >> >> 	        return !test_bit(__LINK_STATE_NOCARRIER, &dev->state);
> >> >> }
> >> >> 
> >> >> So netif_carrier[ok/on/off] are working with on __LINK_STATE_NOCARRIER
> >> >> bit. Not with IFF_RUNNING flag.
> >> >
> >> >What is the code path that you are worried about netif_carrier_ok being set or clear?
> >> >The interaction here is complex, and right now LINK_STATE_NOCARRIER is purely
> >> >controlled by the driver, your patch changes that, but before acking I want
> >> >to make sure why it is required.
> >> 
> >> This patchset would provide a possibility to set or clear the carrier
> >> from userspace. For dummy device it would serve for direct emulation
> >> of link fail.
> >> 
> >> Also for team deriver, that would serve for teamd (userspace part) to
> >> set the carrier actually on or off (in case of LACP runner for example
> >> this is required).
> >> 
> >
> >You want to able to control the dummy device, so that you can test carrier
> >management in the team device. Another alternative is to use carrier control
> >on a virtual device. Vmware can do it, there were patches to do this with KVM/QEMU
> >not sure if they ever got incorporated.
> >
> >Since this is a specific feature of the dummy device which is specialized for
> >testing, maybe it should just be done by adding device specific ioctl rather
> >than letting it creep in as a general facility.
> 
> Ugh, specific ioctl stinks...
> But this is not only for dummy. As I said, we need this for team driver.
> Maybe I did not explain that correctly. Given the fact that the whole
> Team logic is in userspace, teamd (userspace daemon) needs to set the
> carrier state as if it was done in kernel. Yes, we would be able to do
> this by specific Team option in team driver, but I thought this would be
> nicer to do that more generally.

That is what the operstate mechanism was for. Why did we build that mechanism
if it doesn't work from userspace.

Maybe the fix is to make setting linkstate also set carrier bits.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

Wed, Dec 12, 2012 at 08:34:33PM CET, shemminger@vyatta.com wrote:
>On Wed, 12 Dec 2012 20:06:13 +0100
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Wed, Dec 12, 2012 at 07:54:48PM CET, shemminger@vyatta.com wrote:
>> >On Wed, 12 Dec 2012 19:49:26 +0100
>> >Jiri Pirko <jiri@resnulli.us> wrote:
>> >
>> >> Wed, Dec 12, 2012 at 07:36:32PM CET, shemminger@vyatta.com wrote:
>> >> >On Wed, 12 Dec 2012 19:25:56 +0100
>> >> >Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >
>> >> >> Wed, Dec 12, 2012 at 07:12:08PM CET, shemminger@vyatta.com wrote:
>> >> >> >On Wed, 12 Dec 2012 19:10:17 +0100
>> >> >> >Jiri Pirko <jiri@resnulli.us> wrote:
>> >> >> >
>> >> >> >> ># ip li show dev dummy0
>> >> >> >> >12: dummy0: <NO-CARRIER,BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state DORMANT mode DORMANT   
>> >> >> >> 
>> >> >> >> if you mean this "NO-CARRIER"
>> >> >> >> it has no direct relation with netif_carrier_ok().
>> >> >> >
>> >> >> >It is the same value (IFF_RUNNING) that is visible from user space.
>> >> >> 
>> >> >> static inline bool netif_carrier_ok(const struct net_device *dev)
>> >> >> {
>> >> >> 	        return !test_bit(__LINK_STATE_NOCARRIER, &dev->state);
>> >> >> }
>> >> >> 
>> >> >> So netif_carrier[ok/on/off] are working with on __LINK_STATE_NOCARRIER
>> >> >> bit. Not with IFF_RUNNING flag.
>> >> >
>> >> >What is the code path that you are worried about netif_carrier_ok being set or clear?
>> >> >The interaction here is complex, and right now LINK_STATE_NOCARRIER is purely
>> >> >controlled by the driver, your patch changes that, but before acking I want
>> >> >to make sure why it is required.
>> >> 
>> >> This patchset would provide a possibility to set or clear the carrier
>> >> from userspace. For dummy device it would serve for direct emulation
>> >> of link fail.
>> >> 
>> >> Also for team deriver, that would serve for teamd (userspace part) to
>> >> set the carrier actually on or off (in case of LACP runner for example
>> >> this is required).
>> >> 
>> >
>> >You want to able to control the dummy device, so that you can test carrier
>> >management in the team device. Another alternative is to use carrier control
>> >on a virtual device. Vmware can do it, there were patches to do this with KVM/QEMU
>> >not sure if they ever got incorporated.
>> >
>> >Since this is a specific feature of the dummy device which is specialized for
>> >testing, maybe it should just be done by adding device specific ioctl rather
>> >than letting it creep in as a general facility.
>> 
>> Ugh, specific ioctl stinks...
>> But this is not only for dummy. As I said, we need this for team driver.
>> Maybe I did not explain that correctly. Given the fact that the whole
>> Team logic is in userspace, teamd (userspace daemon) needs to set the
>> carrier state as if it was done in kernel. Yes, we would be able to do
>> this by specific Team option in team driver, but I thought this would be
>> nicer to do that more generally.
>
>That is what the operstate mechanism was for. Why did we build that mechanism
>if it doesn't work from userspace.
>
>Maybe the fix is to make setting linkstate also set carrier bits.

Hmm. You mean to call netif_carrier_on/off as a reaction to operstate
change? How exactly would you like to do that?

Thanks

Jiri

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[John Fastabend]

[...]

>> That is what the operstate mechanism was for. Why did we build that mechanism
>> if it doesn't work from userspace.
>>
>> Maybe the fix is to make setting linkstate also set carrier bits.
>
> Hmm. You mean to call netif_carrier_on/off as a reaction to operstate
> change? How exactly would you like to do that?
>
> Thanks
>
> Jiri

This would break existing applications and would not really be in the
spirit of the operstate mechanism as I read the documentation:

./Documentation/networking/operstates.txt
  63 IF_OPER_DORMANT (5):
  64  Interface is L1 up, but waiting for an external event, f.e. for a
  65  protocol to establish. (802.1X)

The L1 up is netif_carrier_on here.

We use this in user space when we do not want applications to start
using the link until we have negotiated and configured some link layer
attributes. To do this we set IFLA_LINKMODE and then use the
IF_OPER_DORMANT event to trigger the application eventually setting
IF_OPER_UP when the link layer negotiation is complete. If you take the
carrier down this breaks. In my case the protocol is LLDP but I think
there are other examples. This is basically the example Stephen already
gave.

I guess I still am missing why teamd doesn't just set IFLA_LINKMODE
then manage the operstate this way? Sure teamd would have to become a
bit smarter but it would save adding additional interfaces to the
kernel. In the LinkAgg case you could just pin the operstate down this
would also allow protocols to run under the linkagg over the LLC in
IEEE speak which I think is being discussed in the latest round of
LLDP/LinkAgg spec updates (I'll check on that later today).

Thanks,
John


-- 
John Fastabend         Intel Corporation

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Flavio Leitner]

On Thu, 13 Dec 2012 09:15:41 -0800
John Fastabend <john.fastabend@gmail.com> wrote:

> [...]
> 
> >> That is what the operstate mechanism was for. Why did we build that mechanism
> >> if it doesn't work from userspace.
> >>
> >> Maybe the fix is to make setting linkstate also set carrier bits.
> >
> > Hmm. You mean to call netif_carrier_on/off as a reaction to operstate
> > change? How exactly would you like to do that?
> >
> > Thanks
> >
> > Jiri
> 
> This would break existing applications and would not really be in the
> spirit of the operstate mechanism as I read the documentation:
> 
> ./Documentation/networking/operstates.txt
>   63 IF_OPER_DORMANT (5):
>   64  Interface is L1 up, but waiting for an external event, f.e. for a
>   65  protocol to establish. (802.1X)
> 
> The L1 up is netif_carrier_on here.

Agreed. I am also not finding how to change carrier bits from the
current states.


> We use this in user space when we do not want applications to start
> using the link until we have negotiated and configured some link layer
> attributes. To do this we set IFLA_LINKMODE and then use the
> IF_OPER_DORMANT event to trigger the application eventually setting
> IF_OPER_UP when the link layer negotiation is complete. If you take the
> carrier down this breaks. In my case the protocol is LLDP but I think
> there are other examples. This is basically the example Stephen already
> gave.
> 
> I guess I still am missing why teamd doesn't just set IFLA_LINKMODE
> then manage the operstate this way? Sure teamd would have to become a
> bit smarter but it would save adding additional interfaces to the
> kernel. In the LinkAgg case you could just pin the operstate down this
> would also allow protocols to run under the linkagg over the LLC in
> IEEE speak which I think is being discussed in the latest round of
> LLDP/LinkAgg spec updates (I'll check on that later today).

Think about the ARP monitoring as an example. If the teamd (userlevel)
doesn't receive the reply packet, it should set the master interface 
as down in terms of carrier because there isn't a valid L1 from the
master interface perspective.

LACP needs the same action. If the ports can't move to DISTRIBUTING/
COLLECTING state, it should set master's carrier bit to down.

Even the lack of ports should keep the master with carrier down.

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.

Therefore, I am not seeing how setting operstate to down helps with the
current code.  Can ethtool report Link based on a logic between operstate
and netif_carrier_ok()? If operstate is down, it would report down regardless
of netif_carrier_ok() for instance.

Calling netif_carrier_off() also calls dev_deactivate() to stop everything.
So, if team sets operstate to down, the interface remains active sending/
receiving as far as I can see, right? Not sure if this would be a problem.

-- 
fbl

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Stephen Hemminger]

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.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Flavio Leitner]

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.

-- 
fbl

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Stephen Hemminger]

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.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Dan Williams]

On Thu, 2012-12-13 at 10:20 -0800, Stephen Hemminger 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.

That's not entirely sufficient, because not everything uses ethtool to
deterine whether the link/carrier is active.  eg, anything listenting to
netlink for IFF_RUNNING won't know anything about this.  I may have
missed previous conversation, but IFF_RUNNING is AFAIK the sole
mechanism to indicate to userspace that the link is operational and
ready for general traffic.  Can the team drivers simply not set
IFF_RUNNING until the interface is actually ready for general traffic?

I'd just caution people to be careful when it comes to carrier/link
states, as a lot of stuff relies on it, and people keep trying to
slightly change the meaning of it.  We need it to be consistent across
net interfaces and driver implementations, not have specific meanings to
certain drivers that don't apply to others.

If teamd needs to manage the carrier state of the teamX interface,
perhaps instead of munging the actual carrier state itself, it can flip
some team-private value that is one component of determining whether
IFF_RUNNING gets set or not?

Dan

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[John Fastabend]

On 12/13/2012 10:33 AM, Dan Williams wrote:
> On Thu, 2012-12-13 at 10:20 -0800, Stephen Hemminger 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.
>
> That's not entirely sufficient, because not everything uses ethtool to
> deterine whether the link/carrier is active.  eg, anything listenting to
> netlink for IFF_RUNNING won't know anything about this.  I may have
> missed previous conversation, but IFF_RUNNING is AFAIK the sole
> mechanism to indicate to userspace that the link is operational and
> ready for general traffic.  Can the team drivers simply not set
> IFF_RUNNING until the interface is actually ready for general traffic?
>
> I'd just caution people to be careful when it comes to carrier/link
> states, as a lot of stuff relies on it, and people keep trying to
> slightly change the meaning of it.  We need it to be consistent across
> net interfaces and driver implementations, not have specific meanings to
> certain drivers that don't apply to others.
>
> If teamd needs to manage the carrier state of the teamX interface,
> perhaps instead of munging the actual carrier state itself, it can flip
> some team-private value that is one component of determining whether
> IFF_RUNNING gets set or not?
>
> Dan

Assuming the operstate documentation is correct IFF_RUNNING should only
be set if the operstate is up or unknown. Quoting the text again,

  32	ifinfomsg::if_flags & IFF_RUNNING:
  33	 Interface is in RFC2863 operational state UP or UNKNOWN. This is for
  34	 backward compatibility, routing daemons, dhcp clients can use this
  35	 flag to determine whether they should use the interface.

So my guess is having a team ethtool get_link handler AND managing the
operstate from teamd would satisfy the requirement. But maybe I missed
something.

.John

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

Thu, Dec 13, 2012 at 08:09:03PM CET, john.r.fastabend@intel.com wrote:
>On 12/13/2012 10:33 AM, Dan Williams wrote:
>>On Thu, 2012-12-13 at 10:20 -0800, Stephen Hemminger 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.
>>
>>That's not entirely sufficient, because not everything uses ethtool to
>>deterine whether the link/carrier is active.  eg, anything listenting to
>>netlink for IFF_RUNNING won't know anything about this.  I may have
>>missed previous conversation, but IFF_RUNNING is AFAIK the sole
>>mechanism to indicate to userspace that the link is operational and
>>ready for general traffic.  Can the team drivers simply not set
>>IFF_RUNNING until the interface is actually ready for general traffic?
>>
>>I'd just caution people to be careful when it comes to carrier/link
>>states, as a lot of stuff relies on it, and people keep trying to
>>slightly change the meaning of it.  We need it to be consistent across
>>net interfaces and driver implementations, not have specific meanings to
>>certain drivers that don't apply to others.
>>
>>If teamd needs to manage the carrier state of the teamX interface,
>>perhaps instead of munging the actual carrier state itself, it can flip
>>some team-private value that is one component of determining whether
>>IFF_RUNNING gets set or not?
>>
>>Dan
>
>Assuming the operstate documentation is correct IFF_RUNNING should only
>be set if the operstate is up or unknown. Quoting the text again,
>
> 32	ifinfomsg::if_flags & IFF_RUNNING:
> 33	 Interface is in RFC2863 operational state UP or UNKNOWN. This is for
> 34	 backward compatibility, routing daemons, dhcp clients can use this
> 35	 flag to determine whether they should use the interface.
>
>So my guess is having a team ethtool get_link handler AND managing the
>operstate from teamd would satisfy the requirement. But maybe I missed
>something.

Yes, I've been thinking about the same thing. I'll check that.

>
>.John

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

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

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Stephen Hemminger]

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.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

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.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Stephen Hemminger]

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

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

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

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Stephen Hemminger]

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.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

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.

>

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

I see that the patchset is in state "Rejected" in patchwork.
Stephen convinced me for a moment that the problem can be handled by operstate.
As it turned out (in last 3-4 emails in thread) operstate use would not
be an option.

So how should I proceed? Should I repost the patchset? Anyone has any other
comments?

thanks.

Wed, Dec 12, 2012 at 11:58:03AM CET, jiri@resnulli.us wrote:
>This is basically a repost of my previous patchset:
>"[patch net-next-2.6 0/2] net: allow to change carrier via sysfs" from Aug 30
>
>The way net-sysfs stores values changed and this patchset reflects it.
>Also, I exposed carrier via rtnetlink iface.
>
>So far, only dummy driver uses carrier change ndo. In very near future
>team driver will use that as well.
>
>Jiri Pirko (4):
>  net: add change_carrier netdev op
>  net: allow to change carrier via sysfs
>  rtnl: expose carrier value with possibility to set it
>  dummy: implement carrier change
>
> drivers/net/dummy.c          | 10 ++++++++++
> include/linux/netdevice.h    |  7 +++++++
> include/uapi/linux/if_link.h |  1 +
> net/core/dev.c               | 19 +++++++++++++++++++
> net/core/net-sysfs.c         | 15 ++++++++++++++-
> net/core/rtnetlink.c         | 10 ++++++++++
> 6 files changed, 61 insertions(+), 1 deletion(-)
>
>-- 
>1.8.0
>

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Stephen Hemminger]

On Sun, 16 Dec 2012 11:54:51 +0100
Jiri Pirko <jiri@resnulli.us> wrote:

> 
> I see that the patchset is in state "Rejected" in patchwork.
> Stephen convinced me for a moment that the problem can be handled by operstate.
> As it turned out (in last 3-4 emails in thread) operstate use would not
> be an option.
> 
> So how should I proceed? Should I repost the patchset? Anyone has any other
> comments?
> 
> thanks.

Don't take my comments so far as negative. Devices to need to be more controllable
from userspace. But I have concerns about introducing a new way to change state causing
more races.  For example, changing carrier state should cause netlink events to fire and
these should post to routing daemons etc. Also, what happens if some confused developer
mixes operstate and direct carrier control.

The root cause of all this confusion is that their are three ways of expressing
the same state, and they are controlled through different paths:
  a. Old BSD style flag bit IFF_RUNNING
  b. LINK_STATE bit in kernel (netif_carrier_ok)
  c. RFC2863 operational state

The operstate stuff is the most complete, but is the weakest in implementation:
  a. kernel drivers check netif_carrier_ok when they should be using netif_dormant
     (bridge is one example). But what will break if this changes?
  b. lower device state is not tracked correctly by tunnels and a few other layered devices
  c. dormant from kernel space was never used by much.

The good news is that the old BSD style IFF_RUNNING bit is the most commonly
used bit by applications and it works correctly in either carrier or operstate mode.

Re: [patch net-next 0/4] net: allow to change carrier from userspace

[Jiri Pirko]

Tue, Dec 18, 2012 at 07:49:57AM CET, shemminger@vyatta.com wrote:
>On Sun, 16 Dec 2012 11:54:51 +0100
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> 
>> I see that the patchset is in state "Rejected" in patchwork.
>> Stephen convinced me for a moment that the problem can be handled by operstate.
>> As it turned out (in last 3-4 emails in thread) operstate use would not
>> be an option.
>> 
>> So how should I proceed? Should I repost the patchset? Anyone has any other
>> comments?
>> 
>> thanks.
>
>Don't take my comments so far as negative. Devices to need to be more controllable
>from userspace. But I have concerns about introducing a new way to change state causing
>more races.  For example, changing carrier state should cause netlink events to fire and
>these should post to routing daemons etc. Also, what happens if some confused developer
>mixes operstate and direct carrier control.

I do not think that the race you are describing is of any concern. The
same can happen now for any device. My patchset only adds a possibility
for "soft devices" to change the carrier as well.

Developer will not be likely confused. As the possibility of carrier
change from userspace will be limited to small set of devices, for other
devices the attempt will lead to -EOPNOTSUPP (in contrast with operstate
which is available for all devices).

I can add a comments/notes to code and operstates.txt stating the
purpose of this iface.

>
>The root cause of all this confusion is that their are three ways of expressing
>the same state, and they are controlled through different paths:
>  a. Old BSD style flag bit IFF_RUNNING
>  b. LINK_STATE bit in kernel (netif_carrier_ok)
>  c. RFC2863 operational state

I do not think so. Yes, for a) and c), these are strictly connected,
expressing the same thing. But b) is not the same. It's on lower level
than a) and c). What b) can be compared to is IFF_LOWER_UP.

>
>The operstate stuff is the most complete, but is the weakest in implementation:
>  a. kernel drivers check netif_carrier_ok when they should be using netif_dormant
>     (bridge is one example). But what will break if this changes?
I agree, that should be changed.

>  b. lower device state is not tracked correctly by tunnels and a few other layered devices
>  c. dormant from kernel space was never used by much.
>
>The good news is that the old BSD style IFF_RUNNING bit is the most commonly
>used bit by applications and it works correctly in either carrier or operstate mode.

That is indeed a good thing.