Patchwork [net-next,1/3] tuntap: allow changing ethtool speed (v2)

login
register
mail settings
Submitter stephen hemminger
Date July 24, 2013, 11:11 p.m.
Message ID <20130724161156.4a8cf02b@nehalam.linuxnetplumber.net>
Download mbox | patch
Permalink /patch/261550/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

stephen hemminger - July 24, 2013, 11:11 p.m.
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(-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - July 24, 2013, 11:40 p.m.
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.
stephen hemminger - July 25, 2013, 12:17 a.m.
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?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - July 25, 2013, 12:29 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- 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,