Jakub Kicinski [off-list ref] writes:

On Sat, 09 Jan 2021 10:39:27 +0100 Leon Schuermann wrote:

quoted

Jakub Kicinski [off-list ref] writes:

quoted

On Fri,  8 Jan 2021 21:27:27 +0100 Leon Schuermann wrote:  

quoted

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.

quoted

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