| Message ID | 20130613154253.GA1380@fedora-17-guest.blr.amer.dell.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2013-06-13 at 08:45 -0700, Narendra_K@Dell.com wrote: [...] > The requirement is to have a generic interface using which > kernel/drivers can provide information/hints to user space about the > physical port number used by a network interface. > > The following options were explored - > > 1. 'dev_id' sysfs attribute: > > In addition to being used to differentiate between devices that share > the same link layer address, it is being used to indicate the physical > port number used by a network interface. This last bit is no longer true. [...] > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1059,6 +1059,18 @@ struct net_device_ops { > bool new_carrier); > }; > > +/* This structure holds a universally unique identifier to > + * identify the physical port used by a netdevice > + */ > +struct port_identifier { > + union { > + u8 mac48[6]; > + u8 eui64[8]; > + u8 uuid[16]; > + } id; > + unsigned short length; > +}; I can't think why those three ID-spaces would not be sufficient, but I also don't see any reason to restrict to them. I think it's preferable to use a simple array of bytes, like for the link-layer address: unsigned char port_id[MAX_ADDR_LEN]; unsigned char port_id_len; [...] > --- a/net/core/net-sysfs.c > +++ b/net/core/net-sysfs.c > @@ -334,6 +334,36 @@ static ssize_t store_group(struct device *dev, struct device_attribute *attr, > return netdev_store(dev, attr, buf, len, change_group); > } > > +static ssize_t show_phys_port(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct net_device *net = to_net_dev(dev); > + ssize_t ret = -EINVAL; > + unsigned short len; > + > + if (!dev_isalive(net)) > + return ret; > + > + len = net->phys_port.length; > + > + switch (len) { > + case 6: > + ret = sysfs_format_mac(buf, net->phys_port.id.mac48, len); > + break; > + case 8: > + ret = sysfs_format_mac(buf, net->phys_port.id.eui64, len); > + break; > + case 16: > + ret = scnprintf(buf, PAGE_SIZE, "%pU\n", > + net->phys_port.id.uuid); > + break; I see, you want to use conventional UUID formatting. But since the port ID might or might not be a UUID, I don't think it's that helpful to userland consumers. I think it would be preferable to always use sysfs_format_mac(). > + default: Zero-length is the normal case now and I think it should result in returning 0 (=> empty file) not -EINVAL. Ben. > + break; > + } > + > + return ret; > +} [...]
On Thu, Jun 13, 2013 at 11:31:28PM +0530, Ben Hutchings wrote: > > On Thu, 2013-06-13 at 08:45 -0700, Narendra_K@Dell.com wrote: > [...] > > The requirement is to have a generic interface using which > > kernel/drivers can provide information/hints to user space about the > > physical port number used by a network interface. > > > > The following options were explored - > > > > 1. 'dev_id' sysfs attribute: > > > > In addition to being used to differentiate between devices that share > > the same link layer address, it is being used to indicate the physical > > port number used by a network interface. > > This last bit is no longer true. Right. mlx4_en driver is setting dev_id. I have sent a RFC patch to not set dev_id. I will modify the commit message in the next version of the patch. > > [...] > > --- a/include/linux/netdevice.h > > +++ b/include/linux/netdevice.h > > @@ -1059,6 +1059,18 @@ struct net_device_ops { > > bool new_carrier); > > }; > > > > +/* This structure holds a universally unique identifier to > > + * identify the physical port used by a netdevice > > + */ > > +struct port_identifier { > > + union { > > + u8 mac48[6]; > > + u8 eui64[8]; > > + u8 uuid[16]; > > + } id; > > + unsigned short length; > > +}; > > I can't think why those three ID-spaces would not be sufficient, but I > also don't see any reason to restrict to them. I think it's preferable > to use a simple array of bytes, like for the link-layer address: > > unsigned char port_id[MAX_ADDR_LEN]; > unsigned char port_id_len; > Ok. With the above change, the switch statement below is probably not required. Could it be like if (len < 0 || > MAX_ADDR_LEN) return -EINVAL; if (!len) return 0; ret = sysfs_format_mac(buf, net->phys_port.port_id, len); > [...] > > --- a/net/core/net-sysfs.c > > +++ b/net/core/net-sysfs.c > > @@ -334,6 +334,36 @@ static ssize_t store_group(struct device *dev, struct > device_attribute *attr, > > return netdev_store(dev, attr, buf, len, change_group); > > } > > > > +static ssize_t show_phys_port(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct net_device *net = to_net_dev(dev); > > + ssize_t ret = -EINVAL; > > + unsigned short len; > > + > > + if (!dev_isalive(net)) > > + return ret; > > + > > + len = net->phys_port.length; > > + > > + switch (len) { > > + case 6: > > + ret = sysfs_format_mac(buf, net->phys_port.id.mac48, len); > > + break; > > + case 8: > > + ret = sysfs_format_mac(buf, net->phys_port.id.eui64, len); > > + break; > > + case 16: > > + ret = scnprintf(buf, PAGE_SIZE, "%pU\n", > > + net->phys_port.id.uuid); > > + break; > > I see, you want to use conventional UUID formatting. But since the port > ID might or might not be a UUID, I don't think it's that helpful to > userland consumers. I think it would be preferable to always use > sysfs_format_mac(). Ok, I will use sysfs_format_mac(). > > > + default: > > Zero-length is the normal case now and I think it should result in > returning 0 (=> empty file) not -EINVAL. > > Ben. > > > + break; > > + } > > + > > + return ret; > > +} > [...] > Thank you for review comments. I will make the changes in the next version. > -- > Ben Hutchings, Staff Engineer, Solarflare > Not speaking for my employer; that's the marketing department's job. > They asked us to note that Solarflare product names are trademarked. > >
On Fri, 2013-06-14 at 08:22 -0700, Narendra_K@Dell.com wrote: > On Thu, Jun 13, 2013 at 11:31:28PM +0530, Ben Hutchings wrote: > > > > On Thu, 2013-06-13 at 08:45 -0700, Narendra_K@Dell.com wrote: > > [...] > > > The requirement is to have a generic interface using which > > > kernel/drivers can provide information/hints to user space about the > > > physical port number used by a network interface. > > > > > > The following options were explored - > > > > > > 1. 'dev_id' sysfs attribute: > > > > > > In addition to being used to differentiate between devices that share > > > the same link layer address, it is being used to indicate the physical > > > port number used by a network interface. > > > > This last bit is no longer true. > > Right. mlx4_en driver is setting dev_id. I have sent a RFC patch to > not set dev_id. I will modify the commit message in the next version > of the patch. Oh, I forgot that mlx4_en also misused dev_id. > > > > [...] > > > --- a/include/linux/netdevice.h > > > +++ b/include/linux/netdevice.h > > > @@ -1059,6 +1059,18 @@ struct net_device_ops { > > > bool new_carrier); > > > }; > > > > > > +/* This structure holds a universally unique identifier to > > > + * identify the physical port used by a netdevice > > > + */ > > > +struct port_identifier { > > > + union { > > > + u8 mac48[6]; > > > + u8 eui64[8]; > > > + u8 uuid[16]; > > > + } id; > > > + unsigned short length; > > > +}; > > > > I can't think why those three ID-spaces would not be sufficient, but I > > also don't see any reason to restrict to them. I think it's preferable > > to use a simple array of bytes, like for the link-layer address: > > > > unsigned char port_id[MAX_ADDR_LEN]; > > unsigned char port_id_len; > > > > Ok. With the above change, the switch statement below is probably not > required. Could it be like > > if (len < 0 || > MAX_ADDR_LEN) I don't think any range check is needed at all. If the port_id_len is out of ragne that's an obvious bug in the driver which I don't think you need to check for. (The networking core doesn't currently check addr_len, for example.) And an unsigned length cannot be < 0 anyway! > return -EINVAL; > if (!len) > return 0; > > ret = sysfs_format_mac(buf, net->phys_port.port_id, len); [...] Right. Ben.
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index e5d6557..1dfb79f 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1059,6 +1059,18 @@ struct net_device_ops { bool new_carrier); }; +/* This structure holds a universally unique identifier to + * identify the physical port used by a netdevice + */ +struct port_identifier { + union { + u8 mac48[6]; + u8 eui64[8]; + u8 uuid[16]; + } id; + unsigned short length; +}; + /* * The DEVICE structure. * Actually, this whole structure is a big mistake. It mixes I/O @@ -1178,6 +1190,11 @@ struct net_device { * that share the same link * layer address */ + struct port_identifier phys_port; /* Universally unique physical + * port identifier, MAC-48 or + * EUI-64 or 128 bit UUID, + * length is zero if not set + */ spinlock_t addr_list_lock; struct netdev_hw_addr_list uc; /* Unicast mac addresses */ struct netdev_hw_addr_list mc; /* Multicast mac addresses */ diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index 981fed3..9338f44 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -334,6 +334,36 @@ static ssize_t store_group(struct device *dev, struct device_attribute *attr, return netdev_store(dev, attr, buf, len, change_group); } +static ssize_t show_phys_port(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct net_device *net = to_net_dev(dev); + ssize_t ret = -EINVAL; + unsigned short len; + + if (!dev_isalive(net)) + return ret; + + len = net->phys_port.length; + + switch (len) { + case 6: + ret = sysfs_format_mac(buf, net->phys_port.id.mac48, len); + break; + case 8: + ret = sysfs_format_mac(buf, net->phys_port.id.eui64, len); + break; + case 16: + ret = scnprintf(buf, PAGE_SIZE, "%pU\n", + net->phys_port.id.uuid); + break; + default: + break; + } + + return ret; +} + static struct device_attribute net_class_attributes[] = { __ATTR(addr_assign_type, S_IRUGO, show_addr_assign_type, NULL), __ATTR(addr_len, S_IRUGO, show_addr_len, NULL), @@ -355,6 +385,7 @@ static struct device_attribute net_class_attributes[] = { __ATTR(tx_queue_len, S_IRUGO | S_IWUSR, show_tx_queue_len, store_tx_queue_len), __ATTR(netdev_group, S_IRUGO | S_IWUSR, show_group, store_group), + __ATTR(phys_port, S_IRUGO, show_phys_port, NULL), {} };
It is useful to know if network interfaces from NIC partitions 'map to/use the' same physical port. For example, when creating bonding in fault tolerance mode, if two network interfaces map to/use the same physical port, it might not have the desired result. This information is not available today in a standard format or it is not present. If this information can be made available in a generic way to user space, tools such as NetworkManager or Libteam or Wicked can make smarter bonding decisions (such as warn users when setting up configurations which will not have desired effect). The requirement is to have a generic interface using which kernel/drivers can provide information/hints to user space about the physical port number used by a network interface. The following options were explored - 1. 'dev_id' sysfs attribute: In addition to being used to differentiate between devices that share the same link layer address, it is being used to indicate the physical port number used by a network interface. As dev_id exists to differentiate between devices sharing the same link layer address, dev_id option is not selected. 2. Re-using 'if_port' field in 'struct net_device': if_port field exists to indicate the media type(please refer to netdevice.h). It seemed like it was also used to indicate the physical port number. As re-using 'if_port' might possibly break user space, this option is not selected. 3. Add a new field 'phys_port' to 'struct net_device' and export it to sysfs: The 'phys_port' will be a universally unique identifier, which would be a MAC-48 or EUI-64 or a 128 bit UUID value. It will uniquely identify the physical port used by a network interface. The 'length' of the identifier will be zero if the field is not set for a network interface. This patch implements option 3. It creates a new sysfs attribute 'phys_port' - /sys/class/net/<interface name>/phys_port References: http://marc.info/?l=linux-netdev&m=136920998009209&w=2 References: http://marc.info/?l=linux-netdev&m=136992041432498&w=2 Signed-off-by: Narendra K <narendra_k@dell.com> --- include/linux/netdevice.h | 17 +++++++++++++++++ net/core/net-sysfs.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+)