On Thu, Dec 23, 2010 at 10:55:39AM -0500, Alan Stern wrote:
> On Wed, 22 Dec 2010, Sarah Sharp wrote:
>
> > On Wed, Dec 22, 2010 at 09:31:49PM -0500, Alan Stern wrote:
> > > On Wed, 22 Dec 2010, Sarah Sharp wrote:
> > >
> > > > Alan,
> > > >
> > > > Please take a look at patch 1 if you have time.
> > >
> > > I still don't understand the reason for adding that test to
> > > suspend_common(). (And I would prefer it if the comment above the test
> > > were more grammatical.)
> >
> > If the xHCI driver has enabled MSI-X, then the legacy PCI irq is no
> > longer associated with our hardware. But the USB core still thinks it
> > is, and will sync pci_dev->irq. I've been told it would be harmless to
> > synchronize someone else's IRQ, but I'd rather not wait for them. It's
> > better to let the xHCI driver synchronize all the MSI-X irqs in its
> > suspend method.
>
> Then instead of guessing about whether or not pdev->irq should be the
> same as hcd->irq, and what it might mean if they are different, just
> add a new "don't synchronize irqs" flag to the usb_hcd structure.
Like so?
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 3799573..da9d96d 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(hcd))
+ 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..07c5f68 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -226,6 +226,7 @@ static int xhci_setup_msi(struct xhci_hcd *xhci)
static int xhci_setup_msix(struct xhci_hcd *xhci)
{
int i, ret = 0;
+ struct usb_hcd *hcd = xhci_to_hcd(xhci);
struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
/*
@@ -265,6 +266,7 @@ static int xhci_setup_msix(struct xhci_hcd *xhci)
goto disable_msix;
}
+ set_bit(HCD_FLAG_MSIX, &hcd->flags);
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);
}
+ clear_bit(HCD_FLAG_MSIX, &hcd->flags);
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..d9a8983 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -99,6 +99,7 @@ struct usb_hcd {
#define HCD_FLAG_POLL_RH 2 /* poll for rh status? */
#define HCD_FLAG_POLL_PENDING 3 /* status has changed? */
#define HCD_FLAG_WAKEUP_PENDING 4 /* root hub is resuming? */
+#define HCD_FLAG_MSIX 5 /* driver has MSI-X enabled? */
/* The flags can be tested using these macros; they are likely to
* be slightly faster than test_bit().
@@ -108,6 +109,7 @@ struct usb_hcd {
#define HCD_POLL_RH(hcd) ((hcd)->flags & (1U << HCD_FLAG_POLL_RH))
#define HCD_POLL_PENDING(hcd) ((hcd)->flags & (1U << HCD_FLAG_POLL_PENDING))
#define HCD_WAKEUP_PENDING(hcd) ((hcd)->flags & (1U << HCD_FLAG_WAKEUP_PENDING))
+#define HCD_MSIX_ENABLED(hcd) ((hcd)->flags & (1U << HCD_FLAG_MSIX))
/* Flags that get set only during HCD registration or removal. */
unsigned rh_registered:1;/* is root hub registered? */
--
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
