Page MenuHomeFreeBSD

acpi_spmc(4): Only run DSM functions reported present
ClosedPublic

Authored by olce on Thu, May 7, 8:18 PM.
Tags
None
Referenced Files
F157484756: D56879.id177413.diff
Thu, May 21, 11:18 PM
F157474544: D56879.id177660.diff
Thu, May 21, 8:52 PM
F157470276: D56879.id177761.diff
Thu, May 21, 7:49 PM
Unknown Object (File)
Wed, May 20, 1:03 PM
Unknown Object (File)
Wed, May 20, 11:38 AM
Unknown Object (File)
Wed, May 20, 10:18 AM
Unknown Object (File)
Wed, May 20, 5:55 AM
Unknown Object (File)
Mon, May 18, 9:42 AM
Subscribers

Details

Summary

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.

Sponsored by: The FreeBSD Foundation

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

olce requested review of this revision.Thu, May 7, 8:18 PM

fine with this conceptually, but just bear in mind that there are some situations where enum functions report the wrong functions, and Linux just ignores this (have seen this with the AMD DSMs e.g.). So this might cause problems on some machines.

Also, typo in your comment description (ADM -> AMD)

This revision is now accepted and ready to land.Mon, May 11, 9:33 AM

fine with this conceptually, but just bear in mind that there are some situations where enum functions report the wrong functions, and Linux just ignores this (have seen this with the AMD DSMs e.g.). So this might cause problems on some machines.

Oh yes? Do you remember some details about that? Was it specifically linked with the revision used? In that latter case, perhaps D56882 fixes that.

It seems to me it could be useful to have a sysctl knob to force the driver to actually call functions that are "expected" (which is basically what it was doing before this change). What do you think?

Also, typo in your comment description (ADM -> AMD)

Thanks. :-) Fixed (locally, will update the series soon with latest versions).

Oh yes? Do you remember some details about that? Was it specifically linked with the revision used? In that latter case, perhaps D56882 fixes that.

Yeah, it was. D56882 probably does fix that (haven't reviewed it super deeply yet), but I just mean not to trust what the vendors put in enum functions too much

It seems to me it could be useful to have a sysctl knob to force the driver to actually call functions that are "expected" (which is basically what it was doing before this change). What do you think?

i mean, if this ends up being an issue it's just as trivial to ask someone to give us their dump to check this than to ask them to run again with a different sysctl. Your call

Yeah, it was. D56882 probably does fix that (haven't reviewed it super deeply yet), but I just mean not to trust what the vendors put in enum functions too much

Ok. But after having read some AML, I'm leaning towards the opposite thinking at the moment. E.g., if vendors would deactivate some function for a particular model (because the base firmware is shared between models, and that specific model does not support it or something doesn't work or is not relevant), they would do it primarily through the enum function, but would not necessarily remove the actual code. Testing the series on an AMD Framework Laptop is important to confirm (or infirm) all this.

i mean, if this ends up being an issue it's just as trivial to ask someone to give us their dump to check this than to ask them to run again with a different sysctl. Your call

For us, yes, but for them having an immediate way out may well make a big difference, and with all the changes this is now trivial to implement, so on second thought I'll do it.

Ok. But after having read some AML, I'm leaning towards the opposite thinking at the moment. E.g., if vendors would deactivate some function for a particular model (because the base firmware is shared between models, and that specific model does not support it or something doesn't work or is not relevant), they would do it primarily through the enum function, but would not necessarily remove the actual code. Testing the series on an AMD Framework Laptop is important to confirm (or infirm) all this.

I think that's reasonable

For us, yes, but for them having an immediate way out may well make a big difference, and with all the changes this is now trivial to implement, so on second thought I'll do it.

oki doki :)

olce edited the summary of this revision. (Show Details)
  • Add a sysctl(8) knob to revert to the previous behavior, just in case.
  • Impacts of changes in earlier revisions.
This revision now requires review to proceed.Tue, May 12, 1:44 PM
This revision is now accepted and ready to land.Tue, May 12, 2:16 PM