Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Rafael J. Wysocki]

[The Lv's address is not valid any more, so drop it from the CC]

On Thursday, July 5, 2018 5:10:20 PM CEST Rafael J. Wysocki wrote:
> On Thu, Jul 5, 2018 at 5:09 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Thu, 05 Jul 2018 16:00:14 +0200,
> > Thomas H4nig wrote:
> >>
> >> Am 05.07.2018 um 14:12 schrieb Takashi Iwai:
> >> > On Thu, 05 Jul 2018 12:41:03 +0200,
> >> > Rafael J. Wysocki wrote:
> >> >>
> >> >> On Thursday, July 5, 2018 11:50:11 AM CEST Takashi Iwai wrote:
> >> >>> On Thu, 05 Jul 2018 11:34:59 +0200,
> >> >>> Rafael J. Wysocki wrote:
> >> >>>>
> >> >>>> Hi,
> >> >>>>
> >> >>>> On Thu, Jul 5, 2018 at 9:05 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >> >>>>> Hi,
> >> >>>>>
> >> >>>>> we've got a regression report since 4.17 about the behavior of
> >> >>>>> power-off with the power button.  When a machine is powered off with
> >> >>>>> the power button on desktop, it reboots after a few seconds instead of
> >> >>>>> power down.
> >> >>>>>
> >> >>>>> The manual power down via "systemctl poweroff" works fine, so it's
> >> >>>>> possibly some spurious wakeup by the power button action, and some
> >> >>>>> ACPI-related change is suspected.
> >> >>>>> The regression still remains in 4.18-rc3.
> >> >>>>
> >> >>>> There are only a few ACPI commits directly related to power management
> >> >>>> between 4.16 and 4.17 and none of them looks particularly suspicious.
> >> >>>
> >> >>> OK, interesting.
> >> >>>
> >> >>>> It looks like the power button state may not be cleared sufficiently
> >> >>>> after it's been pressed which is now visible for some reason.
> >> >>>
> >> >>> Hmm, where can such a state remain?  Since it happens after the
> >> >>> machine turned off, some (ACPI) wakeup bits?
> >> >>
> >> >> Basically, yes.
> >> >>
> >> >> It looks like a GPE may remain active which then triggers wakeup after
> >> >> shutdown.
> >> >>
> >> >> On a hunch, I'm wondering if reverting commit
> >> >>
> >> >> 18996f2db918 ACPICA: Events: Stop unconditionally clearing ACPI IRQs during suspend/resume
> >> >>
> >> >> (may not revert clearly, though) makes any difference.
> >> >
> >> > OK, I'm building a 4.17.x test kernel with that revert, in OBS
> >> > home:tiwai:bsc1099930 repo.
> >> >
> >> > Thomas, could you try later the kernel in
> >> >   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930/standard/
> >> > ?  It'll take an hour or so until the build finishes.
> >>
> >> With your new built kernel
> >> 4.17.4-1.g6f23755-default
> >>
> >> the power button works again, so the revert solved the problem
> >
> > Thanks, that clarifies the cause.
> > Adding Erik and Lv to Cc.
> >
> > I guess it's the side-effect by removing
> >         acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
> > in acpi_hw_disable_all_gpes().
> >
> > This function is called from acpi_power_off_prepare(), and the machine
> > goes to power off without clearing the GPEs, hence it's woken up later
> > unexpectedly.
> 
> That's correct.
> 
> We need to fix up that commit.  I'll try to prepare something.
> 

Below is a patch to test that theory and maybe fix things if it is correct.

What it does is to clear all GPEs after disabling them in
acpi_power_off_prepare() which should address the issue if our theory
about the underlying reason is correct.

Please test.

---
 drivers/acpi/acpica/achware.h |    2 ++
 drivers/acpi/acpica/evxfgpe.c |   33 ++++++++++++++++++++++++++++++++-
 drivers/acpi/acpica/hwgpe.c   |   24 +++++++++++++++++++++++-
 drivers/acpi/sleep.c          |    1 +
 include/acpi/acpixf.h         |    1 +
 5 files changed, 59 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/acpi/acpica/achware.h
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/achware.h
+++ linux-pm/drivers/acpi/acpica/achware.h
@@ -97,6 +97,8 @@ acpi_hw_get_gpe_status(struct acpi_gpe_e
 
 acpi_status acpi_hw_disable_all_gpes(void);
 
+acpi_status acpi_hw_clear_all_gpes(void);
+
 acpi_status acpi_hw_enable_all_runtime_gpes(void);
 
 acpi_status acpi_hw_enable_all_wakeup_gpes(void);
Index: linux-pm/drivers/acpi/acpica/hwgpe.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/hwgpe.c
+++ linux-pm/drivers/acpi/acpica/hwgpe.c
@@ -452,7 +452,7 @@ acpi_hw_enable_wakeup_gpe_block(struct a
  *
  * RETURN:      Status
  *
- * DESCRIPTION: Disable and clear all GPEs in all GPE blocks
+ * DESCRIPTION: Disable all GPEs in all GPE blocks
  *
  ******************************************************************************/
 
@@ -466,6 +466,28 @@ acpi_status acpi_hw_disable_all_gpes(voi
 	return_ACPI_STATUS(status);
 }
 
+/******************************************************************************
+ *
+ * FUNCTION:    acpi_hw_clear_all_gpes
+ *
+ * PARAMETERS:  None
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Clear all GPEs in all GPE blocks
+ *
+ ******************************************************************************/
+
+acpi_status acpi_hw_clear_all_gpes(void)
+{
+	acpi_status status;
+
+	ACPI_FUNCTION_TRACE(hw_clear_all_gpes);
+
+	status = acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
+	return_ACPI_STATUS(status);
+}
+
 /******************************************************************************
  *
  * FUNCTION:    acpi_hw_enable_all_runtime_gpes
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -1246,6 +1246,7 @@ static void acpi_power_off_prepare(void)
 	/* Prepare to power off the system */
 	acpi_sleep_prepare(ACPI_STATE_S5);
 	acpi_disable_all_gpes();
+	acpi_clear_all_gpes();
 	acpi_os_wait_events_complete();
 }
 
Index: linux-pm/drivers/acpi/acpica/evxfgpe.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/evxfgpe.c
+++ linux-pm/drivers/acpi/acpica/evxfgpe.c
@@ -710,7 +710,7 @@ ACPI_EXPORT_SYMBOL(acpi_finish_gpe)
  *
  * RETURN:      Status
  *
- * DESCRIPTION: Disable and clear all GPEs in all GPE blocks
+ * DESCRIPTION: Disable all GPEs in all GPE blocks
  *
  ******************************************************************************/
 
@@ -735,6 +735,37 @@ ACPI_EXPORT_SYMBOL(acpi_disable_all_gpes
 
 /******************************************************************************
  *
+ * FUNCTION:    acpi_clear_all_gpes
+ *
+ * PARAMETERS:  None
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Clear all GPEs in all GPE blocks
+ *
+ ******************************************************************************/
+
+acpi_status acpi_clear_all_gpes(void)
+{
+	acpi_status status;
+
+	ACPI_FUNCTION_TRACE(acpi_clear_all_gpes);
+
+	status = acpi_ut_acquire_mutex(ACPI_MTX_EVENTS);
+	if (ACPI_FAILURE(status)) {
+		return_ACPI_STATUS(status);
+	}
+
+	status = acpi_hw_clear_all_gpes();
+	(void)acpi_ut_release_mutex(ACPI_MTX_EVENTS);
+
+	return_ACPI_STATUS(status);
+}
+
+ACPI_EXPORT_SYMBOL(acpi_clear_all_gpes)
+
+/******************************************************************************
+ *
  * FUNCTION:    acpi_enable_all_runtime_gpes
  *
  * PARAMETERS:  None
Index: linux-pm/include/acpi/acpixf.h
===================================================================
--- linux-pm.orig/include/acpi/acpixf.h
+++ linux-pm/include/acpi/acpixf.h
@@ -755,6 +755,7 @@ ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_sta
 						     *event_status))
 ACPI_HW_DEPENDENT_RETURN_VOID(void acpi_dispatch_gpe(acpi_handle gpe_device, u32 gpe_number))
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_disable_all_gpes(void))
+ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_clear_all_gpes(void))
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_enable_all_runtime_gpes(void))
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_enable_all_wakeup_gpes(void))
 

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Takashi Iwai]

On Thu, 05 Jul 2018 18:02:11 +0200,
Rafael J. Wysocki wrote:
> 
> [The Lv's address is not valid any more, so drop it from the CC]
> 
> On Thursday, July 5, 2018 5:10:20 PM CEST Rafael J. Wysocki wrote:
> > On Thu, Jul 5, 2018 at 5:09 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > > On Thu, 05 Jul 2018 16:00:14 +0200,
> > > Thomas H4nig wrote:
> > >>
> > >> Am 05.07.2018 um 14:12 schrieb Takashi Iwai:
> > >> > On Thu, 05 Jul 2018 12:41:03 +0200,
> > >> > Rafael J. Wysocki wrote:
> > >> >>
> > >> >> On Thursday, July 5, 2018 11:50:11 AM CEST Takashi Iwai wrote:
> > >> >>> On Thu, 05 Jul 2018 11:34:59 +0200,
> > >> >>> Rafael J. Wysocki wrote:
> > >> >>>>
> > >> >>>> Hi,
> > >> >>>>
> > >> >>>> On Thu, Jul 5, 2018 at 9:05 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > >> >>>>> Hi,
> > >> >>>>>
> > >> >>>>> we've got a regression report since 4.17 about the behavior of
> > >> >>>>> power-off with the power button.  When a machine is powered off with
> > >> >>>>> the power button on desktop, it reboots after a few seconds instead of
> > >> >>>>> power down.
> > >> >>>>>
> > >> >>>>> The manual power down via "systemctl poweroff" works fine, so it's
> > >> >>>>> possibly some spurious wakeup by the power button action, and some
> > >> >>>>> ACPI-related change is suspected.
> > >> >>>>> The regression still remains in 4.18-rc3.
> > >> >>>>
> > >> >>>> There are only a few ACPI commits directly related to power management
> > >> >>>> between 4.16 and 4.17 and none of them looks particularly suspicious.
> > >> >>>
> > >> >>> OK, interesting.
> > >> >>>
> > >> >>>> It looks like the power button state may not be cleared sufficiently
> > >> >>>> after it's been pressed which is now visible for some reason.
> > >> >>>
> > >> >>> Hmm, where can such a state remain?  Since it happens after the
> > >> >>> machine turned off, some (ACPI) wakeup bits?
> > >> >>
> > >> >> Basically, yes.
> > >> >>
> > >> >> It looks like a GPE may remain active which then triggers wakeup after
> > >> >> shutdown.
> > >> >>
> > >> >> On a hunch, I'm wondering if reverting commit
> > >> >>
> > >> >> 18996f2db918 ACPICA: Events: Stop unconditionally clearing ACPI IRQs during suspend/resume
> > >> >>
> > >> >> (may not revert clearly, though) makes any difference.
> > >> >
> > >> > OK, I'm building a 4.17.x test kernel with that revert, in OBS
> > >> > home:tiwai:bsc1099930 repo.
> > >> >
> > >> > Thomas, could you try later the kernel in
> > >> >   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930/standard/
> > >> > ?  It'll take an hour or so until the build finishes.
> > >>
> > >> With your new built kernel
> > >> 4.17.4-1.g6f23755-default
> > >>
> > >> the power button works again, so the revert solved the problem
> > >
> > > Thanks, that clarifies the cause.
> > > Adding Erik and Lv to Cc.
> > >
> > > I guess it's the side-effect by removing
> > >         acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
> > > in acpi_hw_disable_all_gpes().
> > >
> > > This function is called from acpi_power_off_prepare(), and the machine
> > > goes to power off without clearing the GPEs, hence it's woken up later
> > > unexpectedly.
> > 
> > That's correct.
> > 
> > We need to fix up that commit.  I'll try to prepare something.
> > 
> 
> Below is a patch to test that theory and maybe fix things if it is correct.
> 
> What it does is to clear all GPEs after disabling them in
> acpi_power_off_prepare() which should address the issue if our theory
> about the underlying reason is correct.
> 
> Please test.

OK, building a new test kernel package in OBS home:tiwai:bsc1099930-2
repo.  It'll appear at
  http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930-2/standard/

Thomas, please give it a try later.


thanks,

Takashi

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Thomas Hänig]

Am 05.07.2018 um 18:56 schrieb Takashi Iwai:
> On Thu, 05 Jul 2018 18:02:11 +0200,
> Rafael J. Wysocki wrote:
>>
>> [The Lv's address is not valid any more, so drop it from the CC]
>>
>> On Thursday, July 5, 2018 5:10:20 PM CEST Rafael J. Wysocki wrote:
>>> On Thu, Jul 5, 2018 at 5:09 PM, Takashi Iwai <tiwai@suse.de> wrote:
>>>> On Thu, 05 Jul 2018 16:00:14 +0200,
>>>> Thomas H4nig wrote:
>>>>>
>>>>> Am 05.07.2018 um 14:12 schrieb Takashi Iwai:
>>>>>> On Thu, 05 Jul 2018 12:41:03 +0200,
>>>>>> Rafael J. Wysocki wrote:
>>>>>>>
>>>>>>> On Thursday, July 5, 2018 11:50:11 AM CEST Takashi Iwai wrote:
>>>>>>>> On Thu, 05 Jul 2018 11:34:59 +0200,
>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On Thu, Jul 5, 2018 at 9:05 AM, Takashi Iwai <tiwai@suse.de> wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> we've got a regression report since 4.17 about the behavior of
>>>>>>>>>> power-off with the power button.  When a machine is powered off with
>>>>>>>>>> the power button on desktop, it reboots after a few seconds instead of
>>>>>>>>>> power down.
>>>>>>>>>>
>>>>>>>>>> The manual power down via "systemctl poweroff" works fine, so it's
>>>>>>>>>> possibly some spurious wakeup by the power button action, and some
>>>>>>>>>> ACPI-related change is suspected.
>>>>>>>>>> The regression still remains in 4.18-rc3.
>>>>>>>>>
>>>>>>>>> There are only a few ACPI commits directly related to power management
>>>>>>>>> between 4.16 and 4.17 and none of them looks particularly suspicious.
>>>>>>>>
>>>>>>>> OK, interesting.
>>>>>>>>
>>>>>>>>> It looks like the power button state may not be cleared sufficiently
>>>>>>>>> after it's been pressed which is now visible for some reason.
>>>>>>>>
>>>>>>>> Hmm, where can such a state remain?  Since it happens after the
>>>>>>>> machine turned off, some (ACPI) wakeup bits?
>>>>>>>
>>>>>>> Basically, yes.
>>>>>>>
>>>>>>> It looks like a GPE may remain active which then triggers wakeup after
>>>>>>> shutdown.
>>>>>>>
>>>>>>> On a hunch, I'm wondering if reverting commit
>>>>>>>
>>>>>>> 18996f2db918 ACPICA: Events: Stop unconditionally clearing ACPI IRQs during suspend/resume
>>>>>>>
>>>>>>> (may not revert clearly, though) makes any difference.
>>>>>>
>>>>>> OK, I'm building a 4.17.x test kernel with that revert, in OBS
>>>>>> home:tiwai:bsc1099930 repo.
>>>>>>
>>>>>> Thomas, could you try later the kernel in
>>>>>>    http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930/standard/
>>>>>> ?  It'll take an hour or so until the build finishes.
>>>>>
>>>>> With your new built kernel
>>>>> 4.17.4-1.g6f23755-default
>>>>>
>>>>> the power button works again, so the revert solved the problem
>>>>
>>>> Thanks, that clarifies the cause.
>>>> Adding Erik and Lv to Cc.
>>>>
>>>> I guess it's the side-effect by removing
>>>>          acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
>>>> in acpi_hw_disable_all_gpes().
>>>>
>>>> This function is called from acpi_power_off_prepare(), and the machine
>>>> goes to power off without clearing the GPEs, hence it's woken up later
>>>> unexpectedly.
>>>
>>> That's correct.
>>>
>>> We need to fix up that commit.  I'll try to prepare something.
>>>
>>
>> Below is a patch to test that theory and maybe fix things if it is correct.
>>
>> What it does is to clear all GPEs after disabling them in
>> acpi_power_off_prepare() which should address the issue if our theory
>> about the underlying reason is correct.
>>
>> Please test.
> 
> OK, building a new test kernel package in OBS home:tiwai:bsc1099930-2
> repo.  It'll appear at
>    http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930-2/standard/
> 
> Thomas, please give it a try later.
> 
> 
> thanks,
> 
> Takashi

Later will have to be tomorrow morning (07:00 UTC+2) as until then I 
have no access to the machine in question.


Thanks
Thomas

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Thomas Hänig]

Am 05.07.2018 um 18:56 schrieb Takashi Iwai:
> On Thu, 05 Jul 2018 18:02:11 +0200,
> Rafael J. Wysocki wrote:
>>
>> [The Lv's address is not valid any more, so drop it from the CC]
>>
>> On Thursday, July 5, 2018 5:10:20 PM CEST Rafael J. Wysocki wrote:
>>> On Thu, Jul 5, 2018 at 5:09 PM, Takashi Iwai <tiwai@suse.de> wrote:
>>>> On Thu, 05 Jul 2018 16:00:14 +0200,
>>>> Thomas H4nig wrote:
>>>>>
>>>>> Am 05.07.2018 um 14:12 schrieb Takashi Iwai:
>>>>>> On Thu, 05 Jul 2018 12:41:03 +0200,
>>>>>> Rafael J. Wysocki wrote:
>>>>>>>
>>>>>>> On Thursday, July 5, 2018 11:50:11 AM CEST Takashi Iwai wrote:
>>>>>>>> On Thu, 05 Jul 2018 11:34:59 +0200,
>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On Thu, Jul 5, 2018 at 9:05 AM, Takashi Iwai <tiwai@suse.de> wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> we've got a regression report since 4.17 about the behavior of
>>>>>>>>>> power-off with the power button.  When a machine is powered off with
>>>>>>>>>> the power button on desktop, it reboots after a few seconds instead of
>>>>>>>>>> power down.
>>>>>>>>>>
>>>>>>>>>> The manual power down via "systemctl poweroff" works fine, so it's
>>>>>>>>>> possibly some spurious wakeup by the power button action, and some
>>>>>>>>>> ACPI-related change is suspected.
>>>>>>>>>> The regression still remains in 4.18-rc3.
>>>>>>>>>
>>>>>>>>> There are only a few ACPI commits directly related to power management
>>>>>>>>> between 4.16 and 4.17 and none of them looks particularly suspicious.
>>>>>>>>
>>>>>>>> OK, interesting.
>>>>>>>>
>>>>>>>>> It looks like the power button state may not be cleared sufficiently
>>>>>>>>> after it's been pressed which is now visible for some reason.
>>>>>>>>
>>>>>>>> Hmm, where can such a state remain?  Since it happens after the
>>>>>>>> machine turned off, some (ACPI) wakeup bits?
>>>>>>>
>>>>>>> Basically, yes.
>>>>>>>
>>>>>>> It looks like a GPE may remain active which then triggers wakeup after
>>>>>>> shutdown.
>>>>>>>
>>>>>>> On a hunch, I'm wondering if reverting commit
>>>>>>>
>>>>>>> 18996f2db918 ACPICA: Events: Stop unconditionally clearing ACPI IRQs during suspend/resume
>>>>>>>
>>>>>>> (may not revert clearly, though) makes any difference.
>>>>>>
>>>>>> OK, I'm building a 4.17.x test kernel with that revert, in OBS
>>>>>> home:tiwai:bsc1099930 repo.
>>>>>>
>>>>>> Thomas, could you try later the kernel in
>>>>>>   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930/standard/
>>>>>> ?  It'll take an hour or so until the build finishes.
>>>>>
>>>>> With your new built kernel
>>>>> 4.17.4-1.g6f23755-default
>>>>>
>>>>> the power button works again, so the revert solved the problem
>>>>
>>>> Thanks, that clarifies the cause.
>>>> Adding Erik and Lv to Cc.
>>>>
>>>> I guess it's the side-effect by removing
>>>>         acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
>>>> in acpi_hw_disable_all_gpes().
>>>>
>>>> This function is called from acpi_power_off_prepare(), and the machine
>>>> goes to power off without clearing the GPEs, hence it's woken up later
>>>> unexpectedly.
>>>
>>> That's correct.
>>>
>>> We need to fix up that commit.  I'll try to prepare something.
>>>
>>
>> Below is a patch to test that theory and maybe fix things if it is correct.
>>
>> What it does is to clear all GPEs after disabling them in
>> acpi_power_off_prepare() which should address the issue if our theory
>> about the underlying reason is correct.
>>
>> Please test.
> 
> OK, building a new test kernel package in OBS home:tiwai:bsc1099930-2
> repo.  It'll appear at
>   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930-2/standard/
> 
> Thomas, please give it a try later.
> 
> 
> thanks,
> 
> Takashi

I am sorry, but with your test kernel 4.17.4-1.g76c6238-default the
notebook again gets not properly powered off but restarts


Thomas

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Takashi Iwai]

On Fri, 06 Jul 2018 07:18:36 +0200,
Thomas H4nig wrote:
> 
> 
> 
> Am 05.07.2018 um 18:56 schrieb Takashi Iwai:
> > On Thu, 05 Jul 2018 18:02:11 +0200,
> > Rafael J. Wysocki wrote:
> >>
> >> [The Lv's address is not valid any more, so drop it from the CC]
> >>
> >> On Thursday, July 5, 2018 5:10:20 PM CEST Rafael J. Wysocki wrote:
> >>> On Thu, Jul 5, 2018 at 5:09 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >>>> On Thu, 05 Jul 2018 16:00:14 +0200,
> >>>> Thomas H4nig wrote:
> >>>>>
> >>>>> Am 05.07.2018 um 14:12 schrieb Takashi Iwai:
> >>>>>> On Thu, 05 Jul 2018 12:41:03 +0200,
> >>>>>> Rafael J. Wysocki wrote:
> >>>>>>>
> >>>>>>> On Thursday, July 5, 2018 11:50:11 AM CEST Takashi Iwai wrote:
> >>>>>>>> On Thu, 05 Jul 2018 11:34:59 +0200,
> >>>>>>>> Rafael J. Wysocki wrote:
> >>>>>>>>>
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> On Thu, Jul 5, 2018 at 9:05 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> we've got a regression report since 4.17 about the behavior of
> >>>>>>>>>> power-off with the power button.  When a machine is powered off with
> >>>>>>>>>> the power button on desktop, it reboots after a few seconds instead of
> >>>>>>>>>> power down.
> >>>>>>>>>>
> >>>>>>>>>> The manual power down via "systemctl poweroff" works fine, so it's
> >>>>>>>>>> possibly some spurious wakeup by the power button action, and some
> >>>>>>>>>> ACPI-related change is suspected.
> >>>>>>>>>> The regression still remains in 4.18-rc3.
> >>>>>>>>>
> >>>>>>>>> There are only a few ACPI commits directly related to power management
> >>>>>>>>> between 4.16 and 4.17 and none of them looks particularly suspicious.
> >>>>>>>>
> >>>>>>>> OK, interesting.
> >>>>>>>>
> >>>>>>>>> It looks like the power button state may not be cleared sufficiently
> >>>>>>>>> after it's been pressed which is now visible for some reason.
> >>>>>>>>
> >>>>>>>> Hmm, where can such a state remain?  Since it happens after the
> >>>>>>>> machine turned off, some (ACPI) wakeup bits?
> >>>>>>>
> >>>>>>> Basically, yes.
> >>>>>>>
> >>>>>>> It looks like a GPE may remain active which then triggers wakeup after
> >>>>>>> shutdown.
> >>>>>>>
> >>>>>>> On a hunch, I'm wondering if reverting commit
> >>>>>>>
> >>>>>>> 18996f2db918 ACPICA: Events: Stop unconditionally clearing ACPI IRQs during suspend/resume
> >>>>>>>
> >>>>>>> (may not revert clearly, though) makes any difference.
> >>>>>>
> >>>>>> OK, I'm building a 4.17.x test kernel with that revert, in OBS
> >>>>>> home:tiwai:bsc1099930 repo.
> >>>>>>
> >>>>>> Thomas, could you try later the kernel in
> >>>>>>   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930/standard/
> >>>>>> ?  It'll take an hour or so until the build finishes.
> >>>>>
> >>>>> With your new built kernel
> >>>>> 4.17.4-1.g6f23755-default
> >>>>>
> >>>>> the power button works again, so the revert solved the problem
> >>>>
> >>>> Thanks, that clarifies the cause.
> >>>> Adding Erik and Lv to Cc.
> >>>>
> >>>> I guess it's the side-effect by removing
> >>>>         acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
> >>>> in acpi_hw_disable_all_gpes().
> >>>>
> >>>> This function is called from acpi_power_off_prepare(), and the machine
> >>>> goes to power off without clearing the GPEs, hence it's woken up later
> >>>> unexpectedly.
> >>>
> >>> That's correct.
> >>>
> >>> We need to fix up that commit.  I'll try to prepare something.
> >>>
> >>
> >> Below is a patch to test that theory and maybe fix things if it is correct.
> >>
> >> What it does is to clear all GPEs after disabling them in
> >> acpi_power_off_prepare() which should address the issue if our theory
> >> about the underlying reason is correct.
> >>
> >> Please test.
> > 
> > OK, building a new test kernel package in OBS home:tiwai:bsc1099930-2
> > repo.  It'll appear at
> >   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930-2/standard/
> > 
> > Thomas, please give it a try later.
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> I am sorry, but with your test kernel 4.17.4-1.g76c6238-default the
> notebook again gets not properly powered off but restarts

Interesting.  The package version shows that the tested kernel must be
the right one.  (Though, it'd be good to double-check -- it's often
confusing if you have multiple same kernel versions on the system.)

If Rafael's patch doesn't work, we'd need to identify which change in
the commit 18996f2db918 has the effect.

Thomas, could you try to build & modify the kernel in your side?
Package build on OBS and test takes too long.

There are three places that remove the GPE-clearing in the patch.

One is in acpi_ev_enable_gpe():

--- a/drivers/acpi/acpica/evgpe.c
+++ b/drivers/acpi/acpica/evgpe.c
@@ -115,13 +115,6 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
 
 	ACPI_FUNCTION_TRACE(ev_enable_gpe);
 
-	/* Clear the GPE (of stale events) */
-
-	status = acpi_hw_clear_gpe(gpe_event_info);
-	if (ACPI_FAILURE(status)) {
-		return_ACPI_STATUS(status);
-	}
-
 	/* Enable the requested GPE */
 
 	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);


... the second one is in acpi_hw_disable_all_gpes():

--- a/drivers/acpi/acpica/hwgpe.c
+++ b/drivers/acpi/acpica/hwgpe.c
@@ -497,7 +497,6 @@ acpi_status acpi_hw_disable_all_gpes(void)
 	ACPI_FUNCTION_TRACE(hw_disable_all_gpes);
 
 	status = acpi_ev_walk_gpe_list(acpi_hw_disable_gpe_block, NULL);
-	status = acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
 	return_ACPI_STATUS(status);
 }
 

... and the last one is in acpi_hw_legacy_sleep():

--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -85,15 +85,8 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
 		return_ACPI_STATUS(status);
 	}
 
