Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations

Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations

[Helmut Grohne]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]


On 16.07.2013, at 23:41, Max Krasnyansky <maxk@qti.qualcomm.com> wrote:
> The patch looks fine to me (must admit that I only glanced at it). Please send it to the netdev
> mailing list netdev@vger.kernel.org, if you have't already done so.
> CC David Miller <davem@davemloft.net> and me.

Doing that. Now summarizing the issue for the new recipients:

Problem:

When querying a tap device for its speed using ethtool the tun driver reports a speed
of SPEED_10. This number is hard coded into the driver. Nowadays virtual network devices
can go way faster than that. When doing automatic bandwidth monitoring based on the
speed reported by ethtool, tap devices tend to come up as false positives. Arguably the
hard coded speed is wrong.

Proposed solution:

To that end I propose supporting the ETHTOOL_SSET command in addition to the already
supported ETHTOOL_GSET. It would deny setting any setting except for the bandwidth
where it would allow arbitrary values. You can find this patch attached.

With this patch an administrator can increase the reported speed for tap devices and
keep using automatic detection of interface speeds.

Workarounds:

Using ethtool a detection utility can determine the driver for an interface. If the
driver matches the string "tun", the reported speed should not be used.

Helmut


[-- Attachment #2: 0001-tuntap-allow-changing-ethtool-speed.patch --]
[-- Type: application/octet-stream, Size: 2816 bytes --]

From 9e0d2928ebed1d74ec92d131dcdc7511dde867ac Mon Sep 17 00:00:00 2001
From: Helmut Grohne <h.grohne@cygnusnetworks.de>
Date: Fri, 17 May 2013 14:03:47 +0200
Subject: [PATCH] tuntap: allow changing ethtool speed

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>
---
 drivers/net/tun.c |   25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index f042b03..2555a11 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -187,6 +187,7 @@ struct tun_struct {
 	struct list_head disabled;
 	void *security;
 	u32 flow_count;
+	u32 advertised_speed;
 };
 
 static inline u32 tun_hashfn(u32 rxhash)
@@ -1404,6 +1405,7 @@ static void tun_setup(struct net_device *dev)
 
 	tun->owner = INVALID_UID;
 	tun->group = INVALID_GID;
+	tun->advertised_speed = SPEED_10;
 
 	dev->ethtool_ops = &tun_ethtool_ops;
 	dev->destructor = tun_free_netdev;
@@ -2197,9 +2199,11 @@ 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);
+	ethtool_cmd_speed_set(cmd, tun->advertised_speed);
 	cmd->duplex		= DUPLEX_FULL;
 	cmd->port		= PORT_TP;
 	cmd->phy_address	= 0;
@@ -2210,6 +2214,24 @@ static int tun_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 	return 0;
 }
 
