ACPI events broken on non-SMP since 3.16

PATCH In review Jim Bos 3 participants 17 messages Apr 5, 2015 – Apr 13, 2015

Subsystems (3)

x86 ACPI ACPI component architecture (acpica)

lore

Share & tools

lore

Jim Bos

author

2015-04-05 14:43 UTC #0 lore raw

Hello,

Since around 3.16 ACPI events aren't working on a single processor system, I found this on an old Dell laptop Pentium-M and on AMD geode board. By ACPI events I mean, acpid (via netlink) and acpi_listen doesn't give anything when pressing ACPI power button.

Simply 'nosmp' on recent kernel on modern 2-core Celeron system also stops acpid from working.
Note that booting an SMP kernel with single core isn't enough to break this, smp really needs to be disabled by 'nosmp' or running it on a older motherboard without I/O apic ('smbboot: SMP disabled' in dmesg).

With some problems managed to bisect this using VirtualBox guest, which allows to to specify number of CPU's and having yes/no an I/O Apic.

It bisects to this commit which unfortunately doesn't revert cleanly on latest 4.0-rcX:

commit 16ee7b3dcc56be14b9a813612cff2cc2339cdced

Author: Jiang Liu <[email protected]>
Date:   Mon Jun 9 16:20:04 2014 +0800

x86, irq: Simplify the way to handle ISA IRQ

On startup, setup_IO_APIC_irqs() will program all IOAPIC pins for ISA IRQs. Later when mp_map_pin_to_irq() is called, it just returns ISA IRQ number without programming corresponding IOAPIC pin.

This patch consolidates the way to program IOAPIC pins for both ISA and non-ISA IRQs into mp_map_pin_to_irq() as below:
1) For ISA IRQs, mp_irqs array is used to map IOAPIC pin to IRQ and mp_irqdomain_map() is used to actually program the pin.
2) For non-ISA IRQs, irqdomain is used to map IOAPIC pin to IRQ, and mp_irqdomain_map() is also used to actually program the pin.

Any ideas ?
_
Jim

Jiang Liu

2015-04-07 14:31 UTC #1 lore raw

PATCH 1/1 x86/ACPI: Fix regression caused by 16ee7b3dcc56

Hi Jim,
Could you please help to test this patch against v4.0-rc6?
Thanks!
Gerry

Signed-off-by Jiang Liu <[email protected]>

---
 arch/x86/kernel/acpi/boot.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

arch/x86/kernel/acpi/boot.c +7 -3

index 803b684676ff..f7f1fe7cd1b0 100644
@@ -403,10 +403,14 @@ static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger,
 static int mp_register_gsi(struct device *dev, u32 gsi, int trigger,
 			   int polarity)
 {
-	int irq, node;
+	int i, irq, node;
 
-	if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC)
-		return gsi;
+	if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC) {
+		for (i = 0; i < nr_legacy_irqs(); i++)
+			if (isa_irq_to_gsi[i] == gsi)
+				return i;
+		return -1;
+	}
 
 	trigger = trigger == ACPI_EDGE_SENSITIVE ? 0 : 1;
 	polarity = polarity == ACPI_ACTIVE_HIGH ? 0 : 1;

Signature · 1 line

1.7.10.4

Jim Bos

author

2015-04-07 16:49 UTC #2 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56

Quoted 23 lines · Jiang Liu

Hi Jim,
Could you please help to test this patch against v4.0-rc6?
Thanks!
Gerry

Signed-off-by Jiang Liu <[email protected]>

---
 arch/x86/kernel/acpi/boot.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

arch/x86/kernel/acpi/boot.c +1 -2

index 803b684676ff..f7f1fe7cd1b0 100644
@@ -403,10 +403,14 @@ static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger,
 static int mp_register_gsi(struct device *dev, u32 gsi, int trigger,
 			   int polarity)
 {
-	int irq, node;
+	int i, irq, node;
 
-	if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC)

arch/x86/kernel/acpi/boot.c static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger, · line 416

Quoted 10 lines

arch/x86/kernel/acpi/boot.c +6 -1

-		return gsi;
+	if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC) {
+		for (i = 0; i < nr_legacy_irqs(); i++)
+			if (isa_irq_to_gsi[i] == gsi)
+				return i;
+		return -1;
+	}
 
 	trigger = trigger == ACPI_EDGE_SENSITIVE ? 0 : 1;
 	polarity = polarity == ACPI_ACTIVE_HIGH ? 0 : 1;

Jiang,

It definitely seems to be an improvement, using Virtualbox guest with your patch applied acpi-events work for all combinations (smp/nosmp with/without I/O APIC assigned to the guest).

However, on the Dell laptop it still doesn't work. To be sure I built a 3.16 kernel on this laptop and acpi_event power-button lid close/open are working just fine.

Attached config + dmesg + cat /proc/interrupt for the working 3.16 case and still not working 4.0-rc6+patch case.

Thanks,
Jim

Jiang Liu

2015-04-08 05:26 UTC #3 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56

Quoted 41 lines · Jim Bos

Quoted 23 lines · Jiang Liu
Hi Jim,
Could you please help to test this patch against v4.0-rc6?
Thanks!
Gerry
Signed-off-by Jiang Liu <[email protected]>
---
 arch/x86/kernel/acpi/boot.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
arch/x86/kernel/acpi/boot.c +1 -2
index 803b684676ff..f7f1fe7cd1b0 100644
@@ -403,10 +403,14 @@ static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger,
 static int mp_register_gsi(struct device *dev, u32 gsi, int trigger,
 			   int polarity)
 {
-	int irq, node;
+	int i, irq, node;
 
-	if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC)
arch/x86/kernel/acpi/boot.c static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger, · line 416
Quoted 10 lines
arch/x86/kernel/acpi/boot.c +6 -1
-		return gsi;
+	if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC) {
+		for (i = 0; i < nr_legacy_irqs(); i++)
+			if (isa_irq_to_gsi[i] == gsi)
+				return i;
+		return -1;
+	}
 
 	trigger = trigger == ACPI_EDGE_SENSITIVE ? 0 : 1;
 	polarity = polarity == ACPI_ACTIVE_HIGH ? 0 : 1;
Jiang,
It definitely seems to be an improvement, using Virtualbox guest with your patch applied acpi-events work for all combinations (smp/nosmp with/without I/O APIC assigned to the guest).

Quoted 6 lines

However, on the Dell laptop it still doesn't work. To be sure I built a 3.16 kernel on this laptop and acpi_event power-button lid close/open are working just fine.
Attached config + dmesg + cat /proc/interrupt for the working 3.16 case and still not working 4.0-rc6+patch case.

Hi Jim,
According to the attached files, you are building a UP kernel
with IOAPIC enabled. This configuration works well on my HP laptop. And according to file IRQs from 4.0-rc6, it shows:
9: 1 XT-PIC acpi
That means kernel has received one ACPI SCI interrupt, but no following-on ACPI SCI interrupts, I can't figure out the root cause yet.

So could you please help to dump ACPI tables from your dell laptop by using acpidump utility?
Thanks!
Gerry

Quoted 10 lines

Thanks,
Jim

Jim Bos

author

2015-04-08 15:51 UTC #4 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56

Quoted 56 lines · Jiang Liu

Quoted 41 lines · Jim Bos
Quoted 23 lines · Jiang Liu
Hi Jim,
Could you please help to test this patch against v4.0-rc6?
Thanks!
Gerry
Signed-off-by Jiang Liu <[email protected]>
---
 arch/x86/kernel/acpi/boot.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
arch/x86/kernel/acpi/boot.c +1 -2
index 803b684676ff..f7f1fe7cd1b0 100644
@@ -403,10 +403,14 @@ static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger,
 static int mp_register_gsi(struct device *dev, u32 gsi, int trigger,
 			   int polarity)
 {
-	int irq, node;
+	int i, irq, node;
 
-	if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC)
arch/x86/kernel/acpi/boot.c static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger, · line 416
Quoted 10 lines
arch/x86/kernel/acpi/boot.c +6 -1
-		return gsi;
+	if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC) {
+		for (i = 0; i < nr_legacy_irqs(); i++)
+			if (isa_irq_to_gsi[i] == gsi)
+				return i;
+		return -1;
+	}
 
 	trigger = trigger == ACPI_EDGE_SENSITIVE ? 0 : 1;
 	polarity = polarity == ACPI_ACTIVE_HIGH ? 0 : 1;
Jiang,
It definitely seems to be an improvement, using Virtualbox guest with your patch applied acpi-events work for all combinations (smp/nosmp with/without I/O APIC assigned to the guest).
Quoted 6 lines
However, on the Dell laptop it still doesn't work. To be sure I built a 3.16 kernel on this laptop and acpi_event power-button lid close/open are working just fine.
Attached config + dmesg + cat /proc/interrupt for the working 3.16 case and still not working 4.0-rc6+patch case.
Hi Jim,
According to the attached files, you are building a UP kernel
with IOAPIC enabled. This configuration works well on my HP laptop. And according to file IRQs from 4.0-rc6, it shows:
9: 1 XT-PIC acpi
That means kernel has received one ACPI SCI interrupt, but no following-on ACPI SCI interrupts, I can't figure out the root cause yet.

Quoted 9 lines

So could you please help to dump ACPI tables from your dell laptop by using acpidump utility?
Thanks!
Gerry
Quoted 4 lines
Thanks,
Jim

Gerry,

Attached acpidump (binary files and regular dump).
I just tested 4.0-rc6+your_patch on another single core system, AMD geode board, and that works fine now!
So indeed it seems there is something special about the dell laptop as that's the only system, I've available here, which still has an issue. _
Jim

Jiang Liu

2015-04-09 10:15 UTC #5 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56

Quoted 67 lines · Jim Bos

Quoted 56 lines · Jiang Liu
Quoted 41 lines · Jim Bos
Quoted 23 lines · Jiang Liu
Hi Jim,
Could you please help to test this patch against v4.0-rc6?
Thanks!
Gerry
Signed-off-by Jiang Liu <[email protected]>
---
 arch/x86/kernel/acpi/boot.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
