Re: [PATCH 9/9] usb: xhci: optimize resuming from S4 (suspend-to-disk)

[Michal Pecio <michal.pecio@gmail.com>]

On Fri, 27 Mar 2026 13:34:40 +0100, Niklas Neronin wrote:
> On resume from S4 (power loss after suspend/hibernation), the xHCI
> driver previously freed, reallocated, and fully reinitialized all
> data structures. Most of this is unnecessary because the data is
> restored from a saved image; only the xHCI registers lose their
> values.

... and all runtime information must be wiped from data structures.
That's the tricky part here.

> 
> This patch optimizes S4 resume by performing only a host controller
> reset, which includes:
> * Freeing or clearing runtime-created data.
> * Rewriting xHCI registers.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci-mem.c |  4 +--
>  drivers/usb/host/xhci.c     | 53 ++++++++++++++++++++++---------------
>  drivers/usb/host/xhci.h     |  2 ++
>  3 files changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index f1b4f06d4b8b..4156822eb000 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -936,7 +936,7 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
>   * that tt_info, then free the child first. Recursive.
>   * We can't rely on udev at this point to find child-parent relationships.
>   */
> -static void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
> +void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
>  {
>  	struct xhci_virt_device *vdev;
>  	struct list_head *tt_list_head;
> @@ -1905,7 +1905,7 @@ void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrup
>  EXPORT_SYMBOL_GPL(xhci_remove_secondary_interrupter);
>  
>  /* Cleanup roothub bandwidth data */
> -static void xhci_rh_bw_cleanup(struct xhci_hcd *xhci)
> +void xhci_rh_bw_cleanup(struct xhci_hcd *xhci)
>  {
>  	struct xhci_root_port_bw_info *rh_bw;
>  	struct xhci_tt_bw_info *tt_info, *tt_next;
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 232e6143ac4b..8fb2b91fc0cc 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1082,9 +1082,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  {
>  	u32			command, temp = 0;
>  	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
> +	struct xhci_segment	*seg;
>  	int			retval = 0;
>  	bool			pending_portevent = false;
>  	bool			suspended_usb3_devs = false;
> +	bool			reset_registers = false;
>  
>  	if (!hcd->state)
>  		return 0;
> @@ -1103,10 +1105,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  
>  	spin_lock_irq(&xhci->lock);
>  
> -	if (xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
> -		power_lost = true;
> -
> -	if (!power_lost) {
> +	if (power_lost || xhci->broken_suspend || xhci->quirks & XHCI_RESET_ON_RESUME) {
> +		xhci_dbg(xhci, "HC state lost, performing host controller reset\n");
> +		reset_registers = true;
> +	} else {
> +		xhci_dbg(xhci, "HC state intact, continuing without reset\n");

Maybe not a bad idea to log this function, but it could simply print
the value of reset_registers instead of two separate xhci_dbg().
And maybe also print power_lost and broken_suspend separately.

>  		/*
>  		 * Some controllers might lose power during suspend, so wait
>  		 * for controller not ready bit to clear, just as in xHC init.
> @@ -1144,11 +1147,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  		temp = readl(&xhci->op_regs->status);
>  		if ((temp & (STS_SRE | STS_HCE)) && !(xhci->xhc_state & XHCI_STATE_REMOVING)) {
>  			xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
> -			power_lost = true;
> +			reset_registers = true;
>  		}
>  	}
>  
> -	if (power_lost) {
> +	if (reset_registers) {
>  		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
>  				!(xhci_all_ports_seen_u0(xhci))) {
>  			timer_delete_sync(&xhci->comp_mode_recovery_timer);
> @@ -1172,27 +1175,33 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  		if (retval)
>  			return retval;

It's suspicious that xhci_halt() is called earlier without checking
retval. If it fails then xhci_reset() returns success and this check
passes and it seems that nothing good will happen.

No sure if the HC can be running here?

>  
> -		xhci_dbg(xhci, "// Disabling event ring interrupts\n");
> -		temp = readl(&xhci->op_regs->status);
> -		writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);
> -		xhci_disable_interrupter(xhci, xhci->interrupters[0]);
> +		cancel_delayed_work_sync(&xhci->cmd_timer);
> +
> +		/* Delete all remaining commands */
> +		xhci_cleanup_command_queue(xhci);

Considering that xhci_suspend() clears the command ring anyway, it
could probably do this too so we don't need to. 

BTW, debugfs/port_bandwidth interface queues commands and I'm not sure
if it's synchronized in any way with power management. IOW, it might be 
possible that command are pending at suspend, but I'm not sure.

> +
> +		/* Clear data which is re-initilized during runtime */
> +		xhci_for_each_ring_seg(xhci->interrupters[0]->event_ring->first_seg, seg)
> +			memset(seg->trbs, 0, sizeof(union xhci_trb) * TRBS_PER_SEGMENT);
> +
> +		for (int i = xhci->max_slots; i > 0; i--)
> +			xhci_free_virt_devices_depth_first(xhci, i);

This loop is a bit ugly, it could be a function in xhci-mem.

> +
> +		xhci_rh_bw_cleanup(xhci);
> +
> +		xhci->cmd_ring_reserved_trbs = 0;
> +		xhci_for_each_ring_seg(xhci->cmd_ring->first_seg, seg)
> +			memset(seg->trbs, 0, sizeof(union xhci_trb) * TRBS_PER_SEGMENT);

This looks like a bug because it nukes link TRBs. I know that
xhci_init() will fix this up (unless somebody changes that without
updating here), but it looks confusing.

I think calling clear_command_ring() was clearer and less bug prone.

>  
> -		xhci_dbg(xhci, "cleaning up memory\n");
> -		xhci_mem_cleanup(xhci);
>  		xhci_debugfs_exit(xhci);

Interesting, it looks like there used to be a window of time when all
memory was freed but debugfs files still existed? Odd...

> -		xhci_dbg(xhci, "xhci_stop completed - status = %x\n",
> -			    readl(&xhci->op_regs->status));
> -
> -		/* USB core calls the PCI reinit and start functions twice:
> -		 * first with the primary HCD, and then with the secondary HCD.
> -		 * If we don't do the same, the host will never be started.
> -		 */
> -		retval = xhci_mem_init(xhci, GFP_KERNEL);
> -		if (retval)
> -			return retval;
>  
>  		xhci_init(hcd);
>  
> +		/*
> +		 * USB core calls the PCI reinit and start functions twice:
> +		 * first with the primary HCD, and then with the secondary HCD.
> +		 * If we don't do the same, the host will never be started.
> +		 */
>  		xhci_dbg(xhci, "Start the primary HCD\n");
>  		retval = xhci_run(hcd);
>  		if (!retval && xhci->shared_hcd) {
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index ade0198bf9ea..a76e183515b3 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1792,6 +1792,7 @@ void xhci_dbg_trace(struct xhci_hcd *xhci, void (*trace)(struct va_format *),
>  void xhci_mem_cleanup(struct xhci_hcd *xhci);
>  int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags);
>  void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev, int slot_id);
> +void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id);
>  int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id, struct usb_device *udev, gfp_t flags);
>  int xhci_setup_addressable_virt_dev(struct xhci_hcd *xhci, struct usb_device *udev);
>  void xhci_copy_ep0_dequeue_into_input_ctx(struct xhci_hcd *xhci,
> @@ -1803,6 +1804,7 @@ void xhci_update_tt_active_eps(struct xhci_hcd *xhci,
>  		struct xhci_virt_device *virt_dev,
>  		int old_active_eps);
>  void xhci_clear_endpoint_bw_info(struct xhci_bw_info *bw_info);
> +void xhci_rh_bw_cleanup(struct xhci_hcd *xhci);
>  void xhci_update_bw_info(struct xhci_hcd *xhci,
>  		struct xhci_container_ctx *in_ctx,
>  		struct xhci_input_control_ctx *ctrl_ctx,
> -- 
> 2.50.1
>