[RFC PATCH 00/12] usb: xhci: groundwork for secondary interrupters

[RFC PATCH 00/12] usb: xhci: groundwork for secondary interrupters

[Niklas Neronin]

Prepare the xhci driver for supporting secondary interrupters. It does not
enable them yet, but removes several obstacles in the current design.

Key changes:
 * Avoid freeing and reallocating structures during hibernation resume.
 * Consolidate interrupter initialization into a single place.
 * Add the ability to distinguish different interrupter types.

These changes lay the foundation needed before secondary interrupters
can be introduced.

Niklas Neronin (12):
  usb: xhci: simplify CMRT initialization logic
  usb: xhci: relocate Restore/Controller error check
  usb: xhci: simplify USBSTS register reset
  usb: xhci: move reserving command ring trb
  usb: xhci: move ring initialization
  usb: xhci: move initialization for lifetime objects
  usb: xhci: split core allocation and initialization
  usb: xhci: improve debug messages during suspend
  usb: xhci: optimize resuming from S4 (suspend-to-RAM)
  usb: xhci: add interrupter type
  usb: xhci: prepare for multiple interrupters
  usb: xhci: prepare IRQ handler for multiple interrupters

 drivers/usb/host/xhci-mem.c      |  33 ++---
 drivers/usb/host/xhci-pci.c      |   6 +-
 drivers/usb/host/xhci-ring.c     |  19 ++-
 drivers/usb/host/xhci-sideband.c |   1 +
 drivers/usb/host/xhci.c          | 205 +++++++++++++++----------------
 drivers/usb/host/xhci.h          |  13 +-
 6 files changed, 150 insertions(+), 127 deletions(-)

-- 
2.50.1

[RFC PATCH 01/12] usb: xhci: simplify CMRT initialization logic

[Niklas Neronin]

The function compliance_mode_recovery_timer_init() is called from
xhci_init() because the Compliance Mode Recovery Timer (CMRT) must be set
up before xhci_run() when the xhci driver is re-initialized.

To handle this case, the boolean flag 'comp_timer_running' was introduced
to track whether xhci_run() had already been called, ensuring that
xhci_resume() would not invoke compliance_mode_recovery_timer_init()
a second time.

This can be simplified by moving the 'done' label in xhci_resume() to
after the compliance_mode_recovery_timer_init() call. With this change,
the timer initialization runs only when the xhci driver has not been
re-initialized, making the 'comp_timer_running' flag unnecessary and
allowing it to be removed.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index b3ba16b9718c..fa3627dbe9a3 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1084,7 +1084,6 @@ 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);
 	int			retval = 0;
-	bool			comp_timer_running = false;
 	bool			pending_portevent = false;
 	bool			suspended_usb3_devs = false;
 
@@ -1196,7 +1195,6 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
 		retval = xhci_init(hcd);
 		if (retval)
 			return retval;
-		comp_timer_running = true;
 
 		xhci_dbg(xhci, "Start the primary HCD\n");
 		retval = xhci_run(hcd);
@@ -1265,16 +1263,16 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
 			usb_hcd_resume_root_hub(hcd);
 		}
 	}
-done:
+
 	/*
 	 * If system is subject to the Quirk, Compliance Mode Timer needs to
 	 * be re-initialized Always after a system resume. Ports are subject
 	 * to suffer the Compliance Mode issue again. It doesn't matter if
 	 * ports have entered previously to U0 before system's suspension.
 	 */
-	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) && !comp_timer_running)
+	if (xhci->quirks & XHCI_COMP_MODE_QUIRK)
 		compliance_mode_recovery_timer_init(xhci);
-
+done:
 	if (xhci->quirks & XHCI_ASMEDIA_MODIFY_FLOWCONTROL)
 		usb_asmedia_modifyflowcontrol(to_pci_dev(hcd->self.controller));
 
-- 
2.50.1

[RFC PATCH 02/12] usb: xhci: relocate Restore/Controller error check

[Niklas Neronin]

A Restore Error or Host Controller Error indicates that the host controller
failed to resume after suspend. In such cases, the xhci driver is fully
re-initialized, similar to a post-hibernation scenario.

The existing error check is only relevant when 'power_lost' is false.
If 'power_lost' is true, a Restore or Controller error has no effect:
no warning is printed and the 'power_lost' state remains unchanged.

Move the entire error check into the if '!power_lost' condition
to make this dependency explicit and simplify the resume logic.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index fa3627dbe9a3..a288f59c604c 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1140,16 +1140,13 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
 			spin_unlock_irq(&xhci->lock);
 			return -ETIMEDOUT;
 		}
-	}
 
-	temp = readl(&xhci->op_regs->status);
-
-	/* re-initialize the HC on Restore Error, or Host Controller Error */
-	if ((temp & (STS_SRE | STS_HCE)) &&
-	    !(xhci->xhc_state & XHCI_STATE_REMOVING)) {
-		if (!power_lost)
+		/* re-initialize the HC on Restore Error, or Host Controller Error */
+		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;
+			power_lost = true;
+		}
 	}
 
 	if (power_lost) {
-- 
2.50.1

[RFC PATCH 03/12] usb: xhci: simplify USBSTS register reset

[Niklas Neronin]

The current code clears the USB Status Register (USBSTS) and then sets
the Enable Interrupt (EINT) bit to '1'. A mask (~0x1fff) is used to avoid
modifying reserved bits 31:13, but it also inadvertently excludes reserved
bits 7:5, 1. These fields are defined as RsvdZ, so writing '0' to them has
no effect.

According to the xHCI specification, section 5.1.1:
"System software shall mask all reserved fields (Rsvd, RsvdP or RsvdZ) to
 '0' before evaluating a register or data structure value. This will enable
 current system software to run with future xHCI implementations that
 define the reserved fields."

Given this, it is safe to write zero to all bits of USBSTS except EINT.
This allows the mask and the register read to be removed, simplifying the
code, avoiding potential future issues and removing unexplained hex.

USB Status Register; xHCI Specification, version 1.2 section 5.4.2

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a288f59c604c..ad095768ed48 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -705,7 +705,6 @@ EXPORT_SYMBOL_GPL(xhci_run);
  */
 void xhci_stop(struct usb_hcd *hcd)
 {
-	u32 temp;
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 	struct xhci_interrupter *ir = xhci->interrupters[0];
 
@@ -740,8 +739,7 @@ void xhci_stop(struct usb_hcd *hcd)
 
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
 			"// Disabling event ring interrupts");
-	temp = readl(&xhci->op_regs->status);
-	writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);
+	writel(STS_EINT, &xhci->op_regs->status);
 	xhci_disable_interrupter(xhci, ir);
 
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "cleaning up memory");
@@ -1174,8 +1172,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
 			return retval;
 
 		xhci_dbg(xhci, "// Disabling event ring interrupts\n");
-		temp = readl(&xhci->op_regs->status);
-		writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);
+		writel(STS_EINT, &xhci->op_regs->status);
 		xhci_disable_interrupter(xhci, xhci->interrupters[0]);
 
 		xhci_dbg(xhci, "cleaning up memory\n");
