Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: "Saha, Shaunak" <shaunak.saha@intel.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>,
	Mario Limonciello <mario_limonciello@dell.com>
Cc: ACPI Devel Maling List <linux-acpi@vger.kernel.org>
Subject: RE: drivers/acpi: Change the lpit function call flow
Date: Wed, 19 Jun 2019 05:33:26 +0000
Message-ID: <593AE8A18AD46543B92D8EF146E2AA9408C326BA@ORSMSX107.amr.corp.intel.com> (raw)
In-Reply-To: <CAJZ5v0iHRzC=ewqUTa_VUdJ2NOztnP+hDYBeJLtfDydArPkkeA@mail.gmail.com>

Hi Rafael,

Thanks for your review comments. Agreed that this patch needs to be fixed. I am working on it. 
1.First off, aborting system suspend because S0ix constraints are not met is a non-starter.  The kernel really cannot refuse to suspend the system for  that reason (and diagnostics can be done after resume anyway).

For this as the LPIT specs says "_DSM function may be invoked when the OS state has reached sufficient criteria
for processor idle state entry matching Entry Trigger defined in LPIT" then  to check that entry trigger from before calling the sleep function would be better approach. Will report an error if that is not met and still go to suspend. Is that a proper way to handle this? please suggest.

2. Second, according to my knowledge, it is not a bug to invoke the ACPI_LPS0_ENTRY _DSM when the constraints are not met.  Do you actually know about any platforms where that may cause real problems to occur?

Actually in the present platform i m working we are seeing the failure to go to suspend whenever this _DSM methods are called. But as you correctly mentioned i found that the bug was not because this function was called early but it's happening because in our case ec seems to be bit noisy and that's why it's coming out of suspend too early. Something is going wrong in the ec polling function it seems in this case.

3. Finally, it is too late to invoke that _DSM from acpi_s2idle_sync(), because that is called after leaving S0ix and resuming some devices.

Agreed. I think moving the call of DSM from s2idle_enter is better if we decide to move it a later stage
________________________________________
From: Rafael J. Wysocki [rafael@kernel.org]
Sent: Tuesday, June 18, 2019 1:44 AM
To: Mario Limonciello
Cc: Saha, Shaunak; ACPI Devel Maling List
Subject: Re: drivers/acpi: Change the lpit function call flow

On Thu, Jun 13, 2019 at 11:37 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Sunday, May 26, 2019 10:27:50 PM CEST Shaunak Saha wrote:
> > In the present implementation sleep function was getting called in
> > acpi_s2idle_prepare and as all the devices may not have suspended
> > at that stage e.g. if we are telling ec about the S0ix then calling early
> > can cause ec reply wrongly interpreted as spurious wake events.
> > Here we call it at much later stage in acpi_s2idle_sync. As per the
> > specification the entry _DSM function may be invoked when the OS state has
> > reached sufficient criteria for processor idle state entry matching
> > Entry Trigger defined in LPIT to be interpreted as a request for the
> > platform to enter a Low Power S0 Idle (LPI) state. Here we are checking if
> > the we reached the minimum D-State defined in the constraint function of
> > the LPIT _DSM method before calling the sleep entry function. Also not
> > checking for constraint in acpi_s2idle_wake anymore and also changed the
> > acpi info loglevel to debug in lpi_check_constraint.
>
> This patch does three different things, two of which are questionable and
> one is done incorrectly.
>
> First off, aborting system suspend because S0ix constraints are not met is
> a non-starter.  The kernel really cannot refuse to suspend the system for
> that reason (and diagnostics can be done after resume anyway).
>
> Second, according to my knowledge, it is not a bug to invoke the
> ACPI_LPS0_ENTRY _DSM when the constraints are not met.  Do you
> actually know about any platforms where that may cause real problems
> to occur?
>
> Finally, it is too late to invoke that _DSM from acpi_s2idle_sync(), because
> that is called after leaving S0ix and resuming some devices.
>
> I can believe the claim that invoking the ACPI_LPS0_ENTRY _DSM in
> acpi_s2idle_prepare() may be too early and it may confuse the EC, say,
> but I'm not sure why the ACPI_LPS0_SCREEN_OFF _DSM would be
> affected by that too.
>
> So overall, the patch below may actually work,

