netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 1/3] tuntap: allow changing ethtool speed (v2)
@ 2013-07-24 23:11 Stephen Hemminger
  2013-07-24 23:13 ` [PATCH net-next 2/3] tuntap: allow overriding ethtool driver info Stephen Hemminger
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Stephen Hemminger @ 2013-07-24 23:11 UTC (permalink / raw)
  To: Helmut Grohne, David Miller; +Cc: netdev

The data reported by the ethtool interface for tun/tap devices is
artificial. The default speed is 10Mbit. Virtual interfaces are often
faster than this nowadays, so the observed bandwidth may exceed the
available bandwidth. This patch allows an administrator to change the
available bandwidth as reported by ethtool.

Signed-off-by: Helmut Grohne <h.grohne@cygnusnetworks.de>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

---
v2 - allow setting duplex as well, fix patch formatting
     ignore other settings in set

 drivers/net/tun.c |   25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

--- a/drivers/net/tun.c	2013-07-24 11:24:55.543040360 -0700
+++ b/drivers/net/tun.c	2013-07-24 11:33:06.759433743 -0700
@@ -187,6 +187,8 @@ struct tun_struct {
 	struct list_head disabled;
 	void *security;
 	u32 flow_count;
+	u32 speed;
+	u8 duplex;
 };
 
 static inline u32 tun_hashfn(u32 rxhash)
@@ -1426,6 +1428,8 @@ static void tun_setup(struct net_device
 
 	tun->owner = INVALID_UID;
 	tun->group = INVALID_GID;
+	tun->speed = SPEED_10;
+	tun->duplex = DUPLEX_FULL;
 
 	dev->ethtool_ops = &tun_ethtool_ops;
 	dev->destructor = tun_free_netdev;
@@ -2229,10 +2233,12 @@ static struct miscdevice tun_miscdev = {
 
 static int tun_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
+	struct tun_struct *tun = netdev_priv(dev);
+
 	cmd->supported		= 0;
 	cmd->advertising	= 0;
-	ethtool_cmd_speed_set(cmd, SPEED_10);
-	cmd->duplex		= DUPLEX_FULL;
+	ethtool_cmd_speed_set(cmd, tun->speed);
+	cmd->duplex		= tun->duplex;
 	cmd->port		= PORT_TP;
 	cmd->phy_address	= 0;
 	cmd->transceiver	= XCVR_INTERNAL;
@@ -2242,6 +2248,21 @@ static int tun_get_settings(struct net_d
 	return 0;
 }
 
+static int tun_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+
+	if (cmd->autoneg != AUTONEG_DISABLE)
+		return -EINVAL;
+
+	if (!(cmd->duplex == DUPLEX_FULL || cmd->duplex == DUPLEX_HALF))
+		return -EINVAL;
+
+	tun->speed = ethtool_cmd_speed(cmd);
+	tun->duplex = cmd->duplex;
+	return 0;
+}
+
 static void tun_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
 {
 	struct tun_struct *tun = netdev_priv(dev);
@@ -2279,6 +2300,7 @@ static void tun_set_msglevel(struct net_
 
 static const struct ethtool_ops tun_ethtool_ops = {
 	.get_settings	= tun_get_settings,
+	.set_settings	= tun_set_settings,
 	.get_drvinfo	= tun_get_drvinfo,
 	.get_msglevel	= tun_get_msglevel,
 	.set_msglevel	= tun_set_msglevel,

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next 2/3] tuntap: allow overriding ethtool driver info
  2013-07-24 23:11 [PATCH net-next 1/3] tuntap: allow changing ethtool speed (v2) Stephen Hemminger
@ 2013-07-24 23:13 ` Stephen Hemminger
  2013-07-24 23:48   ` Ben Hutchings
  2013-07-24 23:15 ` [PATCH net-next 3/3] tuntap: allow overriding link statistics Stephen Hemminger
  2013-07-24 23:40 ` [PATCH net-next 1/3] tuntap: allow changing ethtool speed (v2) Ben Hutchings
  2 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2013-07-24 23:13 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Helmut Grohne, David Miller, netdev

This patch adds new ioctl to allow setting the ethtool information
returned by the TUN device. This is useful when using tun device as a surrogate
for hardware or other software emulation.

If the application does not override the ethtool settings, the
original hard coded values are used.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

---
 drivers/net/tun.c           |   56 +++++++++++++++++++++++++++++++++-----------
 include/uapi/linux/if_tun.h |   15 +++++++++++
 2 files changed, 58 insertions(+), 13 deletions(-)

--- a/drivers/net/tun.c	2013-07-24 11:33:06.759433743 -0700
+++ b/drivers/net/tun.c	2013-07-24 11:39:56.233298852 -0700
@@ -189,6 +189,7 @@ struct tun_struct {
 	u32 flow_count;
 	u32 speed;
 	u8 duplex;
+	struct tun_info info;
 };
 
 static inline u32 tun_hashfn(u32 rxhash)
@@ -872,9 +873,13 @@ static void tun_net_init(struct net_devi
 {
 	struct tun_struct *tun = netdev_priv(dev);
 
+	strlcpy(tun->info.driver, DRV_NAME, sizeof(tun->info.driver));
+	strlcpy(tun->info.version, DRV_VERSION, sizeof(tun->info.version));
+
 	switch (tun->flags & TUN_TYPE_MASK) {
 	case TUN_TUN_DEV:
 		dev->netdev_ops = &tun_netdev_ops;
+		strlcpy(tun->info.bus, "tun", sizeof(tun->info.bus));
 
 		/* Point-to-Point TUN Device */
 		dev->hard_header_len = 0;
@@ -889,6 +894,8 @@ static void tun_net_init(struct net_devi
 
 	case TUN_TAP_DEV:
 		dev->netdev_ops = &tap_netdev_ops;
+		strlcpy(tun->info.bus, "tap", sizeof(tun->info.bus));
+
 		/* Ethernet TAP Device */
 		ether_setup(dev);
 		dev->priv_flags &= ~IFF_TX_SKB_SHARING;
@@ -2092,6 +2099,15 @@ static long __tun_chr_ioctl(struct file
 		tun_detach_filter(tun, tun->numqueues);
 		break;
 
+	case TUNSETINFO: {
+		struct tun_info info;
+
+		if (copy_from_user(&info, argp, sizeof(info)))
+			return -EFAULT;
+
+		tun->info = info;
+		break;
+	}
 	default:
 		ret = -EINVAL;
 		break;
@@ -2120,6 +2136,7 @@ static long tun_chr_compat_ioctl(struct
 	case TUNSETTXFILTER:
 	case TUNGETSNDBUF:
 	case TUNSETSNDBUF:
+	case TUNSETINFO:
 	case SIOCGIFHWADDR:
 	case SIOCSIFHWADDR:
 		arg = (unsigned long)compat_ptr(arg);
@@ -2267,17 +2284,9 @@ static void tun_get_drvinfo(struct net_d
 {
 	struct tun_struct *tun = netdev_priv(dev);
 
-	strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
-	strlcpy(info->version, DRV_VERSION, sizeof(info->version));
-
-	switch (tun->flags & TUN_TYPE_MASK) {
-	case TUN_TUN_DEV:
-		strlcpy(info->bus_info, "tun", sizeof(info->bus_info));
-		break;
-	case TUN_TAP_DEV:
-		strlcpy(info->bus_info, "tap", sizeof(info->bus_info));
-		break;
-	}
+	strlcpy(info->driver, tun->info.driver, sizeof(info->driver));
+	strlcpy(info->version, tun->info.version, sizeof(info->version));
+	strlcpy(info->bus_info, tun->info.bus, sizeof(info->bus_info));
 }
 
 static u32 tun_get_msglevel(struct net_device *dev)
--- a/include/uapi/linux/if_tun.h	2013-07-24 11:24:55.543040360 -0700
+++ b/include/uapi/linux/if_tun.h	2013-07-24 11:35:54.620898504 -0700
@@ -56,6 +56,7 @@
 #define TUNGETVNETHDRSZ _IOR('T', 215, int)
 #define TUNSETVNETHDRSZ _IOW('T', 216, int)
 #define TUNSETQUEUE  _IOW('T', 217, int)
+#define TUNSETINFO     _IOW('T', 219, struct tun_info)
 
 /* TUNSETIFF ifr flags */
 #define IFF_TUN		0x0001
@@ -103,4 +104,11 @@ struct tun_filter {
 	__u8   addr[0][ETH_ALEN];
 };
 
+/* Subset of ethtool_info */
+struct tun_info {
+	char driver[32];
+	char bus[32];
+	char version[32];
+};
+
 #endif /* _UAPI__IF_TUN_H */

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next 3/3] tuntap: allow overriding link statistics
  2013-07-24 23:11 [PATCH net-next 1/3] tuntap: allow changing ethtool speed (v2) Stephen Hemminger
  2013-07-24 23:13 ` [PATCH net-next 2/3] tuntap: allow overriding ethtool driver info Stephen Hemminger
