Re: [PATCH net-next] net: Add phys_port identifier to struct net_device and export it to sysfs

[Or Gerlitz <or.gerlitz@gmail.com>]

On Mon, Jul 15, 2013 at 6:34 PM, <Narendra_K@dell.com> 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/<interface name>/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.

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 <narendra_k@dell.com>
> ---
>  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