[PATCH v2 0/3] enable pci MSI/MSIX in usb core

[PATCH v2 0/3] enable pci MSI/MSIX in usb core

[Alex Shi]

This e-mail thread is correct and latest patchset. sorry for several minutes
ago incorrect patches.

I clean up code and combind them into serial. The first one in already in
stable, list here just to show clear patch dependency.

The second one is main code to enable MSI/MSIX in usb core.

The third one is just for potential function usage. 

Thanks&Regards!

Alex

[PATCH 1/3] USB: Try MSI first before line IRQ for Intel PCIe USB3 HCD

[Alex Shi]

From: Sarah Sharp <sarah.a.sharp@linux.intel.com>

We have a PCI USB xhci host controller on a new platform. It doesn't
have a line IRQ definition in BIOS. The Linux driver refuses to
initial this controller, but Windows works well because it only depends
on MSI.

Actually, Linux also can work for MSI. This patch skips the first line
IRQ checking for our HCD in usb-core pci probe, then try to enable MSI
firstly. That make this HCD works well under Linux.

This patch should be backported to kernels as old as 2.6.32.

Signed-off-by: Alex Shi <alex.shi@intel.com>
Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/usb/core/hcd-pci.c |    5 ++++-
 drivers/usb/core/hcd.c     |    6 ++++--
 drivers/usb/host/xhci.c    |    5 +++++
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index d136b8f..81e2c0d 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -187,7 +187,10 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return -ENODEV;
 	dev->current_state = PCI_D0;
 
-	if (!dev->irq) {
+	/* The xHCI driver supports MSI and MSI-X,
+	 * so don't fail if the BIOS doesn't provide a legacy IRQ.
+	 */
+	if (!dev->irq && (driver->flags & HCD_MASK) != HCD_USB3) {
 		dev_err(&dev->dev,
 			"Found HC with no IRQ.  Check BIOS/PCI %s setup!\n",
 			pci_name(dev));
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index eb19cba..e128232 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2447,8 +2447,10 @@ int usb_add_hcd(struct usb_hcd *hcd,
 			&& device_can_wakeup(&hcd->self.root_hub->dev))
 		dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
 
-	/* enable irqs just before we start the controller */
-	if (usb_hcd_is_primary_hcd(hcd)) {
+	/* enable irqs just before we start the controller,
+	 * if the BIOS provides legacy PCI irqs.
+	 */
+	if (usb_hcd_is_primary_hcd(hcd) && irqnum) {
 		retval = usb_hcd_request_irqs(hcd, irqnum, irqflags);
 		if (retval)
 			goto err_request_irq;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 6bbe3c3..c939f5f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -352,6 +352,11 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
 		/* hcd->irq is -1, we have MSI */
 		return 0;
 
+	if (!pdev->irq) {
+		xhci_err(xhci, "No msi-x/msi found and no IRQ in BIOS\n");
+		return -EINVAL;
+	}
+
 	/* fall back to legacy interrupt*/
 	ret = request_irq(pdev->irq, &usb_hcd_irq, IRQF_SHARED,
 			hcd->irq_descr, hcd);
-- 
1.6.3.3

[PATCH 2/3] usb: enable pci MSI/MSIX in usb core

[Alex Shi]

MSI/MSIX is the favorite interrupt for PCIe device. PCIe usb HCD should
be used more and more in future, but current USB core still just wants
line IRQ, only XHCI usb driver enabled MSI/MSIX.

This patch enabled pci MSI/MSIX in usb core for HCD, and removed MSI/MSIX
setup code from XHCI since it becomes redundant now.
There 3 places need prepare to enable MSI/MSIX in usb driver.
1, set HCD_MSI_FIRST in driver->flags to enable MSI/MSIX.
2, prepare a get_msix_num() to get msix vector numb.
3, prepare msi/msix_irq handler if needed.

XHCI is a good example for this.

Thanks for Sarah's effort on the MSI/MSIX enabling in XHCI. In fact most
of MSI/MSIX setup functions reuse from XHCI.

Signed-off-by: Alex Shi <alex.shi@intel.com>
---
 drivers/usb/core/hcd-pci.c  |  223 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/core/hcd.c      |   22 ++++-
 drivers/usb/host/xhci-pci.c |   16 ++--
 drivers/usb/host/xhci.c     |  216 +++---------------------------------------
 drivers/usb/host/xhci.h     |    5 +-
 include/linux/usb/hcd.h     |   17 ++++
 6 files changed, 276 insertions(+), 223 deletions(-)

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 81e2c0d..8475b25 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -153,6 +153,218 @@ static inline void wait_for_companions(struct pci_dev *d, struct usb_hcd *h) {}
 
 /*-------------------------------------------------------------------------*/
 
+static int hcd_free_msix(struct usb_hcd *hcd)
+{
+	int i;
+	struct pci_dev  *pdev = to_pci_dev(hcd->self.controller);
+
+	if (!hcd->msix_entries) {
+		printk(KERN_ERR "No msix entries found!\n");
+		return -EINVAL;
+	}
+	for (i = 0; i < hcd->msix_count; i++)
+		if (hcd->msix_entries[i].vector)
+			free_irq(hcd->msix_entries[i].vector,
+					hcd);
+
+	pci_disable_msix(pdev);
+	kfree(hcd->msix_entries);
+	hcd->msix_entries = NULL;
+	hcd->msix_enabled = 0;
+
+	return 0;
+}
+
+static int hcd_free_msi(struct usb_hcd *hcd)
+{
+	struct pci_dev  *pdev = to_pci_dev(hcd->self.controller);
+
+	if (pdev->irq > 0 && hcd->irq <0)
+		free_irq(pdev->irq, hcd);
+
+	pci_disable_msi(pdev);
+	return 0;
+}
+
+/* Free and disable msi/msix */
+void usb_hcd_free_msi_msix(struct usb_hcd *hcd)
+{
+	if (hcd->msix_entries)
+		hcd_free_msix(hcd);
+	else
+		hcd_free_msi(hcd);
+
+	return;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_free_msi_msix);
+
+void usb_hcd_msix_sync_irqs(struct usb_hcd *hcd)
+{
+	int i;
+	if (hcd->msix_entries) {
+		for (i = 0; i < hcd->msix_count; i++)
+			synchronize_irq(hcd->msix_entries[i].vector);
+	}
+
+}
+EXPORT_SYMBOL_GPL(usb_hcd_msix_sync_irqs);
+
+/*
+ * Set up MSI
+ */
+static int hcd_setup_msi(struct pci_dev *pdev, struct usb_hcd *hcd)
+{
+	int ret;
+
+	ret = pci_enable_msi(pdev);
+	if (ret)
+		dev_dbg(&pdev->dev, "failed to allocate MSI entry\n");
+	return ret;
+}
+
+/*
+ * Set up MSI-X
+ */
+static int hcd_setup_msix(struct pci_dev *pdev, struct usb_hcd *hcd)
+{
+	int i, ret = 0;
+
+	/*
+	 * calculate number of msi-x vectors supported.
+	 * Add additional 1 vector to ensure always available interrupt.
+	 */
+	hcd->msix_count = min((int)num_online_cpus() + 1,
+				hcd->driver->get_msix_num(hcd));
+
+	hcd->msix_entries =
+		kmalloc((sizeof(struct msix_entry))*hcd->msix_count,
+				GFP_KERNEL);
+	if (!hcd->msix_entries) {
+		dev_err(&pdev->dev, "Failed to allocate MSI-X entries\n");
+		hcd->msix_count = 0;
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < hcd->msix_count; i++) {
+		hcd->msix_entries[i].entry = i;
+		hcd->msix_entries[i].vector = 0;
+	}
+
+	ret = pci_enable_msix(pdev, hcd->msix_entries, hcd->msix_count);
+	if (ret) {
+		dev_dbg(&pdev->dev, "Failed to enable MSI-X\n");
+		goto free_entries;
+	}
+
+	return ret;
+
+free_entries:
+	kfree(hcd->msix_entries);
+	hcd->msix_entries = NULL;
+	hcd->msix_count = 0;
+	return ret;
+}
+
+/* Device for a quirk */
+#define PCI_VENDOR_ID_FRESCO_LOGIC	0x1b73
+#define PCI_DEVICE_ID_FRESCO_LOGIC_PDK	0x1000
+
+/* Check for buggy HCD devices, and driver's expectation for MSI.
+ * Now, only xhci driver want it by HCD_MSI_FIRST setting. Other driver
+ * like ehci/uhci can follow this setting, if they want.
+ */
+static int hcd_no_msi(struct pci_dev *pdev, struct usb_hcd *hcd)
+{
+	if (pdev->vendor == PCI_VENDOR_ID_FRESCO_LOGIC &&
+			pdev->device == PCI_DEVICE_ID_FRESCO_LOGIC_PDK) {
+
+		/* Fresco Logic confirms: all revisions of this chip do not
+		 * support MSI, even though some of them claim to in their PCI
+		 * capabilities.
+		 */
+		dev_dbg(&pdev->dev, "QUIRK: Fresco Logic revision %u "
+				"has broken MSI implementation\n",
+				pdev->revision);
+		return 1;
+	}
+	if (!(hcd->driver->flags & HCD_MSI_FIRST))
+		return 1;
+
+	return 0;
+}
+
+/* setup msi/msix interrupts. if fails, fallback to line irq */
+static int hcd_setup_int(struct pci_dev *pdev, struct usb_hcd *hcd)
+{
+	int ret;
+
+	hcd->irq = -1;
+
+	if (!hcd_no_msi(pdev, hcd)) {
+
+		ret = hcd_setup_msix(pdev, hcd);
+		if (ret)
+			/* fall back to msi*/
+			ret = hcd_setup_msi(pdev, hcd);
+
+		if (!ret)
+			/* hcd->irq is -1, we have MSI */
+			return 0;
+	}
+
+	if (!pdev->irq) {
+		dev_err(&pdev->dev, "No msi or line irq found!\n");
+		return -1;
+	}
+	/* fallback to line irq */
+	hcd->irq = pdev->irq;
+	return 0;
+}
+
+int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd, unsigned int irqnum,
+					unsigned long irqflags)
+{
+	int retval = 1;
+	if (hcd->msix_entries && hcd->driver->msix_irq) {
+		int i;
+		/* register hc_driver's msix_irq handler */
+		for (i = 0; i < hcd->msix_count; i++) {
+			retval = request_irq(hcd->msix_entries[i].vector,
+					(irq_handler_t)hcd->driver->msix_irq,
+					0, hcd->driver->description, hcd);
+			if (retval) {
+				hcd_free_msix(hcd);
+				break;
+			}
+		}
+		if (i == hcd->msix_count)
+			hcd->msix_enabled = 1;
+	} else if (hcd->irq == -1 && hcd->driver->msi_irq) {
+		/* register hc_driver's msi_irq handler */
+		retval = request_irq(irqnum,
+					(irq_handler_t)hcd->driver->msi_irq,
+					0, hcd->driver->description, hcd);
+		if (retval)
+			hcd_free_msi(hcd);
+	}
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_request_msi_msix_irqs);
+
+/* setup msi/msix interrupts and requestion irqs for them */
+int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd)
+{
+	struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
+	int ret = -1;
+
+	if (!hcd_setup_int(pdev, hcd))
+		ret = usb_hcd_request_msi_msix_irqs(hcd,
+				pdev->irq, IRQF_SHARED);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_register_msi_msix_irqs);
+
 /* configure so an HC device and id are always provided */
 /* always called with process context; sleeping is OK */
 
@@ -187,10 +399,8 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return -ENODEV;
 	dev->current_state = PCI_D0;
 
-	/* The xHCI driver supports MSI and MSI-X,
-	 * so don't fail if the BIOS doesn't provide a legacy IRQ.
-	 */
-	if (!dev->irq && (driver->flags & HCD_MASK) != HCD_USB3) {
+	/* skip irq check if hcd wants MSI firstly. */
+	if (!(driver->flags & HCD_MSI_FIRST) && !dev->irq) {
 		dev_err(&dev->dev,
 			"Found HC with no IRQ.  Check BIOS/PCI %s setup!\n",
 			pci_name(dev));
@@ -245,6 +455,11 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	pci_set_master(dev);
 
+	/* try enable MSI, if fail, seek line irq */
+	retval = hcd_setup_int(dev, hcd);
+	if (retval != 0)
+		goto unmap_registers;
+
 	retval = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
 	if (retval != 0)
 		goto unmap_registers;
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index e128232..579cbd3 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2322,7 +2322,7 @@ int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
 }
 EXPORT_SYMBOL_GPL(usb_hcd_is_primary_hcd);
 
-static int usb_hcd_request_irqs(struct usb_hcd *hcd,
+static int usb_hcd_request_default_irqs(struct usb_hcd *hcd,
 		unsigned int irqnum, unsigned long irqflags)
 {
 	int retval;
@@ -2362,6 +2362,20 @@ static int usb_hcd_request_irqs(struct usb_hcd *hcd,
 	return 0;
 }
 
+int usb_hcd_request_irqs(struct usb_hcd *hcd, unsigned int irqnum,
+		unsigned long irqflags)
+{
+	int retval = 1;
+
+#ifdef CONFIG_PCI
+	retval = usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags);
+#endif
+	if (retval)
+		retval = usb_hcd_request_default_irqs(hcd, irqnum, irqflags);
+
+	return retval;
+}
+
 /**
  * usb_add_hcd - finish generic HCD structure initialization and register
  * @hcd: the usb_hcd structure to initialize
@@ -2447,10 +2461,8 @@ int usb_add_hcd(struct usb_hcd *hcd,
 			&& device_can_wakeup(&hcd->self.root_hub->dev))
 		dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
 
-	/* enable irqs just before we start the controller,
-	 * if the BIOS provides legacy PCI irqs.
-	 */
-	if (usb_hcd_is_primary_hcd(hcd) && irqnum) {
+	/* enable irqs just before we start the controller */
+	if (usb_hcd_is_primary_hcd(hcd)) {
 		retval = usb_hcd_request_irqs(hcd, irqnum, irqflags);
 		if (retval)
 			goto err_request_irq;
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index ef98b38..39be795 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -64,14 +64,6 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 			xhci_dbg(xhci, "QUIRK: Fresco Logic xHC needs configure"
 					" endpoint cmd after reset endpoint\n");
 		}
-		/* Fresco Logic confirms: all revisions of this chip do not
-		 * support MSI, even though some of them claim to in their PCI
-		 * capabilities.
-		 */
-		xhci->quirks |= XHCI_BROKEN_MSI;
-		xhci_dbg(xhci, "QUIRK: Fresco Logic revision %u "
-				"has broken MSI implementation\n",
-				pdev->revision);
 	}
 
 	if (pdev->vendor == PCI_VENDOR_ID_NEC)
@@ -124,6 +116,7 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
 	return retval;
 }
 
+
 /*
  * We need to register our own PCI probe function (instead of the USB core's
  * function) in order to create a second roothub under xHCI.
@@ -136,6 +129,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	struct usb_hcd *hcd;
 
 	driver = (struct hc_driver *)id->driver_data;
+
 	/* Register the USB 2.0 roothub.
 	 * FIXME: USB core must know to register the USB 2.0 roothub first.
 	 * This is sort of silly, because we could just set the HCD driver flags
@@ -243,13 +237,17 @@ static const struct hc_driver xhci_pci_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq =			xhci_irq,
-	.flags =		HCD_MEMORY | HCD_USB3 | HCD_SHARED,
+	.msi_irq =		xhci_msi_irq,
+	.msix_irq =		xhci_msi_irq,
+	.flags =		HCD_MEMORY | HCD_USB3 | HCD_SHARED |
+				HCD_MSI_FIRST,
 
 	/*
 	 * basic lifecycle operations
 	 */
 	.reset =		xhci_pci_setup,
 	.start =		xhci_run,
+	.get_msix_num =		xhci_get_msix_num,
 #ifdef CONFIG_PM
 	.pci_suspend =          xhci_pci_suspend,
 	.pci_resume =           xhci_pci_resume,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index c939f5f..95dc48f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -176,210 +176,24 @@ int xhci_reset(struct xhci_hcd *xhci)
 }
 
 #ifdef CONFIG_PCI
-static int xhci_free_msi(struct xhci_hcd *xhci)
-{
-	int i;
-
-	if (!xhci->msix_entries)
-		return -EINVAL;
-
-	for (i = 0; i < xhci->msix_count; i++)
-		if (xhci->msix_entries[i].vector)
-			free_irq(xhci->msix_entries[i].vector,
-					xhci_to_hcd(xhci));
-	return 0;
-}
-
-/*
- * Set up MSI
- */
-static int xhci_setup_msi(struct xhci_hcd *xhci)
-{
-	int ret;
-	struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
-
-	ret = pci_enable_msi(pdev);
-	if (ret) {
-		xhci_dbg(xhci, "failed to allocate MSI entry\n");
-		return ret;
-	}
-
-	ret = request_irq(pdev->irq, (irq_handler_t)xhci_msi_irq,
-				0, "xhci_hcd", xhci_to_hcd(xhci));
-	if (ret) {
-		xhci_dbg(xhci, "disable MSI interrupt\n");
-		pci_disable_msi(pdev);
-	}
-
-	return ret;
-}
-
-/*
- * Free IRQs
- * free all IRQs request
- */
-static void xhci_free_irq(struct xhci_hcd *xhci)
-{
-	struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
-	int ret;
-
-	/* return if using legacy interrupt */
-	if (xhci_to_hcd(xhci)->irq >= 0)
-		return;
-
-	ret = xhci_free_msi(xhci);
-	if (!ret)
-		return;
-	if (pdev->irq >= 0)
-		free_irq(pdev->irq, xhci_to_hcd(xhci));
-
-	return;
-}
-
-/*
- * Set up MSI-X
- */
-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(hcd->self.controller);
-
-	/*
-	 * calculate number of msi-x vectors supported.
-	 * - HCS_MAX_INTRS: the max number of interrupts the host can handle,
-	 *   with max number of interrupters based on the xhci HCSPARAMS1.
-	 * - num_online_cpus: maximum msi-x vectors per CPUs core.
-	 *   Add additional 1 vector to ensure always available interrupt.
-	 */
-	xhci->msix_count = min(num_online_cpus() + 1,
-				HCS_MAX_INTRS(xhci->hcs_params1));
-
-	xhci->msix_entries =
-		kmalloc((sizeof(struct msix_entry))*xhci->msix_count,
-				GFP_KERNEL);
-	if (!xhci->msix_entries) {
-		xhci_err(xhci, "Failed to allocate MSI-X entries\n");
-		return -ENOMEM;
-	}
-
-	for (i = 0; i < xhci->msix_count; i++) {
-		xhci->msix_entries[i].entry = i;
-		xhci->msix_entries[i].vector = 0;
-	}
-
-	ret = pci_enable_msix(pdev, xhci->msix_entries, xhci->msix_count);
-	if (ret) {
-		xhci_dbg(xhci, "Failed to enable MSI-X\n");
-		goto free_entries;
-	}
-
-	for (i = 0; i < xhci->msix_count; i++) {
-		ret = request_irq(xhci->msix_entries[i].vector,
-				(irq_handler_t)xhci_msi_irq,
-				0, "xhci_hcd", xhci_to_hcd(xhci));
-		if (ret)
-			goto disable_msix;
-	}
-
-	hcd->msix_enabled = 1;
-	return ret;
-
-disable_msix:
-	xhci_dbg(xhci, "disable MSI-X interrupt\n");
-	xhci_free_irq(xhci);
-	pci_disable_msix(pdev);
-free_entries:
-	kfree(xhci->msix_entries);
-	xhci->msix_entries = NULL;
-	return ret;
-}
-
-/* Free any IRQs and disable MSI-X */
-static void xhci_cleanup_msix(struct xhci_hcd *xhci)
-{
-	struct usb_hcd *hcd = xhci_to_hcd(xhci);
-	struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
-
-	xhci_free_irq(xhci);
-
-	if (xhci->msix_entries) {
-		pci_disable_msix(pdev);
-		kfree(xhci->msix_entries);
-		xhci->msix_entries = NULL;
-	} else {
-		pci_disable_msi(pdev);
-	}
-
-	hcd->msix_enabled = 0;
-	return;
-}
 
 static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
 {
-	int i;
-
-	if (xhci->msix_entries) {
-		for (i = 0; i < xhci->msix_count; i++)
-			synchronize_irq(xhci->msix_entries[i].vector);
-	}
+	usb_hcd_msix_sync_irqs(xhci_to_hcd(xhci));
 }
 
-static int xhci_try_enable_msi(struct usb_hcd *hcd)
+/* called in interrupt setup during pci probe() */
+int xhci_get_msix_num(struct usb_hcd *hcd)
 {
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
-	struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
-	int ret;
-
-	/*
-	 * Some Fresco Logic host controllers advertise MSI, but fail to
-	 * generate interrupts.  Don't even try to enable MSI.
-	 */
-	if (xhci->quirks & XHCI_BROKEN_MSI)
-		return 0;
-
-	/* unregister the legacy interrupt */
-	if (hcd->irq)
-		free_irq(hcd->irq, hcd);
-	hcd->irq = -1;
 
-	ret = xhci_setup_msix(xhci);
-	if (ret)
-		/* fall back to msi*/
-		ret = xhci_setup_msi(xhci);
-
-	if (!ret)
-		/* hcd->irq is -1, we have MSI */
-		return 0;
-
-	if (!pdev->irq) {
-		xhci_err(xhci, "No msi-x/msi found and no IRQ in BIOS\n");
-		return -EINVAL;
-	}
-
-	/* fall back to legacy interrupt*/
-	ret = request_irq(pdev->irq, &usb_hcd_irq, IRQF_SHARED,
-			hcd->irq_descr, hcd);
-	if (ret) {
-		xhci_err(xhci, "request interrupt %d failed\n",
-				pdev->irq);
-		return ret;
-	}
-	hcd->irq = pdev->irq;
-	return 0;
+	/* danger: xhci may be null, but it's useless in xhci_readl() now */
+	return HCS_MAX_INTRS(xhci_readl(xhci,
+			&((struct xhci_cap_regs *)hcd->regs)->hcs_params1));
 }
 
 #else
 
-static int xhci_try_enable_msi(struct usb_hcd *hcd)
-{
-	return 0;
-}
-
-static void xhci_cleanup_msix(struct xhci_hcd *xhci)
-{
-}
-
 static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
 {
 }
@@ -411,7 +225,6 @@ int xhci_init(struct usb_hcd *hcd)
 
 	return retval;
 }
