| Message ID | 20201224032116.2453938-1-roland@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | CDC-NCM: remove "connected" log message | expand |
On Thu, 24 Dec 2020 08:53:52 +0100 Greg KH wrote: > On Wed, Dec 23, 2020 at 07:21:16PM -0800, Roland Dreier wrote: > > The cdc_ncm driver passes network connection notifications up to > > usbnet_link_change(), which is the right place for any logging. > > Remove the netdev_info() duplicating this from the driver itself. > > > > This stops devices such as my "TRENDnet USB 10/100/1G/2.5G LAN" > > (ID 20f4:e02b) adapter from spamming the kernel log with > > > > cdc_ncm 2-2:2.0 enp0s2u2c2: network connection: connected > > > > messages every 60 msec or so. > > > > Signed-off-by: Roland Dreier <roland@kernel.org> > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Applied to net and queued for LTS, thanks!
> Applied to net and queued for LTS, thanks!
Thanks - is Oliver's series of 3 patches that get rid of the other
half of the log spam also on the way upstream?
- R.
Am Montag, den 28.12.2020, 23:56 -0800 schrieb Roland Dreier: > > Applied to net and queued for LTS, thanks! > > Thanks - is Oliver's series of 3 patches that get rid of the other > half of the log spam also on the way upstream? Hi, I looked at them again and found that there is a way to get the same effect that will make maintenance easier in the long run. Could I send them to you later this week for testing? Regards Oliver
Am Dienstag, den 29.12.2020, 11:50 -0800 schrieb Roland Dreier: > > I looked at them again and found that there is a way to get > > the same effect that will make maintenance easier in the long run. > > Could I send them to you later this week for testing? > > Yes, please. I have a good test setup now so I can easily try out patches. Thank you, here we go. Regards Oliver From 7dfb5f35933ebbe12076e41606b178fa7d8d2e7b Mon Sep 17 00:00:00 2001 From: Oliver Neukum <oneukum@suse.com> Date: Tue, 1 Dec 2020 11:31:15 +0100 Subject: [PATCH 1/2] usbnet: add method for reporting speed without MDIO 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. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/net/usb/usbnet.c | 30 +++++++++++++++++++++++++++++- include/linux/usb/usbnet.h | 7 ++++++- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 1447da1d5729..bcd17f6d6de6 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -944,7 +944,7 @@ EXPORT_SYMBOL_GPL(usbnet_open); * they'll probably want to use this base set. */ -int usbnet_get_link_ksettings(struct net_device *net, +int usbnet_get_link_ksettings_mdio(struct net_device *net, struct ethtool_link_ksettings *cmd) { struct usbnet *dev = netdev_priv(net); @@ -956,6 +956,32 @@ int usbnet_get_link_ksettings(struct net_device *net, return 0; } +EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_mdio); + +int usbnet_get_link_ksettings(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; + /* if a minidriver does not record speed we try to + * fall back on MDIO + */ + else if (!dev->mii.mdio_read) + cmd->base.speed = SPEED_UNKNOWN; + else + mii_ethtool_get_link_ksettings(&dev->mii, cmd); + + return 0; +} EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings); int usbnet_set_link_ksettings(struct net_device *net, @@ -1661,6 +1687,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 = -1; /* unknown or handled by MII */ + dev->txspeed = -1; 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 88a7673894d5..f748c758f82a 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; + int rxspeed; /* if MII is not used */ + int txspeed; /* if MII is not used */ /* various kinds of pending driver work */ struct sk_buff_head rxq; @@ -267,8 +269,11 @@ extern void usbnet_purge_paused_rxq(struct usbnet *); extern int usbnet_get_link_ksettings(struct net_device *net, struct ethtool_link_ksettings *cmd); -extern int usbnet_set_link_ksettings(struct net_device *net, +extern int usbnet_set_link_ksettings_mdio(struct net_device *net, const struct ethtool_link_ksettings *cmd); +/* Legacy - to be used if you really need an error to be returned */ +extern int usbnet_set_link_ksettings(struct net_device *net, + const struct ethtool_link_ksettings *cmd); extern u32 usbnet_get_link(struct net_device *net); extern u32 usbnet_get_msglevel(struct net_device *); extern void usbnet_set_msglevel(struct net_device *, u32);
I haven't tried these patches yet but they don't look quite right to me. inlining the first 0001 patch: > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index 1447da1d5729..bcd17f6d6de6 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -944,7 +944,7 @@ EXPORT_SYMBOL_GPL(usbnet_open); > * they'll probably want to use this base set. > */ > > -int usbnet_get_link_ksettings(struct net_device *net, > +int usbnet_get_link_ksettings_mdio(struct net_device *net, > struct ethtool_link_ksettings *cmd) > { > struct usbnet *dev = netdev_priv(net); > @@ -956,6 +956,32 @@ int usbnet_get_link_ksettings(struct net_device *net, > > return 0; > } > +EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_mdio); why keep and export the old function when it will have no callers? > +int usbnet_get_link_ksettings(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; > + /* if a minidriver does not record speed we try to > + * fall back on MDIO > + */ > + else if (!dev->mii.mdio_read) > + cmd->base.speed = SPEED_UNKNOWN; > + else > + mii_ethtool_get_link_ksettings(&dev->mii, cmd); > + > + return 0; This is a change in behavior for every driver that doesn't set rxspeed / txspeed - the old get_link function would return EOPNOTSUPP if mdio_read isn't implemented, now we give SPEED_UNKNOWN with a successful return code. > @@ -1661,6 +1687,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 = -1; /* unknown or handled by MII */ > + dev->txspeed = -1; Minor nit: if we're going to test these against SPEED_UNKNOWN above, then I think it's clearer to initialize them to that value via the same constant. > diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h > index 88a7673894d5..f748c758f82a 100644 > --- a/include/linux/usb/usbnet.h > +++ b/include/linux/usb/usbnet.h > @@ -267,8 +269,11 @@ extern void usbnet_purge_paused_rxq(struct usbnet *); > > extern int usbnet_get_link_ksettings(struct net_device *net, > struct ethtool_link_ksettings *cmd); > -extern int usbnet_set_link_ksettings(struct net_device *net, > +extern int usbnet_set_link_ksettings_mdio(struct net_device *net, > const struct ethtool_link_ksettings *cmd); > +/* Legacy - to be used if you really need an error to be returned */ > +extern int usbnet_set_link_ksettings(struct net_device *net, > + const struct ethtool_link_ksettings *cmd); > extern u32 usbnet_get_link(struct net_device *net); > extern u32 usbnet_get_msglevel(struct net_device *); > extern void usbnet_set_msglevel(struct net_device *, u32); I think this was meant to be changing get_link, not set_link. Also I don't understand the "Legacy" comment. Is that referring to the EOPNOTSUPP change I mentioned above? If so, wouldn't it be better to preserve the legacy behavior rather than changing the behavior of every usbnet driver all at once? Like make a new usbnet_get_link_ksettings_nonmdio and update only cdc_ncm to use it? - R.
> now that you put it that way, I get the merit of what you are saying. > Very well. I will submit the first set of patches. > > May I add your "Tested-by"? Yes, absolutely: Tested-by: Roland Dreier <roland@kernel.org>
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index a45fcc44facf..50d3a4e6d445 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -1850,9 +1850,6 @@ static void cdc_ncm_status(struct usbnet *dev, struct urb *urb) * USB_CDC_NOTIFY_NETWORK_CONNECTION notification shall be * sent by device after USB_CDC_NOTIFY_SPEED_CHANGE. */ - netif_info(dev, link, dev->net, - "network connection: %sconnected\n", - !!event->wValue ? "" : "dis"); usbnet_link_change(dev, !!event->wValue, 0); break;
The cdc_ncm driver passes network connection notifications up to usbnet_link_change(), which is the right place for any logging. Remove the netdev_info() duplicating this from the driver itself. This stops devices such as my "TRENDnet USB 10/100/1G/2.5G LAN" (ID 20f4:e02b) adapter from spamming the kernel log with cdc_ncm 2-2:2.0 enp0s2u2c2: network connection: connected messages every 60 msec or so. Signed-off-by: Roland Dreier <roland@kernel.org> --- drivers/net/usb/cdc_ncm.c | 3 --- 1 file changed, 3 deletions(-)