arch/x86/kernel/acpi/boot.c +1 -2
index 803b684676ff..f7f1fe7cd1b0 100644
@@ -403,10 +403,14 @@ static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger,
 static int mp_register_gsi(struct device *dev, u32 gsi, int trigger,
 			   int polarity)
 {
-	int irq, node;
+	int i, irq, node;
 
-	if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC)
arch/x86/kernel/acpi/boot.c static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger, · line 416
Quoted 10 lines
arch/x86/kernel/acpi/boot.c +6 -1
-		return gsi;
+	if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC) {
+		for (i = 0; i < nr_legacy_irqs(); i++)
+			if (isa_irq_to_gsi[i] == gsi)
+				return i;
+		return -1;
+	}
 
 	trigger = trigger == ACPI_EDGE_SENSITIVE ? 0 : 1;
 	polarity = polarity == ACPI_ACTIVE_HIGH ? 0 : 1;
Jiang,
It definitely seems to be an improvement, using Virtualbox guest with your patch applied acpi-events work for all combinations (smp/nosmp with/without I/O APIC assigned to the guest).
Quoted 6 lines
However, on the Dell laptop it still doesn't work. To be sure I built a 3.16 kernel on this laptop and acpi_event power-button lid close/open are working just fine.
Attached config + dmesg + cat /proc/interrupt for the working 3.16 case and still not working 4.0-rc6+patch case.
Hi Jim,
According to the attached files, you are building a UP kernel
with IOAPIC enabled. This configuration works well on my HP laptop. And according to file IRQs from 4.0-rc6, it shows:
9: 1 XT-PIC acpi
That means kernel has received one ACPI SCI interrupt, but no following-on ACPI SCI interrupts, I can't figure out the root cause yet.
Quoted 9 lines
So could you please help to dump ACPI tables from your dell laptop by using acpidump utility?
Thanks!
Gerry
Quoted 4 lines
Thanks,
Jim

Quoted 7 lines

Gerry,
Attached acpidump (binary files and regular dump).
I just tested 4.0-rc6+your_patch on another single core system, AMD geode board, and that works fine now!
So indeed it seems there is something special about the dell laptop as that's the only system, I've available here, which still has an issue.

Hi Jim,
I'm really confused. I can't even explain why my previous
patch fixes the issue on AMD geode board now:(

For the Dell laptop, seems you have:
1) build a kernel with Local APIC and IOAPIC enabled
2) lapic is disabled by BIOS, so there's no ACPI MADT(APIC) table at all.
That means the laptop is working with 8259 PICs only. There's little change between 3.16 and 4.0 related to 8259.

For the AMD geode board, I still think original code is right. I can't explain why the patch fix the issue.

So could you please help to:
1) Try to enable lapic on Dell laptop in BIOS
2) Dump acpi tables and dmesg on AMD board

If that still doesn't help, I will try to send you some debug patches to gather more info.
Thanks!
Gerry

Quoted 3 lines

_
Jim

Jim Bos

author

2015-04-09 16:41 UTC #6 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56

Quoted 89 lines · Jiang Liu

Quoted 67 lines · Jim Bos
Quoted 56 lines · Jiang Liu
Quoted 41 lines · Jim Bos
Quoted 23 lines · Jiang Liu
Hi Jim,
Could you please help to test this patch against v4.0-rc6?
Thanks!
Gerry
Signed-off-by Jiang Liu <[email protected]>
---
 arch/x86/kernel/acpi/boot.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
arch/x86/kernel/acpi/boot.c +1 -2
index 803b684676ff..f7f1fe7cd1b0 100644
@@ -403,10 +403,14 @@ static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger,
 static int mp_register_gsi(struct device *dev, u32 gsi, int trigger,
 			   int polarity)
 {
-	int irq, node;
+	int i, irq, node;
 
-	if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC)
arch/x86/kernel/acpi/boot.c static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger, · line 416
Quoted 10 lines
arch/x86/kernel/acpi/boot.c +6 -1
-		return gsi;
+	if (acpi_irq_model != ACPI_IRQ_MODEL_IOAPIC) {
+		for (i = 0; i < nr_legacy_irqs(); i++)
+			if (isa_irq_to_gsi[i] == gsi)
+				return i;
+		return -1;
+	}
 
 	trigger = trigger == ACPI_EDGE_SENSITIVE ? 0 : 1;
 	polarity = polarity == ACPI_ACTIVE_HIGH ? 0 : 1;
Jiang,
It definitely seems to be an improvement, using Virtualbox guest with your patch applied acpi-events work for all combinations (smp/nosmp with/without I/O APIC assigned to the guest).
Quoted 6 lines
However, on the Dell laptop it still doesn't work. To be sure I built a 3.16 kernel on this laptop and acpi_event power-button lid close/open are working just fine.
Attached config + dmesg + cat /proc/interrupt for the working 3.16 case and still not working 4.0-rc6+patch case.
Hi Jim,
According to the attached files, you are building a UP kernel
with IOAPIC enabled. This configuration works well on my HP laptop. And according to file IRQs from 4.0-rc6, it shows:
9: 1 XT-PIC acpi
That means kernel has received one ACPI SCI interrupt, but no following-on ACPI SCI interrupts, I can't figure out the root cause yet.
Quoted 9 lines
So could you please help to dump ACPI tables from your dell laptop by using acpidump utility?
Thanks!
Gerry
Quoted 4 lines
Thanks,
Jim
Quoted 7 lines
Gerry,
Attached acpidump (binary files and regular dump).
I just tested 4.0-rc6+your_patch on another single core system, AMD geode board, and that works fine now!
So indeed it seems there is something special about the dell laptop as that's the only system, I've available here, which still has an issue.
Hi Jim,
I'm really confused. I can't even explain why my previous
patch fixes the issue on AMD geode board now:(
For the Dell laptop, seems you have:
1) build a kernel with Local APIC and IOAPIC enabled
2) lapic is disabled by BIOS, so there's no ACPI MADT(APIC) table at all.
That means the laptop is working with 8259 PICs only. There's little change between 3.16 and 4.0 related to 8259.
For the AMD geode board, I still think original code is right. I can't explain why the patch fix the issue.

Quoted 12 lines

So could you please help to:
1) Try to enable lapic on Dell laptop in BIOS
2) Dump acpi tables and dmesg on AMD board
If that still doesn't help, I will try to send you some debug patches to gather more info.
Thanks!
Gerry
Quoted 3 lines
_
Jim

Gerry,

As you mentioned your patch shouldn't make a difference, run some more tests, as it turns out:
- geode system broken on 3.16+ up to and including 3.19, however, on plain 4.0-rc6 it works! Root cause appears to be there isn't an ACPI interrupt assigned in non-working kernels.
- other system I got my hands on: Pentium(R) CPU G3220, broken on 3.19.0 when boot parameter 'nosmp' is specified, again no acpi entry in /proc/interrupts, working fine on 4.0-rc6

So obviously between 3.19 and 4.0-rc6 something got fixed here!

The Dell laptop remains the only problem then on 4.0-rc6, there IS an acpi interrupt (but firing once apparently).
There isn't an option in BIOS to enable LAPIC, however, when specifying 'lapic' as boot parameter I got interesting result, still not working and /proc/interrups still shows XT-PIC. Doing a diff between dmesg on 3.19 and 4.0-rc6 this pops out:

-Local APIC disabled by BIOS -- you can enable it with "lapic" -APIC: disable apic facility
-APIC: switched to apic NOOP
+Local APIC disabled by BIOS -- reenabling.
+Found and enabled local APIC!

+Enabling APIC mode: Flat. Using 0 I/O APICs

Jim

Jiang Liu

2015-04-10 01:56 UTC #7 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56

Quoted 5 lines · Jim Bos

Quoted 4 lines · Jiang Liu
Quoted 3 lines · Jim Bos
Quoted 2 lines · Jiang Liu
Quoted 1 line · Jim Bos
On 04/07/2015 04:34 PM, Jiang Liu wrote:

<snip>

Quoted 38 lines

Quoted 14 lines
Hi Jim,
I'm really confused. I can't even explain why my previous
patch fixes the issue on AMD geode board now:(
For the Dell laptop, seems you have:
1) build a kernel with Local APIC and IOAPIC enabled
2) lapic is disabled by BIOS, so there's no ACPI MADT(APIC) table at all.
That means the laptop is working with 8259 PICs only. There's little change between 3.16 and 4.0 related to 8259.
For the AMD geode board, I still think original code is right. I can't explain why the patch fix the issue.
Quoted 12 lines
So could you please help to:
1) Try to enable lapic on Dell laptop in BIOS
2) Dump acpi tables and dmesg on AMD board
If that still doesn't help, I will try to send you some debug patches to gather more info.
Thanks!
Gerry
Quoted 3 lines
_
Jim
Gerry,
As you mentioned your patch shouldn't make a difference, run some more tests, as it turns out:
- geode system broken on 3.16+ up to and including 3.19, however, on plain 4.0-rc6 it works! Root cause appears to be there isn't an ACPI interrupt assigned in non-working kernels.
- other system I got my hands on: Pentium(R) CPU G3220, broken on 3.19.0 when boot parameter 'nosmp' is specified, again no acpi entry in /proc/interrupts, working fine on 4.0-rc6

Quoted 1 line

So obviously between 3.19 and 4.0-rc6 something got fixed here!

Hi Jim,
Yes, the bugfix patch should be:
commit 1ea76fbadd66("x86/irq: Fix regression caused by commit b568b8601f05")

Quoted 7 lines

The Dell laptop remains the only problem then on 4.0-rc6, there IS an acpi interrupt (but firing once apparently).
There isn't an option in BIOS to enable LAPIC, however, when specifying 'lapic' as boot parameter I got interesting result, still not working and /proc/interrups still shows XT-PIC. Doing a diff between dmesg on 3.19 and 4.0-rc6 this pops out:

Review

Quoted 7 lines

(quoted diff) +3 -3

-Local APIC disabled by BIOS -- you can enable it with "lapic"
-APIC: disable apic facility
-APIC: switched to apic NOOP
+Local APIC disabled by BIOS -- reenabling.
+Found and enabled local APIC!
 
+Enabling APIC mode:  Flat.  Using 0 I/O APICs

What's the last know working kernel for Dell laptop? Does it work as expected with v3.19 kernel?
Do you means this message is from plain v4.0-rc6 kernel? Thanks!
Gerry

Quoted 3 lines

Jim

Jim Bos

author

2015-04-10 06:19 UTC #8 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56

Quoted 63 lines · Jiang Liu