-
 /*-------------------------------------------------------------------------*/
 
 
@@ -497,7 +310,6 @@ int xhci_run(struct usb_hcd *hcd)
 {
 	u32 temp;
 	u64 temp_64;
-	int ret;
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 
 	/* Start the xHCI host controller running only after the USB 2.0 roothub
@@ -510,10 +322,6 @@ int xhci_run(struct usb_hcd *hcd)
 
 	xhci_dbg(xhci, "xhci_run\n");
 
-	ret = xhci_try_enable_msi(hcd);
-	if (ret)
-		return ret;
-
 #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
 	init_timer(&xhci->event_ring_timer);
 	xhci->event_ring_timer.data = (unsigned long) xhci;
@@ -609,7 +417,7 @@ void xhci_stop(struct usb_hcd *hcd)
 	xhci_reset(xhci);
 	spin_unlock_irq(&xhci->lock);
 
-	xhci_cleanup_msix(xhci);
+	usb_hcd_free_msi_msix(hcd);
 
 #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
 	/* Tell the event ring poll function not to reschedule */
@@ -651,7 +459,7 @@ void xhci_shutdown(struct usb_hcd *hcd)
 	xhci_halt(xhci);
 	spin_unlock_irq(&xhci->lock);
 
-	xhci_cleanup_msix(xhci);
+	usb_hcd_free_msi_msix(hcd);
 
 	xhci_dbg(xhci, "xhci_shutdown completed - status = %x\n",
 		    xhci_readl(xhci, &xhci->op_regs->status));
@@ -853,7 +661,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 		xhci_halt(xhci);
 		xhci_reset(xhci);
 		spin_unlock_irq(&xhci->lock);
-		xhci_cleanup_msix(xhci);
+		usb_hcd_free_msi_msix(hcd);
 
 #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
 		/* Tell the event ring poll function not to reschedule */
@@ -888,7 +696,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 		if (retval)
 			return retval;
 		xhci_dbg(xhci, "Start the primary HCD\n");
-		retval = xhci_run(hcd->primary_hcd);
+		if (dev_is_pci(hcd->self.controller))
+			retval = usb_hcd_register_msi_msix_irqs(
+						hcd->primary_hcd);
+		if (!retval)
+			retval = xhci_run(hcd->primary_hcd);
 		if (!retval) {
 			xhci_dbg(xhci, "Start the secondary HCD\n");
 			retval = xhci_run(secondary_hcd);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index fb99c83..8d511dd 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1388,9 +1388,6 @@ struct xhci_hcd {
 	int		page_size;
 	/* Valid values are 12 to 20, inclusive */
 	int		page_shift;
-	/* msi-x vectors */
-	int		msix_count;
-	struct msix_entry	*msix_entries;
 	/* data structures */
 	struct xhci_device_context_array *dcbaa;
 	struct xhci_ring	*cmd_ring;
@@ -1643,9 +1640,11 @@ void xhci_free_command(struct xhci_hcd *xhci,
 /* xHCI PCI glue */
 int xhci_register_pci(void);
 void xhci_unregister_pci(void);
+int xhci_get_msix_num(struct usb_hcd *hcd);
 #else
 static inline int xhci_register_pci(void) { return 0; }
 static inline void xhci_unregister_pci(void) {}
+static inline int xhci_get_msix_num(struct usb_hcd *hcd) { return 0; }
 #endif
 
 /* xHCI host controller glue */
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index b2f62f3..5253c02 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -22,6 +22,7 @@
 #ifdef __KERNEL__
 
 #include <linux/rwsem.h>
+#include <linux/pci.h>		/* for struct msix_entry */
 
 #define MAX_TOPO_LEVEL		6
 
@@ -133,6 +134,12 @@ struct usb_hcd {
 	u64			rsrc_len;	/* memory/io resource length */
 	unsigned		power_budget;	/* in mA, 0 = no limit */
 
+#ifdef CONFIG_PCI
+	/* msi-x vectors */
+	int			msix_count;
+	struct msix_entry	*msix_entries;
+#endif
+
 	/* bandwidth_mutex should be taken before adding or removing
 	 * any new bus bandwidth constraints:
 	 *   1. Before adding a configuration for a new device.
@@ -205,11 +212,14 @@ struct hc_driver {
 
 	/* irq handler */
 	irqreturn_t	(*irq) (struct usb_hcd *hcd);
+	irqreturn_t	(*msi_irq) (int irq, struct usb_hcd *hcd);
+	irqreturn_t	(*msix_irq) (int irq, struct usb_hcd *hcd);
 
 	int	flags;
 #define	HCD_MEMORY	0x0001		/* HC regs use memory (else I/O) */
 #define	HCD_LOCAL_MEM	0x0002		/* HC needs local memory */
 #define	HCD_SHARED	0x0004		/* Two (or more) usb_hcds share HW */
+#define	HCD_MSI_FIRST	0x0008		/* Try to get MSI first, PCI only */
 #define	HCD_USB11	0x0010		/* USB 1.1 */
 #define	HCD_USB2	0x0020		/* USB 2.0 */
 #define	HCD_USB3	0x0040		/* USB 3.0 */
@@ -218,6 +228,7 @@ struct hc_driver {
 	/* called to init HCD and root hub */
 	int	(*reset) (struct usb_hcd *hcd);
 	int	(*start) (struct usb_hcd *hcd);
+	int	(*get_msix_num) (struct usb_hcd *hcd);
 
 	/* NOTE:  these suspend/resume calls relate to the HC as
 	 * a whole, not just the root hub; they're for PCI bus glue.
@@ -393,6 +404,12 @@ extern int usb_hcd_pci_probe(struct pci_dev *dev,
 extern void usb_hcd_pci_remove(struct pci_dev *dev);
 extern void usb_hcd_pci_shutdown(struct pci_dev *dev);
 
+extern void usb_hcd_free_msi_msix(struct usb_hcd *hcd);
+extern void usb_hcd_msix_sync_irqs(struct usb_hcd *hcd);
+extern int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd,
+		unsigned int irqnum, unsigned long irqflags);
+extern int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd);
+
 #ifdef CONFIG_PM_SLEEP
 extern const struct dev_pm_ops usb_hcd_pci_pm_ops;
 #endif
-- 
1.6.3.3

[PATCH 3/3] usb: export usb_hcd_request_irqs

[Alex Shi]

May it is possible be used by drivers.

Signed-off-by: Alex Shi <alex.shi@intel.com>
---
 drivers/usb/core/hcd.c  |    1 +
 include/linux/usb/hcd.h |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 579cbd3..226235a 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2375,6 +2375,7 @@ int usb_hcd_request_irqs(struct usb_hcd *hcd, unsigned int irqnum,
 
 	return retval;
 }
+EXPORT_SYMBOL_GPL(usb_hcd_request_irqs);
 
 /**
  * usb_add_hcd - finish generic HCD structure initialization and register
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 5253c02..d58e710 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -392,6 +392,8 @@ extern int usb_hcd_is_primary_hcd(struct usb_hcd *hcd);
 extern int usb_add_hcd(struct usb_hcd *hcd,
 		unsigned int irqnum, unsigned long irqflags);
 extern void usb_remove_hcd(struct usb_hcd *hcd);
+extern int usb_hcd_request_irqs(struct usb_hcd *hcd,
+		unsigned int irqnum, unsigned long irqflags);
 
 struct platform_device;
 extern void usb_hcd_platform_shutdown(struct platform_device *dev);
-- 
1.6.3.3

Re: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core

[Sarah Sharp]

On Mon, Feb 20, 2012 at 05:05:32PM +0800, Alex Shi wrote:
> MSI/MSIX is the favorite interrupt for PCIe device. PCIe usb HCD should
> be used more and more in future, but current USB core still just wants
> line IRQ, only XHCI usb driver enabled MSI/MSIX.
> 
> This patch enabled pci MSI/MSIX in usb core for HCD, and removed MSI/MSIX
> setup code from XHCI since it becomes redundant now.
> There 3 places need prepare to enable MSI/MSIX in usb driver.
> 1, set HCD_MSI_FIRST in driver->flags to enable MSI/MSIX.
> 2, prepare a get_msix_num() to get msix vector numb.
> 3, prepare msi/msix_irq handler if needed.
> 
> XHCI is a good example for this.
> 
> Thanks for Sarah's effort on the MSI/MSIX enabling in XHCI. In fact most
> of MSI/MSIX setup functions reuse from XHCI.

Comments below.

Felipe, can you please review this patch for the effects on non-PCI
hosts?  I think nothing will change, since this patchset just modifies
the USB core PCI initialization flow, but I need your eyes on this. :)

> Signed-off-by: Alex Shi <alex.shi@intel.com>
> ---
>  drivers/usb/core/hcd-pci.c  |  223 ++++++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/core/hcd.c      |   22 ++++-
>  drivers/usb/host/xhci-pci.c |   16 ++--
>  drivers/usb/host/xhci.c     |  216 +++---------------------------------------
>  drivers/usb/host/xhci.h     |    5 +-
>  include/linux/usb/hcd.h     |   17 ++++
>  6 files changed, 276 insertions(+), 223 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> index 81e2c0d..8475b25 100644
> --- a/drivers/usb/core/hcd-pci.c
> +++ b/drivers/usb/core/hcd-pci.c
> @@ -153,6 +153,218 @@ static inline void wait_for_companions(struct pci_dev *d, struct usb_hcd *h) {}
>  
>  /*-------------------------------------------------------------------------*/
>  
> +static int hcd_free_msix(struct usb_hcd *hcd)
> +{
> +	int i;
> +	struct pci_dev  *pdev = to_pci_dev(hcd->self.controller);
> +
> +	if (!hcd->msix_entries) {
> +		printk(KERN_ERR "No msix entries found!\n");
> +		return -EINVAL;
> +	}
> +	for (i = 0; i < hcd->msix_count; i++)
> +		if (hcd->msix_entries[i].vector)
> +			free_irq(hcd->msix_entries[i].vector,
> +					hcd);
> +
> +	pci_disable_msix(pdev);
> +	kfree(hcd->msix_entries);
> +	hcd->msix_entries = NULL;
> +	hcd->msix_enabled = 0;
> +
> +	return 0;
> +}
> +
> +static int hcd_free_msi(struct usb_hcd *hcd)
> +{
> +	struct pci_dev  *pdev = to_pci_dev(hcd->self.controller);
> +
> +	if (pdev->irq > 0 && hcd->irq <0)
> +		free_irq(pdev->irq, hcd);
> +
> +	pci_disable_msi(pdev);
> +	return 0;
> +}
> +
> +/* Free and disable msi/msix */
> +void usb_hcd_free_msi_msix(struct usb_hcd *hcd)
> +{
> +	if (hcd->msix_entries)
> +		hcd_free_msix(hcd);
> +	else
> +		hcd_free_msi(hcd);
> +
> +	return;
> +}
> +EXPORT_SYMBOL_GPL(usb_hcd_free_msi_msix);
> +
> +void usb_hcd_msix_sync_irqs(struct usb_hcd *hcd)
> +{
> +	int i;
> +	if (hcd->msix_entries) {
> +		for (i = 0; i < hcd->msix_count; i++)
> +			synchronize_irq(hcd->msix_entries[i].vector);
> +	}
> +
> +}
> +EXPORT_SYMBOL_GPL(usb_hcd_msix_sync_irqs);
> +
> +/*
> + * Set up MSI
> + */
> +static int hcd_setup_msi(struct pci_dev *pdev, struct usb_hcd *hcd)
> +{
> +	int ret;
> +
> +	ret = pci_enable_msi(pdev);
> +	if (ret)
> +		dev_dbg(&pdev->dev, "failed to allocate MSI entry\n");
> +	return ret;
> +}
> +
> +/*
> + * Set up MSI-X
> + */
> +static int hcd_setup_msix(struct pci_dev *pdev, struct usb_hcd *hcd)
> +{
> +	int i, ret = 0;
> +
> +	/*
> +	 * calculate number of msi-x vectors supported.
> +	 * Add additional 1 vector to ensure always available interrupt.
> +	 */
> +	hcd->msix_count = min((int)num_online_cpus() + 1,
> +				hcd->driver->get_msix_num(hcd));
> +
> +	hcd->msix_entries =
> +		kmalloc((sizeof(struct msix_entry))*hcd->msix_count,
> +				GFP_KERNEL);
> +	if (!hcd->msix_entries) {
> +		dev_err(&pdev->dev, "Failed to allocate MSI-X entries\n");
> +		hcd->msix_count = 0;
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < hcd->msix_count; i++) {
> +		hcd->msix_entries[i].entry = i;
> +		hcd->msix_entries[i].vector = 0;
> +	}
> +
> +	ret = pci_enable_msix(pdev, hcd->msix_entries, hcd->msix_count);
> +	if (ret) {
> +		dev_dbg(&pdev->dev, "Failed to enable MSI-X\n");
> +		goto free_entries;
> +	}
> +
> +	return ret;
> +
> +free_entries:
> +	kfree(hcd->msix_entries);
> +	hcd->msix_entries = NULL;
> +	hcd->msix_count = 0;
> +	return ret;
> +}
> +
> +/* Device for a quirk */
> +#define PCI_VENDOR_ID_FRESCO_LOGIC	0x1b73
> +#define PCI_DEVICE_ID_FRESCO_LOGIC_PDK	0x1000

This PCI device and vendor ID is now used in two drivers (xhci and USB
core).  Please create a separate patch to add this ID to the pci_ids.h
file, and remove the reference here and in the xHCI driver.

> +
> +/* Check for buggy HCD devices, and driver's expectation for MSI.
> + * Now, only xhci driver want it by HCD_MSI_FIRST setting. Other driver
> + * like ehci/uhci can follow this setting, if they want.
> + */
> +static int hcd_no_msi(struct pci_dev *pdev, struct usb_hcd *hcd)
> +{
> +	if (pdev->vendor == PCI_VENDOR_ID_FRESCO_LOGIC &&
> +			pdev->device == PCI_DEVICE_ID_FRESCO_LOGIC_PDK) {
> +
> +		/* Fresco Logic confirms: all revisions of this chip do not
> +		 * support MSI, even though some of them claim to in their PCI
> +		 * capabilities.
> +		 */
> +		dev_dbg(&pdev->dev, "QUIRK: Fresco Logic revision %u "
> +				"has broken MSI implementation\n",
> +				pdev->revision);
> +		return 1;
> +	}
> +	if (!(hcd->driver->flags & HCD_MSI_FIRST))
> +		return 1;
> +
> +	return 0;
> +}
> +
> +/* setup msi/msix interrupts. if fails, fallback to line irq */
> +static int hcd_setup_int(struct pci_dev *pdev, struct usb_hcd *hcd)
> +{
> +	int ret;
> +
> +	hcd->irq = -1;
> +
> +	if (!hcd_no_msi(pdev, hcd)) {

Why don't you rename hcd_no_msi() to hcd_supports_msi() and remove the
negation of the return value?

> +
> +		ret = hcd_setup_msix(pdev, hcd);
> +		if (ret)
> +			/* fall back to msi*/
> +			ret = hcd_setup_msi(pdev, hcd);
> +
> +		if (!ret)
> +			/* hcd->irq is -1, we have MSI */
> +			return 0;
> +	}
> +
> +	if (!pdev->irq) {
> +		dev_err(&pdev->dev, "No msi or line irq found!\n");
> +		return -1;
> +	}
> +	/* fallback to line irq */
> +	hcd->irq = pdev->irq;
> +	return 0;
> +}
> +
> +int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd, unsigned int irqnum,
> +					unsigned long irqflags)
> +{
> +	int retval = 1;
> +	if (hcd->msix_entries && hcd->driver->msix_irq) {
> +		int i;
> +		/* register hc_driver's msix_irq handler */
> +		for (i = 0; i < hcd->msix_count; i++) {
> +			retval = request_irq(hcd->msix_entries[i].vector,
> +					(irq_handler_t)hcd->driver->msix_irq,
> +					0, hcd->driver->description, hcd);

I really think you need to allow the host controller driver to set
different pointers for the msix data pointer.  It's something that we
need to figure out, so that we can have the infrastructure in place for
multiple event rings.

I'm not sure whether the new get MSIX count needs to allow the xHCI
driver to return an array of pointers, or if the driver can modify the
irq pointer later?  I don't think you can modify the irq data pointer
after it's been requested (that would lead to all kinds of race
conditions, I think).

It's probably better to allow the xHCI driver to pass this function the
pointers it needs for each MSI-X vector.  You'll always call
usb_hcd_request_msi_msix_irqs() after you call xhci_init(), correct?  At
that point, we should have allocated the multiple event rings, so it
should be easy to pass the pointers to this function.

> +			if (retval) {
> +				hcd_free_msix(hcd);
> +				break;
> +			}
> +		}
> +		if (i == hcd->msix_count)
> +			hcd->msix_enabled = 1;
> +	} else if (hcd->irq == -1 && hcd->driver->msi_irq) {
> +		/* register hc_driver's msi_irq handler */
> +		retval = request_irq(irqnum,
> +					(irq_handler_t)hcd->driver->msi_irq,
> +					0, hcd->driver->description, hcd);
> +		if (retval)
> +			hcd_free_msi(hcd);
> +	}
> +
> +	return retval;
> +}
> +EXPORT_SYMBOL_GPL(usb_hcd_request_msi_msix_irqs);
> +
> +/* setup msi/msix interrupts and requestion irqs for them */
> +int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd)
> +{
> +	struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
> +	int ret = -1;
> +
> +	if (!hcd_setup_int(pdev, hcd))
> +		ret = usb_hcd_request_msi_msix_irqs(hcd,
> +				pdev->irq, IRQF_SHARED);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_hcd_register_msi_msix_irqs);
> +
>  /* configure so an HC device and id are always provided */
>  /* always called with process context; sleeping is OK */
>  
> @@ -187,10 +399,8 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  		return -ENODEV;
>  	dev->current_state = PCI_D0;
>  
> -	/* The xHCI driver supports MSI and MSI-X,
> -	 * so don't fail if the BIOS doesn't provide a legacy IRQ.
> -	 */
> -	if (!dev->irq && (driver->flags & HCD_MASK) != HCD_USB3) {
> +	/* skip irq check if hcd wants MSI firstly. */
> +	if (!(driver->flags & HCD_MSI_FIRST) && !dev->irq) {
>  		dev_err(&dev->dev,
>  			"Found HC with no IRQ.  Check BIOS/PCI %s setup!\n",
>  			pci_name(dev));
> @@ -245,6 +455,11 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  
>  	pci_set_master(dev);
>  
> +	/* try enable MSI, if fail, seek line irq */
> +	retval = hcd_setup_int(dev, hcd);
> +	if (retval != 0)
> +		goto unmap_registers;
> +
>  	retval = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
>  	if (retval != 0)
>  		goto unmap_registers;
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index e128232..579cbd3 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2322,7 +2322,7 @@ int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
>  }
>  EXPORT_SYMBOL_GPL(usb_hcd_is_primary_hcd);
>  
> -static int usb_hcd_request_irqs(struct usb_hcd *hcd,
> +static int usb_hcd_request_default_irqs(struct usb_hcd *hcd,
>  		unsigned int irqnum, unsigned long irqflags)
>  {
>  	int retval;
> @@ -2362,6 +2362,20 @@ static int usb_hcd_request_irqs(struct usb_hcd *hcd,
>  	return 0;
>  }
>  
> +int usb_hcd_request_irqs(struct usb_hcd *hcd, unsigned int irqnum,
> +		unsigned long irqflags)
> +{
> +	int retval = 1;
> +
> +#ifdef CONFIG_PCI
> +	retval = usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags);
> +#endif
> +	if (retval)
> +		retval = usb_hcd_request_default_irqs(hcd, irqnum, irqflags);
> +
> +	return retval;
> +}
> +
>  /**
>   * usb_add_hcd - finish generic HCD structure initialization and register
>   * @hcd: the usb_hcd structure to initialize
> @@ -2447,10 +2461,8 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  			&& device_can_wakeup(&hcd->self.root_hub->dev))
>  		dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
>  
> -	/* enable irqs just before we start the controller,
> -	 * if the BIOS provides legacy PCI irqs.
> -	 */
> -	if (usb_hcd_is_primary_hcd(hcd) && irqnum) {
> +	/* enable irqs just before we start the controller */
> +	if (usb_hcd_is_primary_hcd(hcd)) {
>  		retval = usb_hcd_request_irqs(hcd, irqnum, irqflags);
>  		if (retval)
>  			goto err_request_irq;
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index ef98b38..39be795 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -64,14 +64,6 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>  			xhci_dbg(xhci, "QUIRK: Fresco Logic xHC needs configure"
>  					" endpoint cmd after reset endpoint\n");
>  		}
> -		/* Fresco Logic confirms: all revisions of this chip do not
> -		 * support MSI, even though some of them claim to in their PCI
> -		 * capabilities.
> -		 */
> -		xhci->quirks |= XHCI_BROKEN_MSI;
> -		xhci_dbg(xhci, "QUIRK: Fresco Logic revision %u "
> -				"has broken MSI implementation\n",
> -				pdev->revision);
>  	}
>  
>  	if (pdev->vendor == PCI_VENDOR_ID_NEC)
> @@ -124,6 +116,7 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
>  	return retval;
>  }
>  
> +

Don't add spaces here.

>  /*
>   * We need to register our own PCI probe function (instead of the USB core's
>   * function) in order to create a second roothub under xHCI.
> @@ -136,6 +129,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	struct usb_hcd *hcd;
>  
>  	driver = (struct hc_driver *)id->driver_data;
> +

Or here.

>  	/* Register the USB 2.0 roothub.
>  	 * FIXME: USB core must know to register the USB 2.0 roothub first.
>  	 * This is sort of silly, because we could just set the HCD driver flags
> @@ -243,13 +237,17 @@ static const struct hc_driver xhci_pci_hc_driver = {
>  	 * generic hardware linkage
>  	 */
>  	.irq =			xhci_irq,
> -	.flags =		HCD_MEMORY | HCD_USB3 | HCD_SHARED,
> +	.msi_irq =		xhci_msi_irq,
> +	.msix_irq =		xhci_msi_irq,
> +	.flags =		HCD_MEMORY | HCD_USB3 | HCD_SHARED |
> +				HCD_MSI_FIRST,
>  
>  	/*
>  	 * basic lifecycle operations
>  	 */
>  	.reset =		xhci_pci_setup,
>  	.start =		xhci_run,
> +	.get_msix_num =		xhci_get_msix_num,
>  #ifdef CONFIG_PM
>  	.pci_suspend =          xhci_pci_suspend,
>  	.pci_resume =           xhci_pci_resume,
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index c939f5f..95dc48f 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -176,210 +176,24 @@ int xhci_reset(struct xhci_hcd *xhci)
>  }
>  
>  #ifdef CONFIG_PCI
> -static int xhci_free_msi(struct xhci_hcd *xhci)
> -{
> -	int i;
> -
> -	if (!xhci->msix_entries)
> -		return -EINVAL;
> -
> -	for (i = 0; i < xhci->msix_count; i++)
> -		if (xhci->msix_entries[i].vector)
> -			free_irq(xhci->msix_entries[i].vector,
> -					xhci_to_hcd(xhci));
> -	return 0;
> -}
> -
> -/*
> - * Set up MSI
> - */
> -static int xhci_setup_msi(struct xhci_hcd *xhci)
> -{
> -	int ret;
> -	struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> -
> -	ret = pci_enable_msi(pdev);
> -	if (ret) {
> -		xhci_dbg(xhci, "failed to allocate MSI entry\n");
> -		return ret;
> -	}
> -
> -	ret = request_irq(pdev->irq, (irq_handler_t)xhci_msi_irq,
> -				0, "xhci_hcd", xhci_to_hcd(xhci));
> -	if (ret) {
> -		xhci_dbg(xhci, "disable MSI interrupt\n");
> -		pci_disable_msi(pdev);
> -	}
> -
> -	return ret;
> -}
> -
> -/*
> - * Free IRQs
> - * free all IRQs request
> - */
> -static void xhci_free_irq(struct xhci_hcd *xhci)
> -{
> -	struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> -	int ret;
> -
> -	/* return if using legacy interrupt */
> -	if (xhci_to_hcd(xhci)->irq >= 0)
> -		return;
> -
> -	ret = xhci_free_msi(xhci);
> -	if (!ret)
> -		return;
> -	if (pdev->irq >= 0)
> -		free_irq(pdev->irq, xhci_to_hcd(xhci));
> -
> -	return;
> -}
> -
> -/*
> - * Set up MSI-X
> - */
> -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(hcd->self.controller);
> -
> -	/*
> -	 * calculate number of msi-x vectors supported.
> -	 * - HCS_MAX_INTRS: the max number of interrupts the host can handle,
> -	 *   with max number of interrupters based on the xhci HCSPARAMS1.
> -	 * - num_online_cpus: maximum msi-x vectors per CPUs core.
> -	 *   Add additional 1 vector to ensure always available interrupt.
> -	 */
> -	xhci->msix_count = min(num_online_cpus() + 1,
> -				HCS_MAX_INTRS(xhci->hcs_params1));
> -
> -	xhci->msix_entries =
> -		kmalloc((sizeof(struct msix_entry))*xhci->msix_count,
> -				GFP_KERNEL);
> -	if (!xhci->msix_entries) {
> -		xhci_err(xhci, "Failed to allocate MSI-X entries\n");
> -		return -ENOMEM;
> -	}
> -
> -	for (i = 0; i < xhci->msix_count; i++) {
> -		xhci->msix_entries[i].entry = i;
> -		xhci->msix_entries[i].vector = 0;
> -	}
> -
> -	ret = pci_enable_msix(pdev, xhci->msix_entries, xhci->msix_count);
> -	if (ret) {
> -		xhci_dbg(xhci, "Failed to enable MSI-X\n");
> -		goto free_entries;
> -	}
> -
> -	for (i = 0; i < xhci->msix_count; i++) {
> -		ret = request_irq(xhci->msix_entries[i].vector,
> -				(irq_handler_t)xhci_msi_irq,
> -				0, "xhci_hcd", xhci_to_hcd(xhci));
> -		if (ret)
> -			goto disable_msix;
> -	}
> -
> -	hcd->msix_enabled = 1;
> -	return ret;
> -
> -disable_msix:
> -	xhci_dbg(xhci, "disable MSI-X interrupt\n");
> -	xhci_free_irq(xhci);
> -	pci_disable_msix(pdev);
> -free_entries:
> -	kfree(xhci->msix_entries);
> -	xhci->msix_entries = NULL;
> -	return ret;
> -}
> -
> -/* Free any IRQs and disable MSI-X */
> -static void xhci_cleanup_msix(struct xhci_hcd *xhci)
> -{
> -	struct usb_hcd *hcd = xhci_to_hcd(xhci);
> -	struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
> -
> -	xhci_free_irq(xhci);
> -
> -	if (xhci->msix_entries) {
> -		pci_disable_msix(pdev);
> -		kfree(xhci->msix_entries);
> -		xhci->msix_entries = NULL;
> -	} else {
> -		pci_disable_msi(pdev);
> -	}
> -
> -	hcd->msix_enabled = 0;
> -	return;
> -}
>  
>  static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
>  {
> -	int i;
> -
> -	if (xhci->msix_entries) {
> -		for (i = 0; i < xhci->msix_count; i++)
> -			synchronize_irq(xhci->msix_entries[i].vector);
> -	}
> +	usb_hcd_msix_sync_irqs(xhci_to_hcd(xhci));
>  }
>  
> -static int xhci_try_enable_msi(struct usb_hcd *hcd)
> +/* called in interrupt setup during pci probe() */
> +int xhci_get_msix_num(struct usb_hcd *hcd)
>  {
>  	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> -	struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> -	int ret;
> -
> -	/*
> -	 * Some Fresco Logic host controllers advertise MSI, but fail to
> -	 * generate interrupts.  Don't even try to enable MSI.
> -	 */
> -	if (xhci->quirks & XHCI_BROKEN_MSI)
> -		return 0;
> -
> -	/* unregister the legacy interrupt */
> -	if (hcd->irq)
> -		free_irq(hcd->irq, hcd);
> -	hcd->irq = -1;
>  
> -	ret = xhci_setup_msix(xhci);
> -	if (ret)
> -		/* fall back to msi*/
> -		ret = xhci_setup_msi(xhci);
> -
> -	if (!ret)
> -		/* hcd->irq is -1, we have MSI */
> -		return 0;
> -
> -	if (!pdev->irq) {
> -		xhci_err(xhci, "No msi-x/msi found and no IRQ in BIOS\n");
> -		return -EINVAL;
> -	}
> -
> -	/* fall back to legacy interrupt*/
> -	ret = request_irq(pdev->irq, &usb_hcd_irq, IRQF_SHARED,
> -			hcd->irq_descr, hcd);
> -	if (ret) {
> -		xhci_err(xhci, "request interrupt %d failed\n",
> -				pdev->irq);
> -		return ret;
> -	}
> -	hcd->irq = pdev->irq;
> -	return 0;
> +	/* danger: xhci may be null, but it's useless in xhci_readl() now */
> +	return HCS_MAX_INTRS(xhci_readl(xhci,
> +			&((struct xhci_cap_regs *)hcd->regs)->hcs_params1));
>  }
>  
>  #else
>  
> -static int xhci_try_enable_msi(struct usb_hcd *hcd)
> -{
> -	return 0;
> -}
> -
> -static void xhci_cleanup_msix(struct xhci_hcd *xhci)
> -{
> -}
> -
>  static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
>  {
>  }
> @@ -411,7 +225,6 @@ int xhci_init(struct usb_hcd *hcd)
>  
>  	return retval;
>  }
> -
>  /*-------------------------------------------------------------------------*/
>  
>  
> @@ -497,7 +310,6 @@ int xhci_run(struct usb_hcd *hcd)
>  {
>  	u32 temp;
>  	u64 temp_64;
> -	int ret;
>  	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>  
>  	/* Start the xHCI host controller running only after the USB 2.0 roothub
> @@ -510,10 +322,6 @@ int xhci_run(struct usb_hcd *hcd)
>  
>  	xhci_dbg(xhci, "xhci_run\n");
>  
> -	ret = xhci_try_enable_msi(hcd);
> -	if (ret)
> -		return ret;
> -
>  #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
>  	init_timer(&xhci->event_ring_timer);
>  	xhci->event_ring_timer.data = (unsigned long) xhci;
> @@ -609,7 +417,7 @@ void xhci_stop(struct usb_hcd *hcd)
>  	xhci_reset(xhci);
>  	spin_unlock_irq(&xhci->lock);
>  
> -	xhci_cleanup_msix(xhci);
> +	usb_hcd_free_msi_msix(hcd);
>  
>  #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
>  	/* Tell the event ring poll function not to reschedule */
> @@ -651,7 +459,7 @@ void xhci_shutdown(struct usb_hcd *hcd)
>  	xhci_halt(xhci);
>  	spin_unlock_irq(&xhci->lock);
>  
> -	xhci_cleanup_msix(xhci);
> +	usb_hcd_free_msi_msix(hcd);
>  
>  	xhci_dbg(xhci, "xhci_shutdown completed - status = %x\n",
>  		    xhci_readl(xhci, &xhci->op_regs->status));
> @@ -853,7 +661,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
>  		xhci_halt(xhci);
>  		xhci_reset(xhci);
>  		spin_unlock_irq(&xhci->lock);
> -		xhci_cleanup_msix(xhci);
> +		usb_hcd_free_msi_msix(hcd);
>  
>  #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
>  		/* Tell the event ring poll function not to reschedule */
> @@ -888,7 +696,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
>  		if (retval)
>  			return retval;
>  		xhci_dbg(xhci, "Start the primary HCD\n");
> -		retval = xhci_run(hcd->primary_hcd);
> +		if (dev_is_pci(hcd->self.controller))
> +			retval = usb_hcd_register_msi_msix_irqs(
> +						hcd->primary_hcd);

Why not change this function to take a count of msix vectors and
pointers for data?  Then you don't need the new usb_hcd driver method
for getting the msix count.

> +		if (!retval)
> +			retval = xhci_run(hcd->primary_hcd);
>  		if (!retval) {
>  			xhci_dbg(xhci, "Start the secondary HCD\n");
>  			retval = xhci_run(secondary_hcd);
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index fb99c83..8d511dd 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1388,9 +1388,6 @@ struct xhci_hcd {
>  	int		page_size;
>  	/* Valid values are 12 to 20, inclusive */
>  	int		page_shift;
> -	/* msi-x vectors */
> -	int		msix_count;
> -	struct msix_entry	*msix_entries;
>  	/* data structures */
>  	struct xhci_device_context_array *dcbaa;
>  	struct xhci_ring	*cmd_ring;
> @@ -1643,9 +1640,11 @@ void xhci_free_command(struct xhci_hcd *xhci,
>  /* xHCI PCI glue */
>  int xhci_register_pci(void);
>  void xhci_unregister_pci(void);
> +int xhci_get_msix_num(struct usb_hcd *hcd);
>  #else
>  static inline int xhci_register_pci(void) { return 0; }
>  static inline void xhci_unregister_pci(void) {}
> +static inline int xhci_get_msix_num(struct usb_hcd *hcd) { return 0; }
>  #endif
>  
>  /* xHCI host controller glue */
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index b2f62f3..5253c02 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -22,6 +22,7 @@
>  #ifdef __KERNEL__
>  
>  #include <linux/rwsem.h>
> +#include <linux/pci.h>		/* for struct msix_entry */
>  
>  #define MAX_TOPO_LEVEL		6
>  
> @@ -133,6 +134,12 @@ struct usb_hcd {
>  	u64			rsrc_len;	/* memory/io resource length */
>  	unsigned		power_budget;	/* in mA, 0 = no limit */
>  
> +#ifdef CONFIG_PCI
> +	/* msi-x vectors */
> +	int			msix_count;
> +	struct msix_entry	*msix_entries;
> +#endif
> +
>  	/* bandwidth_mutex should be taken before adding or removing
>  	 * any new bus bandwidth constraints:
>  	 *   1. Before adding a configuration for a new device.
> @@ -205,11 +212,14 @@ struct hc_driver {
>  
>  	/* irq handler */
>  	irqreturn_t	(*irq) (struct usb_hcd *hcd);
> +	irqreturn_t	(*msi_irq) (int irq, struct usb_hcd *hcd);
> +	irqreturn_t	(*msix_irq) (int irq, struct usb_hcd *hcd);
>  
>  	int	flags;
>  #define	HCD_MEMORY	0x0001		/* HC regs use memory (else I/O) */
>  #define	HCD_LOCAL_MEM	0x0002		/* HC needs local memory */
>  #define	HCD_SHARED	0x0004		/* Two (or more) usb_hcds share HW */
> +#define	HCD_MSI_FIRST	0x0008		/* Try to get MSI first, PCI only */
>  #define	HCD_USB11	0x0010		/* USB 1.1 */
>  #define	HCD_USB2	0x0020		/* USB 2.0 */
>  #define	HCD_USB3	0x0040		/* USB 3.0 */
> @@ -218,6 +228,7 @@ struct hc_driver {
>  	/* called to init HCD and root hub */
>  	int	(*reset) (struct usb_hcd *hcd);
>  	int	(*start) (struct usb_hcd *hcd);
> +	int	(*get_msix_num) (struct usb_hcd *hcd);
>  
>  	/* NOTE:  these suspend/resume calls relate to the HC as
>  	 * a whole, not just the root hub; they're for PCI bus glue.
> @@ -393,6 +404,12 @@ extern int usb_hcd_pci_probe(struct pci_dev *dev,
>  extern void usb_hcd_pci_remove(struct pci_dev *dev);
>  extern void usb_hcd_pci_shutdown(struct pci_dev *dev);
>  
> +extern void usb_hcd_free_msi_msix(struct usb_hcd *hcd);
> +extern void usb_hcd_msix_sync_irqs(struct usb_hcd *hcd);
> +extern int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd,
> +		unsigned int irqnum, unsigned long irqflags);
> +extern int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd);
> +
>  #ifdef CONFIG_PM_SLEEP
>  extern const struct dev_pm_ops usb_hcd_pci_pm_ops;
>  #endif
> -- 
> 1.6.3.3
> 

Re: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core

[Alex,Shi]

> This PCI device and vendor ID is now used in two drivers (xhci and USB
> core).  Please create a separate patch to add this ID to the pci_ids.h
> file, and remove the reference here and in the xHCI driver.

Yes, should be. 
> Why don't you rename hcd_no_msi() to hcd_supports_msi() and remove the
> negation of the return value?

OK. 
> 
> > 
> > +		/* register hc_driver's msix_irq handler */
> > +		for (i = 0; i < hcd->msix_count; i++) {
> > +			retval = request_irq(hcd->msix_entries[i].vector,
> > +					(irq_handler_t)hcd->driver->msix_irq,
> > +					0, hcd->driver->description, hcd);
> 
> I really think you need to allow the host controller driver to set
> different pointers for the msix data pointer.  It's something that we
> need to figure out, so that we can have the infrastructure in place for
> multiple event rings.
> 
> I'm not sure whether the new get MSIX count needs to allow the xHCI
> driver to return an array of pointers, or if the driver can modify the
> irq pointer later?  I don't think you can modify the irq data pointer
> after it's been requested (that would lead to all kinds of race
> conditions, I think).
> 
> It's probably better to allow the xHCI driver to pass this function the
> pointers it needs for each MSI-X vector.  You'll always call
> usb_hcd_request_msi_msix_irqs() after you call xhci_init(), correct?  At
> that point, we should have allocated the multiple event rings, so it
> should be easy to pass the pointers to this function.

What do you mean: there is a relation between event rings
msix_entries.vectors.  and we need to presents this relationships in the
msix interrupt handler? 

So does the following mode you like?

request_irq(hcd->msix_entries[i].vector, msix_irq_handler, 0, "",
hcd->ring_handler[i]);

Or another way to do it if we know which ring will handle the irq, like:

irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd)

	switch (irq2ring(irq))
	case ring0: driver_handle_ring(ring0);
	case ring1: driver_handle_ring(ring1);

In fact, since there is no actual usage of multiple rings now, I have no
much idea of the relationships. 

BTW, if it is possible do this change to another patch? 
> 
> > @@ -888,7 +696,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> >  		if (retval)
> >  			return retval;
> >  		xhci_dbg(xhci, "Start the primary HCD\n");
> > -		retval = xhci_run(hcd->primary_hcd);
> > +		if (dev_is_pci(hcd->self.controller))
> > +			retval = usb_hcd_register_msi_msix_irqs(
> > +						hcd->primary_hcd);
> 
> Why not change this function to take a count of msix vectors and
> pointers for data?  Then you don't need the new usb_hcd driver method
> for getting the msix count.
> 

Uh, the key is msix vector numbers maybe changed after be freed in
suspend and re-get here. Are there examples to keep the vector number in
suspending?

Re: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core

[Alex,Shi]

> What do you mean: there is a relation between event rings
> msix_entries.vectors.  and we need to presents this relationships in the
> msix interrupt handler? 
> 
> So does the following mode you like?
> 
> request_irq(hcd->msix_entries[i].vector, msix_irq_handler, 0, "",
> hcd->ring_handler[i]);
> 
> Or another way to do it if we know which ring will handle the irq, like:

here, I mean let the driver remember this relationships. 
> 
> irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd)
> 
> 	switch (irq2ring(irq))
> 	case ring0: driver_handle_ring(ring0);
> 	case ring1: driver_handle_ring(ring1);
> 
> In fact, since there is no actual usage of multiple rings now, I have no
> much idea of the relationships. 
> 
> BTW, if it is possible do this change to another patch? 

Re: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 27120 bytes --]

Hi,

On Wed, Feb 22, 2012 at 07:41:47PM -0800, Sarah Sharp wrote:
> On Mon, Feb 20, 2012 at 05:05:32PM +0800, Alex Shi wrote:
> > MSI/MSIX is the favorite interrupt for PCIe device. PCIe usb HCD should
> > be used more and more in future, but current USB core still just wants
> > line IRQ, only XHCI usb driver enabled MSI/MSIX.
> > 
> > This patch enabled pci MSI/MSIX in usb core for HCD, and removed MSI/MSIX
> > setup code from XHCI since it becomes redundant now.
> > There 3 places need prepare to enable MSI/MSIX in usb driver.
> > 1, set HCD_MSI_FIRST in driver->flags to enable MSI/MSIX.
> > 2, prepare a get_msix_num() to get msix vector numb.
> > 3, prepare msi/msix_irq handler if needed.
> > 
> > XHCI is a good example for this.
> > 
> > Thanks for Sarah's effort on the MSI/MSIX enabling in XHCI. In fact most
> > of MSI/MSIX setup functions reuse from XHCI.
> 
> Comments below.
> 
> Felipe, can you please review this patch for the effects on non-PCI
> hosts?  I think nothing will change, since this patchset just modifies
> the USB core PCI initialization flow, but I need your eyes on this. :)

Sure thing :-)

> > Signed-off-by: Alex Shi <alex.shi@intel.com>
> > ---
> >  drivers/usb/core/hcd-pci.c  |  223 ++++++++++++++++++++++++++++++++++++++++++-
> >  drivers/usb/core/hcd.c      |   22 ++++-
> >  drivers/usb/host/xhci-pci.c |   16 ++--
> >  drivers/usb/host/xhci.c     |  216 +++---------------------------------------
> >  drivers/usb/host/xhci.h     |    5 +-
> >  include/linux/usb/hcd.h     |   17 ++++
> >  6 files changed, 276 insertions(+), 223 deletions(-)
> > 
> > diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> > index 81e2c0d..8475b25 100644
> > --- a/drivers/usb/core/hcd-pci.c
> > +++ b/drivers/usb/core/hcd-pci.c
> > @@ -153,6 +153,218 @@ static inline void wait_for_companions(struct pci_dev *d, struct usb_hcd *h) {}
> >  
> >  /*-------------------------------------------------------------------------*/
> >  
> > +static int hcd_free_msix(struct usb_hcd *hcd)
> > +{
> > +	int i;
> > +	struct pci_dev  *pdev = to_pci_dev(hcd->self.controller);
> > +
> > +	if (!hcd->msix_entries) {
> > +		printk(KERN_ERR "No msix entries found!\n");
> > +		return -EINVAL;
> > +	}
> > +	for (i = 0; i < hcd->msix_count; i++)
> > +		if (hcd->msix_entries[i].vector)
> > +			free_irq(hcd->msix_entries[i].vector,
> > +					hcd);
> > +
> > +	pci_disable_msix(pdev);
> > +	kfree(hcd->msix_entries);
> > +	hcd->msix_entries = NULL;
> > +	hcd->msix_enabled = 0;
> > +
> > +	return 0;
> > +}
> > +
> > +static int hcd_free_msi(struct usb_hcd *hcd)
> > +{
> > +	struct pci_dev  *pdev = to_pci_dev(hcd->self.controller);
> > +
> > +	if (pdev->irq > 0 && hcd->irq <0)
> > +		free_irq(pdev->irq, hcd);
> > +
> > +	pci_disable_msi(pdev);
> > +	return 0;
> > +}
> > +
> > +/* Free and disable msi/msix */
> > +void usb_hcd_free_msi_msix(struct usb_hcd *hcd)
> > +{
> > +	if (hcd->msix_entries)
> > +		hcd_free_msix(hcd);
> > +	else
> > +		hcd_free_msi(hcd);
> > +
> > +	return;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_hcd_free_msi_msix);
> > +
> > +void usb_hcd_msix_sync_irqs(struct usb_hcd *hcd)
> > +{
> > +	int i;
> > +	if (hcd->msix_entries) {
> > +		for (i = 0; i < hcd->msix_count; i++)
> > +			synchronize_irq(hcd->msix_entries[i].vector);
> > +	}
> > +
> > +}
> > +EXPORT_SYMBOL_GPL(usb_hcd_msix_sync_irqs);
> > +
> > +/*
> > + * Set up MSI
> > + */
> > +static int hcd_setup_msi(struct pci_dev *pdev, struct usb_hcd *hcd)
> > +{
> > +	int ret;
> > +
> > +	ret = pci_enable_msi(pdev);
> > +	if (ret)
> > +		dev_dbg(&pdev->dev, "failed to allocate MSI entry\n");
> > +	return ret;
> > +}
> > +
> > +/*
> > + * Set up MSI-X
> > + */
> > +static int hcd_setup_msix(struct pci_dev *pdev, struct usb_hcd *hcd)
> > +{
> > +	int i, ret = 0;
> > +
> > +	/*
> > +	 * calculate number of msi-x vectors supported.
> > +	 * Add additional 1 vector to ensure always available interrupt.
> > +	 */
> > +	hcd->msix_count = min((int)num_online_cpus() + 1,
> > +				hcd->driver->get_msix_num(hcd));
> > +
> > +	hcd->msix_entries =
> > +		kmalloc((sizeof(struct msix_entry))*hcd->msix_count,
> > +				GFP_KERNEL);
> > +	if (!hcd->msix_entries) {
> > +		dev_err(&pdev->dev, "Failed to allocate MSI-X entries\n");
> > +		hcd->msix_count = 0;
> > +		return -ENOMEM;
> > +	}
> > +
> > +	for (i = 0; i < hcd->msix_count; i++) {
> > +		hcd->msix_entries[i].entry = i;
> > +		hcd->msix_entries[i].vector = 0;
> > +	}
> > +
> > +	ret = pci_enable_msix(pdev, hcd->msix_entries, hcd->msix_count);
> > +	if (ret) {
> > +		dev_dbg(&pdev->dev, "Failed to enable MSI-X\n");
> > +		goto free_entries;
> > +	}
> > +
> > +	return ret;
> > +
> > +free_entries:
> > +	kfree(hcd->msix_entries);
> > +	hcd->msix_entries = NULL;
> > +	hcd->msix_count = 0;
> > +	return ret;
> > +}
> > +
> > +/* Device for a quirk */
> > +#define PCI_VENDOR_ID_FRESCO_LOGIC	0x1b73
> > +#define PCI_DEVICE_ID_FRESCO_LOGIC_PDK	0x1000
> 
> This PCI device and vendor ID is now used in two drivers (xhci and USB
> core).  Please create a separate patch to add this ID to the pci_ids.h
> file, and remove the reference here and in the xHCI driver.

Yeah, makes sense

> > +/* Check for buggy HCD devices, and driver's expectation for MSI.

please use the preferred multi-line comment style.

> > + * Now, only xhci driver want it by HCD_MSI_FIRST setting. Other driver
> > + * like ehci/uhci can follow this setting, if they want.
> > + */
> > +static int hcd_no_msi(struct pci_dev *pdev, struct usb_hcd *hcd)
> > +{
> > +	if (pdev->vendor == PCI_VENDOR_ID_FRESCO_LOGIC &&
> > +			pdev->device == PCI_DEVICE_ID_FRESCO_LOGIC_PDK) {
> > +
> > +		/* Fresco Logic confirms: all revisions of this chip do not
> > +		 * support MSI, even though some of them claim to in their PCI
> > +		 * capabilities.
> > +		 */
> > +		dev_dbg(&pdev->dev, "QUIRK: Fresco Logic revision %u "
> > +				"has broken MSI implementation\n",
> > +				pdev->revision);
> > +		return 1;
> > +	}
> > +	if (!(hcd->driver->flags & HCD_MSI_FIRST))
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> > +/* setup msi/msix interrupts. if fails, fallback to line irq */
> > +static int hcd_setup_int(struct pci_dev *pdev, struct usb_hcd *hcd)
> > +{
> > +	int ret;
> > +
> > +	hcd->irq = -1;
> > +
> > +	if (!hcd_no_msi(pdev, hcd)) {
> 
> Why don't you rename hcd_no_msi() to hcd_supports_msi() and remove the
> negation of the return value?

I agree.

> > +		ret = hcd_setup_msix(pdev, hcd);
> > +		if (ret)
> > +			/* fall back to msi*/
> > +			ret = hcd_setup_msi(pdev, hcd);
> > +
> > +		if (!ret)
> > +			/* hcd->irq is -1, we have MSI */
> > +			return 0;
> > +	}
> > +
> > +	if (!pdev->irq) {
> > +		dev_err(&pdev->dev, "No msi or line irq found!\n");
> > +		return -1;
> > +	}
> > +	/* fallback to line irq */
> > +	hcd->irq = pdev->irq;
> > +	return 0;
> > +}
> > +
> > +int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd, unsigned int irqnum,
> > +					unsigned long irqflags)
> > +{
> > +	int retval = 1;
> > +	if (hcd->msix_entries && hcd->driver->msix_irq) {
> > +		int i;
> > +		/* register hc_driver's msix_irq handler */
> > +		for (i = 0; i < hcd->msix_count; i++) {
> > +			retval = request_irq(hcd->msix_entries[i].vector,
> > +					(irq_handler_t)hcd->driver->msix_irq,

do you really need this cast here ?

> > +					0, hcd->driver->description, hcd);
> 
> I really think you need to allow the host controller driver to set
> different pointers for the msix data pointer.  It's something that we
> need to figure out, so that we can have the infrastructure in place for
> multiple event rings.
> 
> I'm not sure whether the new get MSIX count needs to allow the xHCI
> driver to return an array of pointers, or if the driver can modify the
> irq pointer later?  I don't think you can modify the irq data pointer
> after it's been requested (that would lead to all kinds of race
> conditions, I think).
> 
> It's probably better to allow the xHCI driver to pass this function the
> pointers it needs for each MSI-X vector.  You'll always call
> usb_hcd_request_msi_msix_irqs() after you call xhci_init(), correct?  At
> that point, we should have allocated the multiple event rings, so it
> should be easy to pass the pointers to this function.
> 
> > +			if (retval) {
> > +				hcd_free_msix(hcd);
> > +				break;
> > +			}
> > +		}
> > +		if (i == hcd->msix_count)
> > +			hcd->msix_enabled = 1;
> > +	} else if (hcd->irq == -1 && hcd->driver->msi_irq) {
> > +		/* register hc_driver's msi_irq handler */
> > +		retval = request_irq(irqnum,
> > +					(irq_handler_t)hcd->driver->msi_irq,
> > +					0, hcd->driver->description, hcd);
> > +		if (retval)
> > +			hcd_free_msi(hcd);
> > +	}
> > +
> > +	return retval;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_hcd_request_msi_msix_irqs);
> > +
> > +/* setup msi/msix interrupts and requestion irqs for them */
> > +int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
> > +	int ret = -1;
> > +
> > +	if (!hcd_setup_int(pdev, hcd))
> > +		ret = usb_hcd_request_msi_msix_irqs(hcd,
> > +				pdev->irq, IRQF_SHARED);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_hcd_register_msi_msix_irqs);
> > +
> >  /* configure so an HC device and id are always provided */
> >  /* always called with process context; sleeping is OK */
> >  
> > @@ -187,10 +399,8 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >  		return -ENODEV;
> >  	dev->current_state = PCI_D0;
> >  
> > -	/* The xHCI driver supports MSI and MSI-X,
> > -	 * so don't fail if the BIOS doesn't provide a legacy IRQ.
> > -	 */
> > -	if (!dev->irq && (driver->flags & HCD_MASK) != HCD_USB3) {
> > +	/* skip irq check if hcd wants MSI firstly. */
> > +	if (!(driver->flags & HCD_MSI_FIRST) && !dev->irq) {
> >  		dev_err(&dev->dev,
> >  			"Found HC with no IRQ.  Check BIOS/PCI %s setup!\n",
> >  			pci_name(dev));
> > @@ -245,6 +455,11 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >  
> >  	pci_set_master(dev);
> >  
> > +	/* try enable MSI, if fail, seek line irq */
> > +	retval = hcd_setup_int(dev, hcd);
> > +	if (retval != 0)
> > +		goto unmap_registers;
> > +
> >  	retval = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
> >  	if (retval != 0)
> >  		goto unmap_registers;
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index e128232..579cbd3 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -2322,7 +2322,7 @@ int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
> >  }
> >  EXPORT_SYMBOL_GPL(usb_hcd_is_primary_hcd);
> >  
> > -static int usb_hcd_request_irqs(struct usb_hcd *hcd,
> > +static int usb_hcd_request_default_irqs(struct usb_hcd *hcd,
> >  		unsigned int irqnum, unsigned long irqflags)
> >  {
> >  	int retval;
> > @@ -2362,6 +2362,20 @@ static int usb_hcd_request_irqs(struct usb_hcd *hcd,
> >  	return 0;
> >  }
> >  
> > +int usb_hcd_request_irqs(struct usb_hcd *hcd, unsigned int irqnum,
> > +		unsigned long irqflags)
> > +{
> > +	int retval = 1;
> > +
> > +#ifdef CONFIG_PCI
> > +	retval = usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags);
> > +#endif

I would like it better if the #ifdef is in the function body, something
like:

int usb_hcd_request_msi_msix_irqs(struct hcd *hcd, int irqnum, int irqflags)
{
#ifdef CONFIG_PCI
	/* blablabla */
#else
	return -ENODEV;
#endif
}

> > @@ -2447,10 +2461,8 @@ int usb_add_hcd(struct usb_hcd *hcd,
> >  			&& device_can_wakeup(&hcd->self.root_hub->dev))
> >  		dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
> >  
> > -	/* enable irqs just before we start the controller,
> > -	 * if the BIOS provides legacy PCI irqs.
> > -	 */
> > -	if (usb_hcd_is_primary_hcd(hcd) && irqnum) {
> > +	/* enable irqs just before we start the controller */
> > +	if (usb_hcd_is_primary_hcd(hcd)) {
> >  		retval = usb_hcd_request_irqs(hcd, irqnum, irqflags);
> >  		if (retval)
> >  			goto err_request_irq;
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index ef98b38..39be795 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -64,14 +64,6 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> >  			xhci_dbg(xhci, "QUIRK: Fresco Logic xHC needs configure"
> >  					" endpoint cmd after reset endpoint\n");
> >  		}
> > -		/* Fresco Logic confirms: all revisions of this chip do not
> > -		 * support MSI, even though some of them claim to in their PCI
> > -		 * capabilities.
> > -		 */
> > -		xhci->quirks |= XHCI_BROKEN_MSI;
> > -		xhci_dbg(xhci, "QUIRK: Fresco Logic revision %u "
> > -				"has broken MSI implementation\n",
> > -				pdev->revision);
> >  	}
> >  
> >  	if (pdev->vendor == PCI_VENDOR_ID_NEC)
> > @@ -124,6 +116,7 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
> >  	return retval;
> >  }
> >  
> > +
> 
> Don't add spaces here.
> 
> >  /*
> >   * We need to register our own PCI probe function (instead of the USB core's
> >   * function) in order to create a second roothub under xHCI.
> > @@ -136,6 +129,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >  	struct usb_hcd *hcd;
> >  
> >  	driver = (struct hc_driver *)id->driver_data;
> > +
> 
> Or here.
> 
> >  	/* Register the USB 2.0 roothub.
> >  	 * FIXME: USB core must know to register the USB 2.0 roothub first.
> >  	 * This is sort of silly, because we could just set the HCD driver flags
> > @@ -243,13 +237,17 @@ static const struct hc_driver xhci_pci_hc_driver = {
> >  	 * generic hardware linkage
> >  	 */
> >  	.irq =			xhci_irq,
> > -	.flags =		HCD_MEMORY | HCD_USB3 | HCD_SHARED,
> > +	.msi_irq =		xhci_msi_irq,
> > +	.msix_irq =		xhci_msi_irq,
> > +	.flags =		HCD_MEMORY | HCD_USB3 | HCD_SHARED |
> > +				HCD_MSI_FIRST,
> >  
> >  	/*
> >  	 * basic lifecycle operations
> >  	 */
> >  	.reset =		xhci_pci_setup,
> >  	.start =		xhci_run,
> > +	.get_msix_num =		xhci_get_msix_num,
> >  #ifdef CONFIG_PM
> >  	.pci_suspend =          xhci_pci_suspend,
> >  	.pci_resume =           xhci_pci_resume,
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index c939f5f..95dc48f 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -176,210 +176,24 @@ int xhci_reset(struct xhci_hcd *xhci)
> >  }
> >  
> >  #ifdef CONFIG_PCI
> > -static int xhci_free_msi(struct xhci_hcd *xhci)
> > -{
> > -	int i;
> > -
> > -	if (!xhci->msix_entries)
> > -		return -EINVAL;
> > -
> > -	for (i = 0; i < xhci->msix_count; i++)
> > -		if (xhci->msix_entries[i].vector)
> > -			free_irq(xhci->msix_entries[i].vector,
> > -					xhci_to_hcd(xhci));
> > -	return 0;
> > -}
> > -
> > -/*
> > - * Set up MSI
> > - */
> > -static int xhci_setup_msi(struct xhci_hcd *xhci)
> > -{
> > -	int ret;
> > -	struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> > -
> > -	ret = pci_enable_msi(pdev);
> > -	if (ret) {
> > -		xhci_dbg(xhci, "failed to allocate MSI entry\n");
> > -		return ret;
> > -	}
> > -
> > -	ret = request_irq(pdev->irq, (irq_handler_t)xhci_msi_irq,
> > -				0, "xhci_hcd", xhci_to_hcd(xhci));
> > -	if (ret) {
> > -		xhci_dbg(xhci, "disable MSI interrupt\n");
> > -		pci_disable_msi(pdev);
> > -	}
> > -
> > -	return ret;
> > -}
> > -
> > -/*
> > - * Free IRQs
> > - * free all IRQs request
> > - */
> > -static void xhci_free_irq(struct xhci_hcd *xhci)
> > -{
> > -	struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> > -	int ret;
> > -
> > -	/* return if using legacy interrupt */
> > -	if (xhci_to_hcd(xhci)->irq >= 0)
> > -		return;
> > -
> > -	ret = xhci_free_msi(xhci);
> > -	if (!ret)
> > -		return;
> > -	if (pdev->irq >= 0)
> > -		free_irq(pdev->irq, xhci_to_hcd(xhci));
> > -
> > -	return;
> > -}
> > -
> > -/*
> > - * Set up MSI-X
> > - */
> > -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(hcd->self.controller);
> > -
> > -	/*
> > -	 * calculate number of msi-x vectors supported.
> > -	 * - HCS_MAX_INTRS: the max number of interrupts the host can handle,
> > -	 *   with max number of interrupters based on the xhci HCSPARAMS1.
> > -	 * - num_online_cpus: maximum msi-x vectors per CPUs core.
> > -	 *   Add additional 1 vector to ensure always available interrupt.
> > -	 */
> > -	xhci->msix_count = min(num_online_cpus() + 1,
> > -				HCS_MAX_INTRS(xhci->hcs_params1));
> > -
> > -	xhci->msix_entries =
> > -		kmalloc((sizeof(struct msix_entry))*xhci->msix_count,
> > -				GFP_KERNEL);
> > -	if (!xhci->msix_entries) {
> > -		xhci_err(xhci, "Failed to allocate MSI-X entries\n");
> > -		return -ENOMEM;
> > -	}
> > -
> > -	for (i = 0; i < xhci->msix_count; i++) {
> > -		xhci->msix_entries[i].entry = i;
> > -		xhci->msix_entries[i].vector = 0;
> > -	}
> > -
> > -	ret = pci_enable_msix(pdev, xhci->msix_entries, xhci->msix_count);
> > -	if (ret) {
> > -		xhci_dbg(xhci, "Failed to enable MSI-X\n");
> > -		goto free_entries;
> > -	}
> > -
> > -	for (i = 0; i < xhci->msix_count; i++) {
> > -		ret = request_irq(xhci->msix_entries[i].vector,
> > -				(irq_handler_t)xhci_msi_irq,
> > -				0, "xhci_hcd", xhci_to_hcd(xhci));
> > -		if (ret)
> > -			goto disable_msix;
> > -	}
> > -
> > -	hcd->msix_enabled = 1;
> > -	return ret;
> > -
> > -disable_msix:
> > -	xhci_dbg(xhci, "disable MSI-X interrupt\n");
> > -	xhci_free_irq(xhci);
> > -	pci_disable_msix(pdev);
> > -free_entries:
> > -	kfree(xhci->msix_entries);
> > -	xhci->msix_entries = NULL;
> > -	return ret;
> > -}
> > -
> > -/* Free any IRQs and disable MSI-X */
> > -static void xhci_cleanup_msix(struct xhci_hcd *xhci)
> > -{
> > -	struct usb_hcd *hcd = xhci_to_hcd(xhci);
> > -	struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
> > -
> > -	xhci_free_irq(xhci);
> > -
> > -	if (xhci->msix_entries) {
> > -		pci_disable_msix(pdev);
> > -		kfree(xhci->msix_entries);
> > -		xhci->msix_entries = NULL;
> > -	} else {
> > -		pci_disable_msi(pdev);
> > -	}
> > -
> > -	hcd->msix_enabled = 0;
> > -	return;
> > -}
> >  
> >  static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
> >  {
> > -	int i;
> > -
> > -	if (xhci->msix_entries) {
> > -		for (i = 0; i < xhci->msix_count; i++)
> > -			synchronize_irq(xhci->msix_entries[i].vector);
> > -	}
> > +	usb_hcd_msix_sync_irqs(xhci_to_hcd(xhci));
> >  }
> >  
> > -static int xhci_try_enable_msi(struct usb_hcd *hcd)
> > +/* called in interrupt setup during pci probe() */
> > +int xhci_get_msix_num(struct usb_hcd *hcd)
> >  {
> >  	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > -	struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> > -	int ret;
> > -
> > -	/*
> > -	 * Some Fresco Logic host controllers advertise MSI, but fail to
> > -	 * generate interrupts.  Don't even try to enable MSI.
> > -	 */
> > -	if (xhci->quirks & XHCI_BROKEN_MSI)
> > -		return 0;
> > -
> > -	/* unregister the legacy interrupt */
> > -	if (hcd->irq)
> > -		free_irq(hcd->irq, hcd);
> > -	hcd->irq = -1;
> >  
> > -	ret = xhci_setup_msix(xhci);
> > -	if (ret)
> > -		/* fall back to msi*/
> > -		ret = xhci_setup_msi(xhci);
> > -
> > -	if (!ret)
> > -		/* hcd->irq is -1, we have MSI */
> > -		return 0;
> > -
> > -	if (!pdev->irq) {
> > -		xhci_err(xhci, "No msi-x/msi found and no IRQ in BIOS\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	/* fall back to legacy interrupt*/
> > -	ret = request_irq(pdev->irq, &usb_hcd_irq, IRQF_SHARED,
> > -			hcd->irq_descr, hcd);
> > -	if (ret) {
> > -		xhci_err(xhci, "request interrupt %d failed\n",
> > -				pdev->irq);
> > -		return ret;
> > -	}
> > -	hcd->irq = pdev->irq;
> > -	return 0;
> > +	/* danger: xhci may be null, but it's useless in xhci_readl() now */
> > +	return HCS_MAX_INTRS(xhci_readl(xhci,
> > +			&((struct xhci_cap_regs *)hcd->regs)->hcs_params1));
> >  }
> >  
> >  #else
> >  
> > -static int xhci_try_enable_msi(struct usb_hcd *hcd)
> > -{
> > -	return 0;
> > -}
> > -
> > -static void xhci_cleanup_msix(struct xhci_hcd *xhci)
> > -{
> > -}
> > -
> >  static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
> >  {
> >  }
> > @@ -411,7 +225,6 @@ int xhci_init(struct usb_hcd *hcd)
> >  
> >  	return retval;
> >  }
> > -
> >  /*-------------------------------------------------------------------------*/
> >  
> >  
> > @@ -497,7 +310,6 @@ int xhci_run(struct usb_hcd *hcd)
> >  {
> >  	u32 temp;
> >  	u64 temp_64;
> > -	int ret;
> >  	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> >  
> >  	/* Start the xHCI host controller running only after the USB 2.0 roothub
> > @@ -510,10 +322,6 @@ int xhci_run(struct usb_hcd *hcd)
> >  
> >  	xhci_dbg(xhci, "xhci_run\n");
> >  
> > -	ret = xhci_try_enable_msi(hcd);
> > -	if (ret)
> > -		return ret;
> > -
> >  #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
> >  	init_timer(&xhci->event_ring_timer);
> >  	xhci->event_ring_timer.data = (unsigned long) xhci;
> > @@ -609,7 +417,7 @@ void xhci_stop(struct usb_hcd *hcd)
> >  	xhci_reset(xhci);
> >  	spin_unlock_irq(&xhci->lock);
> >  
> > -	xhci_cleanup_msix(xhci);
> > +	usb_hcd_free_msi_msix(hcd);
> >  
> >  #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
> >  	/* Tell the event ring poll function not to reschedule */
> > @@ -651,7 +459,7 @@ void xhci_shutdown(struct usb_hcd *hcd)
> >  	xhci_halt(xhci);
> >  	spin_unlock_irq(&xhci->lock);
> >  
> > -	xhci_cleanup_msix(xhci);
> > +	usb_hcd_free_msi_msix(hcd);
> >  
> >  	xhci_dbg(xhci, "xhci_shutdown completed - status = %x\n",
> >  		    xhci_readl(xhci, &xhci->op_regs->status));
> > @@ -853,7 +661,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> >  		xhci_halt(xhci);
> >  		xhci_reset(xhci);
> >  		spin_unlock_irq(&xhci->lock);
> > -		xhci_cleanup_msix(xhci);
> > +		usb_hcd_free_msi_msix(hcd);
> >  
> >  #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
> >  		/* Tell the event ring poll function not to reschedule */
> > @@ -888,7 +696,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> >  		if (retval)
> >  			return retval;
> >  		xhci_dbg(xhci, "Start the primary HCD\n");
> > -		retval = xhci_run(hcd->primary_hcd);
> > +		if (dev_is_pci(hcd->self.controller))
> > +			retval = usb_hcd_register_msi_msix_irqs(
> > +						hcd->primary_hcd);
> 
> Why not change this function to take a count of msix vectors and
> pointers for data?  Then you don't need the new usb_hcd driver method
> for getting the msix count.
> 
> > +		if (!retval)
> > +			retval = xhci_run(hcd->primary_hcd);
> >  		if (!retval) {
> >  			xhci_dbg(xhci, "Start the secondary HCD\n");
> >  			retval = xhci_run(secondary_hcd);
> > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> > index fb99c83..8d511dd 100644
> > --- a/drivers/usb/host/xhci.h
> > +++ b/drivers/usb/host/xhci.h
> > @@ -1388,9 +1388,6 @@ struct xhci_hcd {
> >  	int		page_size;
> >  	/* Valid values are 12 to 20, inclusive */
> >  	int		page_shift;
> > -	/* msi-x vectors */
> > -	int		msix_count;
> > -	struct msix_entry	*msix_entries;
> >  	/* data structures */
> >  	struct xhci_device_context_array *dcbaa;
> >  	struct xhci_ring	*cmd_ring;
> > @@ -1643,9 +1640,11 @@ void xhci_free_command(struct xhci_hcd *xhci,
> >  /* xHCI PCI glue */
> >  int xhci_register_pci(void);
> >  void xhci_unregister_pci(void);
> > +int xhci_get_msix_num(struct usb_hcd *hcd);
> >  #else
> >  static inline int xhci_register_pci(void) { return 0; }
> >  static inline void xhci_unregister_pci(void) {}
> > +static inline int xhci_get_msix_num(struct usb_hcd *hcd) { return 0; }
> >  #endif
> >  
> >  /* xHCI host controller glue */
> > diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> > index b2f62f3..5253c02 100644
> > --- a/include/linux/usb/hcd.h
> > +++ b/include/linux/usb/hcd.h
> > @@ -22,6 +22,7 @@
> >  #ifdef __KERNEL__
> >  
> >  #include <linux/rwsem.h>
> > +#include <linux/pci.h>		/* for struct msix_entry */
> >  
> >  #define MAX_TOPO_LEVEL		6
> >  
> > @@ -133,6 +134,12 @@ struct usb_hcd {
> >  	u64			rsrc_len;	/* memory/io resource length */
> >  	unsigned		power_budget;	/* in mA, 0 = no limit */
> >  
> > +#ifdef CONFIG_PCI
> > +	/* msi-x vectors */
> > +	int			msix_count;
> > +	struct msix_entry	*msix_entries;
> > +#endif
> > +
> >  	/* bandwidth_mutex should be taken before adding or removing
> >  	 * any new bus bandwidth constraints:
> >  	 *   1. Before adding a configuration for a new device.
> > @@ -205,11 +212,14 @@ struct hc_driver {
> >  
> >  	/* irq handler */
> >  	irqreturn_t	(*irq) (struct usb_hcd *hcd);
> > +	irqreturn_t	(*msi_irq) (int irq, struct usb_hcd *hcd);
> > +	irqreturn_t	(*msix_irq) (int irq, struct usb_hcd *hcd);
> >  
> >  	int	flags;
> >  #define	HCD_MEMORY	0x0001		/* HC regs use memory (else I/O) */
> >  #define	HCD_LOCAL_MEM	0x0002		/* HC needs local memory */
> >  #define	HCD_SHARED	0x0004		/* Two (or more) usb_hcds share HW */
> > +#define	HCD_MSI_FIRST	0x0008		/* Try to get MSI first, PCI only */
> >  #define	HCD_USB11	0x0010		/* USB 1.1 */
> >  #define	HCD_USB2	0x0020		/* USB 2.0 */
> >  #define	HCD_USB3	0x0040		/* USB 3.0 */
> > @@ -218,6 +228,7 @@ struct hc_driver {
> >  	/* called to init HCD and root hub */
> >  	int	(*reset) (struct usb_hcd *hcd);
> >  	int	(*start) (struct usb_hcd *hcd);
> > +	int	(*get_msix_num) (struct usb_hcd *hcd);
> >  
> >  	/* NOTE:  these suspend/resume calls relate to the HC as
> >  	 * a whole, not just the root hub; they're for PCI bus glue.
> > @@ -393,6 +404,12 @@ extern int usb_hcd_pci_probe(struct pci_dev *dev,
> >  extern void usb_hcd_pci_remove(struct pci_dev *dev);
> >  extern void usb_hcd_pci_shutdown(struct pci_dev *dev);
> >  
> > +extern void usb_hcd_free_msi_msix(struct usb_hcd *hcd);
> > +extern void usb_hcd_msix_sync_irqs(struct usb_hcd *hcd);
> > +extern int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd,
> > +		unsigned int irqnum, unsigned long irqflags);
> > +extern int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd);
> > +
> >  #ifdef CONFIG_PM_SLEEP
> >  extern const struct dev_pm_ops usb_hcd_pci_pm_ops;
> >  #endif
> > -- 
> > 1.6.3.3

Yeah, I don't think this will break anything for my non-PCI stuff, but
the very fact that usbcore knows so much about PCI is quite a bummer. I
think usbcore should know about USB Devices and HCDs, no matter if it's
PCI or platform BUS or whatever else, but that's just me and changing
that would be quite a big re-work.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core

[Alex,Shi]

> > Felipe, can you please review this patch for the effects on non-PCI
> > hosts?  I think nothing will change, since this patchset just modifies
> > the USB core PCI initialization flow, but I need your eyes on this. :)
> 
> Sure thing :-)

Thanks a lot!

> > > +/* Check for buggy HCD devices, and driver's expectation for MSI.
> 
> please use the preferred multi-line comment style.

See and thanks!

> > > +			retval = request_irq(hcd->msix_entries[i].vector,
> > > +					(irq_handler_t)hcd->driver->msix_irq,
> 
> do you really need this cast here ?

Yes, otherwise the complain like here:
drivers/usb/core/hcd-pci.c:330: warning: passing argument 2 of ‘request_irq’ from incompatible pointer type
include/linux/interrupt.h:134: note: expected ‘irq_handler_t’ but argument is of type ‘enum irqreturn_t (* const)(int,  struct usb_hcd *)’

> > > +int usb_hcd_request_irqs(struct usb_hcd *hcd, unsigned int irqnum,
> > > +		unsigned long irqflags)
> > > +{
> > > +	int retval = 1;
> > > +
> > > +#ifdef CONFIG_PCI
> > > +	retval = usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags);
> > > +#endif
> 
> I would like it better if the #ifdef is in the function body, something
> like:
> 
> int usb_hcd_request_msi_msix_irqs(struct hcd *hcd, int irqnum, int irqflags)
> {
> #ifdef CONFIG_PCI
> 	/* blablabla */
> #else
> 	return -ENODEV;
> #endif
> }

The function usb_hcd_request_msi_msix_irqs() is defined in hcd-pci.c. If
you don't like to see '#ifdef CONFIG_PCI' in hcd.c, maybe we can add a
same name function usb_hcd_request_msi_msix_irqs() in hcd.h for non-pci
environment.  Is that ok of the following? 
------
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 579cbd3..9bc6568 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2365,12 +2365,9 @@ static int usb_hcd_request_default_irqs(struct usb_hcd *hcd,
 int usb_hcd_request_irqs(struct usb_hcd *hcd, unsigned int irqnum,
                unsigned long irqflags)
 {
-       int retval = 1;
+       int retval = 0;
 
-#ifdef CONFIG_PCI
-       retval = usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags);
-#endif
-       if (retval)
+       if (usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags))
                retval = usb_hcd_request_default_irqs(hcd, irqnum, irqflags);
 
        return retval;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 5253c02..6743ed8 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -413,6 +413,13 @@ extern int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd);
 #ifdef CONFIG_PM_SLEEP
 extern const struct dev_pm_ops usb_hcd_pci_pm_ops;
 #endif
+
+#else
+extern int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd,
+               unsigned int irqnum, unsigned long irqflags)
+{
+       return -ENODEV;
+}
 #endif /* CONFIG_PCI */
 
 /* pci-ish (pdev null is ok) buffer alloc/mapping support */

> Yeah, I don't think this will break anything for my non-PCI stuff, but
> the very fact that usbcore knows so much about PCI is quite a bummer. I
> think usbcore should know about USB Devices and HCDs, no matter if it's
> PCI or platform BUS or whatever else, but that's just me and changing
> that would be quite a big re-work.
> 

Thanks again for all comments!

Re: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3844 bytes --]

On Fri, Feb 24, 2012 at 09:47:08AM +0800, Alex,Shi wrote:
> 
> > > Felipe, can you please review this patch for the effects on non-PCI
> > > hosts?  I think nothing will change, since this patchset just modifies
> > > the USB core PCI initialization flow, but I need your eyes on this. :)
> > 
> > Sure thing :-)
> 
> Thanks a lot!
> 
> > > > +/* Check for buggy HCD devices, and driver's expectation for MSI.
> > 
> > please use the preferred multi-line comment style.
> 
> See and thanks!
> 
> > > > +			retval = request_irq(hcd->msix_entries[i].vector,
> > > > +					(irq_handler_t)hcd->driver->msix_irq,
> > 
> > do you really need this cast here ?
> 
> Yes, otherwise the complain like here:
> drivers/usb/core/hcd-pci.c:330: warning: passing argument 2 of ‘request_irq’ from incompatible pointer type
> include/linux/interrupt.h:134: note: expected ‘irq_handler_t’ but argument is of type ‘enum irqreturn_t (* const)(int,  struct usb_hcd *)’

Alan, Sarah, is the definition of the IRQ handler wrong on the hc_driver
structure ?

Alex, I think you should fix your definition for the msix_irq handler.

> > > > +int usb_hcd_request_irqs(struct usb_hcd *hcd, unsigned int irqnum,
> > > > +		unsigned long irqflags)
> > > > +{
> > > > +	int retval = 1;
> > > > +
> > > > +#ifdef CONFIG_PCI
> > > > +	retval = usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags);
> > > > +#endif
> > 
> > I would like it better if the #ifdef is in the function body, something
> > like:
> > 
> > int usb_hcd_request_msi_msix_irqs(struct hcd *hcd, int irqnum, int irqflags)
> > {
> > #ifdef CONFIG_PCI
> > 	/* blablabla */
> > #else
> > 	return -ENODEV;
> > #endif
> > }
> 
> The function usb_hcd_request_msi_msix_irqs() is defined in hcd-pci.c. If
> you don't like to see '#ifdef CONFIG_PCI' in hcd.c, maybe we can add a
> same name function usb_hcd_request_msi_msix_irqs() in hcd.h for non-pci
> environment.  Is that ok of the following? 
> ------
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 579cbd3..9bc6568 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2365,12 +2365,9 @@ static int usb_hcd_request_default_irqs(struct usb_hcd *hcd,
>  int usb_hcd_request_irqs(struct usb_hcd *hcd, unsigned int irqnum,
>                 unsigned long irqflags)
>  {
> -       int retval = 1;
> +       int retval = 0;
>  
> -#ifdef CONFIG_PCI
> -       retval = usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags);
> -#endif
> -       if (retval)
> +       if (usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags))
>                 retval = usb_hcd_request_default_irqs(hcd, irqnum, irqflags);
>  
>         return retval;
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 5253c02..6743ed8 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -413,6 +413,13 @@ extern int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd);
>  #ifdef CONFIG_PM_SLEEP
>  extern const struct dev_pm_ops usb_hcd_pci_pm_ops;
>  #endif
> +
> +#else
> +extern int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd,
> +               unsigned int irqnum, unsigned long irqflags)
> +{
> +       return -ENODEV;
> +}
>  #endif /* CONFIG_PCI */

