| Message ID | 20210121125731.19425-3-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | usbnet: speed reporting for devices without MDIO | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: gregkh@linuxfoundation.org kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 40 this patch: 40 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Alignment should match open parenthesis CHECK: extern prototypes should be avoided in .h files |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 39 this patch: 39 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, Jan 21, 2021 at 12:57 PM Oliver Neukum <oneukum@suse.com> wrote: > > The old method for reporting network speed upwards > assumed that a device uses MDIO and uses the generic phy > functions based on that. > Add a a primitive internal version not making the assumption > reporting back directly what the status operations record. Excellent! I wasted a bunch of time looking at emulating the MDIO interface and decided that was too hacky. I didn't realize it could be so easy to fork off a different method to collect the most recently reported speed. Thank you! > v2: adjusted to recent changes > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > Tested-by: Roland Dreier <roland@kernel.org> > --- > drivers/net/usb/usbnet.c | 23 +++++++++++++++++++++++ > include/linux/usb/usbnet.h | 4 ++++ > 2 files changed, 27 insertions(+) > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index e2ca88259b05..6f8fcc276ca7 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -961,6 +961,27 @@ int usbnet_get_link_ksettings_mdio(struct net_device *net, > } > EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_mdio); > > +int usbnet_get_link_ksettings_internal(struct net_device *net, > + struct ethtool_link_ksettings *cmd) > +{ > + struct usbnet *dev = netdev_priv(net); > + > + /* the assumption that speed is equal on tx and rx > + * is deeply engrained into the networking layer. > + * For wireless stuff it is not true. > + * We assume that rxspeed matters more. > + */ > + if (dev->rxspeed != SPEED_UNKNOWN) > + cmd->base.speed = dev->rxspeed / 1000000; > + else if (dev->txspeed != SPEED_UNKNOWN) > + cmd->base.speed = dev->txspeed / 1000000; > + else > + cmd->base.speed = SPEED_UNKNOWN; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal); > + > int usbnet_set_link_ksettings_mdio(struct net_device *net, > const struct ethtool_link_ksettings *cmd) > { > @@ -1664,6 +1685,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod) > dev->intf = udev; > dev->driver_info = info; > dev->driver_name = name; > + dev->rxspeed = SPEED_UNKNOWN; /* unknown or handled by MII */ > + dev->txspeed = SPEED_UNKNOWN; Nit: Use of SPEED_UNKNOWN here can be confusing since the units for rxspeed/txspeed are bps, not Mbps (AFAICT SPEED_XXX are Mbps numbers). In other words, this is the only usbnet location that could use SPEED_xxx define and other developers might not realize that. Personally, I'd rather set the fields to zero here. > > net->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats); > if (!net->tstats) > diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h > index fd65b7a5ee15..a91c6defb104 100644 > --- a/include/linux/usb/usbnet.h > +++ b/include/linux/usb/usbnet.h > @@ -53,6 +53,8 @@ struct usbnet { > u32 hard_mtu; /* count any extra framing */ > size_t rx_urb_size; /* size for rx urbs */ > struct mii_if_info mii; > + long rxspeed; /* if MII is not used */ > + long txspeed; /* if MII is not used */ Do you want to note the units used (bps) in the trailing comment? The numbers for cdc_ncm and cdc_ether will be "bits per second" due to how older modems could report "odd" (from an ethernet point of view) speeds. Also, the changes I just submitted (and kuba@kernel.org accepted) named these "rx_speed" (with underscore). I don't care which it is but just wanted to warn about and apologize for the conflict. cheers, grant > /* various kinds of pending driver work */ > struct sk_buff_head rxq; > @@ -269,6 +271,8 @@ extern void usbnet_purge_paused_rxq(struct usbnet *); > > extern int usbnet_get_link_ksettings_mdio(struct net_device *net, > struct ethtool_link_ksettings *cmd); > +extern int usbnet_get_link_ksettings_internal(struct net_device *net, > + struct ethtool_link_ksettings *cmd); > extern int usbnet_set_link_ksettings_mdio(struct net_device *net, > const struct ethtool_link_ksettings *cmd); > extern u32 usbnet_get_link(struct net_device *net); > -- > 2.26.2 >
On Thu, Jan 21, 2021 at 12:57 PM Oliver Neukum <oneukum@suse.com> wrote: > > The old method for reporting network speed upwards > assumed that a device uses MDIO and uses the generic phy > functions based on that. > Add a a primitive internal version not making the assumption > reporting back directly what the status operations record. > > v2: adjusted to recent changes > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > Tested-by: Roland Dreier <roland@kernel.org> > --- > drivers/net/usb/usbnet.c | 23 +++++++++++++++++++++++ > include/linux/usb/usbnet.h | 4 ++++ > 2 files changed, 27 insertions(+) > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index e2ca88259b05..6f8fcc276ca7 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -961,6 +961,27 @@ int usbnet_get_link_ksettings_mdio(struct net_device *net, > } > EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_mdio); > > +int usbnet_get_link_ksettings_internal(struct net_device *net, > + struct ethtool_link_ksettings *cmd) > +{ > + struct usbnet *dev = netdev_priv(net); > + > + /* the assumption that speed is equal on tx and rx > + * is deeply engrained into the networking layer. Another nit: s/engrained/ingrained Also, the word "symmetric" is more commonly used to describe when RX speed == TX speed (vs "equal"). cheers, grant > + * For wireless stuff it is not true. > + * We assume that rxspeed matters more. > + */ > + if (dev->rxspeed != SPEED_UNKNOWN) > + cmd->base.speed = dev->rxspeed / 1000000; > + else if (dev->txspeed != SPEED_UNKNOWN) > + cmd->base.speed = dev->txspeed / 1000000; > + else > + cmd->base.speed = SPEED_UNKNOWN; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal); > + > int usbnet_set_link_ksettings_mdio(struct net_device *net, > const struct ethtool_link_ksettings *cmd) > { > @@ -1664,6 +1685,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod) > dev->intf = udev; > dev->driver_info = info; > dev->driver_name = name; > + dev->rxspeed = SPEED_UNKNOWN; /* unknown or handled by MII */ > + dev->txspeed = SPEED_UNKNOWN; > > net->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats); > if (!net->tstats) > diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h > index fd65b7a5ee15..a91c6defb104 100644 > --- a/include/linux/usb/usbnet.h > +++ b/include/linux/usb/usbnet.h > @@ -53,6 +53,8 @@ struct usbnet { > u32 hard_mtu; /* count any extra framing */ > size_t rx_urb_size; /* size for rx urbs */ > struct mii_if_info mii; > + long rxspeed; /* if MII is not used */ > + long txspeed; /* if MII is not used */ > > /* various kinds of pending driver work */ > struct sk_buff_head rxq; > @@ -269,6 +271,8 @@ extern void usbnet_purge_paused_rxq(struct usbnet *); > > extern int usbnet_get_link_ksettings_mdio(struct net_device *net, > struct ethtool_link_ksettings *cmd); > +extern int usbnet_get_link_ksettings_internal(struct net_device *net, > + struct ethtool_link_ksettings *cmd); > extern int usbnet_set_link_ksettings_mdio(struct net_device *net, > const struct ethtool_link_ksettings *cmd); > extern u32 usbnet_get_link(struct net_device *net); > -- > 2.26.2 >
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index e2ca88259b05..6f8fcc276ca7 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -961,6 +961,27 @@ int usbnet_get_link_ksettings_mdio(struct net_device *net, } EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_mdio); +int usbnet_get_link_ksettings_internal(struct net_device *net, + struct ethtool_link_ksettings *cmd) +{ + struct usbnet *dev = netdev_priv(net); + + /* the assumption that speed is equal on tx and rx + * is deeply engrained into the networking layer. + * For wireless stuff it is not true. + * We assume that rxspeed matters more. + */ + if (dev->rxspeed != SPEED_UNKNOWN) + cmd->base.speed = dev->rxspeed / 1000000; + else if (dev->txspeed != SPEED_UNKNOWN) + cmd->base.speed = dev->txspeed / 1000000; + else + cmd->base.speed = SPEED_UNKNOWN; + + return 0; +} +EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal); + int usbnet_set_link_ksettings_mdio(struct net_device *net, const struct ethtool_link_ksettings *cmd) { @@ -1664,6 +1685,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod) dev->intf = udev; dev->driver_info = info; dev->driver_name = name; + dev->rxspeed = SPEED_UNKNOWN; /* unknown or handled by MII */ + dev->txspeed = SPEED_UNKNOWN; net->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats); if (!net->tstats) diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h index fd65b7a5ee15..a91c6defb104 100644 --- a/include/linux/usb/usbnet.h +++ b/include/linux/usb/usbnet.h @@ -53,6 +53,8 @@ struct usbnet { u32 hard_mtu; /* count any extra framing */ size_t rx_urb_size; /* size for rx urbs */ struct mii_if_info mii; + long rxspeed; /* if MII is not used */ + long txspeed; /* if MII is not used */ /* various kinds of pending driver work */ struct sk_buff_head rxq; @@ -269,6 +271,8 @@ extern void usbnet_purge_paused_rxq(struct usbnet *); extern int usbnet_get_link_ksettings_mdio(struct net_device *net, struct ethtool_link_ksettings *cmd); +extern int usbnet_get_link_ksettings_internal(struct net_device *net, + struct ethtool_link_ksettings *cmd); extern int usbnet_set_link_ksettings_mdio(struct net_device *net, const struct ethtool_link_ksettings *cmd); extern u32 usbnet_get_link(struct net_device *net);