* cdc_ncm kernel log spam with trendnet 2.5G USB adapter @ 2020-11-16 17:23 Roland Dreier 2020-11-26 12:05 ` Oliver Neukum [not found] ` <3a9b2c8c275d56d9c7904cf9b5177047b196173d.camel@neukum.org> 0 siblings, 2 replies; 18+ messages in thread From: Roland Dreier @ 2020-11-16 17:23 UTC (permalink / raw) To: netdev, linux-usb, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: cdc_ncm kernel log spam with trendnet 2.5G USB adapter 2020-11-16 17:23 cdc_ncm kernel log spam with trendnet 2.5G USB adapter Roland Dreier @ 2020-11-26 12:05 ` Oliver Neukum [not found] ` <3a9b2c8c275d56d9c7904cf9b5177047b196173d.camel@neukum.org> 1 sibling, 0 replies; 18+ messages in thread From: Oliver Neukum @ 2020-11-26 12:05 UTC (permalink / raw) To: Roland Dreier, netdev, linux-usb 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <3a9b2c8c275d56d9c7904cf9b5177047b196173d.camel@neukum.org>]
* Re: cdc_ncm kernel log spam with trendnet 2.5G USB adapter [not found] ` <3a9b2c8c275d56d9c7904cf9b5177047b196173d.camel@neukum.org> @ 2020-12-01 20:42 ` Roland Dreier 2020-12-19 22:21 ` Roland Dreier 1 sibling, 0 replies; 18+ messages in thread From: Roland Dreier @ 2020-12-01 20:42 UTC (permalink / raw) To: Oliver Neukum; +Cc: netdev, linux-usb > your criticism was valid. Could you test the attached patches? Thanks - I will build a kernel and report back. - R. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: cdc_ncm kernel log spam with trendnet 2.5G USB adapter [not found] ` <3a9b2c8c275d56d9c7904cf9b5177047b196173d.camel@neukum.org> 2020-12-01 20:42 ` Roland Dreier @ 2020-12-19 22:21 ` Roland Dreier 2020-12-23 2:49 ` Jakub Kicinski 1 sibling, 1 reply; 18+ messages in thread From: Roland Dreier @ 2020-12-19 22:21 UTC (permalink / raw) To: Oliver Neukum; +Cc: netdev, linux-usb (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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: cdc_ncm kernel log spam with trendnet 2.5G USB adapter 2020-12-19 22:21 ` Roland Dreier @ 2020-12-23 2:49 ` Jakub Kicinski 2020-12-23 3:01 ` Roland Dreier 2020-12-24 3:21 ` [PATCH] CDC-NCM: remove "connected" log message Roland Dreier 0 siblings, 2 replies; 18+ messages in thread From: Jakub Kicinski @ 2020-12-23 2:49 UTC (permalink / raw) To: Roland Dreier; +Cc: Oliver Neukum, netdev, linux-usb 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? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: cdc_ncm kernel log spam with trendnet 2.5G USB adapter 2020-12-23 2:49 ` Jakub Kicinski @ 2020-12-23 3:01 ` Roland Dreier 2020-12-24 3:21 ` [PATCH] CDC-NCM: remove "connected" log message Roland Dreier 1 sibling, 0 replies; 18+ messages in thread From: Roland Dreier @ 2020-12-23 3:01 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Oliver Neukum, netdev, linux-usb 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] CDC-NCM: remove "connected" log message 2020-12-23 2:49 ` Jakub Kicinski 2020-12-23 3:01 ` Roland Dreier @ 2020-12-24 3:21 ` Roland Dreier 2020-12-24 7:53 ` Greg KH 1 sibling, 1 reply; 18+ messages in thread From: Roland Dreier @ 2020-12-24 3:21 UTC (permalink / raw) To: Oliver Neukum; +Cc: netdev, linux-usb 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] CDC-NCM: remove "connected" log message 2020-12-24 3:21 ` [PATCH] CDC-NCM: remove "connected" log message Roland Dreier @ 2020-12-24 7:53 ` Greg KH 2020-12-28 21:30 ` Jakub Kicinski 0 siblings, 1 reply; 18+ messages in thread From: Greg KH @ 2020-12-24 7:53 UTC (permalink / raw) To: Roland Dreier; +Cc: Oliver Neukum, netdev, linux-usb 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] CDC-NCM: remove "connected" log message 2020-12-24 7:53 ` Greg KH @ 2020-12-28 21:30 ` Jakub Kicinski 2020-12-29 7:56 ` Roland Dreier 0 siblings, 1 reply; 18+ messages in thread From: Jakub Kicinski @ 2020-12-28 21:30 UTC (permalink / raw) To: Greg KH, Roland Dreier; +Cc: Oliver Neukum, netdev, linux-usb 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! ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] CDC-NCM: remove "connected" log message 2020-12-28 21:30 ` Jakub Kicinski @ 2020-12-29 7:56 ` Roland Dreier 2020-12-29 12:30 ` Oliver Neukum [not found] ` <24c6faa2a4f91c721d9a7f14bb7b641b89ae987d.camel@neukum.org> 0 siblings, 2 replies; 18+ messages in thread From: Roland Dreier @ 2020-12-29 7:56 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Greg KH, Oliver Neukum, netdev, linux-usb > 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] CDC-NCM: remove "connected" log message 2020-12-29 7:56 ` Roland Dreier @ 2020-12-29 12:30 ` Oliver Neukum [not found] ` <24c6faa2a4f91c721d9a7f14bb7b641b89ae987d.camel@neukum.org> 1 sibling, 0 replies; 18+ messages in thread From: Oliver Neukum @ 2020-12-29 12:30 UTC (permalink / raw) To: Roland Dreier, Jakub Kicinski; +Cc: Greg KH, netdev, linux-usb 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <24c6faa2a4f91c721d9a7f14bb7b641b89ae987d.camel@neukum.org>]
* Re: [PATCH] CDC-NCM: remove "connected" log message [not found] ` <24c6faa2a4f91c721d9a7f14bb7b641b89ae987d.camel@neukum.org> @ 2020-12-29 19:50 ` Roland Dreier 2020-12-30 11:03 ` Oliver Neukum 0 siblings, 1 reply; 18+ messages in thread From: Roland Dreier @ 2020-12-29 19:50 UTC (permalink / raw) To: Oliver Neukum; +Cc: Jakub Kicinski, Greg KH, netdev, linux-usb > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] CDC-NCM: remove "connected" log message 2020-12-29 19:50 ` Roland Dreier @ 2020-12-30 11:03 ` Oliver Neukum 2020-12-31 18:51 ` Roland Dreier 0 siblings, 1 reply; 18+ messages in thread From: Oliver Neukum @ 2020-12-30 11:03 UTC (permalink / raw) To: Roland Dreier; +Cc: Jakub Kicinski, Greg KH, netdev, linux-usb [-- 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] CDC-NCM: remove "connected" log message 2020-12-30 11:03 ` Oliver Neukum @ 2020-12-31 18:51 ` Roland Dreier 2021-01-04 14:57 ` Oliver Neukum 0 siblings, 1 reply; 18+ messages in thread From: Roland Dreier @ 2020-12-31 18:51 UTC (permalink / raw) To: Oliver Neukum; +Cc: Jakub Kicinski, Greg KH, netdev, linux-usb 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] CDC-NCM: remove "connected" log message 2020-12-31 18:51 ` Roland Dreier @ 2021-01-04 14:57 ` Oliver Neukum 2021-01-04 19:13 ` Roland Dreier 0 siblings, 1 reply; 18+ messages in thread From: Oliver Neukum @ 2021-01-04 14:57 UTC (permalink / raw) To: Roland Dreier; +Cc: Jakub Kicinski, Greg KH, netdev, linux-usb 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] CDC-NCM: remove "connected" log message 2021-01-04 14:57 ` Oliver Neukum @ 2021-01-04 19:13 ` Roland Dreier 2021-01-05 14:04 ` Oliver Neukum 0 siblings, 1 reply; 18+ messages in thread From: Roland Dreier @ 2021-01-04 19:13 UTC (permalink / raw) To: Oliver Neukum; +Cc: Jakub Kicinski, Greg KH, netdev, linux-usb > > 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] CDC-NCM: remove "connected" log message 2021-01-04 19:13 ` Roland Dreier @ 2021-01-05 14:04 ` Oliver Neukum 2021-01-06 0:19 ` Roland Dreier 0 siblings, 1 reply; 18+ messages in thread From: Oliver Neukum @ 2021-01-05 14:04 UTC (permalink / raw) To: Roland Dreier; +Cc: Jakub Kicinski, Greg KH, netdev, linux-usb 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] CDC-NCM: remove "connected" log message 2021-01-05 14:04 ` Oliver Neukum @ 2021-01-06 0:19 ` Roland Dreier 0 siblings, 0 replies; 18+ messages in thread From: Roland Dreier @ 2021-01-06 0:19 UTC (permalink / raw) To: Oliver Neukum; +Cc: Jakub Kicinski, Greg KH, netdev, linux-usb > 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-01-06 0:20 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-16 17:23 cdc_ncm kernel log spam with trendnet 2.5G USB adapter Roland Dreier 2020-11-26 12:05 ` Oliver Neukum [not found] ` <3a9b2c8c275d56d9c7904cf9b5177047b196173d.camel@neukum.org> 2020-12-01 20:42 ` Roland Dreier 2020-12-19 22:21 ` Roland Dreier 2020-12-23 2:49 ` Jakub Kicinski 2020-12-23 3:01 ` Roland Dreier 2020-12-24 3:21 ` [PATCH] CDC-NCM: remove "connected" log message Roland Dreier 2020-12-24 7:53 ` Greg KH 2020-12-28 21:30 ` Jakub Kicinski 2020-12-29 7:56 ` Roland Dreier 2020-12-29 12:30 ` Oliver Neukum [not found] ` <24c6faa2a4f91c721d9a7f14bb7b641b89ae987d.camel@neukum.org> 2020-12-29 19:50 ` Roland Dreier 2020-12-30 11:03 ` Oliver Neukum 2020-12-31 18:51 ` Roland Dreier 2021-01-04 14:57 ` Oliver Neukum 2021-01-04 19:13 ` Roland Dreier 2021-01-05 14:04 ` Oliver Neukum 2021-01-06 0:19 ` Roland Dreier
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.