Re: [PATCH 1/2] usb: xhci: fix Command Aborting

["Neronin, Niklas" <niklas.neronin@linux.intel.com>]

On 23/03/2026 13.24, Michal Pecio wrote:
> On Mon, 23 Mar 2026 12:25:00 +0200, Neronin, Niklas wrote:
>> On 21/03/2026 15.30, Michal Pecio wrote:
>>> On Mon, 16 Mar 2026 15:27:19 +0100, Niklas Neronin wrote:  
>>>> Aborting the command ring requires setting the Command Abort (CA)
>>>> bit in the 64-bit Command Ring Control Register (CRCR).
>>>> Historically, several attempts have been made to implement this
>>>> correctly, but each introduced its own problems. This patch fixes
>>>> the final remaining issue: accidental modification of the Command
>>>> Ring Pointer (i'll abbreviate it to CRP).
>>>>
>>>> Originally [1], the full 64-bit CRCR value was read and written
>>>> back after setting CA. This is a bit unnecessary, only RsvdP bits
>>>> (5:4) should be read and written back (for future-proofing). All
>>>> other writable fields read as zero.
>>>>
>>>> Later patches attempted to solve an issues, caused by 64-bit writes
>>>> being split into two 32-bit writes. Writing the lower 31:0 bits
>>>> first immediately stopped the ring (CRR=0), and the following
>>>> upper-half write then overwrote part of CRP with zeroes, thus
>>>> corrupting the CRP. Patch [2] avoided this by writing only the
>>>> lower 31:0 bits with CA set, but that broke controllers that latch
>>>> 64-bit registers only when the upper bits are written, as reported
>>>> in [3].  
>>>
>>> I too have HW which waits for the high DWORD after low DWORD write.
>>>
>>> And I have HW which doesn't. If I write 0xdeadbeef to the high DWORD
>>> after waiting for CRR=0, some HW will ignore the write and some will
>>> IOMMU fault at 0xdeadbeefsomething.
>>>
>>> The abort itself takes a few microseconds in my tests.  
>>
>> Yes, abort completion itself is usually very fast. The 5 second
>> timeout comes directly from the xHCI 1.9 specification, which
>> explicitly allows for that duration.
>>
>> 4.6.1.2 Aborting a Command:
>> "If software doesn't see CRR negated in a timely manner (e.g. longer
>>  than 5 seconds), then it should assume that there are larger problems
>>  with the xHC and assert HCRST"
>>
>> The timeout could probably be reduced to 1 second, but I did not do
>> that since I do not have evidence that a shorter timeout would be
>> safe across all hardware.
> 
> As noted in my later response to patch 2/2, in rare corner cases
> ASMedia command abort can take a few seconds and still succeed.
> 
>>> Is this race causing problems in the real world?  
>>
>> I believe so, given that a fix was proposed specifically to address it:
>> https://lore.kernel.org/all/20211008092547.3996295-5-mathias.nyman@linux.intel.com/#t
> 
> It does look like a problem which people would be unlikely to discover
> without being affected by it, but the patch says nothing.
>
> Hi Pavankumar,
> 
> Could you elaborate on the above issue, what was the affected host
> controller and what sort of workloads made this race turn bad?
> 
> In theory the two DWORD writes could be separated by some NMI/SMI
> hitting the CPU or by heavy traffic on the I/O bus.
> 
> 
> And if probelms are known to happen here, then what about other users
> of xhci_write_64(), for example updating ERDP aka erst_dequeue?

They are not related.

The ERDP bit field is 63:4 and can therefore hold any TRB address.
It is also not gated or "locked" behind a control bit in the same register.

Other 63:6 bit fields address in xHCI are all base addresses, which are
always programmed with correctly aligned values. The only 63:6‑bit
address field that is not a base address is the Command Ring Pointer.

> 
>> The current implementation does attempt to change the CRP.
>> The sequence is as follows:
>>  1. Fetch the next TRB.
>>  2. If the TRB is a Link TRB, advance to the next TRB.
>>  3. Get the DMA address of that TRB.
>>  4. Write the lower DWORD with the new CRP address and the CA bit set.
>>  5. Write the upper DWORD with the new CRP address.
>>
>> This results in two possible final states, depending on how quickly
>> CRR is cleared:
>>  1. The CRP is not updated at all.
>>  2. Only the upper DWORD of the CRP is updated.
> 
> I stand corrected, I wasn't aware of some details. That said,
> 
> 1. is a valid outcome and not a bug, jumping to the next TRB is not
>    necessary, it was a workaround for previous issues.
> 2. is a bug, but it shouldn't matter because the ring has one segment
> 
> But I did have problems on NEC and AMD, which means that something else
> is also possible. I think I will need to debug it deeper, but I suspect
> that writing high DWORD alone is enough to trigger bit 4:5 truncation
> or overwrite low DWORD with some garbage.
> 
>> So you believe that when waiting for CRR=0 any software writes to
>> CRCR, which are not immediately acted on, i.e. Command Stop/Abort,
>> are instead buffered internally?
> 
> We all believe that some HW does nothing and waits for high DWORD.
> 
> Other HW is seen to begin aborting more or less immediately. However,
> until the abort is complete, any subsequent write to high DWORD may
> be ignored because we can't change CRP while CRR=1.
> 
> Problems begin if CRR=0 before we write high DWORD. CRP bits become
> writeable and any write may trigger truncation of bits 4:5.

Should this be bits 31:6?

> 
> Note that if the HW interprets this as a single write to high DWORD
> separate from the previous abort operation (which it has already
> completed) then this is out of spec, because we should write low-high.

How I see it; hardware which requires an upper DWORD write will "buffer"
the lower DWORD and wait for the corresponding upper DWORD write.

However, because the specification states that the CRP and RCS are ignored
while CRR=1, the lower DWORD write ignores lower CRP bits and RCS bit.

As a result, if the upper DWORD is written after CRR=0, only upper CRP bits
and RsvdP bits are updated.
Conversely, if the upper DWORD is written before CRR=0, only RsvdP bits are
updated.

> 
> It seems that my Etron HC ignores this high DWORD write after CRR=0.
> I can write 0xdeadbeef there and it continues working normally.
> Or maybe it's broken and should get the disable 64 bits quirk.
> 
> But NEC and AMD start getting "mismatched completion" errors, so it
> seems that their CRP is corrupted somehow. And I could probably replace
> waiting for CRR=0 with usleep(20) and get the same corruption.
> 
> I will later look closer what happens here.
> 
> Regards,
> Michal