[dpdk-dev] [PATCH 0/2] User-space Ethtool

[dpdk-dev] [PATCH 0/2] User-space Ethtool

[Liang-Min Larry Wang]

This implementation is designed to provide a familar interface for applications that rely on kernel-space driver to support ethtool_op and net_device_op for device management. The initial implementation focuses on ops that can be implemented through existing netdev APIs. More ops will be supported in latter release.
ethtool: adding new ethtool api support

v2 change:
- Implement rte_eth_dev_default_mac_addr_set through dev_ops::mac_addr_set so it would support NIC devices other than ixgbe and igb

Liang-Min Larry Wang (2):
  ethdev: add api to set default mac address
  ethtool: add new library to provide ethtool-alike APIs

 MAINTAINERS                                |   4 +
 config/common_linuxapp                     |   5 +
 lib/Makefile                               |   1 +
 lib/librte_ether/rte_ethdev.c              |  16 ++
 lib/librte_ether/rte_ethdev.h              |  14 ++
 lib/librte_ether/rte_ether_version.map     |   1 +
 lib/librte_ethtool/Makefile                |  56 +++++++
 lib/librte_ethtool/rte_ethtool.c           | 155 +++++++++++++++++
 lib/librte_ethtool/rte_ethtool.h           | 257 +++++++++++++++++++++++++++++
 lib/librte_ethtool/rte_ethtool_version.map |  18 ++
 mk/rte.app.mk                              |   1 +
 11 files changed, 528 insertions(+)
 create mode 100644 lib/librte_ethtool/Makefile
 create mode 100644 lib/librte_ethtool/rte_ethtool.c
 create mode 100644 lib/librte_ethtool/rte_ethtool.h
 create mode 100644 lib/librte_ethtool/rte_ethtool_version.map

-- 
2.1.4

[dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address

[Liang-Min Larry Wang]

add a new api: rte_eth_dev_default_mac_addr_set to
support changing default mac address of a NIC

Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
---
 lib/librte_ether/rte_ethdev.c          | 16 ++++++++++++++++
 lib/librte_ether/rte_ethdev.h          | 14 ++++++++++++++
 lib/librte_ether/rte_ether_version.map |  1 +
 3 files changed, 31 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 024fe8b..96ee00e 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -2752,6 +2752,22 @@ rte_eth_dev_mac_addr_remove(uint8_t port_id, struct ether_addr *addr)
 }
 
 int
+rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr *addr)
+{
+	struct rte_eth_dev *dev;
+
+	if (!rte_eth_dev_is_valid_port(port_id)) {
+		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+		return -ENODEV;
+	}
+
+	dev = &rte_eth_devices[port_id];
+	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
+
+	return (*dev->dev_ops->mac_addr_set)(dev, addr);
+}
+
+int
 rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
 				uint16_t rx_mode, uint8_t on)
 {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 16dbe00..5f07e0d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2982,6 +2982,20 @@ int rte_eth_dev_mac_addr_add(uint8_t port, struct ether_addr *mac_addr,
 int rte_eth_dev_mac_addr_remove(uint8_t port, struct ether_addr *mac_addr);
 
 /**
+ * Set the default MAC address.
+ *
+ * @param port
+ *   The port identifier of the Ethernet device.
+ * @param mac_addr
+ *   New default MAC address.
+ * @return
+ *   - (0) if successful, or *mac_addr* didn't exist.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port* invalid.
+ */
+int rte_eth_dev_default_mac_addr_set(uint8_t port, struct ether_addr *mac_addr);
+
+/**
  * Update Redirection Table(RETA) of Receive Side Scaling of Ethernet device.
  *
  * @param port
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index a2d25a6..2dbbaa7 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -102,6 +102,7 @@ DPDK_2.0 {
 	rte_eth_tx_queue_setup;
 	rte_eth_xstats_get;
 	rte_eth_xstats_reset;
+	rte_eth_dev_default_mac_addr_set;
 
 	local: *;
 };
-- 
2.1.4

[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Liang-Min Larry Wang]

adding a new library based upon ethdev APIs to provide API's that bear
the same functionality as ethtool_ops (linux/ethtool.h) and net_device_ops
(linux/netdevice.h).

Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
---
 MAINTAINERS                                |   4 +
 config/common_linuxapp                     |   5 +
 lib/Makefile                               |   1 +
 lib/librte_ethtool/Makefile                |  56 +++++++
 lib/librte_ethtool/rte_ethtool.c           | 155 +++++++++++++++++
 lib/librte_ethtool/rte_ethtool.h           | 257 +++++++++++++++++++++++++++++
 lib/librte_ethtool/rte_ethtool_version.map |  18 ++
 mk/rte.app.mk                              |   1 +
 8 files changed, 497 insertions(+)
 create mode 100644 lib/librte_ethtool/Makefile
 create mode 100644 lib/librte_ethtool/rte_ethtool.c
 create mode 100644 lib/librte_ethtool/rte_ethtool.h
 create mode 100644 lib/librte_ethtool/rte_ethtool_version.map

diff --git a/MAINTAINERS b/MAINTAINERS
index 9362c19..b8b481f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -186,6 +186,10 @@ M: Thomas Monjalon <thomas.monjalon@6wind.com>
 F: lib/librte_ether/
 F: scripts/test-null.sh
 
+Ethtool API
+M: Liang-Min Larry Wang <liang-min.wang@intel.com>
+F: lib/librte_ethtool/
+
 
 Drivers
 -------
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 0078dc9..f5759fd 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -129,6 +129,11 @@ CONFIG_RTE_LIBRTE_EAL_VMWARE_TSC_MAP_SUPPORT=y
 CONFIG_RTE_LIBRTE_KVARGS=y
 
 #
+# Compile user-space ethtool library
+#
+CONFIG_RTE_LIBRTE_ETHTOOL=y
+
+#
 # Compile generic ethernet library
 #
 CONFIG_RTE_LIBRTE_ETHER=y
diff --git a/lib/Makefile b/lib/Makefile
index 5f480f9..a6c7375 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -41,6 +41,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_TIMER) += librte_timer
 DIRS-$(CONFIG_RTE_LIBRTE_CFGFILE) += librte_cfgfile
 DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += librte_cmdline
 DIRS-$(CONFIG_RTE_LIBRTE_ETHER) += librte_ether
+DIRS-$(CONFIG_RTE_LIBRTE_ETHTOOL) += librte_ethtool
 DIRS-$(CONFIG_RTE_LIBRTE_VHOST) += librte_vhost
 DIRS-$(CONFIG_RTE_LIBRTE_HASH) += librte_hash
 DIRS-$(CONFIG_RTE_LIBRTE_LPM) += librte_lpm
diff --git a/lib/librte_ethtool/Makefile b/lib/librte_ethtool/Makefile
new file mode 100644
index 0000000..1d981f6
--- /dev/null
+++ b/lib/librte_ethtool/Makefile
@@ -0,0 +1,56 @@
+#   BSD LICENSE
+#
+#   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+#   All rights reserved.
+#
+#   Redistribution and use in source and binary forms, with or without
+#   modification, are permitted provided that the following conditions
+#   are met:
+#
+#     * Redistributions of source code must retain the above copyright
+#       notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above copyright
+#       notice, this list of conditions and the following disclaimer in
+#       the documentation and/or other materials provided with the
+#       distribution.
+#     * Neither the name of Intel Corporation nor the names of its
+#       contributors may be used to endorse or promote products derived
+#       from this software without specific prior written permission.
+#
+#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+include $(RTE_SDK)/mk/rte.vars.mk
+
+#
+# library name
+#
+LIB = librte_ethtool.a
+
+CFLAGS += -O3
+CFLAGS += $(WERROR_FLAGS)
+
+EXPORT_MAP := rte_ethtool_version.map
+
+LIBABIVER := 1
+
+SRCS-y += rte_ethtool.c
+
+#
+# Export include files
+#
+SYMLINK-y-include += rte_ethtool.h
+
+# this lib depends upon:
+DEPDIRS-y += lib/librte_ether
+
+include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/lib/librte_ethtool/rte_ethtool.c b/lib/librte_ethtool/rte_ethtool.c
new file mode 100644
index 0000000..2ccf06f
--- /dev/null
+++ b/lib/librte_ethtool/rte_ethtool.c
@@ -0,0 +1,155 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <rte_version.h>
+#include <rte_ethdev.h>
+#include "rte_ethtool.h"
+
+int
+rte_ethtool_get_drvinfo(uint8_t port_id, struct ethtool_drvinfo *drvinfo)
+{
+	struct rte_eth_dev_info dev_info;
+
+	memset(&dev_info, 0, sizeof(dev_info));
+	rte_eth_dev_info_get(port_id, &dev_info);
+
+	snprintf(drvinfo->driver, sizeof(drvinfo->driver), "%s",
+		dev_info.driver_name);
+	snprintf(drvinfo->version, sizeof(drvinfo->version), "%s",
+		rte_version());
+	snprintf(drvinfo->bus_info, sizeof(drvinfo->bus_info),
+		"%04x:%02x:%02x.%x",
+		dev_info.pci_dev->addr.domain, dev_info.pci_dev->addr.bus,
+		dev_info.pci_dev->addr.devid, dev_info.pci_dev->addr.function);
+
+	drvinfo->n_stats = sizeof(struct rte_eth_stats) / sizeof(uint64_t);
+	drvinfo->testinfo_len = 0;
+
+	return 0;
+}
+
+int
+rte_ethtool_get_link(uint8_t port_id)
+{
+	struct rte_eth_link link;
+
+	rte_eth_link_get(port_id, &link);
+	return link.link_status;
+}
+
+int
+rte_ethtool_net_open(uint8_t port_id)
+{
+	rte_eth_dev_stop(port_id);
+
+	return rte_eth_dev_start(port_id);
+}
+
+int
+rte_ethtool_net_stop(uint8_t port_id)
+{
+	rte_eth_dev_stop(port_id);
+
+	return 0;
+}
+
+int
+rte_ethtool_net_get_mac_addr(uint8_t port_id, struct ether_addr *addr)
+{
+	rte_eth_macaddr_get(port_id, addr);
+
+	return 0;
+}
+
+int
+rte_ethtool_net_set_mac_addr(uint8_t port_id, struct ether_addr *addr)
+{
+	return rte_eth_dev_default_mac_addr_set(port_id, addr);
+}
+
+int
+rte_ethtool_net_validate_addr(uint8_t port_id __rte_unused,
+	struct ether_addr *addr)
+{
+	return is_valid_assigned_ether_addr(addr);
+}
+
+int
+rte_ethtool_net_set_config(uint8_t port_id, void *config __rte_unused)
+{
+	struct rte_eth_link link;
+
+	memset(&link, 0, sizeof(link));
+	rte_eth_link_get(port_id, &link);
+	if (link.link_status == 1)
+		return -EINVAL;
+	return 0;
+}
+
+int
+rte_ethtool_net_change_mtu(uint8_t port_id, int mtu)
+{
+	return rte_eth_dev_set_mtu(port_id, (uint16_t)mtu);
+}
+
+int
+rte_ethtool_net_get_stats64(uint8_t port_id, struct rte_eth_stats *stats)
+{
+	return rte_eth_stats_get(port_id, stats);
+}
+
+int
+rte_ethtool_net_vlan_rx_add_vid(uint8_t port_id, uint16_t vid)
+{
+	return rte_eth_dev_vlan_filter(port_id, vid, 1);
+}
+
+int
+rte_ethtool_net_vlan_rx_kill_vid(uint8_t port_id, uint16_t vid)
+{
+	return rte_eth_dev_vlan_filter(port_id, vid, 0);
+}
+
+int
+rte_ethtool_net_set_rx_mode(uint8_t port_id __rte_unused)
+{
+	/*
+	 * The set_rx_mode op is part of pmd driver start operation, and
+	 * the ethdev api maintains software configuration parameters and under-
+	 * line hardware states consistent, so no operation is needed for
+	 * rte_ethtool_net_set_rx_mode().
+	 */
+	return 0;
+}
diff --git a/lib/librte_ethtool/rte_ethtool.h b/lib/librte_ethtool/rte_ethtool.h
new file mode 100644
index 0000000..cb68d94
--- /dev/null
+++ b/lib/librte_ethtool/rte_ethtool.h
@@ -0,0 +1,257 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_ETHTOOL_H_
+#define _RTE_ETHTOOL_H_
+
+/*
+ * This new interface is designed to provide a user-space shim layer for
+ * Ethtool and Netdevice op API.
+ *
+ * rte_ethtool_get_driver:          ethtool_ops::get_driverinfo
+ * rte_ethtool_get_link:            ethtool_ops::get_link
+ *
+ * rte_ethtool_net_open:            net_device_ops::ndo_open
+ * rte_ethtool_net_stop:            net_device_ops::ndo_stop
+ * rte_ethtool_net_set_mac_addr:    net_device_ops::ndo_set_mac_address
+ * rte_ethtool_net_validate_addr:   net_device_ops::ndo_validate_addr
+ * rte_ethtool_net_set_config:      net_device_ops::ndo_set_config
+ * rte_ethtool_net_change_mtu:      net_device_ops::ndo_net_change_mtu
+ * rte_ethtool_net_get_stats64:     net_device_ops::ndo_get_stats64
+ * rte_ethtool_net_vlan_rx_add_vid  net_device_ops::ndo_vlan_rx_add_vid
+ * rte_ethtool_net_vlan_rx_kill_vid net_device_ops::ndo_vlan_rx_kill_vid
+ * rte_ethtool_net_set_rx_mode      net_device_ops::ndo_set_rx_mode
+ *
+ */
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdint.h>
+#include <rte_ethdev.h>
+#include <linux/ethtool.h>
+
+/**
+ * Retrieve the Ethernet device driver information according to attributes described by
+ * ethtool data structure, ethtool_drvinfo
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param drvinfo
+ *   A pointer to get driver information
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_get_drvinfo(uint8_t port_id, struct ethtool_drvinfo *drvinfo);
+
+/**
+ * Retrieve the Ethernet device link status
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @return
+ *   - (1) if link up.
+ *   - (0) if link down.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_get_link(uint8_t port_id);
+
+/**
+ * Start the Ethernet device.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_open(uint8_t port_id);
+
+/**
+ * Stop the Ethernet device.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_stop(uint8_t port_id);
+
+/**
+ * Get the Ethernet device MAC address.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param addr
+ *	 MAC address of the Ethernet device.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_get_mac_addr(uint8_t port_id, struct ether_addr *addr);
+
+/**
+ * Setting the Ethernet device MAC address.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param addr
+ *	 The new MAC addr.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_set_mac_addr(uint8_t port_id, struct ether_addr *addr);
+
+/**
+ * Validate if the provided MAC address is valid unicast address
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param addr
+ *	 A pointer to a buffer (6-byte, 48bit) for the target MAC address
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_validate_addr(uint8_t port_id, struct ether_addr *addr);
+
+/**
+ * Setting the Ethernet device configuration.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param config
+ *	 A opintr to a configuration parameter.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_set_config(uint8_t port_id, void *config);
+
+/**
+ * Setting the Ethernet device maximum Tx unit.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param mtu
+ *	 New MTU
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu);
+
+/**
+ * Retrieve the Ethernet device traffic statistics
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param stats
+ *	 A pointer to struct rte_eth_stats for statistics parameters
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_get_stats64(uint8_t port_id, struct rte_eth_stats *stats);
+
+/**
+ * Update the Ethernet device VLAN filter with new vid
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param vid
+ *	 A new VLAN id
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_vlan_rx_add_vid(uint8_t port_id, uint16_t vid);
+
+/**
+ * Remove VLAN id from Ethernet device.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param vid
+ *	 A new VLAN id
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_vlan_rx_kill_vid(uint8_t port_id, uint16_t vid);
+
+/**
+ * Setting the Ethernet device rx mode.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_set_rx_mode(uint8_t port_id);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_ETHTOOL_H_ */
diff --git a/lib/librte_ethtool/rte_ethtool_version.map b/lib/librte_ethtool/rte_ethtool_version.map
new file mode 100644
index 0000000..82fc0d3
--- /dev/null
+++ b/lib/librte_ethtool/rte_ethtool_version.map
@@ -0,0 +1,18 @@
+DPDK_2.0 {
+	global:
+
+	rte_ethtool_net_open;
+	rte_ethtool_net_stop;
+	rte_ethtool_net_get_mac_addr;
+	rte_ethtool_net_get_mac_addr;
+	rte_ethtool_net_validate_addr;
+	rte_ethtool_net_set_config;
+	rte_ethtool_net_change_mtu;
+	rte_ethtool_net_get_stats64;
+	rte_ethtool_net_vlan_rx_add_vid;
+	rte_ethtool_net_vlan_rx_kill_vid;
+	rte_ethtool_get_drvinfo;
+	rte_ethtool_get_link;
+
+	local: *;
+};
diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index 1a2043a..86867a6 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -105,6 +105,7 @@ ifeq ($(CONFIG_RTE_BUILD_COMBINE_LIBS),n)
 _LDLIBS-$(CONFIG_RTE_LIBRTE_KVARGS)         += -lrte_kvargs
 _LDLIBS-$(CONFIG_RTE_LIBRTE_MBUF)           += -lrte_mbuf
 _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
+_LDLIBS-$(CONFIG_RTE_LIBRTE_ETHTOOL)        += -lrte_ethtool
 _LDLIBS-$(CONFIG_RTE_LIBRTE_ETHER)          += -lethdev
 _LDLIBS-$(CONFIG_RTE_LIBRTE_MALLOC)         += -lrte_malloc
 _LDLIBS-$(CONFIG_RTE_LIBRTE_MEMPOOL)        += -lrte_mempool
-- 
2.1.4

Re: [dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address

[Ananyev, Konstantin]

> -----Original Message-----
> From: Wang, Liang-min
> Sent: Friday, May 29, 2015 8:27 PM
> To: dev@dpdk.org
> Cc: Richardson, Bruce; Ananyev, Konstantin; dharton@cisco.com; agh@cisco.com; Wang, Liang-min
> Subject: [PATCH 1/2] ethdev: add api to set default mac address
> 
> add a new api: rte_eth_dev_default_mac_addr_set to
> support changing default mac address of a NIC
> 
> Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.c          | 16 ++++++++++++++++
>  lib/librte_ether/rte_ethdev.h          | 14 ++++++++++++++
>  lib/librte_ether/rte_ether_version.map |  1 +
>  3 files changed, 31 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 024fe8b..96ee00e 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2752,6 +2752,22 @@ rte_eth_dev_mac_addr_remove(uint8_t port_id, struct ether_addr *addr)
>  }
> 
>  int
> +rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr *addr)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	if (!rte_eth_dev_is_valid_port(port_id)) {
> +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> +		return -ENODEV;
> +	}
> +
> +	dev = &rte_eth_devices[port_id];
> +	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
> +
> +	return (*dev->dev_ops->mac_addr_set)(dev, addr);
> +}


