[dpdk-dev] [PATCH] kni: use kni_ethtool_ops only with unknown drivers

[dpdk-dev] [PATCH] kni: use kni_ethtool_ops only with unknown drivers

[Igor Ryzhov]

Current implementation of kni_ethtool_ops just uses corresponding
ethtool_ops function of underlying driver for all functions except for
.get_link. This commit sets kni->net_dev->ethtool_ops directly to the
ethtool_ops of the corresponding driver.

For unknown drivers (all but ixgbe and i40e) we still use
kni_ethtool_ops with implemented .get_link function.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
---
 kernel/linux/kni/Makefile      |   2 +-
 kernel/linux/kni/kni_ethtool.c | 210 ---------------------------------
 kernel/linux/kni/kni_misc.c    |   6 +-
 3 files changed, 6 insertions(+), 212 deletions(-)

diff --git a/kernel/linux/kni/Makefile b/kernel/linux/kni/Makefile
index 282be7b68..ee5e1e136 100644
--- a/kernel/linux/kni/Makefile
+++ b/kernel/linux/kni/Makefile
@@ -30,7 +30,7 @@ endif
 #
 SRCS-y := kni_misc.c
 SRCS-y += kni_net.c
-SRCS-$(CONFIG_RTE_KNI_KMOD_ETHTOOL) += kni_ethtool.c
+SRCS-y += kni_ethtool.c
 
 SRCS-$(CONFIG_RTE_KNI_KMOD_ETHTOOL) += ethtool/ixgbe/ixgbe_main.c
 SRCS-$(CONFIG_RTE_KNI_KMOD_ETHTOOL) += ethtool/ixgbe/ixgbe_api.c
diff --git a/kernel/linux/kni/kni_ethtool.c b/kernel/linux/kni/kni_ethtool.c
index b1c84f8f0..ccfd58ef0 100644
--- a/kernel/linux/kni/kni_ethtool.c
+++ b/kernel/linux/kni/kni_ethtool.c
@@ -8,218 +8,8 @@
 #include <linux/ethtool.h>
 #include "kni_dev.h"
 