-	/* Clear all fixed and general purpose status bits */
-
-	status = acpi_hw_clear_acpi_status();
-	if (ACPI_FAILURE(status)) {
-		return_ACPI_STATUS(status);
-	}
-
 	/*
	 * 1) Disable/Clear all GPEs


The rest are only comments changes.

Try to revert each hunk manually one-by-one and figure out which one
actually causes the regression.  Maybe it'd be safer to test on the
4.17.x kernel, but you can check on 4.18-rc, too.


thanks,

Takashi

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Rafael J. Wysocki]

On Friday, July 6, 2018 8:55:18 AM CEST Takashi Iwai wrote:
> On Fri, 06 Jul 2018 07:18:36 +0200,
> Thomas H4nig wrote:
> > 
> > 
> > 
> > Am 05.07.2018 um 18:56 schrieb Takashi Iwai:
> > > On Thu, 05 Jul 2018 18:02:11 +0200,
> > > Rafael J. Wysocki wrote:
> > >>
> > >> [The Lv's address is not valid any more, so drop it from the CC]
> > >>
> > >> On Thursday, July 5, 2018 5:10:20 PM CEST Rafael J. Wysocki wrote:
> > >>> On Thu, Jul 5, 2018 at 5:09 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > >>>> On Thu, 05 Jul 2018 16:00:14 +0200,
> > >>>> Thomas H4nig wrote:
> > >>>>>
> > >>>>> Am 05.07.2018 um 14:12 schrieb Takashi Iwai:
> > >>>>>> On Thu, 05 Jul 2018 12:41:03 +0200,
> > >>>>>> Rafael J. Wysocki wrote:
> > >>>>>>>
> > >>>>>>> On Thursday, July 5, 2018 11:50:11 AM CEST Takashi Iwai wrote:
> > >>>>>>>> On Thu, 05 Jul 2018 11:34:59 +0200,
> > >>>>>>>> Rafael J. Wysocki wrote:
> > >>>>>>>>>
> > >>>>>>>>> Hi,
> > >>>>>>>>>
> > >>>>>>>>> On Thu, Jul 5, 2018 at 9:05 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > >>>>>>>>>> Hi,
> > >>>>>>>>>>
> > >>>>>>>>>> we've got a regression report since 4.17 about the behavior of
> > >>>>>>>>>> power-off with the power button.  When a machine is powered off with
> > >>>>>>>>>> the power button on desktop, it reboots after a few seconds instead of
> > >>>>>>>>>> power down.
> > >>>>>>>>>>
> > >>>>>>>>>> The manual power down via "systemctl poweroff" works fine, so it's
> > >>>>>>>>>> possibly some spurious wakeup by the power button action, and some
> > >>>>>>>>>> ACPI-related change is suspected.
> > >>>>>>>>>> The regression still remains in 4.18-rc3.
> > >>>>>>>>>
> > >>>>>>>>> There are only a few ACPI commits directly related to power management
> > >>>>>>>>> between 4.16 and 4.17 and none of them looks particularly suspicious.
> > >>>>>>>>
> > >>>>>>>> OK, interesting.
> > >>>>>>>>
> > >>>>>>>>> It looks like the power button state may not be cleared sufficiently
> > >>>>>>>>> after it's been pressed which is now visible for some reason.
> > >>>>>>>>
> > >>>>>>>> Hmm, where can such a state remain?  Since it happens after the
> > >>>>>>>> machine turned off, some (ACPI) wakeup bits?
> > >>>>>>>
> > >>>>>>> Basically, yes.
> > >>>>>>>
> > >>>>>>> It looks like a GPE may remain active which then triggers wakeup after
> > >>>>>>> shutdown.
> > >>>>>>>
> > >>>>>>> On a hunch, I'm wondering if reverting commit
> > >>>>>>>
> > >>>>>>> 18996f2db918 ACPICA: Events: Stop unconditionally clearing ACPI IRQs during suspend/resume
> > >>>>>>>
> > >>>>>>> (may not revert clearly, though) makes any difference.
> > >>>>>>
> > >>>>>> OK, I'm building a 4.17.x test kernel with that revert, in OBS
> > >>>>>> home:tiwai:bsc1099930 repo.
> > >>>>>>
> > >>>>>> Thomas, could you try later the kernel in
> > >>>>>>   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930/standard/
> > >>>>>> ?  It'll take an hour or so until the build finishes.
> > >>>>>
> > >>>>> With your new built kernel
> > >>>>> 4.17.4-1.g6f23755-default
> > >>>>>
> > >>>>> the power button works again, so the revert solved the problem
> > >>>>
> > >>>> Thanks, that clarifies the cause.
> > >>>> Adding Erik and Lv to Cc.
> > >>>>
> > >>>> I guess it's the side-effect by removing
> > >>>>         acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
> > >>>> in acpi_hw_disable_all_gpes().
> > >>>>
> > >>>> This function is called from acpi_power_off_prepare(), and the machine
> > >>>> goes to power off without clearing the GPEs, hence it's woken up later
> > >>>> unexpectedly.
> > >>>
> > >>> That's correct.
> > >>>
> > >>> We need to fix up that commit.  I'll try to prepare something.
> > >>>
> > >>
> > >> Below is a patch to test that theory and maybe fix things if it is correct.
> > >>
> > >> What it does is to clear all GPEs after disabling them in
> > >> acpi_power_off_prepare() which should address the issue if our theory
> > >> about the underlying reason is correct.
> > >>
> > >> Please test.
> > > 
> > > OK, building a new test kernel package in OBS home:tiwai:bsc1099930-2
> > > repo.  It'll appear at
> > >   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930-2/standard/
> > > 
> > > Thomas, please give it a try later.
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > 
> > I am sorry, but with your test kernel 4.17.4-1.g76c6238-default the
> > notebook again gets not properly powered off but restarts
> 
> Interesting.  The package version shows that the tested kernel must be
> the right one.  (Though, it'd be good to double-check -- it's often
> confusing if you have multiple same kernel versions on the system.)
> 
> If Rafael's patch doesn't work, we'd need to identify which change in
> the commit 18996f2db918 has the effect.

I'd rather prefer the following modification of the previous patch to be
tested:

---
 drivers/acpi/acpica/achware.h |    2 ++
 drivers/acpi/acpica/evxfgpe.c |   33 ++++++++++++++++++++++++++++++++-
 drivers/acpi/acpica/hwgpe.c   |   24 +++++++++++++++++++++++-
 drivers/acpi/acpica/hwsleep.c |   13 +++++++++----
 drivers/acpi/sleep.c          |    1 +
 include/acpi/acpixf.h         |    1 +
 6 files changed, 68 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/acpi/acpica/achware.h
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/achware.h
+++ linux-pm/drivers/acpi/acpica/achware.h
@@ -97,6 +97,8 @@ acpi_hw_get_gpe_status(struct acpi_gpe_e
 
 acpi_status acpi_hw_disable_all_gpes(void);
 
+acpi_status acpi_hw_clear_all_gpes(void);
+
 acpi_status acpi_hw_enable_all_runtime_gpes(void);
 
 acpi_status acpi_hw_enable_all_wakeup_gpes(void);
Index: linux-pm/drivers/acpi/acpica/hwgpe.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/hwgpe.c
+++ linux-pm/drivers/acpi/acpica/hwgpe.c
@@ -452,7 +452,7 @@ acpi_hw_enable_wakeup_gpe_block(struct a
  *
  * RETURN:      Status
  *
- * DESCRIPTION: Disable and clear all GPEs in all GPE blocks
+ * DESCRIPTION: Disable all GPEs in all GPE blocks
  *
  ******************************************************************************/
 
