[PATCH] usb: add quirks for Lenovo OneLink+ Dock

[PATCH] usb: add quirks for Lenovo OneLink+ Dock

[JFLF]

The Lenovo OneLink+ Dock contains two VL812 USB3.0 controllers:
17ef:1018 upstream
17ef:1019 downstream

Those two controllers both have problems with some USB3.0 devices,
particularly self-powered ones. Typical error messages include:

  Timeout while waiting for setup device command
  device not accepting address X, error -62
  unable to enumerate USB device

By process of elimination the controllers themselves were identified as
the cause of the problem. Through trial and error the issue was solved
by using USB_QUIRK_RESET_RESUME for both chips.

Signed-off-by: JFLF <jflf_kernel@gmx.com>
---
 drivers/usb/core/quirks.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index f99a65a64..999b7c969 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -437,6 +437,10 @@ static const struct usb_device_id usb_quirk_list[] = {
 	{ USB_DEVICE(0x1532, 0x0116), .driver_info =
 			USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL },

+	/* Lenovo ThinkPad OneLink+ Dock twin hub controllers (VIA Labs VL812) */
+	{ USB_DEVICE(0x17ef, 0x1018), .driver_info = USB_QUIRK_RESET_RESUME },
+	{ USB_DEVICE(0x17ef, 0x1019), .driver_info = USB_QUIRK_RESET_RESUME },
+
 	/* Lenovo USB-C to Ethernet Adapter RTL8153-04 */
 	{ USB_DEVICE(0x17ef, 0x720c), .driver_info = USB_QUIRK_NO_LPM },

--
2.34.1

Re: [PATCH] usb: add quirks for Lenovo OneLink+ Dock

[Greg KH]

On Wed, Aug 24, 2022 at 06:09:46PM +0200, JFLF wrote:
> The Lenovo OneLink+ Dock contains two VL812 USB3.0 controllers:
> 17ef:1018 upstream
> 17ef:1019 downstream
> 
> Those two controllers both have problems with some USB3.0 devices,
> particularly self-powered ones. Typical error messages include:
> 
>   Timeout while waiting for setup device command
>   device not accepting address X, error -62
>   unable to enumerate USB device
> 
> By process of elimination the controllers themselves were identified as
> the cause of the problem. Through trial and error the issue was solved
> by using USB_QUIRK_RESET_RESUME for both chips.
> 
> Signed-off-by: JFLF <jflf_kernel@gmx.com>

We need a real name, sorry, please use what you sign documents with.

thanks,

greg k-h

Re: [PATCH] usb: add quirks for Lenovo OneLink+ Dock

[Oliver Neukum]

On 24.08.22 18:09, JFLF wrote:

> By process of elimination the controllers themselves were identified as
> the cause of the problem. Through trial and error the issue was solved
> by using USB_QUIRK_RESET_RESUME for both chips.

Hi,


aside from the aspects of getting this properly signed off and merged,
this opens up a question. What does resetting a hub do to its children?
That is if the request to wake up comes from a child, do we

a) lose state in the child?
b) retain the knowledge which port requested the wakeup?

How far has this patch been tested?

	Regards
		Oliver

Re: [PATCH] usb: add quirks for Lenovo OneLink+ Dock

[jflf_kernel]

On 30/08/2022 13.54, Oliver Neukum wrote:
> On 24.08.22 18:09, JFLF wrote:
>
>> By process of elimination the controllers themselves were identified as
>> the cause of the problem. Through trial and error the issue was solved
>> by using USB_QUIRK_RESET_RESUME for both chips.
>
> Hi,
>
>
> aside from the aspects of getting this properly signed off and merged,
> this opens up a question. What does resetting a hub do to its children?
> That is if the request to wake up comes from a child, do we
>
> a) lose state in the child?
> b) retain the knowledge which port requested the wakeup?
>
> How far has this patch been tested?
>
> 	Regards
> 		Oliver


Hi Oliver,

