[PATCH] usb: xhci: Simplify clearing the Event Interrupt bit

[PATCH] usb: xhci: Simplify clearing the Event Interrupt bit

[Michal Pecio]

USBSTS is mostly RW1C, so to clear EINT we should write just this
one bit. Remove pointless code which ORs the bit with current value
of the register, even though the bit is already known to be set,
and writes the result back, which clears all active RW1C flags.

We used to inadvertently clear PCD and SRE in this way. PCD isn't
used by the driver and SRE is only used at resume, so clearing them
should make no difference. Don't clear them anymore.

Tested by connecting and mounting a storage device on a few HCs.

Before: xhci_irq USBSTS 0x00000018 EINT PCD -> 0x00000000
        xhci_irq USBSTS 0x00000008 EINT -> 0x00000000
After:  xhci_irq USBSTS 0x00000018 EINT PCD -> 0x00000010 PCD
        xhci_irq USBSTS 0x00000018 EINT PCD -> 0x00000010 PCD

Some flags are RsvdZ - should be written as zero regardless of the
value read, so technically it was a bug. But no problems are known.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 246a2b1573ff..0338daba5f9a 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3209,10 +3209,9 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
 	/*
 	 * Clear the op reg interrupt status first,
 	 * so we can receive interrupts from other MSI-X interrupters.
-	 * Write 1 to clear the interrupt status.
+	 * USBSTS bits are write 1 to clear.
 	 */
-	status |= STS_EINT;
-	writel(status, &xhci->op_regs->status);
+	writel(STS_EINT, &xhci->op_regs->status);
 
 	/* This is the handler of the primary interrupter */
 	xhci_handle_events(xhci, xhci->interrupters[0], false);
-- 
2.48.1

Re: [PATCH] usb: xhci: Simplify clearing the Event Interrupt bit

[Mathias Nyman]

On 3/4/26 12:42, Michal Pecio wrote:
> USBSTS is mostly RW1C, so to clear EINT we should write just this
> one bit. Remove pointless code which ORs the bit with current value
> of the register, even though the bit is already known to be set,
> and writes the result back, which clears all active RW1C flags.
> 
> We used to inadvertently clear PCD and SRE in this way. PCD isn't
> used by the driver and SRE is only used at resume, so clearing them
> should make no difference. Don't clear them anymore.
> 
> Tested by connecting and mounting a storage device on a few HCs.
> 
> Before: xhci_irq USBSTS 0x00000018 EINT PCD -> 0x00000000
>          xhci_irq USBSTS 0x00000008 EINT -> 0x00000000
> After:  xhci_irq USBSTS 0x00000018 EINT PCD -> 0x00000010 PCD
>          xhci_irq USBSTS 0x00000018 EINT PCD -> 0x00000010 PCD
> 
> Some flags are RsvdZ - should be written as zero regardless of the
> value read, so technically it was a bug. But no problems are known.
> 

Thanks for cleaning this up.

Leaving the Port Change Detectd (PCD) bit uncleared worries me a bit.
Did suspend and resume still work properly after this?
especially something like wakeup from suspend (D3) on usb port connect

Thanks
Mathias

Re: [PATCH] usb: xhci: Simplify clearing the Event Interrupt bit

[Michal Pecio]

On Thu, 19 Mar 2026 22:33:49 +0200, Mathias Nyman wrote:
> Leaving the Port Change Detectd (PCD) bit uncleared worries me a bit.
> Did suspend and resume still work properly after this?
> especially something like wakeup from suspend (D3) on usb port connect

The bit seems to only be mentioned in 5.4.2 and 4.15.2.3, with no
indication that it's supposed to have any effect on HW behavior.

And I found no occurences of STS_PORT outside xhci.h.


But to be sure, I tested resuming by connection today.

Chipset xHCI and a PCIe card with Etron are fully functional.

NEC wakes from S3 but not s2idle, also if waking with a keyboard.
Maybe a HW bug: expects absence of core power.

Several other cards (VL805, FL1100, uPD720202, ASM1142, ASM3142) wake
from s2idle but not S3, including with a keyboard.

No difference if this patch is reverted, no difference if PCD is
explicitly cleared in xhci_suspend(). And same problem in Windows.
Looks like a HW deficiency in those cards.


I also ran into this problem (and applied the patch):
https://lore.kernel.org/linux-usb/20260316094811.1559471-1-xu.yang_2@nxp.com/

Re: [PATCH] usb: xhci: Simplify clearing the Event Interrupt bit

[Michal Pecio]

On Fri, 20 Mar 2026 21:18:09 +0100, Michal Pecio wrote:
> Several other cards (VL805, FL1100, uPD720202, ASM1142, ASM3142) wake
> from s2idle but not S3, including with a keyboard.
> 
> No difference if this patch is reverted, no difference if PCD is
> explicitly cleared in xhci_suspend(). And same problem in Windows.
> Looks like a HW deficiency in those cards.

Actually it's quite obvious: I know for sure that some of them derive
Vbus from the 12V rail, and chances are that the others also do.

Now I'm surprised that NEC and Etron cards bothered to make it work.
Looks like PCIe cards aren't the best option for testing suspend.

Re: [PATCH] usb: xhci: Simplify clearing the Event Interrupt bit

[Mathias Nyman]

On 3/20/26 22:28, Michal Pecio wrote:
> On Fri, 20 Mar 2026 21:18:09 +0100, Michal Pecio wrote:
>> Several other cards (VL805, FL1100, uPD720202, ASM1142, ASM3142) wake
>> from s2idle but not S3, including with a keyboard.
>>
>> No difference if this patch is reverted, no difference if PCD is
>> explicitly cleared in xhci_suspend(). And same problem in Windows.
>> Looks like a HW deficiency in those cards.
> 
> Actually it's quite obvious: I know for sure that some of them derive
> Vbus from the 12V rail, and chances are that the others also do.
> 
> Now I'm surprised that NEC and Etron cards bothered to make it work.
> Looks like PCIe cards aren't the best option for testing suspend.

Thanks for testing and clarifying

Adding this patch to queue

-Mathias