@ 2013-07-24 23:15 ` Stephen Hemminger
  2013-07-24 23:40 ` [PATCH net-next 1/3] tuntap: allow changing ethtool speed (v2) Ben Hutchings
  2 siblings, 0 replies; 12+ messages in thread
From: Stephen Hemminger @ 2013-07-24 23:15 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Helmut Grohne, David Miller, netdev

This patch adds new ioctl to allow overriding the link statistics
returned by the TUN device. This is useful when using tun device as a surrogate
for hardware or other software emulation. To use this application periodically
makes ioctl(TUNSETSTATS) to update statistics.

If TUNSETSTATS is not used the original software based statistics
are used.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

---
v2 - don't use exchange, use regular RCU

--- a/drivers/net/tun.c	2013-07-24 12:47:37.344206628 -0700
+++ b/drivers/net/tun.c	2013-07-24 16:14:00.964658040 -0700
@@ -152,6 +152,11 @@ struct tun_flow_entry {
 	unsigned long updated;
 };
 
+struct tun_link_stats {
+	struct rtnl_link_stats64 link_stats;
+	struct rcu_head rcu;
+};
+
 #define TUN_NUM_FLOW_ENTRIES 1024
 
 /* Since the socket were moved to tun_file, to preserve the behavior of persist
@@ -190,6 +195,7 @@ struct tun_struct {
 	u32 speed;
 	u8 duplex;
 	struct tun_info info;
+	struct tun_link_stats __rcu *stats;
 };
 
 static inline u32 tun_hashfn(u32 rxhash)
@@ -803,6 +809,32 @@ static netdev_features_t tun_net_fix_fea
 
 	return (features & tun->set_features) | (features & ~TUN_USER_FEATURES);
 }
+
+static struct rtnl_link_stats64 *
+tun_net_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *storage)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+	struct tun_link_stats *stats;
+
+	rcu_read_lock();
+	stats = rcu_dereference(tun->stats);
+	if (stats) {
+		/* Stats received from device */
+		*storage = stats->link_stats;
+		rcu_read_unlock();
+
+		/* Add tunnel detected errors to mix */
+		storage->tx_dropped += dev->stats.tx_dropped;
+		storage->rx_dropped += dev->stats.rx_dropped;
+		storage->rx_frame_errors += dev->stats.rx_frame_errors;
+		return storage;
+	}
+	rcu_read_unlock();
+
+	netdev_stats_to_stats64(storage, &dev->stats);
+	return storage;
+}
+
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static void tun_poll_controller(struct net_device *dev)
 {
@@ -825,6 +857,7 @@ static const struct net_device_ops tun_n
 	.ndo_open		= tun_net_open,
 	.ndo_stop		= tun_net_close,
 	.ndo_start_xmit		= tun_net_xmit,
+	.ndo_get_stats64	= tun_net_get_stats64,
 	.ndo_change_mtu		= tun_net_change_mtu,
 	.ndo_fix_features	= tun_net_fix_features,
 	.ndo_select_queue	= tun_select_queue,
@@ -838,6 +871,7 @@ static const struct net_device_ops tap_n
 	.ndo_open		= tun_net_open,
 	.ndo_stop		= tun_net_close,
 	.ndo_start_xmit		= tun_net_xmit,
+	.ndo_get_stats64	= tun_net_get_stats64,
 	.ndo_change_mtu		= tun_net_change_mtu,
 	.ndo_fix_features	= tun_net_fix_features,
 	.ndo_set_rx_mode	= tun_net_mclist,
@@ -1422,9 +1456,13 @@ out:
 static void tun_free_netdev(struct net_device *dev)
 {
 	struct tun_struct *tun = netdev_priv(dev);
+	struct tun_link_stats *stats;
 
 	BUG_ON(!(list_empty(&tun->disabled)));
 	tun_flow_uninit(tun);
+	stats = rtnl_dereference(tun->stats);
+	if (stats)
+		kfree_rcu(stats, rcu);
 	security_tun_dev_free_security(tun->security);
 	free_netdev(dev);
 }
@@ -1886,6 +1924,28 @@ unlock:
 	return ret;
 }
 