Partial answer for now: I have been using those quirks via the kernel command line for about a year now. I have been meaning to send in the patch long ago, but kept forgetting about it.

I agree that USB_QUIRK_RESET_RESUME seems fishy with a hub. It's pretty much the last quirk I tried, and the only one that worked. I can't say I understand what it does exactly. The hubs themselves don't seem to reset (or at least not fully), as there is no re-enumeration of existing children.

I have not experienced a single problem or side effect since using those quirks. I use a mix of USB 2.0 and 3.0 devices, some bus- and some self-powered, some permanently connected (including ethernet and audio in the hub itself) and some not.

Thanks!
JF

Re: [PATCH] usb: add quirks for Lenovo OneLink+ Dock

[Oliver Neukum]

On 30.08.22 15:56, jflf_kernel@gmx.com wrote:

Hi,

> I agree that USB_QUIRK_RESET_RESUME seems fishy with a hub. It's pretty much the last quirk I tried, and the only one that worked. I can't say I understand what it does exactly.

Two things

1) force a reset after a resume and call reset_resume() instead of resume()
2) block autosuspend if remote wakeup is required

I suspect you are actually using the second effect. Have you
tested with "usbcore.autosuspend=-1" on the kernel command line.

 The hubs themselves don't seem to reset (or at least not fully), as
there is no re-enumeration of existing children.

Probably because they do not suspend at all. Generally hubs do need
remote wakeup lest they forget the port whose activity woke the system.

> I have not experienced a single problem or side effect since using those quirks. I use a mix of USB 2.0 and 3.0 devices, some bus- and some self-powered, some permanently connected (including ethernet and audio in the hub itself) and some not.

The interesting test would be to see whether a keyboard, ethernet
or mouse can wake your system from suspend. Have you tested WOL?

	Regards
		Oliver

Re: [PATCH] usb: add quirks for Lenovo OneLink+ Dock

[jflf_kernel]

On 30/08/2022 16.47, Oliver Neukum wrote:

> 1) force a reset after a resume and call reset_resume() instead of resume()
> 2) block autosuspend if remote wakeup is required
>
> I suspect you are actually using the second effect. Have you
> tested with "usbcore.autosuspend=-1" on the kernel command line.

After further testing, your suspicion is correct.

TL;DR: the two VL812 hubs don't behave well when suspended.

I'd like to prepare a better patch for that issue. What's the recommended strategy? The current patch works, even if only as a side effect and when there's a wakeup source downstream. It's currently in Greg KH's usb-linus branch, and will land in linux-next at some point. I'm tempted to let it be and undo it later in the better patch. Is that acceptable? Or should I ask Greg KH to pull it?


> The interesting test would be to see whether a keyboard, ethernet
> or mouse can wake your system from suspend. Have you tested WOL?

Right, a bit of background to understand what's going on with that dock.