+static int tun_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+
+	if(cmd->supported != 0 ||
+			cmd->advertising != 0 ||
+			cmd->duplex != DUPLEX_FULL ||
+			cmd->port != PORT_TP ||
+			cmd->phy_address != 0 ||
+			cmd->transceiver != XCVR_INTERNAL ||
+			cmd->autoneg != AUTONEG_DISABLE ||
+			cmd->maxtxpkt != 0 ||
+			cmd->maxrxpkt != 0)
+		return -EOPNOTSUPP;
+	tun->advertised_speed = ethtool_cmd_speed(cmd);
+	return 0;
+}
+
 static void tun_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
 {
 	struct tun_struct *tun = netdev_priv(dev);
@@ -2247,6 +2269,7 @@ static void tun_set_msglevel(struct net_device *dev, u32 value)
 
 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,
-- 
1.7.10.4

Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations

[Rick Jones]

On 07/23/2013 06:32 AM, Helmut Grohne wrote:
>
> On 16.07.2013, at 23:41, Max Krasnyansky <maxk@qti.qualcomm.com> wrote:
>> The patch looks fine to me (must admit that I only glanced at it). Please send it to the netdev
>> mailing list netdev@vger.kernel.org, if you have't already done so.
>> CC David Miller <davem@davemloft.net> and me.
>
> Doing that. Now summarizing the issue for the new recipients:
>
> Problem:
>
> When querying a tap device for its speed using ethtool the tun driver reports a speed
> of SPEED_10. This number is hard coded into the driver. Nowadays virtual network devices
> can go way faster than that. When doing automatic bandwidth monitoring based on the
> speed reported by ethtool, tap devices tend to come up as false positives. Arguably the
> hard coded speed is wrong.
>
> Proposed solution:
>
> To that end I propose supporting the ETHTOOL_SSET command in addition to the already
> supported ETHTOOL_GSET. It would deny setting any setting except for the bandwidth
> where it would allow arbitrary values. You can find this patch attached.
>
> With this patch an administrator can increase the reported speed for tap devices and
> keep using automatic detection of interface speeds.
>
> Workarounds:
>
> Using ethtool a detection utility can determine the driver for an interface. If the
> driver matches the string "tun", the reported speed should not be used.
>
> Helmut
>

I like the idea of being able to set the reported speed for a tun/tap 
interface.

rick jones

Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations

[Stephen Hemminger]

On Tue, 23 Jul 2013 08:49:01 -0700
Rick Jones <rick.jones2@hp.com> wrote:

> On 07/23/2013 06:32 AM, Helmut Grohne wrote:
> >
> > On 16.07.2013, at 23:41, Max Krasnyansky <maxk@qti.qualcomm.com> wrote:
> >> The patch looks fine to me (must admit that I only glanced at it). Please send it to the netdev
> >> mailing list netdev@vger.kernel.org, if you have't already done so.
> >> CC David Miller <davem@davemloft.net> and me.
> >
> > Doing that. Now summarizing the issue for the new recipients:
> >
> > Problem:
> >
> > When querying a tap device for its speed using ethtool the tun driver reports a speed
> > of SPEED_10. This number is hard coded into the driver. Nowadays virtual network devices
> > can go way faster than that. When doing automatic bandwidth monitoring based on the
> > speed reported by ethtool, tap devices tend to come up as false positives. Arguably the
> > hard coded speed is wrong.
> >
> > Proposed solution:
> >
> > To that end I propose supporting the ETHTOOL_SSET command in addition to the already
> > supported ETHTOOL_GSET. It would deny setting any setting except for the bandwidth
> > where it would allow arbitrary values. You can find this patch attached.
> >
> > With this patch an administrator can increase the reported speed for tap devices and
> > keep using automatic detection of interface speeds.
> >
> > Workarounds:
> >
> > Using ethtool a detection utility can determine the driver for an interface. If the
> > driver matches the string "tun", the reported speed should not be used.
> >
> > Helmut
> >
> 
> I like the idea of being able to set the reported speed for a tun/tap 
> interface.
> 
> rick jones

I have hack patch to allow overwriting speed, info, and statistics that is part
of how we use TUN as dummy devices. How far do you think makes sense?

Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations

[Helmut Grohne]

On 23.07.2013, at 17:56, Stephen Hemminger <stephen@networkplumber.org> wrote:
> I have hack patch to allow overwriting speed, info, and statistics that is part
> of how we use TUN as dummy devices. How far do you think makes sense?

That could prove useful for creating test environments for tools, that use ethtool.
If you have a better patch, then go for it. The reason for why I did not include
other settings was that I don't need them. I'm open to other solutions as well, as
long as they either make the speed modifiable or something faster than SPEED_10.

Helmut

[RFC 1/2] tun: allow overrriding ethtool info

[Stephen Hemminger]

This patch adds new ioctl to allow overrriding 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.

This version is compile tested only, included to show how API works.

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-23 09:18:01.936046624 -0700
+++ b/drivers/net/tun.c	2013-07-23 09:52:37.054143993 -0700
@@ -187,6 +187,8 @@ struct tun_struct {
 	struct list_head disabled;
 	void *security;
 	u32 flow_count;
+
+	struct tun_info info;
 };
 
 static inline u32 tun_hashfn(u32 rxhash)
@@ -870,9 +872,12 @@ 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));
+
 	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;
@@ -887,6 +892,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;
@@ -1426,6 +1433,9 @@ static void tun_setup(struct net_device
 
 	tun->owner = INVALID_UID;
 	tun->group = INVALID_GID;
+	tun->info.speed = SPEED_10;
+	tun->info.duplex = DUPLEX_FULL;
+	tun->info.port = PORT_TP;
 
 	dev->ethtool_ops = &tun_ethtool_ops;
 	dev->destructor = tun_free_netdev;
@@ -2088,6 +2098,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(argp, &info, sizeof(info)))
+			ret = -EFAULT;
+
+		tun->info = info;
+		break;
+	}
+
 	default:
 		ret = -EINVAL;
 		break;
