drivers/acpi: Change the lpit function call flow

drivers/acpi: Change the lpit function call flow

[Shaunak Saha]

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@intel.com>
---
 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)

RE: drivers/acpi: Change the lpit function call flow

[Mario.Limonciello]

> -----Original Message-----
> From: Shaunak Saha <shaunak.saha@intel.com>
> Sent: Sunday, May 26, 2019 3:28 PM
> To: linux-acpi@vger.kernel.org
> Cc: rafael.j.wysocki@intel.com; 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@intel.com>

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@dell.com>
> ---
>  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)

RE: drivers/acpi: Change the lpit function call flow

[Saha, Shaunak]

Also in the parsing of the constraints functions i feel still there are some fixes need to be done. We need to check the entry trigger field in the LPIT table (if platform defines one) and check the min d state as per that, so that the check for calling the sleep entry function comply with the spec.

-----Original Message-----
From: Mario.Limonciello@dell.com [mailto:Mario.Limonciello@dell.com] 
Sent: Wednesday, May 29, 2019 12:11 PM
To: Saha, Shaunak <shaunak.saha@intel.com>; linux-acpi@vger.kernel.org
Cc: Wysocki, Rafael J <rafael.j.wysocki@intel.com>
Subject: RE: drivers/acpi: Change the lpit function call flow



> -----Original Message-----
> From: Shaunak Saha <shaunak.saha@intel.com>
> Sent: Sunday, May 26, 2019 3:28 PM
> To: linux-acpi@vger.kernel.org
> Cc: rafael.j.wysocki@intel.com; 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@intel.com>

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@dell.com>
> ---
>  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)

Re: drivers/acpi: Change the lpit function call flow

[Rafael J. Wysocki]

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, 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();

Re: drivers/acpi: Change the lpit function call flow

[Rafael J. Wysocki]

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();
>
>
>

RE: drivers/acpi: Change the lpit function call flow

[Saha, Shaunak]

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();
>
>
>

Re: drivers/acpi: Change the lpit function call flow

[Rafael J. Wysocki]

On Wed, Jun 19, 2019 at 7:33 AM Saha, Shaunak <shaunak.saha@intel.com> wrote:
>
> 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

As I have just tested on one of my systems, moving the invocation of
the ACPI_LPS0_ENTRY _DSM to a later stage causes problems to happen
(spurious wakeups generated by the EC), so this generally doesn't
work.

I thus don't see a reason to change the code in the area at hand.



> ________________________________________
> 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();
> >
> >
> >

RE: drivers/acpi: Change the lpit function call flow

[Saha, Shaunak]

Thanks a lot for your time to review and test this patch. Here are some explanation why we started to look into the LPIT call flow and the outcome.

Problem statement:
--------------------
We are working on a chrome book using wilco ec. For this ec we have the _DSM implemented in the asl code for the s0ix entry/exit. We are seeing that while going to suspend in the suspend path system resumes and then goes back to suspend again. In mosys eventlog which shows the smbios info we see 2 entries for suspend entry/exit. Also when sdcard is inserted s0ix fails. 


Debug:
-----------
Initially I suspected that executing the DSM calls too early in suspend path is causing this. But as you reviewed the patch and mentioned that early call cannot really casue the issue. On further debug i found that In the ec polling from the ec transaction we check the guarding period before polling ec status. In the guarding period ec does busy/wait polling. Before entering the noirq stage always wait polling is done and noirq is called by PM after the LPIT dsm is executed. In the scenerio where ec is noisy and we try
to execute the LPIT dsm call ec does wait polling and calls wait_event_timeout till transaction is completed which results in wake up events. 
Here in our scenerio We are seeing an EC interrupt after setting the EC RAM offset that indicates that the EC should transition to S0ix mode and this is preventing the kernel from going into S0ix on the first try. In the kernel code for advance_transaction adding some extra condition/debug i saw that it is doing some extra loop based on what it reads from the ec ram.

Solution:
----------
As a workaround if we read back from the EC RAM while still in the _DSM handler it seems to prevent this problem. 
Also i tried the below patch where i made ec to enter the busy polling stage for the entire LPIT dsm transaction.

 drivers/acpi/ec.c       | 6 ++++++
 drivers/acpi/internal.h | 1 +
 drivers/acpi/sleep.c    | 2 ++
 3 files changed, 9 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 49e16f0..cda1b58 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1070,6 +1070,12 @@ void acpi_ec_set_gpe_wake_mask(u8 action)
        if (first_ec && !ec_no_wakeup)
                acpi_set_gpe_wake_mask(NULL, first_ec->gpe, action);
 }
+void acpi_ec_set_busy_polling(bool polling_state)
+{
+       struct acpi_ec *ec = first_ec;
+       /* When set to true ec does not perform the wait polling */
+       ec->busy_polling = polling_state;
+}

 void acpi_ec_dispatch_gpe(void)
 {
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index f59d0b9..9aee878 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -190,6 +190,7 @@ void acpi_ec_block_transactions(void);
 void acpi_ec_unblock_transactions(void);
 void acpi_ec_mark_gpe_for_wake(void);
 void acpi_ec_set_gpe_wake_mask(u8 action);
+void acpi_ec_set_busy_polling(bool polling_state);
 void acpi_ec_dispatch_gpe(void);
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
                              acpi_handle handle, acpi_ec_query_func func,
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 6bd58d7..8599c33 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -903,11 +903,13 @@ static void acpi_sleep_run_lps0_dsm(unsigned int func)
        if (!(lps0_dsm_func_mask & (1 << func)))
                return;

+       acpi_ec_set_busy_polling(true);
        out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1, func, NULL);
        ACPI_FREE(out_obj);

        acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation %s\n",
                          func, out_obj ? "successful" : "failed");
+       acpi_ec_set_busy_polling(false);
 }

Please review and suggest if this kernel change can be an acceptable solution.

________________________________________
From: Rafael J. Wysocki [rafael@kernel.org]
Sent: Wednesday, June 19, 2019 1:29 AM
To: Saha, Shaunak
Cc: Rafael J. Wysocki; Mario Limonciello; ACPI Devel Maling List
Subject: Re: drivers/acpi: Change the lpit function call flow

On Wed, Jun 19, 2019 at 7:33 AM Saha, Shaunak <shaunak.saha@intel.com> wrote:
>
> 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

As I have just tested on one of my systems, moving the invocation of
the ACPI_LPS0_ENTRY _DSM to a later stage causes problems to happen
(spurious wakeups generated by the EC), so this generally doesn't
work.

I thus don't see a reason to change the code in the area at hand.



> ________________________________________
> 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();
> >
> >
> >