2013-03-04 08:22:12

by Hannes Reinecke

[permalink] [raw]
Subject: [PATCH][v2] xhci: correctly enable interrupts

xhci has its own interrupt enabling routine, which will try to
use MSI-X/MSI if present. So the usb core shouldn't try to enable
legacy interrupts; on some machines the xhci legacy IRQ setting
is invalid.

Cc: Bjorn Helgaas <[email protected]>
Cc: Oliver Neukum <[email protected]>
Cc: Thomas Renninger <[email protected]>
Cc: Yinghai Lu <[email protected]>
Cc: Frederik Himpe <[email protected]>
Cc: David Haerdeman <[email protected]>
Cc: Alan Stern <[email protected]>
Signed-off-by: Hannes Reinecke <[email protected]>

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 622b4a4..2647e75 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -173,6 +173,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
struct hc_driver *driver;
struct usb_hcd *hcd;
int retval;
+ int hcd_irq = 0;

if (usb_disabled())
return -ENODEV;
@@ -187,15 +188,21 @@ 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.
+ /*
+ * The xHCI driver supports MSI and MSI-X,
+ * so don't fail if the BIOS doesn't provide a legacy IRQ
+ * and do not try to enable legacy IRQs.
*/
- 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));
- retval = -ENODEV;
- goto disable_pci;
+ if ((driver->flags & HCD_MASK) != HCD_USB3) {
+ if (!dev->irq) {
+ dev_err(&dev->dev,
+ "Found HC with no IRQ. "
+ "Check BIOS/PCI %s setup!\n",
+ pci_name(dev));
+ retval = -ENODEV;
+ goto disable_pci;
+ }
+ hcd_irq = dev->irq;
}

hcd = usb_create_hcd(driver, &dev->dev, pci_name(dev));
@@ -245,7 +252,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)

pci_set_master(dev);

- retval = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
+ retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED);
if (retval != 0)
goto unmap_registers;
set_hs_companion(dev, hcd);


2013-03-04 11:25:41

by David Härdeman

[permalink] [raw]
Subject: Re: [PATCH][v2] xhci: correctly enable interrupts

On Mon, Mar 04, 2013 at 09:22:04AM +0100, Hannes Reinecke wrote:
>xhci has its own interrupt enabling routine, which will try to
>use MSI-X/MSI if present. So the usb core shouldn't try to enable
>legacy interrupts; on some machines the xhci legacy IRQ setting
>is invalid.
>
>Cc: Bjorn Helgaas <[email protected]>
>Cc: Oliver Neukum <[email protected]>
>Cc: Thomas Renninger <[email protected]>
>Cc: Yinghai Lu <[email protected]>
>Cc: Frederik Himpe <[email protected]>
>Cc: David Haerdeman <[email protected]>
>Cc: Alan Stern <[email protected]>
>Signed-off-by: Hannes Reinecke <[email protected]>

No idea if it's the "right" solution but it works for me.

Tested-by: David H?rdeman <[email protected]>

>
>diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
>index 622b4a4..2647e75 100644
>--- a/drivers/usb/core/hcd-pci.c
>+++ b/drivers/usb/core/hcd-pci.c
>@@ -173,6 +173,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> struct hc_driver *driver;
> struct usb_hcd *hcd;
> int retval;
>+ int hcd_irq = 0;
>
> if (usb_disabled())
> return -ENODEV;
>@@ -187,15 +188,21 @@ 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.
>+ /*
>+ * The xHCI driver supports MSI and MSI-X,
>+ * so don't fail if the BIOS doesn't provide a legacy IRQ
>+ * and do not try to enable legacy IRQs.
> */
>- 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));
>- retval = -ENODEV;
>- goto disable_pci;
>+ if ((driver->flags & HCD_MASK) != HCD_USB3) {
>+ if (!dev->irq) {
>+ dev_err(&dev->dev,
>+ "Found HC with no IRQ. "
>+ "Check BIOS/PCI %s setup!\n",
>+ pci_name(dev));
>+ retval = -ENODEV;
>+ goto disable_pci;
>+ }
>+ hcd_irq = dev->irq;
> }
>
> hcd = usb_create_hcd(driver, &dev->dev, pci_name(dev));
>@@ -245,7 +252,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>
> pci_set_master(dev);
>
>- retval = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
>+ retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED);
> if (retval != 0)
> goto unmap_registers;
> set_hs_companion(dev, hcd);
>

