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

[Mathias Nyman]

On 5/29/26 13:53, Michal Pecio wrote:
> 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.

True, completely forgot about this patch from February.

The one in my branch was written to solve control endpoint possibly not starting
if dequeue is moved to a no-op TRB.

Solution ends up being basically the same.

> 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).

This should work

> 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.

This could at least be used to print a debug message.

> 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...

Enqueue is only advanced past link TRB if ring is empty, and both are then
set to the beginning of the next segment. Ring expansion isn't an issue here.
This is done to avoid moving dequeue to a link TRB.

Thanks
Mathias