+static int tun_set_stats(struct tun_struct *tun, void __user *argp)
+{
+	struct tun_link_stats *stats, *old;
+
+	stats = kmalloc(sizeof(struct tun_link_stats), GFP_KERNEL);
+	if (!stats)
+		return -ENOMEM;
+
+	if (copy_from_user(&stats->link_stats, argp,
+			   sizeof(struct rtnl_link_stats64))) {
+		kfree(stats);
+		return -EFAULT;
+	}
+
+	old = rtnl_dereference(tun->stats);
+	rcu_assign_pointer(tun->stats, stats);
+	if (old)
+		kfree_rcu(old, rcu);
+
+	return 0;
+}
+
 static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 			    unsigned long arg, int ifreq_len)
 {
@@ -2108,6 +2168,11 @@ static long __tun_chr_ioctl(struct file
 		tun->info = info;
 		break;
 	}
+
+	case TUNSETSTATS:
+		ret = tun_set_stats(tun, argp);
+		break;
+
 	default:
 		ret = -EINVAL;
 		break;
@@ -2137,6 +2202,7 @@ static long tun_chr_compat_ioctl(struct
 	case TUNGETSNDBUF:
 	case TUNSETSNDBUF:
 	case TUNSETINFO:
+	case TUNSETSTATS:
 	case SIOCGIFHWADDR:
 	case SIOCSIFHWADDR:
 		arg = (unsigned long)compat_ptr(arg);
--- a/include/uapi/linux/if_tun.h	2013-07-24 12:47:37.344206628 -0700
+++ b/include/uapi/linux/if_tun.h	2013-07-24 12:47:37.352206531 -0700
@@ -57,6 +57,7 @@
 #define TUNSETVNETHDRSZ _IOW('T', 216, int)
 #define TUNSETQUEUE  _IOW('T', 217, int)
 #define TUNSETINFO     _IOW('T', 219, struct tun_info)
+#define TUNSETSTATS    _IOW('T', 220, struct rtnl_link_stats64)
 
 /* TUNSETIFF ifr flags */
 #define IFF_TUN		0x0001

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 1/3] tuntap: allow changing ethtool speed (v2)
  2013-07-24 23:11 [PATCH net-next 1/3] tuntap: allow changing ethtool speed (v2) Stephen Hemminger
  2013-07-24 23:13 ` [PATCH net-next 2/3] tuntap: allow overriding ethtool driver info Stephen Hemminger
  2013-07-24 23:15 ` [PATCH net-next 3/3] tuntap: allow overriding link statistics Stephen Hemminger
@ 2013-07-24 23:40 ` Ben Hutchings
  2013-07-25  0:17   ` Stephen Hemminger
  2 siblings, 1 reply; 12+ messages in thread
From: Ben Hutchings @ 2013-07-24 23:40 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Helmut Grohne, David Miller, netdev

On Wed, 2013-07-24 at 16:11 -0700, Stephen Hemminger wrote:
> The data reported by the ethtool interface for tun/tap devices is
> artificial. The default speed is 10Mbit. Virtual interfaces are often
> faster than this nowadays, so the observed bandwidth may exceed the
> available bandwidth. This patch allows an administrator to change the
> available bandwidth as reported by ethtool.
> 
> Signed-off-by: Helmut Grohne <h.grohne@cygnusnetworks.de>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> ---
> v2 - allow setting duplex as well, fix patch formatting

It's logically full duplex so this doesn't make sense to me.

>      ignore other settings in set
[...]

No, please never do that in ethtool_ops::set_settings.  Setting
unsupported values is an error.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 2/3] tuntap: allow overriding ethtool driver info
  2013-07-24 23:13 ` [PATCH net-next 2/3] tuntap: allow overriding ethtool driver info Stephen Hemminger
@ 2013-07-24 23:48   ` Ben Hutchings
  2013-07-25  0:16     ` Stephen Hemminger
  0 siblings, 1 reply; 12+ messages in thread
From: Ben Hutchings @ 2013-07-24 23:48 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Helmut Grohne, David Miller, netdev

On Wed, 2013-07-24 at 16:13 -0700, Stephen Hemminger wrote:
> This patch adds new ioctl to allow setting the ethtool information
> returned by the TUN device. This is useful when using tun device as a surrogate
> for hardware or other software emulation.

I don't like this idea.  Which tools are you trying to fool?  How does
this work when you don't implement any driver-specific behaviour (e.g.
SIOCDEVPRIVATE) they expect?

> If the application does not override the ethtool settings, the
> original hard coded values are used.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
[...]
> --- a/include/uapi/linux/if_tun.h	2013-07-24 11:24:55.543040360 -0700
> +++ b/include/uapi/linux/if_tun.h	2013-07-24 11:35:54.620898504 -0700
> @@ -56,6 +56,7 @@
>  #define TUNGETVNETHDRSZ _IOR('T', 215, int)
>  #define TUNSETVNETHDRSZ _IOW('T', 216, int)
>  #define TUNSETQUEUE  _IOW('T', 217, int)
> +#define TUNSETINFO     _IOW('T', 219, struct tun_info)
>  
>  /* TUNSETIFF ifr flags */
>  #define IFF_TUN		0x0001
> @@ -103,4 +104,11 @@ struct tun_filter {
>  	__u8   addr[0][ETH_ALEN];
>  };
>  
> +/* Subset of ethtool_info */
> +struct tun_info {
> +	char driver[32];
> +	char bus[32];
> +	char version[32];
> +};
> +
>  #endif /* _UAPI__IF_TUN_H */

What about fw_version?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 2/3] tuntap: allow overriding ethtool driver info
  2013-07-24 23:48   ` Ben Hutchings