sure, makes sense, but that should be static inline I guess.

>  /* pci-ish (pdev null is ok) buffer alloc/mapping support */
> 
> > Yeah, I don't think this will break anything for my non-PCI stuff, but
> > the very fact that usbcore knows so much about PCI is quite a bummer. I
> > think usbcore should know about USB Devices and HCDs, no matter if it's
> > PCI or platform BUS or whatever else, but that's just me and changing
> > that would be quite a big re-work.
> > 
> 
> Thanks again for all comments!

np.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core

[Alan Stern]

On Fri, 24 Feb 2012, Felipe Balbi wrote:

> > > > > +			retval = request_irq(hcd->msix_entries[i].vector,
> > > > > +					(irq_handler_t)hcd->driver->msix_irq,
> > > 
> > > do you really need this cast here ?
> > 
> > Yes, otherwise the complain like here:
> > drivers/usb/core/hcd-pci.c:330: warning: passing argument 2 of ‘request_irq’ from incompatible pointer type
> > include/linux/interrupt.h:134: note: expected ‘irq_handler_t’ but argument is of type ‘enum irqreturn_t (* const)(int,  struct usb_hcd *)’
> 
> Alan, Sarah, is the definition of the IRQ handler wrong on the hc_driver
> structure ?

No.  It is never passed to request_irq().

> Alex, I think you should fix your definition for the msix_irq handler.

The second parameter in the prototype is supposed to be void *, not 
struct usb_hcd *.

Alan Stern

Re: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core

[Alex Shi]

>

> sure, makes sense, but that should be static inline I guess.
> 


Yes, thanks for reminder!

Re: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core

[Alex,Shi]

On Fri, 2012-02-24 at 10:59 -0500, Alan Stern wrote:
> On Fri, 24 Feb 2012, Felipe Balbi wrote:
> 
> > > > > > +			retval = request_irq(hcd->msix_entries[i].vector,
> > > > > > +					(irq_handler_t)hcd->driver->msix_irq,
> > > > 
> > > > do you really need this cast here ?
> > > 
> > > Yes, otherwise the complain like here:
> > > drivers/usb/core/hcd-pci.c:330: warning: passing argument 2 of ‘request_irq’ from incompatible pointer type
> > > include/linux/interrupt.h:134: note: expected ‘irq_handler_t’ but argument is of type ‘enum irqreturn_t (* const)(int,  struct usb_hcd *)’
> > 
> > Alan, Sarah, is the definition of the IRQ handler wrong on the hc_driver
> > structure ?
> 
> No.  It is never passed to request_irq().
> 
> > Alex, I think you should fix your definition for the msix_irq handler.
> 
> The second parameter in the prototype is supposed to be void *, not 
> struct usb_hcd *.

Yes, Thanks you all for reviewing! 

Is the following change OK?
=======
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 3606afe..a198f4d 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -265,7 +265,8 @@ free_entries:
 	return ret;
 }
 
-/* Check for buggy HCD devices, and driver's expectation for MSI.
+/*
+ * Check for buggy HCD devices, and driver's expectation for MSI.
  * Now, only xhci driver want it by HCD_MSI_FIRST setting. Other driver
  * like ehci/uhci can follow this setting, if they want.
  */
@@ -326,8 +327,8 @@ int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd, unsigned int irqnum,
 		/* register hc_driver's msix_irq handler */
 		for (i = 0; i < hcd->msix_count; i++) {
 			retval = request_irq(hcd->msix_entries[i].vector,
-					(irq_handler_t)hcd->driver->msix_irq,
-					0, hcd->driver->description, hcd);
+					hcd->driver->msix_irq, 0,
+					hcd->driver->description, hcd);
 			if (retval) {
 				hcd_free_msix(hcd);
 				break;
@@ -337,8 +338,7 @@ int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd, unsigned int irqnum,
 			hcd->msix_enabled = 1;
 	} else if (hcd->irq == -1 && hcd->driver->msi_irq) {
 		/* register hc_driver's msi_irq handler */
-		retval = request_irq(irqnum,
-					(irq_handler_t)hcd->driver->msi_irq,
+		retval = request_irq(irqnum, hcd->driver->msi_irq,
 					0, hcd->driver->description, hcd);
 		if (retval)
 			hcd_free_msi(hcd);
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 579cbd3..9bc6568 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2365,12 +2365,9 @@ static int usb_hcd_request_default_irqs(struct usb_hcd *hcd,
 int usb_hcd_request_irqs(struct usb_hcd *hcd, unsigned int irqnum,
 		unsigned long irqflags)
 {
-	int retval = 1;
+	int retval = 0;
 
-#ifdef CONFIG_PCI
-	retval = usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags);
-#endif
-	if (retval)
+	if (usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags))
 		retval = usb_hcd_request_default_irqs(hcd, irqnum, irqflags);
 
 	return retval;
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index b62037b..c223158 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2396,9 +2396,10 @@ hw_died:
 	return IRQ_HANDLED;
 }
 
-irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd)
+irqreturn_t xhci_msi_irq(int irq, void *hcd)
 {
-	return xhci_irq(hcd);
+	struct usb_hcd *xhci_hcd = hcd;
+	return xhci_irq(xhci_hcd);
 }
 
 /****		Endpoint Ring Operations	****/
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 8d511dd..6186d12 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1668,7 +1668,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated);
 
 int xhci_get_frame(struct usb_hcd *hcd);
 irqreturn_t xhci_irq(struct usb_hcd *hcd);
-irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd);
+irqreturn_t xhci_msi_irq(int irq, void *hcd);
 int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev);
 void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev);
 int xhci_alloc_tt_info(struct xhci_hcd *xhci,
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 5253c02..2f94c02 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -212,8 +212,8 @@ struct hc_driver {
 
 	/* irq handler */
 	irqreturn_t	(*irq) (struct usb_hcd *hcd);
-	irqreturn_t	(*msi_irq) (int irq, struct usb_hcd *hcd);
-	irqreturn_t	(*msix_irq) (int irq, struct usb_hcd *hcd);
+	irqreturn_t	(*msi_irq) (int irq, void *hcd);
+	irqreturn_t	(*msix_irq) (int irq, void *hcd);
 
 	int	flags;
 #define	HCD_MEMORY	0x0001		/* HC regs use memory (else I/O) */
@@ -413,6 +413,13 @@ extern int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd);
 #ifdef CONFIG_PM_SLEEP
 extern const struct dev_pm_ops usb_hcd_pci_pm_ops;
 #endif
+
+#else
+static inline int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd,
+		unsigned int irqnum, unsigned long irqflags)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_PCI */
 
 /* pci-ish (pdev null is ok) buffer alloc/mapping support */

Re: [PATCH 1/3] USB: Try MSI first before line IRQ for Intel PCIe USB3 HCD