@@ -466,6 +466,28 @@ acpi_status acpi_hw_disable_all_gpes(voi
 	return_ACPI_STATUS(status);
 }
 
+/******************************************************************************
+ *
+ * FUNCTION:    acpi_hw_clear_all_gpes
+ *
+ * PARAMETERS:  None
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Clear all GPEs in all GPE blocks
+ *
+ ******************************************************************************/
+
+acpi_status acpi_hw_clear_all_gpes(void)
+{
+	acpi_status status;
+
+	ACPI_FUNCTION_TRACE(hw_clear_all_gpes);
+
+	status = acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
+	return_ACPI_STATUS(status);
+}
+
 /******************************************************************************
  *
  * FUNCTION:    acpi_hw_enable_all_runtime_gpes
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -1246,6 +1246,7 @@ static void acpi_power_off_prepare(void)
 	/* Prepare to power off the system */
 	acpi_sleep_prepare(ACPI_STATE_S5);
 	acpi_disable_all_gpes();
+	acpi_clear_all_gpes();
 	acpi_os_wait_events_complete();
 }
 
Index: linux-pm/drivers/acpi/acpica/evxfgpe.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/evxfgpe.c
+++ linux-pm/drivers/acpi/acpica/evxfgpe.c
@@ -710,7 +710,7 @@ ACPI_EXPORT_SYMBOL(acpi_finish_gpe)
  *
  * RETURN:      Status
  *