-- 
2.50.1

[RFC PATCH 04/12] usb: xhci: move reserving command ring trb

[Niklas Neronin]

Move the command ring TRB reservation from xhci_mem_init() to xhci_init().

Function xhci_mem_init() is intended for memory allocation,
while xhci_init() is for initialization.

This split allows xhci_init() to be reused when resuming from S4
suspend-to-disk.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c | 7 -------
 drivers/usb/host/xhci.c     | 6 ++++++
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index c708bdd69f16..2e6c5fe11940 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2477,13 +2477,6 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "First segment DMA is 0x%pad",
 		       &xhci->cmd_ring->first_seg->dma);
 
-	/*
-	 * Reserve one command ring TRB for disabling LPM.
-	 * Since the USB core grabs the shared usb_bus bandwidth mutex before
-	 * disabling LPM, we only need to reserve one TRB for all devices.
-	 */
-	xhci->cmd_ring_reserved_trbs++;
-
 	/* Allocate and set up primary interrupter 0 with an event ring. */
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Allocating primary event ring");
 	xhci->interrupters = kcalloc_node(xhci->max_interrupters, sizeof(*xhci->interrupters),
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index ad095768ed48..e8366aa479ab 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -564,6 +564,12 @@ static int xhci_init(struct usb_hcd *hcd)
 	/* Set the Number of Device Slots Enabled to the maximum supported value */
 	xhci_enable_max_dev_slots(xhci);
 
+	/*
+	 * Reserve one command ring TRB for disabling LPM.
+	 * Since the USB core grabs the shared usb_bus bandwidth mutex before
+	 * disabling LPM, we only need to reserve one TRB for all devices.
+	 */
+	xhci->cmd_ring_reserved_trbs = 1;
 	/* Set the address in the Command Ring Control register */
 	xhci_set_cmd_ring_deq(xhci);
 
-- 
2.50.1

[RFC PATCH 05/12] usb: xhci: move ring initialization

[Niklas Neronin]

Move ring initialization from xhci_ring_alloc() to xhci_ring_init().
Call xhci_ring_init() after xhci_ring_alloc(); in the future,
it can also be used to re-initialize the ring during resume.

Additionally, remove xhci_dbg_trace() from xhci_mem_init(). The command
ring's first segment DMA address is now printed during the trace call in
xhci_ring_init().

This refactoring lays also the groundwork for eventually replacing:
* xhci_dbc_ring_init()
* xhci_clear_command_ring()

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c | 19 ++++++++++++++-----
 drivers/usb/host/xhci.c     |  3 +++
 drivers/usb/host/xhci.h     |  1 +
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 2e6c5fe11940..005b7bc1bfda 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -129,6 +129,13 @@ static void xhci_initialize_ring_segments(struct xhci_hcd *xhci, struct xhci_rin
 	ring->last_seg->trbs[TRBS_PER_SEGMENT - 1].link.control |= cpu_to_le32(LINK_TOGGLE);
 }
 
+void xhci_ring_init(struct xhci_hcd *xhci, struct xhci_ring *ring)
+{
+	xhci_initialize_ring_segments(xhci, ring);
+	xhci_initialize_ring_info(ring);
+	trace_xhci_ring_alloc(ring);
+}
+
 /*
  * Link the src ring segments to the dst ring.
  * Set Toggle Cycle for the new ring if needed.
@@ -389,9 +396,6 @@ struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci, unsigned int num_segs,
 	if (ret)
 		goto fail;
 
-	xhci_initialize_ring_segments(xhci, ring);
-	xhci_initialize_ring_info(ring);
-	trace_xhci_ring_alloc(ring);
 	return ring;
 
 fail:
@@ -668,6 +672,8 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
 		cur_ring = stream_info->stream_rings[cur_stream];
 		if (!cur_ring)
 			goto cleanup_rings;
+
+		xhci_ring_init(xhci, cur_ring);
 		cur_ring->stream_id = cur_stream;
 		cur_ring->trb_address_map = &stream_info->trb_address_map;
 		/* Set deq ptr, cycle bit, and stream context type */
@@ -1011,6 +1017,8 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
 	if (!dev->eps[0].ring)
 		goto fail;
 
+	xhci_ring_init(xhci, dev->eps[0].ring);
+
 	dev->udev = udev;
 
 	/* Point to output device context in dcbaa. */
@@ -1492,6 +1500,7 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
 
 	virt_dev->eps[ep_index].skip = false;
 	ep_ring = virt_dev->eps[ep_index].new_ring;
+	xhci_ring_init(xhci, ep_ring);
 
 	/* Fill the endpoint context */
 	ep_ctx->ep_info = cpu_to_le32(EP_MAX_ESIT_PAYLOAD_HI(max_esit_payload) |
@@ -2362,6 +2371,8 @@ xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs,
 	if (!ir)
 		return NULL;
 
+	xhci_ring_init(xhci, ir->event_ring);
+
 	spin_lock_irq(&xhci->lock);
 	if (!intr_num) {
 		/* Find available secondary interrupter, interrupter 0 is reserved for primary */
@@ -2474,8 +2485,6 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 		goto fail;
 
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Allocated command ring at %p", xhci->cmd_ring);
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "First segment DMA is 0x%pad",
-		       &xhci->cmd_ring->first_seg->dma);
 
 	/* Allocate and set up primary interrupter 0 with an event ring. */
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Allocating primary event ring");
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e8366aa479ab..d9519a9e9e17 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -564,6 +564,8 @@ static int xhci_init(struct usb_hcd *hcd)
 	/* Set the Number of Device Slots Enabled to the maximum supported value */
 	xhci_enable_max_dev_slots(xhci);
 
+	/* Initialize the Command ring */
+	xhci_ring_init(xhci, xhci->cmd_ring);
 	/*
 	 * Reserve one command ring TRB for disabling LPM.
 	 * Since the USB core grabs the shared usb_bus bandwidth mutex before
@@ -583,6 +585,7 @@ static int xhci_init(struct usb_hcd *hcd)
 	xhci_set_dev_notifications(xhci);
 
 	/* Initialize the Primary interrupter */
+	xhci_ring_init(xhci, xhci->interrupters[0]->event_ring);
 	xhci_add_interrupter(xhci, 0);
 	xhci->interrupters[0]->isoc_bei_interval = AVOID_BEI_INTERVAL_MAX;
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 2b0796f6d00e..ade0198bf9ea 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1823,6 +1823,7 @@ void xhci_ring_free(struct xhci_hcd *xhci, struct xhci_ring *ring);
 int xhci_ring_expansion(struct xhci_hcd *xhci, struct xhci_ring *ring,
 		unsigned int num_trbs, gfp_t flags);
 void xhci_initialize_ring_info(struct xhci_ring *ring);
+void xhci_ring_init(struct xhci_hcd *xhci, struct xhci_ring *ring);
 void xhci_free_endpoint_ring(struct xhci_hcd *xhci,
 		struct xhci_virt_device *virt_dev,
 		unsigned int ep_index);
-- 
2.50.1

[RFC PATCH 06/12] usb: xhci: move initialization for lifetime objects

[Niklas Neronin]

Initialize objects that exist for the lifetime of the driver only once,
rather than repeatedly. These objects do not require re-initialization
after events such as S4 (suspend-to-disk).

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c |  1 -
 drivers/usb/host/xhci.c     | 15 ++++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 005b7bc1bfda..fae75969e49a 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2001,7 +2001,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
 	xhci->port_caps = NULL;
 	xhci->interrupters = NULL;
 
-	xhci->page_size = 0;
 	xhci->usb2_rhub.bus_state.bus_suspended = 0;
 	xhci->usb3_rhub.bus_state.bus_suspended = 0;
 }
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index d9519a9e9e17..338e93f39937 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -549,13 +549,6 @@ static int xhci_init(struct usb_hcd *hcd)
 	int retval;
 
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Starting %s", __func__);
-	spin_lock_init(&xhci->lock);
-
-	INIT_LIST_HEAD(&xhci->cmd_list);
-	INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout);
-	init_completion(&xhci->cmd_ring_stop_completion);
-	xhci_hcd_page_size(xhci);
-	memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
 
 	retval = xhci_mem_init(xhci, GFP_KERNEL);
 	if (retval)