As I can see mac_addr_set() is implemented now only for virtio.
Which means that for all Intel HW your new rte_eth_dev_default_mac_addr_set()
would not work right now?
Probably rte_eth_dev_default_mac_addr_set() should combine both approaches:
If mac_addr_set() is implemented by dev, then use it, otherwise try to use addr_remove()/addr_add()
(as your first version did)?
Konstantin   


> +
> +int
>  rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
>  				uint16_t rx_mode, uint8_t on)
>  {
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 16dbe00..5f07e0d 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2982,6 +2982,20 @@ int rte_eth_dev_mac_addr_add(uint8_t port, struct ether_addr *mac_addr,
>  int rte_eth_dev_mac_addr_remove(uint8_t port, struct ether_addr *mac_addr);
> 
>  /**
> + * Set the default MAC address.
> + *
> + * @param port
> + *   The port identifier of the Ethernet device.
> + * @param mac_addr
> + *   New default MAC address.
> + * @return
> + *   - (0) if successful, or *mac_addr* didn't exist.
> + *   - (-ENOTSUP) if hardware doesn't support.
> + *   - (-ENODEV) if *port* invalid.
> + */
> +int rte_eth_dev_default_mac_addr_set(uint8_t port, struct ether_addr *mac_addr);
> +
> +/**
>   * Update Redirection Table(RETA) of Receive Side Scaling of Ethernet device.
>   *
>   * @param port
> diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
> index a2d25a6..2dbbaa7 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -102,6 +102,7 @@ DPDK_2.0 {
>  	rte_eth_tx_queue_setup;
>  	rte_eth_xstats_get;
>  	rte_eth_xstats_reset;
> +	rte_eth_dev_default_mac_addr_set;
> 
>  	local: *;
>  };
> --
> 2.1.4

Re: [dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address

[Thomas Monjalon]

2015-06-02 10:52, Ananyev, Konstantin:
> From: Wang, Liang-min
> >  int
> > +rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr *addr)
> > +{
> > +	struct rte_eth_dev *dev;
> > +
> > +	if (!rte_eth_dev_is_valid_port(port_id)) {
> > +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> > +		return -ENODEV;
> > +	}
> > +
> > +	dev = &rte_eth_devices[port_id];
> > +	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
> > +
> > +	return (*dev->dev_ops->mac_addr_set)(dev, addr);
> > +}
> 
> As I can see mac_addr_set() is implemented now only for virtio.
> Which means that for all Intel HW your new rte_eth_dev_default_mac_addr_set()
> would not work right now?
> Probably rte_eth_dev_default_mac_addr_set() should combine both approaches:
> If mac_addr_set() is implemented by dev, then use it, otherwise try to use addr_remove()/addr_add()
> (as your first version did)?
> Konstantin   

Not sure it is a good idea to use remove/add to set the default unicast mac address.
It would be clearer to add comments to remove/add functions to specify that
they don't apply to the default adress but only to secondary ones. Then use
the same logic for both API and driver ops.
It is the responsibility of the driver implementation to use common functions
for default_set and remove/add functions.

Re: [dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address

[Wang, Liang-min]

>> -----Original Message-----
>> From: Wang, Liang-min
>> Sent: Friday, May 29, 2015 8:27 PM
>> To: dev@dpdk.org
>> Cc: Richardson, Bruce; Ananyev, Konstantin; dharton@cisco.com; 
> >agh@cisco.com; Wang, Liang-min
>> Subject: [PATCH 1/2] ethdev: add api to set default mac address
> >
>> add a new api: rte_eth_dev_default_mac_addr_set to support changing 
>> default mac address of a NIC
> >
> >Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
>> ---
> > lib/librte_ether/rte_ethdev.c          | 16 ++++++++++++++++
> > lib/librte_ether/rte_ethdev.h          | 14 ++++++++++++++
>>  lib/librte_ether/rte_ether_version.map |  1 +
>> 3 files changed, 31 insertions(+)
>>
>> diff --git a/lib/librte_ether/rte_ethdev.c 
>> b/lib/librte_ether/rte_ethdev.c index 024fe8b..96ee00e 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -2752,6 +2752,22 @@ rte_eth_dev_mac_addr_remove(uint8_t port_id, 
>> struct ether_addr *addr)  }
>> 
>>  int
>> +rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr 
>> +*addr) {
>> +	struct rte_eth_dev *dev;
>> +
>> +	if (!rte_eth_dev_is_valid_port(port_id)) {
>> +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
>> +		return -ENODEV;
>> +	}
>> +
>> +	dev = &rte_eth_devices[port_id];
>> +	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
>> +
>> +	return (*dev->dev_ops->mac_addr_set)(dev, addr); }


>As I can see mac_addr_set() is implemented now only for virtio.
>Which means that for all Intel HW your new rte_eth_dev_default_mac_addr_set()
>would not work right now?
>Probably rte_eth_dev_default_mac_addr_set() should combine both approaches:
>If mac_addr_set() is implemented by dev, then use it, otherwise try to use addr_remove()/addr_add() (as your first version did)?
>Konstantin   

The implementation of mac_addr_set() through mac_addr_remove() and mac_addr_add() is very creative, but it may not work for devices other than igb/ixgbe. I will release a patch latter to add hooks on igb and ixgbe(). Just want to keep this interface clean and not to be confined to igb/ixgbe characteristics.

> +
> +int
>  rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
>  				uint16_t rx_mode, uint8_t on)
>  {
> diff --git a/lib/librte_ether/rte_ethdev.h 
> b/lib/librte_ether/rte_ethdev.h index 16dbe00..5f07e0d 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2982,6 +2982,20 @@ int rte_eth_dev_mac_addr_add(uint8_t port, 
> struct ether_addr *mac_addr,  int rte_eth_dev_mac_addr_remove(uint8_t 
> port, struct ether_addr *mac_addr);
> 
>  /**
> + * Set the default MAC address.
> + *
> + * @param port
> + *   The port identifier of the Ethernet device.
> + * @param mac_addr
> + *   New default MAC address.
> + * @return
> + *   - (0) if successful, or *mac_addr* didn't exist.
> + *   - (-ENOTSUP) if hardware doesn't support.
> + *   - (-ENODEV) if *port* invalid.
> + */
> +int rte_eth_dev_default_mac_addr_set(uint8_t port, struct ether_addr 
> +*mac_addr);
> +
> +/**
>   * Update Redirection Table(RETA) of Receive Side Scaling of Ethernet device.
>   *
>   * @param port
> diff --git a/lib/librte_ether/rte_ether_version.map 
> b/lib/librte_ether/rte_ether_version.map
> index a2d25a6..2dbbaa7 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -102,6 +102,7 @@ DPDK_2.0 {
>  	rte_eth_tx_queue_setup;
>  	rte_eth_xstats_get;
>  	rte_eth_xstats_reset;
> +	rte_eth_dev_default_mac_addr_set;
> 
>  	local: *;
>  };
> --
> 2.1.4

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Thomas Monjalon]

2015-05-29 15:26, Liang-Min Larry Wang:
> adding a new library based upon ethdev APIs to provide API's that bear
> the same functionality as ethtool_ops (linux/ethtool.h) and net_device_ops
> (linux/netdevice.h).
> 
> Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
> ---
>  MAINTAINERS                                |   4 +
>  config/common_linuxapp                     |   5 +
>  lib/Makefile                               |   1 +
>  lib/librte_ethtool/Makefile                |  56 +++++++
>  lib/librte_ethtool/rte_ethtool.c           | 155 +++++++++++++++++
>  lib/librte_ethtool/rte_ethtool.h           | 257 +++++++++++++++++++++++++++++
>  lib/librte_ethtool/rte_ethtool_version.map |  18 ++
>  mk/rte.app.mk                              |   1 +

NACK for several reasons:
- It's unclear what benefits this ethdev wrapper is bringing
- There is no obvious interest (how is it supposed to be used?)
- There is no update in the doc/ directory

Other comments:
- the patches are not versionned
- the copyright starts in 2010

I'm curious to understand how renaming rte_eth_dev_set_mtu() to
rte_ethtool_net_change_mtu() will help anyone.

Re: [dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address

[Ananyev, Konstantin]

> -----Original Message-----
> From: Wang, Liang-min
> Sent: Tuesday, June 02, 2015 1:24 PM
> To: Ananyev, Konstantin; dev@dpdk.org
> Cc: Richardson, Bruce; dharton@cisco.com; agh@cisco.com
> Subject: RE: [PATCH 1/2] ethdev: add api to set default mac address
> 
> 
> >> -----Original Message-----
> >> From: Wang, Liang-min
> >> Sent: Friday, May 29, 2015 8:27 PM
> >> To: dev@dpdk.org
> >> Cc: Richardson, Bruce; Ananyev, Konstantin; dharton@cisco.com;
> > >agh@cisco.com; Wang, Liang-min
> >> Subject: [PATCH 1/2] ethdev: add api to set default mac address
> > >
> >> add a new api: rte_eth_dev_default_mac_addr_set to support changing
> >> default mac address of a NIC
> > >
> > >Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
> >> ---
> > > lib/librte_ether/rte_ethdev.c          | 16 ++++++++++++++++
> > > lib/librte_ether/rte_ethdev.h          | 14 ++++++++++++++
> >>  lib/librte_ether/rte_ether_version.map |  1 +
> >> 3 files changed, 31 insertions(+)
> >>
> >> diff --git a/lib/librte_ether/rte_ethdev.c
> >> b/lib/librte_ether/rte_ethdev.c index 024fe8b..96ee00e 100644
> >> --- a/lib/librte_ether/rte_ethdev.c
> >> +++ b/lib/librte_ether/rte_ethdev.c
> >> @@ -2752,6 +2752,22 @@ rte_eth_dev_mac_addr_remove(uint8_t port_id,
> >> struct ether_addr *addr)  }
> >>
> >>  int
> >> +rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr
> >> +*addr) {
> >> +	struct rte_eth_dev *dev;
> >> +
> >> +	if (!rte_eth_dev_is_valid_port(port_id)) {
> >> +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	dev = &rte_eth_devices[port_id];
> >> +	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
> >> +
> >> +	return (*dev->dev_ops->mac_addr_set)(dev, addr); }
> 
> 
> >As I can see mac_addr_set() is implemented now only for virtio.
> >Which means that for all Intel HW your new rte_eth_dev_default_mac_addr_set()
> >would not work right now?
> >Probably rte_eth_dev_default_mac_addr_set() should combine both approaches:
> >If mac_addr_set() is implemented by dev, then use it, otherwise try to use addr_remove()/addr_add() (as your first version did)?
> >Konstantin
> 
> The implementation of mac_addr_set() through mac_addr_remove() and mac_addr_add() is very creative, but it may not work for
> devices other than igb/ixgbe. I will release a patch latter to add hooks on igb and ixgbe(). Just want to keep this interface clean and not
> to be confined to igb/ixgbe characteristics.

Ok, if you think it would be more clean way - no objections from me.
Konstantin

> 
> > +
> > +int
> >  rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
> >  				uint16_t rx_mode, uint8_t on)
> >  {
> > diff --git a/lib/librte_ether/rte_ethdev.h
> > b/lib/librte_ether/rte_ethdev.h index 16dbe00..5f07e0d 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -2982,6 +2982,20 @@ int rte_eth_dev_mac_addr_add(uint8_t port,
> > struct ether_addr *mac_addr,  int rte_eth_dev_mac_addr_remove(uint8_t
> > port, struct ether_addr *mac_addr);
> >
> >  /**
> > + * Set the default MAC address.
> > + *
> > + * @param port
> > + *   The port identifier of the Ethernet device.
> > + * @param mac_addr
> > + *   New default MAC address.
> > + * @return
> > + *   - (0) if successful, or *mac_addr* didn't exist.
> > + *   - (-ENOTSUP) if hardware doesn't support.
> > + *   - (-ENODEV) if *port* invalid.
> > + */
> > +int rte_eth_dev_default_mac_addr_set(uint8_t port, struct ether_addr
> > +*mac_addr);
> > +
> > +/**
> >   * Update Redirection Table(RETA) of Receive Side Scaling of Ethernet device.
> >   *
> >   * @param port
> > diff --git a/lib/librte_ether/rte_ether_version.map
> > b/lib/librte_ether/rte_ether_version.map
> > index a2d25a6..2dbbaa7 100644
> > --- a/lib/librte_ether/rte_ether_version.map
> > +++ b/lib/librte_ether/rte_ether_version.map
> > @@ -102,6 +102,7 @@ DPDK_2.0 {
> >  	rte_eth_tx_queue_setup;
> >  	rte_eth_xstats_get;
> >  	rte_eth_xstats_reset;
> > +	rte_eth_dev_default_mac_addr_set;
> >
> >  	local: *;
> >  };
> > --
> > 2.1.4

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Wang, Liang-min]

>2015-05-29 15:26, Liang-Min Larry Wang:
>> adding a new library based upon ethdev APIs to provide API's that bear 
>> the same functionality as ethtool_ops (linux/ethtool.h) and 
>> net_device_ops (linux/netdevice.h).
>> 
>> Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
>> ---
>>  MAINTAINERS                                |   4 +
>>  config/common_linuxapp                     |   5 +
>>  lib/Makefile                               |   1 +
>>  lib/librte_ethtool/Makefile                |  56 +++++++
>>  lib/librte_ethtool/rte_ethtool.c           | 155 +++++++++++++++++
>>  lib/librte_ethtool/rte_ethtool.h           | 257 +++++++++++++++++++++++++++++
>>  lib/librte_ethtool/rte_ethtool_version.map |  18 ++
>>  mk/rte.app.mk                              |   1 +
>
>NACK for several reasons:
>- It's unclear what benefits this ethdev wrapper is bringing

Since ethtool is provided to assist users migrating from kernel ethtool/net_device_op based design to user-space DPDK device management. The ethtool API's are created to closely maintain its original interface, therefore this library depends on <linux/ethool.h>. To avoid pollute the existing ethdev interface, a new library is created. To minimize code replication and maintain closely 1:1 API definition with kernel space API, this interface is designed based upon available ethdev APIs and add additional dev_ops if it's necessary.

>- There is no obvious interest (how is it supposed to be used?)
There are already two acknowledge on this release. Earlier comment on this patch has that " ... The API's for ethtool like things are valuable ..."

>- There is no update in the doc/ directory
Need more guidance on that.

>Other comments:
>- the patches are not versioned

There is version file. Not sure what do you mean "the patches are not versioned"

>- the copyright starts in 2010

Will fix that.

>I'm curious to understand how renaming rte_eth_dev_set_mtu() to
>rte_ethtool_net_change_mtu() will help anyone.

As described, this interface is designed to provide API closely to kernel space ethtool ops and net_device_op.

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Thomas Monjalon]

Wang, hope it's clear that any new development is welcomed.
One step before integration is to clearly explain why your code
is needed. That's why a nack vote may help to discuss and decide.

Comments below

2015-06-02 13:15, Wang, Liang-min:
> >2015-05-29 15:26, Liang-Min Larry Wang:
> >> adding a new library based upon ethdev APIs to provide API's that bear 
> >> the same functionality as ethtool_ops (linux/ethtool.h) and 
> >> net_device_ops (linux/netdevice.h).
> >> 
> >> Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
> >> ---
> >>  MAINTAINERS                                |   4 +
> >>  config/common_linuxapp                     |   5 +
> >>  lib/Makefile                               |   1 +
> >>  lib/librte_ethtool/Makefile                |  56 +++++++
> >>  lib/librte_ethtool/rte_ethtool.c           | 155 +++++++++++++++++
> >>  lib/librte_ethtool/rte_ethtool.h           | 257 +++++++++++++++++++++++++++++
> >>  lib/librte_ethtool/rte_ethtool_version.map |  18 ++
> >>  mk/rte.app.mk                              |   1 +
> >
> >NACK for several reasons:
> >- It's unclear what benefits this ethdev wrapper is bringing
> 
> Since ethtool is provided to assist users migrating from kernel ethtool/net_device_op based design to user-space DPDK device management. The ethtool API's are created to closely maintain its original interface, therefore this library depends on <linux/ethool.h>. To avoid pollute the existing ethdev interface, a new library is created. To minimize code replication and maintain closely 1:1 API definition with kernel space API, this interface is designed based upon available ethdev APIs and add additional dev_ops if it's necessary.
> 
> >- There is no obvious interest (how is it supposed to be used?)
> There are already two acknowledge on this release. Earlier comment on this patch has that " ... The API's for ethtool like things are valuable ..."

Stephen had some doubts about the real need and 2 people from Cisco
(who never contributed before) give their ack without justification.
Saying it's "valuable" or "very useful" is not enough.
A new library needs to demonstrate in which scenario the added-value is.
Sorry but you have to prove that it deserves to be maintained inside
the dpdk project.

> >- There is no update in the doc/ directory
> Need more guidance on that.

You probably have to add a new chapter in the programmer's guide.

> >Other comments:
> >- the patches are not versioned
> 
> There is version file. Not sure what do you mean "the patches are not versioned"

I mean there is no v2/v3 in the Subject. Please read
	http://dpdk.org/dev#send

> >- the copyright starts in 2010
> 
> Will fix that.
> 
> >I'm curious to understand how renaming rte_eth_dev_set_mtu() to
> >rte_ethtool_net_change_mtu() will help anyone.
> 
> As described, this interface is designed to provide API closely to kernel space ethtool ops and net_device_op.

But the application still needs to adapt the code to call rte_* functions.
So changing to rte_ethtool_net_change_mtu is equivalent to change to
the existing rte_eth_dev_set_mtu. I don't see the benefit.

Re: [dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address

[Stephen Hemminger]

On Tue, 02 Jun 2015 14:23:22 +0200
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> 2015-06-02 10:52, Ananyev, Konstantin:
> > From: Wang, Liang-min  
> > >  int
> > > +rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr *addr)
> > > +{
> > > +	struct rte_eth_dev *dev;
> > > +
> > > +	if (!rte_eth_dev_is_valid_port(port_id)) {
> > > +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	dev = &rte_eth_devices[port_id];
> > > +	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
> > > +
> > > +	return (*dev->dev_ops->mac_addr_set)(dev, addr);
> > > +}  
> > 
> > As I can see mac_addr_set() is implemented now only for virtio.
> > Which means that for all Intel HW your new rte_eth_dev_default_mac_addr_set()
> > would not work right now?
> > Probably rte_eth_dev_default_mac_addr_set() should combine both approaches:
> > If mac_addr_set() is implemented by dev, then use it, otherwise try to use addr_remove()/addr_add()
> > (as your first version did)?
> > Konstantin     
> 
> Not sure it is a good idea to use remove/add to set the default unicast mac address.
> It would be clearer to add comments to remove/add functions to specify that
> they don't apply to the default adress but only to secondary ones. Then use
> the same logic for both API and driver ops.
> It is the responsibility of the driver implementation to use common functions
> for default_set and remove/add functions.

Only vmxnet3 and virtio need special treatment. virtio is already covered.
Here is patch for vmxnet3. We have used this for several releases.


From eef188102338c5687b9075d468eabbe87693b075 Mon Sep 17 00:00:00 2001
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Tue, 2 Jun 2015 07:49:53 -0700
Subject: [PATCH] vmxnet3: support setting primary MAC address

This allows setting primary MAC address on VMXNET3.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/vmxnet3/vmxnet3_ethdev.c | 45 +++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
index 1685ce4..6515f74 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
@@ -85,6 +85,9 @@ static void vmxnet3_dev_stats_get(struct rte_eth_dev *dev,
 				struct rte_eth_stats *stats);
 static void vmxnet3_dev_info_get(struct rte_eth_dev *dev,
 				struct rte_eth_dev_info *dev_info);
+static void vmxnet3_mac_addr_set(struct rte_eth_dev *dev,
+				 struct ether_addr *mac_addr);
+
 #if PROCESS_SYS_EVENTS == 1
 static void vmxnet3_process_events(struct vmxnet3_hw *);
 #endif
@@ -110,6 +113,7 @@ static const struct eth_dev_ops vmxnet3_eth_dev_ops = {
 	.allmulticast_disable = vmxnet3_dev_allmulticast_disable,
 	.link_update          = vmxnet3_dev_link_update,
 	.stats_get            = vmxnet3_dev_stats_get,
+	.mac_addr_set	      = vmxnet3_mac_addr_set,
 	.dev_infos_get        = vmxnet3_dev_info_get,
 	.rx_queue_setup       = vmxnet3_dev_rx_queue_setup,
 	.rx_queue_release     = vmxnet3_dev_rx_queue_release,
@@ -359,6 +363,23 @@ vmxnet3_dev_configure(struct rte_eth_dev *dev)
 	return 0;
 }
 
+static void
+vmxnet3_write_mac(struct vmxnet3_hw *hw, const uint8_t *addr)
+{
+	uint32_t val;
+
+	PMD_INIT_LOG(DEBUG,
+		     "Writing MAC Address : %02x:%02x:%02x:%02x:%02x:%02x",
+		     addr[0], addr[1], addr[2],
+		     addr[3], addr[4], addr[5]);
+
+	val = *(const uint32_t *)addr;
+	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_MACL, val);
+
+	val = (addr[5] << 8) | addr[4];
+	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_MACH, val);
+}
+
 static int
 vmxnet3_setup_driver_shared(struct rte_eth_dev *dev)
 {
@@ -366,8 +387,7 @@ vmxnet3_setup_driver_shared(struct rte_eth_dev *dev)
 	struct vmxnet3_hw *hw = dev->data->dev_private;
 	Vmxnet3_DriverShared *shared = hw->shared;
 	Vmxnet3_DSDevRead *devRead = &shared->devRead;
-	uint32_t *mac_ptr;
-	uint32_t val, i;
+	uint32_t i;
 	int ret;
 
 	shared->magic = VMXNET3_REV1_MAGIC;
@@ -459,18 +479,7 @@ vmxnet3_setup_driver_shared(struct rte_eth_dev *dev)
 			return ret;
 	}
 
-	PMD_INIT_LOG(DEBUG,
-		     "Writing MAC Address : %02x:%02x:%02x:%02x:%02x:%02x",
-		     hw->perm_addr[0], hw->perm_addr[1], hw->perm_addr[2],
-		     hw->perm_addr[3], hw->perm_addr[4], hw->perm_addr[5]);
-
-	/* Write MAC Address back to device */
-	mac_ptr = (uint32_t *)hw->perm_addr;
-	val = *mac_ptr;
-	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_MACL, val);
-
-	val = (hw->perm_addr[5] << 8) | hw->perm_addr[4];
-	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_MACH, val);
+	vmxnet3_write_mac(hw, hw->perm_addr);
 
 	return VMXNET3_SUCCESS;
 }
