From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [PATCH net-next] net: Add phys_port identifier to struct net_device and export it to sysfs Date: Sun, 21 Jul 2013 09:24:55 +0200 Message-ID: <20130721072455.GA1944@minipsycho.orion> References: <20130617181004.GA1364@fedora-17-guest.dell.com> <20130711203938.GA4078@minipsycho.orion> <20130715153410.GA10864@fedora18-dev.oslab.blr.amer.dell.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Narendra_K@dell.com, netdev@vger.kernel.org, bhutchings@solarflare.com, john.r.fastabend@intel.com To: Or Gerlitz Return-path: Received: from mail-ea0-f169.google.com ([209.85.215.169]:52759 "EHLO mail-ea0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752578Ab3GUHZA (ORCPT ); Sun, 21 Jul 2013 03:25:00 -0400 Received: by mail-ea0-f169.google.com with SMTP id h15so3197903eak.0 for ; Sun, 21 Jul 2013 00:24:59 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: Sun, Jul 21, 2013 at 07:55:41AM CEST, or.gerlitz@gmail.com wrote: >On Mon, Jul 15, 2013 at 6:34 PM, wrote: >[...] >> It is implemented in this patch here - >> [RFC,net-next] net: Include phys_port identifier in the RTM_NEWLINK >> message http://patchwork.ozlabs.org/patch/255435/ >> >> Please find the patch with the changes incorporated. If the changes look >> good, i will incorporate them in the next version of the patch. >> >> [PATCH net-next ] Add phys_port identifier to struct net_device and export it to sysfs >> >> 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, but not >> restricted to these spaces. 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//phys_port > >Hi Narendra, John, Ben, Jiri, > >Before diving to the eswitch use cases of this patch/approach, I was >wondering if/how the new >phys_port field could be of use for drivers that just want to place >there an INT saying if this interface >is associated with port #N of the PCI device (e.g N={1,2}). I wasn't >sure how this is supposted >to be done if the semantics of the new field is the one mentioned >above in the change long >(MAC-48 or EUI-64 or a 128 bit UUID value). Both mlx4 and ipoib driver >use the dev_id field and would >be happily ported to the new interface once introduced in a way that >allows to meet the integer requirement. The value could be anything. But note that you have to have different values for card1-port1,2 and card2-port1,2 > >Or. > >> 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 >> --- >> include/linux/netdevice.h | 14 ++++++++++++++ >> net/core/net-sysfs.c | 22 ++++++++++++++++++++++ >> 2 files changed, 36 insertions(+) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index 0741a1e..17db7e5 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -1062,6 +1062,14 @@ struct net_device_ops { >> bool new_carrier); >> }; >> >> +/* This structure holds a universally unique identifier to >> + * identify the physical port used by a netdevice >> + */ >> +struct phys_port_identifier { >> + unsigned char port_id[MAX_ADDR_LEN]; >> + unsigned char port_id_len; >> +}; >> + >> /* >> * The DEVICE structure. >> * Actually, this whole structure is a big mistake. It mixes I/O >> @@ -1181,6 +1189,11 @@ struct net_device { >> * that share the same >> link >> * layer address >> */ >> + struct phys_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 >> */ >> @@ -1633,6 +1646,7 @@ struct packet_offload { >> #define NETDEV_NOTIFY_PEERS 0x0013 >> #define NETDEV_JOIN 0x0014 >> #define NETDEV_CHANGEUPPER 0x0015 >> +#define NETDEV_CHANGE_PHYS_PORT 0x0016 >> >> extern int register_netdevice_notifier(struct notifier_block *nb); >> extern int unregister_netdevice_notifier(struct notifier_block *nb); >> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c >> index 981fed3..b77ebe6 100644 >> --- a/net/core/net-sysfs.c >> +++ b/net/core/net-sysfs.c >> @@ -334,6 +334,27 @@ 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 char len; >> + >> + read_lock(&dev_base_lock); >> + if (dev_isalive(net)) { >> + len = net->phys_port.port_id_len; >> + if (!len) >> + ret = 0; >> + else >> + ret = sysfs_format_mac(buf, >> net->phys_port.port_id, >> + len); >> + } >> + read_unlock(&dev_base_lock); >> + >> + 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 +376,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), >> {} >> }; >> >> -- >> 1.8.0.1 >> >> -- >> With regards, >> Narendra K >> Linux Engineering >> Dell Inc. >> -- >> 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