@ 2013-07-25  0:16     ` Stephen Hemminger
  2013-07-25 21:19       ` Ben Hutchings
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2013-07-25  0:16 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Helmut Grohne, David Miller, netdev

On Thu, 25 Jul 2013 00:48:07 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Wed, 2013-07-24 at 16:13 -0700, Stephen Hemminger wrote:
> > This patch adds new ioctl to allow setting the ethtool information
> > returned by the TUN device. This is useful when using tun device as a surrogate
> > for hardware or other software emulation.
> 
> I don't like this idea.  Which tools are you trying to fool?  How does
> this work when you don't implement any driver-specific behaviour (e.g.
> SIOCDEVPRIVATE) they expect?

We use surrogate interfaces in user mode application and want to
display different information for these than the normal TUN device.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 1/3] tuntap: allow changing ethtool speed (v2)
  2013-07-24 23:40 ` [PATCH net-next 1/3] tuntap: allow changing ethtool speed (v2) Ben Hutchings
@ 2013-07-25  0:17   ` Stephen Hemminger
  2013-07-25  0:29     ` David Miller
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2013-07-25  0:17 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Helmut Grohne, David Miller, netdev

On Thu, 25 Jul 2013 00:40:50 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Wed, 2013-07-24 at 16:11 -0700, Stephen Hemminger wrote:
> > The data reported by the ethtool interface for tun/tap devices is
> > artificial. The default speed is 10Mbit. Virtual interfaces are often
> > faster than this nowadays, so the observed bandwidth may exceed the
> > available bandwidth. This patch allows an administrator to change the
> > available bandwidth as reported by ethtool.
> > 
> > Signed-off-by: Helmut Grohne <h.grohne@cygnusnetworks.de>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > 
> > ---
> > v2 - allow setting duplex as well, fix patch formatting
> 
> It's logically full duplex so this doesn't make sense to me.
> 
> >      ignore other settings in set
> [...]
> 
> No, please never do that in ethtool_ops::set_settings.  Setting
> unsupported values is an error.

Almost every hardware driver out there allows mucking with settings
that it doesn't care about (like transceiver and port). Why be picky now?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 1/3] tuntap: allow changing ethtool speed (v2)
  2013-07-25  0:17   ` Stephen Hemminger
