usb: xhci: Don't unchain link TRBs on quirky HCs
Subsystems (2)
PATCH 1/1 usb: xhci: Don't unchain link TRBs on quirky HCs
Some old HCs ignore transfer ring link TRBs whose chain bit is unset. This breaks endpoint operation and sometimes makes it execute other ring's TDs, which may corrupt their buffers or cause unwanted device action. We avoid this by chaining all link TRBs on affected rings.
Fix an omission which allows them to be unchained by cancelling TDs.
The patch was tested by reproducing this condition on an isochronous endpoint (non-power-of-two TDs are sometimes split not to cross 64K) and printing link TRBs in trb_to_noop() on good and buggy HCs.
Actual hardware malfunction is rare since it requires Missed Service Error shortly before the unchained link TRB, at least on NEC and AMD. I have never seen it after commit bb0ba4cb1065 ("usb: xhci: Apply the link chain quirk on NEC isoc endpoints"), but it's Russian roulette and I can't test all affected hosts and workloads. Fairly often MSEs happen after cancellation because the endpoint was stopped.
--- drivers/usb/host/xhci-ring.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)
drivers/usb/host/xhci-ring.c +16 -11
@@ -128,11 +128,11 @@ static void inc_td_cnt(struct urb *urb) urb_priv->num_tds_done++; } -static void trb_to_noop (union xhci_trb *trb,u32 noop_type) +static void trb_to_noop (union xhci_trb *trb,u32 noop_type,bool unchain_links) {if (trb_is_link (trb)) { -/* unchain chained link TRBs */ - trb->link.control &=cpu_to_le32 (~TRB_CHAIN ); +if (unchain_links) + trb->link.control &=cpu_to_le32 (~TRB_CHAIN ); }else { trb->generic.field[0 ] =0 ; trb->generic.field[1 ] =0 ;
@@ -465,7 +465,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,xhci_dbg (xhci,"Turn aborted command %p to no-op\n" , i_cmd->command_trb); -trb_to_noop (i_cmd->command_trb,TRB_CMD_NOOP ); +trb_to_noop (i_cmd->command_trb,TRB_CMD_NOOP , false);/* * caller waiting for completion is called when command
@@ -797,13 +797,18 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,* (The last TRB actually points to the ring enqueue pointer, which is not part * of this TD.) This is used to remove partially enqueued isoc TDs from a ring. */ -static void td_to_noop (struct xhci_td *td,bool flip_cycle) +static void td_to_noop (struct xhci_hcd *xhci,struct xhci_virt_ep *ep, +struct xhci_td *td,bool flip_cycle) { +bool unchain_links;struct xhci_segment *seg = td->start_seg;union xhci_trb *trb = td->start_trb; +/* link TRBs should now be unchained, but some old HCs expect otherwise */ + unchain_links = !xhci_link_chain_quirk (xhci, ep->ring ? ep->ring->type :TYPE_STREAM ); +while (1 ) { -trb_to_noop (trb,TRB_TR_NOOP ); +trb_to_noop (trb,TRB_TR_NOOP , unchain_links);/* flip cycle if asked to */ if (flip_cycle && trb != td->start_trb && trb != td->end_trb)
@@ -1091,16 +1096,16 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)"Found multiple active URBs %p and %p in stream %u?\n" , td->urb, cached_td->urb, td->urb->stream_id); -td_to_noop (cached_td, false); +td_to_noop (xhci, ep, cached_td, false); cached_td->cancel_status =TD_CLEARED ; } -td_to_noop (td, false); +td_to_noop (xhci, ep, td, false); td->cancel_status =TD_CLEARING_CACHE ; cached_td = td;break ; } }else { -td_to_noop (td, false); +td_to_noop (xhci, ep, td, false); td->cancel_status =TD_CLEARED ; } }
@@ -1125,7 +1130,7 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)continue ;xhci_warn (xhci,"Failed to clear cancelled cached URB %p, mark clear anyway\n" , td->urb); -td_to_noop (td, false); +td_to_noop (xhci, ep, td, false); td->cancel_status =TD_CLEARED ; } }
@@ -4273,7 +4278,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,*/ urb_priv->td[0 ].end_trb = ep_ring->enqueue;/* Every TRB except the first & last will have its cycle bit flipped. */ -td_to_noop (&urb_priv->td[0 ], true); +td_to_noop (xhci, xep, &urb_priv->td[0 ], true);/* Reset the ring enqueue back to the first TRB and its cycle bit. */ ep_ring->enqueue = urb_priv->td[0 ].start_trb;
Signature · 1 line
2.48.1
Quoted 11 lines · Michal Pecio
Some old HCs ignore transfer ring link TRBs whose chain bit is unset. This breaks endpoint operation and sometimes makes it execute other ring's TDs, which may corrupt their buffers or cause unwanted device action. We avoid this by chaining all link TRBs on affected rings.
Fix an omission which allows them to be unchained by cancelling TDs.
The patch was tested by reproducing this condition on an isochronous endpoint (non-power-of-two TDs are sometimes split not to cross 64K) and printing link TRBs in
trb_to_noop()on good and buggy HCs.
Quoted 8 lines
Actual hardware malfunction is rare since it requires Missed Service Error shortly before the unchained link TRB, at least on NEC and AMD. I have never seen it after commit
bb0ba4cb1065("usb: xhci: Apply the link chain quirk on NEC isoc endpoints"), but it's Russian roulette and I can't test all affected hosts and workloads. Fairly often MSEs happen after cancellation because the endpoint was stopped.Signed-off-by Michal Pecio <[email protected]>
Makes sense, thanks for fixing this
Quoted 33 lines
--- drivers/usb/host/xhci-ring.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)drivers/usb/host/xhci-ring.c +4
-4@@ -128,11 +128,11 @@ static void inc_td_cnt(struct urb *urb) urb_priv->num_tds_done++; } -static void trb_to_noop (union xhci_trb *trb,u32 noop_type) +static void trb_to_noop (union xhci_trb *trb,u32 noop_type,bool unchain_links) {if (trb_is_link (trb)) { -/* unchain chained link TRBs */ - trb->link.control &=cpu_to_le32 (~TRB_CHAIN ); +if (unchain_links) + trb->link.control &=cpu_to_le32 (~TRB_CHAIN ); }else { trb->generic.field[0 ] =0 ; trb->generic.field[1 ] =0 ;@@ -465,7 +465,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,xhci_dbg (xhci,"Turn aborted command %p to no-op\n" , i_cmd->command_trb); -trb_to_noop (i_cmd->command_trb,TRB_CMD_NOOP ); +trb_to_noop (i_cmd->command_trb,TRB_CMD_NOOP , false);/* * caller waiting for completion is called when command
drivers/usb/host/xhci-ring.c static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, · line 801
Quoted 6 lines
drivers/usb/host/xhci-ring.c +2
-1@@ -797,13 +797,18 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,* (The last TRB actually points to the ring enqueue pointer, which is not part * of this TD.) This is used to remove partially enqueued isoc TDs from a ring. */ -static void td_to_noop (struct xhci_td *td,bool flip_cycle) +static void td_to_noop (struct xhci_hcd *xhci,struct xhci_virt_ep *ep, +struct xhci_td *td,bool flip_cycle)
we could avoid passing xhci pointer to td_to_noop() and just grab it from the xhci_virt_ep structure instead. i.e. ep->xhci
Otherwise this looks good to me
Thanks
Mathias
Quoted 6 lines · Mathias Nyman
drivers/usb/host/xhci-ring.c static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, · line 801
Quoted 2 lines · Michal Pecio
drivers/usb/host/xhci-ring.c +2
-0+static void td_to_noop (struct xhci_hcd *xhci,struct xhci_virt_ep *ep, +struct xhci_td *td,bool flip_cycle)we could avoid passing xhci pointer to
td_to_noop()and just grab it from thexhci_virt_epstructure instead. i.e.ep->xhci
I can do a v2 if you want.
But OTOH, I didn't expect such pointer to exist (though I'm sure I must have seen it many times) because it doesn't seem strictly necessary.
Maybe do the reverse and get rid of ep->xhci, or stop adding new users and clean up existing ones along the way?
Main users are invalidate_cancelled_tds()/giveback_invalidated_tds(), their callers all have xhci and could easily supply it to them. And we even discussed removing the latter completely, but I got sidetracked by issues with URB_ZERO_PACKET.
Another user is xhci_handle_cmd_set_deq() which already has xhci. I have a compile-tested patch which removes it completely right now, it took five minutes and zero mental effort to prepare.
Regards,
Michal
Quoted 13 lines · Michal Pecio
Quoted 6 lines · Mathias Nyman
drivers/usb/host/xhci-ring.c static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci, · line 801
Quoted 2 lines · Michal Pecio
drivers/usb/host/xhci-ring.c +2
-0+static void td_to_noop (struct xhci_hcd *xhci,struct xhci_virt_ep *ep, +struct xhci_td *td,bool flip_cycle)we could avoid passing xhci pointer to
td_to_noop()and just grab it from thexhci_virt_epstructure instead. i.e.ep->xhciI can do a v2 if you want.
But OTOH, I didn't expect such pointer to exist (though I'm sure I must have seen it many times) because it doesn't seem strictly necessary.
Quoted 7 lines
Maybe do the reverse and get rid of
ep->xhci, or stop adding new users and clean up existing ones along the way?Main users are invalidate_cancelled_tds()/giveback_invalidated_tds(), their callers all have xhci and could easily supply it to them. And we even discussed removing the latter completely, but I got sidetracked by issues with
URB_ZERO_PACKET.
Ok, I'll take this v1 now as it is.
It prevents a potential issue, and it's tested by you.
We can then later figure out how we want to clean up and refactor things.
Thanks
Mathias