-static int
-kni_check_if_running(struct net_device *dev)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	if (priv->lad_dev)
-		return 0;
-	else
-		return -EOPNOTSUPP;
-}
-
-static void
-kni_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_drvinfo(priv->lad_dev, info);
-}
-
-/* ETHTOOL_GLINKSETTINGS replaces ETHTOOL_GSET */
-#ifndef ETHTOOL_GLINKSETTINGS
-static int
-kni_get_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->get_settings(priv->lad_dev, ecmd);
-}
-#endif
-
-/* ETHTOOL_SLINKSETTINGS replaces ETHTOOL_SSET */
-#ifndef ETHTOOL_SLINKSETTINGS
-static int
-kni_set_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->set_settings(priv->lad_dev, ecmd);
-}
-#endif
-
-static void
-kni_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_wol(priv->lad_dev, wol);
-}
-
-static int
-kni_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->set_wol(priv->lad_dev, wol);
-}
-
-static int
-kni_nway_reset(struct net_device *dev)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->nway_reset(priv->lad_dev);
-}
-
-static int
-kni_get_eeprom_len(struct net_device *dev)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->get_eeprom_len(priv->lad_dev);
-}
-
-static int
-kni_get_eeprom(struct net_device *dev, struct ethtool_eeprom *eeprom,
-							u8 *bytes)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->get_eeprom(priv->lad_dev, eeprom,
-								bytes);
-}
-
-static int
-kni_set_eeprom(struct net_device *dev, struct ethtool_eeprom *eeprom,
-							u8 *bytes)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->set_eeprom(priv->lad_dev, eeprom,
-								bytes);
-}
-
-static void
-kni_get_ringparam(struct net_device *dev, struct ethtool_ringparam *ring)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_ringparam(priv->lad_dev, ring);
-}
-
-static int
-kni_set_ringparam(struct net_device *dev, struct ethtool_ringparam *ring)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->set_ringparam(priv->lad_dev, ring);
-}
-
-static void
-kni_get_pauseparam(struct net_device *dev, struct ethtool_pauseparam *pause)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_pauseparam(priv->lad_dev, pause);
-}
-
-static int
-kni_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam *pause)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->set_pauseparam(priv->lad_dev,
-								pause);
-}
-
-static u32
-kni_get_msglevel(struct net_device *dev)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->get_msglevel(priv->lad_dev);
-}
-
-static void
-kni_set_msglevel(struct net_device *dev, u32 data)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->set_msglevel(priv->lad_dev, data);
-}
-
-static int
-kni_get_regs_len(struct net_device *dev)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->get_regs_len(priv->lad_dev);
-}
-
-static void
-kni_get_regs(struct net_device *dev, struct ethtool_regs *regs, void *p)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_regs(priv->lad_dev, regs, p);
-}
-
-static void
-kni_get_strings(struct net_device *dev, u32 stringset, u8 *data)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_strings(priv->lad_dev, stringset,
-								data);
-}
-
-static int
-kni_get_sset_count(struct net_device *dev, int sset)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->get_sset_count(priv->lad_dev, sset);
-}
-
-static void
-kni_get_ethtool_stats(struct net_device *dev, struct ethtool_stats *stats,
-								u64 *data)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_ethtool_stats(priv->lad_dev, stats,
-								data);
-}
-
 struct ethtool_ops kni_ethtool_ops = {
-	.begin			= kni_check_if_running,
-	.get_drvinfo		= kni_get_drvinfo,
-#ifndef ETHTOOL_GLINKSETTINGS
-	.get_settings		= kni_get_settings,
-#endif
-#ifndef ETHTOOL_SLINKSETTINGS
-	.set_settings		= kni_set_settings,
-#endif
-	.get_regs_len		= kni_get_regs_len,
-	.get_regs		= kni_get_regs,
-	.get_wol		= kni_get_wol,
-	.set_wol		= kni_set_wol,
-	.nway_reset		= kni_nway_reset,
 	.get_link		= ethtool_op_get_link,
-	.get_eeprom_len		= kni_get_eeprom_len,
-	.get_eeprom		= kni_get_eeprom,
-	.set_eeprom		= kni_set_eeprom,
-	.get_ringparam		= kni_get_ringparam,
-	.set_ringparam		= kni_set_ringparam,
-	.get_pauseparam		= kni_get_pauseparam,
-	.set_pauseparam		= kni_set_pauseparam,
-	.get_msglevel		= kni_get_msglevel,
-	.set_msglevel		= kni_set_msglevel,
-	.get_strings		= kni_get_strings,
-	.get_sset_count		= kni_get_sset_count,
-	.get_ethtool_stats	= kni_get_ethtool_stats,
 };
 
 void
diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
index 522ae23b9..1c4223185 100644
--- a/kernel/linux/kni/kni_misc.c
+++ b/kernel/linux/kni/kni_misc.c
@@ -426,7 +426,7 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
 							pci, lad_dev);
 			if (ret == 0) {
 				kni->lad_dev = lad_dev;
-				kni_set_ethtool_ops(kni->net_dev);
+				kni->net_dev->ethtool_ops = lad_dev->ethtool_ops;
 			} else {
 				pr_err("Device not supported by ethtool");
 				kni->lad_dev = NULL;
@@ -443,6 +443,10 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
 		pci_dev_put(pci);
 #endif
 
+	if (!kni->net_dev->ethtool_ops) {
+		kni_set_ethtool_ops(kni->net_dev);
+	}
+
 	if (kni->lad_dev)
 		ether_addr_copy(net_dev->dev_addr, kni->lad_dev->dev_addr);
 	else {
-- 
2.19.1

[dpdk-dev] [PATCH v2] kni: use kni_ethtool_ops only with unknown drivers

[Igor Ryzhov]

Current implementation of kni_ethtool_ops just uses corresponding
ethtool_ops function of underlying driver for all functions except for
.get_link. This commit sets kni->net_dev->ethtool_ops directly to the
ethtool_ops of the corresponding driver.

For unknown drivers (all but ixgbe and i40e) we still use
kni_ethtool_ops with implemented .get_link function.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
---
 kernel/linux/kni/Makefile      |   2 +-
 kernel/linux/kni/kni_ethtool.c | 210 ---------------------------------
 kernel/linux/kni/kni_misc.c    |   9 +-
 3 files changed, 7 insertions(+), 214 deletions(-)

diff --git a/kernel/linux/kni/Makefile b/kernel/linux/kni/Makefile
index 282be7b68..ee5e1e136 100644
--- a/kernel/linux/kni/Makefile
+++ b/kernel/linux/kni/Makefile
@@ -30,7 +30,7 @@ endif
 #
 SRCS-y := kni_misc.c
 SRCS-y += kni_net.c
-SRCS-$(CONFIG_RTE_KNI_KMOD_ETHTOOL) += kni_ethtool.c
+SRCS-y += kni_ethtool.c
 
 SRCS-$(CONFIG_RTE_KNI_KMOD_ETHTOOL) += ethtool/ixgbe/ixgbe_main.c
 SRCS-$(CONFIG_RTE_KNI_KMOD_ETHTOOL) += ethtool/ixgbe/ixgbe_api.c
diff --git a/kernel/linux/kni/kni_ethtool.c b/kernel/linux/kni/kni_ethtool.c
index b1c84f8f0..ccfd58ef0 100644
--- a/kernel/linux/kni/kni_ethtool.c
+++ b/kernel/linux/kni/kni_ethtool.c
@@ -8,218 +8,8 @@
 #include <linux/ethtool.h>
 #include "kni_dev.h"
 
-static int
-kni_check_if_running(struct net_device *dev)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	if (priv->lad_dev)
-		return 0;
-	else
-		return -EOPNOTSUPP;
-}
-
-static void
-kni_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_drvinfo(priv->lad_dev, info);
-}
-
-/* ETHTOOL_GLINKSETTINGS replaces ETHTOOL_GSET */
-#ifndef ETHTOOL_GLINKSETTINGS
-static int
-kni_get_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->get_settings(priv->lad_dev, ecmd);
-}
-#endif
-
-/* ETHTOOL_SLINKSETTINGS replaces ETHTOOL_SSET */
-#ifndef ETHTOOL_SLINKSETTINGS
-static int
-kni_set_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->set_settings(priv->lad_dev, ecmd);
-}
-#endif
-
-static void
-kni_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_wol(priv->lad_dev, wol);
-}
-
-static int
-kni_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->set_wol(priv->lad_dev, wol);
-}
-
-static int
-kni_nway_reset(struct net_device *dev)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->nway_reset(priv->lad_dev);
-}
-
-static int
-kni_get_eeprom_len(struct net_device *dev)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->get_eeprom_len(priv->lad_dev);
-}
-
-static int
-kni_get_eeprom(struct net_device *dev, struct ethtool_eeprom *eeprom,
-							u8 *bytes)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->get_eeprom(priv->lad_dev, eeprom,
-								bytes);
-}
-
-static int
-kni_set_eeprom(struct net_device *dev, struct ethtool_eeprom *eeprom,
-							u8 *bytes)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->set_eeprom(priv->lad_dev, eeprom,
-								bytes);
-}
-
-static void
-kni_get_ringparam(struct net_device *dev, struct ethtool_ringparam *ring)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_ringparam(priv->lad_dev, ring);
-}
-
-static int
-kni_set_ringparam(struct net_device *dev, struct ethtool_ringparam *ring)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->set_ringparam(priv->lad_dev, ring);
-}
-
-static void
-kni_get_pauseparam(struct net_device *dev, struct ethtool_pauseparam *pause)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_pauseparam(priv->lad_dev, pause);
-}
-
-static int
-kni_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam *pause)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->set_pauseparam(priv->lad_dev,
-								pause);
-}
-
-static u32
-kni_get_msglevel(struct net_device *dev)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->get_msglevel(priv->lad_dev);
-}
-
-static void
-kni_set_msglevel(struct net_device *dev, u32 data)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->set_msglevel(priv->lad_dev, data);
-}
-
-static int
-kni_get_regs_len(struct net_device *dev)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->get_regs_len(priv->lad_dev);
-}
-
-static void
-kni_get_regs(struct net_device *dev, struct ethtool_regs *regs, void *p)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_regs(priv->lad_dev, regs, p);
-}
-
-static void
-kni_get_strings(struct net_device *dev, u32 stringset, u8 *data)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_strings(priv->lad_dev, stringset,
-								data);
-}
-
-static int
-kni_get_sset_count(struct net_device *dev, int sset)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	return priv->lad_dev->ethtool_ops->get_sset_count(priv->lad_dev, sset);
-}
-
-static void
-kni_get_ethtool_stats(struct net_device *dev, struct ethtool_stats *stats,
-								u64 *data)
-{
-	struct kni_dev *priv = netdev_priv(dev);
-
-	priv->lad_dev->ethtool_ops->get_ethtool_stats(priv->lad_dev, stats,
-								data);
-}
-
 struct ethtool_ops kni_ethtool_ops = {
-	.begin			= kni_check_if_running,
-	.get_drvinfo		= kni_get_drvinfo,
-#ifndef ETHTOOL_GLINKSETTINGS
-	.get_settings		= kni_get_settings,
-#endif
-#ifndef ETHTOOL_SLINKSETTINGS
-	.set_settings		= kni_set_settings,
-#endif
-	.get_regs_len		= kni_get_regs_len,
-	.get_regs		= kni_get_regs,
-	.get_wol		= kni_get_wol,
-	.set_wol		= kni_set_wol,
-	.nway_reset		= kni_nway_reset,
 	.get_link		= ethtool_op_get_link,
-	.get_eeprom_len		= kni_get_eeprom_len,
-	.get_eeprom		= kni_get_eeprom,
-	.set_eeprom		= kni_set_eeprom,
-	.get_ringparam		= kni_get_ringparam,
-	.set_ringparam		= kni_set_ringparam,
-	.get_pauseparam		= kni_get_pauseparam,
-	.set_pauseparam		= kni_set_pauseparam,
-	.get_msglevel		= kni_get_msglevel,
-	.set_msglevel		= kni_set_msglevel,
-	.get_strings		= kni_get_strings,
-	.get_sset_count		= kni_get_sset_count,
-	.get_ethtool_stats	= kni_get_ethtool_stats,
 };
 
 void
diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
index 522ae23b9..e22a1a717 100644
--- a/kernel/linux/kni/kni_misc.c
+++ b/kernel/linux/kni/kni_misc.c
@@ -426,7 +426,6 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
 							pci, lad_dev);
 			if (ret == 0) {
 				kni->lad_dev = lad_dev;
-				kni_set_ethtool_ops(kni->net_dev);
 			} else {
 				pr_err("Device not supported by ethtool");
 				kni->lad_dev = NULL;
@@ -443,9 +442,13 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
 		pci_dev_put(pci);
 #endif
 
-	if (kni->lad_dev)
+	if (kni->lad_dev) {
+		kni->net_dev->ethtool_ops = kni->lad_dev->ethtool_ops;
+
 		ether_addr_copy(net_dev->dev_addr, kni->lad_dev->dev_addr);
-	else {
+	} else {
+		kni_set_ethtool_ops(kni->net_dev);
+
 		/* if user has provided a valid mac address */
 		if (is_valid_ether_addr((unsigned char *)(dev_info.mac_addr)))
 			memcpy(net_dev->dev_addr, dev_info.mac_addr, ETH_ALEN);
-- 
2.19.1

Re: [dpdk-dev] [PATCH v2] kni: use kni_ethtool_ops only with unknown drivers

[Stephen Hemminger]

On Fri, 30 Nov 2018 22:47:50 +0300
Igor Ryzhov <iryzhov@nfware.com> wrote:

> Current implementation of kni_ethtool_ops just uses corresponding
> ethtool_ops function of underlying driver for all functions except for
> .get_link. This commit sets kni->net_dev->ethtool_ops directly to the
> ethtool_ops of the corresponding driver.
> 
> For unknown drivers (all but ixgbe and i40e) we still use
> kni_ethtool_ops with implemented .get_link function.
> 
> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>

Why does KNI still support ethtool which:
  1. Only works on a subset of devices
  2. Requires a 3rd implmentation of the HW device (Linux, DPDK, and KNI)

Then again why does KNI exist at all? What is missing from virtio user which
is faster anyway.

Re: [dpdk-dev] [PATCH v2] kni: use kni_ethtool_ops only with unknown drivers

[Igor Ryzhov]

Hi Stephen,

I also do not see the point of the current implementation of ethtool
support.
That's why I sent this patch – it enables ethtool_ops for all devices,
independent of the underlying driver.
Right now only .get_link is supported, but I am thinking about
implementation of a larger set of functions, using req/resp queue, like
netdev_ops functions are working.

Regarding the KNI itself, we use it as Linux mirror of physical port for:
1. Port configuration from Linux – such functions as set_mac, change_mtu,
etc. And ethtool_ops will be used the same way.
2. Passing control-plane packets to Linux.

Can virtio user be used the same way, as a mirror of physical port?

Best regards,
Igor

On Sat, Dec 1, 2018 at 2:38 AM Stephen Hemminger <stephen@networkplumber.org>
wrote:

> On Fri, 30 Nov 2018 22:47:50 +0300
> Igor Ryzhov <iryzhov@nfware.com> wrote:
>
> > Current implementation of kni_ethtool_ops just uses corresponding
> > ethtool_ops function of underlying driver for all functions except for
> > .get_link. This commit sets kni->net_dev->ethtool_ops directly to the
> > ethtool_ops of the corresponding driver.
> >
> > For unknown drivers (all but ixgbe and i40e) we still use
> > kni_ethtool_ops with implemented .get_link function.
> >
> > Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
>
> Why does KNI still support ethtool which:
>   1. Only works on a subset of devices
>   2. Requires a 3rd implmentation of the HW device (Linux, DPDK, and KNI)
>
> Then again why does KNI exist at all? What is missing from virtio user
> which
> is faster anyway.
>

Re: [dpdk-dev] [PATCH v2] kni: use kni_ethtool_ops only with unknown drivers

[Stephen Hemminger]

On Sat, 1 Dec 2018 14:12:54 +0300
Igor Ryzhov <iryzhov@nfware.com> wrote:

> Hi Stephen,
> 
> I also do not see the point of the current implementation of ethtool
> support.
> That's why I sent this patch – it enables ethtool_ops for all devices,
> independent of the underlying driver.
> Right now only .get_link is supported, but I am thinking about
> implementation of a larger set of functions, using req/resp queue, like
> netdev_ops functions are working.
> 
> Regarding the KNI itself, we use it as Linux mirror of physical port for:
> 1. Port configuration from Linux – such functions as set_mac, change_mtu,
> etc. And ethtool_ops will be used the same way.
> 2. Passing control-plane packets to Linux.
> 
> Can virtio user be used the same way, as a mirror of physical port?
> 
> Best regards,
> Igor

In Linux if device does not supply get_link the base code does the
right thing


u32 ethtool_op_get_link(struct net_device *dev)
{
	return netif_carrier_ok(dev) ? 1 : 0;
}



Doing set_mac, change_mtu and ethtool_ops in virtio_user should be possible
but probably not implemented.

Re: [dpdk-dev] [PATCH v2] kni: use kni_ethtool_ops only with unknown drivers

[Igor Ryzhov]

Stephen,

ethtool_get_link returns EOPNOTSUPP if device doesn't supply get_link:

static int ethtool_get_link(struct net_device *dev, char __user *useraddr)
{
struct ethtool_value edata = { .cmd = ETHTOOL_GLINK };

if (!dev->ethtool_ops->get_link)
return -EOPNOTSUPP;

edata.data = netif_running(dev) && dev->ethtool_ops->get_link(dev);

if (copy_to_user(useraddr, &edata, sizeof(edata)))
return -EFAULT;
return 0;
}

On Sat, Dec 1, 2018 at 8:31 PM Stephen Hemminger <stephen@networkplumber.org>
wrote:

> On Sat, 1 Dec 2018 14:12:54 +0300
> Igor Ryzhov <iryzhov@nfware.com> wrote:
>
> > Hi Stephen,
> >
> > I also do not see the point of the current implementation of ethtool
> > support.
> > That's why I sent this patch – it enables ethtool_ops for all devices,
> > independent of the underlying driver.
> > Right now only .get_link is supported, but I am thinking about
> > implementation of a larger set of functions, using req/resp queue, like
> > netdev_ops functions are working.
> >
> > Regarding the KNI itself, we use it as Linux mirror of physical port for:
> > 1. Port configuration from Linux – such functions as set_mac, change_mtu,
> > etc. And ethtool_ops will be used the same way.
> > 2. Passing control-plane packets to Linux.
> >
> > Can virtio user be used the same way, as a mirror of physical port?
> >
> > Best regards,
> > Igor
>
> In Linux if device does not supply get_link the base code does the
> right thing
>
>
> u32 ethtool_op_get_link(struct net_device *dev)
> {
>         return netif_carrier_ok(dev) ? 1 : 0;
> }
>
>
>
> Doing set_mac, change_mtu and ethtool_ops in virtio_user should be possible
> but probably not implemented.
>
>

Re: [dpdk-dev] [PATCH v2] kni: use kni_ethtool_ops only with unknown drivers

[Stephen Hemminger]

On Sun, 2 Dec 2018 13:54:11 +0300
Igor Ryzhov <iryzhov@nfware.com> wrote:

> Stephen,
> 
> ethtool_get_link returns EOPNOTSUPP if device doesn't supply get_link:
> 

You are right, kni needs to supply ethool ops and can use the standard
ethtool_op_get_link as the callback.

Re: [dpdk-dev] [PATCH v2] kni: use kni_ethtool_ops only with unknown drivers

[Ferruh Yigit]

On 12/1/2018 11:12 AM, Igor Ryzhov wrote:
> Hi Stephen,
> 
> I also do not see the point of the current implementation of ethtool
> support.
> That's why I sent this patch – it enables ethtool_ops for all devices,
> independent of the underlying driver.

I tried to clarify this in the patch, but it doesn't enable ethtool_ops for
underlying driver.

> Right now only .get_link is supported, but I am thinking about
> implementation of a larger set of functions, using req/resp queue, like
> netdev_ops functions are working.

You should go to userspace to get device information, right. Since now userspace
has the driver. So a channel between kernel and userspace is required.
Please share your plans/design on this support.

> 
> Regarding the KNI itself, we use it as Linux mirror of physical port for:
> 1. Port configuration from Linux – such functions as set_mac, change_mtu,
> etc. And ethtool_ops will be used the same way.
> 2. Passing control-plane packets to Linux.
> 
> Can virtio user be used the same way, as a mirror of physical port?
> 
> Best regards,
> Igor
> 
> On Sat, Dec 1, 2018 at 2:38 AM Stephen Hemminger <stephen@networkplumber.org>
> wrote:
> 
>> On Fri, 30 Nov 2018 22:47:50 +0300
>> Igor Ryzhov <iryzhov@nfware.com> wrote:
>>
>>> Current implementation of kni_ethtool_ops just uses corresponding
>>> ethtool_ops function of underlying driver for all functions except for
>>> .get_link. This commit sets kni->net_dev->ethtool_ops directly to the
>>> ethtool_ops of the corresponding driver.
>>>
>>> For unknown drivers (all but ixgbe and i40e) we still use
>>> kni_ethtool_ops with implemented .get_link function.
>>>
>>> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
>>
>> Why does KNI still support ethtool which:
>>   1. Only works on a subset of devices
>>   2. Requires a 3rd implmentation of the HW device (Linux, DPDK, and KNI)
>>
>> Then again why does KNI exist at all? What is missing from virtio user
>> which
>> is faster anyway.
>>

Re: [dpdk-dev] [PATCH v2] kni: use kni_ethtool_ops only with unknown drivers

[Ferruh Yigit]

On 11/30/2018 11:38 PM, Stephen Hemminger wrote:
> On Fri, 30 Nov 2018 22:47:50 +0300
> Igor Ryzhov <iryzhov@nfware.com> wrote:
> 
>> Current implementation of kni_ethtool_ops just uses corresponding
>> ethtool_ops function of underlying driver for all functions except for
>> .get_link. This commit sets kni->net_dev->ethtool_ops directly to the
>> ethtool_ops of the corresponding driver.
>>
>> For unknown drivers (all but ixgbe and i40e) we still use
>> kni_ethtool_ops with implemented .get_link function.
>>
>> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
> 
> Why does KNI still support ethtool which:
>   1. Only works on a subset of devices
>   2. Requires a 3rd implmentation of the HW device (Linux, DPDK, and KNI)

+1 to drop ethtool support, last time we tried concern was anybody may be using
it, perhaps we can try again.

> 
> Then again why does KNI exist at all? What is missing from virtio user which
> is faster anyway.
> 

Re: [dpdk-dev] [PATCH v2] kni: use kni_ethtool_ops only with unknown drivers

[Igor Ryzhov]

Hi Ferruh,

What about the patch?

I also support dropping ethtool for ixgbe and i40e, but to save generic
ethtool_ops
with .get_link implementation, because it's an essential function that
works correctly
after proper implementation of carrier status that was merged into 18.11.

Also, other ethtool operations may be implemented in a driver-independent
way using
the same concept as for netdev_ops.

On Mon, Dec 3, 2018 at 4:09 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 11/30/2018 11:38 PM, Stephen Hemminger wrote:
> > On Fri, 30 Nov 2018 22:47:50 +0300
> > Igor Ryzhov <iryzhov@nfware.com> wrote:
> >
> >> Current implementation of kni_ethtool_ops just uses corresponding
> >> ethtool_ops function of underlying driver for all functions except for
> >> .get_link. This commit sets kni->net_dev->ethtool_ops directly to the
> >> ethtool_ops of the corresponding driver.
> >>
> >> For unknown drivers (all but ixgbe and i40e) we still use
> >> kni_ethtool_ops with implemented .get_link function.
> >>
> >> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
> >
> > Why does KNI still support ethtool which:
> >   1. Only works on a subset of devices
> >   2. Requires a 3rd implmentation of the HW device (Linux, DPDK, and KNI)
>
> +1 to drop ethtool support, last time we tried concern was anybody may be
> using
> it, perhaps we can try again.
>
> >
> > Then again why does KNI exist at all? What is missing from virtio user
> which
> > is faster anyway.
> >
>
>

Re: [dpdk-dev] [PATCH v2] kni: use kni_ethtool_ops only with unknown drivers

[Ferruh Yigit]

On 12/3/2018 2:06 PM, Igor Ryzhov wrote:
> Hi Ferruh,
> 
> What about the patch?
> 
> I also support dropping ethtool for ixgbe and i40e, but to save generic ethtool_ops
> with .get_link implementation, because it's an essential function that works
> correctly
> after proper implementation of carrier status that was merged into 18.11.
> 
> Also, other ethtool operations may be implemented in a driver-independent way using
> the same concept as for netdev_ops.

"carrier status" relies on the sample app support also relies on kni net_device
sysfs interface.
It is good target to have ethtool support in a driver-independent way, please
share more details and lets discuss them.

> 
> On Mon, Dec 3, 2018 at 4:09 PM Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 11/30/2018 11:38 PM, Stephen Hemminger wrote:
>     > On Fri, 30 Nov 2018 22:47:50 +0300
>     > Igor Ryzhov <iryzhov@nfware.com <mailto:iryzhov@nfware.com>> wrote:
>     >
>     >> Current implementation of kni_ethtool_ops just uses corresponding
>     >> ethtool_ops function of underlying driver for all functions except for
>     >> .get_link. This commit sets kni->net_dev->ethtool_ops directly to the
>     >> ethtool_ops of the corresponding driver.
>     >>
>     >> For unknown drivers (all but ixgbe and i40e) we still use
>     >> kni_ethtool_ops with implemented .get_link function.
>     >>
>     >> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com <mailto:iryzhov@nfware.com>>
>     >
>     > Why does KNI still support ethtool which:
>     >   1. Only works on a subset of devices
>     >   2. Requires a 3rd implmentation of the HW device (Linux, DPDK, and KNI)
> 
>     +1 to drop ethtool support, last time we tried concern was anybody may be using
>     it, perhaps we can try again.
> 
>     >
>     > Then again why does KNI exist at all? What is missing from virtio user which
>     > is faster anyway.
>     >
> 

Re: [dpdk-dev] [PATCH v2] kni: use kni_ethtool_ops only with unknown drivers

[Igor Ryzhov]

Hi Ferruh,

Thank you for all your comments.

The only real purpose of the patch is to support .get_link for all KNI
interfaces, not just for those using igb or ixgbe.
So I propose to remove ethtool support at all with your patch, and after
that, I will add .get_link support again.

Yes, I understand that it doesn't implement real link status support, it
just relies on the application, that should set link status using sysfs.

Regarding the ethtool support in a driver-independent way – the idea is to
use the same req/resp queues
which are used for some net_device_ops like .ndo_change_mtu or
.ndo_set_mac_address.
I want to add more types of requests to rte_kni_req_id enum and implement
them.
It's not an urgent task, so I don't know yet when I'll find time to do it.

Best regards,
Igor

On Tue, Dec 18, 2018 at 9:13 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 12/3/2018 2:06 PM, Igor Ryzhov wrote:
> > Hi Ferruh,
> >
> > What about the patch?
> >
> > I also support dropping ethtool for ixgbe and i40e, but to save generic
> ethtool_ops
> > with .get_link implementation, because it's an essential function that
> works
> > correctly
> > after proper implementation of carrier status that was merged into 18.11.
> >
> > Also, other ethtool operations may be implemented in a
> driver-independent way using
> > the same concept as for netdev_ops.
>
> "carrier status" relies on the sample app support also relies on kni
> net_device
> sysfs interface.
> It is good target to have ethtool support in a driver-independent way,
> please
> share more details and lets discuss them.
>
> >
> > On Mon, Dec 3, 2018 at 4:09 PM Ferruh Yigit <ferruh.yigit@intel.com
> > <mailto:ferruh.yigit@intel.com>> wrote:
> >
> >     On 11/30/2018 11:38 PM, Stephen Hemminger wrote:
> >     > On Fri, 30 Nov 2018 22:47:50 +0300
> >     > Igor Ryzhov <iryzhov@nfware.com <mailto:iryzhov@nfware.com>>
> wrote:
> >     >
> >     >> Current implementation of kni_ethtool_ops just uses corresponding
> >     >> ethtool_ops function of underlying driver for all functions
> except for
> >     >> .get_link. This commit sets kni->net_dev->ethtool_ops directly to
> the
> >     >> ethtool_ops of the corresponding driver.
> >     >>
> >     >> For unknown drivers (all but ixgbe and i40e) we still use
> >     >> kni_ethtool_ops with implemented .get_link function.
> >     >>
> >     >> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com <mailto:
> iryzhov@nfware.com>>
> >     >
> >     > Why does KNI still support ethtool which:
> >     >   1. Only works on a subset of devices
> >     >   2. Requires a 3rd implmentation of the HW device (Linux, DPDK,
> and KNI)
> >
> >     +1 to drop ethtool support, last time we tried concern was anybody
> may be using
> >     it, perhaps we can try again.
> >
> >     >
> >     > Then again why does KNI exist at all? What is missing from virtio
> user which
> >     > is faster anyway.
> >     >
> >
>
>

Re: [dpdk-dev] [PATCH v2] kni: use kni_ethtool_ops only with unknown drivers

[Ferruh Yigit]

On 11/30/2018 7:47 PM, Igor Ryzhov wrote:
> Current implementation of kni_ethtool_ops just uses corresponding
> ethtool_ops function of underlying driver for all functions except for
> .get_link. This commit sets kni->net_dev->ethtool_ops directly to the
> ethtool_ops of the corresponding driver.
I think we can't assign ethtool_ops directly to the driver ethtool_ops.

There are two net_device in the system for the ethtool support case:
1) kni net_device
2) driver net_device

Linux kernel driver probed() at some level at least a net_device struct created
for it, that is how we have 2). And 2), driver net_device, is saved in the
context of the kni.