@@ -645,6 +654,14 @@ vmxnet3_dev_info_get(__attribute__((unused))struct rte_eth_dev *dev, struct rte_
 	dev_info->flow_type_rss_offloads = VMXNET3_RSS_OFFLOAD_ALL;
 }
 
+static void
+vmxnet3_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
+{
+	struct vmxnet3_hw *hw = dev->data->dev_private;
+
+	vmxnet3_write_mac(hw, mac_addr->addr_bytes);
+}
+
 /* return 0 means link status changed, -1 means not changed */
 static int
 vmxnet3_dev_link_update(struct rte_eth_dev *dev, __attribute__((unused)) int wait_to_complete)
-- 
2.1.4

Re: [dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address

[Wang, Liang-min]

>On Tue, 02 Jun 2015 14:23:22 +0200
>Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

>> 2015-06-02 10:52, Ananyev, Konstantin:
> >> From: Wang, Liang-min
> > >>  int
> > >>+rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct 
> > >> +ether_addr *addr) {
> > >> +	struct rte_eth_dev *dev;
> > >> +
> > >> +	if (!rte_eth_dev_is_valid_port(port_id)) {
> > >> +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> > >> +		return -ENODEV;
> > >> +	}
> > >> +
> > >> +	dev = &rte_eth_devices[port_id];
> > >> +	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
> > >> +
> > >> +	return (*dev->dev_ops->mac_addr_set)(dev, addr); }
> >> 
> >> As I can see mac_addr_set() is implemented now only for virtio.
> >> Which means that for all Intel HW your new 
> >> rte_eth_dev_default_mac_addr_set()
> >> would not work right now?
> >> Probably rte_eth_dev_default_mac_addr_set() should combine both approaches:
> >> If mac_addr_set() is implemented by dev, then use it, otherwise try 
> >> to use addr_remove()/addr_add() (as your first version did)?
> >> Konstantin     
>> 
>> Not sure it is a good idea to use remove/add to set the default unicast mac address.
>> It would be clearer to add comments to remove/add functions to specify 
>> that they don't apply to the default adress but only to secondary 
>> ones. Then use the same logic for both API and driver ops.
>> It is the responsibility of the driver implementation to use common 
>> functions for default_set and remove/add functions.