@@ -2229,11 +2248,13 @@ 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;
-	cmd->port		= PORT_TP;
+	ethtool_cmd_speed_set(cmd, tun->info.speed);
+	cmd->duplex		= tun->info.duplex;
+	cmd->port		= tun->info.port;
 	cmd->phy_address	= 0;
 	cmd->transceiver	= XCVR_INTERNAL;
 	cmd->autoneg		= AUTONEG_DISABLE;
@@ -2242,21 +2263,24 @@ static int tun_get_settings(struct net_d
 	return 0;
 }
 
+static int tun_set_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+
+	tun->info.speed  = ethtool_cmd_speed(ecmd);
+	tun->info.duplex = ecmd->duplex;
+	tun->info.port   = ecmd->port;
+
+	return 0;
+}
+
 static void tun_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
 {
 	struct tun_struct *tun = netdev_priv(dev);
 
-	strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
+	strlcpy(info->driver, tun->info.driver, 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->bus_info, tun->info.bus, sizeof(info->bus_info));
 }
 
 static u32 tun_get_msglevel(struct net_device *dev)
@@ -2279,6 +2303,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,
--- a/include/uapi/linux/if_tun.h	2013-07-23 09:18:01.936046624 -0700
+++ b/include/uapi/linux/if_tun.h	2013-07-23 09:52:26.870271418 -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,17 @@ struct tun_filter {
 	__u8   addr[0][ETH_ALEN];
 };
 
+/*
+ * Ethtool info
+ * This is used to allow spoofing the speed/duplex and driver information
+ */
+struct tun_info {
+	__u32 speed;
+	__u8  duplex;
+	__u8  port;
+
+	char driver[32];
+	char bus[32];
+};
+
 #endif /* _UAPI__IF_TUN_H */

[RFC 2/2] tun: allow overriding statistics

[Stephen Hemminger]

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

The application perodically uses ioctl(TUNSETSTTS) to periodically update statistics.
Exchange and RCU is used to make this operation atomic.

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

This version compile tested only, to show how API works.

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

--- a/drivers/net/tun.c	2013-07-23 10:17:04.335800821 -0700
+++ b/drivers/net/tun.c	2013-07-23 10:22:10.903948347 -0700
@@ -189,6 +189,10 @@ struct tun_struct {
 	u32 flow_count;
 
 	struct tun_info info;
+	struct tun_stats __rcu {
+		struct rtnl_link_stats64 link_stats;
+		struct rcu_head rcu;
+	} *stats;
 };
 
 static inline u32 tun_hashfn(u32 rxhash)
@@ -802,6 +806,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_stats __rcu *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)
 {
@@ -824,6 +854,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,
@@ -837,6 +868,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,
@@ -1420,9 +1452,13 @@ out:
 static void tun_free_netdev(struct net_device *dev)
 {
 	struct tun_struct *tun = netdev_priv(dev);
+	struct tun_stats *stats;
 
 	BUG_ON(!(list_empty(&tun->disabled)));
 	tun_flow_uninit(tun);
+	stats = xchg(&tun->stats, NULL);
+	if (stats)
+		kfree_rcu(stats, rcu);
 	security_tun_dev_free_security(tun->security);
 	free_netdev(dev);
 }
@@ -1885,6 +1921,28 @@ unlock:
 	return ret;
 }
 
