On Mon, Dec 27, 2010 at 02:14:56PM +0100, Rafael J. Wysocki wrote:
> On Monday, December 27, 2010, Andiry Xu wrote:
> > Synchronize the interrupts instead of free them in xhci_suspend(). This will
> > prevent a double free when the host is suspended and then the card removed.
> >
> > Set the flag hcd->msix_enabled when using MSI-X, and check the flag in
> > suspend_common(). MSI-X synchronization will be handled by xhci_suspend(),
> > and MSI/INTx will be synchronized in suspend_common().
> >
> > Reported-by: Matthew Garrett <mjg@xxxxxxxxxx>
> > Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
>
> Looks good, but you need to remove the xhci_cleanup_msix() from under
> the spinlock in _stop() and _shutdown() too.
There's a separate patch submitted by someone else to do that. I'll
take this patch and work out the conflicts.
Sarah Sharp
> > ---
> > drivers/usb/core/hcd-pci.c | 7 +++++-
> > drivers/usb/host/xhci.c | 46 ++++++++++++++-----------------------------
> > include/linux/usb/hcd.h | 1 +
> > 3 files changed, 22 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> > index 3799573..4de52dc 100644
> > --- a/drivers/usb/core/hcd-pci.c
> > +++ b/drivers/usb/core/hcd-pci.c
> > @@ -406,7 +406,12 @@ static int suspend_common(struct device *dev, bool do_wakeup)
> > return retval;
> > }
> >
> > - synchronize_irq(pci_dev->irq);
> > + /* If MSI-X is enabled, the driver will have synchronized all vectors
> > + * in pci_suspend(). If MSI or legacy PCI is enabled, that will be
> > + * synchronized here.
> > + */
> > + if (!hcd->msix_enabled)
> > + synchronize_irq(pci_dev->irq);
> >
> > /* Downstream ports from this root hub should already be quiesced, so
> > * there will be no DMA activity. Now we can shut down the upstream
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 45e4a31..d48edfa 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -226,7 +226,8 @@ static int xhci_setup_msi(struct xhci_hcd *xhci)
> > static int xhci_setup_msix(struct xhci_hcd *xhci)
> > {
> > int i, ret = 0;
> > - struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> > + struct usb_hcd *hcd = xhci_to_hcd(xhci);
> > + struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
> >
> > /*
> > * calculate number of msi-x vectors supported.
> > @@ -265,6 +266,7 @@ static int xhci_setup_msix(struct xhci_hcd *xhci)
> > goto disable_msix;
> > }
> >
> > + hcd->msix_enabled = 1;
> > return ret;
> >
> > disable_msix:
> > @@ -280,7 +282,8 @@ free_entries:
> > /* Free any IRQs and disable MSI-X */
> > static void xhci_cleanup_msix(struct xhci_hcd *xhci)
> > {
> > - struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> > + struct usb_hcd *hcd = xhci_to_hcd(xhci);
> > + struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
> >
> > xhci_free_irq(xhci);
> >
> > @@ -292,6 +295,7 @@ static void xhci_cleanup_msix(struct xhci_hcd *xhci)
> > pci_disable_msi(pdev);
> > }
> >
> > + hcd->msix_enabled = 0;
> > return;
> > }
> >
> > @@ -647,6 +651,7 @@ int xhci_suspend(struct xhci_hcd *xhci)
> > int rc = 0;
> > struct usb_hcd *hcd = xhci_to_hcd(xhci);
> > u32 command;
> > + int i;
> >
> > spin_lock_irq(&xhci->lock);
> > clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> > @@ -677,10 +682,15 @@ int xhci_suspend(struct xhci_hcd *xhci)
> > spin_unlock_irq(&xhci->lock);
> > return -ETIMEDOUT;
> > }
> > - /* step 5: remove core well power */
> > - xhci_cleanup_msix(xhci);
> > spin_unlock_irq(&xhci->lock);
> >
> > + /* step 5: remove core well power */
> > + /* synchronize irq when using MSI-X */
> > + if (xhci->msix_entries) {
> > + for (i = 0; i < xhci->msix_count; i++)
> > + synchronize_irq(xhci->msix_entries[i].vector);
> > + }
> > +
> > return rc;
> > }
> >
> > @@ -694,7 +704,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,9 +738,8 @@ 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);
> > spin_unlock_irq(&xhci->lock);
> > + xhci_cleanup_msix(xhci);
> >
> > #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
> > /* Tell the event ring poll function not to reschedule */
> > @@ -765,30 +773,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;
> > diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> > index 0b6e751..6c37d78 100644
> > --- a/include/linux/usb/hcd.h
> > +++ b/include/linux/usb/hcd.h
> > @@ -112,6 +112,7 @@ struct usb_hcd {
> > /* Flags that get set only during HCD registration or removal. */
> > unsigned rh_registered:1;/* is root hub registered? */
> > unsigned rh_pollable:1; /* may we poll the root hub? */
> > + unsigned msix_enabled:1; /* driver has MSI-X enabled? */
> >
> > /* The next flag is a stopgap, to be removed when all the HCDs
> > * support the new root-hub polling mechanism. */
> >
>
> --
> 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
--
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
