Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler

["Limonciello, Mario" <mario.limonciello@amd.com>]

>>>> My preference at this point would be to use a notifier chain, unless
>>>> that's not sufficient for some reason, because it appears to match the
>>>> notifier usage model.
>>>
>>> Well, I'm actually not sure about that.
>>>
>>>>> I pointed this out, because making this change would also make 4/5 a bit
>>>>> cleaner. You are recreating the same struct lps0_callback_handler on
>>>>> stack twice there, which looked weird to me.
>>>>>
>>>>> Note if Rafael wants to stick with the approach from this v3, then
>>>>> I guess that the approach in 4/5 is fine.
>>>>>> Rafael - can you please confirm which direction you want to see here
>>> for this?
>>>
>>> So the idea is that the PMC driver's "suspend" method needs to be
>>> invoked from acpi_s2idle_prepare_late(), so it doesn't interfere with
>>> the suspend of the other devices in the system and so it can take the
>>> constraints into account.
>>
>> The reason to do nothing (besides a debug level message right now) with the constraints
>> information is that at least on today's OEM platforms there are some instances constraints
>> aren't met on Linux that need to be investigated and root caused.  These particular constraints
>> don't actually cause problems reaching s0ix residency though.
> 
> Why are they useful at all, then?
> 
>>>
>>> What is it going to do, in the future, depending on whether or not the
>>> constraints are met?
>>
>> The idea was that if constraints were met that it would send the OS_HINT as part of
>> amd_pmc_suspend/amd_pmc_resume, and if they aren't met then skip this step.
>>
>> It would effectively block the system from getting s0ix residency unless the constraints
>> are all met.
> 
> Why do that?

I guess to both of your above questions this begs a comparison of how 
things work in Windows versus Linux.

Windows Modern Standby has this concept of "SW DRIPS" vs "HW DRIPS". 
 From an end user perspective you close your lid or you click Start 
button and hit sleep and the machine is in sleep.  Whether it's in the 
deepest state is "invisible" to you unless you're running a sleep study.

Windows will at this time requests devices to go into their deepest 
states and will continue to monitor them against the constraints table.
When the constraints table is matched a uPEP driver is notified (this is 
the _DSM stuff we have in Linux too for "deepest state") and then it can 
do as it pleases.   ON AMD's platform this sends OS_HINT.  OS_HINT is 
meant to be indicate that the OS is done with all it's suspend actions 
(caches flushed, devices in D3 etc) and the system can "try" to enter s0ix.

Windows will then also distinguish between different types of wakeups 
and have different behaviors for them.   There are wakeups that can be 
treated as keep screen off, and then possibly go back into deepest state.

"As soon as the SoC wakes and the platform exits the DRIPS state, the 
CPUs start running code again. However, the screen stays powered off 
unless the interrupt was a result of user input or connecting to a power 
source"
https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/transitioning-between-idle-and-active-states

"During the Sleep state, specific value-adding SW activity may run"
https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-states

So from the time that I clicked sleep in the OS, I might NOT get into 
the deepest state (HW DRIPS), or I might have gotten in and out several 
times.  If a device driver failed to put a PCI device in D3 for example 
I would not be able to enter HW DRIPS, but the suspend wouldn't "fail".

Now to contrast this to Linux when I enter suspend all drivers will run 
their various PM callbacks and devices will go into their deepest states.
* If a driver fails, the suspend actually fails and you get an error 
code to go investigate what happened.
* If the devices don't get into their deepest state by the time you get 
to the s2idle loop you don't get s0ix residency.

As you can see at least for AMD's platforms OS_HINT is sent too "early". 
  That's why this series exists in the first place.

So with all that said; why look at constraints at all if stuff is working?
 From this design at least on Windows constraints are supposed to be a 
safety guard that you don't start the HW process for s0i3 process "too 
early".

The last commit that is getting reverted in this series is an example of 
what could happen if the process is started prematurely.

> 
>> Given we know some OEM platforms have problems in current generations
>> with constraints it would probably need to be restricted to this behavior only on a future
>> SOC that we are confident of all drivers and firmware are doing the right thing.
>>
>> By passing the information to amd_pmc we can keep that logic restricting the behavior to
>> only those platforms that we're confident on that behavior.
> 
> Honestly, I'm not quite sure why it is a good idea to prevent the
> platform from attempting to get into S0ix via suspend-to-idle in any
> case.
> 
> You know you have to suspend.  You don't know how much time you will
> be suspended.  The constraints can only tell you what's the
> lowest-power state you can achieve at this point, but why is it
> relevant?

Having thought through and said all I did above, I do concede you're 
right with the Linux approach to sleep the constraints really don't add 
a lot of value.  If a device fails to enter it's intended sleep states 
the suspend will "fail".  If the suspend succeeds but the constraints 
table doesn't match, it's just a hint where to focus on problems.

I appreciate your thoughts and I will drop the constraints passing patch 
in this series.

With that intent of dropping that would you still like this reworked as 
a notifier chain or keep it as this design?