>Only vmxnet3 and virtio need special treatment. virtio is already covered.
>Here is patch for vmxnet3. We have used this for several releases.

To be consistent with implementation of ethdev API, rte_eth_macaddr_get, should mac_addr_set ops in both virtio and vmxnet3 update
 dev->data->mac_addrs[0]. So, the mac_addr_set and mac_addr_get are consistent.

>From eef188102338c5687b9075d468eabbe87693b075 Mon Sep 17 00:00:00 2001
>From: Stephen Hemminger <stephen@networkplumber.org>
>Date: Tue, 2 Jun 2015 07:49:53 -0700
>Subject: [PATCH] vmxnet3: support setting primary MAC address
>
>This allows setting primary MAC address on VMXNET3.
>
>Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>---
> drivers/net/vmxnet3/vmxnet3_ethdev.c | 45 +++++++++++++++++++++++++-----------
> 1 file changed, 31 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>index 1685ce4..6515f74 100644
>--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
>+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>@@ -85,6 +85,9 @@ static void vmxnet3_dev_stats_get(struct rte_eth_dev *dev,
> 				struct rte_eth_stats *stats);
> static void vmxnet3_dev_info_get(struct rte_eth_dev *dev,
> 				struct rte_eth_dev_info *dev_info);
>+static void vmxnet3_mac_addr_set(struct rte_eth_dev *dev,
>+				 struct ether_addr *mac_addr);
>+
> #if PROCESS_SYS_EVENTS == 1
> static void vmxnet3_process_events(struct vmxnet3_hw *);  #endif @@ -110,6 +113,7 @@ static const struct eth_dev_ops vmxnet3_eth_dev_ops = {
> 	.allmulticast_disable = vmxnet3_dev_allmulticast_disable,
> 	.link_update          = vmxnet3_dev_link_update,
> 	.stats_get            = vmxnet3_dev_stats_get,
>+	.mac_addr_set	      = vmxnet3_mac_addr_set,
> 	.dev_infos_get        = vmxnet3_dev_info_get,
> 	.rx_queue_setup       = vmxnet3_dev_rx_queue_setup,
> 	.rx_queue_release     = vmxnet3_dev_rx_queue_release,
>@@ -359,6 +363,23 @@ vmxnet3_dev_configure(struct rte_eth_dev *dev)
> 	return 0;
> }
> 
>+static void
>+vmxnet3_write_mac(struct vmxnet3_hw *hw, const uint8_t *addr) {
>+	uint32_t val;
>+
>+	PMD_INIT_LOG(DEBUG,
>+		     "Writing MAC Address : %02x:%02x:%02x:%02x:%02x:%02x",
>+		     addr[0], addr[1], addr[2],
>+		     addr[3], addr[4], addr[5]);
>+
>+	val = *(const uint32_t *)addr;
>+	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_MACL, val);
>+
>+	val = (addr[5] << 8) | addr[4];
>+	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_MACH, val); }
>+
> static int
> vmxnet3_setup_driver_shared(struct rte_eth_dev *dev)  { @@ -366,8 +387,7 @@ vmxnet3_setup_driver_shared(struct rte_eth_dev *dev)
> 	struct vmxnet3_hw *hw = dev->data->dev_private;
> 	Vmxnet3_DriverShared *shared = hw->shared;
> 	Vmxnet3_DSDevRead *devRead = &shared->devRead;
>-	uint32_t *mac_ptr;
>-	uint32_t val, i;
>+	uint32_t i;
> 	int ret;
> 
> 	shared->magic = VMXNET3_REV1_MAGIC;
>@@ -459,18 +479,7 @@ vmxnet3_setup_driver_shared(struct rte_eth_dev *dev)
> 			return ret;
> 	}
> 
>-	PMD_INIT_LOG(DEBUG,
>-		     "Writing MAC Address : %02x:%02x:%02x:%02x:%02x:%02x",
>-		     hw->perm_addr[0], hw->perm_addr[1], hw->perm_addr[2],
>-		     hw->perm_addr[3], hw->perm_addr[4], hw->perm_addr[5]);
>-
>-	/* Write MAC Address back to device */
>-	mac_ptr = (uint32_t *)hw->perm_addr;
>-	val = *mac_ptr;
>-	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_MACL, val);
>-
>-	val = (hw->perm_addr[5] << 8) | hw->perm_addr[4];
>-	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_MACH, val);
>+	vmxnet3_write_mac(hw, hw->perm_addr);
> 
> 	return VMXNET3_SUCCESS;
> }
>@@ -645,6 +654,14 @@ vmxnet3_dev_info_get(__attribute__((unused))struct rte_eth_dev *dev, struct rte_
> 	dev_info->flow_type_rss_offloads = VMXNET3_RSS_OFFLOAD_ALL;  }
> 
>+static void
>+vmxnet3_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr 
>+*mac_addr) {
>+	struct vmxnet3_hw *hw = dev->data->dev_private;
>+
>+	vmxnet3_write_mac(hw, mac_addr->addr_bytes); }
>+
>/* return 0 means link status changed, -1 means not changed */  static int  vmxnet3_dev_link_update(struct rte_eth_dev *dev, __attribute__((unused)) int wait_to_complete)
--
2.1.4

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Wang, Liang-min]

-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
Sent: Tuesday, June 02, 2015 10:33 AM
To: Wang, Liang-min
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

>Wang, hope it's clear that any new development is welcomed.
>One step before integration is to clearly explain why your code is needed. That's why a nack vote may help to discuss and decide.
>
>Comments below
>
>2015-06-02 13:15, Wang, Liang-min:
> >>2015-05-29 15:26, Liang-Min Larry Wang:
> >>> adding a new library based upon ethdev APIs to provide API's that 
> >>> bear the same functionality as ethtool_ops (linux/ethtool.h) and 
> >>> net_device_ops (linux/netdevice.h).
> >>> 
> >>> Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
> >>> ---
> >>>  MAINTAINERS                                |   4 +
> >>>  config/common_linuxapp                     |   5 +
> >>>  lib/Makefile                               |   1 +
> >>>  lib/librte_ethtool/Makefile                |  56 +++++++
> >>>  lib/librte_ethtool/rte_ethtool.c           | 155 +++++++++++++++++
> >>>  lib/librte_ethtool/rte_ethtool.h           | 257 +++++++++++++++++++++++++++++
> >>>  lib/librte_ethtool/rte_ethtool_version.map |  18 ++
> >>>  mk/rte.app.mk                              |   1 +
> >>
> >>NACK for several reasons:
> >>- It's unclear what benefits this ethdev wrapper is bringing
>> 
>> Since ethtool is provided to assist users migrating from kernel ethtool/net_device_op based design to user-space DPDK device management. The ethtool API's are created to closely maintain its original interface, therefore this library depends on <linux/ethool.h>. To avoid pollute the existing ethdev interface, a new library is created. To minimize code replication and maintain closely 1:1 API definition with kernel space API, this interface is designed based upon available ethdev APIs and add additional dev_ops if it's necessary.
>> 
> >>- There is no obvious interest (how is it supposed to be used?)
>> There are already two acknowledge on this release. Earlier comment on this patch has that " ... The API's for ethtool like things are valuable ..."

>Stephen had some doubts about the real need and 2 people from Cisco (who never contributed before) give their ack without justification.
>Saying it's "valuable" or "very useful" is not enough.
>A new library needs to demonstrate in which scenario the added-value is.
>Sorry but you have to prove that it deserves to be maintained inside the dpdk project.

Not sure if the question is the usefulness of user-space ethtool (there was request for user-space ethtool @ dpdk.org last year, right, and Steve's comment ...) or the question on putting ethtool on separate library. For the latter concern, as described, the design is to avoid polluting ethdev library by this inevitable including <linux/ethtool.h>

>> >- There is no update in the doc/ directory
>> Need more guidance on that.

>You probably have to add a new chapter in the programmer's guide.

Sure, I would help doc team adding new section into programmer's guide and other if it's necessary. The first thing is to have this API approved for release first.

> >>Other comments:
> >>- the patches are not versioned
>> 
>> There is version file. Not sure what do you mean "the patches are not versioned"

>I mean there is no v2/v3 in the Subject. Please read
>	http://dpdk.org/dev#send

My bad, I will address this issue on next patch

> >>- the copyright starts in 2010
>> 
>> Will fix that.
>> 
> >>I'm curious to understand how renaming rte_eth_dev_set_mtu() to
> >>rte_ethtool_net_change_mtu() will help anyone.
>> 
>> As described, this interface is designed to provide API closely to kernel space ethtool ops and net_device_op.

>But the application still needs to adapt the code to call rte_* functions.
>So changing to rte_ethtool_net_change_mtu is equivalent to change to the existing rte_eth_dev_set_mtu. I don't see the benefit.

The benefit is single interface for users to access. Instead of looking into two different interfaces (ethtool, ether). 

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Thomas Monjalon]

2015-06-02 15:47, Wang, Liang-min:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
> > >>I'm curious to understand how renaming rte_eth_dev_set_mtu() to
> > >>rte_ethtool_net_change_mtu() will help anyone.
> >> 
> >> As described, this interface is designed to provide API closely to kernel space ethtool ops and net_device_op.
> 
> >But the application still needs to adapt the code to call rte_* functions.
> >So changing to rte_ethtool_net_change_mtu is equivalent to change to the existing rte_eth_dev_set_mtu. I don't see the benefit.
> 
> The benefit is single interface for users to access. Instead of looking into two different interfaces (ethtool, ether). 

Sorry it doesn't help to understand.
Today, there is an ethdev API. Why adding an ethtool-like API would help?

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Wang, Liang-min]

>2015-06-02 15:47, Wang, Liang-min:
>> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
>> > >>I'm curious to understand how renaming rte_eth_dev_set_mtu() to
>> > >>rte_ethtool_net_change_mtu() will help anyone.
>> >> 
>> >> As described, this interface is designed to provide API closely to kernel space ethtool ops and net_device_op.
>> 
>> >But the application still needs to adapt the code to call rte_* functions.
>> >So changing to rte_ethtool_net_change_mtu is equivalent to change to the existing rte_eth_dev_set_mtu. I don't see the benefit.
>> 
>> The benefit is single interface for users to access. Instead of looking into two different interfaces (ethtool, ether). 
>
>Sorry it doesn't help to understand.
>Today, there is an ethdev API. Why adding an ethtool-like API would help?