[Tom Goetz]

Alex Shi <alex.shi <at> intel.com> writes:

> > From: Sarah Sharp <sarah.a.sharp <at> linux.intel.com> > > We have a
PCI USB xhci host controller on a new platform. It doesn't > have a line
IRQ definition in BIOS. The Linux driver refuses to > initial this
controller, but Windows works well because it only depends > on MSI. >

We're having problems with XHCI on 3.2.9 on a new HP Ivybridge laptop.
Some of the evidence suggests that it is MSI related. After suspending
and resuming the platform the XHCI device is not functional.

also submitted here:
http://lists.xen.org/archives/html/xen-devel/2012-03/msg00363.html

Data dump:

- Xen 4.0.3, Linux 3.2.7 PVOPs, Linux 3.2.9 PVOPs
- Happens on HP Ivybridge. Doesn't happen on very similar HP Sandybridge
Clash system.
- Happens on battery, but not on AC.
- Doesn't happen on first suspend/resume. First resume may be long.
Don't have enough samples to be sure of this.
- CPU power governor doesn't effect the issue. Happens regardless of governor.
- Seems to effect MSI devices only.
- IRQ changes appear in good resume case. Not just bad.
- XHCI driver crashes during suspend with pci=nomsi.
- Remove USB HCD drivers from SUSPEND_MODULES and XHCI functionality
survives suspend/resume.
- Is not removing XHCI enough? Guess is that it is.
- Unloading/loading the XHCI driver after a broken resume also fixes it.
- Received a bad IRQ warning in atleast one resume.