Quoted 5 lines · Jim Bos
Quoted 4 lines · Jiang Liu
Quoted 3 lines · Jim Bos
Quoted 2 lines · Jiang Liu
Quoted 1 line · Jim Bos
On 04/07/2015 04:34 PM, Jiang Liu wrote:
<snip>
Quoted 38 lines
Quoted 14 lines
Hi Jim,
I'm really confused. I can't even explain why my previous
patch fixes the issue on AMD geode board now:(
For the Dell laptop, seems you have:
1) build a kernel with Local APIC and IOAPIC enabled
2) lapic is disabled by BIOS, so there's no ACPI MADT(APIC) table at all.
That means the laptop is working with 8259 PICs only. There's little change between 3.16 and 4.0 related to 8259.
For the AMD geode board, I still think original code is right. I can't explain why the patch fix the issue.
Quoted 12 lines
So could you please help to:
1) Try to enable lapic on Dell laptop in BIOS
2) Dump acpi tables and dmesg on AMD board
If that still doesn't help, I will try to send you some debug patches to gather more info.
Thanks!
Gerry
Quoted 3 lines
_
Jim
Gerry,
As you mentioned your patch shouldn't make a difference, run some more tests, as it turns out:
- geode system broken on 3.16+ up to and including 3.19, however, on plain 4.0-rc6 it works! Root cause appears to be there isn't an ACPI interrupt assigned in non-working kernels.
- other system I got my hands on: Pentium(R) CPU G3220, broken on 3.19.0 when boot parameter 'nosmp' is specified, again no acpi entry in /proc/interrupts, working fine on 4.0-rc6
Quoted 1 line
So obviously between 3.19 and 4.0-rc6 something got fixed here!
Hi Jim,
Yes, the bugfix patch should be:
commit 1ea76fbadd66("x86/irq: Fix regression caused by commit b568b8601f05")
Quoted 13 lines
The Dell laptop remains the only problem then on 4.0-rc6, there IS an acpi interrupt (but firing once apparently).
There isn't an option in BIOS to enable LAPIC, however, when specifying 'lapic' as boot parameter I got interesting result, still not working and /proc/interrups still shows XT-PIC. Doing a diff between dmesg on 3.19 and 4.0-rc6 this pops out:
(quoted diff) +2 -3
-Local APIC disabled by BIOS -- you can enable it with "lapic"
-APIC: disable apic facility
-APIC: switched to apic NOOP
+Local APIC disabled by BIOS -- reenabling.
+Found and enabled local APIC!

Quoted 11 lines

Quoted 1 line
+Enabling APIC mode: Flat. Using 0 I/O APICs
What's the last know working kernel for Dell laptop? Does it work as expected with v3.19 kernel?
Do you means this message is from plain v4.0-rc6 kernel? Thanks!
Gerry
Quoted 3 lines
Jim

Last know working version on the Dell laptop is 3.16 and yes this message"

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +2 -3

-Local APIC disabled by BIOS -- you can enable it with "lapic"
-APIC: disable apic facility
-APIC: switched to apic NOOP
+Local APIC disabled by BIOS -- reenabling.
+Found and enabled local APIC!

is from the v4.0-rc6 message. Full dmesg attached.

Jim.

Jim Bos

author

2015-04-11 15:08 UTC #9 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56 & c50f13c672df7

In reply to Jiang Liu

Quoted 63 lines · Jiang Liu

Quoted 5 lines · Jim Bos
Quoted 4 lines · Jiang Liu
Quoted 3 lines · Jim Bos
Quoted 2 lines · Jiang Liu
Quoted 1 line · Jim Bos
On 04/07/2015 04:34 PM, Jiang Liu wrote:
<snip>
Quoted 38 lines
Quoted 14 lines
Hi Jim,
I'm really confused. I can't even explain why my previous
patch fixes the issue on AMD geode board now:(
For the Dell laptop, seems you have:
1) build a kernel with Local APIC and IOAPIC enabled
2) lapic is disabled by BIOS, so there's no ACPI MADT(APIC) table at all.
That means the laptop is working with 8259 PICs only. There's little change between 3.16 and 4.0 related to 8259.
For the AMD geode board, I still think original code is right. I can't explain why the patch fix the issue.
Quoted 12 lines
So could you please help to:
1) Try to enable lapic on Dell laptop in BIOS
2) Dump acpi tables and dmesg on AMD board
If that still doesn't help, I will try to send you some debug patches to gather more info.
Thanks!
Gerry
Quoted 3 lines
_
Jim
Gerry,
As you mentioned your patch shouldn't make a difference, run some more tests, as it turns out:
- geode system broken on 3.16+ up to and including 3.19, however, on plain 4.0-rc6 it works! Root cause appears to be there isn't an ACPI interrupt assigned in non-working kernels.
- other system I got my hands on: Pentium(R) CPU G3220, broken on 3.19.0 when boot parameter 'nosmp' is specified, again no acpi entry in /proc/interrupts, working fine on 4.0-rc6
Quoted 1 line
So obviously between 3.19 and 4.0-rc6 something got fixed here!
Hi Jim,
Yes, the bugfix patch should be:
commit 1ea76fbadd66("x86/irq: Fix regression caused by commit b568b8601f05")
Quoted 13 lines
The Dell laptop remains the only problem then on 4.0-rc6, there IS an acpi interrupt (but firing once apparently).
There isn't an option in BIOS to enable LAPIC, however, when specifying 'lapic' as boot parameter I got interesting result, still not working and /proc/interrups still shows XT-PIC. Doing a diff between dmesg on 3.19 and 4.0-rc6 this pops out:
(quoted diff) +2 -3
-Local APIC disabled by BIOS -- you can enable it with "lapic"
-APIC: disable apic facility
-APIC: switched to apic NOOP
+Local APIC disabled by BIOS -- reenabling.
+Found and enabled local APIC!

Quoted 11 lines

Quoted 1 line
+Enabling APIC mode: Flat. Using 0 I/O APICs
What's the last know working kernel for Dell laptop? Does it work as expected with v3.19 kernel?
Do you means this message is from plain v4.0-rc6 kernel? Thanks!
Gerry
Quoted 3 lines
Jim

Gerry, Rafael,

Ok, found it.
It was very 'interesting' bisect, because there were 2 overlapping issues here. On the Dell laptop some kernels there wasn't an ACPI interrupt at all or it fired once and then seemed to get stuck. Turns out that kernel version:
3.16: OK
3.17: Broken (no acpi interrupt)
3.18: actually fine
3.19: Broken

So started bisecting between 3.18 or 3.19, in the end found Rafael's patch which broke it:

==
commit c50f13c672df758b59e026c15b9118f3ed46edc4

Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Dec 1 23:50:16 2014 +0100

ACPICA: Save current masks of enabled GPEs after enable register writes
==

Reverting that patch on top of 4.0-rc7 (with some offsets and one trivial manual edit) and I finally got a working ACPI interrupt again!

Jim

Jiang Liu

2015-04-11 16:57 UTC #10 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56 & c50f13c672df7

Quoted 93 lines · Jim Bos

Quoted 63 lines · Jiang Liu
Quoted 5 lines · Jim Bos
Quoted 4 lines · Jiang Liu
Quoted 3 lines · Jim Bos
Quoted 2 lines · Jiang Liu
Quoted 1 line · Jim Bos
On 04/07/2015 04:34 PM, Jiang Liu wrote:
<snip>
Quoted 38 lines
Quoted 14 lines
Hi Jim,
I'm really confused. I can't even explain why my previous
patch fixes the issue on AMD geode board now:(
For the Dell laptop, seems you have:
1) build a kernel with Local APIC and IOAPIC enabled
2) lapic is disabled by BIOS, so there's no ACPI MADT(APIC) table at all.
That means the laptop is working with 8259 PICs only. There's little change between 3.16 and 4.0 related to 8259.
For the AMD geode board, I still think original code is right. I can't explain why the patch fix the issue.
Quoted 12 lines
So could you please help to:
1) Try to enable lapic on Dell laptop in BIOS
2) Dump acpi tables and dmesg on AMD board
If that still doesn't help, I will try to send you some debug patches to gather more info.
Thanks!
Gerry
Quoted 3 lines
_
Jim
Gerry,
As you mentioned your patch shouldn't make a difference, run some more tests, as it turns out:
- geode system broken on 3.16+ up to and including 3.19, however, on plain 4.0-rc6 it works! Root cause appears to be there isn't an ACPI interrupt assigned in non-working kernels.
- other system I got my hands on: Pentium(R) CPU G3220, broken on 3.19.0 when boot parameter 'nosmp' is specified, again no acpi entry in /proc/interrupts, working fine on 4.0-rc6
Quoted 1 line
So obviously between 3.19 and 4.0-rc6 something got fixed here!
Hi Jim,
Yes, the bugfix patch should be:
commit 1ea76fbadd66("x86/irq: Fix regression caused by commit b568b8601f05")
Quoted 13 lines
The Dell laptop remains the only problem then on 4.0-rc6, there IS an acpi interrupt (but firing once apparently).
There isn't an option in BIOS to enable LAPIC, however, when specifying 'lapic' as boot parameter I got interesting result, still not working and /proc/interrups still shows XT-PIC. Doing a diff between dmesg on 3.19 and 4.0-rc6 this pops out:
(quoted diff) +2 -3
-Local APIC disabled by BIOS -- you can enable it with "lapic"
-APIC: disable apic facility
-APIC: switched to apic NOOP
+Local APIC disabled by BIOS -- reenabling.
+Found and enabled local APIC!
Quoted 11 lines
Quoted 1 line
+Enabling APIC mode: Flat. Using 0 I/O APICs
What's the last know working kernel for Dell laptop? Does it work as expected with v3.19 kernel?
Do you means this message is from plain v4.0-rc6 kernel? Thanks!
Gerry
Quoted 3 lines
Jim
Gerry, Rafael,
Ok, found it.
It was very 'interesting' bisect, because there were 2 overlapping issues here. On the Dell laptop some kernels there wasn't an ACPI interrupt at all or it fired once and then seemed to get stuck. Turns out that kernel version:
3.16: OK
3.17: Broken (no acpi interrupt)
3.18: actually fine
3.19: Broken
So started bisecting between 3.18 or 3.19, in the end found Rafael's patch which broke it:
==
commit c50f13c672df758b59e026c15b9118f3ed46edc4

Quoted 8 lines

Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Dec 1 23:50:16 2014 +0100
ACPICA: Save current masks of enabled GPEs after enable register writes
==
Reverting that patch on top of 4.0-rc7 (with some offsets and one trivial manual edit) and I finally got a working ACPI interrupt again!

Hi Jim,
Thanks for your great effort to bisect the regression.
And it gets reasonable now:)
Thanks!
Gerry

Quoted 5 lines

Jim

