Not sure about the usefulness of having a separate function for the revision. Actually, the revision may command the actual functions available and what they do (this is what some verbatim in section 9.1.1 of the ACPI spec says).

I would prefer that, instead of passing a struct uuid around, you define, e.g., a new struct dsm_itf containing both a UUID and a revision ID, and then there's no need for rev_for_uuid(). Additionally, we could then enrich that structure to provide methods for the common operations (enter, exit), this could make the code clearer and more flexible, allowing to avoid specifying the right constants in encapsulated ways. With more refactoring (e.g., having an array of struct dsm_itf), in the acpi_spmc*_notif() functions we would get rid of the ifs and impose a common ordering functions (see also inline comments there).

Regarding which revisions to use:

• The Intel documentation mentions only revision 0.
• But the Linux code for Intel has been using revision 1 from the start, without any explanation. The commit introducing it however links to the document mentioning 0 (!).
• Then AMD _DSM code was introduced.
• And finally, MS _DSM, using the same revision as used for the CPU-vendor _DSM except for the initial enumeration (function index 0). In other words, it uses 0 on an AMD CPU and 1 on an Intel CPU for the same MS _DSM (except, as said, for enumeration, where it's 0 in both cases). I'm undecided on whether that's a feature or a bug in Linux, and on the first case a bug in the Intel platform... :-)

So I would just try on Intel hardware (if you have not done so already):

• The current code, checking that the MS methods actually succeed (i.e., do not produce a "failed to call DSM" error). If they don't, then it's an Intel platform bug, and we'll have to implement the Linux's weird behavior described above. If they do, then the weird behavior is just a bug. Not sure if already reported tests in this revision are conclusive, as they list onl a few error lines with other errors.
• Try revision 0 for Intel _DSM. If that works, we should probably just drop revision 1 entirely, even if that means not doing exactly the same as Linux. The refactoring evoked in the first paragraph would, among others, allow strategies where we try both revisions in turn.

Perhaps amend the comment below with what I said about the revision used with MS' _DSM in Linux.

I should be able to help test with my Intel-based Framework laptop.

Style.

Linux has the ordering: AMD, then MS, then Intel. Not a request to change the one here, but something to keep in mind in case of testing problems.

MS' "modern entry" has to be first according to MS' doc (https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-firmware-notifications), and this is also what Linux does.

This is the same ordering as in acpi_spmc_entry_notif(). Maybe this doesn't matter (or perhaps would even make things worse) but I'd find it more logical that the order is reversed. Linux's logic has the same property (flaw?).

This sounds like a good idea and a cleaner solution indeed. is the itf in dsm_itf as in interface?

for the revisions - I will change this to just use 0 on all platforms at the moment, and, when I get around to testing on intel, I can fix this if this needs fixing.

yeah you're right, I have all the correct ordering in a subsequent local commit which I hadn't squashed and updated this revision with yet.

I think this is to be expected

This sounds like a good idea and a cleaner solution indeed.

Just to be clear, you don't *absolutely* need to do it before commit, especially if you have already done a lot of tests, but if you are going to do more of them, then yes, would be nice to do that.

We should also probably add some knobs to control part of the process to ease testing and feedback if people have problems, but that can be done later while the code is in the tree (e.g., rule out some DSM, tweak the ordering, change the revisions, etc.).

is the itf in dsm_itf as in interface?

Yes. That was just an example name, I'm sure we can find a better one.

for the revisions - I will change this to just use 0 on all platforms at the moment, and, when I get around to testing on intel, I can fix this if this needs fixing.

That's the cleanest approach I think.

I'm not so sure, but if that works, then it's likely the ordering just doesn't matter (at least for now on tested machines). A debug knob tweaking that might be useful (for later).

This was a while ago for me now but as I recall the AMD get device constraints DSM was necessary and the MS one didn't have this. I have no idea what the situation is like on Intel though so in the meantime I defaulted to the MS one.

The #ifdef notyet is because the necessary commits for acpi_pwr_get_state had to be reverted, as they were the D-state changes that broke older machines with working S3. I will add this function back at some point, but not a super high priority as this is just an extra debugging check which doesn't seem to affect S0i3 entry (at least on AMD machines).

That makes sense. But since their doc doesn't mention any function to get constraints, I still think we should put it last at least (and remove/disable it later entirely depending on testing feedback; it would be great to print if the MS DSM is used).

Yes, that doesn't affect functionality (on systems where S0i3 indeed works), so there's no urgency.

That said, this is a needed piece for debugging when S0i3 is not entered or if it is but in some degraded fashion that still consumes an unexpected amount of power.

In general, for power and suspend issues, I think we'll never be able to test things on enough hardware, so we'd better prepare for information gathering and steps tweaking in the field to be able to actually diagnose and even bypass problems.

It seems a good idea to keep the constraints parsing code enabled even if it is functionally completely useless as long as the #ifdef notyet stays, so that we can already have feedbacks on how it fares.