From patchwork Fri Jul 6 06:55:18 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 10510853 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 63EBC600CA for ; Fri, 6 Jul 2018 06:55:23 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 525261FFB1 for ; Fri, 6 Jul 2018 06:55:23 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 443C6284AF; Fri, 6 Jul 2018 06:55:23 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 93A6D1FFB1 for ; Fri, 6 Jul 2018 06:55:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753174AbeGFGzV (ORCPT ); Fri, 6 Jul 2018 02:55:21 -0400 Received: from mx2.suse.de ([195.135.220.15]:41652 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753096AbeGFGzU (ORCPT ); Fri, 6 Jul 2018 02:55:20 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 1182AAD72; Fri, 6 Jul 2018 06:55:19 +0000 (UTC) Date: Fri, 06 Jul 2018 08:55:18 +0200 Message-ID: From: Takashi Iwai To: Thomas H4nig Cc: "Rafael J. Wysocki" , Erik Schmauss , Linux PM , Linux Kernel Mailing List , Linux ACPI Subject: Re: [REGRESSION 4.17] Spurious wakeup / reboot with power button In-Reply-To: <5cce6fa6-c089-7a6b-aaef-bdd04a3761c6@cosifan.de> References: <3165315.C5RQ1k0pt5@aspire.rjw.lan> <5cce6fa6-c089-7a6b-aaef-bdd04a3761c6@cosifan.de> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 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 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(): 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 --- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- 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.