- * DESCRIPTION: Disable and clear all GPEs in all GPE blocks
+ * DESCRIPTION: Disable all GPEs in all GPE blocks
  *
  ******************************************************************************/
 
@@ -735,6 +735,37 @@ ACPI_EXPORT_SYMBOL(acpi_disable_all_gpes
 
 /******************************************************************************
  *
+ * FUNCTION:    acpi_clear_all_gpes
+ *
+ * PARAMETERS:  None
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Clear all GPEs in all GPE blocks
+ *
+ ******************************************************************************/
+
+acpi_status acpi_clear_all_gpes(void)
+{
+	acpi_status status;
+
+	ACPI_FUNCTION_TRACE(acpi_clear_all_gpes);
+
+	status = acpi_ut_acquire_mutex(ACPI_MTX_EVENTS);
+	if (ACPI_FAILURE(status)) {
+		return_ACPI_STATUS(status);
+	}
+
+	status = acpi_hw_clear_all_gpes();
+	(void)acpi_ut_release_mutex(ACPI_MTX_EVENTS);
+
+	return_ACPI_STATUS(status);
+}
+
+ACPI_EXPORT_SYMBOL(acpi_clear_all_gpes)
+
+/******************************************************************************
+ *
  * FUNCTION:    acpi_enable_all_runtime_gpes
  *
  * PARAMETERS:  None
Index: linux-pm/include/acpi/acpixf.h
===================================================================
--- linux-pm.orig/include/acpi/acpixf.h
+++ linux-pm/include/acpi/acpixf.h
@@ -755,6 +755,7 @@ ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_sta
 						     *event_status))
 ACPI_HW_DEPENDENT_RETURN_VOID(void acpi_dispatch_gpe(acpi_handle gpe_device, u32 gpe_number))
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_disable_all_gpes(void))
+ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_clear_all_gpes(void))
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_enable_all_runtime_gpes(void))
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_enable_all_wakeup_gpes(void))
 
Index: linux-pm/drivers/acpi/acpica/hwsleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/hwsleep.c
+++ linux-pm/drivers/acpi/acpica/hwsleep.c
@@ -51,16 +51,21 @@ acpi_status acpi_hw_legacy_sleep(u8 slee
 		return_ACPI_STATUS(status);
 	}
 
-	/*
-	 * 1) Disable all GPEs
-	 * 2) Enable all wakeup GPEs
-	 */
+	/* Disable all GPEs */
 	status = acpi_hw_disable_all_gpes();
 	if (ACPI_FAILURE(status)) {
 		return_ACPI_STATUS(status);
 	}
+	/* If the target sleep state is S5, clear all GPEs too */
+	if (sleep_state == ACPI_STATE_S5) {
+		status = acpi_hw_clear_all_gpes();
+		if (ACPI_FAILURE(status)) {
+			return_ACPI_STATUS(status);
+		}
+	}
 	acpi_gbl_system_awake_and_running = FALSE;
 
+	 /* Enable all wakeup GPEs */
 	status = acpi_hw_enable_all_wakeup_gpes();
 	if (ACPI_FAILURE(status)) {
 		return_ACPI_STATUS(status);

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Takashi Iwai]

On Fri, 06 Jul 2018 11:40:43 +0200,
Rafael J. Wysocki wrote:
> 
> On Friday, July 6, 2018 8:55:18 AM CEST Takashi Iwai wrote:
> > On Fri, 06 Jul 2018 07:18:36 +0200,
> > Thomas H4nig wrote:
> > > 
> > > 
> > > 
> > > Am 05.07.2018 um 18:56 schrieb Takashi Iwai:
> > > > On Thu, 05 Jul 2018 18:02:11 +0200,
> > > > Rafael J. Wysocki wrote:
> > > >>
> > > >> [The Lv's address is not valid any more, so drop it from the CC]
> > > >>
> > > >> On Thursday, July 5, 2018 5:10:20 PM CEST Rafael J. Wysocki wrote:
> > > >>> On Thu, Jul 5, 2018 at 5:09 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > > >>>> On Thu, 05 Jul 2018 16:00:14 +0200,
> > > >>>> Thomas H4nig wrote:
> > > >>>>>
> > > >>>>> Am 05.07.2018 um 14:12 schrieb Takashi Iwai:
> > > >>>>>> On Thu, 05 Jul 2018 12:41:03 +0200,
> > > >>>>>> Rafael J. Wysocki wrote:
> > > >>>>>>>
> > > >>>>>>> On Thursday, July 5, 2018 11:50:11 AM CEST Takashi Iwai wrote:
> > > >>>>>>>> On Thu, 05 Jul 2018 11:34:59 +0200,
> > > >>>>>>>> Rafael J. Wysocki wrote:
> > > >>>>>>>>>
> > > >>>>>>>>> Hi,
> > > >>>>>>>>>
> > > >>>>>>>>> On Thu, Jul 5, 2018 at 9:05 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > > >>>>>>>>>> Hi,
> > > >>>>>>>>>>
> > > >>>>>>>>>> we've got a regression report since 4.17 about the behavior of
> > > >>>>>>>>>> power-off with the power button.  When a machine is powered off with
> > > >>>>>>>>>> the power button on desktop, it reboots after a few seconds instead of
> > > >>>>>>>>>> power down.
> > > >>>>>>>>>>
> > > >>>>>>>>>> The manual power down via "systemctl poweroff" works fine, so it's
> > > >>>>>>>>>> possibly some spurious wakeup by the power button action, and some
> > > >>>>>>>>>> ACPI-related change is suspected.
> > > >>>>>>>>>> The regression still remains in 4.18-rc3.
> > > >>>>>>>>>
> > > >>>>>>>>> There are only a few ACPI commits directly related to power management
> > > >>>>>>>>> between 4.16 and 4.17 and none of them looks particularly suspicious.
> > > >>>>>>>>
> > > >>>>>>>> OK, interesting.
> > > >>>>>>>>
> > > >>>>>>>>> It looks like the power button state may not be cleared sufficiently
> > > >>>>>>>>> after it's been pressed which is now visible for some reason.
> > > >>>>>>>>
> > > >>>>>>>> Hmm, where can such a state remain?  Since it happens after the
> > > >>>>>>>> machine turned off, some (ACPI) wakeup bits?
> > > >>>>>>>
> > > >>>>>>> Basically, yes.
> > > >>>>>>>
> > > >>>>>>> It looks like a GPE may remain active which then triggers wakeup after
> > > >>>>>>> shutdown.
> > > >>>>>>>
> > > >>>>>>> On a hunch, I'm wondering if reverting commit
> > > >>>>>>>
> > > >>>>>>> 18996f2db918 ACPICA: Events: Stop unconditionally clearing ACPI IRQs during suspend/resume
> > > >>>>>>>
> > > >>>>>>> (may not revert clearly, though) makes any difference.
> > > >>>>>>
> > > >>>>>> OK, I'm building a 4.17.x test kernel with that revert, in OBS
> > > >>>>>> home:tiwai:bsc1099930 repo.
> > > >>>>>>
> > > >>>>>> Thomas, could you try later the kernel in
> > > >>>>>>   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930/standard/
> > > >>>>>> ?  It'll take an hour or so until the build finishes.
> > > >>>>>
> > > >>>>> With your new built kernel
> > > >>>>> 4.17.4-1.g6f23755-default
> > > >>>>>
> > > >>>>> the power button works again, so the revert solved the problem
> > > >>>>
> > > >>>> Thanks, that clarifies the cause.
> > > >>>> Adding Erik and Lv to Cc.
> > > >>>>
> > > >>>> I guess it's the side-effect by removing
> > > >>>>         acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
> > > >>>> in acpi_hw_disable_all_gpes().
> > > >>>>
> > > >>>> This function is called from acpi_power_off_prepare(), and the machine
> > > >>>> goes to power off without clearing the GPEs, hence it's woken up later
> > > >>>> unexpectedly.
> > > >>>
> > > >>> That's correct.
> > > >>>
> > > >>> We need to fix up that commit.  I'll try to prepare something.
> > > >>>
> > > >>
> > > >> Below is a patch to test that theory and maybe fix things if it is correct.
> > > >>
> > > >> What it does is to clear all GPEs after disabling them in
> > > >> acpi_power_off_prepare() which should address the issue if our theory
> > > >> about the underlying reason is correct.
> > > >>
> > > >> Please test.
> > > > 
> > > > OK, building a new test kernel package in OBS home:tiwai:bsc1099930-2
> > > > repo.  It'll appear at
> > > >   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930-2/standard/
> > > > 
> > > > Thomas, please give it a try later.
> > > > 
> > > > 
> > > > thanks,
> > > > 
> > > > Takashi
> > > 
> > > I am sorry, but with your test kernel 4.17.4-1.g76c6238-default the
> > > notebook again gets not properly powered off but restarts
> > 
> > Interesting.  The package version shows that the tested kernel must be
> > the right one.  (Though, it'd be good to double-check -- it's often
> > confusing if you have multiple same kernel versions on the system.)
> > 
> > If Rafael's patch doesn't work, we'd need to identify which change in
> > the commit 18996f2db918 has the effect.
> 
> I'd rather prefer the following modification of the previous patch to be
> tested:

OK, a new test kernel is being built on OBS home:tiwai:bsc1099930-3
repo.  The release number will be *.g3ecdf0b.
Please give it a try later.


thanks,

Takashi

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Thomas Hänig]

Am 06.07.2018 um 08:55 schrieb Takashi Iwai:
> On Fri, 06 Jul 2018 07:18:36 +0200,
> Thomas H4nig wrote:
>>
>>
>>
>> Am 05.07.2018 um 18:56 schrieb Takashi Iwai:
>>> On Thu, 05 Jul 2018 18:02:11 +0200,
>>> Rafael J. Wysocki wrote:
>>>>
>>>> [The Lv's address is not valid any more, so drop it from the CC]
>>>>
>>>> On Thursday, July 5, 2018 5:10:20 PM CEST Rafael J. Wysocki wrote:
>>>>> On Thu, Jul 5, 2018 at 5:09 PM, Takashi Iwai <tiwai@suse.de> wrote:
>>>>>> On Thu, 05 Jul 2018 16:00:14 +0200,
>>>>>> Thomas H4nig wrote:
>>>>>>>
>>>>>>> Am 05.07.2018 um 14:12 schrieb Takashi Iwai:
>>>>>>>> On Thu, 05 Jul 2018 12:41:03 +0200,
>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>
>>>>>>>>> On Thursday, July 5, 2018 11:50:11 AM CEST Takashi Iwai wrote:
>>>>>>>>>> On Thu, 05 Jul 2018 11:34:59 +0200,
>>>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Jul 5, 2018 at 9:05 AM, Takashi Iwai <tiwai@suse.de> wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> we've got a regression report since 4.17 about the behavior of
>>>>>>>>>>>> power-off with the power button.  When a machine is powered off with
>>>>>>>>>>>> the power button on desktop, it reboots after a few seconds instead of
>>>>>>>>>>>> power down.
>>>>>>>>>>>>
>>>>>>>>>>>> The manual power down via "systemctl poweroff" works fine, so it's
>>>>>>>>>>>> possibly some spurious wakeup by the power button action, and some
>>>>>>>>>>>> ACPI-related change is suspected.
>>>>>>>>>>>> The regression still remains in 4.18-rc3.
>>>>>>>>>>>
>>>>>>>>>>> There are only a few ACPI commits directly related to power management
>>>>>>>>>>> between 4.16 and 4.17 and none of them looks particularly suspicious.
>>>>>>>>>>
>>>>>>>>>> OK, interesting.
>>>>>>>>>>
>>>>>>>>>>> It looks like the power button state may not be cleared sufficiently
>>>>>>>>>>> after it's been pressed which is now visible for some reason.
>>>>>>>>>>
>>>>>>>>>> Hmm, where can such a state remain?  Since it happens after the
>>>>>>>>>> machine turned off, some (ACPI) wakeup bits?
>>>>>>>>>
>>>>>>>>> Basically, yes.
>>>>>>>>>
>>>>>>>>> It looks like a GPE may remain active which then triggers wakeup after
>>>>>>>>> shutdown.
>>>>>>>>>
>>>>>>>>> On a hunch, I'm wondering if reverting commit
>>>>>>>>>
>>>>>>>>> 18996f2db918 ACPICA: Events: Stop unconditionally clearing ACPI IRQs during suspend/resume
>>>>>>>>>
>>>>>>>>> (may not revert clearly, though) makes any difference.
>>>>>>>>
>>>>>>>> OK, I'm building a 4.17.x test kernel with that revert, in OBS
>>>>>>>> home:tiwai:bsc1099930 repo.
>>>>>>>>
>>>>>>>> Thomas, could you try later the kernel in
>>>>>>>>   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930/standard/
>>>>>>>> ?  It'll take an hour or so until the build finishes.
>>>>>>>
>>>>>>> With your new built kernel
>>>>>>> 4.17.4-1.g6f23755-default
>>>>>>>
>>>>>>> the power button works again, so the revert solved the problem
>>>>>>
>>>>>> Thanks, that clarifies the cause.
>>>>>> Adding Erik and Lv to Cc.
>>>>>>
>>>>>> I guess it's the side-effect by removing
>>>>>>         acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
>>>>>> in acpi_hw_disable_all_gpes().
>>>>>>
>>>>>> This function is called from acpi_power_off_prepare(), and the machine
>>>>>> goes to power off without clearing the GPEs, hence it's woken up later
>>>>>> unexpectedly.
>>>>>
>>>>> That's correct.
>>>>>
>>>>> We need to fix up that commit.  I'll try to prepare something.
>>>>>
>>>>
>>>> Below is a patch to test that theory and maybe fix things if it is correct.
>>>>
>>>> What it does is to clear all GPEs after disabling them in
>>>> acpi_power_off_prepare() which should address the issue if our theory
>>>> about the underlying reason is correct.
>>>>
>>>> Please test.
>>>
>>> OK, building a new test kernel package in OBS home:tiwai:bsc1099930-2
>>> repo.  It'll appear at
>>>   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930-2/standard/
>>>
>>> Thomas, please give it a try later.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>
>> I am sorry, but with your test kernel 4.17.4-1.g76c6238-default the
>> notebook again gets not properly powered off but restarts
> 
> Interesting.  The package version shows that the tested kernel must be
> the right one.  (Though, it'd be good to double-check -- it's often
> confusing if you have multiple same kernel versions on the system.)
> 
> If Rafael's patch doesn't work, we'd need to identify which change in
> the commit 18996f2db918 has the effect.
> 
> Thomas, could you try to build & modify the kernel in your side?
> Package build on OBS and test takes too long.
> 
> There are three places that remove the GPE-clearing in the patch.
> 
> One is in acpi_ev_enable_gpe():
> 
> --- a/drivers/acpi/acpica/evgpe.c
> +++ b/drivers/acpi/acpica/evgpe.c
> @@ -115,13 +115,6 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
>  
>  	ACPI_FUNCTION_TRACE(ev_enable_gpe);
>  
> -	/* Clear the GPE (of stale events) */
> -
> -	status = acpi_hw_clear_gpe(gpe_event_info);
> -	if (ACPI_FAILURE(status)) {
> -		return_ACPI_STATUS(status);
> -	}
> -
>  	/* Enable the requested GPE */
>  
>  	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
> 
> 
> ... the second one is in acpi_hw_disable_all_gpes():
> 
> --- a/drivers/acpi/acpica/hwgpe.c
> +++ b/drivers/acpi/acpica/hwgpe.c
> @@ -497,7 +497,6 @@ acpi_status acpi_hw_disable_all_gpes(void)
>  	ACPI_FUNCTION_TRACE(hw_disable_all_gpes);
>  
>  	status = acpi_ev_walk_gpe_list(acpi_hw_disable_gpe_block, NULL);
> -	status = acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
>  	return_ACPI_STATUS(status);
>  }
>  
> 
> ... and the last one is in acpi_hw_legacy_sleep():
> 
> --- a/drivers/acpi/acpica/hwsleep.c
> +++ b/drivers/acpi/acpica/hwsleep.c
> @@ -85,15 +85,8 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
>  		return_ACPI_STATUS(status);
>  	}
>  
> -	/* Clear all fixed and general purpose status bits */
> -
> -	status = acpi_hw_clear_acpi_status();
> -	if (ACPI_FAILURE(status)) {
> -		return_ACPI_STATUS(status);
> -	}
> -
>  	/*
> 	 * 1) Disable/Clear all GPEs
> 
> 
> The rest are only comments changes.
> 
> Try to revert each hunk manually one-by-one and figure out which one
> actually causes the regression.  Maybe it'd be safer to test on the
> 4.17.x kernel, but you can check on 4.18-rc, too.
> 
> 
> thanks,
> 
> Takashi
> 

I am finally through and have results with patch(es) reverted for kernel
4.17.3-1-default:

1 - reboot
2 - reboot
3 - poweroff

being
#1 - evgpe.c
#2 - hwgpe.c
#3 - hwsleep.c

whereas the revert failed at #3 and I tried it manually, resulting in

whereas the revert failed at #3 and I tried it manually, resulting in

--- drivers/acpi/acpica/hwsleep.c       2018-07-06 12:56:07.881379862 +0200
+++ drivers/acpi/acpica/hwsleep.c.orig  2018-06-26 08:45:20.000000000 +0200
@@ -51,13 +51,6 @@ acpi_status acpi_hw_legacy_sleep(u8 slee
                return_ACPI_STATUS(status);
        }

-       /* Clear all fixed and general purpose status bits */
-
-       status = acpi_hw_clear_acpi_status();
-       if (ACPI_FAILURE(status)) {
-               return_ACPI_STATUS(status);
-       }
-
        /*
         * 1) Disable all GPEs
         * 2) Enable all wakeup GPEs

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Rafael J. Wysocki]

On Fri, Jul 6, 2018 at 1:12 PM, Thomas Hänig <haenig@cosifan.de> wrote:
> Am 06.07.2018 um 08:55 schrieb Takashi Iwai:
>> On Fri, 06 Jul 2018 07:18:36 +0200,
>> Thomas H4nig wrote:
>>>
>>>
>>>
>>> Am 05.07.2018 um 18:56 schrieb Takashi Iwai:
>>>> On Thu, 05 Jul 2018 18:02:11 +0200,
>>>> Rafael J. Wysocki wrote:
>>>>>
>>>>> [The Lv's address is not valid any more, so drop it from the CC]
>>>>>
>>>>> On Thursday, July 5, 2018 5:10:20 PM CEST Rafael J. Wysocki wrote:
>>>>>> On Thu, Jul 5, 2018 at 5:09 PM, Takashi Iwai <tiwai@suse.de> wrote:
>>>>>>> On Thu, 05 Jul 2018 16:00:14 +0200,
>>>>>>> Thomas H4nig wrote:
>>>>>>>>
>>>>>>>> Am 05.07.2018 um 14:12 schrieb Takashi Iwai:
>>>>>>>>> On Thu, 05 Jul 2018 12:41:03 +0200,
>>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>>
>>>>>>>>>> On Thursday, July 5, 2018 11:50:11 AM CEST Takashi Iwai wrote:
>>>>>>>>>>> On Thu, 05 Jul 2018 11:34:59 +0200,
>>>>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, Jul 5, 2018 at 9:05 AM, Takashi Iwai <tiwai@suse.de> wrote:
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> we've got a regression report since 4.17 about the behavior of
>>>>>>>>>>>>> power-off with the power button.  When a machine is powered off with
>>>>>>>>>>>>> the power button on desktop, it reboots after a few seconds instead of
>>>>>>>>>>>>> power down.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The manual power down via "systemctl poweroff" works fine, so it's
>>>>>>>>>>>>> possibly some spurious wakeup by the power button action, and some
>>>>>>>>>>>>> ACPI-related change is suspected.
>>>>>>>>>>>>> The regression still remains in 4.18-rc3.
>>>>>>>>>>>>
>>>>>>>>>>>> There are only a few ACPI commits directly related to power management
>>>>>>>>>>>> between 4.16 and 4.17 and none of them looks particularly suspicious.
>>>>>>>>>>>
>>>>>>>>>>> OK, interesting.
>>>>>>>>>>>
>>>>>>>>>>>> It looks like the power button state may not be cleared sufficiently
>>>>>>>>>>>> after it's been pressed which is now visible for some reason.
>>>>>>>>>>>
>>>>>>>>>>> Hmm, where can such a state remain?  Since it happens after the
>>>>>>>>>>> machine turned off, some (ACPI) wakeup bits?
>>>>>>>>>>
>>>>>>>>>> Basically, yes.
>>>>>>>>>>
>>>>>>>>>> It looks like a GPE may remain active which then triggers wakeup after
>>>>>>>>>> shutdown.
>>>>>>>>>>
>>>>>>>>>> On a hunch, I'm wondering if reverting commit
>>>>>>>>>>
>>>>>>>>>> 18996f2db918 ACPICA: Events: Stop unconditionally clearing ACPI IRQs during suspend/resume
>>>>>>>>>>
>>>>>>>>>> (may not revert clearly, though) makes any difference.
>>>>>>>>>
>>>>>>>>> OK, I'm building a 4.17.x test kernel with that revert, in OBS
>>>>>>>>> home:tiwai:bsc1099930 repo.
>>>>>>>>>
>>>>>>>>> Thomas, could you try later the kernel in
>>>>>>>>>   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930/standard/
>>>>>>>>> ?  It'll take an hour or so until the build finishes.
>>>>>>>>
>>>>>>>> With your new built kernel
>>>>>>>> 4.17.4-1.g6f23755-default
>>>>>>>>
>>>>>>>> the power button works again, so the revert solved the problem
>>>>>>>
>>>>>>> Thanks, that clarifies the cause.
>>>>>>> Adding Erik and Lv to Cc.
>>>>>>>
>>>>>>> I guess it's the side-effect by removing
>>>>>>>         acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
>>>>>>> in acpi_hw_disable_all_gpes().
>>>>>>>
>>>>>>> This function is called from acpi_power_off_prepare(), and the machine
>>>>>>> goes to power off without clearing the GPEs, hence it's woken up later
>>>>>>> unexpectedly.
>>>>>>
>>>>>> That's correct.
>>>>>>
>>>>>> We need to fix up that commit.  I'll try to prepare something.
>>>>>>
>>>>>
>>>>> Below is a patch to test that theory and maybe fix things if it is correct.
>>>>>
>>>>> What it does is to clear all GPEs after disabling them in
>>>>> acpi_power_off_prepare() which should address the issue if our theory
>>>>> about the underlying reason is correct.
>>>>>
>>>>> Please test.
>>>>
>>>> OK, building a new test kernel package in OBS home:tiwai:bsc1099930-2
>>>> repo.  It'll appear at
>>>>   http://download.opensuse.org/repositories/home:/tiwai:/bsc1099930-2/standard/
>>>>
>>>> Thomas, please give it a try later.
>>>>
>>>>
>>>> thanks,
>>>>
>>>> Takashi
>>>
>>> I am sorry, but with your test kernel 4.17.4-1.g76c6238-default the
>>> notebook again gets not properly powered off but restarts
>>
>> Interesting.  The package version shows that the tested kernel must be
>> the right one.  (Though, it'd be good to double-check -- it's often
>> confusing if you have multiple same kernel versions on the system.)
>>
>> If Rafael's patch doesn't work, we'd need to identify which change in
>> the commit 18996f2db918 has the effect.
>>
>> Thomas, could you try to build & modify the kernel in your side?
>> Package build on OBS and test takes too long.
>>
>> There are three places that remove the GPE-clearing in the patch.
>>
>> One is in acpi_ev_enable_gpe():
>>
>> --- a/drivers/acpi/acpica/evgpe.c
>> +++ b/drivers/acpi/acpica/evgpe.c
>> @@ -115,13 +115,6 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
>>
>>       ACPI_FUNCTION_TRACE(ev_enable_gpe);
>>
>> -     /* Clear the GPE (of stale events) */
>> -
>> -     status = acpi_hw_clear_gpe(gpe_event_info);
>> -     if (ACPI_FAILURE(status)) {
>> -             return_ACPI_STATUS(status);
>> -     }
>> -
>>       /* Enable the requested GPE */
>>
>>       status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
>>
>>
>> ... the second one is in acpi_hw_disable_all_gpes():
>>
>> --- a/drivers/acpi/acpica/hwgpe.c
>> +++ b/drivers/acpi/acpica/hwgpe.c
>> @@ -497,7 +497,6 @@ acpi_status acpi_hw_disable_all_gpes(void)
>>       ACPI_FUNCTION_TRACE(hw_disable_all_gpes);
>>
>>       status = acpi_ev_walk_gpe_list(acpi_hw_disable_gpe_block, NULL);
>> -     status = acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block, NULL);
>>       return_ACPI_STATUS(status);
>>  }
>>
>>
>> ... and the last one is in acpi_hw_legacy_sleep():
>>
>> --- a/drivers/acpi/acpica/hwsleep.c
>> +++ b/drivers/acpi/acpica/hwsleep.c
>> @@ -85,15 +85,8 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
>>               return_ACPI_STATUS(status);
>>       }
>>
>> -     /* Clear all fixed and general purpose status bits */
>> -
>> -     status = acpi_hw_clear_acpi_status();
>> -     if (ACPI_FAILURE(status)) {
>> -             return_ACPI_STATUS(status);
>> -     }
>> -
>>       /*
>>        * 1) Disable/Clear all GPEs
>>
>>
>> The rest are only comments changes.
>>
>> Try to revert each hunk manually one-by-one and figure out which one
>> actually causes the regression.  Maybe it'd be safer to test on the
>> 4.17.x kernel, but you can check on 4.18-rc, too.
>>
>
> I am finally through and have results with patch(es) reverted for kernel
> 4.17.3-1-default:
>
> 1 - reboot
> 2 - reboot
> 3 - poweroff
>
> being
> #1 - evgpe.c
> #2 - hwgpe.c
> #3 - hwsleep.c
>
> whereas the revert failed at #3 and I tried it manually, resulting in
>
> whereas the revert failed at #3 and I tried it manually, resulting in
>
> --- drivers/acpi/acpica/hwsleep.c       2018-07-06 12:56:07.881379862 +0200
> +++ drivers/acpi/acpica/hwsleep.c.orig  2018-06-26 08:45:20.000000000 +0200
> @@ -51,13 +51,6 @@ acpi_status acpi_hw_legacy_sleep(u8 slee
>                 return_ACPI_STATUS(status);
>         }
>
> -       /* Clear all fixed and general purpose status bits */
> -
> -       status = acpi_hw_clear_acpi_status();
> -       if (ACPI_FAILURE(status)) {
> -               return_ACPI_STATUS(status);
> -       }
> -
>         /*
>          * 1) Disable all GPEs
>          * 2) Enable all wakeup GPEs
>
> --

OK, thanks!

So the latest patch:

https://patchwork.kernel.org/patch/10511211/

should work for you (please verify) and the change in
drivers/acpi/sleep.c in it most likely is not necessary.

If you can confirm that this one works for you, I'll send a smaller
one with the acpi_hw_legacy_sleep() part alone.

Thanks,
Rafael

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Rafael J. Wysocki]

On Friday, July 6, 2018 1:21:50 PM CEST Rafael J. Wysocki wrote:
> On Fri, Jul 6, 2018 at 1:12 PM, Thomas Hänig <haenig@cosifan.de> wrote:

[cut]
 
> So the latest patch:
> 
> https://patchwork.kernel.org/patch/10511211/
> 
> should work for you (please verify) and the change in
> drivers/acpi/sleep.c in it most likely is not necessary.
> 
> If you can confirm that this one works for you, I'll send a smaller
> one with the acpi_hw_legacy_sleep() part alone.

Well, scratch this, sorry.

The power button probably is a fixed event and it won't be effected by
that patch.

Instead, please test the patch below.

---
 drivers/acpi/acpica/hwsleep.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/acpi/acpica/hwsleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/hwsleep.c
+++ linux-pm/drivers/acpi/acpica/hwsleep.c
@@ -51,16 +51,23 @@ acpi_status acpi_hw_legacy_sleep(u8 slee
 		return_ACPI_STATUS(status);
 	}
 
-	/*
-	 * 1) Disable all GPEs
-	 * 2) Enable all wakeup GPEs
-	 */
+	/* Disable all GPEs */
 	status = acpi_hw_disable_all_gpes();
 	if (ACPI_FAILURE(status)) {
 		return_ACPI_STATUS(status);
 	}