@@ -5522,6 +5515,14 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 		dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
 	}
 
+	spin_lock_init(&xhci->lock);
+	INIT_LIST_HEAD(&xhci->cmd_list);
+	INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout);
+	init_completion(&xhci->cmd_ring_stop_completion);
+	xhci_hcd_page_size(xhci);
+
+	memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
+
 	xhci_dbg(xhci, "Calling HCD init\n");
 	/* Initialize HCD and host controller data structures. */
 	retval = xhci_init(hcd);
-- 
2.50.1

[RFC PATCH 07/12] usb: xhci: split core allocation and initialization

[Niklas Neronin]

Separate allocation and initialization in the xHCI core:
* xhci_mem_init() now only handles memory allocation.
* xhci_init() now only handles initialization.

This split allows xhci_init() to be reused when resuming from S4
suspend-to-disk.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 338e93f39937..0c2239b5b805 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -536,23 +536,9 @@ static void xhci_set_dev_notifications(struct xhci_hcd *xhci)
 	writel(dev_notf, &xhci->op_regs->dev_notification);
 }
 
-/*
- * Initialize memory for HCD and xHC (one-time init).
- *
- * Program the PAGESIZE register, initialize the device context array, create
- * device contexts (?), set up a command ring segment (or two?), create event
- * ring (one for now).
- */
-static int xhci_init(struct usb_hcd *hcd)
+static void xhci_init(struct usb_hcd *hcd)
 {
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
-	int retval;
-
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Starting %s", __func__);
-
-	retval = xhci_mem_init(xhci, GFP_KERNEL);
-	if (retval)
-		return retval;
 
 	/* Set the Number of Device Slots Enabled to the maximum supported value */
 	xhci_enable_max_dev_slots(xhci);
@@ -587,9 +573,6 @@ static int xhci_init(struct usb_hcd *hcd)
 		xhci->quirks |= XHCI_COMP_MODE_QUIRK;
 		compliance_mode_recovery_timer_init(xhci);
 	}
-
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Finished %s", __func__);
-	return 0;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -1187,11 +1170,14 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
 		 * 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, "Initialize the xhci_hcd\n");
-		retval = xhci_init(hcd);
+		xhci_dbg(xhci, "Allocate the xhci_hcd\n");
+		retval = xhci_mem_init(xhci, GFP_KERNEL);
 		if (retval)
 			return retval;
 
