Bug 15951 - commit 9630bdd9 changes behavior of the poweroff
Summary: commit 9630bdd9 changes behavior of the poweroff
Status: CLOSED CODE_FIX
Alias: None
Product: ACPI
Classification: Unclassified
Component: Power-Sleep-Wake (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Rafael J. Wysocki
URL:
Keywords:
Depends on:
Blocks: 7216 15310
  Show dependency tree
 
Reported: 2010-05-09 21:41 UTC by Rafael J. Wysocki
Modified: 2010-06-27 21:48 UTC (History)
5 users (show)

See Also:
Kernel Version: 2.6.34-rc
Subsystem:
Regression: Yes
Bisected commit-id:


Attachments
ACPI / ACPICA: Fix problems related to GPE reference counting (22.09 KB, patch)
2010-05-24 21:00 UTC, Rafael J. Wysocki
Details | Diff
Debug patch (1.53 KB, patch)
2010-06-11 15:32 UTC, Rafael J. Wysocki
Details | Diff
dmesg from boot with the debug patch (100.88 KB, text/plain)
2010-06-14 14:48 UTC, Michal Hocko
Details
ACPI: Avoid enabling GPEs for wakeup early (1.14 KB, patch)
2010-06-15 15:39 UTC, Rafael J. Wysocki
Details | Diff
ACPI: Do not enable button GPE (892 bytes, patch)
2010-06-15 19:57 UTC, Rafael J. Wysocki
Details | Diff
ACPICA: Debug disabling of GPEs (539 bytes, patch)
2010-06-15 22:42 UTC, Rafael J. Wysocki
Details | Diff
1st boot with the patch from comment 14 (102.20 KB, text/plain)
2010-06-16 12:55 UTC, Michal Hocko
Details
boot ofter power off with the patch from comment 14 (102.20 KB, text/plain)
2010-06-16 12:56 UTC, Michal Hocko
Details
ACPICA: Avoid updating GPE wakeup masks (1.45 KB, patch)
2010-06-16 14:38 UTC, Rafael J. Wysocki
Details | Diff
ACPI: Do not enable wakeup GPEs on poweroff (534 bytes, patch)
2010-06-16 14:49 UTC, Rafael J. Wysocki
Details | Diff
ACPI / PM: Do not enable wakeup GPEs permanently (3.91 KB, patch)
2010-06-17 00:01 UTC, Rafael J. Wysocki
Details | Diff

Description Rafael J. Wysocki 2010-05-09 21:41:39 UTC
Subject    : commit 9630bdd9 changes behavior of the poweroff - bug?
Submitter  : Michal Hocko <mhocko@suse.cz>
Notify-Also : Tony Vroon <tony@linx.net>
Date       : 2010-04-01 13:39
Message-ID : 20100401133923.GA4104@tiehlicka.suse.cz
References : http://marc.info/?l=linux-kernel&m=127012918316305&w=4
Handled-By : "Rafael J. Wysocki" <rjw@sisk.pl>

This entry is being used for tracking a regression from 2.6.33.  Please don't
close it until the problem is fixed in the mainline.
Comment 1 Rafael J. Wysocki 2010-05-09 22:34:50 UTC
Caused by:

commit 9630bdd9b15d2f489c646d8bc04b60e53eb5ec78
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date:   Wed Feb 17 23:41:07 2010 +0100

    ACPI: Use GPE reference counting to support shared GPEs

    Signed-off-by: Matthew Garrett <mjg@redhat.com>
    Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
    Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

First-Bad-Commit : 9630bdd9b15d2f489c646d8bc04b60e53eb5ec78
Comment 2 Rafael J. Wysocki 2010-05-13 21:01:22 UTC
I wonder if this patch helps: https://bugzilla.kernel.org/attachment.cgi?id=26362
Comment 3 Michal Hocko 2010-05-14 08:10:47 UTC
No it didn't help.
I have applied that patch on top of previous patches (and 2.6.34-rc5). Is that correct or should I test with the clean linus tree with your patch?

$ git diff v2.6.34-rc5..
diff --git a/drivers/acpi/acpica/evgpeblk.c b/drivers/acpi/acpica/evgpeblk.c       
index fef7219..b6e1e0c 100644                                                      
--- a/drivers/acpi/acpica/evgpeblk.c                                               
+++ b/drivers/acpi/acpica/evgpeblk.c                                               
@@ -367,7 +367,7 @@ acpi_ev_match_prw_and_gpe(acpi_handle obj_handle,              
        union acpi_operand_object *pkg_desc;                                       
        union acpi_operand_object *obj_desc;                                       
        u32 gpe_number;                                                            
-       acpi_status status;                                                        
diff --git a/drivers/acpi/acpica/evgpeblk.c b/drivers/acpi/acpica/evgpeblk.c
index fef7219..b6e1e0c 100644
--- a/drivers/acpi/acpica/evgpeblk.c
+++ b/drivers/acpi/acpica/evgpeblk.c
@@ -367,7 +367,7 @@ acpi_ev_match_prw_and_gpe(acpi_handle obj_handle,
        union acpi_operand_object *pkg_desc;
        union acpi_operand_object *obj_desc;
        u32 gpe_number;
-       acpi_status status;
diff --git a/drivers/acpi/acpica/evgpeblk.c b/drivers/acpi/acpica/evgpeblk.c
index fef7219..b6e1e0c 100644
--- a/drivers/acpi/acpica/evgpeblk.c
+++ b/drivers/acpi/acpica/evgpeblk.c
@@ -367,7 +367,7 @@ acpi_ev_match_prw_and_gpe(acpi_handle obj_handle,
        union acpi_operand_object *pkg_desc;
        union acpi_operand_object *obj_desc;
        u32 gpe_number;
-       acpi_status status;
+       acpi_status status = AE_OK;

        ACPI_FUNCTION_TRACE(ev_match_prw_and_gpe);

@@ -447,12 +447,13 @@ acpi_ev_match_prw_and_gpe(acpi_handle obj_handle,
                                                        gpe_block->
                                                        block_base_number];

+               status = acpi_ev_disable_gpe(gpe_event_info);
                gpe_event_info->flags |= ACPI_GPE_CAN_WAKE;
        }

       cleanup:
        acpi_ut_remove_reference(pkg_desc);
-       return_ACPI_STATUS(AE_OK);
+       return_ACPI_STATUS(status);
 }

 /*******************************************************************************
diff --git a/drivers/acpi/system.c b/drivers/acpi/system.c
index 4aaf249..2192fcd 100644
--- a/drivers/acpi/system.c
+++ b/drivers/acpi/system.c
@@ -389,10 +389,10 @@ static ssize_t counter_set(struct kobject *kobj,
        if (index < num_gpes) {
                if (!strcmp(buf, "disable\n") &&
                                (status & ACPI_EVENT_FLAG_ENABLED))
-                       result = acpi_set_gpe(handle, index, ACPI_GPE_DISABLE);
+                       result = acpi_disable_gpe(handle, index, ACPI_GPE_TYPE_RUNTIME);
                else if (!strcmp(buf, "enable\n") &&
                                !(status & ACPI_EVENT_FLAG_ENABLED))
-                       result = acpi_set_gpe(handle, index, ACPI_GPE_ENABLE);
+                       result = acpi_enable_gpe(handle, index, ACPI_GPE_TYPE_RUNTIME);
                else if (!strcmp(buf, "clear\n") &&
                                (status & ACPI_EVENT_FLAG_SET))
                        result = acpi_clear_gpe(handle, index, ACPI_NOT_ISR);
diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c
index 4b9d339..a0f4859 100644
--- a/drivers/acpi/wakeup.c
+++ b/drivers/acpi/wakeup.c
@@ -113,8 +113,9 @@ int __init acpi_wakeup_device_init(void)
                if (!dev->wakeup.flags.always_enabled ||
                    dev->wakeup.state.enabled)
                        continue;
-               acpi_enable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
+               /*acpi_enable_gpe(dev->wakeup.gpe_device, dev->wakeup.gpe_number,
                                ACPI_GPE_TYPE_WAKE);
+                */
                dev->wakeup.state.enabled = 1;
        }
        mutex_unlock(&acpi_device_lock);
Comment 4 Rafael J. Wysocki 2010-05-24 21:00:14 UTC
Created attachment 26533 [details]
ACPI / ACPICA: Fix problems related to GPE reference counting

Please test the attached patch (on top of 2.6.34 final).
Comment 5 Michal Hocko 2010-05-25 10:01:42 UTC
Unfortunately, I have to disappoint you again. This doesn't help as well.
I have tried the patch from comment 4 on the clean .34 kernel and the laptop/system behaves the very same way.
Comment 6 Zhang Rui 2010-06-09 07:42:05 UTC
Rafael,
is this problem also fixed by your GPE ref fixup patch set?
Comment 7 Rafael J. Wysocki 2010-06-09 08:45:19 UTC
Possibly, but not surely.  I think that further debugging should be done on top of that patchset, though.
Comment 9 Michal Hocko 2010-06-11 07:51:53 UTC
$ git shortlog v2.6.35-rc2..
Rafael J. Wysocki (5):
      ACPI / ACPICA: Use helper function for computing GPE masks
      ACPI / ACPICA: Fix low-level GPE manipulation code
      ACPI / ACPICA: Avoid writing full enable masks to GPE registers
      ACPI / ACPICA: Fix GPE initialization
      ACPI / ACPICA: Fix sysfs GPE interface

And unfortunately no success again.
Comment 10 Rafael J. Wysocki 2010-06-11 15:32:33 UTC
Created attachment 26728 [details]
Debug patch

Well, thanks.

Now, we can do some more serious debugging without the need to backport changes, though, so please apply this patch on top of the previous patchset and attach dmesg output from a fresh boot.
Comment 11 Michal Hocko 2010-06-14 14:48:42 UTC
Created attachment 26766 [details]
dmesg from boot with the debug patch
Comment 12 Rafael J. Wysocki 2010-06-15 15:39:48 UTC
Created attachment 26782 [details]
ACPI: Avoid enabling GPEs for wakeup early

Well, everything seems to be correct.

But, let's try this.  Please apply the attached patch and see if the problem is
still there.  Either way, please attach a dmesg output from a fresh boot.
Comment 13 Rafael J. Wysocki 2010-06-15 15:40:53 UTC
Ah, I forgot to say, please apply the patch from comment #12 on top of the
previous patches.
Comment 14 Rafael J. Wysocki 2010-06-15 19:57:09 UTC
Created attachment 26803 [details]
ACPI: Do not enable button GPE

In fact I think the patch from comment #12 won't help, so please apply this one (on top of the previous patches) instead and see if the problem is still there.  Please attach a dmesg output from a fresh boot in either case.
Comment 15 Rafael J. Wysocki 2010-06-15 22:42:17 UTC
Created attachment 26805 [details]
ACPICA: Debug disabling of GPEs

Also, regardless of the result of the previous test, please apply the attached
patch (on top of the previous ones), run "echo core > /sys/power/pm_test"
and "poweroff -u".  Then, please generate a dmesg output and attach it here.

In addition to that, please verify that acpi_power_off_prepare() is executed
on your box during poweroff.
Comment 16 Michal Hocko 2010-06-16 12:54:15 UTC
(In reply to comment #14)
> Created an attachment (id=26803) [details]
> ACPI: Do not enable button GPE
> 
> In fact I think the patch from comment #12 won't help, so please apply this
> one
> (on top of the previous patches) instead and see if the problem is still
> there.
>  Please attach a dmesg output from a fresh boot in either case.

It didn't help. I will upload both dmesgs (1st boot and boot after poweroff - they both look pretty much the same) shortly.
Comment 17 Michal Hocko 2010-06-16 12:55:17 UTC
Created attachment 26807 [details]
1st boot with the patch from comment 14
Comment 18 Michal Hocko 2010-06-16 12:56:02 UTC
Created attachment 26808 [details]
boot ofter power off with the patch from comment 14
Comment 19 Michal Hocko 2010-06-16 13:27:41 UTC
(In reply to comment #15)
> Created an attachment (id=26805) [details]
> ACPICA: Debug disabling of GPEs
> 
> Also, regardless of the result of the previous test, please apply the
> attached
> patch (on top of the previous ones), run "echo core > /sys/power/pm_test"
> and "poweroff -u".  

are you sure about -u parameter. My poweroff doesn't have such a parameter

> 
> In addition to that, please verify that acpi_power_off_prepare() is executed
> on your box during poweroff.

How do I do that?
Comment 20 Rafael J. Wysocki 2010-06-16 14:38:07 UTC
Created attachment 26809 [details]
ACPICA: Avoid updating GPE wakeup masks

Since the patch from comment #14 didn't help, please try this one (and attach
dmesg output).
Comment 21 Rafael J. Wysocki 2010-06-16 14:42:00 UTC
(In reply to comment #19)
> (In reply to comment #15)
> > Created an attachment (id=26805) [details] [details]
> > ACPICA: Debug disabling of GPEs
> > 
> > Also, regardless of the result of the previous test, please apply the
> attached
> > patch (on top of the previous ones), run "echo core > /sys/power/pm_test"
> > and "poweroff -u".  
> 
> are you sure about -u parameter. My poweroff doesn't have such a parameter

The parameter is correct, but the command is not. :-)

I meant "powersave", sorry.

> > In addition to that, please verify that acpi_power_off_prepare() is
> executed
> > on your box during poweroff.
> 
> How do I do that?

You can add something like "while (1) cpu_relax();" at the end of it and see
if the poweroff sequence stops instead of powering off the system.
Comment 22 Rafael J. Wysocki 2010-06-16 14:49:28 UTC
Created attachment 26810 [details]
ACPI: Do not enable wakeup GPEs on poweroff

Also, please check if the problem is still there with this patch applied.
Comment 23 Rafael J. Wysocki 2010-06-17 00:01:27 UTC
Created attachment 26820 [details]
ACPI / PM: Do not enable wakeup GPEs permanently

OK, I think I've found the issue.  It is described in the changelog of this
patch.

Please test it (on top of the patches from comment #8, the debug patch from
comment #10 and the one from comment #15) and report back.
Comment 24 Rafael J. Wysocki 2010-06-17 00:02:28 UTC
In case it doesn't help, please attach full dmesg output from a suspend/resume
cycle.
Comment 25 Michal Hocko 2010-06-17 11:38:23 UTC
OK, so recap what should be tested:
1) "echo core > /sys/power/pm_test
   powersave -u (btw. my debian box doesn't have this - what is the equivalent? - simple suspend to ram?)

2) check whether acpi_power_off_prepare is called during poweroff
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 3fb4bde..0fab34e 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -685,6 +685,8 @@ static void acpi_power_off_prepare(void)
        /* Prepare to power off the system */
        acpi_sleep_prepare(ACPI_STATE_S5);
        acpi_disable_all_gpes();
+
+       while (1) cpu_relax();
 }

 static void acpi_power_off(void)

3) try patch from comment 22

4) try patch from comment 23 if 3) fails

