pci: do not try to assign irq 255

Commit Message

Hannes Reinecke Feb. 18, 2013, 10:09 a.m. UTC

The PCI config space reseves a byte for the interrupt line,
so irq 255 actually refers to 'not set'.
However, the 'irq' field for struct pci_dev is an integer,
so the original meaning is lost, causing the system to
assign an interrupt '255', which fails.

So we should _not_ assign an interrupt value here, and
allow upper layers to fixup things.

This patch make PCI devices with MSI interrupts only
(like the xhci device on certain HP laptops) work properly.

Cc: Frederik Himpe <[e-mail]>
Cc: Oliver Neukum <[e-mail]>
Cc: David Haerdeman <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Hannes Reinecke <[e-mail]>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Härdeman Feb. 19, 2013, 1:13 a.m. UTC | #1

On Mon, Feb 18, 2013 at 11:09:53AM +0100, Hannes Reinecke wrote:
>The PCI config space reseves a byte for the interrupt line,
>so irq 255 actually refers to 'not set'.
>However, the 'irq' field for struct pci_dev is an integer,
>so the original meaning is lost, causing the system to
>assign an interrupt '255', which fails.
>
>So we should _not_ assign an interrupt value here, and
>allow upper layers to fixup things.
>
>This patch make PCI devices with MSI interrupts only
>(like the xhci device on certain HP laptops) work properly.

Just tested and it works for me. Thank you.

Tested-by: David Härdeman <[e-mail]>

>Cc: Frederik Himpe <[e-mail]>
>Cc: Oliver Neukum <[e-mail]>
>Cc: David Haerdeman <[e-mail]>
>Cc: [e-mail]
>Cc: [e-mail]
>Signed-off-by: Hannes Reinecke <[e-mail]>
>
>diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>index 6186f03..a2db887f 100644
>--- a/drivers/pci/probe.c
>+++ b/drivers/pci/probe.c
>@@ -923,7 +923,8 @@ static void pci_read_irq(struct pci_dev *dev)
> 	dev->pin = irq;
> 	if (irq)
> 		pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
>-	dev->irq = irq;
>+	if (irq < 255)
>+		dev->irq = irq;
> }
> 
> void set_pcie_port_type(struct pci_dev *pdev)
>

Yinghai Lu Feb. 19, 2013, 7:40 p.m. UTC | #2

On Mon, Feb 18, 2013 at 2:09 AM, Hannes Reinecke <[e-mail]> wrote:
> The PCI config space reseves a byte for the interrupt line,
> so irq 255 actually refers to 'not set'.
> However, the 'irq' field for struct pci_dev is an integer,
> so the original meaning is lost, causing the system to
> assign an interrupt '255', which fails.
>
> So we should _not_ assign an interrupt value here, and
> allow upper layers to fixup things.
>
> This patch make PCI devices with MSI interrupts only
> (like the xhci device on certain HP laptops) work properly.

looks like the bios does not provide _PRT for device in ACPI.

also according to PCI spec, BIOS *must* set interrupt line.

>
> Cc: Frederik Himpe <[e-mail]>
> Cc: Oliver Neukum <[e-mail]>
> Cc: David Haerdeman <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Hannes Reinecke <[e-mail]>
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6186f03..a2db887f 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -923,7 +923,8 @@ static void pci_read_irq(struct pci_dev *dev)
>         dev->pin = irq;
>         if (irq)
>                 pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
> -       dev->irq = irq;
> +       if (irq < 255)
> +               dev->irq = irq;
>  }
>
>  void set_pcie_port_type(struct pci_dev *pdev)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas Feb. 19, 2013, 7:47 p.m. UTC | #3

On Mon, Feb 18, 2013 at 3:09 AM, Hannes Reinecke <[e-mail]> wrote:
> The PCI config space reseves a byte for the interrupt line,
> so irq 255 actually refers to 'not set'.
> However, the 'irq' field for struct pci_dev is an integer,
> so the original meaning is lost, causing the system to
> assign an interrupt '255', which fails.
>
> So we should _not_ assign an interrupt value here, and
> allow upper layers to fixup things.
>
> This patch make PCI devices with MSI interrupts only
> (like the xhci device on certain HP laptops) work properly.
>
> Cc: Frederik Himpe <[e-mail]>
> Cc: Oliver Neukum <[e-mail]>
> Cc: David Haerdeman <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Hannes Reinecke <[e-mail]>
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6186f03..a2db887f 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -923,7 +923,8 @@ static void pci_read_irq(struct pci_dev *dev)
>         dev->pin = irq;
>         if (irq)
>                 pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
> -       dev->irq = irq;
> +       if (irq < 255)
> +               dev->irq = irq;
>  }
>
>  void set_pcie_port_type(struct pci_dev *pdev)

Is there a bugzilla or other URL with more information (problem
description, hardware involved, dmesg logs, acpidump, etc)?

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Frederik Himpe Feb. 19, 2013, 10:36 p.m. UTC | #4

