acpi_spmc: Call new MS turn on display DSM

[obiwac]

Summary

Microsoft added a new function index (turn on display, 9) to their DSM set. This revision calls this, which fixes S0ix on certain machines, such as the Lenovo Yoga Slim 7i Aura, who's ECs use this method as a trigger to restore power to certain devices.

See commit 229ecbaac6b3 ("ACPI: x86: s2idle: Invoke Microsoft _DSM Function 9 (Turn On Display)") on Linux.

And the following: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications#turn-on-display-notification-function-9

Sponsored by: The FreeBSD Foundation

Test Plan

Need someone with this function index to try this out and make sure acpi_spmc_run_dsm() is being called with DSM_MODERN_TURN_ON_DISPLAY. You can look at your decompiled DSDT (acpidump -d) under UUID 11e00d56-ce64-47ce-837b-1f898f9aa461, revision ID 0, and in the enumerate functions function (index 0) if that value contains 1 << 9, or you can send me your dump ([e-mail]) and I can tell you.

Otherwise we can land this anyway as this won't affect any devices which don't have this DSM.

[obiwac]

sys/dev/acpica/acpi_spmc.c

273

since we're modifying this global variable, we probably want to enforce that this driver is loaded only once (a system can only ever have one SPMC). Is there an established way to do this in FreeBSD, or should i just set a variable in attach and return ENXIO in probe if it is set?

[olce]

Looks good overall. Based on Microsoft's documentation, which leaves a lot to be desired, I would have called DSM_MODERN_TURN_ON_DISPLAY before DSM_MODERN_EXIT_NOTIF exactly as you're doing.

The question you asked inline turned out to be more involved than it seemed at first, at least for me. The usual idiom for devices that have an IDENTIFY method is that they check in that method if a child of their parent has their name (see DEVICE_IDENTIFY(9)). But here, there's no such method. I must admit it took me a while to understand how this driver could indeed have its PROBE and ATTACH method called. After analysis, the only possibility I see it's because acpi(4) walks the namespace on attach, and calls BUS_ADD_CHILD() on devices found there (see acpi_probe_attach(), acpi_probe_children() (misnamed) which calls acpi_probe_child() through AcpiWalkNamespace()). Looking at the code in acpi.c and subr_bus.c, I don't see how the probe method could be called a second time if the probe did not return an error (the driver must have attached), except:

1. If ENXIO is returned because sc->dsm_sets is 0 (no DSM supported), but in that case, it means that the line in acpi_spmc_check_dsm_set() settings dsms_supported (slightly misnamed, maybe functions_supported would be better) cannot have been reached (as the next one precisely ensures that sc->dsm_sets becomes non-zero).
2. There are two devices in the namespace with ID PNP0D80, which is not supposed to happen.

I think you can add to acpi_spmc_probe() a test about whether devclass_get_count() is non-zero, in which case you return ENXIO. This would allow manually detaching acpi_spmc and reattaching it "by hand" using devctl(8), if there's ever a machine with two PNP0D80 devices one day, and only one is functional...

[imp]

Probe is called a second time if it didn't return 0. Acpi does add all the children. Identify is rarely needed with acpi

[obiwac]

which leaves a lot to be desired

Yes, I still have no clue how I'm supposed to parse their description 😄

I think you can add to acpi_spmc_probe() a test about whether devclass_get_count() is non-zero, in which case you return ENXIO. This would allow manually detaching acpi_spmc and reattaching it "by hand" using devctl(8), if there's ever a machine with two PNP0D80 devices one day, and only one is functional...

Tried this out but devclass_get_count() gives 1 in the probe before anything was attached. i will find the time to investigate this soon

[imp]

In D56062#1283586, @obiwac wrote:

which leaves a lot to be desired

Yes, I still have no clue how I'm supposed to parse their description 😄

I think you can add to acpi_spmc_probe() a test about whether devclass_get_count() is non-zero, in which case you return ENXIO. This would allow manually detaching acpi_spmc and reattaching it "by hand" using devctl(8), if there's ever a machine with two PNP0D80 devices one day, and only one is functional...

Tried this out but devclass_get_count() gives 1 in the probe before anything was attached. i will find the time to investigate this soon

Don't add a check for a count. Just probe and succeed. The driver does match multiple of PNP0D80 and it shouldn't second guess in probe. Instead, fail in attach for units > 0.

Generally, probe routines need to be idempotent and not depend on what drivers may or may not be attached / bound to, etc.

[obiwac]

• Fail attach if unit > 0
• Change device description

[obiwac]

sys/dev/acpica/acpi_spmc.c

214–216

will commit this separately

225–229

will commit this separately

[olce]

In D56062#1283586, @obiwac wrote:

Tried this out but devclass_get_count() gives 1 in the probe before anything was attached. i will find the time to investigate this soon

It's because the device we are probing is already counted, so the test would actually be devclass_get_count() > 1. Testing the unit number is also fine.

In D56062#1283588, @imp wrote:

Don't add a check for a count. Just probe and succeed. The driver does match multiple of PNP0D80 and it shouldn't second guess in probe. Instead, fail in attach for units > 0.

These are valuable advices in general, but they don't by themselves solve the problem we are having here. Checking for a count or unit in attach instead of probe without other changes is actually harmful.

The problem is that each execution of acpi_spmc_probe() can overwrite the idea of supported DSMs for all instances, because:

1. There's only one global storage for DSM probes, and that's the (only) reason why this driver currently does not support multiple instances (a case that seems quite unlikely to happen).
2. DSMs are probed during the probe phase, because arguably the result is needed to determine if this driver really can handle the device.
3. The results of the DSMs probes are reused during the attach phase, which is possible since acpi_spmc_probe() returns 0 on success (which guarantees preservation of the softc).

The "best" fix would be to move the storage of dsms_supported from struct dsm_set to some field in struct acpi_spmc_softc. That could be the occasion of grouping all struct dsm_set objects into a single array, and make it a constant (they would then form a cleanly separated specification for the driver). But since supporting multiple instances at this stage did not appear to be worth the trouble, I didn't require such a change.

So, in absence of implementing the change just evoked, the unit check must be moved to acpi_spmc_probe(), which IMO is sufficient and without ill consequences.

(The dependency of point 3. could be broken by performing again the DSMs probes during attach, even if that might not be absolutely straightforward (we have had occurrences of a DSM call working only on first call, and not on subsequent ones), but I do not anticipate any alternate driver to be needed in this area, and so think doing so is not worth the trouble either at this stage.)

sys/dev/acpica/acpi_spmc.c

214–216

👍🏻

225–229

This isn't sufficient, see general comment.

[obiwac]

sys/dev/acpica/acpi_spmc.c

225–229

oops, yeah, acpi_spmc_check_dsm_set() is being done in the probe of course

[obiwac]

The "best" fix would be to move the storage of dsms_supported from struct dsm_set to some field in struct acpi_spmc_softc. That could be the occasion of grouping all struct dsm_set objects into a single array, and make it a constant (they would then form a cleanly separated specification for the driver). But since supporting multiple instances at this stage did not appear to be worth the trouble, I didn't require such a change.

Yeah, I thought of doing something like this, but I think it would make things more complicated than its worth, at least for now

[obiwac]

Move unit check to probe and not attach

[obiwac]

sys/dev/acpica/acpi_spmc.c

214–216

33e095ee097d6a18dca95b8fae4edbda405bbb20