On Thu, Jul 26, 2018 at 5:46 AM, Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> wrote: > Hi Rafael, > >> On 2018Jul24, at 18:36, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> wrote: >>>>> >>>>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> >>>>> >>>>> 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); >> } > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
