| Message ID | 20201224032116.2453938-1-roland@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | CDC-NCM: remove "connected" log message | 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: davem@davemloft.net 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: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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!
> 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
> 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
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(-)