On Mon, Dec 06, 2010 at 06:22:19PM +0800, Andiry Xu wrote:
> Matthew,
>
> Can you help to test if this patch fix your xhci suspend and remove issue? Thanks.
Andiry,
This code doesn't change the code in the USB core that synchronizes the
legacy PCI interrupts. So if you have to fall back to MSI, then
pci_dev->irq is equal to your MSI irq, and suspend_common() will
synchronize the irq a second time after xhci_suspend() is called. Maybe
suspend_common() should only call synchronize_irq() if pci_dev->irq is
the same as hcd->irq?
Also, what happens if you free the legacy IRQ, set up MSI-X, and then
the USB core tries to synchronize pci_dev->irq? Is pci_dev->irq still
set to the legacy IRQ that the driver freed? If so, we're possibly
synchronizing someone else's IRQ.
Sarah Sharp
> From: Andiry Xu <andiry.xu@xxxxxxx>
> Date: Mon, 6 Dec 2010 16:58:16 +0800
> Subject: [PATCH] xHCI: synchronize interrupts in xhci_suspend()
>
> Matthew Garrett reports a interrupt double free in xHCI code when the
> host is suspended and then the card removed.
>
> Synchronize the interrupts instead of free them in xhci_suspned().
>
> Reported-by: Matthew Garrett <mjg@xxxxxxxxxx>
> Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
> ---
> drivers/usb/host/xhci.c | 37 +++++++++----------------------------
> 1 files changed, 9 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 45e4a31..41a9225 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -646,7 +646,9 @@ int xhci_suspend(struct xhci_hcd *xhci)
> {
> int rc = 0;
> struct usb_hcd *hcd = xhci_to_hcd(xhci);
> + struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
> u32 command;
> + int i;
>
> spin_lock_irq(&xhci->lock);
> clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> @@ -678,9 +680,14 @@ int xhci_suspend(struct xhci_hcd *xhci)
> return -ETIMEDOUT;
> }
> /* step 5: remove core well power */
> - xhci_cleanup_msix(xhci);
> spin_unlock_irq(&xhci->lock);
>
> + if (xhci->msix_entries) {
> + for (i = 0; i < xhci->msix_count; i++)
> + synchronize_irq(xhci->msix_entries[i].vector);
> + } else if (pdev->irq >= 0)
> + synchronize_irq(pdev->irq);
> +
> return rc;
> }
>
> @@ -694,7 +701,6 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> {
> u32 command, temp = 0;
> struct usb_hcd *hcd = xhci_to_hcd(xhci);
> - struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
> int old_state, retval;
>
> old_state = hcd->state;
> @@ -729,8 +735,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> xhci_dbg(xhci, "Stop HCD\n");
> xhci_halt(xhci);
> xhci_reset(xhci);
> - if (hibernated)
> - xhci_cleanup_msix(xhci);
> + xhci_cleanup_msix(xhci);
> spin_unlock_irq(&xhci->lock);
>
> #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
> @@ -765,30 +770,6 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> return retval;
> }
>
> - spin_unlock_irq(&xhci->lock);
> - /* Re-setup MSI-X */
> - if (hcd->irq)
> - free_irq(hcd->irq, hcd);
> - hcd->irq = -1;
> -
> - retval = xhci_setup_msix(xhci);
> - if (retval)
> - /* fall back to msi*/
> - retval = xhci_setup_msi(xhci);
> -
> - if (retval) {
> - /* fall back to legacy interrupt*/
> - retval = request_irq(pdev->irq, &usb_hcd_irq, IRQF_SHARED,
> - hcd->irq_descr, hcd);
> - if (retval) {
> - xhci_err(xhci, "request interrupt %d failed\n",
> - pdev->irq);
> - return retval;
> - }
> - hcd->irq = pdev->irq;
> - }
> -
> - spin_lock_irq(&xhci->lock);
> /* step 4: set Run/Stop bit */
> command = xhci_readl(xhci, &xhci->op_regs->command);
> command |= CMD_RUN;
> --
> 1.7.0.4
>
>
>
--
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
