On Tue, Oct 18, 2011 at 05:11:37PM -0400, Don Zickus wrote:
> Hi Sarah,
>
> While trying to debug why a usb3 harddrive doesn't show when a usb3 hub is
> plugged into another port, I stumbled upon this deadlock warning from
> lockdep.
<snip>
> This was found using gregkh's usb-next (fa3ae0c1).
>
> Basically what is happening is in xhci_stop_endpoint_command_watchdog()
> the xhci->lock is grabbed with just spin_lock. What lockdep deduces is
> that if an interrupt occurred while in this function it would deadlock
> with xhci_irq because that function also grabs the xhci->lock.
The watchdog timer only runs when the hardware is seriously hosed (i.e.
has stopped giving back events for the stop endpoint command). It's
possible, I suppose, for the host controller to interrupt with a
different event, so your patch is probably necessary.
> Fixing it is trivial by using spin_lock_irqsave instead. However, I am
> not sure if that is really the right thing to do. The patch below fixes
> the lockdep warning, but I am open to other suggestions (and willing to test
> them).
I think your patch is fine, and I'll get it off to Greg.
Sarah
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index e4b7f00..3339ec3 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -816,23 +816,24 @@ void xhci_stop_endpoint_command_watchdog(unsigned long arg)
> struct xhci_ring *ring;
> struct xhci_td *cur_td;
> int ret, i, j;
> + unsigned long flags;
>
> ep = (struct xhci_virt_ep *) arg;
> xhci = ep->xhci;
>
> - spin_lock(&xhci->lock);
> + spin_lock_irqsave(&xhci->lock, flags);
>
> ep->stop_cmds_pending--;
> if (xhci->xhc_state & XHCI_STATE_DYING) {
> xhci_dbg(xhci, "Stop EP timer ran, but another timer marked "
> "xHCI as DYING, exiting.\n");
> - spin_unlock(&xhci->lock);
> + spin_unlock_irqrestore(&xhci->lock, flags);
> return;
> }
> if (!(ep->stop_cmds_pending == 0 && (ep->ep_state & EP_HALT_PENDING))) {
> xhci_dbg(xhci, "Stop EP timer ran, but no command pending, "
> "exiting.\n");
> - spin_unlock(&xhci->lock);
> + spin_unlock_irqrestore(&xhci->lock, flags);
> return;
> }
>
> @@ -844,11 +845,11 @@ void xhci_stop_endpoint_command_watchdog(unsigned long arg)
> xhci->xhc_state |= XHCI_STATE_DYING;
> /* Disable interrupts from the host controller and start halting it */
> xhci_quiesce(xhci);
> - spin_unlock(&xhci->lock);
> + spin_unlock_irqrestore(&xhci->lock, flags);
>
> ret = xhci_halt(xhci);
>
> - spin_lock(&xhci->lock);
> + spin_lock_irqsave(&xhci->lock, flags);
> if (ret < 0) {
> /* This is bad; the host is not responding to commands and it's
> * not allowing itself to be halted. At least interrupts are
> @@ -896,7 +897,7 @@ void xhci_stop_endpoint_command_watchdog(unsigned long arg)
> }
> }
> }
> - spin_unlock(&xhci->lock);
> + spin_unlock_irqrestore(&xhci->lock, flags);
> xhci_dbg(xhci, "Calling usb_hc_died()\n");
> usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
> xhci_dbg(xhci, "xHCI host controller is dead.\n");
--
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