I've diffed lspci output across a working resume on AC and across a bad resume
on battery. There seem to be the differences unique to the bad case:

 00:14.0 USB controller: Intel Corporation Panther Point USB xHCI Host
 Controller (rev 04) (prog-if 30 [XHCI])
 Subsystem: Hewlett-Packard Company Device 179b
-Flags: bus master, medium devsel, latency 0, IRQ 326
+Flags: medium devsel, IRQ 327       <-------------- lose bus master, latency 0
 Memory at 94720000 (64-bit, non-prefetchable) [size=64K]
 Capabilities: [70] Power Management version 2
 Capabilities: [80] MSI: Enable+ Count=1/8 Maskable- 64bit+
 Kernel driver in use: xhci_hcd
 Kernel modules: xhci-hcd

--- 

 00:19.0 Ethernet controller: Intel Corporation 82579LM Gigabit Network
 Connection (rev 04)
 Subsystem: Hewlett-Packard Company Device 179b
-Flags: bus master, fast devsel, latency 0, IRQ 327
-Memory at 94700000 (32-bit, non-prefetchable) [size=128K]
-Memory at 9473a000 (32-bit, non-prefetchable) [size=4K]
-I/O ports at 4060 [size=32]
+Flags: fast devsel, IRQ 20    <-------------- lose bus master, latency 0
+Memory at 94700000 (32-bit, non-prefetchable) [disabled] [size=128K]
	<------ becomes disabled
