drivers/pci/quirks.c: irq_reroute_variant is dead code?

drivers/pci/quirks.c: irq_reroute_variant is dead code?

[Jeremy Fitzhardinge]

quirk_reroute_to_boot_interrupts_intel() goes to some effort to scan for
a bunch of Intel devices and set dev->irq_reroute_variant =
INTEL_IRQ_REROUTE_VARIANT.  However, as far as I can see that's the only
reference to dev->irq_reroute_variant and INTEL_IRQ_REROUTE_VARIANT, so
the whole thing appears to have no effect.

Is it all dead code?  Is there something else which should be there missing?

Thanks,
    J

Re: drivers/pci/quirks.c: irq_reroute_variant is dead code?

[Jesse Barnes]

On Tue, 28 Jul 2009 12:50:08 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> quirk_reroute_to_boot_interrupts_intel() goes to some effort to scan
> for a bunch of Intel devices and set dev->irq_reroute_variant =
> INTEL_IRQ_REROUTE_VARIANT.  However, as far as I can see that's the
> only reference to dev->irq_reroute_variant and
> INTEL_IRQ_REROUTE_VARIANT, so the whole thing appears to have no
> effect.
> 
> Is it all dead code?  Is there something else which should be there
> missing?

Hm my quick look couldn't find a user either.  The initial patch to add
that stuff (e1d3a90846b40ad3160bf4b648d36c6badad39ac) had an ACPI
component, but it looks like that may have vanished somehow (maybe
some of the IOAPIC code got shuffled around and this bit was
missed?).

Stefan or Ingo?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: drivers/pci/quirks.c: irq_reroute_variant is dead code?

[Ingo Molnar]

* Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Tue, 28 Jul 2009 12:50:08 -0700
> Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> 
> > quirk_reroute_to_boot_interrupts_intel() goes to some effort to 
> > scan for a bunch of Intel devices and set 
> > dev->irq_reroute_variant = INTEL_IRQ_REROUTE_VARIANT.  However, 
> > as far as I can see that's the only reference to 
> > dev->irq_reroute_variant and INTEL_IRQ_REROUTE_VARIANT, so the 
> > whole thing appears to have no effect.
> > 
> > Is it all dead code?  Is there something else which should be 
> > there missing?
> 
> Hm my quick look couldn't find a user either.  The initial patch 
> to add that stuff (e1d3a90846b40ad3160bf4b648d36c6badad39ac) had 
> an ACPI component, but it looks like that may have vanished 
> somehow (maybe some of the IOAPIC code got shuffled around and 
> this bit was missed?).
> 
> Stefan or Ingo?

I dont remember ioapic interactions. Stefan?

	Ingo

Re: drivers/pci/quirks.c: irq_reroute_variant is dead code?

[Jesse Barnes]

