Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751443AbcLEDAS (ORCPT ); Sun, 4 Dec 2016 22:00:18 -0500 Received: from mail-yb0-f173.google.com ([209.85.213.173]:36641 "EHLO mail-yb0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751168AbcLEDAR (ORCPT ); Sun, 4 Dec 2016 22:00:17 -0500 MIME-Version: 1.0 In-Reply-To: <3981341555a44bcbb1a339b8f980fd95a67b4b69.1480677144.git.baolin.wang@linaro.org> References: <3981341555a44bcbb1a339b8f980fd95a67b4b69.1480677144.git.baolin.wang@linaro.org> From: Baolin Wang Date: Mon, 5 Dec 2016 10:59:39 +0800 Message-ID: Subject: Re: [PATCH] usb: host: xhci: Handle the right timeout command To: mathias.nyman@intel.com, Greg KH Cc: USB , LKML , Mark Brown , Baolin Wang Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5332 Lines: 121 On 2 December 2016 at 19:21, Baolin Wang wrote: > If a command event is found on the event ring during an interrupt, > we need to stop the command timer with del_timer(). Since del_timer() > can fail if the timer is running and waiting on the xHCI lock, then > it maybe get the wrong timeout command in xhci_handle_command_timeout() > if host fetched a new command and updated the xhci->current_cmd in > handle_cmd_completion(). For this situation, we need a way to signal > to the command timer that everything is fine and it should exit. > > We should introduce a counter (xhci->current_cmd_pending) for the number > of pending commands. If we need to cancel the command timer and del_timer() > succeeds, we decrement the number of pending commands. If del_timer() fails, > we leave the number of pending commands alone. > > For handling timeout command, in xhci_handle_command_timeout() we will check > the counter after decrementing it, if the counter (xhci->current_cmd_pending) > is 0, which means xhci->current_cmd is the right timeout command. If the > counter (xhci->current_cmd_pending) is greater than 0, which means current > timeout command has been handled by host and host has fetched new command > as xhci->current_cmd, then just return and wait for new current command. > > Signed-off-by: Baolin Wang > --- > This patch is based on Lu Baolu's new fix patch: > usb: xhci: fix possible wild pointer > --- > drivers/usb/host/xhci-ring.c | 26 +++++++++++++++++++++++++- > drivers/usb/host/xhci.h | 1 + > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 62dd1c7..a62904e 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -1253,6 +1253,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci, > if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) && > !(xhci->xhc_state & XHCI_STATE_DYING)) { > xhci->current_cmd = cur_cmd; > + xhci->current_cmd_pending++; > mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT); > xhci_ring_cmd_db(xhci); > } > @@ -1274,6 +1275,21 @@ void xhci_handle_command_timeout(unsigned long data) > return; > } > > + xhci->current_cmd_pending--; I realized I should move the decrement before 'xhci->current_cmd' checking, and I will wait for a while for new comments before sending out v2 patch. > + /* > + * If the current_cmd_pending is 0, which means current command is > + * timeout. > + * > + * If the current_cmd_pending is greater than 0, which means current > + * timeout command has been handled by host and host has fetched new > + * command as xhci->current_cmd, then just return and wait for new > + * current command. > + */ > + if (xhci->current_cmd_pending > 0) { > + spin_unlock_irqrestore(&xhci->lock, flags); > + return; > + } > + > if (xhci->current_cmd->status == COMP_CMD_ABORT) > second_timeout = true; > xhci->current_cmd->status = COMP_CMD_ABORT; > @@ -1336,7 +1352,13 @@ static void handle_cmd_completion(struct xhci_hcd *xhci, > > cmd = list_entry(xhci->cmd_list.next, struct xhci_command, cmd_list); > > - del_timer(&xhci->cmd_timer); > + /* > + * If the command timer is running on another CPU, we don't decrement > + * current_cmd_pending, since we didn't successfully stop the command > + * timer. > + */ > + if (del_timer(&xhci->cmd_timer)) > + xhci->current_cmd_pending--; > > trace_xhci_cmd_completion(cmd_trb, (struct xhci_generic_trb *) event); > > @@ -1424,6 +1446,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci, > if (cmd->cmd_list.next != &xhci->cmd_list) { > xhci->current_cmd = list_entry(cmd->cmd_list.next, > struct xhci_command, cmd_list); > + xhci->current_cmd_pending++; > mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT); > } else if (xhci->current_cmd == cmd) { > xhci->current_cmd = NULL; > @@ -3924,6 +3947,7 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd, > if (xhci->cmd_list.next == &cmd->cmd_list && > !timer_pending(&xhci->cmd_timer)) { > xhci->current_cmd = cmd; > + xhci->current_cmd_pending++; > mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT); > } > > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > index 9dbaacf..5d81257 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -1567,6 +1567,7 @@ struct xhci_hcd { > unsigned int cmd_ring_reserved_trbs; > struct timer_list cmd_timer; > struct xhci_command *current_cmd; > + u32 current_cmd_pending; > struct xhci_ring *event_ring; > struct xhci_erst erst; > /* Scratchpad */ > -- > 1.7.9.5 > -- Baolin.wang Best Regards