Rafael J. Wysocki

2015-04-12 01:04 UTC #11 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56 & c50f13c672df7

In reply to Jim Bos

Quoted 93 lines · Jim Bos

Quoted 63 lines · Jiang Liu
Quoted 5 lines · Jim Bos
Quoted 4 lines · Jiang Liu
Quoted 3 lines · Jim Bos
Quoted 2 lines · Jiang Liu
Quoted 1 line · Jim Bos
On 04/07/2015 04:34 PM, Jiang Liu wrote:
<snip>
Quoted 38 lines
Quoted 14 lines
Hi Jim,
I'm really confused. I can't even explain why my previous
patch fixes the issue on AMD geode board now:(
For the Dell laptop, seems you have:
1) build a kernel with Local APIC and IOAPIC enabled
2) lapic is disabled by BIOS, so there's no ACPI MADT(APIC) table at all.
That means the laptop is working with 8259 PICs only. There's little change between 3.16 and 4.0 related to 8259.
For the AMD geode board, I still think original code is right. I can't explain why the patch fix the issue.
Quoted 12 lines
So could you please help to:
1) Try to enable lapic on Dell laptop in BIOS
2) Dump acpi tables and dmesg on AMD board
If that still doesn't help, I will try to send you some debug patches to gather more info.
Thanks!
Gerry
Quoted 3 lines
_
Jim
Gerry,
As you mentioned your patch shouldn't make a difference, run some more tests, as it turns out:
- geode system broken on 3.16+ up to and including 3.19, however, on plain 4.0-rc6 it works! Root cause appears to be there isn't an ACPI interrupt assigned in non-working kernels.
- other system I got my hands on: Pentium(R) CPU G3220, broken on 3.19.0 when boot parameter 'nosmp' is specified, again no acpi entry in /proc/interrupts, working fine on 4.0-rc6
Quoted 1 line
So obviously between 3.19 and 4.0-rc6 something got fixed here!
Hi Jim,
Yes, the bugfix patch should be:
commit 1ea76fbadd66("x86/irq: Fix regression caused by commit b568b8601f05")
Quoted 13 lines
The Dell laptop remains the only problem then on 4.0-rc6, there IS an acpi interrupt (but firing once apparently).
There isn't an option in BIOS to enable LAPIC, however, when specifying 'lapic' as boot parameter I got interesting result, still not working and /proc/interrups still shows XT-PIC. Doing a diff between dmesg on 3.19 and 4.0-rc6 this pops out:
(quoted diff) +2 -3
-Local APIC disabled by BIOS -- you can enable it with "lapic"
-APIC: disable apic facility
-APIC: switched to apic NOOP
+Local APIC disabled by BIOS -- reenabling.
+Found and enabled local APIC!
Quoted 11 lines
Quoted 1 line
+Enabling APIC mode: Flat. Using 0 I/O APICs
What's the last know working kernel for Dell laptop? Does it work as expected with v3.19 kernel?
Do you means this message is from plain v4.0-rc6 kernel? Thanks!
Gerry
Quoted 3 lines
Jim
Gerry, Rafael,
Ok, found it.
It was very 'interesting' bisect, because there were 2 overlapping issues here. On the Dell laptop some kernels there wasn't an ACPI interrupt at all or it fired once and then seemed to get stuck. Turns out that kernel version:
3.16: OK
3.17: Broken (no acpi interrupt)
3.18: actually fine
3.19: Broken
So started bisecting between 3.18 or 3.19, in the end found Rafael's patch which broke it:
==
commit c50f13c672df758b59e026c15b9118f3ed46edc4

Quoted 8 lines

Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Dec 1 23:50:16 2014 +0100
ACPICA: Save current masks of enabled GPEs after enable register writes
==
Reverting that patch on top of 4.0-rc7 (with some offsets and one trivial manual edit) and I finally got a working ACPI interrupt again!

That's unexpected.

Is system suspend/resume involved in the reproduction of the problem in any way?

In any case, does it help if you replace "enable_mask" with "enable_for_run" in line 127 of drivers/acpi/acpica/hwgpe.c (without reverting the whole commit)?

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Jim Bos

author Tested-by

2015-04-12 09:03 UTC #12 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56 & c50f13c672df7

Quoted 103 lines · Rafael J. Wysocki

Quoted 93 lines · Jim Bos
Quoted 63 lines · Jiang Liu
Quoted 5 lines · Jim Bos
Quoted 4 lines · Jiang Liu
Quoted 3 lines · Jim Bos
Quoted 2 lines · Jiang Liu
Quoted 1 line · Jim Bos
On 04/07/2015 04:34 PM, Jiang Liu wrote:
<snip>
Quoted 38 lines
Quoted 14 lines
Hi Jim,
I'm really confused. I can't even explain why my previous
patch fixes the issue on AMD geode board now:(
For the Dell laptop, seems you have:
1) build a kernel with Local APIC and IOAPIC enabled
2) lapic is disabled by BIOS, so there's no ACPI MADT(APIC) table at all.
That means the laptop is working with 8259 PICs only. There's little change between 3.16 and 4.0 related to 8259.
For the AMD geode board, I still think original code is right. I can't explain why the patch fix the issue.
Quoted 12 lines
So could you please help to:
1) Try to enable lapic on Dell laptop in BIOS
2) Dump acpi tables and dmesg on AMD board
If that still doesn't help, I will try to send you some debug patches to gather more info.
Thanks!
Gerry
Quoted 3 lines
_
Jim
Gerry,
As you mentioned your patch shouldn't make a difference, run some more tests, as it turns out:
- geode system broken on 3.16+ up to and including 3.19, however, on plain 4.0-rc6 it works! Root cause appears to be there isn't an ACPI interrupt assigned in non-working kernels.
- other system I got my hands on: Pentium(R) CPU G3220, broken on 3.19.0 when boot parameter 'nosmp' is specified, again no acpi entry in /proc/interrupts, working fine on 4.0-rc6
Quoted 1 line
So obviously between 3.19 and 4.0-rc6 something got fixed here!
Hi Jim,
Yes, the bugfix patch should be:
commit 1ea76fbadd66("x86/irq: Fix regression caused by commit b568b8601f05")
Quoted 13 lines
The Dell laptop remains the only problem then on 4.0-rc6, there IS an acpi interrupt (but firing once apparently).
There isn't an option in BIOS to enable LAPIC, however, when specifying 'lapic' as boot parameter I got interesting result, still not working and /proc/interrups still shows XT-PIC. Doing a diff between dmesg on 3.19 and 4.0-rc6 this pops out:
(quoted diff) +2 -3
-Local APIC disabled by BIOS -- you can enable it with "lapic"
-APIC: disable apic facility
-APIC: switched to apic NOOP
+Local APIC disabled by BIOS -- reenabling.
+Found and enabled local APIC!
Quoted 11 lines
Quoted 1 line
+Enabling APIC mode: Flat. Using 0 I/O APICs
What's the last know working kernel for Dell laptop? Does it work as expected with v3.19 kernel?
Do you means this message is from plain v4.0-rc6 kernel? Thanks!
Gerry
Quoted 3 lines
Jim
Gerry, Rafael,
Ok, found it.
It was very 'interesting' bisect, because there were 2 overlapping issues here. On the Dell laptop some kernels there wasn't an ACPI interrupt at all or it fired once and then seemed to get stuck. Turns out that kernel version:
3.16: OK
3.17: Broken (no acpi interrupt)
3.18: actually fine
3.19: Broken
So started bisecting between 3.18 or 3.19, in the end found Rafael's patch which broke it:
==
commit c50f13c672df758b59e026c15b9118f3ed46edc4
Quoted 8 lines
Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Dec 1 23:50:16 2014 +0100
ACPICA: Save current masks of enabled GPEs after enable register writes
==
Reverting that patch on top of 4.0-rc7 (with some offsets and one trivial manual edit) and I finally got a working ACPI interrupt again!

Quoted 8 lines

That's unexpected.
Is system suspend/resume involved in the reproduction of the problem in any way?
In any case, does it help if you replace "enable_mask" with "enable_for_run" in line 127 of drivers/acpi/acpica/hwgpe.c (without reverting the whole commit)?

No suspends/resumes, but this suggestion works :-)

hwgpe.c 2015-04-12 10:42:38.021283593 +0200 renamed from hwgpe.c.ORIG 2015-04-12 10:41:11.754104398 +0200 +0 -0

@@ -124,7 +124,7 @@
 
                /* Only enable if the corresponding enable_mask bit is

set */