There are actually three USB hubs in the dock: the two VL812, and a third Genesys Logic USB 2.0 only:

    $ lsusb -d 17ef:
    Bus 002 Device 005: ID 17ef:3054 Lenovo OneLink+ Giga
    Bus 002 Device 004: ID 17ef:1019 Lenovo USB3.0 Hub
    Bus 002 Device 002: ID 17ef:1018 Lenovo USB3.0 Hub
    Bus 001 Device 011: ID 17ef:3055 Lenovo ThinkPad OneLink Plus Dock Audio
    Bus 001 Device 006: ID 17ef:1019 Lenovo USB2.0 Hub
    Bus 001 Device 004: ID 17ef:1018 Lenovo USB2.0 Hub

    $ lsusb -d 05e3:
    Bus 001 Device 007: ID 05e3:0608 Genesys Logic, Inc. Hub


    $ lsusb -tv | grep -B1 -e 17ef: -e 05e3:
        |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M
            ID 17ef:1018 Lenovo
            |__ Port 1: Dev 4, If 0, Class=Hub, Driver=hub/4p, 5000M
                ID 17ef:1019 Lenovo
                |__ Port 3: Dev 5, If 1, Class=CDC Data, Driver=cdc_ether, 5000M
                    ID 17ef:3054 Lenovo
                |__ Port 3: Dev 5, If 0, Class=Communications, Driver=cdc_ether, 5000M
                    ID 17ef:3054 Lenovo
    --
        |__ Port 6: Dev 4, If 0, Class=Hub, Driver=hub/4p, 480M
            ID 17ef:1018 Lenovo
    --
            |__ Port 2: Dev 7, If 0, Class=Hub, Driver=hub/2p, 480M
                ID 05e3:0608 Genesys Logic, Inc. Hub
    --
            |__ Port 1: Dev 6, If 0, Class=Hub, Driver=hub/4p, 480M
                ID 17ef:1019 Lenovo
                |__ Port 4: Dev 8, If 1, Class=Audio, Driver=snd-usb-audio, 12M
                    ID 17ef:3055 Lenovo
                |__ Port 4: Dev 8, If 2, Class=Audio, Driver=snd-usb-audio, 12M
                    ID 17ef:3055 Lenovo
                |__ Port 4: Dev 8, If 0, Class=Audio, Driver=snd-usb-audio, 12M
                    ID 17ef:3055 Lenovo
                |__ Port 4: Dev 8, If 3, Class=Human Interface Device, Driver=usbhid, 12M
                    ID 17ef:3055 Lenovo


The user-friendly port view (front and back refer to the locations of USB-A ports):

    1018  [p1] -->  1019  [p1] -->  USB3 back 4
       |               |  [p2] -->  USB3 back 3
       |               |  [p3] -->  USB3 RTL8153 ethernet
       |               |  [p4] -->  USB2 CM6533 audio
       |
       |  [p2] -->  GL850S  [p1] --> USB2 back 2
       |                 |  [p2] --> USB2 back 1 "keyboard/mouse"
       |
       |  [p3] -->  USB3 front 1 "power"
       |  [p4] -->  USB3 front 2


I have always kept keyboard and mouse on the USB2 ports connected to the Genesys Logic hub. They work there without any problem.

With WoL enabled in the BIOS, there are only two wakeup sources:

    $ grep . /sys/bus/usb/devices/*/power/wakeup | grep enabled
    /sys/bus/usb/devices/1-6.2.2/power/wakeup:enabled       # keyboard on GL850S
    /sys/bus/usb/devices/2-1.1.3/power/wakeup:enabled       # net

While suspended I can wake up the laptop with the keyboard and everything works.

Moving the keyboard to one of the VL812 ports moves the wakeup source::

    $ grep . /sys/bus/usb/devices/*/power/wakeup | grep enabled
    /sys/bus/usb/devices/1-6.1.1/power/wakeup:enabled       # keyboard on the :1019 VL812 (USB2)
    /sys/bus/usb/devices/2-1.1.3/power/wakeup:enabled       # net


Now here's the interesting bit. Without "usbcore.autosuspend=-1" I can still wakeup the laptop from suspend, but after waking up the keyboard is gone:

    kernel: hub 1-6.1:1.0: hub_ext_port_status failed (err = -71)
    kernel: hub 1-6:1.0: hub_ext_port_status failed (err = -71)
    kernel: hub 1-6.1:1.0: hub_ext_port_status failed (err = -71)
    kernel: hub 1-6.1:1.0: hub_ext_port_status failed (err = -71)
    kernel: usb 1-6.1-port4: cannot disable (err = -71)
    kernel: usb 1-6.1-port1: cannot disable (err = -71)

After that the port to which the keyboard is connected is essentially dead. Replugging doesn't help, I need to shutdown the laptop (powered via the dock) and cut all power to the dock to get that port back to operational status.

With "usbcore.autosuspend=-1", everything behaves as it should.