On my Dell XPS13 9360 it clearly makes things worse by causing the EC
to generate spurious wakeup events all the time, so I'm afraid that
this like of reasoning is misguided overall.

> but not the $subject one
> (if evaluating the ACPI_LPS0_ENTRY _DSM when the constraints are not
> met is *really* problematic, it may be necessary to add the check
> for that on top of it).
>
> ---
>  drivers/acpi/sleep.c    |   13 +++++++++----
>  include/linux/suspend.h |    1 +
>  kernel/power/suspend.c  |    8 ++++++--
>  3 files changed, 16 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -967,8 +967,6 @@ static int acpi_s2idle_prepare(void)
>  {
>         if (lps0_device_handle) {
>                 acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
> -               acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
> -
>                 acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
>         }
>
> @@ -983,6 +981,12 @@ static int acpi_s2idle_prepare(void)
>         return 0;
>  }
>
> +static void acpi_s2idle_sleep(void)
> +{
> +       if (lps0_device_handle)
> +               acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
> +}
> +
>  static void acpi_s2idle_wake(void)
>  {
>         if (!lps0_device_handle)
> @@ -1007,6 +1011,8 @@ static void acpi_s2idle_wake(void)
>                  */
>                 acpi_ec_dispatch_gpe();
>         }
> +
> +       acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
>  }
>
>  static void acpi_s2idle_sync(void)
> @@ -1034,8 +1040,6 @@ static void acpi_s2idle_restore(void)
>
>         if (lps0_device_handle) {
>                 acpi_ec_set_gpe_wake_mask(ACPI_GPE_DISABLE);
> -
> -               acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
>                 acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
>         }
>  }
> @@ -1049,6 +1053,7 @@ static void acpi_s2idle_end(void)
>  static const struct platform_s2idle_ops acpi_s2idle_ops = {
>         .begin = acpi_s2idle_begin,
>         .prepare = acpi_s2idle_prepare,
> +       .sleep = acpi_s2idle_sleep,
>         .wake = acpi_s2idle_wake,
>         .sync = acpi_s2idle_sync,
>         .restore = acpi_s2idle_restore,
> Index: linux-pm/include/linux/suspend.h
> ===================================================================
> --- linux-pm.orig/include/linux/suspend.h
> +++ linux-pm/include/linux/suspend.h
> @@ -190,6 +190,7 @@ struct platform_suspend_ops {
>  struct platform_s2idle_ops {
>         int (*begin)(void);
>         int (*prepare)(void);
> +       void (*sleep)(void);
>         void (*wake)(void);
>         void (*sync)(void);
>         void (*restore)(void);
> Index: linux-pm/kernel/power/suspend.c
> ===================================================================
> --- linux-pm.orig/kernel/power/suspend.c
> +++ linux-pm/kernel/power/suspend.c
> @@ -136,10 +136,14 @@ static void s2idle_loop(void)
>                  * so prevent them from terminating the loop right away.
>                  */
>                 error = dpm_noirq_suspend_devices(PMSG_SUSPEND);
> -               if (!error)
> +               if (!error) {
> +                       if (s2idle_ops && s2idle_ops->sleep)
> +                               s2idle_ops->sleep();
> +
>                         s2idle_enter();
> -               else if (error == -EBUSY && pm_wakeup_pending())
> +               } else if (error == -EBUSY && pm_wakeup_pending()) {
>                         error = 0;
> +               }
>
>                 if (!error && s2idle_ops && s2idle_ops->wake)
>                         s2idle_ops->wake();
>
>
>

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-26 20:27 Shaunak Saha
2019-05-29 19:10 ` Mario.Limonciello
2019-05-31  6:21   ` Saha, Shaunak
2019-06-13 21:37 ` Rafael J. Wysocki
2019-06-18  8:44   ` Rafael J. Wysocki
2019-06-19  5:33     ` Saha, Shaunak [this message]
2019-06-19  8:29       ` Rafael J. Wysocki
2019-07-08 19:44         ` Saha, Shaunak

Reply instructions:

You may reply publically 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=593AE8A18AD46543B92D8EF146E2AA9408C326BA@ORSMSX107.amr.corp.intel.com \
    --to=shaunak.saha@intel.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=mario_limonciello@dell.com \
    --cc=rafael@kernel.org \
    /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

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git