linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch v1] x86, ACPI, irq: Fix a regression caused by
@ 2015-08-09  8:58 Jiang Liu
  2015-08-18 19:36 ` Thomas Gleixner
  0 siblings, 1 reply; 21+ messages in thread
From: Jiang Liu @ 2015-08-09  8:58 UTC (permalink / raw)
  To: Rafael J . Wysocki, Nick Meier, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86
  Cc: Jiang Liu, linux-acpi, linux-kernel, linux-pm

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")
Reported-and-tested-by: Nick Meier <nmeier@microsoft.com>
Cc: <stable@vger.kernel.org> # 3.19
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [Patch v1] x86, ACPI, irq: Fix a regression caused by
  2015-08-09  8:58 [Patch v1] x86, ACPI, irq: Fix a regression caused by Jiang Liu
@ 2015-08-18 19:36 ` Thomas Gleixner
  2015-08-19  5:53   ` [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV Jiang Liu
  0 siblings, 1 reply; 21+ messages in thread
From: Thomas Gleixner @ 2015-08-18 19:36 UTC (permalink / raw)
  To: Jiang Liu
  Cc: Rafael J . Wysocki, Nick Meier, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Ingo Molnar, H. Peter Anvin, x86, linux-acpi,
	linux-kernel, linux-pm

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV
  2015-08-18 19:36 ` Thomas Gleixner
@ 2015-08-19  5:53   ` Jiang Liu
  2015-08-19  6:04     ` Rafael J. Wysocki
  0 siblings, 1 reply; 21+ messages in thread
From: Jiang Liu @ 2015-08-19  5:53 UTC (permalink / raw)
  To: Thomas Gleixner, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86
  Cc: Jiang Liu, linux-acpi, linux-kernel, linux-pm

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 <nmeier@microsoft.com>
Cc: <stable@vger.kernel.org> # 3.19
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV
  2015-08-19  5:53   ` [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV Jiang Liu
@ 2015-08-19  6:04     ` Rafael J. Wysocki
  2015-08-19  6:26       ` Jiang Liu
  0 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2015-08-19  6:04 UTC (permalink / raw)
  To: Jiang Liu
  Cc: Thomas Gleixner, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm

Hi,

On Wed, Aug 19, 2015 at 7:53 AM, Jiang Liu <jiang.liu@linux.intel.com> 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV
  2015-08-19  6:04     ` Rafael J. Wysocki
@ 2015-08-19  6:26       ` Jiang Liu
  2015-08-19  6:45         ` Rafael J. Wysocki
  0 siblings, 1 reply; 21+ messages in thread
From: Jiang Liu @ 2015-08-19  6:26 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Thomas Gleixner, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm

On 2015/8/19 14:04, Rafael J. Wysocki wrote:
> Hi,
> 
> On Wed, Aug 19, 2015 at 7:53 AM, Jiang Liu <jiang.liu@linux.intel.com> 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV
  2015-08-19  6:26       ` Jiang Liu
@ 2015-08-19  6:45         ` Rafael J. Wysocki
  2015-08-19  6:53           ` Jiang Liu
  0 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2015-08-19  6:45 UTC (permalink / raw)
  To: Jiang Liu
  Cc: Rafael J. Wysocki, Thomas Gleixner, Rafael J . Wysocki,
	Nick Meier, Rafael J. Wysocki, Len Brown, Pavel Machek,
	Ingo Molnar, H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm

Hi,

On Wed, Aug 19, 2015 at 8:26 AM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> On 2015/8/19 14:04, Rafael J. Wysocki wrote:
>> Hi,
>>
>> On Wed, Aug 19, 2015 at 7:53 AM, Jiang Liu <jiang.liu@linux.intel.com> 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV
  2015-08-19  6:45         ` Rafael J. Wysocki
@ 2015-08-19  6:53           ` Jiang Liu
  2015-08-19  8:29             ` Thomas Gleixner
  0 siblings, 1 reply; 21+ messages in thread
From: Jiang Liu @ 2015-08-19  6:53 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Thomas Gleixner, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm

On 2015/8/19 14:45, Rafael J. Wysocki wrote:
> Hi,
> 
> On Wed, Aug 19, 2015 at 8:26 AM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
>> On 2015/8/19 14:04, Rafael J. Wysocki wrote:
>>> Hi,
>>>
>>> On Wed, Aug 19, 2015 at 7:53 AM, Jiang Liu <jiang.liu@linux.intel.com> 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
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV
  2015-08-19  6:53           ` Jiang Liu
@ 2015-08-19  8:29             ` Thomas Gleixner
  2015-08-19  8:40               ` Thomas Gleixner
  0 siblings, 1 reply; 21+ messages in thread
From: Thomas Gleixner @ 2015-08-19  8:29 UTC (permalink / raw)
  To: Jiang Liu
  Cc: Rafael J. Wysocki, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm

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




^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV
  2015-08-19  8:29             ` Thomas Gleixner
@ 2015-08-19  8:40               ` Thomas Gleixner
  2015-08-19  9:05                 ` Jiang Liu
  2015-08-20  6:19                 ` Jiang Liu
  0 siblings, 2 replies; 21+ messages in thread
From: Thomas Gleixner @ 2015-08-19  8:40 UTC (permalink / raw)
  To: Jiang Liu
  Cc: Rafael J. Wysocki, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV
  2015-08-19  8:40               ` Thomas Gleixner
@ 2015-08-19  9:05                 ` Jiang Liu
  2015-08-20  6:19                 ` Jiang Liu
  1 sibling, 0 replies; 21+ messages in thread
From: Jiang Liu @ 2015-08-19  9:05 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Rafael J. Wysocki, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm

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?
Sure, will send out revert patch after basic tests.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV
  2015-08-19  8:40               ` Thomas Gleixner
  2015-08-19  9:05                 ` Jiang Liu
@ 2015-08-20  6:19                 ` Jiang Liu
  2015-08-20  9:15                   ` Thomas Gleixner
  1 sibling, 1 reply; 21+ messages in thread
From: Jiang Liu @ 2015-08-20  6:19 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Rafael J. Wysocki, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV
  2015-08-20  6:19                 ` Jiang Liu
@ 2015-08-20  9:15                   ` Thomas Gleixner
  2015-08-20  9:35                     ` Jiang Liu
  0 siblings, 1 reply; 21+ messages in thread
From: Thomas Gleixner @ 2015-08-20  9:15 UTC (permalink / raw)
  To: Jiang Liu
  Cc: Rafael J. Wysocki, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV
  2015-08-20  9:15                   ` Thomas Gleixner
@ 2015-08-20  9:35                     ` Jiang Liu
  2015-08-20 11:13                       ` Thomas Gleixner
  0 siblings, 1 reply; 21+ messages in thread
From: Jiang Liu @ 2015-08-20  9:35 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Rafael J. Wysocki, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm



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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV
  2015-08-20  9:35                     ` Jiang Liu
@ 2015-08-20 11:13                       ` Thomas Gleixner
  2015-08-21  7:36                         ` [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI Jiang Liu
  0 siblings, 1 reply; 21+ messages in thread
From: Thomas Gleixner @ 2015-08-20 11:13 UTC (permalink / raw)
  To: Jiang Liu
  Cc: Rafael J. Wysocki, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI
  2015-08-20 11:13                       ` Thomas Gleixner
@ 2015-08-21  7:36                         ` Jiang Liu
  2015-08-25  8:01                           ` Thomas Gleixner
  0 siblings, 1 reply; 21+ messages in thread
From: Jiang Liu @ 2015-08-21  7:36 UTC (permalink / raw)
  To: Thomas Gleixner, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86
  Cc: Jiang Liu, linux-acpi, linux-kernel, linux-pm

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 <nmeier@microsoft.com>
Cc: <stable@vger.kernel.org> # 3.19
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI
  2015-08-21  7:36                         ` [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI Jiang Liu
@ 2015-08-25  8:01                           ` Thomas Gleixner
  2015-08-26  2:42                             ` Aaron Lu
  0 siblings, 1 reply; 21+ messages in thread
From: Thomas Gleixner @ 2015-08-25  8:01 UTC (permalink / raw)
  To: Jiang Liu
  Cc: Rafael J . Wysocki, Nick Meier, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Ingo Molnar, H. Peter Anvin, x86, linux-acpi,
	linux-kernel, linux-pm

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI
  2015-08-25  8:01                           ` Thomas Gleixner