Need to understand your specific concern. Do you oppose introducing new API to support ethtool ops and net_device_ops? 
Or your concern is on the implementation. If that's latter. 
Do you oppose adding a new library to implement ethtool ops and net_device_ops?
    If so, are you satisfied with my previous answer on avoiding polluting ethdev APIs? 
        If not, do you suggest integrating ethtool APIs into ethdev API?
    If not, is your concern on the implementation of common functionality between ethtool and ethdev APIs?
        If so, as explained, it is designed to provide a unified interface to assist users to migrate from kernel ethtool/net-device-op API to DPDK
BTW, as my reply to Steve's comment, more ops are on their way. This patch is to open up the interface.

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Thomas Monjalon]

I have the feeling we are not progressing in this discussion.
Please bring new explanations or I'll give up.
David Harton already acked it so maybe he could explain why it is useful.

Comments below

2015-06-02 17:06, Wang, Liang-min:
> >2015-06-02 15:47, Wang, Liang-min:
> >> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
> >> > >>I'm curious to understand how renaming rte_eth_dev_set_mtu() to
> >> > >>rte_ethtool_net_change_mtu() will help anyone.
> >> >> 
> >> >> As described, this interface is designed to provide API closely to kernel space ethtool ops and net_device_op.
> >> 
> >> >But the application still needs to adapt the code to call rte_* functions.
> >> >So changing to rte_ethtool_net_change_mtu is equivalent to change to the existing rte_eth_dev_set_mtu. I don't see the benefit.
> >> 
> >> The benefit is single interface for users to access. Instead of looking into two different interfaces (ethtool, ether). 
> >
> >Sorry it doesn't help to understand.
> >Today, there is an ethdev API. Why adding an ethtool-like API would help?
> 
> Need to understand your specific concern. Do you oppose introducing new API to support ethtool ops and net_device_ops? 

They are not ethtool/netdev ops but fake ones.
In linux:
	int dev_set_mtu(struct net_device *dev, int new_mtu)
In dpdk:
	int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu);
So yes, I'm opposed to add an API which is neither ethdev, neither ethtool.
But I may be missing an obvious justification.

> Or your concern is on the implementation. If that's latter. 
> Do you oppose adding a new library to implement ethtool ops and net_device_ops?

Already answered above

>     If so, are you satisfied with my previous answer on avoiding polluting ethdev APIs? 
>         If not, do you suggest integrating ethtool APIs into ethdev API?

No, it's better as a separate library.

>     If not, is your concern on the implementation of common functionality between ethtool and ethdev APIs?
>         If so, as explained, it is designed to provide a unified interface to assist users to migrate from kernel ethtool/net-device-op API to DPDK

Do you mean it would help migrating some code from kernel space to dpdk?
How it would help since the functions and data are different from the kernel ones?

> BTW, as my reply to Steve's comment, more ops are on their way. This patch is to open up the interface.

Currently they are only some one-liners plus ethtool_drvinfo formatting
so there is no real benefit.
Why not wait to have more ops implemented?

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Wang, Liang-min]

>I have the feeling we are not progressing in this discussion.
>Please bring new explanations or I'll give up.
>David Harton already acked it so maybe he could explain why it is useful.
>
>Comments below
>
>2015-06-02 17:06, Wang, Liang-min:
>> >2015-06-02 15:47, Wang, Liang-min:
>> >> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>> >> > >>I'm curious to understand how renaming rte_eth_dev_set_mtu() to
>> >> > >>rte_ethtool_net_change_mtu() will help anyone.
>> >> >> 
>> >> >> As described, this interface is designed to provide API closely to kernel space ethtool ops and net_device_op.
>> >> 
>> >> >But the application still needs to adapt the code to call rte_* functions.
>> >> >So changing to rte_ethtool_net_change_mtu is equivalent to change to the existing rte_eth_dev_set_mtu. I don't see the benefit.
>> >> 
>> >> The benefit is single interface for users to access. Instead of looking into two different interfaces (ethtool, ether). 
>> >
>> >Sorry it doesn't help to understand.
>> >Today, there is an ethdev API. Why adding an ethtool-like API would help?
>> 
>> Need to understand your specific concern. Do you oppose introducing new API to support ethtool ops and net_device_ops? 
>
>They are not ethtool/netdev ops but fake ones.
>In linux:
>	int dev_set_mtu(struct net_device *dev, int new_mtu) In dpdk:
>	int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu); So yes, I'm opposed to add an API which is neither ethdev, neither ethtool.
>But I may be missing an obvious justification.

So, the design purposely stays away from struct net_device to avoid carrying kernel states. We consciously choose port in place of net_device.
The kni already provides ethtool for kernel space code, this interface is designed for user-space application (fast-path comparing to kni).

>> Or your concern is on the implementation. If that's latter. 
>> Do you oppose adding a new library to implement ethtool ops and net_device_ops?
>
>Already answered above
>>
>>     If so, are you satisfied with my previous answer on avoiding polluting ethdev APIs? 
>>         If not, do you suggest integrating ethtool APIs into ethdev API?
>
>No, it's better as a separate library.
>
>>     If not, is your concern on the implementation of common functionality between ethtool and ethdev APIs?
>>         If so, as explained, it is designed to provide a unified 
>> interface to assist users to migrate from kernel ethtool/net-device-op 
>> API to DPDK
>
>Do you mean it would help migrating some code from kernel space to dpdk?
>How it would help since the functions and data are different from the kernel ones?

For application that was designed based upon kernel-space ethtool-op and net-device-op, the user-space APIs provide a path for quick integration.

>> BTW, as my reply to Steve's comment, more ops are on their way. This patch is to open up the interface.
>
>Currently they are only some one-liners plus ethtool_drvinfo formatting so there is no real benefit.
>Why not wait to have more ops implemented?
Due to amount of code change, I was advised to put into separate release and started with APIs that only requires minor changes on ethdev.
The rest of API requires changes on NIC pmd driver to support ops that are not currently supported.

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[David Harton (dharton)]

Hi Thomas,

I think Larry explains things pretty clearly below.

A new application facing API is bring provided that avoids reusing kernel specific types and structs or dipping into the kernel itself.  It also clearly separates the user space API from driver related functions/types.

We do want to limit dipping into the kernel for performance reasons.

I hope this helps,
Dave

> -----Original Message-----
> From: Wang, Liang-min [mailto:liang-min.wang@intel.com]
> Sent: Tuesday, June 02, 2015 4:56 PM
> To: Thomas Monjalon
> Cc: dev@dpdk.org; David Harton (dharton)
> Subject: RE: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide
> ethtool-alike APIs
> 
> >I have the feeling we are not progressing in this discussion.
> >Please bring new explanations or I'll give up.
> >David Harton already acked it so maybe he could explain why it is useful.
> >
> >Comments below
> >
> >2015-06-02 17:06, Wang, Liang-min:
> >> >2015-06-02 15:47, Wang, Liang-min:
> >> >> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> >> >> > >>I'm curious to understand how renaming rte_eth_dev_set_mtu()
> >> >> > >>to
> >> >> > >>rte_ethtool_net_change_mtu() will help anyone.
> >> >> >>
> >> >> >> As described, this interface is designed to provide API closely
> to kernel space ethtool ops and net_device_op.
> >> >>
> >> >> >But the application still needs to adapt the code to call rte_*
> functions.
> >> >> >So changing to rte_ethtool_net_change_mtu is equivalent to change
> to the existing rte_eth_dev_set_mtu. I don't see the benefit.
> >> >>
> >> >> The benefit is single interface for users to access. Instead of
> looking into two different interfaces (ethtool, ether).
> >> >
> >> >Sorry it doesn't help to understand.
> >> >Today, there is an ethdev API. Why adding an ethtool-like API would
> help?
> >>
> >> Need to understand your specific concern. Do you oppose introducing new
> API to support ethtool ops and net_device_ops?
> >
> >They are not ethtool/netdev ops but fake ones.
> >In linux:
> >	int dev_set_mtu(struct net_device *dev, int new_mtu) In dpdk:
> >	int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu); So yes,
> I'm opposed to add an API which is neither ethdev, neither ethtool.
> >But I may be missing an obvious justification.
> 
> So, the design purposely stays away from struct net_device to avoid
> carrying kernel states. We consciously choose port in place of net_device.
> The kni already provides ethtool for kernel space code, this interface is
> designed for user-space application (fast-path comparing to kni).
> 
> >> Or your concern is on the implementation. If that's latter.
> >> Do you oppose adding a new library to implement ethtool ops and
> net_device_ops?
> >
> >Already answered above
> >>
> >>     If so, are you satisfied with my previous answer on avoiding
> polluting ethdev APIs?
> >>         If not, do you suggest integrating ethtool APIs into ethdev
> API?
> >
> >No, it's better as a separate library.
> >
> >>     If not, is your concern on the implementation of common
> functionality between ethtool and ethdev APIs?
> >>         If so, as explained, it is designed to provide a unified
> >> interface to assist users to migrate from kernel
> >> ethtool/net-device-op API to DPDK
> >
> >Do you mean it would help migrating some code from kernel space to dpdk?
> >How it would help since the functions and data are different from the
> kernel ones?
> 
> For application that was designed based upon kernel-space ethtool-op and
> net-device-op, the user-space APIs provide a path for quick integration.
> 
> >> BTW, as my reply to Steve's comment, more ops are on their way. This
> patch is to open up the interface.
> >
> >Currently they are only some one-liners plus ethtool_drvinfo formatting
> so there is no real benefit.
> >Why not wait to have more ops implemented?
> Due to amount of code change, I was advised to put into separate release
> and started with APIs that only requires minor changes on ethdev.
> The rest of API requires changes on NIC pmd driver to support ops that are
> not currently supported.

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Andrew Harvey (agh)]

I believe that their is value in this interface for software stacks not
based on Linux being moved toward DPDK that need simple operations like
getting the mac address.  Some of these stacks have a dearth of resources
available and dedicating a core/thread to KNI to get/set a mac address
is considered excessive. There are also issues with 32/64 bit kernel
integration
using KNI.  If the ethtool interface is not the correct interface then
please help me
understand what should/could have been used. If ethtool is considered 'old
and clunky¹
Stephen's and your input would be valuable in designing another interface
with
similar properties.  The use-case is pretty simple and there is no plans
for moving
anything back into the kernel on the contrary its the complete opposite.

‹ Andy


On 6/2/15, 1:37 PM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:

>I have the feeling we are not progressing in this discussion.
>Please bring new explanations or I'll give up.
>David Harton already acked it so maybe he could explain why it is useful.
>
>Comments below
>
>2015-06-02 17:06, Wang, Liang-min:
>> >2015-06-02 15:47, Wang, Liang-min:
>> >> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>> >> > >>I'm curious to understand how renaming rte_eth_dev_set_mtu() to
>> >> > >>rte_ethtool_net_change_mtu() will help anyone.
>> >> >> 
>> >> >> As described, this interface is designed to provide API closely
>>to kernel space ethtool ops and net_device_op.
>> >> 
>> >> >But the application still needs to adapt the code to call rte_*
>>functions.
>> >> >So changing to rte_ethtool_net_change_mtu is equivalent to change
>>to the existing rte_eth_dev_set_mtu. I don't see the benefit.
>> >> 
>> >> The benefit is single interface for users to access. Instead of
>>looking into two different interfaces (ethtool, ether).
>> >
>> >Sorry it doesn't help to understand.
>> >Today, there is an ethdev API. Why adding an ethtool-like API would
>>help?
>> 
>> Need to understand your specific concern. Do you oppose introducing new
>>API to support ethtool ops and net_device_ops?
>
>They are not ethtool/netdev ops but fake ones.
>In linux:
>	int dev_set_mtu(struct net_device *dev, int new_mtu)
>In dpdk:
>	int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu);
>So yes, I'm opposed to add an API which is neither ethdev, neither
>ethtool.
>But I may be missing an obvious justification.
>
>> Or your concern is on the implementation. If that's latter.
>> Do you oppose adding a new library to implement ethtool ops and
>>net_device_ops?
>
>Already answered above
>
>>     If so, are you satisfied with my previous answer on avoiding
>>polluting ethdev APIs?
>>         If not, do you suggest integrating ethtool APIs into ethdev API?
>
>No, it's better as a separate library.
>
>>     If not, is your concern on the implementation of common
>>functionality between ethtool and ethdev APIs?
>>         If so, as explained, it is designed to provide a unified
>>interface to assist users to migrate from kernel ethtool/net-device-op
>>API to DPDK
>
>Do you mean it would help migrating some code from kernel space to dpdk?
>How it would help since the functions and data are different from the
>kernel ones?
>
>> BTW, as my reply to Steve's comment, more ops are on their way. This
>>patch is to open up the interface.
>
>Currently they are only some one-liners plus ethtool_drvinfo formatting
>so there is no real benefit.
>Why not wait to have more ops implemented?
>

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[O'Driscoll, Tim]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andrew Harvey (agh)
> Sent: Wednesday, June 3, 2015 3:10 AM
> To: Thomas Monjalon; Wang, Liang-min
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide
> ethtool-alike APIs
> 
> I believe that their is value in this interface for software stacks not
> based on Linux being moved toward DPDK that need simple operations like
> getting the mac address.  Some of these stacks have a dearth of
> resources
> available and dedicating a core/thread to KNI to get/set a mac address
> is considered excessive. There are also issues with 32/64 bit kernel
> integration
> using KNI.  If the ethtool interface is not the correct interface then
> please help me
> understand what should/could have been used. If ethtool is considered
> 'old
> and clunky¹
> Stephen's and your input would be valuable in designing another
> interface
> with
> similar properties.  The use-case is pretty simple and there is no plans
> for moving
> anything back into the kernel on the contrary its the complete opposite.
> 
> < Andy

Stephen, Thomas: I think it was the two of you that originally questioned the justification for including this change. Now that Andy and Dave Harton have clarified, does this resolve your concerns or do you still have questions? I just want to make sure that we reach a timely conclusion on this.


Tim

> 
> 
> On 6/2/15, 1:37 PM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:
> 
> >I have the feeling we are not progressing in this discussion.
> >Please bring new explanations or I'll give up.
> >David Harton already acked it so maybe he could explain why it is
> useful.
> >
> >Comments below
> >
> >2015-06-02 17:06, Wang, Liang-min:
> >> >2015-06-02 15:47, Wang, Liang-min:
> >> >> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> >> >> > >>I'm curious to understand how renaming rte_eth_dev_set_mtu()
> to
> >> >> > >>rte_ethtool_net_change_mtu() will help anyone.
> >> >> >>
> >> >> >> As described, this interface is designed to provide API closely
> >>to kernel space ethtool ops and net_device_op.
> >> >>
> >> >> >But the application still needs to adapt the code to call rte_*
> >>functions.
> >> >> >So changing to rte_ethtool_net_change_mtu is equivalent to change
> >>to the existing rte_eth_dev_set_mtu. I don't see the benefit.
> >> >>
> >> >> The benefit is single interface for users to access. Instead of
> >>looking into two different interfaces (ethtool, ether).
> >> >
> >> >Sorry it doesn't help to understand.
> >> >Today, there is an ethdev API. Why adding an ethtool-like API would
> >>help?
> >>
> >> Need to understand your specific concern. Do you oppose introducing
> new
> >>API to support ethtool ops and net_device_ops?
> >
> >They are not ethtool/netdev ops but fake ones.
> >In linux:
> >	int dev_set_mtu(struct net_device *dev, int new_mtu)
> >In dpdk:
> >	int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu);
> >So yes, I'm opposed to add an API which is neither ethdev, neither
> >ethtool.
> >But I may be missing an obvious justification.
> >
> >> Or your concern is on the implementation. If that's latter.
> >> Do you oppose adding a new library to implement ethtool ops and
> >>net_device_ops?
> >
> >Already answered above
> >
> >>     If so, are you satisfied with my previous answer on avoiding
> >>polluting ethdev APIs?
> >>         If not, do you suggest integrating ethtool APIs into ethdev
> API?
> >
> >No, it's better as a separate library.
> >
> >>     If not, is your concern on the implementation of common
> >>functionality between ethtool and ethdev APIs?
> >>         If so, as explained, it is designed to provide a unified
> >>interface to assist users to migrate from kernel ethtool/net-device-op
> >>API to DPDK
> >
> >Do you mean it would help migrating some code from kernel space to
> dpdk?
> >How it would help since the functions and data are different from the
> >kernel ones?
> >
> >> BTW, as my reply to Steve's comment, more ops are on their way. This
> >>patch is to open up the interface.
> >
> >Currently they are only some one-liners plus ethtool_drvinfo formatting
> >so there is no real benefit.
> >Why not wait to have more ops implemented?
> >

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Stephen Hemminger]