On Tue, Feb 19, 2013 at 12:47:32PM -0700, Bjorn Helgaas wrote:
> On Mon, Feb 18, 2013 at 3:09 AM, Hannes Reinecke <[e-mail]> wrote:
> > The PCI config space reseves a byte for the interrupt line,
> > so irq 255 actually refers to 'not set'.
> > However, the 'irq' field for struct pci_dev is an integer,
> > so the original meaning is lost, causing the system to
> > assign an interrupt '255', which fails.
> >
> > So we should _not_ assign an interrupt value here, and
> > allow upper layers to fixup things.
> >
> > This patch make PCI devices with MSI interrupts only
> > (like the xhci device on certain HP laptops) work properly.
> >
> > Cc: Frederik Himpe <[e-mail]>
> > Cc: Oliver Neukum <[e-mail]>
> > Cc: David Haerdeman <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Signed-off-by: Hannes Reinecke <[e-mail]>
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 6186f03..a2db887f 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -923,7 +923,8 @@ static void pci_read_irq(struct pci_dev *dev)
> >         dev->pin = irq;
> >         if (irq)
> >                 pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
> > -       dev->irq = irq;
> > +       if (irq < 255)
> > +               dev->irq = irq;
> >  }
> >
> >  void set_pcie_port_type(struct pci_dev *pdev)
> 
> Is there a bugzilla or other URL with more information (problem
> description, hardware involved, dmesg logs, acpidump, etc)?

https://bugzilla.kernel.org/show_bug.cgi?id=52591
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1072918

Basically it looks like most HP Probook/Elitebook of the Ivy Bridge
generation are affected.

Hannes Reinecke Feb. 20, 2013, 7:58 a.m. UTC | #5

On 02/19/2013 08:40 PM, Yinghai Lu wrote:
> On Mon, Feb 18, 2013 at 2:09 AM, Hannes Reinecke <[e-mail]> wrote:
>> The PCI config space reseves a byte for the interrupt line,
>> so irq 255 actually refers to 'not set'.
>> However, the 'irq' field for struct pci_dev is an integer,
>> so the original meaning is lost, causing the system to
>> assign an interrupt '255', which fails.
>>
>> So we should _not_ assign an interrupt value here, and
>> allow upper layers to fixup things.
>>
>> This patch make PCI devices with MSI interrupts only
>> (like the xhci device on certain HP laptops) work properly.
>
> looks like the bios does not provide _PRT for device in ACPI.
>
Correct.

> also according to PCI spec, BIOS *must* set interrupt line.
>
Apparently this device is meant to use MSI _only_ so the BIOS 
developer didn't feel the need to assign an INTx here.

According to PCI-3.0, section 6.8 (Message Signalled Interrupts):
 > It is recommended that devices implement interrupt pins to
 > provide compatibility in systems that do not support MSI
 > (devices default to interrupt pins). However, it is expected
 > that the need for interrupt pins will diminish over time.
 > Devices that do not support interrupt pins due to pin
 > constraints (rely on polling for device service) may implement
 > messages to increase performance without adding additional pins. 
 > Therefore, system configuration software must not assume that a
 > message capable device has an interrupt pin.

Which sounds to me as if the implementation is valid...
And in either case, I've added the relevant details plus patch
to bnc#52591.
Including ACPI dump, so you can check for yourself.

And correct me if I'm wrong, of course :-)

Cheers,

Hannes

Yinghai Lu Feb. 20, 2013, 4:57 p.m. UTC | #6

On Tue, Feb 19, 2013 at 11:58 PM, Hannes Reinecke <[e-mail]> wrote:
>>
> Apparently this device is meant to use MSI _only_ so the BIOS developer
> didn't feel the need to assign an INTx here.
>
> According to PCI-3.0, section 6.8 (Message Signalled Interrupts):
>> It is recommended that devices implement interrupt pins to
>> provide compatibility in systems that do not support MSI
>> (devices default to interrupt pins). However, it is expected
>> that the need for interrupt pins will diminish over time.
>> Devices that do not support interrupt pins due to pin
>> constraints (rely on polling for device service) may implement
>> messages to increase performance without adding additional pins. >
>> Therefore, system configuration software must not assume that a
>> message capable device has an interrupt pin.
>
> Which sounds to me as if the implementation is valid...

it seems you mess pin with interrupt line.

current code:
        unsigned char irq;

        pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq);
        dev->pin = irq;
        if (irq)
                pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
        dev->irq = irq;

so if the device does not have interrupt pin implemented, pin should be zero.
and  pin and irq in dev should
be all 0.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6186f03..a2db887f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -923,7 +923,8 @@  static void pci_read_irq(struct pci_dev *dev)
 	dev->pin = irq;
 	if (irq)
 		pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
-	dev->irq = irq;
+	if (irq < 255)
+		dev->irq = irq;
 }
 
 void set_pcie_port_type(struct pci_dev *pdev)