LinuxLists.cc - [Patch v1] x86, ACPI, irq: Fix a regression caused by

2015-08-09 08:55:21

Subject: [Patch v1] x86, ACPI, irq: Fix a regression caused by

2015-08-18 19:36:45

Subject: Re: [Patch v1] x86, ACPI, irq: Fix a regression caused by

On Sun, 9 Aug 2015, Jiang Liu wrote:
> Nick Meier reported a regression with HyperV that "
> After rebooting the VM, the following messages are logged in syslog
> when trying to load the tulip driver:
> tulip: Linux Tulip drivers version 1.1.15 (Feb 27, 2007)
> tulip: 0000:00:0a.0: PCI INT A: failed to register GSI
> tulip: Cannot enable tulip board #0, aborting
> tulip: probe of 0000:00:0a.0 failed with error -16
> Errors occur in 3.19.0 kernel
> Works in 3.17 kernel.
> "
>
> According to the ACPI dump file posted by Nick at
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1440072
>
> The ACPI MADT table includes an interrupt source overridden entry for
> ACPI SCI:
> [236h 0566 1] Subtable Type : 02 <Interrupt Source Override>
> [237h 0567 1] Length : 0A
> [238h 0568 1] Bus : 00
> [239h 0569 1] Source : 09
> [23Ah 0570 4] Interrupt : 00000009
> [23Eh 0574 2] Flags (decoded below) : 000D
> Polarity : 1
> Trigger Mode : 3
> That means ACPI SCI interrupt(Interrupt : 00000009) works in
> level(Trigger Mode : 3), high(Polarity : 1) mode.
>
> And in DSDT table, we have _PRT method to define PCI interrupts, which
> eventually goes to:
> Name (PRSA, ResourceTemplate ()
> {
> IRQ (Level, ActiveLow, Shared, )
> {3,4,5,7,9,10,11,12,14,15}
> })
> Name (PRSB, ResourceTemplate ()
> {
> IRQ (Level, ActiveLow, Shared, )
> {3,4,5,7,9,10,11,12,14,15}
> })
> Name (PRSC, ResourceTemplate ()
> {
> IRQ (Level, ActiveLow, Shared, )
> {3,4,5,7,9,10,11,12,14,15}
> })
> Name (PRSD, ResourceTemplate ()
> {
> IRQ (Level, ActiveLow, Shared, )
> {3,4,5,7,9,10,11,12,14,15}
> })
> which means it's also possible to use IRQ9 for PCI interrupt, but works
> in Level, ActiveLow mode. So it conflicts with ACPI SCI interrupt source
> overriddern.
>
> So implement a quirk to correct interrupt attribute for HyperV SCI
> interrupt. Nick reports the proposed patch fixes the regression as "
> Applied the above proposed patch with the DMI values substituted.
> The tulip driver loaded, and an address was assigned via DHCP.
> "
> Please refer to following links for more information:
> https://bugzilla.kernel.org/show_bug.cgi?id=101301
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1440072
>
> Fixes: cd68f6bd53cf ("x86, irq, acpi: Get rid of special handling of GSI for ACPI SCI")

I have a hard time to understand WHY that particular patch actually
caused that regression, WHY the original code worked with this weird
ACPI table and WHY we are better off with a quirk for this case.

Can you please elaborate?

Thanks,

tglx

2015-08-19 05:50:47

by Jiang Liu

[permalink] [raw]

Subject: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV

Nick Meier reported a regression with HyperV that "
After rebooting the VM, the following messages are logged in syslog
when trying to load the tulip driver:
tulip: Linux Tulip drivers version 1.1.15 (Feb 27, 2007)
tulip: 0000:00:0a.0: PCI INT A: failed to register GSI
tulip: Cannot enable tulip board #0, aborting
tulip: probe of 0000:00:0a.0 failed with error -16
Errors occur in 3.19.0 kernel
Works in 3.17 kernel.
"

According to the ACPI dump file posted by Nick at
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1440072

The ACPI MADT table includes an interrupt source overridden entry for
ACPI SCI:
[236h 0566 1] Subtable Type : 02 <Interrupt Source Override>
[237h 0567 1] Length : 0A
[238h 0568 1] Bus : 00
[239h 0569 1] Source : 09
[23Ah 0570 4] Interrupt : 00000009
[23Eh 0574 2] Flags (decoded below) : 000D
Polarity : 1
Trigger Mode : 3

And in DSDT table, we have _PRT method to define PCI interrupts, which
eventually goes to:
Name (PRSA, ResourceTemplate ()
{
IRQ (Level, ActiveLow, Shared, )
{3,4,5,7,9,10,11,12,14,15}
})
Name (PRSB, ResourceTemplate ()
{
IRQ (Level, ActiveLow, Shared, )
{3,4,5,7,9,10,11,12,14,15}
})
Name (PRSC, ResourceTemplate ()
{
IRQ (Level, ActiveLow, Shared, )
{3,4,5,7,9,10,11,12,14,15}
})
Name (PRSD, ResourceTemplate ()
{
IRQ (Level, ActiveLow, Shared, )
{3,4,5,7,9,10,11,12,14,15}
})

According to the MADT and DSDT tables above, IRQ 9 may be used for:
1) ACPI SCI in level, high mode
2) PCI legacy IRQ in level, low mode
So there's a possible conflict in polarity setting for IRQ 9.

