[PATCH] usbnet: Do not implement ethtool get_link() if link state is unknown

[PATCH] usbnet: Do not implement ethtool get_link() if link state is unknown

[Ben Hutchings]

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

usbnet implements the ethtool get_link() operation, using the first
available method out of (1) driver_info->check_connect()
(2) mii_link_ok() (3) netif_carrier_ok().  Some drivers do not support
any of these methods and usbnet will always report the link as up,
confusing tools such as Network Manager.

Since we cannot tell in advance whether a driver will implement method
(2) or (3), add a driver_info flag to indicate this.  Define the
get_link() operation in usbnet only if the driver sets this flag or
implements check_connect().

Compile-tested only.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/net/usb/asix.c      |   12 ++++++------
 drivers/net/usb/cdc_ether.c |    2 +-
 drivers/net/usb/dm9601.c    |    2 +-
 drivers/net/usb/mcs7830.c   |    4 ++--
 drivers/net/usb/smsc95xx.c  |    2 +-
 drivers/net/usb/usbnet.c    |   17 +++++++++++++++--
 include/linux/usb/usbnet.h  |    2 ++
 7 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/net/usb/asix.c b/drivers/net/usb/asix.c
index 6ce7f77..34483c0 100644
--- a/drivers/net/usb/asix.c
+++ b/drivers/net/usb/asix.c
@@ -1327,7 +1327,7 @@ static const struct driver_info ax8817x_info = {
 	.status = asix_status,
 	.link_reset = ax88172_link_reset,
 	.reset = ax88172_link_reset,
-	.flags =  FLAG_ETHER,
+	.flags =  FLAG_ETHER | FLAG_GET_LINK,
 	.data = 0x00130103,
 };
 
@@ -1337,7 +1337,7 @@ static const struct driver_info dlink_dub_e100_info = {
 	.status = asix_status,
 	.link_reset = ax88172_link_reset,
 	.reset = ax88172_link_reset,
-	.flags =  FLAG_ETHER,
+	.flags =  FLAG_ETHER | FLAG_GET_LINK,
 	.data = 0x009f9d9f,
 };
 
@@ -1347,7 +1347,7 @@ static const struct driver_info netgear_fa120_info = {
 	.status = asix_status,
 	.link_reset = ax88172_link_reset,
 	.reset = ax88172_link_reset,
-	.flags =  FLAG_ETHER,
+	.flags =  FLAG_ETHER | FLAG_GET_LINK,
 	.data = 0x00130103,
 };
 
@@ -1357,7 +1357,7 @@ static const struct driver_info hawking_uf200_info = {
 	.status = asix_status,
 	.link_reset = ax88172_link_reset,
 	.reset = ax88172_link_reset,
-	.flags =  FLAG_ETHER,
+	.flags =  FLAG_ETHER | FLAG_GET_LINK,
 	.data = 0x001f1d1f,
 };
 
@@ -1367,7 +1367,7 @@ static const struct driver_info ax88772_info = {
 	.status = asix_status,
 	.link_reset = ax88772_link_reset,
 	.reset = ax88772_link_reset,
-	.flags = FLAG_ETHER | FLAG_FRAMING_AX,
+	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_GET_LINK,
 	.rx_fixup = asix_rx_fixup,
 	.tx_fixup = asix_tx_fixup,
 };