+		xhci_dbg(xhci, "Initialize the xhci_hcd\n");
+		xhci_init(hcd);
+
 		xhci_dbg(xhci, "Start the primary HCD\n");
 		retval = xhci_run(hcd);
 		if (!retval && xhci->shared_hcd) {
@@ -5523,12 +5509,15 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 
 	memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
 
-	xhci_dbg(xhci, "Calling HCD init\n");
-	/* Initialize HCD and host controller data structures. */
-	retval = xhci_init(hcd);
+	xhci_dbg(xhci, "Allocate the xhci_hcd\n");
+	/* Allocate xHCI data structures */
+	retval = xhci_mem_init(xhci, GFP_KERNEL);
 	if (retval)
 		return retval;
-	xhci_dbg(xhci, "Called HCD init\n");
+
+	xhci_dbg(xhci, "Initialize the xhci_hcd\n");
+	/* Initialize HCD and host controller data structures */
+	xhci_init(hcd);
 
 	if (xhci_hcd_is_usb3(hcd))
 		xhci_hcd_init_usb3_data(xhci, hcd);
-- 
2.50.1

[RFC PATCH 08/12] usb: xhci: improve debug messages during suspend

[Niklas Neronin]

Improve debug output for suspend failures, particularly when the controller
handshake does not complete. This will become important as upcoming patches
significantly rework the resume path, making more detailed suspend-side
messages valuable for debugging.

Add an explicit check of the Save/Restore Error (SRE) flag after a
successful Save State (CSS) operation. The xHCI specification
(note in section 4.23.2) states:

 "After a Save or Restore State operation completes, the
  Save/Restore Error (SRE) flag in USBSTS should be checked to
  ensure the operation completed successfully."

Currently, the SRE error is only observed and warning is printed.
This patch does not introduce deeper error handling, as the correct
response is unclear and changes to suspend behavior may risk regressions
once the resume path is updated.

Additionally, simplify and clean up the suspend USBSTS CSS/SSS
handling code, improving readability and quirk handling for AMD
SNPS xHC controllers that occasionally do not clear the SSS bit.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci.c | 65 +++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 0c2239b5b805..b22f10cfbe7b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -950,11 +950,11 @@ static bool xhci_pending_portevent(struct xhci_hcd *xhci)
  */
 int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 {
-	int			rc = 0;
+	int			err;
 	unsigned int		delay = XHCI_MAX_HALT_USEC * 2;
 	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
 	u32			command;
-	u32			res;
+	u32			usbsts;
 
 	if (!hcd->state)
 		return 0;
@@ -1000,11 +1000,10 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 	/* Some chips from Fresco Logic need an extraordinary delay */
 	delay *= (xhci->quirks & XHCI_SLOW_SUSPEND) ? 10 : 1;
 
-	if (xhci_handshake(&xhci->op_regs->status,
-		      STS_HALT, STS_HALT, delay)) {
-		xhci_warn(xhci, "WARN: xHC CMD_RUN timeout\n");
-		spin_unlock_irq(&xhci->lock);
-		return -ETIMEDOUT;
+	err = xhci_handshake(&xhci->op_regs->status, STS_HALT, STS_HALT, delay);
+	if (err) {
+		xhci_warn(xhci, "Clearing Run/Stop bit failed %d\n", err);
+		goto handshake_error;
 	}
 	xhci_clear_command_ring(xhci);
 
@@ -1015,28 +1014,34 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 	command = readl(&xhci->op_regs->command);
 	command |= CMD_CSS;
 	writel(command, &xhci->op_regs->command);
+
+	err = xhci_handshake(&xhci->op_regs->status, STS_SAVE, 0, 20 * USEC_PER_MSEC);
+	usbsts = readl(&xhci->op_regs->status);
 	xhci->broken_suspend = 0;
-	if (xhci_handshake(&xhci->op_regs->status,
-				STS_SAVE, 0, 20 * 1000)) {
-	/*
-	 * AMD SNPS xHC 3.0 occasionally does not clear the
-	 * SSS bit of USBSTS and when driver tries to poll
-	 * to see if the xHC clears BIT(8) which never happens
-	 * and driver assumes that controller is not responding
-	 * and times out. To workaround this, its good to check
-	 * if SRE and HCE bits are not set (as per xhci
-	 * Section 5.4.2) and bypass the timeout.
-	 */
-		res = readl(&xhci->op_regs->status);
-		if ((xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND) &&
-		    (((res & STS_SRE) == 0) &&
-				((res & STS_HCE) == 0))) {
-			xhci->broken_suspend = 1;
-		} else {
-			xhci_warn(xhci, "WARN: xHC save state timeout\n");
-			spin_unlock_irq(&xhci->lock);
-			return -ETIMEDOUT;
+	if (err) {
+		/*
+		 * AMD SNPS xHC 3.0 occasionally does not clear the
+		 * SSS bit of USBSTS and when driver tries to poll
+		 * to see if the xHC clears BIT(8) which never happens
+		 * and driver assumes that controller is not responding
+		 * and times out. To workaround this, its good to check
+		 * if SRE and HCE bits are not set (as per xhci
+		 * Section 5.4.2) and bypass the timeout.
+		 */
+		if (!(xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND)) {
+			xhci_warn(xhci, "Controller Save State failed %d\n", err);
+			goto handshake_error;
 		}
+
+		if (usbsts & (STS_SRE | STS_HCE)) {
+			xhci_warn(xhci, "Controller Save State failed, USBSTS 0x%08x\n", usbsts);
+			goto handshake_error;
+		}
+
+		xhci_dbg(xhci, "SNPS broken suspend, save state unreliable\n");
+		xhci->broken_suspend = 1;
+	} else if (usbsts & STS_SRE) {
+		xhci_warn(xhci, "Suspend Save Error (SRE), USBSTS 0x%08x\n", usbsts);
 	}
 	spin_unlock_irq(&xhci->lock);
 
@@ -1052,7 +1057,11 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
 				__func__);
 	}
 
-	return rc;
+	return 0;
+
+handshake_error:
+	spin_unlock_irq(&xhci->lock);
+	return -ETIMEDOUT;
 }
 EXPORT_SYMBOL_GPL(xhci_suspend);
 
-- 
2.50.1

[RFC PATCH 09/12] usb: xhci: optimize resuming from S4 (suspend-to-RAM)

[Niklas Neronin]

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.

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 |  2 +-
 drivers/usb/host/xhci.c     | 46 +++++++++++++++++++------------------
 drivers/usb/host/xhci.h     |  1 +
 3 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index fae75969e49a..46d977f9e5c5 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;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index b22f10cfbe7b..e03f717d2314 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1075,9 +1075,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;
@@ -1096,10 +1098,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");
 		/*
 		 * Some controllers might lose power during suspend, so wait
 		 * for controller not ready bit to clear, just as in xHC init.
@@ -1137,11 +1140,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);
@@ -1165,28 +1168,27 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
 		if (retval)
 			return retval;
 
-		xhci_dbg(xhci, "// Disabling event ring interrupts\n");
-		writel(STS_EINT, &xhci->op_regs->status);
-		xhci_disable_interrupter(xhci, xhci->interrupters[0]);
+		/* 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_ports; i > 0; i--)
+			xhci_free_virt_devices_depth_first(xhci, i);
+
+		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);
 
-		xhci_dbg(xhci, "cleaning up memory\n");
-		xhci_mem_cleanup(xhci);
 		xhci_debugfs_exit(xhci);
-		xhci_dbg(xhci, "xhci_stop completed - status = %x\n",
-			    readl(&xhci->op_regs->status));
 
-		/* USB core calls the PCI reinit and start functions twice:
+		xhci_dbg(xhci, "Re-initializing xHCI registers\n");
+		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, "Allocate the xhci_hcd\n");
-		retval = xhci_mem_init(xhci, GFP_KERNEL);
-		if (retval)
-			return retval;
-
-		xhci_dbg(xhci, "Initialize the xhci_hcd\n");
-		xhci_init(hcd);
-
 		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..e6a51f1318c2 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,
-- 
2.50.1

[RFC PATCH 10/12] usb: xhci: add interrupter type

[Niklas Neronin]

xhci-sideband creates a secondary interrupter without an associated IRQ.
Such interrupters are non-operational and cannot enabled or disabled.

Add a type field to struct 'xhci_interrupter' to distinguish
non-operational interrupters. When the type is set to 'INTR_NOOP',
the interrupter enable/disable helpers become no-ops.

This allows callers to iterate over all allocated interrupters without
special-casing, while ensuring that actions are applied only to operational
interrupters. It also provides a simple extension point for adding
additional interrupter types in the future.

Operational interrupters remain the default; no-op interrupters are the
exception.

No functional changes are introduced; behavior remains the same.

This change is necessary to prepare the driver for multiple interrupters.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-sideband.c | 1 +
 drivers/usb/host/xhci.c          | 6 ++++++
 drivers/usb/host/xhci.h          | 8 ++++++++
 3 files changed, 15 insertions(+)

diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index 2bd77255032b..21ee4e96bc70 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -352,6 +352,7 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
 	ret = usb_offload_get(udev);
 
 	sb->ir->ip_autoclear = ip_autoclear;
+	sb->ir->type = INTR_NOOP;
 
 	return ret;
 }
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e03f717d2314..329fe9ff19f8 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -317,6 +317,9 @@ int xhci_enable_interrupter(struct xhci_interrupter *ir)
 	if (!ir || !ir->ir_set)
 		return -EINVAL;
 
+	if (ir->type == INTR_NOOP)
+		return 0;
+
 	iman = readl(&ir->ir_set->iman);
 	iman &= ~IMAN_IP;
 	iman |= IMAN_IE;
@@ -334,6 +337,9 @@ int xhci_disable_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
 	if (!ir || !ir->ir_set)
 		return -EINVAL;
 
+	if (ir->type == INTR_NOOP)
+		return 0;
+
 	iman = readl(&ir->ir_set->iman);
 	iman &= ~IMAN_IP;
 	iman &= ~IMAN_IE;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index e6a51f1318c2..1e9ea6507813 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1443,6 +1443,13 @@ struct xhci_bus_state {
 	unsigned long		resuming_ports;
 };
 
+enum interrupter_type {
+	/* Normal interrupter, e.g. Primary */
+	INTR_DEFAULT,
+	/* Non-operational, e.g. xhci-sideband */
+	INTR_NOOP,
+};
+
 struct xhci_interrupter {
 	struct xhci_ring	*event_ring;
 	struct xhci_erst	erst;
@@ -1450,6 +1457,7 @@ struct xhci_interrupter {
 	unsigned int		intr_num;
 	bool			ip_autoclear;
 	u32			isoc_bei_interval;
+	enum interrupter_type	type;
 	/* For interrupter registers save and restore over suspend/resume */
 	u32	s3_iman;
 	u32	s3_imod;
-- 
2.50.1

[RFC PATCH 11/12] usb: xhci: prepare for multiple interrupters

[Niklas Neronin]

The core xhci driver only uses a single interrupter. Prepares the driver
to handle multiple interrupters, although it still uses only one interrupt
for now.

Some initialization, such as IP bit auto-clear and IMODI, was previously
done in xhci_run() (called once per xHCI instance). For simplicity,
move these to xhci_init() to consolidate all interrupter-related setup
in a single place.

Move the ERST dequeue debug message from xhci_run() to the existing ERST
debug message in xhci_set_hc_event_deq(). This function is always called
before xhci_run(), and is called for all interrupters.

This change is necessary to prepare the driver for multiple interrupters.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c |  3 +--
 drivers/usb/host/xhci-pci.c |  4 ++++
 drivers/usb/host/xhci.c     | 13 +------------
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 46d977f9e5c5..a6fde127c569 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2017,8 +2017,7 @@ static void xhci_set_hc_event_deq(struct xhci_hcd *xhci, struct xhci_interrupter
 	/* Don't clear the EHB bit (which is RW1C) because
 	 * there might be more events to service.
 	 */
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-		       "// Write event ring dequeue pointer, preserving EHB bit");
+	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Write ERST %pad, preserving EHB bit", &deq);
 	xhci_write_64(xhci, deq & ERST_PTR_MASK, &ir->ir_set->erst_dequeue);
 }
 
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 585b2f3117b0..ea43b0153430 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -181,6 +181,10 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
 
 	hcd->msi_enabled = 1;
 	hcd->msix_enabled = pdev->msix_enabled;
+
+	/* MSI/MSI-X interrupts clear IMAN IP bit automatically */
+	xhci->interrupters[0]->ip_autoclear = true;
+
 	return 0;
 
 free_irq_vectors:
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 329fe9ff19f8..e846ab402d3e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -573,6 +573,7 @@ static void xhci_init(struct usb_hcd *hcd)
 	xhci_ring_init(xhci, xhci->interrupters[0]->event_ring);
 	xhci_add_interrupter(xhci, 0);
 	xhci->interrupters[0]->isoc_bei_interval = AVOID_BEI_INTERVAL_MAX;
+	xhci_set_interrupter_moderation(xhci->interrupters[0], xhci->imod_interval);
 
 	/* Initializing Compliance Mode Recovery Data If Needed */
 	if (xhci_compliance_mode_recovery_timer_quirk_check()) {
@@ -633,30 +634,18 @@ static int xhci_run_finished(struct xhci_hcd *xhci)
  */
 int xhci_run(struct usb_hcd *hcd)
 {
-	u64 temp_64;
 	int ret;
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
-	struct xhci_interrupter *ir = xhci->interrupters[0];
 	/* Start the xHCI host controller running only after the USB 2.0 roothub
 	 * is setup.
 	 */
 
 	hcd->uses_new_polling = 1;
-	if (hcd->msi_enabled)
-		ir->ip_autoclear = true;
-
 	if (!usb_hcd_is_primary_hcd(hcd))
 		return xhci_run_finished(xhci);
 
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "xhci_run");
 
-	temp_64 = xhci_read_64(xhci, &ir->ir_set->erst_dequeue);
-	temp_64 &= ERST_PTR_MASK;
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-			"ERST deq = 64'h%0lx", (long unsigned int) temp_64);
-
-	xhci_set_interrupter_moderation(ir, xhci->imod_interval);
-
 	if (xhci->quirks & XHCI_NEC_HOST) {
 		struct xhci_command *command;
 
-- 
2.50.1

[RFC PATCH 12/12] usb: xhci: prepare IRQ handler for multiple interrupters

[Niklas Neronin]

The xHCI driver currently assumes a single interrupter (index 0), and the
IRQ handler receives a pointer to the struct 'xhci', from which it directly
accesses 'xhci->interrupters[0]'.

To support multiple interrupters, the IRQ handler must be able to identify
the specific interrupter instance that triggered the interrupt. Do this by
passing a pointer to the corresponding struct 'xhci_interrupter' instead of
the global struct 'xhci'. Add a back-pointer to struct 'xhci' in struct
'xhci_interrupter' so the handler can still access ore data.

This change is necessary to prepare the driver for multiple interrupters.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c  |  1 +
 drivers/usb/host/xhci-pci.c  |  2 +-
 drivers/usb/host/xhci-ring.c | 19 ++++++++++++++-----
 drivers/usb/host/xhci.h      |  3 ++-
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index a6fde127c569..30eb0a79ef20 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2331,6 +2331,7 @@ void xhci_add_interrupter(struct xhci_hcd *xhci, unsigned int intr_num)
 	u32 erst_size;
 
 	ir = xhci->interrupters[intr_num];
+	ir->xhci = xhci;
 	ir->intr_num = intr_num;
 	ir->ir_set = &xhci->run_regs->ir_set[intr_num];
 
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index ea43b0153430..c1b8062c5467 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -175,7 +175,7 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
 	}
 
 	ret = request_irq(pci_irq_vector(pdev, 0), xhci_msi_irq, 0, "xhci_hcd",
-			  xhci_to_hcd(xhci));
+			  &xhci->interrupters[0]);
 	if (ret)
 		goto free_irq_vectors;
 
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 9315ba18310d..5e303f8f0e4f 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3174,9 +3174,9 @@ void xhci_skip_sec_intr_events(struct xhci_hcd *xhci,
  * we might get bad data out of the event ring.  Section 4.10.2.7 has a list of
  * indicators of an event TRB error, but we check the status *first* to be safe.
  */
-irqreturn_t xhci_irq(struct usb_hcd *hcd)
+static irqreturn_t xhci_irq_handler(struct xhci_interrupter *ir)
 {
-	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+	struct xhci_hcd *xhci = ir->xhci;
 	irqreturn_t ret = IRQ_HANDLED;
 	u32 status;
 
@@ -3213,16 +3213,25 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
 	writel(status, &xhci->op_regs->status);
 
 	/* This is the handler of the primary interrupter */
-	xhci_handle_events(xhci, xhci->interrupters[0], false);
+	xhci_handle_events(xhci, ir, false);
 out:
 	spin_unlock(&xhci->lock);
 
 	return ret;
 }
 
-irqreturn_t xhci_msi_irq(int irq, void *hcd)
+irqreturn_t xhci_irq(struct usb_hcd *hcd)
 {
-	return xhci_irq(hcd);
+	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+
+	return xhci_irq_handler(xhci->interrupters[0]);
+}
+
+irqreturn_t xhci_msi_irq(int irq, void *data)
+{
+	struct xhci_interrupter *ir = *(struct xhci_interrupter **)data;
+
+	return xhci_irq_handler(ir);
 }
 EXPORT_SYMBOL_GPL(xhci_msi_irq);
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 1e9ea6507813..1b177882851f 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1451,6 +1451,7 @@ enum interrupter_type {
 };
 
 struct xhci_interrupter {
+	struct xhci_hcd		*xhci;
 	struct xhci_ring	*event_ring;
 	struct xhci_erst	erst;
 	struct xhci_intr_reg __iomem *ir_set;
@@ -1905,7 +1906,7 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup);
 int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume);
 
 irqreturn_t xhci_irq(struct usb_hcd *hcd);
-irqreturn_t xhci_msi_irq(int irq, void *hcd);
+irqreturn_t xhci_msi_irq(int irq, void *data);
 int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev);
 int xhci_alloc_tt_info(struct xhci_hcd *xhci,
 		struct xhci_virt_device *virt_dev,
-- 
2.50.1

Re: [RFC PATCH 03/12] usb: xhci: simplify USBSTS register reset

[Michal Pecio]

On Thu,  5 Mar 2026 15:48:15 +0100, Niklas Neronin wrote:
> The current code clears the USB Status Register (USBSTS) and then sets
> the Enable Interrupt (EINT) bit to '1'. A mask (~0x1fff) is used to
> avoid modifying reserved bits 31:13, but it also inadvertently
> excludes reserved bits 7:5, 1. These fields are defined as RsvdZ, so
> writing '0' to them has no effect.

This paragraph is misleading because it suggests that treatment of bits
31:13 was correct and treatment of bits 7:5 was harmlessly wrong.

In reality, bits 7:5 were done correctly, while preserving bits 31:13
was a bug. They should be written as zero just like 7:5.

Table 5-3 is completely clear:
Software shall use ‘0’ for writes to these bits.

> According to the xHCI specification, section 5.1.1:
> "System software shall mask all reserved fields (Rsvd, RsvdP or RsvdZ) to
>  '0' before evaluating a register or data structure value. This will enable
>  current system software to run with future xHCI implementations that
>  define the reserved fields."

This quote seems irrelevant. We aren't evaluating anything here, just
changing one bit.

It obviously doesn't apply to the case of read-modify-write, because if
we mask RsvdP bits to zero and then write back, that's a bug too.

> 
> Given this, it is safe to write zero to all bits of USBSTS except EINT.
> This allows the mask and the register read to be removed, simplifying the
> code, avoiding potential future issues and removing unexplained hex.
> 
> USB Status Register; xHCI Specification, version 1.2 section 5.4.2
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index a288f59c604c..ad095768ed48 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -705,7 +705,6 @@ EXPORT_SYMBOL_GPL(xhci_run);
>   */
>  void xhci_stop(struct usb_hcd *hcd)
>  {
> -	u32 temp;
>  	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>  	struct xhci_interrupter *ir = xhci->interrupters[0];
>  
> @@ -740,8 +739,7 @@ void xhci_stop(struct usb_hcd *hcd)
>  
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
>  			"// Disabling event ring interrupts");
> -	temp = readl(&xhci->op_regs->status);
> -	writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);

