| Message ID | 1257218818.3136.440.camel@localhost |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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
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. -- 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
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. -- 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
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. -- 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
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.
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. -- 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
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 *);
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(-)