All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] net: add support for phys_port_name
@ 2015-03-16 15:46 David Ahern
  2015-03-16 15:46 ` [PATCH 2/3] rocker: " David Ahern
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: David Ahern @ 2015-03-16 15:46 UTC (permalink / raw)
  To: netdev; +Cc: David Ahern, Jiri Pirko, Scott Feldman

Similar to port id allow netdevices to specify port names and export
the name via sysfs. Drivers can implement the netdevice operation to
assist udev in having sane default names for the devices using the
rule:

$ cat /etc/udev/rules.d/80-net-setup-link.rules
SUBSYSTEM=="net", ACTION=="add", ATTR{phys_port_name}!="",
NAME="$attr{phys_port_name}"

Use of phys_name versus phys_id was suggested-by Jiri Pirko.

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: Scott Feldman <sfeldma@gmail.com>
---
 include/linux/netdevice.h    | 13 +++++++++++++
 include/uapi/linux/if_link.h |  1 +
 net/core/dev.c               | 18 ++++++++++++++++++
 net/core/net-sysfs.c         | 23 +++++++++++++++++++++++
 net/core/rtnetlink.c         | 21 +++++++++++++++++++++
 5 files changed, 76 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index dd1d069758be..47a773b9bee0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -765,6 +765,15 @@ struct netdev_phys_item_id {
 	unsigned char id_len;
 };
 
+#define MAX_PHYS_ITEM_NAME_LEN 32
+
+/* This structure holds a unique name to identify some
+ * physical item (port for example) used by a netdevice.
+ */
+struct netdev_phys_item_name {
+	char str[MAX_PHYS_ITEM_NAME_LEN];
+};
+
 typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
 				       struct sk_buff *skb);
 
@@ -1159,6 +1168,8 @@ struct net_device_ops {
 						      bool new_carrier);
 	int			(*ndo_get_phys_port_id)(struct net_device *dev,
 							struct netdev_phys_item_id *ppid);
+	int			(*ndo_get_phys_port_name)(struct net_device *dev,
+							  struct netdev_phys_item_name *name);
 	void			(*ndo_add_vxlan_port)(struct  net_device *dev,
 						      sa_family_t sa_family,
 						      __be16 port);
@@ -2939,6 +2950,8 @@ int dev_set_mac_address(struct net_device *, struct sockaddr *);
 int dev_change_carrier(struct net_device *, bool new_carrier);
 int dev_get_phys_port_id(struct net_device *dev,
 			 struct netdev_phys_item_id *ppid);
+int dev_get_phys_port_name(struct net_device *dev,
+			   struct netdev_phys_item_name *name);
 struct sk_buff *validate_xmit_skb_list(struct sk_buff *skb, struct net_device *dev);
 struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 				    struct netdev_queue *txq, int *ret);
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 756436e1ce89..7158fd00a109 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -147,6 +147,7 @@ enum {
 	IFLA_CARRIER_CHANGES,
 	IFLA_PHYS_SWITCH_ID,
 	IFLA_LINK_NETNSID,
+	IFLA_PHYS_PORT_NAME,
 	__IFLA_MAX
 };
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 39fe369b46ad..38b04093e4c9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5912,6 +5912,24 @@ int dev_get_phys_port_id(struct net_device *dev,
 EXPORT_SYMBOL(dev_get_phys_port_id);
 
 /**
+ *	dev_get_phys_port_name - Get device physical port name
+ *	@dev: device
+ *	@name: port name
+ *
+ *	Get device physical port name
+ */
+int dev_get_phys_port_name(struct net_device *dev,
+			   struct netdev_phys_item_name *name)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+
+	if (!ops->ndo_get_phys_port_name)
+		return -EOPNOTSUPP;
+	return ops->ndo_get_phys_port_name(dev, name);
+}
+EXPORT_SYMBOL(dev_get_phys_port_name);
+
+/**
  *	dev_new_index	-	allocate an ifindex
  *	@net: the applicable net namespace
  *
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index cf30620a88e1..beecfd499341 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -418,6 +418,28 @@ static ssize_t phys_port_id_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(phys_port_id);
 
+static ssize_t phys_port_name_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct net_device *netdev = to_net_dev(dev);
+	ssize_t ret = -EINVAL;
+
+	if (!rtnl_trylock())
+		return restart_syscall();
+
+	if (dev_isalive(netdev)) {
+		struct netdev_phys_item_name name;
+
+		ret = dev_get_phys_port_name(netdev, &name);
+		if (!ret)
+			ret = sprintf(buf, "%s\n", name.str);
+	}
+	rtnl_unlock();
+
+	return ret;
+}
+static DEVICE_ATTR_RO(phys_port_name);
+
 static ssize_t phys_switch_id_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
@@ -465,6 +487,7 @@ static struct attribute *net_class_attrs[] = {
 	&dev_attr_tx_queue_len.attr,
 	&dev_attr_gro_flush_timeout.attr,
 	&dev_attr_phys_port_id.attr,
+	&dev_attr_phys_port_name.attr,
 	&dev_attr_phys_switch_id.attr,
 	NULL,
 };
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 25b4b5d23485..6f4d17e13db7 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -982,6 +982,24 @@ static int rtnl_phys_port_id_fill(struct sk_buff *skb, struct net_device *dev)
 	return 0;
 }
 
+static int rtnl_phys_port_name_fill(struct sk_buff *skb, struct net_device *dev)
+{
+	struct netdev_phys_item_name name;
+	int err;
+
+	err = dev_get_phys_port_name(dev, &name);
+	if (err) {
+		if (err == -EOPNOTSUPP)
+			return 0;
+		return err;
+	}
+
+	if (nla_put(skb, IFLA_PHYS_PORT_NAME, strlen(name.str), name.str))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
 static int rtnl_phys_switch_id_fill(struct sk_buff *skb, struct net_device *dev)
 {
 	int err;
@@ -1072,6 +1090,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	if (rtnl_phys_port_id_fill(skb, dev))
 		goto nla_put_failure;
 
+	if (rtnl_phys_port_name_fill(skb, dev))
+		goto nla_put_failure;
+
 	if (rtnl_phys_switch_id_fill(skb, dev))
 		goto nla_put_failure;
 
-- 
2.2.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] rocker: add support for phys_port_name
  2015-03-16 15:46 [PATCH 1/3] net: add support for phys_port_name David Ahern
@ 2015-03-16 15:46 ` David Ahern
  2015-03-16 15:46 ` [PATCH 3/3] iproute2: Add " David Ahern
  2015-03-16 16:00 ` [PATCH 1/3] net: add " Jiri Pirko
  2 siblings, 0 replies; 13+ messages in thread
