Date: Fri, 1 Jun 2018 09:44:23 +0000 (UTC) From: Andriy Gapon <avg@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r334479 - head/sys/dev/acpica Message-ID: <201806010944.w519iNkT006375@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: avg Date: Fri Jun 1 09:44:23 2018 New Revision: 334479 URL: https://svnweb.freebsd.org/changeset/base/334479 Log: call AcpiLeaveSleepStatePrep after re-enabling interrupts I want to do this change because this call (actually, AcpiHwLegacyWakePrep) does a memory allocation and ACPI namespace evaluation. Although it is not very likely to run into any trouble, it is still not safe to make those calls with interrupts disabled. witness(4) and malloc(9) do not currently check for a context with interrupts disabled via intr_disable and we lack a facility for doing that. So, those unsafe operations fly under the radar. But if intr_disable in acpi_EnterSleepState was replaced with spinlock_enter (which it probably should be), then witness and malloc would immediately complain. Also, AcpiLeaveSleepStatePrep is documented as called when interrupts are enabled. It used to require disabled interrupts, but that requirement was changed a long time ago when support for _BFS and _GTS was removed from ACPICA. The ACPI wakeup sequence is very sensitive to changes. I consider this change to be correct, but there can be fallouts from it. What AcpiHwLegacyWakePrep essentially does is writing a value corresponding to S0 into SLP_TYPx bits of PM1 Control Register(s). According to ACPI specifications that write should be a NOP as SLP_EN bit is not set. But I see in some chipset specifications that they allow to ignore SLP_EN altogether and to act on a change of SLP_TYPx alone. Also, there are a couple of accesses to ACPI hardware before the new location of the call to AcpiLeaveSleepStatePrep. One is to clear the power button status and the other is to enable SCI. So, the move may affect the interaction between then OS and ACPI platform. I have not seen any regressions on my test system, but it's a desktop. MFC after: 5 weeks Modified: head/sys/dev/acpica/acpi.c Modified: head/sys/dev/acpica/acpi.c ============================================================================== --- head/sys/dev/acpica/acpi.c Fri Jun 1 09:41:15 2018 (r334478) +++ head/sys/dev/acpica/acpi.c Fri Jun 1 09:44:23 2018 (r334479) @@ -2977,8 +2977,6 @@ acpi_EnterSleepState(struct acpi_softc *sc, int state) if (sleep_result == 1 && state != ACPI_STATE_S4) AcpiWriteBitRegister(ACPI_BITREG_SCI_ENABLE, ACPI_ENABLE_EVENT); - AcpiLeaveSleepStatePrep(state); - if (sleep_result == 1 && state == ACPI_STATE_S3) { /* * Prevent mis-interpretation of the wakeup by power button @@ -3007,6 +3005,8 @@ acpi_EnterSleepState(struct acpi_softc *sc, int state) /* call acpi_wakeup_machdep() again with interrupt enabled */ acpi_wakeup_machdep(sc, state, sleep_result, 1); + AcpiLeaveSleepStatePrep(state); + if (sleep_result == -1) goto backout; @@ -3015,8 +3015,8 @@ acpi_EnterSleepState(struct acpi_softc *sc, int state) AcpiEnable(); } else { status = AcpiEnterSleepState(state); - AcpiLeaveSleepStatePrep(state); intr_restore(intr); + AcpiLeaveSleepStatePrep(state); if (ACPI_FAILURE(status)) { device_printf(sc->acpi_dev, "AcpiEnterSleepState failed - %s\n", AcpiFormatException(status));
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201806010944.w519iNkT006375>