cdc_ncm kernel log spam with trendnet 2.5G USB adapter

cdc_ncm kernel log spam with trendnet 2.5G USB adapter

[Roland Dreier]

Hi, I recently got a 2.5G USB adapter, and although it works fine, the
driver continually spams the kernel log with messages like

[127662.025647] cdc_ncm 4-1:2.0 enx3c8cf8f942e0: network connection: connected
[127662.057680] cdc_ncm 4-1:2.0 enx3c8cf8f942e0: 2500 mbit/s downlink
2500 mbit/s uplink
[127662.089794] cdc_ncm 4-1:2.0 enx3c8cf8f942e0: network connection: connected
[127662.121831] cdc_ncm 4-1:2.0 enx3c8cf8f942e0: 2500 mbit/s downlink
2500 mbit/s uplink
[127662.153858] cdc_ncm 4-1:2.0 enx3c8cf8f942e0: network connection: connected
...

Looking at the code in cdc_ncm.c it seems the device is just sending
USB_CDC_NOTIFY_NETWORK_CONNECTION and USB_CDC_NOTIFY_SPEED_CHANGE urbs
over and over, and the driver logs every single one.

Should we add code to the driver to keep track of what the last event
was and only log if the state has really change?

Thanks!
  Roland

Full details on the adapter, in case it matters:

Bus 004 Device 005: ID 20f4:e02b TRENDnet USB 10/100/1G/2.5G LAN
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               3.20
  bDeviceClass            0
  bDeviceSubClass         0
  bDeviceProtocol         0
  bMaxPacketSize0         9
  idVendor           0x20f4 TRENDnet
  idProduct          0xe02b
  bcdDevice           30.04
  iManufacturer           1
  iProduct                2
  iSerial                 6
  bNumConfigurations      3
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength       0x0039
    bNumInterfaces          1
    bConfigurationValue     1
    iConfiguration          0
    bmAttributes         0xa0
      (Bus Powered)
      Remote Wakeup
    MaxPower              512mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           3
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass    255 Vendor Specific Subclass
      bInterfaceProtocol      0
      iInterface              0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0400  1x 1024 bytes
        bInterval               0
        bMaxBurst               3
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x02  EP 2 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0400  1x 1024 bytes
        bInterval               0
        bMaxBurst               3
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0002  1x 2 bytes
        bInterval              11
        bMaxBurst               0
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength       0x0068
    bNumInterfaces          2
    bConfigurationValue     2
    iConfiguration          0
    bmAttributes         0xa0
      (Bus Powered)
      Remote Wakeup
    MaxPower              512mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         2 Communications
      bInterfaceSubClass     13
      bInterfaceProtocol      0
      iInterface              5
      CDC Header:
        bcdCDC               1.10
      CDC Union:
        bMasterInterface        0
        bSlaveInterface         1
      CDC Ethernet:
        iMacAddress                      3 (??)
        bmEthernetStatistics    0x0031500f
        wMaxSegmentSize               1518
        wNumberMCFilters            0x8000
        bNumberPowerFilters              0
      CDC NCM:
        bcdNcmVersion        1.00
        bmNetworkCapabilities 0x2b
          8-byte ntb input size
          max datagram size
          net address
          packet filter
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0010  1x 16 bytes
        bInterval               8
        bMaxBurst               0
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       0
      bNumEndpoints           0
      bInterfaceClass        10 CDC Data
      bInterfaceSubClass      0
      bInterfaceProtocol      1
      iInterface              0
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       1
      bNumEndpoints           2
      bInterfaceClass        10 CDC Data
      bInterfaceSubClass      0
      bInterfaceProtocol      1
      iInterface              4
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0400  1x 1024 bytes
        bInterval               0
        bMaxBurst               3
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x02  EP 2 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0400  1x 1024 bytes
        bInterval               0
        bMaxBurst               3
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength       0x0062
    bNumInterfaces          2
    bConfigurationValue     3
    iConfiguration          0
    bmAttributes         0xa0
      (Bus Powered)
      Remote Wakeup
    MaxPower              512mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         2 Communications
      bInterfaceSubClass      6 Ethernet Networking
      bInterfaceProtocol      0
      iInterface              5
      CDC Header:
        bcdCDC               1.10
      CDC Union:
        bMasterInterface        0
        bSlaveInterface         1
      CDC Ethernet:
        iMacAddress                      3 (??)
        bmEthernetStatistics    0x0031500f
        wMaxSegmentSize               1518
        wNumberMCFilters            0x8000
        bNumberPowerFilters              0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0010  1x 16 bytes
        bInterval               8
        bMaxBurst               0
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       0
      bNumEndpoints           0
      bInterfaceClass        10 CDC Data
      bInterfaceSubClass      0
      bInterfaceProtocol      0
      iInterface              0
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       1
      bNumEndpoints           2
      bInterfaceClass        10 CDC Data
      bInterfaceSubClass      0
      bInterfaceProtocol      0
      iInterface              4
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0400  1x 1024 bytes
        bInterval               0
        bMaxBurst               3
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x02  EP 2 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0400  1x 1024 bytes
        bInterval               0
        bMaxBurst               3

