Re: [PATCH] USB: core: hub_port_reset: Remove extra 40 ms reset recovery time

From: Paul Menzel
Date: Wed Jul 24 2024 - 14:15:32 EST

[Cc: -gregkh@xxxxxxx]

Dear Alan,


Thank you for your reply.

Am 24.07.24 um 16:10 schrieb Alan Stern:
On Wed, Jul 24, 2024 at 01:15:23PM +0200, Paul Menzel wrote:
This basically reverts commit b789696af8b4102b7cc26dec30c2c51ce51ee18b
("[PATCH] USB: relax usbcore reset timings") from 2005.

This adds unneeded 40 ms during resume from suspend on a majority of

Wrong. It adds 40 ms to the recovery time from a port reset -- see the
commit's title. Suspend and resume do not in general involve port
resets (although sometimes they do).

It looks like on my system the ports are reset:

```
$ grep suspend-240501-063619/hub_port_reset abreu_mem_ftrace.txt
6416.257589 | 3) kworker-9023 | | hub_port_reset [usbcore]() {
6416.387182 | 2) kworker-9023 | 129593.0 us | } /* hub_port_reset [usbcore] */
6416.387380 | 2) kworker-9023 | | hub_port_reset [usbcore]() {
6416.513458 | 3) kworker-9023 | 126078.4 us | } /* hub_port_reset [usbcore] */
6416.537813 | 2) kworker-9844 | | hub_port_reset [usbcore]() {
6416.666142 | 3) kworker-9844 | 128328.5 us | } /* hub_port_reset [usbcore] */
6416.666429 | 3) kworker-9844 | | hub_port_reset [usbcore]() {
6416.793315 | 1) kworker-9844 | 126885.9 us | } /* hub_port_reset [usbcore] */
6416.813559 | 1) kworker-9849 | | hub_port_reset [usbcore]() {
6416.941882 | 2) kworker-9849 | 128322.4 us | } /* hub_port_reset [usbcore] */
6416.942633 | 2) kworker-9849 | | hub_port_reset [usbcore]() {
6417.069205 | 3) kworker-9849 | 126572.4 us | } /* hub_port_reset [usbcore] */
```

devices, where it’s not needed, like the Dell XPS 13 9360/0596KF, BIOS
2.21.0 06/02/2022 with

The commit messages unfortunately does not list the devices needing this.
Should they surface again, these should be added to the quirk list for
USB_QUIRK_HUB_SLOW_RESET.

This quirk applies to hubs that need extra time when one of their ports
gets reset. However, it seems likely that the patch you are reverting
was meant to help the device attached to the port, not the hub itself.
Which would mean that the adding hubs to the quirk list won't help
unless every hub is added -- in which case there's no point reverting
the patch.

Furthermore, should any of these bad hubs or devices still be in use,
your change would cause them to stop working reliably. It would be a
regression.

A better approach would be to add a sysfs boolean attribute to the hub
driver to enable the 40-ms reset-recovery delay, and make it default to
True. Then people who don't need the delay could disable it from
userspace, say by a udev rule.

How would you name it?


Kind regards,

Paul


