| Message ID | 20210108202727.11728-2-leon@is.currently.online (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | r8152: Add Lenovo Powered USB-C Travel Hub | 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 | 6 maintainers not CCed: kai.heng.feng@canonical.com lee.jones@linaro.org hkallweit1@gmail.com treding@nvidia.com kuba@kernel.org gustavoars@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 | warning | CHECK: Alignment should match open parenthesis |
| 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 Fri, 8 Jan 2021 21:27:27 +0100 Leon Schuermann wrote: > This USB-C Hub (17ef:721e) based on the Realtek RTL8153B chip used to > work with the cdc_ether driver. When you say "used to work" do you mean there was a regression where the older kernels would work fine and newer don't? Or just "it works most of the time"? > However, using this driver, with the > system suspended the device sends pause-frames as soon as the receive > buffer fills up. This produced substantial network load, up to the > point where some Ethernet switches stopped processing packets > altogether. > > Using the Realtek driver (r8152) fixes this issue. Pause frames are no > longer sent while the host system is suspended. > > Signed-off-by: Leon Schuermann <leon@is.currently.online> > Tested-by: Leon Schuermann <leon@is.currently.online>
Jakub Kicinski <kuba@kernel.org> writes: > On Fri, 8 Jan 2021 21:27:27 +0100 Leon Schuermann wrote: >> This USB-C Hub (17ef:721e) based on the Realtek RTL8153B chip used to >> work with the cdc_ether driver. > > When you say "used to work" do you mean there was a regression where > the older kernels would work fine and newer don't? Or just "it works > most of the time"? Sorry, I should've clarified that. "Used to work" is supposed to say "the device used the generic cdc_ether driver", as in [ +0.000004] usb 4-1.1: Product: Lenovo Powered Hub [ +0.000003] usb 4-1.1: Manufacturer: Lenovo [ +0.000002] usb 4-1.1: SerialNumber: xxxxxxxxx [ +0.024803] cdc_ether 4-1.1:2.0 eth0: register 'cdc_ether' at usb-0000:2f:00.0-1.1, CDC Ethernet Device, xx:xx:xx:xx:xx:xx I guess it did technically work correctly, except for the reported issue when the host system suspends, which is fixed by using the dedicated Realtek driver. As far as I know this hasn't been fixed before, so it's not a regression. Should I update the commit message accordingly? Thanks! Leon
On Sat, 09 Jan 2021 10:39:27 +0100 Leon Schuermann wrote: > Jakub Kicinski <kuba@kernel.org> writes: > > On Fri, 8 Jan 2021 21:27:27 +0100 Leon Schuermann wrote: > >> This USB-C Hub (17ef:721e) based on the Realtek RTL8153B chip used to > >> work with the cdc_ether driver. > > > > When you say "used to work" do you mean there was a regression where > > the older kernels would work fine and newer don't? Or just "it works > > most of the time"? > > Sorry, I should've clarified that. "Used to work" is supposed to say > "the device used the generic cdc_ether driver", as in > > [ +0.000004] usb 4-1.1: Product: Lenovo Powered Hub > [ +0.000003] usb 4-1.1: Manufacturer: Lenovo > [ +0.000002] usb 4-1.1: SerialNumber: xxxxxxxxx > [ +0.024803] cdc_ether 4-1.1:2.0 eth0: register 'cdc_ether' at > usb-0000:2f:00.0-1.1, CDC Ethernet Device, > xx:xx:xx:xx:xx:xx > > I guess it did technically work correctly, except for the reported issue > when the host system suspends, which is fixed by using the dedicated > Realtek driver. As far as I know this hasn't been fixed before, so it's > not a regression. I see. In the last release cycle there were patches for allowing cdc_ether to drive RTL8153 devices when r8152 is not available. I wanted to double check with you that nothing changed here, that's to say that the cdc_ether is not used even if r8152 is built after an upgrade to 5.11-rc. > Should I update the commit message accordingly? Thanks! Yes please, otherwise backporters may be confused about how to classify this change.
Jakub Kicinski <kuba@kernel.org> writes: > On Sat, 09 Jan 2021 10:39:27 +0100 Leon Schuermann wrote: >> Jakub Kicinski <kuba@kernel.org> writes: >> > On Fri, 8 Jan 2021 21:27:27 +0100 Leon Schuermann wrote: >> >> This USB-C Hub (17ef:721e) based on the Realtek RTL8153B chip used to >> >> work with the cdc_ether driver. >> > >> > When you say "used to work" do you mean there was a regression where >> > the older kernels would work fine and newer don't? Or just "it works >> > most of the time"? >> >> Sorry, I should've clarified that. "Used to work" is supposed to say >> "the device used the generic cdc_ether driver", as in >> >> [ +0.000004] usb 4-1.1: Product: Lenovo Powered Hub >> [ +0.000003] usb 4-1.1: Manufacturer: Lenovo >> [ +0.000002] usb 4-1.1: SerialNumber: xxxxxxxxx >> [ +0.024803] cdc_ether 4-1.1:2.0 eth0: register 'cdc_ether' at >> usb-0000:2f:00.0-1.1, CDC Ethernet Device, >> xx:xx:xx:xx:xx:xx >> >> I guess it did technically work correctly, except for the reported issue >> when the host system suspends, which is fixed by using the dedicated >> Realtek driver. As far as I know this hasn't been fixed before, so it's >> not a regression. > > I see. In the last release cycle there were patches for allowing > cdc_ether to drive RTL8153 devices when r8152 is not available. > I wanted to double check with you that nothing changed here, > that's to say that the cdc_ether is not used even if r8152 is > built after an upgrade to 5.11-rc. Thanks for the info, I didn't notice that. I can confirm that `cdc_ether` (for this specific USB-C Hub) is used prior and after the patches introducing r8153_ecm. However, the r8153_ecm driver resolves the issue of my first patch, being unable to use the device without r8152 available. To enable a fallback onto this driver I added a second commit, because my device uses a different VID/PID combination compared to the default Realtek VID/PID on which the r8153_ecm currently matches. I've tested the first commit standalone (r8152: Add Lenovo...), both commits (r8153_ecm: Add Le...), as well as two vanilla kernel versions, each with and without the r8152 driver available, with the following results: | | CONFIG_USB_RTL8152 | !(CONFIG_USB_RTL8152) | |----------------------+--------------------+-----------------------| | r8153_ecm: Add Le... | `r8152` used | `r8153_ecm` used | | r8152: Add Lenovo... | `r8152` used | No matching driver | | 5.11.0-rc3 | `cdc_ether` used | `cdc_ether` used | | 5.10.3 | `cdc_ether` used | `cdc_ether` used | Unfortunately, r8153_ecm has the same issue with regards to pause frames during host system suspend as does cdc_ether and potentially requires some special handling (if that is even possible in ECM mode). That is outside of the scope of this patchset though. Nonetheless, I do believe that the option of using r8152 if it is available and falling back to r8153_ecm (applying both patches) is the most appropriate, as it is unlikely to break anyone's hardware while still fixing my issue. I suppose that in theory, it might make sense to add all devices listed as using the RTL8153 in cdc_ether.c to the products of r8153_ecm, as none of them will currently work without r5182. I can't test them though, so I'm not sure whether that's a good idea. This patch therefore only resolves the issue for my specific USB-C Hub. >> Should I update the commit message accordingly? Thanks! > > Yes please, otherwise backporters may be confused about how > to classify this change. I've updated the commit message. Let me know what you think. Thanks! Leon Leon Schuermann (2): r8152: Add Lenovo Powered USB-C Travel Hub r8153_ecm: Add Lenovo Powered USB-C Hub as a fallback of r8152 drivers/net/usb/cdc_ether.c | 7 +++++++ drivers/net/usb/r8152.c | 1 + drivers/net/usb/r8153_ecm.c | 8 ++++++++ 3 files changed, 16 insertions(+) base-commit: 7c53f6b671f4aba70ff15e1b05148b10d58c2837
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c index 8c1d61c2cbac..6aaa0675c28a 100644 --- a/drivers/net/usb/cdc_ether.c +++ b/drivers/net/usb/cdc_ether.c @@ -793,6 +793,13 @@ static const struct usb_device_id products[] = { .driver_info = 0, }, +/* Lenovo Powered USB-C Travel Hub (4X90S92381, based on Realtek RTL8153) */ +{ + USB_DEVICE_AND_INTERFACE_INFO(LENOVO_VENDOR_ID, 0x721e, USB_CLASS_COMM, + USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE), + .driver_info = 0, +}, + /* ThinkPad USB-C Dock Gen 2 (based on Realtek RTL8153) */ { USB_DEVICE_AND_INTERFACE_INFO(LENOVO_VENDOR_ID, 0xa387, USB_CLASS_COMM, diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index c448d6089821..67cd6986634f 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -6877,6 +6877,7 @@ static const struct usb_device_id rtl8152_table[] = { {REALTEK_USB_DEVICE(VENDOR_ID_LENOVO, 0x7205)}, {REALTEK_USB_DEVICE(VENDOR_ID_LENOVO, 0x720c)}, {REALTEK_USB_DEVICE(VENDOR_ID_LENOVO, 0x7214)}, + {REALTEK_USB_DEVICE(VENDOR_ID_LENOVO, 0x721e)}, {REALTEK_USB_DEVICE(VENDOR_ID_LENOVO, 0xa387)}, {REALTEK_USB_DEVICE(VENDOR_ID_LINKSYS, 0x0041)}, {REALTEK_USB_DEVICE(VENDOR_ID_NVIDIA, 0x09ff)},