From: David Ahern @ 2015-03-16 15:46 UTC (permalink / raw)
  To: netdev; +Cc: David Ahern, Scott Feldman, Jiri Pirko

Implement the phys_port_name operation. Port names are pulled from the
rocker hardware model in qemu and default to the qemu name + port id.
e.g.,

sw1p1: flags=4098<BROADCAST,MULTICAST>  mtu 1500
        ether 52:54:00:12:35:01  txqueuelen 1000  (Ethernet)
        RX packets 0  bytes 0 (0.0 B)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 0  bytes 0 (0.0 B)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

where 'sw1' comes from the qemu command line -device rocker,name=sw1, and
'p1' is port 1.

Patch is adapted from Scott's phys_port_id patch.

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Scott Feldman <sfeldma@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
---
 drivers/net/ethernet/rocker/rocker.c | 58 ++++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/rocker/rocker.h |  1 +
 2 files changed, 59 insertions(+)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index bc5f27aa3131..8de390499952 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -30,6 +30,7 @@
 #include <linux/if_vlan.h>
 #include <linux/if_bridge.h>
 #include <linux/bitops.h>
+#include <linux/ctype.h>
 #include <net/switchdev.h>
 #include <net/rtnetlink.h>
 #include <net/ip_fib.h>
@@ -1631,6 +1632,48 @@ rocker_cmd_get_port_settings_macaddr_proc(struct rocker *rocker,
 }
 
 static int
+rocker_cmd_get_port_settings_phys_name_proc(struct rocker *rocker,
+					    struct rocker_port *rocker_port,
+					    struct rocker_desc_info *desc_info,
+					    void *priv)
+{
+	struct rocker_tlv *info_attrs[ROCKER_TLV_CMD_PORT_SETTINGS_MAX + 1];
+	struct rocker_tlv *attrs[ROCKER_TLV_CMD_MAX + 1];
+	struct netdev_phys_item_name *name = priv;
+	struct rocker_tlv *attr;
+	int i, j = 0, len;
+	char *str;
+
+	rocker_tlv_parse_desc(attrs, ROCKER_TLV_CMD_MAX, desc_info);
+	if (!attrs[ROCKER_TLV_CMD_INFO])
+		return -EIO;
+
+	rocker_tlv_parse_nested(info_attrs, ROCKER_TLV_CMD_PORT_SETTINGS_MAX,
+				attrs[ROCKER_TLV_CMD_INFO]);
+	attr = info_attrs[ROCKER_TLV_CMD_PORT_SETTINGS_PHYS_NAME];
+	if (!attr)
+		return -EIO;
+
+	len = min_t(int, rocker_tlv_len(attr), sizeof(name->str) - 1);
+	str = rocker_tlv_data(attr);
+
+	/* make sure name only contains alphanumeric characters */
+	for (i = 0; i < len; ++i) {
+		if (isalnum(str[i])) {
+			name->str[j] = str[i];
+			j++;
+		}
+	}
+
+	if (j == 0)
+		return -EIO;
+
+	name->str[j] = '\0';
+
+	return 0;
+}
+
+static int
 rocker_cmd_set_port_settings_ethtool_prep(struct rocker *rocker,
 					  struct rocker_port *rocker_port,
 					  struct rocker_desc_info *desc_info,
@@ -4131,6 +4174,20 @@ static int rocker_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
 				       rocker_port->brport_flags, mask);
 }
 