I met the exact same behaviour ("dead" port) with USB 3.0 devices too, although in my experience so far only with bus-powered devices. Sample kernel output:

    kernel: xhci_hcd 0000:00:14.0: Timeout while waiting for setup device command
    kernel: xhci_hcd 0000:00:14.0: Timeout while waiting for setup device command
    kernel: usb 2-1.4: device not accepting address 6, error -62
    kernel: xhci_hcd 0000:00:14.0: Timeout while waiting for setup device command
    kernel: xhci_hcd 0000:00:14.0: Timeout while waiting for setup device command
    kernel: usb 2-1.4: device not accepting address 7, error -62
    kernel: usb 2-1-port4: attempt power cycle
    kernel: xhci_hcd 0000:00:14.0: Timeout while waiting for setup device command
    kernel: xhci_hcd 0000:00:14.0: Timeout while waiting for setup device command
    kernel: usb 2-1.4: device not accepting address 8, error -62
    kernel: xhci_hcd 0000:00:14.0: Timeout while waiting for setup device command
    kernel: xhci_hcd 0000:00:14.0: Timeout while waiting for setup device command
    kernel: usb 2-1.4: device not accepting address 9, error -62
    kernel: usb 2-1-port4: unable to enumerate USB device


I'll run some more tests out of curiosity to see how things behave in corner cases.

Thanks!
JF

Re: [PATCH] usb: add quirks for Lenovo OneLink+ Dock

[Greg KH]

On Tue, Aug 30, 2022 at 09:50:03PM +0200, jflf_kernel@gmx.com wrote:
> 
> On 30/08/2022 16.47, Oliver Neukum wrote:
> 
> > 1) force a reset after a resume and call reset_resume() instead of resume()
> > 2) block autosuspend if remote wakeup is required
> >
> > I suspect you are actually using the second effect. Have you
> > tested with "usbcore.autosuspend=-1" on the kernel command line.
> 
> After further testing, your suspicion is correct.
> 
> TL;DR: the two VL812 hubs don't behave well when suspended.
> 
> I'd like to prepare a better patch for that issue. What's the recommended strategy? The current patch works, even if only as a side effect and when there's a wakeup source downstream. It's currently in Greg KH's usb-linus branch, and will land in linux-next at some point. I'm tempted to let it be and undo it later in the better patch. Is that acceptable? Or should I ask Greg KH to pull it?

I can revert it if you want me to, just let me know.

thanks,

greg k-h

Re: [PATCH] usb: add quirks for Lenovo OneLink+ Dock

[jflf_kernel]

On 31/08/2022 09.31, Greg KH wrote:
> On Tue, Aug 30, 2022 at 09:50:03PM +0200, jflf_kernel@gmx.com wrote:
>>
>> On 30/08/2022 16.47, Oliver Neukum wrote:
>>
>>> 1) force a reset after a resume and call reset_resume() instead of resume()
>>> 2) block autosuspend if remote wakeup is required
>>>
>>> I suspect you are actually using the second effect. Have you
>>> tested with "usbcore.autosuspend=-1" on the kernel command line.
>>
>> After further testing, your suspicion is correct.
>>
>> TL;DR: the two VL812 hubs don't behave well when suspended.
>>
>> I'd like to prepare a better patch for that issue. What's the recommended strategy? The current patch works, even if only as a side effect and when there's a wakeup source downstream. It's currently in Greg KH's usb-linus branch, and will land in linux-next at some point. I'm tempted to let it be and undo it later in the better patch. Is that acceptable? Or should I ask Greg KH to pull it?
>
> I can revert it if you want me to, just let me know.
>
> thanks,
>
> greg k-h

[keeping the lists in CC this time]

Please revert if possible, and apologies for the trouble.

Thanks!
JF

Re: [PATCH] usb: add quirks for Lenovo OneLink+ Dock

[Greg KH]

