On Tue, Nov 16, 2010 at 05:10:27PM -0800, Paul Zimmerman wrote: > Hi Sarah, > > As I have mentioned before, we have been having problems with the USB-IF Gold > Tree tests when plugging and unplugging devices from the tree. I have seen > that the reset-device and configure-endpoint commands, which are invoked from > xhci_discover_or_reset_device() and xhci_configure_endpoint(), will sometimes > time out. > > After much debugging, I determined that the commands themselves do not actually > time out, but rather their completion events do not get delivered to the right > place. > > This happens when the command ring has just wrapped around, and it's enqueue > pointer is left pointing to the link TRB. xhci_discover_or_reset_device() and > xhci_configure_endpoint() use the enqueue pointer directly as their command > TRB pointer, without checking whether it's pointing to the link TRB. > > When the completion event arrives, if the command TRB is pointing to the link > TRB, the check against the command ring dequeue pointer in > handle_cmd_in_cmd_wait_list() fails, so the completion inside the command does > not get signaled. > > The patch below fixes the timeout problem for me. I'm not sure if this is the > correct way to fix it, though. What do you think? Good catch. I think this is the correct fix, but I need to think about the underlying issue. The link TRB activation change was put in because of the issue with bulk stream rings. It doesn't seem like we should delay giving the link TRB to the host for the command ring though. But I'll take this fix for now and pass it along to Greg. Can you please resend with a proper Signed-off-by line? Checkpatch also complains about some over-80 character lines: WARNING: line over 80 characters #43: FILE: drivers/usb/host/xhci.c:1553: + /* Enqueue pointer can be left pointing to link TRB, we must handle that */ WARNING: line over 80 characters #46: FILE: drivers/usb/host/xhci.c:1556: + command->command_trb = xhci->cmd_ring->enq_seg->next->trbs; WARNING: line over 80 characters #57: FILE: drivers/usb/host/xhci.c:2283: + /* Enqueue pointer can be left pointing to link TRB, we must handle that */ WARNING: line over 80 characters #60: FILE: drivers/usb/host/xhci.c:2286: + reset_device_cmd->command_trb = xhci->cmd_ring->enq_seg->next->trbs; ERROR: Missing Signed-off-by: line(s) total: 1 errors, 4 warnings, 26 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Please fix, commit, and run `git show HEAD | scripts/checkpatch.pl -`. Or if you want git to run checkpatch automatically before you commit, you can put this file in your .git/hooks directory: http://minilop.net/~sarah/pre-commit Depending your git version, you may need to make the file executable. Thanks for catching this! Sarah Sharp > --- > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 06fca08..c09d2fc 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -1549,6 +1549,13 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci, > cmd_completion = command->completion; > cmd_status = &command->status; > command->command_trb = xhci->cmd_ring->enqueue; > + > + /* Enqueue pointer can be left pointing to link TRB, we must handle that */ > + if ((command->command_trb->link.control & TRB_TYPE_BITMASK) > + == TRB_TYPE(TRB_LINK)) { > + command->command_trb = xhci->cmd_ring->enq_seg->next->trbs; > + } > + > list_add_tail(&command->cmd_list, &virt_dev->cmd_list); > } else { > in_ctx = virt_dev->in_ctx; > @@ -2272,6 +2279,13 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd, struct usb_device *udev) > /* Attempt to submit the Reset Device command to the command ring */ > spin_lock_irqsave(&xhci->lock, flags); > reset_device_cmd->command_trb = xhci->cmd_ring->enqueue; > + > + /* Enqueue pointer can be left pointing to link TRB, we must handle that */ > + if ((reset_device_cmd->command_trb->link.control & TRB_TYPE_BITMASK) > + == TRB_TYPE(TRB_LINK)) { > + reset_device_cmd->command_trb = xhci->cmd_ring->enq_seg->next->trbs; > + } > + > list_add_tail(&reset_device_cmd->cmd_list, &virt_dev->cmd_list); > ret = xhci_queue_reset_device(xhci, slot_id); > if (ret) { > -- 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