+static int rocker_port_get_phys_port_name(struct net_device *dev,
+					  struct netdev_phys_item_name *name)
+{
+	struct rocker_port *rocker_port = netdev_priv(dev);
+	int err;
+
+	err = rocker_cmd_exec(rocker_port->rocker, rocker_port,
+			      rocker_cmd_get_port_settings_prep, NULL,
+			      rocker_cmd_get_port_settings_phys_name_proc,
+			      name, false);
+
+	return err ? -EOPNOTSUPP : 0;
+}
+
 static const struct net_device_ops rocker_port_netdev_ops = {
 	.ndo_open			= rocker_port_open,
 	.ndo_stop			= rocker_port_stop,
@@ -4143,6 +4200,7 @@ static const struct net_device_ops rocker_port_netdev_ops = {
 	.ndo_fdb_dump			= rocker_port_fdb_dump,
 	.ndo_bridge_setlink		= rocker_port_bridge_setlink,
 	.ndo_bridge_getlink		= rocker_port_bridge_getlink,
+	.ndo_get_phys_port_name		= rocker_port_get_phys_port_name,
 };
 
 /********************
diff --git a/drivers/net/ethernet/rocker/rocker.h b/drivers/net/ethernet/rocker/rocker.h
index 51e430d25138..a4e9591d7457 100644
--- a/drivers/net/ethernet/rocker/rocker.h
+++ b/drivers/net/ethernet/rocker/rocker.h
@@ -158,6 +158,7 @@ enum {
 	ROCKER_TLV_CMD_PORT_SETTINGS_MACADDR,		/* binary */
 	ROCKER_TLV_CMD_PORT_SETTINGS_MODE,		/* u8 */
 	ROCKER_TLV_CMD_PORT_SETTINGS_LEARNING,		/* u8 */
+	ROCKER_TLV_CMD_PORT_SETTINGS_PHYS_NAME,		/* binary */
 
 	__ROCKER_TLV_CMD_PORT_SETTINGS_MAX,
 	ROCKER_TLV_CMD_PORT_SETTINGS_MAX =
-- 
2.2.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] iproute2: Add support for phys_port_name
  2015-03-16 15:46 [PATCH 1/3] net: add support for phys_port_name David Ahern
  2015-03-16 15:46 ` [PATCH 2/3] rocker: " David Ahern
@ 2015-03-16 15:46 ` David Ahern
  2015-03-24 22:50   ` Stephen Hemminger
  2015-03-16 16:00 ` [PATCH 1/3] net: add " Jiri Pirko
  2 siblings, 1 reply; 13+ messages in thread
From: David Ahern @ 2015-03-16 15:46 UTC (permalink / raw)
  To: netdev; +Cc: David Ahern

Display phys_port_name attribute if returned:

./ip link show sw1p1
4: sw1p1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop port name sw1p1 state DOWN mode DEFAULT group default qlen 1000
    link/ether 52:54:00:12:35:01 brd ff:ff:ff:ff:ff:ff

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 include/linux/if_link.h | 1 +
 ip/ipaddress.c          | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 3450c3fbdc65..c5ee8c91b1c6 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -147,6 +147,7 @@ enum {
 	IFLA_CARRIER_CHANGES,
 	IFLA_PHYS_SWITCH_ID,
 	IFLA_LINK_NETNSID,
+	IFLA_PHYS_PORT_NAME,
 	__IFLA_MAX
 };
 
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 99a6ab5977e3..9e65015b7a48 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -645,6 +645,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
 				      b1, sizeof(b1)));
 	}
 
+	if (tb[IFLA_PHYS_PORT_NAME])
+		fprintf(fp, "port name %s ", rta_getattr_str(tb[IFLA_PHYS_PORT_NAME]));
+
 	if (tb[IFLA_OPERSTATE])
 		print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
 
-- 
2.2.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] net: add support for phys_port_name
  2015-03-16 15:46 [PATCH 1/3] net: add support for phys_port_name David Ahern
  2015-03-16 15:46 ` [PATCH 2/3] rocker: " David Ahern
  2015-03-16 15:46 ` [PATCH 3/3] iproute2: Add " David Ahern
