xhci: Simplify TD cancellation and drop some states
Subsystems (2)
cover xhci: Simplify TD cancellation and drop some states
Hi,
Thoughts about this?
Thanks to asynchronous giveback we can give back TDs at any time we want without dropping xhci->lock and worrying what we might find when we take it back. So give back TDs as soon as they are no longer needed instead of keeping them around in TD_CLEARED state and using a second pass through the list to give them back.
This turns out to be a surprisingly simple patch and I gave it a quick test and it's working fine.
The second patch removes another almost meaningless cancel status and shortens the code a bit while adding functionality which may or may not turn out to be useful. And hopefully won't be harmful?
That too was tested, by patching xhci_move_dequeue_past_td() to fail randomly and unlinking from multiple streams on UAS. Works exactly as described in the commit message.
Overall, the driver lost 40 lines of code and 2KB in binary size. The latter is surprising, I think I haven't removed 2KB worth of code, but maybe it affects how gcc inlines or otherwise changes functions. YMMV.
Regards,
Michal
RFC PATCH 1/2 usb: xhci: Drop the TD_CLEARED cancel status
There seems to be no need to keep invalidated TDs around and require callers to follow up with xhci_giveback_invalidated_tds(), so make xhci_invalidate_cancelled_tds() give back invalidated TDs right away.
TD_CLEARED cancel status is no longer useful and therefore removed. Code complexity and overhead of repeated list traversals are reduced.
There is no more need to debug interactions between these functions, so a big source of xhci_dbg() noise (and potential bugs) goes away. From now, "Removing cancelled TD starting at ..." really means that the TD is being removed, unless one of the errors below is logged or dynamic debug indicates that Set TR Dequeue is queued or deferred.
Also clean up some debug noise from xhci_handle_cmd_set_deq(), which practically duplicates xhci_invalidate_cancelled_tds() messages that will be printed just before this completion handler returns.
--- drivers/usb/host/xhci-ring.c | 49 +++++++----------------------------- drivers/usb/host/xhci.h | 1 - 2 files changed, 9 insertions(+), 41 deletions(-)
drivers/usb/host/xhci-ring.c +9 -40
@@ -927,30 +927,6 @@ static void xhci_dequeue_td(struct xhci_hcd *xhci, struct xhci_td *td, struct xhxhci_td_cleanup (xhci, td, ring, status); } -/* Complete the cancelled URBs we unlinked from td_list. */ -static void xhci_giveback_invalidated_tds (struct xhci_virt_ep *ep) -{ -struct xhci_ring *ring; -struct xhci_td *td, *tmp_td; - -list_for_each_entry_safe (td, tmp_td, &ep->cancelled_td_list, - cancelled_td_list) { - - ring =xhci_urb_to_transfer_ring (ep->xhci, td->urb); - -if (td->cancel_status ==TD_CLEARED ) { -xhci_dbg (ep->xhci,"%s: Giveback cancelled URB %p TD\n" , - __func__, td->urb); -xhci_td_cleanup (ep->xhci, td, ring, td->status); - }else { -xhci_dbg (ep->xhci,"%s: Keep cancelled URB %p TD as cancel_status is %d\n" , - __func__, td->urb, td->cancel_status); - } -if (ep->xhci->xhc_state &XHCI_STATE_DYING ) -return ; - } -} -static int xhci_reset_halted_ep (struct xhci_hcd *xhci,unsigned int slot_id,unsigned int ep_index,enum xhci_ep_reset_type reset_type) {
@@ -1031,7 +1007,7 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)struct xhci_td *td =NULL ;struct xhci_td *tmp_td =NULL ;struct xhci_td *cached_td =NULL ; -struct xhci_ring *ring; +struct xhci_ring *ring, *cached_ring =NULL ;u64 hw_deq;unsigned int slot_id = ep->vdev->slot_id;int err;
@@ -1070,7 +1046,6 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)if (td->cancel_status ==TD_HALTED ||trb_in_td (td, hw_deq)) {switch (td->cancel_status) { -case TD_CLEARED :/* TD is already no-op */ case TD_CLEARING_CACHE :/* set TR deq command already queued */ break ;case TD_DIRTY :/* TD is cached, clear it */
@@ -1092,16 +1067,18 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) td->urb, cached_td->urb, td->urb->stream_id);td_to_noop (cached_td, false); - cached_td->cancel_status =TD_CLEARED ; +xhci_td_cleanup (xhci, cached_td, cached_ring, + cached_td->status); }td_to_noop (td, false); td->cancel_status =TD_CLEARING_CACHE ; + cached_ring = ring; cached_td = td;break ; } }else {td_to_noop (td, false); - td->cancel_status =TD_CLEARED ; +xhci_td_cleanup (xhci, td, ring, td->status); } }
@@ -1123,10 +1100,12 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)if (td->cancel_status !=TD_CLEARING_CACHE && td->cancel_status !=TD_CLEARING_CACHE_DEFERRED )continue ; -xhci_warn (xhci,"Failed to clear cancelled cached URB %p, mark clear anyway\n" , +xhci_warn (xhci,"Failed to clear cancelled cached URB %p, give back anyway\n" , td->urb);td_to_noop (td, false); - td->cancel_status =TD_CLEARED ; + ring =xhci_urb_to_transfer_ring (xhci, td->urb); +xhci_td_cleanup (xhci, td, ring, td->status); + } }return 0 ;
@@ -1142,7 +1121,6 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)void xhci_process_cancelled_tds (struct xhci_virt_ep *ep) {xhci_invalidate_cancelled_tds (ep); -xhci_giveback_invalidated_tds (ep); }/*
@@ -1295,7 +1273,6 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id, ep->ep_state &= ~EP_STOP_CMD_PENDING ;/* Otherwise ring the doorbell(s) to restart queued transfers */ -xhci_giveback_invalidated_tds (ep);ring_doorbell_for_active_rings (xhci, slot_id, ep_index); }
@@ -1517,13 +1494,9 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, cancelled_td_list) { ep_ring =xhci_urb_to_transfer_ring (ep->xhci, td->urb);if (td->cancel_status ==TD_CLEARING_CACHE ) { - td->cancel_status =TD_CLEARED ;xhci_dbg (ep->xhci,"%s: Giveback cancelled URB %p TD\n" , __func__, td->urb);xhci_td_cleanup (ep->xhci, td, ep_ring, td->status); - }else { -xhci_dbg (ep->xhci,"%s: Keep cancelled URB %p TD as cancel_status is %d\n" , - __func__, td->urb, td->cancel_status); } } cleanup:
@@ -1538,8 +1511,6 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,xhci_invalidate_cancelled_tds (ep);/* Try to restart the endpoint if all is done */ ring_doorbell_for_active_rings (xhci, slot_id, ep_index); -/* Start giving back any TDs invalidated above */ -xhci_giveback_invalidated_tds (ep); }else {/* Restart any rings with pending URBs */ xhci_dbg (ep->xhci,"%s: All TDs cleared, ring doorbell\n" , __func__);
@@ -1574,8 +1545,6 @@ static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, int slot_id,/* Clear our internal halted state */ ep->ep_state &= ~EP_HALTED ; -xhci_giveback_invalidated_tds (ep); -/* if this was a soft reset, then restart */ if ((le32_to_cpu (trb->generic.field[3 ])) &TRB_TSP )ring_doorbell_for_active_rings (xhci, slot_id, ep_index);
drivers/usb/host/xhci.h +0 -1
@@ -1297,7 +1297,6 @@ enum xhci_cancelled_td_status {TD_HALTED ,TD_CLEARING_CACHE , TD_CLEARING_CACHE_DEFERRED, - TD_CLEARED, }; struct xhci_td {
Signature · 1 line
2.48.1
Re: [PATCH RFC 1/2] usb: xhci: Drop the TD_CLEARED cancel status
Quoted 13 lines · Michal Pecio
There seems to be no need to keep invalidated TDs around and require callers to follow up with
xhci_giveback_invalidated_tds(), so makexhci_invalidate_cancelled_tds()give back invalidated TDs right away.
TD_CLEAREDcancel status is no longer useful and therefore removed. Code complexity and overhead of repeated list traversals are reduced.There is no more need to debug interactions between these functions, so a big source of
xhci_dbg()noise (and potential bugs) goes away.
From now, "Removing cancelled TD starting at ..." really means that
the TD is being removed, unless one of the errors below is logged or dynamic debug indicates that Set TR Dequeue is queued or deferred.
Quoted 6 lines
Also clean up some debug noise from
xhci_handle_cmd_set_deq(), which practically duplicatesxhci_invalidate_cancelled_tds()messages that will be printed just before this completion handler returns.Signed-off-by Michal Pecio <[email protected]>---
Looks good to me.
xhci_giveback_invalidated_tds() was anyways called right after xhci_invalidate_cancelled_tds() under the same lock protection so we can as well give back those non-cached cancelled TDs right away.
Thanks
Mathias
Re: [PATCH RFC 1/2] usb: xhci: Drop the TD_CLEARED cancel status
Quoted 22 lines · Mathias Nyman
Quoted 13 lines · Michal Pecio
There seems to be no need to keep invalidated TDs around and require callers to follow up with
xhci_giveback_invalidated_tds(), so makexhci_invalidate_cancelled_tds()give back invalidated TDs right away.
TD_CLEAREDcancel status is no longer useful and therefore removed. Code complexity and overhead of repeated list traversals are reduced.There is no more need to debug interactions between these functions, so a big source of
xhci_dbg()noise (and potential bugs) goes away.
From now, "Removing cancelled TD starting at ..." really means that
the TD is being removed, unless one of the errors below is logged or dynamic debug indicates that Set TR Dequeue is queued or deferred.Quoted 6 lines
Also clean up some debug noise from
xhci_handle_cmd_set_deq(), which practically duplicatesxhci_invalidate_cancelled_tds()messages that will be printed just before this completion handler returns.Signed-off-by Michal Pecio <[email protected]>---
Quoted 5 lines
Looks good to me.
xhci_giveback_invalidated_tds()was anyways called right afterxhci_invalidate_cancelled_tds()under the same lock protection so we can as well give back those non-cached cancelled TDs right away.
Thank you, that's what it looked like to me.
The only reason I could imagine for possibly wanting this state to exist is if somebody somehow demanded that cancelled TDs are given back in some particular order (submit order, cancellation order), but this driver has been delaying pending TD giveback due to Set Deq for years. Nobody seems to be complaining and I couldn't find any mention of giveback order in places like usb_unlink_urb().
I'm thikning about a v2 with small changes:
1. add a dynamic debug when a cached TD is selected for clearing - currently only the deffered cached TDs are logged instantly and we need to look for "Set TR Deq ptr ..." announcement later to know which TD was chosen.
2. add a patch to replace all 'break' with 'continue' in this function, because they break out of a switch nested in a loop, skip over an inactive 'else' clause and trigger a new iteration immediately.
Regards,
Michal
RFC PATCH 2/2 usb: xhci: Drop the TD_CLEARING_CACHE_DEFERRED cancel
In reply to Michal PecioThe last remaining use is inside xhci_invalidate_cancelled_tds() for destroying deferred TDs under certain failure conditions.
Instead of violating the spec by crudely erasing those deferred TDs, we may give them one last chance to get their Set TR Dequeue. Maybe the prior failure was caused by some corruption of one stream's ring and other streams are fine, maybe it was a transient -ENOMEM.
Or maybe we want to cover this obscure case with less extra code.
Remove the search for TD_CLEARING_CACHE_DEFERRED TDs, nuke the one cached TD we tried and failed to skip, and jump back to the main loop to handle any TDs still in TD_DIRTY or TD_HALTED state. These are our deferred TDs - all other dirty TDs have been given back.
If we fail again, the deferred TD will be nuked and so on, until we run out of TDs. We are under the lock so nobody can keep submitting and cancelling them forever.
--- drivers/usb/host/xhci-ring.c | 26 +++++++++----------------- drivers/usb/host/xhci.h | 1 - 2 files changed, 9 insertions(+), 18 deletions(-)
drivers/usb/host/xhci-ring.c +9 -17
@@ -1021,6 +1021,7 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) xhci = ep->xhci; +again:list_for_each_entry_safe (td, tmp_td, &ep->cancelled_td_list, cancelled_td_list) { xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb, "Removing canceled TD starting at 0x%llx (dma) in stream %u URB %p",
@@ -1050,14 +1051,12 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)break ;case TD_DIRTY :/* TD is cached, clear it */ case TD_HALTED : -case TD_CLEARING_CACHE_DEFERRED :if (cached_td) {if (cached_td->urb->stream_id != td->urb->stream_id) {/* Multiple streams case, defer move dq */ xhci_dbg (xhci,"Move dq deferred: stream %u URB %p\n" , td->urb->stream_id, td->urb); - td->cancel_status =TD_CLEARING_CACHE_DEFERRED ;break ; }
@@ -1091,22 +1090,15 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) cached_td);if (err) {/* Failed to move past cached td, just set cached TDs to no-op */ -list_for_each_entry_safe (td, tmp_td, &ep->cancelled_td_list, cancelled_td_list) { -/* -* Deferred TDs need to have the deq pointer set after the above command -* completes, so if that failed we just give up on all of them (and -* complain loudly since this could cause issues due to caching). -*/ -if (td->cancel_status !=TD_CLEARING_CACHE && - td->cancel_status !=TD_CLEARING_CACHE_DEFERRED ) -continue ; -xhci_warn (xhci,"Failed to clear cancelled cached URB %p, give back anyway\n" , - td->urb); -td_to_noop (td, false); - ring =xhci_urb_to_transfer_ring (xhci, td->urb); -xhci_td_cleanup (xhci, td, ring, td->status); +xhci_warn (xhci,"Failed to clear cancelled cached URB %p, give back anyway\n" , + cached_td->urb); +td_to_noop (cached_td, false); +xhci_td_cleanup (xhci, cached_td, cached_ring, cached_td->status); - } +/* Try to clear any deferred TDs on other streams */ + cached_ring =NULL ; + cached_td =NULL ; + goto again; }return 0 ; }
drivers/usb/host/xhci.h +0 -1
@@ -1296,7 +1296,6 @@ enum xhci_cancelled_td_status {TD_DIRTY =0 ,TD_HALTED , TD_CLEARING_CACHE, - TD_CLEARING_CACHE_DEFERRED, }; struct xhci_td {
Signature · 1 line
2.48.1
Re: [PATCH RFC 2/2] usb: xhci: Drop the TD_CLEARING_CACHE_DEFERRED cancel
Quoted 15 lines · Michal Pecio
The last remaining use is inside
xhci_invalidate_cancelled_tds()for destroying deferred TDs under certain failure conditions.Instead of violating the spec by crudely erasing those deferred TDs, we may give them one last chance to get their Set TR Dequeue. Maybe the prior failure was caused by some corruption of one stream's ring and other streams are fine, maybe it was a transient
-ENOMEM.Or maybe we want to cover this obscure case with less extra code.
Remove the search for
TD_CLEARING_CACHE_DEFERREDTDs, nuke the one cached TD we tried and failed to skip, and jump back to the main loop to handle any TDs still inTD_DIRTYorTD_HALTEDstate. These are our deferred TDs - all other dirty TDs have been given back.
Quoted 5 lines
If we fail again, the deferred TD will be nuked and so on, until we run out of TDs. We are under the lock so nobody can keep submitting and cancelling them forever.
Signed-off-by Michal Pecio <[email protected]>
Looks good to me.
Gives every stream ring a chance to move past the cancelled TD, unlike the current code that bails out all stream rings on first set TR Deq failure.
Thanks
Mathias