--
David H?rdeman

2013-03-04 13:28:28

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH][v2] xhci: correctly enable interrupts

Hello.

On 04-03-2013 12:22, Hannes Reinecke wrote:

> xhci has its own interrupt enabling routine, which will try to
> use MSI-X/MSI if present. So the usb core shouldn't try to enable
> legacy interrupts; on some machines the xhci legacy IRQ setting
> is invalid.

> Cc: Bjorn Helgaas <[email protected]>
> Cc: Oliver Neukum <[email protected]>
> Cc: Thomas Renninger <[email protected]>
> Cc: Yinghai Lu <[email protected]>
> Cc: Frederik Himpe <[email protected]>
> Cc: David Haerdeman <[email protected]>
> Cc: Alan Stern <[email protected]>
> Signed-off-by: Hannes Reinecke <[email protected]>

> diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> index 622b4a4..2647e75 100644
> --- a/drivers/usb/core/hcd-pci.c
> +++ b/drivers/usb/core/hcd-pci.c
[...]
> @@ -187,15 +188,21 @@ 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.
> + /*
> + * The xHCI driver supports MSI and MSI-X,
> + * so don't fail if the BIOS doesn't provide a legacy IRQ
> + * and do not try to enable legacy IRQs.
> */
> - 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));
> - retval = -ENODEV;
> - goto disable_pci;
> + if ((driver->flags & HCD_MASK) != HCD_USB3) {
> + if (!dev->irq) {
> + dev_err(&dev->dev,
> + "Found HC with no IRQ. "
> + "Check BIOS/PCI %s setup!\n",

The message strings are allowed to take more than 80 chars (to facilitate
grepping for them), so no need to break this one; checkpatch.pl should not
complain...

> + pci_name(dev));
> + retval = -ENODEV;
> + goto disable_pci;
> + }
> + hcd_irq = dev->irq;
> }
>
> hcd = usb_create_hcd(driver, &dev->dev, pci_name(dev));

WBR, Sergei

2013-03-04 15:26:43

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH][v2] xhci: correctly enable interrupts

On Mon, 4 Mar 2013, Hannes Reinecke wrote:

> xhci has its own interrupt enabling routine, which will try to
> use MSI-X/MSI if present. So the usb core shouldn't try to enable
> legacy interrupts; on some machines the xhci legacy IRQ setting
> is invalid.

This version of the patch is much better than the first one, IMO.
Thank you.

Alan Stern

2013-03-04 16:11:11

by Thomas Renninger

[permalink] [raw]
Subject: Re: [PATCH][v2] xhci: correctly enable interrupts

On Monday, March 04, 2013 10:26:40 AM Alan Stern wrote:
> On Mon, 4 Mar 2013, Hannes Reinecke wrote:
> > xhci has its own interrupt enabling routine, which will try to
> > use MSI-X/MSI if present. So the usb core shouldn't try to enable
> > legacy interrupts; on some machines the xhci legacy IRQ setting
> > is invalid.
>
> This version of the patch is much better than the first one, IMO.

I also have this issue. Unfortunately pci read only gives an irq of 255
in secure boot mode and I don't want to struggle with kernel/module
signing to test this.

I found one issue with this patch:
For xhci legacy PCI is not tried to be set up now anymore (before MSI(x)
is tried) which is a correct fix.

But in xhci_try_enable_msi() drivers/usb/host/xhci.c if MSI is known broken
(xhci->quirks & XHCI_BROKEN_MSI), it relies on legacy IRQ being enabled
already.
Instead it should use the "enable legacy IRQ" code later in the same function
which is the fallback if MSI setup does not succeed.

I send an updated version taking care about above and including the "do not
split string" concern Sergei mentioned.

Thomas

2013-03-04 16:14:46

by Thomas Renninger

[permalink] [raw]
Subject: [PATCH][v3] xhci: correctly enable interrupts

From: Hannes Reinecke <[email protected]>

xhci has its own interrupt enabling routine, which will try to
use MSI-X/MSI if present. So the usb core shouldn't try to enable
legacy interrupts; on some machines the xhci legacy IRQ setting
is invalid.

v3: Be careful to not break XHCI_BROKEN_MSI workaround (by trenn)

