[PATCH 0/1] r8152: Add Lenovo Powered USB-C Travel Hub

[PATCH 0/1] r8152: Add Lenovo Powered USB-C Travel Hub

[Leon Schuermann]

Hi,

this patch resolves an issue with my Lenovo USB-C Hub with an
integrated Realtek USB Ethernet controller.

When suspending my host system, along with an active Ethernet link
using the Realtek USB Ethernet adapter on a somewhat busy network,
after a few minutes the NIC would start to constantly send MAC pause
frames. Presumably this is because flow control in the NIC is not
disabled while the host system suspends, which triggers the pause
frame transmission as soon as the internal buffers fill up. With
multiple Ethernet switches from different manufactures this manages to
bring large parts of my network down as soon as the laptop is in
standby.

Because of Lenovo's somewhat confusing naming scheme around their
hubs, I've taken the liberty to add their product ID to the entry,
such that others can find the device I'm referring to.

I did not find any specific git tree to base this patch on, so it is
based on Linus' latest master. If you have a more appropriate tree I
will of course rebase accordingly. Also, if this is the wrong address
to direct this patch, I'd be happy if you could point me to the right
people.

Thanks!

Leon


Leon Schuermann (1):
  r8152: Add Lenovo Powered USB-C Travel Hub

 drivers/net/usb/cdc_ether.c | 7 +++++++
 drivers/net/usb/r8152.c     | 1 +
 2 files changed, 8 insertions(+)


base-commit: f5e6c330254ae691f6d7befe61c786eb5056007e
-- 
2.29.2

[PATCH 1/1] r8152: Add Lenovo Powered USB-C Travel Hub

[Leon Schuermann]

This USB-C Hub (17ef:721e) based on the Realtek RTL8153B chip used to
work with the cdc_ether driver. 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>
---
 drivers/net/usb/cdc_ether.c | 7 +++++++
 drivers/net/usb/r8152.c     | 1 +
 2 files changed, 8 insertions(+)

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)},
-- 
2.29.2

Re: [PATCH 1/1] r8152: Add Lenovo Powered USB-C Travel Hub

[Jakub Kicinski]

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>

Re: [PATCH 1/1] r8152: Add Lenovo Powered USB-C Travel Hub

[Leon Schuermann]

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

Re: [PATCH 1/1] r8152: Add Lenovo Powered USB-C Travel Hub

[Jakub Kicinski]

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.

Re: [PATCH 1/1] r8152: Add Lenovo Powered USB-C Travel Hub

[Leon Schuermann]

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

[PATCH 1/2] r8152: Add Lenovo Powered USB-C Travel Hub

[Leon Schuermann]

This USB-C Hub (17ef:721e) based on the Realtek RTL8153B chip used to
use the cdc_ether driver. However, using this driver, with the system
suspended the device constantly sends pause-frames as soon as the
receive buffer fills up. This causes issues with other devices, where
some Ethernet switches stop forwarding 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>
---
 drivers/net/usb/cdc_ether.c | 7 +++++++
 drivers/net/usb/r8152.c     | 1 +
 2 files changed, 8 insertions(+)

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)},
-- 
2.29.2

[PATCH 2/2] r8153_ecm: Add Lenovo Powered USB-C Hub as a fallback of r8152

[Leon Schuermann]

This commit enables the use of the r8153_ecm driver, introduced with
commit c1aedf015ebdd0 ("net/usb/r8153_ecm: support ECM mode for
RTL8153") for the Lenovo Powered USB-C Hub (17ef:721e) based on the
Realtek RTL8153B chip.

This results in the following driver preference:

- if r8152 is available, use the r8152 driver
- if r8152 is not available, use the r8153_ecm driver

This is done to prevent the NIC from constantly sending pause frames
when the host system enters standby (fixed by using the r8152 driver
in "r8152: Add Lenovo Powered USB-C Travel Hub"), while still allowing
the device to work with the r8153_ecm driver as a fallback.

Signed-off-by: Leon Schuermann <leon@is.currently.online>
Tested-by: Leon Schuermann <leon@is.currently.online>
---
 drivers/net/usb/r8153_ecm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/usb/r8153_ecm.c b/drivers/net/usb/r8153_ecm.c
index 2c3fabd38b16..20b2df8d74ae 100644
--- a/drivers/net/usb/r8153_ecm.c
+++ b/drivers/net/usb/r8153_ecm.c
@@ -122,12 +122,20 @@ static const struct driver_info r8153_info = {
 };
 
 static const struct usb_device_id products[] = {
+/* Realtek RTL8153 Based USB 3.0 Ethernet Adapters */
 {
 	USB_DEVICE_AND_INTERFACE_INFO(VENDOR_ID_REALTEK, 0x8153, USB_CLASS_COMM,
 				      USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
 	.driver_info = (unsigned long)&r8153_info,
 },
 
+/* Lenovo Powered USB-C Travel Hub (4X90S92381, based on Realtek RTL8153) */
+{
+	USB_DEVICE_AND_INTERFACE_INFO(VENDOR_ID_LENOVO, 0x721e, USB_CLASS_COMM,
+				      USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
+	.driver_info = (unsigned long)&r8153_info,
+},
+
 	{ },		/* END */
 };
 MODULE_DEVICE_TABLE(usb, products);
-- 
2.29.2

Re: [PATCH 1/2] r8152: Add Lenovo Powered USB-C Travel Hub

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Mon, 11 Jan 2021 20:03:13 +0100 you wrote:
> This USB-C Hub (17ef:721e) based on the Realtek RTL8153B chip used to
> use the cdc_ether driver. However, using this driver, with the system
> suspended the device constantly sends pause-frames as soon as the
> receive buffer fills up. This causes issues with other devices, where
> some Ethernet switches stop forwarding packets altogether.
> 
> Using the Realtek driver (r8152) fixes this issue. Pause frames are no
> longer sent while the host system is suspended.
> 
> [...]

Here is the summary with links:
  - [1/2] r8152: Add Lenovo Powered USB-C Travel Hub
    https://git.kernel.org/netdev/net/c/cb82a54904a9
  - [2/2] r8153_ecm: Add Lenovo Powered USB-C Hub as a fallback of r8152
    https://git.kernel.org/netdev/net/c/2284bbd0cf39

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html