Current abort operation has race. xhci_handle_command_timeout() xhci_abort_cmd_ring() xhci_write_64(CMD_RING_ABORT) xhci_handshake(5s) do { check CMD_RING_RUNNING udelay(1) ... COMP_CMD_ABORT event COMP_CMD_STOP event xhci_handle_stopped_cmd_ring() restart cmd_ring CMD_RING_RUNNING become 1 again } while () return -ETIMEDOUT xhci_write_64(CMD_RING_ABORT) /* can abort random command */ To do abort operation correctly, we have to wait both of COMP_CMD_STOP event and negation of CMD_RING_RUNNING. But like above, while timeout handler is waiting negation of CMD_RING_RUNNING, event handler can restart cmd_ring. So timeout handler never be notice negation of CMD_RING_RUNNING, and retry of CMD_RING_ABORT can abort random command (BTW, I guess retry of CMD_RING_ABORT was workaround of this race). To fix this race, this moves xhci_handle_stopped_cmd_ring() to xhci_abort_cmd_ring(). And timeout handler waits COMP_CMD_STOP event. At this point, timeout handler is owner of cmd_ring, and safely restart cmd_ring by using xhci_handle_stopped_cmd_ring(). [FWIW, as bonus, this way would be easily extend to add CMD_RING_PAUSE operation] Signed-off-by: OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> --- drivers/usb/host/xhci-mem.c | 1 drivers/usb/host/xhci-ring.c | 163 ++++++++++++++++++++++-------------------- drivers/usb/host/xhci.h | 1 3 files changed, 88 insertions(+), 77 deletions(-) diff -puN drivers/usb/host/xhci-mem.c~xhci-fix-abort-race2 drivers/usb/host/xhci-mem.c --- xhci/drivers/usb/host/xhci-mem.c~xhci-fix-abort-race2 2016-11-16 13:36:07.219329211 +0900 +++ xhci-hirofumi/drivers/usb/host/xhci-mem.c 2016-11-16 13:36:07.221329211 +0900 @@ -2344,6 +2344,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, /* init command timeout work */ INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout); + init_completion(&xhci->stop_completion); page_size = readl(&xhci->op_regs->page_size); xhci_dbg_trace(xhci, trace_xhci_dbg_init, diff -puN drivers/usb/host/xhci-ring.c~xhci-fix-abort-race2 drivers/usb/host/xhci-ring.c --- xhci/drivers/usb/host/xhci-ring.c~xhci-fix-abort-race2 2016-11-16 13:36:07.219329211 +0900 +++ xhci-hirofumi/drivers/usb/host/xhci-ring.c 2016-11-16 13:36:07.221329211 +0900 @@ -284,6 +284,61 @@ static bool xhci_mod_cmd_timer(struct xh return mod_delayed_work(system_wq, &xhci->cmd_timer, delay); } +static struct xhci_command *xhci_next_queued_cmd(struct xhci_hcd *xhci) +{ + if (list_empty(&xhci->cmd_list)) + return NULL; + return list_first_entry(&xhci->cmd_list, struct xhci_command, cmd_list); +} + +/* + * Turn all commands on command ring with status set to "aborted" to no-op trbs. + * If there are other commands waiting then restart the ring and kick the timer. + * This must be called with command ring stopped and xhci->lock held. + */ +static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci, + struct xhci_command *cur_cmd) +{ + struct xhci_command *i_cmd; + u32 cycle_state; + + /* Turn all aborted commands in list to no-ops, then restart */ + list_for_each_entry(i_cmd, &xhci->cmd_list, cmd_list) { + + if (i_cmd->status != COMP_CMD_ABORT) + continue; + + i_cmd->status = COMP_CMD_STOP; + + xhci_dbg(xhci, "Turn aborted command %p to no-op\n", + i_cmd->command_trb); + /* get cycle state from the original cmd trb */ + cycle_state = le32_to_cpu( + i_cmd->command_trb->generic.field[3]) & TRB_CYCLE; + /* modify the command trb to no-op command */ + i_cmd->command_trb->generic.field[0] = 0; + i_cmd->command_trb->generic.field[1] = 0; + i_cmd->command_trb->generic.field[2] = 0; + i_cmd->command_trb->generic.field[3] = cpu_to_le32( + TRB_TYPE(TRB_CMD_NOOP) | cycle_state); + + /* + * caller waiting for completion is called when command + * completion event is received for these no-op commands + */ + } + + xhci->cmd_ring_state = CMD_RING_STATE_RUNNING; + + /* ring command ring doorbell to restart the command ring */ + if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) && + !(xhci->xhc_state & XHCI_STATE_DYING)) { + xhci->current_cmd = cur_cmd; + xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT); + xhci_ring_cmd_db(xhci); + } +} + static int xhci_abort_cmd_ring(struct xhci_hcd *xhci) { u64 temp_64; @@ -291,16 +346,9 @@ static int xhci_abort_cmd_ring(struct xh xhci_dbg(xhci, "Abort command ring\n"); - temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring); - xhci->cmd_ring_state = CMD_RING_STATE_ABORTED; + reinit_completion(&xhci->stop_completion); - /* - * Writing the CMD_RING_ABORT bit should cause a cmd completion event, - * however on some host hw the CMD_RING_RUNNING bit is correctly cleared - * but the completion event in never sent. Use the cmd timeout timer to - * handle those cases. Use twice the time to cover the bit polling retry - */ - xhci_mod_cmd_timer(xhci, 2 * XHCI_CMD_DEFAULT_TIMEOUT); + temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring); xhci_write_64(xhci, temp_64 | CMD_RING_ABORT, &xhci->op_regs->cmd_ring); @@ -320,15 +368,29 @@ static int xhci_abort_cmd_ring(struct xh udelay(1000); ret = xhci_handshake(&xhci->op_regs->cmd_ring, CMD_RING_RUNNING, 0, 3 * 1000 * 1000); - if (ret == 0) - return 0; + if (ret < 0) { + xhci_err(xhci, "Stopped the command ring failed, " + "maybe the host is dead\n"); + xhci->xhc_state |= XHCI_STATE_DYING; + xhci_halt(xhci); + return -ESHUTDOWN; + } + } - xhci_err(xhci, "Stopped the command ring failed, " - "maybe the host is dead\n"); - cancel_delayed_work(&xhci->cmd_timer); - xhci->xhc_state |= XHCI_STATE_DYING; - xhci_halt(xhci); - return -ESHUTDOWN; + /* + * Writing the CMD_RING_ABORT bit should cause a cmd completion event, + * however on some host hw the CMD_RING_RUNNING bit is correctly cleared + * but the completion event in never sent. Wait 2 secs (arbitrary + * number) to handle those cases after negation of CMD_RING_RUNNING. + */ + if (!wait_for_completion_timeout(&xhci->stop_completion, 2 * HZ)) { + xhci_dbg(xhci, "No stop event for abort, ring start fail?\n"); + xhci_cleanup_command_queue(xhci); + } else { + unsigned long flags; + spin_lock_irqsave(&xhci->lock, flags); + xhci_handle_stopped_cmd_ring(xhci, xhci_next_queued_cmd(xhci)); + spin_unlock_irqrestore(&xhci->lock, flags); } return 0; @@ -1212,79 +1274,26 @@ void xhci_cleanup_command_queue(struct x xhci_complete_del_and_free_cmd(cur_cmd, COMP_CMD_ABORT); } -/* - * Turn all commands on command ring with status set to "aborted" to no-op trbs. - * If there are other commands waiting then restart the ring and kick the timer. - * This must be called with command ring stopped and xhci->lock held. - */ -static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci, - struct xhci_command *cur_cmd) -{ - struct xhci_command *i_cmd, *tmp_cmd; - u32 cycle_state; - - /* Turn all aborted commands in list to no-ops, then restart */ - list_for_each_entry_safe(i_cmd, tmp_cmd, &xhci->cmd_list, - cmd_list) { - - if (i_cmd->status != COMP_CMD_ABORT) - continue; - - i_cmd->status = COMP_CMD_STOP; - - xhci_dbg(xhci, "Turn aborted command %p to no-op\n", - i_cmd->command_trb); - /* get cycle state from the original cmd trb */ - cycle_state = le32_to_cpu( - i_cmd->command_trb->generic.field[3]) & TRB_CYCLE; - /* modify the command trb to no-op command */ - i_cmd->command_trb->generic.field[0] = 0; - i_cmd->command_trb->generic.field[1] = 0; - i_cmd->command_trb->generic.field[2] = 0; - i_cmd->command_trb->generic.field[3] = cpu_to_le32( - TRB_TYPE(TRB_CMD_NOOP) | cycle_state); - - /* - * caller waiting for completion is called when command - * completion event is received for these no-op commands - */ - } - - xhci->cmd_ring_state = CMD_RING_STATE_RUNNING; - - /* ring command ring doorbell to restart the command ring */ - if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) && - !(xhci->xhc_state & XHCI_STATE_DYING)) { - xhci->current_cmd = cur_cmd; - xhci_mod_cmd_timer(xhci, XHCI_CMD_DEFAULT_TIMEOUT); - xhci_ring_cmd_db(xhci); - } - return; -} - - void xhci_handle_command_timeout(struct work_struct *work) { struct xhci_hcd *xhci; int ret; unsigned long flags; u64 hw_ring_state; - bool second_timeout = false; xhci = container_of(to_delayed_work(work), struct xhci_hcd, cmd_timer); /* mark this command to be cancelled */ spin_lock_irqsave(&xhci->lock, flags); - if (xhci->current_cmd) { - if (xhci->current_cmd->status == COMP_CMD_ABORT) - second_timeout = true; + if (xhci->current_cmd) xhci->current_cmd->status = COMP_CMD_ABORT; - } /* Make sure command ring is running before aborting it */ hw_ring_state = xhci_read_64(xhci, &xhci->op_regs->cmd_ring); if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) && (hw_ring_state & CMD_RING_RUNNING)) { + /* Prevent new doorbell, and start command abort */ + xhci->cmd_ring_state = CMD_RING_STATE_ABORTED; spin_unlock_irqrestore(&xhci->lock, flags); xhci_dbg(xhci, "Command timeout\n"); ret = xhci_abort_cmd_ring(xhci); @@ -1297,10 +1306,10 @@ void xhci_handle_command_timeout(struct return; } - /* command ring failed to restart, or host removed. Bail out */ - if (second_timeout || xhci->xhc_state & XHCI_STATE_REMOVING) { + /* host removed. Bail out */ + if (xhci->xhc_state & XHCI_STATE_REMOVING) { spin_unlock_irqrestore(&xhci->lock, flags); - xhci_dbg(xhci, "command timed out twice, ring start fail?\n"); + xhci_dbg(xhci, "host removed, ring start fail?\n"); xhci_cleanup_command_queue(xhci); return; } @@ -1347,7 +1356,7 @@ static void handle_cmd_completion(struct /* If CMD ring stopped we own the trbs between enqueue and dequeue */ if (cmd_comp_code == COMP_CMD_STOP) { - xhci_handle_stopped_cmd_ring(xhci, cmd); + complete_all(&xhci->stop_completion); return; } diff -puN drivers/usb/host/xhci.h~xhci-fix-abort-race2 drivers/usb/host/xhci.h --- xhci/drivers/usb/host/xhci.h~xhci-fix-abort-race2 2016-11-16 13:36:07.220329211 +0900 +++ xhci-hirofumi/drivers/usb/host/xhci.h 2016-11-16 13:36:07.222329210 +0900 @@ -1574,6 +1574,7 @@ struct xhci_hcd { struct list_head cmd_list; unsigned int cmd_ring_reserved_trbs; struct delayed_work cmd_timer; + struct completion stop_completion; struct xhci_command *current_cmd; struct xhci_ring *event_ring; struct xhci_erst erst; _ -- OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html