Cc: Bjorn Helgaas <[email protected]>
Cc: Oliver Neukum <[email protected]>
Cc: Thomas Renninger <[email protected]>
Cc: Yinghai Lu <[email protected]>
Cc: Frederik Himpe <[email protected]>
Cc: David Haerdeman <[email protected]>
Cc: Alan Stern <[email protected]>

Reviewed-by: Thomas Renninger <[email protected]>
Signed-off-by: Hannes Reinecke <[email protected]>

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 622b4a4..2b487d4 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -173,6 +173,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
struct hc_driver *driver;
struct usb_hcd *hcd;
int retval;
+ int hcd_irq = 0;

if (usb_disabled())
return -ENODEV;
@@ -187,15 +188,19 @@ 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.
+ /*
+ * The xHCI driver has its own irq management
+ * make sure irq setup is not touched for xhci in generic hcd code
*/
- 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));
- retval = -ENODEV;
- goto disable_pci;
+ if ((driver->flags & HCD_MASK) != HCD_USB3) {
+ if (!dev->irq) {
+ dev_err(&dev->dev,
+ "Found HC with no IRQ. Check BIOS/PCI %s setup!\n",
+ pci_name(dev));
+ retval = -ENODEV;
+ goto disable_pci;
+ }
+ hcd_irq = dev->irq;
}

hcd = usb_create_hcd(driver, &dev->dev, pci_name(dev));
@@ -245,7 +250,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)

pci_set_master(dev);

- retval = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
+ retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED);
if (retval != 0)
goto unmap_registers;
set_hs_companion(dev, hcd);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index f1f01a8..849470b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -350,7 +350,7 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
* generate interrupts. Don't even try to enable MSI.
*/
if (xhci->quirks & XHCI_BROKEN_MSI)
- return 0;
+ goto legacy_irq;

/* unregister the legacy interrupt */
if (hcd->irq)
@@ -371,6 +371,7 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
return -EINVAL;
}

+ legacy_irq:
/* fall back to legacy interrupt*/
ret = request_irq(pdev->irq, &usb_hcd_irq, IRQF_SHARED,
hcd->irq_descr, hcd);

2013-03-04 17:08:03

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH][v3] xhci: correctly enable interrupts

Hello.

On 03/04/2013 07:14 PM, Thomas Renninger wrote:

> From: Hannes Reinecke<[email protected]>
>
> xhci has its own interrupt enabling routine, which will try to
> use MSI-X/MSI if present. So the usb core shouldn't try to enable
> legacy interrupts; on some machines the xhci legacy IRQ setting
> is invalid.
>
> v3: Be careful to not break XHCI_BROKEN_MSI workaround (by trenn)
>
> Cc: Bjorn Helgaas<[email protected]>
> Cc: Oliver Neukum<[email protected]>
> Cc: Thomas Renninger<[email protected]>
> Cc: Yinghai Lu<[email protected]>
> Cc: Frederik Himpe<[email protected]>
> Cc: David Haerdeman<[email protected]>
> Cc: Alan Stern<[email protected]>
>
> Reviewed-by: Thomas Renninger<[email protected]>
> Signed-off-by: Hannes Reinecke<[email protected]>
Still a couple of style comments...

[...]

> @@ -187,15 +188,19 @@ 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.
> + /*
> + * The xHCI driver has its own irq management
> + * make sure irq setup is not touched for xhci in generic hcd code
> */
> - 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));
> - retval = -ENODEV;
> - goto disable_pci;
> + if ((driver->flags& HCD_MASK) != HCD_USB3) {
> + if (!dev->irq) {
> + dev_err(&dev->dev,
> + "Found HC with no IRQ. Check BIOS/PCI %s setup!\n",
>

Could you indent the string like the line below?

> + pci_name(dev));
> + retval = -ENODEV;
> + goto disable_pci;
> + }
> + hcd_irq = dev->irq;
> }
>
> hcd = usb_create_hcd(driver,&dev->dev, pci_name(dev));
>
[...]
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index f1f01a8..849470b 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
>
[...]
> @@ -371,6 +371,7 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
> return -EINVAL;
> }
>
> + legacy_irq:
>

Labels shouldn't be indented by a space (unless the existing coding
style has them indented already, of course).
Although that might be a rule dropped by checkpatch.pl already -- it
doesn't complain.