Is that correct?
Comment 26 Rafael J. Wysocki 2010-06-17 12:25:35 UTC
Please try the patch from comment #23 first, because that's what I think should
be applied (provided that it works).

If it doesn't work, please try the patches from comment #22 and from comment #20
both at the same time (on top of comment #8, comment #10, and comment #15).

Failing the above, please run the "echo core > ..." test with the patches from
comment #8, comment #10, comment #15, comment #20, and comment #22 applied and
attach the output of dmesg.

[On openSUSE "powersave -u" suspends to RAM, but it additionally executes some
user space suspend scripts around that.]
Comment 27 Michal Hocko 2010-06-17 14:20:54 UTC
(In reply to comment #26)
> Please try the patch from comment #23 first, because that's what I think
> should
> be applied (provided that it works).

Bingo! It seems to be working.

$ git shortlog v2.6.35-rc2..
Michal Hocko (2):
      Debugging patch from bko
      acpi: acpica debug disabling of gpes

Rafael J. Wysocki (6):
      ACPI / ACPICA: Use helper function for computing GPE masks
      ACPI / ACPICA: Fix low-level GPE manipulation code
      ACPI / ACPICA: Avoid writing full enable masks to GPE registers
      ACPI / ACPICA: Fix GPE initialization
      ACPI / ACPICA: Fix sysfs GPE interface
      ACPI / PM: Do not enable GPEs for system wakeup in advance

Do you need boot dmesg?
Comment 28 Rafael J. Wysocki 2010-06-17 15:24:14 UTC
(In reply to comment #27)
> (In reply to comment #26)
> > Please try the patch from comment #23 first, because that's what I think
> > should be applied (provided that it works).
> 
> Bingo! It seems to be working.

Good.

> $ git shortlog v2.6.35-rc2..
> Michal Hocko (2):
>       Debugging patch from bko
>       acpi: acpica debug disabling of gpes
> 
> Rafael J. Wysocki (6):
>       ACPI / ACPICA: Use helper function for computing GPE masks
>       ACPI / ACPICA: Fix low-level GPE manipulation code
>       ACPI / ACPICA: Avoid writing full enable masks to GPE registers
>       ACPI / ACPICA: Fix GPE initialization
>       ACPI / ACPICA: Fix sysfs GPE interface
>       ACPI / PM: Do not enable GPEs for system wakeup in advance
> 
> Do you need boot dmesg?

No, thanks.
Comment 29 Rafael J. Wysocki 2010-06-17 16:06:52 UTC
Patch : https://patchwork.kernel.org/patch/106701/
Comment 30 Len Brown 2010-06-17 16:19:15 UTC
patch is now applied to acpi tree

Note You need to log in before you can comment on or make changes to this bug.