@ 2015-03-16 16:00 ` Jiri Pirko
  2015-03-16 16:02   ` David Ahern
  2015-03-17  3:29   ` David Ahern
  2 siblings, 2 replies; 13+ messages in thread
From: Jiri Pirko @ 2015-03-16 16:00 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, Scott Feldman

Mon, Mar 16, 2015 at 04:46:56PM CET, dsahern@gmail.com wrote:
>Similar to port id allow netdevices to specify port names and export
>the name via sysfs. Drivers can implement the netdevice operation to
>assist udev in having sane default names for the devices using the
>rule:
>
>$ cat /etc/udev/rules.d/80-net-setup-link.rules
>SUBSYSTEM=="net", ACTION=="add", ATTR{phys_port_name}!="",
>NAME="$attr{phys_port_name}"
>
>Use of phys_name versus phys_id was suggested-by Jiri Pirko.
>
>Signed-off-by: David Ahern <dsahern@gmail.com>
>Cc: Jiri Pirko <jiri@resnulli.us>
>Cc: Scott Feldman <sfeldma@gmail.com>
>---
> include/linux/netdevice.h    | 13 +++++++++++++
> include/uapi/linux/if_link.h |  1 +
> net/core/dev.c               | 18 ++++++++++++++++++
> net/core/net-sysfs.c         | 23 +++++++++++++++++++++++
> net/core/rtnetlink.c         | 21 +++++++++++++++++++++
> 5 files changed, 76 insertions(+)
>
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index dd1d069758be..47a773b9bee0 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -765,6 +765,15 @@ struct netdev_phys_item_id {
> 	unsigned char id_len;
> };
> 
>+#define MAX_PHYS_ITEM_NAME_LEN 32
>+
>+/* This structure holds a unique name to identify some
>+ * physical item (port for example) used by a netdevice.
>+ */
>+struct netdev_phys_item_name {
>+	char str[MAX_PHYS_ITEM_NAME_LEN];
>+};
>+
> typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
> 				       struct sk_buff *skb);
> 
>@@ -1159,6 +1168,8 @@ struct net_device_ops {
> 						      bool new_carrier);
> 	int			(*ndo_get_phys_port_id)(struct net_device *dev,
> 							struct netdev_phys_item_id *ppid);
>+	int			(*ndo_get_phys_port_name)(struct net_device *dev,
>+							  struct netdev_phys_item_name *name);

I think that we do not need the structure. Just pass "char *name" for buffer
where to put the name and "size_t len" for len of the buffer. Have:
#define PORT_NAME_MAX_LEN 32
and have called to have "char name[PORT_NAME_MAX_LEN]"

Also, given that this is related to switches, won't it make sense to
push this into switchdev code?


> 	void			(*ndo_add_vxlan_port)(struct  net_device *dev,
> 						      sa_family_t sa_family,
> 						      __be16 port);
>@@ -2939,6 +2950,8 @@ int dev_set_mac_address(struct net_device *, struct sockaddr *);
> int dev_change_carrier(struct net_device *, bool new_carrier);
> int dev_get_phys_port_id(struct net_device *dev,
> 			 struct netdev_phys_item_id *ppid);
>+int dev_get_phys_port_name(struct net_device *dev,
>+			   struct netdev_phys_item_name *name);
> struct sk_buff *validate_xmit_skb_list(struct sk_buff *skb, struct net_device *dev);
> struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> 				    struct netdev_queue *txq, int *ret);
>diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>index 756436e1ce89..7158fd00a109 100644
>--- a/include/uapi/linux/if_link.h
>+++ b/include/uapi/linux/if_link.h
>@@ -147,6 +147,7 @@ enum {
> 	IFLA_CARRIER_CHANGES,
> 	IFLA_PHYS_SWITCH_ID,
> 	IFLA_LINK_NETNSID,
>+	IFLA_PHYS_PORT_NAME,
> 	__IFLA_MAX
> };
> 
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 39fe369b46ad..38b04093e4c9 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -5912,6 +5912,24 @@ int dev_get_phys_port_id(struct net_device *dev,
> EXPORT_SYMBOL(dev_get_phys_port_id);
> 
> /**
>+ *	dev_get_phys_port_name - Get device physical port name
>+ *	@dev: device
>+ *	@name: port name
>+ *
>+ *	Get device physical port name
>+ */
>+int dev_get_phys_port_name(struct net_device *dev,
>+			   struct netdev_phys_item_name *name)
>+{
>+	const struct net_device_ops *ops = dev->netdev_ops;
>+
>+	if (!ops->ndo_get_phys_port_name)
>+		return -EOPNOTSUPP;
>+	return ops->ndo_get_phys_port_name(dev, name);
>+}
>+EXPORT_SYMBOL(dev_get_phys_port_name);
>+
>+/**
>  *	dev_new_index	-	allocate an ifindex
>  *	@net: the applicable net namespace
>  *
>diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
>index cf30620a88e1..beecfd499341 100644
>--- a/net/core/net-sysfs.c
>+++ b/net/core/net-sysfs.c
>@@ -418,6 +418,28 @@ static ssize_t phys_port_id_show(struct device *dev,
> }
> static DEVICE_ATTR_RO(phys_port_id);
> 
>+static ssize_t phys_port_name_show(struct device *dev,
>+				   struct device_attribute *attr, char *buf)
>+{
>+	struct net_device *netdev = to_net_dev(dev);
>+	ssize_t ret = -EINVAL;
>+
>+	if (!rtnl_trylock())
>+		return restart_syscall();
>+
>+	if (dev_isalive(netdev)) {
>+		struct netdev_phys_item_name name;
>+
>+		ret = dev_get_phys_port_name(netdev, &name);
>+		if (!ret)
>+			ret = sprintf(buf, "%s\n", name.str);
>+	}
>+	rtnl_unlock();
>+
>+	return ret;
>+}
>+static DEVICE_ATTR_RO(phys_port_name);
>+
> static ssize_t phys_switch_id_show(struct device *dev,
> 				   struct device_attribute *attr, char *buf)
> {
>@@ -465,6 +487,7 @@ static struct attribute *net_class_attrs[] = {
> 	&dev_attr_tx_queue_len.attr,
> 	&dev_attr_gro_flush_timeout.attr,
> 	&dev_attr_phys_port_id.attr,
>+	&dev_attr_phys_port_name.attr,
> 	&dev_attr_phys_switch_id.attr,
> 	NULL,
> };
>diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>index 25b4b5d23485..6f4d17e13db7 100644
>--- a/net/core/rtnetlink.c
>+++ b/net/core/rtnetlink.c
>@@ -982,6 +982,24 @@ static int rtnl_phys_port_id_fill(struct sk_buff *skb, struct net_device *dev)
> 	return 0;
> }
> 
>+static int rtnl_phys_port_name_fill(struct sk_buff *skb, struct net_device *dev)
>+{
>+	struct netdev_phys_item_name name;
>+	int err;
>+
>+	err = dev_get_phys_port_name(dev, &name);
>+	if (err) {
>+		if (err == -EOPNOTSUPP)
>+			return 0;
>+		return err;
>+	}
>+
>+	if (nla_put(skb, IFLA_PHYS_PORT_NAME, strlen(name.str), name.str))
>+		return -EMSGSIZE;
>+
>+	return 0;
>+}
>+
> static int rtnl_phys_switch_id_fill(struct sk_buff *skb, struct net_device *dev)
> {
> 	int err;
>@@ -1072,6 +1090,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
> 	if (rtnl_phys_port_id_fill(skb, dev))
> 		goto nla_put_failure;
> 
>+	if (rtnl_phys_port_name_fill(skb, dev))
>+		goto nla_put_failure;
>+
> 	if (rtnl_phys_switch_id_fill(skb, dev))
> 		goto nla_put_failure;
> 
>-- 
>2.2.1
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] net: add support for phys_port_name
  2015-03-16 16:00 ` [PATCH 1/3] net: add " Jiri Pirko