WBR, Sergei

2013-03-04 17:36:13

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH][v3] xhci: correctly enable interrupts

On Mon, 4 Mar 2013, Sergei Shtylyov wrote:

> > @@ -371,6 +371,7 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
> > return -EINVAL;
> > }
> >
> > + legacy_irq:
> >
>
> Labels shouldn't be indented by a space (unless the existing coding
> style has them indented already, of course).
> Although that might be a rule dropped by checkpatch.pl already -- it
> doesn't complain.

Indeed, there's a definite advantage to putting a space before a label:
The diff program doesn't get confused into thinking the label is the
start of a new function.

Alan Stern

2013-03-05 10:45:33

by Thomas Renninger

[permalink] [raw]
Subject: Re: [PATCH][v3] xhci: correctly enable interrupts

On Monday, March 04, 2013 05:14:43 PM Thomas Renninger wrote:
> From: Hannes Reinecke <[email protected]>
>
> xhci has its own interrupt enabling routine, which will try to
> use MSI-X/MSI if present. So the usb core shouldn't try to enable
> legacy interrupts; on some machines the xhci legacy IRQ setting
> is invalid.
>
> v3: Be careful to not break XHCI_BROKEN_MSI workaround (by trenn)
>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Oliver Neukum <[email protected]>
> Cc: Thomas Renninger <[email protected]>
> Cc: Yinghai Lu <[email protected]>
> Cc: Frederik Himpe <[email protected]>
> Cc: David Haerdeman <[email protected]>
> Cc: Alan Stern <[email protected]>
>
> Reviewed-by: Thomas Renninger <[email protected]>
> Signed-off-by: Hannes Reinecke <[email protected]>

Argh, I should have added:
CC: [email protected]


This seem to be a common issue on new machines.
And having xhci not functional at all there is nasty.

The patch is not complex and should fulfill all stable@ criteria.

Is this the official USB mainline subtree?:
https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git

Adding Felipe, would be great if this one can get queued for 3.9
inclusion. Would be nice if:
CC: [email protected]
can be added manually, I can also explicitly submit it to stable@
as soon as it hits Linus' tree.

Thanks,

Thomas

>
> diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> index 622b4a4..2b487d4 100644
> --- a/drivers/usb/core/hcd-pci.c
> +++ b/drivers/usb/core/hcd-pci.c
> @@ -173,6 +173,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct
> pci_device_id *id) struct hc_driver *driver;
> struct usb_hcd *hcd;
> int retval;
> + int hcd_irq = 0;
>
> if (usb_disabled())
> return -ENODEV;
> @@ -187,15 +188,19 @@ 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.
> + /*
> + * The xHCI driver has its own irq management
> + * make sure irq setup is not touched for xhci in generic hcd code
> */
> - 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));
> - retval = -ENODEV;
> - goto disable_pci;
> + if ((driver->flags & HCD_MASK) != HCD_USB3) {
> + if (!dev->irq) {
> + dev_err(&dev->dev,
> + "Found HC with no IRQ. Check BIOS/PCI %s setup!\n",
> + pci_name(dev));
> + retval = -ENODEV;
> + goto disable_pci;
> + }
> + hcd_irq = dev->irq;
> }
>
> hcd = usb_create_hcd(driver, &dev->dev, pci_name(dev));
> @@ -245,7 +250,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
>
> pci_set_master(dev);
>
> - retval = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
> + retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED);
> if (retval != 0)
> goto unmap_registers;
> set_hs_companion(dev, hcd);
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index f1f01a8..849470b 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -350,7 +350,7 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
> * generate interrupts. Don't even try to enable MSI.
> */
> if (xhci->quirks & XHCI_BROKEN_MSI)
> - return 0;
> + goto legacy_irq;
>
> /* unregister the legacy interrupt */
> if (hcd->irq)
> @@ -371,6 +371,7 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
> return -EINVAL;
> }
>
> + legacy_irq:
> /* fall back to legacy interrupt*/
> ret = request_irq(pdev->irq, &usb_hcd_irq, IRQF_SHARED,
> hcd->irq_descr, hcd);

2013-03-05 23:38:05

by Sarah Sharp

[permalink] [raw]
Subject: Re: [PATCH][v3] xhci: correctly enable interrupts