[Corrected Stefan's address.]

On Sat, 8 Aug 2009 16:45:20 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > On Tue, 28 Jul 2009 12:50:08 -0700
> > Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> > 
> > > quirk_reroute_to_boot_interrupts_intel() goes to some effort to 
> > > scan for a bunch of Intel devices and set 
> > > dev->irq_reroute_variant = INTEL_IRQ_REROUTE_VARIANT.  However, 
> > > as far as I can see that's the only reference to 
> > > dev->irq_reroute_variant and INTEL_IRQ_REROUTE_VARIANT, so the 
> > > whole thing appears to have no effect.
> > > 
> > > Is it all dead code?  Is there something else which should be 
> > > there missing?
> > 
> > Hm my quick look couldn't find a user either.  The initial patch 
> > to add that stuff (e1d3a90846b40ad3160bf4b648d36c6badad39ac) had 
> > an ACPI component, but it looks like that may have vanished 
> > somehow (maybe some of the IOAPIC code got shuffled around and 
> > this bit was missed?).
> > 
> > Stefan or Ingo?
> 
> I dont remember ioapic interactions. Stefan?
> 
> 	Ingo
> 


-- 
Jesse Barnes, Intel Open Source Technology Center

Re: drivers/pci/quirks.c: irq_reroute_variant is dead code?

[Stefan Assmann]

On 10.08.2009 18:32, Jesse Barnes wrote:
> [Corrected Stefan's address.]

Thanks Jesse! CCing Olaf as well.

> On Sat, 8 Aug 2009 16:45:20 +0200
> Ingo Molnar<mingo@elte.hu>  wrote:
>
>> * Jesse Barnes<jbarnes@virtuousgeek.org>  wrote:
>>
>>> On Tue, 28 Jul 2009 12:50:08 -0700
>>> Jeremy Fitzhardinge<jeremy@goop.org>  wrote:
>>>
>>>> quirk_reroute_to_boot_interrupts_intel() goes to some effort to
>>>> scan for a bunch of Intel devices and set
>>>> dev->irq_reroute_variant = INTEL_IRQ_REROUTE_VARIANT.  However,
>>>> as far as I can see that's the only reference to
>>>> dev->irq_reroute_variant and INTEL_IRQ_REROUTE_VARIANT, so the
>>>> whole thing appears to have no effect.
>>>>
>>>> Is it all dead code?  Is there something else which should be
>>>> there missing?
>>> Hm my quick look couldn't find a user either.  The initial patch
>>> to add that stuff (e1d3a90846b40ad3160bf4b648d36c6badad39ac) had
>>> an ACPI component, but it looks like that may have vanished
>>> somehow (maybe some of the IOAPIC code got shuffled around and
>>> this bit was missed?).
>>>
>>> Stefan or Ingo?
>> I dont remember ioapic interactions. Stefan?
>>
>> 	Ingo
>>

Actually the ACPI part of the commit got trashed by some major ACPI
rewrite. The changes in ACPI code made it non-trivial (as far as I can
see) to manipulate the ACPI IRQ for the relevant devices. I'm not an
ACPI expert and couldn't come up with a solution yet. I'm aware of the
fact that especially the ACPI part was considered arguable by some
people but it was the only place we found to alter the IRQ a driver will
get installed on.
I'd be happy to finally set this right. Any help is greatly appreciated
and I'll try to spend some time with recent ACPI code searching for
possible solutions.

   Stefan
--
Stefan Assmann         | Red Hat GmbH
Software Engineer      | Otto-Hahn-Strasse 20, 85609 Dornach
                        | HR: Amtsgericht Muenchen HRB 153243
                        | GF: Brendan Lane, Charlie Peters,
sassmann at redhat.com |     Michael Cunningham, Charles Cachera

Re: drivers/pci/quirks.c: irq_reroute_variant is dead code?

[Ingo Molnar]

* Stefan Assmann <sassmann@redhat.com> wrote:

> On 10.08.2009 18:32, Jesse Barnes wrote:
>> [Corrected Stefan's address.]
>
> Thanks Jesse! CCing Olaf as well.
>
>> On Sat, 8 Aug 2009 16:45:20 +0200
>> Ingo Molnar<mingo@elte.hu>  wrote:
>>
>>> * Jesse Barnes<jbarnes@virtuousgeek.org>  wrote:
>>>
>>>> On Tue, 28 Jul 2009 12:50:08 -0700
>>>> Jeremy Fitzhardinge<jeremy@goop.org>  wrote:
>>>>
>>>>> quirk_reroute_to_boot_interrupts_intel() goes to some effort to
>>>>> scan for a bunch of Intel devices and set
>>>>> dev->irq_reroute_variant = INTEL_IRQ_REROUTE_VARIANT.  However,
>>>>> as far as I can see that's the only reference to
>>>>> dev->irq_reroute_variant and INTEL_IRQ_REROUTE_VARIANT, so the
>>>>> whole thing appears to have no effect.
>>>>>
>>>>> Is it all dead code?  Is there something else which should be
>>>>> there missing?
>>>> Hm my quick look couldn't find a user either.  The initial patch
>>>> to add that stuff (e1d3a90846b40ad3160bf4b648d36c6badad39ac) had
>>>> an ACPI component, but it looks like that may have vanished
>>>> somehow (maybe some of the IOAPIC code got shuffled around and
>>>> this bit was missed?).
>>>>
>>>> Stefan or Ingo?
>>> I dont remember ioapic interactions. Stefan?
>>>
>>> 	Ingo
>>>
>
> Actually the ACPI part of the commit got trashed by some major 
> ACPI rewrite. [...]

Yeah, i remember some larger reject resolved by Len i think, about 
two cycles ago, in this area. So we probably lost those bits there - 
i've Cc:-ed Len.

There's no urgency in terms of v2.6.31 (since we never really 
exposed this new functionality in a new release), but it would be 
nice to get to the bottom of this.

	Ingo

Re: drivers/pci/quirks.c: irq_reroute_variant is dead code?

[Olaf Dabrunz]

On 11-Aug-09, Ingo Molnar wrote:
> 
> * Stefan Assmann <sassmann@redhat.com> wrote:
> 
> > On 10.08.2009 18:32, Jesse Barnes wrote:
> >> [Corrected Stefan's address.]
> >
> > Thanks Jesse! CCing Olaf as well.
> >
> >> On Sat, 8 Aug 2009 16:45:20 +0200
> >> Ingo Molnar<mingo@elte.hu>  wrote:
> >>
> >>> * Jesse Barnes<jbarnes@virtuousgeek.org>  wrote:
> >>>
> >>>> On Tue, 28 Jul 2009 12:50:08 -0700
> >>>> Jeremy Fitzhardinge<jeremy@goop.org>  wrote:
> >>>>
> >>>>> quirk_reroute_to_boot_interrupts_intel() goes to some effort to
> >>>>> scan for a bunch of Intel devices and set
> >>>>> dev->irq_reroute_variant = INTEL_IRQ_REROUTE_VARIANT.  However,
> >>>>> as far as I can see that's the only reference to
> >>>>> dev->irq_reroute_variant and INTEL_IRQ_REROUTE_VARIANT, so the
> >>>>> whole thing appears to have no effect.
> >>>>>
> >>>>> Is it all dead code?  Is there something else which should be
> >>>>> there missing?
> >>>> Hm my quick look couldn't find a user either.  The initial patch
> >>>> to add that stuff (e1d3a90846b40ad3160bf4b648d36c6badad39ac) had
> >>>> an ACPI component, but it looks like that may have vanished
> >>>> somehow (maybe some of the IOAPIC code got shuffled around and
> >>>> this bit was missed?).
> >>>>
> >>>> Stefan or Ingo?
> >>> I dont remember ioapic interactions. Stefan?
> >>>
> >>> 	Ingo
> >>>
> >
> > Actually the ACPI part of the commit got trashed by some major 
> > ACPI rewrite. [...]
> 
> Yeah, i remember some larger reject resolved by Len i think, about 
> two cycles ago, in this area. So we probably lost those bits there - 
> i've Cc:-ed Len.
> 
> There's no urgency in terms of v2.6.31 (since we never really 
> exposed this new functionality in a new release), but it would be 
> nice to get to the bottom of this.

Yep, that would be nice.

I was made aware of that problem early this year when the ACPI code
change was made. Since then I heard that someone already wrote some
patch for the boot IRQ quirks. But obviously that referred to some other
part of the quirk. Stefan?

As I am currently unemployed, demotivated and trying to get my work on
fctrace going again (as well as to get the rest of my life in order),
it seems I have no time to fix this. I would love to, really. We need to
get these quirks in, and I would like to discuss how to do this The Right
Way with the rewritten ACPI code (and the existing MP code).

Regards,

-- 
Olaf Dabrunz (Olaf.Dabrunz <at> gmx.net)

Re: drivers/pci/quirks.c: irq_reroute_variant is dead code?

[Stefan Assmann]

On 11.08.2009 14:36, Olaf Dabrunz wrote:
> On 11-Aug-09, Ingo Molnar wrote:
>> * Stefan Assmann<sassmann@redhat.com>  wrote:
>>
>>> On 10.08.2009 18:32, Jesse Barnes wrote:
>>>> [Corrected Stefan's address.]
>>> Thanks Jesse! CCing Olaf as well.
>>>
>>>> On Sat, 8 Aug 2009 16:45:20 +0200
>>>> Ingo Molnar<mingo@elte.hu>   wrote:
>>>>
>>>>> * Jesse Barnes<jbarnes@virtuousgeek.org>   wrote:
>>>>>
>>>>>> On Tue, 28 Jul 2009 12:50:08 -0700
>>>>>> Jeremy Fitzhardinge<jeremy@goop.org>   wrote:
>>>>>>
>>>>>>> quirk_reroute_to_boot_interrupts_intel() goes to some effort to
>>>>>>> scan for a bunch of Intel devices and set
>>>>>>> dev->irq_reroute_variant = INTEL_IRQ_REROUTE_VARIANT.  However,
>>>>>>> as far as I can see that's the only reference to
>>>>>>> dev->irq_reroute_variant and INTEL_IRQ_REROUTE_VARIANT, so the
>>>>>>> whole thing appears to have no effect.
>>>>>>>
>>>>>>> Is it all dead code?  Is there something else which should be
>>>>>>> there missing?
>>>>>> Hm my quick look couldn't find a user either.  The initial patch
>>>>>> to add that stuff (e1d3a90846b40ad3160bf4b648d36c6badad39ac) had
>>>>>> an ACPI component, but it looks like that may have vanished
>>>>>> somehow (maybe some of the IOAPIC code got shuffled around and
>>>>>> this bit was missed?).
>>>>>>
>>>>>> Stefan or Ingo?
>>>>> I dont remember ioapic interactions. Stefan?
>>>>>
>>>>> 	Ingo
>>>>>
>>> Actually the ACPI part of the commit got trashed by some major
>>> ACPI rewrite. [...]
>> Yeah, i remember some larger reject resolved by Len i think, about
>> two cycles ago, in this area. So we probably lost those bits there -
>> i've Cc:-ed Len.
>>
>> There's no urgency in terms of v2.6.31 (since we never really
>> exposed this new functionality in a new release), but it would be
>> nice to get to the bottom of this.
>
> Yep, that would be nice.
>
> I was made aware of that problem early this year when the ACPI code
> change was made. Since then I heard that someone already wrote some
> patch for the boot IRQ quirks. But obviously that referred to some other
> part of the quirk. Stefan?

That's a whole different story (other chipset). ;-)

> As I am currently unemployed, demotivated and trying to get my work on
> fctrace going again (as well as to get the rest of my life in order),
> it seems I have no time to fix this. I would love to, really. We need to
> get these quirks in, and I would like to discuss how to do this The Right
> Way with the rewritten ACPI code (and the existing MP code).

Well, best of luck!

   Stefan
--
Stefan Assmann         | Red Hat GmbH
Software Engineer      | Otto-Hahn-Strasse 20, 85609 Dornach
                        | HR: Amtsgericht Muenchen HRB 153243
                        | GF: Brendan Lane, Charlie Peters,
sassmann at redhat.com |     Michael Cunningham, Charles Cachera

Re: drivers/pci/quirks.c: irq_reroute_variant is dead code?

[Stefan Assmann]

On 11.08.2009 10:06, Ingo Molnar wrote:
[snip]
>>>>> On Tue, 28 Jul 2009 12:50:08 -0700
>>>>> Jeremy Fitzhardinge<jeremy@goop.org>  wrote:
>>>>>
>>>>>> quirk_reroute_to_boot_interrupts_intel() goes to some effort to
>>>>>> scan for a bunch of Intel devices and set
>>>>>> dev->irq_reroute_variant = INTEL_IRQ_REROUTE_VARIANT.  However,
>>>>>> as far as I can see that's the only reference to
>>>>>> dev->irq_reroute_variant and INTEL_IRQ_REROUTE_VARIANT, so the
>>>>>> whole thing appears to have no effect.
>>>>>>
>>>>>> Is it all dead code?  Is there something else which should be
>>>>>> there missing?
>>>>> Hm my quick look couldn't find a user either.  The initial patch
>>>>> to add that stuff (e1d3a90846b40ad3160bf4b648d36c6badad39ac) had
>>>>> an ACPI component, but it looks like that may have vanished
>>>>> somehow (maybe some of the IOAPIC code got shuffled around and
>>>>> this bit was missed?).
>>>>>
>>>>> Stefan or Ingo?
>>>> I dont remember ioapic interactions. Stefan?
>>>>
>>>> 	Ingo
>>>>
>>
>> Actually the ACPI part of the commit got trashed by some major 
>> ACPI rewrite. [...]
> 
> Yeah, i remember some larger reject resolved by Len i think, about 
> two cycles ago, in this area. So we probably lost those bits there - 
> i've Cc:-ed Len.
> 
> There's no urgency in terms of v2.6.31 (since we never really 
> exposed this new functionality in a new release), but it would be 
> nice to get to the bottom of this.

sorry it took me a while to come up with a patch. Here's some first
attempt of how we could turn CONFIG_X86_REROUTE_FOR_BROKEN_BOOT_IRQS
into something useful again.

Ccing linux-acpi@vger.kernel.org as this changes ACPI code.

I'm not really sure if the ACPI GSI is always matches the final IRQ
number. From looking at the code it looks like it is but there might
be exceptions. So please let me know of any troubles that this patch
might cause.

I've tested this patch on a machine with 6700PXH PCI Express-to-PCI
Bridge and the machine booted fine. With the patch enabled and
CONFIG_X86_REROUTE_FOR_BROKEN_BOOT_IRQS=y set the NICs IRQ was
successfully moved from IRQ
  48:          0          0      16163        195   IO-APIC-fasteoi   eth0
to
  16:          0          0       4715        237   IO-APIC-fasteoi   uhci_hcd:usb2, uhci_hcd:usb5, eth0

Len, I know that you weren't that pleased by this whole "altering ACPI
information" approach the last time. Please consider it anyway.

  Stefan
---
 drivers/acpi/pci_irq.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -301,6 +301,61 @@ void acpi_pci_irq_del_prt(struct pci_bus
 /* --------------------------------------------------------------------------
                           PCI Interrupt Routing Support
    -------------------------------------------------------------------------- */
+#ifdef CONFIG_X86_IO_APIC
+extern int noioapicquirk;
+extern int noioapicreroute;
+
+static int bridge_has_boot_interrupt_variant(struct pci_bus *bus)
+{
+	struct pci_bus *bus_it;
+
+	for (bus_it = bus ; bus_it ; bus_it = bus_it->parent) {
+		if (!bus_it->self)
+			return 0;
+		if (bus_it->self->irq_reroute_variant)
+			return bus_it->self->irq_reroute_variant;
+	}
+	return 0;
+}
+
+/*
+ * Some chipsets (e.g. Intel 6700PXH) generate a legacy INTx when the IRQ
+ * entry in the chipset's IO-APIC is masked (as, e.g. the RT kernel does
+ * during interrupt handling). When this INTx generation cannot be disabled,
+ * we reroute these interrupts to their legacy equivalent to get rid of
+ * spurious interrupts.
+ */
+static int acpi_reroute_boot_interrupt(struct pci_dev *dev,
+				       struct acpi_prt_entry *entry)
+{
+	if (noioapicquirk || noioapicreroute) {
+		return 0;
+	} else {
+		switch (bridge_has_boot_interrupt_variant(dev->bus)) {
+		case 0:
+			/* no rerouting necessary */
+			return 0;
+		case INTEL_IRQ_REROUTE_VARIANT:
+			/*
+			 * Remap according to INTx routing table in 6700PXH
+			 * specs, intel order number 302628-002, section
+			 * 2.15.2. Other chipsets (80332, ...) have the same
+			 * mapping and are handled here as well.
+			 */
+			dev_info(&dev->dev, "PCI IRQ %d -> rerouted to legacy "
+				 "IRQ %d\n", entry->index,
+				 (entry->index % 4) + 16);
+			entry->index = (entry->index % 4) + 16;
+			return 1;
+		default:
+			dev_warn(&dev->dev, "Cannot reroute IRQ %d to legacy "
+				 "IRQ: unknown mapping\n", entry->index);
+			return -1;
+		}
+	}
+}
+#endif /* CONFIG_X86_IO_APIC */
+
 static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin)
 {
 	struct acpi_prt_entry *entry;
@@ -309,6 +364,9 @@ static struct acpi_prt_entry *acpi_pci_i

 	entry = acpi_pci_irq_find_prt_entry(dev, pin);
 	if (entry) {
+#ifdef CONFIG_X86_IO_APIC
+		acpi_reroute_boot_interrupt(dev, entry);
+#endif /* CONFIG_X86_IO_APIC */
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found %s[%c] _PRT entry\n",
 				  pci_name(dev), pin_name(pin)));
 		return entry;

--
Stefan Assmann         | Red Hat GmbH
Software Engineer      | Otto-Hahn-Strasse 20, 85609 Dornach
                       | HR: Amtsgericht Muenchen HRB 153243
                       | GF: Brendan Lane, Charlie Peters,
sassmann at redhat.com |     Michael Cunningham, Charles Cachera