+	/*
+	 * If the target sleep state is S5, clear all GPEs and fixed events too
+	 */
+	if (sleep_state == ACPI_STATE_S5) {
+		status = acpi_hw_clear_acpi_status();
+		if (ACPI_FAILURE(status)) {
+			return_ACPI_STATUS(status);
+		}
+	}
 	acpi_gbl_system_awake_and_running = FALSE;
 
+	 /* Enable all wakeup GPEs */
 	status = acpi_hw_enable_all_wakeup_gpes();
 	if (ACPI_FAILURE(status)) {
 		return_ACPI_STATUS(status);

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Takashi Iwai]

On Fri, 06 Jul 2018 14:13:04 +0200,
Rafael J. Wysocki wrote:
> 
> On Friday, July 6, 2018 1:21:50 PM CEST Rafael J. Wysocki wrote:
> > On Fri, Jul 6, 2018 at 1:12 PM, Thomas Hänig <haenig@cosifan.de> wrote:
> 
> [cut]
>  
> > So the latest patch:
> > 
> > https://patchwork.kernel.org/patch/10511211/
> > 
> > should work for you (please verify) and the change in
> > drivers/acpi/sleep.c in it most likely is not necessary.
> > 
> > If you can confirm that this one works for you, I'll send a smaller
> > one with the acpi_hw_legacy_sleep() part alone.
> 
> Well, scratch this, sorry.
> 
> The power button probably is a fixed event and it won't be effected by
> that patch.
> 
> Instead, please test the patch below.