Looks fine on the xHCI portion. Thanks for catching the XHCI_BROKEN_MSI
case.

Acked-by: Sarah Sharp <[email protected]>

On Mon, Mar 04, 2013 at 05:14:43PM +0100, Thomas Renninger wrote:
> From: Hannes Reinecke <[email protected]>
>
> xhci has its own interrupt enabling routine, which will try to
> use MSI-X/MSI if present. So the usb core shouldn't try to enable
> legacy interrupts; on some machines the xhci legacy IRQ setting
> is invalid.
>
> v3: Be careful to not break XHCI_BROKEN_MSI workaround (by trenn)
>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Oliver Neukum <[email protected]>
> Cc: Thomas Renninger <[email protected]>
> Cc: Yinghai Lu <[email protected]>
> Cc: Frederik Himpe <[email protected]>
> Cc: David Haerdeman <[email protected]>
> Cc: Alan Stern <[email protected]>
>
> Reviewed-by: Thomas Renninger <[email protected]>
> Signed-off-by: Hannes Reinecke <[email protected]>
>
> diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> index 622b4a4..2b487d4 100644
> --- a/drivers/usb/core/hcd-pci.c
> +++ b/drivers/usb/core/hcd-pci.c
> @@ -173,6 +173,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> struct hc_driver *driver;
> struct usb_hcd *hcd;
> int retval;
> + int hcd_irq = 0;
>
> if (usb_disabled())
> return -ENODEV;
> @@ -187,15 +188,19 @@ 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.
> + /*
> + * The xHCI driver has its own irq management
> + * make sure irq setup is not touched for xhci in generic hcd code
> */
> - 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));
> - retval = -ENODEV;
> - goto disable_pci;
> + if ((driver->flags & HCD_MASK) != HCD_USB3) {
> + if (!dev->irq) {
> + dev_err(&dev->dev,
> + "Found HC with no IRQ. Check BIOS/PCI %s setup!\n",
> + pci_name(dev));
> + retval = -ENODEV;
> + goto disable_pci;
> + }
> + hcd_irq = dev->irq;
> }
>
> hcd = usb_create_hcd(driver, &dev->dev, pci_name(dev));
> @@ -245,7 +250,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>
> pci_set_master(dev);
>
> - retval = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
> + retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED);
> if (retval != 0)
> goto unmap_registers;
> set_hs_companion(dev, hcd);
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index f1f01a8..849470b 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -350,7 +350,7 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
> * generate interrupts. Don't even try to enable MSI.
> */
> if (xhci->quirks & XHCI_BROKEN_MSI)
> - return 0;
> + goto legacy_irq;
>
> /* unregister the legacy interrupt */
> if (hcd->irq)
> @@ -371,6 +371,7 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd)
> return -EINVAL;
> }
>
> + legacy_irq:
> /* fall back to legacy interrupt*/
> ret = request_irq(pdev->irq, &usb_hcd_irq, IRQF_SHARED,
> hcd->irq_descr, hcd);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-03-15 18:34:57

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH][v3] xhci: correctly enable interrupts

On Tue, Mar 05, 2013 at 11:45:28AM +0100, Thomas Renninger wrote:
> On Monday, March 04, 2013 05:14:43 PM Thomas Renninger wrote:
> > From: Hannes Reinecke <[email protected]>
> >
> > xhci has its own interrupt enabling routine, which will try to
> > use MSI-X/MSI if present. So the usb core shouldn't try to enable
> > legacy interrupts; on some machines the xhci legacy IRQ setting
> > is invalid.
> >
> > v3: Be careful to not break XHCI_BROKEN_MSI workaround (by trenn)
> >
> > Cc: Bjorn Helgaas <[email protected]>
> > Cc: Oliver Neukum <[email protected]>
> > Cc: Thomas Renninger <[email protected]>
> > Cc: Yinghai Lu <[email protected]>
> > Cc: Frederik Himpe <[email protected]>
> > Cc: David Haerdeman <[email protected]>
> > Cc: Alan Stern <[email protected]>
> >
> > Reviewed-by: Thomas Renninger <[email protected]>
> > Signed-off-by: Hannes Reinecke <[email protected]>
>
> Argh, I should have added:
> CC: [email protected]

You mean "[email protected]" :)

I'll go do it...

thanks,

greg k-h