Fixes: b789696af8b4 ("[PATCH] USB: relax usbcore reset timings")
Cc: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
Cc: David Brownell <david-b@xxxxxxxxxxx>
Signed-off-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
---
drivers/usb/core/hub.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 4b93c0bd1d4b..487d5fe60f0c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3110,7 +3110,7 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
usleep_range(10000, 12000);
else {
/* TRSTRCY = 10 ms; plus some extra */
- reset_recovery_time = 10 + 40;
+ reset_recovery_time = 10;
/* Hub needs extra delay after resetting its port. */
if (hub->hdev->quirks & USB_QUIRK_HUB_SLOW_RESET)
# suspend-050124-063619 abreu mem 6.9.0-rc6-00046-g18daea77cca6
# sysinfo | man:Dell Inc. | plat:XPS 13 9360 | cpu:Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz | bios:2.21.0 | biosdate:06/02/2022 | numcpu:4 | memsz:15934724 | memfr:6179424 | os:Debian GNU/Linux trixie/sid
# command | analyze_suspend.py -f
# fwsuspend 0 fwresume 1078072
[ 6413.931060] PM: suspend entry (deep)
[ 6413.940286] Filesystems sync: 0.009 seconds
[ 6413.944886] Freezing user space processes
[ 6413.962822] Freezing user space processes completed (elapsed 0.017 seconds)
[ 6413.962860] OOM killer disabled.
[ 6413.962888] Freezing remaining freezable tasks
[ 6413.968678] Freezing remaining freezable tasks completed (elapsed 0.005 seconds)
[ 6413.968814] printk: Suspending console(s) (use no_console_suspend to debug)
[ 6413.988706] wlp58s0: deauthenticating from 88:71:b1:81:93:1b by local choice (Reason: 3=DEAUTH_LEAVING)
[ 6414.660605] PM: suspend of devices complete after 672.728 msecs
[ 6414.660635] PM: start suspend of devices complete after 691.932 msecs
[ 6414.696056] PM: late suspend of devices complete after 35.416 msecs
[ 6414.707963] ACPI: EC: interrupt blocked
[ 6414.784133] PM: noirq suspend of devices complete after 86.923 msecs
[ 6414.784175] ACPI: PM: Preparing to enter system sleep state S3
[ 6414.982414] ACPI: EC: event blocked
[ 6414.982432] ACPI: EC: EC stopped
[ 6414.982449] ACPI: PM: Saving platform NVS memory
[ 6414.984069] Disabling non-boot CPUs ...
[ 6414.986995] smpboot: CPU 1 is now offline
[ 6414.992323] smpboot: CPU 2 is now offline
[ 6414.997333] smpboot: CPU 3 is now offline
[ 6415.000365] Checking wakeup interrupts
[ 6415.000389] Calling kvm_suspend+0x0/0x40 [kvm]
[ 6415.000492] Calling intel_epb_save+0x0/0x30
[ 6415.000520] Calling mce_syscore_suspend+0x0/0x20
[ 6415.000545] Calling ledtrig_cpu_syscore_suspend+0x0/0x20
[ 6415.000574] Calling timekeeping_suspend+0x0/0x2f0
[ 6415.000740] Calling save_ioapic_entries+0x0/0xd0
[ 6415.002013] Calling i8259A_suspend+0x0/0x30
[ 6415.002046] Calling fw_suspend+0x0/0x20
[ 6415.002072] Calling acpi_save_bm_rld+0x0/0x30
[ 6415.002106] Calling lapic_suspend+0x0/0x160
[ 6415.005627] ACPI: PM: Low-level resume complete
[ 6415.005692] ACPI: EC: EC started
[ 6415.005696] ACPI: PM: Restoring platform NVS memory
[ 6415.006905] Calling init_counter_refs+0x0/0x40
[ 6415.006931] Calling lapic_resume+0x0/0x220
[ 6415.007003] Calling acpi_restore_bm_rld+0x0/0x70
[ 6415.007033] Calling irqrouter_resume+0x0/0x50
[ 6415.007056] Calling i8259A_resume+0x0/0x40
[ 6415.007250] Calling ioapic_resume+0x0/0xc0
[ 6415.007516] Calling irq_pm_syscore_resume+0x0/0x20
[ 6415.008560] Calling timekeeping_resume+0x0/0x1e0
[ 6415.008713] Timekeeping suspended for 14.141 seconds
[ 6415.008763] Calling ledtrig_cpu_syscore_resume+0x0/0x20
[ 6415.008787] Calling mce_syscore_resume+0x0/0x30
[ 6415.008830] Calling intel_epb_restore+0x0/0x90
[ 6415.008852] Calling microcode_bsp_resume+0x0/0xd0
[ 6415.008873] Calling kvm_resume+0x0/0x60 [kvm]
[ 6415.008968] PM: Triggering wakeup from IRQ 51
[ 6415.009012] Enabling non-boot CPUs ...
[ 6415.009269] smpboot: Booting Node 0 Processor 1 APIC 0x2
[ 6415.010763] CPU1 is up
[ 6415.011011] smpboot: Booting Node 0 Processor 2 APIC 0x1
[ 6415.012811] CPU2 is up
[ 6415.013058] smpboot: Booting Node 0 Processor 3 APIC 0x3
[ 6415.014687] CPU3 is up
[ 6415.020688] ACPI: PM: Waking up from system sleep state S3
[ 6416.076910] ACPI: EC: interrupt unblocked
[ 6416.106718] PM: noirq resume of devices complete after 30.451 msecs
[ 6416.108539] PM: early resume of devices complete after 1.566 msecs
[ 6416.108988] ACPI: EC: event unblocked
[ 6416.124707] nvme nvme0: 4/0/0 default/read/poll queues
[ 6416.142134] i915 0000:00:02.0: [drm] [ENCODER:94:DDI A/PHY A] is disabled/in DSI mode with an ungated DDI clock, gate it
[ 6416.172126] i915 0000:00:02.0: [drm] [ENCODER:102:DDI B/PHY B] is disabled/in DSI mode with an ungated DDI clock, gate it
[ 6416.248103] i915 0000:00:02.0: [drm] [ENCODER:113:DDI C/PHY C] is disabled/in DSI mode with an ungated DDI clock, gate it
[ 6416.387187] usb 1-3: reset full-speed USB device number 2 using xhci_hcd
[ 6416.515241] ACPI Debug: "iGfx Supported Functions Bitmap "
[ 6416.666145] usb 1-4: reset full-speed USB device number 3 using xhci_hcd
[ 6416.941885] usb 1-5: reset high-speed USB device number 4 using xhci_hcd
[ 6417.143205] PM: resume of devices complete after 1035.149 msecs
[ 6417.155389] mei_hdcp 0000:00:16.0-b638ab7e-94e2-4ea2-a552-d1c54b627f04: bound 0000:00:02.0 (ops i915_hdcp_ops [i915])
[ 6417.169468] OOM killer enabled.
[ 6417.169500] Restarting tasks ... done.
[ 6417.217430] random: crng reseeded on system resumption
[ 6417.423976] wlp58s0: authenticate with 88:71:b1:81:93:1a (local address=9c:b6:d0:d1:6a:b1)
[ 6417.424204] wlp58s0: send auth to 88:71:b1:81:93:1a (try 1/3)
[ 6417.429901] wlp58s0: authenticated
[ 6417.432975] wlp58s0: associate with 88:71:b1:81:93:1a (try 1/3)
[ 6417.475306] wlp58s0: RX AssocResp from 88:71:b1:81:93:1a (capab=0x1431 status=0 aid=1)
[ 6417.480154] wlp58s0: associated
[ 6417.556416] wlp58s0: Limiting TX power to 20 (20 - 0) dBm as advertised by 88:71:b1:81:93:1a
[ 6418.649695] PM: suspend exit