+Memory at 9473a000 (32-bit, non-prefetchable) [disabled] [size=4K]
	<------ becomes disabled
+I/O ports at 4060 [disabled] [size=32]     <------ becomes disabled
 Capabilities: [c8] Power Management version 2
-Capabilities: [d0] MSI: Enable+ Count=1/1 Maskable- 64bit+
+Capabilities: [d0] MSI: Enable- Count=1/1 Maskable- 64bit+
	<---- toggle enable polarity
 Capabilities: [e0] PCI Advanced Features
 Kernel driver in use: e1000e
 Kernel modules: e1000e

---

 00:1a.0 USB controller: Intel Corporation Panther Point USB Enhanced Host
 Controller #2 (rev 04) (prog-if 20 [EHCI])
 Subsystem: Hewlett-Packard Company Device 179b
-Flags: bus master, medium devsel, latency 0, IRQ 16
+Flags: medium devsel, IRQ 16  <-------------- lose bus master, latency 0
 Memory at 94739000 (32-bit, non-prefetchable) [size=1K]
 Capabilities: [50] Power Management version 2
 Capabilities: [58] Debug port: BAR=1 offset=00a0
 Capabilities: [98] PCI Advanced Features
 Kernel driver in use: ehci_hcd
 Kernel modules: ehci-hcd

syslog looks like this for both good and bad resumes:

[  289.730303] xhci_hcd 0000:00:14.0: can't derive routing for PCI INT A
[  289.730311] xhci_hcd 0000:00:14.0: PCI INT A: no GSI - using ISA IRQ 10
[  289.730319] xen: registering gsi 10 triggering 0 polarity 1
[  289.730327] xen_map_pirq_gsi: returning irq 10 for gsi 10
[  289.730332] xen: --> pirq=10 -> irq=10 (gsi=10)
[  289.730337] Already setup the GSI :10
[  289.730445] xhci_hcd 0000:00:14.0: setting latency timer to 64
[  289.730455] xhci_hcd 0000:00:14.0: xHCI Host Controller
[  289.730687] xhci_hcd 0000:00:14.0: new USB bus registered, assigned bus
number 3
[  289.730826] xhci_hcd 0000:00:14.0: cache line size of 64 is not supported
[  289.730858] xhci_hcd 0000:00:14.0: irq 10, io mem 0x94720000
[  289.731470] xHCI xhci_add_endpoint called for root hub
[  289.731477] xHCI xhci_check_bandwidth called for root hub
[  289.731551] hub 3-0:1.0: USB hub found
[  289.731568] hub 3-0:1.0: 4 ports detected
[  289.843501] xhci_hcd 0000:00:14.0: xHCI Host Controller
[  289.843722] xhci_hcd 0000:00:14.0: new USB bus registered, assigned bus
number 4
[  289.844015] xHCI xhci_add_endpoint called for root hub
[  289.844021] xHCI xhci_check_bandwidth called for root hub
[  289.844084] hub 4-0:1.0: USB hub found
[  289.844104] hub 4-0:1.0: 4 ports detected

[PATCH 2/3] usb: enable pci MSI/MSIX in usb core

[Alex Shi]

MSI/MSIX is the favorite interrupt for PCIe device. PCIe usb HCD should
be used more and more in future, but current USB core still just wants
line IRQ, only XHCI usb driver enabled MSI/MSIX.

This patch enabled pci MSI/MSIX in usb core for HCD, and removed MSI/MSIX
setup code from XHCI since it becomes redundant now.
There 3 places need prepare to enable MSI/MSIX in usb driver.
1, set HCD_MSI_FIRST in driver->flags to enable MSI/MSIX.
2, prepare a get_msix_num() to get msix vector numb.
3, prepare msi/msix_irq handler if needed.

XHCI is a good example for this.

Thanks for Sarah's effort on the MSI/MSIX enabling in XHCI. In fact most
of MSI/MSIX setup functions reuse from XHCI.

Signed-off-by: Alex Shi <alex.shi@intel.com>
---
 drivers/usb/core/hcd-pci.c  |  223 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/core/hcd.c      |   16 +++-
 drivers/usb/host/xhci-pci.c |   16 ++--
 drivers/usb/host/xhci.c     |  216 +++---------------------------------------
 drivers/usb/host/xhci.h     |    5 +-
 include/linux/usb/hcd.h     |   17 ++++
 6 files changed, 274 insertions(+), 219 deletions(-)

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 81e2c0d..8475b25 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -153,6 +153,218 @@ static inline void wait_for_companions(struct pci_dev *d, struct usb_hcd *h) {}
 
 /*-------------------------------------------------------------------------*/
 
+static int hcd_free_msix(struct usb_hcd *hcd)
+{
+	int i;
+	struct pci_dev  *pdev = to_pci_dev(hcd->self.controller);
+
+	if (!hcd->msix_entries) {
+		printk(KERN_ERR "No msix entries found!\n");
+		return -EINVAL;
+	}
+	for (i = 0; i < hcd->msix_count; i++)
+		if (hcd->msix_entries[i].vector)
+			free_irq(hcd->msix_entries[i].vector,
+					hcd);
+
+	pci_disable_msix(pdev);
+	kfree(hcd->msix_entries);
+	hcd->msix_entries = NULL;
+	hcd->msix_enabled = 0;
+
+	return 0;
+}
+
+static int hcd_free_msi(struct usb_hcd *hcd)
+{
+	struct pci_dev  *pdev = to_pci_dev(hcd->self.controller);
+
+	if (pdev->irq > 0 && hcd->irq <0)
+		free_irq(pdev->irq, hcd);
+
+	pci_disable_msi(pdev);
+	return 0;
+}
+
+/* Free and disable msi/msix */
+void usb_hcd_free_msi_msix(struct usb_hcd *hcd)
+{
+	if (hcd->msix_entries)
+		hcd_free_msix(hcd);
+	else
+		hcd_free_msi(hcd);
+
+	return;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_free_msi_msix);
+
+void usb_hcd_msix_sync_irqs(struct usb_hcd *hcd)
+{
+	int i;
+	if (hcd->msix_entries) {
+		for (i = 0; i < hcd->msix_count; i++)
+			synchronize_irq(hcd->msix_entries[i].vector);
+	}
+
+}
+EXPORT_SYMBOL_GPL(usb_hcd_msix_sync_irqs);
+
+/*
+ * Set up MSI
+ */
+static int hcd_setup_msi(struct pci_dev *pdev, struct usb_hcd *hcd)
+{
+	int ret;
+
+	ret = pci_enable_msi(pdev);
+	if (ret)
+		dev_dbg(&pdev->dev, "failed to allocate MSI entry\n");
+	return ret;
+}
+
+/*
+ * Set up MSI-X
+ */
+static int hcd_setup_msix(struct pci_dev *pdev, struct usb_hcd *hcd)
+{
+	int i, ret = 0;
+
+	/*
+	 * calculate number of msi-x vectors supported.
+	 * Add additional 1 vector to ensure always available interrupt.
+	 */
+	hcd->msix_count = min((int)num_online_cpus() + 1,
+				hcd->driver->get_msix_num(hcd));
+
+	hcd->msix_entries =
+		kmalloc((sizeof(struct msix_entry))*hcd->msix_count,
+				GFP_KERNEL);
+	if (!hcd->msix_entries) {
+		dev_err(&pdev->dev, "Failed to allocate MSI-X entries\n");
+		hcd->msix_count = 0;
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < hcd->msix_count; i++) {
+		hcd->msix_entries[i].entry = i;
+		hcd->msix_entries[i].vector = 0;
+	}
+
+	ret = pci_enable_msix(pdev, hcd->msix_entries, hcd->msix_count);
+	if (ret) {
+		dev_dbg(&pdev->dev, "Failed to enable MSI-X\n");
+		goto free_entries;
+	}
+
+	return ret;
+
+free_entries:
+	kfree(hcd->msix_entries);
+	hcd->msix_entries = NULL;
+	hcd->msix_count = 0;
+	return ret;
+}
+
+/* Device for a quirk */
+#define PCI_VENDOR_ID_FRESCO_LOGIC	0x1b73
+#define PCI_DEVICE_ID_FRESCO_LOGIC_PDK	0x1000
+
+/* Check for buggy HCD devices, and driver's expectation for MSI.
+ * Now, only xhci driver want it by HCD_MSI_FIRST setting. Other driver
+ * like ehci/uhci can follow this setting, if they want.
+ */
+static int hcd_no_msi(struct pci_dev *pdev, struct usb_hcd *hcd)
+{
+	if (pdev->vendor == PCI_VENDOR_ID_FRESCO_LOGIC &&
+			pdev->device == PCI_DEVICE_ID_FRESCO_LOGIC_PDK) {
+
+		/* Fresco Logic confirms: all revisions of this chip do not
+		 * support MSI, even though some of them claim to in their PCI
+		 * capabilities.
+		 */
+		dev_dbg(&pdev->dev, "QUIRK: Fresco Logic revision %u "
+				"has broken MSI implementation\n",
+				pdev->revision);
+		return 1;
+	}
+	if (!(hcd->driver->flags & HCD_MSI_FIRST))
+		return 1;
+
+	return 0;
+}
+
+/* setup msi/msix interrupts. if fails, fallback to line irq */
+static int hcd_setup_int(struct pci_dev *pdev, struct usb_hcd *hcd)
+{
+	int ret;
+
+	hcd->irq = -1;
+
+	if (!hcd_no_msi(pdev, hcd)) {
+
+		ret = hcd_setup_msix(pdev, hcd);
+		if (ret)
+			/* fall back to msi*/
+			ret = hcd_setup_msi(pdev, hcd);
+
+		if (!ret)
+			/* hcd->irq is -1, we have MSI */
+			return 0;
+	}
+
+	if (!pdev->irq) {
+		dev_err(&pdev->dev, "No msi or line irq found!\n");
+		return -1;
+	}
+	/* fallback to line irq */
+	hcd->irq = pdev->irq;
+	return 0;
+}
+
+int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd, unsigned int irqnum,
+					unsigned long irqflags)
+{
+	int retval = 1;
+	if (hcd->msix_entries && hcd->driver->msix_irq) {
+		int i;
+		/* register hc_driver's msix_irq handler */
+		for (i = 0; i < hcd->msix_count; i++) {
+			retval = request_irq(hcd->msix_entries[i].vector,
+					(irq_handler_t)hcd->driver->msix_irq,
+					0, hcd->driver->description, hcd);
+			if (retval) {
+				hcd_free_msix(hcd);
+				break;
+			}
+		}
+		if (i == hcd->msix_count)
+			hcd->msix_enabled = 1;
+	} else if (hcd->irq == -1 && hcd->driver->msi_irq) {
+		/* register hc_driver's msi_irq handler */
+		retval = request_irq(irqnum,
+					(irq_handler_t)hcd->driver->msi_irq,
+					0, hcd->driver->description, hcd);
+		if (retval)
+			hcd_free_msi(hcd);
+	}
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_request_msi_msix_irqs);
+
+/* setup msi/msix interrupts and requestion irqs for them */
+int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd)
+{
+	struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
+	int ret = -1;
+
+	if (!hcd_setup_int(pdev, hcd))
+		ret = usb_hcd_request_msi_msix_irqs(hcd,
+				pdev->irq, IRQF_SHARED);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_register_msi_msix_irqs);
+
 /* configure so an HC device and id are always provided */
 /* always called with process context; sleeping is OK */
 