On Wed, 3 Jun 2015 02:09:39 +0000
"Andrew Harvey (agh)" <agh@cisco.com> wrote:

> I believe that their is value in this interface for software stacks not
> based on Linux being moved toward DPDK that need simple operations like
> getting the mac address.  Some of these stacks have a dearth of resources
> available and dedicating a core/thread to KNI to get/set a mac address
> is considered excessive. There are also issues with 32/64 bit kernel
> integration
> using KNI.  If the ethtool interface is not the correct interface then
> please help me
> understand what should/could have been used. If ethtool is considered 'old
> and clunky¹
> Stephen's and your input would be valuable in designing another interface
> with
> similar properties.  The use-case is pretty simple and there is no plans
> for moving
> anything back into the kernel on the contrary its the complete opposite.
> 
> ‹ Andy

We have DPDK API's to do this, and any added wrappers make it bigger.
I don't see why calling your ethtool API is better than calling
rte_eth* API.

If there is a missing functionality in the rte_ethXXX api's for an
application then add that. For example: rte_eth_mac_addr_get()

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Andrew Harvey (agh)]

On 6/4/15, 7:58 AM, "Stephen Hemminger" <stephen@networkplumber.org> wrote:

>On Wed, 3 Jun 2015 02:09:39 +0000
>"Andrew Harvey (agh)" <agh@cisco.com> wrote:
>
>> I believe that their is value in this interface for software stacks not
>> based on Linux being moved toward DPDK that need simple operations like
>> getting the mac address.  Some of these stacks have a dearth of
>>resources
>> available and dedicating a core/thread to KNI to get/set a mac address
>> is considered excessive. There are also issues with 32/64 bit kernel
>> integration
>> using KNI.  If the ethtool interface is not the correct interface then
>> please help me
>> understand what should/could have been used. If ethtool is considered
>>'old
>> and clunky¹
>> Stephen's and your input would be valuable in designing another
>>interface
>> with
>> similar properties.  The use-case is pretty simple and there is no plans
>> for moving
>> anything back into the kernel on the contrary its the complete opposite.
>> 
>> ‹ Andy
>
>We have DPDK API's to do this, and any added wrappers make it bigger.
>I don't see why calling your ethtool API is better than calling
>rte_eth* API.
>
>If there is a missing functionality in the rte_ethXXX api's for an
>application then add that. For example: rte_eth_mac_addr_get()

I am getting somewhat confused by your latest comments.  Your first email
(referenced below) looked really positive and I found your suggestions
useful. Your latest post appears to contradict this and now the interface
was there all the time.  The wrapper façade provided by the ethtool
library provide a clean separation of concerns and will allow people to
migrate from not only KNI but in our case from a legacy system.  If a
software stack has requirements to work with multiple IO abstractions
then the ethtool approach is attractive. I would speculate that many
other stacks moving towards dpdk will have similar issues.

Summarizing, for our use-cases the ethtool interface facilitated our
adoption to dpdk while allowing us to support our legacy IO abstractions.

— Andy

http://dpdk.org/ml/archives/dev/2015-May/018408.html (original email)

http://dpdk.org/ml/archives/dev/2014-June/003005.html (ethtool request)

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Thomas Monjalon]

2015-06-04 22:10, Andrew Harvey:
> On 6/4/15, 7:58 AM, "Stephen Hemminger" <stephen@networkplumber.org> wrote:
> >"Andrew Harvey (agh)" <agh@cisco.com> wrote:
> >> I believe that their is value in this interface for software stacks not
> >> based on Linux being moved toward DPDK that need simple operations like
> >> getting the mac address.  Some of these stacks have a dearth of
> >>resources
> >> available and dedicating a core/thread to KNI to get/set a mac address
> >> is considered excessive. There are also issues with 32/64 bit kernel
> >> integration
> >> using KNI.  If the ethtool interface is not the correct interface then
> >> please help me
> >> understand what should/could have been used. If ethtool is considered
> >>'old
> >> and clunky¹
> >> Stephen's and your input would be valuable in designing another
> >>interface
> >> with
> >> similar properties.  The use-case is pretty simple and there is no plans
> >> for moving
> >> anything back into the kernel on the contrary its the complete opposite.
> >> 
> >> ‹ Andy
> >
> >We have DPDK API's to do this, and any added wrappers make it bigger.
> >I don't see why calling your ethtool API is better than calling
> >rte_eth* API.
> >
> >If there is a missing functionality in the rte_ethXXX api's for an
> >application then add that. For example: rte_eth_mac_addr_get()
> 
> I am getting somewhat confused by your latest comments.  Your first email
> (referenced below) looked really positive and I found your suggestions
> useful. Your latest post appears to contradict this and now the interface
> was there all the time.  The wrapper façade provided by the ethtool
> library provide a clean separation of concerns and will allow people to
> migrate from not only KNI but in our case from a legacy system.  If a
> software stack has requirements to work with multiple IO abstractions
> then the ethtool approach is attractive. I would speculate that many
> other stacks moving towards dpdk will have similar issues.
> 
> Summarizing, for our use-cases the ethtool interface facilitated our
> adoption to dpdk while allowing us to support our legacy IO abstractions.

Stephen and me say the same thing about using the ethdev API.
We don't understand why using a fake ethtool lib would be easier.
Though you are saying it "facilitated [your] adoption to dpdk".
Please could you explain why using an ethtool-like API is easier than
using the existing ethdev API?
In any case, you have to develop a specific backend for DPDK
(rte_ethtool would be also DPDK-specific).

It seems you already started to use such an ethtool implementation.
Please note that our goal is not to prevent Cisco from upstreaming
(evidence with enic driver integration) but we want to guide you, and
others having the same needs, to the best solution for everybody.
That's why we need to understand what we (or you) are missing.
Maybe that it would be clearer with some code examples (which would
go in the lib documentation if any).

Thanks

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Wang, Liang-min]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, June 05, 2015 6:47 AM
> To: Andrew Harvey (agh)
> Cc: Stephen Hemminger; Wang, Liang-min; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide
> ethtool-alike APIs
> 
> 2015-06-04 22:10, Andrew Harvey:
> > On 6/4/15, 7:58 AM, "Stephen Hemminger"
> <stephen@networkplumber.org> wrote:
> > >"Andrew Harvey (agh)" <agh@cisco.com> wrote:
> > >> I believe that their is value in this interface for software stacks
> > >>not  based on Linux being moved toward DPDK that need simple
> > >>operations like  getting the mac address.  Some of these stacks have
> > >>a dearth of resources  available and dedicating a core/thread to KNI
> > >>to get/set a mac address  is considered excessive. There are also
> > >>issues with 32/64 bit kernel  integration  using KNI.  If the
> > >>ethtool interface is not the correct interface then  please help me
> > >>understand what should/could have been used. If ethtool is
> > >>considered 'old  and clunky¹  Stephen's and your input would be
> > >>valuable in designing another interface  with  similar properties.
> > >>The use-case is pretty simple and there is no plans  for moving
> > >>anything back into the kernel on the contrary its the complete opposite.
> > >>
> > >> ‹ Andy
> > >
> > >We have DPDK API's to do this, and any added wrappers make it bigger.
> > >I don't see why calling your ethtool API is better than calling
> > >rte_eth* API.
> > >
> > >If there is a missing functionality in the rte_ethXXX api's for an
> > >application then add that. For example: rte_eth_mac_addr_get()
> >
> > I am getting somewhat confused by your latest comments.  Your first
> > email (referenced below) looked really positive and I found your
> > suggestions useful. Your latest post appears to contradict this and
> > now the interface was there all the time.  The wrapper façade provided
> > by the ethtool library provide a clean separation of concerns and will
> > allow people to migrate from not only KNI but in our case from a
> > legacy system.  If a software stack has requirements to work with
> > multiple IO abstractions then the ethtool approach is attractive. I
> > would speculate that many other stacks moving towards dpdk will have
> similar issues.
> >
> > Summarizing, for our use-cases the ethtool interface facilitated our
> > adoption to dpdk while allowing us to support our legacy IO abstractions.
> 
> Stephen and me say the same thing about using the ethdev API.
> We don't understand why using a fake ethtool lib would be easier.
> Though you are saying it "facilitated [your] adoption to dpdk".
> Please could you explain why using an ethtool-like API is easier than using
> the existing ethdev API?
> In any case, you have to develop a specific backend for DPDK (rte_ethtool
> would be also DPDK-specific).

As described earlier in this patch comment reply, there are other ethtool ops that have been implemented.
Those ops includes set/get eeprom, set/get pauseparam, set/get ringparam which are not available in the exiting ethdev library.
For this release, we focus on releasing some basic functions (btw, mac_addr_set is not available but is covered by this patch).
The key reason that this set of library is not released as part of ethdev is the ethtool API dependency on kernel include file.
To faithfully carry the ethtool ops and net dev ops API parameters, the ethtool APIs are designed to follow the original definition except avoiding carry kernel states.
With that, to support ethtool APIs faithfully, we need to include <linux/ethtool.h>. 
As suggested by many DPDK veterans including Thomas (indicated over your reply), you would prefer these APIs in a separate library.

> 
> It seems you already started to use such an ethtool implementation.
> Please note that our goal is not to prevent Cisco from upstreaming (evidence
> with enic driver integration) but we want to guide you, and others having the
> same needs, to the best solution for everybody.
> That's why we need to understand what we (or you) are missing.
> Maybe that it would be clearer with some code examples (which would go in
> the lib documentation if any).
> 
> Thanks

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Bruce Richardson]