Makes sense,

> +	writel(STS_EINT, &xhci->op_regs->status);

but what is actually the point of clearing this bit here at all?

This is USBSTS.EINT, not USBCMD.INTE. Per 5.4.2 (including notes),
this bit has no effect on anything, it's only for SW's convenience.
And we are shutting down the driver right now.

>  	xhci_disable_interrupter(xhci, ir);
>  
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "cleaning up memory");
> @@ -1174,8 +1172,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  			return retval;
>  
>  		xhci_dbg(xhci, "// Disabling event ring interrupts\n");
> -		temp = readl(&xhci->op_regs->status);
> -		writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);
> +		writel(STS_EINT, &xhci->op_regs->status);
>  		xhci_disable_interrupter(xhci, xhci->interrupters[0]);

This runs right after xhci_reset(), so I suspect this whole block is
pointless. Apparently copy-pasted with other stuff from xhci_stop().

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

Re: [RFC PATCH 06/12] usb: xhci: move initialization for lifetime objects

[Michal Pecio]

On Thu,  5 Mar 2026 15:48:18 +0100, Niklas Neronin wrote:
> Initialize objects that exist for the lifetime of the driver only once,
> rather than repeatedly. These objects do not require re-initialization
> after events such as S4 (suspend-to-disk).
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci-mem.c |  1 -
>  drivers/usb/host/xhci.c     | 15 ++++++++-------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 005b7bc1bfda..fae75969e49a 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -2001,7 +2001,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  	xhci->port_caps = NULL;
>  	xhci->interrupters = NULL;
>  
> -	xhci->page_size = 0;
>  	xhci->usb2_rhub.bus_state.bus_suspended = 0;
>  	xhci->usb3_rhub.bus_state.bus_suspended = 0;
>  }
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index d9519a9e9e17..338e93f39937 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -549,13 +549,6 @@ static int xhci_init(struct usb_hcd *hcd)
>  	int retval;
>  
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Starting %s", __func__);
> -	spin_lock_init(&xhci->lock);
> -
> -	INIT_LIST_HEAD(&xhci->cmd_list);