Prior to commit cd68f6bd53cf ("x86, irq, acpi: Get rid of special
handling of GSI for ACPI SCI"), ACPI SCI is handled specially and
there's no check for conflicts between ACPI SCI and PCI legagy IRQ.
And it seems that the HyperV hypervisor doesn't make use of the
polarity configuration in IOAPIC entry, so it just works.

Commit cd68f6bd53cf gets rid of the specially handling of ACPI SCI,
and then the pin attribute checking logic discloses the conflicts
between ACPI SCI and PCI legacy IRQ on HyperV virtual machine,
and rejects the request to assign IRQ9 to PCI devices.

Since HyperV doesn't make use of the IOAPIC polarity setting,
add a quirk to enforce ACPI SCI as level, low on HyperV guests,
so IRQ9 could be used for both ACPI SCI and PCI legacy IRQ.

Nick reports the proposed patch fixes the regression as "
Applied the above proposed patch with the DMI values substituted.
The tulip driver loaded, and an address was assigned via DHCP.
"
Please refer to following links for more information:
https://bugzilla.kernel.org/show_bug.cgi?id=101301
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1440072

Fixes: cd68f6bd53cf ("x86, irq, acpi: Get rid of special handling of GSI for ACPI SCI")
Reported-and-tested-by: Nick Meier <[email protected]>
Cc: <[email protected]> # 3.19
Signed-off-by: Jiang Liu <[email protected]>
---
Hi Thomas,
I have refined the commit message to explain why it works with
the old code.
Thanks!
Gerry
---
arch/x86/kernel/acpi/boot.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index e49ee24da85e..47d95a86d56d 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1308,6 +1308,13 @@ static int __init dmi_ignore_irq0_timer_override(const struct dmi_system_id *d)
return 0;
}

