[PATCH] drivers/acpi: Clear status of an event before enabling it

[PATCH] drivers/acpi: Clear status of an event before enabling it

[Furquan Shaikh]

Commit 18996f2db918 ("ACPICA: Events: Stop unconditionally
clearing ACPI IRQs during suspend/resume") was added to stop clearing
of event status bits unconditionally on suspend and resume paths. This
was done because of an issue
reported (https://bugzilla.kernel.org/show_bug.cgi?id=196249) where
lid status stays closed even on resume (which happens because event
status bits are cleared unconditionally on resume). Though this change
fixed the issue on suspend path, it introduced regressions on several
resume paths.

First regression was reported and fixed on S5 path by the following
change: commit fa85015c0d95 ("ACPICA: Clear status of all events when
entering S5"). Next regression was reported and fixed on all legacy
sleep paths by the commit f317c7dc12b7 ("ACPICA: Clear status of all
events when entering sleep states"). However, regression still exists
on S0ix sleep path since it does not follow the legacy sleep path.

In case of S0ix, events are enabled as part of device suspend path. If
status bits for the events are set when they are enabled, it could
result in premature wake from S0ix. This change ensures that status is
cleared for any event that is being enabled so that any stale events
are cleared out.

Signed-off-by: Furquan Shaikh <furquan@google.com>
---
 drivers/acpi/acpica/evgpe.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
index 62d3aa74277b4..61455ab42fc87 100644
--- a/drivers/acpi/acpica/evgpe.c
+++ b/drivers/acpi/acpica/evgpe.c
@@ -81,8 +81,12 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
 
 	ACPI_FUNCTION_TRACE(ev_enable_gpe);
 
-	/* Enable the requested 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);
 	return_ACPI_STATUS(status);
 }
-- 
2.21.0.225.g810b269d1ac-goog

Re: [PATCH] drivers/acpi: Clear status of an event before enabling it

[Furquan Shaikh]

Adding authors of the original commit whom I failed to copy in the
original patch.

On Wed, Mar 20, 2019 at 4:34 PM Furquan Shaikh <furquan@google.com> wrote:
>
> Commit 18996f2db918 ("ACPICA: Events: Stop unconditionally
> clearing ACPI IRQs during suspend/resume") was added to stop clearing
> of event status bits unconditionally on suspend and resume paths. This
> was done because of an issue
> reported (https://bugzilla.kernel.org/show_bug.cgi?id=196249) where
> lid status stays closed even on resume (which happens because event
> status bits are cleared unconditionally on resume). Though this change
> fixed the issue on suspend path, it introduced regressions on several
> resume paths.
>
> First regression was reported and fixed on S5 path by the following
> change: commit fa85015c0d95 ("ACPICA: Clear status of all events when
> entering S5"). Next regression was reported and fixed on all legacy
> sleep paths by the commit f317c7dc12b7 ("ACPICA: Clear status of all
> events when entering sleep states"). However, regression still exists
> on S0ix sleep path since it does not follow the legacy sleep path.
>
> In case of S0ix, events are enabled as part of device suspend path. If
> status bits for the events are set when they are enabled, it could
> result in premature wake from S0ix. This change ensures that status is
> cleared for any event that is being enabled so that any stale events
> are cleared out.
>
> Signed-off-by: Furquan Shaikh <furquan@google.com>
> ---
>  drivers/acpi/acpica/evgpe.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
> index 62d3aa74277b4..61455ab42fc87 100644
> --- a/drivers/acpi/acpica/evgpe.c
> +++ b/drivers/acpi/acpica/evgpe.c
> @@ -81,8 +81,12 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
>
>         ACPI_FUNCTION_TRACE(ev_enable_gpe);
>
> -       /* Enable the requested 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);
>         return_ACPI_STATUS(status);
>  }
> --
> 2.21.0.225.g810b269d1ac-goog
>

Re: [PATCH] drivers/acpi: Clear status of an event before enabling it

[Rafael J. Wysocki]

On Wed, Mar 20, 2019 at 11:34 PM Furquan Shaikh <furquan@google.com> wrote:
>
> Commit 18996f2db918 ("ACPICA: Events: Stop unconditionally
> clearing ACPI IRQs during suspend/resume") was added to stop clearing
> of event status bits unconditionally on suspend and resume paths. This
> was done because of an issue
> reported (https://bugzilla.kernel.org/show_bug.cgi?id=196249) where
> lid status stays closed even on resume (which happens because event
> status bits are cleared unconditionally on resume). Though this change
> fixed the issue on suspend path, it introduced regressions on several
> resume paths.
>
> First regression was reported and fixed on S5 path by the following
> change: commit fa85015c0d95 ("ACPICA: Clear status of all events when
> entering S5"). Next regression was reported and fixed on all legacy
> sleep paths by the commit f317c7dc12b7 ("ACPICA: Clear status of all
> events when entering sleep states"). However, regression still exists
> on S0ix sleep path since it does not follow the legacy sleep path.

What exactly is failing?

> In case of S0ix, events are enabled as part of device suspend path. If
> status bits for the events are set when they are enabled, it could
> result in premature wake from S0ix. This change ensures that status is
> cleared for any event that is being enabled so that any stale events
> are cleared out.
>
> Signed-off-by: Furquan Shaikh <furquan@google.com>
> ---
>  drivers/acpi/acpica/evgpe.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
> index 62d3aa74277b4..61455ab42fc87 100644
> --- a/drivers/acpi/acpica/evgpe.c
> +++ b/drivers/acpi/acpica/evgpe.c
> @@ -81,8 +81,12 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
>
>         ACPI_FUNCTION_TRACE(ev_enable_gpe);
>
> -       /* Enable the requested GPE */
> +       /* Clear the GPE (of stale events) */
> +       status = acpi_hw_clear_gpe(gpe_event_info);
> +       if (ACPI_FAILURE(status))
> +               return_ACPI_STATUS(status);

Well, this may cause events to be missed.

>
> +       /* Enable the requested GPE */
>         status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
>         return_ACPI_STATUS(status);
>  }
> --
> 2.21.0.225.g810b269d1ac-goog
>

Re: [Devel] [PATCH] drivers/acpi: Clear status of an event before enabling it

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 2390 bytes --]

On Wed, Mar 20, 2019 at 11:34 PM Furquan Shaikh <furquan(a)google.com> wrote:
>
> Commit 18996f2db918 ("ACPICA: Events: Stop unconditionally
> clearing ACPI IRQs during suspend/resume") was added to stop clearing
> of event status bits unconditionally on suspend and resume paths. This
> was done because of an issue
> reported (https://bugzilla.kernel.org/show_bug.cgi?id=196249) where
> lid status stays closed even on resume (which happens because event
> status bits are cleared unconditionally on resume). Though this change
> fixed the issue on suspend path, it introduced regressions on several
> resume paths.
>
> First regression was reported and fixed on S5 path by the following
> change: commit fa85015c0d95 ("ACPICA: Clear status of all events when
> entering S5"). Next regression was reported and fixed on all legacy
> sleep paths by the commit f317c7dc12b7 ("ACPICA: Clear status of all
> events when entering sleep states"). However, regression still exists
> on S0ix sleep path since it does not follow the legacy sleep path.

What exactly is failing?

> In case of S0ix, events are enabled as part of device suspend path. If
> status bits for the events are set when they are enabled, it could
> result in premature wake from S0ix. This change ensures that status is
> cleared for any event that is being enabled so that any stale events
> are cleared out.
>
> Signed-off-by: Furquan Shaikh <furquan(a)google.com>
> ---
>  drivers/acpi/acpica/evgpe.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
> index 62d3aa74277b4..61455ab42fc87 100644
> --- a/drivers/acpi/acpica/evgpe.c
> +++ b/drivers/acpi/acpica/evgpe.c
> @@ -81,8 +81,12 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
>
>         ACPI_FUNCTION_TRACE(ev_enable_gpe);
>
> -       /* Enable the requested GPE */
> +       /* Clear the GPE (of stale events) */
> +       status = acpi_hw_clear_gpe(gpe_event_info);
> +       if (ACPI_FAILURE(status))
> +               return_ACPI_STATUS(status);

Well, this may cause events to be missed.

>
> +       /* Enable the requested GPE */
>         status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
>         return_ACPI_STATUS(status);
>  }
> --
> 2.21.0.225.g810b269d1ac-goog
>

Re: [PATCH] drivers/acpi: Clear status of an event before enabling it

[Furquan Shaikh]

On Wed, Mar 20, 2019 at 5:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Mar 20, 2019 at 11:34 PM Furquan Shaikh <furquan@google.com> wrote:
> >
> > Commit 18996f2db918 ("ACPICA: Events: Stop unconditionally
> > clearing ACPI IRQs during suspend/resume") was added to stop clearing
> > of event status bits unconditionally on suspend and resume paths. This
> > was done because of an issue
> > reported (https://bugzilla.kernel.org/show_bug.cgi?id=196249) where
> > lid status stays closed even on resume (which happens because event
> > status bits are cleared unconditionally on resume). Though this change
> > fixed the issue on suspend path, it introduced regressions on several
> > resume paths.
> >
> > First regression was reported and fixed on S5 path by the following
> > change: commit fa85015c0d95 ("ACPICA: Clear status of all events when
> > entering S5"). Next regression was reported and fixed on all legacy
> > sleep paths by the commit f317c7dc12b7 ("ACPICA: Clear status of all
> > events when entering sleep states"). However, regression still exists
> > on S0ix sleep path since it does not follow the legacy sleep path.
>
> What exactly is failing?

Here is the failure scenario:

1. Consider the case of trackpad which acts as a wake source.
2. Since the pad is configured for SCI, GPE_STS gets set for that pad
during normal interrupts as well (i.e. during probe at boot or when
using the trackpad)
3. Now, when the platform decides to enter S0ix, it enables the wake
on trackpad by enabling appropriate GPE_EN bit.
4. So, at this point, we are in a situation where GPE_EN as well as
GPE_STS are set.
5. Now, if the platform enters S0ix, having GPE_STS set will result in
unwanted wakes because of stale events.

This is similar to what was fixed on the legacy sleep path:
https://lkml.org/lkml/2018/8/12/42. However, as S0ix does not follow
the legacy sleep path, clearing of GPE status does not happen. Thus,
it is causing failures to go into S0ix on our platforms because of the
stale wake events as described above.

>
> > In case of S0ix, events are enabled as part of device suspend path. If
> > status bits for the events are set when they are enabled, it could
> > result in premature wake from S0ix. This change ensures that status is
> > cleared for any event that is being enabled so that any stale events
> > are cleared out.
> >
> > Signed-off-by: Furquan Shaikh <furquan@google.com>
> > ---
> >  drivers/acpi/acpica/evgpe.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
> > index 62d3aa74277b4..61455ab42fc87 100644
> > --- a/drivers/acpi/acpica/evgpe.c
> > +++ b/drivers/acpi/acpica/evgpe.c
> > @@ -81,8 +81,12 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
> >
> >         ACPI_FUNCTION_TRACE(ev_enable_gpe);
> >
> > -       /* Enable the requested GPE */
> > +       /* Clear the GPE (of stale events) */
> > +       status = acpi_hw_clear_gpe(gpe_event_info);
> > +       if (ACPI_FAILURE(status))
> > +               return_ACPI_STATUS(status);
>
> Well, this may cause events to be missed.

Won't those be stale events? i.e. any event that has occurred before
GPE is enabled should be ignored.

>
> >
> > +       /* Enable the requested GPE */
> >         status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
> >         return_ACPI_STATUS(status);
> >  }
> > --
> > 2.21.0.225.g810b269d1ac-goog
> >

Re: [PATCH] drivers/acpi: Clear status of an event before enabling it

[Rafael J. Wysocki]

On Thu, Mar 21, 2019 at 3:16 AM Furquan Shaikh <furquan@google.com> wrote:
>
> On Wed, Mar 20, 2019 at 5:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Mar 20, 2019 at 11:34 PM Furquan Shaikh <furquan@google.com> wrote:
> > >
> > > Commit 18996f2db918 ("ACPICA: Events: Stop unconditionally
> > > clearing ACPI IRQs during suspend/resume") was added to stop clearing
> > > of event status bits unconditionally on suspend and resume paths. This
> > > was done because of an issue
> > > reported (https://bugzilla.kernel.org/show_bug.cgi?id=196249) where
> > > lid status stays closed even on resume (which happens because event
> > > status bits are cleared unconditionally on resume). Though this change
> > > fixed the issue on suspend path, it introduced regressions on several
> > > resume paths.
> > >
> > > First regression was reported and fixed on S5 path by the following
> > > change: commit fa85015c0d95 ("ACPICA: Clear status of all events when
> > > entering S5"). Next regression was reported and fixed on all legacy
> > > sleep paths by the commit f317c7dc12b7 ("ACPICA: Clear status of all
> > > events when entering sleep states"). However, regression still exists
> > > on S0ix sleep path since it does not follow the legacy sleep path.
> >
> > What exactly is failing?
>
> Here is the failure scenario:
>
> 1. Consider the case of trackpad which acts as a wake source.
> 2. Since the pad is configured for SCI, GPE_STS gets set for that pad
> during normal interrupts as well (i.e. during probe at boot or when
> using the trackpad)

I don't quite understand this.

Is the same GPE used for signaling trackpad events in the system
working state and for wakeup?

> 3. Now, when the platform decides to enter S0ix, it enables the wake
> on trackpad by enabling appropriate GPE_EN bit.
> 4. So, at this point, we are in a situation where GPE_EN as well as
> GPE_STS are set.
> 5. Now, if the platform enters S0ix, having GPE_STS set will result in
> unwanted wakes because of stale events.
>
> This is similar to what was fixed on the legacy sleep path:
> https://lkml.org/lkml/2018/8/12/42. However, as S0ix does not follow
> the legacy sleep path, clearing of GPE status does not happen. Thus,
> it is causing failures to go into S0ix on our platforms because of the
> stale wake events as described above.
>
> >
> > > In case of S0ix, events are enabled as part of device suspend path. If
> > > status bits for the events are set when they are enabled, it could
> > > result in premature wake from S0ix. This change ensures that status is
> > > cleared for any event that is being enabled so that any stale events
> > > are cleared out.
> > >
> > > Signed-off-by: Furquan Shaikh <furquan@google.com>
> > > ---
> > >  drivers/acpi/acpica/evgpe.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
> > > index 62d3aa74277b4..61455ab42fc87 100644
> > > --- a/drivers/acpi/acpica/evgpe.c
> > > +++ b/drivers/acpi/acpica/evgpe.c
> > > @@ -81,8 +81,12 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
> > >
> > >         ACPI_FUNCTION_TRACE(ev_enable_gpe);
> > >
> > > -       /* Enable the requested GPE */
> > > +       /* Clear the GPE (of stale events) */
> > > +       status = acpi_hw_clear_gpe(gpe_event_info);
> > > +       if (ACPI_FAILURE(status))
> > > +               return_ACPI_STATUS(status);
> >
> > Well, this may cause events to be missed.
>
> Won't those be stale events?

They need not be stale, the device may have already detected some
activity before the GPE is enabled.

> i.e. any event that has occurred before GPE is enabled should be ignored.

But this is a good point.

>
> >
> > >
> > > +       /* Enable the requested GPE */
> > >         status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
> > >         return_ACPI_STATUS(status);
> > >  }
> > > --

Re: [Devel] [PATCH] drivers/acpi: Clear status of an event before enabling it

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 4054 bytes --]

On Thu, Mar 21, 2019 at 3:16 AM Furquan Shaikh <furquan(a)google.com> wrote:
>
> On Wed, Mar 20, 2019 at 5:11 PM Rafael J. Wysocki <rafael(a)kernel.org> wrote:
> >
> > On Wed, Mar 20, 2019 at 11:34 PM Furquan Shaikh <furquan(a)google.com> wrote:
> > >
> > > Commit 18996f2db918 ("ACPICA: Events: Stop unconditionally
> > > clearing ACPI IRQs during suspend/resume") was added to stop clearing
> > > of event status bits unconditionally on suspend and resume paths. This
> > > was done because of an issue
> > > reported (https://bugzilla.kernel.org/show_bug.cgi?id=196249) where
> > > lid status stays closed even on resume (which happens because event
> > > status bits are cleared unconditionally on resume). Though this change
> > > fixed the issue on suspend path, it introduced regressions on several
> > > resume paths.
> > >
> > > First regression was reported and fixed on S5 path by the following
> > > change: commit fa85015c0d95 ("ACPICA: Clear status of all events when
> > > entering S5"). Next regression was reported and fixed on all legacy
> > > sleep paths by the commit f317c7dc12b7 ("ACPICA: Clear status of all
> > > events when entering sleep states"). However, regression still exists
> > > on S0ix sleep path since it does not follow the legacy sleep path.
> >
> > What exactly is failing?
>
> Here is the failure scenario:
>
> 1. Consider the case of trackpad which acts as a wake source.
> 2. Since the pad is configured for SCI, GPE_STS gets set for that pad
> during normal interrupts as well (i.e. during probe at boot or when
> using the trackpad)

I don't quite understand this.

Is the same GPE used for signaling trackpad events in the system
working state and for wakeup?

> 3. Now, when the platform decides to enter S0ix, it enables the wake
> on trackpad by enabling appropriate GPE_EN bit.
> 4. So, at this point, we are in a situation where GPE_EN as well as
> GPE_STS are set.
> 5. Now, if the platform enters S0ix, having GPE_STS set will result in
> unwanted wakes because of stale events.
>
> This is similar to what was fixed on the legacy sleep path:
> https://lkml.org/lkml/2018/8/12/42. However, as S0ix does not follow
> the legacy sleep path, clearing of GPE status does not happen. Thus,
> it is causing failures to go into S0ix on our platforms because of the
> stale wake events as described above.
>
> >
> > > In case of S0ix, events are enabled as part of device suspend path. If
> > > status bits for the events are set when they are enabled, it could
> > > result in premature wake from S0ix. This change ensures that status is
> > > cleared for any event that is being enabled so that any stale events
> > > are cleared out.
> > >
> > > Signed-off-by: Furquan Shaikh <furquan(a)google.com>
> > > ---
> > >  drivers/acpi/acpica/evgpe.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
> > > index 62d3aa74277b4..61455ab42fc87 100644
> > > --- a/drivers/acpi/acpica/evgpe.c
> > > +++ b/drivers/acpi/acpica/evgpe.c
> > > @@ -81,8 +81,12 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
> > >
> > >         ACPI_FUNCTION_TRACE(ev_enable_gpe);
> > >
> > > -       /* Enable the requested GPE */
> > > +       /* Clear the GPE (of stale events) */
> > > +       status = acpi_hw_clear_gpe(gpe_event_info);
> > > +       if (ACPI_FAILURE(status))
> > > +               return_ACPI_STATUS(status);
> >
> > Well, this may cause events to be missed.
>
> Won't those be stale events?

They need not be stale, the device may have already detected some
activity before the GPE is enabled.

> i.e. any event that has occurred before GPE is enabled should be ignored.

But this is a good point.

>
> >
> > >
> > > +       /* Enable the requested GPE */
> > >         status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
> > >         return_ACPI_STATUS(status);
> > >  }
> > > --

Re: [PATCH] drivers/acpi: Clear status of an event before enabling it

[Furquan Shaikh]

On Wed, Mar 27, 2019 at 5:24 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Mar 21, 2019 at 3:16 AM Furquan Shaikh <furquan@google.com> wrote:
> >
> > On Wed, Mar 20, 2019 at 5:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Mar 20, 2019 at 11:34 PM Furquan Shaikh <furquan@google.com> wrote:
> > > >
> > > > Commit 18996f2db918 ("ACPICA: Events: Stop unconditionally
> > > > clearing ACPI IRQs during suspend/resume") was added to stop clearing
> > > > of event status bits unconditionally on suspend and resume paths. This
> > > > was done because of an issue
> > > > reported (https://bugzilla.kernel.org/show_bug.cgi?id=196249) where
> > > > lid status stays closed even on resume (which happens because event
> > > > status bits are cleared unconditionally on resume). Though this change
> > > > fixed the issue on suspend path, it introduced regressions on several
> > > > resume paths.
> > > >
> > > > First regression was reported and fixed on S5 path by the following
> > > > change: commit fa85015c0d95 ("ACPICA: Clear status of all events when
> > > > entering S5"). Next regression was reported and fixed on all legacy
> > > > sleep paths by the commit f317c7dc12b7 ("ACPICA: Clear status of all
> > > > events when entering sleep states"). However, regression still exists
> > > > on S0ix sleep path since it does not follow the legacy sleep path.
> > >
> > > What exactly is failing?
> >
> > Here is the failure scenario:
> >
> > 1. Consider the case of trackpad which acts as a wake source.
> > 2. Since the pad is configured for SCI, GPE_STS gets set for that pad
> > during normal interrupts as well (i.e. during probe at boot or when
> > using the trackpad)
>
> I don't quite understand this.
>
> Is the same GPE used for signaling trackpad events in the system
> working state and for wakeup?

Yes. The same pad is being configured for interrupts (i.e. routed to
APIC) during S0 as well as configured for GPE (i.e. routed for SCI) to
cause wakes when in S0ix/S3. This pad is externally connected to
trackpad interrupt line.

>
> > 3. Now, when the platform decides to enter S0ix, it enables the wake
> > on trackpad by enabling appropriate GPE_EN bit.
> > 4. So, at this point, we are in a situation where GPE_EN as well as
> > GPE_STS are set.
> > 5. Now, if the platform enters S0ix, having GPE_STS set will result in
> > unwanted wakes because of stale events.
> >
> > This is similar to what was fixed on the legacy sleep path:
> > https://lkml.org/lkml/2018/8/12/42. However, as S0ix does not follow
> > the legacy sleep path, clearing of GPE status does not happen. Thus,
> > it is causing failures to go into S0ix on our platforms because of the
> > stale wake events as described above.
> >
> > >
> > > > In case of S0ix, events are enabled as part of device suspend path. If
> > > > status bits for the events are set when they are enabled, it could
> > > > result in premature wake from S0ix. This change ensures that status is
> > > > cleared for any event that is being enabled so that any stale events
> > > > are cleared out.
> > > >
> > > > Signed-off-by: Furquan Shaikh <furquan@google.com>
> > > > ---
> > > >  drivers/acpi/acpica/evgpe.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
> > > > index 62d3aa74277b4..61455ab42fc87 100644
> > > > --- a/drivers/acpi/acpica/evgpe.c
> > > > +++ b/drivers/acpi/acpica/evgpe.c
> > > > @@ -81,8 +81,12 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
> > > >
> > > >         ACPI_FUNCTION_TRACE(ev_enable_gpe);
> > > >
> > > > -       /* Enable the requested GPE */
> > > > +       /* Clear the GPE (of stale events) */
> > > > +       status = acpi_hw_clear_gpe(gpe_event_info);
> > > > +       if (ACPI_FAILURE(status))
> > > > +               return_ACPI_STATUS(status);
> > >
> > > Well, this may cause events to be missed.
> >
> > Won't those be stale events?
>
> They need not be stale, the device may have already detected some
> activity before the GPE is enabled.
>
> > i.e. any event that has occurred before GPE is enabled should be ignored.
>
> But this is a good point.
>
> >
> > >
> > > >
> > > > +       /* Enable the requested GPE */
> > > >         status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
> > > >         return_ACPI_STATUS(status);
> > > >  }
> > > > --

Re: [PATCH] drivers/acpi: Clear status of an event before enabling it

[Rafael J. Wysocki]

On Thu, Mar 28, 2019 at 1:46 AM Furquan Shaikh <furquan@google.com> wrote:
>
> On Wed, Mar 27, 2019 at 5:24 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Mar 21, 2019 at 3:16 AM Furquan Shaikh <furquan@google.com> wrote:
> > >
> > > On Wed, Mar 20, 2019 at 5:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Wed, Mar 20, 2019 at 11:34 PM Furquan Shaikh <furquan@google.com> wrote:
> > > > >
> > > > > Commit 18996f2db918 ("ACPICA: Events: Stop unconditionally
> > > > > clearing ACPI IRQs during suspend/resume") was added to stop clearing
> > > > > of event status bits unconditionally on suspend and resume paths. This
> > > > > was done because of an issue
> > > > > reported (https://bugzilla.kernel.org/show_bug.cgi?id=196249) where
> > > > > lid status stays closed even on resume (which happens because event
> > > > > status bits are cleared unconditionally on resume). Though this change
> > > > > fixed the issue on suspend path, it introduced regressions on several
> > > > > resume paths.
> > > > >
> > > > > First regression was reported and fixed on S5 path by the following
> > > > > change: commit fa85015c0d95 ("ACPICA: Clear status of all events when
> > > > > entering S5"). Next regression was reported and fixed on all legacy
> > > > > sleep paths by the commit f317c7dc12b7 ("ACPICA: Clear status of all
> > > > > events when entering sleep states"). However, regression still exists
> > > > > on S0ix sleep path since it does not follow the legacy sleep path.
> > > >
> > > > What exactly is failing?
> > >
> > > Here is the failure scenario:
> > >
> > > 1. Consider the case of trackpad which acts as a wake source.
> > > 2. Since the pad is configured for SCI, GPE_STS gets set for that pad
> > > during normal interrupts as well (i.e. during probe at boot or when
> > > using the trackpad)
> >
> > I don't quite understand this.
> >
> > Is the same GPE used for signaling trackpad events in the system
> > working state and for wakeup?
>
> Yes. The same pad is being configured for interrupts (i.e. routed to
> APIC) during S0 as well as configured for GPE (i.e. routed for SCI) to
> cause wakes when in S0ix/S3. This pad is externally connected to
> trackpad interrupt line.

And the way the system is wired up causes the GPE status to be set
when it is enabled, I suppose?

So effectively, you need to disable the active-state IRQ, then clear
the GPE status and enable it for wakeup.

OK, I'll queue up the patch (and talk to the upstream ACPICA people
about taking it in there).

Thanks!

Re: [Devel] [PATCH] drivers/acpi: Clear status of an event before enabling it

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 2622 bytes --]

On Thu, Mar 28, 2019 at 1:46 AM Furquan Shaikh <furquan(a)google.com> wrote:
>
> On Wed, Mar 27, 2019 at 5:24 AM Rafael J. Wysocki <rafael(a)kernel.org> wrote:
> >
> > On Thu, Mar 21, 2019 at 3:16 AM Furquan Shaikh <furquan(a)google.com> wrote:
> > >
> > > On Wed, Mar 20, 2019 at 5:11 PM Rafael J. Wysocki <rafael(a)kernel.org> wrote:
> > > >
> > > > On Wed, Mar 20, 2019 at 11:34 PM Furquan Shaikh <furquan(a)google.com> wrote:
> > > > >
> > > > > Commit 18996f2db918 ("ACPICA: Events: Stop unconditionally
> > > > > clearing ACPI IRQs during suspend/resume") was added to stop clearing
> > > > > of event status bits unconditionally on suspend and resume paths. This
> > > > > was done because of an issue
> > > > > reported (https://bugzilla.kernel.org/show_bug.cgi?id=196249) where
> > > > > lid status stays closed even on resume (which happens because event
> > > > > status bits are cleared unconditionally on resume). Though this change
> > > > > fixed the issue on suspend path, it introduced regressions on several
> > > > > resume paths.
> > > > >
> > > > > First regression was reported and fixed on S5 path by the following
> > > > > change: commit fa85015c0d95 ("ACPICA: Clear status of all events when
> > > > > entering S5"). Next regression was reported and fixed on all legacy
> > > > > sleep paths by the commit f317c7dc12b7 ("ACPICA: Clear status of all
> > > > > events when entering sleep states"). However, regression still exists
> > > > > on S0ix sleep path since it does not follow the legacy sleep path.
> > > >
> > > > What exactly is failing?
> > >
> > > Here is the failure scenario:
> > >
> > > 1. Consider the case of trackpad which acts as a wake source.
> > > 2. Since the pad is configured for SCI, GPE_STS gets set for that pad
> > > during normal interrupts as well (i.e. during probe at boot or when
> > > using the trackpad)
> >
> > I don't quite understand this.
> >
> > Is the same GPE used for signaling trackpad events in the system
> > working state and for wakeup?
>
> Yes. The same pad is being configured for interrupts (i.e. routed to
> APIC) during S0 as well as configured for GPE (i.e. routed for SCI) to
> cause wakes when in S0ix/S3. This pad is externally connected to
> trackpad interrupt line.

And the way the system is wired up causes the GPE status to be set
when it is enabled, I suppose?

So effectively, you need to disable the active-state IRQ, then clear
the GPE status and enable it for wakeup.

OK, I'll queue up the patch (and talk to the upstream ACPICA people
about taking it in there).

Thanks!

Re: [PATCH] drivers/acpi: Clear status of an event before enabling it

[Furquan Shaikh]

On Thu, Mar 28, 2019 at 1:57 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Mar 28, 2019 at 1:46 AM Furquan Shaikh <furquan@google.com> wrote:
> >
> > On Wed, Mar 27, 2019 at 5:24 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Thu, Mar 21, 2019 at 3:16 AM Furquan Shaikh <furquan@google.com> wrote:
> > > >
> > > > On Wed, Mar 20, 2019 at 5:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Wed, Mar 20, 2019 at 11:34 PM Furquan Shaikh <furquan@google.com> wrote:
> > > > > >
> > > > > > Commit 18996f2db918 ("ACPICA: Events: Stop unconditionally
> > > > > > clearing ACPI IRQs during suspend/resume") was added to stop clearing
> > > > > > of event status bits unconditionally on suspend and resume paths. This
> > > > > > was done because of an issue
> > > > > > reported (https://bugzilla.kernel.org/show_bug.cgi?id=196249) where
> > > > > > lid status stays closed even on resume (which happens because event
> > > > > > status bits are cleared unconditionally on resume). Though this change
> > > > > > fixed the issue on suspend path, it introduced regressions on several
> > > > > > resume paths.
> > > > > >
> > > > > > First regression was reported and fixed on S5 path by the following
> > > > > > change: commit fa85015c0d95 ("ACPICA: Clear status of all events when
> > > > > > entering S5"). Next regression was reported and fixed on all legacy
> > > > > > sleep paths by the commit f317c7dc12b7 ("ACPICA: Clear status of all
> > > > > > events when entering sleep states"). However, regression still exists
> > > > > > on S0ix sleep path since it does not follow the legacy sleep path.
> > > > >
> > > > > What exactly is failing?
> > > >
> > > > Here is the failure scenario:
> > > >
> > > > 1. Consider the case of trackpad which acts as a wake source.
> > > > 2. Since the pad is configured for SCI, GPE_STS gets set for that pad
> > > > during normal interrupts as well (i.e. during probe at boot or when
> > > > using the trackpad)
> > >
> > > I don't quite understand this.
> > >
> > > Is the same GPE used for signaling trackpad events in the system
> > > working state and for wakeup?
> >
> > Yes. The same pad is being configured for interrupts (i.e. routed to
> > APIC) during S0 as well as configured for GPE (i.e. routed for SCI) to
> > cause wakes when in S0ix/S3. This pad is externally connected to
> > trackpad interrupt line.
>
> And the way the system is wired up causes the GPE status to be set
> when it is enabled, I suppose?

That is correct.

>
> So effectively, you need to disable the active-state IRQ, then clear
> the GPE status and enable it for wakeup.
>
> OK, I'll queue up the patch (and talk to the upstream ACPICA people
> about taking it in there).

Thank you!

>
> Thanks!