> -----Original Message-----
> From: Shaunak Saha <shaunak.saha@xxxxxxxxx>
> Sent: Sunday, May 26, 2019 3:28 PM
> To: linux-acpi@xxxxxxxxxxxxxxx
> Cc: rafael.j.wysocki@xxxxxxxxx; Limonciello, Mario; Shaunak Saha
> Subject: drivers/acpi: Change the lpit function call flow
>
>
> [EXTERNAL EMAIL]
>
> 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.
>
> Signed-off-by: Shaunak Saha <shaunak.saha@xxxxxxxxx>
I did test this patch on an XPS 9380 and can confirm that it prevents the device from
going in/out of S2I twice in a row.
I was not able to test the config option related to constraints blocking entry because
an I2C device was preventing this on this system.
Tested-by: Mario Limonciello <mario.limonciello@xxxxxxxx>
> ---
> drivers/acpi/Kconfig | 13 +++++++++
> drivers/acpi/sleep.c | 76 +++++++++++++++++++++++++++++++++++++++++++--------
> -
> 2 files changed, 77 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 283ee94..57a9b2e 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -93,6 +93,19 @@ config ACPI_LPIT
> depends on X86_64
> default y
>
> +config LPIT_CONSTRAINTS_CHECK_FAILURE
> + bool
> + depends on X86_64
> + default n
> + help
> + For platforms defining the device constraints _DSM function in the
> + Low Power Idle table this option allows the platform to choose
> + if they wants to fail or succeed in calling the Low Power Entry
> + Notification _DSM function. If this config is defined by a
> + platform then the Low Power S0 Entry Notification _DSM function
> + is not called if the idle state has not achieved the Entry Trigger
> + defined in LPIT.
> +
> config ACPI_SLEEP
> bool
> depends on SUSPEND || HIBERNATION
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index e52f123..9f2359c 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -860,11 +860,25 @@ static void lpi_device_get_constraints(void)
>
> free_acpi_buffer:
> ACPI_FREE(out_obj);
> + /*
> + * lpi_constraints_table_size only increments if we have proper package
> + * definitions for the constraints, its done after parsing all the
> + * fields for constraints. Memory is allocated for lpi_constraints_table
> + * through kcalloc much before we start parsing the packages we need to
> + * free the memory here in case of any failures.
> + */
> + if (!lpi_constraints_table_size && lpi_constraints_table)
> + kfree(lpi_constraints_table);
> }
>
> -static void lpi_check_constraints(void)
> +static int lpi_check_constraints(void)
> {
> int i;
> + int lpi_error = 0;
> +
> + /* We do not have any table so return from here */
> + if (!lpi_constraints_table_size)
> + return lpi_error;
>
> for (i = 0; i < lpi_constraints_table_size; ++i) {
> acpi_handle handle = lpi_constraints_table[i].handle;
> @@ -879,17 +893,21 @@ static void lpi_check_constraints(void)
> acpi_power_state_string(adev->power.state));
>
> if (!adev->flags.power_manageable) {
> - acpi_handle_info(handle, "LPI: Device not power
> manageable\n");
> + acpi_handle_debug(handle, "LPI: Device not power
> manageable\n");
> lpi_constraints_table[i].handle = NULL;
> continue;
> }
>
> - if (adev->power.state < lpi_constraints_table[i].min_dstate)
> - acpi_handle_info(handle,
> + if (adev->power.state < lpi_constraints_table[i].min_dstate) {
> + acpi_handle_debug(handle,
> "LPI: Constraint not met; min power state:%s
> current power state:%s\n",
>
> acpi_power_state_string(lpi_constraints_table[i].min_dstate),
> acpi_power_state_string(adev->power.state));
> + /* Error is set here if any of the constraints fail */
> + lpi_error = 1;
> + }
> }
> + return lpi_error;
> }
>
> static void acpi_sleep_run_lps0_dsm(unsigned int func)
> @@ -967,12 +985,8 @@ static int acpi_s2idle_begin(void)
>
> 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);
> -
> + if (lps0_device_handle)
> acpi_ec_set_gpe_wake_mask(ACPI_GPE_ENABLE);
> - }
>
> if (acpi_sci_irq_valid())
> enable_irq_wake(acpi_sci_irq);
> @@ -990,9 +1004,6 @@ static void acpi_s2idle_wake(void)
> if (!lps0_device_handle)
> return;
>
> - if (pm_debug_messages_on)
> - lpi_check_constraints();
> -
> /*
> * If IRQD_WAKEUP_ARMED is not set for the SCI at this point, it means
> * that the SCI has triggered while suspended, so cancel the wakeup in
> @@ -1013,6 +1024,7 @@ static void acpi_s2idle_wake(void)
>
> static void acpi_s2idle_sync(void)
> {
> + int lpi_check;
> /*
> * Process all pending events in case there are any wakeup ones.
> *
> @@ -1023,6 +1035,46 @@ static void acpi_s2idle_sync(void)
> acpi_ec_flush_work();
> acpi_os_wait_events_complete(); /* synchronize Notify handling */
> s2idle_wakeup = false;
> +
> + if (!lps0_device_handle)
> + return;
> +
> + acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
> +
> + /*
> + * Check here if device constraints(Function 1) is defined.
> + * If only the sleep functions are defined in lpit we run that here.
> + */
> + if (!lpi_constraints_table_size)
> + goto lps0_entry;
> +
> + lpi_check = lpi_check_constraints();
> +
> + /*
> + * _DSM function may be invoked when the OS state
> + * has reached sufficient criteria for processor idle
> + * state.
> + */
> + if (lpi_check) {
> + /*
> + * We check the config LPIT_CONSTRAINTS_FAILURE
> + * here for the devices which defines the constraints
> + * properly and LPIT_CONSTRAINTS_FAILURE config in kernel.
> + * We fail here if constraints are not met. But we still run
> + * the sleep function for the devices which do not define
> + * the LPIT_CONSTRAINTS_FAILURE kernel config.
> + */
> + if (!IS_ENABLED(CONFIG_LPIT_CONSTRAINTS_CHECK_FAILURE))
> + goto lps0_entry;
> +
> + return;
> + }
> +
> +lps0_entry:
> + /*
> + * If only the sleep functions are defined in lpit we run that here.
> + */
> + acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
> }
>
> static void acpi_s2idle_restore(void)