Can we prove that this list is empty?

If there is any leftover garbage there, INIT_LIST_HEAD simply leaks it.
Without it, the garbage stays on the list.

> -	INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout);
> -	init_completion(&xhci->cmd_ring_stop_completion);
> -	xhci_hcd_page_size(xhci);
> -	memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
>  
>  	retval = xhci_mem_init(xhci, GFP_KERNEL);
>  	if (retval)
> @@ -5522,6 +5515,14 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>  		dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>  	}
>  
> +	spin_lock_init(&xhci->lock);
> +	INIT_LIST_HEAD(&xhci->cmd_list);
> +	INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout);
> +	init_completion(&xhci->cmd_ring_stop_completion);
> +	xhci_hcd_page_size(xhci);
> +
> +	memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
> +
>  	xhci_dbg(xhci, "Calling HCD init\n");
>  	/* Initialize HCD and host controller data structures. */
>  	retval = xhci_init(hcd);
> -- 
> 2.50.1
> 

Re: [RFC PATCH 07/12] usb: xhci: split core allocation and initialization

[Michal Pecio]

On Thu,  5 Mar 2026 15:48:19 +0100, Niklas Neronin wrote:
> Separate allocation and initialization in the xHCI core:
> * xhci_mem_init() now only handles memory allocation.
> * xhci_init() now only handles initialization.
> 
> This split allows xhci_init() to be reused when resuming from S4
> suspend-to-disk.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci.c | 37 +++++++++++++------------------------
>  1 file changed, 13 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 338e93f39937..0c2239b5b805 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -536,23 +536,9 @@ static void xhci_set_dev_notifications(struct xhci_hcd *xhci)
>  	writel(dev_notf, &xhci->op_regs->dev_notification);
>  }
>  
> -/*
> - * Initialize memory for HCD and xHC (one-time init).
> - *
> - * Program the PAGESIZE register, initialize the device context array, create
> - * device contexts (?), set up a command ring segment (or two?), create event
> - * ring (one for now).
> - */

