[PATCH 0/2] usb: xhci: simplify interrupter handling

[PATCH 0/2] usb: xhci: simplify interrupter handling

[Niklas Neronin]

Make the interrupter type explicit in the driver, simplifying the code
paths and clarifying intent. These changes make it easier to add support
for secondary interrupters.

No functional changes are introduced; behavior remains the same.

Niklas Neronin (2):
  usb: xhci: move Primary Interrupter IMODI setup
  usb: xhci: add interrupter type

 drivers/usb/host/xhci-sideband.c | 1 +
 drivers/usb/host/xhci.c          | 9 +++++++--
 drivers/usb/host/xhci.h          | 8 ++++++++
 3 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.50.1

[PATCH 1/2] usb: xhci: move Primary Interrupter IMODI setup

[Niklas Neronin]

Move IMODI programming into the Primary Interrupter initialization path,
keeping all interrupter-related setup in a single place.

xHCI platform drivers set 'xhci->imod_interval' well before xhci_init()
is called, so the value is available when the Primary Interrupter is
initialized.

The IMODI value does not change unless explicitly written by software.
See xHCI Specification v1.2, section 5.5.2.2.

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

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index b3ba16b9718c..fcf8b486b0e0 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -578,6 +578,7 @@ static int xhci_init(struct usb_hcd *hcd)
 
 	/* Initialize the Primary interrupter */
 	xhci_add_interrupter(xhci, 0);
+	xhci_set_interrupter_moderation(xhci->interrupters[0], xhci->imod_interval);
 	xhci->interrupters[0]->isoc_bei_interval = AVOID_BEI_INTERVAL_MAX;
 
 	/* Initializing Compliance Mode Recovery Data If Needed */
@@ -664,8 +665,6 @@ int xhci_run(struct usb_hcd *hcd)
 	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

