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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20210111190312.12589-1-leon@is.currently.online>
Date:   Mon, 11 Jan 2021 20:03:11 +0100
From:   Leon Schuermann <leon@...currently.online>
To:     kuba@...nel.org, oliver@...kum.org, davem@...emloft.net
Cc:     hayeswang@...ltek.com, linux-usb@...r.kernel.org,
        netdev@...r.kernel.org, Leon Schuermann <leon@...currently.online>
Subject: Re: [PATCH 1/1] r8152: Add Lenovo Powered USB-C Travel Hub

Jakub Kicinski <kuba@...nel.org> writes:
> On Sat, 09 Jan 2021 10:39:27 +0100 Leon Schuermann wrote:
>> Jakub Kicinski <kuba@...nel.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