FWIW, the test kernel on OBS home:tiwai:bsc1099930-3 was refreshed
with this one.  The release number will be *.g2351e2d.


Takashi

> 
> ---
>  drivers/acpi/acpica/hwsleep.c |   15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/drivers/acpi/acpica/hwsleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/hwsleep.c
> +++ linux-pm/drivers/acpi/acpica/hwsleep.c
> @@ -51,16 +51,23 @@ acpi_status acpi_hw_legacy_sleep(u8 slee
>  		return_ACPI_STATUS(status);
>  	}
>  
> -	/*
> -	 * 1) Disable all GPEs
> -	 * 2) Enable all wakeup GPEs
> -	 */
> +	/* Disable all GPEs */
>  	status = acpi_hw_disable_all_gpes();
>  	if (ACPI_FAILURE(status)) {
>  		return_ACPI_STATUS(status);
>  	}
> +	/*
> +	 * If the target sleep state is S5, clear all GPEs and fixed events too
> +	 */
> +	if (sleep_state == ACPI_STATE_S5) {
> +		status = acpi_hw_clear_acpi_status();
> +		if (ACPI_FAILURE(status)) {
> +			return_ACPI_STATUS(status);
> +		}
> +	}
>  	acpi_gbl_system_awake_and_running = FALSE;
>  
> +	 /* Enable all wakeup GPEs */
>  	status = acpi_hw_enable_all_wakeup_gpes();
>  	if (ACPI_FAILURE(status)) {
>  		return_ACPI_STATUS(status);
> 

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Thomas Hänig]

