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...

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

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

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.

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.)

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