| Message ID | 20201224032116.2453938-1-roland@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | CDC-NCM: remove "connected" log message | expand |
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> > --- > drivers/net/usb/cdc_ncm.c | 3 --- > 1 file changed, 3 deletions(-) > > 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; > > -- > 2.29.2 > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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!
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
> 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. Thanks, Roland
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.
Am Donnerstag, den 31.12.2020, 10:51 -0800 schrieb Roland Dreier: > I haven't tried these patches yet but they don't look quite right to > me. inlining the first 0001 patch: OK, let's see. > > 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 [...] > > +EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_mdio); > > why keep and export the old function when it will have no callers? But they will callers. Have a dozen drivers use them. The goal of this patch set is to not touch them. > > > +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. Yes. This is a drawback. Yet the speed is unknown is it not? > > @@ -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. Correct. The next iteration will do that. > > 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 Yes. > 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? Then I would have to touch them all. The problem is that the MDIO stuff really is pretty much a layering violation. It should never have been default. But now it is. Regards Oliver
> > 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? > > Then I would have to touch them all. The problem is that the MDIO > stuff really is pretty much a layering violation. It should never > have been default. But now it is. I don't understand this. Your 0001 patch changes the behavior of usbnet_get_link_ksettings() and you have to touch all of the 8 drivers that use it if you don't want to change their behavior. If you keep the old usbnet_get_link_ksettings() and add usbnet_get_link_ksettings_nonmdio() then you can just update cdc_ncm to start with, and then gradually migrate other drivers. And eventually fix the layering violation and get rid of the legacy function when the whole transition is done. - R.
Am Montag, den 04.01.2021, 11:13 -0800 schrieb Roland Dreier: > > > 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? > > > > Then I would have to touch them all. The problem is that the MDIO > > stuff really is pretty much a layering violation. It should never > > have been default. But now it is. > > I don't understand this. Your 0001 patch changes the behavior of > usbnet_get_link_ksettings() and you have to touch all of the 8 drivers > that use it if you don't want to change their behavior. If you keep > the old usbnet_get_link_ksettings() and add > usbnet_get_link_ksettings_nonmdio() then you can just update cdc_ncm > to start with, and then gradually migrate other drivers. And > eventually fix the layering violation and get rid of the legacy > function when the whole transition is done. Hi, 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"? Regards Oliver
> 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(-)