usbnet: Do not implement ethtool get_link() if link state is unknown
Subsystems (8)
PATCH 1/1 usbnet: Do not implement ethtool get_link() if link state is unknown
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.
--- 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(-)
drivers/net/usb/asix.c +6 -6
@@ -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, };
drivers/net/usb/cdc_ether.c +1 -1
@@ -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,
drivers/net/usb/dm9601.c +1 -1
@@ -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,
drivers/net/usb/mcs7830.c +2 -2
@@ -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, };
drivers/net/usb/smsc95xx.c +1 -1
@@ -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[] = {
drivers/net/usb/usbnet.c +15 -2
@@ -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 ...
include/linux/usb/usbnet.h +2 -0
@@ -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 *);
Signature · 1 line
1.6.5.2
Quoted 12 lines · Ben Hutchings
drivers/net/usb/usbnet.c +0
-0@@ -854,7 +854,7 @@ void usbnet_set_msglevel (struct net_device *net, u32 level) EXPORT_SYMBOL_GPL(usbnet_set_msglevel);
/* drivers may override defaultethtool_opsin theirbind()routine */drivers/net/usb/usbnet.c +1
-1-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,drivers/net/usb/usbnet.c +0
-0@@ -864,6 +864,15 @@ static const struct ethtool_ops usbnet_ethtool_ops = { .set_msglevel = usbnet_set_msglevel, };
Quoted 4 lines
+static const structethtool_opsusbnet_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.
Quoted 9 lines
+ .nway_reset =
usbnet_nway_reset, + .get_drvinfo =usbnet_get_drvinfo, + .get_msglevel =usbnet_get_msglevel, + .set_msglevel =usbnet_set_msglevel,drivers/net/usb/usbnet.c +2
-0+}; +/*-------------------------------------------------------------------------*/
/* work that cannot be done in interrupt context uses keventd.
David Miller
maintainerFrom: David Brownell <[email protected]>
Date: Mon, 2 Nov 2009 23:01:30 -0700
Quoted 21 lines · Ben Hutchings
Quoted 12 lines
drivers/net/usb/usbnet.c +0
-0@@ -854,7 +854,7 @@ void usbnet_set_msglevel (struct net_device *net, u32 level) EXPORT_SYMBOL_GPL(usbnet_set_msglevel);
/* drivers may override defaultethtool_opsin theirbind()routine */drivers/net/usb/usbnet.c +1
-1-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,drivers/net/usb/usbnet.c +0
-0@@ -864,6 +864,15 @@ static const struct ethtool_ops usbnet_ethtool_ops = { .set_msglevel = usbnet_set_msglevel, };Quoted 4 lines
+static const structethtool_opsusbnet_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"?
Quoted 1 line
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.
Quoted 1 line
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.
Quoted 4 lines · David Miller
Quoted 1 line
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.
Quoted 3 lines
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.
David Miller
maintainerFrom: David Brownell <[email protected]>
Date: Tue, 3 Nov 2009 02:41:27 -0700
Quoted 7 lines · David Miller
Quoted 3 lines
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.
Quoted 4 lines · David Miller
From: David Brownell <[email protected]>
Date: Tue, 3 Nov 2009 02:41:27 -0700Quoted 1 line · David Miller
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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.
Signature · 3 lines
Ben Hutchings
The generation of random numbers is too important to be left to chance.
- Robert CoveyouDavid Miller
maintainerFrom: Ben Hutchings <[email protected]>
Date: Wed, 04 Nov 2009 03:20:00 +0000
Quoted 2 lines
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.