Date: Wed, 13 May 2026 12:40:03 +0000 From: Olivier Certner <olce@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 65ecfb4a66f3 - main - acpi_spmc(4): Only run DSM functions reported present Message-ID: <6a047123.1c88f.689dad22@gitrepo.freebsd.org>
index | next in thread | raw e-mail
The branch main has been updated by olce: URL: https://cgit.FreeBSD.org/src/commit/?id=65ecfb4a66f377ca72569d7a96de0347b4541b52 commit 65ecfb4a66f377ca72569d7a96de0347b4541b52 Author: Olivier Certner <olce@FreeBSD.org> AuthorDate: 2026-05-06 11:53:02 +0000 Commit: Olivier Certner <olce@FreeBSD.org> CommitDate: 2026-05-13 12:38:25 +0000 acpi_spmc(4): Only run DSM functions reported present Examination of the DSDT in a Framework laptop generally hints at firmware designers sometimes providing ACPI methods for convenience (e.g., same firmware for multiple models) but not using them (or not expecting them to be used) depending on tweaks or the actual hardware platform. On an Intel Framework laptop, we specifically observe the presence of a Microsoft DSM that just reports availability of the SLEEP_ENTRY and SLEEP_EXIT (7 and 8) functions although the Microsoft specification requires other functions, whose purpose is similar to corresponding Intel DSM's ones (such as DISPLAY_OFF). However, we currently always call the latter even on the Microsoft DSM. On that laptop, fortunately, the way the code is structured in the _DSM method leads to nothing being executed on this call. Given the similarity of intent between most functions from the Microsoft DSM on one side and those of ADM and Intel on the other, it is imaginable that other firmware developers could use a strategy where functions are in fact aliased, in which case insisting on calling the Microsoft's DSM function even if not enumerated would cause the action to be performed twice (because we also call the corresponding function on the Intel/AMD DSM), which may or may not cause other problems and in any case seems a waste. So, by default, do not try to run any function that is not enumerated, as that looks like the safest approach. Add a debug sysctl(8) knob to revert to the previous behavior, just in case ('debug.acpi.spmc.force_call_expected_functions'). acpi_spmc_run() now checks if a DSM/function combination has been enumerated, and skips the actual call if it does not. This allows to remove all checks from the acpi_spmc_*_notif() functions, making the code much more compact. acpi_spmc_get_constraints() now checks whether DSM_GET_DEVICE_CONSTRAINTS is supported in order to determine which DSM to use and whether to call the function at all. Reviewed by: obiwac Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D56879 --- sys/dev/acpica/acpi_spmc.c | 89 +++++++++++++++++++--------------------------- 1 file changed, 37 insertions(+), 52 deletions(-) diff --git a/sys/dev/acpica/acpi_spmc.c b/sys/dev/acpica/acpi_spmc.c index da9c80266952..c33336d10ec3 100644 --- a/sys/dev/acpica/acpi_spmc.c +++ b/sys/dev/acpica/acpi_spmc.c @@ -206,6 +206,11 @@ SYSCTL_INT(_debug_acpi_spmc, OID_AUTO, verbose, CTLFLAG_RW, #define VERBOSE() (verbose || bootverbose) +static bool force_call_expected_functions; +SYSCTL_BOOL(_debug_acpi_spmc, OID_AUTO, always_call_expected_functions, + CTLFLAG_RW, &force_call_expected_functions, 0, + "Call all expected functions on a present DSM, even those not enumerated."); + struct acpi_spmc_constraint { bool enabled; char *name; @@ -461,7 +466,7 @@ acpi_spmc_dsm_print_functions(const struct acpi_spmc_softc *const sc, print_bit_field(buf, sizeof(buf), missing, "FUNC", pbf_function_name, dsm); device_printf(sc->dev, "DSM %s: Does not enumerate expected " - "functions %#" PRIx64 "%s. Calls to them may fail.\n", + "functions %#" PRIx64 "%s. Will skip calling them.\n", dsm->name, missing, buf); } @@ -635,14 +640,14 @@ acpi_spmc_get_constraints(struct acpi_spmc_softc *const sc) * report that condition to us, and somewhat arbitrarily favor the Intel * one because it at least has a written specification. */ - if (has_dsm(sc, DSM_INTEL)) { + if (supports_function(sc, DSM_INTEL, DSM_GET_DEVICE_CONSTRAINTS)) { dsm = &dsm_intel; - if (has_dsm(sc, DSM_AMD)) + if (supports_function(sc, DSM_AMD, DSM_GET_DEVICE_CONSTRAINTS)) device_printf(sc->dev, "Constraints: Both Intel and " - "AMD DSMs are present!\n" + "AMD DSMs support getting them!\n" "Using constraints from Intel.\nPlease report.\n"); - } else if (has_dsm(sc, DSM_AMD)) + } else if (supports_function(sc, DSM_AMD, DSM_GET_DEVICE_CONSTRAINTS)) dsm = &dsm_amd; else return (0); @@ -690,8 +695,9 @@ acpi_spmc_get_constraints(struct acpi_spmc_softc *const sc) } static void -acpi_spmc_check_constraints(struct acpi_spmc_softc *sc) +acpi_spmc_check_constraints(device_t dev) { + const struct acpi_spmc_softc *const sc = device_get_softc(dev); #ifdef notyet bool violation = false; #endif @@ -703,7 +709,8 @@ acpi_spmc_check_constraints(struct acpi_spmc_softc *sc) if (sc->constraint_count == 0) return; for (size_t i = 0; i < sc->constraint_count; i++) { - struct acpi_spmc_constraint *constraint = &sc->constraints[i]; + const struct acpi_spmc_constraint *constraint = + &sc->constraints[i]; if (!constraint->enabled) continue; @@ -738,6 +745,7 @@ acpi_spmc_check_constraints(struct acpi_spmc_softc *sc) /* * Run a single DSM function. * + * Only runs the function if it was reported present during enumeration. * Discards the result, but prints a message on error. */ static void @@ -748,6 +756,10 @@ acpi_spmc_run(device_t dev, const struct dsm_desc *const dsm, ACPI_STATUS status; ACPI_BUFFER result; + if (!(supports_function(sc, dsm->index, function_index) || + (force_call_expected_functions && has_dsm(sc, dsm->index)))) + return; + status = acpi_EvaluateDSMTyped(sc->handle, (const uint8_t *)&dsm->uuid, dsm->revision, function_index, NULL, &result, ACPI_TYPE_ANY); @@ -767,67 +779,40 @@ acpi_spmc_run(device_t dev, const struct dsm_desc *const dsm, static void acpi_spmc_display_off_notif(device_t dev) { - struct acpi_spmc_softc *sc = device_get_softc(dev); - - if (has_dsm(sc, DSM_INTEL)) - acpi_spmc_run(dev, &dsm_intel, - DSM_INTEL_MS_DISPLAY_OFF_NOTIF); - if (has_dsm(sc, DSM_MS)) - acpi_spmc_run(dev, &dsm_ms, - DSM_INTEL_MS_DISPLAY_OFF_NOTIF); - if (has_dsm(sc, DSM_AMD)) - acpi_spmc_run(dev, &dsm_amd, DSM_AMD_DISPLAY_OFF_NOTIF); + acpi_spmc_run(dev, &dsm_intel, DSM_INTEL_MS_DISPLAY_OFF_NOTIF); + acpi_spmc_run(dev, &dsm_ms, DSM_INTEL_MS_DISPLAY_OFF_NOTIF); + acpi_spmc_run(dev, &dsm_amd, DSM_AMD_DISPLAY_OFF_NOTIF); } static void acpi_spmc_display_on_notif(device_t dev) { - struct acpi_spmc_softc *sc = device_get_softc(dev); - - if (has_dsm(sc, DSM_INTEL)) - acpi_spmc_run(dev, &dsm_intel, - DSM_INTEL_MS_DISPLAY_ON_NOTIF); - if (has_dsm(sc, DSM_MS)) - acpi_spmc_run(dev, &dsm_ms, - DSM_INTEL_MS_DISPLAY_ON_NOTIF); - if (has_dsm(sc, DSM_AMD)) - acpi_spmc_run(dev, &dsm_amd, DSM_AMD_DISPLAY_ON_NOTIF); + acpi_spmc_run(dev, &dsm_intel, DSM_INTEL_MS_DISPLAY_ON_NOTIF); + acpi_spmc_run(dev, &dsm_ms, DSM_INTEL_MS_DISPLAY_ON_NOTIF); + acpi_spmc_run(dev, &dsm_amd, DSM_AMD_DISPLAY_ON_NOTIF); } static void acpi_spmc_entry_notif(device_t dev) { - struct acpi_spmc_softc *sc = device_get_softc(dev); + /* XXX - No real check currently. Check return code when it does. */ + acpi_spmc_check_constraints(dev); - acpi_spmc_check_constraints(sc); - - if (has_dsm(sc, DSM_AMD)) - acpi_spmc_run(dev, &dsm_amd, DSM_AMD_LPI_ENTRY_NOTIF); - if (has_dsm(sc, DSM_MS)) { - acpi_spmc_run(dev, &dsm_ms, DSM_MS_SLEEP_ENTRY_NOTIF); - acpi_spmc_run(dev, &dsm_ms, DSM_INTEL_MS_LPI_ENTRY_NOTIF); - } - if (has_dsm(sc, DSM_INTEL)) - acpi_spmc_run(dev, &dsm_intel, - DSM_INTEL_MS_LPI_ENTRY_NOTIF); + acpi_spmc_run(dev, &dsm_amd, DSM_AMD_LPI_ENTRY_NOTIF); + acpi_spmc_run(dev, &dsm_ms, DSM_MS_SLEEP_ENTRY_NOTIF); + acpi_spmc_run(dev, &dsm_ms, DSM_INTEL_MS_LPI_ENTRY_NOTIF); + acpi_spmc_run(dev, &dsm_intel, DSM_INTEL_MS_LPI_ENTRY_NOTIF); } static void acpi_spmc_exit_notif(device_t dev) { - struct acpi_spmc_softc *sc = device_get_softc(dev); - - if (has_dsm(sc, DSM_INTEL)) - acpi_spmc_run(dev, &dsm_intel, DSM_INTEL_MS_LPI_EXIT_NOTIF); - if (has_dsm(sc, DSM_AMD)) - acpi_spmc_run(dev, &dsm_amd, DSM_AMD_LPI_EXIT_NOTIF); - if (has_dsm(sc, DSM_MS)) { - acpi_spmc_run(dev, &dsm_ms, DSM_INTEL_MS_LPI_EXIT_NOTIF); - if (supports_function(sc, DSM_MS, DSM_MS_TURN_ON_DISPLAY)) - acpi_spmc_run(dev, &dsm_ms, - DSM_MS_TURN_ON_DISPLAY); - acpi_spmc_run(dev, &dsm_ms, DSM_MS_SLEEP_EXIT_NOTIF); - } + acpi_spmc_run(dev, &dsm_intel, DSM_INTEL_MS_LPI_EXIT_NOTIF); + acpi_spmc_run(dev, &dsm_amd, DSM_AMD_LPI_EXIT_NOTIF); + acpi_spmc_run(dev, &dsm_ms, DSM_INTEL_MS_LPI_EXIT_NOTIF); + /* Hint to the platform we are soon going to turn on the display. */ + acpi_spmc_run(dev, &dsm_ms, DSM_MS_TURN_ON_DISPLAY); + acpi_spmc_run(dev, &dsm_ms, DSM_MS_SLEEP_EXIT_NOTIF); } static voidhome | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6a047123.1c88f.689dad22>