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

[Michal Pecio <michal.pecio@gmail.com>]

On Mon, 1 Jun 2026 13:45:37 +0300, Mathias Nyman wrote:
> On 5/29/26 13:53, Michal Pecio wrote:
> > 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.

This is the code:

+       if (list_empty(&ring->td_list)) {
+               if (trb_is_link(ring->enqueue))
+                       inc_enq_past_link(xhci, ring, 0);
+               ep->queued_deq_seg = ring->enq_seg;
+               ep->queued_deq_ptr = ring->enqueue;

This checks for empty td_list, but not for dequeue position,
so it can move enqueue to the beginning of ep_ring->deq_seg.
New URBs may be submitted before the command completes and
ep_ring->dequeue is updated, so ring expansion check may run
under abnormal conditions.

Now, I looked at xhci_ring_expansion_needed() and I think it
would consider the whole ring empty, but somebody could submit
a patch to optimize this function (use segment numbers and
TRB offsets to quickly calculate enq-deq distance) and nobody
will expect that this patch may cause ring expansions when
Set TR Dequeue is pending on a ring with no TDs.

And if the command fails, the xHC would progress to the newly
linked segment. Though to be honest, Set Deq failure is always
a complete disaster anyway, so I don't know if it's a big deal.

But any potential issues can be trivially prevented:

+               if (trb_is_link(ring->enqueue) && ring->enq_seg->next != ring->deq_seg)
+                       inc_enq_past_link(xhci, ring, 0);

Regards,
Michal