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.

Actually I need to check if acpi_EvaluateDSMTyped() is already always freeing the result pointer if returning an error status. other uses of this function in FreeBSD don't do this, so those would need to be fixed too if not

updated this because I was told the exact ordering was important, at least for AMD. Not sure yet how true this is for Intel, but might aswell

Which diagram are you referring to actually? I don't think I've seen that one.

You're right, and in fact in order to be reflective of what Windows and Linux as of f198478 does, we should even be calling the AMD DSM first.

I'd add some comment before the loop saying that sc->constraint_count can be modified inside the loop (hence using object->Package.Count as a guard).

Comment stale now, can be removed too.

I just removed it entirely because I can confirm that on Intel the Microsoft DSM UUID doesn't contain that function anyway. See:

https://raw.githubusercontent.com/linuxhw/ACPI/refs/heads/master/Notebook/Framework/Laptop/Laptop%2013/717EDB7C4975

and search for 11e00d56-ce64-47ce-837b-1f898f9aa461.

it is

actually this is checking for enum functions (1 means bit 0 not DSM #1). I think this is the canonical way of checking if a DSM exists at all.

although we should probably (and might aswell) check that all the functions we necessarily need from a given DSM set are available here. Will do that in a bit

thanks, this was a misunderstanding of how this worked on my end :)

i tend to agree but i was previously told that style advised against this now and I do see in style(9) they don't align this

Ha ha... You're right. Too much wine at lunch :-p... Sorry for the noise.

Checking for the necessary functions would be great (and ideally printing some message if a DSM is present but does not have the expected function), up to you.

Yeah, style(9) does not require it, and effectively it's not done in the few examples there, but it does not explicitly discourage it either. (More of a note for myself to ask and possibly amend style(9).) Anyway, I do not insist.

in the meantime I made it not accept the DSM set if it doesn't have all of the expected DSM functions. Maybe i should accept it so long as enum functions exists, and warn if not all the expected DSMs are available. I don't think anything bad can happen if you call a DSM ID that doesn't really exist.

i will align it because it bothers me too to be honest :)