Possible regression caused by commit a192aa923b66a

Possible regression caused by commit a192aa923b66a

[Kai-Heng Feng]

Hi Rafael,

There's a regression report [1] that says commit a192aa923b66a ("ACPI /  
LPSS: Consolidate runtime PM and system sleep handling") is the first bad  
commit.

 From the looks of it, it didn't introduce any behavioral change. So your  
help is appreciated.

[1] https://bugs.launchpad.net/bugs/1774950

Kai-Heng

Re: Possible regression caused by commit a192aa923b66a

[Rafael J. Wysocki]

On Mon, Jun 11, 2018 at 8:26 AM, Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> Hi Rafael,
>
> There's a regression report [1] that says commit a192aa923b66a ("ACPI /
> LPSS: Consolidate runtime PM and system sleep handling") is the first bad
> commit.
>
> From the looks of it, it didn't introduce any behavioral change. So your
> help is appreciated.
>
> [1] https://bugs.launchpad.net/bugs/1774950

Well, the only difference is the iosf quirk AFAICS, but that should be
easy enough to check.  I'll try to cut a patch for that later today.

Re: Possible regression caused by commit a192aa923b66a

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]

On Mon, Jun 11, 2018 at 10:09 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, Jun 11, 2018 at 8:26 AM, Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>> Hi Rafael,
>>
>> There's a regression report [1] that says commit a192aa923b66a ("ACPI /
>> LPSS: Consolidate runtime PM and system sleep handling") is the first bad
>> commit.
>>
>> From the looks of it, it didn't introduce any behavioral change. So your
>> help is appreciated.
>>
>> [1] https://bugs.launchpad.net/bugs/1774950
>
> Well, the only difference is the iosf quirk AFAICS, but that should be
> easy enough to check.  I'll try to cut a patch for that later today.

If the iosf quirk is the source of the problem, the attached patch should help.