On Fri, Jun 05, 2015 at 11:25:09AM +0000, Wang, Liang-min wrote:
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Friday, June 05, 2015 6:47 AM
> > To: Andrew Harvey (agh)
> > Cc: Stephen Hemminger; Wang, Liang-min; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide
> > ethtool-alike APIs
> > 
> > 2015-06-04 22:10, Andrew Harvey:
> > > On 6/4/15, 7:58 AM, "Stephen Hemminger"
> > <stephen@networkplumber.org> wrote:
> > > >"Andrew Harvey (agh)" <agh@cisco.com> wrote:
> > > >> I believe that their is value in this interface for software stacks
> > > >>not  based on Linux being moved toward DPDK that need simple
> > > >>operations like  getting the mac address.  Some of these stacks have
> > > >>a dearth of resources  available and dedicating a core/thread to KNI
> > > >>to get/set a mac address  is considered excessive. There are also
> > > >>issues with 32/64 bit kernel  integration  using KNI.  If the
> > > >>ethtool interface is not the correct interface then  please help me
> > > >>understand what should/could have been used. If ethtool is
> > > >>considered 'old  and clunky¹  Stephen's and your input would be
> > > >>valuable in designing another interface  with  similar properties.
> > > >>The use-case is pretty simple and there is no plans  for moving
> > > >>anything back into the kernel on the contrary its the complete opposite.
> > > >>
> > > >> ‹ Andy
> > > >
> > > >We have DPDK API's to do this, and any added wrappers make it bigger.
> > > >I don't see why calling your ethtool API is better than calling
> > > >rte_eth* API.
> > > >
> > > >If there is a missing functionality in the rte_ethXXX api's for an
> > > >application then add that. For example: rte_eth_mac_addr_get()
> > >
> > > I am getting somewhat confused by your latest comments.  Your first
> > > email (referenced below) looked really positive and I found your
> > > suggestions useful. Your latest post appears to contradict this and
> > > now the interface was there all the time.  The wrapper façade provided
> > > by the ethtool library provide a clean separation of concerns and will
> > > allow people to migrate from not only KNI but in our case from a
> > > legacy system.  If a software stack has requirements to work with
> > > multiple IO abstractions then the ethtool approach is attractive. I
> > > would speculate that many other stacks moving towards dpdk will have
> > similar issues.
> > >
> > > Summarizing, for our use-cases the ethtool interface facilitated our
> > > adoption to dpdk while allowing us to support our legacy IO abstractions.
> > 
> > Stephen and me say the same thing about using the ethdev API.
> > We don't understand why using a fake ethtool lib would be easier.
> > Though you are saying it "facilitated [your] adoption to dpdk".
> > Please could you explain why using an ethtool-like API is easier than using
> > the existing ethdev API?
> > In any case, you have to develop a specific backend for DPDK (rte_ethtool
> > would be also DPDK-specific).
> 
> As described earlier in this patch comment reply, there are other ethtool ops that have been implemented.
> Those ops includes set/get eeprom, set/get pauseparam, set/get ringparam which are not available in the exiting ethdev library.
> For this release, we focus on releasing some basic functions (btw, mac_addr_set is not available but is covered by this patch).
> The key reason that this set of library is not released as part of ethdev is the ethtool API dependency on kernel include file.
> To faithfully carry the ethtool ops and net dev ops API parameters, the ethtool APIs are designed to follow the original definition except avoiding carry kernel states.
> With that, to support ethtool APIs faithfully, we need to include <linux/ethtool.h>. 
> As suggested by many DPDK veterans including Thomas (indicated over your reply), you would prefer these APIs in a separate library.
> 
> > 
> > It seems you already started to use such an ethtool implementation.
> > Please note that our goal is not to prevent Cisco from upstreaming (evidence
> > with enic driver integration) but we want to guide you, and others having the
> > same needs, to the best solution for everybody.
> > That's why we need to understand what we (or you) are missing.
> > Maybe that it would be clearer with some code examples (which would go in
> > the lib documentation if any).
> > 
> > Thanks

How about doing this work as a sample application initially, to demonstrate how
an application written using ethtool APIs could be shimmed to use DPDK underneath.
The ethtool to dpdk mapping could be contained in a single header file (or header
and c file) inside the sample app. This would allow easy re-use of the shim
layer, while at the same time not making it part of the core DPDK libraries.

Regards,
/Bruce

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Thomas Monjalon]

2015-06-05 11:25, Wang, Liang-min:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Stephen and me say the same thing about using the ethdev API.
> > We don't understand why using a fake ethtool lib would be easier.
> > Though you are saying it "facilitated [your] adoption to dpdk".
> > Please could you explain why using an ethtool-like API is easier than using
> > the existing ethdev API?
> > In any case, you have to develop a specific backend for DPDK (rte_ethtool
> > would be also DPDK-specific).
> 
> As described earlier in this patch comment reply, there are other ethtool ops that have been implemented.
> Those ops includes set/get eeprom, set/get pauseparam, set/get ringparam which are not available in the exiting ethdev library.

1/ We cannot really consider code which is not public
2/ You may extend ethdev if some functions are missing

> For this release, we focus on releasing some basic functions (btw, mac_addr_set is not available but is covered by this patch).

Yes, you are extending ethdev by adding rte_eth_dev_default_mac_addr_set.

> The key reason that this set of library is not released as part of ethdev is the ethtool API dependency on kernel include file.

It is a good reason to separate the library.
But it doesn't justify its need.

> To faithfully carry the ethtool ops and net dev ops API parameters, the ethtool APIs are designed to follow the original definition except avoiding carry kernel states.
> With that, to support ethtool APIs faithfully, we need to include <linux/ethtool.h>. 
> As suggested by many DPDK veterans including Thomas (indicated over your reply), you would prefer these APIs in a separate library.

I think I'm starting to understand that you really need ethtool conversion
(implemented in rte_ethtool_get_drvinfo) but not the other functions which
are simple wrappers. Right?

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Wang, Liang-min]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, June 05, 2015 9:41 AM
> To: Wang, Liang-min
> Cc: Andrew Harvey (agh); Stephen Hemminger; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide
> ethtool-alike APIs
> 
> 2015-06-05 11:25, Wang, Liang-min:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > Stephen and me say the same thing about using the ethdev API.
> > > We don't understand why using a fake ethtool lib would be easier.
> > > Though you are saying it "facilitated [your] adoption to dpdk".
> > > Please could you explain why using an ethtool-like API is easier
> > > than using the existing ethdev API?
> > > In any case, you have to develop a specific backend for DPDK
> > > (rte_ethtool would be also DPDK-specific).
> >
> > As described earlier in this patch comment reply, there are other ethtool
> ops that have been implemented.
> > Those ops includes set/get eeprom, set/get pauseparam, set/get
> ringparam which are not available in the exiting ethdev library.
> 
> 1/ We cannot really consider code which is not public 2/ You may extend
> ethdev if some functions are missing
> 
> > For this release, we focus on releasing some basic functions (btw,
> mac_addr_set is not available but is covered by this patch).
> 
> Yes, you are extending ethdev by adding
> rte_eth_dev_default_mac_addr_set.
> 
> > The key reason that this set of library is not released as part of ethdev is
> the ethtool API dependency on kernel include file.
> 
> It is a good reason to separate the library.
> But it doesn't justify its need.
> 
> > To faithfully carry the ethtool ops and net dev ops API parameters, the
> ethtool APIs are designed to follow the original definition except avoiding
> carry kernel states.
> > With that, to support ethtool APIs faithfully, we need to include
> <linux/ethtool.h>.
> > As suggested by many DPDK veterans including Thomas (indicated over
> your reply), you would prefer these APIs in a separate library.
> 
> I think I'm starting to understand that you really need ethtool conversion
> (implemented in rte_ethtool_get_drvinfo) but not the other functions which
> are simple wrappers. Right?

The rte_ethtool_get_drvinfo and many others ethtool ops have the same conversion requirement.
As for ethtool and net dev ops that don't require conversion. For the sake of clean API interface, they are implemented in the same ethtool library.

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Andrew Harvey (agh)]

On 6/5/15, 3:46 AM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:

>2015-06-04 22:10, Andrew Harvey:
>> On 6/4/15, 7:58 AM, "Stephen Hemminger" <stephen@networkplumber.org>
>>wrote:
>> >"Andrew Harvey (agh)" <agh@cisco.com> wrote:
>> >> I believe that their is value in this interface for software stacks
>>not
>> >> based on Linux being moved toward DPDK that need simple operations
>>like
>> >> getting the mac address.  Some of these stacks have a dearth of
>> >>resources
>> >> available and dedicating a core/thread to KNI to get/set a mac
>>address
>> >> is considered excessive. There are also issues with 32/64 bit kernel
>> >> integration
>> >> using KNI.  If the ethtool interface is not the correct interface
>>then
>> >> please help me
>> >> understand what should/could have been used. If ethtool is considered
>> >>'old
>> >> and clunky¹
>> >> Stephen's and your input would be valuable in designing another
>> >>interface
>> >> with
>> >> similar properties.  The use-case is pretty simple and there is no
>>plans
>> >> for moving
>> >> anything back into the kernel on the contrary its the complete
>>opposite.
>> >> 
>> >> ‹ Andy
>> >
>> >We have DPDK API's to do this, and any added wrappers make it bigger.
>> >I don't see why calling your ethtool API is better than calling
>> >rte_eth* API.
>> >
>> >If there is a missing functionality in the rte_ethXXX api's for an
>> >application then add that. For example: rte_eth_mac_addr_get()
>> 
>> I am getting somewhat confused by your latest comments.  Your first
>>email
>> (referenced below) looked really positive and I found your suggestions
>> useful. Your latest post appears to contradict this and now the
>>interface
>> was there all the time.  The wrapper façade provided by the ethtool
>> library provide a clean separation of concerns and will allow people to
>> migrate from not only KNI but in our case from a legacy system.  If a
>> software stack has requirements to work with multiple IO abstractions
>> then the ethtool approach is attractive. I would speculate that many
>> other stacks moving towards dpdk will have similar issues.
>> 
>> Summarizing, for our use-cases the ethtool interface facilitated our
>> adoption to dpdk while allowing us to support our legacy IO
>>abstractions.
>
>Stephen and me say the same thing about using the ethdev API.

And your would have a point would be valid if dpdk were available to every
interface we support (it is not) and on every processor architecture that
we support (it is not) and every OS we support (it is not).  So to
minimize entropy in the code why not leave the client code the same
ioctl(fd, …) and hide the implementation
detail in a wrapper library.  We have a large legacy code base to move
forward and sprinkling special interest code like rte_xxx throughout every
client we have is not appropriate at this time.

 
>We don't understand why using a fake ethtool lib would be easier.
>Though you are saying it "facilitated [your] adoption to dpdk".
>Please could you explain why using an ethtool-like API is easier than
>using the existing ethdev API?
>In any case, you have to develop a specific backend for DPDK
>(rte_ethtool would be also DPDK-specific).
>
>It seems you already started to use such an ethtool implementation.
>Please note that our goal is not to prevent Cisco from upstreaming
>(evidence with enic driver integration) but we want to guide you, and
>others having the same needs, to the best solution for everybody.
>That's why we need to understand what we (or you) are missing.
>Maybe that it would be clearer with some code examples (which would
>go in the lib documentation if any).
>
>Thanks

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Andrew Harvey (agh)]

On 6/5/15, 5:47 AM, "Bruce Richardson" <bruce.richardson@intel.com> wrote:

>On Fri, Jun 05, 2015 at 11:25:09AM +0000, Wang, Liang-min wrote:
>> 
>> 
>> > -----Original Message-----
>> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>> > Sent: Friday, June 05, 2015 6:47 AM
>> > To: Andrew Harvey (agh)
>> > Cc: Stephen Hemminger; Wang, Liang-min; dev@dpdk.org
>> > Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to
>>provide
>> > ethtool-alike APIs
>> > 
>> > 2015-06-04 22:10, Andrew Harvey:
>> > > On 6/4/15, 7:58 AM, "Stephen Hemminger"
>> > <stephen@networkplumber.org> wrote:
>> > > >"Andrew Harvey (agh)" <agh@cisco.com> wrote:
>> > > >> I believe that their is value in this interface for software
>>stacks
>> > > >>not  based on Linux being moved toward DPDK that need simple
>> > > >>operations like  getting the mac address.  Some of these stacks
>>have
>> > > >>a dearth of resources  available and dedicating a core/thread to
>>KNI
>> > > >>to get/set a mac address  is considered excessive. There are also
>> > > >>issues with 32/64 bit kernel  integration  using KNI.  If the
>> > > >>ethtool interface is not the correct interface then  please help
>>me
>> > > >>understand what should/could have been used. If ethtool is
>> > > >>considered 'old  and clunky¹  Stephen's and your input would be
>> > > >>valuable in designing another interface  with  similar properties.
>> > > >>The use-case is pretty simple and there is no plans  for moving
>> > > >>anything back into the kernel on the contrary its the complete
>>opposite.
>> > > >>
>> > > >> ‹ Andy
>> > > >
>> > > >We have DPDK API's to do this, and any added wrappers make it
>>bigger.
>> > > >I don't see why calling your ethtool API is better than calling
>> > > >rte_eth* API.
>> > > >
>> > > >If there is a missing functionality in the rte_ethXXX api's for an
>> > > >application then add that. For example: rte_eth_mac_addr_get()
>> > >
>> > > I am getting somewhat confused by your latest comments.  Your first
>> > > email (referenced below) looked really positive and I found your
>> > > suggestions useful. Your latest post appears to contradict this and
>> > > now the interface was there all the time.  The wrapper façade
>>provided
>> > > by the ethtool library provide a clean separation of concerns and
>>will
>> > > allow people to migrate from not only KNI but in our case from a
>> > > legacy system.  If a software stack has requirements to work with
>> > > multiple IO abstractions then the ethtool approach is attractive. I
>> > > would speculate that many other stacks moving towards dpdk will have
>> > similar issues.
>> > >
>> > > Summarizing, for our use-cases the ethtool interface facilitated our
>> > > adoption to dpdk while allowing us to support our legacy IO
>>abstractions.
>> > 
>> > Stephen and me say the same thing about using the ethdev API.
>> > We don't understand why using a fake ethtool lib would be easier.
>> > Though you are saying it "facilitated [your] adoption to dpdk".
>> > Please could you explain why using an ethtool-like API is easier than
>>using
>> > the existing ethdev API?
>> > In any case, you have to develop a specific backend for DPDK
>>(rte_ethtool
>> > would be also DPDK-specific).
>> 
>> As described earlier in this patch comment reply, there are other
>>ethtool ops that have been implemented.
>> Those ops includes set/get eeprom, set/get pauseparam, set/get
>>ringparam which are not available in the exiting ethdev library.
>> For this release, we focus on releasing some basic functions (btw,
>>mac_addr_set is not available but is covered by this patch).
>> The key reason that this set of library is not released as part of
>>ethdev is the ethtool API dependency on kernel include file.
>> To faithfully carry the ethtool ops and net dev ops API parameters, the
>>ethtool APIs are designed to follow the original definition except
>>avoiding carry kernel states.
>> With that, to support ethtool APIs faithfully, we need to include
>><linux/ethtool.h>.
>> As suggested by many DPDK veterans including Thomas (indicated over
>>your reply), you would prefer these APIs in a separate library.
>> 
>> > 
>> > It seems you already started to use such an ethtool implementation.
>> > Please note that our goal is not to prevent Cisco from upstreaming
>>(evidence
>> > with enic driver integration) but we want to guide you, and others
>>having the
>> > same needs, to the best solution for everybody.
>> > That's why we need to understand what we (or you) are missing.
>> > Maybe that it would be clearer with some code examples (which would
>>go in
>> > the lib documentation if any).
>> > 
>> > Thanks
>
>How about doing this work as a sample application initially, to
>demonstrate how
>an application written using ethtool APIs could be shimmed to use DPDK
>underneath.
>The ethtool to dpdk mapping could be contained in a single header file
>(or header
>and c file) inside the sample app. This would allow easy re-use of the
>shim
>layer, while at the same time not making it part of the core DPDK
>libraries.
>
>Regards,
>/Bruce

