From: Furquan Shaikh <furquan@google.com>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>,
Len Brown <lenb@kernel.org>,
ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Rajat Jain <rajatja@google.com>
Subject: Re: [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling
Date: Thu, 13 Jun 2019 17:58:54 -0700 [thread overview]
Message-ID: <CAEGmHFFmfyB=bEx_UMi4ZGeTR9Bxs2TdnscyzJMrv2fW1_fDMg@mail.gmail.com> (raw)
In-Reply-To: <13361760.nMXA0SR1Mq@kreacher>
On Thu, Jun 13, 2019 at 1:24 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Thursday, May 16, 2019 9:36:16 PM CEST Furquan Shaikh wrote:
> > This change clears GPE status for wake-up devices before enabling that
> > GPE. This is required to ensure that stale GPE status does
> > not result in pre-mature wake on enabling GPE for wake-up devices.
> >
> > Without this change, here is the sequence of events that is causing
> > suspend aborts on recent chrome books:
> >
> > 1. System decides to enter sleep.
> > 2. All devices in the system are put into low power mode.
> > 3. This results in acpi_dev_suspend being called for each ACPI
> > device.
> > 4. If the device is wake capable, then acpi_dev_suspend calls
> > acpi_device_wakeup_enable to enable GPE for the device.
> > 5. If GPE status is already set, enabling GPE for the wakeup device
> > results in generating a SCI which is handled by acpi_ev_detect_gpe
> > ultimately calling wakeup_source_activate that increments wakeup
> > events, and thus aborting the suspend attempt.
> >
> > Signed-off-by: Furquan Shaikh <furquan@google.com>
> > ---
> > drivers/acpi/device_pm.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > index b859d75eaf9f6..e05ee3ff45683 100644
> > --- a/drivers/acpi/device_pm.c
> > +++ b/drivers/acpi/device_pm.c
> > @@ -721,6 +721,8 @@ static int __acpi_device_wakeup_enable(struct acpi_device *adev,
> > if (error)
> > goto out;
> >
> > + acpi_clear_gpe(wakeup->gpe_device, wakeup->gpe_number);
> > +
> > status = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number);
> > if (ACPI_FAILURE(status)) {
> > acpi_disable_wakeup_device_power(adev);
> >
>
> This patch may cause events to be missed if the GPE. I guess what you reall mean is
> something like the patch below.
Thanks for the patch Rafael! This indeed fixes the issue on my platform.
FWIW, Tested-By: Furquan Shaikh <furquan@google.com>
>
> This should allow the kernel to see the events generated before the GPEs are
> implicitly enabled, but it should clear them for the explicit users of acpi_enable_gpe().
>
> Mika, what do you think?
>
> ---
> drivers/acpi/acpica/acevents.h | 3 ++-
> drivers/acpi/acpica/evgpe.c | 8 +++++++-
> drivers/acpi/acpica/evgpeblk.c | 2 +-
> drivers/acpi/acpica/evxface.c | 2 +-
> drivers/acpi/acpica/evxfgpe.c | 2 +-
> 5 files changed, 12 insertions(+), 5 deletions(-)
>
> Index: linux-pm/drivers/acpi/acpica/acevents.h
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/acevents.h
> +++ linux-pm/drivers/acpi/acpica/acevents.h
> @@ -69,7 +69,8 @@ acpi_status
> acpi_ev_mask_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 is_masked);
>
> acpi_status
> -acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info);
> +acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info,
> + u8 clear_on_enable);
>
> acpi_status
> acpi_ev_remove_gpe_reference(struct acpi_gpe_event_info *gpe_event_info);
> Index: linux-pm/drivers/acpi/acpica/evgpe.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/evgpe.c
> +++ linux-pm/drivers/acpi/acpica/evgpe.c
> @@ -146,6 +146,7 @@ acpi_ev_mask_gpe(struct acpi_gpe_event_i
> * FUNCTION: acpi_ev_add_gpe_reference
> *
> * PARAMETERS: gpe_event_info - Add a reference to this GPE
> + * clear_on_enable - Clear GPE status before enabling it
> *
> * RETURN: Status
> *
> @@ -155,7 +156,8 @@ acpi_ev_mask_gpe(struct acpi_gpe_event_i
> ******************************************************************************/
>
> acpi_status
> -acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info)
> +acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info,
> + u8 clear_on_enable)
> {
> acpi_status status = AE_OK;
>
> @@ -170,6 +172,10 @@ acpi_ev_add_gpe_reference(struct acpi_gp
>
> /* Enable on first reference */
>
> + if (clear_on_enable) {
> + (void)acpi_hw_clear_gpe(gpe_event_info);
> + }
> +
> status = acpi_ev_update_gpe_enable_mask(gpe_event_info);
> if (ACPI_SUCCESS(status)) {
> status = acpi_ev_enable_gpe(gpe_event_info);
> Index: linux-pm/drivers/acpi/acpica/evgpeblk.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/evgpeblk.c
> +++ linux-pm/drivers/acpi/acpica/evgpeblk.c
> @@ -453,7 +453,7 @@ acpi_ev_initialize_gpe_block(struct acpi
> continue;
> }
>
> - status = acpi_ev_add_gpe_reference(gpe_event_info);
> + status = acpi_ev_add_gpe_reference(gpe_event_info, FALSE);
> if (ACPI_FAILURE(status)) {
> ACPI_EXCEPTION((AE_INFO, status,
> "Could not enable GPE 0x%02X",
> Index: linux-pm/drivers/acpi/acpica/evxface.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/evxface.c
> +++ linux-pm/drivers/acpi/acpica/evxface.c
> @@ -971,7 +971,7 @@ acpi_remove_gpe_handler(acpi_handle gpe_
> ACPI_GPE_DISPATCH_METHOD) ||
> (ACPI_GPE_DISPATCH_TYPE(handler->original_flags) ==
> ACPI_GPE_DISPATCH_NOTIFY)) && handler->originally_enabled) {
> - (void)acpi_ev_add_gpe_reference(gpe_event_info);
> + (void)acpi_ev_add_gpe_reference(gpe_event_info, FALSE);
> if (ACPI_GPE_IS_POLLING_NEEDED(gpe_event_info)) {
>
> /* Poll edge triggered GPEs to handle existing events */
> Index: linux-pm/drivers/acpi/acpica/evxfgpe.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/evxfgpe.c
> +++ linux-pm/drivers/acpi/acpica/evxfgpe.c
> @@ -108,7 +108,7 @@ acpi_status acpi_enable_gpe(acpi_handle
> if (gpe_event_info) {
> if (ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) !=
> ACPI_GPE_DISPATCH_NONE) {
> - status = acpi_ev_add_gpe_reference(gpe_event_info);
> + status = acpi_ev_add_gpe_reference(gpe_event_info, TRUE);
> if (ACPI_SUCCESS(status) &&
> ACPI_GPE_IS_POLLING_NEEDED(gpe_event_info)) {
>
>
>
>
next prev parent reply other threads:[~2019-06-14 0:59 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-16 19:36 [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling Furquan Shaikh
2019-06-13 20:24 ` Rafael J. Wysocki
2019-06-14 0:58 ` Furquan Shaikh [this message]
2019-06-14 10:45 ` Mika Westerberg
2019-06-14 13:27 ` Rafael J. Wysocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEGmHFFmfyB=bEx_UMi4ZGeTR9Bxs2TdnscyzJMrv2fW1_fDMg@mail.gmail.com' \
--to=furquan@google.com \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mika.westerberg@linux.intel.com \
--cc=rajatja@google.com \
--cc=rjw@rjwysocki.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).