[-- Attachment #2: acpi-lpss-pm-debug.patch --]
[-- Type: text/x-patch, Size: 2389 bytes --]

---
 drivers/acpi/acpi_lpss.c |   17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/acpi/acpi_lpss.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_lpss.c
+++ linux-pm/drivers/acpi/acpi_lpss.c
@@ -940,9 +940,10 @@ static void lpss_iosf_exit_d3_state(void
 	mutex_unlock(&lpss_iosf_mutex);
 }
 
-static int acpi_lpss_suspend(struct device *dev, bool wakeup)
+static int acpi_lpss_suspend(struct device *dev, bool runtime)
 {
 	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)
@@ -955,13 +956,14 @@ static int acpi_lpss_suspend(struct devi
 	 * wrong status for devices being about to be powered off. See
 	 * lpss_iosf_enter_d3_state() for further information.
 	 */
-	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
+	if (runtime && 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)
+static int acpi_lpss_resume(struct device *dev, bool runtime)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
 	int ret;
@@ -970,7 +972,8 @@ static int acpi_lpss_resume(struct devic
 	 * This call is kept first to be in symmetry with
 	 * acpi_lpss_runtime_suspend() one.
 	 */
-	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
+	if (runtime && lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON &&
+	    iosf_mbi_available())
 		lpss_iosf_exit_d3_state();
 
 	ret = acpi_dev_resume(dev);
@@ -994,12 +997,12 @@ static int acpi_lpss_suspend_late(struct
 		return 0;
 
 	ret = pm_generic_suspend_late(dev);
-	return ret ? ret : acpi_lpss_suspend(dev, device_may_wakeup(dev));
+	return ret ? ret : acpi_lpss_suspend(dev, false);
 }
 
 static int acpi_lpss_resume_early(struct device *dev)
 {
-	int ret = acpi_lpss_resume(dev);
+	int ret = acpi_lpss_resume(dev, false);
 
 	return ret ? ret : pm_generic_resume_early(dev);
 }
@@ -1014,7 +1017,7 @@ static int acpi_lpss_runtime_suspend(str
 
 static int acpi_lpss_runtime_resume(struct device *dev)
 {
-	int ret = acpi_lpss_resume(dev);
+	int ret = acpi_lpss_resume(dev, true);
 
 	return ret ? ret : pm_generic_runtime_resume(dev);
 }

Re: Possible regression caused by commit a192aa923b66a

[Rafael J. Wysocki]

On Monday, June 11, 2018 11:52:34 PM CEST Rafael J. Wysocki wrote:
> 
> --000000000000703623056e64c488
> Content-Type: text/plain; charset="UTF-8"
> 
> On Mon, Jun 11, 2018 at 10:09 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Mon, Jun 11, 2018 at 8:26 AM, Kai-Heng Feng
> > <kai.heng.feng@canonical.com> wrote:
> >> Hi Rafael,
> >>
> >> There's a regression report [1] that says commit a192aa923b66a ("ACPI /
> >> LPSS: Consolidate runtime PM and system sleep handling") is the first bad
> >> commit.
> >>
> >> From the looks of it, it didn't introduce any behavioral change. So your
> >> help is appreciated.
> >>
> >> [1] https://bugs.launchpad.net/bugs/1774950
> >
> > Well, the only difference is the iosf quirk AFAICS, but that should be
> > easy enough to check.  I'll try to cut a patch for that later today.
> 
> If the iosf quirk is the source of the problem, the attached patch should help.

The one below should be slightly better, please test this one.

---
 drivers/acpi/acpi_lpss.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/acpi/acpi_lpss.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_lpss.c
+++ linux-pm/drivers/acpi/acpi_lpss.c
@@ -22,6 +22,7 @@
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
+#include <linux/suspend.h>
 #include <linux/delay.h>
 
 #include "internal.h"
@@ -940,9 +941,10 @@ static void lpss_iosf_exit_d3_state(void
 	mutex_unlock(&lpss_iosf_mutex);
 }
 
-static int acpi_lpss_suspend(struct device *dev, bool wakeup)
+static int acpi_lpss_suspend(struct device *dev, bool runtime)
 {
 	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)
@@ -955,13 +957,14 @@ static int acpi_lpss_suspend(struct devi
 	 * wrong status for devices being about to be powered off. See
 	 * lpss_iosf_enter_d3_state() for further information.
 	 */
-	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
+	if ((runtime || !pm_suspend_via_firmware()) &&
+	    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)
+static int acpi_lpss_resume(struct device *dev, bool runtime)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
 	int ret;
@@ -970,7 +973,8 @@ static int acpi_lpss_resume(struct devic
 	 * This call is kept first to be in symmetry with
 	 * acpi_lpss_runtime_suspend() one.
 	 */
-	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
+	if ((runtime || !pm_resume_via_firmware()) &&
+	    lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
 		lpss_iosf_exit_d3_state();
 
 	ret = acpi_dev_resume(dev);
@@ -994,12 +998,12 @@ static int acpi_lpss_suspend_late(struct
 		return 0;
 
 	ret = pm_generic_suspend_late(dev);
-	return ret ? ret : acpi_lpss_suspend(dev, device_may_wakeup(dev));
+	return ret ? ret : acpi_lpss_suspend(dev, false);
 }
 
 static int acpi_lpss_resume_early(struct device *dev)
 {
-	int ret = acpi_lpss_resume(dev);
+	int ret = acpi_lpss_resume(dev, false);
 
 	return ret ? ret : pm_generic_resume_early(dev);
 }
@@ -1014,7 +1018,7 @@ static int acpi_lpss_runtime_suspend(str
 
 static int acpi_lpss_runtime_resume(struct device *dev)
 {
-	int ret = acpi_lpss_resume(dev);
+	int ret = acpi_lpss_resume(dev, true);
 
 	return ret ? ret : pm_generic_runtime_resume(dev);
 }

Re: Possible regression caused by commit a192aa923b66a

[Kai Heng Feng]

Hi Rafael,

> On Jun 12, 2018, at 5:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Monday, June 11, 2018 11:52:34 PM CEST Rafael J. Wysocki wrote:
>> --000000000000703623056e64c488
>> Content-Type: text/plain; charset="UTF-8"
>>
>> On Mon, Jun 11, 2018 at 10:09 AM, Rafael J. Wysocki <rafael@kernel.org>  
>> wrote:
>>> On Mon, Jun 11, 2018 at 8:26 AM, Kai-Heng Feng
>>> <kai.heng.feng@canonical.com> wrote:
>>>> Hi Rafael,
>>>>
>>>> There's a regression report [1] that says commit a192aa923b66a ("ACPI /
>>>> LPSS: Consolidate runtime PM and system sleep handling") is the first  
>>>> bad
>>>> commit.
>>>>
>>>> From the looks of it, it didn't introduce any behavioral change. So your
>>>> help is appreciated.
>>>>
>>>> [1] https://bugs.launchpad.net/bugs/1774950
>>>
>>> Well, the only difference is the iosf quirk AFAICS, but that should be
>>> easy enough to check.  I'll try to cut a patch for that later today.
>>
>> If the iosf quirk is the source of the problem, the attached patch  
>> should help.
>
> The one below should be slightly better, please test this one.

Affected users reported that this patch solves the issue for them.

Kai-Heng

>
> ---
>  drivers/acpi/acpi_lpss.c |   18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> Index: linux-pm/drivers/acpi/acpi_lpss.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpi_lpss.c
> +++ linux-pm/drivers/acpi/acpi_lpss.c
> @@ -22,6 +22,7 @@
>  #include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pwm.h>
> +#include <linux/suspend.h>
>  #include <linux/delay.h>
>
>  #include "internal.h"
> @@ -940,9 +941,10 @@ static void lpss_iosf_exit_d3_state(void
>  	mutex_unlock(&lpss_iosf_mutex);
>  }
>
> -static int acpi_lpss_suspend(struct device *dev, bool wakeup)
> +static int acpi_lpss_suspend(struct device *dev, bool runtime)
>  {
>  	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)
> @@ -955,13 +957,14 @@ static int acpi_lpss_suspend(struct devi
>  	 * wrong status for devices being about to be powered off. See
>  	 * lpss_iosf_enter_d3_state() for further information.
>  	 */
> -	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
> +	if ((runtime || !pm_suspend_via_firmware()) &&
> +	    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)
> +static int acpi_lpss_resume(struct device *dev, bool runtime)
>  {
>  	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
>  	int ret;
> @@ -970,7 +973,8 @@ static int acpi_lpss_resume(struct devic
>  	 * This call is kept first to be in symmetry with
>  	 * acpi_lpss_runtime_suspend() one.
>  	 */
> -	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
> +	if ((runtime || !pm_resume_via_firmware()) &&
> +	    lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
>  		lpss_iosf_exit_d3_state();
>
>  	ret = acpi_dev_resume(dev);
> @@ -994,12 +998,12 @@ static int acpi_lpss_suspend_late(struct
>  		return 0;
>
>  	ret = pm_generic_suspend_late(dev);
> -	return ret ? ret : acpi_lpss_suspend(dev, device_may_wakeup(dev));
> +	return ret ? ret : acpi_lpss_suspend(dev, false);
>  }
>
>  static int acpi_lpss_resume_early(struct device *dev)
>  {
> -	int ret = acpi_lpss_resume(dev);
> +	int ret = acpi_lpss_resume(dev, false);
>
>  	return ret ? ret : pm_generic_resume_early(dev);
>  }
> @@ -1014,7 +1018,7 @@ static int acpi_lpss_runtime_suspend(str
>
>  static int acpi_lpss_runtime_resume(struct device *dev)
>  {
> -	int ret = acpi_lpss_resume(dev);
> +	int ret = acpi_lpss_resume(dev, true);
>
>  	return ret ? ret : pm_generic_runtime_resume(dev);
>  }

Re: Possible regression caused by commit a192aa923b66a

[Rafael J. Wysocki]

On Wed, Jun 13, 2018 at 4:45 AM, Kai Heng Feng
<kai.heng.feng@canonical.com> wrote:
> Hi Rafael,
>
>> On Jun 12, 2018, at 5:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> On Monday, June 11, 2018 11:52:34 PM CEST Rafael J. Wysocki wrote:
>>>
>>> --000000000000703623056e64c488
>>> Content-Type: text/plain; charset="UTF-8"
>>>
>>> On Mon, Jun 11, 2018 at 10:09 AM, Rafael J. Wysocki <rafael@kernel.org>
>>> wrote:
>>>>
>>>> On Mon, Jun 11, 2018 at 8:26 AM, Kai-Heng Feng
>>>> <kai.heng.feng@canonical.com> wrote:
>>>>>
>>>>> Hi Rafael,
>>>>>
>>>>> There's a regression report [1] that says commit a192aa923b66a ("ACPI /
>>>>> LPSS: Consolidate runtime PM and system sleep handling") is the first
>>>>> bad
>>>>> commit.
>>>>>
>>>>> From the looks of it, it didn't introduce any behavioral change. So
>>>>> your
>>>>> help is appreciated.
>>>>>
>>>>> [1] https://bugs.launchpad.net/bugs/1774950
>>>>
>>>>
>>>> Well, the only difference is the iosf quirk AFAICS, but that should be
>>>> easy enough to check.  I'll try to cut a patch for that later today.
>>>
>>>
>>> If the iosf quirk is the source of the problem, the attached patch should
>>> help.
>>
>>
>> The one below should be slightly better, please test this one.
>
>
> Affected users reported that this patch solves the issue for them.

OK, thanks for verifying.  Let me add a changelog to it and resend it, then.

Thanks,
Rafael

[PATCH] ACPI / LPSS: Avoid PM quirks on suspend and resume from S3

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It is reported that commit a192aa923b66a (ACPI / LPSS: Consolidate
runtime PM and system sleep handling) introduced a system suspend
regression on some machines, but the only functional change made by
it was to cause the PM quirks in the LPSS to also be used during
system suspend and resume.  While that should always work for
suspend-to-idle, it turns out to be problematic for S3
(suspend-to-RAM).

To address that issue restore the previous S3 suspend and resume
behavior of the LPSS to avoid applying PM quirks then.

Fixes: a192aa923b66a (ACPI / LPSS: Consolidate runtime PM and system sleep handling)
Link: https://bugs.launchpad.net/bugs/1774950
Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/acpi_lpss.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/acpi/acpi_lpss.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_lpss.c
+++ linux-pm/drivers/acpi/acpi_lpss.c
@@ -22,6 +22,7 @@
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
+#include <linux/suspend.h>
 #include <linux/delay.h>
 
 #include "internal.h"
@@ -940,9 +941,10 @@ static void lpss_iosf_exit_d3_state(void
 	mutex_unlock(&lpss_iosf_mutex);
 }
 
-static int acpi_lpss_suspend(struct device *dev, bool wakeup)
+static int acpi_lpss_suspend(struct device *dev, bool runtime)
 {
 	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)
@@ -955,13 +957,14 @@ static int acpi_lpss_suspend(struct devi
 	 * wrong status for devices being about to be powered off. See
 	 * lpss_iosf_enter_d3_state() for further information.
 	 */
-	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
+	if ((runtime || !pm_suspend_via_firmware()) &&
+	    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)
+static int acpi_lpss_resume(struct device *dev, bool runtime)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
 	int ret;
@@ -970,7 +973,8 @@ static int acpi_lpss_resume(struct devic
 	 * This call is kept first to be in symmetry with
 	 * acpi_lpss_runtime_suspend() one.
 	 */
-	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
+	if ((runtime || !pm_resume_via_firmware()) &&
+	    lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
 		lpss_iosf_exit_d3_state();
 
 	ret = acpi_dev_resume(dev);
@@ -994,12 +998,12 @@ static int acpi_lpss_suspend_late(struct
 		return 0;
 
 	ret = pm_generic_suspend_late(dev);
-	return ret ? ret : acpi_lpss_suspend(dev, device_may_wakeup(dev));
+	return ret ? ret : acpi_lpss_suspend(dev, false);
 }
 
 static int acpi_lpss_resume_early(struct device *dev)
 {
-	int ret = acpi_lpss_resume(dev);
+	int ret = acpi_lpss_resume(dev, false);
 
 	return ret ? ret : pm_generic_resume_early(dev);
 }
@@ -1014,7 +1018,7 @@ static int acpi_lpss_runtime_suspend(str
 
 static int acpi_lpss_runtime_resume(struct device *dev)
 {
-	int ret = acpi_lpss_resume(dev);
+	int ret = acpi_lpss_resume(dev, true);
 
 	return ret ? ret : pm_generic_runtime_resume(dev);
 }

Re: [PATCH] ACPI / LPSS: Avoid PM quirks on suspend and resume from S3

[Ulf Hansson]

On 13 June 2018 at 13:17, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> It is reported that commit a192aa923b66a (ACPI / LPSS: Consolidate
> runtime PM and system sleep handling) introduced a system suspend
> regression on some machines, but the only functional change made by
> it was to cause the PM quirks in the LPSS to also be used during
> system suspend and resume.  While that should always work for
> suspend-to-idle, it turns out to be problematic for S3
> (suspend-to-RAM).
>
> To address that issue restore the previous S3 suspend and resume
> behavior of the LPSS to avoid applying PM quirks then.
>
> Fixes: a192aa923b66a (ACPI / LPSS: Consolidate runtime PM and system sleep handling)
> Link: https://bugs.launchpad.net/bugs/1774950
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/acpi/acpi_lpss.c |   18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> Index: linux-pm/drivers/acpi/acpi_lpss.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpi_lpss.c
> +++ linux-pm/drivers/acpi/acpi_lpss.c
> @@ -22,6 +22,7 @@
>  #include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pwm.h>
> +#include <linux/suspend.h>
>  #include <linux/delay.h>
>
>  #include "internal.h"
> @@ -940,9 +941,10 @@ static void lpss_iosf_exit_d3_state(void
>         mutex_unlock(&lpss_iosf_mutex);
>  }
>
> -static int acpi_lpss_suspend(struct device *dev, bool wakeup)
> +static int acpi_lpss_suspend(struct device *dev, bool runtime)
>  {
>         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)
> @@ -955,13 +957,14 @@ static int acpi_lpss_suspend(struct devi
>          * wrong status for devices being about to be powered off. See
>          * lpss_iosf_enter_d3_state() for further information.
>          */
> -       if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
> +       if ((runtime || !pm_suspend_via_firmware()) &&
> +           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)
> +static int acpi_lpss_resume(struct device *dev, bool runtime)
>  {
>         struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
>         int ret;
> @@ -970,7 +973,8 @@ static int acpi_lpss_resume(struct devic
>          * This call is kept first to be in symmetry with
>          * acpi_lpss_runtime_suspend() one.
>          */
> -       if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
> +       if ((runtime || !pm_resume_via_firmware()) &&
> +           lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
>                 lpss_iosf_exit_d3_state();
>
>         ret = acpi_dev_resume(dev);
> @@ -994,12 +998,12 @@ static int acpi_lpss_suspend_late(struct
>                 return 0;
>
>         ret = pm_generic_suspend_late(dev);
> -       return ret ? ret : acpi_lpss_suspend(dev, device_may_wakeup(dev));
> +       return ret ? ret : acpi_lpss_suspend(dev, false);
>  }
>
>  static int acpi_lpss_resume_early(struct device *dev)
>  {
> -       int ret = acpi_lpss_resume(dev);
> +       int ret = acpi_lpss_resume(dev, false);
>
>         return ret ? ret : pm_generic_resume_early(dev);
>  }
> @@ -1014,7 +1018,7 @@ static int acpi_lpss_runtime_suspend(str
>
>  static int acpi_lpss_runtime_resume(struct device *dev)
>  {
> -       int ret = acpi_lpss_resume(dev);
> +       int ret = acpi_lpss_resume(dev, true);
>
>         return ret ? ret : pm_generic_runtime_resume(dev);
>  }
>

Re: [PATCH] ACPI / LPSS: Avoid PM quirks on suspend and resume from S3

[Andy Shevchenko]

On Wed, 2018-06-13 at 13:17 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It is reported that commit a192aa923b66a (ACPI / LPSS: Consolidate
> runtime PM and system sleep handling) introduced a system suspend
> regression on some machines, but the only functional change made by
> it was to cause the PM quirks in the LPSS to also be used during
> system suspend and resume.  While that should always work for
> suspend-to-idle, it turns out to be problematic for S3
> (suspend-to-RAM).
> 
> To address that issue restore the previous S3 suspend and resume
> behavior of the LPSS to avoid applying PM quirks then.
> 

LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: a192aa923b66a (ACPI / LPSS: Consolidate runtime PM and system
> sleep handling)
> Link: https://bugs.launchpad.net/bugs/1774950
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/acpi_lpss.c |   18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> Index: linux-pm/drivers/acpi/acpi_lpss.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpi_lpss.c
> +++ linux-pm/drivers/acpi/acpi_lpss.c
> @@ -22,6 +22,7 @@
>  #include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pwm.h>
> +#include <linux/suspend.h>
>  #include <linux/delay.h>
>  
>  #include "internal.h"
> @@ -940,9 +941,10 @@ static void lpss_iosf_exit_d3_state(void
>  	mutex_unlock(&lpss_iosf_mutex);
>  }
>  
> -static int acpi_lpss_suspend(struct device *dev, bool wakeup)
> +static int acpi_lpss_suspend(struct device *dev, bool runtime)
>  {
>  	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)
> @@ -955,13 +957,14 @@ static int acpi_lpss_suspend(struct devi
>  	 * wrong status for devices being about to be powered off.
> See
>  	 * lpss_iosf_enter_d3_state() for further information.
>  	 */
> -	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON &&
> iosf_mbi_available())
> +	if ((runtime || !pm_suspend_via_firmware()) &&
> +	    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)
> +static int acpi_lpss_resume(struct device *dev, bool runtime)
>  {
>  	struct lpss_private_data *pdata =
> acpi_driver_data(ACPI_COMPANION(dev));
>  	int ret;
> @@ -970,7 +973,8 @@ static int acpi_lpss_resume(struct devic
>  	 * This call is kept first to be in symmetry with
>  	 * acpi_lpss_runtime_suspend() one.
>  	 */
> -	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON &&
> iosf_mbi_available())
> +	if ((runtime || !pm_resume_via_firmware()) &&
> +	    lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON &&
> iosf_mbi_available())
>  		lpss_iosf_exit_d3_state();
>  
>  	ret = acpi_dev_resume(dev);
> @@ -994,12 +998,12 @@ static int acpi_lpss_suspend_late(struct
>  		return 0;
>  
>  	ret = pm_generic_suspend_late(dev);
> -	return ret ? ret : acpi_lpss_suspend(dev,
> device_may_wakeup(dev));
> +	return ret ? ret : acpi_lpss_suspend(dev, false);
>  }
>  
>  static int acpi_lpss_resume_early(struct device *dev)
>  {
> -	int ret = acpi_lpss_resume(dev);
> +	int ret = acpi_lpss_resume(dev, false);
>  
>  	return ret ? ret : pm_generic_resume_early(dev);
>  }
> @@ -1014,7 +1018,7 @@ static int acpi_lpss_runtime_suspend(str
>  
>  static int acpi_lpss_runtime_resume(struct device *dev)
>  {
> -	int ret = acpi_lpss_resume(dev);
> +	int ret = acpi_lpss_resume(dev, true);
>  
>  	return ret ? ret : pm_generic_runtime_resume(dev);
>  }
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] ACPI / LPSS: Avoid PM quirks on suspend and resume from S3

[Mika Westerberg]

On Wed, Jun 13, 2018 at 03:53:07PM +0300, Andy Shevchenko wrote:
> On Wed, 2018-06-13 at 13:17 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > It is reported that commit a192aa923b66a (ACPI / LPSS: Consolidate
> > runtime PM and system sleep handling) introduced a system suspend
> > regression on some machines, but the only functional change made by
> > it was to cause the PM quirks in the LPSS to also be used during
> > system suspend and resume.  While that should always work for
> > suspend-to-idle, it turns out to be problematic for S3
> > (suspend-to-RAM).
> > 
> > To address that issue restore the previous S3 suspend and resume
> > behavior of the LPSS to avoid applying PM quirks then.
> > 
> 
> LGTM,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Also,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH] ACPI / LPSS: Avoid PM quirks on suspend and resume from S3

[Kai Heng Feng]

Hi Rafael,

> On Jun 13, 2018, at 7:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> It is reported that commit a192aa923b66a (ACPI / LPSS: Consolidate
> runtime PM and system sleep handling) introduced a system suspend
> regression on some machines, but the only functional change made by
> it was to cause the PM quirks in the LPSS to also be used during
> system suspend and resume.  While that should always work for
> suspend-to-idle, it turns out to be problematic for S3
> (suspend-to-RAM).
>
> To address that issue restore the previous S3 suspend and resume
> behavior of the LPSS to avoid applying PM quirks then.

The users reported that this patch does fix the S3 issue, but the S4 still  
fails.

Please refer to [1] for more for user's testing result.

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1774950/comments/60

Kai-Heng

>
> Fixes: a192aa923b66a (ACPI / LPSS: Consolidate runtime PM and system  
> sleep handling)
> Link: https://bugs.launchpad.net/bugs/1774950
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/acpi_lpss.c |   18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> Index: linux-pm/drivers/acpi/acpi_lpss.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpi_lpss.c
> +++ linux-pm/drivers/acpi/acpi_lpss.c
> @@ -22,6 +22,7 @@
>  #include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pwm.h>
> +#include <linux/suspend.h>
>  #include <linux/delay.h>
>
>  #include "internal.h"
> @@ -940,9 +941,10 @@ static void lpss_iosf_exit_d3_state(void
>  	mutex_unlock(&lpss_iosf_mutex);
>  }
>
> -static int acpi_lpss_suspend(struct device *dev, bool wakeup)
> +static int acpi_lpss_suspend(struct device *dev, bool runtime)
>  {
>  	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)
> @@ -955,13 +957,14 @@ static int acpi_lpss_suspend(struct devi
>  	 * wrong status for devices being about to be powered off. See
>  	 * lpss_iosf_enter_d3_state() for further information.
>  	 */
> -	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
> +	if ((runtime || !pm_suspend_via_firmware()) &&
> +	    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)
> +static int acpi_lpss_resume(struct device *dev, bool runtime)
>  {
>  	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
>  	int ret;
> @@ -970,7 +973,8 @@ static int acpi_lpss_resume(struct devic
>  	 * This call is kept first to be in symmetry with
>  	 * acpi_lpss_runtime_suspend() one.
>  	 */
> -	if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
> +	if ((runtime || !pm_resume_via_firmware()) &&
> +	    lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
>  		lpss_iosf_exit_d3_state();
>
>  	ret = acpi_dev_resume(dev);
> @@ -994,12 +998,12 @@ static int acpi_lpss_suspend_late(struct
>  		return 0;
>
>  	ret = pm_generic_suspend_late(dev);
> -	return ret ? ret : acpi_lpss_suspend(dev, device_may_wakeup(dev));
> +	return ret ? ret : acpi_lpss_suspend(dev, false);
>  }
>
>  static int acpi_lpss_resume_early(struct device *dev)
>  {
> -	int ret = acpi_lpss_resume(dev);
> +	int ret = acpi_lpss_resume(dev, false);
>
>  	return ret ? ret : pm_generic_resume_early(dev);
>  }
> @@ -1014,7 +1018,7 @@ static int acpi_lpss_runtime_suspend(str
>
>  static int acpi_lpss_runtime_resume(struct device *dev)
>  {
> -	int ret = acpi_lpss_resume(dev);
> +	int ret = acpi_lpss_resume(dev, true);
>
>  	return ret ? ret : pm_generic_runtime_resume(dev);
>  }

Re: [PATCH] ACPI / LPSS: Avoid PM quirks on suspend and resume from S3

[Rafael J. Wysocki]

On Tuesday, July 24, 2018 10:46:09 AM CEST Kai Heng Feng wrote:
> Hi Rafael,
> 
> > On Jun 13, 2018, at 7:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > It is reported that commit a192aa923b66a (ACPI / LPSS: Consolidate
> > runtime PM and system sleep handling) introduced a system suspend
> > regression on some machines, but the only functional change made by
> > it was to cause the PM quirks in the LPSS to also be used during
> > system suspend and resume.  While that should always work for
> > suspend-to-idle, it turns out to be problematic for S3
> > (suspend-to-RAM).
> >
> > To address that issue restore the previous S3 suspend and resume
> > behavior of the LPSS to avoid applying PM quirks then.
> 
> The users reported that this patch does fix the S3 issue, but the S4 still  
> fails.
> 
> Please refer to [1] for more for user's testing result.
> 
> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1774950/comments/60

Please ask the users to test the patch below, then, on top of the $subject one.

---
 drivers/acpi/acpi_lpss.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/acpi/acpi_lpss.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_lpss.c
+++ linux-pm/drivers/acpi/acpi_lpss.c
@@ -879,6 +879,7 @@ static void acpi_lpss_dismiss(struct 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)
 {
@@ -921,6 +922,9 @@ static void lpss_iosf_enter_d3_state(voi
 
 	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);
 }
@@ -935,6 +939,9 @@ static void lpss_iosf_exit_d3_state(void
 
 	mutex_lock(&lpss_iosf_mutex);
 
+	if (!lpss_iosf_d3_entered)
+		goto exit;
+
 	iosf_mbi_modify(LPSS_IOSF_UNIT_LPIOEP, MBI_CR_WRITE,
 			LPSS_IOSF_GPIODEF0, value1, mask1);
 
@@ -944,13 +951,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)
@@ -963,14 +970,14 @@ static int acpi_lpss_suspend(struct devi
 	 * 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;
@@ -979,8 +986,7 @@ static int acpi_lpss_resume(struct devic
 	 * 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);
@@ -1004,12 +1010,12 @@ static int acpi_lpss_suspend_late(struct
 		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);
 }
@@ -1024,7 +1030,7 @@ static int acpi_lpss_runtime_suspend(str
 
 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);
 }

Re: [PATCH] ACPI / LPSS: Avoid PM quirks on suspend and resume from S3

[Rafael J. Wysocki]

On Tuesday, July 24, 2018 11:13:42 AM CEST Rafael J. Wysocki wrote:
> On Tuesday, July 24, 2018 10:46:09 AM CEST Kai Heng Feng wrote:
> > Hi Rafael,
> > 
> > > On Jun 13, 2018, at 7:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > It is reported that commit a192aa923b66a (ACPI / LPSS: Consolidate
> > > runtime PM and system sleep handling) introduced a system suspend
> > > regression on some machines, but the only functional change made by
> > > it was to cause the PM quirks in the LPSS to also be used during
> > > system suspend and resume.  While that should always work for
> > > suspend-to-idle, it turns out to be problematic for S3
> > > (suspend-to-RAM).
> > >
> > > To address that issue restore the previous S3 suspend and resume
> > > behavior of the LPSS to avoid applying PM quirks then.
> > 
> > The users reported that this patch does fix the S3 issue, but the S4 still  
> > fails.
> > 
> > Please refer to [1] for more for user's testing result.
> > 
> > [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1774950/comments/60
> 
> Please ask the users to test the patch below, then, on top of the $subject one.

There was a mistake in the previous patch I posted, sorry about that.

Please test this one instead:

---
 drivers/acpi/acpi_lpss.c |   26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/acpi/acpi_lpss.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_lpss.c
+++ linux-pm/drivers/acpi/acpi_lpss.c
@@ -879,6 +879,7 @@ static void acpi_lpss_dismiss(struct 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)
 {
@@ -921,6 +922,9 @@ static void lpss_iosf_enter_d3_state(voi
 
 	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);
 }
@@ -935,6 +939,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);
 
@@ -944,13 +953,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)
@@ -963,14 +972,14 @@ static int acpi_lpss_suspend(struct devi
 	 * 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;
@@ -979,8 +988,7 @@ static int acpi_lpss_resume(struct devic
 	 * 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);
@@ -1004,12 +1012,12 @@ static int acpi_lpss_suspend_late(struct
 		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);
 }
@@ -1024,7 +1032,7 @@ static int acpi_lpss_runtime_suspend(str
 
 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);
 }

Re: [PATCH] ACPI / LPSS: Avoid PM quirks on suspend and resume from S3

[Kai-Heng Feng]

Hi Rafael,

> On 2018Jul24, at 18:36, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> 
> On Tuesday, July 24, 2018 11:13:42 AM CEST Rafael J. Wysocki wrote:
>> On Tuesday, July 24, 2018 10:46:09 AM CEST Kai Heng Feng wrote:
>>> Hi Rafael,
>>> 
>>>> On Jun 13, 2018, at 7:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> 
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> 
>>>> It is reported that commit a192aa923b66a (ACPI / LPSS: Consolidate
>>>> runtime PM and system sleep handling) introduced a system suspend
>>>> regression on some machines, but the only functional change made by
>>>> it was to cause the PM quirks in the LPSS to also be used during
>>>> system suspend and resume.  While that should always work for
>>>> suspend-to-idle, it turns out to be problematic for S3
>>>> (suspend-to-RAM).
>>>> 
>>>> To address that issue restore the previous S3 suspend and resume
>>>> behavior of the LPSS to avoid applying PM quirks then.
>>> 
>>> The users reported that this patch does fix the S3 issue, but the S4 still  
>>> fails.
>>> 
>>> Please refer to [1] for more for user's testing result.
>>> 
>>> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1774950/comments/60
>> 
>> Please ask the users to test the patch below, then, on top of the $subject one.
> 
> There was a mistake in the previous patch I posted, sorry about that.
> 
> Please test this one instead:

The result is positive, thanks!

Kai-Heng

> 
> ---
> drivers/acpi/acpi_lpss.c |   26 +++++++++++++++++---------
> 1 file changed, 17 insertions(+), 9 deletions(-)
> 
> Index: linux-pm/drivers/acpi/acpi_lpss.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpi_lpss.c
> +++ linux-pm/drivers/acpi/acpi_lpss.c
> @@ -879,6 +879,7 @@ static void acpi_lpss_dismiss(struct 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)
> {
> @@ -921,6 +922,9 @@ static void lpss_iosf_enter_d3_state(voi
> 
> 	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);
> }
> @@ -935,6 +939,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);
> 
> @@ -944,13 +953,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)
> @@ -963,14 +972,14 @@ static int acpi_lpss_suspend(struct devi
> 	 * 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;
> @@ -979,8 +988,7 @@ static int acpi_lpss_resume(struct devic
> 	 * 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);
> @@ -1004,12 +1012,12 @@ static int acpi_lpss_suspend_late(struct
> 		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);
> }
> @@ -1024,7 +1032,7 @@ static int acpi_lpss_runtime_suspend(str
> 
> 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);
> }

Re: [PATCH] ACPI / LPSS: Avoid PM quirks on suspend and resume from S3

[Rafael J. Wysocki]

On Thu, Jul 26, 2018 at 5:46 AM, Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> Hi Rafael,
>
>> On 2018Jul24, at 18:36, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> On Tuesday, July 24, 2018 11:13:42 AM CEST Rafael J. Wysocki wrote:
>>> On Tuesday, July 24, 2018 10:46:09 AM CEST Kai Heng Feng wrote:
>>>> Hi Rafael,
>>>>
>>>>> On Jun 13, 2018, at 7:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>>
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>
>>>>> It is reported that commit a192aa923b66a (ACPI / LPSS: Consolidate
>>>>> runtime PM and system sleep handling) introduced a system suspend
>>>>> regression on some machines, but the only functional change made by
>>>>> it was to cause the PM quirks in the LPSS to also be used during
>>>>> system suspend and resume.  While that should always work for
>>>>> suspend-to-idle, it turns out to be problematic for S3
>>>>> (suspend-to-RAM).
>>>>>
>>>>> To address that issue restore the previous S3 suspend and resume
>>>>> behavior of the LPSS to avoid applying PM quirks then.
>>>>
>>>> The users reported that this patch does fix the S3 issue, but the S4 still
>>>> fails.
>>>>
>>>> Please refer to [1] for more for user's testing result.
>>>>
>>>> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1774950/comments/60
>>>
>>> Please ask the users to test the patch below, then, on top of the $subject one.
>>
>> There was a mistake in the previous patch I posted, sorry about that.
>>
>> Please test this one instead:
>
> The result is positive, thanks!

Good, thanks!

I assume that S3 still works with this patch applied too.

>> ---
>> drivers/acpi/acpi_lpss.c |   26 +++++++++++++++++---------
>> 1 file changed, 17 insertions(+), 9 deletions(-)
>>
>> Index: linux-pm/drivers/acpi/acpi_lpss.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/acpi_lpss.c
>> +++ linux-pm/drivers/acpi/acpi_lpss.c
>> @@ -879,6 +879,7 @@ static void acpi_lpss_dismiss(struct 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)
>> {
>> @@ -921,6 +922,9 @@ static void lpss_iosf_enter_d3_state(voi
>>
>>       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);
>> }
>> @@ -935,6 +939,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);
>>
>> @@ -944,13 +953,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)
>> @@ -963,14 +972,14 @@ static int acpi_lpss_suspend(struct devi
>>        * 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;
>> @@ -979,8 +988,7 @@ static int acpi_lpss_resume(struct devic
>>        * 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);
>> @@ -1004,12 +1012,12 @@ static int acpi_lpss_suspend_late(struct
>>               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);
>> }
>> @@ -1024,7 +1032,7 @@ static int acpi_lpss_runtime_suspend(str
>>
>> 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);
>> }
>

Re: [PATCH] ACPI / LPSS: Avoid PM quirks on suspend and resume from S3

[Kai-Heng Feng]

> On 2018Jul26, at 16:14, Rafael J. Wysocki <rafael@kernel.org> wrote:
> 
> On Thu, Jul 26, 2018 at 5:46 AM, Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>> Hi Rafael,
>> 
>>> On 2018Jul24, at 18:36, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> 
>>> On Tuesday, July 24, 2018 11:13:42 AM CEST Rafael J. Wysocki wrote:
>>>> On Tuesday, July 24, 2018 10:46:09 AM CEST Kai Heng Feng wrote:
>>>>> Hi Rafael,
>>>>> 
>>>>>> On Jun 13, 2018, at 7:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>>> 
>>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>> 
>>>>>> It is reported that commit a192aa923b66a (ACPI / LPSS: Consolidate
>>>>>> runtime PM and system sleep handling) introduced a system suspend
>>>>>> regression on some machines, but the only functional change made by
>>>>>> it was to cause the PM quirks in the LPSS to also be used during
>>>>>> system suspend and resume.  While that should always work for
>>>>>> suspend-to-idle, it turns out to be problematic for S3
>>>>>> (suspend-to-RAM).
>>>>>> 
>>>>>> To address that issue restore the previous S3 suspend and resume
>>>>>> behavior of the LPSS to avoid applying PM quirks then.
>>>>> 
>>>>> The users reported that this patch does fix the S3 issue, but the S4 still
>>>>> fails.
>>>>> 
>>>>> Please refer to [1] for more for user's testing result.
>>>>> 
>>>>> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1774950/comments/60
>>>> 
>>>> Please ask the users to test the patch below, then, on top of the $subject one.
>>> 
>>> There was a mistake in the previous patch I posted, sorry about that.
>>> 
>>> Please test this one instead:
>> 
>> The result is positive, thanks!
> 
> Good, thanks!
> 
> I assume that S3 still works with this patch applied too.

Yes, S3 continues to work with this patch.

Kai-Heng

> 
>>> ---
>>> drivers/acpi/acpi_lpss.c |   26 +++++++++++++++++---------
>>> 1 file changed, 17 insertions(+), 9 deletions(-)
>>> 
>>> Index: linux-pm/drivers/acpi/acpi_lpss.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/acpi/acpi_lpss.c
>>> +++ linux-pm/drivers/acpi/acpi_lpss.c
>>> @@ -879,6 +879,7 @@ static void acpi_lpss_dismiss(struct 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)
>>> {
>>> @@ -921,6 +922,9 @@ static void lpss_iosf_enter_d3_state(voi
>>> 
>>>      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);
>>> }
>>> @@ -935,6 +939,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);
>>> 
>>> @@ -944,13 +953,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)
>>> @@ -963,14 +972,14 @@ static int acpi_lpss_suspend(struct devi
>>>       * 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;
>>> @@ -979,8 +988,7 @@ static int acpi_lpss_resume(struct devic
>>>       * 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);
>>> @@ -1004,12 +1012,12 @@ static int acpi_lpss_suspend_late(struct
>>>              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);
>>> }
>>> @@ -1024,7 +1032,7 @@ static int acpi_lpss_runtime_suspend(str
>>> 
>>> 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);
>>> }

Re: [PATCH] ACPI / LPSS: Avoid PM quirks on suspend and resume from S3

[Rafael J. Wysocki]

On Thursday, July 26, 2018 10:15:03 AM CEST Kai-Heng Feng wrote:
> 
> > On 2018Jul26, at 16:14, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > 
> > On Thu, Jul 26, 2018 at 5:46 AM, Kai-Heng Feng
> > <kai.heng.feng@canonical.com> wrote:
> >> Hi Rafael,
> >> 
> >>> On 2018Jul24, at 18:36, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>> 
> >>> On Tuesday, July 24, 2018 11:13:42 AM CEST Rafael J. Wysocki wrote:
> >>>> On Tuesday, July 24, 2018 10:46:09 AM CEST Kai Heng Feng wrote:
> >>>>> Hi Rafael,
> >>>>> 
> >>>>>> On Jun 13, 2018, at 7:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>>>>> 
> >>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>>>> 
> >>>>>> It is reported that commit a192aa923b66a (ACPI / LPSS: Consolidate
> >>>>>> runtime PM and system sleep handling) introduced a system suspend
> >>>>>> regression on some machines, but the only functional change made by
> >>>>>> it was to cause the PM quirks in the LPSS to also be used during
> >>>>>> system suspend and resume.  While that should always work for
> >>>>>> suspend-to-idle, it turns out to be problematic for S3
> >>>>>> (suspend-to-RAM).
> >>>>>> 
> >>>>>> To address that issue restore the previous S3 suspend and resume
> >>>>>> behavior of the LPSS to avoid applying PM quirks then.
> >>>>> 
> >>>>> The users reported that this patch does fix the S3 issue, but the S4 still
> >>>>> fails.
> >>>>> 
> >>>>> Please refer to [1] for more for user's testing result.
> >>>>> 
> >>>>> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1774950/comments/60
> >>>> 
> >>>> Please ask the users to test the patch below, then, on top of the $subject one.
> >>> 
> >>> There was a mistake in the previous patch I posted, sorry about that.
> >>> 
> >>> Please test this one instead:
> >> 
> >> The result is positive, thanks!
> > 
> > Good, thanks!
> > 
> > I assume that S3 still works with this patch applied too.
> 
> Yes, S3 continues to work with this patch.

OK, thanks!

Let me resend the patch with a proper subject and changelog, then.