- if (!(register_bit & gpe_register_info->enable_mask)) {

+               if (!(register_bit & gpe_register_info->enable_for_run)) {
                        return (AE_BAD_PARAMETER);
                }

Tested-by Jim Bos <[email protected]>

Thanks,
Jim

hwgpe.c renamed from hwgpe.c.ORIG +1 -1

@@ -124,7 +124,7 @@
 
 		/* Only enable if the corresponding enable_mask bit is set */
 
-		if (!(register_bit & gpe_register_info->enable_mask)) {
+		if (!(register_bit & gpe_register_info->enable_for_run)) {
 			return (AE_BAD_PARAMETER);
 		}

Rafael J. Wysocki

2015-04-12 23:05 UTC #13 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56 & c50f13c672df7

Quoted 106 lines · Jim Bos

Quoted 105 lines · Rafael J. Wysocki
Quoted 93 lines · Jim Bos
Quoted 63 lines · Jiang Liu
Quoted 5 lines · Jim Bos
Quoted 4 lines · Jiang Liu
Quoted 3 lines · Jim Bos
Quoted 2 lines · Jiang Liu
Quoted 1 line · Jim Bos
On 04/07/2015 04:34 PM, Jiang Liu wrote:
<snip>
Quoted 38 lines
Quoted 14 lines
Hi Jim,
I'm really confused. I can't even explain why my previous
patch fixes the issue on AMD geode board now:(
For the Dell laptop, seems you have:
1) build a kernel with Local APIC and IOAPIC enabled
2) lapic is disabled by BIOS, so there's no ACPI MADT(APIC) table at all.
That means the laptop is working with 8259 PICs only. There's little change between 3.16 and 4.0 related to 8259.
For the AMD geode board, I still think original code is right. I can't explain why the patch fix the issue.
Quoted 12 lines
So could you please help to:
1) Try to enable lapic on Dell laptop in BIOS
2) Dump acpi tables and dmesg on AMD board
If that still doesn't help, I will try to send you some debug patches to gather more info.
Thanks!
Gerry
Quoted 3 lines
_
Jim
Gerry,
As you mentioned your patch shouldn't make a difference, run some more tests, as it turns out:
- geode system broken on 3.16+ up to and including 3.19, however, on plain 4.0-rc6 it works! Root cause appears to be there isn't an ACPI interrupt assigned in non-working kernels.
- other system I got my hands on: Pentium(R) CPU G3220, broken on 3.19.0 when boot parameter 'nosmp' is specified, again no acpi entry in /proc/interrupts, working fine on 4.0-rc6
Quoted 1 line
So obviously between 3.19 and 4.0-rc6 something got fixed here!
Hi Jim,
Yes, the bugfix patch should be:
commit 1ea76fbadd66("x86/irq: Fix regression caused by commit b568b8601f05")
Quoted 13 lines
The Dell laptop remains the only problem then on 4.0-rc6, there IS an acpi interrupt (but firing once apparently).
There isn't an option in BIOS to enable LAPIC, however, when specifying 'lapic' as boot parameter I got interesting result, still not working and /proc/interrups still shows XT-PIC. Doing a diff between dmesg on 3.19 and 4.0-rc6 this pops out:
(quoted diff) +2 -3
-Local APIC disabled by BIOS -- you can enable it with "lapic"
-APIC: disable apic facility
-APIC: switched to apic NOOP
+Local APIC disabled by BIOS -- reenabling.
+Found and enabled local APIC!
Quoted 11 lines
Quoted 1 line
+Enabling APIC mode: Flat. Using 0 I/O APICs
What's the last know working kernel for Dell laptop? Does it work as expected with v3.19 kernel?
Do you means this message is from plain v4.0-rc6 kernel? Thanks!
Gerry
Quoted 3 lines
Jim
Gerry, Rafael,
Ok, found it.
It was very 'interesting' bisect, because there were 2 overlapping issues here. On the Dell laptop some kernels there wasn't an ACPI interrupt at all or it fired once and then seemed to get stuck. Turns out that kernel version:
3.16: OK
3.17: Broken (no acpi interrupt)
3.18: actually fine
3.19: Broken
So started bisecting between 3.18 or 3.19, in the end found Rafael's patch which broke it:
==
commit c50f13c672df758b59e026c15b9118f3ed46edc4
Quoted 8 lines
Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Dec 1 23:50:16 2014 +0100
ACPICA: Save current masks of enabled GPEs after enable register writes
==
Reverting that patch on top of 4.0-rc7 (with some offsets and one trivial manual edit) and I finally got a working ACPI interrupt again!
That's unexpected.

Quoted 8 lines

Quoted 6 lines
Is system suspend/resume involved in the reproduction of the problem in any way?
In any case, does it help if you replace "enable_mask" with "enable_for_run" in line 127 of drivers/acpi/acpica/hwgpe.c (without reverting the whole commit)?
No suspends/resumes, but this suggestion works :-)

Quoted 12 lines

hwgpe.c 2015-04-12 10:42:38.021283593 +0200 renamed from hwgpe.c.ORIG 2015-04-12 10:41:11.754104398 +0200 +0 -0
@@ -124,7 +124,7 @@
 
                /* Only enable if the corresponding enable_mask bit is
set */
hwgpe.c 2015-04-12 10:42:38.021283593 +0200 renamed from hwgpe.c.ORIG 2015-04-12 10:41:11.754104398 +0200 +1 -1
-               if (!(register_bit & gpe_register_info->enable_mask)) {
+               if (!(register_bit & gpe_register_info->enable_for_run)) {
                        return (AE_BAD_PARAMETER);
                }

Quoted 1 line

Tested-by Jim Bos <[email protected]>

No, no, this is not a fix. :-)

It means, though, that enable_for_run and enable_mask diverge at one point and, moreover, enable_for_run has more bits set, which is *really* mysterious.

So the only place modifying enable_for_run is acpi_ev_update_gpe_enable_mask() which roughly does this:
(a) Find the register bit corresponding to the given GPE.
(b) Clear that bit in enable_for_run.
(c) If runtime_count is set for the GPE, set that bit in enable_for_run.
Thus the only case when a bit may be set in enable_for_run is when runtime_count is nonzero for the GPE in acpi_ev_update_gpe_enable_mask().

Now, acpi_ev_update_gpe_enable_mask() is only called from two places, acpi_ev_add_gpe_reference() and acpi_ev_remove_gpe_reference(). The former calls it when runtime_count has just been incremented and is now equal to one and the latter calls it when runtime_count has just been decremented and is now equal to zero. Hence, if all of the involved functions return AE_OK, acpi_ev_add_gpe_reference() may set the corresponding bit in enable_for_run and acpi_ev_remove_gpe_reference() may clear it.

Further, having set the bit in enable_for_run, acpi_ev_add_gpe_reference() calls acpi_ev_enable_gpe() which then calls acpi_hw_low_set_gpe() with ACPI_GPE_SAVE_MASK set in the second argument. Again, this causes the corresponding bit to be set in the register's enable_mask, unless any errors are returned. In that case the bit will be set in both enable_for_run and enable_mask and analogously for acpi_ev_remove_gpe_reference(). So if I'm not overlooking anything and if all of the involved calls are successful, enable_for_run and enable_mask will always be in sync.

As far as I can say that may change *only* if there's an error, because in that case (1) we may not save the mask that we attempted to write to the register and
(2) we will reset runtime_count *without* updating enable_for_run which arguably is a bug. So the previous patch might just work accidentally.

If that theory holds any water, the patch below may help too (instead of the previous one), so please test it. If it doesn't help, we'll need to find out what exactly happens on that system, but surely it is *not* usual behavior.

---
 drivers/acpi/acpica/hwgpe.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/acpica/hwgpe.c +1 -1

Index: linux-pm/drivers/acpi/acpica/hwgpe.c
===================================================================
@@ -149,7 +149,7 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 	/* Write the updated enable mask */
 
 	status = acpi_hw_write(enable_mask, &gpe_register_info->enable_address);
-	if (ACPI_SUCCESS(status) && (action & ACPI_GPE_SAVE_MASK)) {
+	if (action & ACPI_GPE_SAVE_MASK) {
 		gpe_register_info->enable_mask = (u8)enable_mask;
 	}
 	return (status);

Rafael J. Wysocki

2015-04-13 13:12 UTC #14 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56 & c50f13c672df7

Quoted 162 lines · Rafael J. Wysocki

Quoted 106 lines · Jim Bos
Quoted 105 lines · Rafael J. Wysocki
Quoted 93 lines · Jim Bos
Quoted 63 lines · Jiang Liu
Quoted 5 lines · Jim Bos
Quoted 4 lines · Jiang Liu
Quoted 3 lines · Jim Bos
Quoted 2 lines · Jiang Liu
Quoted 1 line · Jim Bos
On 04/07/2015 04:34 PM, Jiang Liu wrote:
<snip>
Quoted 38 lines
Quoted 14 lines
Hi Jim,
I'm really confused. I can't even explain why my previous
patch fixes the issue on AMD geode board now:(
For the Dell laptop, seems you have:
1) build a kernel with Local APIC and IOAPIC enabled
2) lapic is disabled by BIOS, so there's no ACPI MADT(APIC) table at all.
That means the laptop is working with 8259 PICs only. There's little change between 3.16 and 4.0 related to 8259.
For the AMD geode board, I still think original code is right. I can't explain why the patch fix the issue.
Quoted 12 lines
So could you please help to:
1) Try to enable lapic on Dell laptop in BIOS
2) Dump acpi tables and dmesg on AMD board
If that still doesn't help, I will try to send you some debug patches to gather more info.
Thanks!
Gerry
Quoted 3 lines
_
Jim
Gerry,
As you mentioned your patch shouldn't make a difference, run some more tests, as it turns out:
- geode system broken on 3.16+ up to and including 3.19, however, on plain 4.0-rc6 it works! Root cause appears to be there isn't an ACPI interrupt assigned in non-working kernels.
- other system I got my hands on: Pentium(R) CPU G3220, broken on 3.19.0 when boot parameter 'nosmp' is specified, again no acpi entry in /proc/interrupts, working fine on 4.0-rc6
Quoted 1 line
So obviously between 3.19 and 4.0-rc6 something got fixed here!
Hi Jim,
Yes, the bugfix patch should be:
commit 1ea76fbadd66("x86/irq: Fix regression caused by commit b568b8601f05")
Quoted 13 lines
The Dell laptop remains the only problem then on 4.0-rc6, there IS an acpi interrupt (but firing once apparently).
There isn't an option in BIOS to enable LAPIC, however, when specifying 'lapic' as boot parameter I got interesting result, still not working and /proc/interrups still shows XT-PIC. Doing a diff between dmesg on 3.19 and 4.0-rc6 this pops out:
(quoted diff) +2 -3
-Local APIC disabled by BIOS -- you can enable it with "lapic"
-APIC: disable apic facility
-APIC: switched to apic NOOP
+Local APIC disabled by BIOS -- reenabling.
+Found and enabled local APIC!
Quoted 11 lines
Quoted 1 line
+Enabling APIC mode: Flat. Using 0 I/O APICs
What's the last know working kernel for Dell laptop? Does it work as expected with v3.19 kernel?
Do you means this message is from plain v4.0-rc6 kernel? Thanks!
Gerry
Quoted 3 lines
Jim
Gerry, Rafael,
Ok, found it.
It was very 'interesting' bisect, because there were 2 overlapping issues here. On the Dell laptop some kernels there wasn't an ACPI interrupt at all or it fired once and then seemed to get stuck. Turns out that kernel version:
3.16: OK
3.17: Broken (no acpi interrupt)
3.18: actually fine
3.19: Broken
So started bisecting between 3.18 or 3.19, in the end found Rafael's patch which broke it:
==
commit c50f13c672df758b59e026c15b9118f3ed46edc4
Quoted 8 lines
Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Dec 1 23:50:16 2014 +0100
ACPICA: Save current masks of enabled GPEs after enable register writes
==
Reverting that patch on top of 4.0-rc7 (with some offsets and one trivial manual edit) and I finally got a working ACPI interrupt again!
That's unexpected.
Quoted 8 lines
Quoted 6 lines
Is system suspend/resume involved in the reproduction of the problem in any way?
In any case, does it help if you replace "enable_mask" with "enable_for_run" in line 127 of drivers/acpi/acpica/hwgpe.c (without reverting the whole commit)?
No suspends/resumes, but this suggestion works :-)
OK
Quoted 12 lines
hwgpe.c 2015-04-12 10:42:38.021283593 +0200 renamed from hwgpe.c.ORIG 2015-04-12 10:41:11.754104398 +0200 +0 -0
@@ -124,7 +124,7 @@
 
                /* Only enable if the corresponding enable_mask bit is
set */
hwgpe.c 2015-04-12 10:42:38.021283593 +0200 renamed from hwgpe.c.ORIG 2015-04-12 10:41:11.754104398 +0200 +1 -1
-               if (!(register_bit & gpe_register_info->enable_mask)) {
+               if (!(register_bit & gpe_register_info->enable_for_run)) {
                        return (AE_BAD_PARAMETER);
                }
Quoted 1 line
Tested-by Jim Bos <[email protected]>
No, no, this is not a fix. :-)
It means, though, that enable_for_run and enable_mask diverge at one point and, moreover, enable_for_run has more bits set, which is *really* mysterious.
So the only place modifying enable_for_run is acpi_ev_update_gpe_enable_mask() which roughly does this:
(a) Find the register bit corresponding to the given GPE.
(b) Clear that bit in enable_for_run.
(c) If runtime_count is set for the GPE, set that bit in enable_for_run.
Thus the only case when a bit may be set in enable_for_run is when runtime_count is nonzero for the GPE in acpi_ev_update_gpe_enable_mask().
Now, acpi_ev_update_gpe_enable_mask() is only called from two places, acpi_ev_add_gpe_reference() and acpi_ev_remove_gpe_reference(). The former calls it when runtime_count has just been incremented and is now equal to one and the latter calls it when runtime_count has just been decremented and is now equal to zero. Hence, if all of the involved functions return AE_OK, acpi_ev_add_gpe_reference() may set the corresponding bit in enable_for_run and acpi_ev_remove_gpe_reference() may clear it.
Further, having set the bit in enable_for_run, acpi_ev_add_gpe_reference() calls acpi_ev_enable_gpe() which then calls acpi_hw_low_set_gpe() with ACPI_GPE_SAVE_MASK set in the second argument. Again, this causes the corresponding bit to be set in the register's enable_mask, unless any errors are returned. In that case the bit will be set in both enable_for_run and enable_mask and analogously for acpi_ev_remove_gpe_reference(). So if I'm not overlooking anything and if all of the involved calls are successful, enable_for_run and enable_mask will always be in sync.