On Wed, Aug 31, 2022 at 09:43:02AM +0200, jflf_kernel@gmx.com wrote:
> 
> On 31/08/2022 09.31, Greg KH wrote:
> > On Tue, Aug 30, 2022 at 09:50:03PM +0200, jflf_kernel@gmx.com wrote:
> >>
> >> On 30/08/2022 16.47, Oliver Neukum wrote:
> >>
> >>> 1) force a reset after a resume and call reset_resume() instead of resume()
> >>> 2) block autosuspend if remote wakeup is required
> >>>
> >>> I suspect you are actually using the second effect. Have you
> >>> tested with "usbcore.autosuspend=-1" on the kernel command line.
> >>
> >> After further testing, your suspicion is correct.
> >>
> >> TL;DR: the two VL812 hubs don't behave well when suspended.
> >>
> >> I'd like to prepare a better patch for that issue. What's the recommended strategy? The current patch works, even if only as a side effect and when there's a wakeup source downstream. It's currently in Greg KH's usb-linus branch, and will land in linux-next at some point. I'm tempted to let it be and undo it later in the better patch. Is that acceptable? Or should I ask Greg KH to pull it?
> >
> > I can revert it if you want me to, just let me know.
> >
> > thanks,
> >
> > greg k-h
> 
> [keeping the lists in CC this time]
> 
> Please revert if possible, and apologies for the trouble.

No problem at all, now reverted.

greg k-h

Re: [PATCH] usb: add quirks for Lenovo OneLink+ Dock

[Oliver Neukum]

On 30.08.22 21:50, jflf_kernel@gmx.com wrote:

Hi,

> TL;DR: the two VL812 hubs don't behave well when suspended.

We need to know the exact nature of their transgressions to inflict
the correct correctional measures.

> I have always kept keyboard and mouse on the USB2 ports connected to the Genesys Logic hub. They work there without any problem.

So they work even without your patch? We should make sure we are
absolutely clear on details.

> With WoL enabled in the BIOS, there are only two wakeup sources:
> 
>     $ grep . /sys/bus/usb/devices/*/power/wakeup | grep enabled
>     /sys/bus/usb/devices/1-6.2.2/power/wakeup:enabled       # keyboard on GL850S
>     /sys/bus/usb/devices/2-1.1.3/power/wakeup:enabled       # net
> 
> While suspended I can wake up the laptop with the keyboard and everything works.

Again, with or without RESET_RESUME?

> Moving the keyboard to one of the VL812 ports moves the wakeup source::
> 
>     $ grep . /sys/bus/usb/devices/*/power/wakeup | grep enabled
>     /sys/bus/usb/devices/1-6.1.1/power/wakeup:enabled       # keyboard on the :1019 VL812 (USB2)
>     /sys/bus/usb/devices/2-1.1.3/power/wakeup:enabled       # net
> 
> 
> Now here's the interesting bit. Without "usbcore.autosuspend=-1" I can still wakeup the laptop from suspend, but after waking up the keyboard is gone:
> 
>     kernel: hub 1-6.1:1.0: hub_ext_port_status failed (err = -71)
>     kernel: hub 1-6:1.0: hub_ext_port_status failed (err = -71)
>     kernel: hub 1-6.1:1.0: hub_ext_port_status failed (err = -71)
>     kernel: hub 1-6.1:1.0: hub_ext_port_status failed (err = -71)
>     kernel: usb 1-6.1-port4: cannot disable (err = -71)
>     kernel: usb 1-6.1-port1: cannot disable (err = -71)
> 
> After that the port to which the keyboard is connected is essentially dead. Replugging doesn't help, I need to shutdown the laptop (powered via the dock) and cut all power to the dock to get that port back to operational status.
> 
> With "usbcore.autosuspend=-1", everything behaves as it should.

Do the keyboard and the hub suspend? Is this with your patch or without?

> I'll run some more tests out of curiosity to see how things behave in corner cases.

While you do so, which is a good idea, could you restate your current
results in a more precise way? We should have 4 results for each hub.
It is not the case that RESET_RESUME has no effect if you give
usbcore.autosuspend=-1 if the system has been suspended. Hence we have
the cases of