@ 2015-03-16 16:02   ` David Ahern
  2015-03-16 16:15     ` Jiri Pirko
  2015-03-17  3:29   ` David Ahern
  1 sibling, 1 reply; 13+ messages in thread
From: David Ahern @ 2015-03-16 16:02 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: netdev, Scott Feldman

On 3/16/15 10:00 AM, Jiri Pirko wrote:
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> >index dd1d069758be..47a773b9bee0 100644
>> >--- a/include/linux/netdevice.h
>> >+++ b/include/linux/netdevice.h
>> >@@ -765,6 +765,15 @@ struct netdev_phys_item_id {
>> >	unsigned char id_len;
>> >};
>> >
>> >+#define MAX_PHYS_ITEM_NAME_LEN 32
>> >+
>> >+/* This structure holds a unique name to identify some
>> >+ * physical item (port for example) used by a netdevice.
>> >+ */
>> >+struct netdev_phys_item_name {
>> >+	char str[MAX_PHYS_ITEM_NAME_LEN];
>> >+};
>> >+
>> >typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
>> >				       struct sk_buff *skb);
>> >
>> >@@ -1159,6 +1168,8 @@ struct net_device_ops {
>> >						      bool new_carrier);
>> >	int			(*ndo_get_phys_port_id)(struct net_device *dev,
>> >							struct netdev_phys_item_id *ppid);
>> >+	int			(*ndo_get_phys_port_name)(struct net_device *dev,
>> >+							  struct netdev_phys_item_name *name);
> I think that we do not need the structure. Just pass "char *name" for buffer
> where to put the name and "size_t len" for len of the buffer. Have:
> #define PORT_NAME_MAX_LEN 32
> and have called to have "char name[PORT_NAME_MAX_LEN]"
>
> Also, given that this is related to switches, won't it make sense to
> push this into switchdev code?
>
>

Doesn't seem right to have assumptions on buffer length like that.

David

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] net: add support for phys_port_name
  2015-03-16 16:02   ` David Ahern
