This is a note to let you know that I've just added the patch titled
xhci: Fix failure to give back some cached cancelled URBs.
to my usb git tree which can be found at git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git in the usb-testing branch.
The patch will show up in the next release of the linux-next tree (usually sometime within the next 24 hours during the week.)
The patch will be merged to the usb-next branch sometime soon, after it passes testing, and the merge window is open.
If you have any questions about this process, please let me know.
From 94f339147fc3eb9edef7ee4ef6e39c569c073753 Mon Sep 17 00:00:00 2001
From: Mathias Nyman mathias.nyman@linux.intel.com Date: Fri, 20 Aug 2021 15:35:00 +0300 Subject: xhci: Fix failure to give back some cached cancelled URBs.
Only TDs with status TD_CLEARING_CACHE will be given back after cache is cleared with a set TR deq command.
xhci_invalidate_cached_td() failed to set the TD_CLEARING_CACHE status for some cancelled TDs as it assumed an endpoint only needs to clear the TD it stopped on.
This isn't always true. For example with streams enabled an endpoint may have several stream rings, each stopping on a different TDs.
Note that if an endpoint has several stream rings, the current code will still only clear the cache of the stream pointed to by the last cancelled TD in the cancel list.
This patch only focus on making sure all canceled TDs are given back, avoiding hung task after device removal. Another fix to solve clearing the caches of all stream rings with cancelled TDs is needed, but not as urgent.
This issue was simultanously discovered and debugged by by Tao Wang, with a slightly different fix proposal.
Fixes: 674f8438c121 ("xhci: split handling halted endpoints into two steps") Cc: stable@vger.kernel.org #5.12 Reported-by: Tao Wang wat@codeaurora.org Signed-off-by: Mathias Nyman mathias.nyman@linux.intel.com Link: https://lore.kernel.org/r/20210820123503.2605901-4-mathias.nyman@linux.intel... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/usb/host/xhci-ring.c | 40 ++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index d0faa67a689d..9017986241f5 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -942,17 +942,21 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) td->urb->stream_id); hw_deq &= ~0xf;
- if (td->cancel_status == TD_HALTED) { - cached_td = td; - } else if (trb_in_td(xhci, td->start_seg, td->first_trb, - td->last_trb, hw_deq, false)) { + if (td->cancel_status == TD_HALTED || + trb_in_td(xhci, td->start_seg, td->first_trb, td->last_trb, hw_deq, false)) { 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 */ case TD_HALTED: - /* FIXME stream case, several stopped rings */ + td->cancel_status = TD_CLEARING_CACHE; + if (cached_td) + /* FIXME stream case, several stopped rings */ + xhci_dbg(xhci, + "Move dq past stream %u URB %p instead of stream %u URB %p\n", + td->urb->stream_id, td->urb, + cached_td->urb->stream_id, cached_td->urb); cached_td = td; break; } @@ -961,18 +965,24 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) td->cancel_status = TD_CLEARED; } } - if (cached_td) { - cached_td->cancel_status = TD_CLEARING_CACHE;
- err = xhci_move_dequeue_past_td(xhci, slot_id, ep->ep_index, - cached_td->urb->stream_id, - cached_td); - /* Failed to move past cached td, try just setting it noop */ - if (err) { - td_to_noop(xhci, ring, cached_td, false); - cached_td->cancel_status = TD_CLEARED; + /* If there's no need to move the dequeue pointer then we're done */ + if (!cached_td) + return 0; + + err = xhci_move_dequeue_past_td(xhci, slot_id, ep->ep_index, + cached_td->urb->stream_id, + 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) { + if (td->cancel_status != TD_CLEARING_CACHE) + continue; + xhci_dbg(xhci, "Failed to clear cancelled cached URB %p, mark clear anyway\n", + td->urb); + td_to_noop(xhci, ring, td, false); + td->cancel_status = TD_CLEARED; } - cached_td = NULL; } return 0; }
On 2021-08-26 19:06, gregkh@linuxfoundation.org wrote:
This is a note to let you know that I've just added the patch titled
xhci: Fix failure to give back some cached cancelled URBs.to my usb git tree which can be found at git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git in the usb-testing branch.
The patch will show up in the next release of the linux-next tree (usually sometime within the next 24 hours during the week.)
The patch will be merged to the usb-next branch sometime soon, after it passes testing, and the merge window is open.
If you have any questions about this process, please let me know.
Thanks, we have tested the patch, it's ok.
From 94f339147fc3eb9edef7ee4ef6e39c569c073753 Mon Sep 17 00:00:00 2001 From: Mathias Nyman mathias.nyman@linux.intel.com Date: Fri, 20 Aug 2021 15:35:00 +0300 Subject: xhci: Fix failure to give back some cached cancelled URBs.
Only TDs with status TD_CLEARING_CACHE will be given back after cache is cleared with a set TR deq command.
xhci_invalidate_cached_td() failed to set the TD_CLEARING_CACHE status for some cancelled TDs as it assumed an endpoint only needs to clear the TD it stopped on.
This isn't always true. For example with streams enabled an endpoint may have several stream rings, each stopping on a different TDs.
Note that if an endpoint has several stream rings, the current code will still only clear the cache of the stream pointed to by the last cancelled TD in the cancel list.
This patch only focus on making sure all canceled TDs are given back, avoiding hung task after device removal. Another fix to solve clearing the caches of all stream rings with cancelled TDs is needed, but not as urgent.
This issue was simultanously discovered and debugged by by Tao Wang, with a slightly different fix proposal.
Fixes: 674f8438c121 ("xhci: split handling halted endpoints into two steps") Cc: stable@vger.kernel.org #5.12 Reported-by: Tao Wang wat@codeaurora.org Signed-off-by: Mathias Nyman mathias.nyman@linux.intel.com Link: https://lore.kernel.org/r/20210820123503.2605901-4-mathias.nyman@linux.intel... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
drivers/usb/host/xhci-ring.c | 40 ++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index d0faa67a689d..9017986241f5 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -942,17 +942,21 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) td->urb->stream_id); hw_deq &= ~0xf;
if (td->cancel_status == TD_HALTED) {cached_td = td;} else if (trb_in_td(xhci, td->start_seg, td->first_trb,td->last_trb, hw_deq, false)) {
if (td->cancel_status == TD_HALTED ||trb_in_td(xhci, td->start_seg, td->first_trb, td->last_trb,hw_deq, false)) { 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 */ case TD_HALTED:
/* FIXME stream case, several stopped rings */
td->cancel_status = TD_CLEARING_CACHE;if (cached_td)/* FIXME stream case, several stopped rings */xhci_dbg(xhci,"Move dq past stream %u URB %p instead of stream %u URB %p\n",td->urb->stream_id, td->urb,cached_td->urb->stream_id, cached_td->urb); cached_td = td; break; }@@ -961,18 +965,24 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) td->cancel_status = TD_CLEARED; } }
if (cached_td) {
cached_td->cancel_status = TD_CLEARING_CACHE;err = xhci_move_dequeue_past_td(xhci, slot_id, ep->ep_index,cached_td->urb->stream_id,cached_td);/* Failed to move past cached td, try just setting it noop */if (err) {td_to_noop(xhci, ring, cached_td, false);cached_td->cancel_status = TD_CLEARED;
- /* If there's no need to move the dequeue pointer then we're done */
- if (!cached_td)
return 0;- err = xhci_move_dequeue_past_td(xhci, slot_id, ep->ep_index,
cached_td->urb->stream_id,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) {
if (td->cancel_status != TD_CLEARING_CACHE)continue;xhci_dbg(xhci, "Failed to clear cancelled cached URB %p, markclear anyway\n",
td->urb);td_to_noop(xhci, ring, td, false);td->cancel_status = TD_CLEARED;
cached_td = NULL;}
linux-stable-mirror@lists.linaro.org
-
gregkh＠linuxfoundation.org -
wat＠codeaurora.org