no RESET_RESUME/default autosuspend
no RESET_RESUME/autosuspend=-1
with RESET_RESUME/default autosuspend
with RESET_RESUME/autosuspend=-1

	Regards
		Oliver

Re: [PATCH] usb: add quirks for Lenovo OneLink+ Dock

[jflf_kernel]

On 31/08/2022 11.16, Oliver Neukum wrote:

>> I'll run some more tests out of curiosity to see how things behave in corner cases.
>
> While you do so, which is a good idea, could you restate your current
> results in a more precise way? We should have 4 results for each hub.
> It is not the case that RESET_RESUME has no effect if you give
> usbcore.autosuspend=-1 if the system has been suspended. Hence we have
> the cases of
>
> no RESET_RESUME/default autosuspend
> no RESET_RESUME/autosuspend=-1
> with RESET_RESUME/default autosuspend
> with RESET_RESUME/autosuspend=-1


Hi Oliver,

Apologies for my last unclear message.

Both VL812 hubs behave the same way so I have a single table for both. The settings are applied to both hubs, on both USB2 and USB3.

I have separated USB2 and USB3 as they behave slightly differently.


=======================================================================================
Settings                        USB2 hotplug    USB3 hotplug    State after waking up
---------------------------------------------------------------------------------------

power/control=auto              works (1)       fails           broken (2)

usbcore.autosuspend=-1          works           works           broken (2)
OR power/control=on

power/control=auto              works (3)       works (3)       works
and USB_QUIRK_RESET_RESUME

power/control=on                works           works           works
and USB_QUIRK_RESET_RESUME

HUB_QUIRK_DISABLE_AUTOSUSPEND   works           works           works
and USB_QUIRK_RESET_RESUME

=======================================================================================


(1) I have yet to find a device that would be rejected by the USB2 side, while I have reliable reproducers for USB3. The problem appears with specific devices only, which otherwise work flawlessly on other hubs, other systems, etc.

(2) After a system wake-up the hubs are in a seemingly random state. Numerous errors appear in the syslog, and some ports may be dead (changes from wake-up to wake-up). This can affect both internal (to dock built-in devices) and external hub ports.

(3) With RESET_RESUME the hubs (both USB2 and USB3) stay active and don't autosuspend, even when power/control==auto.


In all situations above a keyboard and the integrated ethernet controller can wake up the laptop from suspend. In situations (2) they may not work anymore after waking up.


The bottom line is: we need USB_QUIRK_RESET_RESUME to deal with the spurious hub state after waking up, and we need to prevent the hubs from autosuspending to work around the USB3 issue. RESET_RESUME does all of that right now, but can its behaviour be considered stable? Is there any chance that some day it would no longer block autosuspend?

If RESET_RESUME is stable, then my current patch is enough and it can be re-merged. If not, I have an updated patch ready with HUB_QUIRK_DISABLE_AUTOSUSPEND.


Also, RESET_RESUME keeps the device nodes active, but not the hub interface nodes. HUB_QUIRK_DISABLE_AUTOSUSPEND and usbcore.autosuspend=-1 keep both of them active:

    $ grep active /sys/bus/usb/devices/2*/power/runtime_status    # USB_QUIRK_RESET_RESUME
    /sys/bus/usb/devices/2-1.1/power/runtime_status:active
    /sys/bus/usb/devices/2-1/power/runtime_status:active

    $ grep active /sys/bus/usb/devices/2*/power/runtime_status    # HUB_QUIRK_DISABLE_AUTOSUSPEND
    /sys/bus/usb/devices/2-1:1.0/power/runtime_status:active
    /sys/bus/usb/devices/2-1.1:1.0/power/runtime_status:active
    /sys/bus/usb/devices/2-1.1/power/runtime_status:active
    /sys/bus/usb/devices/2-1/power/runtime_status:active

I do not know what impact this could have. In my tests both accepted the hotplugged USB3 devices, so keeping the device nodes active is enough.


Thanks!
JF