Re: [PATCH v2 2/3] usb: xhci: Simplify moving HW Dequeue Pointer past cancelled TDs

[Michal Pecio]

On Tue, 25 Feb 2025 16:55:49 +0200, Mathias Nyman wrote:
> On 25.2.2025 13.59, Michal Pecio wrote:
> > xhci_move_dequeue_past_td() uses a relatively complex and inefficient
> > procedure to find new dequeue position after the cancelled TD.
> > 
> > Replace it with a simpler function which moves dequeue immediately to
> > the first pending TD, or to enqueue if the ring is empty.
> > 
> > The outcome should be basically equivalent, because we only clear xHC
> > cache if it stopped or halted on some cancelled TD and moving past the
> > TD effectively means moving to the first remaining TD, if any.  
> 
> This new way relies on td_list being in sync and up to date.
> i.e. hardware dequeue can't be ahead of first TD in list.
> 
> One bad scenario could be something like:
> 
> class driver queues TD1
> class driver queues TD2
> Class driver cancels TD2, queue stop endpoint command
> (Class driver cancels TD1) (optional)
> 
> xHC hardware just completed TD1 and stop endpoint command at the same time,
> xHC hw may have advanced the hw dequeue to TD2, write event for stop endpoint command, and
> then write transfer event for TD1 completion. (xHC hardware may do things in odd order)

Hi,

I noticed that your xhci repository now contains a very similar patch.
The same problem seems to still apply.

I would say that the HW writing TD1 completion event after TD2 stopped
event would be a blatant spec violation and I don't recall seeing it
happen, but there is also a possibility that TD1 generates no event at
all or the event is missed due to a bug (no IOC, broken HW, whatever).

Then we could make things works by rewinding back to TD1.

A safer approach could be to retain the 'td' argument and use td->next
instead of list_first_entry(td_list).

Today we also have the dma_in_range() technology, so an efficient check
can be performed whether hw_dequeue lies between td->next and enqueue.
In such case something is clearly wrong and Set Deq seems unnecessary.

And one more problem: unconditionally advancing enqueue past a link TRB
creates risk that enqueue will enter deq_seg if the queued command
fails, which breaks ring expansion later. If we care...

Regards,
Michal