Quoted 8 lines

As far as I can say that may change *only* if there's an error, because in that case (1) we may not save the mask that we attempted to write to the register and
(2) we will reset runtime_count *without* updating enable_for_run which arguably is a bug. So the previous patch might just work accidentally.
If that theory holds any water, the patch below may help too (instead of the previous one), so please test it. If it doesn't help, we'll need to find out what exactly happens on that system, but surely it is *not* usual behavior.

Actually, the one below is better, so please test this one instead.

---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPICA: Store GPE register enable masks upfront

It is reported that ACPI interrupts do not work any more on Dell Latitude D600 after commit c50f13c672df (ACPICA: Save current masks of enabled GPEs after enable register writes). The problem turns out to be related to the fact that the enable_mask and enable_for_run GPE bit masks are not in sync (in the absence of any system suspend/resume events) for at least one GPE register on that machine.

Address this problem by writing the enable_for_run mask into enable_mask as soon as enable_for_run is updated instead of doing that only after the subsequent register write has succeeded. For consistency, update acpi_hw_gpe_enable_write() to store the bit mask to be written into the GPE register in enable_mask unconditionally before the write.

Since the ACPI_GPE_SAVE_MASK flag is not necessary any more after that, drop it along with the symbols depending on it.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/acpica/evgpe.c |    5 +++--
 drivers/acpi/acpica/hwgpe.c |   11 ++++-------
 include/acpi/actypes.h      |    4 ----
 3 files changed, 7 insertions(+), 13 deletions(-)

drivers/acpi/acpica/hwgpe.c +4 -7

Index: linux-pm/drivers/acpi/acpica/hwgpe.c
===================================================================
@@ -89,6 +89,8 @@ u32 acpi_hw_get_gpe_register_bit(struct
  * RETURN:	Status
  *
  * DESCRIPTION: Enable or disable a single GPE in the parent enable register.
+ *              The enable_mask field of the involved GPE register structure
+ *              must be updated by the caller if necessary.
  *
  ******************************************************************************/

@@ -119,7 +121,7 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 	/* Set or clear just the bit that corresponds to this GPE */
 
 	register_bit = acpi_hw_get_gpe_register_bit(gpe_event_info);
-	switch (action & ~ACPI_GPE_SAVE_MASK) {
+	switch (action) {
 	case ACPI_GPE_CONDITIONAL_ENABLE:
 
 		/* Only enable if the corresponding enable_mask bit is set */

@@ -149,9 +151,6 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 	/* Write the updated enable mask */
 
 	status = acpi_hw_write(enable_mask, &gpe_register_info->enable_address);
-	if (ACPI_SUCCESS(status) && (action & ACPI_GPE_SAVE_MASK)) {
-		gpe_register_info->enable_mask = (u8)enable_mask;
-	}
 	return (status);
 }

@@ -286,10 +285,8 @@ acpi_hw_gpe_enable_write(u8 enable_mask,
 {
 	acpi_status status;
 
+	gpe_register_info->enable_mask = enable_mask;
 	status = acpi_hw_write(enable_mask, &gpe_register_info->enable_address);
-	if (ACPI_SUCCESS(status)) {
-		gpe_register_info->enable_mask = enable_mask;
-	}
 	return (status);
 }

include/acpi/actypes.h +0 -4

Index: linux-pm/include/acpi/actypes.h
===================================================================
@@ -736,10 +736,6 @@ typedef u32 acpi_event_status;
 #define ACPI_GPE_ENABLE                 0
 #define ACPI_GPE_DISABLE                1
 #define ACPI_GPE_CONDITIONAL_ENABLE     2
-#define ACPI_GPE_SAVE_MASK              4
-
-#define ACPI_GPE_ENABLE_SAVE            (ACPI_GPE_ENABLE | ACPI_GPE_SAVE_MASK)
-#define ACPI_GPE_DISABLE_SAVE           (ACPI_GPE_DISABLE | ACPI_GPE_SAVE_MASK)
 
 /*
  * GPE info flags - Per GPE

drivers/acpi/acpica/evgpe.c +3 -2

Index: linux-pm/drivers/acpi/acpica/evgpe.c
===================================================================
@@ -92,6 +92,7 @@ acpi_ev_update_gpe_enable_mask(struct ac
 		ACPI_SET_BIT(gpe_register_info->enable_for_run,
 			     (u8)register_bit);
 	}
+	gpe_register_info->enable_mask = gpe_register_info->enable_for_run;
 
 	return_ACPI_STATUS(AE_OK);
 }

@@ -123,7 +124,7 @@ acpi_status acpi_ev_enable_gpe(struct ac
 
 	/* Enable the requested GPE */
 
-	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE_SAVE);
+	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
 	return_ACPI_STATUS(status);
 }

@@ -202,7 +203,7 @@ acpi_ev_remove_gpe_reference(struct acpi
 		if (ACPI_SUCCESS(status)) {
 			status =
 			    acpi_hw_low_set_gpe(gpe_event_info,
-						ACPI_GPE_DISABLE_SAVE);
+						ACPI_GPE_DISABLE);
 		}
 
 		if (ACPI_FAILURE(status)) {

Jim Bos

author Tested-by

2015-04-13 14:54 UTC #15 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56 & c50f13c672df7

Quoted 294 lines · Rafael J. Wysocki

Quoted 162 lines · Rafael J. Wysocki
Quoted 106 lines · Jim Bos
Quoted 105 lines · Rafael J. Wysocki
Quoted 93 lines · Jim Bos
Quoted 63 lines · Jiang Liu
Quoted 5 lines · Jim Bos
Quoted 4 lines · Jiang Liu
Quoted 3 lines · Jim Bos
Quoted 2 lines · Jiang Liu
Quoted 1 line · Jim Bos
On 04/07/2015 04:34 PM, Jiang Liu wrote:
<snip>
Quoted 38 lines
Quoted 14 lines
Hi Jim,
I'm really confused. I can't even explain why my previous
patch fixes the issue on AMD geode board now:(
For the Dell laptop, seems you have:
1) build a kernel with Local APIC and IOAPIC enabled
2) lapic is disabled by BIOS, so there's no ACPI MADT(APIC) table at all.
That means the laptop is working with 8259 PICs only. There's little change between 3.16 and 4.0 related to 8259.
For the AMD geode board, I still think original code is right. I can't explain why the patch fix the issue.
Quoted 12 lines
So could you please help to:
1) Try to enable lapic on Dell laptop in BIOS
2) Dump acpi tables and dmesg on AMD board
If that still doesn't help, I will try to send you some debug patches to gather more info.
Thanks!
Gerry
Quoted 3 lines
_
Jim
Gerry,
As you mentioned your patch shouldn't make a difference, run some more tests, as it turns out:
- geode system broken on 3.16+ up to and including 3.19, however, on plain 4.0-rc6 it works! Root cause appears to be there isn't an ACPI interrupt assigned in non-working kernels.
- other system I got my hands on: Pentium(R) CPU G3220, broken on 3.19.0 when boot parameter 'nosmp' is specified, again no acpi entry in /proc/interrupts, working fine on 4.0-rc6
Quoted 1 line
So obviously between 3.19 and 4.0-rc6 something got fixed here!
Hi Jim,
Yes, the bugfix patch should be:
commit 1ea76fbadd66("x86/irq: Fix regression caused by commit b568b8601f05")
Quoted 13 lines
The Dell laptop remains the only problem then on 4.0-rc6, there IS an acpi interrupt (but firing once apparently).
There isn't an option in BIOS to enable LAPIC, however, when specifying 'lapic' as boot parameter I got interesting result, still not working and /proc/interrups still shows XT-PIC. Doing a diff between dmesg on 3.19 and 4.0-rc6 this pops out:
(quoted diff) +2 -3
-Local APIC disabled by BIOS -- you can enable it with "lapic"
-APIC: disable apic facility
-APIC: switched to apic NOOP
+Local APIC disabled by BIOS -- reenabling.
+Found and enabled local APIC!
Quoted 11 lines
Quoted 1 line
+Enabling APIC mode: Flat. Using 0 I/O APICs
What's the last know working kernel for Dell laptop? Does it work as expected with v3.19 kernel?
Do you means this message is from plain v4.0-rc6 kernel? Thanks!
Gerry
Quoted 3 lines
Jim
Gerry, Rafael,
Ok, found it.
It was very 'interesting' bisect, because there were 2 overlapping issues here. On the Dell laptop some kernels there wasn't an ACPI interrupt at all or it fired once and then seemed to get stuck. Turns out that kernel version:
3.16: OK
3.17: Broken (no acpi interrupt)
3.18: actually fine
3.19: Broken
So started bisecting between 3.18 or 3.19, in the end found Rafael's patch which broke it:
==
commit c50f13c672df758b59e026c15b9118f3ed46edc4
Quoted 8 lines
Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Dec 1 23:50:16 2014 +0100
ACPICA: Save current masks of enabled GPEs after enable register writes
==
Reverting that patch on top of 4.0-rc7 (with some offsets and one trivial manual edit) and I finally got a working ACPI interrupt again!
That's unexpected.
Quoted 8 lines
Quoted 6 lines
Is system suspend/resume involved in the reproduction of the problem in any way?
In any case, does it help if you replace "enable_mask" with "enable_for_run" in line 127 of drivers/acpi/acpica/hwgpe.c (without reverting the whole commit)?
No suspends/resumes, but this suggestion works :-)
OK
Quoted 12 lines
hwgpe.c 2015-04-12 10:42:38.021283593 +0200 renamed from hwgpe.c.ORIG 2015-04-12 10:41:11.754104398 +0200 +0 -0
@@ -124,7 +124,7 @@
 
                /* Only enable if the corresponding enable_mask bit is
set */
hwgpe.c 2015-04-12 10:42:38.021283593 +0200 renamed from hwgpe.c.ORIG 2015-04-12 10:41:11.754104398 +0200 +1 -1
-               if (!(register_bit & gpe_register_info->enable_mask)) {
+               if (!(register_bit & gpe_register_info->enable_for_run)) {
                        return (AE_BAD_PARAMETER);
                }
Quoted 1 line
Tested-by Jim Bos <[email protected]>
No, no, this is not a fix. :-)
It means, though, that enable_for_run and enable_mask diverge at one point and, moreover, enable_for_run has more bits set, which is *really* mysterious.
So the only place modifying enable_for_run is acpi_ev_update_gpe_enable_mask() which roughly does this:
(a) Find the register bit corresponding to the given GPE.
(b) Clear that bit in enable_for_run.
(c) If runtime_count is set for the GPE, set that bit in enable_for_run.
Thus the only case when a bit may be set in enable_for_run is when runtime_count is nonzero for the GPE in acpi_ev_update_gpe_enable_mask().
Now, acpi_ev_update_gpe_enable_mask() is only called from two places, acpi_ev_add_gpe_reference() and acpi_ev_remove_gpe_reference(). The former calls it when runtime_count has just been incremented and is now equal to one and the latter calls it when runtime_count has just been decremented and is now equal to zero. Hence, if all of the involved functions return AE_OK, acpi_ev_add_gpe_reference() may set the corresponding bit in enable_for_run and acpi_ev_remove_gpe_reference() may clear it.
Further, having set the bit in enable_for_run, acpi_ev_add_gpe_reference() calls acpi_ev_enable_gpe() which then calls acpi_hw_low_set_gpe() with ACPI_GPE_SAVE_MASK set in the second argument. Again, this causes the corresponding bit to be set in the register's enable_mask, unless any errors are returned. In that case the bit will be set in both enable_for_run and enable_mask and analogously for acpi_ev_remove_gpe_reference(). So if I'm not overlooking anything and if all of the involved calls are successful, enable_for_run and enable_mask will always be in sync.
Quoted 8 lines
As far as I can say that may change *only* if there's an error, because in that case (1) we may not save the mask that we attempted to write to the register and
(2) we will reset runtime_count *without* updating enable_for_run which arguably is a bug. So the previous patch might just work accidentally.
If that theory holds any water, the patch below may help too (instead of the previous one), so please test it. If it doesn't help, we'll need to find out what exactly happens on that system, but surely it is *not* usual behavior.
Actually, the one below is better, so please test this one instead.
---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPICA: Store GPE register enable masks upfront
It is reported that ACPI interrupts do not work any more on Dell Latitude D600 after commit c50f13c672df (ACPICA: Save current masks of enabled GPEs after enable register writes). The problem turns out to be related to the fact that the enable_mask and enable_for_run GPE bit masks are not in sync (in the absence of any system suspend/resume events) for at least one GPE register on that machine.
Address this problem by writing the enable_for_run mask into enable_mask as soon as enable_for_run is updated instead of doing that only after the subsequent register write has succeeded. For consistency, update acpi_hw_gpe_enable_write() to store the bit mask to be written into the GPE register in enable_mask unconditionally before the write.
Since the ACPI_GPE_SAVE_MASK flag is not necessary any more after that, drop it along with the symbols depending on it.
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/acpi/acpica/evgpe.c |    5 +++--
 drivers/acpi/acpica/hwgpe.c |   11 ++++-------
 include/acpi/actypes.h      |    4 ----
 3 files changed, 7 insertions(+), 13 deletions(-)