OK, the old comment is no longer relevant, but maybe add a new one?
/* Setup basic xHCI registers after HC reset */ or something like that.

> -static int xhci_init(struct usb_hcd *hcd)
> +static void xhci_init(struct usb_hcd *hcd)
>  {
>  	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> -	int retval;
> -
> -	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Starting %s", __func__);
> -
> -	retval = xhci_mem_init(xhci, GFP_KERNEL);
> -	if (retval)
> -		return retval;
>  
>  	/* Set the Number of Device Slots Enabled to the maximum supported value */
>  	xhci_enable_max_dev_slots(xhci);
> @@ -587,9 +573,6 @@ static int xhci_init(struct usb_hcd *hcd)
>  		xhci->quirks |= XHCI_COMP_MODE_QUIRK;
>  		compliance_mode_recovery_timer_init(xhci);
>  	}
> -
> -	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Finished %s", __func__);
> -	return 0;
>  }
>  
>  /*-------------------------------------------------------------------------*/
> @@ -1187,11 +1170,14 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  		 * 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, "Initialize the xhci_hcd\n");
> -		retval = xhci_init(hcd);
> +		xhci_dbg(xhci, "Allocate the xhci_hcd\n");
> +		retval = xhci_mem_init(xhci, GFP_KERNEL);

xhci_hcd is allocated, now we allocate everything else :)

I would move those xhci_dbg() inside xhci_mem_init() and xhci_init()
instead of replicating them at every call site. If they are neeeded.

>  		if (retval)
>  			return retval;
>  
> +		xhci_dbg(xhci, "Initialize the xhci_hcd\n");
> +		xhci_init(hcd);
> +
>  		xhci_dbg(xhci, "Start the primary HCD\n");
>  		retval = xhci_run(hcd);
>  		if (!retval && xhci->shared_hcd) {
> @@ -5523,12 +5509,15 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>  
>  	memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
>  
> -	xhci_dbg(xhci, "Calling HCD init\n");
> -	/* Initialize HCD and host controller data structures. */
> -	retval = xhci_init(hcd);
> +	xhci_dbg(xhci, "Allocate the xhci_hcd\n");
> +	/* Allocate xHCI data structures */
> +	retval = xhci_mem_init(xhci, GFP_KERNEL);
>  	if (retval)
>  		return retval;
> -	xhci_dbg(xhci, "Called HCD init\n");
> +
> +	xhci_dbg(xhci, "Initialize the xhci_hcd\n");
> +	/* Initialize HCD and host controller data structures */
> +	xhci_init(hcd);
>  
>  	if (xhci_hcd_is_usb3(hcd))
>  		xhci_hcd_init_usb3_data(xhci, hcd);
> -- 
> 2.50.1
> 

Re: [RFC PATCH 09/12] usb: xhci: optimize resuming from S4 (suspend-to-RAM)

[raoxu]

On 2026-03-05 14:48 Niklas Neronin wrote:

> -	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);
> @@ -1165,28 +1168,27 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  		if (retval)
>  			return retval;
>
> -		xhci_dbg(xhci, "// Disabling event ring interrupts\n");
> -		writel(STS_EINT, &xhci->op_regs->status);
> -		xhci_disable_interrupter(xhci, xhci->interrupters[0]);
> +		/* 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);

If the driver is moving toward multiple operational
interrupters, the current reset logic seems to only
reset interrupter 0 explicitly. Should the secondary
operational interrupters be reset here as well? If so,
that would likely require additional
xhci_for_each_ring_seg() handling and may make the
resume/reset path more complex.

> +
> +		for (int i = xhci->max_ports; i > 0; i--)
> +			xhci_free_virt_devices_depth_first(xhci, i);
> +
> +		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);
>
> -		xhci_dbg(xhci, "cleaning up memory\n");
> -		xhci_mem_cleanup(xhci);
>  		xhci_debugfs_exit(xhci);
> -		xhci_dbg(xhci, "xhci_stop completed - status = %x\n",
> -			    readl(&xhci->op_regs->status));
>

xhci_mem_cleanup() also clears software-side runtime
state, not just rings. Without equivalent cleanup,
state such as the command queue, cmd_timer, cmd_list,
and TT-related list state may remain across
reset-resume, which could lead to unexpected issues on
some hardware.

Thanks,

Xu Rao

Re: [RFC PATCH 12/12] usb: xhci: prepare IRQ handler for multiple interrupters

[raoxu]

On Thu,  5 Mar 2026 15:48:19 +0100, Niklas Neronin wrote:

> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index a6fde127c569..30eb0a79ef20 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -2331,6 +2331,7 @@ void xhci_add_interrupter(struct xhci_hcd *xhci, unsigned int intr_num)
>  	u32 erst_size;
>
>  	ir = xhci->interrupters[intr_num];
> +	ir->xhci = xhci;
>  	ir->intr_num = intr_num;
>  	ir->ir_set = &xhci->run_regs->ir_set[intr_num];
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index ea43b0153430..c1b8062c5467 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -175,7 +175,7 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
>  	}
>
>  	ret = request_irq(pci_irq_vector(pdev, 0), xhci_msi_irq, 0, "xhci_hcd",
> -			  xhci_to_hcd(xhci));
> +			  &xhci->interrupters[0]);

It looks like the IRQ free path should be updated to
use the same dev_id as request_irq(). If request_irq()
uses &xhci->interrupters[0], then free_irq() should be
changed accordingly as well.

> -irqreturn_t xhci_msi_irq(int irq, void *hcd)
> +irqreturn_t xhci_irq(struct usb_hcd *hcd)
>  {
> -	return xhci_irq(hcd);
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +
> +	return xhci_irq_handler(xhci->interrupters[0]);
> +}
> +
> +irqreturn_t xhci_msi_irq(int irq, void *data)
> +{
> +	struct xhci_interrupter *ir = *(struct xhci_interrupter **)data;
> +
> +	return xhci_irq_handler(ir);
>  }
>  EXPORT_SYMBOL_GPL(xhci_msi_irq);

This approach does make the code simpler, and together
with patch v9 it seems to work. However, it also feels
somewhat fragile to me, since it now relies on the
interrupters array remaining stable across the relevant
resume/reinit paths. That may make future runtime
reinitialization or rebuilding of interrupters harder
and reduce flexibility.

Thanks,

Xu Rao

Re: [RFC PATCH 09/12] usb: xhci: optimize resuming from S4 (suspend-to-RAM)

[Michal Pecio]

In the subject, I think you meant S4 (suspend to *disk*).

On Thu,  5 Mar 2026 15:48:21 +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.

Actually, I don't understand this logic. Isn't it the case that:
- the Core Power Well always loses power in S3, S4 and S5
- the AUX Power Well usually keeps power in S3, S4 and S5, although it
  may also lose it in either state (in S3: weird/broken HW, FW bug)

So it seems that the driver should explicitly inspect root hub state
instead of guessing based on sleep level?

> Most of this is unnecessary because the data is restored from a saved
> image; only the xHCI registers lose their values.

Well, it was a simple way to guarantee that all driver data really are
at their defaults. So no stale entries on xhci->cmd_list, for example.

> 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 |  2 +-
>  drivers/usb/host/xhci.c     | 46 +++++++++++++++++++------------------
>  drivers/usb/host/xhci.h     |  1 +
>  3 files changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index fae75969e49a..46d977f9e5c5 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;
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index b22f10cfbe7b..e03f717d2314 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1075,9 +1075,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;
> @@ -1096,10 +1098,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");
>  		/*
>  		 * Some controllers might lose power during suspend, so wait
>  		 * for controller not ready bit to clear, just as in xHC init.
> @@ -1137,11 +1140,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);
> @@ -1165,28 +1168,27 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  		if (retval)
>  			return retval;
>  
> -		xhci_dbg(xhci, "// Disabling event ring interrupts\n");
> -		writel(STS_EINT, &xhci->op_regs->status);
> -		xhci_disable_interrupter(xhci, xhci->interrupters[0]);
> +		/* 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_ports; i > 0; i--)
> +			xhci_free_virt_devices_depth_first(xhci, i);
> +
> +		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);
>  
> -		xhci_dbg(xhci, "cleaning up memory\n");
> -		xhci_mem_cleanup(xhci);
>  		xhci_debugfs_exit(xhci);
> -		xhci_dbg(xhci, "xhci_stop completed - status = %x\n",
> -			    readl(&xhci->op_regs->status));
>  
> -		/* USB core calls the PCI reinit and start functions twice:
> +		xhci_dbg(xhci, "Re-initializing xHCI registers\n");
> +		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, "Allocate the xhci_hcd\n");
> -		retval = xhci_mem_init(xhci, GFP_KERNEL);
> -		if (retval)
> -			return retval;
> -
> -		xhci_dbg(xhci, "Initialize the xhci_hcd\n");
> -		xhci_init(hcd);
> -
>  		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..e6a51f1318c2 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,
> -- 
> 2.50.1
> 

Re: [RFC PATCH 06/12] usb: xhci: move initialization for lifetime objects

[Neronin, Niklas]

On 06/03/2026 0.14, Michal Pecio wrote:
> On Thu,  5 Mar 2026 15:48:18 +0100, Niklas Neronin wrote:
>> Initialize objects that exist for the lifetime of the driver only once,
>> rather than repeatedly. These objects do not require re-initialization
>> after events such as S4 (suspend-to-disk).
>>
>> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
>> ---
>>  drivers/usb/host/xhci-mem.c |  1 -
>>  drivers/usb/host/xhci.c     | 15 ++++++++-------
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 005b7bc1bfda..fae75969e49a 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -2001,7 +2001,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>>  	xhci->port_caps = NULL;
>>  	xhci->interrupters = NULL;
>>  
>> -	xhci->page_size = 0;
>>  	xhci->usb2_rhub.bus_state.bus_suspended = 0;
>>  	xhci->usb3_rhub.bus_state.bus_suspended = 0;
>>  }
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index d9519a9e9e17..338e93f39937 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -549,13 +549,6 @@ static int xhci_init(struct usb_hcd *hcd)
>>  	int retval;
>>  
>>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Starting %s", __func__);
>> -	spin_lock_init(&xhci->lock);
>> -
>> -	INIT_LIST_HEAD(&xhci->cmd_list);
> 
> Can we prove that this list is empty?
> 
> If there is any leftover garbage there, INIT_LIST_HEAD simply leaks it.
> Without it, the garbage stays on the list.

I assume you are referring to the resume path.
In this case xhci_cleanup_command_queue() is called before xhci_init().

xhci_resume()
├─xhci_mem_cleanup()
│  └─xhci_cleanup_command_queue()
└─xhci_init()

> 
>> -	INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout);
>> -	init_completion(&xhci->cmd_ring_stop_completion);
>> -	xhci_hcd_page_size(xhci);
>> -	memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
>>  
>>  	retval = xhci_mem_init(xhci, GFP_KERNEL);
>>  	if (retval)
>> @@ -5522,6 +5515,14 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>>  		dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>>  	}
>>  
>> +	spin_lock_init(&xhci->lock);
>> +	INIT_LIST_HEAD(&xhci->cmd_list);
>> +	INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout);
>> +	init_completion(&xhci->cmd_ring_stop_completion);
>> +	xhci_hcd_page_size(xhci);
>> +
>> +	memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
>> +
>>  	xhci_dbg(xhci, "Calling HCD init\n");
>>  	/* Initialize HCD and host controller data structures. */
>>  	retval = xhci_init(hcd);
>> -- 
>> 2.50.1
>>

