[PATCH] xen-netfront: report link speed to ethtool

[PATCH] xen-netfront: report link speed to ethtool

[Olaf Hering]

Add .get_settings function, return fake data so that ethtool can get
enough information. For some application like VCS, this is useful,
otherwise some of application logic will get panic.
The reported data refers to VMWare vmxnet.

Signed-off-by: Xin Wei Hu <xwhu@suse.com>
Signed-off-by: Chunyan Liu <cyliu@suse.com>
Signed-off-by: Olaf Hering <olaf@aepfle.de>

---
 drivers/net/xen-netfront.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Index: linux-3.2-rc2/drivers/net/xen-netfront.c
===================================================================
--- linux-3.2-rc2.orig/drivers/net/xen-netfront.c
+++ linux-3.2-rc2/drivers/net/xen-netfront.c
@@ -1727,6 +1727,17 @@ static void netback_changed(struct xenbu
 	}
 }
 
+static int xennet_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
+{
+	ecmd->supported = SUPPORTED_1000baseT_Full | SUPPORTED_TP;
+	ecmd->advertising = ADVERTISED_TP;
+	ecmd->port = PORT_TP;
+	ecmd->transceiver = XCVR_INTERNAL;
+	ecmd->speed = SPEED_1000;
+	ecmd->duplex = DUPLEX_FULL;
+	return 0;
+}
+
 static const struct xennet_stat {
 	char name[ETH_GSTRING_LEN];
 	u16 offset;
@@ -1774,6 +1785,7 @@ static const struct ethtool_ops xennet_e
 {
 	.get_link = ethtool_op_get_link,
 
+	.get_settings = xennet_get_settings,
 	.get_sset_count = xennet_get_sset_count,
 	.get_ethtool_stats = xennet_get_ethtool_stats,
 	.get_strings = xennet_get_strings,

Re: [PATCH] xen-netfront: report link speed to ethtool

[Ben Hutchings]

On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
> Add .get_settings function, return fake data so that ethtool can get
> enough information. For some application like VCS, this is useful,
> otherwise some of application logic will get panic.
> The reported data refers to VMWare vmxnet.
> 
> Signed-off-by: Xin Wei Hu <xwhu@suse.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

NAK, we should not just make things up.

Ben.

> ---
>  drivers/net/xen-netfront.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> Index: linux-3.2-rc2/drivers/net/xen-netfront.c
> ===================================================================
> --- linux-3.2-rc2.orig/drivers/net/xen-netfront.c
> +++ linux-3.2-rc2/drivers/net/xen-netfront.c
> @@ -1727,6 +1727,17 @@ static void netback_changed(struct xenbu
>  	}
>  }
>  
> +static int xennet_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
> +{
> +	ecmd->supported = SUPPORTED_1000baseT_Full | SUPPORTED_TP;
> +	ecmd->advertising = ADVERTISED_TP;
> +	ecmd->port = PORT_TP;
> +	ecmd->transceiver = XCVR_INTERNAL;
> +	ecmd->speed = SPEED_1000;
> +	ecmd->duplex = DUPLEX_FULL;
> +	return 0;
> +}
> +
>  static const struct xennet_stat {
>  	char name[ETH_GSTRING_LEN];
>  	u16 offset;
> @@ -1774,6 +1785,7 @@ static const struct ethtool_ops xennet_e
>  {
>  	.get_link = ethtool_op_get_link,
>  
> +	.get_settings = xennet_get_settings,
>  	.get_sset_count = xennet_get_sset_count,
>  	.get_ethtool_stats = xennet_get_ethtool_stats,
>  	.get_strings = xennet_get_strings,
> --
> 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, 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: [PATCH] xen-netfront: report link speed to ethtool

[Olaf Hering]

On Fri, Nov 18, Ben Hutchings wrote:

> On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
> > The reported data refers to VMWare vmxnet.
> NAK, we should not just make things up.

So how about removing veth_get_settings, vmxnet3_get_settings,
tun_get_settings and other functions that escaped my grep?

Olaf

Re: [PATCH] xen-netfront: report link speed to ethtool

[Rick Jones]

On 11/18/2011 09:46 AM, Ben Hutchings wrote:
> On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
>> Add .get_settings function, return fake data so that ethtool can get
>> enough information. For some application like VCS, this is useful,
>> otherwise some of application logic will get panic.
>> The reported data refers to VMWare vmxnet.
>>
>> Signed-off-by: Xin Wei Hu<xwhu@suse.com>
>> Signed-off-by: Chunyan Liu<cyliu@suse.com>
>> Signed-off-by: Olaf Hering<olaf@aepfle.de>
>
> NAK, we should not just make things up.

Which raises an interesting question for a virtual interface that isn't 
pretending to be a specific NIC type. What should the reported speed be? 
  Is it a 10/100 NIC?  A 1 or 10 GbE NIC? 3.14 GbE?  For other emulated 
interfaces, it rather falls-out from the emulation.  We can say that the 
driver may not make stuff up, but it would seem what is running in the 
host/hypervisor/dom0/whatever will have to.  It could I suppose, decide 
based on the physical NIC to which it is attached, so long as folks 
using the virtual NIC don't expect its attributes to be the same from 
system to system.

rick

Re: [PATCH] xen-netfront: report link speed to ethtool

[Jeremy Fitzhardinge]

On 11/18/2011 10:44 AM, Rick Jones wrote:
>  It could I suppose, decide 
> based on the physical NIC to which it is attached, so long as folks 
> using the virtual NIC don't expect its attributes to be the same from 
> system to system.

And assuming there's a physical NIC at all.

    J

use a special value of -2 for virtual devices to report indeterminate speed?

[Rick Jones]

On 11/18/2011 10:46 AM, Jeremy Fitzhardinge wrote:
> On 11/18/2011 10:44 AM, Rick Jones wrote:
>>   It could I suppose, decide
>> based on the physical NIC to which it is attached, so long as folks
>> using the virtual NIC don't expect its attributes to be the same from
>> system to system.
>
> And assuming there's a physical NIC at all.

It sounds like we need a way to specify "Indeterminate" for link speed? 
  Or some verbiage to that effect. Right now 0 and -1 cause ethtool to 
report "Unknown!"

         if (speed == 0 || speed == (u16)(-1) || speed == (u32)(-1))
                 fprintf(stdout, "Unknown!\n");
         else
                 fprintf(stdout, "%uMb/s\n", speed);


How about -2 for the u32 cast value of speed returning "Indeterminate" 
or something like that?  Not in "proper" patch format:

	if (speed == 0 || speed == (u16)(-1) || speed == (u32)(-1))
		fprintf(stdout, "Unknown!\n");
	else if (speed == (u32)(-2))
		fprintf(stdout, "Indeterminate.");
	else
		fprintf(stdout, "%uMb/s\n", speed);

Signed-off-by: Rick Jones <rick.jones2@hp.com>	

rick jones

Re: [PATCH] xen-netfront: report link speed to ethtool

[Ben Hutchings]

On Fri, 2011-11-18 at 19:43 +0100, Olaf Hering wrote:
> On Fri, Nov 18, Ben Hutchings wrote:
> 
> > On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
> > > The reported data refers to VMWare vmxnet.
> > NAK, we should not just make things up.
> 
> So how about removing veth_get_settings, vmxnet3_get_settings,
> tun_get_settings and other functions that escaped my grep?

If they can't provide meaningful information then maybe they should be
removed.  However, that could result in a regression for existing
working configurations.  (This isn't the same as the case you're trying
to fix, since those applications have never worked with xen-netfront or
many other drivers that don't implement get_settings.)

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: [PATCH] xen-netfront: report link speed to ethtool

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 18 Nov 2011 17:46:34 +0000

> On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
>> Add .get_settings function, return fake data so that ethtool can get
>> enough information. For some application like VCS, this is useful,
>> otherwise some of application logic will get panic.
>> The reported data refers to VMWare vmxnet.
>> 
>> Signed-off-by: Xin Wei Hu <xwhu@suse.com>
>> Signed-off-by: Chunyan Liu <cyliu@suse.com>
>> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> NAK, we should not just make things up.

Agreed, if you cannot determine the values with certainty do not
implement this method.

Fix the tools which cannot function without this information.

Re: use a special value of -2 for virtual devices to report indeterminate speed?

[Ben Hutchings]

On Fri, 2011-11-18 at 10:58 -0800, Rick Jones wrote:
> On 11/18/2011 10:46 AM, Jeremy Fitzhardinge wrote:
> > On 11/18/2011 10:44 AM, Rick Jones wrote:
> >>   It could I suppose, decide
> >> based on the physical NIC to which it is attached, so long as folks
> >> using the virtual NIC don't expect its attributes to be the same from
> >> system to system.
> >
> > And assuming there's a physical NIC at all.
> 
> It sounds like we need a way to specify "Indeterminate" for link speed? 
>   Or some verbiage to that effect. Right now 0 and -1 cause ethtool to 
> report "Unknown!"
> 
>          if (speed == 0 || speed == (u16)(-1) || speed == (u32)(-1))
>                  fprintf(stdout, "Unknown!\n");
>          else
>                  fprintf(stdout, "%uMb/s\n", speed);
> 
> 
> How about -2 for the u32 cast value of speed returning "Indeterminate" 
> or something like that?  Not in "proper" patch format:
> 
> 	if (speed == 0 || speed == (u16)(-1) || speed == (u32)(-1))
> 		fprintf(stdout, "Unknown!\n");
> 	else if (speed == (u32)(-2))
> 		fprintf(stdout, "Indeterminate.");
> 	else
> 		fprintf(stdout, "%uMb/s\n", speed);

I'm open to something like this, but the problem with assigning new
magic numbers is that older versions of ethtool won't know to report
them as special.

We should also consider stacked drivers like bonding (and presumably
team) that expect real numbers when the link is up.

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: [PATCH] xen-netfront: report link speed to ethtool

[Olaf Hering]

On Fri, Nov 18, Ben Hutchings wrote:

> On Fri, 2011-11-18 at 19:43 +0100, Olaf Hering wrote:
> > On Fri, Nov 18, Ben Hutchings wrote:
> > 
> > > On Fri, 2011-11-18 at 17:48 +0100, Olaf Hering wrote:
> > > > The reported data refers to VMWare vmxnet.
> > > NAK, we should not just make things up.
> > 
> > So how about removing veth_get_settings, vmxnet3_get_settings,
> > tun_get_settings and other functions that escaped my grep?
> 
> If they can't provide meaningful information then maybe they should be
> removed.  However, that could result in a regression for existing
> working configurations.  (This isn't the same as the case you're trying
> to fix, since those applications have never worked with xen-netfront or
> many other drivers that don't implement get_settings.)

That may be.
How about a new generic ethtool_op_get_settings_veth which returns fake
values for all relevant drivers (virtio, xen-netfront, and the ones
listed above)?

Olaf