| Message ID | 20210121125731.19425-1-oneukum@suse.com |
|---|---|
| Headers | show |
| Series | usbnet: speed reporting for devices without MDIO | expand |
On Thu, Jan 21, 2021 at 01:57:28PM +0100, Oliver Neukum wrote: > This series introduces support for USB network devices that report > speed as a part of their protocol, not emulating an MII to be accessed > over MDIO. > > v2: adjusted to recent changes Hi Oliver Please give more details what actually changed. Does this mean you just rebased it on net-next? Or have you made real changes? The discussion with v1 suggested that this framework should also be used by anything which gets notified in CDC style. So i was expecting to see cdc_ether.c also use this. Andrew
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 Fri, Jan 22, 2021 at 1:29 AM Andrew Lunn <andrew@lunn.ch> wrote: > > On Thu, Jan 21, 2021 at 01:57:28PM +0100, Oliver Neukum wrote: > > This series introduces support for USB network devices that report > > speed as a part of their protocol, not emulating an MII to be accessed > > over MDIO. > > > > v2: adjusted to recent changes > > Hi Oliver > > Please give more details what actually changed. Does this mean you > just rebased it on net-next? Or have you made real changes? My apologies to Oliver - the changes he's referring to are the ones I submitted: https://www.spinics.net/lists/netdev/msg715248.html which is related to this series: https://www.spinics.net/lists/netdev/msg714493.html I wasn't aware of and didn't look for the series Oliver had previously posted. *sigh* I have been talking to Realtek about getting the issue of RTL8156 spewing notifications every 32ms fixed (thinking a FW change could fix it) for nearly three months. It is unfortunate timing that Roland Dreier decided to do something about it in December - which I didn't expect to happen given this problem was reported nearly two years ago. > The discussion with v1 suggested that this framework should also be > used by anything which gets notified in CDC style. So i was expecting > to see cdc_ether.c also use this. Agreed. That's a two lines change to cdc_ether.c. I can submit this if Oliver doesn't want to spin the series. I've reviewed all three patches and besides one nit (which could be ignored or fixed later), I'm offering my Reviewed-by: Grant Grundler <grundler@chromium.org> in the off chance that helps get this accepted into net-next (and/or 5.11 RC release). cheers, grant > > Andrew
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 >
Oliver, it's been a few weeks, do you have time to post an updated patchset or is there someone else you would trust to help land this series? cheers, grant On Fri, Jan 22, 2021 at 2:10 AM Grant Grundler <grundler@chromium.org> wrote: > > On Fri, Jan 22, 2021 at 1:29 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > On Thu, Jan 21, 2021 at 01:57:28PM +0100, Oliver Neukum wrote: > > > This series introduces support for USB network devices that report > > > speed as a part of their protocol, not emulating an MII to be accessed > > > over MDIO. > > > > > > v2: adjusted to recent changes > > > > Hi Oliver > > > > Please give more details what actually changed. Does this mean you > > just rebased it on net-next? Or have you made real changes? > > My apologies to Oliver - the changes he's referring to are the ones I submitted: > https://www.spinics.net/lists/netdev/msg715248.html > > which is related to this series: > https://www.spinics.net/lists/netdev/msg714493.html > > I wasn't aware of and didn't look for the series Oliver had previously > posted. *sigh* I have been talking to Realtek about getting the issue > of RTL8156 spewing notifications every 32ms fixed (thinking a FW > change could fix it) for nearly three months. It is unfortunate > timing that Roland Dreier decided to do something about it in December > - which I didn't expect to happen given this problem was reported > nearly two years ago. > > > The discussion with v1 suggested that this framework should also be > > used by anything which gets notified in CDC style. So i was expecting > > to see cdc_ether.c also use this. > > Agreed. That's a two lines change to cdc_ether.c. I can submit this if > Oliver doesn't want to spin the series. > > I've reviewed all three patches and besides one nit (which could be > ignored or fixed later), I'm offering my > Reviewed-by: Grant Grundler <grundler@chromium.org> > > in the off chance that helps get this accepted into net-next (and/or > 5.11 RC release). > > cheers, > grant > > > > > Andrew