@ 2013-07-25  0:29     ` David Miller
  0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2013-07-25  0:29 UTC (permalink / raw)
  To: stephen; +Cc: bhutchings, h.grohne, netdev

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Wed, 24 Jul 2013 17:17:50 -0700

> On Thu, 25 Jul 2013 00:40:50 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
>> On Wed, 2013-07-24 at 16:11 -0700, Stephen Hemminger wrote:
>> > The data reported by the ethtool interface for tun/tap devices is
>> > artificial. The default speed is 10Mbit. Virtual interfaces are often
>> > faster than this nowadays, so the observed bandwidth may exceed the
>> > available bandwidth. This patch allows an administrator to change the
>> > available bandwidth as reported by ethtool.
>> > 
>> > Signed-off-by: Helmut Grohne <h.grohne@cygnusnetworks.de>
>> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> > 
>> > ---
>> > v2 - allow setting duplex as well, fix patch formatting
>> 
>> It's logically full duplex so this doesn't make sense to me.
>> 
>> >      ignore other settings in set
>> [...]
>> 
>> No, please never do that in ethtool_ops::set_settings.  Setting
>> unsupported values is an error.
> 
> Almost every hardware driver out there allows mucking with settings
> that it doesn't care about (like transceiver and port). Why be picky now?

Those are bugs to be fixed, rather than things to emulate.

And honestly this is an unrelated thing to mention given the point Ben
is trying to make.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 2/3] tuntap: allow overriding ethtool driver info
  2013-07-25  0:16     ` Stephen Hemminger