Index: linux-pm/drivers/acpi/acpica/hwgpe.c
===================================================================
drivers/acpi/acpica/hwgpe.c +4 -7
@@ -89,6 +89,8 @@ u32 acpi_hw_get_gpe_register_bit(struct
  * RETURN:	Status
  *
  * DESCRIPTION: Enable or disable a single GPE in the parent enable register.
+ *              The enable_mask field of the involved GPE register structure
+ *              must be updated by the caller if necessary.
  *
  ******************************************************************************/
 
@@ -119,7 +121,7 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 	/* Set or clear just the bit that corresponds to this GPE */
 
 	register_bit = acpi_hw_get_gpe_register_bit(gpe_event_info);
-	switch (action & ~ACPI_GPE_SAVE_MASK) {
+	switch (action) {
 	case ACPI_GPE_CONDITIONAL_ENABLE:
 
 		/* Only enable if the corresponding enable_mask bit is set */
@@ -149,9 +151,6 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 	/* Write the updated enable mask */
 
 	status = acpi_hw_write(enable_mask, &gpe_register_info->enable_address);
-	if (ACPI_SUCCESS(status) && (action & ACPI_GPE_SAVE_MASK)) {
-		gpe_register_info->enable_mask = (u8)enable_mask;
-	}
 	return (status);
 }
 
@@ -286,10 +285,8 @@ acpi_hw_gpe_enable_write(u8 enable_mask,
 {
 	acpi_status status;
 
+	gpe_register_info->enable_mask = enable_mask;
 	status = acpi_hw_write(enable_mask, &gpe_register_info->enable_address);
-	if (ACPI_SUCCESS(status)) {
-		gpe_register_info->enable_mask = enable_mask;
-	}
 	return (status);
 }
 
Index: linux-pm/include/acpi/actypes.h
===================================================================
include/acpi/actypes.h +0 -4
@@ -736,10 +736,6 @@ typedef u32 acpi_event_status;
 #define ACPI_GPE_ENABLE                 0
 #define ACPI_GPE_DISABLE                1
 #define ACPI_GPE_CONDITIONAL_ENABLE     2
-#define ACPI_GPE_SAVE_MASK              4
-
-#define ACPI_GPE_ENABLE_SAVE            (ACPI_GPE_ENABLE | ACPI_GPE_SAVE_MASK)
-#define ACPI_GPE_DISABLE_SAVE           (ACPI_GPE_DISABLE | ACPI_GPE_SAVE_MASK)
 
 /*
  * GPE info flags - Per GPE
Index: linux-pm/drivers/acpi/acpica/evgpe.c
===================================================================
drivers/acpi/acpica/evgpe.c +3 -3
@@ -92,6 +92,7 @@ acpi_ev_update_gpe_enable_mask(struct ac
 		ACPI_SET_BIT(gpe_register_info->enable_for_run,
 			     (u8)register_bit);
 	}
+	gpe_register_info->enable_mask = gpe_register_info->enable_for_run;
 
 	return_ACPI_STATUS(AE_OK);
 }
@@ -123,7 +124,7 @@ acpi_status acpi_ev_enable_gpe(struct ac
 
 	/* Enable the requested GPE */
 
-	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE_SAVE);
+	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
 	return_ACPI_STATUS(status);
 }
 
@@ -202,7 +203,7 @@ acpi_ev_remove_gpe_reference(struct acpi
 		if (ACPI_SUCCESS(status)) {
 			status =
 			    acpi_hw_low_set_gpe(gpe_event_info,
-						ACPI_GPE_DISABLE_SAVE);
+						ACPI_GPE_DISABLE);
 		}
 
 		if (ACPI_FAILURE(status)) {
 
--

Quoted 5 lines

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Works! If this is actually the fix ;-)

Tested-by Jim Bos <[email protected]>

_
Jim

Rafael J. Wysocki

2015-04-13 19:22 UTC #16 lore raw

Re: [PATCH] x86/ACPI: Fix regression caused by 16ee7b3dcc56 & c50f13c672df7

Quoted 303 lines · Jim Bos

Quoted 294 lines · Rafael J. Wysocki
Quoted 162 lines · Rafael J. Wysocki
Quoted 106 lines · Jim Bos
Quoted 105 lines · Rafael J. Wysocki
Quoted 93 lines · Jim Bos
Quoted 63 lines · Jiang Liu
Quoted 5 lines · Jim Bos
Quoted 4 lines · Jiang Liu
Quoted 3 lines · Jim Bos
Quoted 2 lines · Jiang Liu
Quoted 1 line · Jim Bos
On 04/07/2015 04:34 PM, Jiang Liu wrote:
<snip>
Quoted 38 lines
Quoted 14 lines
Hi Jim,
I'm really confused. I can't even explain why my previous
patch fixes the issue on AMD geode board now:(
For the Dell laptop, seems you have:
1) build a kernel with Local APIC and IOAPIC enabled
2) lapic is disabled by BIOS, so there's no ACPI MADT(APIC) table at all.
That means the laptop is working with 8259 PICs only. There's little change between 3.16 and 4.0 related to 8259.
For the AMD geode board, I still think original code is right. I can't explain why the patch fix the issue.
Quoted 12 lines
So could you please help to:
1) Try to enable lapic on Dell laptop in BIOS
2) Dump acpi tables and dmesg on AMD board
If that still doesn't help, I will try to send you some debug patches to gather more info.
Thanks!
Gerry
Quoted 3 lines
_
Jim
Gerry,
As you mentioned your patch shouldn't make a difference, run some more tests, as it turns out:
- geode system broken on 3.16+ up to and including 3.19, however, on plain 4.0-rc6 it works! Root cause appears to be there isn't an ACPI interrupt assigned in non-working kernels.
- other system I got my hands on: Pentium(R) CPU G3220, broken on 3.19.0 when boot parameter 'nosmp' is specified, again no acpi entry in /proc/interrupts, working fine on 4.0-rc6
Quoted 1 line
So obviously between 3.19 and 4.0-rc6 something got fixed here!
Hi Jim,
Yes, the bugfix patch should be:
commit 1ea76fbadd66("x86/irq: Fix regression caused by commit b568b8601f05")
Quoted 13 lines
The Dell laptop remains the only problem then on 4.0-rc6, there IS an acpi interrupt (but firing once apparently).
There isn't an option in BIOS to enable LAPIC, however, when specifying 'lapic' as boot parameter I got interesting result, still not working and /proc/interrups still shows XT-PIC. Doing a diff between dmesg on 3.19 and 4.0-rc6 this pops out:
(quoted diff) +2 -3
-Local APIC disabled by BIOS -- you can enable it with "lapic"
-APIC: disable apic facility
-APIC: switched to apic NOOP
+Local APIC disabled by BIOS -- reenabling.
+Found and enabled local APIC!
Quoted 11 lines
Quoted 1 line
+Enabling APIC mode: Flat. Using 0 I/O APICs
What's the last know working kernel for Dell laptop? Does it work as expected with v3.19 kernel?
Do you means this message is from plain v4.0-rc6 kernel? Thanks!
Gerry
Quoted 3 lines
Jim
Gerry, Rafael,
Ok, found it.
It was very 'interesting' bisect, because there were 2 overlapping issues here. On the Dell laptop some kernels there wasn't an ACPI interrupt at all or it fired once and then seemed to get stuck. Turns out that kernel version:
3.16: OK
3.17: Broken (no acpi interrupt)
3.18: actually fine
3.19: Broken
So started bisecting between 3.18 or 3.19, in the end found Rafael's patch which broke it:
==
commit c50f13c672df758b59e026c15b9118f3ed46edc4
Quoted 8 lines
Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Dec 1 23:50:16 2014 +0100
ACPICA: Save current masks of enabled GPEs after enable register writes
==
Reverting that patch on top of 4.0-rc7 (with some offsets and one trivial manual edit) and I finally got a working ACPI interrupt again!
That's unexpected.
Quoted 8 lines
Quoted 6 lines
Is system suspend/resume involved in the reproduction of the problem in any way?
In any case, does it help if you replace "enable_mask" with "enable_for_run" in line 127 of drivers/acpi/acpica/hwgpe.c (without reverting the whole commit)?
No suspends/resumes, but this suggestion works :-)
OK
Quoted 12 lines
hwgpe.c 2015-04-12 10:42:38.021283593 +0200 renamed from hwgpe.c.ORIG 2015-04-12 10:41:11.754104398 +0200 +0 -0
@@ -124,7 +124,7 @@
 
                /* Only enable if the corresponding enable_mask bit is
set */
hwgpe.c 2015-04-12 10:42:38.021283593 +0200 renamed from hwgpe.c.ORIG 2015-04-12 10:41:11.754104398 +0200 +1 -1
-               if (!(register_bit & gpe_register_info->enable_mask)) {
+               if (!(register_bit & gpe_register_info->enable_for_run)) {
                        return (AE_BAD_PARAMETER);
                }
Quoted 1 line
Tested-by Jim Bos <[email protected]>
No, no, this is not a fix. :-)
It means, though, that enable_for_run and enable_mask diverge at one point and, moreover, enable_for_run has more bits set, which is *really* mysterious.
So the only place modifying enable_for_run is acpi_ev_update_gpe_enable_mask() which roughly does this:
(a) Find the register bit corresponding to the given GPE.
(b) Clear that bit in enable_for_run.
(c) If runtime_count is set for the GPE, set that bit in enable_for_run.
Thus the only case when a bit may be set in enable_for_run is when runtime_count is nonzero for the GPE in acpi_ev_update_gpe_enable_mask().
Now, acpi_ev_update_gpe_enable_mask() is only called from two places, acpi_ev_add_gpe_reference() and acpi_ev_remove_gpe_reference(). The former calls it when runtime_count has just been incremented and is now equal to one and the latter calls it when runtime_count has just been decremented and is now equal to zero. Hence, if all of the involved functions return AE_OK, acpi_ev_add_gpe_reference() may set the corresponding bit in enable_for_run and acpi_ev_remove_gpe_reference() may clear it.
Further, having set the bit in enable_for_run, acpi_ev_add_gpe_reference() calls acpi_ev_enable_gpe() which then calls acpi_hw_low_set_gpe() with ACPI_GPE_SAVE_MASK set in the second argument. Again, this causes the corresponding bit to be set in the register's enable_mask, unless any errors are returned. In that case the bit will be set in both enable_for_run and enable_mask and analogously for acpi_ev_remove_gpe_reference(). So if I'm not overlooking anything and if all of the involved calls are successful, enable_for_run and enable_mask will always be in sync.
Quoted 8 lines
As far as I can say that may change *only* if there's an error, because in that case (1) we may not save the mask that we attempted to write to the register and
(2) we will reset runtime_count *without* updating enable_for_run which arguably is a bug. So the previous patch might just work accidentally.
If that theory holds any water, the patch below may help too (instead of the previous one), so please test it. If it doesn't help, we'll need to find out what exactly happens on that system, but surely it is *not* usual behavior.
Actually, the one below is better, so please test this one instead.
---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPICA: Store GPE register enable masks upfront
It is reported that ACPI interrupts do not work any more on Dell Latitude D600 after commit c50f13c672df (ACPICA: Save current masks of enabled GPEs after enable register writes). The problem turns out to be related to the fact that the enable_mask and enable_for_run GPE bit masks are not in sync (in the absence of any system suspend/resume events) for at least one GPE register on that machine.
Address this problem by writing the enable_for_run mask into enable_mask as soon as enable_for_run is updated instead of doing that only after the subsequent register write has succeeded. For consistency, update acpi_hw_gpe_enable_write() to store the bit mask to be written into the GPE register in enable_mask unconditionally before the write.
Since the ACPI_GPE_SAVE_MASK flag is not necessary any more after that, drop it along with the symbols depending on it.
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/acpi/acpica/evgpe.c |    5 +++--
 drivers/acpi/acpica/hwgpe.c |   11 ++++-------
 include/acpi/actypes.h      |    4 ----
 3 files changed, 7 insertions(+), 13 deletions(-)