+static int __init acpi_force_hyperv_sci_attr(const struct dmi_system_id *d)
+{
+ acpi_sci_flags = ACPI_MADT_POLARITY_ACTIVE_LOW |
+ (acpi_sci_flags & ~ACPI_MADT_POLARITY_MASK);
+ return 0;
+}
+
/*
* ACPI offers an alternative platform interface model that removes
* ACPI hardware requirements for platforms that do not implement
@@ -1458,6 +1465,14 @@ static struct dmi_system_id __initdata acpi_dmi_table_late[] = {
DMI_MATCH(DMI_PRODUCT_NAME, "AMILO PRO V2030"),
},
},
+ {
+ .callback = acpi_force_hyperv_sci_attr,
+ .ident = "HyperV",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
+ },
+ },
{}
};

--
1.7.10.4

2015-08-19 06:04:44

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV

Hi,

On Wed, Aug 19, 2015 at 7:53 AM, Jiang Liu <[email protected]> wrote:
> Nick Meier reported a regression with HyperV that "
> After rebooting the VM, the following messages are logged in syslog
> when trying to load the tulip driver:
> tulip: Linux Tulip drivers version 1.1.15 (Feb 27, 2007)
> tulip: 0000:00:0a.0: PCI INT A: failed to register GSI
> tulip: Cannot enable tulip board #0, aborting
> tulip: probe of 0000:00:0a.0 failed with error -16
> Errors occur in 3.19.0 kernel
> Works in 3.17 kernel.
> "
>
> According to the ACPI dump file posted by Nick at
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1440072
>
> The ACPI MADT table includes an interrupt source overridden entry for
> ACPI SCI:
> [236h 0566 1] Subtable Type : 02 <Interrupt Source Override>
> [237h 0567 1] Length : 0A
> [238h 0568 1] Bus : 00
> [239h 0569 1] Source : 09
> [23Ah 0570 4] Interrupt : 00000009
> [23Eh 0574 2] Flags (decoded below) : 000D
> Polarity : 1
> Trigger Mode : 3
>
> And in DSDT table, we have _PRT method to define PCI interrupts, which
> eventually goes to:
> Name (PRSA, ResourceTemplate ()
> {
> IRQ (Level, ActiveLow, Shared, )
> {3,4,5,7,9,10,11,12,14,15}
> })
> Name (PRSB, ResourceTemplate ()
> {
> IRQ (Level, ActiveLow, Shared, )
> {3,4,5,7,9,10,11,12,14,15}
> })
> Name (PRSC, ResourceTemplate ()
> {
> IRQ (Level, ActiveLow, Shared, )
> {3,4,5,7,9,10,11,12,14,15}
> })
> Name (PRSD, ResourceTemplate ()
> {
> IRQ (Level, ActiveLow, Shared, )
> {3,4,5,7,9,10,11,12,14,15}
> })
>
> According to the MADT and DSDT tables above, IRQ 9 may be used for:
> 1) ACPI SCI in level, high mode
> 2) PCI legacy IRQ in level, low mode
> So there's a possible conflict in polarity setting for IRQ 9.
>
> Prior to commit cd68f6bd53cf ("x86, irq, acpi: Get rid of special
> handling of GSI for ACPI SCI"), ACPI SCI is handled specially and
> there's no check for conflicts between ACPI SCI and PCI legagy IRQ.
> And it seems that the HyperV hypervisor doesn't make use of the
> polarity configuration in IOAPIC entry, so it just works.

That likely means the previous behavior was the same as (or at least
analogous to) what happens on Windows which is what the firmware has
been tested against and that's why the bug in it has not been caught.
That in turn indicates that there may be more systems having this kind
of problems, possibly from other vendors too, so I'm wondering if we
can do something more generic than using a quirk here?

Rafael

2015-08-19 06:26:32

by Jiang Liu

[permalink] [raw]

Subject: Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV

On 2015/8/19 14:04, Rafael J. Wysocki wrote:
> Hi,
>
> On Wed, Aug 19, 2015 at 7:53 AM, Jiang Liu <[email protected]> wrote:
>> Nick Meier reported a regression with HyperV that "
>> After rebooting the VM, the following messages are logged in syslog
>> when trying to load the tulip driver:
>> tulip: Linux Tulip drivers version 1.1.15 (Feb 27, 2007)
>> tulip: 0000:00:0a.0: PCI INT A: failed to register GSI
>> tulip: Cannot enable tulip board #0, aborting
>> tulip: probe of 0000:00:0a.0 failed with error -16
>> Errors occur in 3.19.0 kernel
>> Works in 3.17 kernel.
>> "
>>
>> According to the ACPI dump file posted by Nick at
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1440072
>>
>> The ACPI MADT table includes an interrupt source overridden entry for
>> ACPI SCI:
>> [236h 0566 1] Subtable Type : 02 <Interrupt Source Override>
>> [237h 0567 1] Length : 0A
>> [238h 0568 1] Bus : 00
>> [239h 0569 1] Source : 09
>> [23Ah 0570 4] Interrupt : 00000009
>> [23Eh 0574 2] Flags (decoded below) : 000D
>> Polarity : 1
>> Trigger Mode : 3
>>
>> And in DSDT table, we have _PRT method to define PCI interrupts, which
>> eventually goes to:
>> Name (PRSA, ResourceTemplate ()
>> {
>> IRQ (Level, ActiveLow, Shared, )
>> {3,4,5,7,9,10,11,12,14,15}
>> })
>> Name (PRSB, ResourceTemplate ()
>> {
>> IRQ (Level, ActiveLow, Shared, )
>> {3,4,5,7,9,10,11,12,14,15}
>> })
>> Name (PRSC, ResourceTemplate ()
>> {
>> IRQ (Level, ActiveLow, Shared, )
>> {3,4,5,7,9,10,11,12,14,15}
>> })
>> Name (PRSD, ResourceTemplate ()
>> {
>> IRQ (Level, ActiveLow, Shared, )
>> {3,4,5,7,9,10,11,12,14,15}
>> })
>>
>> According to the MADT and DSDT tables above, IRQ 9 may be used for:
>> 1) ACPI SCI in level, high mode
>> 2) PCI legacy IRQ in level, low mode
>> So there's a possible conflict in polarity setting for IRQ 9.
>>
>> Prior to commit cd68f6bd53cf ("x86, irq, acpi: Get rid of special
>> handling of GSI for ACPI SCI"), ACPI SCI is handled specially and
>> there's no check for conflicts between ACPI SCI and PCI legagy IRQ.
>> And it seems that the HyperV hypervisor doesn't make use of the
>> polarity configuration in IOAPIC entry, so it just works.
>
> That likely means the previous behavior was the same as (or at least
> analogous to) what happens on Windows which is what the firmware has
> been tested against and that's why the bug in it has not been caught.
> That in turn indicates that there may be more systems having this kind
> of problems, possibly from other vendors too, so I'm wondering if we
> can do something more generic than using a quirk here?
Hi Rafael,
Good question. I was thought only virtual machines may generate
such fake ACPI tables with conflicting pin attribute settings. For bare
metal systems, I guess BIOS writers should report the correct hardware
configurations, otherwise wrong pin attributes may cause hardware
malfunction.
If that's not the case, we may try to revert cd68f6bd53cf
instead.
Thanks!
Gerry

2015-08-19 06:45:36

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV

Hi,

On Wed, Aug 19, 2015 at 8:26 AM, Jiang Liu <[email protected]> wrote:
> On 2015/8/19 14:04, Rafael J. Wysocki wrote:
>> Hi,
>>
>> On Wed, Aug 19, 2015 at 7:53 AM, Jiang Liu <[email protected]> wrote:
>>> Nick Meier reported a regression with HyperV that "
>>> After rebooting the VM, the following messages are logged in syslog
>>> when trying to load the tulip driver:
>>> tulip: Linux Tulip drivers version 1.1.15 (Feb 27, 2007)
>>> tulip: 0000:00:0a.0: PCI INT A: failed to register GSI
>>> tulip: Cannot enable tulip board #0, aborting
>>> tulip: probe of 0000:00:0a.0 failed with error -16
>>> Errors occur in 3.19.0 kernel
>>> Works in 3.17 kernel.
>>> "
>>>
>>> According to the ACPI dump file posted by Nick at
>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1440072
>>>
>>> The ACPI MADT table includes an interrupt source overridden entry for
>>> ACPI SCI:
>>> [236h 0566 1] Subtable Type : 02 <Interrupt Source Override>
>>> [237h 0567 1] Length : 0A
>>> [238h 0568 1] Bus : 00
>>> [239h 0569 1] Source : 09
>>> [23Ah 0570 4] Interrupt : 00000009
>>> [23Eh 0574 2] Flags (decoded below) : 000D
>>> Polarity : 1
>>> Trigger Mode : 3
>>>
>>> And in DSDT table, we have _PRT method to define PCI interrupts, which
>>> eventually goes to:
>>> Name (PRSA, ResourceTemplate ()
>>> {
>>> IRQ (Level, ActiveLow, Shared, )
>>> {3,4,5,7,9,10,11,12,14,15}
>>> })
>>> Name (PRSB, ResourceTemplate ()
>>> {
>>> IRQ (Level, ActiveLow, Shared, )
>>> {3,4,5,7,9,10,11,12,14,15}
>>> })
>>> Name (PRSC, ResourceTemplate ()
>>> {
>>> IRQ (Level, ActiveLow, Shared, )
>>> {3,4,5,7,9,10,11,12,14,15}
>>> })
>>> Name (PRSD, ResourceTemplate ()
>>> {
>>> IRQ (Level, ActiveLow, Shared, )
>>> {3,4,5,7,9,10,11,12,14,15}
>>> })
>>>
>>> According to the MADT and DSDT tables above, IRQ 9 may be used for:
>>> 1) ACPI SCI in level, high mode
>>> 2) PCI legacy IRQ in level, low mode
>>> So there's a possible conflict in polarity setting for IRQ 9.
>>>
>>> Prior to commit cd68f6bd53cf ("x86, irq, acpi: Get rid of special
>>> handling of GSI for ACPI SCI"), ACPI SCI is handled specially and
>>> there's no check for conflicts between ACPI SCI and PCI legagy IRQ.
>>> And it seems that the HyperV hypervisor doesn't make use of the
>>> polarity configuration in IOAPIC entry, so it just works.
>>
>> That likely means the previous behavior was the same as (or at least
>> analogous to) what happens on Windows which is what the firmware has
>> been tested against and that's why the bug in it has not been caught.
>> That in turn indicates that there may be more systems having this kind
>> of problems, possibly from other vendors too, so I'm wondering if we
>> can do something more generic than using a quirk here?
> Hi Rafael,
> Good question. I was thought only virtual machines may generate
> such fake ACPI tables with conflicting pin attribute settings. For bare
> metal systems, I guess BIOS writers should report the correct hardware
> configurations, otherwise wrong pin attributes may cause hardware
> malfunction.
> If that's not the case, we may try to revert cd68f6bd53cf

Well, the regression at hand has just shown that the assertion in the
changelog of that commit ("no need for for special treatment for GSI
used by ACPI SCI") does not really hold. So, if the only motivation
for it was to get rid of one extra check in mp_unregister_gsi()
(mp_register_gsi() still needs to check if it is dealing with the SCI
anyway), I'd vote for reverting it.

Thanks,
Rafael

2015-08-19 06:53:34

by Jiang Liu

[permalink] [raw]

Subject: Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV

On 2015/8/19 14:45, Rafael J. Wysocki wrote:
> Hi,
>
> On Wed, Aug 19, 2015 at 8:26 AM, Jiang Liu <[email protected]> wrote:
>> On 2015/8/19 14:04, Rafael J. Wysocki wrote:
>>> Hi,
>>>
>>> On Wed, Aug 19, 2015 at 7:53 AM, Jiang Liu <[email protected]> wrote:
>>>> Nick Meier reported a regression with HyperV that "
>>>> After rebooting the VM, the following messages are logged in syslog
>>>> when trying to load the tulip driver:
>>>> tulip: Linux Tulip drivers version 1.1.15 (Feb 27, 2007)
>>>> tulip: 0000:00:0a.0: PCI INT A: failed to register GSI
>>>> tulip: Cannot enable tulip board #0, aborting
>>>> tulip: probe of 0000:00:0a.0 failed with error -16
>>>> Errors occur in 3.19.0 kernel
>>>> Works in 3.17 kernel.
>>>> "
>>>>
>>>> According to the ACPI dump file posted by Nick at
>>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1440072
>>>>
>>>> The ACPI MADT table includes an interrupt source overridden entry for
>>>> ACPI SCI:
>>>> [236h 0566 1] Subtable Type : 02 <Interrupt Source Override>
>>>> [237h 0567 1] Length : 0A
>>>> [238h 0568 1] Bus : 00
>>>> [239h 0569 1] Source : 09
>>>> [23Ah 0570 4] Interrupt : 00000009
>>>> [23Eh 0574 2] Flags (decoded below) : 000D
>>>> Polarity : 1
>>>> Trigger Mode : 3
>>>>
>>>> And in DSDT table, we have _PRT method to define PCI interrupts, which
>>>> eventually goes to:
>>>> Name (PRSA, ResourceTemplate ()
>>>> {
>>>> IRQ (Level, ActiveLow, Shared, )
>>>> {3,4,5,7,9,10,11,12,14,15}
>>>> })
>>>> Name (PRSB, ResourceTemplate ()
>>>> {
>>>> IRQ (Level, ActiveLow, Shared, )
>>>> {3,4,5,7,9,10,11,12,14,15}
>>>> })
>>>> Name (PRSC, ResourceTemplate ()
>>>> {
>>>> IRQ (Level, ActiveLow, Shared, )
>>>> {3,4,5,7,9,10,11,12,14,15}
>>>> })
>>>> Name (PRSD, ResourceTemplate ()
>>>> {
>>>> IRQ (Level, ActiveLow, Shared, )
>>>> {3,4,5,7,9,10,11,12,14,15}
>>>> })
>>>>
>>>> According to the MADT and DSDT tables above, IRQ 9 may be used for:
>>>> 1) ACPI SCI in level, high mode
>>>> 2) PCI legacy IRQ in level, low mode
>>>> So there's a possible conflict in polarity setting for IRQ 9.
>>>>
>>>> Prior to commit cd68f6bd53cf ("x86, irq, acpi: Get rid of special
>>>> handling of GSI for ACPI SCI"), ACPI SCI is handled specially and
>>>> there's no check for conflicts between ACPI SCI and PCI legagy IRQ.
>>>> And it seems that the HyperV hypervisor doesn't make use of the
>>>> polarity configuration in IOAPIC entry, so it just works.
>>>
>>> That likely means the previous behavior was the same as (or at least
>>> analogous to) what happens on Windows which is what the firmware has
>>> been tested against and that's why the bug in it has not been caught.
>>> That in turn indicates that there may be more systems having this kind
>>> of problems, possibly from other vendors too, so I'm wondering if we
>>> can do something more generic than using a quirk here?
>> Hi Rafael,
>> Good question. I was thought only virtual machines may generate
>> such fake ACPI tables with conflicting pin attribute settings. For bare
>> metal systems, I guess BIOS writers should report the correct hardware
>> configurations, otherwise wrong pin attributes may cause hardware
>> malfunction.
>> If that's not the case, we may try to revert cd68f6bd53cf
>
> Well, the regression at hand has just shown that the assertion in the
> changelog of that commit ("no need for for special treatment for GSI
> used by ACPI SCI") does not really hold. So, if the only motivation
> for it was to get rid of one extra check in mp_unregister_gsi()
> (mp_register_gsi() still needs to check if it is dealing with the SCI
> anyway), I'd vote for reverting it.
Hi Rafael,
The motivation is to treat SCI as normal IOAPIC interrupt so
we could enforce stricter pin attribute checking. Now it does reveal
flaws in ACPI BIOS implementations, but we ran into trouble about how to
handle those flaws:(
Thanks!
Gerry
ACPI bios with flaws:(
>
> Thanks,
> Rafael
>

2015-08-19 08:29:45

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV

On Wed, 19 Aug 2015, Jiang Liu wrote:
> On 2015/8/19 14:45, Rafael J. Wysocki wrote:
> > Well, the regression at hand has just shown that the assertion in the
> > changelog of that commit ("no need for for special treatment for GSI
> > used by ACPI SCI") does not really hold. So, if the only motivation
> > for it was to get rid of one extra check in mp_unregister_gsi()
> > (mp_register_gsi() still needs to check if it is dealing with the SCI
> > anyway), I'd vote for reverting it.
> Hi Rafael,
> The motivation is to treat SCI as normal IOAPIC interrupt so
> we could enforce stricter pin attribute checking. Now it does reveal
> flaws in ACPI BIOS implementations, but we ran into trouble about how to
> handle those flaws:(

The intent of this change is entirely correct, though it seems that
reality of ACPI is just different.

To be on the safe side of things, I agree with Rafael that we should
revert that patch instead of introducing a single platform quirk.

What we can do is to emit a pr_warn() if we detect inconsistency of
the SCI configuration. That way we might be able to gather information
how wide spread this disease is.

Thanks,

tglx

2015-08-19 08:40:59

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV

On Wed, 19 Aug 2015, Thomas Gleixner wrote:
> On Wed, 19 Aug 2015, Jiang Liu wrote:
> > On 2015/8/19 14:45, Rafael J. Wysocki wrote:
> > > Well, the regression at hand has just shown that the assertion in the
> > > changelog of that commit ("no need for for special treatment for GSI
> > > used by ACPI SCI") does not really hold. So, if the only motivation
> > > for it was to get rid of one extra check in mp_unregister_gsi()
> > > (mp_register_gsi() still needs to check if it is dealing with the SCI
> > > anyway), I'd vote for reverting it.
> > Hi Rafael,
> > The motivation is to treat SCI as normal IOAPIC interrupt so
> > we could enforce stricter pin attribute checking. Now it does reveal
> > flaws in ACPI BIOS implementations, but we ran into trouble about how to
> > handle those flaws:(
>
> The intent of this change is entirely correct, though it seems that
> reality of ACPI is just different.
>
> To be on the safe side of things, I agree with Rafael that we should
> revert that patch instead of introducing a single platform quirk.

Jiang,

can you please prepare a revert patch for this?

Thanks,

tglx

2015-08-19 09:06:08

by Jiang Liu

[permalink] [raw]

Subject: Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV

2015-08-20 06:19:18

by Jiang Liu

[permalink] [raw]

Subject: Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV

On 2015/8/19 16:40, Thomas Gleixner wrote:
> On Wed, 19 Aug 2015, Thomas Gleixner wrote:
>> On Wed, 19 Aug 2015, Jiang Liu wrote:
>>> On 2015/8/19 14:45, Rafael J. Wysocki wrote:
>>>> Well, the regression at hand has just shown that the assertion in the
>>>> changelog of that commit ("no need for for special treatment for GSI
>>>> used by ACPI SCI") does not really hold. So, if the only motivation
>>>> for it was to get rid of one extra check in mp_unregister_gsi()
>>>> (mp_register_gsi() still needs to check if it is dealing with the SCI
>>>> anyway), I'd vote for reverting it.
>>> Hi Rafael,
>>> The motivation is to treat SCI as normal IOAPIC interrupt so
>>> we could enforce stricter pin attribute checking. Now it does reveal
>>> flaws in ACPI BIOS implementations, but we ran into trouble about how to
>>> handle those flaws:(
>>
>> The intent of this change is entirely correct, though it seems that
>> reality of ACPI is just different.
>>
>> To be on the safe side of things, I agree with Rafael that we should
>> revert that patch instead of introducing a single platform quirk.
>
> Jiang,
>
> can you please prepare a revert patch for this?
Hi Rafael and Thomas,
I have tried to revert commit cd68f6bd53cf, but found
it's not an easy task now.
When converting to hierarchical irqdomain, the IOAPIC
internal and interfaces have changed much, and seems no easy
way to revert cd68f6bd53cf. There may be three possible solutions
here:
1) use quirk to correct SCI polarity, as the patch does.
2) change IOAPIC interfaces to provide a special way to
handle SCI interrupt.
3) change drivers/acpi/pci_link.c to penalize SCI IRQ so it
won't be used for PCI IRQ if SCI polarity conflicts with
PCI IRQ polarity.

What's your thoughts here?

Thanks!
Gerry

2015-08-20 09:15:57

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV

On Thu, 20 Aug 2015, Jiang Liu wrote:
> On 2015/8/19 16:40, Thomas Gleixner wrote:
> > On Wed, 19 Aug 2015, Thomas Gleixner wrote:
> >> On Wed, 19 Aug 2015, Jiang Liu wrote:
> >>> On 2015/8/19 14:45, Rafael J. Wysocki wrote:
> >>>> Well, the regression at hand has just shown that the assertion in the
> >>>> changelog of that commit ("no need for for special treatment for GSI
> >>>> used by ACPI SCI") does not really hold. So, if the only motivation
> >>>> for it was to get rid of one extra check in mp_unregister_gsi()
> >>>> (mp_register_gsi() still needs to check if it is dealing with the SCI
> >>>> anyway), I'd vote for reverting it.
> >>> Hi Rafael,
> >>> The motivation is to treat SCI as normal IOAPIC interrupt so
> >>> we could enforce stricter pin attribute checking. Now it does reveal
> >>> flaws in ACPI BIOS implementations, but we ran into trouble about how to
> >>> handle those flaws:(
> >>
> >> The intent of this change is entirely correct, though it seems that
> >> reality of ACPI is just different.
> >>
> >> To be on the safe side of things, I agree with Rafael that we should
> >> revert that patch instead of introducing a single platform quirk.
> >
> > Jiang,
> >
> > can you please prepare a revert patch for this?
> Hi Rafael and Thomas,
> I have tried to revert commit cd68f6bd53cf, but found
> it's not an easy task now.

That's what I feared

> When converting to hierarchical irqdomain, the IOAPIC
> internal and interfaces have changed much, and seems no easy
> way to revert cd68f6bd53cf. There may be three possible solutions
> here:
> 1) use quirk to correct SCI polarity, as the patch does.
> 2) change IOAPIC interfaces to provide a special way to
> handle SCI interrupt.
> 3) change drivers/acpi/pci_link.c to penalize SCI IRQ so it
> won't be used for PCI IRQ if SCI polarity conflicts with
> PCI IRQ polarity.

Stupid question. Is the SCI polarity ever the opposite of PCI
polarity? I.e. is such a ACPI override valid at all?

Thanks,

tglx

2015-08-20 09:35:58

by Jiang Liu

[permalink] [raw]

Subject: Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV

On 2015/8/20 17:15, Thomas Gleixner wrote:
> On Thu, 20 Aug 2015, Jiang Liu wrote:
>> On 2015/8/19 16:40, Thomas Gleixner wrote:
>>> On Wed, 19 Aug 2015, Thomas Gleixner wrote:
>>>> On Wed, 19 Aug 2015, Jiang Liu wrote:
>>>>> On 2015/8/19 14:45, Rafael J. Wysocki wrote:
>>>>>> Well, the regression at hand has just shown that the assertion in the
>>>>>> changelog of that commit ("no need for for special treatment for GSI
>>>>>> used by ACPI SCI") does not really hold. So, if the only motivation
>>>>>> for it was to get rid of one extra check in mp_unregister_gsi()
>>>>>> (mp_register_gsi() still needs to check if it is dealing with the SCI
>>>>>> anyway), I'd vote for reverting it.
>>>>> Hi Rafael,
>>>>> The motivation is to treat SCI as normal IOAPIC interrupt so
>>>>> we could enforce stricter pin attribute checking. Now it does reveal
>>>>> flaws in ACPI BIOS implementations, but we ran into trouble about how to
>>>>> handle those flaws:(
>>>>
>>>> The intent of this change is entirely correct, though it seems that
>>>> reality of ACPI is just different.
>>>>
>>>> To be on the safe side of things, I agree with Rafael that we should
>>>> revert that patch instead of introducing a single platform quirk.
>>>
>>> Jiang,
>>>
>>> can you please prepare a revert patch for this?
>> Hi Rafael and Thomas,
>> I have tried to revert commit cd68f6bd53cf, but found
>> it's not an easy task now.
>
> That's what I feared
>
>> When converting to hierarchical irqdomain, the IOAPIC
>> internal and interfaces have changed much, and seems no easy
>> way to revert cd68f6bd53cf. There may be three possible solutions
>> here:
>> 1) use quirk to correct SCI polarity, as the patch does.
>> 2) change IOAPIC interfaces to provide a special way to
>> handle SCI interrupt.
>> 3) change drivers/acpi/pci_link.c to penalize SCI IRQ so it
>> won't be used for PCI IRQ if SCI polarity conflicts with
>> PCI IRQ polarity.
>
> Stupid question. Is the SCI polarity ever the opposite of PCI
> polarity? I.e. is such a ACPI override valid at all?
Hi Thomas,
I have analyzed another system which works:
1) SCI(IRQ9) works in level, high mode (so such an ACPI override is valid)
2) there's a flag to detect whether system works in PIC or APIC mode.
3) IRQ9 may be used for PCI IRQ if system works in PIC mode.
4) IRQ9 won't be used for PCI IRQ if system works in APIC mode.
Based on the above observation, I feel solution 3) may be the best one.
Thanks!
Gerry

2015-08-20 11:14:14

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV

On Thu, 20 Aug 2015, Jiang Liu wrote:
> On 2015/8/20 17:15, Thomas Gleixner wrote:
> > On Thu, 20 Aug 2015, Jiang Liu wrote:
> >> When converting to hierarchical irqdomain, the IOAPIC
> >> internal and interfaces have changed much, and seems no easy
> >> way to revert cd68f6bd53cf. There may be three possible solutions
> >> here:
> >> 1) use quirk to correct SCI polarity, as the patch does.
> >> 2) change IOAPIC interfaces to provide a special way to
> >> handle SCI interrupt.
> >> 3) change drivers/acpi/pci_link.c to penalize SCI IRQ so it
> >> won't be used for PCI IRQ if SCI polarity conflicts with
> >> PCI IRQ polarity.
> >
> > Stupid question. Is the SCI polarity ever the opposite of PCI
> > polarity? I.e. is such a ACPI override valid at all?
> Hi Thomas,
> I have analyzed another system which works:
> 1) SCI(IRQ9) works in level, high mode (so such an ACPI override is valid)
> 2) there's a flag to detect whether system works in PIC or APIC mode.
> 3) IRQ9 may be used for PCI IRQ if system works in PIC mode.

Why?

> 4) IRQ9 won't be used for PCI IRQ if system works in APIC mode.
> Based on the above observation, I feel solution 3) may be the best one.

Can you whip up a patch and test it on hyperv and maybe other machines
which have that SCI override.

Thanks

tglx

2015-08-21 07:33:12

by Jiang Liu

[permalink] [raw]

Subject: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI

Nick Meier reported a regression with HyperV that "
After rebooting the VM, the following messages are logged in syslog
when trying to load the tulip driver:
tulip: Linux Tulip drivers version 1.1.15 (Feb 27, 2007)
tulip: 0000:00:0a.0: PCI INT A: failed to register GSI
tulip: Cannot enable tulip board #0, aborting
tulip: probe of 0000:00:0a.0 failed with error -16
Errors occur in 3.19.0 kernel
Works in 3.17 kernel.
"

According to the ACPI dump file posted by Nick at
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1440072

The ACPI MADT table includes an interrupt source overridden entry for
ACPI SCI:
[236h 0566 1] Subtable Type : 02 <Interrupt Source Override>
[237h 0567 1] Length : 0A
[238h 0568 1] Bus : 00
[239h 0569 1] Source : 09
[23Ah 0570 4] Interrupt : 00000009
[23Eh 0574 2] Flags (decoded below) : 000D
Polarity : 1
Trigger Mode : 3

And in DSDT table, we have _PRT method to define PCI interrupts, which
eventually goes to:
Name (PRSA, ResourceTemplate ()
{
IRQ (Level, ActiveLow, Shared, )
{3,4,5,7,9,10,11,12,14,15}
})
Name (PRSB, ResourceTemplate ()
{
IRQ (Level, ActiveLow, Shared, )
{3,4,5,7,9,10,11,12,14,15}
})
Name (PRSC, ResourceTemplate ()
{
IRQ (Level, ActiveLow, Shared, )
{3,4,5,7,9,10,11,12,14,15}
})
Name (PRSD, ResourceTemplate ()
{
IRQ (Level, ActiveLow, Shared, )
{3,4,5,7,9,10,11,12,14,15}
})

According to the MADT and DSDT tables, IRQ 9 may be used for:
1) ACPI SCI in level, high mode
2) PCI legacy IRQ in level, low mode
So there's a conflict in polarity setting for IRQ 9.

Prior to commit cd68f6bd53cf ("x86, irq, acpi: Get rid of special
handling of GSI for ACPI SCI"), ACPI SCI is handled specially and
there's no check for conflicts between ACPI SCI and PCI legagy IRQ.
And it seems that the HyperV hypervisor doesn't make use of the
polarity configuration in IOAPIC entry, so it just works.

Commit cd68f6bd53cf gets rid of the specially handling of ACPI SCI,
and then the pin attribute checking code discloses the conflicts
between ACPI SCI and PCI legacy IRQ on HyperV virtual machine,
and rejects the request to assign IRQ9 to PCI devices.

So penalize legacy IRQ used by ACPI SCI and mark it unusable if ACPI
SCI attributes conflict with PCI IRQ attributes.

Please refer to following links for more information:
https://bugzilla.kernel.org/show_bug.cgi?id=101301
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1440072

Fixes: cd68f6bd53cf ("x86, irq, acpi: Get rid of special handling of GSI for ACPI SCI")
Reported-and-tested-by: Nick Meier <[email protected]>
Cc: <[email protected]> # 3.19
Signed-off-by: Jiang Liu <[email protected]>
---
Hi Nick,
Rafael and Thomas have concerns about the way to solve the
regression by quirk, so could you please help to test this patch?
Thanks!
Gerry

---
arch/x86/kernel/acpi/boot.c | 1 +
drivers/acpi/pci_link.c | 16 ++++++++++++++++
include/linux/acpi.h | 2 +-
3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index e49ee24da85e..9393896717d0 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -445,6 +445,7 @@ static void __init acpi_sci_ioapic_setup(u8 bus_irq, u16 polarity, u16 trigger,
polarity = acpi_sci_flags & ACPI_MADT_POLARITY_MASK;

mp_override_legacy_irq(bus_irq, polarity, trigger, gsi);
+ acpi_penalize_sci_irq(bus_irq, trigger, polarity);

/*
* stash over-ride to indicate we've been here
diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index cfd7581cc19f..b09ad554430a 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -826,6 +826,22 @@ void acpi_penalize_isa_irq(int irq, int active)
}

/*
+ * Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict with
+ * PCI IRQ attributes, mark ACPI SCI as ISA_ALWAYS so it won't be use for
+ * PCI IRQs.
+ */
+void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
+{
+ if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) {
+ if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
+ polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
+ acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_ALWAYS;
+ else
+ acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
+ }
+}
+
+/*
* Over-ride default table to reserve additional IRQs for use by ISA
* e.g. acpi_irq_isa=5
* Useful for telling ACPI how not to interfere with your ISA sound card.
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d2445fa9999f..0b2394f61af4 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -221,7 +221,7 @@ struct pci_dev;

int acpi_pci_irq_enable (struct pci_dev *dev);
void acpi_penalize_isa_irq(int irq, int active);
-
+void acpi_penalize_sci_irq(int irq, int trigger, int polarity);
void acpi_pci_irq_disable (struct pci_dev *dev);

extern int ec_read(u8 addr, u8 *val);
--
1.7.10.4

2015-08-25 08:02:16

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI

On Fri, 21 Aug 2015, Jiang Liu wrote:
> ---
> Hi Nick,
> Rafael and Thomas have concerns about the way to solve the
> regression by quirk, so could you please help to test this patch?

Nick, can you please confirm that this works?

Thanks,

tglx

2015-08-26 02:40:57

by Aaron Lu

[permalink] [raw]

Subject: Re: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI

On 08/25/2015 04:01 PM, Thomas Gleixner wrote:
> On Fri, 21 Aug 2015, Jiang Liu wrote:
>> ---
>> Hi Nick,
>> Rafael and Thomas have concerns about the way to solve the
>> regression by quirk, so could you please help to test this patch?
>
> Nick, can you please confirm that this works?

I saw Nick said this in bugzilla:
"
Tested the v3 patch - it works in a Hyper-V environment. The Tulip driver loads and the NIC was successfully assigned an IP address via DHCP.
"
https://bugzilla.kernel.org/show_bug.cgi?id=101301#c17

Regards,
Aaron

2015-08-26 07:56:49

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI

On Wed, 26 Aug 2015, Aaron Lu wrote:
> On 08/25/2015 04:01 PM, Thomas Gleixner wrote:
> > On Fri, 21 Aug 2015, Jiang Liu wrote:
> >> ---
> >> Hi Nick,
> >> Rafael and Thomas have concerns about the way to solve the
> >> regression by quirk, so could you please help to test this patch?
> >
> > Nick, can you please confirm that this works?
>
> I saw Nick said this in bugzilla:
> "
> Tested the v3 patch - it works in a Hyper-V environment. The Tulip driver loads and the NIC was successfully assigned an IP address via DHCP.
> "
> https://bugzilla.kernel.org/show_bug.cgi?id=101301#c17

Bah. Can we please keep such things on the mailinglist? The bugzilla
entry is good for tracking the issue, but certainly not for patch
discussion and validation.

Thanks

tglx

2015-08-26 09:28:19

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI

On Wed, 26 Aug 2015, Aaron Lu wrote:
> On 08/25/2015 04:01 PM, Thomas Gleixner wrote:
> > On Fri, 21 Aug 2015, Jiang Liu wrote:
> >> ---
> >> Hi Nick,
> >> Rafael and Thomas have concerns about the way to solve the
> >> regression by quirk, so could you please help to test this patch?
> >
> > Nick, can you please confirm that this works?
>
> I saw Nick said this in bugzilla:
> "
> Tested the v3 patch - it works in a Hyper-V environment. The Tulip driver loads and the NIC was successfully assigned an IP address via DHCP.

Rafael, can you take it via the ACPI tree?

Thanks,

tglx

2015-08-26 20:23:00

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI

On Wed, Aug 26, 2015 at 11:27 AM, Thomas Gleixner <[email protected]> wrote:
> On Wed, 26 Aug 2015, Aaron Lu wrote:
>> On 08/25/2015 04:01 PM, Thomas Gleixner wrote:
>> > On Fri, 21 Aug 2015, Jiang Liu wrote:
>> >> ---
>> >> Hi Nick,
>> >> Rafael and Thomas have concerns about the way to solve the
>> >> regression by quirk, so could you please help to test this patch?
>> >
>> > Nick, can you please confirm that this works?
>>
>> I saw Nick said this in bugzilla:
>> "
>> Tested the v3 patch - it works in a Hyper-V environment. The Tulip driver loads and the NIC was successfully assigned an IP address via DHCP.
>
> Rafael, can you take it via the ACPI tree?

I'll do that.

Thanks,
Rafael

2015-08-26 22:08:33

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI

On Wed, 26 Aug 2015, Rafael J. Wysocki wrote:
> On Wed, Aug 26, 2015 at 11:27 AM, Thomas Gleixner <[email protected]> wrote:
> > On Wed, 26 Aug 2015, Aaron Lu wrote:
> >> On 08/25/2015 04:01 PM, Thomas Gleixner wrote:
> >> > On Fri, 21 Aug 2015, Jiang Liu wrote:
> >> >> ---
> >> >> Hi Nick,
> >> >> Rafael and Thomas have concerns about the way to solve the
> >> >> regression by quirk, so could you please help to test this patch?
> >> >
> >> > Nick, can you please confirm that this works?
> >>
> >> I saw Nick said this in bugzilla:
> >> "
> >> Tested the v3 patch - it works in a Hyper-V environment. The Tulip driver loads and the NIC was successfully assigned an IP address via DHCP.
> >
> > Rafael, can you take it via the ACPI tree?
>
> I'll do that.

Forgot to say:

Acked-by: Thomas Gleixner <[email protected]>