@ 2015-03-16 16:15     ` Jiri Pirko
  0 siblings, 0 replies; 13+ messages in thread
From: Jiri Pirko @ 2015-03-16 16:15 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, Scott Feldman

Mon, Mar 16, 2015 at 05:02:09PM CET, dsahern@gmail.com wrote:
>On 3/16/15 10:00 AM, Jiri Pirko wrote:
>>>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>>index dd1d069758be..47a773b9bee0 100644
>>>>--- a/include/linux/netdevice.h
>>>>+++ b/include/linux/netdevice.h
>>>>@@ -765,6 +765,15 @@ struct netdev_phys_item_id {
>>>>	unsigned char id_len;
>>>>};
>>>>
>>>>+#define MAX_PHYS_ITEM_NAME_LEN 32
>>>>+
>>>>+/* This structure holds a unique name to identify some
>>>>+ * physical item (port for example) used by a netdevice.
>>>>+ */
>>>>+struct netdev_phys_item_name {
>>>>+	char str[MAX_PHYS_ITEM_NAME_LEN];
>>>>+};
>>>>+
>>>>typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
>>>>				       struct sk_buff *skb);
>>>>
>>>>@@ -1159,6 +1168,8 @@ struct net_device_ops {
>>>>						      bool new_carrier);
>>>>	int			(*ndo_get_phys_port_id)(struct net_device *dev,
>>>>							struct netdev_phys_item_id *ppid);
>>>>+	int			(*ndo_get_phys_port_name)(struct net_device *dev,
>>>>+							  struct netdev_phys_item_name *name);
>>I think that we do not need the structure. Just pass "char *name" for buffer
>>where to put the name and "size_t len" for len of the buffer. Have:
>>#define PORT_NAME_MAX_LEN 32
>>and have called to have "char name[PORT_NAME_MAX_LEN]"
>>
>>Also, given that this is related to switches, won't it make sense to
>>push this into switchdev code?
>>
>>
>
>Doesn't seem right to have assumptions on buffer length like that.

There are no assumptions. You pass real buffer length in len parameter.

>
>David

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] net: add support for phys_port_name
  2015-03-16 16:00 ` [PATCH 1/3] net: add " Jiri Pirko
  2015-03-16 16:02   ` David Ahern
@ 2015-03-17  3:29   ` David Ahern
  1 sibling, 0 replies; 13+ messages in thread
From: David Ahern @ 2015-03-17  3:29 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: netdev, Scott Feldman

On 3/16/15 10:00 AM, Jiri Pirko wrote:
>> @@ -1159,6 +1168,8 @@ struct net_device_ops {
>> 						      bool new_carrier);
>> 	int			(*ndo_get_phys_port_id)(struct net_device *dev,
>> 							struct netdev_phys_item_id *ppid);
>> +	int			(*ndo_get_phys_port_name)(struct net_device *dev,
>> +							  struct netdev_phys_item_name *name);
>
> I think that we do not need the structure. Just pass "char *name" for buffer
> where to put the name and "size_t len" for len of the buffer. Have:
> #define PORT_NAME_MAX_LEN 32
> and have called to have "char name[PORT_NAME_MAX_LEN]"
>
> Also, given that this is related to switches, won't it make sense to
> push this into switchdev code?


BTW, in regards to the latter comment phys_port_name should stay with 
phys_port_id which is currenly not a switchdev op.

David

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] iproute2: Add support for phys_port_name
  2015-03-16 15:46 ` [PATCH 3/3] iproute2: Add " David Ahern
@ 2015-03-24 22:50   ` Stephen Hemminger
  2015-03-27  3:41     ` David Ahern
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Hemminger @ 2015-03-24 22:50 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev

On Mon, 16 Mar 2015 09:46:58 -0600
David Ahern <dsahern@gmail.com> wrote:

> Display phys_port_name attribute if returned:
> 
> ./ip link show sw1p1
> 4: sw1p1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop port name sw1p1 state DOWN mode DEFAULT group default qlen 1000
>     link/ether 52:54:00:12:35:01 brd ff:ff:ff:ff:ff:ff
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  include/linux/if_link.h | 1 +
>  ip/ipaddress.c          | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/include/linux/if_link.h b/include/linux/if_link.h
> index 3450c3fbdc65..c5ee8c91b1c6 100644
> --- a/include/linux/if_link.h
> +++ b/include/linux/if_link.h
> @@ -147,6 +147,7 @@ enum {
>  	IFLA_CARRIER_CHANGES,
>  	IFLA_PHYS_SWITCH_ID,
>  	IFLA_LINK_NETNSID,
> +	IFLA_PHYS_PORT_NAME,
>  	__IFLA_MAX
>  };
>  
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 99a6ab5977e3..9e65015b7a48 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -645,6 +645,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
>  				      b1, sizeof(b1)));
>  	}
>  
> +	if (tb[IFLA_PHYS_PORT_NAME])
> +		fprintf(fp, "port name %s ", rta_getattr_str(tb[IFLA_PHYS_PORT_NAME]));
> +
>  	if (tb[IFLA_OPERSTATE])
>  		print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
>  

In general, this is a good idea, but there are several of issues.
1. There are programs that parse ip route output, therefore all new
   fields need to go at the end.
2. The two word format 'port name' does not match other parts
3. Extra stuff usually goes int 'details' section
4. There are VPN scripts etc that expect all ip commands to
   be invertable. I.e the output of ip link should also be easily
   convertable to input.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] iproute2: Add support for phys_port_name
  2015-03-24 22:50   ` Stephen Hemminger