Re: cdc_ncm kernel log spam with trendnet 2.5G USB adapter

[Oliver Neukum]

Am Montag, den 16.11.2020, 09:23 -0800 schrieb Roland Dreier:
> Hi, I recently got a 2.5G USB adapter, and although it works fine, the
> driver continually spams the kernel log with messages like
> 
> [127662.025647] cdc_ncm 4-1:2.0 enx3c8cf8f942e0: network connection: connected
> [127662.057680] cdc_ncm 4-1:2.0 enx3c8cf8f942e0: 2500 mbit/s downlink
> 2500 mbit/s uplink
> [127662.089794] cdc_ncm 4-1:2.0 enx3c8cf8f942e0: network connection: connected
> [127662.121831] cdc_ncm 4-1:2.0 enx3c8cf8f942e0: 2500 mbit/s downlink
> 2500 mbit/s uplink
> [127662.153858] cdc_ncm 4-1:2.0 enx3c8cf8f942e0: network connection: connected
> ...
> 
> Looking at the code in cdc_ncm.c it seems the device is just sending
> USB_CDC_NOTIFY_NETWORK_CONNECTION and USB_CDC_NOTIFY_SPEED_CHANGE urbs
> over and over, and the driver logs every single one.
> 
> Should we add code to the driver to keep track of what the last event
> was and only log if the state has really change?

Hi,

in theory I suppose we could do that, but it would be kind of stupid.
Events like these should be reported to higher layers and not logged at
all.

	Regards
		Oliver

Re: cdc_ncm kernel log spam with trendnet 2.5G USB adapter

[Roland Dreier]

> your criticism was valid. Could you test the attached patches?

Thanks - I will build a kernel and report back.

 - R.

Re: cdc_ncm kernel log spam with trendnet 2.5G USB adapter

[Roland Dreier]

(Apologies, trying one more time with a better mailer)

Sorry it took so long, but I finally got a chance to test the patches.  They
seem to work well, but they only get rid of the downlink / uplink speed spam -
I still get the following filling my kernel log with a patched kernel:

  [   29.830383] cdc_ncm 2-2:2.0 enp0s2u2c2: network connection: connected
  [   29.894359] cdc_ncm 2-2:2.0 enp0s2u2c2: network connection: connected
  [   29.958601] cdc_ncm 2-2:2.0 enp0s2u2c2: network connection: connected
  [   30.022473] cdc_ncm 2-2:2.0 enp0s2u2c2: network connection: connected
  [   30.086548] cdc_ncm 2-2:2.0 enp0s2u2c2: network connection: connected

with the below patch on top of your 3, then my kernel log is clean.

Please apply your patches plus my patch, and feel free to add

Tested-by: Roland Dreier <roland@kernel.org>

to the other three.

--- 8< ---------- 8< ---

Subject: [PATCH] CDC-NCM: remove "connected" log message

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

Re: cdc_ncm kernel log spam with trendnet 2.5G USB adapter

[Jakub Kicinski]

On Sat, 19 Dec 2020 14:21:40 -0800 Roland Dreier wrote:
> (Apologies, trying one more time with a better mailer)
> 
> Sorry it took so long, but I finally got a chance to test the patches.  They
> seem to work well, but they only get rid of the downlink / uplink speed spam -
> I still get the following filling my kernel log with a patched kernel:
> 
>   [   29.830383] cdc_ncm 2-2:2.0 enp0s2u2c2: network connection: connected
>   [   29.894359] cdc_ncm 2-2:2.0 enp0s2u2c2: network connection: connected
>   [   29.958601] cdc_ncm 2-2:2.0 enp0s2u2c2: network connection: connected
>   [   30.022473] cdc_ncm 2-2:2.0 enp0s2u2c2: network connection: connected
>   [   30.086548] cdc_ncm 2-2:2.0 enp0s2u2c2: network connection: connected
> 
> with the below patch on top of your 3, then my kernel log is clean.
> 
> Please apply your patches plus my patch, and feel free to add
> 
> Tested-by: Roland Dreier <roland@kernel.org>
> 
> to the other three.

Hi Ronald, thanks for the patch.

