On 21/08/2025 12.32, Michał Pecio wrote: > On 2025年8月18日 14:57:39 +0200, Niklas Neronin wrote: >> Set TR Deq TRB Error can occur for two reasons: > > Surely for other reasons too, like the infamous ASMedia case > I mentioned last time when this patch showed up here. I poorly worded it, the reasons are according to Set TR Deq TRB Error reasons outlined in xHCI specification Section 4.6.10, page 142. The entire series adds only error handling according to the reasons in the xHCI specification. > > In general, any illegal value in any TRB field is a TRB Error. > > And in general, I think it would be better if those log messages simply > named the error received from HW, without trying to speculate about > possible causes (and getting it wrong). It would be less misleading and > it would instantly give the keywoard to search for in the spec, without > grepping for the error message to find which event code triggers it. Good point, there can still be "out-of-spec" reasons for a TRB Error. A generic TRB error warning is better. > >> 1. Stream ID is zero or more than the Max Primary Streams. >> This is currently caught by xhci_virt_ep_to_ring(), at the start of the >> function. Thus, this commit does not add handling for this error. > > Nit: this is only true if ep->stream_info->num_streams at the time of > handling this event <= MaxPStreams at the time of command execution ;) > > So there are two theoretical bugs which could make this check fail. > >> 2. Stream ID is non-zero and Stream ID boundary check failed. > > Not sure what's the difference? Per spec, boundary check *is* checking > if Stream ID is in range, to prevent the xHC from writing to a Stream > Context outside actual SC Array and corrupting some random memory. > >> Add a warning detailing the exact reason for this failure and print >> the deq ptr from the Set TR Deq command. >> Macro 'SCTX_DEQ_MASK' is a mask for the TR Dequeue ptr bit field 63:4. >> >> Reuse local variable 'deq'; just change it to 'dma_addr_t', which is what >> it should have originally been. > > Not sure, this is an always-64bit value read from xHCI data structures. > > On a 32bit DMA system it probably won't have the high bits set if you > are reading it from the command (unless there was a bug), but later > other code will read it from HW and then all bets are off. Note that > a 64bit xHC may be installed in a 32bit system. And it may be buggy, > or it may have escaped the transfer ring due to broken Link TRB. As I understand it, the actual address values stored in the bitfields may vary depending on the system architecture, but the bitfield size itself remains consistent. On a 32-bit system, 'dma_addr_t' is 32 bits, and placing a 64-bit value into it will result in the upper 32 bits being dropped. Thank you for the review.