Re: [RFC PATCH 09/12] usb: xhci: optimize resuming from S4 (suspend-to-RAM)

[Neronin, Niklas]

On 06/03/2026 8.52, raoxu wrote:
> On 2026-03-05 14:48 Niklas Neronin wrote:
> 
>> -	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);
>> @@ -1165,28 +1168,27 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>>  		if (retval)
>>  			return retval;
>>
>> -		xhci_dbg(xhci, "// Disabling event ring interrupts\n");
>> -		writel(STS_EINT, &xhci->op_regs->status);
>> -		xhci_disable_interrupter(xhci, xhci->interrupters[0]);
>> +		/* 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);
> 
> If the driver is moving toward multiple operational
> interrupters, the current reset logic seems to only
> reset interrupter 0 explicitly. Should the secondary
> operational interrupters be reset here as well? If so,
> that would likely require additional
> xhci_for_each_ring_seg() handling and may make the
> resume/reset path more complex.

Yes, I tried to keep my patches as strictly preparatory work for secondary
interrupters. Since this patch set does not actually add support for
secondary interrupters, I also did not introduce any interrupter‑loop logic.

That said, once secondary interrupters are added, something along these lines
will be required:

	for (int i = 0; i < xhci->max_interrupters; i++) {
		if (!xhci->interrupters[i])
			continue;
		
		xhci_for_each_ring_seg(xhci->interrupters[i]->event_ring->first_seg, seg)
			memset(seg->trbs, 0, sizeof(union xhci_trb) * TRBS_PER_SEGMENT);
	}

> 
>> +
>> +		for (int i = xhci->max_ports; i > 0; i--)
>> +			xhci_free_virt_devices_depth_first(xhci, i);
>> +
>> +		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);
>>
>> -		xhci_dbg(xhci, "cleaning up memory\n");
>> -		xhci_mem_cleanup(xhci);
>>  		xhci_debugfs_exit(xhci);
>> -		xhci_dbg(xhci, "xhci_stop completed - status = %x\n",
>> -			    readl(&xhci->op_regs->status));
>>
> 
> xhci_mem_cleanup() also clears software-side runtime
> state, not just rings. Without equivalent cleanup,
> state such as the command queue, cmd_timer, cmd_list,
> and TT-related list state may remain across
> reset-resume, which could lead to unexpected issues on
> some hardware.

Good point, I'll clear them all in v2.

Best Regards,
Niklas

> 
> Thanks,
> 
> Xu Rao