Re: [PATCH] usb: pci-quirks: always assert xHCI OS ownership

[Mario Limonciello <superm1@kernel.org>]

On 8/16/26 15:03, Michal Pecio wrote:
> On Sun, 16 Aug 2026 11:33:56 -0500, Mario Limonciello wrote:
>> On 8/14/26 20:45, Rishabh Jain wrote:
>>> The xHCI ownership protocol requires the OS driver to assert the HC
>>> OS Owned semaphore before using the host controller, then wait for
>>> HC BIOS Owned to clear if firmware owns it.
>>>
>>> quirk_usb_handoff_xhci() currently asserts OS Owned only when BIOS
>>> Owned is already set. If firmware leaves BIOS Owned clear, Linux
>>> uses the xHC while both ownership semaphores remain clear.
>>>
>>> On an AMD PROM21 xHCI controller (1022:43fc), this caused every S3
>>> resume to terminate Controller Restore State with USBSTS 0x401.
>>> Linux then reset the host controller, both root hubs and the USB
>>> Bluetooth adapter.
> 
> Not sure if this has anything to do with PROM21, or if some BIOS
> is just trying to use the xHC at resume because it's permitted to.
> Then it makes too many changes for Restore State to still work.
> 
> Potentially, such bugs may have happened and been left unsolved or
> "solved" with RESET_ON_RESUME quirks and other hacks.

What do you think about making this a series with each of those quirks 
dropped one-by-one?  The head patch (a variation of this one) could go 
to stable and the rest of the commits dropping those can go to linux-next.

All the authors and associated people with thoes quirks can be CC'ed to 
give us a shot at cleaning up all of them.  If any continue to fail then 
it's a simple revert for each one that failed.

> 
>>> The controller entered resume ready and halted with USBSTS 0x1.
>>> Endpoint state, 100 ms save/restore delays, scratchpads, the DCBAA,
>>> device contexts and command, event and transfer rings were verified not
>>> to cause the restore error.
>>>
>>> Asserting only HC OS Owned changed USBLEGSUP from 0x00000801 to
>>> 0x01000801 and eliminated the restore failure across four S3 cycles,
>>> including a stock-kernel test. Clearing USBLEGCTLSTS was independently
>>> verified to be unnecessary.
>>>
>>> Always assert OS Owned when the xHCI Legacy Support capability is
>>> present. Use the independently accessible ownership byte so firmware
>>> can update BIOS Owned without racing a 32-bit read-modify-write. Keep
>>> the existing BIOS handoff wait and legacy SMI cleanup unchanged.
>>>
>>> Fixes: 66d4eadd8d06 ("USB: xhci: BIOS handoff and HW initialization.")
>>> Tested-by: Rishabh Jain <rishabh.jain1198@gmail.com>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Rishabh Jain <rishabh.jain1198@gmail.com>
>>> ---
>>> Additional context:
>>>
>>> * Kernel Bugzilla #216470 documents the same USBSTS 0x401/reinitialize
>>>     behavior and its impact on attached USB devices:
>>>     https://bugzilla.kernel.org/show_bug.cgi?id=216470
>>>
>>> * Commit a7d57abcc8a5 ("xhci: workaround CSS timeout on AMD SNPS 3.0
>>>     xHC") is related workaround history: it tolerates a distinct AMD CSS
>>>     timeout and resets the controller on resume:
>>>     https://github.com/torvalds/linux/commit/a7d57abcc8a5bdeb53bbf8e87558e8e0a2c2a29d
>>>
>>> The external reports do not record their ownership semaphore values but
>>> are included as corroborating failure signatures that this might fix.
>>>
>>> drivers/usb/host/pci-quirks.c | 12 +++++++++---
>>>    1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
>>> index 0404489c2f6a..d76a4791b8f5 100644
>>> --- a/drivers/usb/host/pci-quirks.c
>>> +++ b/drivers/usb/host/pci-quirks.c
>>> @@ -1185,6 +1185,14 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
>>>    		dev_warn(&pdev->dev, "xHCI controller failing to respond");
>>>    		goto iounmap;
>>>    	}
>>> +
>>> +	/*
>>> +	 * The OS ownership semaphore must be asserted while the OS owns the
>>> +	 * xHC, even if firmware did not assert the BIOS ownership semaphore.
>>> +	 * Update only the OS ownership byte to avoid racing with firmware.
>>> +	 */
>>> +	writeb(readb(base + ext_cap_offset + 3) | BIT(0),
>>> +	       base + ext_cap_offset + 3);
>>
>> I'm assuming you are actually meaning XHCI_EXT_CAPS_PM for the 3 here.
>> Why are you doing all this math?
>>
>> We already have the defines XHCI_HC_OS_OWNED, can't you just use that?
>>
>> And for that matter it sounds like you are really proposing to just
>> remove this check but adding more complexity in the process.
>>
>> 	if (val & XHCI_HC_BIOS_OWNED)
> 
> All explained by the comment above and xHCI 4.22.1.
> 
> Though curiously, while HW is required to enable doing the sensible
> thing, the spec doesn't clearly state that SW must actually do it...
> 
> And BTW, I checked if any of my HCs refuses to honor DWORD writes to
> this register to protect SW from itself, but none does.
> 
>> I guess the way I would do this is at least leave a debug breadcrumb
> 
> Is anyone ever going to look at that pci_debug()?
> 
> If it works, who cares if it was claimed by the BIOS or not.
> If it doesn't, you know that it was. And the full register is dumped.
> 

I guess I see your point.  This debug message I mentioned can probably 
be dropped in a v3.

>> since you're reading the register something like this:
>>
>> val = readl(base + ext_cap_offset);
>> if (val & XHCI_HC_BIOS_OWNED)
>> 	pci_debug(pdev, "BIOS owns XHCI HC\n"0;
>> writel(val | XHCI_HC_OS_OWNED, base + ext_cap_offset);
>> timeout = handshake(...)
>> if (timeout && (val & XHCI_HC_BIOS_OWNED)) {
>> 	dev_warn(...)
>> 	writel(val & ~XHCI_HC_BIOS_OWNED, base + ext_cap_offset);
>> }
>>
>> Then you have a single read, no extra writes.
>>
>>>    	val = readl(base + ext_cap_offset);
>>>    
>>>    	/* Auto handoff never worked for these devices. Force it and continue */
>>> @@ -1195,10 +1203,8 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
>>>    		writel(val, base + ext_cap_offset);
>>>    	}
>>>    
>>> -	/* If the BIOS owns the HC, signal that the OS wants it, and wait */
>>> +	/* If the BIOS owns the HC, wait for it to hand over control */
>>>    	if (val & XHCI_HC_BIOS_OWNED) {
>>> -		writel(val | XHCI_HC_OS_OWNED, base + ext_cap_offset);
>>> -
>>>    		/* Wait for 1 second with 10 microsecond polling interval */
>>>    		timeout = handshake(base + ext_cap_offset, XHCI_HC_BIOS_OWNED,
>>>    				0, 1000000, 10);
>>