Index: linux-pm/drivers/acpi/acpica/hwgpe.c
===================================================================
drivers/acpi/acpica/hwgpe.c +4 -7
@@ -89,6 +89,8 @@ u32 acpi_hw_get_gpe_register_bit(struct
  * RETURN:	Status
  *
  * DESCRIPTION: Enable or disable a single GPE in the parent enable register.
+ *              The enable_mask field of the involved GPE register structure
+ *              must be updated by the caller if necessary.
  *
  ******************************************************************************/
 
@@ -119,7 +121,7 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 	/* Set or clear just the bit that corresponds to this GPE */
 
 	register_bit = acpi_hw_get_gpe_register_bit(gpe_event_info);
-	switch (action & ~ACPI_GPE_SAVE_MASK) {
+	switch (action) {
 	case ACPI_GPE_CONDITIONAL_ENABLE:
 
 		/* Only enable if the corresponding enable_mask bit is set */
@@ -149,9 +151,6 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 	/* Write the updated enable mask */
 
 	status = acpi_hw_write(enable_mask, &gpe_register_info->enable_address);
-	if (ACPI_SUCCESS(status) && (action & ACPI_GPE_SAVE_MASK)) {
-		gpe_register_info->enable_mask = (u8)enable_mask;
-	}
 	return (status);
 }
 
@@ -286,10 +285,8 @@ acpi_hw_gpe_enable_write(u8 enable_mask,
 {
 	acpi_status status;
 
+	gpe_register_info->enable_mask = enable_mask;
 	status = acpi_hw_write(enable_mask, &gpe_register_info->enable_address);
-	if (ACPI_SUCCESS(status)) {
-		gpe_register_info->enable_mask = enable_mask;
-	}
 	return (status);
 }
 
Index: linux-pm/include/acpi/actypes.h
===================================================================
include/acpi/actypes.h +0 -4
@@ -736,10 +736,6 @@ typedef u32 acpi_event_status;
 #define ACPI_GPE_ENABLE                 0
 #define ACPI_GPE_DISABLE                1
 #define ACPI_GPE_CONDITIONAL_ENABLE     2
-#define ACPI_GPE_SAVE_MASK              4
-
-#define ACPI_GPE_ENABLE_SAVE            (ACPI_GPE_ENABLE | ACPI_GPE_SAVE_MASK)
-#define ACPI_GPE_DISABLE_SAVE           (ACPI_GPE_DISABLE | ACPI_GPE_SAVE_MASK)
 
 /*
  * GPE info flags - Per GPE
Index: linux-pm/drivers/acpi/acpica/evgpe.c
===================================================================
drivers/acpi/acpica/evgpe.c +3 -3
@@ -92,6 +92,7 @@ acpi_ev_update_gpe_enable_mask(struct ac
 		ACPI_SET_BIT(gpe_register_info->enable_for_run,
 			     (u8)register_bit);
 	}
+	gpe_register_info->enable_mask = gpe_register_info->enable_for_run;
 
 	return_ACPI_STATUS(AE_OK);
 }
@@ -123,7 +124,7 @@ acpi_status acpi_ev_enable_gpe(struct ac
 
 	/* Enable the requested GPE */
 
-	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE_SAVE);
+	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
 	return_ACPI_STATUS(status);
 }
 
@@ -202,7 +203,7 @@ acpi_ev_remove_gpe_reference(struct acpi
 		if (ACPI_SUCCESS(status)) {
 			status =
 			    acpi_hw_low_set_gpe(gpe_event_info,
-						ACPI_GPE_DISABLE_SAVE);
+						ACPI_GPE_DISABLE);
 		}
 
 		if (ACPI_FAILURE(status)) {
 
--
Quoted 5 lines
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Works! If this is actually the fix ;-)

Quoted 1 line

Tested-by Jim Bos <[email protected]>

Yes, this is the fix I'd like to apply unless others have objections.

Thanks!

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.