@ 2015-03-27  3:41     ` David Ahern
  0 siblings, 0 replies; 13+ messages in thread
From: David Ahern @ 2015-03-27  3:41 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev

On 3/24/15 4:50 PM, Stephen Hemminger wrote:
>> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
>> index 99a6ab5977e3..9e65015b7a48 100644
>> --- a/ip/ipaddress.c
>> +++ b/ip/ipaddress.c
>> @@ -645,6 +645,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
>>   				      b1, sizeof(b1)));
>>   	}
>>
>> +	if (tb[IFLA_PHYS_PORT_NAME])
>> +		fprintf(fp, "port name %s ", rta_getattr_str(tb[IFLA_PHYS_PORT_NAME]));
>> +
>>   	if (tb[IFLA_OPERSTATE])
>>   		print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
>>
>
> In general, this is a good idea, but there are several of issues.
> 1. There are programs that parse ip route output, therefore all new
>     fields need to go at the end.
> 2. The two word format 'port name' does not match other parts
> 3. Extra stuff usually goes int 'details' section
> 4. There are VPN scripts etc that expect all ip commands to
>     be invertable. I.e the output of ip link should also be easily
>     convertable to input.

The location of PORT_NAME is right after PORT_ID. I take it from this 
list you want it elsewhere. At the end?

And rather than 'port name' how about 'port-name'? no precedence for 
CamelCase and both 'port' and 'name' are too generic. 'pname' is not 
intuitive. Suggestions?

David

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] iproute2: Add support for phys_port_name
  2015-03-18  2:23 [PATCH v4 1/3] net: add " David Ahern
@ 2015-03-18  2:23 ` David Ahern
  0 siblings, 0 replies; 13+ messages in thread
From: David Ahern @ 2015-03-18  2:23 UTC (permalink / raw)
  To: netdev, jiri, sfeldma; +Cc: David Ahern

Display phys_port_name attribute if returned:

./ip link show sw1p1
4: sw1p1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop port name sw1p1 state DOWN mode DEFAULT group default qlen 1000
    link/ether 52:54:00:12:35:01 brd ff:ff:ff:ff:ff:ff

Signed-off-by: David Ahern <dsahern@gmail.com>
Acked-by: Jiri Pirko <jiri@resnulli.us>
Cc: Scott Feldman <sfeldma@gmail.com>

---
 include/linux/if_link.h | 1 +
 ip/ipaddress.c          | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 3450c3fbdc65..c5ee8c91b1c6 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -147,6 +147,7 @@ enum {
 	IFLA_CARRIER_CHANGES,
 	IFLA_PHYS_SWITCH_ID,
 	IFLA_LINK_NETNSID,
+	IFLA_PHYS_PORT_NAME,
 	__IFLA_MAX
 };
 
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 99a6ab5977e3..9e65015b7a48 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -645,6 +645,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
 				      b1, sizeof(b1)));
 	}
 
+	if (tb[IFLA_PHYS_PORT_NAME])
+		fprintf(fp, "port name %s ", rta_getattr_str(tb[IFLA_PHYS_PORT_NAME]));
+
 	if (tb[IFLA_OPERSTATE])
 		print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
 
-- 
2.2.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] iproute2: Add support for phys_port_name
  2015-03-17 15:30 [PATCH v3 1/3] net: add " David Ahern
@ 2015-03-17 15:30 ` David Ahern
  0 siblings, 0 replies; 13+ messages in thread
From: David Ahern @ 2015-03-17 15:30 UTC (permalink / raw)
  To: netdev, jiri, sfeldma; +Cc: David Ahern

Display phys_port_name attribute if returned:

./ip link show sw1p1
4: sw1p1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop port name sw1p1 state DOWN mode DEFAULT group default qlen 1000
    link/ether 52:54:00:12:35:01 brd ff:ff:ff:ff:ff:ff

Signed-off-by: David Ahern <dsahern@gmail.com>
Acked-by: Jiri Pirko <jiri@resnulli.us>
Cc: Scott Feldman <sfeldma@gmail.com>

---
 include/linux/if_link.h | 1 +
 ip/ipaddress.c          | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 3450c3fbdc65..c5ee8c91b1c6 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -147,6 +147,7 @@ enum {
 	IFLA_CARRIER_CHANGES,
 	IFLA_PHYS_SWITCH_ID,
 	IFLA_LINK_NETNSID,
+	IFLA_PHYS_PORT_NAME,
 	__IFLA_MAX
 };
 
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 99a6ab5977e3..9e65015b7a48 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -645,6 +645,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
 				      b1, sizeof(b1)));
 	}
 
+	if (tb[IFLA_PHYS_PORT_NAME])
+		fprintf(fp, "port name %s ", rta_getattr_str(tb[IFLA_PHYS_PORT_NAME]));
+
 	if (tb[IFLA_OPERSTATE])
 		print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
 
-- 
2.2.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] iproute2: Add support for phys_port_name
  2015-03-17  3:27 ` [PATCH 3/3] iproute2: Add " David Ahern