@@ -187,10 +399,8 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		return -ENODEV;
 	dev->current_state = PCI_D0;
 
-	/* The xHCI driver supports MSI and MSI-X,
-	 * so don't fail if the BIOS doesn't provide a legacy IRQ.
-	 */
-	if (!dev->irq && (driver->flags & HCD_MASK) != HCD_USB3) {
+	/* skip irq check if hcd wants MSI firstly. */
+	if (!(driver->flags & HCD_MSI_FIRST) && !dev->irq) {
 		dev_err(&dev->dev,
 			"Found HC with no IRQ.  Check BIOS/PCI %s setup!\n",
 			pci_name(dev));
@@ -245,6 +455,11 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	pci_set_master(dev);
 
+	/* try enable MSI, if fail, seek line irq */
+	retval = hcd_setup_int(dev, hcd);
+	if (retval != 0)
+		goto unmap_registers;
+
 	retval = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
 	if (retval != 0)
 		goto unmap_registers;
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index e128232..32ddad0 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2322,7 +2322,7 @@ int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
 }
 EXPORT_SYMBOL_GPL(usb_hcd_is_primary_hcd);
 
-static int usb_hcd_request_irqs(struct usb_hcd *hcd,
+static int usb_hcd_request_default_irqs(struct usb_hcd *hcd,
 		unsigned int irqnum, unsigned long irqflags)
 {
 	int retval;
@@ -2362,6 +2362,20 @@ static int usb_hcd_request_irqs(struct usb_hcd *hcd,
 	return 0;
 }
 
+int usb_hcd_request_irqs(struct usb_hcd *hcd, unsigned int irqnum,
+		unsigned long irqflags)
+{
+	int retval = 1;
+
+#ifdef CONFIG_PCI
+	retval = usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags);
+#endif
+	if (retval)
+		retval = usb_hcd_request_default_irqs(hcd, irqnum, irqflags);
+
+	return retval;
+}
+
 /**
  * usb_add_hcd - finish generic HCD structure initialization and register
  * @hcd: the usb_hcd structure to initialize
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index ef98b38..39be795 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -64,14 +64,6 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 			xhci_dbg(xhci, "QUIRK: Fresco Logic xHC needs configure"
 					" endpoint cmd after reset endpoint\n");
 		}
-		/* Fresco Logic confirms: all revisions of this chip do not
-		 * support MSI, even though some of them claim to in their PCI
-		 * capabilities.
-		 */
-		xhci->quirks |= XHCI_BROKEN_MSI;
-		xhci_dbg(xhci, "QUIRK: Fresco Logic revision %u "
-				"has broken MSI implementation\n",
-				pdev->revision);
 	}
 
 	if (pdev->vendor == PCI_VENDOR_ID_NEC)
@@ -124,6 +116,7 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
 	return retval;
 }
 
+
 /*
  * We need to register our own PCI probe function (instead of the USB core's
  * function) in order to create a second roothub under xHCI.
@@ -136,6 +129,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	struct usb_hcd *hcd;
 
 	driver = (struct hc_driver *)id->driver_data;
+
 	/* Register the USB 2.0 roothub.
 	 * FIXME: USB core must know to register the USB 2.0 roothub first.
 	 * This is sort of silly, because we could just set the HCD driver flags
@@ -243,13 +237,17 @@ static const struct hc_driver xhci_pci_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq =			xhci_irq,
-	.flags =		HCD_MEMORY | HCD_USB3 | HCD_SHARED,
+	.msi_irq =		xhci_msi_irq,
+	.msix_irq =		xhci_msi_irq,
+	.flags =		HCD_MEMORY | HCD_USB3 | HCD_SHARED |
+				HCD_MSI_FIRST,
 
 	/*
 	 * basic lifecycle operations
 	 */
 	.reset =		xhci_pci_setup,
 	.start =		xhci_run,
+	.get_msix_num =		xhci_get_msix_num,
 #ifdef CONFIG_PM
 	.pci_suspend =          xhci_pci_suspend,
 	.pci_resume =           xhci_pci_resume,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index c939f5f..95dc48f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -176,210 +176,24 @@ int xhci_reset(struct xhci_hcd *xhci)
 }
 
 #ifdef CONFIG_PCI
-static int xhci_free_msi(struct xhci_hcd *xhci)
-{
-	int i;
-
-	if (!xhci->msix_entries)
-		return -EINVAL;
-
-	for (i = 0; i < xhci->msix_count; i++)
-		if (xhci->msix_entries[i].vector)
-			free_irq(xhci->msix_entries[i].vector,
-					xhci_to_hcd(xhci));
-	return 0;
-}
-
-/*
- * Set up MSI
- */
-static int xhci_setup_msi(struct xhci_hcd *xhci)
-{
-	int ret;
-	struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
-
-	ret = pci_enable_msi(pdev);
-	if (ret) {
-		xhci_dbg(xhci, "failed to allocate MSI entry\n");
-		return ret;
-	}
-
-	ret = request_irq(pdev->irq, (irq_handler_t)xhci_msi_irq,
-				0, "xhci_hcd", xhci_to_hcd(xhci));
-	if (ret) {
-		xhci_dbg(xhci, "disable MSI interrupt\n");
-		pci_disable_msi(pdev);
-	}
-
-	return ret;
-}
-
-/*
- * Free IRQs
- * free all IRQs request
- */
-static void xhci_free_irq(struct xhci_hcd *xhci)
-{
-	struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
-	int ret;
-
-	/* return if using legacy interrupt */
-	if (xhci_to_hcd(xhci)->irq >= 0)
-		return;
-
-	ret = xhci_free_msi(xhci);
-	if (!ret)
-		return;
-	if (pdev->irq >= 0)
-		free_irq(pdev->irq, xhci_to_hcd(xhci));
-
-	return;
-}
-
-/*
- * Set up MSI-X
- */
-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(hcd->self.controller);
-
-	/*
-	 * calculate number of msi-x vectors supported.
-	 * - HCS_MAX_INTRS: the max number of interrupts the host can handle,
-	 *   with max number of interrupters based on the xhci HCSPARAMS1.
-	 * - num_online_cpus: maximum msi-x vectors per CPUs core.
-	 *   Add additional 1 vector to ensure always available interrupt.
-	 */
-	xhci->msix_count = min(num_online_cpus() + 1,
-				HCS_MAX_INTRS(xhci->hcs_params1));
-
-	xhci->msix_entries =
-		kmalloc((sizeof(struct msix_entry))*xhci->msix_count,
-				GFP_KERNEL);
-	if (!xhci->msix_entries) {
-		xhci_err(xhci, "Failed to allocate MSI-X entries\n");
-		return -ENOMEM;
-	}
-
-	for (i = 0; i < xhci->msix_count; i++) {
-		xhci->msix_entries[i].entry = i;
-		xhci->msix_entries[i].vector = 0;
-	}
-
-	ret = pci_enable_msix(pdev, xhci->msix_entries, xhci->msix_count);
-	if (ret) {
-		xhci_dbg(xhci, "Failed to enable MSI-X\n");
-		goto free_entries;
-	}
-
-	for (i = 0; i < xhci->msix_count; i++) {
-		ret = request_irq(xhci->msix_entries[i].vector,
-				(irq_handler_t)xhci_msi_irq,
-				0, "xhci_hcd", xhci_to_hcd(xhci));
-		if (ret)
-			goto disable_msix;
-	}
-
-	hcd->msix_enabled = 1;
-	return ret;
-
-disable_msix:
-	xhci_dbg(xhci, "disable MSI-X interrupt\n");
-	xhci_free_irq(xhci);
-	pci_disable_msix(pdev);
-free_entries:
-	kfree(xhci->msix_entries);
-	xhci->msix_entries = NULL;
-	return ret;
-}
-
-/* Free any IRQs and disable MSI-X */
-static void xhci_cleanup_msix(struct xhci_hcd *xhci)
-{
-	struct usb_hcd *hcd = xhci_to_hcd(xhci);
-	struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
-
-	xhci_free_irq(xhci);
-
-	if (xhci->msix_entries) {
-		pci_disable_msix(pdev);
-		kfree(xhci->msix_entries);
-		xhci->msix_entries = NULL;
-	} else {
-		pci_disable_msi(pdev);
-	}
-
-	hcd->msix_enabled = 0;
-	return;
-}
 
 static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
 {
-	int i;
-
-	if (xhci->msix_entries) {
-		for (i = 0; i < xhci->msix_count; i++)
-			synchronize_irq(xhci->msix_entries[i].vector);
-	}
+	usb_hcd_msix_sync_irqs(xhci_to_hcd(xhci));
 }
 
-static int xhci_try_enable_msi(struct usb_hcd *hcd)
+/* called in interrupt setup during pci probe() */
+int xhci_get_msix_num(struct usb_hcd *hcd)
 {
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
-	struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
-	int ret;
-
-	/*
-	 * Some Fresco Logic host controllers advertise MSI, but fail to
-	 * generate interrupts.  Don't even try to enable MSI.
-	 */
-	if (xhci->quirks & XHCI_BROKEN_MSI)
-		return 0;
-
-	/* unregister the legacy interrupt */
-	if (hcd->irq)
-		free_irq(hcd->irq, hcd);
-	hcd->irq = -1;
 
-	ret = xhci_setup_msix(xhci);
-	if (ret)
-		/* fall back to msi*/
-		ret = xhci_setup_msi(xhci);
-
-	if (!ret)
-		/* hcd->irq is -1, we have MSI */
-		return 0;
-
-	if (!pdev->irq) {
-		xhci_err(xhci, "No msi-x/msi found and no IRQ in BIOS\n");
-		return -EINVAL;
-	}
-
-	/* fall back to legacy interrupt*/
-	ret = request_irq(pdev->irq, &usb_hcd_irq, IRQF_SHARED,
-			hcd->irq_descr, hcd);
-	if (ret) {
-		xhci_err(xhci, "request interrupt %d failed\n",
-				pdev->irq);
-		return ret;
-	}
-	hcd->irq = pdev->irq;
-	return 0;
+	/* danger: xhci may be null, but it's useless in xhci_readl() now */
+	return HCS_MAX_INTRS(xhci_readl(xhci,
+			&((struct xhci_cap_regs *)hcd->regs)->hcs_params1));
 }
 
 #else
 
-static int xhci_try_enable_msi(struct usb_hcd *hcd)
-{
-	return 0;
-}
-
-static void xhci_cleanup_msix(struct xhci_hcd *xhci)
-{
-}
-
 static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
 {
 }
@@ -411,7 +225,6 @@ int xhci_init(struct usb_hcd *hcd)
 
 	return retval;
 }
-
 /*-------------------------------------------------------------------------*/
 
 
@@ -497,7 +310,6 @@ int xhci_run(struct usb_hcd *hcd)
 {
 	u32 temp;
 	u64 temp_64;
-	int ret;
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 
 	/* Start the xHCI host controller running only after the USB 2.0 roothub
@@ -510,10 +322,6 @@ int xhci_run(struct usb_hcd *hcd)
 
 	xhci_dbg(xhci, "xhci_run\n");
 
-	ret = xhci_try_enable_msi(hcd);
-	if (ret)
-		return ret;
-
 #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
 	init_timer(&xhci->event_ring_timer);
 	xhci->event_ring_timer.data = (unsigned long) xhci;
@@ -609,7 +417,7 @@ void xhci_stop(struct usb_hcd *hcd)
 	xhci_reset(xhci);
 	spin_unlock_irq(&xhci->lock);
 
-	xhci_cleanup_msix(xhci);
+	usb_hcd_free_msi_msix(hcd);
 
 #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
 	/* Tell the event ring poll function not to reschedule */
@@ -651,7 +459,7 @@ void xhci_shutdown(struct usb_hcd *hcd)
 	xhci_halt(xhci);
 	spin_unlock_irq(&xhci->lock);
 
-	xhci_cleanup_msix(xhci);
+	usb_hcd_free_msi_msix(hcd);
 
 	xhci_dbg(xhci, "xhci_shutdown completed - status = %x\n",
 		    xhci_readl(xhci, &xhci->op_regs->status));
@@ -853,7 +661,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 		xhci_halt(xhci);
 		xhci_reset(xhci);
 		spin_unlock_irq(&xhci->lock);
-		xhci_cleanup_msix(xhci);
+		usb_hcd_free_msi_msix(hcd);
 
 #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
 		/* Tell the event ring poll function not to reschedule */
@@ -888,7 +696,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 		if (retval)
 			return retval;
 		xhci_dbg(xhci, "Start the primary HCD\n");
-		retval = xhci_run(hcd->primary_hcd);
+		if (dev_is_pci(hcd->self.controller))
+			retval = usb_hcd_register_msi_msix_irqs(
+						hcd->primary_hcd);
+		if (!retval)
+			retval = xhci_run(hcd->primary_hcd);
 		if (!retval) {
 			xhci_dbg(xhci, "Start the secondary HCD\n");
 			retval = xhci_run(secondary_hcd);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index fb99c83..8d511dd 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1388,9 +1388,6 @@ struct xhci_hcd {
 	int		page_size;
 	/* Valid values are 12 to 20, inclusive */
 	int		page_shift;
-	/* msi-x vectors */
-	int		msix_count;
-	struct msix_entry	*msix_entries;
 	/* data structures */
 	struct xhci_device_context_array *dcbaa;
 	struct xhci_ring	*cmd_ring;
@@ -1643,9 +1640,11 @@ void xhci_free_command(struct xhci_hcd *xhci,
 /* xHCI PCI glue */
 int xhci_register_pci(void);
 void xhci_unregister_pci(void);
+int xhci_get_msix_num(struct usb_hcd *hcd);
 #else
 static inline int xhci_register_pci(void) { return 0; }
 static inline void xhci_unregister_pci(void) {}
+static inline int xhci_get_msix_num(struct usb_hcd *hcd) { return 0; }
 #endif
 
 /* xHCI host controller glue */
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index b2f62f3..5253c02 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -22,6 +22,7 @@
 #ifdef __KERNEL__
 
 #include <linux/rwsem.h>
+#include <linux/pci.h>		/* for struct msix_entry */
 
 #define MAX_TOPO_LEVEL		6
 
@@ -133,6 +134,12 @@ struct usb_hcd {
 	u64			rsrc_len;	/* memory/io resource length */
 	unsigned		power_budget;	/* in mA, 0 = no limit */
 
+#ifdef CONFIG_PCI
+	/* msi-x vectors */
+	int			msix_count;
+	struct msix_entry	*msix_entries;
+#endif
+
 	/* bandwidth_mutex should be taken before adding or removing
 	 * any new bus bandwidth constraints:
 	 *   1. Before adding a configuration for a new device.
@@ -205,11 +212,14 @@ struct hc_driver {
 
 	/* irq handler */
 	irqreturn_t	(*irq) (struct usb_hcd *hcd);
+	irqreturn_t	(*msi_irq) (int irq, struct usb_hcd *hcd);
+	irqreturn_t	(*msix_irq) (int irq, struct usb_hcd *hcd);
 
 	int	flags;
 #define	HCD_MEMORY	0x0001		/* HC regs use memory (else I/O) */
 #define	HCD_LOCAL_MEM	0x0002		/* HC needs local memory */
 #define	HCD_SHARED	0x0004		/* Two (or more) usb_hcds share HW */
+#define	HCD_MSI_FIRST	0x0008		/* Try to get MSI first, PCI only */
 #define	HCD_USB11	0x0010		/* USB 1.1 */
 #define	HCD_USB2	0x0020		/* USB 2.0 */
 #define	HCD_USB3	0x0040		/* USB 3.0 */
@@ -218,6 +228,7 @@ struct hc_driver {
 	/* called to init HCD and root hub */
 	int	(*reset) (struct usb_hcd *hcd);
 	int	(*start) (struct usb_hcd *hcd);
+	int	(*get_msix_num) (struct usb_hcd *hcd);
 
 	/* NOTE:  these suspend/resume calls relate to the HC as
 	 * a whole, not just the root hub; they're for PCI bus glue.
@@ -393,6 +404,12 @@ extern int usb_hcd_pci_probe(struct pci_dev *dev,
 extern void usb_hcd_pci_remove(struct pci_dev *dev);
 extern void usb_hcd_pci_shutdown(struct pci_dev *dev);
 
+extern void usb_hcd_free_msi_msix(struct usb_hcd *hcd);
+extern void usb_hcd_msix_sync_irqs(struct usb_hcd *hcd);
+extern int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd,
+		unsigned int irqnum, unsigned long irqflags);
+extern int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd);
+
 #ifdef CONFIG_PM_SLEEP
 extern const struct dev_pm_ops usb_hcd_pci_pm_ops;
 #endif
-- 
1.6.3.3