@ 2013-07-25 21:19       ` Ben Hutchings
  2013-07-25 21:32         ` Stephen Hemminger
  0 siblings, 1 reply; 12+ messages in thread
From: Ben Hutchings @ 2013-07-25 21:19 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Helmut Grohne, David Miller, netdev

On Wed, 2013-07-24 at 17:16 -0700, Stephen Hemminger wrote:
> On Thu, 25 Jul 2013 00:48:07 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > On Wed, 2013-07-24 at 16:13 -0700, Stephen Hemminger wrote:
> > > This patch adds new ioctl to allow setting the ethtool information
> > > returned by the TUN device. This is useful when using tun device as a surrogate
> > > for hardware or other software emulation.
> > 
> > I don't like this idea.  Which tools are you trying to fool?  How does
> > this work when you don't implement any driver-specific behaviour (e.g.
> > SIOCDEVPRIVATE) they expect?
> 
> We use surrogate interfaces in user mode application and want to
> display different information for these than the normal TUN device.

What is the problem that can't be solved purely in userland?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 2/3] tuntap: allow overriding ethtool driver info
  2013-07-25 21:19       ` Ben Hutchings
@ 2013-07-25 21:32         ` Stephen Hemminger
  2013-07-25 21:36           ` Ben Hutchings
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2013-07-25 21:32 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Helmut Grohne, David Miller, netdev

On Thu, 25 Jul 2013 22:19:17 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Wed, 2013-07-24 at 17:16 -0700, Stephen Hemminger wrote:
> > On Thu, 25 Jul 2013 00:48:07 +0100
> > Ben Hutchings <bhutchings@solarflare.com> wrote:
> > 
> > > On Wed, 2013-07-24 at 16:13 -0700, Stephen Hemminger wrote:
> > > > This patch adds new ioctl to allow setting the ethtool information
> > > > returned by the TUN device. This is useful when using tun device as a surrogate
> > > > for hardware or other software emulation.
> > > 
> > > I don't like this idea.  Which tools are you trying to fool?  How does
> > > this work when you don't implement any driver-specific behaviour (e.g.
> > > SIOCDEVPRIVATE) they expect?
> > 
> > We use surrogate interfaces in user mode application and want to
> > display different information for these than the normal TUN device.
> 
> What is the problem that can't be solved purely in userland?
> 
> Ben.
> 

There applications (like SNMP) that use ethtool info.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 2/3] tuntap: allow overriding ethtool driver info
  2013-07-25 21:32         ` Stephen Hemminger
@ 2013-07-25 21:36           ` Ben Hutchings
  2013-07-25 21:56             ` Stephen Hemminger
  0 siblings, 1 reply; 12+ messages in thread
From: Ben Hutchings @ 2013-07-25 21:36 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Helmut Grohne, David Miller, netdev

