[2/2] ACPI / LPSS: Avoid PM quirks on suspend and resume from hibernation

Message ID 20180801081742.12590-3-kai.heng.feng@canonical.com
State New
Headers show
Series
  • Fix regression on acpi-lpss
Related show

Commit Message

Kai-Heng Feng Aug. 1, 2018, 8:17 a.m.
From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

BugLink: https://bugs.launchpad.net/bugs/1774950

Commit a09c59130688 (ACPI / LPSS: Avoid PM quirks on suspend and
resume from S3) modified the ACPI driver for Intel SoCs (LPSS) to
avoid applying PM quirks on suspend and resume from S3 to address
system-wide suspend and resume problems on some systems, but it is
reported that the same issue also affects hibernation, so extend
the approach used by that commit to cover hibernation as well.

Fixes: a09c59130688 (ACPI / LPSS: Avoid PM quirks on suspend and resume from S3)
Link: https://bugs.launchpad.net/bugs/1774950
Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: 4.15+ <stable@vger.kernel.org> # 4.15+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
(cherry picked from commit 12864ff8545f6b8144fdf1bb89b5663357f29ec4)
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/acpi/acpi_lpss.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

Comments

Colin King Aug. 1, 2018, 8:23 a.m. | #1
On 01/08/18 09:17, Kai-Heng Feng wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1774950
> 
> Commit a09c59130688 (ACPI / LPSS: Avoid PM quirks on suspend and
> resume from S3) modified the ACPI driver for Intel SoCs (LPSS) to
> avoid applying PM quirks on suspend and resume from S3 to address
> system-wide suspend and resume problems on some systems, but it is
> reported that the same issue also affects hibernation, so extend
> the approach used by that commit to cover hibernation as well.
> 
> Fixes: a09c59130688 (ACPI / LPSS: Avoid PM quirks on suspend and resume from S3)
> Link: https://bugs.launchpad.net/bugs/1774950
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Cc: 4.15+ <stable@vger.kernel.org> # 4.15+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> (cherry picked from commit 12864ff8545f6b8144fdf1bb89b5663357f29ec4)
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/acpi/acpi_lpss.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 35dd15fce2fa..8e37c2317e27 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -736,6 +736,7 @@ static void acpi_lpss_dismiss(struct device *dev)
>  #define LPSS_GPIODEF0_DMA_LLP		BIT(13)
>  
>  static DEFINE_MUTEX(lpss_iosf_mutex);
> +static bool lpss_iosf_d3_entered;
>  
>  static void lpss_iosf_enter_d3_state(void)
>  {
> @@ -778,6 +779,9 @@ static void lpss_iosf_enter_d3_state(void)
>  
>  	iosf_mbi_modify(LPSS_IOSF_UNIT_LPIOEP, MBI_CR_WRITE,
>  			LPSS_IOSF_GPIODEF0, value1, mask1);
> +
> +	lpss_iosf_d3_entered = true;
> +
>  exit:
>  	mutex_unlock(&lpss_iosf_mutex);
>  }
> @@ -792,6 +796,11 @@ static void lpss_iosf_exit_d3_state(void)
>  
>  	mutex_lock(&lpss_iosf_mutex);
>  
> +	if (!lpss_iosf_d3_entered)
> +		goto exit;
> +
> +	lpss_iosf_d3_entered = false;
> +
>  	iosf_mbi_modify(LPSS_IOSF_UNIT_LPIOEP, MBI_CR_WRITE,
>  			LPSS_IOSF_GPIODEF0, value1, mask1);
>  
> @@ -801,13 +810,13 @@ static void lpss_iosf_exit_d3_state(void)
>  	iosf_mbi_modify(LPSS_IOSF_UNIT_LPIO1, MBI_CFG_WRITE,
>  			LPSS_IOSF_PMCSR, value2, mask2);
>  
> +exit:
>  	mutex_unlock(&lpss_iosf_mutex);
>  }
>  
> -static int acpi_lpss_suspend(struct device *dev, bool runtime)
> +static int acpi_lpss_suspend(struct device *dev, bool wakeup)
>  {
>  	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> -	bool wakeup = runtime || device_may_wakeup(dev);
>  	int ret;
>  
>  	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
> @@ -820,14 +829,14 @@ static int acpi_lpss_suspend(struct device *dev, bool runtime)
>  	 * wrong status for devices being about to be powered off. See
>  	 * lpss_iosf_enter_d3_state() for further information.
>  	 */
> -	if ((runtime || !pm_suspend_via_firmware()) &&
> +	if (acpi_target_system_state() == ACPI_STATE_S0 &&
>  	    lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
>  		lpss_iosf_enter_d3_state();
>  
>  	return ret;
>  }
>  
> -static int acpi_lpss_resume(struct device *dev, bool runtime)
> +static int acpi_lpss_resume(struct device *dev)
>  {
>  	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
>  	int ret;
> @@ -836,8 +845,7 @@ static int acpi_lpss_resume(struct device *dev, bool runtime)
>  	 * This call is kept first to be in symmetry with
>  	 * acpi_lpss_runtime_suspend() one.
>  	 */
> -	if ((runtime || !pm_resume_via_firmware()) &&
> -	    lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
> +	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
>  		lpss_iosf_exit_d3_state();
>  
>  	ret = acpi_dev_resume(dev);
> @@ -861,12 +869,12 @@ static int acpi_lpss_suspend_late(struct device *dev)
>  		return 0;
>  
>  	ret = pm_generic_suspend_late(dev);
> -	return ret ? ret : acpi_lpss_suspend(dev, false);
> +	return ret ? ret : acpi_lpss_suspend(dev, device_may_wakeup(dev));
>  }
>  
>  static int acpi_lpss_resume_early(struct device *dev)
>  {
> -	int ret = acpi_lpss_resume(dev, false);
> +	int ret = acpi_lpss_resume(dev);
>  
>  	return ret ? ret : pm_generic_resume_early(dev);
>  }
> @@ -881,7 +889,7 @@ static int acpi_lpss_runtime_suspend(struct device *dev)
>  
>  static int acpi_lpss_runtime_resume(struct device *dev)
>  {
> -	int ret = acpi_lpss_resume(dev, true);
> +	int ret = acpi_lpss_resume(dev);
>  
>  	return ret ? ret : pm_generic_runtime_resume(dev);
>  }
> 

Positive test results, and clean upstream cherry pick,

Acked-by: Colin Ian King <colin.king@canonical.com>

Patch

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 35dd15fce2fa..8e37c2317e27 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -736,6 +736,7 @@  static void acpi_lpss_dismiss(struct device *dev)
 #define LPSS_GPIODEF0_DMA_LLP		BIT(13)
 
 static DEFINE_MUTEX(lpss_iosf_mutex);
+static bool lpss_iosf_d3_entered;
 
 static void lpss_iosf_enter_d3_state(void)
 {
@@ -778,6 +779,9 @@  static void lpss_iosf_enter_d3_state(void)
 
 	iosf_mbi_modify(LPSS_IOSF_UNIT_LPIOEP, MBI_CR_WRITE,
 			LPSS_IOSF_GPIODEF0, value1, mask1);
+
+	lpss_iosf_d3_entered = true;
+
 exit:
 	mutex_unlock(&lpss_iosf_mutex);
 }
@@ -792,6 +796,11 @@  static void lpss_iosf_exit_d3_state(void)
 
 	mutex_lock(&lpss_iosf_mutex);
 
+	if (!lpss_iosf_d3_entered)
+		goto exit;
+
+	lpss_iosf_d3_entered = false;
+
 	iosf_mbi_modify(LPSS_IOSF_UNIT_LPIOEP, MBI_CR_WRITE,
 			LPSS_IOSF_GPIODEF0, value1, mask1);
 
@@ -801,13 +810,13 @@  static void lpss_iosf_exit_d3_state(void)
 	iosf_mbi_modify(LPSS_IOSF_UNIT_LPIO1, MBI_CFG_WRITE,
 			LPSS_IOSF_PMCSR, value2, mask2);
 
+exit:
 	mutex_unlock(&lpss_iosf_mutex);
 }
 
-static int acpi_lpss_suspend(struct device *dev, bool runtime)
+static int acpi_lpss_suspend(struct device *dev, bool wakeup)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
-	bool wakeup = runtime || device_may_wakeup(dev);
 	int ret;
 
 	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
@@ -820,14 +829,14 @@  static int acpi_lpss_suspend(struct device *dev, bool runtime)
 	 * wrong status for devices being about to be powered off. See
 	 * lpss_iosf_enter_d3_state() for further information.
 	 */
-	if ((runtime || !pm_suspend_via_firmware()) &&
+	if (acpi_target_system_state() == ACPI_STATE_S0 &&
 	    lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
 		lpss_iosf_enter_d3_state();
 
 	return ret;
 }
 
-static int acpi_lpss_resume(struct device *dev, bool runtime)
+static int acpi_lpss_resume(struct device *dev)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
 	int ret;
@@ -836,8 +845,7 @@  static int acpi_lpss_resume(struct device *dev, bool runtime)
 	 * This call is kept first to be in symmetry with
 	 * acpi_lpss_runtime_suspend() one.
 	 */
-	if ((runtime || !pm_resume_via_firmware()) &&
-	    lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
+	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
 		lpss_iosf_exit_d3_state();
 
 	ret = acpi_dev_resume(dev);
@@ -861,12 +869,12 @@  static int acpi_lpss_suspend_late(struct device *dev)
 		return 0;
 
 	ret = pm_generic_suspend_late(dev);
-	return ret ? ret : acpi_lpss_suspend(dev, false);
+	return ret ? ret : acpi_lpss_suspend(dev, device_may_wakeup(dev));
 }
 
 static int acpi_lpss_resume_early(struct device *dev)
 {
-	int ret = acpi_lpss_resume(dev, false);
+	int ret = acpi_lpss_resume(dev);
 
 	return ret ? ret : pm_generic_resume_early(dev);
 }
@@ -881,7 +889,7 @@  static int acpi_lpss_runtime_suspend(struct device *dev)
 
 static int acpi_lpss_runtime_resume(struct device *dev)
 {
-	int ret = acpi_lpss_resume(dev, true);
+	int ret = acpi_lpss_resume(dev);
 
 	return ret ? ret : pm_generic_runtime_resume(dev);
 }