diff mbox

usb: host: xhci: Handle the right timeout command

Message ID 3981341555a44bcbb1a339b8f980fd95a67b4b69.1480677144.git.baolin.wang@linaro.org
State New
Headers show

Commit Message

(Exiting) Baolin Wang Dec. 2, 2016, 11:21 a.m. UTC 
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 <baolin.wang@linaro.org>

---
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(-)

-- 
1.7.9.5

Comments

(Exiting) Baolin Wang Dec. 5, 2016, 2:59 a.m. UTC | #1 
On 2 December 2016 at 19:21, Baolin Wang <baolin.wang@linaro.org> 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 <baolin.wang@linaro.org>

> ---

> 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
diff mbox

Patch

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--;
+	/*
+	 * 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 */