@ 2015-08-26  2:42                             ` Aaron Lu
  2015-08-26  7:56                               ` Thomas Gleixner
  2015-08-26  9:27                               ` Thomas Gleixner
  0 siblings, 2 replies; 21+ messages in thread
From: Aaron Lu @ 2015-08-26  2:42 UTC (permalink / raw)
  To: Thomas Gleixner, Jiang Liu
  Cc: Rafael J . Wysocki, Nick Meier, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Ingo Molnar, H. Peter Anvin, x86, linux-acpi,
	linux-kernel, linux-pm

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI
  2015-08-26  2:42                             ` Aaron Lu
@ 2015-08-26  7:56                               ` Thomas Gleixner
  2015-08-26  9:27                               ` Thomas Gleixner
  1 sibling, 0 replies; 21+ messages in thread
From: Thomas Gleixner @ 2015-08-26  7:56 UTC (permalink / raw)
  To: Aaron Lu
  Cc: Jiang Liu, Rafael J . Wysocki, Nick Meier, Rafael J. Wysocki,
	Len Brown, Pavel Machek, Ingo Molnar, H. Peter Anvin, x86,
	linux-acpi, linux-kernel, linux-pm

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI
  2015-08-26  2:42                             ` Aaron Lu
  2015-08-26  7:56                               ` Thomas Gleixner
@ 2015-08-26  9:27                               ` Thomas Gleixner
  2015-08-26 20:22                                 ` Rafael J. Wysocki
  1 sibling, 1 reply; 21+ messages in thread
From: Thomas Gleixner @ 2015-08-26  9:27 UTC (permalink / raw)
  To: Aaron Lu
  Cc: Jiang Liu, Rafael J . Wysocki, Nick Meier, Rafael J. Wysocki,
	Len Brown, Pavel Machek, Ingo Molnar, H. Peter Anvin, x86,
	linux-acpi, linux-kernel, linux-pm

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI
  2015-08-26  9:27                               ` Thomas Gleixner
@ 2015-08-26 20:22                                 ` Rafael J. Wysocki
  2015-08-26 22:07                                   ` Thomas Gleixner
  0 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2015-08-26 20:22 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Aaron Lu, Jiang Liu, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm

On Wed, Aug 26, 2015 at 11:27 AM, Thomas Gleixner <tglx@linutronix.de> 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI
  2015-08-26 20:22                                 ` Rafael J. Wysocki
@ 2015-08-26 22:07                                   ` Thomas Gleixner
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Gleixner @ 2015-08-26 22:07 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Aaron Lu, Jiang Liu, Rafael J . Wysocki, Nick Meier,
	Rafael J. Wysocki, Len Brown, Pavel Machek, Ingo Molnar,
	H. Peter Anvin, x86, ACPI Devel Maling List,
	Linux Kernel Mailing List, linux-pm

On Wed, 26 Aug 2015, Rafael J. Wysocki wrote:
> On Wed, Aug 26, 2015 at 11:27 AM, Thomas Gleixner <tglx@linutronix.de> 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 <tglx@linutronix.de>

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2015-08-26 22:08 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-09  8:58 [Patch v1] x86, ACPI, irq: Fix a regression caused by Jiang Liu
2015-08-18 19:36 ` Thomas Gleixner
2015-08-19  5:53   ` [Patch v2] x86, ACPI, irq: Add a quirk to override SCI polarity for HyperV Jiang Liu
2015-08-19  6:04     ` Rafael J. Wysocki
2015-08-19  6:26       ` Jiang Liu
2015-08-19  6:45         ` Rafael J. Wysocki
2015-08-19  6:53           ` Jiang Liu
2015-08-19  8:29             ` Thomas Gleixner
2015-08-19  8:40               ` Thomas Gleixner
2015-08-19  9:05                 ` Jiang Liu
2015-08-20  6:19                 ` Jiang Liu
2015-08-20  9:15                   ` Thomas Gleixner
2015-08-20  9:35                     ` Jiang Liu
2015-08-20 11:13                       ` Thomas Gleixner
2015-08-21  7:36                         ` [Patch v3] ACPI, PCI: Penalize legacy IRQ used by ACPI SCI Jiang Liu
2015-08-25  8:01                           ` Thomas Gleixner
2015-08-26  2:42                             ` Aaron Lu
2015-08-26  7:56                               ` Thomas Gleixner
2015-08-26  9:27                               ` Thomas Gleixner
2015-08-26 20:22                                 ` Rafael J. Wysocki
2015-08-26 22:07                                   ` Thomas Gleixner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).