On Thu, 2013-07-25 at 14:32 -0700, Stephen Hemminger wrote:
> On Thu, 25 Jul 2013 22:19:17 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > On Wed, 2013-07-24 at 17:16 -0700, Stephen Hemminger wrote:
> > > On Thu, 25 Jul 2013 00:48:07 +0100
> > > Ben Hutchings <bhutchings@solarflare.com> wrote:
> > > 
> > > > On Wed, 2013-07-24 at 16:13 -0700, Stephen Hemminger wrote:
> > > > > This patch adds new ioctl to allow setting the ethtool information
> > > > > returned by the TUN device. This is useful when using tun device as a surrogate
> > > > > for hardware or other software emulation.
> > > > 
> > > > I don't like this idea.  Which tools are you trying to fool?  How does
> > > > this work when you don't implement any driver-specific behaviour (e.g.
> > > > SIOCDEVPRIVATE) they expect?
> > > 
> > > We use surrogate interfaces in user mode application and want to
> > > display different information for these than the normal TUN device.
> > 
> > What is the problem that can't be solved purely in userland?
> > 
> > Ben.
> > 
> 
> There applications (like SNMP) that use ethtool info.

Huh, I never expected they would record the driver and version.  So you
want them to report the userland software name and version?  I can sort
of see how this might be useful as part of a test harness, but I would
hate to see this get used in production.  Users ought to be able to
trust that 'ethtool -i' shows them the kernel driver information.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 2/3] tuntap: allow overriding ethtool driver info
  2013-07-25 21:36           ` Ben Hutchings
@ 2013-07-25 21:56             ` Stephen Hemminger
  0 siblings, 0 replies; 12+ messages in thread
From: Stephen Hemminger @ 2013-07-25 21:56 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Helmut Grohne, David Miller, netdev

On Thu, 25 Jul 2013 22:36:22 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Thu, 2013-07-25 at 14:32 -0700, Stephen Hemminger wrote:
> > On Thu, 25 Jul 2013 22:19:17 +0100
> > Ben Hutchings <bhutchings@solarflare.com> wrote:
> > 
> > > On Wed, 2013-07-24 at 17:16 -0700, Stephen Hemminger wrote:
> > > > On Thu, 25 Jul 2013 00:48:07 +0100
> > > > Ben Hutchings <bhutchings@solarflare.com> wrote:
> > > > 
> > > > > On Wed, 2013-07-24 at 16:13 -0700, Stephen Hemminger wrote:
> > > > > > This patch adds new ioctl to allow setting the ethtool information
> > > > > > returned by the TUN device. This is useful when using tun device as a surrogate
> > > > > > for hardware or other software emulation.
> > > > > 
> > > > > I don't like this idea.  Which tools are you trying to fool?  How does
> > > > > this work when you don't implement any driver-specific behaviour (e.g.
> > > > > SIOCDEVPRIVATE) they expect?
> > > > 
> > > > We use surrogate interfaces in user mode application and want to
> > > > display different information for these than the normal TUN device.
> > > 
> > > What is the problem that can't be solved purely in userland?
> > > 
> > > Ben.
> > > 
> > 
> > There applications (like SNMP) that use ethtool info.
> 
> Huh, I never expected they would record the driver and version.  So you
> want them to report the userland software name and version?  I can sort
> of see how this might be useful as part of a test harness, but I would
> hate to see this get used in production.  Users ought to be able to
> trust that 'ethtool -i' shows them the kernel driver information.
> 
> Ben.
> 

It is more the bus info.
The user mode driver knows the PCI address of the device it is controlling.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-07-25 21:56 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-24 23:11 [PATCH net-next 1/3] tuntap: allow changing ethtool speed (v2) Stephen Hemminger
2013-07-24 23:13 ` [PATCH net-next 2/3] tuntap: allow overriding ethtool driver info Stephen Hemminger
2013-07-24 23:48   ` Ben Hutchings
2013-07-25  0:16     ` Stephen Hemminger
2013-07-25 21:19       ` Ben Hutchings
2013-07-25 21:32         ` Stephen Hemminger
2013-07-25 21:36           ` Ben Hutchings
2013-07-25 21:56             ` Stephen Hemminger
2013-07-24 23:15 ` [PATCH net-next 3/3] tuntap: allow overriding link statistics Stephen Hemminger
2013-07-24 23:40 ` [PATCH net-next 1/3] tuntap: allow changing ethtool speed (v2) Ben Hutchings
2013-07-25  0:17   ` Stephen Hemminger
2013-07-25  0:29     ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).