[PATCH] acpi: reintroduce LNKS

[Paolo Bonzini]

Commit 4540409 (acpi: LNKS is not needed, 2012-08-07) removed LNKS because
it basically worked by chance: _CRS returns something else then one of the
possible resources from _PRS, _DIS would not really disable the interrupt,
and there was no _SRS method.  It just happened to work because all OSes
have some kind of special-casing for SCI.

Unfortunately, the code after the patch is also against the spec, and it
breaks FreeBSD because it treats IRQ 9 polarity as active low without
the Interrupt() entry.  Actually, numeric _PRT entries are handled the
same in Linux and FreeBSD (as active-low).  However, under Linux it just
happens to trigger another special casing of SCI which sets SCI up from
its override entry in the MADT, ignoring the DSDT completely.

This patch adds back the LNKS, but without using the PIIX register for
LNKA in its methods.

Tested-by: Luigi Rizzo 
Signed-off-by: Paolo Bonzini 
---
 src/acpi-dsdt.dsl | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index 8019b71..b58ef62 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -187,7 +187,7 @@ DefinitionBlock (

prt_slot0(0x0000),
                 /* Device 1 is power mgmt device, and can only use irq 9 */
-                Package() { 0x1ffff, 0,    0, 9 },
+                Package() { 0x1ffff, 0, LNKS, 0 },
                 Package() { 0x1ffff, 1, LNKB, 0 },
                 Package() { 0x1ffff, 2, LNKC, 0 },
                 Package() { 0x1ffff, 3, LNKD, 0 },
@@ -278,6 +278,22 @@ DefinitionBlock (
         define_link(LNKB, 1, PRQ1)
         define_link(LNKC, 2, PRQ2)
         define_link(LNKD, 3, PRQ3)
+
+        Device(LNKS) {
+            Name(_HID, EISAID("PNP0C0F"))
+            Name(_UID, 4)
+            Name(_PRS, ResourceTemplate() {
+                Interrupt(, Level, ActiveHigh, Shared) { 9 }
+            })
+
+            // The SCI cannot be disabled and is always attached to GSI 9,
+            // so these are no-ops.  We only need this link to override the
+            // polarity to active high and match the content of the MADT.
+            Method(_STA, 0, NotSerialized) { Return (0x0b) }
+            Method(_DIS, 0, NotSerialized) { }
+            Method(_CRS, 0, NotSerialized) { Return (_PRS) }
+            Method(_SRS, 1, NotSerialized) { }
+        }
     }

#include "acpi-dsdt-cpu-hotplug.dsl"

[Laszlo Ersek]

On 12/13/12 08:42, Paolo Bonzini wrote:
> Commit 4540409 (acpi: LNKS is not needed, 2012-08-07) removed LNKS because
> it basically worked by chance: _CRS returns something else then one of the

^
("than")
> possible resources from _PRS, _DIS would not really disable the interrupt,
> and there was no _SRS method.  It just happened to work because all OSes
> have some kind of special-casing for SCI.
>
> Unfortunately, the code after the patch is also against the spec, and it
> breaks FreeBSD because it treats IRQ 9 polarity as active low without
> the Interrupt() entry.  Actually, numeric _PRT entries are handled the
> same in Linux and FreeBSD (as active-low).  However, under Linux it just
> happens to trigger another special casing of SCI which sets SCI up from
> its override entry in the MADT, ignoring the DSDT completely.

I won't pretend I understand what I'm talking about, but the ACPI spec
5.0 says in "5.2.12.5 Interrupt Source Override Structure",

Interrupt Source Overrides are also necessary when an identity
    mapped interrupt input has a non-standard polarity.

Hence "necessary but not sufficient", is that it?

OTOH, one might argue that the spec requires this override (if
appropriate) because it expects OSPM to look at it, and to base
decisions on it. There's also a

Note: You must have an interrupt source override entry for the IRQ
    mapped to the SCI interrupt if this IRQ is not identity mapped.
    This entry will override the value in SCI_INT in FADT. [...]

SCI_INT in the FADT is explained as

[...] OSPM is required to treat the ACPI SCI interrupt as a
    sharable, level, active low interrupt.

which is then overridden in the MADT, stating active-high polarity.

... I think the (current) Linux functions you mean are in
"arch/x86/kernel/acpi":

acpi_parse_int_src_ovr()
  acpi_sci_ioapic_setup()

The bit that fetches the polarity from the override dates back to 2004:

- https://bugzilla.kernel.org/show_bug.cgi?id=1622
-
http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commitdiff;h=e1...

How does FreeBSD handle the override?
> This patch adds back the LNKS, but without using the PIIX register for
> LNKA in its methods.
>
> Tested-by: Luigi Rizzo 
> Signed-off-by: Paolo Bonzini
>
> src/acpi-dsdt.dsl | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
> index 8019b71..b58ef62 100644
> --- a/src/acpi-dsdt.dsl
> +++ b/src/acpi-dsdt.dsl
> @@ -187,7 +187,7 @@ DefinitionBlock (
>              prt_slot0(0x0000),
>              /* Device 1 is power mgmt device, and can only use irq 9 */
>            Package() { 0x1ffff, 0,    0, 9 },
>            Package() { 0x1ffff, 0, LNKS, 0 },
>            Package() { 0x1ffff, 1, LNKB, 0 },
>            Package() { 0x1ffff, 2, LNKC, 0 },
>            Package() { 0x1ffff, 3, LNKD, 0 },

Would it be possible to restore the prt_slot() macro here that was
removed by commit 4540409? The new code seems to match what the macro
would expand to.
> @@ -278,6 +278,22 @@ DefinitionBlock (
>          define_link(LNKB, 1, PRQ1)
>          define_link(LNKC, 2, PRQ2)
>          define_link(LNKD, 3, PRQ3)
>    Device(LNKS) {
>        Name(_HID, EISAID("PNP0C0F"))
>        Name(_UID, 4)
>        Name(_PRS, ResourceTemplate() {
>            Interrupt(, Level, ActiveHigh, Shared) { 9 }
>        })
>        // The SCI cannot be disabled and is always attached to GSI 9,
>        // so these are no-ops.  We only need this link to override the
>        // polarity to active high and match the content of the MADT.
>        Method(_STA, 0, NotSerialized) { Return (0x0b) }
>        Method(_DIS, 0, NotSerialized) { }
>        Method(_CRS, 0, NotSerialized) { Return (_PRS) }
>        Method(_SRS, 1, NotSerialized) { }
>    }
> }
>
> #include "acpi-dsdt-cpu-hotplug.dsl"

Reviewed-by: Laszlo Ersek

although I'm not sure FreeBSD does the right thing.

[Paolo Bonzini]

Il 13/12/2012 14:33, Laszlo Ersek ha scritto:
> > > Unfortunately, the code after the patch is also against the spec, and it
> > > breaks FreeBSD because it treats IRQ 9 polarity as active low without
> > > the Interrupt() entry.  Actually, numeric _PRT entries are handled the
> > > same in Linux and FreeBSD (as active-low).  However, under Linux it just
> > > happens to trigger another special casing of SCI which sets SCI up from
> > > its override entry in the MADT, ignoring the DSDT completely.
>
> I won't pretend I understand what I'm talking about, but the ACPI spec
> 5.0 says in "5.2.12.5 Interrupt Source Override Structure",
> Interrupt Source Overrides are also necessary when an identity
> mapped interrupt input has a non-standard polarity.
>
> Hence "necessary but not sufficient", is that it?

The MADT is about 8259 pins, while the _PRT entry identifies a GSI.  So
we have the same GSI (9) specified twice.  Linux ignores the settings of
the second entry and reuses those that came from the GSI.  The important
bit here is this:

/* Don't set up the ACPI SCI because it's already set up */
        if (acpi_gbl_FADT.sci_interrupt == gsi)
                return gsi;

(And as you can see it's wrong, sci_interrupt is an 8259 interrupt not a
GSI).
> SCI_INT in the FADT is explained as
> [...] OSPM is required to treat the ACPI SCI interrupt as a
> sharable, level, active low interrupt.
>
> which is then overridden in the MADT, stating active-high polarity.

Yes, but this doesn't affect the definition of this GSI in the _PRT.  It
is always level/active-low for a numeric entry.  Among the two
conflicting choices, Linux happens to favor the MADT.  FreeBSD doesn't.

Paolo

[Kevin O'Connor]

On Thu, Dec 13, 2012 at 08:42:02AM +0100, Paolo Bonzini wrote:
> Commit 4540409 (acpi: LNKS is not needed, 2012-08-07) removed LNKS because
> it basically worked by chance: _CRS returns something else then one of the
> possible resources from _PRS, _DIS would not really disable the interrupt,
> and there was no _SRS method.  It just happened to work because all OSes
> have some kind of special-casing for SCI.
>
> Unfortunately, the code after the patch is also against the spec, and it
> breaks FreeBSD because it treats IRQ 9 polarity as active low without
> the Interrupt() entry.  Actually, numeric _PRT entries are handled the
> same in Linux and FreeBSD (as active-low).  However, under Linux it just
> happens to trigger another special casing of SCI which sets SCI up from
> its override entry in the MADT, ignoring the DSDT completely.
>
> This patch adds back the LNKS, but without using the PIIX register for
> LNKA in its methods.

Thanks - looks okay to me.  The prt_slot() macro should probably be
used.

-Kevin

[Kevin O'Connor]

On Thu, Dec 13, 2012 at 08:42:02AM +0100, Paolo Bonzini wrote:
> Commit 4540409 (acpi: LNKS is not needed, 2012-08-07) removed LNKS because
> it basically worked by chance: _CRS returns something else then one of the
> possible resources from _PRS, _DIS would not really disable the interrupt,
> and there was no _SRS method.  It just happened to work because all OSes
> have some kind of special-casing for SCI.
>
> Unfortunately, the code after the patch is also against the spec, and it
> breaks FreeBSD because it treats IRQ 9 polarity as active low without
> the Interrupt() entry.  Actually, numeric _PRT entries are handled the
> same in Linux and FreeBSD (as active-low).  However, under Linux it just
> happens to trigger another special casing of SCI which sets SCI up from
> its override entry in the MADT, ignoring the DSDT completely.
>
> This patch adds back the LNKS, but without using the PIIX register for
> LNKA in its methods.

Thanks.  I committed this change, along with a follow up patch to use
the prt_slot() macro.

-Kevin