This would appear to be the most pragmatic way forward.  It would allow
others to see more of the code and judge its value for themselves. I have
no issues with this approach if others agree.

— Andy

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Thomas Monjalon]

2015-06-05 16:07, Andrew Harvey:
> On 6/5/15, 3:46 AM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:
> >Stephen and me say the same thing about using the ethdev API.
> 
> And your would have a point would be valid if dpdk were available to every
> interface we support (it is not) and on every processor architecture that
> we support (it is not) and every OS we support (it is not).  So to
> minimize entropy in the code why not leave the client code the same
> ioctl(fd, …) and hide the implementation
> detail in a wrapper library.

Please, explain the relation between an ioctl and the DPDK.

Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

[Thomas Monjalon]

2015-06-05 17:24, Andrew Harvey:
> On 6/5/15, 5:47 AM, "Bruce Richardson" <bruce.richardson@intel.com> wrote:
> >> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> >> > That's why we need to understand what we (or you) are missing.
> >> > Maybe that it would be clearer with some code examples (which would
> >> > go in the lib documentation if any).
> >> > 
> >> > Thanks
> >
> >How about doing this work as a sample application initially, to
> >demonstrate how
> >an application written using ethtool APIs could be shimmed to use DPDK
> >underneath.
> >The ethtool to dpdk mapping could be contained in a single header file
> >(or header
> >and c file) inside the sample app. This would allow easy re-use of the
> >shim
> >layer, while at the same time not making it part of the core DPDK
> >libraries.
> >
> >Regards,
> >/Bruce
> 
> This would appear to be the most pragmatic way forward.  It would allow
> others to see more of the code and judge its value for themselves. I have
> no issues with this approach if others agree.

Since the beginning of this thread, a doc is requested (many times) to
show the benefit of integrating such a layer.
If you prefer coding a full example, it would probably also be fine.

Re: [dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address

[Andrew Harvey (agh)]

On 5/29/15, 5:37 PM, "Liang-Min Larry Wang" <liang-min.wang@intel.com>
wrote:
>add a new api: rte_eth_dev_default_mac_addr_set to
>support changing default mac address of a NIC
>
>Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
>---
> lib/librte_ether/rte_ethdev.c          | 18 ++++++++++++++++++
> lib/librte_ether/rte_ethdev.h          | 14 ++++++++++++++
> lib/librte_ether/rte_ether_version.map |  1 +
> 3 files changed, 33 insertions(+)
>
>diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>index 024fe8b..85ce72e 100644
>--- a/lib/librte_ether/rte_ethdev.c
>+++ b/lib/librte_ether/rte_ethdev.c
>@@ -2752,6 +2752,24 @@ rte_eth_dev_mac_addr_remove(uint8_t port_id,
>struct ether_addr *addr)
> }
> 
> int
>+rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr
>*addr)
>+{
>+	struct rte_eth_dev *dev;
>+
>+	if (!rte_eth_dev_is_valid_port(port_id)) {
>+		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
>+		return -ENODEV;
>+	}
>+
>+	dev = &rte_eth_devices[port_id];
>+	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
>+
>+	(*dev->dev_ops->mac_addr_set)(dev, addr);
>+
>+	return 0;
>+}
>+
>+int
> rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
> 				uint16_t rx_mode, uint8_t on)
> {
>diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
>index 16dbe00..5f07e0d 100644
>--- a/lib/librte_ether/rte_ethdev.h
>+++ b/lib/librte_ether/rte_ethdev.h
>@@ -2982,6 +2982,20 @@ int rte_eth_dev_mac_addr_add(uint8_t port, struct
>ether_addr *mac_addr,
> int rte_eth_dev_mac_addr_remove(uint8_t port, struct ether_addr
>*mac_addr);
> 
> /**
>+ * Set the default MAC address.
>+ *
>+ * @param port
>+ *   The port identifier of the Ethernet device.
>+ * @param mac_addr
>+ *   New default MAC address.
>+ * @return
>+ *   - (0) if successful, or *mac_addr* didn't exist.
>+ *   - (-ENOTSUP) if hardware doesn't support.
>+ *   - (-ENODEV) if *port* invalid.
>+ */
>+int rte_eth_dev_default_mac_addr_set(uint8_t port, struct ether_addr
>*mac_addr);
>+
>+/**
>  * Update Redirection Table(RETA) of Receive Side Scaling of Ethernet
>device.
>  *
>  * @param port
>diff --git a/lib/librte_ether/rte_ether_version.map
>b/lib/librte_ether/rte_ether_version.map
>index a2d25a6..2dbbaa7 100644
>--- a/lib/librte_ether/rte_ether_version.map
>+++ b/lib/librte_ether/rte_ether_version.map
>@@ -102,6 +102,7 @@ DPDK_2.0 {
> 	rte_eth_tx_queue_setup;
> 	rte_eth_xstats_get;
> 	rte_eth_xstats_reset;
>+	rte_eth_dev_default_mac_addr_set;
> 
> 	local: *;
> };
>-- 
>2.1.4

Acked-by: Andrew Harvey (agh) <agh@cisco.com>

[dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address

[Liang-Min Larry Wang]

add a new api: rte_eth_dev_default_mac_addr_set to
support changing default mac address of a NIC

Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
---
 lib/librte_ether/rte_ethdev.c          | 18 ++++++++++++++++++
 lib/librte_ether/rte_ethdev.h          | 14 ++++++++++++++
 lib/librte_ether/rte_ether_version.map |  1 +
 3 files changed, 33 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 024fe8b..85ce72e 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -2752,6 +2752,24 @@ rte_eth_dev_mac_addr_remove(uint8_t port_id, struct ether_addr *addr)
 }
 
 int
+rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr *addr)
+{
+	struct rte_eth_dev *dev;
+
+	if (!rte_eth_dev_is_valid_port(port_id)) {
+		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+		return -ENODEV;
+	}
+
+	dev = &rte_eth_devices[port_id];
+	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
+
+	(*dev->dev_ops->mac_addr_set)(dev, addr);
+
+	return 0;
+}
+
+int
 rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
 				uint16_t rx_mode, uint8_t on)
 {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 16dbe00..5f07e0d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2982,6 +2982,20 @@ int rte_eth_dev_mac_addr_add(uint8_t port, struct ether_addr *mac_addr,
 int rte_eth_dev_mac_addr_remove(uint8_t port, struct ether_addr *mac_addr);
 
 /**
+ * Set the default MAC address.
+ *
+ * @param port
+ *   The port identifier of the Ethernet device.
+ * @param mac_addr
+ *   New default MAC address.
+ * @return
+ *   - (0) if successful, or *mac_addr* didn't exist.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port* invalid.
+ */
+int rte_eth_dev_default_mac_addr_set(uint8_t port, struct ether_addr *mac_addr);
+
+/**
  * Update Redirection Table(RETA) of Receive Side Scaling of Ethernet device.
  *
  * @param port
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index a2d25a6..2dbbaa7 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -102,6 +102,7 @@ DPDK_2.0 {
 	rte_eth_tx_queue_setup;
 	rte_eth_xstats_get;
 	rte_eth_xstats_reset;
+	rte_eth_dev_default_mac_addr_set;
 
 	local: *;
 };
-- 
2.1.4

Re: [dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address

[Wang, Liang-min]

>On Fri, 29 May 2015 09:15:08 -0400
>Liang-Min Larry Wang <liang-min.wang@intel.com> wrote:
>
>>  }
> > 
>>  int
>> +rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr 
>> +*addr) {
>> +	struct rte_eth_dev *dev;
>> +	const int index = 0;
>> +	const uint32_t pool = 0;
>> +
>> +	if (!rte_eth_dev_is_valid_port(port_id)) {
>> +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
>> +		return -ENODEV;
>> +	}
>> +
>> +	dev = &rte_eth_devices[port_id];
>> +	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_remove, -ENOTSUP);
>> +	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_add, -ENOTSUP);
>> +
>> +	/* Update NIC default MAC address*/
>> +	(*dev->dev_ops->mac_addr_remove)(dev, index);
>> +	(*dev->dev_ops->mac_addr_add)(dev, addr, index, pool);
>> +
>> +	/* Update default address in NIC data structure */
>> +	ether_addr_copy(addr, &dev->data->mac_addrs[index]);
>> +
>> +	return 0;
>> +}
>> +
>
>No. this won't work. for some devices.
>
>Please use mac_addr_set hook added in recent DPDK

I tested over ixgbe and igb, and both work. As for your concern, it's legit. I will take your suggestion and make modification.

Re: [dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address

[Stephen Hemminger]

On Fri, 29 May 2015 09:15:08 -0400
Liang-Min Larry Wang <liang-min.wang@intel.com> wrote:

>  }
>  
>  int
> +rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr *addr)
> +{
> +	struct rte_eth_dev *dev;
> +	const int index = 0;
> +	const uint32_t pool = 0;
> +
> +	if (!rte_eth_dev_is_valid_port(port_id)) {
> +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> +		return -ENODEV;
> +	}
> +
> +	dev = &rte_eth_devices[port_id];
> +	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_remove, -ENOTSUP);
> +	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_add, -ENOTSUP);
> +
> +	/* Update NIC default MAC address*/
> +	(*dev->dev_ops->mac_addr_remove)(dev, index);
> +	(*dev->dev_ops->mac_addr_add)(dev, addr, index, pool);
> +
> +	/* Update default address in NIC data structure */
> +	ether_addr_copy(addr, &dev->data->mac_addrs[index]);
> +
> +	return 0;
> +}
> +

No. this won't work. for some devices.

Please use mac_addr_set hook added in recent DPDK

[dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address

[Liang-Min Larry Wang]

add a new api: rte_eth_dev_default_mac_addr_set to
support changing default mac address of a NIC

Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
---
 lib/librte_ether/rte_ethdev.c          | 26 ++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.h          | 14 ++++++++++++++
 lib/librte_ether/rte_ether_version.map |  1 +
 3 files changed, 41 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 024fe8b..850b83c 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -2752,6 +2752,32 @@ rte_eth_dev_mac_addr_remove(uint8_t port_id, struct ether_addr *addr)
 }
 
 int
+rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr *addr)
+{
+	struct rte_eth_dev *dev;
+	const int index = 0;
+	const uint32_t pool = 0;
+
+	if (!rte_eth_dev_is_valid_port(port_id)) {
+		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+		return -ENODEV;
+	}
+
+	dev = &rte_eth_devices[port_id];
+	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_remove, -ENOTSUP);
+	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_add, -ENOTSUP);
+
+	/* Update NIC default MAC address*/
+	(*dev->dev_ops->mac_addr_remove)(dev, index);
+	(*dev->dev_ops->mac_addr_add)(dev, addr, index, pool);
+
+	/* Update default address in NIC data structure */
+	ether_addr_copy(addr, &dev->data->mac_addrs[index]);
+
+	return 0;
+}
+
+int
 rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
 				uint16_t rx_mode, uint8_t on)
 {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 16dbe00..5f07e0d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2982,6 +2982,20 @@ int rte_eth_dev_mac_addr_add(uint8_t port, struct ether_addr *mac_addr,
 int rte_eth_dev_mac_addr_remove(uint8_t port, struct ether_addr *mac_addr);
 
 /**
+ * Set the default MAC address.
+ *
+ * @param port
+ *   The port identifier of the Ethernet device.
+ * @param mac_addr
+ *   New default MAC address.
+ * @return
+ *   - (0) if successful, or *mac_addr* didn't exist.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port* invalid.
+ */
+int rte_eth_dev_default_mac_addr_set(uint8_t port, struct ether_addr *mac_addr);
+
+/**
  * Update Redirection Table(RETA) of Receive Side Scaling of Ethernet device.
  *
  * @param port
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index a2d25a6..2dbbaa7 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -102,6 +102,7 @@ DPDK_2.0 {
 	rte_eth_tx_queue_setup;
 	rte_eth_xstats_get;
 	rte_eth_xstats_reset;
+	rte_eth_dev_default_mac_addr_set;
 
 	local: *;
 };
-- 
2.1.4