The function of the kni_ethtool.c is, convert "kni net_device" to "driver
net_device":

 struct kni_dev *priv = netdev_priv(dev);
 ethtool_ops->xxx(priv->lad_dev, ...);

When you are directly set kni ethtool_ops to driver ethtool_ops, you will be
passing "kni net_device" to the kernel driver. I can't predict what will be the
results there.
How this can work, did you able to test this?

> 
> For unknown drivers (all but ixgbe and i40e) we still use

It is igb & ixgbe, i40e is not supported but it is a detail only.

> kni_ethtool_ops with implemented .get_link function.

Not exactly, unless I am missing something.

.get_link function is not implemented for "unknown drivers". It gets carrier
status of the kni net_device. There is nothing related to the underlying device
here.

For the ethtool functions you have removed, they goes to hardware mapped to kni
interface and gets the value from them. Existing .get_link just gets the kni
interface value, it doesn't go to any hardware.

In *sample* application a logic has been added to reflect the hardware link
status to kni interface thorough the sysfs interface kni has, if you are
referring that one, it is not implementing ethtool support for "unknown
drivers", it is relying to application later to reflect hardware link status to
kni interface so that you can read from kni interface.

> 
> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
> ---
>  kernel/linux/kni/Makefile      |   2 +-
>  kernel/linux/kni/kni_ethtool.c | 210 ---------------------------------
>  kernel/linux/kni/kni_misc.c    |   9 +-
>  3 files changed, 7 insertions(+), 214 deletions(-)

<...>