[PATCH 2/2] 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.

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 fcf8b486b0e0..c4e0c1cfb94e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -314,6 +314,9 @@ int xhci_enable_interrupter(struct xhci_interrupter *ir)
 {
 	u32 iman;
 
+	if (ir->type == INTR_NOOP)
+		return 0;
+
 	if (!ir || !ir->ir_set)
 		return -EINVAL;
 
@@ -331,6 +334,9 @@ int xhci_disable_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
 {
 	u32 iman;
 
+	if (ir->type == INTR_NOOP)
+		return 0;
+
 	if (!ir || !ir->ir_set)
 		return -EINVAL;
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 2b0796f6d00e..59840d613e94 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 = 0,
+	/* 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

Re: [PATCH 2/2] usb: xhci: add interrupter type

[Dan Carpenter]

Hi Niklas,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Niklas-Neronin/usb-xhci-move-Primary-Interrupter-IMODI-setup/20260128-211728
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20260128131504.124322-3-niklas.neronin%40linux.intel.com
patch subject: [PATCH 2/2] usb: xhci: add interrupter type
config: x86_64-randconfig-161-20260129 (https://download.01.org/0day-ci/archive/20260129/202601290847.Cb6aLduh-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
smatch version: v0.5.0-8994-gd50c5a4c

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202601290847.Cb6aLduh-lkp@intel.com/

smatch warnings:
drivers/usb/host/xhci.c:320 xhci_enable_interrupter() warn: variable dereferenced before check 'ir' (see line 317)
drivers/usb/host/xhci.c:340 xhci_disable_interrupter() warn: variable dereferenced before check 'ir' (see line 337)

vim +/ir +320 drivers/usb/host/xhci.c

bea5892d0ed274 Mathias Nyman  2025-04-10  313  int xhci_enable_interrupter(struct xhci_interrupter *ir)
52dd0483e822d0 Mathias Nyman  2023-02-02  314  {
52dd0483e822d0 Mathias Nyman  2023-02-02  315  	u32 iman;
52dd0483e822d0 Mathias Nyman  2023-02-02  316  
486b278520fd70 Niklas Neronin 2026-01-28 @317  	if (ir->type == INTR_NOOP)
                                                    ^^^^^^^^
Dereferenced.

486b278520fd70 Niklas Neronin 2026-01-28  318  		return 0;
486b278520fd70 Niklas Neronin 2026-01-28  319  
52dd0483e822d0 Mathias Nyman  2023-02-02 @320  	if (!ir || !ir->ir_set)
                                                    ^^^
Checked too late.

52dd0483e822d0 Mathias Nyman  2023-02-02  321  		return -EINVAL;
52dd0483e822d0 Mathias Nyman  2023-02-02  322  
bf9cce90da311a Niklas Neronin 2025-05-15  323  	iman = readl(&ir->ir_set->iman);
ff9a09b3e09c7b Niklas Neronin 2025-08-19  324  	iman &= ~IMAN_IP;
9f7f74735ac295 Niklas Neronin 2025-05-15  325  	iman |= IMAN_IE;
bf9cce90da311a Niklas Neronin 2025-05-15  326  	writel(iman, &ir->ir_set->iman);
52dd0483e822d0 Mathias Nyman  2023-02-02  327  
f5bce30ad25e74 Niklas Neronin 2025-05-15  328  	/* Read operation to guarantee the write has been flushed from posted buffers */
bf9cce90da311a Niklas Neronin 2025-05-15  329  	readl(&ir->ir_set->iman);
52dd0483e822d0 Mathias Nyman  2023-02-02  330  	return 0;
52dd0483e822d0 Mathias Nyman  2023-02-02  331  }
52dd0483e822d0 Mathias Nyman  2023-02-02  332  
e1db856bd28891 Niklas Neronin 2025-05-15  333  int xhci_disable_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
52dd0483e822d0 Mathias Nyman  2023-02-02  334  {
52dd0483e822d0 Mathias Nyman  2023-02-02  335  	u32 iman;
52dd0483e822d0 Mathias Nyman  2023-02-02  336  
486b278520fd70 Niklas Neronin 2026-01-28 @337  	if (ir->type == INTR_NOOP)
                                                    ^^^^^^^^

486b278520fd70 Niklas Neronin 2026-01-28  338  		return 0;
486b278520fd70 Niklas Neronin 2026-01-28  339  
52dd0483e822d0 Mathias Nyman  2023-02-02 @340  	if (!ir || !ir->ir_set)
                                                    ^^^
Same.

52dd0483e822d0 Mathias Nyman  2023-02-02  341  		return -EINVAL;
52dd0483e822d0 Mathias Nyman  2023-02-02  342  
bf9cce90da311a Niklas Neronin 2025-05-15  343  	iman = readl(&ir->ir_set->iman);
ff9a09b3e09c7b Niklas Neronin 2025-08-19  344  	iman &= ~IMAN_IP;
9f7f74735ac295 Niklas Neronin 2025-05-15  345  	iman &= ~IMAN_IE;
bf9cce90da311a Niklas Neronin 2025-05-15  346  	writel(iman, &ir->ir_set->iman);
52dd0483e822d0 Mathias Nyman  2023-02-02  347  
bf9cce90da311a Niklas Neronin 2025-05-15  348  	iman = readl(&ir->ir_set->iman);
e1db856bd28891 Niklas Neronin 2025-05-15  349  	if (iman & IMAN_IP)
e1db856bd28891 Niklas Neronin 2025-05-15  350  		xhci_dbg(xhci, "%s: Interrupt pending\n", __func__);
e1db856bd28891 Niklas Neronin 2025-05-15  351  
52dd0483e822d0 Mathias Nyman  2023-02-02  352  	return 0;
52dd0483e822d0 Mathias Nyman  2023-02-02  353  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] usb: xhci: add interrupter type

[Wesley Cheng]

On 1/28/2026 5:15 AM, Niklas Neronin wrote:
> xhci-sideband creates a secondary interrupter without an associated IRQ.
> Such interrupters are non-operational and cannot enabled or disabled.
> 

Hi Niklas,

Is the wording here correct?  Every interrupter should have a physical 
interrupt line, however, it may not be owned by the processor running 
Linux. (ie offloaded to another DSP)

> 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.
> 
> 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 fcf8b486b0e0..c4e0c1cfb94e 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -314,6 +314,9 @@ int xhci_enable_interrupter(struct xhci_interrupter *ir)
>   {
>   	u32 iman;
>   
> +	if (ir->type == INTR_NOOP)
> +		return 0;
> +
>   	if (!ir || !ir->ir_set)
>   		return -EINVAL;
>   
> @@ -331,6 +334,9 @@ int xhci_disable_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
>   {
>   	u32 iman;
>   
> +	if (ir->type == INTR_NOOP)
> +		return 0;
> +

In some of my previous discussions with Mathias, I think he mentioned there 
was a use case where Linux would handle interrupts for both the primary and 
secondary interrupters.  I didn't get much more info, but maybe it was for 
segmenting events occurring on different EPs.  However, I'm not sure where 
that control/trigger comes from, ie from the class driver level, or within 
xHCI.

Just wondering if this is going to lay out the groundwork for that to be 
added, as in xhci_sideband_create_interrupter() we're just assuming that 
its a INTR_NOOP versus allowing it to be configured by the caller.

Thanks
Wesley Cheng

>   	if (!ir || !ir->ir_set)
>   		return -EINVAL;
>   
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 2b0796f6d00e..59840d613e94 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 = 0,
> +	/* 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;

Re: [PATCH 2/2] usb: xhci: add interrupter type

[Neronin, Niklas]

On 04/02/2026 3.17, Wesley Cheng wrote:
> 
> 
> On 1/28/2026 5:15 AM, Niklas Neronin wrote:
>> xhci-sideband creates a secondary interrupter without an associated IRQ.
>> Such interrupters are non-operational and cannot enabled or disabled.
>>
> 
> Is the wording here correct?  Every interrupter should have a physical interrupt line, however, it may not be owned by the processor running Linux. (ie offloaded to another DSP)
> 

In this case, the xhci driver allocates an xhci-sideband interrupter
structure but does not bind it to an IRQ or register an interrupt handler.
Event processing for this interrupter is performed by the sideband code via
polling of the event ring rather than through an interrupt.

From the xhci core driver's perspective, the interrupter structure exists,
but it requires no interrupt management. Beyond allocation and teardown,
the xhci core driver does not interact with it, which is why it is treated
as a "noop" interrupter.

>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index fcf8b486b0e0..c4e0c1cfb94e 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -314,6 +314,9 @@ int xhci_enable_interrupter(struct xhci_interrupter *ir)
>>   {
>>       u32 iman;
>>   +    if (ir->type == INTR_NOOP)
>> +        return 0;
>> +
>>       if (!ir || !ir->ir_set)
>>           return -EINVAL;
>>   @@ -331,6 +334,9 @@ int xhci_disable_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
>>   {
>>       u32 iman;
>>   +    if (ir->type == INTR_NOOP)
>> +        return 0;
>> +
> 
> In some of my previous discussions with Mathias, I think he mentioned there was a use case where Linux would handle interrupts for both the primary and secondary interrupters.  I didn't get much more info, but maybe it was for segmenting events occurring on different EPs.  However, I'm not sure where that control/trigger comes from, ie from the class driver level, or within xHCI.

In such a scenario, secondary interrupters would be associated with an IRQ
and handled similarly to the primary interrupter, using the same interrupt
handling paths in the xhci driver.

> 
> Just wondering if this is going to lay out the groundwork for that to be added, as in xhci_sideband_create_interrupter() we're just assuming that its a INTR_NOOP versus allowing it to be configured by the caller.
> 

All interrupters are stored in the 'xhci->interrupter[]' array, and this
change introduces a way to distinguish interrupters that are handled by
the xhci core from those that are not.

Thanks,
Niklas