I'm not sure what the story here is but if this change is expected to
get into the networking tree we'll need a fresh posting. This sort of
scissored reply does not get into patchwork.

> Subject: [PATCH] CDC-NCM: remove "connected" log message
> 
> 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;
>  

It sounds like you're getting tens of those messages a second, we can
remove the message but the device is still generating spurious events,
wasting CPU cycles. Was blocking those events deemed unfeasible? 

Re: cdc_ncm kernel log spam with trendnet 2.5G USB adapter

[Roland Dreier]

On Tue, Dec 22, 2020 at 6:49 PM Jakub Kicinski <kuba@kernel.org> wrote:

> I'm not sure what the story here is but if this change is expected to
> get into the networking tree we'll need a fresh posting. This sort of
> scissored reply does not get into patchwork.

OK, will resend.  Too bad about patchwork, "git am" drops everything
before scissors lines by default.

> It sounds like you're getting tens of those messages a second, we can
> remove the message but the device is still generating spurious events,
> wasting CPU cycles. Was blocking those events deemed unfeasible?

I certainly don't know enough about the USB CDC class to know why the
spurious messages are showing up or whether they could be suppressed
without a fix in the adapter firmware.  But even ~30 spurious messages
per second doesn't seem so bad for a multi-gig adapter that might be
handling 100,000 or more packets per second.

 - R.

[PATCH] CDC-NCM: remove "connected" log message

[Roland Dreier]

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

Re: [PATCH] CDC-NCM: remove "connected" log message

[Greg KH]

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>

Re: [PATCH] CDC-NCM: remove "connected" log message

[Jakub Kicinski]

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!

Re: [PATCH] CDC-NCM: remove "connected" log message

[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?

 - R.

Re: [PATCH] CDC-NCM: remove "connected" log message

[Oliver Neukum]

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

Re: [PATCH] CDC-NCM: remove "connected" log message

[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.

Thanks,
  Roland

Re: [PATCH] CDC-NCM: remove "connected" log message

[Oliver Neukum]

[-- Attachment #1: Type: text/plain, Size: 383 bytes --]

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


[-- Attachment #2: 0001-usbnet-add-method-for-reporting-speed-without-MDIO.patch --]
[-- Type: text/x-patch, Size: 3693 bytes --]

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);
-- 
2.26.2


[-- Attachment #3: 0002-CDC-NCM-record-speed-in-status-method.patch --]
[-- Type: text/x-patch, Size: 1802 bytes --]

From 447850c0e90aef5a8bb569d3888c76032a82c7c7 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Tue, 1 Dec 2020 11:33:38 +0100
Subject: [PATCH 2/2] CDC-NCM: record speed in status method

The driver has a status method for receiving speed updates.
The framework, however, had support functions only for devices
that reported their speed upon an explicit query over a MDIO
interface.
CDC_NCM however gets direct notifications from the device.
As new support functions have become available, we shall now
record such notifications and tell the usbnet framework
to make direct use of them without going through the PHY layer.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/cdc_ncm.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 2bac57d5e8d5..78cb3da8de0b 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1823,21 +1823,8 @@ cdc_ncm_speed_change(struct usbnet *dev,
 	uint32_t rx_speed = le32_to_cpu(data->DLBitRRate);
 	uint32_t tx_speed = le32_to_cpu(data->ULBitRate);
 
-	/*
-	 * Currently the USB-NET API does not support reporting the actual
-	 * device speed. Do print it instead.
-	 */
-	if ((tx_speed > 1000000) && (rx_speed > 1000000)) {
-		netif_info(dev, link, dev->net,
-			   "%u mbit/s downlink %u mbit/s uplink\n",
-			   (unsigned int)(rx_speed / 1000000U),
-			   (unsigned int)(tx_speed / 1000000U));
-	} else {
-		netif_info(dev, link, dev->net,
-			   "%u kbit/s downlink %u kbit/s uplink\n",
-			   (unsigned int)(rx_speed / 1000U),
-			   (unsigned int)(tx_speed / 1000U));
-	}
+	dev->rxspeed = rx_speed;
+	dev->txspeed = tx_speed;
 }
 
 static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
-- 
2.26.2

Re: [PATCH] CDC-NCM: remove "connected" log message

[Roland Dreier]

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.

Re: [PATCH] CDC-NCM: remove "connected" log message

[Oliver Neukum]

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

Re: [PATCH] CDC-NCM: remove "connected" log message

[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.

 - R.

Re: [PATCH] CDC-NCM: remove "connected" log message

[Oliver Neukum]

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

Re: [PATCH] CDC-NCM: remove "connected" log message

[Roland Dreier]

> 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>