Am 06.07.2018 um 14:27 schrieb Takashi Iwai:
> On Fri, 06 Jul 2018 14:13:04 +0200,
> Rafael J. Wysocki wrote:
>>
>> On Friday, July 6, 2018 1:21:50 PM CEST Rafael J. Wysocki wrote:
>>> On Fri, Jul 6, 2018 at 1:12 PM, Thomas Hänig <haenig@cosifan.de> wrote:
>>
>> [cut]
>>  
>>> So the latest patch:
>>>
>>> https://patchwork.kernel.org/patch/10511211/
>>>
>>> should work for you (please verify) and the change in
>>> drivers/acpi/sleep.c in it most likely is not necessary.
>>>
>>> If you can confirm that this one works for you, I'll send a smaller
>>> one with the acpi_hw_legacy_sleep() part alone.
>>
>> Well, scratch this, sorry.
>>
>> The power button probably is a fixed event and it won't be effected by
>> that patch.
>>
>> Instead, please test the patch below.
> 
> FWIW, the test kernel on OBS home:tiwai:bsc1099930-3 was refreshed
> with this one.  The release number will be *.g2351e2d.
> 
> 
> Takashi

to make shure I have made no mistake while building my own, I will try
out your kernel as well.

Thomas

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Thomas Hänig]

Am 06.07.2018 um 14:27 schrieb Takashi Iwai:
> On Fri, 06 Jul 2018 14:13:04 +0200,
> Rafael J. Wysocki wrote:
>>
>> On Friday, July 6, 2018 1:21:50 PM CEST Rafael J. Wysocki wrote:
>>> On Fri, Jul 6, 2018 at 1:12 PM, Thomas Hänig <haenig@cosifan.de> wrote:
>>
>> [cut]
>>  
>>> So the latest patch:
>>>
>>> https://patchwork.kernel.org/patch/10511211/
>>>
>>> should work for you (please verify) and the change in
>>> drivers/acpi/sleep.c in it most likely is not necessary.
>>>
>>> If you can confirm that this one works for you, I'll send a smaller
>>> one with the acpi_hw_legacy_sleep() part alone.
>>
>> Well, scratch this, sorry.
>>
>> The power button probably is a fixed event and it won't be effected by
>> that patch.
>>
>> Instead, please test the patch below.
> 
> FWIW, the test kernel on OBS home:tiwai:bsc1099930-3 was refreshed
> with this one.  The release number will be *.g2351e2d.
> 
> 
> Takashi
> 
>>
>> ---
>>  drivers/acpi/acpica/hwsleep.c |   15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> Index: linux-pm/drivers/acpi/acpica/hwsleep.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/acpica/hwsleep.c
>> +++ linux-pm/drivers/acpi/acpica/hwsleep.c
>> @@ -51,16 +51,23 @@ acpi_status acpi_hw_legacy_sleep(u8 slee
>>  		return_ACPI_STATUS(status);
>>  	}
>>  
>> -	/*
>> -	 * 1) Disable all GPEs
>> -	 * 2) Enable all wakeup GPEs
>> -	 */
>> +	/* Disable all GPEs */
>>  	status = acpi_hw_disable_all_gpes();
>>  	if (ACPI_FAILURE(status)) {
>>  		return_ACPI_STATUS(status);
>>  	}
>> +	/*
>> +	 * If the target sleep state is S5, clear all GPEs and fixed events too
>> +	 */
>> +	if (sleep_state == ACPI_STATE_S5) {
>> +		status = acpi_hw_clear_acpi_status();
>> +		if (ACPI_FAILURE(status)) {
>> +			return_ACPI_STATUS(status);
>> +		}
>> +	}
>>  	acpi_gbl_system_awake_and_running = FALSE;
>>  
>> +	 /* Enable all wakeup GPEs */
>>  	status = acpi_hw_enable_all_wakeup_gpes();
>>  	if (ACPI_FAILURE(status)) {
>>  		return_ACPI_STATUS(status);
>>
Hello Takashi,
with the kernel built by you the system shuts down when pressing the
power button and stays off too!  :-)

So the above mentioned patch to hwsleep.c seems to do the trick.

Regards and thanks a lot @all!
Thomas

thomas@tslb:~> uname -a
Linux tslb 4.17.4-2.g2351e2d-default #1 SMP PREEMPT Fri Jul 6 12:24:35
UTC 2018 (2351e2d) x86_64 x86_64 x86_64 GNU/Linux


thomas@tslb:~> rpm -qi kernel-default-4.17.4-2.1.g2351e2d.x86_64
Name        : kernel-default
Version     : 4.17.4
Release     : 2.1.g2351e2d
Architecture: x86_64
Install Date: Fr 06 Jul 2018 19:43:38 CEST
Group       : System/Kernel
Size        : 360100489
License     : GPL-2.0
Signature   : RSA/SHA256, Fr 06 Jul 2018 16:47:25 CEST, Key ID
4bf05f46f6e74bf5
Source RPM  : kernel-default-4.17.4-2.1.g2351e2d.nosrc.rpm
Build Date  : Fr 06 Jul 2018 16:42:04 CEST
Build Host  : lamb63
Relocations : (not relocatable)
Vendor      : obs://build.opensuse.org/home:tiwai
URL         : http://www.kernel.org/
Summary     : The Standard Kernel
Description :
The standard kernel for both uniprocessor and multiprocessor systems.

Source Timestamp: 2018-07-06 14:24:35 +0200
GIT Revision: 2351e2d70cbcb963c461d55abeee967ea9a940ea
GIT Branch: users/tiwai/stable/bsc1099930
Distribution: home:tiwai:bsc1099930-3

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Rafael J. Wysocki]

On Fri, Jul 6, 2018 at 7:57 PM, Thomas Hänig <haenig@cosifan.de> wrote:
>
>
> Am 06.07.2018 um 14:27 schrieb Takashi Iwai:
>> On Fri, 06 Jul 2018 14:13:04 +0200,
>> Rafael J. Wysocki wrote:
>>>
>>> On Friday, July 6, 2018 1:21:50 PM CEST Rafael J. Wysocki wrote:
>>>> On Fri, Jul 6, 2018 at 1:12 PM, Thomas Hänig <haenig@cosifan.de> wrote:
>>>
>>> [cut]
>>>
>>>> So the latest patch:
>>>>
>>>> https://patchwork.kernel.org/patch/10511211/
>>>>
>>>> should work for you (please verify) and the change in
>>>> drivers/acpi/sleep.c in it most likely is not necessary.
>>>>
>>>> If you can confirm that this one works for you, I'll send a smaller
>>>> one with the acpi_hw_legacy_sleep() part alone.
>>>
>>> Well, scratch this, sorry.
>>>
>>> The power button probably is a fixed event and it won't be effected by
>>> that patch.
>>>
>>> Instead, please test the patch below.
>>
>> FWIW, the test kernel on OBS home:tiwai:bsc1099930-3 was refreshed
>> with this one.  The release number will be *.g2351e2d.
>>
>>
>> Takashi
>>
>>>
>>> ---
>>>  drivers/acpi/acpica/hwsleep.c |   15 +++++++++++----
>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> Index: linux-pm/drivers/acpi/acpica/hwsleep.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/acpi/acpica/hwsleep.c
>>> +++ linux-pm/drivers/acpi/acpica/hwsleep.c
>>> @@ -51,16 +51,23 @@ acpi_status acpi_hw_legacy_sleep(u8 slee
>>>              return_ACPI_STATUS(status);
>>>      }
>>>
>>> -    /*
>>> -     * 1) Disable all GPEs
>>> -     * 2) Enable all wakeup GPEs
>>> -     */
>>> +    /* Disable all GPEs */
>>>      status = acpi_hw_disable_all_gpes();
>>>      if (ACPI_FAILURE(status)) {
>>>              return_ACPI_STATUS(status);
>>>      }
>>> +    /*
>>> +     * If the target sleep state is S5, clear all GPEs and fixed events too
>>> +     */
>>> +    if (sleep_state == ACPI_STATE_S5) {
>>> +            status = acpi_hw_clear_acpi_status();
>>> +            if (ACPI_FAILURE(status)) {
>>> +                    return_ACPI_STATUS(status);
>>> +            }
>>> +    }
>>>      acpi_gbl_system_awake_and_running = FALSE;
>>>
>>> +     /* Enable all wakeup GPEs */
>>>      status = acpi_hw_enable_all_wakeup_gpes();
>>>      if (ACPI_FAILURE(status)) {
>>>              return_ACPI_STATUS(status);
>>>
> Hello Takashi,
> with the kernel built by you the system shuts down when pressing the
> power button and stays off too!  :-)
>
> So the above mentioned patch to hwsleep.c seems to do the trick.

