Re: [PATCH v4 0/3] xhci: Fix Stop Endpoint problems

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 4.11.2024 11.32, Michal Pecio wrote:
> v4 of the attempt to sort out Stop Endpoint retries and related issues.
> 
> General design:
> 
> 1/3 guarantee no infinite retries under any failure conditions, ever.
> 2/3 fix bugs and make Set TR Dequeue handler call invalidate/giveback.
> 3/3 eliminate cases of pointless Stop EP commands, depends on 2/3.
> 
> Changes since v3:
> 
> 2/3 no longer tries to add code cleanups (and bugs, as it turned out)
>      to a bugfix patch, and became much shorter as a result.
> 3/3 dropped a desperate attempt at detecting a halt/unlink race, which
>      I was unable to reproduce because it seems impossible - EP_HALTED
>      or SET_DEQ_PENDING remains set until after the halted URB is given
>      back to USB core, so we would never queue a pointless Stop command.
> 
> Also some commit message and comment cleanups in all three.
> And I documented another bug which 2/3 appears to (partly) fix.
> 
> 
> The state of other chips:
> 
> VIA VL805 and Etron EJ168A don't seem to have the "latent restart" bug
> so they don't care about this code, unless it causes pointless retries
> on an already stopped endpoint for 100ms, if it's enabled for them.
> 
> ASM1042 and ASM3142 have the bug, although not as common as on NEC. It
> seems to be particularly triggered by multiple endpoints cancelling at
> the same time, so for example, both loops below simultaneously:
> 
> while true; do timeout .3 yavta -c /dev/video0; done
> while true; do timeout .1 cat /dev/ttyUSB0 >/dev/null; done
> 
> On ASM3142 this triggers the bug about once per minute, and prints:
> 
> WARN Set TR Deq Ptr cmd failed due to incorrect slot or ep state.
> 
> The "mitigation" patch seems to be helping, because reverting it adds:
> 
> ERROR Transfer event TRB DMA ptr not part of current TD ep_index 6 comp_code 13
> Looking for event-dma 0000000003715ec0 trb-start 0000000003715ed0 trb-end 0000000003715ed0 seg-start 0000000003715000 seg-end 0000000003715ff0
> 
> Enabling the NEC workaround for this chip appears to fix the problem
> entirely and so far it only took one retry each time:
> 
> Stop ep completion ctx error, ctx_state 3
> 
> I have left this running overninght and nothing unexpected was logged
> in dmesg. IOW, no undesirable side effects known so far. But I haven't
> done much tests with bad cables yet, and ASMedia chips appear to be
> quite buggy and prone to strange internal race conditions. Examples:
> 
> - the ugly ifconfig up/down lockup issue on 3142 (not tried 1042 yet)
> - a pair of "stopped" events out of order preceding the above lockup
> - on 1042 Set Deq on streams seems to be no-op (streams are disabled)
> - Soft Retry preceding a Set Deq seems to undo the Set Deq's effect
> - Stop Endpoint *success* and then a spontaneous restart (1042 only)
> - spontaneous restart causes a halt, but transfer event is missing
> - TRB 15 (Stop EP) on the event ring (seen only once on 1042)
> 
> ASM1042 still not tested for compatilibity with the workaround at all.

Thanks for all the work you put into this series.
I added all patches to my for-usb-next branch and will send them to Greg shortly.

I think this code won't be NEC specific for very long as other vendors
have these similar issues.

Out of curiosity, what was the longest needed 'stop endpoint' retry time
you saw which still had a successful outcome?

Thanks
Mathias