acpi_spmc(4): Auto-detect DSM revisions by default

[olce]

Summary

Which revisions to use for the Intel and AMD DSMs is unclear. For the
Intel one, the written specification indicates only 0, but Linux uses
1 (possibly an oversight). For the AMD one, for which there is no
specification, Linux uses 0, but at least on the Framework 13 AMD 7040
series, the "enumerate functions" function only returns a mask that
covers all the functions we expect when called with revision 2.

Introduce an auto-detection strategy where each revision starting from
0 is tried in turn up to some limit (included; default: 15). As soon as
a revision implements all expected functions, we stop the loop and use
that one, in effect selecting the minimum revision that implements all
we need, which should avoid potential backwards-compatibility problems.
If no revision implements all expected functions, the highest available
revision in the checked range is selected, but higher revisions that do
not bring new functions are discarded (see the explanatory comment in
acpi_spmc_probe_dsm()).

The revision policy is still tunable using the same existing sysctl(8)
knobs 'debug.acpi.spmc.intel_dsm_revision' and
'debug.acpi.spmc.amd_dsm_revision'. They have been extended so that
a negative value indicates to use the auto-detection mechanism up to
a revision of minus the value. As before, a 0 or positive value
requests a specific revision. A new knob is introduced for the
Microsoft DSM just in case ('debug.acpi.spmc.ms_dsm_revision').

Since now the revision can be auto-detected, and thus depends on
a particular device instance, move it into 'struct dsm_info' on the
softc. This also enables finishing the split between static and
dynamic/tunable information, allowing to constify all the DSM
descriptors.

Print the revision eventually used along with the supported functions.

Tested on an Intel Framework laptop.

Sponsored by: The FreeBSD Foundation

[obiwac]

logic here seems sound

sys/dev/acpica/acpi_spmc.c

35–62

wasn't this previously moved down by a previous revision?

43

i like this :)

101–103

would be nice to keep this mention.

168–171

ditto

587–590

not 100% sure this is the right approach. What if a vendor updates their DSM with an important fix to an issue that existed in a previous revision, and they just kept that previous revision for some quirky old software that didn't update their revision number?

[olce]

sys/dev/acpica/acpi_spmc.c

35–62

This revision moves it up, which I couldn't do before because of the former revision field in DSM descriptors.

43

Ha ha ha

And 15 revisions ought to be enough for anyone. :-)

101–103

I moved it instead in the commit message itself. Isn't that enough?

587–590

The case you describe should be unlikely because of the constraint that newer revisions must be compatible with old ones (i.e., previously existing functions must be there, with same parameters and same semantics). So either there was an implementation problem, and then the vendor can simply update the old implementations without changing the revision, or there's a deeper problem with the interface, but then there must be new revisions implementing new functions which the driver anyway does not know about and cannot leverage automatically.

So, nothing indeed prevents a vendor for having an implementation fix only in new revisions, but that would just be... completely silly. We also provide knobs to force the revision selection if that ever becomes necessary.

[olce]

sys/dev/acpica/acpi_spmc.c

587–590

So, nothing indeed prevents a vendor for having an implementation fix only in new revisions, but that would just be... completely silly.

Actually, even that may not be a possibility because of the backwards compatibility requirement (in the context you evoked, if really fixing the bug causes an important change in behavior).

[obiwac]

sys/dev/acpica/acpi_spmc.c

101–103

yeah, I guess now that the default value auto-detects the best revision there isn't really much point keeping this in the source code

587–590

So, nothing indeed prevents a vendor for having an implementation fix only in new revisions, but that would just be... completely silly. We also provide knobs to force the revision selection if that ever becomes necessary.

well, we do already have contingencies in place for if a vendor does something incredibly silly...

We also provide knobs to force the revision selection if that ever becomes necessary.

that is true

[olce]

sys/dev/acpica/acpi_spmc.c

587–590

well, we do already have contingencies in place for if a vendor does something incredibly silly...

Yes, I very well know, because I introduced lots of them. :-)

It's rather that I wouldn't base some automatic detection too much on yet-to-be-observed relatively improbable silliness. The trick of discarding higher revisions because they look essentially the same is to workaround some other caveat that we are already observing. I'm not too fan of showing, e.g., revision 15 for the Microsoft DSM on my Framework laptop, where in fact it is the same as revision 0 (the firmware doesn't care about the passed revision).

And, if we ever observe such a behavior in the wild, I don't see how we can keep a fully generic automatic detection, we would have to resort to quirks (or accept showing revision 15 in the example above even if that looks weird; but that's why I would prefer to wait for such a case to materialize, if ever).

[obiwac]

sys/dev/acpica/acpi_spmc.c

587–590

Yes, I very well know, because I introduced lots of them. :-)

i know, that's why I mention it :)

I'm not too fan of showing, e.g., revision 15 for the Microsoft DSM on my Framework laptop, where in fact it is the same as revision 0 (the firmware doesn't care about the passed revision).

yeah that is true, a firmware vendor could just always return a full mask from enum functions regardless of the revision. I wish this DSM stuff wasn't so sloppy...

i'm happy with this approach now

[olce]

• Fix typo in commit message.
• Impacts of changes in earlier revisions.