+static int tun_set_stats(struct tun_struct *tun, struct ifreq *ifr)
+{
+	struct tun_stats *stats, *old;
+
+	stats = kmalloc(sizeof(struct tun_stats), GFP_KERNEL);
+	if (!stats)
+		return -ENOMEM;
+
+	if (copy_from_user(&stats->link_stats,
+			   ifr->ifr_ifru.ifru_data,
+			   sizeof(struct rtnl_link_stats64))) {
+		kfree(stats);
+		return -EFAULT;
+	}
+
+	old = xchg(&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)
 {
@@ -2107,6 +2165,10 @@ static long __tun_chr_ioctl(struct file
 		break;
 	}
 
+	case TUNSETSTATS:
+		ret = tun_set_stats(tun, &ifr);
+		break;
+
 	default:
 		ret = -EINVAL;
 		break;
--- a/include/uapi/linux/if_tun.h	2013-07-23 10:17:04.335800821 -0700
+++ b/include/uapi/linux/if_tun.h	2013-07-23 10:19:09.358230639 -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

Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations

[Ben Hutchings]

On Tue, 2013-07-23 at 15:32 +0200, Helmut Grohne wrote:
> On 16.07.2013, at 23:41, Max Krasnyansky <maxk@qti.qualcomm.com> wrote:
> > The patch looks fine to me (must admit that I only glanced at it). Please send it to the netdev
> > mailing list netdev@vger.kernel.org, if you have't already done so.
> > CC David Miller <davem@davemloft.net> and me.
> 
> Doing that. Now summarizing the issue for the new recipients:
> 
> Problem:
> 
> When querying a tap device for its speed using ethtool the tun driver reports a speed
> of SPEED_10. This number is hard coded into the driver. Nowadays virtual network devices
> can go way faster than that. When doing automatic bandwidth monitoring based on the
> speed reported by ethtool, tap devices tend to come up as false positives. Arguably the
> hard coded speed is wrong.
> 
> Proposed solution:
> 
> To that end I propose supporting the ETHTOOL_SSET command in addition to the already
> supported ETHTOOL_GSET. It would deny setting any setting except for the bandwidth
> where it would allow arbitrary values. You can find this patch attached.

Yes, I think this makes some sense.

[...]
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
[...]
> @@ -2197,9 +2199,11 @@ 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);
> +       ethtool_cmd_speed_set(cmd, tun->advertised_speed);
>         cmd->duplex             = DUPLEX_FULL;
>         cmd->port               = PORT_TP;
>
>         cmd->phy_address        = 0;
> @@ -2210,6 +2214,24 @@ static int tun_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
>         return 0;
>  }
>  
> +static int tun_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> +       struct tun_struct *tun = netdev_priv(dev);
> +
> +       if(cmd->supported != 0 ||
> +                       cmd->advertising != 0 ||
> +                       cmd->duplex != DUPLEX_FULL ||
> +                       cmd->port != PORT_TP ||
> +                       cmd->phy_address != 0 ||
> +                       cmd->transceiver != XCVR_INTERNAL ||
> +                       cmd->autoneg != AUTONEG_DISABLE ||
> +                       cmd->maxtxpkt != 0 ||
> +                       cmd->maxrxpkt != 0)

This is formatted wrongly - needs a space after 'if' and all the
continuation lines should be lined up under 'cmd->supported'.

> +               return -EOPNOTSUPP;

EINVAL

> +       tun->advertised_speed = ethtool_cmd_speed(cmd);
> +       return 0;
> +}
> +
>  static void tun_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
>  {
>         struct tun_struct *tun = netdev_priv(dev);
[...]

I notice the port type is reported as PORT_TP.  Perhaps that should also
be changed to PORT_NONE (in a separate patch)?

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.

Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations

[Helmut Grohne]

Thanks for your review! Given the v2 patch Stephen Hemminger posted, I
think that his series is where development should continue.

On 23.07.2013, at 21:04, Ben Hutchings <bhutchings@solarflare.com> wrote:
> This is formatted wrongly - needs a space after 'if' and all the
> continuation lines should be lined up under 'cmd->supported'.

I am sorry for not having run checkpatch and will try to remember next time.

>> +               return -EOPNOTSUPP;
> 
> EINVAL

Makes sense, once you are pointed to it.

> I notice the port type is reported as PORT_TP.  Perhaps that should also
> be changed to PORT_NONE (in a separate patch)?

This appears not yet addressed in the v2 patch yet.

I am not sure how to go on here. Clearly the v2 patch set, is an improvement
over my initial version. The additional capabilities of changing the reported
driver and similar aspects appear controversial though. Would it make sense
to split this into two patch sets to be handled independently?

Helmut

Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations

[Ben Hutchings]

On Tue, 2013-07-30 at 08:20 +0200, Helmut Grohne wrote:
> Thanks for your review! Given the v2 patch Stephen Hemminger posted, I
> think that his series is where development should continue.
> 
> On 23.07.2013, at 21:04, Ben Hutchings <bhutchings@solarflare.com> wrote:
> > This is formatted wrongly - needs a space after 'if' and all the
> > continuation lines should be lined up under 'cmd->supported'.
> 
> I am sorry for not having run checkpatch and will try to remember next time.
> 
> >> +               return -EOPNOTSUPP;
> > 
> > EINVAL
> 
> Makes sense, once you are pointed to it.
> 
> > I notice the port type is reported as PORT_TP.  Perhaps that should also
> > be changed to PORT_NONE (in a separate patch)?
> 
> This appears not yet addressed in the v2 patch yet.
> 
> I am not sure how to go on here. Clearly the v2 patch set, is an improvement
> over my initial version. The additional capabilities of changing the reported
> driver and similar aspects appear controversial though. Would it make sense
> to split this into two patch sets to be handled independently?

Yes, I think that would make sense.

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.