@ 2015-03-17  7:20   ` Jiri Pirko
  0 siblings, 0 replies; 13+ messages in thread
From: Jiri Pirko @ 2015-03-17  7:20 UTC (permalink / raw)
  To: David Ahern; +Cc: netdev, Scott Feldman

Tue, Mar 17, 2015 at 04:27:17AM CET, dsahern@gmail.com wrote:
>Display phys_port_name attribute if returned:
>
>./ip link show sw1p1
>4: sw1p1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop port name sw1p1 state DOWN mode DEFAULT group default qlen 1000
>    link/ether 52:54:00:12:35:01 brd ff:ff:ff:ff:ff:ff
>
>Signed-off-by: David Ahern <dsahern@gmail.com>
>Cc: Jiri Pirko <jiri@resnulli.us>
>Cc: Scott Feldman <sfeldma@gmail.com>

Acked-by: Jiri Pirko <jiri@resnulli.us>


>
>---
> include/linux/if_link.h | 1 +
> ip/ipaddress.c          | 3 +++
> 2 files changed, 4 insertions(+)
>
>diff --git a/include/linux/if_link.h b/include/linux/if_link.h
>index 3450c3fbdc65..c5ee8c91b1c6 100644
>--- a/include/linux/if_link.h
>+++ b/include/linux/if_link.h
>@@ -147,6 +147,7 @@ enum {
> 	IFLA_CARRIER_CHANGES,
> 	IFLA_PHYS_SWITCH_ID,
> 	IFLA_LINK_NETNSID,
>+	IFLA_PHYS_PORT_NAME,
> 	__IFLA_MAX
> };
> 
>diff --git a/ip/ipaddress.c b/ip/ipaddress.c
>index 99a6ab5977e3..9e65015b7a48 100644
>--- a/ip/ipaddress.c
>+++ b/ip/ipaddress.c
>@@ -645,6 +645,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
> 				      b1, sizeof(b1)));
> 	}
> 
>+	if (tb[IFLA_PHYS_PORT_NAME])
>+		fprintf(fp, "port name %s ", rta_getattr_str(tb[IFLA_PHYS_PORT_NAME]));
>+
> 	if (tb[IFLA_OPERSTATE])
> 		print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
> 
>-- 
>2.2.1
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] iproute2: Add support for phys_port_name
  2015-03-17  3:27 [PATCH v2 " David Ahern
@ 2015-03-17  3:27 ` David Ahern
  2015-03-17  7:20   ` Jiri Pirko
  0 siblings, 1 reply; 13+ messages in thread
From: David Ahern @ 2015-03-17  3:27 UTC (permalink / raw)
  To: netdev; +Cc: David Ahern, Jiri Pirko, Scott Feldman

Display phys_port_name attribute if returned:

./ip link show sw1p1
4: sw1p1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop port name sw1p1 state DOWN mode DEFAULT group default qlen 1000
    link/ether 52:54:00:12:35:01 brd ff:ff:ff:ff:ff:ff

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: Scott Feldman <sfeldma@gmail.com>

---
 include/linux/if_link.h | 1 +
 ip/ipaddress.c          | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 3450c3fbdc65..c5ee8c91b1c6 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -147,6 +147,7 @@ enum {
 	IFLA_CARRIER_CHANGES,
 	IFLA_PHYS_SWITCH_ID,
 	IFLA_LINK_NETNSID,
+	IFLA_PHYS_PORT_NAME,
 	__IFLA_MAX
 };
 
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 99a6ab5977e3..9e65015b7a48 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -645,6 +645,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
 				      b1, sizeof(b1)));
 	}
 
+	if (tb[IFLA_PHYS_PORT_NAME])
+		fprintf(fp, "port name %s ", rta_getattr_str(tb[IFLA_PHYS_PORT_NAME]));
+
 	if (tb[IFLA_OPERSTATE])
 		print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
 
-- 
2.2.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-03-27  3:41 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-16 15:46 [PATCH 1/3] net: add support for phys_port_name David Ahern
2015-03-16 15:46 ` [PATCH 2/3] rocker: " David Ahern
2015-03-16 15:46 ` [PATCH 3/3] iproute2: Add " David Ahern
2015-03-24 22:50   ` Stephen Hemminger
2015-03-27  3:41     ` David Ahern
2015-03-16 16:00 ` [PATCH 1/3] net: add " Jiri Pirko
2015-03-16 16:02   ` David Ahern
2015-03-16 16:15     ` Jiri Pirko
2015-03-17  3:29   ` David Ahern
2015-03-17  3:27 [PATCH v2 " David Ahern
2015-03-17  3:27 ` [PATCH 3/3] iproute2: Add " David Ahern
2015-03-17  7:20   ` Jiri Pirko
2015-03-17 15:30 [PATCH v3 1/3] net: add " David Ahern
2015-03-17 15:30 ` [PATCH 3/3] iproute2: Add " David Ahern
2015-03-18  2:23 [PATCH v4 1/3] net: add " David Ahern
2015-03-18  2:23 ` [PATCH 3/3] iproute2: Add " David Ahern

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.