Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: "Saha, Shaunak" <shaunak.saha@intel.com>
To: "Mario.Limonciello@dell.com" <Mario.Limonciello@dell.com>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
Cc: "Wysocki, Rafael J" <rafael.j.wysocki@intel.com>
Subject: RE: drivers/acpi: Change the lpit function call flow
Date: Fri, 31 May 2019 06:21:18 +0000
Message-ID: <593AE8A18AD46543B92D8EF146E2AA9408C06360@ORSMSX107.amr.corp.intel.com> (raw)
In-Reply-To: <e194fa374cb64106b7a34fff616734c2@AUSX13MPC105.AMER.DELL.COM>

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)


  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 [this message]
2019-06-13 21:37 ` Rafael J. Wysocki
2019-06-18  8:44   ` Rafael J. Wysocki
2019-06-19  5:33     ` Saha, Shaunak
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=593AE8A18AD46543B92D8EF146E2AA9408C06360@ORSMSX107.amr.corp.intel.com \
    --to=shaunak.saha@intel.com \
    --cc=Mario.Limonciello@dell.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=rafael.j.wysocki@intel.com \
    /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 linux-acpi@archiver.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