On Thu, Jan 12, 2012 at 09:04:34AM +0800, Alex,Shi wrote:
>
> > This doesn't belong here either. All it does is remove code that was
> > added by the previous patch. If you don't want the code, don't add it
> > in the first place.
>
> Did you mean to merge the 2 patches into one? That should resolve your
> concern. So, how about the following new patch.
No, Alan meant that, in a patchset, you shouldn't add comments or code,
only to have delete them in the next patchset. The whole patchset needs
to be clean, with each patch being in its final form, so that bisection
works to find out where a bug was introduced. If you submit a patchset
where you introduce some buggy code, and then delete it in a later
patch, the bisect might fail. You can use git rebase, git reset --mixed,
and git add -p to move code changes from one patch in a series to
another.
I can't say whether he meant you should merge all your patches into one
patch, but I don't think that was Alan's intent.
Sarah Sharp
> ----
> >From 880edd30dffa60dc19159fb12b213b5a5e6a5705 Mon Sep 17 00:00:00 2001
> From: Alex Shi <alex.shi@xxxxxxxxx>
> Date: Wed, 11 Jan 2012 14:32:47 +0800
> Subject: [PATCH v4] USB: Try MSI first before line IRQ in XHCI PCI driver.
>
> Current PCI hcd driver will check and enable line IRQ first, if line
> IRQ is no setting in BIOS, XHCI PCIe driver will exit. But infact, if
> MSI can work for this HCD, this behavior will refuse a workable HCD.
>
> This patch change the behavior. It skips the first line IRQ checking for
> PCI XHCI HCD in usb-core, and try to enable MSI first, if the MSI out of
> work, it is back to line IRQ at that time. And further more, trying MSI
> first can avoid a unnecessary request/free irq for line IRQ for PCIe HCD.
>
> Thanks for Sarah and Andiry's suggestion and review for this patch.
>
> Signed-off-by: Alex Shi <alex.shi@xxxxxxxxx>
> ---
> drivers/usb/core/hcd-pci.c | 3 ++-
> drivers/usb/core/hcd.c | 7 +++++--
> drivers/usb/host/xhci-pci.c | 3 ++-
> drivers/usb/host/xhci.c | 28 ++++++++++++++--------------
> include/linux/usb/hcd.h | 1 +
> 5 files changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> index a004db3..266ea2f 100644
> --- a/drivers/usb/core/hcd-pci.c
> +++ b/drivers/usb/core/hcd-pci.c
> @@ -187,7 +187,8 @@ 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) {
> + /* 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));
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 13222d3..0a4f185 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2466,8 +2466,11 @@ 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, except MSI
> + * first try HCD. That will do it in following driver->start();
> + */
> + if (usb_hcd_is_primary_hcd(hcd) &&
> + !(hcd->driver->flags & HCD_MSI_FIRST)) {
> 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..3283e62 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -243,7 +243,8 @@ static const struct hc_driver xhci_pci_hc_driver = {
> * generic hardware linkage
> */
> .irq = xhci_irq,
> - .flags = HCD_MEMORY | HCD_USB3 | HCD_SHARED,
> + .flags = HCD_MEMORY | HCD_USB3 | HCD_SHARED |
> + HCD_MSI_FIRST,
>
> /*
> * basic lifecycle operations
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index a1afb7c..e385a55 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -331,26 +331,26 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
> struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> int ret;
>
> + hcd->irq = -1;
> /*
> * 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;
> + if (!(xhci->quirks & XHCI_BROKEN_MSI)) {
> + ret = xhci_setup_msix(xhci);
> + if (ret)
> + /* fall back to msi*/
> + ret = xhci_setup_msi(xhci);
>
> - 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 (!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,
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 03354d5..ea1637b 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -212,6 +212,7 @@ struct hc_driver {
> #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 */
> --
> 1.6.3.3
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