@@ -1378,7 +1378,7 @@ static const struct driver_info ax88178_info = {
 	.status = asix_status,
 	.link_reset = ax88178_link_reset,
 	.reset = ax88178_link_reset,
-	.flags = FLAG_ETHER | FLAG_FRAMING_AX,
+	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_GET_LINK,
 	.rx_fixup = asix_rx_fixup,
 	.tx_fixup = asix_tx_fixup,
 };
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 71e65fc..b7ff514 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -413,7 +413,7 @@ static int cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 
 static const struct driver_info	cdc_info = {
 	.description =	"CDC Ethernet Device",
-	.flags =	FLAG_ETHER,
+	.flags =	FLAG_ETHER | FLAG_GET_LINK,
 	// .check_connect = cdc_check_connect,
 	.bind =		cdc_bind,
 	.unbind =	usbnet_cdc_unbind,
diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
index a2b30a1..495f2b4 100644
--- a/drivers/net/usb/dm9601.c
+++ b/drivers/net/usb/dm9601.c
@@ -611,7 +611,7 @@ static int dm9601_link_reset(struct usbnet *dev)
 
 static const struct driver_info dm9601_info = {
 	.description	= "Davicom DM9601 USB Ethernet",
-	.flags		= FLAG_ETHER,
+	.flags		= FLAG_ETHER | FLAG_GET_LINK,
 	.bind		= dm9601_bind,
 	.rx_fixup	= dm9601_rx_fixup,
 	.tx_fixup	= dm9601_tx_fixup,
diff --git a/drivers/net/usb/mcs7830.c b/drivers/net/usb/mcs7830.c
index 10873d9..168f3be 100644
--- a/drivers/net/usb/mcs7830.c
+++ b/drivers/net/usb/mcs7830.c
@@ -549,7 +549,7 @@ static const struct driver_info moschip_info = {
 	.description	= "MOSCHIP 7830/7730 usb-NET adapter",
 	.bind		= mcs7830_bind,
 	.rx_fixup	= mcs7830_rx_fixup,
-	.flags		= FLAG_ETHER,
+	.flags		= FLAG_ETHER | FLAG_GET_LINK,
 	.in		= 1,
 	.out		= 2,
 };
@@ -558,7 +558,7 @@ static const struct driver_info sitecom_info = {
 	.description    = "Sitecom LN-30 usb-NET adapter",
 	.bind		= mcs7830_bind,
 	.rx_fixup	= mcs7830_rx_fixup,
-	.flags		= FLAG_ETHER,
+	.flags		= FLAG_ETHER | FLAG_GET_LINK,
 	.in		= 1,
 	.out		= 2,
 };
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index c6c9222..f1062c7 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1227,7 +1227,7 @@ static const struct driver_info smsc95xx_info = {
 	.rx_fixup	= smsc95xx_rx_fixup,
 	.tx_fixup	= smsc95xx_tx_fixup,
 	.status		= smsc95xx_status,
-	.flags		= FLAG_ETHER | FLAG_SEND_ZLP,
+	.flags		= FLAG_ETHER | FLAG_SEND_ZLP | FLAG_GET_LINK,
 };
 
 static const struct usb_device_id products[] = {
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 378da8c..85b5b5d 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -854,7 +854,7 @@ void usbnet_set_msglevel (struct net_device *net, u32 level)
 EXPORT_SYMBOL_GPL(usbnet_set_msglevel);
 
 /* drivers may override default ethtool_ops in their bind() routine */
-static const struct ethtool_ops usbnet_ethtool_ops = {
+static const struct ethtool_ops usbnet_get_link_ethtool_ops = {
 	.get_settings		= usbnet_get_settings,
 	.set_settings		= usbnet_set_settings,
 	.get_link		= usbnet_get_link,
@@ -864,6 +864,15 @@ static const struct ethtool_ops usbnet_ethtool_ops = {
 	.set_msglevel		= usbnet_set_msglevel,
 };
 
+static const struct ethtool_ops usbnet_ethtool_ops = {
+	.get_settings		= usbnet_get_settings,
+	.set_settings		= usbnet_set_settings,
+	.nway_reset		= usbnet_nway_reset,
+	.get_drvinfo		= usbnet_get_drvinfo,
+	.get_msglevel		= usbnet_get_msglevel,
+	.set_msglevel		= usbnet_set_msglevel,
+};
+
 /*-------------------------------------------------------------------------*/
 
 /* work that cannot be done in interrupt context uses keventd.
@@ -1285,7 +1294,11 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 
 	net->netdev_ops = &usbnet_netdev_ops;
 	net->watchdog_timeo = TX_TIMEOUT_JIFFIES;
-	net->ethtool_ops = &usbnet_ethtool_ops;
+	if (dev->driver_info->check_connect ||
+	    dev->driver_info->flags & FLAG_GET_LINK)
+		net->ethtool_ops = &usbnet_get_link_ethtool_ops;
+	else
+		net->ethtool_ops = &usbnet_ethtool_ops;
 
 	// allow device-specific bind/init procedures
 	// NOTE net->name still not usable ...
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 86c31b7..a00424d 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -92,6 +92,8 @@ struct driver_info {
 #define FLAG_SEND_ZLP	0x0200		/* hw requires ZLPs are sent */
 #define FLAG_WWAN	0x0400		/* use "wwan%d" names */
 
+#define FLAG_GET_LINK	0x0800		/* link state is available through
+					 * MII or netif_carrier_ok() */
 
 	/* init device ... can sleep, or cause probe() failure */
 	int	(*bind)(struct usbnet *, struct usb_interface *);
-- 
1.6.5.2



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] usbnet: Do not implement ethtool get_link() if link state is unknown

[David Brownell]

On Monday 02 November 2009, Ben Hutchings wrote:
> @@ -854,7 +854,7 @@ void usbnet_set_msglevel (struct net_device *net, u32 level)
>  EXPORT_SYMBOL_GPL(usbnet_set_msglevel);
>  
>  /* drivers may override default ethtool_ops in their bind() routine */
> -static const struct ethtool_ops usbnet_ethtool_ops = {
> +static const struct ethtool_ops usbnet_get_link_ethtool_ops = {
>         .get_settings           = usbnet_get_settings,
>         .set_settings           = usbnet_set_settings,
>         .get_link               = usbnet_get_link,
> @@ -864,6 +864,15 @@ static const struct ethtool_ops usbnet_ethtool_ops = {
>         .set_msglevel           = usbnet_set_msglevel,
>  };
>  
> +static const struct ethtool_ops usbnet_ethtool_ops = {
> +       .get_settings           = usbnet_get_settings,
> +       .set_settings           = usbnet_set_settings,

Surely there's a code that usbnet_get_link() could return
to say "I can't really tell"?

And if there isn't, there should be one.

Having two tables for this is needlessly ugly.

> +       .nway_reset             = usbnet_nway_reset,
> +       .get_drvinfo            = usbnet_get_drvinfo,
> +       .get_msglevel           = usbnet_get_msglevel,
> +       .set_msglevel           = usbnet_set_msglevel,
> +};
> +
>  /*-------------------------------------------------------------------------*/
>  
>  /* work that cannot be done in interrupt context uses keventd.

Re: [PATCH] usbnet: Do not implement ethtool get_link() if link state is unknown

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Mon, 2 Nov 2009 23:01:30 -0700

> On Monday 02 November 2009, Ben Hutchings wrote:
>> @@ -854,7 +854,7 @@ void usbnet_set_msglevel (struct net_device *net, u32 level)
>>  EXPORT_SYMBOL_GPL(usbnet_set_msglevel);
>>  
>>  /* drivers may override default ethtool_ops in their bind() routine */
>> -static const struct ethtool_ops usbnet_ethtool_ops = {
>> +static const struct ethtool_ops usbnet_get_link_ethtool_ops = {
>>         .get_settings           = usbnet_get_settings,
>>         .set_settings           = usbnet_set_settings,
>>         .get_link               = usbnet_get_link,
>> @@ -864,6 +864,15 @@ static const struct ethtool_ops usbnet_ethtool_ops = {
>>         .set_msglevel           = usbnet_set_msglevel,
>>  };
>>  
>> +static const struct ethtool_ops usbnet_ethtool_ops = {
>> +       .get_settings           = usbnet_get_settings,
>> +       .set_settings           = usbnet_set_settings,
> 
> Surely there's a code that usbnet_get_link() could return
> to say "I can't really tell"?
> 
> And if there isn't, there should be one.

Having a NULL operations pointer for this function is how to
indicate this.  It's a static situation based upon the device
type, not a dynamic one which would be resolved at run time
when inspecting the device registers for example.

> Having two tables for this is needlessly ugly.

Yes, it's really cruddy how the USB network driver tries to share
so much state amongst such very different devices :-)

All kidding aside, I think the alternative is for the USB network
driver to call ethtool_op_get_link() if it cannot determine the
link state in hardware.

Re: [PATCH] usbnet: Do not implement ethtool get_link() if link state is unknown

[David Brownell]

On Tuesday 03 November 2009, David Miller wrote:
> > Having two tables for this is needlessly ugly.
> 
> Yes, it's really cruddy how the USB network driver tries to share
> so much state amongst such very different devices :-)

That framework just grew ... started out as one driver,
nearly ten years back (yow!!), then generalized.  Folk
seemed to appreciate not reinventing some stuff.  ;)

If it had started out with this many devices, it might have
looked more like a library.  Sharing code implies sharing
at least some state representations; the balance could might
be worth shifting by now.


> All kidding aside, I think the alternative is for the USB network
> driver to call ethtool_op_get_link() if it cannot determine the
> link state in hardware.

There's usbnet_get_link() which does just that.  But
there may be some ancient debris confusing things.

Re: [PATCH] usbnet: Do not implement ethtool get_link() if link state is unknown

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Tue, 3 Nov 2009 02:41:27 -0700

> On Tuesday 03 November 2009, David Miller wrote:
>> All kidding aside, I think the alternative is for the USB network
>> driver to call ethtool_op_get_link() if it cannot determine the
>> link state in hardware.
> 
> There's usbnet_get_link() which does just that.  But
> there may be some ancient debris confusing things.

It's perfect, and Ben's patch is completely unnecessary.

Re: [PATCH] usbnet: Do not implement ethtool get_link() if link state is unknown

[Ben Hutchings]

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

On Tue, 2009-11-03 at 02:04 -0800, David Miller wrote:
> From: David Brownell <david-b@pacbell.net>
> Date: Tue, 3 Nov 2009 02:41:27 -0700
> 
> > On Tuesday 03 November 2009, David Miller wrote:
> >> All kidding aside, I think the alternative is for the USB network
> >> driver to call ethtool_op_get_link() if it cannot determine the
> >> link state in hardware.
> > 
> > There's usbnet_get_link() which does just that.  But
> > there may be some ancient debris confusing things.
> 
> It's perfect, and Ben's patch is completely unnecessary.

I don't see how it's 'perfect' since it reports the link as up where it
is really unknown.  Still, this is a fairly minor bug.

Ben.

-- 
Ben Hutchings
The generation of random numbers is too important to be left to chance.
                                                            - Robert Coveyou

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] usbnet: Do not implement ethtool get_link() if link state is unknown

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Wed, 04 Nov 2009 03:20:00 +0000

> I don't see how it's 'perfect' since it reports the link as up where it
> is really unknown.  Still, this is a fairly minor bug.

That is the policy we enforce for all drivers, especially virtual
ones.  If you can't tell, you specify that the link is up.

Otherwise automated tools like NetworkManager et al. will not even
attempt to bring do DHCP and bring the network device online.