OK, thanks for verifying!

> Regards and thanks a lot @all!

Cheers,
Rafael

Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button

[Thomas Hänig]

Am 06.07.2018 um 14:13 schrieb Rafael J. Wysocki:
> On Friday, July 6, 2018 1:21:50 PM CEST Rafael J. Wysocki wrote:
>> On Fri, Jul 6, 2018 at 1:12 PM, Thomas Hänig <haenig@cosifan.de> wrote:
> 
> [cut]
>  
>> So the latest patch:
>>
>> https://patchwork.kernel.org/patch/10511211/
>>
>> should work for you (please verify) and the change in
>> drivers/acpi/sleep.c in it most likely is not necessary.
>>
>> If you can confirm that this one works for you, I'll send a smaller
>> one with the acpi_hw_legacy_sleep() part alone.
> 
> Well, scratch this, sorry.
> 
> The power button probably is a fixed event and it won't be effected by
> that patch.
> 
> Instead, please test the patch below.
> 
> ---
>  drivers/acpi/acpica/hwsleep.c |   15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/drivers/acpi/acpica/hwsleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/hwsleep.c
> +++ linux-pm/drivers/acpi/acpica/hwsleep.c
> @@ -51,16 +51,23 @@ acpi_status acpi_hw_legacy_sleep(u8 slee
>  		return_ACPI_STATUS(status);
>  	}
>  
> -	/*
> -	 * 1) Disable all GPEs
> -	 * 2) Enable all wakeup GPEs
> -	 */
> +	/* Disable all GPEs */
>  	status = acpi_hw_disable_all_gpes();
>  	if (ACPI_FAILURE(status)) {
>  		return_ACPI_STATUS(status);
>  	}
> +	/*
> +	 * If the target sleep state is S5, clear all GPEs and fixed events too
> +	 */
> +	if (sleep_state == ACPI_STATE_S5) {
> +		status = acpi_hw_clear_acpi_status();
> +		if (ACPI_FAILURE(status)) {
> +			return_ACPI_STATUS(status);
> +		}
> +	}
>  	acpi_gbl_system_awake_and_running = FALSE;
>  
> +	 /* Enable all wakeup GPEs */
>  	status = acpi_hw_enable_all_wakeup_gpes();
>  	if (ACPI_FAILURE(status)) {
>  		return_ACPI_STATUS(status);
> 
after cancelling the build with patch 10511211 and rolling everything
back, I applied the patch above to my kernel 4.17.3-1-default and when
pressing the power button the system shuts down and stays off! :-)

Thanks
Thomas

[PATCH] ACPICA: Clear status of all events when entering S5

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After commit 18996f2db918 (ACPICA: Events: Stop unconditionally
clearing ACPI IRQs during suspend/resume) the status of ACPI events
is not cleared any more when entering the ACPI S5 system state (power
off) which causes some systems to power up immediately after turing
off power in certain situations.

That is a functional regression, so address it by making the code
clear the status of all ACPI events again when entering S5 (for
system-wide suspend or hibernation the clearing of the status of all
events is not desirable, as it might cause the kernel to miss wakeup
events sometimes).

Fixes: 18996f2db918 (ACPICA: Events: Stop unconditionally clearing ACPI IRQs during suspend/resume)
Reported-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Thomas Hänig <haenig@cosifan.de>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Resend https://patchwork.kernel.org/patch/10511451/ with a changelig and tags.

---
 drivers/acpi/acpica/hwsleep.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/acpi/acpica/hwsleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/hwsleep.c
+++ linux-pm/drivers/acpi/acpica/hwsleep.c
@@ -51,16 +51,23 @@ acpi_status acpi_hw_legacy_sleep(u8 slee
 		return_ACPI_STATUS(status);
 	}
 
-	/*
-	 * 1) Disable all GPEs
-	 * 2) Enable all wakeup GPEs
-	 */
+	/* Disable all GPEs */
 	status = acpi_hw_disable_all_gpes();
 	if (ACPI_FAILURE(status)) {
 		return_ACPI_STATUS(status);
 	}
+	/*
+	 * If the target sleep state is S5, clear all GPEs and fixed events too
+	 */
+	if (sleep_state == ACPI_STATE_S5) {
+		status = acpi_hw_clear_acpi_status();
+		if (ACPI_FAILURE(status)) {
+			return_ACPI_STATUS(status);
+		}
+	}
 	acpi_gbl_system_awake_and_running = FALSE;
 
+	 /* Enable all wakeup GPEs */
 	status = acpi_hw_enable_all_wakeup_gpes();
 	if (ACPI_FAILURE(status)) {
 		return_ACPI_STATUS(status);

RE: [PATCH] ACPICA: Clear status of all events when entering S5

[Schmauss, Erik]

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Sunday, July 8, 2018 2:10 AM
> To: Linux ACPI <linux-acpi@vger.kernel.org>
> Cc: Thomas Hänig <haenig@cosifan.de>; Takashi Iwai <tiwai@suse.de>;
> Schmauss, Erik <erik.schmauss@intel.com>; Linux PM <linux-
> pm@vger.kernel.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
> Subject: [PATCH] ACPICA: Clear status of all events when entering S5
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After commit 18996f2db918 (ACPICA: Events: Stop unconditionally clearing ACPI
> IRQs during suspend/resume) the status of ACPI events is not cleared any more
> when entering the ACPI S5 system state (power
> off) which causes some systems to power up immediately after turing off power
> in certain situations.
> 
> That is a functional regression, so address it by making the code clear the status
> of all ACPI events again when entering S5 (for system-wide suspend or
> hibernation the clearing of the status of all events is not desirable, as it might
> cause the kernel to miss wakeup events sometimes).
> 
> Fixes: 18996f2db918 (ACPICA: Events: Stop unconditionally clearing ACPI IRQs
> during suspend/resume)
> Reported-by: Takashi Iwai <tiwai@suse.de>
> Tested-by: Thomas Hänig <haenig@cosifan.de>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Resend https://patchwork.kernel.org/patch/10511451/ with a changelig and
> tags.
> 
> ---
>  drivers/acpi/acpica/hwsleep.c |   15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/drivers/acpi/acpica/hwsleep.c
> =================================================================
> ==
> --- linux-pm.orig/drivers/acpi/acpica/hwsleep.c
> +++ linux-pm/drivers/acpi/acpica/hwsleep.c
> @@ -51,16 +51,23 @@ acpi_status acpi_hw_legacy_sleep(u8 slee
>  		return_ACPI_STATUS(status);
>  	}
> 
> -	/*
> -	 * 1) Disable all GPEs
> -	 * 2) Enable all wakeup GPEs
> -	 */
> +	/* Disable all GPEs */
>  	status = acpi_hw_disable_all_gpes();
>  	if (ACPI_FAILURE(status)) {
>  		return_ACPI_STATUS(status);
>  	}
> +	/*
> +	 * If the target sleep state is S5, clear all GPEs and fixed events too
> +	 */
> +	if (sleep_state == ACPI_STATE_S5) {
> +		status = acpi_hw_clear_acpi_status();
> +		if (ACPI_FAILURE(status)) {
> +			return_ACPI_STATUS(status);
> +		}
> +	}
>  	acpi_gbl_system_awake_and_running = FALSE;
> 
> +	 /* Enable all wakeup GPEs */
>  	status = acpi_hw_enable_all_wakeup_gpes();
>  	if (ACPI_FAILURE(status)) {
>  		return_ACPI_STATUS(status);

Rafael, I've created an ACPICA pull request containing this patch.

Thanks,
Erik

Re: [PATCH] ACPICA: Clear status of all events when entering S5

[Rafael J. Wysocki]

On Mon, Jul 16, 2018 at 7:59 PM, Schmauss, Erik <erik.schmauss@intel.com> wrote:
>
>> -----Original Message-----
>> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
>> Sent: Sunday, July 8, 2018 2:10 AM
>> To: Linux ACPI <linux-acpi@vger.kernel.org>
>> Cc: Thomas Hänig <haenig@cosifan.de>; Takashi Iwai <tiwai@suse.de>;
>> Schmauss, Erik <erik.schmauss@intel.com>; Linux PM <linux-
>> pm@vger.kernel.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
>> Subject: [PATCH] ACPICA: Clear status of all events when entering S5
>>
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> After commit 18996f2db918 (ACPICA: Events: Stop unconditionally clearing ACPI
>> IRQs during suspend/resume) the status of ACPI events is not cleared any more
>> when entering the ACPI S5 system state (power
>> off) which causes some systems to power up immediately after turing off power
>> in certain situations.
>>
>> That is a functional regression, so address it by making the code clear the status
>> of all ACPI events again when entering S5 (for system-wide suspend or
>> hibernation the clearing of the status of all events is not desirable, as it might
>> cause the kernel to miss wakeup events sometimes).
>>
>> Fixes: 18996f2db918 (ACPICA: Events: Stop unconditionally clearing ACPI IRQs
>> during suspend/resume)
>> Reported-by: Takashi Iwai <tiwai@suse.de>
>> Tested-by: Thomas Hänig <haenig@cosifan.de>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>
>> Resend https://patchwork.kernel.org/patch/10511451/ with a changelig and
>> tags.
>>
>> ---
>>  drivers/acpi/acpica/hwsleep.c |   15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> Index: linux-pm/drivers/acpi/acpica/hwsleep.c
>> =================================================================
>> ==
>> --- linux-pm.orig/drivers/acpi/acpica/hwsleep.c
>> +++ linux-pm/drivers/acpi/acpica/hwsleep.c
>> @@ -51,16 +51,23 @@ acpi_status acpi_hw_legacy_sleep(u8 slee
>>               return_ACPI_STATUS(status);
>>       }
>>
>> -     /*
>> -      * 1) Disable all GPEs
>> -      * 2) Enable all wakeup GPEs
>> -      */
>> +     /* Disable all GPEs */
>>       status = acpi_hw_disable_all_gpes();
>>       if (ACPI_FAILURE(status)) {
>>               return_ACPI_STATUS(status);
>>       }
>> +     /*
>> +      * If the target sleep state is S5, clear all GPEs and fixed events too
>> +      */
>> +     if (sleep_state == ACPI_STATE_S5) {
>> +             status = acpi_hw_clear_acpi_status();
>> +             if (ACPI_FAILURE(status)) {
>> +                     return_ACPI_STATUS(status);
>> +             }
>> +     }
>>       acpi_gbl_system_awake_and_running = FALSE;
>>
>> +      /* Enable all wakeup GPEs */
>>       status = acpi_hw_enable_all_wakeup_gpes();
>>       if (ACPI_FAILURE(status)) {
>>               return_ACPI_STATUS(status);
>
> Rafael, I've created an ACPICA pull request containing this patch.

OK, thanks!

The patch is in the mainline kernel tree already at this point, so
that's an upstream only thing.

Can you please also do likewise for
https://patchwork.kernel.org/patch/10495943/ ?

Thanks,
Rafael